multitenant: allow secondary tenants to split/scatter by default

AdminSplit and AdminScatter requests are subject to capability checks.
Previously, these capabilities were codified in the "enabled" form. As
such, by default, secondary tenants did not have the ability to perform
these operations. This is in violation of what secondary tenants could
do prior to 23.1, at a time before capabilities existed. Moreover,
RESTORE/IMPORT rely on performing these operations for performance.
This made disallowing these operations by default a performance
regression.

This patch flips the phrasing of how these capabilities are stored on
the proto to use the "disable" verbiage. As such, secondary tenants are
able to perform splits and scatters by default. However, no change is
made to the public interface -- users above the `tenantcapabilitiespb`
package continue to interact with these capabilities as they were
before, oblivious to how these things are stored on disk.

There's a few testing changes here:
- As part of this change, we also clean up a testing knob that was used
by various backup, CDC, and logictests to override capability checks in
the authorizer. This isn't required with the new default behaviour.
- We also add some missing E2E tests for the `CanAdminUnsplit` capability
which were missing when it was introduced.
- The construction of `TestMultiTenantAdminFunction` was making it
cumborsome to work with now that we have changed the default behavior
of splits and scatters. Instead of trying to wrestle with that test,
I decided to convert some of the SPLIT/SCATTER tests to the datadriven
E2E tests we have for capabilities. At some point, we should get rid
of this entire thing, but I'm going to limit scope here for now.

Fixes cockroachdb#96736

Release note: None

pkg/ccl/backupccl/BUILD.bazel

@@ -240,7 +240,6 @@ go_test(
         "//pkg/kv/kvserver/protectedts/ptpb",
         "//pkg/kv/kvserver/protectedts/ptutil",
         "//pkg/multitenant/mtinfopb",
-        "//pkg/multitenant/tenantcapabilities",
         "//pkg/roachpb",
         "//pkg/scheduledjobs",
         "//pkg/scheduledjobs/schedulebase",

pkg/ccl/backupccl/backup_test.go

@@ -67,7 +67,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptutil"
 	"github.com/cockroachdb/cockroach/pkg/multitenant/mtinfopb"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
@@ -2949,13 +2948,6 @@ func TestBackupRestoreCrossTableReferences(t *testing.T) {
 	defer cleanupFn()
 	args := base.TestServerArgs{
 		ExternalIODir: dir,
-		Knobs: base.TestingKnobs{
-			TenantCapabilitiesTestingKnobs: &tenantcapabilities.TestingKnobs{
-				// TODO(arul): This can be removed once
-				// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-				AuthorizerSkipAdminSplitCapabilityChecks: true,
-			},
-		},
 	}
 
 	// Generate some testdata and back it up.
@@ -5296,13 +5288,6 @@ func TestBackupRestoreSequence(t *testing.T) {
 	defer cleanupFn()
 	args := base.TestServerArgs{
 		ExternalIODir: dir,
-		Knobs: base.TestingKnobs{
-			TenantCapabilitiesTestingKnobs: &tenantcapabilities.TestingKnobs{
-				// TODO(arul): This can be removed once
-				// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-				AuthorizerSkipAdminSplitCapabilityChecks: true,
-			},
-		},
 	}
 
 	backupLoc := localFoo

pkg/ccl/backupccl/datadriven_test.go

@@ -31,7 +31,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/closedts"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
@@ -155,14 +154,6 @@ func (d *datadrivenTestState) addCluster(t *testing.T, cfg clusterCfg) error {
 	params.ServerArgs.Knobs = base.TestingKnobs{
 		JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
 	}
-	// Backups issue splits underneath the hood, and as such, will fail capability
-	// checks for tests that run as secondary tenants. Skip these checks at a
-	// global level using a testing knob.
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 
 	settings := cluster.MakeTestingClusterSettings()
 

pkg/ccl/backupccl/utils_test.go

@@ -35,7 +35,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keyvisualizer"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -96,11 +95,6 @@ func backupRestoreTestSetupWithParams(
 		}
 		params.ServerArgs.Knobs.Store.(*kvserver.StoreTestingKnobs).SmallEngineBlocks = true
 	}
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 
 	params.ServerArgs.Knobs.KeyVisualizer = &keyvisualizer.TestingKnobs{
 		SkipJobBootstrap:        true,
@@ -154,13 +148,6 @@ func backupRestoreTestSetup(
 		base.TestClusterArgs{
 			ServerArgs: base.TestServerArgs{
 				DisableDefaultTestTenant: true,
-				Knobs: base.TestingKnobs{
-					TenantCapabilitiesTestingKnobs: &tenantcapabilities.TestingKnobs{
-						// TODO(arul): This can be removed once
-						// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-						AuthorizerSkipAdminSplitCapabilityChecks: true,
-					},
-				},
 			}})
 }
 
@@ -173,11 +160,6 @@ func backupRestoreTestSetupEmpty(
 ) (tc *testcluster.TestCluster, sqlDB *sqlutils.SQLRunner, cleanup func()) {
 	// TODO (msbutler): this should be disabled by callers of this function
 	params.ServerArgs.DisableDefaultTestTenant = true
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 	return backupRestoreTestSetupEmptyWithParams(t, clusterSize, tempDir, init, params)
 }
 
@@ -205,11 +187,6 @@ func backupRestoreTestSetupEmptyWithParams(
 		}
 		params.ServerArgs.Knobs.Store.(*kvserver.StoreTestingKnobs).SmallEngineBlocks = true
 	}
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 
 	tc = testcluster.StartTestCluster(t, clusterSize, params)
 	init(tc)
@@ -235,11 +212,6 @@ func createEmptyCluster(
 	params.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{
 		SmallEngineBlocks: smallEngineBlocks,
 	}
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 	tc := testcluster.StartTestCluster(t, clusterSize, params)
 
 	sqlDB = sqlutils.MakeSQLRunner(tc.Conns[0])

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup

@@ -1,6 +1,5 @@
 # tenant-cluster-setting-override-opt: sql.zone_configs.allow_for_secondary_tenant.enabled=true sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-no-los
-# cluster-opt: can-admin-split
 
 # Tests in this file assume no multi-region tenant setup as tenants have no
 # access to nodelocal.

pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_mr

@@ -1,5 +1,4 @@
 # tenant-cluster-setting-override-opt: sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
-# cluster-opt: can-admin-split
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant multiregion-9node-3region-3azs-no-los
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_query_plan

@@ -1,4 +1,3 @@
-# cluster-opt: can-admin-split
 # LogicTest: 5node !metamorphic-batch-sizes
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index

@@ -1,5 +1,4 @@
 # tenant-cluster-setting-override-opt: sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
-# cluster-opt: can-admin-split
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-no-los
 
 statement ok

pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go

@@ -100,6 +100,7 @@ func TestDataDriven(t *testing.T) {
 		// us from being able to do so.
 		settings := cluster.MakeTestingClusterSettings()
 		sql.SecondaryTenantSplitAtEnabled.Override(ctx, &settings.SV, true)
+		sql.SecondaryTenantScatterEnabled.Override(ctx, &settings.SV, true)
 		tenantArgs := base.TestTenantArgs{
 			TenantID: serverutils.TestTenantID(),
 			Settings: settings,
@@ -115,7 +116,7 @@ func TestDataDriven(t *testing.T) {
 		}()
 		require.NoError(t, err)
 
-		var lastUpdateTS hlc.Timestamp
+		lastUpdateTS := tc.Server(0).Clock().Now() // ensure watcher isn't starting out empty
 		datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
 
 			switch d.Cmd {

pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter

@@ -0,0 +1,79 @@
+query-sql-system
+SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_scatter'
+----
+10 tenant-10 ready none can_admin_scatter true
+
+exec-sql-tenant
+CREATE TABLE t(a INT)
+----
+ok
+
+exec-sql-tenant
+CREATE INDEX idx on t(a)
+----
+ok
+
+# By default, we should be able to scatter.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+ok
+
+# ditto for the index.
+exec-privileged-op-tenant
+ALTER INDEX t@idx SCATTER
+----
+ok
+
+
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter=false
+----
+ok
+
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+pq: ba: AdminScatter [/Tenant/10/Table/104/1,/Tenant/10/Table/104/2) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
+# Check the index as well.
+exec-privileged-op-tenant
+ALTER INDEX t@idx SCATTER
+----
+pq: ba: AdminScatter [/Tenant/10/Table/104/2,/Tenant/10/Table/104/3) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
+# Grant the capability without providing an explicit value.
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter
+----
+ok
+
+# Scatters should work now.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+ok
+
+# Revoke the capability using REVOKE syntax.
+update-capabilities
+ALTER TENANT [10] REVOKE CAPABILITY can_admin_scatter
+----
+ok
+
+# Scatters should no longer work.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+pq: ba: AdminScatter [/Tenant/10/Table/104/1,/Tenant/10/Table/104/2) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
+# Lastly, use the explicitly set to true syntax.
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter=true
+----
+ok
+
+# Scatters should now work.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+ok

pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split

@@ -1,34 +1,79 @@
 query-sql-system
 SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_split'
 ----
-10 tenant-10 ready none can_admin_split false
+10 tenant-10 ready none can_admin_split true
 
 exec-sql-tenant
 CREATE TABLE t(a INT)
 ----
 ok
 
+exec-sql-tenant
+CREATE INDEX idx on t(a)
+----
+ok
+
+# By default, we should be able to split.
+exec-privileged-op-tenant
+ALTER TABLE t SPLIT AT VALUES (0)
+----
+ok
+
+# ditto for the index.
+exec-privileged-op-tenant
+ALTER INDEX t@idx SPLIT AT VALUES (1)
+----
+ok
+
+
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_split=false
+----
+ok
+
 exec-privileged-op-tenant
 ALTER TABLE t SPLIT AT VALUES (0)
 ----
 pq: ba: AdminSplit [/Tenant/10/Table/104/1/0,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest)
 
+# Check the index as well.
+exec-privileged-op-tenant
+ALTER INDEX t@idx SPLIT AT VALUES (1)
+----
+pq: ba: AdminSplit [/Tenant/10/Table/104/2/1,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest)
+
+# Grant the capability without providing an explicit value.
 update-capabilities
-ALTER TENANT [10] GRANT CAPABILITY can_admin_split=true
+ALTER TENANT [10] GRANT CAPABILITY can_admin_split
 ----
 ok
 
+# Splits should work now.
 exec-privileged-op-tenant
 ALTER TABLE t SPLIT AT VALUES (0)
 ----
 ok
 
+# Revoke the capability using REVOKE syntax.
 update-capabilities
 ALTER TENANT [10] REVOKE CAPABILITY can_admin_split
 ----
 ok
 
+# Splits should no longer work.
 exec-privileged-op-tenant
 ALTER TABLE t SPLIT AT VALUES (0)
 ----
 pq: ba: AdminSplit [/Tenant/10/Table/104/1/0,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest)
+
+# Lastly, use the explicitly set to true syntax.
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_split=true
+----
+ok
+
+# Splits should now work.
+exec-privileged-op-tenant
+ALTER TABLE t SPLIT AT VALUES (0)
+----
+ok

pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit

@@ -0,0 +1,34 @@
+query-sql-system
+SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_unsplit'
+----
+10 tenant-10 ready none can_admin_unsplit false
+
+exec-sql-tenant
+CREATE TABLE t(a INT)
+----
+ok
+
+exec-privileged-op-tenant
+ALTER TABLE t UNSPLIT AT VALUES (0)
+----
+pq: could not UNSPLIT AT (0): ba: AdminUnsplit [/Tenant/10/Table/104/1/0,/Min) RPC error: grpc: client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) [code 16/Unauthenticated]
+
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_unsplit=true
+----
+ok
+
+exec-privileged-op-tenant
+ALTER TABLE t UNSPLIT AT VALUES (0)
+----
+pq: could not UNSPLIT AT (0): key /Tenant/10/Table/104/1/0 is not the start of a range
+
+update-capabilities
+ALTER TENANT [10] REVOKE CAPABILITY can_admin_unsplit
+----
+ok
+
+exec-privileged-op-tenant
+ALTER TABLE t UNSPLIT AT VALUES (0)
+----
+pq: could not UNSPLIT AT (0): ba: AdminUnsplit [/Tenant/10/Table/104/1/0,/Min) RPC error: grpc: client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) [code 16/Unauthenticated]

pkg/ccl/streamingccl/streamingest/BUILD.bazel

@@ -112,7 +112,6 @@ go_test(
         "//pkg/kv/kvpb",
         "//pkg/kv/kvserver",
         "//pkg/kv/kvserver/protectedts",
-        "//pkg/multitenant/tenantcapabilities",
         "//pkg/repstream/streampb",
         "//pkg/roachpb",
         "//pkg/security/securityassets",