-
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
storage: add config option for enabling encryption-at-rest #68931
Conversation
f909972
to
1dda7f6
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 @andyyang890)
pkg/storage/open.go, line 112 at r1 (raw file):
// registry. It is used to configure encryption-at-rest for in-memory engines, // which are used in tests. func UseFileRegistry(useRegistry bool) ConfigOption {
Can we consolidate these into a single EncryptionAtRest(encryptionOptions []byte)
option that turns on UseFileRegistry
and sets EncryptionOptions
?
pkg/storage/open.go, line 125 at r1 (raw file):
return func(cfg *engineConfig) error { cfg.EncryptionOptions = make([]byte, len(b)) copy(cfg.EncryptionOptions, b)
Does cfg.EncryptionOptions
get mutated? Is the copy necessary? IF so, can you document where/why?
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 @andyyang890 and @jbowens)
pkg/storage/open.go, line 125 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Does
cfg.EncryptionOptions
get mutated? Is the copy necessary? IF so, can you document where/why?
I was thinking it would be safer to make a copy in case the caller modifies the slice afterwards. Is that not necessary here?
1dda7f6
to
7652457
Compare
This patch adds the ability to configure encryption-at-rest for in-memory engines, which are used in tests. Release note: None
7652457
to
506a61c
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 @jbowens)
pkg/storage/open.go, line 112 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Can we consolidate these into a single
EncryptionAtRest(encryptionOptions []byte)
option that turns onUseFileRegistry
and setsEncryptionOptions
?
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.
Reviewed 3 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andyyang890 and @jbowens)
pkg/storage/open.go, line 125 at r1 (raw file):
Previously, andyyang890 (Andy Yang) wrote…
I was thinking it would be safer to make a copy in case the caller modifies the slice afterwards. Is that not necessary here?
I suspect no callers modify the slice, but this is fine if we're unsure.
bors r=jbowens |
Build succeeded: |
This patch adds the ability to configure encryption-at-rest for
in-memory engines, which are used in tests.
Release note: None