From 57dd19c2a16e4f00be56108e17cd7fe787293ae0 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Mon, 13 Mar 2023 19:34:21 -0400 Subject: [PATCH] multitenant: allow secondary tenants to split/scatter by default AdminSplit and AdminScatter requests are subject to capability checks. Previously, these capabilities were codified in the "enabled" form. As such, by default, secondary tenants did not have the ability to perform these operations. This is in violation of what secondary tenants could do prior to 23.1, at a time before capabilities existed. Moreover, RESTORE/IMPORT rely on performing these operations for performance. This made disallowing these operations by default a performance regression. This patch flips the phrasing of how these capabilities are stored on the proto to use the "disable" verbiage. As such, secondary tenants are able to perform splits and scatters by default. As part of this change, we also clean up a testing knob that was used by various backup, CDC, and logictests to override capability checks in the authorizer. This isn't required with the new default behaviour. We also add some missing E2E tests for the `CanAdminUnsplit` capability which were missing when it was introduced. Fixes #96736 Release note: None --- pkg/ccl/backupccl/BUILD.bazel | 1 - pkg/ccl/backupccl/backup_test.go | 15 ----- pkg/ccl/backupccl/datadriven_test.go | 9 --- pkg/ccl/backupccl/utils_test.go | 28 --------- .../testdata/logic_test/multi_region_backup | 1 - .../partitioning_hash_sharded_index_mr | 1 - ...partitioning_hash_sharded_index_query_plan | 1 - .../regional_by_row_hash_sharded_index | 1 - .../testdata/can_admin_split | 19 ++---- .../testdata/can_admin_unsplit | 34 +++++++++++ pkg/ccl/streamingccl/streamingest/BUILD.bazel | 1 - .../replication_stream_e2e_test.go | 4 -- .../tenantcapabilities/capabilities.go | 23 ++++---- .../tenantcapabilities/capabilityid_string.go | 8 +-- .../authorizer.go | 47 ++++++++++----- .../testdata/authorizer_enabled | 6 +- .../testdata/basic | 23 ++++---- .../testdata/default_capabilities | 58 +++++++++++++++++++ .../testdata/no_capabilities | 30 ---------- .../testdata/system_tenant | 2 +- .../tenantcapabilitiespb/capabilities.go | 21 +++---- .../tenantcapabilitiespb/capabilities.proto | 14 ++--- .../tenantcapabilitieswatcher/decoder_test.go | 2 +- .../tenantcapabilitieswatcher/testdata/basic | 40 ++++++------- .../testdata/initial_scan | 18 +++--- .../testdata/rangefeed_errors | 42 +++++++------- .../tenantcapabilities/testingknobs.go | 4 -- pkg/sql/logictest/BUILD.bazel | 1 - pkg/sql/logictest/logic.go | 29 ---------- .../testdata/logic_test/alter_primary_key | 1 - .../testdata/logic_test/create_index | 1 - .../logic_test/distsql_tenant_locality | 1 - .../testdata/logic_test/hash_sharded_index | 1 - .../logictest/testdata/logic_test/sql_keys | 1 - pkg/sql/multitenant_admin_function_test.go | 35 ++++------- .../testdata/distsql_tenant_locality | 1 - 36 files changed, 241 insertions(+), 283 deletions(-) create mode 100644 pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit create mode 100644 pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities delete mode 100644 pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/no_capabilities 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/testdata/can_admin_split b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split index b670f661bd69..d6dbfba2aee3 100644 --- a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split @@ -1,34 +1,25 @@ query-sql-system -SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_split' +SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'disable_admin_split' ---- -10 tenant-10 ready none can_admin_split false +10 tenant-10 ready none disable_admin_split false exec-sql-tenant CREATE TABLE t(a INT) ---- 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) - -update-capabilities -ALTER TENANT [10] GRANT CAPABILITY can_admin_split=true ----- -ok - +# By default, we should be able to split. exec-privileged-op-tenant ALTER TABLE t SPLIT AT VALUES (0) ---- ok update-capabilities -ALTER TENANT [10] REVOKE CAPABILITY can_admin_split +ALTER TENANT [10] GRANT CAPABILITY disable_admin_split=true ---- 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) +pq: ba: AdminSplit [/Tenant/10/Table/104/1/0,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant capability "disable_admin_split" prevents operation (*kvpb.AdminSplitRequest) 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..4af170d67394 100644 --- a/pkg/multitenant/tenantcapabilities/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/capabilities.go @@ -162,17 +162,16 @@ 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 // can_admin_scatter + // DisableAdminScatter disallows a secondary tenant from being able 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. + DisableAdminScatter // disable_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 // can_admin_split + // DisableAdminSplit disallows a secondary tenant from being able 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. + DisableAdminSplit // disable_admin_split // CanAdminUnsplit describes the ability of a tenant to perform manual // KV range unsplit requests. These operations need a capability @@ -230,8 +229,8 @@ const ( func (c CapabilityID) CapabilityType() Type { switch c { case - CanAdminScatter, - CanAdminSplit, + DisableAdminScatter, + DisableAdminSplit, CanAdminUnsplit, CanViewNodeInfo, CanViewTSDBMetrics: diff --git a/pkg/multitenant/tenantcapabilities/capabilityid_string.go b/pkg/multitenant/tenantcapabilities/capabilityid_string.go index f4dd2d5076b4..e7264ca8b0a4 100644 --- a/pkg/multitenant/tenantcapabilities/capabilityid_string.go +++ b/pkg/multitenant/tenantcapabilities/capabilityid_string.go @@ -8,8 +8,8 @@ func _() { // An "invalid array index" compiler error signifies that the constant values have changed. // Re-run the stringer command to generate them again. var x [1]struct{} - _ = x[CanAdminScatter-1] - _ = x[CanAdminSplit-2] + _ = x[DisableAdminScatter-1] + _ = x[DisableAdminSplit-2] _ = x[CanAdminUnsplit-3] _ = x[CanViewNodeInfo-4] _ = x[CanViewTSDBMetrics-5] @@ -17,9 +17,9 @@ func _() { _ = x[MaxCapabilityID-6] } -const _CapabilityID_name = "can_admin_scattercan_admin_splitcan_admin_unsplitcan_view_node_infocan_view_tsdb_metricsspan_config_bounds" +const _CapabilityID_name = "disable_admin_scatterdisable_admin_splitcan_admin_unsplitcan_view_node_infocan_view_tsdb_metricsspan_config_bounds" -var _CapabilityID_index = [...]uint8{0, 17, 32, 49, 67, 88, 106} +var _CapabilityID_index = [...]uint8{0, 21, 40, 57, 75, 96, 114} func (i CapabilityID) String() string { i -= 1 diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go index d38b6d71f81a..20be86a3fe63 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go @@ -78,22 +78,39 @@ func (a *Authorizer) HasCapabilityForBatch( for _, ru := range ba.Requests { request := ru.GetInner() - requiredCap, hasCap := reqMethodToCap[request.Method()] - if requiredCap == noCapCheckNeeded { + capability, hasCap := reqMethodToCap[request.Method()] + if capability == noCapCheckNeeded { continue } - if !hasCap || requiredCap == onlySystemTenant || !found || !cp.GetBool(requiredCap) { - if (requiredCap == tenantcapabilities.CanAdminSplit || requiredCap == tenantcapabilities.CanAdminScatter) && - a.knobs.AuthorizerSkipAdminSplitCapabilityChecks { - continue + switch request.Method() { + case kvpb.AdminSplit, kvpb.AdminScatter: + // AdminSplit and AdminScatter requests must are codified as + // "disable_admin_{split,scatter}" in the capabilities proto, as tenants + // have the ability to perform splits and scatters by default. This is + // because things like IMPORT/RESTORE rely on these operations for + // performance. As such, they must be explicitly revoked by the operator. + // This is in contrast to other capabilities that require authorization, + // which are explicitly granted. + if !found { + return nil // allowed by default + } + if disabled := cp.GetBool(capability); disabled { + return errors.Newf( + "client tenant capability %q prevents operation (%T)", capability, request, + ) + } + default: + // All other requests that require capabilities are expressed in their + // "enabled" form. + if !hasCap || capability == onlySystemTenant || !found || !cp.GetBool(capability) { + // 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)", capability, request) } - // 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) } } return nil @@ -131,8 +148,8 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.CapabilityID{ kvpb.Scan: noCapCheckNeeded, // The following are authorized via specific capabilities. - kvpb.AdminScatter: tenantcapabilities.CanAdminScatter, - kvpb.AdminSplit: tenantcapabilities.CanAdminSplit, + kvpb.AdminScatter: tenantcapabilities.DisableAdminScatter, + kvpb.AdminSplit: tenantcapabilities.DisableAdminSplit, kvpb.AdminUnsplit: tenantcapabilities.CanAdminUnsplit, // TODO(ecwall): The following should also be authorized via specific capabilities. diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled index c22bf964834f..c1503f9d3ac6 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled @@ -1,10 +1,10 @@ -upsert ten=10 can_admin_scatter=false can_admin_split=false can_view_node_info=false can_view_tsdb_metrics=false +upsert ten=10 disable_admin_scatter=true disable_admin_split=true can_view_node_info=false can_view_tsdb_metrics=false ---- ok has-capability-for-batch ten=10 cmds=(AdminScatter, Scan) ---- -client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) +client tenant capability "disable_admin_scatter" prevents operation (*kvpb.AdminScatterRequest) has-node-status-capability ten=10 ---- @@ -42,7 +42,7 @@ ok has-capability-for-batch ten=10 cmds=(AdminScatter, Scan) ---- -client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) +client tenant capability "disable_admin_scatter" prevents operation (*kvpb.AdminScatterRequest) has-node-status-capability ten=10 ---- diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic index 553c6e3110ba..da7062c6923e 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic @@ -1,8 +1,8 @@ -upsert ten=10 can_admin_scatter=true can_admin_split=true can_admin_unsplit=true can_view_node_info=true can_view_tsdb_metrics=true +upsert ten=10 disable_admin_scatter=false disable_admin_split=false can_admin_unsplit=true can_view_node_info=true can_view_tsdb_metrics=true ---- ok -upsert ten=11 can_admin_scatter=false can_admin_split=false can_admin_unsplit=false can_view_node_info=false can_view_tsdb_metrics=false +upsert ten=11 disable_admin_scatter=true disable_admin_split=true can_admin_unsplit=false can_view_node_info=false can_view_tsdb_metrics=false ---- ok @@ -22,21 +22,21 @@ ok has-capability-for-batch ten=11 cmds=(AdminScatter, Scan, ConditionalPut) ---- -client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) +client tenant capability "disable_admin_scatter" prevents operation (*kvpb.AdminScatterRequest) # Tenant 11 shouldn't be able to issue splits. has-capability-for-batch ten=11 cmds=(AdminSplit, Scan, ConditionalPut) ---- -client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) +client tenant capability "disable_admin_split" prevents operation (*kvpb.AdminSplitRequest) 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 capability "disable_admin_split" prevents operation (*kvpb.AdminSplitRequest) # However, a batch request which doesn't include a split (by tenant 11) should # work as you'd expect. @@ -51,17 +51,18 @@ 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 disable_admin_scatter=false disable_admin_split=true 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) +has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) ---- -client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) +client tenant capability "disable_admin_split" prevents operation (*kvpb.AdminSplitRequest) -has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) +# Does not affect admin scatters. +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) ---- -client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) +ok has-capability-for-batch ten=10 cmds=(AdminUnsplit, Scan, ConditionalPut) ---- diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities new file mode 100644 index 000000000000..1ebc4f8a4f2a --- /dev/null +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/default_capabilities @@ -0,0 +1,58 @@ +# 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. + +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) + +has-capability-for-batch ten=10 cmds=(AdminSplit, Scan, ConditionalPut) +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut) +---- +ok 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/tenantcapabilitiesauthorizer/testdata/system_tenant b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/system_tenant index 2f7fd9b9eeca..8d09a630ea40 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/system_tenant +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/system_tenant @@ -12,7 +12,7 @@ has-capability-for-batch ten=system cmds=(AdminSplit) ---- ok -upsert ten=system can_admin_split=false +upsert ten=system disable_admin_split=false ---- ok diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go index f33e0318ad46..bbc58750dab9 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go @@ -47,15 +47,15 @@ func (b boolCap) Set(val interface{}) { *b.cap = bval } -// For implements the tenantcapabilities.TenantCapabilities interface. +// Cap implements the tenantcapabilities.TenantCapabilities interface. func (t *TenantCapabilities) Cap( capabilityID tenantcapabilities.CapabilityID, ) tenantcapabilities.Capability { switch capabilityID { - case tenantcapabilities.CanAdminScatter: - return boolCap{&t.CanAdminScatter} - case tenantcapabilities.CanAdminSplit: - return boolCap{&t.CanAdminSplit} + case tenantcapabilities.DisableAdminScatter: + return boolCap{&t.DisableAdminScatter} + case tenantcapabilities.DisableAdminSplit: + return boolCap{&t.DisableAdminSplit} case tenantcapabilities.CanAdminUnsplit: return boolCap{&t.CanAdminUnsplit} case tenantcapabilities.CanViewNodeInfo: @@ -68,13 +68,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 - case tenantcapabilities.CanAdminSplit: - return t.CanAdminSplit + case tenantcapabilities.DisableAdminScatter: + return t.DisableAdminScatter + case tenantcapabilities.DisableAdminSplit: + 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..9bcc01caea76 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto @@ -30,16 +30,16 @@ 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; + bool disable_admin_split = 1; - // CanViewNodeInfo if set to true, grants the tenant the ability + // 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 +58,13 @@ 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 + // DisableAdminScatter, if set to true, revokes a tenant's the ability to // successfully perform `AdminScatter` requests. - bool can_admin_scatter = 6; + bool disable_admin_scatter = 6; }; // SpanConfigBound is used to constrain the possible values a SpanConfig may 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..f622796f0794 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 disable_admin_split=true ---- ok -upsert ten=11 can_admin_split=false +upsert ten=11 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} +update: ten=11 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} +ten=11 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} -upsert ten=11 can_admin_split=true +upsert ten=11 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} 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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} 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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} -upsert ten=15 can_admin_split=true +upsert ten=15 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} # 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 disable_admin_split=false ---- ok -upsert ten=11 can_admin_split=true +upsert ten=11 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} # 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 disable_admin_split=true ---- ok -upsert ten=15 can_admin_split=false +upsert ten=15 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/initial_scan b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/initial_scan index a93b960655f7..87a82668ec66 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 disable_admin_split=true ---- ok -upsert ten=11 can_admin_split=false +upsert ten=11 disable_admin_split=false ---- ok -upsert ten=15 can_admin_split=false +upsert ten=15 disable_admin_split=false ---- ok @@ -18,7 +18,7 @@ delete ten=10 ---- ok -upsert ten=15 can_admin_split=true +upsert ten=15 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} +update: ten=15 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} 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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} +ten=15 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} 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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/rangefeed_errors b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/rangefeed_errors index a6202e19299f..2092960ef8f0 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 disable_admin_split=true ---- ok -upsert ten=11 can_admin_split=false +upsert ten=11 disable_admin_split=false ---- ok -upsert ten=12 can_admin_split=false +upsert ten=12 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} +update: ten=11 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} +update: ten=12 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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 disable_admin_split=true ---- ok @@ -44,7 +44,7 @@ delete ten=10 ---- ok -upsert ten=50 can_admin_split=false +upsert ten=50 disable_admin_split=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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} +ten=11 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} +ten=12 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} # 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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} +update: ten=12 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} +update: ten=50 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:false} +ten=12 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} +ten=50 cap={can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split: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_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false disable_admin_scatter:false disable_admin_split:true} 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..0e5839fe4f4a 100644 --- a/pkg/sql/multitenant_admin_function_test.go +++ b/pkg/sql/multitenant_admin_function_test.go @@ -454,16 +454,16 @@ func TestMultiTenantAdminFunction(t *testing.T) { result: [][]string{{"\xf0\x89\x89", "/1", maxTimestamp}}, }, secondary: tenantExpected{ - result: [][]string{{"\xfe\x92\xf0\x89\x89", "/104/1/1", maxTimestamp}}, + errorMessage: `capability "disable_admin_split" prevents operation`, }, 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"`, + result: [][]string{{"\xfe\x9c\xf0\x89\x89", "/104/1/1", maxTimestamp}}, }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, - queryCapability: tenantcapabilities.CanAdminSplit, + queryCapability: tenantcapabilities.DisableAdminSplit, }, { desc: "ALTER INDEX x SPLIT AT", @@ -473,16 +473,16 @@ func TestMultiTenantAdminFunction(t *testing.T) { result: [][]string{{"\xf0\x8a\x89", "/1", maxTimestamp}}, }, secondary: tenantExpected{ - result: [][]string{{"\xf0\x8a\x89", "/104/2/1", maxTimestamp}}, + errorMessage: `capability "disable_admin_split" prevents operation`, }, 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"`, + result: [][]string{{"\xf0\x8a\x89", "/104/2/1", maxTimestamp}}, }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, - queryCapability: tenantcapabilities.CanAdminSplit, + queryCapability: tenantcapabilities.DisableAdminSplit, }, { desc: "ALTER TABLE x UNSPLIT AT", @@ -498,7 +498,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 +517,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 +533,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 +552,6 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, queryCapability: tenantcapabilities.CanAdminUnsplit, }, { @@ -565,16 +561,16 @@ func TestMultiTenantAdminFunction(t *testing.T) { result: [][]string{{systemKey, systemKeyPretty}}, }, secondary: tenantExpected{ - result: [][]string{{"\xfe\x92", secondaryKeyPretty}}, + errorMessage: `capability "disable_admin_scatter" prevents operation`, }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.scatter.allow_for_secondary_tenant.enabled disabled", }, secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_scatter"`, + result: [][]string{{"\xfe\x9c", "/Tenant/20"}}, }, queryClusterSetting: sql.SecondaryTenantScatterEnabled, - queryCapability: tenantcapabilities.CanAdminScatter, + queryCapability: tenantcapabilities.DisableAdminScatter, }, { desc: "ALTER INDEX x SCATTER", @@ -584,16 +580,16 @@ func TestMultiTenantAdminFunction(t *testing.T) { result: [][]string{{"\xf0\x8a", "/Table/104/2"}}, }, secondary: tenantExpected{ - result: [][]string{{"\xfe\x92", "/Tenant/10"}}, + errorMessage: `capability "disable_admin_scatter" prevents operation`, }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.scatter.allow_for_secondary_tenant.enabled disabled", }, secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_scatter"`, + result: [][]string{{"\xfe\x9c", "/Tenant/20"}}, }, queryClusterSetting: sql.SecondaryTenantScatterEnabled, - queryCapability: tenantcapabilities.CanAdminScatter, + queryCapability: tenantcapabilities.DisableAdminScatter, }, } @@ -641,14 +637,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