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

CloudSqlEnvironmentPostProcessor not compatible with Spring Cloud Bootstrap PropertySourceLocator mechanism #433

Closed
mzeijen opened this issue Apr 7, 2021 · 21 comments
Assignees

Comments

@mzeijen
Copy link
Contributor

mzeijen commented Apr 7, 2021

I discovered that the new CloudSqlEnvironmentPostProcessor within Spring Cloud GCP 2.x causes issues when you combine it with properties that are being loaded via a PropertySource that is created via the Spring Cloud Bootstrap PropertySourceLocator. The reason is that the properties loaded via a PropertySource that is created by a PropertySourceLocator are loaded after the CloudSqlEnvironmentPostProcessor is executed.

The case in which we discovered this is as follows:

  • In the application.yml, on the classpath, the property spring.cloud.gcp.sql.enabled is set to false
  • When the application is deployed to a GKE environment, Spring Cloud Kubernetes is used to load a Kubernetes ConfigMap, via the Kubernetes API so the ConfigMap is not mounted in the container, and this ConfigMap contains the same spring.cloud.gcp.sql.enabled property but it set to true
  • The intention is to have CloudSQL disabled when developing locally, but it is used when running the application within GKE.
  • When deploying in GKE with the ConfigMap, the spring.cloud.gcp.sql.enabled=true is never picked up by the CloudSqlEnvironmentPostProcessor and thus it doesn't do its thing to setup the required correct Datasource for CloudSQL. The application fails to startup.
  • The issue is that Spring Cloud Kubernetes uses the Spring Cloud Bootstrap PropertySourceLocator mechanism to create a PropertySource for the ConfigMap but this happens when the ApplicationContextInitializers are executed, which is after the EnvironmentPostProcessor have been executed in the normal application context startup phase (so the PropertySourceLocator is only created in the bootstrap contact startup phase and executed in the application context startup phase). Because of that the CloudSqlEnvironmentPostProcessor never sees the properties from the ConfigMap PropertySource.

To figure out what was going on took some time. In the end, there are plenty workaround like simply mounting the ConfigMap and loading via one of the ways that Spring Boot has to load configuration, but it would be best if the CloudSQL support mechanism in Spring Cloud GCP would simply be compatible with Spring Cloud Bootstrap and it all works as expected.

I am not sure what a good solution is. Maybe go back to the mechanism from Spring Cloud GCP 1.x?

Sample
I have not provided a code sample, as this is requires fairly complicated setup to demonstrate the issue. I hope that the explanation above suffices to make the issue clear.

@dzou
Copy link
Contributor

dzou commented Apr 7, 2021

Thanks for the report. Unfortunately we migrated from the old way of doing things due to some issues that emerged in the ilford releases (context here). I don't think we can switch back.

Do you know if the CloudSqlEnvironmentPostProcessor gets executed once or twice?

We had a bootstrap issue here before which sounds like it might have solved a similar problem? Perhaps we can add your property source to that if-condition described in the thread.

if (environment.getPropertySources().contains("bootstrap") || /* Your property source name here */) {
      // Do not run in the bootstrap phase as the user configuration is not available yet
      return false;
}

@dzou dzou added the type: bug Something isn't working label Apr 7, 2021
@mzeijen
Copy link
Contributor Author

mzeijen commented Apr 8, 2021

Thanks for the background information. It will be a hard one to fix then. A possibly ugly solution would be to create a ApplicationContextInitializer that does the work instead?
At least it would be good to add some kind of warning to the documentation that the current implementation doesn't play nice with PropertySourceLocators like those from Spring Cloud Kubernetes?

Regarding your other question. The CloudSqlEnvironmentPostProcessor is indeed executed twice, once in the bootstrap startup phase and once in the application startup phase, but that conditional indeed ensures it only does work on the application startup phase. But at that point the PropertySourceLocators haven't created the PropertySources yet.
TBH, I believe the PropertySourceLocator mechanism is actually the problem here. In my opinion it should have resolved all the properties in the bootstrap phase and then have injected the PropertySources into the application startup phase even before or at the start of the execution of the EnvironmentPostProcessors. But I guess the PropertySourceLocator mechanism is something that is going to be removed over time (and the bootstrap phase as a whole) now that Spring Boot has improved mechanism to read properties from external sources.

@dzou
Copy link
Contributor

dzou commented Apr 13, 2021

I see. Yeah we can do something to update the docs. I would still be interested to see if it's possible to reproduce the issue -- Is the overall idea to set the spring.cloud.gcp.sql.enabled=false and to be able to override that to true with a bootstrap configuration?

@meltsufin
Copy link
Member

Notice that EnvironmentPostProcessors can be ordered. We did not provide a specific order for CloudSqlEnvironmentPostProcessor, and maybe that doesn't play way with the Kubernates PropertySourceLocators. I'd be happy to look into this issue, but we need some kind of repro for it. Can the Cloud SQL sample be modified to illustrate this issue?

@mzeijen
Copy link
Contributor Author

mzeijen commented Apr 15, 2021

@meltsufin Changing the order of the EnvironmentPostProcessors won't help because the PropertySourceLocator is executed in another phase, as a ApplicationContextInitializer, and that always comes after the EnvironmentPostProcessor phase.

I will create a todo for me to see if I can create a simple reproduction somewhere next week.

@meltsufin
Copy link
Member

@snicoll Do you have any insight into this kind of issue?

@snicoll
Copy link
Contributor

snicoll commented Apr 15, 2021

I thought we already discussed this, didn't we? See #215 (comment)

@mzeijen
Copy link
Contributor Author

mzeijen commented Apr 15, 2021

This problem indeed occurs due to limitations in the deprecated bootstrap mechanism. I am not sure if you should try to fix this issue. Adding a warning to the documentation regarding which properties should not be configured via property sources that require the legacy bootstrap methods, should suffice in my opinion.

@meltsufin
Copy link
Member

I'm confused on this because we specifically skip the CloudSqlEnvironmentPostProcessor in the bootstrap phase since #273. So, bootstrap should not interfere with it. The CloudSqlEnvironmentPostProcessor then actually runs during the normal application context init phase which should have all the property sources available at that point.

@snicoll
Copy link
Contributor

snicoll commented Apr 15, 2021

That's a different problem with the same cause. Even if your BPP runs in the right "context" it runs before Spring Cloud Config Server has loaded the remote config, see spring-cloud/spring-cloud-commons#608 (comment)

@mzeijen
Copy link
Contributor Author

mzeijen commented Apr 15, 2021

Indeed, but in this case it is not the Spring Cloud Config Server but a Spring Cloud Kubernetes ConfigMap PropertySource, but the mechanism is the same.

@snicoll
Copy link
Contributor

snicoll commented Apr 15, 2021

Right. Same design issue yeah.

@limkinZero
Copy link

Any change? I've the same issue when we trying to disable gcp sql module in local environment. I've configured bootstrap.yaml to disable it but the starting throws an error:

# Disable Spring Cloud Kubernetes
spring:
  cloud:
    kubernetes:
      enabled: false
    gcp:
      sql:
        enabled: false

Ouput log:

2021-10-29T09:12:25,113 WARN  [restartedMain] o.s.c.k.c.p.AbstractKubernetesProfileEnvironmentPostProcessor: Not running inside kubernetes. Skipping 'kubernetes' profile activation.
2021-10-29T09:12:25,184 INFO  [restartedMain] o.s.b.d.e.DevToolsPropertyDefaultsPostProcessor: Devtools property defaults active! Set 'spring.devtools.add-properties' to 'false' to disable
2021-10-29T09:12:25,714 INFO  [restartedMain] c.u.j.c.EnableEncryptablePropertiesBeanFactoryPostProcessor: Post-processing PropertySource instances
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource configurationProperties [org.springframework.boot.context.properties.source.ConfigurationPropertySourcesPropertySource] to AOP Proxy
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource bootstrap [org.springframework.core.env.MapPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource systemProperties [org.springframework.core.env.PropertiesPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource systemEnvironment [org.springframework.boot.env.SystemEnvironmentPropertySourceEnvironmentPostProcessor$OriginAwareSystemEnvironmentPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource random [org.springframework.boot.env.RandomValuePropertySource] to EncryptablePropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource cachedrandom [org.springframework.cloud.util.random.CachedRandomPropertySource] to EncryptablePropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource springCloudClientHostInfo [org.springframework.core.env.MapPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource Config resource 'class path resource [bootstrap.yaml]' via location 'optional:classpath:/' [org.springframework.boot.env.OriginTrackedMapPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource devtools [org.springframework.core.env.MapPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,815 INFO  [restartedMain] c.u.j.EncryptablePropertySourceConverter: Converting PropertySource defaultProperties [org.springframework.core.env.MapPropertySource] to EncryptableMapPropertySourceWrapper
2021-10-29T09:12:25,915 INFO  [restartedMain] c.u.j.r.DefaultLazyPropertyResolver: Property Resolver custom Bean not found with name 'encryptablePropertyResolver'. Initializing Default Property Resolver
2021-10-29T09:12:25,915 INFO  [restartedMain] c.u.j.d.DefaultLazyPropertyDetector: Property Detector custom Bean not found with name 'encryptablePropertyDetector'. Initializing Default Property Detector
2021-10-29T09:12:26,612 ERROR [restartedMain] o.s.b.SpringApplication: Application run failed
java.lang.IllegalArgumentException: A database name must be provided.
	at org.springframework.util.Assert.hasText(Assert.java:289)
	at com.google.cloud.spring.autoconfigure.sql.DefaultCloudSqlJdbcInfoProvider.<init>(DefaultCloudSqlJdbcInfoProvider.java:41)
	at com.google.cloud.spring.autoconfigure.sql.CloudSqlEnvironmentPostProcessor.postProcessEnvironment(CloudSqlEnvironmentPostProcessor.java:86)
	at org.springframework.boot.env.EnvironmentPostProcessorApplicationListener.onApplicationEnvironmentPreparedEvent(EnvironmentPostProcessorApplicationListener.java:100)

@meltsufin
Copy link
Member

@limkinZero Are you able to use the setting in application.yaml? CloudSqlEnvironmentPostProcessor is not part of the bootstrap phase.

@limkinZero
Copy link

limkinZero commented Oct 30, 2021 via email

@meltsufin
Copy link
Member

@limkinZero That's interesting. You might have something else going on in your application configuration. Would you be able to provide a reproducible example of this issue?

@limkinZero
Copy link

limkinZero commented Oct 30, 2021 via email

@meltsufin
Copy link
Member

@limkinZero Would you be able to make the necessary changes in a fork and provide exact instructions on how to reproduce the issue?

@elefeint
Copy link
Contributor

elefeint commented Aug 8, 2022

I wonder if we should try implementing the Cloud SQL configuration as a custom ConfigDataResource, similarly to what @JoeWang1127 is doing with Secret Manager now. This would allow the datasource properties to be truly ordered together with the other spring.config.import property sources.

This would not help with refreshing credentials and such because of GoogleCloudPlatform/cloud-sql-jdbc-socket-factory#776 / #1204, but it may be an improvement.

@mzeijen What are you doing as a workaround in the meantime -- maven profiles?

@JoeWang1127 JoeWang1127 self-assigned this Aug 24, 2022
@JoeWang1127
Copy link
Contributor

@mzeijen @limkinZero @elefeint I created a demo project in here. It can be treated as a workaround of this issue.

@JoeWang1127
Copy link
Contributor

Close this issue as no response, please reopen the issue if needed.

prash-mi pushed a commit that referenced this issue Jun 20, 2023
Bumps [checkstyle](https://github.com/checkstyle/checkstyle) from 8.45.1 to 9.0.
- [Release notes](https://github.com/checkstyle/checkstyle/releases)
- [Commits](checkstyle/checkstyle@checkstyle-8.45.1...checkstyle-9.0)

---
updated-dependencies:
- dependency-name: com.puppycrawl.tools:checkstyle
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants