Spring Cloud activates unexpected profiles when some conditions met

leovx commented 1 year ago

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:

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

spring.profiles.active: local
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
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 commented 1 year ago

It should not be too hard to fix this, and I would like to raise a PR for it. https://github.com/spring-cloud/spring-cloud-commons/pull/1286

leovx commented 1 year ago

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

ryanjbaxter commented 1 year ago

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

leovx commented 1 year ago

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

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

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

Can you try using Spring Cloud 2022.0.4?

leovx commented 1 year ago

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:

not all companies are able to upgrade to Spring Boot 3.x and Spring Cloud 2022.x.x at this point of time
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 commented 1 year ago

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

leovx commented 1 year ago

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

ryanjbaxter commented 1 year ago

No I will merge the changes forward

leovx commented 1 year ago

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

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

ryanjbaxter commented 12 months ago

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

spring-cloud / spring-cloud-commons