-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
catalog: add descriptor repair to remove missing roles #122557
Conversation
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
74a6be3
to
699f5a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this should be really useful! just had a comment
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/sem/builtins/builtins.go
line 5454 at r1 (raw file):
for _, roleDatum := range (*roles).Array { role := tree.MustBeDString(roleDatum) roleMap[username.MakeSQLUsernameFromPreNormalizedString(string(role))] = struct{}{}
this can come from user input, so we must use MakeSQLUsernameFromUserInput
instead.
Previously, we had a bug that could lead to descriptors having privileages to roles that no longer exist. This could lead to certain commands like SHOW GRANTS breaking. To address this, this patch will add descirptor repair logic to automatically clean up oprhaned privileges. Fixes: cockroachdb#122552 Release note (bug fix): Add automated clean up / validation for dropped roles inside descriptors.
699f5a6
to
91b2074
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/sem/builtins/builtins.go
line 5454 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this can come from user input, so we must use
MakeSQLUsernameFromUserInput
instead.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! could we backport to 24.1?
Reviewed 23 of 24 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
@rafiss TFTR! bors r+ |
Build failed (retrying...): |
blathers backport 23.2 |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 91b2074 to blathers/backport-release-23.2-122557: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, we had a bug that could lead to descriptors having privileages to roles that no longer exist. This could lead to certain commands like SHOW GRANTS breaking. To address this, this patch will add descirptor repair logic to automatically clean up oprhaned privileges.
Fixes: #122552
Release note (bug fix): Add automated clean up / validation for dropped roles inside descriptors.