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/crdb_internal b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal index b0d538692a62..a0cc0dc81302 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal @@ -194,8 +194,7 @@ query ITT colnames,retry SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_id = 'can_admin_split' ---- tenant_id capability_id capability_value -1 can_admin_split false -5 can_admin_split false +1 can_admin_split true statement ok ALTER TENANT [5] GRANT CAPABILITY can_admin_split @@ -205,7 +204,7 @@ query ITT colnames,retry SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_id = 'can_admin_split' ---- tenant_id capability_id capability_value -1 can_admin_split false +1 can_admin_split true 5 can_admin_split true subtest end 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/logictestccl/testdata/logic_test/tenant_capability b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability index 093a37aa0f61..83b9b1893627 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability +++ b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability @@ -31,8 +31,8 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "no-capabilities-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false -can_admin_split false +can_admin_scatter true +can_admin_split true can_admin_unsplit false can_view_node_info false can_view_tsdb_metrics false @@ -51,7 +51,7 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-no-value-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false +can_admin_scatter true can_admin_split true can_admin_unsplit false can_view_node_info false @@ -64,7 +64,7 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-no-value-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false +can_admin_scatter true can_admin_split false can_admin_unsplit false can_view_node_info false @@ -84,7 +84,7 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-with-value-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false +can_admin_scatter true can_admin_split true can_admin_unsplit false can_view_node_info false @@ -104,7 +104,7 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-with-expression-value-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false +can_admin_scatter true can_admin_split true can_admin_unsplit false can_view_node_info false @@ -124,7 +124,7 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false +can_admin_scatter true can_admin_split true can_admin_unsplit false can_view_node_info true @@ -137,7 +137,7 @@ query TT colnames SELECT capability_id, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES] ---- capability_id capability_value -can_admin_scatter false +can_admin_scatter true can_admin_split false can_admin_unsplit false can_view_node_info false 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..d04a6ceb7587 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go @@ -82,23 +82,46 @@ func (a *Authorizer) HasCapabilityForBatch( if requiredCap == noCapCheckNeeded { continue } - if !hasCap || requiredCap == onlySystemTenant || !found || !cp.GetBool(requiredCap) { - if (requiredCap == tenantcapabilities.CanAdminSplit || requiredCap == tenantcapabilities.CanAdminScatter) && - a.knobs.AuthorizerSkipAdminSplitCapabilityChecks { + if !found { + switch request.Method() { + case kvpb.AdminSplit, kvpb.AdminScatter: + // Secondary tenants are allowed to run AdminSplit and AdminScatter + // requests by default, as they're integral to the performance of IMPORT + // and RESTORE. If no entry is found in the capabilities map, we + // fallback to this default behavior. Note that this isn't expected to + // be the case during normal operation, as tenants that exist should + // always have an entry in this map. It does help for some tests, + // however. continue + default: + // For all other requests we conservatively return an error if no entry + // is to be found for the tenant. + return newTenantDoesNotHaveCapabilityError(requiredCap, request) } + } + if !hasCap || requiredCap == onlySystemTenant || !cp.GetBool(requiredCap) { // 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. - return errors.Newf("client tenant does not have capability %q (%T)", requiredCap, request) + // + // 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 newTenantDoesNotHaveCapabilityError(requiredCap, request) } } return nil } +func newTenantDoesNotHaveCapabilityError( + cap tenantcapabilities.CapabilityID, req kvpb.Request, +) error { + return errors.Newf("client tenant does not have capability %q (%T)", cap, req) +} + var reqMethodToCap = map[kvpb.Method]tenantcapabilities.CapabilityID{ // The following requests are authorized for all workloads. kvpb.AddSSTable: noCapCheckNeeded, 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..5aa70a237223 --- /dev/null +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities @@ -0,0 +1,71 @@ +# 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 are allowed to perform splits if no entry exists for them in the +# Authorizer. +has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) +---- +ok + +# ditto for scatters. +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) +---- +ok + +# However, a batch with both unsplit and split should return an appropriate error. +has-capability-for-batch ten=10 cmds=(AdminSplit, AdminUnsplit, Scan, ConditionalPut) +---- +client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) 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 b5989d0165c5..48114990334c 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. // // // ########################################### @@ -1947,22 +1936,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 { @@ -2145,8 +2118,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/logictest/testdata/logic_test/tenant_builtins b/pkg/sql/logictest/testdata/logic_test/tenant_builtins index 2cb686aee22a..d477192f5f30 100644 --- a/pkg/sql/logictest/testdata/logic_test/tenant_builtins +++ b/pkg/sql/logictest/testdata/logic_test/tenant_builtins @@ -47,10 +47,10 @@ FROM system.tenants ORDER BY id ---- id active name data_state service_mode crdb_internal.pb_to_json -1 true system 1 2 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} -2 true tenant-number-eleven 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} -5 true tenant-5 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "5", "droppedName": "", "tenantReplicationJobId": "0"} -10 true tenant-number-ten 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "10", "droppedName": "", "tenantReplicationJobId": "0"} +1 true system 1 2 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} +2 true tenant-number-eleven 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} +5 true tenant-5 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "5", "droppedName": "", "tenantReplicationJobId": "0"} +10 true tenant-number-ten 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "10", "droppedName": "", "tenantReplicationJobId": "0"} # Check we can add a name where none existed before. statement ok @@ -118,10 +118,10 @@ FROM system.tenants ORDER BY id ---- id active name data_state service_mode crdb_internal.pb_to_json -1 true system 1 2 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} -2 true tenant-number-eleven 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} -5 false NULL 2 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "my-new-tenant-name", "tenantReplicationJobId": "0"} -10 true tenant-number-ten 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "10", "droppedName": "", "tenantReplicationJobId": "0"} +1 true system 1 2 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} +2 true tenant-number-eleven 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} +5 false NULL 2 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "my-new-tenant-name", "tenantReplicationJobId": "0"} +10 true tenant-number-ten 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "10", "droppedName": "", "tenantReplicationJobId": "0"} # Try to recreate an existing tenant. @@ -229,9 +229,9 @@ FROM system.tenants ORDER BY id ---- id active name data_state service_mode crdb_internal.pb_to_json -1 true system 1 2 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} -2 true tenant-number-eleven 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} -10 true tenant-number-ten 1 0 {"capabilities": {"canAdminScatter": false, "canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "10", "droppedName": "", "tenantReplicationJobId": "0"} +1 true system 1 2 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} +2 true tenant-number-eleven 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} +10 true tenant-number-ten 1 0 {"capabilities": {"canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "disableAdminScatter": false, "disableAdminSplit": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "10", "droppedName": "", "tenantReplicationJobId": "0"} query error tenant resource limits require a CCL binary SELECT crdb_internal.update_tenant_resource_limits(10, 1000, 100, 0, now(), 0) 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