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

fix(kms): to honor kms credentials when present. #1272

Merged
merged 6 commits into from
Oct 5, 2022
Merged

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Sep 28, 2022

fixes #1271.

I find it hard to increase test coverage without exposing more getter methods from KmsTemplate.class or GcpKmsAutoConfiguration.class. Any thoughts?

@zhumin8 zhumin8 marked this pull request as ready for review September 29, 2022 21:20
@zhumin8 zhumin8 requested a review from elefeint September 29, 2022 21:20
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 think it's fine to have a package-private getCredentialsProvider and test that under different autoconfig scenarios -- GcpFirestoreAutoConfiguration does this.. It can also be annotated with @VisibleForTesting, although I have mixed feelings about the annotation generally.

@zhumin8
Copy link
Contributor Author

zhumin8 commented Sep 30, 2022

Turns out KmsAutoConfiguration is also not taking core project id when missing kms specific project id as stated in documentation.
Fixed both and added tests for all 4 scenarios.

@zhumin8 zhumin8 requested a review from elefeint September 30, 2022 22:40
Comment on lines 51 to 53
TestConfiguration.class, GcpKmsAutoConfiguration.class).properties(
"spring.cloud.gcp.kms.project-id=" + KMS_PROJECT_NAME, "spring.cloud.bootstrap.enabled=true",
"spring.cloud.gcp.sql.enabled=false").web(WebApplicationType.NONE);
Copy link
Contributor

Choose a reason for hiding this comment

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

original formatting with property per line was probably easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! thanks for catching this. My formatter messed up in a few places, reverting this.

"auth_uri": "https://accounts.google.com/o/oauth2/auth",
"token_uri": "https://oauth2.googleapis.com/token",
"auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
"client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/one-time-test-account%40mzhu-test3.iam.gserviceaccount.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be more fake? Very impressive fake key, btw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually borrowed the idea from this fake-project-key.json. And you are right, I missed the last few lines, they can be fake too.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2022

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 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@zhumin8 zhumin8 merged commit 259a5d1 into main Oct 5, 2022
@zhumin8 zhumin8 deleted the fix-kms-cred branch October 5, 2022 18:11
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
…#1272)

fixes GoogleCloudPlatform#1271.
Fix so `KmsAutoConfiguration` takes kms specific project id & credentials when present, otherwise fall back to core project id & credentials. Documentation is accurate, no change.
Added tests for all 4 scenarios.
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.

kms credentials configuration property does not work as documented
2 participants