Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow values from configtree to resolve nested placeholders #34195

Closed
ChristianAnke opened this issue Feb 15, 2023 · 11 comments
Closed

Allow values from configtree to resolve nested placeholders #34195

ChristianAnke opened this issue Feb 15, 2023 · 11 comments
Labels
for: external-project For an external project and not something we can fix type: enhancement A general enhancement

Comments

@ChristianAnke
Copy link

It would be helpful if for properties which come from a configtree the placeholders will be replaced. Currently this seems to be blocked by the ConfigTreePropertySource producing non String results when getting the value of a property.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 15, 2023
@wilkinsona
Copy link
Member

Thanks for the suggestion.

This is a little bit similar to #29642. A notable difference here, however, is that org.springframework.boot.env.ConfigTreePropertySource.Value is part of the property source implementation and, unlike the String[] in #29642, something that cannot be controlled. It feels like placeholder replacement should be possible, but it also doesn't feel right for PropertySourcesPlaceholdersResolver to know about org.springframework.boot.env.ConfigTreePropertySource.Value.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Feb 20, 2023
@ChristianAnke
Copy link
Author

Thanks for the answer!

Sounds legit to me. Surely it is not easy to to simply support this for all the various property source providers. Somehow it has to be unified how replacements can be performed with the different providers.

I see minimum two ways to solve this:
A: It is possible to convert a value to a string (by asking the property provider) and handle then the nested placeholders
B: let the provider itself perform the nested replacement
C: ???

what do you think?

@philwebb
Copy link
Member

philwebb commented Mar 2, 2023

We might be able to change PropertySourcesPlaceholdersResolver to check for a CharSequence rather than a String since ConfigTreePropertySource.Value implements CharSequence. There is a risk a configtree file might be binary and we accidentally change the contents.

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed for: team-meeting An issue we'd like to discuss as a team to make progress status: waiting-for-triage An issue we've not yet triaged labels Jun 21, 2023
@wilkinsona wilkinsona added this to the 3.x milestone Jun 21, 2023
@wilkinsona
Copy link
Member

We're going to try the CharSequence check.

@lazystone
Copy link

I tried to use env variables in configtree in SB 3.2.5 and it does not seem to work unfortunately.

Is there any progress at the moment?

@philwebb
Copy link
Member

Not as yet I'm afraid @lazystone

@lazystone
Copy link

Ok, i understand the struggle here(we don't want to accidentally replace something in binary files), but I also realized that my problem was wrong from the beginning, so I'll describe it here for others.

Initially we used configmaps in k8s environments to provide configuration overrides for SB. And standard way to pass secrets to it was to read them from k8s secrets, put them in environment variables and SB would resolve them later.

Then, with introduction of configtree in SB we started to use it. And we did exactly the same thing as before - tried to use placeholders for secrets. One thing that we were missing I think - the whole purpose of configtree approach.

Using configtree we could mount secrets as we mount configmaps in k8s pod and use them directly as configuration overrides.

So, whoever faces this problem - have another look on your use case. Maybe you don't need this feature that much.

@quaff
Copy link
Contributor

quaff commented May 22, 2024

We're going to try the CharSequence check.

See #40862

quaff added a commit to quaff/spring-boot that referenced this issue May 22, 2024
@mhalbritter
Copy link
Contributor

Superseded by #40862.

@mhalbritter mhalbritter closed this as not planned Won't fix, can't repro, duplicate, stale May 22, 2024
@mhalbritter mhalbritter removed this from the 3.x milestone May 22, 2024
@mhalbritter mhalbritter added status: superseded An issue that has been superseded by another and removed type: enhancement A general enhancement labels May 22, 2024
quaff added a commit to quaff/spring-framework that referenced this issue May 23, 2024
quaff added a commit to quaff/spring-boot that referenced this issue May 31, 2024
@wilkinsona
Copy link
Member

wilkinsona commented Jul 22, 2024

Prompted by a review of #40862, I've realised that supporting this consistently isn't going to be straightforward. For example, LoggingSystemProperties uses PropertySourcesPropertyResolver so it would not benefit from a change made to the Boot-specific ConfigurationPropertySourcesPropertyResolver as #40862 proposes. There are also other classes that make use of PropertySourcesPropertyResolver outside of Boot to consider, such as Framework's PropertySourcesPlaceholderConfigurer.

It looks like a change in Framework may be a better option for fixing this consistently. spring-projects/spring-framework#32876 proposed a step in that direction, but it was declined. We'll discuss this with the Framework team and consider our options.

@wilkinsona wilkinsona reopened this Jul 22, 2024
@wilkinsona wilkinsona removed the status: superseded An issue that has been superseded by another label Jul 22, 2024
@wilkinsona wilkinsona added the type: enhancement A general enhancement label Jul 22, 2024
@wilkinsona wilkinsona added this to the 3.x milestone Jul 22, 2024
@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jul 22, 2024
@wilkinsona
Copy link
Member

With thanks to @quaff and @snicoll, this has now been addressed in Framework. The enhancement will be available in Spring Boot 3.4.0-M2 as part of an upgrade to Framework 6.2.0-M7.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Jul 22, 2024
@wilkinsona wilkinsona removed this from the 3.x milestone Jul 22, 2024
@wilkinsona wilkinsona added for: external-project For an external project and not something we can fix and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants