spring-cloud / spring-cloud-vault

Configuration Integration with HashiCorp Vault
http://cloud.spring.io/spring-cloud-vault/
Apache License 2.0
272 stars 151 forks source link

Add support for config data api and consul backend #580

Closed krisiye closed 3 years ago

krisiye commented 3 years ago

Register SecretBackendMetadataFactory for consul under spring.factories for config data API to discover consul backend.

Fixes #579

mp911de commented 3 years ago

With the Config Data API, we need to inject an ApplicationEventPublisher into ConsulSecretBackendMetadataFactory so that we can rebind changed config properties.

mp911de commented 3 years ago

Thank you for your contribution. That's merged and polished now.

krisiye commented 3 years ago

@mp911de - How do we ensure the ordering of the data loaders? For example spring.config.import: vault://,consul:// ? With the vault consul backend enabled, we need to get the consul acl-token before the consul data loader attempts to load. Otherwise, it fails right away with a 403 Forbidden. Any Thoughts?

spencergibb commented 3 years ago

@krisiye AFAIK it's the ordering of the imports. That's all handled by Spring Boot. @philwebb or @mbhave could provide some more ordering insight.

krisiye commented 3 years ago

@philwebb @mbhave -Any thoughts on how we could introduce dependencies in the config import for the use cases such as vault and consul being used in an integrated fashion? I am not sure if such capability exists. But please let me know if you would like me to raise an issue under spring-boot. Thanks for your help!

mbhave commented 3 years ago

@krisiye Can you provide more details on what the use case is and how it's expected to work? I'm not sure I understand what is supposed to happen so hard to say without that information.

krisiye commented 3 years ago

Sure @mbhave. I have a spring boot 2.4.3 app set up with spring cloud 2020.0.1 where I use vault for secret management and consul for service configuration. The configuration I use for the config data API is spring.config.import: vault://,consul://. This works great if consul was setup for open access (without ACL). However, in any production configuration, we expect consul to be locked down and be behind ACL and that we expect vault to provide the consul acl token through the vault consul backend (had some issues but was addressed in this bug above). I noticed that the data loaders do not have any dependencies established and hence we end up loading consul even before the acl-token is made available in the property sources and thus end up with a 403 Forbidden.

In the legacy bootstrap strategy, we had a workaround that is documented here: https://gist.github.com/mp911de/17f550ffecdc9e8f22061bfdf896bbb4

The question here is if this can be fixed for the config data api with any dependency configuration for the imports? I could not find anything in the documentation that indicates such a capability exists. Please let me know if you need anymore information.

philwebb commented 3 years ago

@krisiye Looking at the code, I think that import are loaded in reverse order. So if you have vault://,consul:// then consul:// is loaded first and then vault://. If I remember correctly, things are done this way to ensure that later elements in the list win. I think we were trying to be consistent with the file itself (where if you have duplicate key, the lower one wins).

You could try switching the order to see if that solves your issue.

krisiye commented 3 years ago

Thanks @philwebb. Tried reversing the order here to spring.config.import: consul://,vault:// but no luck. Still seeing the same behaviour resulting in a 403 Forbidden. Should I raise an issue for this under spring-boot?

philwebb commented 3 years ago

@krisiye Yes please. It would also be helpful if you're able to provide a sample that shows the problem.

krisiye commented 3 years ago

@philwebb Logged an issue for this - https://github.com/spring-projects/spring-boot/issues/25705

krisiye commented 3 years ago

@philwebb @mbhave - Updated spring-projects/spring-boot#25705 and provided a test case for this issue. Sorry, this took a while as it was a little challenging to get a self-contained example with test containers and roles for demonstrating this issue.