Merge #79536

79536: sql: gate tenant's multi-region abstraction usage behind cluster setting r=arulajmani a=arulajmani

This patch introduces a tenant read-only cluster setting called
`sql.multi_region.allow_abstractions_for_secondary_tenants.enabled`
which allows the operator to control if secondary tenants can make use
of multi-region abstractions. It defaults to false.

This setting is checked against when creating new MR databases or
altering existing ones to make use of MR features (by setting the
primary region). It has nothing to do with MR databases that may have
been created previously which could happen if this setting was ever
flipped to true by the operator.

This setting doesn't effect the system tenant. It also takes precedence
over `sql.defaults.primary_region`, which is tenant writeable.

Release note (sql change): introduces new cluster setting which allows
the operator to control if a secondary tenant can make use of MR
abstractions. The setting is called
`sql.multi_region.allow_abstractions_for_secondary_tenants.enabled`.

Co-authored-by: arulajmani <[email protected]>

pkg/ccl/backupccl/BUILD.bazel

@@ -187,6 +187,7 @@ go_test(
         "//pkg/blobs",
         "//pkg/ccl/kvccl",
         "//pkg/ccl/multiregionccl",
+        "//pkg/ccl/multiregionccl/multiregionccltestutils",
         "//pkg/ccl/multitenantccl",
         "//pkg/ccl/partitionccl",
         "//pkg/ccl/storageccl",

pkg/ccl/backupccl/backup_tenant_test.go

@@ -10,17 +10,21 @@ package backupccl
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils"
 	_ "github.com/cockroachdb/cockroach/pkg/cloud/impl" // register cloud storage providers
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/sql"
 	_ "github.com/cockroachdb/cockroach/pkg/sql/importer"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -109,3 +113,114 @@ func TestBackupTenantImportingTable(t *testing.T) {
 	}
 	require.Equal(t, 1, rowCount)
 }
+
+// TestTenantBackupMultiRegionDatabases ensures secondary tenants restoring
+// MR databases respect the
+// sql.multi_region.allow_abstractions_for_secondary_tenants.enabled cluster
+// setting.
+func TestTenantBackupMultiRegionDatabases(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	skip.UnderStressRace(t, "times out")
+
+	tc, db, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(
+		t, 3 /*numServers*/, base.TestingKnobs{},
+	)
+	defer cleanup()
+	sqlDB := sqlutils.MakeSQLRunner(db)
+
+	tenID := roachpb.MakeTenantID(10)
+	_, tSQL := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{
+		TenantID:     tenID,
+		TestingKnobs: base.TestingKnobs{JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals()},
+	})
+	defer tSQL.Close()
+	tenSQLDB := sqlutils.MakeSQLRunner(tSQL)
+
+	waitForSettingToTakeEffect := func(expValue string) {
+		testutils.SucceedsSoon(t, func() error {
+			var val string
+			tenSQLDB.QueryRow(t,
+				fmt.Sprintf(
+					"SHOW CLUSTER SETTING %s", sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+				),
+			).Scan(&val)
+
+			if val != expValue {
+				return errors.Newf("waiting for cluster setting to be set to %q", expValue)
+			}
+			return nil
+		})
+	}
+
+	setTenantReadOnlyClusterSetting := func(val string) {
+		sqlDB.Exec(
+			t,
+			fmt.Sprintf(
+				"ALTER TENANT $1 SET CLUSTER	SETTING %s = '%s'",
+				sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+				val,
+			),
+			tenID.ToUint64(),
+		)
+	}
+
+	setTenantReadOnlyClusterSetting("true")
+	waitForSettingToTakeEffect("true")
+
+	// Setup.
+	const tenDst = "userfile:///ten_backup"
+	const hostDst = "userfile:///host_backup"
+	tenSQLDB.Exec(t, `CREATE DATABASE mrdb PRIMARY REGION "us-east1"`)
+	tenSQLDB.Exec(t, fmt.Sprintf("BACKUP DATABASE mrdb INTO '%s'", tenDst))
+
+	sqlDB.Exec(t, `CREATE DATABASE mrdb PRIMARY REGION "us-east1"`)
+	sqlDB.Exec(t, fmt.Sprintf("BACKUP DATABASE mrdb INTO '%s'", hostDst))
+
+	{
+		// Flip the tenant-read only cluster setting; ensure database can be restored
+		// on the system tenant but not on the secondary tenant.
+		setTenantReadOnlyClusterSetting("false")
+		waitForSettingToTakeEffect("false")
+
+		tenSQLDB.Exec(t, "DROP DATABASE mrdb CASCADE")
+		tenSQLDB.ExpectErr(
+			t,
+			"setting .* disallows secondary tenant to restore a multi-region database",
+			fmt.Sprintf("RESTORE DATABASE mrdb FROM LATEST IN '%s'", tenDst),
+		)
+
+		// The system tenant should remain unaffected.
+		sqlDB.Exec(t, "DROP DATABASE mrdb CASCADE")
+		sqlDB.Exec(t, fmt.Sprintf("RESTORE DATABASE mrdb FROM LATEST IN '%s'", hostDst))
+	}
+
+	{
+		// Flip the tenant-read only cluster setting back to true and ensure the
+		// restore succeeds.
+		setTenantReadOnlyClusterSetting("true")
+		waitForSettingToTakeEffect("true")
+
+		tenSQLDB.Exec(t, fmt.Sprintf("RESTORE DATABASE mrdb FROM LATEST IN '%s'", tenDst))
+	}
+
+	{
+		// Ensure tenant's restoring non multi-region databases are unaffected
+		// by this setting. We set sql.defaults.primary_region for good measure.
+		tenSQLDB.Exec(
+			t,
+			fmt.Sprintf(
+				"SET CLUSTER SETTING %s = 'us-east1'", sql.DefaultPrimaryRegionClusterSettingName,
+			),
+		)
+		setTenantReadOnlyClusterSetting("false")
+		waitForSettingToTakeEffect("false")
+
+		tenSQLDB.Exec(t, "CREATE DATABASE nonMrDB")
+		tenSQLDB.Exec(t, fmt.Sprintf("BACKUP DATABASE nonMrDB INTO '%s'", tenDst))
+
+		tenSQLDB.Exec(t, "DROP DATABASE nonMrDB CASCADE")
+		tenSQLDB.Exec(t, fmt.Sprintf("RESTORE DATABASE nonMrDB FROM LATEST IN '%s'", tenDst))
+	}
+}

pkg/ccl/backupccl/restore_planning.go

@@ -1598,6 +1598,11 @@ func doRestorePlan(
 		}
 	}
 
+	err = ensureMultiRegionDatabaseRestoreIsAllowed(p, restoreDBs)
+	if err != nil {
+		return err
+	}
+
 	databaseModifiers, newTypeDescs, err := planDatabaseModifiersForRestore(ctx, p, sqlDescs, restoreDBs)
 	if err != nil {
 		return err
@@ -1949,6 +1954,33 @@ func renameTargetDatabaseDescriptor(
 	return nil
 }
 
+// ensureMultiRegionDatabaseRestoreIsAllowed returns an error if restoring a
+// multi-region database is not allowed.
+func ensureMultiRegionDatabaseRestoreIsAllowed(
+	p sql.PlanHookState, restoreDBs []catalog.DatabaseDescriptor,
+) error {
+	if p.ExecCfg().Codec.ForSystemTenant() ||
+		sql.SecondaryTenantsMultiRegionAbstractionsEnabled.Get(&p.ExecCfg().Settings.SV) {
+		// The system tenant is always allowed to restore multi-region databases;
+		// secondary tenants are only allowed to restore multi-region databases
+		// if the cluster setting above allows such.
+		return nil
+	}
+	for _, dbDesc := range restoreDBs {
+		// If a database descriptor being restored is a multi-region database,
+		// return an error.
+		if dbDesc.IsMultiRegion() {
+			return pgerror.Newf(
+				pgcode.InvalidDatabaseDefinition,
+				"setting %s disallows secondary tenant to restore a multi-region database",
+				sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+			)
+		}
+	}
+	// We're good.
+	return nil
+}
+
 func planDatabaseModifiersForRestore(
 	ctx context.Context,
 	p sql.PlanHookState,
@@ -1959,6 +1991,12 @@ func planDatabaseModifiersForRestore(
 	defaultPrimaryRegion := catpb.RegionName(
 		sql.DefaultPrimaryRegion.Get(&p.ExecCfg().Settings.SV),
 	)
+	if !p.ExecCfg().Codec.ForSystemTenant() &&
+		!sql.SecondaryTenantsMultiRegionAbstractionsEnabled.Get(&p.ExecCfg().Settings.SV) {
+		// We don't want to restore "regular" databases as multi-region databases
+		// for secondary tenants.
+		return nil, nil, nil
+	}
 	if defaultPrimaryRegion == "" {
 		return nil, nil, nil
 	}

pkg/ccl/logictestccl/testdata/logic_test/multi_region

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 query TTTT colnames

pkg/ccl/logictestccl/testdata/logic_test/multi_region_default_primary_region

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/multi_region_drop_region

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 query TTTT

pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 user root

pkg/ccl/logictestccl/testdata/logic_test/multi_region_secondary_tenants_abstractions_disallowed

@@ -0,0 +1,10 @@
+# LogicTest: multiregion-9node-3region-3azs-tenant
+
+statement error pq: setting sql.multi_region.allow_abstractions_for_secondary_tenants.enabled disallows use of multi-region abstractions
+CREATE DATABASE db PRIMARY REGION "us-east1"
+
+statement ok
+CREATE DATABASE db
+
+statement error pq: setting sql.multi_region.allow_abstractions_for_secondary_tenants.enabled disallows use of multi-region abstractions
+ALTER DATABASE db SET PRIMARY REGION "us-east-1"

pkg/ccl/logictestccl/testdata/logic_test/multi_region_show

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs

@@ -1,4 +1,4 @@
-# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants
+# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 query TTTT

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row

@@ -1,4 +1,4 @@
-# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants
+# tenant-cluster-setting-override-opt: allow-zone-configs-for-secondary-tenants allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_auto_rehoming

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_placement_restricted

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_rename_column

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant
 
 # This test seems to flake (#60717).

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_placement_restricted

@@ -1,3 +1,4 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant
 
 statement ok

pkg/sql/descriptor.go

@@ -361,6 +361,25 @@ var DefaultPrimaryRegion = settings.RegisterStringSetting(
 	"",
 ).WithPublic()
 
+// SecondaryTenantsMultiRegionAbstractionsEnabledSettingName is the name of the
+// cluster setting that governs secondary tenant multi-region abstraction usage.
+const SecondaryTenantsMultiRegionAbstractionsEnabledSettingName = "sql.multi_region.allow_abstractions_for_secondary_tenants.enabled"
+
+// SecondaryTenantsMultiRegionAbstractionsEnabled controls if secondary tenants
+// are allowed to use multi-region abstractions. In particular, it controls if
+// secondary tenants are allowed to add a region to their database. It has no
+// effect on the system tenant.
+//
+// This setting has no effect for existing multi-region databases that have
+// already been configured. It only affects regions being added to new
+// databases.
+var SecondaryTenantsMultiRegionAbstractionsEnabled = settings.RegisterBoolSetting(
+	settings.TenantReadOnly,
+	SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+	"allow secondary tenants to use multi-region abstractions",
+	false,
+)
+
 // maybeInitializeMultiRegionMetadata initializes multi-region metadata if a
 // primary region is supplied and works as a pass-through otherwise. It creates
 // a new region config from the given parameters and reserves an ID for the
@@ -372,6 +391,21 @@ func (p *planner) maybeInitializeMultiRegionMetadata(
 	regions []tree.Name,
 	placement tree.DataPlacement,
 ) (*multiregion.RegionConfig, error) {
+	if !p.execCfg.Codec.ForSystemTenant() &&
+		!SecondaryTenantsMultiRegionAbstractionsEnabled.Get(&p.execCfg.Settings.SV) {
+		// There was no primary region provided, let the thing pass through.
+		if primaryRegion == "" && len(regions) == 0 {
+			return nil, nil
+		}
+
+		return nil, errors.WithHint(pgerror.Newf(
+			pgcode.InvalidDatabaseDefinition,
+			"setting %s disallows use of multi-region abstractions",
+			SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+		),
+			"consider omitting the primary region")
+	}
+
 	if primaryRegion == "" && len(regions) == 0 {
 		defaultPrimaryRegion := DefaultPrimaryRegion.Get(&p.execCfg.Settings.SV)
 		if defaultPrimaryRegion == "" {