From 7adcd34284b72a60f5a82bb61edfd83b52d14460 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Wed, 26 Oct 2022 13:41:15 -0400 Subject: [PATCH] sql: add role_id and member_id columns to system.role_members table This patch adds user ID columns to the system.role_members table. The new `role_id` column corresponds to the existing `role` column and the new `member_id` column corresponds to the existing `member` column. Migrations are also added to alter and backfill the table in older clusters. Release note: None --- .../settings/settings-for-tenants.txt | 2 +- docs/generated/settings/settings.html | 2 +- .../testdata/benchmark_expectations | 4 +- pkg/ccl/backupccl/backup_test.go | 34 +-- pkg/clusterversion/cockroach_versions.go | 16 ++ pkg/sql/catalog/systemschema/system.go | 59 ++++- .../systemschema_test/testdata/bootstrap | 11 +- pkg/sql/grant_role.go | 95 ++++++-- .../testdata/logic_test/information_schema | 5 + .../logictest/testdata/logic_test/pg_catalog | 7 + pkg/sql/logictest/testdata/logic_test/role | 166 ++++++------- pkg/sql/logictest/testdata/logic_test/system | 13 +- pkg/upgrade/upgrades/BUILD.bazel | 2 + pkg/upgrade/upgrades/permanent_upgrades.go | 10 +- .../upgrades/role_members_ids_migration.go | 118 ++++++++++ .../role_members_ids_migration_test.go | 219 ++++++++++++++++++ pkg/upgrade/upgrades/upgrades.go | 10 + 17 files changed, 629 insertions(+), 144 deletions(-) create mode 100644 pkg/upgrade/upgrades/role_members_ids_migration.go create mode 100644 pkg/upgrade/upgrades/role_members_ids_migration_test.go diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index a19c7a7ab8b5..b038a5d03d67 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -297,4 +297,4 @@ trace.jaeger.agent string the address of a Jaeger agent to receive traces using trace.opentelemetry.collector string address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as :. If no port is specified, 4317 will be used. trace.span_registry.enabled boolean true if set, ongoing traces can be seen at https:///#/debug/tracez trace.zipkin.collector string the address of a Zipkin instance to receive traces, as :. If no port is specified, 9411 will be used. -version version 1000022.2-10 set the active cluster version in the format '.' +version version 1000022.2-14 set the active cluster version in the format '.' diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index f4a568ec9ba0..55aee4856a71 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -233,6 +233,6 @@ trace.opentelemetry.collectorstringaddress of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as :. If no port is specified, 4317 will be used. trace.span_registry.enabledbooleantrueif set, ongoing traces can be seen at https:///#/debug/tracez trace.zipkin.collectorstringthe address of a Zipkin instance to receive traces, as :. If no port is specified, 9411 will be used. -versionversion1000022.2-10set the active cluster version in the format '.' +versionversion1000022.2-14set the active cluster version in the format '.' diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index 547116b8c9d5..8b0394e1ac8b 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -50,8 +50,8 @@ exp,benchmark 11,Grant/grant_all_on_1_table 12,Grant/grant_all_on_2_tables 12,Grant/grant_all_on_3_tables -9,GrantRole/grant_1_role -11,GrantRole/grant_2_roles +11,GrantRole/grant_1_role +15,GrantRole/grant_2_roles 3,ORMQueries/activerecord_type_introspection_query 3,ORMQueries/django_table_introspection_1_table 3,ORMQueries/django_table_introspection_4_tables diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 1ae9d9beadf3..a021cd5e6ad3 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -9468,19 +9468,19 @@ func TestBackupRestoreSystemUsers(t *testing.T) { // Role 'app_role' and user 'app' will be added, and 'app' is granted with 'app_role' // User test will remain untouched with no role granted - sqlDBRestore.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\" FROM system.users", [][]string{ - {"admin", "", "true"}, - {"app", "NULL", "false"}, - {"app_role", "NULL", "true"}, - {"root", "", "false"}, - {"test", "NULL", "false"}, - {"test_role", "NULL", "true"}, + sqlDBRestore.CheckQueryResults(t, "SELECT * FROM system.users", [][]string{ + {"admin", "", "true", "2"}, + {"app", "NULL", "false", "101"}, + {"app_role", "NULL", "true", "102"}, + {"root", "", "false", "1"}, + {"test", "NULL", "false", "100"}, + {"test_role", "NULL", "true", "103"}, }) sqlDBRestore.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{ - {"admin", "app", "false"}, - {"admin", "root", "true"}, - {"app_role", "app", "false"}, - {"app_role", "test_role", "false"}, + {"admin", "app", "false", "NULL", "NULL"}, + {"admin", "root", "true", "2", "1"}, + {"app_role", "app", "false", "NULL", "NULL"}, + {"app_role", "test_role", "false", "NULL", "NULL"}, }) sqlDBRestore.CheckQueryResults(t, "SHOW USERS", [][]string{ {"admin", "", "{}"}, @@ -9501,10 +9501,10 @@ func TestBackupRestoreSystemUsers(t *testing.T) { defer cleanupEmptyCluster1() t.Run("restore-from-backup-with-no-system-role-members", func(t *testing.T) { sqlDBRestore1.Exec(t, "RESTORE SYSTEM USERS FROM $1", localFoo+"/3") - sqlDBRestore1.CheckQueryResults(t, "SELECT \"role\", \"member\", \"isAdmin\" FROM system.role_members", [][]string{ - {"admin", "root", "true"}, + sqlDBRestore1.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{ + {"admin", "root", "true", "2", "1"}, }) - sqlDBRestore1.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\", \"user_id\" FROM system.users", [][]string{ + sqlDBRestore1.CheckQueryResults(t, "SELECT * FROM system.users", [][]string{ {"admin", "", "true", "2"}, {"app", "NULL", "false", "100"}, {"app_role", "NULL", "true", "101"}, @@ -9528,10 +9528,10 @@ func TestBackupRestoreSystemUsers(t *testing.T) { // allocated properly in the restore. sqlDBRestore2.Exec(t, "CREATE USER testuser") sqlDBRestore2.Exec(t, "RESTORE SYSTEM USERS FROM $1", localFoo+"/3") - sqlDBRestore2.CheckQueryResults(t, "SELECT \"role\", \"member\", \"isAdmin\" FROM system.role_members", [][]string{ - {"admin", "root", "true"}, + sqlDBRestore2.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{ + {"admin", "root", "true", "2", "1"}, }) - sqlDBRestore2.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\", \"user_id\" FROM system.users", [][]string{ + sqlDBRestore2.CheckQueryResults(t, "SELECT * FROM system.users", [][]string{ {"admin", "", "true", "2"}, {"app", "NULL", "false", "101"}, {"app_role", "NULL", "true", "102"}, diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 3abd607ef13b..e12f3a4f6f8d 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -342,6 +342,14 @@ const ( // V23_1_CreateSystemJobInfoTable creates the system.job_info table. V23_1CreateSystemJobInfoTable + // V23_1RoleMembersTableHasIDColumns is the version where the role_members + // system table has columns for ids. + V23_1RoleMembersTableHasIDColumns + + // V23_1RoleMembersIDColumnsBackfilled is the version where the columns for + // ids in the role_members system table have been backfilled. + V23_1RoleMembersIDColumnsBackfilled + // ************************************************* // Step (1): Add new versions here. // Do not add new versions to a patch release. @@ -588,6 +596,14 @@ var rawVersionsSingleton = keyedVersions{ Key: V23_1CreateSystemJobInfoTable, Version: roachpb.Version{Major: 22, Minor: 2, Internal: 10}, }, + { + Key: V23_1RoleMembersTableHasIDColumns, + Version: roachpb.Version{Major: 22, Minor: 2, Internal: 12}, + }, + { + Key: V23_1RoleMembersIDColumnsBackfilled, + Version: roachpb.Version{Major: 22, Minor: 2, Internal: 14}, + }, // ************************************************* // Step (2): Add new versions here. diff --git a/pkg/sql/catalog/systemschema/system.go b/pkg/sql/catalog/systemschema/system.go index 36f2c1a1b3fa..c5e80738ee03 100644 --- a/pkg/sql/catalog/systemschema/system.go +++ b/pkg/sql/catalog/systemschema/system.go @@ -274,12 +274,17 @@ CREATE TABLE system.locations ( // role_members stores relationships between roles (role->role and role->user). RoleMembersTableSchema = ` CREATE TABLE system.role_members ( - "role" STRING NOT NULL, - "member" STRING NOT NULL, - "isAdmin" BOOL NOT NULL, + "role" STRING NOT NULL, + "member" STRING NOT NULL, + "isAdmin" BOOL NOT NULL, + role_id OID, + member_id OID, CONSTRAINT "primary" PRIMARY KEY ("role", "member"), INDEX ("role"), - INDEX ("member") + INDEX ("member"), + INDEX (role_id), + INDEX (member_id), + UNIQUE INDEX (role_id, member_id) );` // comments stores comments(database, table, column...). @@ -1579,6 +1584,8 @@ var ( {Name: "role", ID: 1, Type: types.String}, {Name: "member", ID: 2, Type: types.String}, {Name: "isAdmin", ID: 3, Type: types.Bool}, + {Name: "role_id", ID: 4, Type: types.Oid, Nullable: true}, + {Name: "member_id", ID: 5, Type: types.Oid, Nullable: true}, }, []descpb.ColumnFamilyDescriptor{ { @@ -1594,6 +1601,20 @@ var ( ColumnIDs: []descpb.ColumnID{3}, DefaultColumnID: 3, }, + { + Name: "fam_4_role_id", + ID: 4, + ColumnNames: []string{"role_id"}, + ColumnIDs: []descpb.ColumnID{4}, + DefaultColumnID: 4, + }, + { + Name: "fam_5_member_id", + ID: 5, + ColumnNames: []string{"member_id"}, + ColumnIDs: []descpb.ColumnID{5}, + DefaultColumnID: 5, + }, }, descpb.IndexDescriptor{ Name: "primary", @@ -1623,6 +1644,36 @@ var ( KeySuffixColumnIDs: []descpb.ColumnID{1}, Version: descpb.StrictIndexColumnIDGuaranteesVersion, }, + descpb.IndexDescriptor{ + Name: "role_members_role_id_idx", + ID: 4, + Unique: false, + KeyColumnNames: []string{"role_id"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{4}, + KeySuffixColumnIDs: []descpb.ColumnID{1, 2}, + Version: descpb.StrictIndexColumnIDGuaranteesVersion, + }, + descpb.IndexDescriptor{ + Name: "role_members_member_id_idx", + ID: 5, + Unique: false, + KeyColumnNames: []string{"member_id"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{5}, + KeySuffixColumnIDs: []descpb.ColumnID{1, 2}, + Version: descpb.StrictIndexColumnIDGuaranteesVersion, + }, + descpb.IndexDescriptor{ + Name: "role_members_role_id_member_id_key", + ID: 6, + Unique: true, + KeyColumnNames: []string{"role_id", "member_id"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC, catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{4, 5}, + KeySuffixColumnIDs: []descpb.ColumnID{1, 2}, + Version: descpb.StrictIndexColumnIDGuaranteesVersion, + }, )) // CommentsTable is the descriptor for the comments table. diff --git a/pkg/sql/catalog/systemschema_test/testdata/bootstrap b/pkg/sql/catalog/systemschema_test/testdata/bootstrap index 9dccf2274f67..b482e1a2fe35 100644 --- a/pkg/sql/catalog/systemschema_test/testdata/bootstrap +++ b/pkg/sql/catalog/systemschema_test/testdata/bootstrap @@ -152,11 +152,18 @@ CREATE TABLE public.role_members ( "role" STRING NOT NULL, member STRING NOT NULL, "isAdmin" BOOL NOT NULL, + role_id OID NULL, + member_id OID NULL, CONSTRAINT "primary" PRIMARY KEY ("role" ASC, member ASC), INDEX role_members_role_idx ("role" ASC), INDEX role_members_member_idx (member ASC), + INDEX role_members_role_id_idx (role_id ASC), + INDEX role_members_member_id_idx (member_id ASC), + UNIQUE INDEX role_members_role_id_member_id_key (role_id ASC, member_id ASC), FAMILY "primary" ("role", member), - FAMILY "fam_3_isAdmin" ("isAdmin") + FAMILY "fam_3_isAdmin" ("isAdmin"), + FAMILY fam_4_role_id (role_id), + FAMILY fam_5_member_id (member_id) ); CREATE TABLE public.comments ( type INT8 NOT NULL, @@ -427,7 +434,7 @@ schema_telemetry {"table":{"name":"replication_stats","id":27,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"zone_id","id":1,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"subzone_id","id":2,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"report_id","id":3,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"total_ranges","id":4,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"unavailable_ranges","id":5,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"under_replicated_ranges","id":6,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"over_replicated_ranges","id":7,"type":{"family":"IntFamily","width":64,"oid":20}}],"nextColumnId":8,"families":[{"name":"primary","columnNames":["zone_id","subzone_id","report_id","total_ranges","unavailable_ranges","under_replicated_ranges","over_replicated_ranges"],"columnIds":[1,2,3,4,5,6,7]}],"nextFamilyId":1,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["zone_id","subzone_id"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["report_id","total_ranges","unavailable_ranges","under_replicated_ranges","over_replicated_ranges"],"keyColumnIds":[1,2],"storeColumnIds":[3,4,5,6,7],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":2}} {"table":{"name":"reports_meta","id":28,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"id","id":1,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"generated","id":2,"type":{"family":"TimestampTZFamily","oid":1184}}],"nextColumnId":3,"families":[{"name":"primary","columnNames":["id","generated"],"columnIds":[1,2],"defaultColumnId":2}],"nextFamilyId":1,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["id"],"keyColumnDirections":["ASC"],"storeColumnNames":["generated"],"keyColumnIds":[1],"storeColumnIds":[2],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":2}} {"table":{"name":"role_id_seq","id":48,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"value","id":1,"type":{"family":"IntFamily","width":64,"oid":20}}],"families":[{"name":"primary","columnNames":["value"],"columnIds":[1],"defaultColumnId":1}],"primaryIndex":{"name":"primary","id":1,"version":4,"keyColumnNames":["value"],"keyColumnDirections":["ASC"],"keyColumnIds":[1],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{}},"privileges":{"users":[{"userProto":"admin","privileges":800,"withGrantOption":800},{"userProto":"root","privileges":800,"withGrantOption":800}],"ownerProto":"node","version":2},"formatVersion":3,"sequenceOpts":{"increment":"1","minValue":"100","maxValue":"2147483647","start":"100","sequenceOwner":{},"cacheSize":"1"},"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"}}} -{"table":{"name":"role_members","id":23,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"role","id":1,"type":{"family":"StringFamily","oid":25}},{"name":"member","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"isAdmin","id":3,"type":{"oid":16}}],"nextColumnId":4,"families":[{"name":"primary","columnNames":["role","member"],"columnIds":[1,2]},{"name":"fam_3_isAdmin","id":3,"columnNames":["isAdmin"],"columnIds":[3],"defaultColumnId":3}],"nextFamilyId":4,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["role","member"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["isAdmin"],"keyColumnIds":[1,2],"storeColumnIds":[3],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"indexes":[{"name":"role_members_role_idx","id":2,"version":3,"keyColumnNames":["role"],"keyColumnDirections":["ASC"],"keyColumnIds":[1],"keySuffixColumnIds":[2],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}},{"name":"role_members_member_idx","id":3,"version":3,"keyColumnNames":["member"],"keyColumnDirections":["ASC"],"keyColumnIds":[2],"keySuffixColumnIds":[1],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}}],"nextIndexId":4,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":2}} +{"table":{"name":"role_members","id":23,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"role","id":1,"type":{"family":"StringFamily","oid":25}},{"name":"member","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"isAdmin","id":3,"type":{"oid":16}},{"name":"role_id","id":4,"type":{"family":"OidFamily","oid":26},"nullable":true},{"name":"member_id","id":5,"type":{"family":"OidFamily","oid":26},"nullable":true}],"nextColumnId":6,"families":[{"name":"primary","columnNames":["role","member"],"columnIds":[1,2]},{"name":"fam_3_isAdmin","id":3,"columnNames":["isAdmin"],"columnIds":[3],"defaultColumnId":3},{"name":"fam_4_role_id","id":4,"columnNames":["role_id"],"columnIds":[4],"defaultColumnId":4},{"name":"fam_5_member_id","id":5,"columnNames":["member_id"],"columnIds":[5],"defaultColumnId":5}],"nextFamilyId":6,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["role","member"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["isAdmin","role_id","member_id"],"keyColumnIds":[1,2],"storeColumnIds":[3,4,5],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":2},"indexes":[{"name":"role_members_role_idx","id":2,"version":3,"keyColumnNames":["role"],"keyColumnDirections":["ASC"],"keyColumnIds":[1],"keySuffixColumnIds":[2],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}},{"name":"role_members_member_idx","id":3,"version":3,"keyColumnNames":["member"],"keyColumnDirections":["ASC"],"keyColumnIds":[2],"keySuffixColumnIds":[1],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}},{"name":"role_members_role_id_idx","id":4,"version":3,"keyColumnNames":["role_id"],"keyColumnDirections":["ASC"],"keyColumnIds":[4],"keySuffixColumnIds":[1,2],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}},{"name":"role_members_member_id_idx","id":5,"version":3,"keyColumnNames":["member_id"],"keyColumnDirections":["ASC"],"keyColumnIds":[5],"keySuffixColumnIds":[1,2],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}},{"name":"role_members_role_id_member_id_key","id":6,"unique":true,"version":3,"keyColumnNames":["role_id","member_id"],"keyColumnDirections":["ASC","ASC"],"keyColumnIds":[4,5],"keySuffixColumnIds":[1,2],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{},"constraintId":1}],"nextIndexId":7,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":3}} {"table":{"name":"role_options","id":33,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"username","id":1,"type":{"family":"StringFamily","oid":25}},{"name":"option","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"value","id":3,"type":{"family":"StringFamily","oid":25},"nullable":true},{"name":"user_id","id":4,"type":{"family":"OidFamily","oid":26}}],"nextColumnId":5,"families":[{"name":"primary","columnNames":["username","option","value","user_id"],"columnIds":[1,2,3,4]}],"nextFamilyId":1,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["username","option"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["value","user_id"],"keyColumnIds":[1,2],"storeColumnIds":[3,4],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"indexes":[{"name":"users_user_id_idx","id":2,"version":3,"keyColumnNames":["user_id"],"keyColumnDirections":["ASC"],"keyColumnIds":[4],"keySuffixColumnIds":[1,2],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}}],"nextIndexId":3,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":2}} {"table":{"name":"scheduled_jobs","id":37,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"schedule_id","id":1,"type":{"family":"IntFamily","width":64,"oid":20},"defaultExpr":"unique_rowid()"},{"name":"schedule_name","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"created","id":3,"type":{"family":"TimestampTZFamily","oid":1184},"defaultExpr":"now():::TIMESTAMPTZ"},{"name":"owner","id":4,"type":{"family":"StringFamily","oid":25}},{"name":"next_run","id":5,"type":{"family":"TimestampTZFamily","oid":1184},"nullable":true},{"name":"schedule_state","id":6,"type":{"family":"BytesFamily","oid":17},"nullable":true},{"name":"schedule_expr","id":7,"type":{"family":"StringFamily","oid":25},"nullable":true},{"name":"schedule_details","id":8,"type":{"family":"BytesFamily","oid":17},"nullable":true},{"name":"executor_type","id":9,"type":{"family":"StringFamily","oid":25}},{"name":"execution_args","id":10,"type":{"family":"BytesFamily","oid":17}}],"nextColumnId":11,"families":[{"name":"sched","columnNames":["schedule_id","next_run","schedule_state"],"columnIds":[1,5,6]},{"name":"other","id":1,"columnNames":["schedule_name","created","owner","schedule_expr","schedule_details","executor_type","execution_args"],"columnIds":[2,3,4,7,8,9,10]}],"nextFamilyId":2,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["schedule_id"],"keyColumnDirections":["ASC"],"storeColumnNames":["schedule_name","created","owner","next_run","schedule_state","schedule_expr","schedule_details","executor_type","execution_args"],"keyColumnIds":[1],"storeColumnIds":[2,3,4,5,6,7,8,9,10],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"indexes":[{"name":"next_run_idx","id":2,"version":3,"keyColumnNames":["next_run"],"keyColumnDirections":["ASC"],"keyColumnIds":[5],"keySuffixColumnIds":[1],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{}}],"nextIndexId":3,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":2}} {"table":{"name":"settings","id":6,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"name","id":1,"type":{"family":"StringFamily","oid":25}},{"name":"value","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"lastUpdated","id":3,"type":{"family":"TimestampFamily","oid":1114},"defaultExpr":"now():::TIMESTAMP"},{"name":"valueType","id":4,"type":{"family":"StringFamily","oid":25},"nullable":true}],"nextColumnId":5,"families":[{"name":"fam_0_name_value_lastUpdated_valueType","columnNames":["name","value","lastUpdated","valueType"],"columnIds":[1,2,3,4]}],"nextFamilyId":1,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["name"],"keyColumnDirections":["ASC"],"storeColumnNames":["value","lastUpdated","valueType"],"keyColumnIds":[1],"storeColumnIds":[2,3,4],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":480,"withGrantOption":480},{"userProto":"root","privileges":480,"withGrantOption":480}],"ownerProto":"node","version":2},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{"wallTime":"0"},"nextConstraintId":2}} diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go index 808de5ec8fdc..a177a7a6d77a 100644 --- a/pkg/sql/grant_role.go +++ b/pkg/sql/grant_role.go @@ -14,6 +14,8 @@ import ( "context" "strings" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/sql/decodeusername" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -22,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" @@ -166,35 +169,77 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR } func (n *GrantRoleNode) startExec(params runParams) error { - opName := "grant-role" - // Add memberships. Existing memberships are allowed. - // If admin option is false, we do not remove it from existing memberships. - memberStmt := `INSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ($1, $2, $3) ON CONFLICT ("role", "member")` - if n.adminOption { - // admin option: true, set "isAdmin" even if the membership exists. - memberStmt += ` DO UPDATE SET "isAdmin" = true` - } else { - // admin option: false, do not clear it from existing memberships. - memberStmt += ` DO NOTHING` - } - var rowsAffected int - for _, r := range n.roles { - for _, m := range n.members { - affected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( - params.ctx, - opName, - params.p.txn, - sessiondata.InternalExecutorOverride{User: username.RootUserName()}, - memberStmt, - r.Normalized(), m.Normalized(), n.adminOption, - ) - if err != nil { - return err + roleMembersHasIDs := params.p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.V23_1RoleMembersTableHasIDColumns) + if err := params.p.WithInternalExecutor(params.ctx, func(ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor) error { + // Add memberships. Existing memberships are allowed. + // If admin option is false, we do not remove it from existing memberships. + memberStmt := `INSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ($1, $2, $3) ON CONFLICT ("role", "member")` + if roleMembersHasIDs { + memberStmt = `INSERT INTO system.role_members ("role", "member", "isAdmin", "role_id", "member_id") VALUES ($1, $2, $3, $4, $5) ON CONFLICT ("role", "member")` + } + if n.adminOption { + // admin option: true, set "isAdmin" even if the membership exists. + memberStmt += ` DO UPDATE SET "isAdmin" = true` + } else { + // admin option: false, do not clear it from existing memberships. + memberStmt += ` DO NOTHING` + } + + // Get user IDs for both role and member if ID columns have been added. + var qargs []interface{} + if roleMembersHasIDs { + qargs = make([]interface{}, 5) + } else { + qargs = make([]interface{}, 3) + } + + qargs[2] = n.adminOption + for _, r := range n.roles { + qargs[0] = r.Normalized() + + if roleMembersHasIDs { + idRow, err := ie.QueryRowEx( + ctx, "get-user-id", txn, + sessiondata.NodeUserSessionDataOverride, + `SELECT user_id FROM system.users WHERE username = $1`, r.Normalized(), + ) + if err != nil { + return err + } + qargs[3] = tree.MustBeDOid(idRow[0]) } - rowsAffected += affected + for _, m := range n.members { + qargs[1] = m.Normalized() + + if roleMembersHasIDs { + idRow, err := ie.QueryRowEx( + ctx, "get-user-id", txn, + sessiondata.NodeUserSessionDataOverride, + `SELECT user_id FROM system.users WHERE username = $1`, m.Normalized(), + ) + if err != nil { + return err + } + qargs[4] = tree.MustBeDOid(idRow[0]) + } + + memberStmtRowsAffected, err := ie.ExecEx( + ctx, "grant-role", txn, + sessiondata.InternalExecutorOverride{User: username.RootUserName()}, + memberStmt, qargs..., + ) + if err != nil { + return err + } + rowsAffected += memberStmtRowsAffected + } } + + return nil + }); err != nil { + return err } // We need to bump the table version to trigger a refresh if anything changed. diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index e12bca3b057c..a076b128f3a8 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -1585,6 +1585,7 @@ system public 29_23_1_not_null system public 29_23_2_not_null system public role_members CHECK NO NO system public 29_23_3_not_null system public role_members CHECK NO NO system public primary system public role_members PRIMARY KEY NO NO +system public role_members_role_id_member_id_key system public role_members UNIQUE NO NO system public 29_33_1_not_null system public role_options CHECK NO NO system public 29_33_2_not_null system public role_options CHECK NO NO system public 29_33_4_not_null system public role_options CHECK NO NO @@ -1933,7 +1934,9 @@ system public replication_stats zone_id system public reports_meta id system public primary system public role_id_seq value system public primary system public role_members member system public primary +system public role_members member_id system public role_members_role_id_member_id_key system public role_members role system public primary +system public role_members role_id system public role_members_role_id_member_id_key system public role_options option system public primary system public role_options username system public primary system public scheduled_jobs schedule_id system public primary @@ -2179,7 +2182,9 @@ system public reports_meta id system public role_id_seq value 1 system public role_members isAdmin 3 system public role_members member 2 +system public role_members member_id 5 system public role_members role 1 +system public role_members role_id 4 system public role_options option 2 system public role_options user_id 4 system public role_options username 1 diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 0a90ae64776b..7c4cb01ebb9c 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -1117,6 +1117,9 @@ indexrelid indrelid indnatts indisunique indisprimary indisexclusion indim 543291288 23 1 false false false false false true false false true false 1 3403232968 0 2 NULL NULL 1 543291289 23 1 false false false false false true false false true false 2 3403232968 0 2 NULL NULL 1 543291291 23 2 true true false true false true false false true false 1 2 3403232968 3403232968 0 0 2 2 NULL NULL 2 +543291292 23 2 true false false true false true false false true false 4 5 0 0 0 0 2 2 NULL NULL 2 +543291294 23 1 false false false false false true false false true false 4 0 0 2 NULL NULL 1 +543291295 23 1 false false false false false true false false true false 5 0 0 2 NULL NULL 1 663840565 42 2 false false false false false true false false true false 2 3 0 0 0 0 2 2 NULL NULL 2 663840566 42 6 true true false true false true false false true false 1 2 3 4 5 6 0 0 0 0 3403232968 0 0 0 0 0 0 0 2 2 2 2 2 2 NULL NULL 6 803027558 26 3 true true false true false true false false true false 1 2 3 0 0 3403232968 0 0 0 2 2 2 NULL NULL 3 @@ -1187,6 +1190,10 @@ indexrelid operator_argument_type_oid operator_argument_position 543291289 0 1 543291291 0 1 543291291 0 2 +543291292 0 1 +543291292 0 2 +543291294 0 1 +543291295 0 1 663840565 0 1 663840565 0 2 663840566 0 1 diff --git a/pkg/sql/logictest/testdata/logic_test/role b/pkg/sql/logictest/testdata/logic_test/role index b8c200fcfc1c..ebb9269f81ee 100644 --- a/pkg/sql/logictest/testdata/logic_test/role +++ b/pkg/sql/logictest/testdata/logic_test/role @@ -157,12 +157,12 @@ user root statement ok GRANT testrole TO testuser -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -testrole testuser false +role member isAdmin role_id member_id +admin root true 2 1 +testrole testuser false 108 100 query TTB colnames SHOW GRANTS ON ROLE @@ -186,14 +186,14 @@ CREATE ROLE child_role; GRANT testrole to child_role; GRANT testuser TO child_role; -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -testrole child_role false -testrole testuser true -testuser child_role false +role member isAdmin role_id member_id +admin root true 2 1 +testrole child_role false 108 109 +testrole testuser true 108 100 +testuser child_role false 100 109 statement ok SET ROLE child_role @@ -247,14 +247,14 @@ SELECT crdb_internal.is_admin() true # Dropping users/roles deletes all their memberships. -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -admin testuser false -testrole testuser true -testrole testuser2 true +role member isAdmin role_id member_id +admin root true 2 1 +admin testuser false 2 100 +testrole testuser true 108 100 +testrole testuser2 true 108 107 query TTB colnames SHOW GRANTS ON ROLE @@ -313,21 +313,21 @@ DROP USER testuser statement ok CREATE USER testuser -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -testrole testuser2 true +role member isAdmin role_id member_id +admin root true 2 1 +testrole testuser2 true 108 107 statement ok DROP ROLE testrole -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin role_id member_id +admin root true 2 1 # Test cycle detection. statement error pq: admin cannot be a member of itself @@ -451,17 +451,17 @@ testuser user root -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -rolea roleb true -rolea rolee false -roleb rolec false -rolec roled false -rolec testuser false -roled testuser false +role member isAdmin role_id member_id +admin root true 2 1 +rolea roleb true 111 112 +rolea rolee false 111 115 +roleb rolec false 112 113 +rolec roled false 113 114 +rolec testuser false 113 110 +roled testuser false 114 110 statement ok DROP ROLE rolea @@ -469,12 +469,12 @@ DROP ROLE rolea statement ok DROP ROLE rolec -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -roled testuser false +role member isAdmin role_id member_id +admin root true 2 1 +roled testuser false 114 110 query TTT SHOW ROLES @@ -523,13 +523,13 @@ CREATE ROLE roleb statement ok GRANT rolea,roleb TO testuser WITH ADMIN OPTION -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -rolea testuser true -roleb testuser true +role member isAdmin role_id member_id +admin root true 2 1 +rolea testuser true 116 110 +roleb testuser true 117 110 user testuser @@ -538,15 +538,15 @@ GRANT rolea,roleb TO root WITH ADMIN OPTION user root -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -rolea root true -rolea testuser true -roleb root true -roleb testuser true +role member isAdmin role_id member_id +admin root true 2 1 +rolea root true 116 1 +rolea testuser true 116 110 +roleb root true 117 1 +roleb testuser true 117 110 query TTT colnames,rowsort SELECT * FROM information_schema.administrable_role_authorizations @@ -608,45 +608,45 @@ REVOKE roleb FROM root user root -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -rolea root true -rolea testuser false -roleb testuser true +role member isAdmin role_id member_id +admin root true 2 1 +rolea root true 116 1 +rolea testuser false 116 110 +roleb testuser true 117 110 statement ok REVOKE rolea, roleb FROM testuser, root -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin role_id member_id +admin root true 2 1 # Verify that GRANT/REVOKE are not sensitive to the case of role names. statement ok GRANT roLea,rOleB TO tEstUSER WITH ADMIN OPTION -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -rolea testuser true -roleb testuser true +role member isAdmin role_id member_id +admin root true 2 1 +rolea testuser true 116 110 +roleb testuser true 117 110 statement ok REVOKE roleA, roleB FROM TestUser -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin role_id member_id +admin root true 2 1 # Test privilege checks. @@ -677,13 +677,13 @@ GRANT admin TO newgroup user testuser -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin newgroup false -admin root true -newgroup testuser false +role member isAdmin role_id member_id +admin newgroup false 2 118 +admin root true 2 1 +newgroup testuser false 118 110 user root @@ -717,12 +717,12 @@ CREATE TABLE db2.s1.foo (k int); user root -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true -newgroup testuser false +role member isAdmin role_id member_id +admin root true 2 1 +newgroup testuser false 118 110 statement ok GRANT ALL ON DATABASE db2 TO newgroup @@ -802,11 +802,11 @@ DROP TABLE db2.foo statement ok DROP USER testuser -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin role_id member_id +admin root true 2 1 statement error cannot drop role/user newgroup: grants still exist on db2 DROP ROLE newgroup @@ -860,11 +860,11 @@ REVOKE admin FROM public statement ok CREATE USER testuser -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin role_id member_id +admin root true 2 1 user testuser @@ -936,11 +936,11 @@ CREATE ROLE IF NOT EXISTS anotherrolewithcreate2 CREATEROLE statement ok CREATE ROLE IF NOT EXISTS rolewithoutcreate2 WITH NOCREATEROLE -query TTB colnames +query TTBOO colnames SELECT * FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin role_id member_id +admin root true 2 1 user testuser diff --git a/pkg/sql/logictest/testdata/logic_test/system b/pkg/sql/logictest/testdata/logic_test/system index c213026ed03e..e5921ff1e29d 100644 --- a/pkg/sql/logictest/testdata/logic_test/system +++ b/pkg/sql/logictest/testdata/logic_test/system @@ -307,10 +307,11 @@ valueType STRING true NULL · {primary} false query TTBTTTB SHOW COLUMNS FROM system.role_members ---- -role STRING false NULL · {primary,role_members_member_idx,role_members_role_idx} false -member STRING false NULL · {primary,role_members_member_idx,role_members_role_idx} false -isAdmin BOOL false NULL · {primary} false - +role STRING false NULL · {primary,role_members_member_id_idx,role_members_member_idx,role_members_role_id_idx,role_members_role_id_member_id_key,role_members_role_idx} false +member STRING false NULL · {primary,role_members_member_id_idx,role_members_member_idx,role_members_role_id_idx,role_members_role_id_member_id_key,role_members_role_idx} false +isAdmin BOOL false NULL · {primary} false +role_id OID true NULL · {primary,role_members_role_id_idx,role_members_role_id_member_id_key} false +member_id OID true NULL · {primary,role_members_member_id_idx,role_members_role_id_member_id_key} false # Verify default privileges on system tables. query TTTB @@ -1082,10 +1083,10 @@ UPSERT INTO system.settings (name, value) VALUES ('somesetting', 'somevalueother user root -query TTB +query TTBOO SELECT * from system.role_members ---- -admin root true +admin root true 2 1 statement ok SET DATABASE = ""; diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index cd08cf41a671..e2979024492a 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -14,6 +14,7 @@ go_library( "permanent_upgrades.go", "precondition_before_starting_an_upgrade.go", "role_id_sequence_migration.go", + "role_members_ids_migration.go", "role_options_table_migration.go", "sampled_stmt_diagnostics_requests.go", "schema_changes.go", @@ -87,6 +88,7 @@ go_test( "helpers_test.go", "main_test.go", "precondition_before_starting_an_upgrade_external_test.go", + "role_members_ids_migration_test.go", "role_options_migration_test.go", "sampled_stmt_diagnostics_requests_test.go", "schema_changes_external_test.go", diff --git a/pkg/upgrade/upgrades/permanent_upgrades.go b/pkg/upgrade/upgrades/permanent_upgrades.go index 06533d40c1fe..ca360e7336c0 100644 --- a/pkg/upgrade/upgrades/permanent_upgrades.go +++ b/pkg/upgrade/upgrades/permanent_upgrades.go @@ -52,11 +52,15 @@ func addRootUser( // Upsert the role membership into the table. We intentionally override any existing entry. const upsertMembership = ` - UPSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ($1, $2, true) + UPSERT INTO system.role_members ("role", "member", "isAdmin", role_id, member_id) VALUES ($1, $2, true, $3, $4) ` _, err = deps.InternalExecutor.Exec( - ctx, "addRootToAdminRole", nil /* txn */, upsertMembership, username.AdminRole, username.RootUser) - return err + ctx, "addRootToAdminRole", nil /* txn */, upsertMembership, username.AdminRole, username.RootUser, username.AdminRoleID, username.RootUserID) + if err != nil { + return err + } + + return nil } func optInToDiagnosticsStatReporting( diff --git a/pkg/upgrade/upgrades/role_members_ids_migration.go b/pkg/upgrade/upgrades/role_members_ids_migration.go new file mode 100644 index 000000000000..67a325026abe --- /dev/null +++ b/pkg/upgrade/upgrades/role_members_ids_migration.go @@ -0,0 +1,118 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package upgrades + +import ( + "context" + + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/upgrade" +) + +const addIDColumnsToRoleMembersStmt = ` +ALTER TABLE system.role_members +ADD COLUMN IF NOT EXISTS role_id OID, +ADD COLUMN IF NOT EXISTS member_id OID +` + +const addIndexOnRoleIDToRoleMembersStmt = ` +CREATE INDEX IF NOT EXISTS role_members_role_id_idx ON system.role_members (role_id ASC) +` + +const addIndexOnMemberIDToRoleMembersStmt = ` +CREATE INDEX IF NOT EXISTS role_members_member_id_idx ON system.role_members (member_id ASC) +` + +const addUniqueIndexOnIDsToRoleMembersStmt = ` +CREATE UNIQUE INDEX IF NOT EXISTS role_members_role_id_member_id_key ON system.role_members (role_id ASC, member_id ASC) +` + +func alterSystemRoleMembersAddIDColumns( + ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, +) error { + for _, op := range []operation{ + { + name: "add-id-columns-system-role-members", + schemaList: []string{"role_id", "member_id"}, + query: addIDColumnsToRoleMembersStmt, + schemaExistsFn: columnExists, + }, + { + name: "add-role-id-index-system-role-members", + schemaList: []string{"role_members_role_id_idx"}, + query: addIndexOnRoleIDToRoleMembersStmt, + schemaExistsFn: hasIndex, + }, + { + name: "add-member-id-index-system-role-members", + schemaList: []string{"role_members_member_id_idx"}, + query: addIndexOnMemberIDToRoleMembersStmt, + schemaExistsFn: hasIndex, + }, + { + name: "add-id-unique-index-system-role-members", + schemaList: []string{"role_members_role_id_member_id_key"}, + query: addUniqueIndexOnIDsToRoleMembersStmt, + schemaExistsFn: hasIndex, + }, + } { + if err := migrateTable(ctx, cs, d, op, keys.RoleMembersTableID, systemschema.RoleMembersTable); err != nil { + return err + } + } + + return nil +} + +const backfillRoleIDColumnRoleMemberStmt = ` +UPDATE system.role_members +SET role_id = u.user_id +FROM system.users AS u +WHERE role_id IS NULL AND role = u.username +LIMIT 1000 +` + +const backfillMemberIDColumnRoleMembersStmt = ` +UPDATE system.role_members +SET member_id = u.user_id +FROM system.users AS u +WHERE member_id IS NULL AND member = u.username +LIMIT 1000 +` + +func backfillSystemRoleMembersIDColumns( + ctx context.Context, _ clusterversion.ClusterVersion, d upgrade.TenantDeps, +) error { + ie := d.InternalExecutorFactory.MakeInternalExecutorWithoutTxn() + for _, backfillStmt := range []string{backfillRoleIDColumnRoleMemberStmt, backfillMemberIDColumnRoleMembersStmt} { + for { + rowsAffected, err := ie.ExecEx(ctx, "backfill-id-columns-system-role-members", nil, /* txn */ + sessiondata.NodeUserSessionDataOverride, + backfillStmt, + ) + if err != nil { + return err + } + if rowsAffected == 0 { + break + } + } + } + + return nil +} + +// TODO(yang): Add a migration for making the ID columns not-null. Choosing to +// put this in a separate migration so that we can handle any BACKUP/RESTORE +// changes in a separate PR. diff --git a/pkg/upgrade/upgrades/role_members_ids_migration_test.go b/pkg/upgrade/upgrades/role_members_ids_migration_test.go new file mode 100644 index 000000000000..399914a470d8 --- /dev/null +++ b/pkg/upgrade/upgrades/role_members_ids_migration_test.go @@ -0,0 +1,219 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package upgrades_test + +import ( + "context" + "fmt" + "math/rand" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" + "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/cockroach/pkg/upgrade/upgrades" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/stretchr/testify/require" +) + +func TestRoleMembersIDMigrationNoUsers(t *testing.T) { + runTestRoleMembersIDMigration(t, 0) +} + +func TestRoleMembersIDMigration10Users(t *testing.T) { + runTestRoleMembersIDMigration(t, 10) +} + +func TestRoleMembersIDMigration1500Users(t *testing.T) { + skip.UnderRace(t) + skip.UnderStress(t) + runTestRoleMembersIDMigration(t, 1500) +} + +func runTestRoleMembersIDMigration(t *testing.T, numUsers int) { + defer leaktest.AfterTest(t)() + ctx := context.Background() + + settings := cluster.MakeTestingClusterSettingsWithVersions( + clusterversion.TestingBinaryVersion, + clusterversion.ByKey(clusterversion.V23_1RoleMembersTableHasIDColumns-1), + false, + ) + + tc := testcluster.StartTestCluster(t, 1 /* nodes */, base.TestClusterArgs{ + ServerArgs: base.TestServerArgs{ + Settings: settings, + Knobs: base.TestingKnobs{ + Server: &server.TestingKnobs{ + DisableAutomaticVersionUpgrade: make(chan struct{}), + BinaryVersionOverride: clusterversion.ByKey(clusterversion.V23_1RoleMembersTableHasIDColumns - 1), + }, + }, + }, + }) + defer tc.Stopper().Stop(ctx) + + db := tc.ServerConn(0) + defer db.Close() + tdb := sqlutils.MakeSQLRunner(db) + s := tc.Server(0) + + // Delete all rows from the system.role_members table. + tdb.Exec(t, "DELETE FROM system.role_members WHERE true") + tdb.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{}) + + // Inject the descriptor for the system.role_members table from before the + // ID columns were added. + upgrades.InjectLegacyTable(ctx, t, s, systemschema.RoleMembersTable, getTableDescForSystemRoleMembersTableBeforeIDCols) + + // Insert row that would have been added in a legacy startup migration. + tdb.Exec(t, `INSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ('admin', 'root', true)`) + tdb.CheckQueryResults(t, `SELECT * FROM system.role_members`, [][]string{ + {"admin", "root", "true"}, + }) + + // Create test users. + expectedNumRoleMembersRows := 1 + for i := 0; i < numUsers; i++ { + tdb.Exec(t, fmt.Sprintf("CREATE USER testuser%d", i)) + if i == 0 { + continue + } + // Randomly choose an earlier testuser to grant to the current testuser. + grantStmt := fmt.Sprintf("GRANT testuser%d to testuser%d", rand.Intn(i), i) + if rand.Intn(2) == 1 { + grantStmt += " WITH ADMIN OPTION" + } + tdb.Exec(t, grantStmt) + expectedNumRoleMembersRows += 1 + } + tdb.CheckQueryResults(t, "SELECT count(*) FROM system.role_members", [][]string{ + {fmt.Sprintf("%d", expectedNumRoleMembersRows)}, + }) + + // Run migrations. + _, err := tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V23_1RoleMembersTableHasIDColumns).String()) + require.NoError(t, err) + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.V23_1RoleMembersIDColumnsBackfilled).String()) + require.NoError(t, err) + + // Check some basic conditions on system.role_members after the migration is complete. + tdb.CheckQueryResults(t, "SELECT * FROM system.role_members WHERE role_id IS NULL OR member_id IS NULL", [][]string{}) + tdb.CheckQueryResults(t, "SELECT count(*) FROM system.role_members AS a JOIN system.users AS b on a.role = b.username AND a.role_id <> b.user_id", [][]string{{"0"}}) + tdb.CheckQueryResults(t, "SELECT count(*) FROM system.role_members AS a JOIN system.users AS b on a.member = b.username AND a.member_id <> b.user_id", [][]string{{"0"}}) + + tdb.CheckQueryResults(t, `SELECT * FROM system.role_members WHERE "role" = 'admin'`, [][]string{ + {"admin", "root", "true", "2", "1"}, + }) + + // Verify that the final schema matches the expected one. + expectedSchema := `CREATE TABLE public.role_members ( + "role" STRING NOT NULL, + member STRING NOT NULL, + "isAdmin" BOOL NOT NULL, + role_id OID NULL, + member_id OID NULL, + CONSTRAINT "primary" PRIMARY KEY ("role" ASC, member ASC), + INDEX role_members_role_idx ("role" ASC), + INDEX role_members_member_idx (member ASC), + INDEX role_members_role_id_idx (role_id ASC), + INDEX role_members_member_id_idx (member_id ASC), + UNIQUE INDEX role_members_role_id_member_id_key (role_id ASC, member_id ASC), + FAMILY "primary" ("role", member), + FAMILY "fam_3_isAdmin" ("isAdmin"), + FAMILY fam_4_role_id (role_id), + FAMILY fam_5_member_id (member_id) +)` + r := tc.Conns[0].QueryRow("SELECT create_statement FROM [SHOW CREATE TABLE system.role_members]") + var actualSchema string + require.NoError(t, r.Scan(&actualSchema)) + require.Equal(t, expectedSchema, actualSchema) +} + +func getTableDescForSystemRoleMembersTableBeforeIDCols() *descpb.TableDescriptor { + return &descpb.TableDescriptor{ + Name: "role_members", + ID: keys.RoleMembersTableID, + ParentID: keys.SystemDatabaseID, + UnexposedParentSchemaID: keys.PublicSchemaID, + Version: 1, + Columns: []descpb.ColumnDescriptor{ + {Name: "role", ID: 1, Type: types.String}, + {Name: "member", ID: 2, Type: types.String}, + {Name: "isAdmin", ID: 3, Type: types.Bool}, + }, + NextColumnID: 4, + Families: []descpb.ColumnFamilyDescriptor{ + { + Name: "primary", + ID: 0, + ColumnNames: []string{"role", "member"}, + ColumnIDs: []descpb.ColumnID{1, 2}, + }, + { + Name: "fam_3_isAdmin", + ID: 3, + ColumnNames: []string{"isAdmin"}, + ColumnIDs: []descpb.ColumnID{3}, + DefaultColumnID: 3, + }, + }, + NextFamilyID: 4, + PrimaryIndex: descpb.IndexDescriptor{ + Name: "primary", + ID: 1, + Unique: true, + KeyColumnNames: []string{"role", "member"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC, catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{1, 2}, + }, + Indexes: []descpb.IndexDescriptor{ + { + Name: "role_members_role_idx", + ID: 2, + Unique: false, + KeyColumnNames: []string{"role"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{1}, + KeySuffixColumnIDs: []descpb.ColumnID{2}, + Version: descpb.StrictIndexColumnIDGuaranteesVersion, + }, + { + Name: "role_members_member_idx", + ID: 3, + Unique: false, + KeyColumnNames: []string{"member"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{2}, + KeySuffixColumnIDs: []descpb.ColumnID{1}, + Version: descpb.StrictIndexColumnIDGuaranteesVersion, + }, + }, + NextIndexID: 4, + Privileges: catpb.NewCustomSuperuserPrivilegeDescriptor(privilege.ReadWriteData, username.NodeUserName()), + FormatVersion: descpb.InterleavedFormatVersion, + NextMutationID: 1, + NextConstraintID: 1, + } +} diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index 1e6dd38a687b..bb65bf8a856f 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -224,6 +224,16 @@ var upgrades = []upgradebase.Upgrade{ upgrade.NoPrecondition, systemJobInfoTableMigration, ), + upgrade.NewTenantUpgrade("add role_id and member_id columns to system.role_members", + toCV(clusterversion.V23_1RoleMembersTableHasIDColumns), + upgrade.NoPrecondition, + alterSystemRoleMembersAddIDColumns, + ), + upgrade.NewTenantUpgrade("backfill role_id and member_id columns in system.role_members", + toCV(clusterversion.V23_1RoleMembersIDColumnsBackfilled), + upgrade.NoPrecondition, + backfillSystemRoleMembersIDColumns, + ), } func init() {