From bacfb0f47214c8e5f48da330fab42a494b5a73ae Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 14 Mar 2023 18:29:15 +0100 Subject: [PATCH 1/3] sql: update TestMultiTenantAdminFunction to not assume a default cap state Release note: None Co-authored-by: Evan Wall --- pkg/sql/multitenant_admin_function_test.go | 77 +++++++++++-------- .../serverutils/test_cluster_shim.go | 2 +- pkg/testutils/testcluster/testcluster.go | 16 ++-- 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/pkg/sql/multitenant_admin_function_test.go b/pkg/sql/multitenant_admin_function_test.go index 57edda3b02bd..cd9b1d4c8f37 100644 --- a/pkg/sql/multitenant_admin_function_test.go +++ b/pkg/sql/multitenant_admin_function_test.go @@ -238,9 +238,14 @@ type testCase struct { queryClusterSetting *settings.BoolSetting // Used for tests that have a capability prereq // (eq SPLIT AT is required for UNSPLIT AT). - setupCapability tenantcapabilities.CapabilityID + setupCapability capValue // Capability required for secondary tenant query. - queryCapability tenantcapabilities.CapabilityID + queryCapability capValue +} + +type capValue struct { + cap tenantcapabilities.CapabilityID + value string } func (tc testCase) runTest( @@ -303,32 +308,30 @@ func (tc testCase) runTest( var waitForTenantCapabilitiesFns []func() setCapabilities := func( tenantID roachpb.TenantID, - capabilityIDs ...tenantcapabilities.CapabilityID, + capVals ...capValue, ) { // Filter out empty capabilities. - var caps []tenantcapabilities.CapabilityID - for _, capabilityID := range capabilityIDs { - if capabilityID == 0 { + var caps []capValue + for _, capVal := range capVals { + if capVal.cap == 0 { // This can happen if e.g. setupCapability / queryCapability // are unset in a test. continue } - caps = append(caps, capabilityID) + caps = append(caps, capVal) } - capabilityIDs = caps - if len(capabilityIDs) > 0 { - var builder strings.Builder - for i, capabilityID := range capabilityIDs { - if i > 0 { - builder.WriteString(", ") - } - builder.WriteString(capabilityID.String()) + capVals = caps + if len(capVals) > 0 { + expected := map[tenantcapabilities.CapabilityID]string{} + for _, capVal := range capVals { + query := fmt.Sprintf("ALTER TENANT [$1] GRANT CAPABILITY %s = %s", capVal.cap.String(), capVal.value) + _, err := systemDB.ExecContext(ctx, query, tenantID.ToUint64()) + require.NoError(t, err, query) + expected[capVal.cap] = capVal.value } - query := fmt.Sprintf("ALTER TENANT [$1] GRANT CAPABILITY %s", builder.String()) - _, err := systemDB.ExecContext(ctx, query, tenantID.ToUint64()) - require.NoError(t, err, query) + waitForTenantCapabilitiesFns = append(waitForTenantCapabilitiesFns, func() { - testCluster.WaitForTenantCapabilities(t, tenantID, capabilityIDs...) + testCluster.WaitForTenantCapabilities(t, tenantID, expected) }) } } @@ -403,6 +406,10 @@ func (te tenantExpected) validate(t *testing.T, rows *gosql.Rows, err error, mes } } +func bcap(cap tenantcapabilities.CapabilityID, val bool) capValue { + return capValue{cap: cap, value: fmt.Sprint(val)} +} + // TestMultiTenantAdminFunction tests the "simple" admin functions that do not require complex setup. func TestMultiTenantAdminFunction(t *testing.T) { defer leaktest.AfterTest(t)() @@ -464,7 +471,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_split"`, }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, - queryCapability: tenantcapabilities.CanAdminSplit, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, false), + queryCapability: bcap(tenantcapabilities.CanAdminSplit, true), }, { desc: "ALTER INDEX x SPLIT AT", @@ -483,7 +491,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_split"`, }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, - queryCapability: tenantcapabilities.CanAdminSplit, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, false), + queryCapability: bcap(tenantcapabilities.CanAdminSplit, true), }, { desc: "ALTER TABLE x UNSPLIT AT", @@ -499,8 +508,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, - queryCapability: tenantcapabilities.CanAdminUnsplit, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, true), + queryCapability: bcap(tenantcapabilities.CanAdminUnsplit, true), }, { desc: "ALTER INDEX x UNSPLIT AT", @@ -519,8 +528,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, - queryCapability: tenantcapabilities.CanAdminUnsplit, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, true), + queryCapability: bcap(tenantcapabilities.CanAdminUnsplit, true), }, { desc: "ALTER TABLE x UNSPLIT ALL", @@ -536,8 +545,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, - queryCapability: tenantcapabilities.CanAdminUnsplit, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, true), + queryCapability: bcap(tenantcapabilities.CanAdminUnsplit, true), }, { desc: "ALTER INDEX x UNSPLIT ALL", @@ -556,8 +565,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_unsplit"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, - queryCapability: tenantcapabilities.CanAdminUnsplit, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, true), + queryCapability: bcap(tenantcapabilities.CanAdminUnsplit, true), }, { desc: "ALTER TABLE x SCATTER", @@ -575,7 +584,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_scatter"`, }, queryClusterSetting: sql.SecondaryTenantScatterEnabled, - queryCapability: tenantcapabilities.CanAdminScatter, + setupCapability: bcap(tenantcapabilities.CanAdminScatter, false), + queryCapability: bcap(tenantcapabilities.CanAdminScatter, true), }, { desc: "ALTER INDEX x SCATTER", @@ -594,7 +604,8 @@ func TestMultiTenantAdminFunction(t *testing.T) { errorMessage: `does not have capability "can_admin_scatter"`, }, queryClusterSetting: sql.SecondaryTenantScatterEnabled, - queryCapability: tenantcapabilities.CanAdminScatter, + setupCapability: bcap(tenantcapabilities.CanAdminScatter, false), + queryCapability: bcap(tenantcapabilities.CanAdminScatter, true), }, } @@ -648,8 +659,8 @@ func TestTruncateTable(t *testing.T) { errorMessage: `does not have capability "can_admin_scatter"`, }, setupClusterSetting: sql.SecondaryTenantSplitAtEnabled, - setupCapability: tenantcapabilities.CanAdminSplit, - queryCapability: tenantcapabilities.CanAdminScatter, + setupCapability: bcap(tenantcapabilities.CanAdminSplit, true), + queryCapability: bcap(tenantcapabilities.CanAdminScatter, true), } tc.runTest( t, diff --git a/pkg/testutils/serverutils/test_cluster_shim.go b/pkg/testutils/serverutils/test_cluster_shim.go index 3583ecbef020..bf2c741ee48a 100644 --- a/pkg/testutils/serverutils/test_cluster_shim.go +++ b/pkg/testutils/serverutils/test_cluster_shim.go @@ -236,7 +236,7 @@ type TestClusterInterface interface { // tenant capabilities for the specified tenant ID. // Only boolean capabilities are currently supported as we wait for the // specified capabilities to have a "true" value. - WaitForTenantCapabilities(*testing.T, roachpb.TenantID, ...tenantcapabilities.CapabilityID) + WaitForTenantCapabilities(*testing.T, roachpb.TenantID, map[tenantcapabilities.CapabilityID]string) } // SplitPoint describes a split point that is passed to SplitTable. diff --git a/pkg/testutils/testcluster/testcluster.go b/pkg/testutils/testcluster/testcluster.go index dcbe14aca1ae..e93191328a2c 100644 --- a/pkg/testutils/testcluster/testcluster.go +++ b/pkg/testutils/testcluster/testcluster.go @@ -1778,7 +1778,7 @@ func (tc *TestCluster) SplitTable( // WaitForTenantCapabilities implements TestClusterInterface. func (tc *TestCluster) WaitForTenantCapabilities( - t *testing.T, tenID roachpb.TenantID, capIDs ...tenantcapabilities.CapabilityID, + t *testing.T, tenID roachpb.TenantID, targetCaps map[tenantcapabilities.CapabilityID]string, ) { for i, ts := range tc.Servers { testutils.SucceedsSoon(t, func() error { @@ -1786,20 +1786,18 @@ func (tc *TestCluster) WaitForTenantCapabilities( return nil } - if len(capIDs) > 0 { + if len(targetCaps) > 0 { missingCapabilityError := func(capID tenantcapabilities.CapabilityID) error { - return errors.Newf("server=%d tenant %s does not have capability %q", i, tenID, capID) + return errors.Newf("server=%d tenant %s cap %q not at expected value", i, tenID, capID) } capabilities, found := ts.Server.TenantCapabilitiesReader().GetCapabilities(tenID) if !found { - return missingCapabilityError(capIDs[0]) + return errors.Newf("capabilities not ready for tenant %s", tenID) } - for _, capID := range capIDs { - if capID.CapabilityType() != tenantcapabilities.Bool { - return errors.AssertionFailedf("WaitForTenantCapabilities only supports boolean capabilities") - } - if !capabilities.GetBool(capID) { + for capID, expectedValue := range targetCaps { + curVal := capabilities.Cap(capID).Get().String() + if curVal != expectedValue { return missingCapabilityError(capID) } } From f9dd5e06889df180e4663de5c79ab201a81ca67a Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Tue, 14 Mar 2023 11:00:33 -0400 Subject: [PATCH 2/3] tenantcapabilitiesccl: add some capability tests to datadriven tests Now that we have a datadriven framework available to exercise tenant capabilities, we can convert some tests in `TestMultiTenantAdminFunction` to make use of this. This patch adds SPLIT/SCATTER related tests to make use of the datadriven test framework. As is, the construction is cumbersome to work with. The move is done with the next commit in mind, where we will change the default behavior of split/scatter capabilities. Release note: None Co-authored-by: Raphael 'kena' Poss --- .../capabilities_test.go | 3 +- .../testdata/can_admin_scatter | 79 +++++++++++++++++++ .../testdata/can_admin_split | 49 +++++++++++- .../testdata/can_admin_unsplit | 34 ++++++++ pkg/sql/multitenant_admin_function_test.go | 33 ++------ 5 files changed, 167 insertions(+), 31 deletions(-) create mode 100644 pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter create mode 100644 pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go index 0e1267a04088..91299e4040c6 100644 --- a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go @@ -100,6 +100,7 @@ func TestDataDriven(t *testing.T) { // us from being able to do so. settings := cluster.MakeTestingClusterSettings() sql.SecondaryTenantSplitAtEnabled.Override(ctx, &settings.SV, true) + sql.SecondaryTenantScatterEnabled.Override(ctx, &settings.SV, true) tenantArgs := base.TestTenantArgs{ TenantID: serverutils.TestTenantID(), Settings: settings, @@ -115,7 +116,7 @@ func TestDataDriven(t *testing.T) { }() require.NoError(t, err) - var lastUpdateTS hlc.Timestamp + lastUpdateTS := tc.Server(0).Clock().Now() // ensure watcher isn't starting out empty datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string { switch d.Cmd { diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter new file mode 100644 index 000000000000..1ac15f590f47 --- /dev/null +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter @@ -0,0 +1,79 @@ +query-sql-system +SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_scatter' +---- +10 tenant-10 ready none can_admin_scatter true + +exec-sql-tenant +CREATE TABLE t(a INT) +---- +ok + +exec-sql-tenant +CREATE INDEX idx on t(a) +---- +ok + +# By default, we should be able to scatter. +exec-privileged-op-tenant +ALTER TABLE t SCATTER +---- +ok + +# ditto for the index. +exec-privileged-op-tenant +ALTER INDEX t@idx SCATTER +---- +ok + + +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter=false +---- +ok + +exec-privileged-op-tenant +ALTER TABLE t SCATTER +---- +pq: ba: AdminScatter [/Tenant/10/Table/104/1,/Tenant/10/Table/104/2) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) + +# Check the index as well. +exec-privileged-op-tenant +ALTER INDEX t@idx SCATTER +---- +pq: ba: AdminScatter [/Tenant/10/Table/104/2,/Tenant/10/Table/104/3) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) + +# Grant the capability without providing an explicit value. +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter +---- +ok + +# Scatters should work now. +exec-privileged-op-tenant +ALTER TABLE t SCATTER +---- +ok + +# Revoke the capability using REVOKE syntax. +update-capabilities +ALTER TENANT [10] REVOKE CAPABILITY can_admin_scatter +---- +ok + +# Scatters should no longer work. +exec-privileged-op-tenant +ALTER TABLE t SCATTER +---- +pq: ba: AdminScatter [/Tenant/10/Table/104/1,/Tenant/10/Table/104/2) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) + +# Lastly, use the explicitly set to true syntax. +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter=true +---- +ok + +# Scatters should now work. +exec-privileged-op-tenant +ALTER TABLE t SCATTER +---- +ok diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split index b670f661bd69..42d20b80c3b2 100644 --- a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split @@ -1,34 +1,79 @@ query-sql-system SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_split' ---- -10 tenant-10 ready none can_admin_split false +10 tenant-10 ready none can_admin_split true exec-sql-tenant CREATE TABLE t(a INT) ---- ok +exec-sql-tenant +CREATE INDEX idx on t(a) +---- +ok + +# By default, we should be able to split. +exec-privileged-op-tenant +ALTER TABLE t SPLIT AT VALUES (0) +---- +ok + +# ditto for the index. +exec-privileged-op-tenant +ALTER INDEX t@idx SPLIT AT VALUES (1) +---- +ok + + +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_admin_split=false +---- +ok + exec-privileged-op-tenant ALTER TABLE t SPLIT AT VALUES (0) ---- pq: ba: AdminSplit [/Tenant/10/Table/104/1/0,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) +# Check the index as well. +exec-privileged-op-tenant +ALTER INDEX t@idx SPLIT AT VALUES (1) +---- +pq: ba: AdminSplit [/Tenant/10/Table/104/2/1,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) + +# Grant the capability without providing an explicit value. update-capabilities -ALTER TENANT [10] GRANT CAPABILITY can_admin_split=true +ALTER TENANT [10] GRANT CAPABILITY can_admin_split ---- ok +# Splits should work now. exec-privileged-op-tenant ALTER TABLE t SPLIT AT VALUES (0) ---- ok +# Revoke the capability using REVOKE syntax. update-capabilities ALTER TENANT [10] REVOKE CAPABILITY can_admin_split ---- ok +# Splits should no longer work. exec-privileged-op-tenant ALTER TABLE t SPLIT AT VALUES (0) ---- pq: ba: AdminSplit [/Tenant/10/Table/104/1/0,/Min) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_split" (*kvpb.AdminSplitRequest) + +# Lastly, use the explicitly set to true syntax. +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_admin_split=true +---- +ok + +# Splits should now work. +exec-privileged-op-tenant +ALTER TABLE t SPLIT AT VALUES (0) +---- +ok diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit new file mode 100644 index 000000000000..5550b75bbbe6 --- /dev/null +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit @@ -0,0 +1,34 @@ +query-sql-system +SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_unsplit' +---- +10 tenant-10 ready none can_admin_unsplit false + +exec-sql-tenant +CREATE TABLE t(a INT) +---- +ok + +exec-privileged-op-tenant +ALTER TABLE t UNSPLIT AT VALUES (0) +---- +pq: could not UNSPLIT AT (0): ba: AdminUnsplit [/Tenant/10/Table/104/1/0,/Min) RPC error: grpc: client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) [code 16/Unauthenticated] + +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_admin_unsplit=true +---- +ok + +exec-privileged-op-tenant +ALTER TABLE t UNSPLIT AT VALUES (0) +---- +pq: could not UNSPLIT AT (0): key /Tenant/10/Table/104/1/0 is not the start of a range + +update-capabilities +ALTER TENANT [10] REVOKE CAPABILITY can_admin_unsplit +---- +ok + +exec-privileged-op-tenant +ALTER TABLE t UNSPLIT AT VALUES (0) +---- +pq: could not UNSPLIT AT (0): ba: AdminUnsplit [/Tenant/10/Table/104/1/0,/Min) RPC error: grpc: client tenant does not have capability "can_admin_unsplit" (*kvpb.AdminUnsplitRequest) [code 16/Unauthenticated] diff --git a/pkg/sql/multitenant_admin_function_test.go b/pkg/sql/multitenant_admin_function_test.go index cd9b1d4c8f37..951548860696 100644 --- a/pkg/sql/multitenant_admin_function_test.go +++ b/pkg/sql/multitenant_admin_function_test.go @@ -374,7 +374,9 @@ func (tc testCase) runTest( } execQueries(testCluster, systemDB, "system", tc.system) - execQueries(testCluster, secondaryDB, "secondary", tc.secondary) + if tc.secondary.isSet() { + execQueries(testCluster, secondaryDB, "secondary", tc.secondary) + } if tc.secondaryWithoutClusterSetting.isSet() { execQueries(testCluster, secondaryWithoutClusterSettingDB, "secondary_without_cluster_setting", tc.secondaryWithoutClusterSetting) } @@ -410,7 +412,8 @@ func bcap(cap tenantcapabilities.CapabilityID, val bool) capValue { return capValue{cap: cap, value: fmt.Sprint(val)} } -// TestMultiTenantAdminFunction tests the "simple" admin functions that do not require complex setup. +// TestMultiTenantAdminFunction tests the "simple" admin functions +// that do not require complex setup. func TestMultiTenantAdminFunction(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -461,15 +464,9 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{"\xf0\x89\x89", "/1", maxTimestamp}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xfe\x92\xf0\x89\x89", "/104/1/1", maxTimestamp}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.split_at.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_split"`, - }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, setupCapability: bcap(tenantcapabilities.CanAdminSplit, false), queryCapability: bcap(tenantcapabilities.CanAdminSplit, true), @@ -481,15 +478,9 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{"\xf0\x8a\x89", "/1", maxTimestamp}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xf0\x8a\x89", "/104/2/1", maxTimestamp}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.split_at.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_split"`, - }, queryClusterSetting: sql.SecondaryTenantSplitAtEnabled, setupCapability: bcap(tenantcapabilities.CanAdminSplit, false), queryCapability: bcap(tenantcapabilities.CanAdminSplit, true), @@ -574,9 +565,6 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{systemKey, systemKeyPretty}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xfe\x92", secondaryKeyPretty}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.scatter.allow_for_secondary_tenant.enabled disabled", }, @@ -594,15 +582,9 @@ func TestMultiTenantAdminFunction(t *testing.T) { system: tenantExpected{ result: [][]string{{"\xf0\x8a", "/Table/104/2"}}, }, - secondary: tenantExpected{ - result: [][]string{{"\xfe\x92", "/Tenant/10"}}, - }, secondaryWithoutClusterSetting: tenantExpected{ errorMessage: "tenant cluster setting sql.scatter.allow_for_secondary_tenant.enabled disabled", }, - secondaryWithoutCapability: tenantExpected{ - errorMessage: `does not have capability "can_admin_scatter"`, - }, queryClusterSetting: sql.SecondaryTenantScatterEnabled, setupCapability: bcap(tenantcapabilities.CanAdminScatter, false), queryCapability: bcap(tenantcapabilities.CanAdminScatter, true), @@ -653,11 +635,6 @@ 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: bcap(tenantcapabilities.CanAdminSplit, true), queryCapability: bcap(tenantcapabilities.CanAdminScatter, true), From 879bd1c9efb9c4e8f11910dac5db7c01ebf84c79 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Tue, 14 Mar 2023 10:36:34 -0400 Subject: [PATCH 3/3] multitenant: allow secondary tenants to split/scatter by default AdminSplit and AdminScatter requests are subject to capability checks. Previously, these capabilities were codified in the "enabled" form. As such, by default, secondary tenants did not have the ability to perform these operations. This is in violation of what secondary tenants could do prior to 23.1, at a time before capabilities existed. Moreover, RESTORE/IMPORT rely on performing these operations for performance. This made disallowing these operations by default a performance regression. This patch flips the phrasing of how these capabilities are stored on the proto to use the "disable" verbiage. As such, secondary tenants are able to perform splits and scatters by default. However, no change is made to the public interface -- users above the `tenantcapabilitiespb` package continue to interact with these capabilities as they were before, oblivious to how these things are stored on disk. As part of this patch, we also clean up a testing knob that was used by various backup, CDC, and logictests to override capability checks in the Authorizer. We no longer need this with the new defaults. Fixes cockroachdb#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/crdb_internal | 5 +- .../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/logic_test/tenant_capability | 16 ++--- pkg/ccl/streamingccl/streamingest/BUILD.bazel | 1 - .../replication_stream_e2e_test.go | 4 -- .../tenantcapabilities/capabilities.go | 14 ++-- .../authorizer.go | 31 ++++++-- .../testdata/basic | 15 ++-- .../testdata/default_capabilities | 71 +++++++++++++++++++ .../testdata/no_capabilities | 30 -------- .../tenantcapabilitiespb/BUILD.bazel | 5 +- .../tenantcapabilitiespb/capabilities.go | 35 +++++++-- .../tenantcapabilitiespb/capabilities.proto | 28 +++++--- .../tenantcapabilitiespb/capabilities_test.go | 3 + .../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 - .../testdata/logic_test/tenant_builtins | 22 +++--- .../testdata/distsql_tenant_locality | 1 - 35 files changed, 240 insertions(+), 239 deletions(-) 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/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