feat(parametermanager): Added samples for kms_key field in regional parameter #10094
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
Hello @durgesh-ninave-crest, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.
This PR, authored by durgesh-ninave-crest, introduces new sample code for the Parameter Manager regional API, specifically focusing on integrating with KMS keys. The samples demonstrate how to create a regional parameter with a specified KMS key, update an existing regional parameter to use a different KMS key, and remove the KMS key association from a regional parameter. Corresponding integration tests have also been added to verify the functionality of these new samples.
Highlights
- New Samples for KMS Inte
8000
gration: Adds three new Java samples demonstrating how to manage the
kms_keyfield for regional Parameter Manager parameters:create_regional_param_with_kms_key,update_regional_param_kms_key, andremove_regional_param_kms_key. - Integration Tests Added: Includes new integration tests in
SnippetsIT.javato validate the functionality of the new KMS-related samples. This involves setting up and tearing down necessary KMS resources (KeyRings and CryptoKeys) as part of the test lifecycle. - Dependency Updates: Updates the
libraries-bomversion inpom.xmland adds thegoogle-cloud-kmsdependency for testing purposes.
Changelog
Click here to see the changelog
- parametermanager/pom.xml
- Updated
libraries-bomversion from26.54.0to26.60.0(line 46). - Added
google-cloud-kmsdependency withtestscope (lines 94-98).
- Updated
- parametermanager/src/main/java/parametermanager/regionalsamples/CreateRegionalParamWithKmsKey.java
- Added new file containing the
createRegionalParameterWithKmsKeysample, demonstrating how to create a regional parameter linked to a KMS key (lines 1-75).
- Added new file containing the
- parametermanager/src/main/java/parametermanager/regionalsamples/RemoveRegionalParamKmsKey.java
- Added new file containing the
removeRegionalParamKmsKeysample, demonstrating how to remove the KMS key association from a regional parameter using a field mask (lines 1-80).
- Added new file containing the
- parametermanager/src/main/java/parametermanager/regionalsamples/UpdateRegionalParamKmsKey.java
- Added new file containing the
updateRegionalParamKmsKeysample, demonstrating how to update the KMS key associated with a regional parameter using a field mask (lines 1-82).
- Added new file containing the
- parametermanager/src/test/java/parametermanager/regionalsamples/SnippetsIT.java
- Added necessary imports for KMS client and related classes (lines 20-31, 53-54).
- Added static variables to hold test parameters and KMS resource IDs (lines 95-100).
- Modified
beforeAllmethod to create KMS KeyRings and CryptoKeys (HSM) required for the new tests, and create initial parameters for update/delete KMS tests (lines 201-226). - Modified
afterAllmethod to clean up the test parameters and destroy KMS CryptoKeyVersions created during tests (lines 258-283). - Added
getKeyRingNameandgetLocationNamehelper methods for KMS resource naming (lines 291-297). - Added
createParameterWithKms,createKeyRing, andcreateHsmKeyhelper methods for test setup (lines 314-357). - Added three new test methods:
testCreateRegionalParamWithKmsKey,testUpdateRegionalParamKmsKey, andtestRemoveRegionalParamKmsKeyto validate the new samples (lines 648-697).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A parameter's key,
Protected carefully,
With KMS linked,
Secrets are synced,
Secure for all to see.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces valuable samples for managing KMS keys with regional parameters, including creation, update, and removal scenarios. The accompanying tests cover these new functionalities. The core sample logic is well-implemented.
However, there are a couple of significant concerns in the SnippetsIT.java test setup that need attention to ensure test correctness and robustness. Specifically, the management of HSM_KEY_ID for different test cases and the idempotency of the createHsmKey helper method.
Summary of Findings
- Test Logic with Static Fields: The static field
HSM_KEY_IDis reused and overwritten inbeforeAllfor different test setups. This leads to tests potentially using incorrect KMS key IDs, affecting test validity. This is a high severity issue. - Test Helper Idempotency: The
createHsmKeytest helper inSnippetsIT.javadoes not handleAlreadyExistsException, which could make tests less robust on retries or in environments with pre-existing resources. This is a medium severity issue. - Copyright Year: The copyright year in new Java sample files is
2025. This should be updated to the current year. (Low severity, not commented in code) - Javadoc Clarity: The Javadoc for
RemoveRegionalParamKmsKey.javacould be more specific about the action of removing the KMS key. (Low severity, not commented in code)
Merge Readiness
The pull request adds important new samples. However, due to the high severity issue identified in the test logic (SnippetsIT.java), which could affect the correctness of test validations, I recommend addressing this and the medium severity idempotency issue before merging. As an AI reviewer, I am not authorized to approve pull requests; please ensure these changes are reviewed and approved by a human maintainer after the suggested modifications are made.
| TEST_PARAMETER_NAME_WITH_KMS = ParameterName.of(PROJECT_ID, LOCATION_ID, randomId()); | ||
| KEY_RING_ID = "test-regional-parameter-manager-snippets"; | ||
| HSM_KEY_ID = randomId(); | ||
| createKeyRing(KEY_RING_ID); | ||
| createHsmKey(HSM_KEY_ID); | ||
|
|
||
| // test update kms key of parameter | ||
| TEST_PARAMETER_NAME_UPDATE_WITH_KMS = ParameterName.of(PROJECT_ID, LOCATION_ID, randomId()); | ||
| KEY_RING_ID = "test-regional-parameter-manager-snippets"; | ||
| HSM_KEY_ID = randomId(); | ||
| NEW_HSM_KEY_ID = randomId(); | ||
| createKeyRing(KEY_RING_ID); | ||
| createHsmKey(HSM_KEY_ID); | ||
| createHsmKey(NEW_HSM_KEY_ID); | ||
| String kmsKeyId = CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, HSM_KEY_ID).toString(); | ||
| createParameterWithKms(TEST_PARAMETER_NAME_UPDATE_WITH_KMS.getParameter(), kmsKeyId); | ||
|
|
||
| // test delete kms key of parameter | ||
| TEST_PARAMETER_NAME_DELETE_WITH_KMS = ParameterName.of(PROJECT_ID, LOCATION_ID, randomId()); | ||
| KEY_RING_ID = "test-regional-parameter-manager-snippets"; | ||
| HSM_KEY_ID = randomId(); | ||
| createKeyRing(KEY_RING_ID); | ||
| createHsmKey(HSM_KEY_ID); | ||
| kmsKeyId = CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, HSM_KEY_ID).toString(); | ||
| createParameterWithKms(TEST_PARAMETER_NAME_DELETE_WITH_KMS.getParameter(), kmsKeyId); | ||
| } |
There was a problem hiding this comment.
The static field HSM_KEY_ID (and KEY_RING_ID, though its value remains constant) is reassigned multiple times within beforeAll for different test setups (lines 204, 211, 222). Since HSM_KEY_ID is static, its final value after beforeAll completes will be the one set in the last block (for TEST_PARAMETER_NAME_DELETE_WITH_KMS).
This causes issues:
- In
testCreateRegionalParamWithKmsKey,CryptoKeyName.of(..., HSM_KEY_ID)will use the final value ofHSM_KEY_ID, not the specificHSM_KEY_IDintended and created forTEST_PARAMETER_NAME_WITH_KMSin lines 204-206. - In
testUpdateRegionalParamKmsKey, the assertionassertThat(updatedParameter.getKmsKey()).doesNotContain(HSM_KEY_ID)will also use the final value ofHSM_KEY_ID, not the original HSM key ID thatTEST_PARAMETER_NAME_UPDATE_WITH_KMSwas initially created with (lines 211-217).
This can lead to tests passing or failing for incorrect reasons. Could you refactor this to use distinct static fields for each unique HSM key ID required by the tests? For example:
// At class level
private static String CREATE_TEST_HSM_KEY_ID;
private static String UPDATE_TEST_ORIGINAL_HSM_KEY_ID;
// private static String NEW_HSM_KEY_ID; // This one is already distinct
private static String DELETE_TEST_HSM_KEY_ID;
// In beforeAll():
// For create test setup (lines 202-206)
CREATE_TEST_HSM_KEY_ID = randomId();
createHsmKey(CREATE_TEST_HSM_KEY_ID);
// For update test setup (lines 209-217)
UPDATE_TEST_ORIGINAL_HSM_KEY_ID = randomId();
// NEW_HSM_KEY_ID is already fine
createHsmKey(UPDATE_TEST_ORIGINAL_HSM_KEY_ID);
createHsmKey(NEW_HSM_KEY_ID);
String kmsKeyIdForUpdate = CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, UPDATE_TEST_ORIGINAL_HSM_KEY_ID).toString();
createParameterWithKms(TEST_PARAMETER_NAME_UPDATE_WITH_KMS.getParameter(), kmsKeyIdForUpdate);
// For delete test setup (lines 220-226)
DELETE_TEST_HSM_KEY_ID = randomId();
createHsmKey(DELETE_TEST_HSM_KEY_ID);
String kmsKeyIdForDelete = CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, DELETE_TEST_HSM_KEY_ID).toString();
createParameterWithKms(TEST_PARAMETER_NAME_DELETE_WITH_KMS.getParameter(), kmsKeyIdForDelete);Then, tests would use these specific static fields (e.g., CREATE_TEST_HSM_KEY_ID, UPDATE_TEST_ORIGINAL_HSM_KEY_ID).
| ParameterName parameterName = TEST_PARAMETER_NAME_WITH_KMS; | ||
| String cryptoKey = CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, HSM_KEY_ID) | ||
| .toString(); | ||
| CreateRegionalParamWithKmsKey | ||
| .createRegionalParameterWithKmsKey( | ||
| parameterName.getProject(), | ||
| LOCATION_ID, | ||
| parameterName.getParameter(), | ||
| cryptoKey); |
There was a problem hiding this comment.
Due to the reassignment of the static HSM_KEY_ID in beforeAll, this test will use the HSM_KEY_ID value that was set during the setup for the delete test (TEST_PARAMETER_NAME_DELETE_WITH_KMS), not the HSM_KEY_ID that was specifically created for TEST_PARAMETER_NAME_WITH_KMS.
This test will attempt to create a parameter using an unintended KMS key. Please ensure this test uses the correct KMS key ID that was provisioned for TEST_PARAMETER_NAME_WITH_KMS (e.g., by using a distinct static field like CREATE_TEST_HSM_KEY_ID as suggested in the comment for the beforeAll method).
| assertThat(updatedParameter.getKmsKey()).contains(NEW_HSM_KEY_ID); | ||
| assertThat(updatedParameter.getKmsKey()).doesNotContain(HSM_KEY_ID); | ||
| } |
There was a problem hiding this comment.
This assertion assertThat(updatedParameter.getKmsKey()).doesNotContain(HSM_KEY_ID) uses the static HSM_KEY_ID. Due to its reassignment in beforeAll, this HSM_KEY_ID will hold the value from the setup for the delete test, not the original KMS key ID that TEST_PARAMETER_NAME_UPDATE_WITH_KMS was created with.
The assertion should verify that the updated parameter's KMS key does not contain the ID of the key it was originally encrypted with. Please use the correct original key ID for this parameter (e.g., by using a distinct static field like UPDATE_TEST_ORIGINAL_HSM_KEY_ID as suggested in the comment for the beforeAll method).
| private static CryptoKey createHsmKey(String keyId) throws IOException { | ||
| try (KeyManagementServiceClient client = KeyManagementServiceClient.create()) { | ||
| CryptoKey key = | ||
| CryptoKey.newBuilder() | ||
| .setPurpose(CryptoKey.CryptoKeyPurpose.ENCRYPT_DECRYPT) | ||
| .setVersionTemplate( | ||
| CryptoKeyVersionTemplate.newBuilder() | ||
| .setAlgorithm(CryptoKeyVersion | ||
| .CryptoKeyVersionAlgorithm.GOOGLE_SYMMETRIC_ENCRYPTION) | ||
| .setProtectionLevel(ProtectionLevel.HSM) | ||
| .build()) | ||
| .putLabels("foo", "bar") | ||
| .putLabels("zip", "zap") | ||
| .build(); | ||
| CryptoKey createdKey = client.createCryptoKey(getKeyRingName(), keyId, key); | ||
| return createdKey; | ||
| } | ||
| } |
There was a problem hiding this comment.
The createHsmKey method doesn't seem to handle AlreadyExistsException, unlike createKeyRing. If a test run fails and is re-executed, or if keys with the same ID somehow pre-exist, this could cause the test setup in beforeAll to fail.
Would it be beneficial to add similar AlreadyExistsException handling here to make the test setup more robust? For example:
private static CryptoKey createHsmKey(String keyId) throws IOException {
try (KeyManagementServiceClient client = KeyManagementServiceClient.create()) {
CryptoKey key = // ... as before ...
CryptoKey createdKey = client.createCryptoKey(getKeyRingName(), keyId, key);
return createdKey;
} catch (AlreadyExistsException e) {
// If the key already exists, retrieve and return it.
try (KeyManagementServiceClient client = KeyManagementServiceClient.create()) {
return client.getCryptoKey(CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, keyId));
}
}
}This would make the key creation step idempotent.
private static CryptoKey createHsmKey(String keyId) throws IOException {
try (KeyManagementServiceClient client = KeyManagementServiceClient.create()) {
CryptoKey key =
CryptoKey.newBuilder()
.setPurpose(CryptoKey.CryptoKeyPurpose.ENCRYPT_DECRYPT)
.setVersionTemplate(
CryptoKeyVersionTemplate.newBuilder()
.setAlgorithm(CryptoKeyVersion
.CryptoKeyVersionAlgorithm.GOOGLE_SYMMETRIC_ENCRYPTION)
.setProtectionLevel(ProtectionLevel.HSM)
.build())
.putLabels("foo", "bar")
.putLabels("zip", "zap")
.build();
CryptoKey createdKey = client.createCryptoKey(getKeyRingName(), keyId, key);
return createdKey;
} catch (AlreadyExistsException e) {
// If the key already exists, it might be from a previous partial run.
// For test idempotency, we can try to fetch and return the existing key.
try (KeyManagementServiceClient client = KeyManagementServiceClient.create()) {
return client.getCryptoKey(CryptoKeyName.of(PROJECT_ID, LOCATION_ID, KEY_RING_ID, keyId));
}
}
}
Description
Added samples for kms_key field in regional parameter using Parameter manager SDK
Sample List (regional):
Added required Tests for the same.
Checklist
pom.xmlparent set to latestshared-configurationmvn clean verifyrequiredmvn -P lint checkstyle:checkrequiredmvn -P lint clean compile pmd:cpd-check spotbugs:checkadvisory only