From 644c458333e05bbb821dab1b9bb693d8003ef4fa Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 16 Dec 2024 18:17:41 -0800 Subject: [PATCH 1/2] sql: include DB name in `create_statement` in `create_type_statements` This commit updates how we populate `crdb_internal.create_type_statements` virtual table so that `create_statement` column included the DB name (previously only the schema and the UDT name were included). This will make it easier to reproduce stmt bundles with cross-DB references. Also, this commit fixes how we use this virtual table during bundle creation - we now properly use `"".crdb_internal` search path and cast the type oid to `OID` type. The same fix is applied for `create_function_statements` and `create_procedure_statements` vtables. Release note: None --- .../testdata/logic_test/crdb_internal_tenant | 4 ++-- pkg/sql/crdb_internal.go | 2 +- pkg/sql/explain_bundle.go | 6 ++++-- .../testdata/logic_test/composite_types | 2 +- .../testdata/logic_test/crdb_internal | 15 +++++++++++--- .../testdata/logic_test/show_create_all_types | 20 +++++++++---------- 6 files changed, 30 insertions(+), 19 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant index 59eb5544feb6..9d8d14340a25 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant @@ -479,8 +479,8 @@ CREATE TYPE enum2 AS ENUM () query ITTITTT SELECT * FROM crdb_internal.create_type_statements ORDER BY descriptor_id ---- -104 test public 113 enum1 CREATE TYPE public.enum1 AS ENUM ('hello', 'hi') {hello,hi} -104 test public 115 enum2 CREATE TYPE public.enum2 AS ENUM () {} +104 test public 113 enum1 CREATE TYPE test.public.enum1 AS ENUM ('hello', 'hi') {hello,hi} +104 test public 115 enum2 CREATE TYPE test.public.enum2 AS ENUM () {} # Test the virtual index as well. diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 8924555e5a7d..80a5342b56a9 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -3493,7 +3493,7 @@ func writeCreateTypeDescRow( return false, errors.AssertionFailedf("unknown type descriptor kind %s", typeDesc.GetKind()) } - name, err := tree.NewUnresolvedObjectName(2, [3]string{typeDesc.GetName(), sc.GetName()}, 0) + name, err := tree.NewUnresolvedObjectName(3, [3]string{typeDesc.GetName(), sc.GetName(), db.GetName()}, 0) if err != nil { return false, err } diff --git a/pkg/sql/explain_bundle.go b/pkg/sql/explain_bundle.go index 2b41b9f30dac..1d5dc668b4d4 100644 --- a/pkg/sql/explain_bundle.go +++ b/pkg/sql/explain_bundle.go @@ -1012,7 +1012,8 @@ func (c *stmtEnvCollector) PrintCreateSequence(w io.Writer, tn *tree.TableName) func (c *stmtEnvCollector) PrintCreateUDT(w io.Writer, id oid.Oid, redactValues bool) error { descID := catid.UserDefinedOIDToID(id) - query := fmt.Sprintf("SELECT create_statement FROM crdb_internal.create_type_statements WHERE descriptor_id = %d", descID) + // Use "".crdb_internal to allow for cross-DB lookups. + query := fmt.Sprintf(`SELECT create_statement FROM "".crdb_internal.create_type_statements WHERE descriptor_id = %d::OID`, descID) if redactValues { query = fmt.Sprintf("SELECT crdb_internal.redact(crdb_internal.redactable_sql_constants(create_statement)) FROM (%s)", query) } @@ -1031,7 +1032,8 @@ func (c *stmtEnvCollector) PrintCreateRoutine( ) error { var createRoutineQuery string descID := catid.UserDefinedOIDToID(id) - queryTemplate := "SELECT create_statement FROM crdb_internal.create_%[1]s_statements WHERE %[1]s_id = %[2]d" + // Use "".crdb_internal to allow for cross-DB lookups. + queryTemplate := `SELECT create_statement FROM "".crdb_internal.create_%[1]s_statements WHERE %[1]s_id = %[2]d::OID` if procedure { createRoutineQuery = fmt.Sprintf(queryTemplate, "procedure", descID) } else { diff --git a/pkg/sql/logictest/testdata/logic_test/composite_types b/pkg/sql/logictest/testdata/logic_test/composite_types index e169c17187a0..8ec9b5010ebf 100644 --- a/pkg/sql/logictest/testdata/logic_test/composite_types +++ b/pkg/sql/logictest/testdata/logic_test/composite_types @@ -126,7 +126,7 @@ DROP TABLE tab query TTTT SELECT database_name, schema_name, descriptor_name, create_statement FROM crdb_internal.create_type_statements ---- -test public t CREATE TYPE public.t AS (a INT8, b INT8) +test public t CREATE TYPE test.public.t AS (a INT8, b INT8) ## Can't drop type t because t2 depends on it #statement error cannot drop type \"t\" because other objects .* still depend on it diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal index 3badea94370c..ddb60ee80608 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal @@ -926,15 +926,15 @@ CREATE TYPE enum2 AS ENUM () query ITTITTT SELECT * FROM crdb_internal.create_type_statements ORDER BY descriptor_id ---- -104 test public 116 enum1 CREATE TYPE public.enum1 AS ENUM ('hello', 'hi') {hello,hi} -104 test public 118 enum2 CREATE TYPE public.enum2 AS ENUM () {} +104 test public 116 enum1 CREATE TYPE test.public.enum1 AS ENUM ('hello', 'hi') {hello,hi} +104 test public 118 enum2 CREATE TYPE test.public.enum2 AS ENUM () {} # Test the virtual index as well. query ITTITTT SELECT * FROM crdb_internal.create_type_statements WHERE descriptor_id = (('enum1'::regtype::oid::int) - 100000)::oid ---- -104 test public 116 enum1 CREATE TYPE public.enum1 AS ENUM ('hello', 'hi') {hello,hi} +104 test public 116 enum1 CREATE TYPE test.public.enum1 AS ENUM ('hello', 'hi') {hello,hi} query ITTITTT SELECT * FROM crdb_internal.create_type_statements WHERE descriptor_id = 'foo'::regclass::oid @@ -1678,3 +1678,12 @@ statement ok DROP USER real_user; subtest end + +statement ok +CREATE TYPE other_db.public.enum1 AS ENUM ('yo'); + +# Ensure that the cross-DB lookup of UDTs works. +query ITTITTT +SELECT * FROM "".crdb_internal.create_type_statements WHERE descriptor_id = (('other_db.public.enum1'::regtype::int) - 100000)::oid +---- +121 other_db public 151 enum1 CREATE TYPE other_db.public.enum1 AS ENUM ('yo') {yo} diff --git a/pkg/sql/logictest/testdata/logic_test/show_create_all_types b/pkg/sql/logictest/testdata/logic_test/show_create_all_types index 197a0801fa89..05283a7fc94e 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_create_all_types +++ b/pkg/sql/logictest/testdata/logic_test/show_create_all_types @@ -16,7 +16,7 @@ query T colnames SHOW CREATE ALL TYPES ---- create_statement -CREATE TYPE public.status AS ENUM ('open', 'closed', 'inactive'); +CREATE TYPE d.public.status AS ENUM ('open', 'closed', 'inactive'); statement ok CREATE TYPE tableObj AS ENUM('row', 'col'); @@ -25,8 +25,8 @@ query T colnames,rowsort SHOW CREATE ALL TYPES ---- create_statement -CREATE TYPE public.status AS ENUM ('open', 'closed', 'inactive'); -CREATE TYPE public.tableobj AS ENUM ('row', 'col'); +CREATE TYPE d.public.status AS ENUM ('open', 'closed', 'inactive'); +CREATE TYPE d.public.tableobj AS ENUM ('row', 'col'); statement ok DROP TYPE status @@ -35,7 +35,7 @@ query T colnames SHOW CREATE ALL TYPES ---- create_statement -CREATE TYPE public.tableobj AS ENUM ('row', 'col'); +CREATE TYPE d.public.tableobj AS ENUM ('row', 'col'); # type in user-defined schema statement ok @@ -48,8 +48,8 @@ query T colnames,rowsort SHOW CREATE ALL TYPES ---- create_statement -CREATE TYPE public.tableobj AS ENUM ('row', 'col'); -CREATE TYPE s.status AS ENUM ('a', 'b', 'c'); +CREATE TYPE d.public.tableobj AS ENUM ('row', 'col'); +CREATE TYPE d.s.status AS ENUM ('a', 'b', 'c'); # Make sure database names with hyphens work well. statement ok @@ -83,8 +83,8 @@ query T colnames SHOW CREATE ALL TYPES ---- create_statement -CREATE TYPE public.address AS (street STRING, city STRING, state STRING, zipcode STRING); -COMMENT ON TYPE public.address IS 'comment for composite type address'; +CREATE TYPE "a""bc".public.address AS (street STRING, city STRING, state STRING, zipcode STRING); +COMMENT ON TYPE "a""bc".public.address IS 'comment for composite type address'; skipif config local-legacy-schema-changer statement ok @@ -103,7 +103,7 @@ query T colnames SHOW CREATE ALL TYPES ---- create_statement -CREATE TYPE public.roaches AS ENUM ('papa_roach', 'mama_roach', 'baby_roach'); -COMMENT ON TYPE public.roaches IS 'comment for enum type roaches'; +CREATE TYPE "a""bc".public.roaches AS ENUM ('papa_roach', 'mama_roach', 'baby_roach'); +COMMENT ON TYPE "a""bc".public.roaches IS 'comment for enum type roaches'; subtest end From 7b23988a264675e40653654a696cc2d236450aa0 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Fri, 13 Dec 2024 16:46:56 -0500 Subject: [PATCH 2/2] multitenant: add can_prepare_txns tenant capability Informs #22329. This commit adds a new `can_prepare_txns` tenant capability, so that we don't allow secondary tenants to prepare transactions by default. Allowing an untrusted tenant to prepare transactions would allow it to block the progress of system-wide backups, so it is too dangerous to allow by default. Release note: None --- .../testdata/logic_test/tenant_capability | 15 ++ .../tests/3node-tenant/generated_test.go | 7 - .../capabilities_test.go | 6 +- .../testdata/can_prepare_txns | 133 ++++++++++++++++++ .../tenantcapabilities/capabilities.go | 5 + .../tenantcapabilities/id_string.go | 9 +- .../authorizer.go | 83 ++++++++--- .../authorizer_test.go | 29 +++- .../testdata/authorizer_enabled | 4 +- .../testdata/basic | 35 ++++- .../tenantcapabilitiespb/capabilities.proto | 4 + .../tenantcapabilitiestestutils/testutils.go | 24 +++- pkg/multitenant/tenantcapabilities/values.go | 2 + .../testdata/logic_test/two_phase_commit | 2 +- 14 files changed, 319 insertions(+), 39 deletions(-) create mode 100644 pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_prepare_txns diff --git a/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability index 0fa24145760c..511f710cc29d 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability +++ b/pkg/ccl/logictestccl/testdata/logic_test/tenant_capability @@ -37,6 +37,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -64,6 +65,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -84,6 +86,7 @@ can_admin_split false can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -111,6 +114,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -138,6 +142,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -165,6 +170,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info true @@ -185,6 +191,7 @@ can_admin_split false can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -205,6 +212,7 @@ can_admin_split false can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -225,6 +233,7 @@ can_admin_split false can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -246,6 +255,7 @@ can_admin_split true can_admin_unsplit true can_check_consistency true can_debug_process true +can_prepare_txns true can_use_nodelocal_storage true can_view_all_metrics true can_view_node_info true @@ -289,6 +299,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -320,6 +331,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -356,6 +368,7 @@ can_admin_split true can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -376,6 +389,7 @@ can_admin_split false can_admin_unsplit false can_check_consistency false can_debug_process false +can_prepare_txns false can_use_nodelocal_storage false can_view_all_metrics false can_view_node_info false @@ -396,6 +410,7 @@ can_admin_split true can_admin_unsplit true can_check_consistency true can_debug_process true +can_prepare_txns true can_use_nodelocal_storage true can_view_all_metrics true can_view_node_info true diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index d534e65b3f12..4a0fabfbddfd 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -2206,13 +2206,6 @@ func TestTenantLogic_tuple( runLogicTest(t, "tuple") } -func TestTenantLogic_two_phase_commit( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "two_phase_commit") -} - func TestTenantLogic_txn( t *testing.T, ) { diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go index 1bbade3e9d8f..388fc7b38f28 100644 --- a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go @@ -8,6 +8,7 @@ package tenantcapabilitiesccl import ( "context" "fmt" + "regexp" "testing" "github.com/cockroachdb/cockroach/pkg/base" @@ -112,7 +113,10 @@ func TestDataDriven(t *testing.T) { }) _, err := tenantSQLDB.Exec(d.Input) if err != nil { - return err.Error() + errStr := err.Error() + // Redact transaction IDs from error strings, for determinism. + errStr = regexp.MustCompile(`\[txn: [0-9a-f]+]`).ReplaceAllString(errStr, `[txn: ‹×›]`) + return errStr } case "exec-sql-tenant": diff --git a/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_prepare_txns b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_prepare_txns new file mode 100644 index 000000000000..7e6bce259b82 --- /dev/null +++ b/pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_prepare_txns @@ -0,0 +1,133 @@ +query-sql-system +SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_name = 'can_prepare_txns' +---- +10 cluster-10 ready external can_prepare_txns false + +exec-sql-tenant +CREATE TABLE t(a INT PRIMARY KEY) +---- +ok + +# By default, we should not be able to prepare transactions. +exec-privileged-op-tenant +BEGIN +---- +ok + +exec-privileged-op-tenant +INSERT INTO t VALUES (1) +---- +ok + +exec-privileged-op-tenant +PREPARE TRANSACTION 'txn1' +---- +pq: ba: QueryIntent [/Tenant/10/Table/104/1/1/0], EndTxn(commit) [/Tenant/10/Table/104/1/1/0], [txn: ‹×›], [can-forward-ts] RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_prepare_txns" (*kvpb.EndTxnRequest) + + +# Grant the capability. +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_prepare_txns=true +---- +ok + +exec-privileged-op-tenant +BEGIN +---- +ok + +exec-privileged-op-tenant +INSERT INTO t VALUES (1) +---- +ok + +exec-privileged-op-tenant +PREPARE TRANSACTION 'txn2' +---- +ok + +exec-privileged-op-tenant +ROLLBACK PREPARED 'txn2' +---- +ok + + +# Revoke the capability using REVOKE syntax. +update-capabilities +ALTER TENANT [10] REVOKE CAPABILITY can_prepare_txns +---- +ok + +# Prepared transactions should no longer work. +exec-privileged-op-tenant +BEGIN +---- +ok + +exec-privileged-op-tenant +INSERT INTO t VALUES (1) +---- +ok + +exec-privileged-op-tenant +PREPARE TRANSACTION 'txn3' +---- +pq: ba: QueryIntent [/Tenant/10/Table/104/1/1/0], EndTxn(commit) [/Tenant/10/Table/104/1/1/0], [txn: ‹×›], [can-forward-ts] RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_prepare_txns" (*kvpb.EndTxnRequest) + + +# However, transactions that have not acquired locks are able to be prepared, +# since they don't actually prepare a transaction record in the KV layer. This +# isn't necessarily intentional, but it is also not harmful or worth changing. +exec-privileged-op-tenant +BEGIN +---- +ok + +exec-privileged-op-tenant +SELECT * FROM t +---- +ok + +exec-privileged-op-tenant +PREPARE TRANSACTION 'txn4' +---- +ok + +exec-privileged-op-tenant +COMMIT PREPARED 'txn4' +---- +ok + + +# Grant the capability one more time. +update-capabilities +ALTER TENANT [10] GRANT CAPABILITY can_prepare_txns +---- +ok + +exec-privileged-op-tenant +BEGIN +---- +ok + +exec-privileged-op-tenant +INSERT INTO t VALUES (1) +---- +ok + +exec-privileged-op-tenant +PREPARE TRANSACTION 'txn5' +---- +ok + +# Revoke the capability one more time, which will **not** prevent us from +# committing (or rolling back) the already prepared transaction. +update-capabilities +ALTER TENANT [10] REVOKE CAPABILITY can_prepare_txns +---- +ok + +exec-privileged-op-tenant +COMMIT PREPARED 'txn5' +---- +ok diff --git a/pkg/multitenant/tenantcapabilities/capabilities.go b/pkg/multitenant/tenantcapabilities/capabilities.go index 79f453340efc..a1ec9e49b6b1 100644 --- a/pkg/multitenant/tenantcapabilities/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/capabilities.go @@ -92,6 +92,10 @@ const ( // metrics, but this implementation is simpler). CanViewAllMetrics // can_view_all_metrics + // CanPrepareTxns describes the ability of a tenant to prepare transactions as + // part of the XA two-phase commit protocol. + CanPrepareTxns // can_prepare_txns + MaxCapabilityID ID = iota - 1 ) @@ -124,6 +128,7 @@ var capabilities = [MaxCapabilityID + 1]Capability{ TenantSpanConfigBounds: spanConfigBoundsCapability(TenantSpanConfigBounds), CanDebugProcess: boolCapability(CanDebugProcess), CanViewAllMetrics: boolCapability(CanViewAllMetrics), + CanPrepareTxns: boolCapability(CanPrepareTxns), } // EnableAll enables maximum access to services. diff --git a/pkg/multitenant/tenantcapabilities/id_string.go b/pkg/multitenant/tenantcapabilities/id_string.go index f40cf2317d5f..64ce6b173cdc 100644 --- a/pkg/multitenant/tenantcapabilities/id_string.go +++ b/pkg/multitenant/tenantcapabilities/id_string.go @@ -25,7 +25,8 @@ func _() { _ = x[TenantSpanConfigBounds-10] _ = x[CanDebugProcess-11] _ = x[CanViewAllMetrics-12] - _ = x[MaxCapabilityID-12] + _ = x[CanPrepareTxns-13] + _ = x[MaxCapabilityID-13] } func (i ID) String() string { @@ -54,6 +55,8 @@ func (i ID) String() string { return "can_debug_process" case CanViewAllMetrics: return "can_view_all_metrics" + case CanPrepareTxns: + return "can_prepare_txns" default: return "ID(" + strconv.FormatInt(int64(i), 10) + ")" } @@ -72,7 +75,8 @@ var stringToCapabilityIDMap = map[string]ID{ "span_config_bounds": 10, "can_debug_process": 11, "can_view_all_metrics": 12, - "MaxCapabilityID": 12, + "can_prepare_txns": 13, + "MaxCapabilityID": 13, } var IDs = []ID{ @@ -82,6 +86,7 @@ var IDs = []ID{ CanAdminUnsplit, CanCheckConsistency, CanDebugProcess, + CanPrepareTxns, CanUseNodelocalStorage, CanViewAllMetrics, CanViewNodeInfo, diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go index 250040944e89..c169b1e41296 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go @@ -146,8 +146,8 @@ func (a *Authorizer) authBatchNoCap( ) error { for _, ru := range ba.Requests { request := ru.GetInner() - requiredCap := reqMethodToCap[request.Method()] - if requiredCap == noCapCheckNeeded { + requiredCap := reqMethodToCap[request.Method()].get(request) + if requiredCap == noCapCheckNeededID { continue } switch request.Method() { @@ -170,11 +170,11 @@ func (a *Authorizer) capCheckForBatch( ) error { for _, ru := range ba.Requests { request := ru.GetInner() - requiredCap, hasCap := reqMethodToCap[request.Method()] - if requiredCap == noCapCheckNeeded { + requiredCap := reqMethodToCap[request.Method()].get(request) + if requiredCap == noCapCheckNeededID { continue } - if !hasCap || requiredCap == onlySystemTenant || + if requiredCap == unknownMethodID || requiredCap == onlySystemTenantID || !tenantcapabilities.MustGetBoolByID(entry.TenantCapabilities, requiredCap) { // All allowable request types must be explicitly opted into the // reqMethodToCap map. If a request type is missing from the map @@ -200,7 +200,48 @@ var ( errCannotDebugProcess = errors.New("client tenant does not have capability to debug the process") ) -var reqMethodToCap = map[kvpb.Method]tenantcapabilities.ID{ +// methodCapability associates a KV method with a capability. The capability can +// either be static for all instances of the method, or it can be determined +// dynamically by a function based on the request's contents. +type methodCapability struct { + capID tenantcapabilities.ID + capFn func(kvpb.Request) tenantcapabilities.ID +} + +func (mc methodCapability) get(req kvpb.Request) tenantcapabilities.ID { + if mc.capID == 0 && mc.capFn == nil { + return unknownMethodID + } + if mc.capFn != nil { + return mc.capFn(req) + } + return mc.capID +} + +// staticCap returns a methodCapability that requires a specific capability, +// regardless of the request's contents. +func staticCap(capID tenantcapabilities.ID) methodCapability { + return methodCapability{capID: capID} +} + +// dynamicCap returns a methodCapability that requires a capability determined +// by a function based on the request's contents. +func dynamicCap(capFn func(kvpb.Request) tenantcapabilities.ID) methodCapability { + return methodCapability{capFn: capFn} +} + +const ( + noCapCheckNeededID = iota + tenantcapabilities.MaxCapabilityID + 1 + onlySystemTenantID + unknownMethodID +) + +var ( + noCapCheckNeeded = staticCap(noCapCheckNeededID) + onlySystemTenant = staticCap(onlySystemTenantID) +) + +var reqMethodToCap = map[kvpb.Method]methodCapability{ // The following requests are authorized for all workloads. kvpb.AddSSTable: noCapCheckNeeded, kvpb.Barrier: noCapCheckNeeded, @@ -208,7 +249,6 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.ID{ kvpb.ConditionalPut: noCapCheckNeeded, kvpb.Delete: noCapCheckNeeded, kvpb.DeleteRange: noCapCheckNeeded, - kvpb.EndTxn: noCapCheckNeeded, kvpb.Export: noCapCheckNeeded, kvpb.Get: noCapCheckNeeded, kvpb.HeartbeatTxn: noCapCheckNeeded, @@ -231,14 +271,24 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.ID{ kvpb.RevertRange: noCapCheckNeeded, kvpb.Scan: noCapCheckNeeded, + // The following have dynamic capabilities, depending on the type of request + // and the request's contents. + kvpb.EndTxn: dynamicCap(func(req kvpb.Request) tenantcapabilities.ID { + et := req.(*kvpb.EndTxnRequest) + if et.Prepare { + return tenantcapabilities.CanPrepareTxns + } + return noCapCheckNeededID + }), + // The following are authorized via specific capabilities. - kvpb.AdminChangeReplicas: tenantcapabilities.CanAdminRelocateRange, - kvpb.AdminScatter: tenantcapabilities.CanAdminScatter, - kvpb.AdminSplit: tenantcapabilities.CanAdminSplit, - kvpb.AdminUnsplit: tenantcapabilities.CanAdminUnsplit, - kvpb.AdminRelocateRange: tenantcapabilities.CanAdminRelocateRange, - kvpb.AdminTransferLease: tenantcapabilities.CanAdminRelocateRange, - kvpb.CheckConsistency: tenantcapabilities.CanCheckConsistency, + kvpb.AdminChangeReplicas: staticCap(tenantcapabilities.CanAdminRelocateRange), + kvpb.AdminScatter: staticCap(tenantcapabilities.CanAdminScatter), + kvpb.AdminSplit: staticCap(tenantcapabilities.CanAdminSplit), + kvpb.AdminUnsplit: staticCap(tenantcapabilities.CanAdminUnsplit), + kvpb.AdminRelocateRange: staticCap(tenantcapabilities.CanAdminRelocateRange), + kvpb.AdminTransferLease: staticCap(tenantcapabilities.CanAdminRelocateRange), + kvpb.CheckConsistency: staticCap(tenantcapabilities.CanCheckConsistency), // TODO(knz,arul): Verify with the relevant teams whether secondary // tenants have legitimate access to any of those. @@ -259,11 +309,6 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.ID{ kvpb.LinkExternalSSTable: onlySystemTenant, } -const ( - noCapCheckNeeded = iota + tenantcapabilities.MaxCapabilityID + 1 - onlySystemTenant -) - // BindReader implements the tenantcapabilities.Authorizer interface. func (a *Authorizer) BindReader(reader tenantcapabilities.Reader) { a.Lock() diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go index 5712b7603875..7332fd1d435b 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go @@ -177,15 +177,40 @@ func (m mockReader) GetGlobalCapabilityState() map[roachpb.TenantID]*tenantcapab } func TestAllBatchCapsAreBoolean(t *testing.T) { - for _, capID := range reqMethodToCap { + checkCap := func(t *testing.T, capID tenantcapabilities.ID) { if capID >= tenantcapabilities.MaxCapabilityID { // One of the special values. - continue + return } caps := &tenantcapabilitiespb.TenantCapabilities{} var v *tenantcapabilities.BoolValue require.Implements(t, v, tenantcapabilities.MustGetValueByID(caps, capID)) } + + for m, mc := range reqMethodToCap { + if mc.capFn != nil { + switch m { + case kvpb.EndTxn: + // Handled below. + default: + t.Fatalf("unexpected capability function for %s", m) + } + } else { + checkCap(t, mc.capID) + } + } + + { + const method = kvpb.EndTxn + mc := reqMethodToCap[method] + capIDs := []tenantcapabilities.ID{ + mc.get(&kvpb.EndTxnRequest{}), + mc.get(&kvpb.EndTxnRequest{Prepare: true}), + } + for _, capID := range capIDs { + checkCap(t, capID) + } + } } func TestAllBatchRequestTypesHaveAssociatedCaps(t *testing.T) { diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled index a43472ddacdc..8d880c8be930 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled @@ -8,7 +8,7 @@ client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRe has-capability-for-batch ten=10 cmds=(Merge) ---- -client tenant does not have capability "ID(14)" (*kvpb.MergeRequest) +client tenant does not have capability "ID(15)" (*kvpb.MergeRequest) has-node-status-capability ten=10 ---- @@ -38,7 +38,7 @@ ok has-capability-for-batch ten=10 cmds=(Merge) ---- -client tenant does not have capability "ID(14)" (*kvpb.MergeRequest) +client tenant does not have capability "ID(15)" (*kvpb.MergeRequest) has-node-status-capability ten=10 ---- diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic index 0f1f2d4d6277..ad6c71b70b64 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/basic @@ -1,8 +1,8 @@ -upsert ten=10 can_admin_relocate_range=true can_admin_scatter=true can_admin_split=true can_admin_unsplit=true can_view_node_info=true can_view_tsdb_metrics=true +upsert ten=10 can_admin_relocate_range=true can_admin_scatter=true can_admin_split=true can_admin_unsplit=true can_view_node_info=true can_view_tsdb_metrics=true can_prepare_txns=true ---- ok -upsert ten=11 can_admin_relocate_range=false 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_relocate_range=false can_admin_scatter=false can_admin_split=false can_admin_unsplit=false can_view_node_info=false can_view_tsdb_metrics=false can_prepare_txns=false ---- ok @@ -67,7 +67,7 @@ ok # Lastly, flip tenant 10's capability for splits; ensure it can no longer issue # splits as a result. -upsert ten=10 can_admin_relocate_range=false can_admin_scatter=true can_admin_split=false can_admin_unsplit=false can_view_node_info=true can_view_tsdb_metrics=true +upsert ten=10 can_admin_relocate_range=false can_admin_scatter=true can_admin_split=false can_admin_unsplit=false can_view_node_info=true can_view_tsdb_metrics=true can_prepare_txns=true ---- ok @@ -109,6 +109,35 @@ has-capability-for-batch ten=10 cmds=(Scan, ConditionalPut) ---- ok +# Tenant 10 should be able to prepare transactions, but tenant 11 should not. +has-capability-for-batch ten=10 cmds=(EndTxn) +---- +ok + +has-capability-for-batch ten=10 cmds=(EndTxn{Prepare}) +---- +ok + +has-capability-for-batch ten=11 cmds=(EndTxn) +---- +ok + +has-capability-for-batch ten=11 cmds=(EndTxn{Prepare}) +---- +client tenant does not have capability "can_prepare_txns" (*kvpb.EndTxnRequest) + +upsert ten=10 can_admin_relocate_range=false can_admin_scatter=true can_admin_split=false can_admin_unsplit=false can_view_node_info=true can_view_tsdb_metrics=true can_prepare_txns=false +---- +ok + +has-capability-for-batch ten=10 cmds=(EndTxn) +---- +ok + +has-capability-for-batch ten=10 cmds=(EndTxn{Prepare}) +---- +client tenant does not have capability "can_prepare_txns" (*kvpb.EndTxnRequest) + has-node-status-capability ten=10 ---- ok diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto index d03d04ab315b..bf0cfe2b590a 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto @@ -95,6 +95,10 @@ message TenantCapabilities { // CanViewAllMetrics, if set to true, grants the tenant the ability // to query any metrics from the host. bool can_view_all_metrics = 12; + + // CanPrepareTxns, if set to true, grants the tenant the ability to prepare + // transactions as part of the XA two-phase commit protocol. + bool can_prepare_txns = 13; }; // SpanConfigBound is used to constrain the possible values a SpanConfig may diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils/testutils.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils/testutils.go index 14fc9d621360..1098572219da 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils/testutils.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils/testutils.go @@ -6,6 +6,7 @@ package tenantcapabilitiestestutils import ( + "regexp" "strconv" "strings" "testing" @@ -23,21 +24,40 @@ import ( "github.com/stretchr/testify/require" ) +var reqWithParamRE = regexp.MustCompile(`(\w+)(?:{(\w+)})?`) + // ParseBatchRequests is a helper function to parse datadriven input that // declares (empty) batch requests of supported types, for a particular tenant. // The constructed batch request is returned. The cmds are of the following // form: // -// cmds=(split, scan, cput) +// cmds=(AdminSplit, Scan, ConditionalPut, EndTxn{Prepare}) func ParseBatchRequests(t *testing.T, d *datadriven.TestData) (ba kvpb.BatchRequest) { for _, cmd := range d.CmdArgs { if cmd.Key == "cmds" { for _, z := range cmd.Vals { - method, ok := kvpb.StringToMethodMap[z] + reqWithParam := reqWithParamRE.FindStringSubmatch(z) + reqStr := reqWithParam[1] + paramStr := reqWithParam[2] + method, ok := kvpb.StringToMethodMap[reqStr] if !ok { t.Fatalf("unsupported request type: %s", z) } request := kvpb.CreateRequest(method) + if paramStr != "" { + ok = false + switch method { + case kvpb.EndTxn: + switch paramStr { + case "Prepare": + request.(*kvpb.EndTxnRequest).Prepare = true + ok = true + } + } + if !ok { + t.Fatalf("unsupported %s param: %s", method, paramStr) + } + } ba.Add(request) } } diff --git a/pkg/multitenant/tenantcapabilities/values.go b/pkg/multitenant/tenantcapabilities/values.go index 84f6e266bfcc..4be35b3e9ffd 100644 --- a/pkg/multitenant/tenantcapabilities/values.go +++ b/pkg/multitenant/tenantcapabilities/values.go @@ -55,6 +55,8 @@ func GetValueByID(t *tenantcapabilitiespb.TenantCapabilities, id ID) (Value, err return (*boolValue)(&t.CanDebugProcess), nil case CanViewAllMetrics: return (*boolValue)(&t.CanViewAllMetrics), nil + case CanPrepareTxns: + return (*boolValue)(&t.CanPrepareTxns), nil default: return nil, errors.AssertionFailedf("unknown capability: %q", id.String()) } diff --git a/pkg/sql/logictest/testdata/logic_test/two_phase_commit b/pkg/sql/logictest/testdata/logic_test/two_phase_commit index 507b819b3cde..4fd0e67d9d68 100644 --- a/pkg/sql/logictest/testdata/logic_test/two_phase_commit +++ b/pkg/sql/logictest/testdata/logic_test/two_phase_commit @@ -1,4 +1,4 @@ -# LogicTest: !local-mixed-24.3 +# LogicTest: !3node-tenant-default-configs !local-mixed-24.3 query T SHOW max_prepared_transactions