secrets: Fix vault CSI support #588
Conversation
fishy
requested review from
konradreiche and
mterwill
and removed request for
a team
fishy
force-pushed
the
fix-vault-csi
branch
3 times, most recently
from
b3c5c9a
to
4d3fac9
Compare
| secretsDocument := Document{ | ||
| Secrets: make(map[string]GenericSecret), | ||
| } | ||
| err := fs.WalkDir( | ||
| dir, | ||
| ".", | ||
| k8sSubdirectory, |
There was a problem hiding this comment.
🔕 Probably worth a quick stat to make sure it exists, and if it doesn't, return an actionable error ("dir %q does not appear to be the root of a Vault CSI mount point" or something).
Alternately you can probably check the error from WalkDir to see if it's a PathError that's NotExists and matches the k8sSubdirectory.
There was a problem hiding this comment.
that's basically the first thing fs.WalkDir does, and the error returned will be returned to the caller on
baseplate.go/secrets/secrets.go
Line 355 in 2dd3ab8
There was a problem hiding this comment.
My concern is the error message, given that we're pointing WalkDir at a directory that the user did not specify directly for an "implementation detail" reason. os.Stat("/path/to/your/dir/..data"): file does not exist is going to be a very opaque error message, and it kinda looks like someone forgot a slash after a ...
However, dir "/path/to/your/dir" does not appear to be the root of a Vault CSI mount point: os.Stat("/path/to/your/dir/..data"): file does not exist or something is going to be much easier to use to figure out what you need to do -- i.e. point it at a Vault CSI mount point, not any old dir.
There was a problem hiding this comment.
Done in latest fixup commit.
secrets/store.go
Outdated
| @@ -41,6 +42,11 @@ type Store struct { | |||
| // Context should come with a timeout otherwise this might block forever, i.e. | |||
| // if the path never becomes available. | |||
| func NewStore(ctx context.Context, path string, logger log.Wrapper, middlewares ...SecretMiddleware) (*Store, error) { | |||
| return newStoreOverrideFSEventsDelay(ctx, filewatcher.DefaultFSEventsDelay, path, logger, middlewares...) | |||
There was a problem hiding this comment.
🔕 normally when a public constructor calls an internal one, I just make it the corresponding name (newStore), even if it has different args. The suffixes are useful for when there is more than one of something imo
| secretsDocument := Document{ | ||
| Secrets: make(map[string]GenericSecret), | ||
| } | ||
| err := fs.WalkDir( | ||
| dir, | ||
| ".", | ||
| k8sSubdirectory, |
There was a problem hiding this comment.
My concern is the error message, given that we're pointing WalkDir at a directory that the user did not specify directly for an "implementation detail" reason. os.Stat("/path/to/your/dir/..data"): file does not exist is going to be a very opaque error message, and it kinda looks like someone forgot a slash after a ...
However, dir "/path/to/your/dir" does not appear to be the root of a Vault CSI mount point: os.Stat("/path/to/your/dir/..data"): file does not exist or something is going to be much easier to use to figure out what you need to do -- i.e. point it at a Vault CSI mount point, not any old dir.
| if err != nil { | ||
| // Should not happen as this means path is not under k8sSubdirectory, | ||
| // but just in case | ||
| return nil |
There was a problem hiding this comment.
If we handle it for just in case, we should also just return the actual error, shouldn't we?
There was a problem hiding this comment.
that would actually stop the walking and cause the whole parser to fail, which has bigger consequences, so probably not. but I don't feel too strongly either way
In Vault CSI the secret files are actually written under `..data` subdirectory which is also a symlink, and fs.WalkDir will skip symlinks unless they are the root. So walk under `..data` instead, and also strip the prefix from the path key. Also add a test to test the rotation case and some improvements on existing tests.
In Vault CSI the secret files are actually written under
..datasubdirectory which is also a symlink, and fs.WalkDir will skip symlinks unless they are the root.So walk under
..datainstead, and also strip the prefix from the path key. Also add a test to test the rotation case and some improvements on existing tests.