spring-cloud-consul icon indicating copy to clipboard operation
spring-cloud-consul copied to clipboard

Spring fails to resolve properties from Consul KVs if new config model (no bootstrap) is used and Consul ACLs are enabled

Open oliverlockwood opened this issue 3 years ago • 2 comments

Describe the bug

Versions:

  • Java 11
  • Spring-Boot 2.4.8
  • Spring-Cloud 2020.0.3

Scenario:

  • Consul ACLs are enabled
  • You provide the ACL to your application using the CONSUL_TOKEN environment variable
  • You have @Value annotations referencing Consul KVs
  • You are using the new config model (application.yml, no bootstrap.yml, and no spring-cloud-starter-bootstrap dependency

Behaviour:

  • The application registers with Consul successfully (so clearly the provided CONSUL_TOKEN is being picked up correctly in some contexts!)
  • KV values in Consul are not considered when resolving the @Value annotations.

Workaround:

  • Revert to the old config model (split between application.yml and bootstrap.yml) and include the org.springframework.cloud:spring-cloud-starter-bootstrap dependency in your service.

Sample

A sample application, with detailed README allowing for easy reproduction, is available at https://github.com/oliverlockwood/sb2.4-consul-example

oliverlockwood avatar Jul 22 '21 09:07 oliverlockwood

Hello. We are also facing the same issue. Setting config.aclToken explicitly works though.

TwinkleTShah avatar Jul 27 '21 11:07 TwinkleTShah

Hi @spencergibb, apologies for tagging you directly, but I wondered if you or any of your colleagues have had any thoughts on this. To me this comes across as a major problem (Consul ACLs not properly supported!) but this issue has sat for almost a month without even being triaged, so I thought I'd ask the question.

oliverlockwood avatar Aug 16 '21 11:08 oliverlockwood

Found nice workaround. In addition to CONSUL_TOKEN, also set token into SPRING_CLOUD_CONSUL_CONFIG_ACLTOKEN environment variable.

According to ConsulConfigProperties, acl token looked up in several places:

https://github.com/spring-cloud/spring-cloud-consul/blob/601a7d3cea337904b48245db68c1b4d1787ab15f/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConsulConfigProperties.java#L68-L69

@Value annotation ignored here. Not sure why. But since this class annotated with @ConfigurationProperties("spring.cloud.consul.config"), we can do the trick and set spring.cloud.consul.config.aclToken property.

onyn avatar Jan 12 '23 13:01 onyn

@OlgaMaciaszek since we've spoken on a couple of other Spring-Cloud issues, I wonder if you would please be kind enough to help me understand whether the spring-cloud-consul project is being abandoned?

As far as I can see, there's a lot of issues in https://github.com/spring-cloud/spring-cloud-consul/issues (including this one!) which seem to just have been left in "waiting-for-triage" state and ignored by the maintainers, since circa early 2021.

We're looking to upgrade to Spring-Boot 3, but as @onyn highlighted in the linked issue #768, this bug still exists, 18 months on, and I can't believe that use of Consul ACLs is a particularly extreme edge case!

oliverlockwood avatar Mar 07 '23 21:03 oliverlockwood

The project is still supported. I'll try and look at it in the coming days

spencergibb avatar Mar 07 '23 21:03 spencergibb

@Value annotation ignored here. Not sure why. But since this class annotated with @ConfigurationProperties("spring.cloud.consul.config"), we can do the trick and set spring.cloud.consul.config.aclToken property.

CONSUL_TOKEN was always a shortcut. The official standard property is spring.cloud.consul.config.acl-token. @Value doesn't work in spring.config.import because ConsulConfigProperties is not a bean. I'm tempted to say that for spring.config.import, if you want to use an env var, it should be SPRING_CLOUD_CONSUL_CONFIG_ACLTOKEN rather than CONSUL_TOKEN.

I could, perhaps, be convinced otherwise.

spencergibb avatar Mar 07 '23 21:03 spencergibb

I guess the argument might be a single property to share between config and discovery. Those are the two places that use CONSUL_TOKEN and SPRING_CLOUD_CONSUL_TOKEN.

spencergibb avatar Mar 07 '23 21:03 spencergibb

The work would be done in this method https://github.com/spring-cloud/spring-cloud-consul/blob/310ccfb99c2700e6c469fa67eed466890dea9f3a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConsulConfigDataLocationResolver.java#L210

Similar to https://github.com/spring-cloud/spring-cloud-consul/blob/310ccfb99c2700e6c469fa67eed466890dea9f3a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConsulConfigDataLocationResolver.java#L217-L219

spencergibb avatar Mar 07 '23 22:03 spencergibb

@spencergibb thank you for taking the time to respond.

Regarding your comment

CONSUL_TOKEN was always a shortcut. The official standard property is spring.cloud.consul.config.acl-token. @Value doesn't work in spring.config.import because ConsulConfigProperties is not a bean. I'm tempted to say that for spring.config.import, if you want to use an env var, it should be SPRING_CLOUD_CONSUL_CONFIG_ACLTOKEN rather than CONSUL_TOKEN.

I could, perhaps, be convinced otherwise.

Allow me an attempt to do that convincing :-)

  • CONSUL_TOKEN is conventional in the world of Consul - see e.g. https://registry.terraform.io/providers/hashicorp/consul/latest/docs . I'm not sure why you say it was "always a shortcut".
  • Consider also the recommendation in https://github.com/hashicorp/envconsul, specifically:

    For additional security, tokens may also be read from the environment using the CONSUL_TOKEN or VAULT_TOKEN environment variables respectively. It is highly recommended that you do not put your tokens in plain-text in a configuration file.

  • To the best of my knowledge, it has been supported in spring-cloud-consul for many years prior to the regression described by this bug ticket, which was introduced (as far as I can tell) with the move to the new spring.config.import model. Correct me if I'm wrong, but it appears that this was very much an accidental regression rather than a deliberate deprecation of usage, so to be honest I'd really like to hear your perspective arguing why the CONSUL_TOKEN usage should be deprecated / removed, if you really believe that it should!
  • From a personal use-case perspective, we operate a mix of microservices, some of which are based on Spring-Boot, others are built on NodeJS / Typescript (for example). All of these need to register with Consul and use it for service discovery. We could of course duplicate the CONSUL_TOKEN environment variable that we use throughout into SPRING_CLOUD_CONSUL_CONFIG_ACLTOKEN (though from your comments, it sounds like even that doesn't work at present!). However, to me it seems logical that Spring should continue to support the Consul-conventional env var, which has been supported for years, regardless of whether it also supports a Spring-specific env var. Surely one of Spring's objectives is intuitive ease of use; doesn't this fall into that category?

oliverlockwood avatar Mar 07 '23 22:03 oliverlockwood

Call me convinced.

spencergibb avatar Mar 07 '23 22:03 spencergibb