CONSUL_TOKEN and SPRING_CLOUD_CONSUL_TOKEN are not read when using spring.config.import

oliverlockwood commented 3 years ago

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

TwinkleTShah commented 2 years ago

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

oliverlockwood commented 2 years ago

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.

onyn commented 1 year ago

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.

oliverlockwood commented 1 year ago

@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!

spencergibb commented 1 year ago

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

spencergibb commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

oliverlockwood commented 1 year ago

@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?

spencergibb commented 1 year ago

Call me convinced.

spring-cloud / spring-cloud-consul