sql: fix race condition on audit logging CCL hook

docs/generated/settings/settings-for-tenants.txt

@@ -237,7 +237,7 @@ sql.insights.latency_threshold	duration	100ms	amount of time after which an exec
 sql.log.slow_query.experimental_full_table_scans.enabled	boolean	false	when set to true, statements that perform a full table/index scan will be logged to the slow query log even if they do not meet the latency threshold. Must have the slow query log enabled for this setting to have any effect.	tenant-rw
 sql.log.slow_query.internal_queries.enabled	boolean	false	when set to true, internal queries which exceed the slow query log threshold are logged to a separate log. Must have the slow query log enabled for this setting to have any effect.	tenant-rw
 sql.log.slow_query.latency_threshold	duration	0s	when set to non-zero, log statements whose service latency exceeds the threshold to a secondary logger on each node	tenant-rw
-sql.log.user_audit	string		user/role-based audit logging configuration	tenant-rw
+sql.log.user_audit	string		user/role-based audit logging configuration. An enterprise license is required for this cluster setting to take effect.	tenant-rw
 sql.log.user_audit.reduced_config.enabled	boolean	false	enables logic to compute a reduced audit configuration, computing the audit configuration only once at session start instead of at each SQL event. The tradeoff with the increase in performance (~5%), is that changes to the audit configuration (user role memberships/cluster setting) are not reflected within session. Users will need to start a new session to see these changes in their auditing behaviour.	tenant-rw
 sql.metrics.index_usage_stats.enabled	boolean	true	collect per index usage statistics	tenant-rw
 sql.metrics.max_mem_reported_stmt_fingerprints	integer	100000	the maximum number of reported statement fingerprints stored in memory	tenant-rw

docs/generated/settings/settings.html

@@ -190,7 +190,7 @@
 <tr><td><div id="setting-sql-log-slow-query-experimental-full-table-scans-enabled" class="anchored"><code>sql.log.slow_query.experimental_full_table_scans.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when set to true, statements that perform a full table/index scan will be logged to the slow query log even if they do not meet the latency threshold. Must have the slow query log enabled for this setting to have any effect.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-log-slow-query-internal-queries-enabled" class="anchored"><code>sql.log.slow_query.internal_queries.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when set to true, internal queries which exceed the slow query log threshold are logged to a separate log. Must have the slow query log enabled for this setting to have any effect.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-log-slow-query-latency-threshold" class="anchored"><code>sql.log.slow_query.latency_threshold</code></div></td><td>duration</td><td><code>0s</code></td><td>when set to non-zero, log statements whose service latency exceeds the threshold to a secondary logger on each node</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-sql-log-user-audit" class="anchored"><code>sql.log.user_audit</code></div></td><td>string</td><td><code></code></td><td>user/role-based audit logging configuration</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-sql-log-user-audit" class="anchored"><code>sql.log.user_audit</code></div></td><td>string</td><td><code></code></td><td>user/role-based audit logging configuration. An enterprise license is required for this cluster setting to take effect.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-log-user-audit-reduced-config-enabled" class="anchored"><code>sql.log.user_audit.reduced_config.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables logic to compute a reduced audit configuration, computing the audit configuration only once at session start instead of at each SQL event. The tradeoff with the increase in performance (~5%), is that changes to the audit configuration (user role memberships/cluster setting) are not reflected within session. Users will need to start a new session to see these changes in their auditing behaviour.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-metrics-index-usage-stats-enabled" class="anchored"><code>sql.metrics.index_usage_stats.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>collect per index usage statistics</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-metrics-max-mem-reported-stmt-fingerprints" class="anchored"><code>sql.metrics.max_mem_reported_stmt_fingerprints</code></div></td><td>integer</td><td><code>100000</code></td><td>the maximum number of reported statement fingerprints stored in memory</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

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",
     ],
 )

pkg/ccl/auditloggingccl/audit_log_config.go

@@ -15,9 +15,8 @@ import (
 	"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/cockroach/pkg/util/uuid"
 	"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,8 @@ 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 {
+	return UserAuditLogConfig.Get(&st.SV) != "" && utilccl.IsEnterpriseEnabled(st, clusterID, "role-based audit logging")
 }
 
 var UserAuditReducedConfigEnabled = func(sv *settings.Values) bool {
@@ -114,6 +104,6 @@ var UserAuditReducedConfigEnabled = func(sv *settings.Values) bool {
 
 func init() {
 	auditlogging.ConfigureRoleBasedAuditClusterSettings = ConfigureRoleBasedAuditClusterSettings
-	auditlogging.UserAuditLogConfigEmpty = UserAuditLogConfigEmpty
+	auditlogging.UserAuditEnabled = UserAuditEnabled
 	auditlogging.UserAuditReducedConfigEnabled = UserAuditReducedConfigEnabled
 }

pkg/ccl/auditloggingccl/audit_logging_test.go

@@ -34,20 +34,79 @@ import (
 
 func TestRoleBasedAuditEnterpriseGated(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	sc := log.ScopeWithoutShowLogs(t)
+	defer sc.Close(t)
+
+	cleanup := logtestutils.InstallLogFileSink(sc, t, logpb.Channel_SENSITIVE_ACCESS)
+	defer cleanup()
 
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	rootRunner := sqlutils.MakeSQLRunner(sqlDB)
 	defer s.Stopper().Stop(context.Background())
 
-	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()
-	rootRunner.ExpectErr(t, "role-based audit logging requires enterprise license", testQuery)
-	// Enable enterprise.
+
+	// Enable auditing.
+	rootRunner.Exec(t, `SET CLUSTER SETTING sql.log.user_audit = 'ALL ALL'`)
+
+	testutils.SucceedsSoon(t, func() error {
+		var currentVal string
+		rootRunner.QueryRow(t,
+			"SHOW CLUSTER SETTING sql.log.user_audit",
+		).Scan(&currentVal)
+		if currentVal == "" {
+			return errors.Newf("waiting for cluster setting to be set")
+		}
+		return nil
+	})
+
+	// Run a test query.
+	rootRunner.Exec(t, `SHOW CLUSTER SETTING sql.log.user_audit`)
+
+	log.Flush()
+
+	entries, err := log.FetchEntriesFromFiles(
+		0,
+		math.MaxInt64,
+		10000,
+		regexp.MustCompile(`"EventType":"role_based_audit_event"`),
+		log.WithMarkedSensitiveData,
+	)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Enterprise is disabled, expect the number of entries to be 0.
+	if len(entries) != 0 {
+		t.Fatal(errors.Newf("enterprise is disabled, found unexpected entries"))
+	}
+
+	// Enable enterprise
 	enableEnterprise()
-	// Test that we can change the cluster setting when enterprise is enabled.
-	rootRunner.Exec(t, testQuery)
+
+	// Run a test query.
+	rootRunner.Exec(t, `SHOW CLUSTER SETTING sql.log.user_audit`)
+
+	log.Flush()
+
+	entries, err = log.FetchEntriesFromFiles(
+		0,
+		math.MaxInt64,
+		10000,
+		regexp.MustCompile(`"EventType":"role_based_audit_event"`),
+		log.WithMarkedSensitiveData,
+	)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Enterprise is enabled, expect an audit log.
+	if len(entries) != 1 {
+		t.Fatal(errors.Newf("enterprise is enabled, expected 1 entry, got %d", len(entries)))
+	}
 }
 
 func TestSingleRoleAuditLogging(t *testing.T) {

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{

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
 }

pkg/sql/auditlogging/audit_log.go

@@ -24,7 +24,6 @@ import (
 	"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)