vault-creds icon indicating copy to clipboard operation
vault-creds copied to clipboard

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

Open aaron-trout opened this issue 6 years ago • 5 comments

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").

aaron-trout avatar Oct 18 '18 14:10 aaron-trout

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.

Joseph-Irving avatar Oct 19 '18 15:10 Joseph-Irving

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

aaron-trout avatar Oct 19 '18 15:10 aaron-trout

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 avatar Oct 19 '18 15:10 pingles

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

pingles avatar Oct 19 '18 15:10 pingles

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.

Joseph-Irving avatar Feb 27 '19 11:02 Joseph-Irving