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

sql/builtins: remove the root special case #76518

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Feb 14, 2022

Discovered by @dt. This was leftover complexity from an earlier age.

Release note (sql change): The buil-in functions
crdb_internal.force_panic, crdb_internal.force_log_fatal,
crdb_internal.set_vmodule, crdb_internal.get_vmodule are now
available to all admin users, not just root.

Release note (sql change): The buil-in functions
`crdb_internal.force_panic`, `crdb_internal.force_log_fatal`,
`crdb_internal.set_vmodule`, `crdb_internal.get_vmodule` are now
available to all `admin` users, not just `root`.
@knz knz requested a review from a team February 14, 2022 17:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt
Copy link
Member

dt commented Feb 14, 2022

Does this seem like a bug / do we think a backport would be (in)appropriate here?

@knz
Copy link
Contributor Author

knz commented Feb 14, 2022

I don't see it as a bug unless you can point me to a doc that spells out these specific functions should have been allowed to all admin users from the start.

I put this PR out as a general v22.1 enhancement, but happy to backport if you have the evidence it's a bug fix.

@rafiss
Copy link
Collaborator

rafiss commented Feb 14, 2022

lgtm, but one question: this would mean that a CC admin user (either serverless or dedicated) could now cause a lot of extra logging. does our SRE team already expect that?

@knz
Copy link
Contributor Author

knz commented Feb 14, 2022

a CC admin user (either serverless or dedicated) could now cause a lot of extra logging

They could do so already via cluster settings (statements exec log, txn tracing, session log).

In dedicated they could also do it via the /debug/vmodule HTTP endpoint.

@knz
Copy link
Contributor Author

knz commented Feb 14, 2022

I do see your point with serverless though. These don't have access to cluster settings (yet, but will soon) and don't have access to the HTTP service.

Perhaps we're missing a big picture analysis of what opening cluster settings to serverless admins will enable?

@dt
Copy link
Member

dt commented Feb 14, 2022

I'm sure you can already find a statement or query that logs and just execute it a bunch if you want to generate more log-lines, so don't know that locking admins out of vmodule is really buying us protection there, vs just making things difficult/annoying to debug.

@knz
Copy link
Contributor Author

knz commented Feb 14, 2022

thanks

bors r=rafiss,dt

@craig
Copy link
Contributor

craig bot commented Feb 14, 2022

Build succeeded:

@craig craig bot merged commit 07a3683 into cockroachdb:master Feb 14, 2022
@knz knz deleted the 20220214-admin branch February 15, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants