Skip to content

Commit

Permalink
restoreccl: allow restoring system users without user ids
Browse files Browse the repository at this point in the history
During restore, when users in the backup did not have ids, they'll be given one.

Release note: None
  • Loading branch information
RichardJCai committed Aug 3, 2022
1 parent 4589a8a commit 46113ff
Show file tree
Hide file tree
Showing 35 changed files with 369 additions and 19 deletions.
1 change: 1 addition & 0 deletions pkg/ccl/backupccl/BUILD.bazel
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
55 changes: 38 additions & 17 deletions pkg/ccl/backupccl/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1072,17 +1072,14 @@ func TestBackupRestoreSystemTables(t *testing.T) {

// At the time this test was written, these were the only system tables that
// were reasonable for a user to backup and restore into another cluster.
tables := []string{"locations", "role_members", "users", "zones", "role_id_seq"}
tables := []string{"locations", "role_members", "users", "zones"}
tableSpec := "system." + strings.Join(tables, ", system.")

// Take a consistent fingerprint of the original tables.
var backupAsOf string
expectedFingerprints := map[string][][]string{}
err := crdb.ExecuteTx(ctx, conn, nil /* txopts */, func(tx *gosql.Tx) error {
for _, table := range tables {
if table == "role_id_seq" {
continue
}
rows, err := conn.Query("SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE system." + table)
if err != nil {
return err
Expand All @@ -1108,9 +1105,6 @@ func TestBackupRestoreSystemTables(t *testing.T) {

// Verify the fingerprints match.
for _, table := range tables {
if table == "role_id_seq" {
continue
}
a := sqlDB.QueryStr(t, "SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE system_new."+table)
if e := expectedFingerprints[table]; !reflect.DeepEqual(e, a) {
t.Fatalf("fingerprints between system.%[1]s and system_new.%[1]s did not match:%s\n",
Expand Down Expand Up @@ -9556,7 +9550,6 @@ func TestExcludeDataFromBackupDoesNotHoldupGC(t *testing.T) {
func TestBackupRestoreSystemUsers(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
skip.WithIssue(t, 78963)

sqlDB, tempDir, cleanupFn := createEmptyCluster(t, singleNode)
_, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{})
Expand Down Expand Up @@ -9617,18 +9610,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 username, \"hashedPassword\", \"isRole\" FROM system.users", [][]string{
{"admin", "", "true"},
{"app", "NULL", "false"},
{"app_role", "NULL", "true"},
{"root", "", "false"},
{"test", "NULL", "false"},
{"test_role", "NULL", "true"},
})
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 @@ -9638,6 +9630,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
48 changes: 48 additions & 0 deletions pkg/ccl/backupccl/full_cluster_backup_restore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1112,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
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
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, fmt.Sprintf("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, fmt.Sprintf("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, fmt.Sprintf("DROP ROLE testrole"))
sqlDB.Exec(t, fmt.Sprintf("DROP ROLE testuser2"))
sqlDB.Exec(t, fmt.Sprintf("DROP ROLE testuser3"))
sqlDB.Exec(t, fmt.Sprintf("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, fmt.Sprintf("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 46113ff

Please sign in to comment.