sql: disallow tenants from setting zone configs by default

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs

@@ -1,8 +1,6 @@
+# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
-statement ok
-SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true
-
 query TTTT
 SHOW REGIONS
 ----

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant
 
 statement ok
@@ -271,7 +272,6 @@ public       regional_by_row_table_explicit_crdb_region_column  table  root   0
 
 # Add a gc.ttlseconds to a partition and ensure it displays.
 statement ok
-SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true;
 ALTER PARTITION "us-east-1" OF INDEX public.regional_by_row_table@regional_by_row_table_a_idx
 CONFIGURE ZONE USING gc.ttlseconds = 10
 

pkg/ccl/logictestccl/testdata/logic_test/zone_config_secondary_tenants

@@ -1,18 +1,10 @@
 # LogicTest: 3node-tenant
+# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants
 # Zone config logic tests that are only meant to work for secondary tenants.
 
 statement ok
 CREATE TABLE t();
 
-statement ok
-SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = false
-
-statement error pq: unimplemented: operation is unsupported in multi-tenancy mode
-ALTER TABLE t CONFIGURE ZONE USING num_replicas = 5;
-
-statement ok
-SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true
-
 statement ok
 ALTER TABLE t CONFIGURE ZONE USING num_replicas = 5;
 

pkg/ccl/logictestccl/testdata/logic_test/zone_config_secondary_tenants_disallowed

@@ -0,0 +1,11 @@
+# LogicTest: 3node-tenant
+
+# Note that we haven't used the setting override directive in this file to
+# override the default.
+
+statement error pq: unimplemented: operation is unsupported in multi-tenancy mode
+ALTER TABLE t CONFIGURE ZONE USING num_replicas = 5;
+
+statement error setting sql.zone_configs.allow_for_secondary_tenant.enabled is only settable by the operator
+SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true
+

pkg/ccl/spanconfigccl/spanconfigcomparedccl/datadriven_test.go

@@ -186,7 +186,8 @@ func TestDataDriven(t *testing.T) {
 			switch d.Cmd {
 			case "initialize":
 				secondaryTenant := spanConfigTestCluster.InitializeTenant(ctx, tenantID)
-				secondaryTenant.Exec(`SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true`)
+				spanConfigTestCluster.AllowSecondaryTenantToSetZoneConfigurations(t, tenantID)
+				spanConfigTestCluster.EnsureTenantCanSetZoneConfigurationsOrFatal(t, secondaryTenant)
 
 			case "exec-sql":
 				// Run under an explicit transaction -- we rely on having a

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go

@@ -136,7 +136,8 @@ func TestDataDriven(t *testing.T) {
 			switch d.Cmd {
 			case "initialize":
 				secondaryTenant := spanConfigTestCluster.InitializeTenant(ctx, tenantID)
-				secondaryTenant.Exec(`SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true`)
+				spanConfigTestCluster.AllowSecondaryTenantToSetZoneConfigurations(t, tenantID)
+				spanConfigTestCluster.EnsureTenantCanSetZoneConfigurationsOrFatal(t, secondaryTenant)
 
 			case "exec-sql":
 				// Run under an explicit transaction -- we rely on having a

pkg/ccl/spanconfigccl/spanconfigsplitterccl/datadriven_test.go

@@ -80,8 +80,10 @@ func TestDataDriven(t *testing.T) {
 
 		var tenant *spanconfigtestcluster.Tenant
 		if strings.Contains(path, "tenant") {
-			tenant = spanConfigTestCluster.InitializeTenant(ctx, roachpb.MakeTenantID(10))
-			tenant.Exec(`SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true`)
+			tenantID := roachpb.MakeTenantID(10)
+			tenant = spanConfigTestCluster.InitializeTenant(ctx, tenantID)
+			spanConfigTestCluster.AllowSecondaryTenantToSetZoneConfigurations(t, tenantID)
+			spanConfigTestCluster.EnsureTenantCanSetZoneConfigurationsOrFatal(t, tenant)
 		} else {
 			tenant = spanConfigTestCluster.InitializeTenant(ctx, roachpb.SystemTenantID)
 		}

pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go

@@ -121,8 +121,10 @@ func TestDataDriven(t *testing.T) {
 
 		var tenant *spanconfigtestcluster.Tenant
 		if strings.Contains(path, "tenant") {
-			tenant = spanConfigTestCluster.InitializeTenant(ctx, roachpb.MakeTenantID(10))
-			tenant.Exec(`SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true`)
+			tenantID := roachpb.MakeTenantID(10)
+			tenant = spanConfigTestCluster.InitializeTenant(ctx, tenantID)
+			spanConfigTestCluster.AllowSecondaryTenantToSetZoneConfigurations(t, tenantID)
+			spanConfigTestCluster.EnsureTenantCanSetZoneConfigurationsOrFatal(t, tenant)
 		} else {
 			tenant = spanConfigTestCluster.InitializeTenant(ctx, roachpb.SystemTenantID)
 		}

pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster/BUILD.bazel

@@ -28,12 +28,14 @@ go_library(
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/distsql",
         "//pkg/sql/sem/tree",
+        "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/hlc",
         "//pkg/util/syncutil",
         "//pkg/util/uuid",
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",
     ],
 )

pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster/cluster.go

@@ -24,8 +24,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigsqltranslator"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 )
 
@@ -108,6 +110,39 @@ func (h *Handle) InitializeTenant(ctx context.Context, tenID roachpb.TenantID) *
 	return tenantState
 }
 
+// AllowSecondaryTenantToSetZoneConfigurations enables zone configuration
+// support for the given tenant. Given the cluster setting involved is tenant
+// read-only, the SQL statement is run as the system tenant.
+func (h *Handle) AllowSecondaryTenantToSetZoneConfigurations(t *testing.T, tenID roachpb.TenantID) {
+	_, found := h.LookupTenant(tenID)
+	require.True(t, found)
+	sqlDB := sqlutils.MakeSQLRunner(h.tc.ServerConn(0))
+	sqlDB.Exec(
+		t,
+		"ALTER TENANT $1 SET CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled = true",
+		tenID.ToUint64(),
+	)
+}
+
+// EnsureTenantCanSetZoneConfigurationsOrFatal ensures that the tenant observes
+// a 'true' value for sql.zone_configs.allow_for_secondary_tenants.enabled. It
+// fatals if this condition doesn't evaluate within SucceedsSoonDuration.
+func (h *Handle) EnsureTenantCanSetZoneConfigurationsOrFatal(t *testing.T, tenant *Tenant) {
+	testutils.SucceedsSoon(t, func() error {
+		var val string
+		tenant.QueryRow(
+			"SHOW CLUSTER SETTING sql.zone_configs.allow_for_secondary_tenant.enabled",
+		).Scan(&val)
+
+		if val == "false" {
+			return errors.New(
+				"waiting for sql.zone_configs.allow_for_secondary_tenant.enabled to be updated",
+			)
+		}
+		return nil
+	})
+}
+
 // LookupTenant returns the relevant tenant state, if any.
 func (h *Handle) LookupTenant(tenantID roachpb.TenantID) (_ *Tenant, found bool) {
 	s, ok := h.ts[tenantID]

pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster/tenant_state.go

@@ -116,6 +116,11 @@ func (s *Tenant) Query(query string, args ...interface{}) *gosql.Rows {
 	return s.db.Query(s.t, query, args...)
 }
 
+// QueryRow is a wrapper around gosql.QueryRow that kills the test on error.
+func (s *Tenant) QueryRow(query string, args ...interface{}) *sqlutils.Row {
+	return s.db.QueryRow(s.t, query, args...)
+}
+
 // Reconciler returns the reconciler associated with the given tenant.
 func (s *Tenant) Reconciler() spanconfig.Reconciler {
 	return s.reconciler

pkg/sql/exec_util.go

@@ -197,14 +197,11 @@ const secondaryTenantsZoneConfigsEnabledSettingName = "sql.zone_configs.allow_fo
 // to set zone configurations. It has no effect for the system tenant.
 //
 // This setting has no effect on zone configurations that have already been set.
-//
-// TODO(irfansharif): This should be a tenant-readonly setting, possible after
-// the work for #73349 is completed.
 var secondaryTenantZoneConfigsEnabled = settings.RegisterBoolSetting(
-	settings.TenantWritable,
+	settings.TenantReadOnly,
 	secondaryTenantsZoneConfigsEnabledSettingName,
 	"allow secondary tenants to set zone configurations; does not affect the system tenant",
-	true,
+	false,
 )
 
 // traceTxnThreshold can be used to log SQL transactions that take