Skip to content

Commit

Permalink
Merge #81457
Browse files Browse the repository at this point in the history 
81457: sql: add user id column to system.users table and perform migration r=RichardJCai a=RichardJCai

Previously we used usernames to access user information and privileges.
We will transition to using IDs by first adding user ids and updating the code
to use IDs where possible.

Release justification: Justified in RFC #77453
Release note: None

Co-authored-by: richardjcai <caioftherichard@gmail.com>
  • Loading branch information
@RichardJCai
craig[bot] and RichardJCai committed Aug 5, 2022
2 parents 0190d4f + 1bc6c6d commit cc73cad
Show file tree
Hide file tree
Showing 111 changed files with 1,568 additions and 345 deletions.
2 changes: 1 addition & 1 deletion docs/generated/settings/settings-for-tenants.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,4 +283,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 22.1-32 set the active cluster version in the format '<major>.<minor>'
version version 22.1-40 set the active cluster version in the format '<major>.<minor>'
2 changes: 1 addition & 1 deletion docs/generated/settings/settings.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,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>22.1-32</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>22.1-40</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
</tbody>
</table>
6 changes: 3 additions & 3 deletions pkg/bench/rttanalysis/testdata/benchmark_expectations
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ exp,benchmark
16,DropDatabase/drop_database_1_table
17,DropDatabase/drop_database_2_tables
18,DropDatabase/drop_database_3_tables
18,DropRole/drop_1_role
25,DropRole/drop_2_roles
32,DropRole/drop_3_roles
19,DropRole/drop_1_role
27,DropRole/drop_2_roles
35,DropRole/drop_3_roles
17,DropSequence/drop_1_sequence
19,DropSequence/drop_2_sequences
21,DropSequence/drop_3_sequences
Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/backupccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ go_library(
"//pkg/sql/catalog/colinfo",
"//pkg/sql/catalog/dbdesc",
"//pkg/sql/catalog/descbuilder",
"//pkg/sql/catalog/descidgen",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/catalog/ingesting",
Expand Down
50 changes: 39 additions & 11 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9574,7 +9574,7 @@ 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 * FROM system.users", [][]string{
sqlDBRestore.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\" FROM system.users", [][]string{
{"admin", "", "true"},
{"app", "NULL", "false"},
{"app_role", "NULL", "true"},
Expand Down Expand Up @@ -9607,18 +9607,17 @@ 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 * FROM system.users", [][]string{
{"admin", "", "true"},
{"app", "NULL", "false"},
{"app_role", "NULL", "true"},
{"root", "", "false"},
{"test", "NULL", "false"},
{"test_role", "NULL", "true"},
})
sqlDBRestore1.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{
sqlDBRestore1.CheckQueryResults(t, "SELECT \"role\", \"member\", \"isAdmin\" FROM system.role_members", [][]string{
{"admin", "root", "true"},
})
sqlDBRestore1.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\", \"user_id\" FROM system.users", [][]string{
{"admin", "", "true", "2"},
{"app", "NULL", "false", "100"},
{"app_role", "NULL", "true", "101"},
{"root", "", "false", "1"},
{"test", "NULL", "false", "102"},
{"test_role", "NULL", "true", "103"},
})
sqlDBRestore1.CheckQueryResults(t, "SHOW USERS", [][]string{
{"admin", "", "{}"},
{"app", "", "{}"},
Expand All @@ -9628,6 +9627,35 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
{"test_role", "", "{}"},
})
})
_, sqlDBRestore2, cleanupEmptyCluster2 := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{})
defer cleanupEmptyCluster2()
t.Run("restore-from-backup-with-existing-user", func(t *testing.T) {
// Create testuser and verify that the system user ids are
// 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 username, \"hashedPassword\", \"isRole\", \"user_id\" FROM system.users", [][]string{
{"admin", "", "true", "2"},
{"app", "NULL", "false", "101"},
{"app_role", "NULL", "true", "102"},
{"root", "", "false", "1"},
{"test", "NULL", "false", "103"},
{"test_role", "NULL", "true", "104"},
{"testuser", "NULL", "false", "100"},
})
sqlDBRestore2.CheckQueryResults(t, "SHOW USERS", [][]string{
{"admin", "", "{}"},
{"app", "", "{}"},
{"app_role", "", "{}"},
{"root", "", "{admin}"},
{"test", "", "{}"},
{"test_role", "", "{}"},
{"testuser", "", "{}"},
})
})
}

// TestUserfileNormalizationIncrementalShowBackup tests to see that file
Expand Down
52 changes: 51 additions & 1 deletion pkg/ccl/backupccl/full_cluster_backup_restore_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ func TestRestoreFromFullClusterBackup(t *testing.T) {

t.Run("system tables", func(t *testing.T) {
sqlDB.Exec(t, `CREATE DATABASE temp_sys`)
sqlDB.Exec(t, `RESTORE system.users FROM $1 WITH into_db='temp_sys'`, localFoo)
sqlDB.Exec(t, `RESTORE system.users, system.role_id_seq FROM $1 WITH into_db='temp_sys'`, localFoo)
sqlDB.CheckQueryResults(t, "SELECT * FROM temp_sys.users", sqlDB.QueryStr(t, "SELECT * FROM system.users"))
})
}
Expand Down Expand Up @@ -642,6 +642,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) {
{"database_role_settings"},
{"external_connections"},
{"locations"},
{"role_id_seq"},
{"role_members"},
{"role_options"},
{"scheduled_jobs"},
Expand Down Expand Up @@ -733,6 +734,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) {
{"database_role_settings"},
{"external_connections"},
{"locations"},
{"role_id_seq"},
{"role_members"},
{"role_options"},
{"scheduled_jobs"},
Expand Down Expand Up @@ -1110,3 +1112,51 @@ DROP DATABASE defaultdb;
{fmt.Sprint(parentID), fmt.Sprint(parentSchemaID), name, fmt.Sprint(ID)},
})
}

func TestFullClusterRestoreWithUserIDs(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

params := base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
},
},
}
const numAccounts = 10
_, sqlDB, tempDir, cleanupFn := backupRestoreTestSetupWithParams(t, singleNode, numAccounts, InitManualReplication, params)
_, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, params)
defer cleanupFn()
defer cleanupEmptyCluster()

sqlDB.Exec(t, `CREATE USER test1`)
sqlDB.Exec(t, `CREATE USER test2`)
sqlDB.Exec(t, `BACKUP TO $1`, localFoo)

sqlDB.CheckQueryResults(t, `SELECT * FROM system.users ORDER BY user_id`, [][]string{
{"root", "", "false", "1"},
{"admin", "", "true", "2"},
{"test1", "NULL", "false", "100"},
{"test2", "NULL", "false", "101"},
})
// Ensure that the new backup succeeds.
sqlDBRestore.Exec(t, `RESTORE FROM $1`, localFoo)

sqlDBRestore.CheckQueryResults(t, `SELECT * FROM system.users ORDER BY user_id`, [][]string{
{"root", "", "false", "1"},
{"admin", "", "true", "2"},
{"test1", "NULL", "false", "100"},
{"test2", "NULL", "false", "101"},
})

sqlDBRestore.Exec(t, `CREATE USER test3`)

sqlDBRestore.CheckQueryResults(t, `SELECT * FROM system.users ORDER BY user_id`, [][]string{
{"root", "", "false", "1"},
{"admin", "", "true", "2"},
{"test1", "NULL", "false", "100"},
{"test2", "NULL", "false", "101"},
{"test3", "NULL", "false", "102"},
})
}
17 changes: 16 additions & 1 deletion pkg/ccl/backupccl/restore_job.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/ingesting"
Expand Down Expand Up @@ -2400,11 +2401,25 @@ func (r *restoreResumer) restoreSystemUsers(
}

insertUser := `INSERT INTO system.users ("username", "hashedPassword", "isRole") VALUES ($1, $2, $3)`
if r.execCfg.Settings.Version.IsActive(ctx, clusterversion.AddSystemUserIDColumn) {
insertUser = `INSERT INTO system.users ("username", "hashedPassword", "isRole", "user_id") VALUES ($1, $2, $3, $4)`
}
newUsernames := make(map[string]bool)
args := make([]interface{}, 4)
for _, user := range users {
newUsernames[user[0].String()] = true
args[0] = user[0]
args[1] = user[1]
args[2] = user[2]
if r.execCfg.Settings.Version.IsActive(ctx, clusterversion.AddSystemUserIDColumn) {
id, err := descidgen.GenerateUniqueRoleID(ctx, r.execCfg.DB, r.execCfg.Codec)
if err != nil {
return err
}
args[3] = id
}
if _, err = executor.Exec(ctx, "insert-non-existent-users", txn, insertUser,
user[0], user[1], user[2]); err != nil {
args...); err != nil {
return err
}
}
Expand Down
139 changes: 139 additions & 0 deletions pkg/ccl/backupccl/restore_old_versions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ func TestRestoreOldVersions(t *testing.T) {
privilegeDirs = testdataBase + "/privileges"
multiRegionDirs = testdataBase + "/multi-region"
publicSchemaDirs = testdataBase + "/public-schema-remap"
systemUsersDirs = testdataBase + "/system-users-restore"
)

t.Run("table-restore", func(t *testing.T) {
Expand Down Expand Up @@ -299,6 +300,28 @@ ORDER BY object_type, object_name`, [][]string{
t.Run(dir.Name(), restoreSyntheticPublicSchemaNamespaceEntryCleanupOnFail(exportDir))
}
})

t.Run("system-users-restore", func(t *testing.T) {
dirs, err := ioutil.ReadDir(systemUsersDirs)
require.NoError(t, err)
for _, dir := range dirs {
require.True(t, dir.IsDir())
exportDir, err := filepath.Abs(filepath.Join(systemUsersDirs, dir.Name()))
require.NoError(t, err)
t.Run(dir.Name(), restoreSystemUsersWithoutIDs(exportDir))
}
})

t.Run("full-cluster-restore-users-without-ids", func(t *testing.T) {
dirs, err := ioutil.ReadDir(systemUsersDirs)
require.NoError(t, err)
for _, dir := range dirs {
require.True(t, dir.IsDir())
exportDir, err := filepath.Abs(filepath.Join(systemUsersDirs, dir.Name()))
require.NoError(t, err)
t.Run(dir.Name(), fullClusterRestoreUsersWithoutIDs(exportDir))
}
})
}

func restoreOldVersionTestWithInterleave(exportDir string) func(t *testing.T) {
Expand Down Expand Up @@ -1111,3 +1134,119 @@ func restoreSyntheticPublicSchemaNamespaceEntryCleanupOnFail(exportDir string) f
sqlDB.CheckQueryResults(t, `SELECT id FROM system.namespace WHERE name = 'public' AND id=29 AND "parentID"!=1`, [][]string{})
}
}

func fullClusterRestoreUsersWithoutIDs(exportDir string) func(t *testing.T) {
return func(t *testing.T) {
const numAccounts = 1000
_, _, tmpDir, cleanupFn := backupRestoreTestSetup(t, multiNode, numAccounts, InitManualReplication)
defer cleanupFn()

_, sqlDB, cleanup := backupRestoreTestSetupEmpty(t, singleNode, tmpDir,
InitManualReplication, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
},
}})
defer cleanup()
err := os.Symlink(exportDir, filepath.Join(tmpDir, "foo"))
require.NoError(t, err)

sqlDB.Exec(t, fmt.Sprintf("RESTORE FROM '%s'", localFoo))

sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
{"admin", "", "true", "2"},
{"root", "", "false", "1"},
{"testrole", "NULL", "true", "100"},
{"testuser", "NULL", "false", "101"},
{"testuser2", "NULL", "false", "102"},
{"testuser3", "NULL", "false", "103"},
})

// Verify that the next user we create uses the next biggest ID.
sqlDB.Exec(t, "CREATE USER testuser4")

sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
{"admin", "", "true", "2"},
{"root", "", "false", "1"},
{"testrole", "NULL", "true", "100"},
{"testuser", "NULL", "false", "101"},
{"testuser2", "NULL", "false", "102"},
{"testuser3", "NULL", "false", "103"},
{"testuser4", "NULL", "false", "104"},
})
}
}

func restoreSystemUsersWithoutIDs(exportDir string) func(t *testing.T) {
return func(t *testing.T) {
const numAccounts = 1000
_, _, tmpDir, cleanupFn := backupRestoreTestSetup(t, multiNode, numAccounts, InitManualReplication)
defer cleanupFn()

_, sqlDB, cleanup := backupRestoreTestSetupEmpty(t, singleNode, tmpDir,
InitManualReplication, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
},
}})
defer cleanup()
err := os.Symlink(exportDir, filepath.Join(tmpDir, "foo"))
require.NoError(t, err)

sqlDB.Exec(t, fmt.Sprintf("RESTORE SYSTEM USERS FROM '%s'", localFoo))

sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
{"admin", "", "true", "2"},
{"root", "", "false", "1"},
{"testrole", "NULL", "true", "100"},
{"testuser", "NULL", "false", "101"},
{"testuser2", "NULL", "false", "102"},
{"testuser3", "NULL", "false", "103"},
})

// Verify that the next user we create uses the next biggest ID.
sqlDB.Exec(t, "CREATE USER testuser4")

sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
{"admin", "", "true", "2"},
{"root", "", "false", "1"},
{"testrole", "NULL", "true", "100"},
{"testuser", "NULL", "false", "101"},
{"testuser2", "NULL", "false", "102"},
{"testuser3", "NULL", "false", "103"},
{"testuser4", "NULL", "false", "104"},
})

// Drop some users and try restoring again.
sqlDB.Exec(t, "DROP ROLE testrole")
sqlDB.Exec(t, "DROP ROLE testuser2")
sqlDB.Exec(t, "DROP ROLE testuser3")
sqlDB.Exec(t, "DROP ROLE testuser4")

sqlDB.Exec(t, fmt.Sprintf("RESTORE SYSTEM USERS FROM '%s'", localFoo))

// testrole, testuser2, testuser3 should be reassigned higher ids.
sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
{"admin", "", "true", "2"},
{"root", "", "false", "1"},
{"testrole", "NULL", "true", "105"},
{"testuser", "NULL", "false", "101"},
{"testuser2", "NULL", "false", "106"},
{"testuser3", "NULL", "false", "107"},
})

// Verify that the next user we create uses the next biggest ID.
sqlDB.Exec(t, "CREATE USER testuser4")
sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
{"admin", "", "true", "2"},
{"root", "", "false", "1"},
{"testrole", "NULL", "true", "105"},
{"testuser", "NULL", "false", "101"},
{"testuser2", "NULL", "false", "106"},
{"testuser3", "NULL", "false", "107"},
{"testuser4", "NULL", "false", "108"},
})
}
}
Loading

0 comments on commit cc73cad

Please sign in to comment.