-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: include current user in EXPLAIN bundle #143671
sql: include current user in EXPLAIN bundle #143671
Conversation
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! I have a couple of comments / questions.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/explain_bundle.go
line 556 at r1 (raw file):
fmt.Fprintf(&buf, "\n") c.PrintUser(&buf)
Do you think we should include the stmt(s) to actually create the user / role into the bundle too? We try to make our bundles easily recreateable (via cockroach debug sb recreate
command), and I think right now the bundle recreation will fail on a "custom" user. Perhaps we actually want to just automatically comment out SET role ...
stmt in env.sql
?
BTW we have a handful of test_sb_recreate*
acceptance docker cli tests for this.
pkg/sql/explain_bundle.go
line 1017 at r1 (raw file):
func (c *stmtEnvCollector) PrintUser(w io.Writer) { fmt.Fprintf(w, "-- User: %s\n", c.p.User())
nit: double-checking - is "user" is different from current "role" ? We will already include role
session variable in the form like
SET role = rls_user; -- default value: none
into env.sql
(we include all session variables that differ from their defaults, in addition to a handful of read-only session vars).
pkg/sql/explain_bundle_test.go
line 986 at r1 (raw file):
) r.Exec(t, "SET ROLE root")
nit: might be a bit cleaner to defer
this after setting the role to rls_user
above.
204d78e
to
04829b1
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 @yuzefovich)
pkg/sql/explain_bundle.go
line 556 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Do you think we should include the stmt(s) to actually create the user / role into the bundle too? We try to make our bundles easily recreateable (via
cockroach debug sb recreate
command), and I think right now the bundle recreation will fail on a "custom" user. Perhaps we actually want to just automatically comment outSET role ...
stmt inenv.sql
?BTW we have a handful of
test_sb_recreate*
acceptance docker cli tests for this.
Good point — I decided to comment out the SET ROLE
statement. While we could probably recreate the user easily, the key part for RLS is the role membership, which can be tricky to reconstruct due to its hierarchical nature. So I'm not sure recreating the user adds much value.
Thanks for the tip on the test! I confirmed that recreate
runs into issues when a custom role is set, so I added a test to cover that case.
pkg/sql/explain_bundle.go
line 1017 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: double-checking - is "user" is different from current "role" ? We will already include
role
session variable in the form likeSET role = rls_user; -- default value: noneinto
env.sql
(we include all session variables that differ from their defaults, in addition to a handful of read-only session vars).
Yeah, the connected user is distinct from the current role. I double-checked that if you connect as user x
without any SET ROLE
commands, there's no mention of the user in env.sql
prior to my change.
Include the current user in the env.sql file of the EXPLAIN bundle. This information is useful for debugging issues related to row-level security (RLS), as the user determines which policies are applied to a query. This change also verifies the presence of other relevant RLS metadata in the bundle, including: - the list of existing policies, - whether RLS is enabled on the target table, - the policies enforced in the query plan (with EXPLAIN (VERBOSE)), and - modifications to the plan such as injected filters that enforce RLS policies. Includes a new test to validate these additions. Informs cockroachdb#139329 Epic: CRDB-11724 Release note: none
04829b1
to
a529131
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.
Reviewed 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/explain_bundle.go
line 556 at r1 (raw file):
Previously, spilchen wrote…
Good point — I decided to comment out the
SET ROLE
statement. While we could probably recreate the user easily, the key part for RLS is the role membership, which can be tricky to reconstruct due to its hierarchical nature. So I'm not sure recreating the user adds much value.Thanks for the tip on the test! I confirmed that
recreate
runs into issues when a custom role is set, so I added a test to cover that case.
Thanks!
TFTR! bors r+ |
Include the current user in the env.sql file of the EXPLAIN bundle. This information is useful for debugging issues related to row-level security (RLS), as the user determines which policies are applied to a query.
This change also verifies the presence of other relevant RLS metadata in the bundle, including:
Includes a new test to validate these additions.
Informs #139329
Epic: CRDB-11724
Release note: none