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

Spring Cloud activates unexpected profiles when some conditions met #1285

Closed
leovx opened this issue Sep 29, 2023 · 14 comments · Fixed by #1286
Closed

Spring Cloud activates unexpected profiles when some conditions met #1285

leovx opened this issue Sep 29, 2023 · 14 comments · Fixed by #1286
Labels
Milestone

Comments

@leovx
Copy link
Contributor

leovx commented Sep 29, 2023

According to 3.1. Adding Active Profiles:

"The spring.profiles.active property follows the same ordering rules as other properties: The highest PropertySource wins. This means that you can specify active profiles in application.properties and then replace them by using the command line switch."

Hence, I would expect spring.profiles.active can only introduce profiles in property source with highest priority, while spring.profiles.include can accept any profiles from all valid property sources.

3.1.6 is the last version that holds above rules. Since 3.1.7, profiles under spring.profiles.active from all property sources are merged, which does not follow the existing behaviours.

I have quickly written a demo and detailed descriptions for this issue here.

Trigger Conditions:

  1. Using Spring Cloud 2021.0.8 (Spring Cloud Common 3.1.7) onwards
  2. Using Spring Boot 2.7.x onwards
  3. In application.yml, we give (for out-of-box experience, run locally for dev and test once after clone)

spring.profiles.active: local

  1. In command line or any other remote property source which has higher priority, e.g. Kubernetes ConfigMap, we give

spring.profiles.active: prod, secure
#intended to override and replace value in application.yml

  1. Provide a bean that implements org.springframework.cloud.bootstrap.config.PropertySourceLocator

Buggy Behaviour

Instead of accepting "prod" and "secure" profiles only, it accepts local profile as well.

INFO 5445 --- [ main] com.jinghao.demo.DemoApplication : The following 3 profiles are active: "prod", "secure", "local"

Potential Hazard

In this demostration project, 2 beans of AuthManager would be candidates to use - LocalBypassAuthManager should only be used in local development environment due to some reasons (say something cannot be installed locally but should not impact your BAU functional implementation), while NormalAuthManager should be applied in any valid remote environment - especially Production. In this case, both "prod" and "local" profiles are activated and hence the illegal bean of LocalBypassAuthManager will be used in Production making any authentication tokens bypass and become valid.

@leovx
Copy link
Contributor Author

leovx commented Sep 29, 2023

It should not be too hard to fix this, and I would like to raise a PR for it.
#1286

@leovx
Copy link
Contributor Author

leovx commented Oct 11, 2023

Hi @ryanjbaxter @spencergibb, would you please help check and advise?

@ryanjbaxter
Copy link
Contributor

Can you try setting spring.cloud.config.initialize-on-context-refresh=true in bootstrap.yaml?

@leovx
Copy link
Contributor Author

leovx commented Oct 12, 2023

Can you try setting spring.cloud.config.initialize-on-context-refresh=true in bootstrap.yaml?

Yep, thanks. I tried this config in bootstrap.yml, this could possibly solve quite some problems. However, it would not prevent the merge of spring.profiles.active from other property sources with lower priority, e.g., having such key in environment variables together with command line arguments would result in the merge instead of replacement, which still breaks our expectation. I would therefore insist to have such preemptive checking as we can have 2 benefits:

  • make sure property source with top priority wins and prevent any unexpected potential risks (defensive coding)
  • make sure the framework enhancement change does not always force users to add extra properties (being user-agnostic) as we are not able to estimate the risks of having such change for old big projects

Let me know your opnions. :)

@ryanjbaxter
Copy link
Contributor

However, it would not prevent the merge of spring.profiles.active from other property sources with lower priority, e.g., having such key in environment variables together with command line arguments would result in the merge instead of replacement, which still breaks our expectation.

Can you provide a sample that reproduces this? When I use your sample it behaves as expected when using spring.cloud.config.initialize-on-context-refresh=true.

@leovx
Copy link
Contributor Author

leovx commented Oct 13, 2023

  • in environment variable, add spring.profiles.active=xyz
  • in command line argument add --spring.profiles.active=prod
  • in bootstrap.yml add spring.cloud.config.initialize-on-context-refresh: true

Our expectation:

INFO 30471 --- [ main] com.jinghao.demo.DemoApplication : The following 1 profile is active: "prod"

Reproduced ground truth:

INFO 30327 --- [ main] com.jinghao.demo.DemoApplication : The following 2 profiles are active: "prod", "xyz"

Full log below:

INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : Active from env method: prod
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : configurationProperties: prod
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : bootstrap: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : commandLineArgs: prod
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : systemProperties: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : systemEnvironment: xyz
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : random: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : cachedrandom: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : springCloudClientHostInfo: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : Config resource 'class path resource [bootstrap.yml]' via location 'optional:classpath:/': null
INFO 30327 --- [ main] c.j.d.l.SecurityPropertySourceLocator : Get some properties securely from some vault
INFO 30327 --- [ main] b.c.PropertySourceBootstrapConfiguration : Located property source: [BootstrapPropertySource {name='bootstrapProperties-securityPropertiesMapping'}]
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : Active from env method: prod;xyz
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : configurationProperties: prod
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : commandLineArgs: prod
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : servletConfigInitParams: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : servletContextInitParams: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : systemProperties: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : systemEnvironment: xyz
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : random: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : springCloudDefaultProperties: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : cachedrandom: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : springCloudClientHostInfo: null
INFO 30327 --- [ main] c.j.d.listener.ActiveProfileLogListener : Config resource 'class path resource [application.yml]' via location 'optional:classpath:/': local

:: Spring Boot :: (v2.7.14)

INFO 30327 --- [ main] c.j.d.l.SecurityPropertySourceLocator : Get some properties securely from some vault
INFO 30327 --- [ main] b.c.PropertySourceBootstrapConfiguration : Located property source: [BootstrapPropertySource {name='bootstrapProperties-securityPropertiesMapping'}]
INFO 30327 --- [ main] com.jinghao.demo.DemoApplication : The following 2 profiles are active: "prod", "xyz"
INFO 30327 --- [ main] o.s.cloud.context.scope.GenericScope : BeanFactory id=4d52baba-1c80-34c9-b6df-57a3c1fb58b0
INFO 30327 --- [ main] o.s.b.w.embedded.tomcat.TomcatWebServer : Tomcat initialized with port(s): 8080 (http)
INFO 30327 --- [ main] o.apache.catalina.core.StandardService : Starting service [Tomcat]
INFO 30327 --- [ main] org.apache.catalina.core.StandardEngine : Starting Servlet engine: [Apache Tomcat/9.0.78]
INFO 30327 --- [ main] o.a.c.c.C.[Tomcat].[localhost].[/] : Initializing Spring embedded WebApplicationContext
INFO 30327 --- [ main] w.s.c.ServletWebServerApplicationContext : Root WebApplicationContext: initialization completed in 956 ms
INFO 30327 --- [ main] c.j.d.a.ApplicationAutoConfiguration : PROD env should use this bean
INFO 30327 --- [ main] o.s.b.w.embedded.tomcat.TomcatWebServer : Tomcat started on port(s): 8080 (http) with context path ''
INFO 30327 --- [ main] com.jinghao.demo.DemoApplication : Started DemoApplication in 2.387 seconds (JVM running for 3.271)

What's more, if we try to add spring-boot-actuator and trigger /actuator/refresh, we would see order changed after refresh.

INFO 30327 --- [ main] com.jinghao.demo.DemoApplication : The following 2 profiles are active: "xyz", "prod"

If we miss spring.cloud.config.initialize-on-context-refresh: true in bootstrap.yml, we will have:

INFO 30463 --- [ main] com.jinghao.demo.DemoApplication : The following 3 profiles are active: "prod", "xyz", "local"

This is just a quick sample for the wrong 'merge' behaviour, the profile should not be necessarily only from environment variable.

@ryanjbaxter
Copy link
Contributor

Can you try using Spring Cloud 2022.0.4?

@leovx
Copy link
Contributor Author

leovx commented Oct 14, 2023

I tried Spring Boot 3.1.4 + Spring Cloud 2022.0.4 which points to spring-cloud-context:4.0.4, this remediates the issue, even set spring.cloud.config.initialize-on-context-refresh: false.

Comparing code snippets of 4.0.4 and 3.1.7, the difference mainly comes from method addProfilesTo, where in 3.1.7, it collects all values of given property in else clause while 4.0.4 eventually only collects spring.profiles.include regardless any given property.

3.1.7
Collections.addAll(profiles, getProfilesForValue(propertySource.getProperty(property), environment));

4.0.4
Collections.addAll(profiles, getProfilesForValue(propertySource.getProperty(BootstrapConfigFileApplicationListener.INCLUDE_PROFILES_PROPERTY), environment));

Then, we could have below 2 problems to answer:

  1. not all companies are able to upgrade to Spring Boot 3.x and Spring Cloud 2022.x.x at this point of time
  2. what is the point of having addActiveProfilesTo(activeProfiles, propertySource, environment); inside handleProfiles method? Because, by any means, when it finally hits addProfilesTo, it will only collect property of BootstrapConfigFileApplicationListener.INCLUDE_PROFILES_PROPERTY, i.e. spring.profiles.include. Should this be removed then if meaningless? (sounds like rollbacking this back to 3.1.6 behaviour)

@ryanjbaxter
Copy link
Contributor

The reason for the different behaviors between branches is that there was a bad merge, all the branches should be identical. I added some comments to your PR

@ryanjbaxter ryanjbaxter added this to the 3.1.8 milestone Oct 17, 2023
@leovx
Copy link
Contributor Author

leovx commented Oct 18, 2023

Do you need me to raise another PR to sync methods addActiveProfilesTo and addProfilesTo for 4.0.x as well?

@ryanjbaxter
Copy link
Contributor

No I will merge the changes forward

ryanjbaxter pushed a commit that referenced this issue Oct 18, 2023
…ions met (#1286)

* Fix #1285 Spring Cloud activates unexpected profiles when some conditions met

* Fix code style
@leovx leovx closed this as completed Oct 19, 2023
@leovx
Copy link
Contributor Author

leovx commented Oct 21, 2023

BTW, once this fix is applied, you do not really need to have spring.cloud.config.initialize-on-context-refresh=true configured in bootstrap.yml, all existing BAU code and config should work like a charm just as expected as before.

@internetstaff
Copy link

This is a pretty bad bug as outlined in "Potential Hazard" above - can 3.1.8 be kicked out soon?

@ryanjbaxter
Copy link
Contributor

We are planning a release towards the end of December
https://github.com/spring-cloud/spring-cloud-release/milestones?with_issues=no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Status: Done
Status: Done
4 participants