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

feat: support for spring.config.import GCP Secret Manager #1204

Merged

Conversation

JoeWang1127
Copy link
Contributor

@JoeWang1127 JoeWang1127 commented Jul 27, 2022

External resources from GCP Secret Manager can be imported to custom project by add the following line in the application.properties:
spring.config.import=sm://

This feature is implemented using Spring Boot’s Config Data API, more info can be found in here
fix #149

Users who want to use this feature should either exclude <artifactId>spring-cloud-starter-bootstrap</artifactId> from their dependency or to set spring.cloud.gcp.secretmanager.legacy=false

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not finish reviewing, sorry. But initial couple of comments.

@JoeWang1127 JoeWang1127 marked this pull request as ready for review August 15, 2022 19:20
@JoeWang1127 JoeWang1127 requested a review from elefeint August 15, 2022 19:24
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested everything, and functionality is great -- everything works, even when both legacy and config.data ways work together. Having refresh work is also awesome.

I have a
I've been thinking about the different usage scenarios for Secret Manager, and realized I was missing the one where users don't need property discovery, but still want things like SecretManagerTemplate autoconfigured.

  1. Users still on the "old" system of using legacy bootstrap phase. This should be the default, and they should both get SecretManagerTemplate autoconfigured, and have "sm://property" type values resolved.

  2. Users on the new system -- they would have "spring.config.import=sm://" in application.properties and "spring.cloud.gcp.secretmanager.legacy=false" in bootstrap.properities. They should get SecretManagerTemplate autoconfigured, and have "sm://property" type values resolved, just like in (1).

  3. Users with ""spring.cloud.gcp.secretmanager.legacy=false" and no "spring.config.import=sm://" -- the end result here is that "sm://" properties do not get resolved at all, but these users still need SecretManagerTemplate to be autoconfigured. Currently, this is a completely contrived scenario because why would someone do that? But once we remove the legacy bootstrap option in the next major release, this will be a common default scenario.

To make sure we are covering these options, could you add an autoconfiguration test similar to SecretManagerBootstrapConfigurationTests where those 3 scenarios are tested?

To do so, you'll need to mock out the real SecretManagerClient. Take a look at SecretManagerBootstrapConfigurationTests.secretManagerClient() -- this works because the boostrap configuration has @ConditionalOnMissingBean. Since you are programmatically registering the client instead, this exact approach won't work, but I think the current implementation using context.getBootstrapContext().registerIfAbsent() will do something similar and avoid creating a real client if a mock one is already present in context. You'll have to test it.

@JoeWang1127 JoeWang1127 force-pushed the feature/support-spring-config-import-for-SecretManager branch from e35153c to f6181b6 Compare August 18, 2022 21:12
@JoeWang1127
Copy link
Contributor Author

I've tested everything, and functionality is great -- everything works, even when both legacy and config.data ways work together. Having refresh work is also awesome.

I have a I've been thinking about the different usage scenarios for Secret Manager, and realized I was missing the one where users don't need property discovery, but still want things like SecretManagerTemplate autoconfigured.

  1. Users still on the "old" system of using legacy bootstrap phase. This should be the default, and they should both get SecretManagerTemplate autoconfigured, and have "sm://property" type values resolved.
  2. Users on the new system -- they would have "spring.config.import=sm://" in application.properties and "spring.cloud.gcp.secretmanager.legacy=false" in bootstrap.properities. They should get SecretManagerTemplate autoconfigured, and have "sm://property" type values resolved, just like in (1).
  3. Users with ""spring.cloud.gcp.secretmanager.legacy=false" and no "spring.config.import=sm://" -- the end result here is that "sm://" properties do not get resolved at all, but these users still need SecretManagerTemplate to be autoconfigured. Currently, this is a completely contrived scenario because why would someone do that? But once we remove the legacy bootstrap option in the next major release, this will be a common default scenario.

To make sure we are covering these options, could you add an autoconfiguration test similar to SecretManagerBootstrapConfigurationTests where those 3 scenarios are tested?

To do so, you'll need to mock out the real SecretManagerClient. Take a look at SecretManagerBootstrapConfigurationTests.secretManagerClient() -- this works because the boostrap configuration has @ConditionalOnMissingBean. Since you are programmatically registering the client instead, this exact approach won't work, but I think the current implementation using context.getBootstrapContext().registerIfAbsent() will do something similar and avoid creating a real client if a mock one is already present in context. You'll have to test it.

Unit tests on different scenarios are added.

@JoeWang1127 JoeWang1127 requested a review from elefeint August 19, 2022 15:11
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've retested both scenarios manually, and they work great.
At this point, I only have minor nits.

Could you also add documentation in both secretmanager.adoc and secretmanager.md to instruct users how to enable the new, Config Data way?

@JoeWang1127 JoeWang1127 force-pushed the feature/support-spring-config-import-for-SecretManager branch from f359552 to ba52c81 Compare August 24, 2022 15:20
@JoeWang1127 JoeWang1127 requested a review from elefeint August 24, 2022 15:20
docs/src/main/asciidoc/secretmanager.adoc Show resolved Hide resolved
docs/src/main/asciidoc/secretmanager.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/secretmanager.adoc Outdated Show resolved Hide resolved
@JoeWang1127 JoeWang1127 requested a review from elefeint August 24, 2022 17:14
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny wording updates, and done!!!

Merge it in.

docs/src/main/asciidoc/secretmanager.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/secretmanager.adoc Outdated Show resolved Hide resolved
docs/src/main/md/secretmanager.md Outdated Show resolved Hide resolved
docs/src/main/md/secretmanager.md Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

84.0% 84.0% Coverage
0.0% 0.0% Duplication

@JoeWang1127 JoeWang1127 merged commit 58b9fb9 into main Aug 24, 2022
@JoeWang1127 JoeWang1127 deleted the feature/support-spring-config-import-for-SecretManager branch August 24, 2022 18:11
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
…oudPlatform#1204)

* feat: support  for GCP Secret Manager

* add copyright info

* use auto configuration rather than registering service client and serect template manually.

* fix broken unit tests

* disable old boostrap way; enable new config data way

* deprecate bootstrap configuration for secret manager

* remove legacy dependency: spring-cloud-starter-bootstrap

* enable  for secret manager

* restore secret manager resource converter

* modify secret manager sample application to enable secret refreshing mechanism

* fix broken test

* restore sm bootstrap configuration to fix broken uts

* add copyright info

* add  related commit

* fix broken unit test

* fix broken unit test

* fix broken unit test

* disable a broken unit test

* prevent creating a new client while refreshing

* refactor after code review

* register sm client lazily

* fix broken test

* fix broken sm intergration test

* change method signature for testing

* increase code coverage

* modify secret manager sample for integration test in pipeline

* remove broken unit test

* bring back legacy dependency to avoid breaking changes

* refactor code according to code review

* instruct the user on how to enable spring boot's config data api

* refactor code after code review

* disable SecretManagerPropertySourceLocator bean when the user enable

* add unit tests to check compatibility of secret manager

* add docs on how to refresh secrets

* modify sample project for backward compatibility

* remove public identifier in test class

* add instructions on secret refreshing in docs

* change secret value in

* modify secret manager sample app to emphasize on refreshing secrets

* update docs of secret manager

* update wording in secret manager docs

Co-authored-by: Elena Felder <41136058+elefeint@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support spring.config.import for Secret Manager
2 participants