From df2efecc84b1ccd052e0b9f9afe5d6f0e79e1f49 Mon Sep 17 00:00:00 2001 From: richardjcai Date: Wed, 10 Aug 2022 18:05:26 -0400 Subject: [PATCH] sql: add role_id, member_id columns to system.role_members Release note: None Release justification: Adding is to system.role_members in same way as system.users, last users table migration for this version. We don't use the IDs to delete or lookup users yet. --- .../settings/settings-for-tenants.txt | 2 +- docs/generated/settings/settings.html | 2 +- .../testdata/benchmark_expectations | 8 +- pkg/ccl/backupccl/backup_test.go | 10 +- pkg/ccl/backupccl/restore_job.go | 41 ++- .../backupccl/restore_old_versions_test.go | 38 +++ pkg/ccl/backupccl/system_schema.go | 108 ++++++- pkg/clusterversion/cockroach_versions.go | 22 ++ pkg/clusterversion/key_string.go | 7 +- pkg/server/testserver_http.go | 2 +- pkg/sql/catalog/systemschema/system.go | 64 +++- .../systemschema_test/testdata/bootstrap | 11 +- pkg/sql/grant_role.go | 39 ++- .../testdata/logic_test/information_schema | 11 + .../logictest/testdata/logic_test/pg_catalog | 9 + pkg/sql/logictest/testdata/logic_test/role | 64 ++-- pkg/sql/revoke_role.go | 3 + pkg/startupmigrations/migrations.go | 5 +- pkg/upgrade/upgrades/BUILD.bazel | 2 + .../upgrades/role_id_migration_test.go | 3 +- .../upgrades/role_members_table_migration.go | 190 ++++++++++++ .../role_members_table_migration_test.go | 286 ++++++++++++++++++ .../upgrades/system_privileges_test.go | 4 +- pkg/upgrade/upgrades/upgrades.go | 15 + 24 files changed, 872 insertions(+), 74 deletions(-) create mode 100644 pkg/upgrade/upgrades/role_members_table_migration.go create mode 100644 pkg/upgrade/upgrades/role_members_table_migration_test.go diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index 2160193d4c18..c1630a51437b 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -289,4 +289,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 22.1-58 set the active cluster version in the format '.' +version version 22.1-64 set the active cluster version in the format '.' diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index 64236ee0cdb6..b874915784df 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -220,6 +220,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. -versionversion22.1-58set the active cluster version in the format '.' +versionversion22.1-64set the active cluster version in the format '.' diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index c982902bd868..910d6704e8cb 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -48,10 +48,10 @@ exp,benchmark 19,DropView/drop_2_views 19,DropView/drop_3_views 14,Grant/grant_all_on_1_table -14,Grant/grant_all_on_2_tables -14,Grant/grant_all_on_3_tables -11,GrantRole/grant_1_role -13,GrantRole/grant_2_roles +15,Grant/grant_all_on_2_tables +15,Grant/grant_all_on_3_tables +13,GrantRole/grant_1_role +17,GrantRole/grant_2_roles 2,ORMQueries/activerecord_type_introspection_query 2,ORMQueries/django_table_introspection_1_table 2,ORMQueries/django_table_introspection_4_tables diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 31edd044ff92..0b4a9b91dda3 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -9459,11 +9459,11 @@ func TestBackupRestoreSystemUsers(t *testing.T) { {"test", "NULL", "false"}, {"test_role", "NULL", "true"}, }) - sqlDBRestore.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{ - {"admin", "app", "false"}, - {"admin", "root", "true"}, - {"app_role", "app", "false"}, - {"app_role", "test_role", "false"}, + sqlDBRestore.CheckQueryResults(t, `SELECT role, member, "isAdmin", role_id, member_id FROM system.role_members`, [][]string{ + {"admin", "app", "false", "2", "100"}, + {"admin", "root", "true", "2", "1"}, + {"app_role", "app", "false", "102", "100"}, + {"app_role", "test_role", "false", "102", "103"}, }) sqlDBRestore.CheckQueryResults(t, "SHOW USERS", [][]string{ {"admin", "", "{}"}, diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index 9d5344865160..175f18fdf896 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -2442,6 +2442,14 @@ func (r *restoreResumer) restoreSystemUsers( return nil } + hasIDColumnsQuery := fmt.Sprintf( + `SELECT EXISTS (SELECT 1 FROM [SHOW COLUMNS FROM %s] WHERE column_name = 'role_id')`, "crdb_temp_system.role_members") + row, err := executor.QueryRow(ctx, "has-id-column", txn, hasIDColumnsQuery) + if err != nil { + return err + } + backupSystemRoleMembersHsIDColumns := bool(tree.MustBeDBool(row[0])) + selectNonExistentRoleMembers := "SELECT * FROM crdb_temp_system.role_members temp_rm WHERE " + "NOT EXISTS (SELECT * FROM system.role_members rm WHERE temp_rm.role = rm.role AND temp_rm.member = rm.member)" roleMembers, err := executor.QueryBuffered(ctx, "get-role-members", @@ -2450,12 +2458,43 @@ func (r *restoreResumer) restoreSystemUsers( return err } + roleMembersHasIDColumn := r.execCfg.Settings.Version.IsActive(ctx, clusterversion.RoleMembersTableHasIDColumns) insertRoleMember := `INSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ($1, $2, $3)` + if roleMembersHasIDColumn { + insertRoleMember = `INSERT INTO system.role_members ("role", "member", "isAdmin", "role_id", "member_id") VALUES ($1, $2, $3, $4, $5)` + } + + var qargs []interface{} + if roleMembersHasIDColumn { + qargs = make([]interface{}, 5) + } else { + qargs = make([]interface{}, 3) + } + for _, roleMember := range roleMembers { // Only grant roles to users that don't currently exist, i.e., new users we just added if _, ok := newUsernames[roleMember[1].String()]; ok { + qargs[0], qargs[1], qargs[2] = roleMember[0], roleMember[1], roleMember[2] + + if roleMembersHasIDColumn && !backupSystemRoleMembersHsIDColumns { + // Populate the IDs by getting it from the system.users table. + roleID, err := getIDForUser(ctx, tree.MustBeDString(roleMember[0]), executor, txn) + if err != nil { + return err + } + memberID, err := getIDForUser(ctx, tree.MustBeDString(roleMember[1]), executor, txn) + if err != nil { + return err + } + qargs[3] = roleID + qargs[4] = memberID + } else { + qargs[3] = roleMember[3] + qargs[4] = roleMember[4] + } + if _, err = executor.Exec(ctx, "insert-non-existent-role-members", txn, insertRoleMember, - roleMember[0], roleMember[1], roleMember[2]); err != nil { + qargs...); err != nil { return err } } diff --git a/pkg/ccl/backupccl/restore_old_versions_test.go b/pkg/ccl/backupccl/restore_old_versions_test.go index 86136062677a..fdd1f0462cd8 100644 --- a/pkg/ccl/backupccl/restore_old_versions_test.go +++ b/pkg/ccl/backupccl/restore_old_versions_test.go @@ -1164,6 +1164,17 @@ func fullClusterRestoreUsersWithoutIDs(exportDir string) func(t *testing.T) { {"testuser4", "NULL", "false", "104"}, }) + sqlDB.CheckQueryResults(t, `SELECT role, member "isAdmin", role_id, member_id FROM system.role_members`, [][]string{ + {"admin", "root", "2", "1"}, + {"admin", "testuser4", "2", "104"}, + {"testrole", "testuser", "100", "101"}, + {"testrole", "testuser2", "100", "102"}, + {"testuser", "root", "101", "1"}, + {"testuser", "testuser2", "101", "102"}, + {"testuser", "testuser3", "101", "103"}, + {"testuser4", "testuser2", "104", "102"}, + }) + sqlDB.CheckQueryResults(t, `SELECT * FROM system.role_options`, [][]string{ {"testrole", "NOLOGIN", "NULL", "100"}, {"testuser", "CREATEROLE", "NULL", "101"}, @@ -1194,6 +1205,19 @@ func fullClusterRestoreUsersWithoutIDs(exportDir string) func(t *testing.T) { {"testuser4", "NULL", "false", "104"}, {"testuser5", "NULL", "false", "105"}, }) + + sqlDB.Exec(t, "GRANT testuser5 TO testuser") + sqlDB.CheckQueryResults(t, `SELECT role, member "isAdmin", role_id, member_id FROM system.role_members`, [][]string{ + {"admin", "root", "2", "1"}, + {"admin", "testuser4", "2", "104"}, + {"testrole", "testuser", "100", "101"}, + {"testrole", "testuser2", "100", "102"}, + {"testuser", "root", "101", "1"}, + {"testuser", "testuser2", "101", "102"}, + {"testuser", "testuser3", "101", "103"}, + {"testuser4", "testuser2", "104", "102"}, + {"testuser5", "testuser", "105", "101"}, + }) } } @@ -1226,6 +1250,20 @@ func restoreSystemUsersWithoutIDs(exportDir string) func(t *testing.T) { {"testuser4", "NULL", "false", "104"}, }) + // Note: in the backup, we grant `testuser` to `root`. + // However, we don't restore that row since we only restore + // memberships for users that did not previously exist in + // the cluster. + sqlDB.CheckQueryResults(t, `SELECT role, member "isAdmin", role_id, member_id FROM system.role_members`, [][]string{ + {"admin", "root", "2", "1"}, + {"admin", "testuser4", "2", "104"}, + {"testrole", "testuser", "100", "101"}, + {"testrole", "testuser2", "100", "102"}, + {"testuser", "testuser2", "101", "102"}, + {"testuser", "testuser3", "101", "103"}, + {"testuser4", "testuser2", "104", "102"}, + }) + // Verify that the next user we create uses the next biggest ID. sqlDB.Exec(t, "CREATE USER testuser5") diff --git a/pkg/ccl/backupccl/system_schema.go b/pkg/ccl/backupccl/system_schema.go index 1e9564199680..e2324f53d9b4 100644 --- a/pkg/ccl/backupccl/system_schema.go +++ b/pkg/ccl/backupccl/system_schema.go @@ -296,18 +296,9 @@ func roleOptionsRestoreFunc( option := tree.MustBeDString(it.Cur()[1]) val := it.Cur()[2] - var id int64 - if username == "root" { - id = 1 - } else if username == "admin" { - id = 2 - } else { - row, err := executor.QueryRow(ctx, `get-user-id`, txn, `SELECT user_id FROM system.users WHERE username = $1`, username) - if err != nil { - return err - } - oid := tree.MustBeDOid(row[0]) - id = int64(oid.Oid) + id, err := getIDForUser(ctx, username, executor, txn) + if err != nil { + return err } restoreQuery := fmt.Sprintf("INSERT INTO system.%s VALUES ($1, $2, $3, $4)", @@ -320,6 +311,97 @@ func roleOptionsRestoreFunc( return nil } +func roleMembersRestoreFunc( + ctx context.Context, + execCfg *sql.ExecutorConfig, + txn *kv.Txn, + systemTableName, tempTableName string, +) error { + if !execCfg.Settings.Version.IsActive(ctx, clusterversion.RoleMembersTableHasIDColumns) { + return defaultSystemTableRestoreFunc( + ctx, execCfg, txn, systemTableName, tempTableName, + ) + } + + executor := execCfg.InternalExecutor + hasIDColumnsQuery := fmt.Sprintf( + `SELECT EXISTS (SELECT 1 FROM [SHOW COLUMNS FROM %s] WHERE column_name = 'role_id')`, tempTableName) + row, err := executor.QueryRow(ctx, "has-id-column", txn, hasIDColumnsQuery) + if err != nil { + return err + } + hasIDColumn := tree.MustBeDBool(row[0]) + if hasIDColumn { + return defaultSystemTableRestoreFunc( + ctx, execCfg, txn, systemTableName, tempTableName, + ) + } + + deleteQuery := fmt.Sprintf("DELETE FROM system.%s WHERE true", systemTableName) + opName := systemTableName + "-data-deletion" + log.Eventf(ctx, "clearing data from system table %s with query %q", + systemTableName, deleteQuery) + + _, err = executor.Exec(ctx, opName, txn, deleteQuery) + if err != nil { + return errors.Wrapf(err, "deleting data from system.%s", systemTableName) + } + + it, err := executor.QueryIteratorEx(ctx, "query-system-role-members-in-backup", + txn, sessiondata.NodeUserSessionDataOverride, + fmt.Sprintf(`SELECT * FROM %s`, tempTableName)) + if err != nil { + return err + } + + for { + ok, err := it.Next(ctx) + if err != nil { + return err + } + if !ok { + break + } + + roleName := tree.MustBeDString(it.Cur()[0]) + memberName := tree.MustBeDString(it.Cur()[1]) + isAdmin := tree.MustBeDBool(it.Cur()[2]) + + roleID, err := getIDForUser(ctx, roleName, executor, txn) + if err != nil { + return err + } + memberID, err := getIDForUser(ctx, memberName, executor, txn) + if err != nil { + return err + } + + restoreQuery := fmt.Sprintf("INSERT INTO system.%s VALUES ($1, $2, $3, $4, $5)", + systemTableName) + opName = systemTableName + "-data-insert" + if _, err := executor.Exec(ctx, opName, txn, restoreQuery, roleName, memberName, isAdmin, roleID, memberID); err != nil { + return errors.Wrapf(err, "inserting data to system.%s", systemTableName) + } + } + return nil +} + +func getIDForUser( + ctx context.Context, name tree.DString, executor *sql.InternalExecutor, txn *kv.Txn, +) (int64, error) { + if name == "root" { + return 1, nil + } else if name == "admin" { + return 2, nil + } else { + row, err := executor.QueryRow(ctx, `get-user-id`, txn, `SELECT user_id FROM system.users WHERE username = $1`, name) + if err != nil { + return 0, err + } + return int64(tree.MustBeDOid(row[0]).Oid), nil + } +} + // When restoring the settings table, we want to make sure to not override the // version. func settingsRestoreFunc( @@ -400,6 +482,8 @@ var systemTableBackupConfiguration = map[string]systemBackupConfiguration{ }, systemschema.RoleMembersTable.GetName(): { shouldIncludeInClusterBackup: optInToClusterBackup, // No desc ID columns. + customRestoreFunc: roleMembersRestoreFunc, + restoreInOrder: 1, // Restore after system.users. }, systemschema.RoleOptionsTable.GetName(): { shouldIncludeInClusterBackup: optInToClusterBackup, // No desc ID columns. diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index f2c553097ba9..82c377862d1a 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -320,6 +320,16 @@ const ( // WaitedForDelRangeInGCJob corresponds to the migration which waits for // the GC jobs to adopt the use of DelRange with tombstones. WaitedForDelRangeInGCJob + // RoleMembersTableHasIDColumns is the version where the role + // members table has ids. + RoleMembersTableHasIDColumns + // RoleMembersIDColumnsAreBackfilled is the version where ids in the + // role options table are backfilled. + RoleMembersIDColumnsAreBackfilled + // SetRoleMembersUserIDColumnsNotNull is the version where the role + // members table id columns cannot be null. This is the final step + // of the system.role_members table migration. + SetRoleMembersUserIDColumnsNotNull // ************************************************* // Step (1): Add new versions here. @@ -542,6 +552,18 @@ var versionsSingleton = keyedVersions{ Key: WaitedForDelRangeInGCJob, Version: roachpb.Version{Major: 22, Minor: 1, Internal: 58}, }, + { + Key: RoleMembersTableHasIDColumns, + Version: roachpb.Version{Major: 22, Minor: 1, Internal: 60}, + }, + { + Key: RoleMembersIDColumnsAreBackfilled, + Version: roachpb.Version{Major: 22, Minor: 1, Internal: 62}, + }, + { + Key: SetRoleMembersUserIDColumnsNotNull, + Version: roachpb.Version{Major: 22, Minor: 1, Internal: 64}, + }, // ************************************************* // Step (2): Add new versions here. // Do not add new versions to a patch release. diff --git a/pkg/clusterversion/key_string.go b/pkg/clusterversion/key_string.go index e3ad7ef650dc..9c2de91013ac 100644 --- a/pkg/clusterversion/key_string.go +++ b/pkg/clusterversion/key_string.go @@ -54,11 +54,14 @@ func _() { _ = x[SetRoleOptionsUserIDColumnNotNull-43] _ = x[UseDelRangeInGCJob-44] _ = x[WaitedForDelRangeInGCJob-45] + _ = x[RoleMembersTableHasIDColumns-46] + _ = x[RoleMembersIDColumnsAreBackfilled-47] + _ = x[SetRoleMembersUserIDColumnsNotNull-48] } -const _Key_name = "V21_2Start22_1ProbeRequestPublicSchemasWithDescriptorsEnsureSpanConfigReconciliationEnsureSpanConfigSubscriptionEnableSpanConfigStoreEnablePebbleFormatVersionBlockPropertiesEnableLeaseHolderRemovalChangefeedIdlenessRowLevelTTLEnableNewStoreRebalancerClusterLocksVirtualTableAutoStatsTableSettingsSuperRegionsEnableNewChangefeedOptionsV22_1Start22_2LocalTimestampsPebbleFormatSplitUserKeysMarkedCompactedEnsurePebbleFormatVersionRangeKeysEnablePebbleFormatVersionRangeKeysTrigramInvertedIndexesRemoveGrantPrivilegeMVCCRangeTombstonesUpgradeSequenceToBeReferencedByIDSampledStmtDiagReqsAddSSTableTombstonesSystemPrivilegesTableEnablePredicateProjectionChangefeedAlterSystemSQLInstancesAddLocalitySystemExternalConnectionsTableAlterSystemStatementStatisticsAddIndexRecommendationsRoleIDSequenceAddSystemUserIDColumnSystemUsersIDColumnIsBackfilledSetSystemUsersUserIDColumnNotNullSQLSchemaTelemetryScheduledJobsSchemaChangeSupportsCreateFunctionDeleteRequestReturnKeyPebbleFormatPrePebblev1MarkedRoleOptionsTableHasIDColumnRoleOptionsIDColumnIsBackfilledSetRoleOptionsUserIDColumnNotNullUseDelRangeInGCJobWaitedForDelRangeInGCJob" +const _Key_name = "V21_2Start22_1ProbeRequestPublicSchemasWithDescriptorsEnsureSpanConfigReconciliationEnsureSpanConfigSubscriptionEnableSpanConfigStoreEnablePebbleFormatVersionBlockPropertiesEnableLeaseHolderRemovalChangefeedIdlenessRowLevelTTLEnableNewStoreRebalancerClusterLocksVirtualTableAutoStatsTableSettingsSuperRegionsEnableNewChangefeedOptionsV22_1Start22_2LocalTimestampsPebbleFormatSplitUserKeysMarkedCompactedEnsurePebbleFormatVersionRangeKeysEnablePebbleFormatVersionRangeKeysTrigramInvertedIndexesRemoveGrantPrivilegeMVCCRangeTombstonesUpgradeSequenceToBeReferencedByIDSampledStmtDiagReqsAddSSTableTombstonesSystemPrivilegesTableEnablePredicateProjectionChangefeedAlterSystemSQLInstancesAddLocalitySystemExternalConnectionsTableAlterSystemStatementStatisticsAddIndexRecommendationsRoleIDSequenceAddSystemUserIDColumnSystemUsersIDColumnIsBackfilledSetSystemUsersUserIDColumnNotNullSQLSchemaTelemetryScheduledJobsSchemaChangeSupportsCreateFunctionDeleteRequestReturnKeyPebbleFormatPrePebblev1MarkedRoleOptionsTableHasIDColumnRoleOptionsIDColumnIsBackfilledSetRoleOptionsUserIDColumnNotNullUseDelRangeInGCJobWaitedForDelRangeInGCJobRoleMembersTableHasIDColumnsRoleMembersIDColumnsAreBackfilledSetRoleMembersUserIDColumnsNotNull" -var _Key_index = [...]uint16{0, 5, 14, 26, 54, 84, 112, 133, 173, 197, 215, 226, 250, 274, 296, 308, 334, 339, 348, 363, 403, 437, 471, 493, 513, 532, 565, 584, 604, 625, 660, 694, 724, 777, 791, 812, 843, 876, 907, 941, 963, 992, 1019, 1050, 1083, 1101, 1125} +var _Key_index = [...]uint16{0, 5, 14, 26, 54, 84, 112, 133, 173, 197, 215, 226, 250, 274, 296, 308, 334, 339, 348, 363, 403, 437, 471, 493, 513, 532, 565, 584, 604, 625, 660, 694, 724, 777, 791, 812, 843, 876, 907, 941, 963, 992, 1019, 1050, 1083, 1101, 1125, 1153, 1186, 1220} func (i Key) String() string { if i < 0 || i >= Key(len(_Key_index)-1) { diff --git a/pkg/server/testserver_http.go b/pkg/server/testserver_http.go index ef6f0aaf163b..2d2144c6fde9 100644 --- a/pkg/server/testserver_http.go +++ b/pkg/server/testserver_http.go @@ -155,7 +155,7 @@ func (ts *httpTestServer) createAuthUser(userName username.SQLUsername, isAdmin if _, err := ts.t.sqlServer.internalExecutor.ExecEx(context.TODO(), "grant-admin", nil, sessiondata.InternalExecutorOverride{User: username.RootUserName()}, - "INSERT INTO system.role_members (role, member, \"isAdmin\") VALUES ('admin', $1, true)", userName.Normalized(), + fmt.Sprintf(`GRANT admin TO %s`, userName.Normalized()), ); err != nil { return err } diff --git a/pkg/sql/catalog/systemschema/system.go b/pkg/sql/catalog/systemschema/system.go index 574cb280a370..1eb4aa86e4a9 100644 --- a/pkg/sql/catalog/systemschema/system.go +++ b/pkg/sql/catalog/systemschema/system.go @@ -258,13 +258,18 @@ 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, - CONSTRAINT "primary" PRIMARY KEY ("role", "member"), - INDEX ("role"), - INDEX ("member") -);` + role STRING NOT NULL, + member STRING NOT NULL, + "isAdmin" BOOL NOT NULL, + role_id OID NOT NULL, + member_id OID NOT NULL, + CONSTRAINT "primary" PRIMARY KEY (role, member), + INDEX (role), + INDEX (member), + INDEX (role_id), + INDEX (member_id), + UNIQUE INDEX (role, member, role_id, member_id) +)` // comments stores comments(database, table, column...). CommentsTableSchema = ` @@ -1465,6 +1470,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}, + {Name: "member_id", ID: 5, Type: types.Oid}, }, []descpb.ColumnFamilyDescriptor{ { @@ -1480,6 +1487,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", @@ -1509,6 +1530,35 @@ 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_member_role_id_member_id_key", + ID: 6, + Unique: true, + KeyColumnNames: []string{"role", "member", "role_id", "member_id"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC, catpb.IndexColumn_ASC, catpb.IndexColumn_ASC, catpb.IndexColumn_ASC}, + KeyColumnIDs: []descpb.ColumnID{1, 2, 4, 5}, + 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 56bceb8f4a75..02071a2f48f1 100644 --- a/pkg/sql/catalog/systemschema_test/testdata/bootstrap +++ b/pkg/sql/catalog/systemschema_test/testdata/bootstrap @@ -148,11 +148,18 @@ CREATE TABLE public.role_members ( "role" STRING NOT NULL, member STRING NOT NULL, "isAdmin" BOOL NOT NULL, + role_id OID NOT NULL, + member_id OID NOT 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_member_role_id_member_id_key ("role" ASC, member ASC, 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, @@ -413,7 +420,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}},{"name":"member_id","id":5,"type":{"family":"OidFamily","oid":26}}],"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_member_role_id_member_id_key","id":6,"unique":true,"version":3,"keyColumnNames":["role","member","role_id","member_id"],"keyColumnDirections":["ASC","ASC","ASC","ASC"],"keyColumnIds":[1,2,4,5],"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..8475ec36a794 100644 --- a/pkg/sql/grant_role.go +++ b/pkg/sql/grant_role.go @@ -14,6 +14,7 @@ import ( "context" "strings" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/sql/decodeusername" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -169,7 +170,11 @@ 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")` + withID := params.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.RoleMembersTableHasIDColumns) + memberStmt := `INSERT INTO system.role_members ("role", "member", "isAdmin", "role_id", "member_id") VALUES ($1, $2, $3, $4, $5) ON CONFLICT ("role", "member", "role_id", "member_id")` + if !withID { + 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` @@ -178,16 +183,46 @@ func (n *GrantRoleNode) startExec(params runParams) error { memberStmt += ` DO NOTHING` } + var qargs []interface{} + if withID { + qargs = make([]interface{}, 5) + } else { + qargs = make([]interface{}, 3) + } var rowsAffected int for _, r := range n.roles { + qargs[0] = r.Normalized() + qargs[2] = n.adminOption + if withID { + idRow, err := params.p.ExecCfg().InternalExecutor.QueryRowEx( + params.ctx, `get-user-id`, params.p.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]) + } + for _, m := range n.members { + qargs[1] = m.Normalized() + if withID { + idRow, err := params.p.ExecCfg().InternalExecutor.QueryRowEx( + params.ctx, `get-user-id`, params.p.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]) + } 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, + qargs..., ) if err != nil { return err diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 9f9d5e9aff4f..c67ed1716791 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -1576,7 +1576,10 @@ system public primary system public 630200280_23_1_not_null system public role_members CHECK NO NO system public 630200280_23_2_not_null system public role_members CHECK NO NO system public 630200280_23_3_not_null system public role_members CHECK NO NO +system public 630200280_23_4_not_null system public role_members CHECK NO NO +system public 630200280_23_5_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_member_role_id_member_id_key system public role_members UNIQUE NO NO system public 630200280_33_1_not_null system public role_options CHECK NO NO system public 630200280_33_2_not_null system public role_options CHECK NO NO system public 630200280_33_4_not_null system public role_options CHECK NO NO @@ -1739,6 +1742,8 @@ system public 630200280_21_4_not_null system public 630200280_23_1_not_null role IS NOT NULL system public 630200280_23_2_not_null member IS NOT NULL system public 630200280_23_3_not_null isAdmin IS NOT NULL +system public 630200280_23_4_not_null role_id IS NOT NULL +system public 630200280_23_5_not_null member_id IS NOT NULL system public 630200280_24_1_not_null type IS NOT NULL system public 630200280_24_2_not_null object_id IS NOT NULL system public 630200280_24_3_not_null sub_id IS NOT NULL @@ -1915,7 +1920,11 @@ 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 system public role_members_role_member_role_id_member_id_key +system public role_members member_id system public role_members_role_member_role_id_member_id_key system public role_members role system public primary +system public role_members role system public role_members_role_member_role_id_member_id_key +system public role_members role_id system public role_members_role_member_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 @@ -2154,7 +2163,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 5aa73fd783fb..25ae661393d7 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -1112,6 +1112,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 4 true false false true false true false false true false 1 2 4 5 3403232968 3403232968 0 0 0 0 0 0 2 2 2 2 NULL NULL 4 +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 @@ -1179,6 +1182,12 @@ indexrelid operator_argument_type_oid operator_argument_position 543291289 0 1 543291291 0 1 543291291 0 2 +543291292 0 1 +543291292 0 2 +543291292 0 3 +543291292 0 4 +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 635ad545450d..44f7a756b9a0 100644 --- a/pkg/sql/logictest/testdata/logic_test/role +++ b/pkg/sql/logictest/testdata/logic_test/role @@ -158,7 +158,7 @@ statement ok GRANT testrole TO testuser query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -187,7 +187,7 @@ GRANT testrole to child_role; GRANT testuser TO child_role; query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -248,7 +248,7 @@ true # Dropping users/roles deletes all their memberships. query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -314,7 +314,7 @@ statement ok CREATE USER testuser query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -324,10 +324,10 @@ statement ok DROP ROLE testrole query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin +admin root true # Test cycle detection. statement error pq: admin cannot be a member of itself @@ -452,7 +452,7 @@ testuser user root query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -470,11 +470,11 @@ statement ok DROP ROLE rolec query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true -roled testuser false +roled testuser false query TTT SHOW ROLES @@ -524,7 +524,7 @@ statement ok GRANT rolea,roleb TO testuser WITH ADMIN OPTION query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -539,7 +539,7 @@ GRANT rolea,roleb TO root WITH ADMIN OPTION user root query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -609,7 +609,7 @@ REVOKE roleb FROM root user root query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -621,10 +621,10 @@ statement ok REVOKE rolea, roleb FROM testuser, root query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin +admin root true # Verify that GRANT/REVOKE are not sensitive to the case of role names. @@ -632,7 +632,7 @@ statement ok GRANT roLea,rOleB TO tEstUSER WITH ADMIN OPTION query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -643,10 +643,10 @@ statement ok REVOKE roleA, roleB FROM TestUser query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin +admin root true # Test privilege checks. @@ -678,7 +678,7 @@ GRANT admin TO newgroup user testuser query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin newgroup false @@ -706,7 +706,7 @@ CREATE SCHEMA db2.s1; user testuser statement error user testuser does not have SELECT privilege on relation role_members -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members # This doesn't error because testuser has CREATE privilege on db2.public. statement ok @@ -718,7 +718,7 @@ CREATE TABLE db2.s1.foo (k int); user root query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- role member isAdmin admin root true @@ -803,10 +803,10 @@ statement ok DROP USER testuser query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin +admin root true statement error cannot drop role/user newgroup: grants still exist on db2 DROP ROLE newgroup @@ -861,10 +861,10 @@ statement ok CREATE USER testuser query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin +admin root true user testuser @@ -937,10 +937,10 @@ statement ok CREATE ROLE IF NOT EXISTS rolewithoutcreate2 WITH NOCREATEROLE query TTB colnames -SELECT * FROM system.role_members +SELECT role, member, "isAdmin" FROM system.role_members ---- -role member isAdmin -admin root true +role member isAdmin +admin root true user testuser diff --git a/pkg/sql/revoke_role.go b/pkg/sql/revoke_role.go index f77f4259bd60..8119803185e0 100644 --- a/pkg/sql/revoke_role.go +++ b/pkg/sql/revoke_role.go @@ -111,6 +111,9 @@ func (n *RevokeRoleNode) startExec(params runParams) error { opName := "revoke-role" var memberStmt string + + // Note that we don't use role ids to delete here. This is okay + // in 22.2 because role members and role ids map 1:1. if n.adminOption { // ADMIN OPTION FOR is specified, we don't remove memberships just remove the admin option. memberStmt = `UPDATE system.role_members SET "isAdmin" = false WHERE "role" = $1 AND "member" = $2` diff --git a/pkg/startupmigrations/migrations.go b/pkg/startupmigrations/migrations.go index 52e634e958fe..312f53c61d39 100644 --- a/pkg/startupmigrations/migrations.go +++ b/pkg/startupmigrations/migrations.go @@ -796,10 +796,11 @@ func addAdminRole(ctx context.Context, r runner) error { func addRootToAdminRole(ctx context.Context, r runner) error { // Upsert the role membership into the table. We intentionally override any existing entry. const upsertAdminStmt = ` - 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) ` return r.execAsRootWithRetry( - ctx, "addRootToAdminRole", upsertAdminStmt, username.AdminRole, username.RootUser) + ctx, "addRootToAdminRole", upsertAdminStmt, + username.AdminRole, username.RootUser, username.AdminRoleID, username.RootUserID) } func disallowPublicUserOrRole(ctx context.Context, r runner) error { diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 10efa66149bb..959cd06fba14 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -11,6 +11,7 @@ go_library( "precondition_before_starting_an_upgrade.go", "remove_grant_migration.go", "role_id_sequence_migration.go", + "role_members_table_migration.go", "role_options_table_migration.go", "sampled_stmt_diagnostics_requests.go", "schema_changes.go", @@ -72,6 +73,7 @@ go_test( "precondition_before_starting_an_upgrade_external_test.go", "remove_grant_migration_test.go", "role_id_migration_test.go", + "role_members_table_migration_test.go", "role_options_migration_test.go", "sampled_stmt_diagnostics_requests_test.go", "system_privileges_test.go", diff --git a/pkg/upgrade/upgrades/role_id_migration_test.go b/pkg/upgrade/upgrades/role_id_migration_test.go index c6f2e81fa857..313ba4cbc1ab 100644 --- a/pkg/upgrade/upgrades/role_id_migration_test.go +++ b/pkg/upgrade/upgrades/role_id_migration_test.go @@ -66,7 +66,8 @@ func runTestRoleIDMigration(t *testing.T, numUsers int) { // Delete system.role_id_seq. tdb.Exec(t, `INSERT INTO system.users VALUES ('node', '', false, 0)`) - tdb.Exec(t, `GRANT node TO root`) + tdb.Exec(t, `INSERT INTO system.role_members VALUES ($1, $2, false, $3, $4)`, + username.NodeUser, username.RootUser, username.NodeUserID, username.RootUserID) tdb.Exec(t, `DROP SEQUENCE system.role_id_seq`) tdb.Exec(t, `REVOKE node FROM root`) diff --git a/pkg/upgrade/upgrades/role_members_table_migration.go b/pkg/upgrade/upgrades/role_members_table_migration.go new file mode 100644 index 000000000000..df187fcb8048 --- /dev/null +++ b/pkg/upgrade/upgrades/role_members_table_migration.go @@ -0,0 +1,190 @@ +// 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" + "fmt" + "strings" + + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/jobs" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/upgrade" +) + +const addRoleIDColumnToRoleMembers = ` +ALTER TABLE system.role_members ADD COLUMN IF NOT EXISTS "role_id" OID +` + +const addMemberIDColumnToRoleMembers = ` +ALTER TABLE system.role_members ADD COLUMN IF NOT EXISTS "member_id" OID +` + +const addIndexToRoleMembersRoleIDColumn = ` +CREATE INDEX role_members_role_id_idx ON system.role_members ("role_id" ASC) +` + +const addIndexToRoleMembersMemberIDColumn = ` +CREATE INDEX role_members_member_id_idx ON system.role_members ("member_id" ASC) +` + +const addUniqueIndexToRoleMembers = ` +CREATE UNIQUE INDEX role_members_role_member_role_id_member_id_key ON system.role_members ("role", "member", "role_id", "member_id") +` + +const updateRoleIDColumnRoleMembersSetNotNull = ` +ALTER TABLE system.role_members ALTER COLUMN "role_id" SET NOT NULL +` + +const updateMemberIDColumnRoleMembersSetNotNull = ` +ALTER TABLE system.role_members ALTER COLUMN "member_id" SET NOT NULL +` + +func alterSystemRoleMembersAddColumnsAndIndexes( + ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, _ *jobs.Job, +) error { + for _, op := range []operation{ + { + name: "add-system-role-members-role-id-column", + schemaList: []string{"role_id"}, + query: addRoleIDColumnToRoleMembers, + schemaExistsFn: hasColumn, + }, + { + name: "add-system-role-members-member-id-column", + schemaList: []string{"member_id"}, + query: addMemberIDColumnToRoleMembers, + schemaExistsFn: hasColumn, + }, + { + name: "alter-system-role-members-add-index-on-role-id", + schemaList: []string{"role_members_role_id_idx"}, + query: addIndexToRoleMembersRoleIDColumn, + schemaExistsFn: hasIndex, + }, + { + name: "alter-system-role-members-add-index-on-member-id", + schemaList: []string{"role_members_member_id_idx"}, + query: addIndexToRoleMembersMemberIDColumn, + schemaExistsFn: hasIndex, + }, + { + name: "alter-system-role-members-add-index", + schemaList: []string{"role_members_role_member_role_id_member_id_key"}, + query: addUniqueIndexToRoleMembers, + schemaExistsFn: hasIndex, + }, + } { + if err := migrateTable(ctx, cs, d, op, keys.RoleMembersTableID, systemschema.RoleMembersTable); err != nil { + return err + } + } + + return nil +} + +func backfillSystemRoleMembersIDColumns( + ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, _ *jobs.Job, +) error { + row, err := d.InternalExecutor.QueryRowEx(ctx, `get-num-null-user-ids`, nil, sessiondata.NodeUserSessionDataOverride, + `SELECT count(1) FROM system.role_members WHERE role_id IS NULL OR member_id IS NULL`) + if err != nil { + return err + } + numUsersToUpdate := int(tree.MustBeDInt(row[0])) + const batchSize = 10000 + updateUserIDs := fmt.Sprintf(` +UPDATE + system.role_members +SET + role_id = temp.role_id, member_id = temp.member_id +FROM + ( + SELECT + * + FROM + ( + SELECT + role, + member, + u1.user_id AS role_id, + u2.user_id AS member_id + FROM + system.role_members AS rm + JOIN system.users AS u1 ON + rm.role = u1.username + JOIN system.users AS u2 ON + rm.member = u2.username + ) + ) + AS temp +WHERE + system.role_members.role = temp.role + AND system.role_members.member = temp.member + AND system.role_members.role_id IS NULL +LIMIT %d; +`, batchSize) + for i := 0; i < numUsersToUpdate; i += batchSize { + _, err = d.InternalExecutor.ExecEx(ctx, "update-role-ids", nil, + sessiondata.NodeUserSessionDataOverride, updateUserIDs) + if err != nil { + return err + } + } + return nil +} + +func setSystemRoleMembersIDColumnsNotNull( + ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, _ *jobs.Job, +) error { + for _, op := range []operation{ + { + name: "alter-system-role-members-role-id-column-not-null", + schemaList: []string{"role_id"}, + query: updateRoleIDColumnRoleMembersSetNotNull, + schemaExistsFn: func(storedTable, _ catalog.TableDescriptor, colName string) (bool, error) { + storedCol, err := storedTable.FindColumnWithName(tree.Name(colName)) + if err != nil { + if strings.Contains(err.Error(), "does not exist") { + return false, nil + } + return false, err + } + return !storedCol.IsNullable(), nil + }, + }, + { + name: "alter-system-role-members-member-id-column-not-null", + schemaList: []string{"member_id"}, + query: updateMemberIDColumnRoleMembersSetNotNull, + schemaExistsFn: func(storedTable, _ catalog.TableDescriptor, colName string) (bool, error) { + storedCol, err := storedTable.FindColumnWithName(tree.Name(colName)) + if err != nil { + if strings.Contains(err.Error(), "does not exist") { + return false, nil + } + return false, err + } + return !storedCol.IsNullable(), nil + }, + }, + } { + if err := migrateTable(ctx, cs, d, op, keys.RoleMembersTableID, systemschema.RoleMembersTable); err != nil { + return err + } + } + return nil +} diff --git a/pkg/upgrade/upgrades/role_members_table_migration_test.go b/pkg/upgrade/upgrades/role_members_table_migration_test.go new file mode 100644 index 000000000000..cabb4905faf5 --- /dev/null +++ b/pkg/upgrade/upgrades/role_members_table_migration_test.go @@ -0,0 +1,286 @@ +// 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" + "sync" + "sync/atomic" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/keys" + "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/sem/catconstants" + "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 runTestRoleMembersMigration(t *testing.T, numUsers int) { + defer leaktest.AfterTest(t)() + ctx := context.Background() + + settings := cluster.MakeTestingClusterSettingsWithVersions( + clusterversion.TestingBinaryVersion, + clusterversion.ByKey(clusterversion.RoleMembersTableHasIDColumns-1), + false, + ) + + tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ + ServerArgs: base.TestServerArgs{ + Settings: settings, + Knobs: base.TestingKnobs{ + Server: &server.TestingKnobs{ + DisableAutomaticVersionUpgrade: make(chan struct{}), + BinaryVersionOverride: clusterversion.ByKey(clusterversion.RoleMembersTableHasIDColumns - 1), + }, + }, + }, + }) + + defer tc.Stopper().Stop(ctx) + + db := tc.ServerConn(0) + defer db.Close() + tdb := sqlutils.MakeSQLRunner(db) + s := tc.Server(0) + + tdb.Exec(t, `DELETE FROM system.role_members WHERE true`) + + // Inject the old copy of the descriptor. + upgrades.InjectLegacyTable(ctx, t, s, systemschema.UsersTable, + getDeprecatedSystemMembersOptionsTable) + + // Always create user 0. + tdb.Exec(t, "CREATE USER testuser0") + + if numUsers > 100 { + var id uint32 + // We're not guaranteed that the ID of the first created + // user is 100. + idRow := tdb.QueryRow(t, `SELECT last_value - 1 FROM system.role_id_seq`) + idRow.Scan(&id) + var wg sync.WaitGroup + wg.Add(100) + for i := 0; i < 100; i++ { + go func(capI int) { + defer wg.Done() + for j := 0; j < numUsers/100; j++ { + val := atomic.AddUint32(&id, 1) + tdb.Exec(t, fmt.Sprintf(`INSERT INTO system.users VALUES ('testuser%dx%d', '', false, %d)`, capI, j, val)) + if j > 0 { + tdb.Exec(t, fmt.Sprintf( + `INSERT INTO system.role_members VALUES ('testuser%dx%d', 'testuser%dx%d', %t)`, + capI, j, capI, j-1, j%2 == 0), + ) + } + } + }(i) + } + wg.Wait() + tdb.Exec(t, fmt.Sprintf(`SELECT setval('system.role_id_seq', %d)`, id+1)) + } else { + for i := 1; i < numUsers; i++ { + tdb.Exec(t, fmt.Sprintf(`CREATE USER testuser%d`, i)) + tdb.Exec(t, fmt.Sprintf(`GRANT testuser%d to testuser%d`, i, i-1)) + } + } + + var originalCount int + countRow := tdb.QueryRow(t, `SELECT count(*) FROM system.role_members`) + countRow.Scan(&originalCount) + + _, err := tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.RoleMembersTableHasIDColumns).String()) + require.NoError(t, err) + + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.RoleMembersIDColumnsAreBackfilled).String()) + require.NoError(t, err) + + _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, + clusterversion.ByKey(clusterversion.SetRoleMembersUserIDColumnsNotNull).String()) + require.NoError(t, err) + + 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 a JOIN system.users b ON a.role = b.username AND a.role_id <> b.user_id`, [][]string{{"0"}}) + tdb.CheckQueryResults(t, `SELECT count(*) FROM system.role_members a JOIN system.users b ON a.member = b.username AND a.member_id <> b.user_id`, [][]string{{"0"}}) + + tdb.CheckQueryResults(t, `SELECT count(*) FROM system.role_members`, [][]string{ + {fmt.Sprintf("%d", originalCount)}, + }) + + tdb.Exec(t, `CREATE USER testuser_last`) + tdb.Exec(t, `GRANT testuser_last TO testuser0`) + + tdb.CheckQueryResults(t, `SELECT count(*) FROM system.role_members a JOIN system.users b ON a.role = b.username AND a.role_id <> b.user_id`, [][]string{{"0"}}) + tdb.CheckQueryResults(t, `SELECT count(*) FROM system.role_members a JOIN system.users b ON a.member = b.username AND a.member_id <> b.user_id`, [][]string{{"0"}}) + + tdb.CheckQueryResults(t, `SELECT count(*) FROM system.role_members`, [][]string{ + {fmt.Sprintf("%d", originalCount+1)}, + }) + + // Verify that the schema is correct. + expectedSchema := `CREATE TABLE public.role_members ( + "role" STRING NOT NULL, + member STRING NOT NULL, + "isAdmin" BOOL NOT NULL, + role_id OID NOT NULL, + member_id OID NOT 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_member_role_id_member_id_key ("role" ASC, member ASC, 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) +)` + + row := tc.Conns[0].QueryRow(`SELECT create_statement FROM [SHOW CREATE TABLE system.role_members]`) + var actualSchema string + err = row.Scan(&actualSchema) + require.NoError(t, err) + + require.Equal(t, expectedSchema, actualSchema) +} + +func TestRoleMemberMigration1User(t *testing.T) { + runTestRoleMembersMigration(t, 1) +} + +func TestRoleMemberMigration100User(t *testing.T) { + runTestRoleMembersMigration(t, 100) +} + +func TestRoleMemberMigration15000User(t *testing.T) { + skip.UnderStress(t) + runTestRoleMembersMigration(t, 15000) +} + +// systemTable is copied over from systemschema/system.go. +func systemTable( + name catconstants.SystemTableName, + id descpb.ID, + columns []descpb.ColumnDescriptor, + families []descpb.ColumnFamilyDescriptor, + indexes ...descpb.IndexDescriptor, +) *descpb.TableDescriptor { + tbl := descpb.TableDescriptor{ + Name: string(name), + ID: id, + ParentID: keys.SystemDatabaseID, + UnexposedParentSchemaID: keys.SystemPublicSchemaID, + Version: 1, + Columns: columns, + Families: families, + PrimaryIndex: indexes[0], + Indexes: indexes[1:], + FormatVersion: descpb.InterleavedFormatVersion, + NextMutationID: 1, + NextConstraintID: 1, + } + for _, col := range columns { + if tbl.NextColumnID <= col.ID { + tbl.NextColumnID = col.ID + 1 + } + } + for _, fam := range families { + if tbl.NextFamilyID <= fam.ID { + tbl.NextFamilyID = fam.ID + 1 + } + } + for i, idx := range indexes { + if tbl.NextIndexID <= idx.ID { + tbl.NextIndexID = idx.ID + 1 + } + // Only assigned constraint IDs to unique non-primary indexes. + if idx.Unique && i >= 1 { + tbl.Indexes[i-1].ConstraintID = tbl.NextConstraintID + tbl.NextConstraintID++ + } + } + + // When creating tables normally, unique index constraint ids are + // assigned before the primary index. + tbl.PrimaryIndex.ConstraintID = tbl.NextConstraintID + tbl.NextConstraintID++ + return &tbl +} + +func getDeprecatedSystemMembersOptionsTable() *descpb.TableDescriptor { + return systemTable( + catconstants.RoleMembersTableName, + keys.RoleMembersTableID, + []descpb.ColumnDescriptor{ + {Name: "role", ID: 1, Type: types.String}, + {Name: "member", ID: 2, Type: types.String}, + {Name: "isAdmin", ID: 3, Type: types.Bool}, + }, + []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, + }, + }, + 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}, + }, + 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, + }, + descpb.IndexDescriptor{ + 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, + }, + ) +} diff --git a/pkg/upgrade/upgrades/system_privileges_test.go b/pkg/upgrade/upgrades/system_privileges_test.go index 529faad0aea8..b17ed0132751 100644 --- a/pkg/upgrade/upgrades/system_privileges_test.go +++ b/pkg/upgrade/upgrades/system_privileges_test.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "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/testutils/skip" @@ -55,7 +56,8 @@ func TestSystemPrivilegesMigration(t *testing.T) { // Delete system.role_id_seq. tdb.Exec(t, `INSERT INTO system.users VALUES ('node', '', false, 3)`) - tdb.Exec(t, `GRANT node TO root`) + tdb.Exec(t, `INSERT INTO system.role_members VALUES ($1, $2, false, $3, $4)`, + username.NodeUser, username.RootUser, username.NodeUserID, username.RootUserID) tdb.Exec(t, `DROP TABLE system.privileges`) tdb.Exec(t, `REVOKE node FROM root`) diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index 992567be0d7e..c24c660a3d12 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -148,6 +148,21 @@ var upgrades = []upgrade.Upgrade{ checkForPausedGCJobs, waitForDelRangeInGCJob, ), + upgrade.NewTenantUpgrade("alter system.role_members to include role_id and member_id columns", + toCV(clusterversion.RoleMembersTableHasIDColumns), + NoPrecondition, + alterSystemRoleMembersAddColumnsAndIndexes, + ), + upgrade.NewTenantUpgrade("backfill entries in system.role_members to include IDs", + toCV(clusterversion.RoleMembersIDColumnsAreBackfilled), + NoPrecondition, + backfillSystemRoleMembersIDColumns, + ), + upgrade.NewTenantUpgrade("set system.role_members role_id and member_id columns to not null", + toCV(clusterversion.SetRoleMembersUserIDColumnsNotNull), + NoPrecondition, + setSystemRoleMembersIDColumnsNotNull, + ), } func init() {