Merge cockroachdb#98395

98395: multitenant: add AdminScatterRequest capability r=knz a=ecwall Fixes cockroachdb#98394 1) Add a new `tenantcapabilities.CanAdminScatter` capability. 2) Check capability in `Authorizer.HasCapabilityForBatch`. Release note: None Co-authored-by: Evan Wall <[email protected]>
jeffswenson · Mar 11, 2023 · e417cdd · e417cdd
2 parents 5b2a567 + 39a723d
commit e417cdd
Show file tree

Hide file tree

Showing 15 changed files with 191 additions and 186 deletions.
diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-tenants b/pkg/ccl/backupccl/testdata/backup-restore/restore-tenants
@@ -27,11 +27,11 @@ CREATE DATABASE db1;
 ----
 
 query-sql
-SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
+SELECT id,name,data_state,service_mode,active,json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'deprecatedDataState'),json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'droppedName') FROM system.tenants;
 ----
-1 system 1 2 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-5 <nil> 2 0 false {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "tenant-5", "tenantReplicationJobId": "0"}
-6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 system 1 2 true READY 
+5 <nil> 2 0 false DROP tenant-5
+6 tenant-6 1 1 true READY 
 
 exec-sql
 BACKUP INTO 'nodelocal://1/cluster_without_tenants'
@@ -79,10 +79,10 @@ job paused at pausepoint
 # Application tenants backed up in an ACTIVE state should be moved to an ADD
 # state during restore.
 query-sql
-SELECT id,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
+SELECT id,active,json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'deprecatedDataState'),json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'droppedName') FROM system.tenants;
 ----
-1 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-6 false {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "ADD", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 true READY 
+6 false ADD 
 
 exec-sql
 SET CLUSTER SETTING jobs.debug.pausepoints = ''
@@ -100,10 +100,10 @@ USE defaultdb;
 
 # A dropped tenant should be restored as an inactive tenant.
 query-sql
-SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
+SELECT id,name,data_state,service_mode,active,json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'deprecatedDataState'),json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'droppedName') FROM system.tenants;
 ----
-1 system 1 2 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 system 1 2 true READY 
+6 tenant-6 1 1 true READY 
 
 
 exec-sql expect-error-regex=(tenant 6 not in backup)
@@ -131,11 +131,11 @@ RESTORE TENANT 6 FROM LATEST IN 'nodelocal://1/tenant6' WITH tenant_name = 'newn
 ----
 
 query-sql
-SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
+SELECT id,name,data_state,service_mode,active,json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'deprecatedDataState'),json_extract_path_text(crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true), 'droppedName') FROM system.tenants;
 ----
-1 system 1 2 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-2 newname 1 1 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"}
-6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false, "canAdminUnsplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false, "spanConfigBounds": null}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 system 1 2 true READY 
+2 newname 1 1 true READY 
+6 tenant-6 1 1 true READY 
 
 # Check that another service mode is also preserved.
 exec-sql

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal
@@ -191,33 +191,21 @@ SELECT crdb_internal.create_tenant(5)
 
 # Use retry because source data is eventually consistent.
 query ITT colnames,retry
-SELECT * FROM crdb_internal.node_tenant_capabilities_cache
+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_unsplit      false
-1          can_view_node_info     false
-1          can_view_tsdb_metrics  false
-5          can_admin_split        false
-5          can_admin_unsplit      false
-5          can_view_node_info     false
-5          can_view_tsdb_metrics  false
+tenant_id  capability_id    capability_value
+1          can_admin_split  false
+5          can_admin_split  false
 
 statement ok
 ALTER TENANT [5] GRANT CAPABILITY can_admin_split
 
 # Use retry because source data is eventually consistent.
 query ITT colnames,retry
-SELECT * FROM crdb_internal.node_tenant_capabilities_cache
+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_unsplit      false
-1          can_view_node_info     false
-1          can_view_tsdb_metrics  false
-5          can_admin_split        true
-5          can_admin_unsplit      false
-5          can_view_node_info     false
-5          can_view_tsdb_metrics  false
+tenant_id  capability_id    capability_value
+1          can_admin_split  false
+5          can_admin_split  true
 
 subtest end
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability
@@ -31,6 +31,7 @@ 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_unsplit      false
 can_view_node_info     false
@@ -50,6 +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_split        true
 can_admin_unsplit      false
 can_view_node_info     false
@@ -62,6 +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_split        false
 can_admin_unsplit      false
 can_view_node_info     false
@@ -81,6 +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_split        true
 can_admin_unsplit      false
 can_view_node_info     false
@@ -100,6 +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_split        true
 can_admin_unsplit      false
 can_view_node_info     false
@@ -119,6 +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_split        true
 can_admin_unsplit      false
 can_view_node_info     true
@@ -131,6 +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_split        false
 can_admin_unsplit      false
 can_view_node_info     false

diff --git a/pkg/multitenant/tenantcapabilities/capabilities.go b/pkg/multitenant/tenantcapabilities/capabilities.go
@@ -160,6 +160,12 @@ 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
+
 	// 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
@@ -222,6 +228,7 @@ const (
 func (c CapabilityID) CapabilityType() Type {
 	switch c {
 	case
+		CanAdminScatter,
 		CanAdminSplit,
 		CanAdminUnsplit,
 		CanViewNodeInfo,

diff --git a/pkg/multitenant/tenantcapabilities/capabilityid_string.go b/pkg/multitenant/tenantcapabilities/capabilityid_string.go
diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go
@@ -70,7 +70,8 @@ func (a *Authorizer) HasCapabilityForBatch(
 			continue
 		}
 		if !hasCap || requiredCap == onlySystemTenant || !found || !cp.GetBool(requiredCap) {
-			if requiredCap == tenantcapabilities.CanAdminSplit && a.knobs.AuthorizerSkipAdminSplitCapabilityChecks {
+			if (requiredCap == tenantcapabilities.CanAdminSplit || requiredCap == tenantcapabilities.CanAdminScatter) &&
+				a.knobs.AuthorizerSkipAdminSplitCapabilityChecks {
 				continue
 			}
 			// All allowable request types must be explicitly opted into the
@@ -117,14 +118,14 @@ 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.AdminUnsplit: tenantcapabilities.CanAdminUnsplit,
 
 	// TODO(ecwall): The following should also be authorized via specific capabilities.
 	kvpb.AdminChangeReplicas: noCapCheckNeeded,
 	kvpb.AdminMerge:          noCapCheckNeeded,
 	kvpb.AdminRelocateRange:  noCapCheckNeeded,
-	kvpb.AdminScatter:        noCapCheckNeeded,
 	kvpb.AdminTransferLease:  noCapCheckNeeded,
 
 	// TODO(knz,arul): Verify with the relevant teams whether secondary

diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic
@@ -1,8 +1,12 @@
-upsert ten=10 can_admin_split=true can_admin_unsplit=true can_view_node_info=true can_view_tsdb_metrics=true
+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
 ----
 ok
 
-upsert ten=11 can_admin_split=false can_admin_unsplit=false can_view_node_info=false can_view_tsdb_metrics=false
+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
+----
+ok
+
+has-capability-for-batch ten=10 cmds=(AdminScatter, Scan, ConditionalPut)
 ----
 ok
 
@@ -16,6 +20,10 @@ has-capability-for-batch ten=10 cmds=(AdminUnsplit, Scan, ConditionalPut)
 ----
 ok
 
+has-capability-for-batch ten=11 cmds=(AdminScatter, Scan, ConditionalPut)
+----
+client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
 # Tenant 11 shouldn't be able to issue splits.
 has-capability-for-batch ten=11 cmds=(AdminSplit, Scan, ConditionalPut)
 ----
@@ -43,10 +51,14 @@ ok
 
 # Lastly, flip tenant 10's capability for splits; ensure it can no longer issue
 # splits as a result.
-upsert ten=10 can_admin_split=false can_admin_unsplit=false can_view_node_info=true can_view_tsdb_metrics=true
+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
 ----
 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)

diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go
@@ -52,6 +52,8 @@ 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.CanAdminUnsplit:
@@ -69,6 +71,8 @@ func (t *TenantCapabilities) Cap(
 // 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.CanAdminUnsplit:

diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto
@@ -61,6 +61,10 @@ message TenantCapabilities {
   // 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;
 };
 
 // SpanConfigBound is used to constrain the possible values a SpanConfig may

diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/basic b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/testdata/basic
@@ -18,13 +18,13 @@ ok
 updates
 ----
 Incremental Update
-update: ten=10 cap={can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
-update: ten=11 cap={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: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}
 
 flush-state
 ----
-ten=10 cap={can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
-ten=11 cap={can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
+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}
 
 upsert ten=11 can_admin_split=true
 ----
@@ -33,11 +33,11 @@ ok
 updates
 ----
 Incremental Update
-update: ten=11 cap={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:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
 
 get-capabilities ten=11
 ----
-{can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
+{can_admin_scatter:false can_admin_split:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
 
 delete ten=10
 ----
@@ -64,7 +64,7 @@ updates
 # what we'd expect.
 flush-state
 ----
-ten=11 cap={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:true can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
 
 upsert ten=15 can_admin_split=true
 ----
@@ -73,7 +73,7 @@ ok
 updates
 ----
 Incremental Update
-update: ten=15 cap={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:false can_admin_split:true can_admin_unsplit:false 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.
@@ -100,7 +100,7 @@ not-found
 
 flush-state
 ----
-ten=15 cap={can_admin_split:true 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}
 
 # Same thing, but this time instead of deleting the key, leave it behind.
 delete ten=15
@@ -118,12 +118,12 @@ ok
 updates
 ----
 Incremental Update
-update: ten=15 cap={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:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
 
 flush-state
 ----
-ten=15 cap={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:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
 
 get-capabilities ten=15
 ----
-{can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}
+{can_admin_scatter:false can_admin_split:false can_admin_unsplit:false can_view_node_info:false can_view_tsdb_metrics:false}