search

uswitch / vault-creds

Sidecar container for requesting dynamic Vault database secrets
Apache License 2.0
84 stars 14 forks source link

Remove assumption that there is always a "Username" and "Password" field #19

Open aaron-trout opened 5 years ago

aaron-trout commented 5 years ago

Current behaviour tries to pull the "Username" and "Password" fields out of the data returned from Vault and sticks them in this custom "Credentials" struct. This is unnessecary and breaks usage in cases where the returned secret does not have those fields. (For example with the AWS secrets engine, you get "access_key" and "secret_key").

Joseph-Irving commented 5 years ago

Hey, thanks this looks pretty useful, we don't currently use the aws vault stuff, we've got https://github.com/uswitch/kiam for that, but I definitely see the value in expanding this to handle more generic secrets. We're gonna look at using this for vault generated certs as well in the future.

Your changes looks good to me, but due to our shocking lack of tests in this repo it's hard to be certain so I think I shall go and write some first.

aaron-trout commented 5 years ago

Haha sounds good! I have very minor experience with Go so would definitely recommend testing out my changes first :D

pingles commented 5 years ago

Agreed- it's nice to offer the more general access to secrets rather than exclusively database credentials.

My only comment/concern is that api.Secret has a different interface to vault.Credentials (the struct that username, password were read from). I don't know whether we'd just release with a major version bump and make it very obvious that they're different objects or whether it would be controlled via some kind of flag.

I feel like the former is probably sufficient and better.

pingles commented 5 years ago

But yeah, tests to prove behaviour would be good to add @Joseph-Irving 😄

Joseph-Irving commented 5 years ago

Hey sorry for taking so long, done some refactoring work to this project to make it a bit more atomic. My main concern with this change is how big a breaking change this is, even with a major version change converting all your configmaps/templates etc from .Username to .username would be a giant pain, it would be a lot nicer if we could instead support both.