Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backupccl,ingesting: support RESTORE WITH preserve_grants_for=<users> #79144

Closed
wants to merge 3 commits into from

Conversation

adityamaru
Copy link
Contributor

Co-authored-by: casper@cockroachlabs.com

Fixes: #45359

Release note (enterprise change): This change adds an option to database
and table level restores to preserve_grants_for user(s) when restoring
the table or database.

A cluster restore is not allowed to specify this option since we already
restore all users as part of the system.users table, and therefore the descriptor
privileges are restored exactly how they were backed up.

In the case of table and database level restores, the users specified in
the preserve_grants_for option must be present in the restoring cluster.
If they are, then the privileges on the restored database, schema, type and
table descriptors for those users will be restored exactly how they were backed
up. Privileges for users not specified in the preserve_grants_for list will
continue to be wiped and the descriptors will be restored with the "default"
privileges as they are today.

Previously, when restoring a table descriptor as part of a non-cluster
backup, we were incorrectly generating its privileges without passing in
the parent schema's DefaultPrivilegeDescriptor. This change fixes that.

Additionally, this change rewrites the `restore-grants` datadriven test
to be more understandble. There were also some errors in the test that have
been fixed in this change. The expected behavior is that:

1) A cluster restore will just restore all privileges as was backed up.

2) A database restore will first generate ALL privileges for root and admin on the
database, and subsequent table, type, schema descriptors will inherit these privileges.

3) A table restore into an existing database will generate its set of privileges
based on the default privileges of the parentDB and parentSchema.

All restored schema objects are owned by the user running the restore.

All backed up privileges on these descriptors are wiped, since there is no
guarantee that a backed up user will be present in the restoring cluster.

Release note (bug fix): Privileges for restored table's were being generated
incorrectly without taking into consideration their parent schema's default privilege
descriptor.
The number of arguments was ballooning so this change passes in
the params wrapped in a struct instead.

Release note: None
Co-authored-by: casper@cockroachlabs.com

Fixes: cockroachdb#45359

Release note (enterprise change): This change adds an option to database
and table level restores to `preserve_grants_for` user(s) when restoring
the table or database.

A cluster restore is not allowed to specify this option since we already
restore all users as part of the `system.users` table, and therefore the descriptor
privileges are restored exactly how they were backed up.

In the case of table and database level restores, the users specified in
the `preserve_grants_for` option must be present in the restoring cluster.
If they are, then the privileges on the restored database, schema, type and
table descriptors for those users will be restored exactly how they were backed
up. Privileges for users not specified in the `preserve_grants_for` list will
continue to be wiped and the descriptors will be restored with the "default"
privileges as they are today.
@adityamaru adityamaru requested a review from a team as a code owner March 31, 2022 16:43
@adityamaru adityamaru requested review from a team March 31, 2022 16:43
@adityamaru adityamaru requested a review from a team as a code owner March 31, 2022 16:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru removed request for a team March 31, 2022 16:43
@adityamaru
Copy link
Contributor Author

First commit is from #79085.

@adityamaru
Copy link
Contributor Author

@RichardJCai sorry to put this on your plate so late into the release cycle but its a work item that slipped in the shuffle and we might want to consider a backport. I'm going to lean on your understanding of the privilege model to make sure what I'm doing here is sane, I've tried to comment my thinking around the major code changes.

@rafiss
Copy link
Collaborator

rafiss commented Jun 28, 2022

What is the plan with this PR?

@RichardJCai
Copy link
Contributor

What is the plan with this PR?

I asked Aditya to hold off on merging this a while back until the on-going privilege work is done. We should be able to get back to this soon

@livlobo
Copy link
Contributor

livlobo commented Sep 16, 2022

@adityamaru @RichardJCai - can this PR be merged now?

@data-matt
Copy link

Hi folks,

Can we provide a wildcard for this?

Something like:
RESTORE WITH preserve_grants_for=*

The problem is, it will be an overhead for the customer to loop through and find the users themselves.

Thanks

@data-matt
Copy link

Hey folks, whats the latest on this? @adityamaru

@adityamaru
Copy link
Contributor Author

This is in our backlog but hasn't been prioritized, when we do pick this up we're probably going to have to start from scratch given how much has changed. cc: @livlobo

@adityamaru adityamaru closed this May 3, 2023
@data-matt
Copy link

@adityamaru , this was closed, what happened? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backupccl: Add option to RESTORE to preserve grants to specified users
7 participants