-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
cli: fix debug pebble commands on encrypted stores #110150
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
I will look into adding some tests, but it will be a separate PR in case it's harder to backport. I will keep the issue open until then. |
d34b227
to
558e47b
Compare
I found that there were existing tests, and they worked because they also passed a matching |
24dcdc0
to
4225737
Compare
Added a test for |
19e3c4a
to
4c7d441
Compare
Added some other reviewers since Jackson is out. I plan to backport to 23.1 and 22.2. |
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.
Clever approach!
Reviewed 3 of 7 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @bananabrick, @itsbilal, and @RaduBerinde)
pkg/cli/auto_decrypt_fs.go
line 245 at r3 (raw file):
// path. Otherwise, returns the default FS. // // Assumes that path is absolute; the returned FS also assumes that any paths
nit: we also assume the path is "clean", eg, filepath.Clean
, right?
pkg/cli/auto_decrypt_fs_test.go
line 80 at r3 (raw file):
` require.Equal(t, expected, output) }
Do other tests in the package exercise the autoDecryptFS, maybe indirectly through an Engine
's access? I want to make sure the breadth of the autoDecryptFS
operations are covered (looks like code coverage flaked, otherwise that would probably tell me)
Currently the debug pebble commands only work correctly on an encrypted store if the encrypted store's path is `cockroach-data` or the store directory is passed using `--store` (in addition to being passed to the pebble subcommand itself). What's worse, knowledge of this subtle fact was lost among team members. The root cause is that we are trying to resolve encryption options using the server config. The difficulty is that there are a bunch of different commands and there is no unified way to obtain the store directory of interest To fix this, we create `autoDecryptFS`. This is a `vfs.FS` implementation which is able to automatically detect encrypted paths and use the correct unencrypted FS. It does this by having a list of known encrypted stores (the ones in the `--enterprise-encryption` flag), and looking for any of these paths as ancestors of any path in an operation. This new implementation replaces `swappableFS` and `absoluteFS`. We also improve the error message when we try to open an encrypted store without setting up the key correctly. Fixes: cockroachdb#110121 Release note (bug fix): `cockroach debug pebble` commands now work correctly with encrypted stores which don't use the default `cockroach-data` path without having to also pass `--store`.
4c7d441
to
c049ba0
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! 1 of 0 LGTMs obtained (waiting on @bananabrick, @itsbilal, and @jbowens)
pkg/cli/auto_decrypt_fs.go
line 245 at r3 (raw file):
Previously, jbowens (Jackson Owens) wrote…
nit: we also assume the path is "clean", eg,
filepath.Clean
, right?
Done.
pkg/cli/auto_decrypt_fs_test.go
line 80 at r3 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Do other tests in the package exercise the autoDecryptFS, maybe indirectly through an
Engine
's access? I want to make sure the breadth of theautoDecryptFS
operations are covered (looks like code coverage flaked, otherwise that would probably tell me)
Looks like the cli tests are way too big for the coverage runner, I don't think we'll get coverage in this PR.
The interactive tests would cover the operations performed by debug pebble db lsm
Previously, RaduBerinde wrote…
Let me know if you have more ideas of what tests to add or if I should merge. TFTR! |
|
bors r+ |
Build succeeded: |
Currently the debug pebble commands only work correctly on an
encrypted store if the encrypted store's path is
cockroach-data
orthe store directory is passed using
--store
(in addition to beingpassed to the pebble subcommand itself). What's worse, knowledge of
this subtle fact was lost among team members.
The root cause is that we are trying to resolve encryption options
using the server config. The difficulty is that there are a bunch of
different commands and there is no unified way to obtain the store
directory of interest
To fix this, we create
autoDecryptFS
. This is avfs.FS
implementation which is able to automatically detect encrypted paths
and use the correct unencrypted FS. It does this by having a list of
known encrypted stores (the ones in the
--enterprise-encryption
flag), and looking for any of these paths as ancestors of any path in
an operation. This new implementation replaces
swappableFS
andabsoluteFS
.We also improve the error message when we try to open an encrypted
store without setting up the key correctly.
Fixes: #110121
Release note (bug fix):
cockroach debug pebble
commands now workcorrectly with encrypted stores which don't use the default
cockroach-data
path without having to also pass--store
.