Skip to content

Commit

Permalink
sql: add id column to system.users and system.role_id_seq
Browse files Browse the repository at this point in the history 
In preparation for keying roles by ids.
See RFC: #77453

Release note: None

Co-authored-by: Fenil-P <fenil.patel@cockroachlabs.com>
  • Loading branch information
@RichardJCai @Fenil-P
RichardJCai and Fenil-P committed Aug 4, 2022
1 parent 4cf3330 commit 2548cb7
Show file tree
Hide file tree
Showing 80 changed files with 1,289 additions and 342 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
89 changes: 85 additions & 4 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,13 @@ import (
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/security"
"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/spanconfig"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
Expand Down Expand Up @@ -1069,14 +1071,17 @@ 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"}
tables := []string{"locations", "role_members", "users", "zones", "role_id_seq"}
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 @@ -1102,6 +1107,9 @@ 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 @@ -9547,6 +9555,7 @@ 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 @@ -9574,7 +9583,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 @@ -9608,15 +9617,15 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
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{
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 * FROM system.role_members", [][]string{
sqlDBRestore1.CheckQueryResults(t, "SELECT \"role\", \"member\", \"isAdmin\" FROM system.role_members", [][]string{
{"admin", "root", "true"},
})
sqlDBRestore1.CheckQueryResults(t, "SHOW USERS", [][]string{
Expand Down Expand Up @@ -10148,6 +10157,78 @@ func TestBackupNoOverwriteLatest(t *testing.T) {
require.NotEqual(t, firstLatest, thirdLatest)
}

// TestBackupLatestInBaseDirectory tests to see that a LATEST
// file in the base directory can be properly read when one is not found
// in metadata/latest. This can occur when an older version node creates
// the backup.
func TestBackupLatestInBaseDirectory(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

disableUpgradeCh := make(chan struct{})
const numAccounts = 1
const userfile = "'userfile:///a'"
args := base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
BinaryVersionOverride: clusterversion.ByKey(clusterversion.BackupDoesNotOverwriteLatestAndCheckpoint - 1),
DisableAutomaticVersionUpgrade: disableUpgradeCh,
},
},
},
}

tc, sqlDB, _, cleanupFn := backupRestoreTestSetupWithParams(t, singleNode, numAccounts, InitManualReplication, args)
defer cleanupFn()
execCfg := tc.Server(0).ExecutorConfig().(sql.ExecutorConfig)
ctx := context.Background()
store, err := execCfg.DistSQLSrv.ExternalStorageFromURI(ctx, "userfile:///a", username.RootUserName())
require.NoError(t, err)

query := fmt.Sprintf("BACKUP INTO %s", userfile)
sqlDB.Exec(t, query)

// Confirm that the LATEST file was written to the base directory.
r, err := store.ReadFile(ctx, backupbase.LatestFileName)
require.NoError(t, err)
r.Close(ctx)

// Drop the system.role_seq_id so that we can perform the migration.
sqlDB.Exec(t, `INSERT INTO system.users VALUES ('node', '', false, 0)`)
sqlDB.Exec(t, `GRANT node TO root`)
sqlDB.Exec(t, `DROP SEQUENCE system.role_id_seq`)
sqlDB.Exec(t, `REVOKE node FROM root`)

err = tc.Servers[0].DB().Del(ctx, catalogkeys.MakeDescMetadataKey(keys.SystemSQLCodec, keys.RoleIDSequenceID))
require.NoError(t, err)
err = tc.Servers[0].DB().Del(ctx, keys.SystemSQLCodec.SequenceKey(uint32(keys.RoleIDSequenceID)))
require.NoError(t, err)

// Close the channel so that the cluster version is upgraded.
close(disableUpgradeCh)
// Check the cluster version is bumped to newVersion.
testutils.SucceedsSoon(t, func() error {
var version string
sqlDB.QueryRow(t, "SELECT value FROM system.settings WHERE name = 'version'").Scan(&version)
var v clusterversion.ClusterVersion
if err := protoutil.Unmarshal([]byte(version), &v); err != nil {
return err
}
version = v.String()
if version != clusterversion.TestingBinaryVersion.String() {
return errors.Errorf("cluster version is still %s, should be %s", version, clusterversion.TestingBinaryVersion.String())
}
return nil
})

// Take an incremental backup on the new version using the latest file
// written by the old version in the base directory.
query = fmt.Sprintf("BACKUP INTO LATEST IN %s", userfile)
sqlDB.Exec(t, query)

}

// TestBackupRestoreTelemetryEvents tests that BACKUP and RESTORE correctly
// publishes telemetry events.
func TestBackupRestoreTelemetryEvents(t *testing.T) {
Expand Down
4 changes: 3 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
3 changes: 3 additions & 0 deletions pkg/ccl/backupccl/system_schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,9 @@ var systemTableBackupConfiguration = map[string]systemBackupConfiguration{
systemschema.SystemExternalConnectionsTable.GetName(): {
shouldIncludeInClusterBackup: optInToClusterBackup, // No desc ID columns.
},
systemschema.RoleIDSequence.GetName(): {
shouldIncludeInClusterBackup: optOutOfClusterBackup,
},
}

func rekeySystemTable(
Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/external-connections
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public schema full
public schema full
public schema full
public schema full
role_id_seq table full
role_members table full
role_options table full
scheduled_jobs table full
Expand Down
6 changes: 0 additions & 6 deletions pkg/ccl/backupccl/testdata/backup-restore/feature-flags
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,4 @@ RESTORE TABLE d.t FROM 'nodelocal://1/deprecated';
----
NOTICE: The `RESTORE FROM <backup>` syntax will be removed in a future release, please switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories


exec-sql
RESTORE SYSTEM USERS FROM 'nodelocal://1/deprecated';
----
NOTICE: The `RESTORE FROM <backup>` syntax will be removed in a future release, please switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories

subtest end
1 change: 0 additions & 1 deletion pkg/ccl/backupccl/testdata/backup-restore/rangekeys
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,3 @@ SELECT count(*) from orig1.baz
1



4 changes: 4 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/basic
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ upsert /Table/4{4-5} database system (host)
upsert /Table/4{5-6} ttl_seconds=7200 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
upsert /Table/4{6-7} database system (host)
upsert /Table/4{7-8} database system (host)
upsert /Table/4{8-9} database system (host)
upsert /Table/5{0-1} database system (host)
upsert /Table/5{1-2} database system (host)
upsert /Table/5{2-3} database system (host)
Expand Down Expand Up @@ -93,6 +94,7 @@ upsert /Table/11{2-3} num_replicas=7
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand Down Expand Up @@ -185,6 +187,8 @@ delete /Table/4{6-7}
upsert /Table/4{6-7} ttl_seconds=100 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
delete /Table/4{7-8}
upsert /Table/4{7-8} ttl_seconds=100 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
delete /Table/4{8-9}
upsert /Table/4{8-9} ttl_seconds=100 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
delete /Table/5{0-1}
upsert /Table/5{0-1} ttl_seconds=100 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
delete /Table/5{1-2}
Expand Down
5 changes: 5 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/indexes
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ upsert /Table/10{6-7} range default
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand All @@ -45,6 +46,7 @@ upsert /Table/10{6/3-7} num_replicas=7
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand All @@ -71,6 +73,7 @@ upsert /Table/10{6/3-7} ttl_seconds=3600 num_replicas=7
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand All @@ -87,6 +90,7 @@ ALTER TABLE db.t CONFIGURE ZONE USING num_replicas = 9
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand Down Expand Up @@ -117,6 +121,7 @@ state offset=46
----
...
/Table/4{7-8} database system (host)
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
6 changes: 6 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/basic
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ mutations
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand Down Expand Up @@ -69,13 +70,15 @@ upsert /Tenant/10/Table/4{2-3} database system (tenant)
upsert /Tenant/10/Table/4{3-4} database system (tenant)
upsert /Tenant/10/Table/4{4-5} database system (tenant)
upsert /Tenant/10/Table/4{6-7} database system (tenant)
upsert /Tenant/10/Table/4{8-9} database system (tenant)
upsert /Tenant/10/Table/5{0-1} database system (tenant)
upsert /Tenant/10/Table/5{1-2} database system (tenant)
upsert /Tenant/10/Table/5{2-3} database system (tenant)

state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand Down Expand Up @@ -113,6 +116,7 @@ state offset=47
/Tenant/10/Table/4{3-4} database system (tenant)
/Tenant/10/Table/4{4-5} database system (tenant)
/Tenant/10/Table/4{6-7} database system (tenant)
/Tenant/10/Table/4{8-9} database system (tenant)
/Tenant/10/Table/5{0-1} database system (tenant)
/Tenant/10/Table/5{1-2} database system (tenant)
/Tenant/10/Table/5{2-3} database system (tenant)
Expand All @@ -139,9 +143,11 @@ upsert /Tenant/10/Table/11{3-4} range default
state offset=81
----
...
/Tenant/10/Table/4{2-3} database system (tenant)
/Tenant/10/Table/4{3-4} database system (tenant)
/Tenant/10/Table/4{4-5} database system (tenant)
/Tenant/10/Table/4{6-7} database system (tenant)
/Tenant/10/Table/4{8-9} database system (tenant)
/Tenant/10/Table/5{0-1} database system (tenant)
/Tenant/10/Table/5{1-2} database system (tenant)
/Tenant/10/Table/5{2-3} database system (tenant)
Expand Down
2 changes: 2 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/protectedts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mutations
state offset=47
----
...
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand Down Expand Up @@ -85,6 +86,7 @@ upsert /Tenant/10/Table/4{2-3} database system (tenant)
upsert /Tenant/10/Table/4{3-4} database system (tenant)
upsert /Tenant/10/Table/4{4-5} database system (tenant)
upsert /Tenant/10/Table/4{6-7} database system (tenant)
upsert /Tenant/10/Table/4{8-9} database system (tenant)
upsert /Tenant/10/Table/5{0-1} database system (tenant)
upsert /Tenant/10/Table/5{1-2} database system (tenant)
upsert /Tenant/10/Table/5{2-3} database system (tenant)
Expand Down
2 changes: 2 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/named_zones
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ state offset=46
----
...
/Table/4{7-8} database system (host)
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand All @@ -142,6 +143,7 @@ state offset=46
----
...
/Table/4{7-8} database system (host)
/Table/4{8-9} database system (host)
/Table/5{0-1} database system (host)
/Table/5{1-2} database system (host)
/Table/5{2-3} database system (host)
Expand Down
Loading

0 comments on commit 2548cb7

Please sign in to comment.