From 1443da7a4344304ee285660e15b64f53b3ccb93b Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Tue, 23 Aug 2022 00:14:54 -0400 Subject: [PATCH 1/3] gen: add schemachanger to code generation target Before this, dev gen go did not regenerate schemachanger code. Release justification: build tooling only change. Release note: None --- pkg/gen/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/gen/BUILD.bazel b/pkg/gen/BUILD.bazel index a8e260bfb580..6d1c317152e8 100644 --- a/pkg/gen/BUILD.bazel +++ b/pkg/gen/BUILD.bazel @@ -57,6 +57,7 @@ gen( ":misc", ":optgen", ":parser", + ":schemachanger", ":stringer", ], ) From 9f447c58c2547fd4d0106ddafff6033467b0998c Mon Sep 17 00:00:00 2001 From: adityamaru Date: Wed, 24 Aug 2022 18:35:35 -0400 Subject: [PATCH 2/3] privilege,backupccl: introduce a RESTORE privilege This change introduces a RESTORE privilege that is grantable as a system, or database, or table level privilege. The purpose of this privilege is to offer a fine-grained permission model for users that wish to run restores. First let us outline the existing privilege model that governs backups: Cluster restores - admin only. Database restores - users must have CREATEDB role option. Table restores - users must have CREATE on the database we are restoring into. With the new fine grained permission model we would like to stop having to grant users privileges such as CREATE and CREATEDB only for the purpose of being able to restore a target. To this effect the new privilege model that will govern restores is: Cluster backups - user requires the system RESTORE privilege Database backups - user requires the system RESTORE privilege Table backups - user requires the database RESTORE privilege Note, admins will ofcourse continue to bypass all these checks. This diff does not change the privilege checks we perform on the backup destination URIs related to IMPLICIT authentication. That will be done as a follow-up. In 22.2 to prevent breaking user flows we will continue to respect the old privilege model, but emit a notice indicating our plans to replace this model in 23.1. At which point users will need to be granted the appropriate BACKUP privileges. Informs: #86263 Release note (sql change): This change introduces a new RESTORE privilege that is grantable as a system or database level privilege. Users can opt-in to the new privilege model by granting the appropriate privileges as per the following model: Cluster backups - user requires the system RESTORE privilege Database backups - user requires the system RESTORE privilege Table backups - user requires the database RESTORE privilege In 22.2 we will continue to respect the old privilege model, but will completely swithover to the RESTORE privilege in 23.1. Release justification: high impact change to offer fine grained privileges for bulk operations --- pkg/ccl/backupccl/backup_planning.go | 19 +- pkg/ccl/backupccl/restore_planning.go | 177 +++++++++++++---- .../backup-restore/backup-permissions | 16 +- .../backup-permissions-deprecated | 12 +- .../external-connections-privileges | 5 +- .../testdata/backup-restore/rangekeys | 2 - .../testdata/backup-restore/restore-grants | 3 + .../backup-restore/restore-permissions | 186 +++++++++++++----- .../restore-permissions-deprecated | 106 ++++++++++ .../backup-restore/restore-schema-only | 2 +- .../backup-restore/restore-validation-only | 2 +- pkg/sql/catalog/catpb/privilege_test.go | 6 +- .../testdata/logic_test/grant_database | 7 + pkg/sql/privilege/kind_string.go | 5 +- pkg/sql/privilege/privilege.go | 11 +- 15 files changed, 432 insertions(+), 127 deletions(-) create mode 100644 pkg/ccl/backupccl/testdata/backup-restore/restore-permissions-deprecated diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go index 67cca812dc22..da83a2b96c9e 100644 --- a/pkg/ccl/backupccl/backup_planning.go +++ b/pkg/ccl/backupccl/backup_planning.go @@ -73,6 +73,9 @@ const ( // backupPartitionDescriptorPrefix is the file name prefix for serialized // BackupPartitionDescriptor protos. backupPartitionDescriptorPrefix = "BACKUP_PART" + + deprecatedPrivilegesPreamble = "The existing privileges are being deprecated " + + "in favour of a fine-grained privilege model explained here . In a future release, to run" ) type tableAndIndex struct { @@ -353,10 +356,10 @@ func checkBackupDestinationPrivileges(ctx context.Context, p sql.PlanHookState, return nil } -// privilegesDeprecationNotice returns a notice that outlines the deprecation of -// the existing privilege model for backups, and the steps to take to adopt the -// new privilege model, based on the type of backup being run. -func privilegesDeprecationNotice( +// backupPrivilegesDeprecationNotice returns a notice that outlines the +// deprecation of the existing privilege model for backups, and the steps to +// take to adopt the new privilege model, based on the type of backup being run. +func backupPrivilegesDeprecationNotice( p sql.PlanHookState, backupStmt *annotatedBackupStatement, targetDescs []catalog.Descriptor, ) string { if backupStmt.Targets == nil { @@ -366,8 +369,6 @@ func privilegesDeprecationNotice( // If a user is running `BACKUP DATABASE` buffer all the databases that will // require the `BACKUP` privilege. var notice string - preamble := "The existing privileges required for backup are being deprecated " + - "in favour of a fine-grained privilege model explained here . In a future release, to run" if backupStmt.Targets.Databases != nil { dbsRequireBackupPrivilege := make([]string, 0) for _, desc := range targetDescs { @@ -379,7 +380,7 @@ func privilegesDeprecationNotice( notice = fmt.Sprintf("%s BACKUP DATABASE, user %s will exclusively require the "+ "BACKUP privilege on database %s.", - preamble, p.User().Normalized(), strings.Join(dbsRequireBackupPrivilege, ", ")) + deprecatedPrivilegesPreamble, p.User().Normalized(), strings.Join(dbsRequireBackupPrivilege, ", ")) } else if backupStmt.Targets.Tables.TablePatterns != nil { // If a user is running `BACKUP TABLE` buffer all the tables that will require the `BACKUP` privilege. tablesRequireBackupPrivilege := make([]string, 0) @@ -392,7 +393,7 @@ func privilegesDeprecationNotice( notice = fmt.Sprintf("%s BACKUP TABLE, user %s will exclusively require the "+ "BACKUP privilege on tables: %s.", - preamble, + deprecatedPrivilegesPreamble, p.User().Normalized(), strings.Join(tablesRequireBackupPrivilege, ", ")) } @@ -494,7 +495,7 @@ func checkPrivilegesForBackup( // // TODO(adityamaru): Delete deprecated privilege checks in 23.1. Users will be // required to have the appropriate `BACKUP` privilege instead. - notice := privilegesDeprecationNotice(p, backupStmt, targetDescs) + notice := backupPrivilegesDeprecationNotice(p, backupStmt, targetDescs) p.BufferClientNotice(ctx, pgnotice.Newf("%s", notice)) for _, desc := range targetDescs { diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index 700e37fdbb3d..b77aedc2c0ff 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -275,6 +275,9 @@ func allocateDescriptorRewrites( } } + var shouldBufferDeprecatedPrivilegeNotice bool + databasesWithDeprecatedPrivileges := make(map[string]struct{}) + // Fail fast if the necessary databases don't exist or are otherwise // incompatible with this restore. if err := sql.DescsTxn(ctx, p.ExecCfg(), func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error { @@ -309,8 +312,11 @@ func allocateDescriptorRewrites( if err != nil { return err } - if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil { + if usesDeprecatedPrivileges, err := checkRestorePrivilegesOnDatabase(ctx, p, parentDB); err != nil { return err + } else if usesDeprecatedPrivileges { + shouldBufferDeprecatedPrivilegeNotice = true + databasesWithDeprecatedPrivileges[parentDB.GetName()] = struct{}{} } // See if there is an existing schema with the same name. @@ -378,8 +384,11 @@ func allocateDescriptorRewrites( return errors.Wrapf(err, "failed to lookup parent DB %d", errors.Safe(parentID)) } - if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil { + if usesDeprecatedPrivileges, err := checkRestorePrivilegesOnDatabase(ctx, p, parentDB); err != nil { return err + } else if usesDeprecatedPrivileges { + shouldBufferDeprecatedPrivilegeNotice = true + databasesWithDeprecatedPrivileges[parentDB.GetName()] = struct{}{} } // We're restoring a table and not its parent database. We may block @@ -465,8 +474,11 @@ func allocateDescriptorRewrites( // need to create the type. // Ensure that the user has the correct privilege to create types. - if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil { + if usesDeprecatedPrivileges, err := checkRestorePrivilegesOnDatabase(ctx, p, parentDB); err != nil { return err + } else if usesDeprecatedPrivileges { + shouldBufferDeprecatedPrivilegeNotice = true + databasesWithDeprecatedPrivileges[parentDB.GetName()] = struct{}{} } // Create a rewrite entry for the type. @@ -531,6 +543,15 @@ func allocateDescriptorRewrites( return nil, err } + if shouldBufferDeprecatedPrivilegeNotice { + dbNames := make([]string, 0, len(databasesWithDeprecatedPrivileges)) + for dbName := range databasesWithDeprecatedPrivileges { + dbNames = append(dbNames, dbName) + } + p.BufferClientNotice(ctx, pgnotice.Newf("%s RESTORE TABLE, user %s will exclusively require the RESTORE privilege on databases %s", + deprecatedPrivilegesPreamble, p.User(), strings.Join(dbNames, ", "))) + } + // Allocate new IDs for each database and table. // // NB: we do this in a standalone transaction, not one that covers the @@ -653,12 +674,12 @@ func getDatabaseIDAndDesc( func dropDefaultUserDBs(ctx context.Context, execCfg *sql.ExecutorConfig) error { return sql.DescsTxn(ctx, execCfg, func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error { ie := execCfg.InternalExecutor - _, err := ie.Exec(ctx, "drop-defaultdb", nil, "DROP DATABASE IF EXISTS defaultdb") + _, err := ie.Exec(ctx, "drop-defaultdb", txn, "DROP DATABASE IF EXISTS defaultdb") if err != nil { return err } - _, err = ie.Exec(ctx, "drop-postgres", nil, "DROP DATABASE IF EXISTS postgres") + _, err = ie.Exec(ctx, "drop-postgres", txn, "DROP DATABASE IF EXISTS postgres") if err != nil { return err } @@ -1101,40 +1122,11 @@ func restorePlanHook( return fn, jobs.BulkJobExecutionResultHeader, nil, false, nil } -func checkPrivilegesForRestore( - ctx context.Context, restoreStmt *tree.Restore, p sql.PlanHookState, from [][]string, +// checkRestoreDestinationPrivileges iterates over the External Storage URIs and +// ensures the user has adequate privileges to use each of them. +func checkRestoreDestinationPrivileges( + ctx context.Context, p sql.PlanHookState, from [][]string, ) error { - hasAdmin, err := p.HasAdminRole(ctx) - if err != nil { - return err - } - if hasAdmin { - return nil - } - // Do not allow full cluster restores. - if restoreStmt.DescriptorCoverage == tree.AllDescriptors { - return pgerror.Newf( - pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to restore full cluster backups") - } - // Do not allow tenant restores. - if restoreStmt.Targets.TenantID.IsSet() { - return pgerror.Newf( - pgcode.InsufficientPrivilege, - "only users with the admin role can perform RESTORE TENANT") - } - // Database restores require the CREATEDB privileges. - if len(restoreStmt.Targets.Databases) > 0 { - hasCreateDB, err := p.HasRoleOption(ctx, roleoption.CREATEDB) - if err != nil { - return err - } - if !hasCreateDB { - return pgerror.Newf( - pgcode.InsufficientPrivilege, - "only users with the CREATEDB privilege can restore databases") - } - } if p.ExecCfg().ExternalIODirConfig.EnableNonAdminImplicitAndArbitraryOutbound { return nil } @@ -1172,9 +1164,118 @@ func checkPrivilegesForRestore( } } } + return nil } +// checkRestorePrivilegesOnDatabase check that the user has adequate privileges +// on the parent database to restore schema objects into the database. This is +// used to check the privileges required for a `RESTORE TABLE`. +func checkRestorePrivilegesOnDatabase( + ctx context.Context, p sql.PlanHookState, parentDB catalog.DatabaseDescriptor, +) (shouldBufferNotice bool, err error) { + if err := p.CheckPrivilege(ctx, parentDB, privilege.RESTORE); err == nil { + return false, nil + } + + if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil { + notice := fmt.Sprintf("%s RESTORE TABLE, user %s will exclusively require the "+ + "RESTORE privilege on database %s.", deprecatedPrivilegesPreamble, p.User().Normalized(), parentDB.GetName()) + p.BufferClientNotice(ctx, pgnotice.Newf("%s", notice)) + return false, errors.WithHint(err, notice) + } + + return true, nil +} + +// checkPrivilegesForRestore checks that the user has sufficient privileges to +// run a cluster or a database restore. A table restore requires us to know the +// parent database we will be writing to and so that happens at a later stage of +// restore planning. +// +// This method is also responsible for checking the privileges on the +// destination URIs the restore is reading from. +func checkPrivilegesForRestore( + ctx context.Context, restoreStmt *tree.Restore, p sql.PlanHookState, from [][]string, +) error { + // If the user is admin no further checks need to be performed. + hasAdmin, err := p.HasAdminRole(ctx) + if err != nil { + return err + } + if hasAdmin { + return nil + } + + { + // Cluster and tenant restores require the `RESTORE` system privilege for + // non-admin users. + requiresRestoreSystemPrivilege := restoreStmt.DescriptorCoverage == tree.AllDescriptors || + restoreStmt.Targets.TenantID.IsSet() + + var hasRestoreSystemPrivilege bool + if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) { + err := p.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, + privilege.RESTORE, p.User()) + hasRestoreSystemPrivilege = err == nil + } + + if requiresRestoreSystemPrivilege && hasRestoreSystemPrivilege { + return checkRestoreDestinationPrivileges(ctx, p, from) + } else if requiresRestoreSystemPrivilege && !hasRestoreSystemPrivilege { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role or the RESTORE system privilege are allowed to perform"+ + " a cluster restore") + } + } + + var hasRestoreSystemPrivilege bool + // If running a database restore, check that the user has the `RESTORE` system + // privilege. + // + // TODO(adityamaru): In 23.1 a missing `RESTORE` privilege should return an + // error. In 22.2 we continue to check for old style privileges and role + // options. + if len(restoreStmt.Targets.Databases) > 0 { + if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) { + err := p.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, + privilege.RESTORE, p.User()) + hasRestoreSystemPrivilege = err == nil + } + } + + if hasRestoreSystemPrivilege { + return checkRestoreDestinationPrivileges(ctx, p, from) + } + + // The following checks are to maintain compatability with pre-22.2 privilege + // requirements to run the backup. If we have failed to find the appropriate + // `RESTORE` privileges, we default to our old-style privilege checks and + // buffer a notice urging users to switch to `RESTORE` privileges. + // + // TODO(adityamaru): Delete deprecated privilege checks in 23.1. Users will be + // required to have the appropriate `RESTORE` privilege instead. + // + // Database restores require the CREATEDB privileges. + if len(restoreStmt.Targets.Databases) > 0 { + notice := fmt.Sprintf("%s RESTORE DATABASE, user %s will exclusively require the "+ + "RESTORE system privilege.", deprecatedPrivilegesPreamble, p.User().Normalized()) + p.BufferClientNotice(ctx, pgnotice.Newf("%s", notice)) + + hasCreateDB, err := p.HasRoleOption(ctx, roleoption.CREATEDB) + if err != nil { + return err + } + if !hasCreateDB { + return errors.WithHint(pgerror.Newf( + pgcode.InsufficientPrivilege, + "only users with the CREATEDB privilege can restore databases"), notice) + } + } + + return checkRestoreDestinationPrivileges(ctx, p, from) +} + func checkClusterRegions( ctx context.Context, p sql.PlanHookState, typesByID map[descpb.ID]*typedesc.Mutable, ) error { diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions index 57ebafc1a77b..e02cd5f31e2c 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions +++ b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions @@ -63,13 +63,13 @@ exec-sql user=testuser BACKUP DATABASE d INTO 'userfile:///test-nonroot-db'; ---- pq: user testuser does not have SELECT privilege on relation t -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d. exec-sql user=testuser BACKUP TABLE d.t INTO 'userfile:///test-nonroot-table'; ---- pq: user testuser does not have SELECT privilege on relation t -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. exec-sql REVOKE SYSTEM BACKUP FROM testuser; @@ -87,7 +87,7 @@ exec-sql user=testuser BACKUP DATABASE d INTO 'userfile:///test-nonroot-db' ---- pq: user testuser does not have SELECT privilege on relation t -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d. # Grant the user `BACKUP` on the database. exec-sql @@ -111,7 +111,7 @@ exec-sql user=testuser BACKUP TABLE d.t INTO 'userfile:///test-nonroot-table' ---- pq: user testuser does not have SELECT privilege on relation t -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. # Set the default privileges on the database and create a new table. exec-sql @@ -160,19 +160,19 @@ exec-sql user=testuser BACKUP TABLE d.t INTO 'userfile:///test-nonroot-table' ---- pq: user testuser does not have SELECT privilege on relation t -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. exec-sql user=testuser BACKUP TABLE d.t2 INTO 'userfile:///test-nonroot-table' ---- pq: user testuser does not have SELECT privilege on relation t2 -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t2. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t2. exec-sql user=testuser BACKUP TABLE d.t3 INTO 'userfile:///test-nonroot-table' ---- pq: user testuser does not have SELECT privilege on relation t3 -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t3. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t3. exec-sql GRANT BACKUP ON TABLE d.t,d.t2,d.t3 TO testuser; @@ -200,7 +200,7 @@ exec-sql user=testuser BACKUP TABLE d.foo.t1 INTO 'userfile:///test-nonroot-table' ---- pq: user testuser does not have USAGE privilege on schema foo -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t1. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t1. exec-sql GRANT BACKUP ON TABLE foo.t1 TO testuser; diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated index a2e480eeb838..e91a9e05dd5c 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated +++ b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated @@ -68,7 +68,7 @@ exec-sql user=testuser BACKUP DATABASE d2 TO 'nodelocal://0/d2' ---- pq: user testuser does not have CONNECT privilege on database d2 -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. exec-sql GRANT CONNECT ON DATABASE d2 TO testuser; @@ -79,7 +79,7 @@ exec-sql user=testuser BACKUP TABLE d2.t INTO 'nodelocal://0/d2-table' ---- pq: user testuser does not have SELECT privilege on relation t -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. exec-sql GRANT SELECT ON TABLE d2.t TO testuser; @@ -95,7 +95,7 @@ exec-sql user=testuser BACKUP DATABASE d2 INTO 'nodelocal://0/d2-schema'; ---- pq: user testuser does not have USAGE privilege on schema sc2 -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. exec-sql GRANT USAGE ON SCHEMA sc2 TO testuser; @@ -114,7 +114,7 @@ exec-sql user=testuser BACKUP DATABASE d2 INTO 'nodelocal://0/d2-schema'; ---- pq: user testuser does not have USAGE privilege on type greeting -HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. exec-sql GRANT USAGE ON TYPE d2.greeting TO testuser; @@ -124,12 +124,12 @@ GRANT USAGE ON TYPE d2.greeting TO testuser; exec-sql server=s2 user=testuser BACKUP DATABASE d2 INTO 'nodelocal://0/d2'; ---- -NOTICE: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d2. exec-sql server=s2 user=testuser BACKUP TABLE d2.t INTO 'nodelocal://0/d2-table'; ---- -NOTICE: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t. # Test that implicit access is disallowed when the testing knob is not set. new-server name=s3 share-io-dir=s1 diff --git a/pkg/ccl/backupccl/testdata/backup-restore/external-connections-privileges b/pkg/ccl/backupccl/testdata/backup-restore/external-connections-privileges index 01f5e7638fab..e27a5d3e847b 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/external-connections-privileges +++ b/pkg/ccl/backupccl/testdata/backup-restore/external-connections-privileges @@ -36,7 +36,7 @@ GRANT SELECT ON TABLE foo TO testuser exec-sql user=testuser BACKUP TABLE foo INTO 'external://testuser-ec' ---- -NOTICE: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: foo. +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: foo. exec-sql GRANT USAGE ON EXTERNAL CONNECTION "testuser-ec" TO testuser; @@ -45,7 +45,7 @@ GRANT USAGE ON EXTERNAL CONNECTION "testuser-ec" TO testuser; exec-sql user=testuser BACKUP TABLE foo INTO LATEST IN 'external://testuser-ec' ---- -NOTICE: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: foo. +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: foo. # Sanity check that the user can't write to any other external connection. exec-sql user=testuser @@ -91,5 +91,6 @@ GRANT CREATE ON DATABASE failsdb TO testuser; exec-sql user=testuser RESTORE TABLE foo FROM LATEST IN 'external://testuser-ec' WITH into_db=failsdb; ---- +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE TABLE, user testuser will exclusively require the RESTORE privilege on databases failsdb subtest end diff --git a/pkg/ccl/backupccl/testdata/backup-restore/rangekeys b/pkg/ccl/backupccl/testdata/backup-restore/rangekeys index 26677222a2c4..eeccd607bdd8 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/rangekeys +++ b/pkg/ccl/backupccl/testdata/backup-restore/rangekeys @@ -79,5 +79,3 @@ query-sql SELECT count(*) from orig1.baz ---- 1 - - diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-grants b/pkg/ccl/backupccl/testdata/backup-restore/restore-grants index f695409b56d0..9cd558f7eacf 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-grants +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-grants @@ -249,11 +249,13 @@ GRANT CREATE ON DATABASE testuser_db TO testuser; exec-sql user=testuser RESTORE testdb.sc.othertable FROM LATEST IN 'nodelocal://1/test' WITH into_db='testuser_db'; ---- +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE TABLE, user testuser will exclusively require the RESTORE privilege on databases testuser_db # Restore a table where only `testuser` had privileges. exec-sql user=testuser RESTORE testdb.testtable_greeting_usage FROM LATEST IN 'nodelocal://1/test' WITH into_db='testuser_db'; ---- +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE TABLE, user testuser will exclusively require the RESTORE privilege on databases testuser_db exec-sql USE testuser_db @@ -505,6 +507,7 @@ subtest restore-database-as-non-admin exec-sql user=testuser RESTORE DATABASE testdb FROM LATEST IN 'nodelocal://0/test/'; ---- +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE DATABASE, user testuser will exclusively require the RESTORE system privilege. exec-sql USE testdb diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions b/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions index fb7849ef48ad..4f69e5f12ecb 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions @@ -1,102 +1,186 @@ -# Test permissions checks for non-admin users running RESTORE. -new-server name=s1 +# We allow implicit access to non-admins as well to simplify our test of +# grantable privileges. +new-server name=s1 allow-implicit-access ---- exec-sql CREATE DATABASE d; -CREATE TABLE d.t (x INT); -INSERT INTO d.t VALUES (1), (2), (3); +CREATE SCHEMA d.ds; +CREATE TYPE d.ds.typ AS ENUM ('hello', 'hi'); +CREATE TABLE d.ds.t (x INT PRIMARY KEY, y d.ds.typ); +INSERT INTO d.ds.t VALUES (1, 'hi'), (2, 'hello'), (3, 'hi'); ---- +# Take a cluster backup that can be used to test cluster, database and tables +# restores. exec-sql -BACKUP INTO 'nodelocal://0/test/' +BACKUP INTO 'nodelocal://1/foo'; ---- -# Restores should succeed as a non-root user with admin role. +subtest database-restore + +# Root can run every restore. +exec-sql +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/foo' WITH new_db_name = ddup; +---- + +query-sql +SELECT * FROM ddup.ds.t; +---- +1 hi +2 hello +3 hi + +# Non-root admin can run every restore. exec-sql CREATE USER testuser; GRANT ADMIN TO testuser; ---- exec-sql user=testuser -DROP DATABASE d; +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/foo' WITH new_db_name = ddup_nonroot; +---- + +query-sql user=testuser +SELECT * FROM ddup_nonroot.ds.t; +---- +1 hi +2 hello +3 hi + +exec-sql +REVOKE ADMIN FROM testuser; ---- +# Non-admin cannot run database restore unless they have CREATEDB today but we recommend the system +# RESTORE privilege. exec-sql user=testuser -RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/test/'; +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/foo' WITH new_db_name = ddup_nonroot_fail; ---- +pq: only users with the CREATEDB privilege can restore databases +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE DATABASE, user testuser will exclusively require the RESTORE system privilege. -# Start a new cluster with the same IO dir. -new-server name=s2 share-io-dir=s1 allow-implicit-access +exec-sql +GRANT SYSTEM RESTORE TO testuser; ---- -exec-sql server=s2 -CREATE USER testuser +exec-sql user=testuser +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/foo' WITH new_db_name = ddup_nonroot_fail; ---- -# Restore into the new cluster. -exec-sql server=s2 user=testuser -RESTORE FROM LATEST IN 'nodelocal://0/test/' +query-sql +SELECT * FROM ddup_nonroot_fail.ds.t; ---- -pq: only users with the admin role are allowed to restore full cluster backups +1 hi +2 hello +3 hi -exec-sql server=s2 user=testuser -RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/test/' +exec-sql +REVOKE SYSTEM RESTORE FROM testuser; ---- -pq: only users with the CREATEDB privilege can restore databases -exec-sql server=s2 -CREATE DATABASE d +subtest end + +subtest table-restore + +exec-sql +CREATE DATABASE root; ---- -exec-sql server=s2 user=testuser -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test/' +# Root can run every restore. +exec-sql +RESTORE TABLE d.ds.t FROM LATEST IN 'nodelocal://1/foo' WITH into_db = root; ---- -pq: user testuser does not have CREATE privilege on database d -exec-sql server=s2 -GRANT CREATE ON DATABASE d TO testuser +query-sql +SELECT * FROM root.ds.t; ---- +1 hi +2 hello +3 hi -exec-sql server=s2 user=testuser -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test/' +# Non-root admin can run every restore. +exec-sql +GRANT ADMIN TO testuser; ---- -query-sql server=s2 -SELECT x FROM d.t ORDER BY x +exec-sql user=testuser +CREATE DATABASE nonroot_admin; ---- -1 -2 -3 -exec-sql server=s2 -DROP DATABASE d +exec-sql user=testuser +RESTORE TABLE d.ds.t FROM LATEST IN 'nodelocal://1/foo' WITH into_db = nonroot_admin; ---- -exec-sql server=s2 -ALTER USER testuser CREATEDB +query-sql user=testuser +SELECT * FROM nonroot_admin.ds.t; ---- +1 hi +2 hello +3 hi -exec-sql server=s2 user=testuser -RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/test/' +exec-sql +REVOKE ADMIN FROM testuser; ---- -query-sql server=s2 -SELECT x FROM d.t ORDER BY x +exec-sql +CREATE DATABASE nonadmin; ---- -1 -2 -3 -# Test that implicit access is disallowed when the testing knob isn't set. -new-server name=s3 share-io-dir=s1 +# Non-admin cannot run table restore unless they have CREATE on the database +# today but we recommend the database RESTORE privilege. +exec-sql user=testuser +RESTORE TABLE d.ds.t FROM LATEST IN 'nodelocal://1/foo' WITH into_db = nonadmin; ---- +pq: user testuser does not have CREATE privilege on database nonadmin +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE TABLE, user testuser will exclusively require the RESTORE privilege on database nonadmin. -exec-sql server=s3 -CREATE USER testuser +exec-sql +GRANT RESTORE ON DATABASE nonadmin TO testuser; +---- + +exec-sql user=testuser +RESTORE TABLE d.ds.t FROM LATEST IN 'nodelocal://1/foo' WITH into_db = nonadmin; +---- + +query-sql user=testuser +SELECT * FROM nonadmin.ds.t; +---- +1 hi +2 hello +3 hi + +query-sql user=testuser +USE nonadmin; +SHOW TYPES; +---- +ds typ testuser + +exec-sql +REVOKE RESTORE ON DATABASE nonadmin FROM testuser; +---- + +subtest end + +subtest cluster-restore +new-server name=s2 allow-implicit-access share-io-dir=s1 ---- -exec-sql server=s3 user=testuser -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test/' +exec-sql +CREATE USER testuser; ---- -pq: only users with the admin role are allowed to RESTORE from the specified nodelocal URI + +exec-sql user=testuser +RESTORE FROM LATEST IN 'nodelocal://1/foo'; +---- +pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore + +exec-sql +GRANT SYSTEM RESTORE TO testuser; +---- + +exec-sql user=testuser +RESTORE FROM LATEST IN 'nodelocal://1/foo'; +---- + +subtest end diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions-deprecated b/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions-deprecated new file mode 100644 index 000000000000..a95052537f36 --- /dev/null +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-permissions-deprecated @@ -0,0 +1,106 @@ +# Test permissions checks for non-admin users running RESTORE. +new-server name=s1 +---- + +exec-sql +CREATE DATABASE d; +CREATE TABLE d.t (x INT); +INSERT INTO d.t VALUES (1), (2), (3); +---- + +exec-sql +BACKUP INTO 'nodelocal://0/test/' +---- + +# Restores should succeed as a non-root user with admin role. +exec-sql +CREATE USER testuser; +GRANT ADMIN TO testuser; +---- + +exec-sql user=testuser +DROP DATABASE d; +---- + +exec-sql user=testuser +RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/test/'; +---- + +# Start a new cluster with the same IO dir. +new-server name=s2 share-io-dir=s1 allow-implicit-access +---- + +exec-sql server=s2 +CREATE USER testuser +---- + +# Restore into the new cluster. +exec-sql server=s2 user=testuser +RESTORE FROM LATEST IN 'nodelocal://0/test/' +---- +pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore + +exec-sql server=s2 user=testuser +RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/test/' +---- +pq: only users with the CREATEDB privilege can restore databases +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE DATABASE, user testuser will exclusively require the RESTORE system privilege. + +exec-sql server=s2 +CREATE DATABASE d +---- + +exec-sql server=s2 user=testuser +RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test/' +---- +pq: user testuser does not have CREATE privilege on database d +HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE TABLE, user testuser will exclusively require the RESTORE privilege on database d. + +exec-sql server=s2 +GRANT CREATE ON DATABASE d TO testuser +---- + +exec-sql server=s2 user=testuser +RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test/' +---- +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE TABLE, user testuser will exclusively require the RESTORE privilege on databases d + +query-sql server=s2 +SELECT x FROM d.t ORDER BY x +---- +1 +2 +3 + +exec-sql server=s2 +DROP DATABASE d +---- + +exec-sql server=s2 +ALTER USER testuser CREATEDB +---- + +exec-sql server=s2 user=testuser +RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/test/' +---- +NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here . In a future release, to run RESTORE DATABASE, user testuser will exclusively require the RESTORE system privilege. + +query-sql server=s2 +SELECT x FROM d.t ORDER BY x +---- +1 +2 +3 + +# Test that implicit access is disallowed when the testing knob isn't set. +new-server name=s3 share-io-dir=s1 +---- + +exec-sql server=s3 +CREATE USER testuser +---- + +exec-sql server=s3 user=testuser +RESTORE TABLE d.t FROM LATEST IN 'nodelocal://0/test/' +---- +pq: only users with the admin role are allowed to RESTORE from the specified nodelocal URI diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-schema-only b/pkg/ccl/backupccl/testdata/backup-restore/restore-schema-only index 623a8c43c917..b94b1b9ac5bb 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-schema-only +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-schema-only @@ -71,7 +71,7 @@ CREATE USER testuser exec-sql user=testuser RESTORE FROM LATEST IN 'nodelocal://0/full_cluster_backup/' with schema_only ---- -pq: only users with the admin role are allowed to restore full cluster backups +pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore # Fail fast using a database backup exec-sql diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-validation-only b/pkg/ccl/backupccl/testdata/backup-restore/restore-validation-only index 86974f1e08da..402804313ef8 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-validation-only +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-validation-only @@ -77,7 +77,7 @@ CREATE USER testuser exec-sql user=testuser RESTORE FROM LATEST IN 'nodelocal://0/full_cluster_backup/' with schema_only, verify_backup_table_data ---- -pq: only users with the admin role are allowed to restore full cluster backups +pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore # Fail fast using a database backup exec-sql diff --git a/pkg/sql/catalog/catpb/privilege_test.go b/pkg/sql/catalog/catpb/privilege_test.go index 2695eccef09b..50bfee88267c 100644 --- a/pkg/sql/catalog/catpb/privilege_test.go +++ b/pkg/sql/catalog/catpb/privilege_test.go @@ -113,7 +113,7 @@ func TestPrivilege(t *testing.T) { }, // Ensure revoking USAGE from a user with ALL privilege on a type // leaves the user with no privileges. - {testUser, privilege.List{privilege.ALL}, privilege.List{privilege.BACKUP, privilege.USAGE}, + {testUser, privilege.List{privilege.ALL}, privilege.List{privilege.USAGE}, []catpb.UserPrivilege{ {username.AdminRoleName(), []privilege.Privilege{{Kind: privilege.ALL, GrantOption: true}}}, }, @@ -139,12 +139,12 @@ func TestPrivilege(t *testing.T) { }, privilege.Table, }, - // Ensure revoking BACKUP, CONNECT, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG + // Ensure revoking BACKUP, CONNECT, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG, RESTORE // from a user with ALL privilege on a database leaves the user with no privileges. {testUser, privilege.List{privilege.ALL}, privilege.List{privilege.BACKUP, privilege.CONNECT, privilege.CREATE, privilege.DROP, privilege.SELECT, - privilege.INSERT, privilege.DELETE, privilege.UPDATE, privilege.ZONECONFIG}, + privilege.INSERT, privilege.DELETE, privilege.UPDATE, privilege.ZONECONFIG, privilege.RESTORE}, []catpb.UserPrivilege{ {username.AdminRoleName(), []privilege.Privilege{{Kind: privilege.ALL, GrantOption: true}}}, }, diff --git a/pkg/sql/logictest/testdata/logic_test/grant_database b/pkg/sql/logictest/testdata/logic_test/grant_database index 029bc4f19d21..8b586533f046 100644 --- a/pkg/sql/logictest/testdata/logic_test/grant_database +++ b/pkg/sql/logictest/testdata/logic_test/grant_database @@ -59,11 +59,13 @@ a public CONNECT false a readwrite BACKUP true a readwrite CREATE true a readwrite DROP true +a readwrite RESTORE true a readwrite ZONECONFIG true a root ALL true a test-user BACKUP true a test-user CREATE true a test-user DROP true +a test-user RESTORE true a test-user ZONECONFIG true query TTTB @@ -72,10 +74,12 @@ SHOW GRANTS ON DATABASE a FOR readwrite, "test-user" a readwrite BACKUP true a readwrite CREATE true a readwrite DROP true +a readwrite RESTORE true a readwrite ZONECONFIG true a test-user BACKUP true a test-user CREATE true a test-user DROP true +a test-user RESTORE true a test-user ZONECONFIG true statement ok @@ -89,10 +93,12 @@ a public CONNECT false a readwrite BACKUP true a readwrite CREATE true a readwrite DROP true +a readwrite RESTORE true a readwrite ZONECONFIG true a root ALL true a test-user BACKUP true a test-user DROP true +a test-user RESTORE true a test-user ZONECONFIG true statement ok @@ -104,6 +110,7 @@ SHOW GRANTS ON DATABASE a FOR readwrite, "test-user" a readwrite BACKUP true a readwrite CREATE true a readwrite DROP true +a readwrite RESTORE true a readwrite ZONECONFIG true statement ok diff --git a/pkg/sql/privilege/kind_string.go b/pkg/sql/privilege/kind_string.go index 88396d52245f..c309f0da2637 100644 --- a/pkg/sql/privilege/kind_string.go +++ b/pkg/sql/privilege/kind_string.go @@ -31,11 +31,12 @@ func _() { _ = x[VIEWCLUSTERMETADATA-21] _ = x[VIEWDEBUG-22] _ = x[BACKUP-23] + _ = x[RESTORE-24] } -const _Kind_name = "ALLCREATEDROPGRANTSELECTINSERTDELETEUPDATEUSAGEZONECONFIGCONNECTRULEMODIFYCLUSTERSETTINGEXTERNALCONNECTIONVIEWACTIVITYVIEWACTIVITYREDACTEDVIEWCLUSTERSETTINGCANCELQUERYNOSQLLOGINEXECUTEVIEWCLUSTERMETADATAVIEWDEBUGBACKUP" +const _Kind_name = "ALLCREATEDROPGRANTSELECTINSERTDELETEUPDATEUSAGEZONECONFIGCONNECTRULEMODIFYCLUSTERSETTINGEXTERNALCONNECTIONVIEWACTIVITYVIEWACTIVITYREDACTEDVIEWCLUSTERSETTINGCANCELQUERYNOSQLLOGINEXECUTEVIEWCLUSTERMETADATAVIEWDEBUGBACKUPRESTORE" -var _Kind_index = [...]uint8{0, 3, 9, 13, 18, 24, 30, 36, 42, 47, 57, 64, 68, 88, 106, 118, 138, 156, 167, 177, 184, 203, 212, 218} +var _Kind_index = [...]uint8{0, 3, 9, 13, 18, 24, 30, 36, 42, 47, 57, 64, 68, 88, 106, 118, 138, 156, 167, 177, 184, 203, 212, 218, 225} func (i Kind) String() string { i -= 1 diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go index c0386c90ab71..0f0abb6fc318 100644 --- a/pkg/sql/privilege/privilege.go +++ b/pkg/sql/privilege/privilege.go @@ -56,6 +56,7 @@ const ( VIEWCLUSTERMETADATA Kind = 21 VIEWDEBUG Kind = 22 BACKUP Kind = 23 + RESTORE Kind = 24 ) // Privilege represents a privilege parsed from an Access Privilege Inquiry @@ -109,11 +110,11 @@ var isDescriptorBacked = map[ObjectType]bool{ // Predefined sets of privileges. var ( - AllPrivileges = List{ALL, CONNECT, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, USAGE, ZONECONFIG, EXECUTE, BACKUP} + AllPrivileges = List{ALL, CONNECT, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, USAGE, ZONECONFIG, EXECUTE, BACKUP, RESTORE} ReadData = List{SELECT} ReadWriteData = List{SELECT, INSERT, DELETE, UPDATE} ReadWriteSequenceData = List{SELECT, UPDATE, USAGE} - DBPrivileges = List{ALL, BACKUP, CONNECT, CREATE, DROP, ZONECONFIG} + DBPrivileges = List{ALL, BACKUP, CONNECT, CREATE, DROP, RESTORE, ZONECONFIG} TablePrivileges = List{ALL, BACKUP, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG} SchemaPrivileges = List{ALL, CREATE, USAGE} TypePrivileges = List{ALL, USAGE} @@ -123,7 +124,7 @@ var ( // certain privileges unavailable after upgrade migration. // Note that "CREATE, INSERT, DELETE, ZONECONFIG" are no-op privileges on sequences. SequencePrivileges = List{ALL, USAGE, SELECT, UPDATE, CREATE, DROP, INSERT, DELETE, ZONECONFIG} - GlobalPrivileges = List{ALL, BACKUP, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG} + GlobalPrivileges = List{ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG} VirtualTablePrivileges = List{ALL, SELECT} ExternalConnectionPrivileges = List{ALL, USAGE, DROP} ) @@ -169,6 +170,7 @@ var ByName = map[string]Kind{ "VIEWCLUSTERMETADATA": VIEWCLUSTERMETADATA, "VIEWDEBUG": VIEWDEBUG, "BACKUP": BACKUP, + "RESTORE": RESTORE, } // List is a list of privileges. @@ -360,7 +362,8 @@ var orderedPrivs = List{CREATE, USAGE, INSERT, CONNECT, DELETE, SELECT, UPDATE, // ListToACL converts a list of privileges to a list of Postgres // ACL items. // See: https://www.postgresql.org/docs/13/ddl-priv.html#PRIVILEGE-ABBREVS-TABLE -// for privileges and their ACL abbreviations. +// +// for privileges and their ACL abbreviations. func (pl List) ListToACL(grantOptions List, objectType ObjectType) string { privileges := pl // If ALL is present, explode ALL into the underlying privileges. From f3f5cc534d2ec3d4e05a0cf75a1b79a513bc6af4 Mon Sep 17 00:00:00 2001 From: adityamaru Date: Tue, 23 Aug 2022 14:49:20 -0400 Subject: [PATCH 3/3] bulk: fix missing memory accounting call This change fixes a memory accounting bug where we were appending a KV to the kvBuf without accounting for its memory. Release note (bug fix): adds a missing memory accounting call when appending a KV to the underlying kvBuf Release justification: low risk bug fix that prevents an import from panicking on non-release builds --- pkg/kv/bulk/buffering_adder.go | 15 ++++++++++++++- pkg/kv/bulk/kv_buf.go | 2 +- pkg/kv/bulk/sst_batcher_test.go | 4 ++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/kv/bulk/buffering_adder.go b/pkg/kv/bulk/buffering_adder.go index 86e58197e1f8..bd272fa80d6f 100644 --- a/pkg/kv/bulk/buffering_adder.go +++ b/pkg/kv/bulk/buffering_adder.go @@ -191,7 +191,20 @@ func (b *BufferingAdder) Add(ctx context.Context, key roachpb.Key, value []byte) b.underfill = 0 } - return b.curBuf.append(key, value) + // At this point we've flushed the buffer which implies that the slab and + // entries slices have been emptied out. Furthermore, we may have nil'ed out + // the slab and entries slices if we identified the allocation as skewed in + // the condition above. In both cases we should always be able to fit the key + // and value, so this call is to ensure proper reallocation and memory + // monitoring. + if b.curBuf.fits(ctx, need, sz(b.maxBufferLimit()), &b.memAcc) { + return b.curBuf.append(key, value) + } + // If we are unable to fit this key and value even after flushing then we + // should error since we do not have enough headroom to buffer this single + // key+value. + return errors.Newf("failed to fit KV of size %d in the buffering adder, "+ + "current max buffer limit is %d", need, b.maxBufferLimit()) } func (b *BufferingAdder) bufferedKeys() int { diff --git a/pkg/kv/bulk/kv_buf.go b/pkg/kv/bulk/kv_buf.go index 86ea3260c9f2..adc805d54799 100644 --- a/pkg/kv/bulk/kv_buf.go +++ b/pkg/kv/bulk/kv_buf.go @@ -188,7 +188,7 @@ func (b *kvBuf) Swap(i, j int) { b.entries[i], b.entries[j] = b.entries[j], b.entries[i] } -func (b kvBuf) MemSize() sz { +func (b *kvBuf) MemSize() sz { return sz(cap(b.entries)<