Azure / kubernetes-keyvault-flexvol

Azure keyvault integration with Kubernetes via a Flex Volume
MIT License
253 stars 84 forks source link

Feature/support multiple secrets #49

Closed idanlevin closed 5 years ago

idanlevin commented 5 years ago

this is a best-effort implementation of multiple secrets, I wanted to do it nicer by allowing the user to create an array of objects but it turns out the flexVolume options only support strings, so I ended up going with a semi-colon separated list of objects

I don't have access to mcr.microsoft.com/k8s/flexvolume so I replaced it with my personal Dockerhub until fixed

fixes this: https://github.com/Azure/kubernetes-keyvault-flexvol/issues/37

msftclas commented 5 years ago

CLA assistant check
All CLA requirements met.

ritazh commented 5 years ago

@idanlevin Thank you for your contribution! 👏 Just reviewed and tested the code. Overall, this looks great. One concern is going fromkeyvaultobjectname to keyvaultobjectnames is a breaking change for existing users. We need to support but deprecatekeyvaultobjectname, keyvaultobjecttype, and keyvaultobjectversion for at least few more releases before removing them altogether.

idanlevin commented 5 years ago

@ritazh fixed

idanlevin commented 5 years ago

@serbrech I also wrote in the PR comments, the options doesn't support an array, so it's only string.

I can think of three alternative ways to implement it:

Option #1 - three strings

keyvaultobjectnames: "secret1;secret2"
keyvaultobjecttypes: "secret;secret"
keyvaultobjectversions: "version1;version2"

Option #2 - one long string

keyvaultobjects: "secret1:secret:version1;secret2:secret:version2"

Option #3 - embedded json

keyvaultobjects: '[{"name":"secret1","type":"secret","version":"version1"},{"name":"secret2","type":"secret","version":"version2"}]'

Honestly, all three are bad options in my opinion, so I just chose the one easiest to implement which is #1.

If you see any advantages in other options, LMK and we can re-consider the implementation.

serbrech commented 5 years ago

I also wrote in the PR comments, the options doesn't support an array, so it's only string

Thanks for the extended reply. Sorry I missed your initial comment on this, and I agree with your choice. It's a shame that flexvol options are so limited.

ritazh commented 5 years ago

LGTM