(Originally posted in https://github.com/spring-projects/spring-boot/issues/43579)
Overview
Hello, I'm a maintainer of the Spring Framework on Google Cloud. We have recently been working on upgrading Spring Cloud to 2024.0 and Spring Boot to 3.4. So far, so good, except for our secretmanager integration.
The way we support bean injections for this integration is by specifying properties with the syntax ${sm://secret_id}, implying a request to the Secret Manager service.
Unfortunately, the colon : character is now being interpreted differently since https://github.com/spring-projects/spring-framework/commit/00e05e603d4423d33c99dadeb52fef26be71dfb8, with the resulting behavior being to interpret it as "property sm with fallback value of //secret_id". We have confirmed this in our code sample.
What have we tried so far
We have tried introducing our own static PropertySourcesPlaceholderConfigurer bean (see bean declaration, see source), set up to be configured before the default PlaceholderConfigurer autoconfiguration.
Interestingly, this works perfectly for @Value annotations such as @Value("${sm://my_secret}") (see example), but not for @Configuration classes that reference a properties file (in our case, simply application.properties). I confirmed that the following code has the default PropertySourcesPlaceholderConfigurer (this.placeholdersResolver in Binder.java) when debugging my application:
https://github.com/spring-projects/spring-boot/blob/60e0de79ea9de2989ab03e9a193ae95de1cfcbf0/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/Binder.java#L459-L465
More context
- We use a custom config data location resolver. I suspect this may have to do with how the properties file decides to resolve its placeholder.
Alternatives considered
We are also thinking of additionally supporting the ${sm//my_secret} syntax (without the colon) as a workaround to escape the colon with a backslash, however we would like to avoid our users changing their code as much as possible.
Questions
My first question is: Why would the properties file be parsed with a different PlaceholderConfigurer than what's specified in the static bean? I believe this bean is meant to be a singleton, so I'm not sure where in the application it's decided to associate the default configurer to this configuration.
* PS: I found that Binder will by default create a plain PropertyPlaceholderHelper
My second question is: Is there a preferred way to adapt to this new behavior?
Comment From: snicoll
We should be able to find a way to restore the previous behavior in case of perfect match if I understood the use case correctly. To get started, we'd need a small sample that demonstrates the problem. I know you've shared one but it would be nice if we didn't have to use the Google Cloud SDK to get it running.
Creating your own PropertySourcesPlaceholderConfigurer at that level is a bad idea as it's an application context-wide change that should not happen.
Comment From: diegomarquezp
Thanks for the swift response! I'll soon provide a concise reproducer.
Creating your own PropertySourcesPlaceholderConfigurer at that level is a bad idea as it's an application context-wide change that should not happen.
How can we narrow down the scope of this bean?
Comment From: diegomarquezp
I have created a minimal reproducer: https://github.com/diegomarquezp/spring-placeholder-configurer-behavior-demo/tree/main
Again, thanks for looking into this.
Comment From: snicoll
@diegomarquezp I don't really think this is an upgrade problem, is it?
I've tried to downgrade your sample to Spring Boot 3.3.7 and Spring Cloud 2023.0.4 and it fails as well. If you want to demonstrate an upgrade problem, then it should work with Spring Boot 3.3.x unchanged (that uses Spring Framework 6.1.x).
This pattern for properties isn't simply supported. If you were able to workaround it by creating your own implementation, then you differ from the standard behavior. It's OK-ish for an application as a team can take the decision of including their own patterns. it's totally not ok for a library as you can't control that. Imagine that another library takes the same decision as Google Cloud, who's going to win?
IntelliJ IDEA also warns you that this pattern isn't valid:
I've tried to create a unit test with 6.1.x with what I understood from your report and it fails:
@Test
void gh34124() {
String text = "value=${foo://bar:fallback-value}";
Properties props = new Properties();
props.setProperty("foo://bar", "bar");
assertThat(this.helper.replacePlaceholders(text, props)).isEqualTo("value=bar");
}
Fails with:
expected: "foo=bar"
but was: "value=${foo://bar:fallback-value}"
In short I don't understand what you were doing in 6.1.x, and the sample is still using Google Cloud and should not IMO. You should be able to provide a sample where the values are in application.properties and have it working in Spring Boot 3.3.x and failing with Spring Boot 3.4.x
Comment From: diegomarquezp
You should be able to provide a sample where the values are in application.properties and have it working in Spring Boot 3.3.x and failing with Spring Boot 3.4.x
Apologies, I think I was a few steps ahead by providing a sample using a custom PlaceholderConfigurer (our library does not use a placeholder configurer and this is just an approach we were considering).
In order to demonstrate how our autoconfiguration behavior changes when upgrading from Spring Cloud 2023 to 2024, I have created two branches:
Using Spring Cloud 2023 + Spring Boot 3.3.x
https://github.com/diegomarquezp/spring-placeholder-configurer-behavior-demo/tree/demo-without-placeholder-configurer-3.3 - the demo works as expected (the value expansion matches our current setup in spring-cloud-gcp)
* The parsing of @Value("${foo://bar}") is interpreted as "expand property foo://bar, there is no fallback)
Using Spring Cloud 2024 + Spring Boot 3.4.x
https://github.com/diegomarquezp/spring-placeholder-configurer-behavior-demo/tree/demo-without-placeholder-configurer-3.4 represents an upgrade to Spring Cloud 2024 and Spring Boot 3.4 leaving the rest of the code intact.
* The parsing of @Value("${foo://bar}") is interpreted as "expand property foo, otherwise fallback to //bar"
This pattern for properties isn't simply supported. If you were able to workaround it by creating your own implementation, then you differ from the standard behavior.
We are still considering if we want to introduce a new format for these annotations in our secret manager integration. I'm not sure on how to interpret this behavior change these two branches show in terms of changing behavior for our end users.
cc: @burkedavison
Comment From: snicoll
Thanks for the updated sample. There is still a lot going on that is completely unnecessary. For reference, I've trimmed down your sample in https://github.com/snicoll-scratches/spring-placeholder-configurer-behavior-demo/commit/894ace5b09479a62c1e2e968751c2f0b3d2b0e01.
Turns out this was the problem I was referring to all along. For the purpose of restoring backward compatibility, I am going to fix this but please consider moving to a different format. Asking users to inject "${something://value}" is breaking pretty much anything that's using : as the separator between the key and the fallback value.
Looking at your property source implementation, you can easily figure out if the key starts with the prefix and log a warning. At the same time you can define an alternative format that will not clash with the reserved keywords. For instance ${something@value} where you'd replace something:// by something@.
While Spring Framework does not have an opinion about property placeholder resolution configuration, Spring Boot configures one at the application level and a library can't deviate from that. It should be noted Spring Boot just exposes the "standard" PropertySourcesPlaceholderConfigurer which has 4 reserved keywords.
Comment From: diegomarquezp
Thanks @snicoll for the actions taken and trimming down the sample to the essentials.
Warning users that keep using sm:// and suggest using sm// instead definitely seems reasonable - I'll discuss this with my team.
Regarding the release of https://github.com/spring-projects/spring-framework/milestone/388, do you have a target release date?
Comment From: philwebb
@diegomarquezp The due date is set on the milestone. It's January 16, 2025. Also check out https://calendar.spring.io
Comment From: diegomarquezp
Oh, that was a big miss. Thanks a lot!
Comment From: c4lm
@snicoll @philwebb
Hi, we happen to use ${ssm:/some/path} ${azkv:/some/path} ${gsm:/some/path} as values everywhere, served by adding PropertySource
Comment From: snicoll
I'd like to know if by any chance it is possible that this fix will be deprecated in the future?
Yes. : is a reserved keyword used to separate the key from the fallback value and we may very well enforce that format in the future. This is an opportunity to adapt those values so that they don't use such a reserved character. You can also escape it as of 6.2, i.e. ${ssm\:/some/path}.
Comment From: c4lm
I'd like to know if by any chance it is possible that this fix will be deprecated in the future?
Yes.
:is a reserved keyword used to separate the key from the fallback value and we may very well enforce that format in the future. This is an opportunity to adapt those values so that they don't use such a reserved character. You can also escape it as of 6.2, i.e.${ssm\:/some/path}.
Thank you for letting me know. While we will figure out a way to do it, I suspect that this is going to be coming back to you again and again 😄 because this is a popular way to wire up app-level properties (from env variables).