diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go index 9e68a76e8b58..b5b4c594bf4a 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 { @@ -354,10 +357,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 { @@ -367,8 +370,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 { @@ -380,7 +381,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) @@ -393,7 +394,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, ", ")) } @@ -495,7 +496,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 37b3c66e8fb0..a25aeba3b889 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 @@ -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 501ff9e709d9..ee770d97b883 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/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", ], ) 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)<