Skip to content

Commit

Permalink
sql: add role_id and member_id columns to system.role_members table
Browse files Browse the repository at this point in the history
  • Loading branch information
andyyang890 committed Nov 23, 2022
1 parent b12438a commit c9ac159
Show file tree
Hide file tree
Showing 10 changed files with 480 additions and 11 deletions.
2 changes: 1 addition & 1 deletion docs/generated/settings/settings-for-tenants.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 <host>:<port>. If no port is specified, 4317 will be used.
trace.span_registry.enabled boolean true if set, ongoing traces can be seen at https://<ui>/#/debug/tracez
trace.zipkin.collector string the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.
version version 1000022.2-8 set the active cluster version in the format '<major>.<minor>'
version version 1000022.2-14 set the active cluster version in the format '<major>.<minor>'
2 changes: 1 addition & 1 deletion docs/generated/settings/settings.html
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,6 @@
<tr><td><code>trace.opentelemetry.collector</code></td><td>string</td><td><code></code></td><td>address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.</td></tr>
<tr><td><code>trace.span_registry.enabled</code></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://<ui>/#/debug/tracez</td></tr>
<tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.</td></tr>
<tr><td><code>version</code></td><td>version</td><td><code>1000022.2-8</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
<tr><td><code>version</code></td><td>version</td><td><code>1000022.2-14</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
</tbody>
</table>
23 changes: 21 additions & 2 deletions pkg/clusterversion/cockroach_versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,15 +314,22 @@ const (

// V23_1TenantNames adds a name column to system.tenants.
V23_1TenantNames

// V23_1DescIDSequenceForSystemTenant migrates the descriptor ID generator
// counter from a meta key to the system.descriptor_id_seq sequence for the
// system tenant.
V23_1DescIDSequenceForSystemTenant

// V23_1AddPartialStatisticsPredicateCol adds a column to store the predicate
// for a partial statistics collection.
V23_1AddPartialStatisticsPredicateCol
// V23_1RoleMembersTableHasIDColumns is the version where the role_members
// system table has columns for ids.
V23_1RoleMembersTableHasIDColumns
// V23_1RoleMembersTableHasIndexesForIDColumns is the version where the
// role_members system table has indexes for its id columns.
V23_1RoleMembersTableHasIndexesForIDColumns
// 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.
Expand Down Expand Up @@ -538,6 +545,18 @@ var rawVersionsSingleton = keyedVersions{
Key: V23_1AddPartialStatisticsPredicateCol,
Version: roachpb.Version{Major: 22, Minor: 2, Internal: 8},
},
{
Key: V23_1RoleMembersTableHasIDColumns,
Version: roachpb.Version{Major: 22, Minor: 2, Internal: 10},
},
{
Key: V23_1RoleMembersTableHasIndexesForIDColumns,
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.
Expand Down
59 changes: 55 additions & 4 deletions pkg/sql/catalog/systemschema/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...).
Expand Down Expand Up @@ -1563,6 +1568,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{
{
Expand All @@ -1578,6 +1585,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",
Expand Down Expand Up @@ -1607,6 +1628,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.
Expand Down
39 changes: 38 additions & 1 deletion pkg/sql/grant_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -167,9 +168,13 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR

func (n *GrantRoleNode) startExec(params runParams) error {
opName := "grant-role"
roleMembersHasIDs := params.p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.V23_1RoleMembersTableHasIDColumns)
// 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`
Expand All @@ -179,15 +184,47 @@ func (n *GrantRoleNode) startExec(params runParams) error {
}

var rowsAffected int
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 := 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 roleMembersHasIDs {
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
Expand Down
4 changes: 2 additions & 2 deletions pkg/startupmigrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -734,10 +734,10 @@ 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 {
Expand Down
2 changes: 2 additions & 0 deletions pkg/upgrade/upgrades/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ go_library(
"fix_userfile_descriptor_corruption.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",
Expand Down Expand Up @@ -85,6 +86,7 @@ go_test(
"main_test.go",
"precondition_before_starting_an_upgrade_external_test.go",
"role_id_migration_test.go",
"role_members_ids_migration_test.go",
"role_options_migration_test.go",
"sampled_stmt_diagnostics_requests_test.go",
"schema_changes_external_test.go",
Expand Down
123 changes: 123 additions & 0 deletions pkg/upgrade/upgrades/role_members_ids_migration.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// 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
`

func alterSystemRoleMembersAddIDColumns(
ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps,
) error {
op := operation{
name: "add-id-columns-system-role-members",
schemaList: []string{"role_id", "member_id"},
query: addIDColumnsToRoleMembersStmt,
schemaExistsFn: columnExists,
}
return migrateTable(ctx, cs, d, op, keys.RoleMembersTableID, systemschema.RoleMembersTable)
}

const addIndexOnRoleIDToRoleMembersStmt = `
CREATE INDEX role_members_role_id_idx ON system.role_members (role_id ASC)
`

const addIndexOnMemberIDToRoleMembersStmt = `
CREATE INDEX role_members_member_id_idx ON system.role_members (member_id ASC)
`

const addUniqueIndexOnIDsToRoleMembersStmt = `
CREATE UNIQUE INDEX role_members_role_id_member_id_key ON system.role_members (role_id ASC, member_id ASC)
`

func alterSystemRoleMembersAddIndexesForIDColumns(
ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps,
) error {
for _, op := range []operation{
{
name: "add-role-id-index-system-role-members",
schemaList: []string{"role_id"},
query: addIndexOnRoleIDToRoleMembersStmt,
schemaExistsFn: hasIndex,
},
{
name: "add-member-id-index-system-role-members",
schemaList: []string{"member_id"},
query: addIndexOnMemberIDToRoleMembersStmt,
schemaExistsFn: hasIndex,
},
{
name: "add-id-unique-index-system-role-members",
schemaList: []string{"role_id", "member_id"},
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 10000
`

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 10000
`

func backfillSystemRoleMembersIDColumns(
ctx context.Context, _ clusterversion.ClusterVersion, d upgrade.TenantDeps,
) error {
for _, backfillStmt := range []string{backfillRoleIDColumnRoleMemberStmt, backfillMemberIDColumnRoleMembersStmt} {
for {
rowsAffected, err := d.InternalExecutor.ExecEx(ctx, "backfill-id-columns-system-role-members", nil,
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.
Loading

0 comments on commit c9ac159

Please sign in to comment.