Allow overriding of volume mount path and added a flag to preserve case sensitivity of secrets #71
Conversation
|
|
smitthakkar96
requested review from
jasonodonnell and
catsby
and removed request for
jasonodonnell
There was a problem hiding this comment.
Hi @smitthakkar96,
Added some inline comments about things that need to be fixed. Additionally these annotations will require tests. Case sensitivity can be added here, secret path can be added here and a new test case to verify the mount path can be added here.
Thanks!
|
Thanks @jasonodonnell will address the comments |
|
@jasonodonnell I addressed the comments |
|
@jasonodonnell any updates? |
|
any update, I want this feature too |
|
Hi @smitthakkar96, sorry for the delay, but I'm ready to review this now! When you get a chance can you resolve the conflicts? Thanks! |
smitthakkar96
force-pushed
the
smit/allow_override_behaviour
branch
from
5cb4371 to
a8dc585
Compare
|
@jasonodonnell resolved |
| } | ||
|
|
||
| func TestSecretAnnotationsWithPreserveCaseSensitivityFlagOn(t *testing.T) { | ||
| tests := []struct { |
There was a problem hiding this comment.
@jasonodonnell this is not dry but making it dry would make tests a bit complicated so I duplicated the code
|
ping! |
There was a problem hiding this comment.
Hi @smitthakkar96, I finally got some time to review this!
I like what this PR is trying to do and it's a much needed feature. I do think that the path and preserve case should be configurable per secret and not global.
For example I think this would be more useful if the following were possible:
vault.hashicorp.com/agent-inject-secret-db-creds: "database/creds/db-app"
vault.hashicorp.com/agent-inject-template-db-creds: |
{{- with secret "database/creds/db-app" -}}
postgres://{{ .Data.username }}:{{ .Data.password }}@postgres.postgres.svc:5432/wizard?sslmode=disable
{{- end }}
vault.hashicorp.com/agent-inject-path-db-creds: "/etc/db"
vault.hashicorp.com/agent-inject-preserve-case-db-creds: "true"Admittedly this does complicate things because we'll now need to iterate over any custom paths and create/mount volumes for those.
Thoughts?
|
@jasonodonnell isn't it better to still keep it global and allow overriding at secret level if needed? I am happy to update the code to allow overriding the path and case sensitivity flag at secret level if we agree |
|
@smitthakkar96 @jasonodonnell Your comments make more sense to have path configurable per secret. Specially with pattern you suggested . |
|
@saurav-k I still think having a global flag makes it easy for people to configure incase if you want to override the path for all secrets you are fetching. Incase of if you want to override per secret you can still do if we implement what @jasonodonnell is suggesting |
|
@smitthakkar96 Makes sense to have a global flag in case you want to mount all secret at same path and this PR can be scoped to global flag and another PR can be created for the feature suggested by @jasonodonnell , My thoughts on suggested feature. I have seen apps need files from different mount location as secret . and in that case suggested feature will be very useful. Let's wait for @jasonodonnell to give his view on scope of this PR. |
|
@smitthakkar96 I agree the global flag is also valuable because in some situations it would eliminate a lot of annotations. It would likely be that secret specific configurations override the globals. |
|
Ok I will work on it tonight and hopefully update the PR.
Best Regards,
Smit
…On Tue, Feb 25, 2020 at 16:11 jasonodonnell ***@***.***> wrote:
@smitthakkar96 <https://github.com/smitthakkar96> I agree the global flag
is also valuable because in some situations it would eliminate a lot of
annotations. It would likely be that secret specific configurations
override the globals.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#71?email_source=notifications&email_token=ACN3OKQKNV3UEVG2FINYLVLREUDH5A5CNFSM4KQ5KPA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM3XGRI#issuecomment-590836549>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACN3OKULRKHDY3BPYBU5QH3REUDH5ANCNFSM4KQ5KPAQ>
.
|
…r96/vault-k8s into smit/allow_override_behaviour
|
@jasonodonnell I made the changes, please have a look when you get chance |
|
@jasonodonnell thanks for merging it :) |
…se sensitivity of secrets (hashicorp#71) * Allow overriding of volume mount path and also the case sensitivity * Addressed CR comments and fix broken test * cleanup * allow paths to be configurable at secret level * added tests * run go fmt
This PR adds two new annotations:
vault.hashicorp.com/secret-volume-pathvault.hashicorp.com/preserve-secret-caseWhy?
Currently vault-injector doesn't allow overriding the path where it mounts the secrets. In our case our base image expects the secrets to be present in a particular directory so it gets picked up as env vars when the container starts.
Currently vault-injector assumes that the case of the secret name doesn't matter well in our case it is very important, so to not break backwards compatibility and also fullfil the need I added
vault.hashicorp.com/preserve-secret-case