sql: fix race/deadlock condition on audit logging CCL hook

Fixes cockroachdb#104660. Fixes cockroachdb#105174. This change fixes a race/deadlock condition that can occur when multiple SQLServers are created simulatenously, causing simultaneous write to an unprotected global variable used to configure a CCL audit logging feature. Release note (bug fix): This change fixes a race/deadlock condition that can occur when multiple SQLServers are created simulatenously, causing simultaneous writes to an unprotected global variable used to configure a CCL audit logging feature.
THardy98 · Jun 21, 2023 · da1afd6 · da1afd6
1 parent 9633594
commit da1afd6
Show file tree

Hide file tree

Showing 6 changed files with 20 additions and 37 deletions.
diff --git a/pkg/ccl/auditloggingccl/BUILD.bazel b/pkg/ccl/auditloggingccl/BUILD.bazel
@@ -10,9 +10,8 @@ go_library(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql/auditlogging",
-        "//pkg/sql/pgwire/pgcode",
-        "//pkg/sql/pgwire/pgerror",
         "//pkg/util/log",
+        "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
     ],
 )

diff --git a/pkg/ccl/auditloggingccl/audit_log_config.go b/pkg/ccl/auditloggingccl/audit_log_config.go
@@ -10,13 +10,12 @@ package auditloggingccl
 
 import (
 	"context"
-
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
+
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/auditlogging"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
 )
@@ -27,7 +26,7 @@ const auditConfigDefaultValue = ""
 var UserAuditLogConfig = settings.RegisterValidatedStringSetting(
 	settings.TenantWritable,
 	"sql.log.user_audit",
-	"user/role-based audit logging configuration",
+	"User/role-based audit logging configuration. An enterprise license is required for this cluster setting to take effect.",
 	auditConfigDefaultValue,
 	validateAuditLogConfig,
 ).WithPublic()
@@ -54,15 +53,6 @@ func validateAuditLogConfig(_ *settings.Values, input string) error {
 		// Empty config
 		return nil
 	}
-	st, clusterID, err := auditlogging.UserAuditEnterpriseParamsHook()
-	if err != nil {
-		return err
-	}
-	enterpriseCheckErr := utilccl.CheckEnterpriseEnabled(st, clusterID, "role-based audit logging")
-	if enterpriseCheckErr != nil {
-		return pgerror.Wrap(enterpriseCheckErr,
-			pgcode.InsufficientPrivilege, "role-based audit logging requires enterprise license")
-	}
 	// Ensure it can be parsed.
 	conf, err := auditlogging.Parse(input)
 	if err != nil {
@@ -104,8 +94,9 @@ var ConfigureRoleBasedAuditClusterSettings = func(ctx context.Context, acl *audi
 	UpdateAuditConfigOnChange(ctx, acl, st)
 }
 
-var UserAuditLogConfigEmpty = func(sv *settings.Values) bool {
-	return UserAuditLogConfig.Get(sv) == ""
+var UserAuditEnabled = func(st *cluster.Settings, clusterID uuid.UUID) bool {
+	enterpriseEnabled := utilccl.IsEnterpriseEnabled(st, clusterID, "role-based audit logging")
+	return UserAuditLogConfig.Get(&st.SV) != "" && enterpriseEnabled
 }
 
 var UserAuditReducedConfigEnabled = func(sv *settings.Values) bool {
@@ -114,6 +105,6 @@ var UserAuditReducedConfigEnabled = func(sv *settings.Values) bool {
 
 func init() {
 	auditlogging.ConfigureRoleBasedAuditClusterSettings = ConfigureRoleBasedAuditClusterSettings
-	auditlogging.UserAuditLogConfigEmpty = UserAuditLogConfigEmpty
+	auditlogging.UserAuditEnabled = UserAuditEnabled
 	auditlogging.UserAuditReducedConfigEnabled = UserAuditReducedConfigEnabled
 }
diff --git a/pkg/ccl/auditloggingccl/audit_logging_test.go b/pkg/ccl/auditloggingccl/audit_logging_test.go
@@ -41,11 +41,15 @@ func TestRoleBasedAuditEnterpriseGated(t *testing.T) {
 
 	testQuery := `SET CLUSTER SETTING sql.log.user_audit = 'ALL ALL'`
 
-	// Test that we cannot change the cluster setting when enterprise is disabled.
+	// Disable enterprise.
 	enableEnterprise := utilccl.TestingDisableEnterprise()
+
+	// Test that we cannot change the cluster setting when enterprise is disabled.
 	rootRunner.ExpectErr(t, "role-based audit logging requires enterprise license", testQuery)
-	// Enable enterprise.
+
+	// Enable enterprise
 	enableEnterprise()
+
 	// Test that we can change the cluster setting when enterprise is enabled.
 	rootRunner.Exec(t, testQuery)
 }

diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go
@@ -1380,12 +1380,6 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 	vmoduleSetting.SetOnChange(&cfg.Settings.SV, fn)
 	fn(ctx)
 
-	auditlogging.UserAuditEnterpriseParamsHook = func(st *cluster.Settings, clusterID uuid.UUID) func() (*cluster.Settings, uuid.UUID, error) {
-		return func() (*cluster.Settings, uuid.UUID, error) {
-			return st, clusterID, nil
-		}
-	}(execCfg.Settings, cfg.ClusterIDContainer.Get())
-
 	auditlogging.ConfigureRoleBasedAuditClusterSettings(ctx, execCfg.SessionInitCache.AuditConfig, execCfg.Settings, &execCfg.Settings.SV)
 
 	return &SQLServer{

diff --git a/pkg/sql/audit_logging.go b/pkg/sql/audit_logging.go
@@ -116,8 +116,8 @@ func (p *planner) initializeReducedAuditConfig(ctx context.Context) {
 
 // shouldNotRoleBasedAudit checks if we should do any auditing work for RoleBasedAuditEvents.
 func (p *planner) shouldNotRoleBasedAudit() bool {
-	// Do not do audit work if the cluster setting is empty.
+	// Do not do audit work if role-based auditing is not enabled.
 	// Do not emit audit events for reserved users/roles. This does not omit the root user.
 	// Do not emit audit events for internal planners.
-	return auditlogging.UserAuditLogConfigEmpty(&p.execCfg.Settings.SV) || p.User().IsReserved() || p.isInternalPlanner
+	return auditlogging.UserAuditEnabled(p.execCfg.Settings, p.EvalContext().ClusterID) || p.User().IsReserved() || p.isInternalPlanner
 }
diff --git a/pkg/sql/auditlogging/audit_log.go b/pkg/sql/auditlogging/audit_log.go
@@ -13,6 +13,7 @@ package auditlogging
 import (
 	"context"
 	"fmt"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -23,8 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
-	"github.com/cockroachdb/cockroach/pkg/util/uuid"
-	"github.com/cockroachdb/errors"
 	"github.com/olekukonko/tablewriter"
 )
 
@@ -33,20 +32,16 @@ import (
 var ConfigureRoleBasedAuditClusterSettings = func(ctx context.Context, acl *AuditConfigLock, st *cluster.Settings, sv *settings.Values) {
 }
 
-// UserAuditLogConfigEmpty is a noop global var injected by CCL hook.
-var UserAuditLogConfigEmpty = func(sv *settings.Values) bool {
-	return true
+// UserAuditEnabled is a noop global var injected by CCL hook.
+var UserAuditEnabled = func(st *cluster.Settings, clusterID uuid.UUID) bool {
+	return false
 }
 
 // UserAuditReducedConfigEnabled is a noop global var injected by CCL hook.
 var UserAuditReducedConfigEnabled = func(sv *settings.Values) bool {
 	return false
 }
 
-var UserAuditEnterpriseParamsHook = func() (*cluster.Settings, uuid.UUID, error) {
-	return nil, uuid.Nil, errors.New("Cannot validate log config, enterprise parameters not initialized yet")
-}
-
 // Auditor is an interface used to check and build different audit events.
 type Auditor interface {
 	GetQualifiedTableNameByID(ctx context.Context, id int64, requiredType tree.RequiredTableKind) (*tree.TableName, error)