Extend SecretsStore to support Vault CSI #625
Conversation
SecretsStore reworked to support multiple filewatchers given a directory instead of a secret file Added parsing classes to separate some of the Vault secret format from the SecretsStore, since format is different between the Vault secrets fetcher and Vault CSI More information on vault-csi: https://www.vaultproject.io/docs/platform/k8s/csi Add deprecation notice for Vault-specific functions in SecretsStore
There was a problem hiding this comment.
Cool! Thanks for doing this. I think the biggest thing is it'd really be best not to be branching in every function like that but instead use separate classes for each implementation. We'll have to keep an eye on performance and consider our options if checking all those mtimes is too slow. Excited to see the CSI provider replace the secrets fetcher soon!
baseplate/lib/secrets.py
Outdated
| self.path_type = "dir" if os.path.isdir(path) else "file" | ||
| self.parser = parser or VaultFileParser | ||
| if self.path_type == "dir": | ||
| for root, _, files in os.walk(path): |
There was a problem hiding this comment.
Are there ever going to be subdirectories? or just a flat directory of files? In the latter case it'd be simpler to use os.listdir or Path(...).iterdir().
Do we need to worry about new files being created later on? (i.e. new secrets) If so we'd want to do this directory listing operation more than once.
There was a problem hiding this comment.
yep, there can be subdirectories if the objectName has /s in them. e.g. trying to use secret/blah instead of blah would create an additional directory secret.
There was a problem hiding this comment.
second question - new files wouldn't be created unless a change is made to the deploy config to add additional objects.
There was a problem hiding this comment.
It looks like spladug has this review, but from my perspective I think it would be useful for the PR / commit to summarize the high-level considerations that need to be made in the transition. Notably, this will need to be mimicked in the Go and TypeScript baseplate libraries, so having a concise description of the changes are that motivate the change would be useful so that the implementation doesn't have to be the canonical documentation of how it works.
|
Split the SecretsStore logic into 2 subclasses representing the file/directory implementations. |
|
@spladug / @kylelemons ready to merge - will squash with the following commit message: |
This change adds an additional way to load secrets besides the existing secrets fetcher: - When vault.provider is set to vault_csi, vault.path will be assumed to be a directory containing individual files representing secrets, rather than a single file containing every secret. - Vault CSI loads secrets automatically into these files on pod startup, so no additional sidecar is required. - Secrets are written to files in a different JSON format - Each secrets file has its own filewatcher representation with per-request caching behavior preserved Implementation details: - Add DirectorySecretsStore subclass of SecretsStore supporting multiple filewatchers - Duplicate SecretsStore unit tests in the context of DirectorySecretsStore - Modify secrets_store_from_config to return SecretsStore or DirectorySecretsStore based off vault.provider - Split out vault-specific file parsing logic into functions outside of SecretsStore - Add deprecation notice for get_vault_url, get_vault_token in SecretsStore
parser instance var was previously added in reddit#625 however, the child classes dont call the superclass init so these were never set properly.
parser instance var was previously added in #625 however, the child classes dont call the superclass init so these were never set properly.
Reddit-only design link: https://docs.google.com/document/d/1nQ8Fhh-WQ4egtIMG_FZzW-dvqAEObsp-onKrQzEn9CA/edit#
This change should be completely transparent for services sticking with secrets fetchers. For those wanting to use vault-csi, they will have to do the following:
secrets.path = some_directorysecrets.provider = vault_csiNote for testing: I moved the existing test suite to its own file and duplicated the tests in the context of SecretsStore using a dir with multiple secrets files vs a single file - I think this provides better coverage though I'm open to collapsing it back down to one file.