diff --git a/pkg/ccl/backupccl/BUILD.bazel b/pkg/ccl/backupccl/BUILD.bazel index a5025629a3d7..42ce178f8008 100644 --- a/pkg/ccl/backupccl/BUILD.bazel +++ b/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", diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index f427ca11e25b..ac2353d7b0e2 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/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 diff --git a/pkg/ccl/backupccl/datadriven_test.go b/pkg/ccl/backupccl/datadriven_test.go index ca0f51359315..279ecc00da75 100644 --- a/pkg/ccl/backupccl/datadriven_test.go +++ b/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() diff --git a/pkg/ccl/backupccl/utils_test.go b/pkg/ccl/backupccl/utils_test.go index bef1fe75f8d6..0136cbe36fbf 100644 --- a/pkg/ccl/backupccl/utils_test.go +++ b/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]) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup index b26cbacc99b2..95875a428226 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup +++ b/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. diff --git a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_mr b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_mr index 837451598e0b..0854dcb80c46 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_mr +++ b/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 diff --git a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_query_plan b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_query_plan index d9b6e191e789..f518445e19e7 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_query_plan +++ b/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 diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index index d9fd13ab6ae5..b4b6d2af579f 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index +++ b/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 diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go index 0e1267a04088..91299e4040c6 100644 --- a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go +++ b/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 { diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter new file mode 100644 index 000000000000..1ac15f590f47 --- /dev/null +++ b/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 diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split index b670f661bd69..42d20b80c3b2 100644 --- a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split +++ b/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 diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit new file mode 100644 index 000000000000..5550b75bbbe6 --- /dev/null +++ b/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] diff --git a/pkg/ccl/streamingccl/streamingest/BUILD.bazel b/pkg/ccl/streamingccl/streamingest/BUILD.bazel index 566e7b2776b3..9b7d99703cde 100644 --- a/pkg/ccl/streamingccl/streamingest/BUILD.bazel +++ b/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", diff --git a/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go b/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go index 19b20217e39e..c84fc8334a9f 100644 --- a/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go +++ b/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go @@ -22,7 +22,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts" - "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" @@ -685,9 +684,6 @@ func TestTenantStreamingMultipleNodes(t *testing.T) { clientAddresses[addr] = struct{}{} }, } - args.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{ - AuthorizerSkipAdminSplitCapabilityChecks: true, - } c, cleanup := replicationtestutils.CreateTenantStreamingClusters(ctx, t, args) defer cleanup() diff --git a/pkg/multitenant/tenantcapabilities/capabilities.go b/pkg/multitenant/tenantcapabilities/capabilities.go index 85c71407fd80..c1d0e23ee1f2 100644 --- a/pkg/multitenant/tenantcapabilities/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/capabilities.go @@ -162,16 +162,14 @@ type TenantCapabilities interface { const ( _ CapabilityID = iota - // CanAdminScatter describes the ability of a tenant to perform manual - // KV scatter requests. These operations need a capability - // because excessive KV range scatter can overwhelm the storage - // cluster. + // CanAdminScatter describes the ability of a tenant to scatter ranges using + // an AdminScatter request. By default, secondary tenants are allowed to + // scatter as doing so is integral to the performance of IMPORT/RESTORE. CanAdminScatter // can_admin_scatter - // CanAdminSplit describes the ability of a tenant to perform manual - // KV range split requests. These operations need a capability - // because excessive KV range splits can overwhelm the storage - // cluster. + // CanAdminSplit describes the ability of a tenant to perform KV requests to + // split ranges. By default, secondary tenants are allowed to perform splits + // as doing so is integral to performance of IMPORT/RESTORE. CanAdminSplit // can_admin_split // CanAdminUnsplit describes the ability of a tenant to perform manual diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go index d38b6d71f81a..9199a12dec73 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go @@ -83,16 +83,16 @@ func (a *Authorizer) HasCapabilityForBatch( continue } if !hasCap || requiredCap == onlySystemTenant || !found || !cp.GetBool(requiredCap) { - if (requiredCap == tenantcapabilities.CanAdminSplit || requiredCap == tenantcapabilities.CanAdminScatter) && - a.knobs.AuthorizerSkipAdminSplitCapabilityChecks { - continue - } // All allowable request types must be explicitly opted into the // reqMethodToCap map. If a request type is missing from the map // (!hasCap), we must be conservative and assume it is // disallowed. This prevents accidents where someone adds a new // sensitive request type in KV and forgets to add an explicit // authorization rule for it here. + // + // TODO(arul): This should be caught by a linter instead. Add a test that + // goes over all request types and ensures there's an entry in this map + // instead. return errors.Newf("client tenant does not have capability %q (%T)", requiredCap, request) } } diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic index 553c6e3110ba..f8ac12f62a94 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic @@ -34,9 +34,9 @@ has-capability-for-batch ten=11 cmds=(AdminUnsplit, Scan, ConditionalPut) client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) # Test that the order of the split request doesn't have any effect. -has-capability-for-batch ten=11 cmds=(Scan, ConditionalPut) +has-capability-for-batch ten=11 cmds=(Scan, ConditionalPut, AdminSplit) ---- -ok +client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) # However, a batch request which doesn't include a split (by tenant 11) should # work as you'd expect. @@ -51,18 +51,19 @@ ok # Lastly, flip tenant 10's capability for splits; ensure it can no longer issue # splits as a result. -upsert ten=10 can_admin_scatter=false can_admin_split=false can_admin_unsplit=false can_view_node_info=true can_view_tsdb_metrics=true +upsert ten=10 can_admin_scatter=true can_admin_split=false can_admin_unsplit=false can_view_node_info=true can_view_tsdb_metrics=true ---- ok -has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) ----- -client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) - has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) ---- client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) +# Does not affect admin scatters. +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) +---- +ok + has-capability-for-batch ten=10 cmds=(AdminUnsplit, Scan, ConditionalPut) ---- client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities new file mode 100644 index 000000000000..c6f26a187de2 --- /dev/null +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities @@ -0,0 +1,66 @@ +# Ensure if no capabilities have been set for the tenant, i.e the default +# capabilities exist, requests issued correctly conform to default checks. This +# entails allowing splits/scatters through but disallowing other requests. + +# Initialize an entry for this tenant with default values. +upsert ten=10 +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) +---- +ok + +# However, Unsplit requests aren't allowed. +has-capability-for-batch ten=10 cmds=(AdminUnsplit, Scan, ConditionalPut) +---- +client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) + +# However, if there was no unsplit in the batch, the batch should be allowed to +# go through. +has-capability-for-batch ten=10 cmds=(Scan, ConditionalPut) +---- +ok + +# Querying tsdb metrics isn't allowed either. +has-tsdb-query-capability ten=10 +---- +client tenant does not have capability to query timeseries data + +# and node status queries. +has-node-status-capability ten=10 +---- +client tenant does not have capability to query cluster node metadata + +# Update the capability state to give tenant 10 the capability to run unsplits. +upsert ten=10 can_admin_unsplit=true +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminUnsplit, Scan, ConditionalPut) +---- +ok + +# Remove the capability for the tenant entirely. +delete ten=10 +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminUnsplit, Scan, ConditionalPut) +---- +client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) + +# Tenants aren't allowed to perform splits if no capability entry exists for +# them in the Authorizer. +has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) +---- +client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) + +# ditto for scatters. +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) +---- +client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/no_capabilities b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/no_capabilities deleted file mode 100644 index 7a91899ab049..000000000000 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/no_capabilities +++ /dev/null @@ -1,30 +0,0 @@ -# Ensure if no capability exists for a tenant (or if an entry existed, but is -# deleted), split requests can't be issued. - -has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) ----- -client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) - -# However, if there was no split in the batch, the batch should be allowed to -# go through. -has-capability-for-batch ten=10 cmds=(Scan, ConditionalPut) ----- -ok - -# Update the capability state to give tenant 10 the capability to run splits. -upsert ten=10 can_admin_split=true ----- -ok - -has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) ----- -ok - -# Remove the capability. -delete ten=10 ----- -ok - -has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) ----- -client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/BUILD.bazel b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/BUILD.bazel index 0c76fdd39eb8..744fc08b9601 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/BUILD.bazel +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/BUILD.bazel @@ -45,7 +45,10 @@ go_test( srcs = ["capabilities_test.go"], args = ["-test.timeout=295s"], embed = [":tenantcapabilitiespb"], - deps = ["//pkg/multitenant/tenantcapabilities"], + deps = [ + "//pkg/multitenant/tenantcapabilities", + "@com_github_stretchr_testify//require", + ], ) get_x_data(name = "get_x_data") diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go index f33e0318ad46..c557fe9d4104 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go @@ -47,15 +47,37 @@ func (b boolCap) Set(val interface{}) { *b.cap = bval } -// For implements the tenantcapabilities.TenantCapabilities interface. +// invertedBoolCap is an accessor struct for boolean capabilities that are +// stored as "disabled" in the underlying proto. Layers above this package +// interact are oblivious to this detail. +type invertedBoolCap struct { + cap *bool +} + +// Get implements the tenantcapabilities.Capability interface. +func (i invertedBoolCap) Get() tenantcapabilities.Value { + val := *i.cap + return boolCapValue(!val) // inverted +} + +// Set implements the tenantcapabilities.Capability interface. +func (i invertedBoolCap) Set(val interface{}) { + bval, ok := val.(bool) + if !ok { + panic(errors.AssertionFailedf("invalid value type: %T", val)) + } + *i.cap = !bval +} + +// Cap implements the tenantcapabilities.TenantCapabilities interface. func (t *TenantCapabilities) Cap( capabilityID tenantcapabilities.CapabilityID, ) tenantcapabilities.Capability { switch capabilityID { case tenantcapabilities.CanAdminScatter: - return boolCap{&t.CanAdminScatter} + return invertedBoolCap{&t.DisableAdminScatter} case tenantcapabilities.CanAdminSplit: - return boolCap{&t.CanAdminSplit} + return invertedBoolCap{&t.DisableAdminSplit} case tenantcapabilities.CanAdminUnsplit: return boolCap{&t.CanAdminUnsplit} case tenantcapabilities.CanViewNodeInfo: @@ -68,13 +90,14 @@ func (t *TenantCapabilities) Cap( } } -// GetBool implements the tenantcapabilities.TenantCapabilities interface. It is an optimization. +// GetBool implements the tenantcapabilities.TenantCapabilities interface. +// It is an optimization. func (t *TenantCapabilities) GetBool(capabilityID tenantcapabilities.CapabilityID) bool { switch capabilityID { case tenantcapabilities.CanAdminScatter: - return t.CanAdminScatter + return !t.DisableAdminScatter case tenantcapabilities.CanAdminSplit: - return t.CanAdminSplit + return !t.DisableAdminSplit case tenantcapabilities.CanAdminUnsplit: return t.CanAdminUnsplit case tenantcapabilities.CanViewNodeInfo: diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto index 922e4373ffe9..b7c9bddf4f22 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto @@ -30,16 +30,30 @@ import "roachpb/span_config.proto"; message TenantCapabilities { option (gogoproto.equal) = true; - // CanAdminSplit if set to true, grants the tenant the ability to + // DisableAdminSplit, if set to true, revokes the tenants ability to // successfully perform `AdminSplit` requests. - bool can_admin_split = 1; + // + // This field uses the "disabled" verbiage, unlike the other fields on this + // proto. Doing so ensures the zero value translates to `AdminSplits` being + // allowed by default for secondary tenants. This is because splits and + // scatters are integral to the performance of IMPORT/RESTORE. + bool disable_admin_split = 1; - // CanViewNodeInfo if set to true, grants the tenant the ability + // DisableAdminScatter, if set to true, revokes a tenant's the ability to + // successfully perform `AdminScatter` requests. + // + // This field uses the "disabled" verbiage, unlike the other fields on this + // proto. Doing so ensures the zero value translates to `AdminScatters` being + // allowed by default for secondary tenants. This is because splits and + // scatters are integral to the performance of IMPORT/RESTORE. + bool disable_admin_scatter = 6; + + // CanViewNodeInfo, if set to true, grants the tenant the ability // retrieve node-level observability data at endpoints such as `_status/nodes` // and in the DB Console overview page. bool can_view_node_info = 2; - // CanViewTSDBMetrics if set to true, grants the tenant the ability to + // CanViewTSDBMetrics, if set to true, grants the tenant the ability to // make arbitrary queries of the TSDB of the entire cluster. Currently, // we do not store per-tenant metrics so this will surface system metrics // to the tenant. @@ -58,13 +72,11 @@ message TenantCapabilities { // positioned to surface errors to the user. SpanConfigBounds span_config_bounds = 4; - // CanAdminUnsplit if set to true, grants the tenant the ability to + // CanAdminUnsplit if, set to true, grants the tenant the ability to // successfully perform `AdminUnsplit` requests. bool can_admin_unsplit = 5; - // CanAdminScatter if set to true, grants the tenant the ability to - // successfully perform `AdminScatter` requests. - bool can_admin_scatter = 6; + // Next id: 7 }; // SpanConfigBound is used to constrain the possible values a SpanConfig may diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities_test.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities_test.go index 1e5f8b301c10..1d7f7918a1e0 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities_test.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" + "github.com/stretchr/testify/require" ) func TestCapabilityGetSet(t *testing.T) { @@ -22,5 +23,7 @@ func TestCapabilityGetSet(t *testing.T) { capability := capabilities.Cap(capID) value := capability.Get().Unwrap() capability.Set(value) + gotVal := capability.Get().Unwrap() + require.Equal(t, value, gotVal) } } diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/decoder_test.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/decoder_test.go index a7b22ef2d375..2a853fb0416a 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/decoder_test.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/decoder_test.go @@ -54,7 +54,7 @@ func TestDecodeCapabilities(t *testing.T) { require.NoError(t, err) info := mtinfopb.ProtoInfo{ Capabilities: tenantcapabilitiespb.TenantCapabilities{ - CanAdminSplit: true, + DisableAdminSplit: true, }, } buf, err := protoutil.Marshal(&info) diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/basic b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/basic index bde38ef1a181..b79ed22420a8 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/basic +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/basic @@ -7,37 +7,37 @@ ok updates ---- -upsert ten=10 can_admin_split=true +upsert ten=10 can_admin_unsplit=true ---- ok -upsert ten=11 can_admin_split=false +upsert ten=11 can_admin_unsplit=false ---- ok updates ---- Incremental Update -update: ten=10 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -update: ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=10 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} flush-state ---- -ten=10 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=10 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} +ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -upsert ten=11 can_admin_split=true +upsert ten=11 can_admin_unsplit=true ---- ok updates ---- Incremental Update -update: ten=11 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=11 ---- -{can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} delete ten=10 ---- @@ -64,24 +64,24 @@ updates # what we'd expect. flush-state ---- -ten=11 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} -upsert ten=15 can_admin_split=true +upsert ten=15 can_admin_unsplit=true ---- ok updates ---- Incremental Update -update: ten=15 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=15 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} # Ensure only the last update is applied, even when there are multiple updates # to a single key. -upsert ten=11 can_admin_split=false +upsert ten=11 can_admin_unsplit=false ---- ok -upsert ten=11 can_admin_split=true +upsert ten=11 can_admin_unsplit=true ---- ok @@ -100,30 +100,30 @@ not-found flush-state ---- -ten=15 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=15 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} # Same thing, but this time instead of deleting the key, leave it behind. delete ten=15 ---- ok -upsert ten=15 can_admin_split=true +upsert ten=15 can_admin_unsplit=true ---- ok -upsert ten=15 can_admin_split=false +upsert ten=15 can_admin_unsplit=false ---- ok updates ---- Incremental Update -update: ten=15 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=15 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} flush-state ---- -ten=15 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=15 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=15 ---- -{can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/initial_scan b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/initial_scan index a93b960655f7..6513342eb2e0 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/initial_scan +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/initial_scan @@ -2,15 +2,15 @@ # We also ensure that initial scans see the most recent state when they're # started. -upsert ten=10 can_admin_split=true +upsert ten=10 can_admin_unsplit=true ---- ok -upsert ten=11 can_admin_split=false +upsert ten=11 can_admin_unsplit=false ---- ok -upsert ten=15 can_admin_split=false +upsert ten=15 can_admin_unsplit=false ---- ok @@ -18,7 +18,7 @@ delete ten=10 ---- ok -upsert ten=15 can_admin_split=true +upsert ten=15 can_admin_unsplit=true ---- ok @@ -38,13 +38,13 @@ ok updates ---- Complete Update -update: ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -update: ten=15 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=15 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} flush-state ---- -ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -ten=15 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=15 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=10 ---- @@ -52,4 +52,4 @@ not-found get-capabilities ten=15 ---- -{can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/rangefeed_errors b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/rangefeed_errors index a6202e19299f..61292230937f 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/rangefeed_errors +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/rangefeed_errors @@ -10,24 +10,24 @@ ok updates ---- -upsert ten=10 can_admin_split=true +upsert ten=10 can_admin_unsplit=true ---- ok -upsert ten=11 can_admin_split=false +upsert ten=11 can_admin_unsplit=false ---- ok -upsert ten=12 can_admin_split=false +upsert ten=12 can_admin_unsplit=false ---- ok updates ---- Incremental Update -update: ten=10 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -update: ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -update: ten=12 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=10 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=12 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} inject-error ---- @@ -36,7 +36,7 @@ big-yikes # Update some more state. However, because of the injected error, we shouldn't # observe any updates. -upsert ten=12 can_admin_split=true +upsert ten=12 can_admin_unsplit=true ---- ok @@ -44,7 +44,7 @@ delete ten=10 ---- ok -upsert ten=50 can_admin_split=false +upsert ten=50 can_admin_unsplit=false ---- ok @@ -56,9 +56,9 @@ updates flush-state ---- -ten=10 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -ten=12 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=10 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} +ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=12 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=50 ---- @@ -66,11 +66,11 @@ not-found get-capabilities ten=12 ---- -{can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=10 ---- -{can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} # Let the Watcher attempt to restart. restart-after-injected-error @@ -82,23 +82,23 @@ ok updates ---- Complete Update -update: ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -update: ten=12 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -update: ten=50 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=12 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} +update: ten=50 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} flush-state ---- -ten=11 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -ten=12 cap={can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} -ten=50 cap={can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=11 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +ten=12 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} +ten=50 cap={can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=50 ---- -{can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=12 ---- -{can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false} +{can_admin_scatter:true can_admin_split:true can_admin_unsplit:true can_view_node_info:false can_view_tsdb_metrics:false} get-capabilities ten=10 ---- diff --git a/pkg/multitenant/tenantcapabilities/testingknobs.go b/pkg/multitenant/tenantcapabilities/testingknobs.go index eeae79e90950..ccbc709be49e 100644 --- a/pkg/multitenant/tenantcapabilities/testingknobs.go +++ b/pkg/multitenant/tenantcapabilities/testingknobs.go @@ -17,10 +17,6 @@ import "github.com/cockroachdb/cockroach/pkg/base" type TestingKnobs struct { // WatcherTestingKnobs can be used to test the tenant capabilities Watcher. WatcherTestingKnobs base.ModuleTestingKnobs - - // AuthorizerSkipAdminSplitCapabilityChecks, if set, skips capability checks - // for AdminSplit requests in the Authorizer for secondary tenants. - AuthorizerSkipAdminSplitCapabilityChecks bool } // ModuleTestingKnobs is part of the base.ModuleTestingKnobs interface. diff --git a/pkg/sql/logictest/BUILD.bazel b/pkg/sql/logictest/BUILD.bazel index 07ad2e873238..eb29971cc39a 100644 --- a/pkg/sql/logictest/BUILD.bazel +++ b/pkg/sql/logictest/BUILD.bazel @@ -35,7 +35,6 @@ go_library( "//pkg/kv/kvclient/rangefeed", "//pkg/kv/kvserver", "//pkg/kv/kvserver/kvserverbase", - "//pkg/multitenant/tenantcapabilities", "//pkg/roachpb", "//pkg/security/username", "//pkg/server", diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 31b2d83babc1..61b832bb7634 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -48,7 +48,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" - "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server" @@ -218,16 +217,6 @@ import ( // - tracing-off: If specified, tracing defaults to being turned off. This is // used to override the environment, which may ask for tracing to be on by // default. -// TODO(ecwall): We already have a tenant-capability-override-opt directive, -// and ideally, this option would be better suited in that category. However, -// that thing doesn't actually work. In particular, tenant capabilities are -// checked against an eventually consistent, in-memory state. However, that -// directive makes no attempt to ensure the in-memory state is sufficiently -// caught up. We should probably get rid of this cluster option once that -// directive is fixed. -// - can-admin-split: If specified, allows secondary tenants to perform -// AdminSplit operations regardless of the underlying tenant capabilities -// state. // // // ########################################### @@ -1938,22 +1927,6 @@ func (c clusterOptTracingOff) apply(args *base.TestServerArgs) { args.TracingDefault = tracing.TracingModeOnDemand } -// clusterOptAllowAdminSplitsForSecondaryTenants overrides can_admin_split capability -// checks using the AuthorizerOverrideCapabilities testing knob. -type clusterOptAllowAdminSplitsForSecondaryTenants struct{} - -// apply implements the knobOpt interface. -func (a clusterOptAllowAdminSplitsForSecondaryTenants) apply(args *base.TestServerArgs) { - _, ok := args.Knobs.TenantCapabilitiesTestingKnobs.(*tenantcapabilities.TestingKnobs) - if !ok { - args.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{} - } - args.Knobs.TenantCapabilitiesTestingKnobs.(*tenantcapabilities.TestingKnobs). - AuthorizerSkipAdminSplitCapabilityChecks = true -} - -var _ clusterOpt = clusterOptAllowAdminSplitsForSecondaryTenants{} - // knobOpt is implemented by options for configuring the testing knobs // for the cluster under which a test will run. type knobOpt interface { @@ -2136,8 +2109,6 @@ func readClusterOptions(t *testing.T, path string) []clusterOpt { res = append(res, clusterOptTracingOff{}) case "ignore-tenant-strict-gc-enforcement": res = append(res, clusterOptIgnoreStrictGCForTenants{}) - case "can-admin-split": - res = append(res, clusterOptAllowAdminSplitsForSecondaryTenants{}) default: t.Fatalf("unrecognized cluster option: %s", opt) } diff --git a/pkg/sql/logictest/testdata/logic_test/alter_primary_key b/pkg/sql/logictest/testdata/logic_test/alter_primary_key index 2f41fc17fced..9da29b62c285 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_primary_key +++ b/pkg/sql/logictest/testdata/logic_test/alter_primary_key @@ -1,4 +1,3 @@ -# cluster-opt: can-admin-split statement ok CREATE TABLE t (x INT PRIMARY KEY, y INT NOT NULL, z INT NOT NULL, w INT, INDEX i (x), INDEX i2 (z)) diff --git a/pkg/sql/logictest/testdata/logic_test/create_index b/pkg/sql/logictest/testdata/logic_test/create_index index 5f4e197f2596..bca10b313251 100644 --- a/pkg/sql/logictest/testdata/logic_test/create_index +++ b/pkg/sql/logictest/testdata/logic_test/create_index @@ -1,5 +1,4 @@ # LogicTest: default-configs local-mixed-22.2-23.1 -# cluster-opt: can-admin-split statement ok CREATE TABLE t ( diff --git a/pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality b/pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality index 552f90d6b688..4f21408ab4df 100644 --- a/pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality +++ b/pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality @@ -1,4 +1,3 @@ -# cluster-opt: can-admin-split # LogicTest: 3node-tenant-multiregion # tenant-cluster-setting-override-opt: sql.split_at.allow_for_secondary_tenant.enabled=true sql.zone_configs.allow_for_secondary_tenant.enabled=true sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true diff --git a/pkg/sql/logictest/testdata/logic_test/hash_sharded_index b/pkg/sql/logictest/testdata/logic_test/hash_sharded_index index 7419a82ef55e..87bbdff04373 100644 --- a/pkg/sql/logictest/testdata/logic_test/hash_sharded_index +++ b/pkg/sql/logictest/testdata/logic_test/hash_sharded_index @@ -1,4 +1,3 @@ -# cluster-opt: can-admin-split # Tests for creating a hash sharded primary key statement ok diff --git a/pkg/sql/logictest/testdata/logic_test/sql_keys b/pkg/sql/logictest/testdata/logic_test/sql_keys index f62399586de3..27772e93fc46 100644 --- a/pkg/sql/logictest/testdata/logic_test/sql_keys +++ b/pkg/sql/logictest/testdata/logic_test/sql_keys @@ -1,6 +1,5 @@ # LogicTest: local 3node-tenant # tenant-cluster-setting-override-opt: sql.split_at.allow_for_secondary_tenant.enabled=true -# cluster-opt: can-admin-split # This test depends on table ID's being stable, so add new tests at the bottom # of the file. diff --git a/pkg/sql/multitenant_admin_function_test.go b/pkg/sql/multitenant_admin_function_test.go index 68b2b7c49a8c..97e5b03be772 100644 --- a/pkg/sql/multitenant_admin_function_test.go +++ b/pkg/sql/multitenant_admin_function_test.go @@ -370,7 +370,9 @@ func (tc testCase) runTest( } execQueries(testCluster, systemDB, "system", tc.system) - execQueries(testCluster, secondaryDB, "secondary", tc.secondary) + if tc.secondary.isSet() { + execQueries(testCluster, secondaryDB, "secondary", tc.secondary) + } if tc.secondaryWithoutClusterSetting.isSet() { execQueries(testCluster, secondaryWithoutClusterSettingDB, "secondary_without_cluster_setting", tc.secondaryWithoutClusterSetting) } @@ -453,17 +455,10 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{"\xf0\x89\x89", "/1", maxTimestamp}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xfe\x92\xf0\x89\x89", "/104/1/1", maxTimestamp}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.split_at.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_split"`, - }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, - queryCapability: tenantcapabilities.CanAdminSplit, }, { desc: "ALTER INDEX x SPLIT AT", @@ -472,17 +467,10 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{"\xf0\x8a\x89", "/1", maxTimestamp}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xf0\x8a\x89", "/104/2/1", maxTimestamp}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.split_at.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_split"`, - }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, - queryCapability: tenantcapabilities.CanAdminSplit, }, { desc: "ALTER TABLE x UNSPLIT AT", @@ -498,7 +486,6 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, queryCapability: tenantcapabilities.CanAdminUnsplit, }, { @@ -518,7 +505,6 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, queryCapability: tenantcapabilities.CanAdminUnsplit, }, { @@ -535,7 +521,6 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, queryCapability: tenantcapabilities.CanAdminUnsplit, }, { @@ -555,7 +540,6 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, queryCapability: tenantcapabilities.CanAdminUnsplit, }, { @@ -564,17 +548,10 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{systemKey, systemKeyPretty}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xfe\x92", secondaryKeyPretty}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.scatter.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_scatter"`, - }, - queryClusterSetting: sql.SecondaryTenantScatterEnabled, - queryCapability: tenantcapabilities.CanAdminScatter, + queryCapability: tenantcapabilities.CanAdminScatter, }, { desc: "ALTER INDEX x SCATTER", @@ -583,17 +560,10 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{"\xf0\x8a", "/Table/104/2"}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xfe\x92", "/Tenant/10"}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.scatter.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_scatter"`, - }, queryClusterSetting: sql.SecondaryTenantScatterEnabled, - queryCapability: tenantcapabilities.CanAdminScatter, }, } @@ -641,14 +611,7 @@ func TestTruncateTable(t *testing.T) { {"…/104/2/1", ""}, }, }, - secondaryWithoutCapability: tenantExpected{ - // CanAdminScatter will default to true so this will open happen if it is - // set to false. - errorMessage: `does not have capability "can_admin_scatter"`, - }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, - queryCapability: tenantcapabilities.CanAdminScatter, } tc.runTest( t, diff --git a/pkg/sql/opt/exec/execbuilder/testdata/distsql_tenant_locality b/pkg/sql/opt/exec/execbuilder/testdata/distsql_tenant_locality index 2c3e41a99ee6..fc9562b7a23d 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/distsql_tenant_locality +++ b/pkg/sql/opt/exec/execbuilder/testdata/distsql_tenant_locality @@ -1,6 +1,5 @@ # LogicTest: 3node-tenant-multiregion # tenant-cluster-setting-override-opt: sql.split_at.allow_for_secondary_tenant.enabled=true sql.zone_configs.allow_for_secondary_tenant.enabled=true sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true -# cluster-opt: can-admin-split # Create a table on the secondary tenant. statement ok