feat(parametermanager): add global and regional samples for kms_key field #4059
Conversation
product-auto-label
bot
added
the
samples
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @vatsal-vora-crestdata, 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
This pull request adds samples for the KMS key field in the global and regional parameter manager. It includes samples for creating, updating, and removing KMS keys for both global and regional parameters using the Parameter Manager SDK. The changes include new JavaScript files for each of these operations, a package.json file to manage dependencies, an .eslintrc.yaml file for linting tests, and a test file to verify the functionality of the new samples.
Highlights
- New Samples: Added six new samples:
create_regional_param_with_kms_key,remove_regional_param_kms_key,update_regional_param_kms_key,create_param_with_kms_key,remove_param_kms_key, andupdate_param_kms_key. - Regional Parameter Support: The pull request introduces regional parameter support by adding samples that use a regional endpoint for the Parameter Manager client.
- KMS Key Integration: The samples demonstrate how to create, update, and remove KMS keys associated with parameters, enhancing security.
- Testing: Added a test suite to verify the functionality of the new samples, including setup and teardown of test parameters and KMS keys.
Changelog
Click here to see the changelog
- parametermanager/createParamWithKmsKey.js
- Added a new sample to create a global parameter with a KMS key.
- The sample uses the Parameter Manager SDK to create the parameter with the specified KMS key.
- parametermanager/package.json
- Added dependencies for
@google-cloud/parametermanager,@google-cloud/kms,c8,chai,mocha, anduuid. - Updated the test script to use
c8 mochafor code coverage.
- Added dependencies for
- parametermanager/regional_samples/createRegionalParamWithKmsKey.js
- Added a new sample to create a regional parameter with a KMS key.
- The sample configures the Parameter Manager client to use a regional endpoint.
- parametermanager/regional_samples/removeRegionalParamKmsKey.js
- Added a new sample to remove a KMS key from a regional parameter.
- The sample uses an update mask to remove the KMS key from the parameter.
- parametermanager/regional_samples/updateRegionalParamKmsKey.js
- Added a new sample to update a regional parameter with a KMS key.
- The sample uses an update mask to update the KMS key for the parameter.
- parametermanager/removeParamKmsKey.js
- Added a new sample to remove a KMS key from a global parameter.
- The sample uses an update mask to remove the KMS key from the parameter.
- parametermanager/test/.eslintrc.yaml
- Added a new file to configure ESLint for the test directory.
- Configured the environment to include Mocha.
- parametermanager/test/parametermanager.test.js
- Added a new test suite to verify the functionality of the KMS key samples.
- The test suite includes tests for creating, updating, and removing KMS keys for both global and regional parameters.
- The test suite sets up and tears down test parameters and KMS keys.
- parametermanager/updateParamKmsKey.js
- Added a new sample to update a global parameter with a KMS key.
- The sample uses an update mask to update the KMS key for the parameter.
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.
In realms of code, where secrets reside,
KMS keys stand, as guardians confide.
Global or regional,
Security's the legion,
Protecting parameters, far and wide.
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 samples for using KMS keys with the Parameter Manager API, covering both global and regional parameters. The samples include creating, updating, and removing KMS keys. The code is well-structured and includes necessary error handling. However, there are a few areas that could be improved for clarity and consistency.
Summary of Findings
- Inconsistent parameter deletion in tests: The tests create parameters with
-1suffixes, but theparametersToDeletearray is populated with the parameter names without the suffix. This could lead to issues during cleanup. - Missing error handling for KMS key creation: The tests attempt to create KMS keys but don't handle errors other than
error.code === 5. Other potential errors during KMS key creation should be considered. - Incomplete removal of KMS keys: The tests destroy crypto key versions, but not the crypto keys themselves, which could lead to resource accumulation.
Merge Readiness
The pull request is almost ready for merging. However, the issues identified in the review comments, particularly those related to test cleanup and error handling, should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issues should be addressed before merging.
| format: 'JSON', | ||
| }, | ||
| }); | ||
| parametersToDelete.push(parameter); |
There was a problem hiding this comment.
The parameter variable contains the full parameter object, but you're only pushing the object to the parametersToDelete array. Since the deleteParameter method takes the parameter name, you should extract the name from the parameter object and push that to the array instead.
| parametersToDelete.push(parameter); | |
| parametersToDelete.push(parameter.name); |
| format: 'JSON', | ||
| }, | ||
| }); | ||
| regionalParametersToDelete.push(regionalParameter); |
There was a problem hiding this comment.
The regionalParameter variable contains the full parameter object, but you're only pushing the object to the regionalParametersToDelete array. Since the deleteParameter method takes the parameter name, you should extract the name from the parameter object and push that to the array instead.
| regionalParametersToDelete.push(regionalParameter); | |
| regionalParametersToDelete.push(regionalParameter.name); |
| parametersToDelete.forEach(async parameterName => { | ||
| await client.deleteParameter({name: parameterName}); | ||
| }); |
There was a problem hiding this comment.
The parametersToDelete array contains parameter names with -1 suffixes in some cases, but the loop iterates through the array and attempts to delete parameters without the suffix. This will cause the deletion to fail for those parameters. Ensure that the parameter names in the array match the actual parameter names that were created.
| regionalParametersToDelete.forEach(async regionalParameterName => { | ||
| await regionalClient.deleteParameter({name: regionalParameterName}); | ||
| }); |
There was a problem hiding this comment.
The regionalParametersToDelete array contains parameter names with -1 suffixes in some cases, but the loop iterates through the array and attempts to delete parameters without the suffix. This will cause the deletion to fail for those parameters. Ensure that the parameter names in the array match the actual parameter names that were created.
| const request = { | ||
| parameter: { | ||
| name: name, | ||
| }, |
There was a problem hiding this comment.
The request object only contains the name in the parameter field. It might be clearer to directly pass the name to the client.updateParameter function, if the API allows it.
| const request = { | ||
| parameter: { | ||
| name: name, | ||
| kmsKey: kmsKey, | ||
| }, |
There was a problem hiding this comment.
The request object contains both the name and kmsKey in the parameter field. It might be clearer to separate these out if the API allows it, for better readability.
| cryptoKeyId: keyId1, | ||
| cryptoKey: { | ||
| purpose: 'ASYMMETRIC_DECRYPT', | ||
| versionTemplate: { | ||
| algorithm: 'RSA_DECRYPT_OAEP_2048_SHA256', | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This try...catch block only handles the case where the crypto key doesn't exist (error.code === 5). Consider handling other potential errors that might occur during crypto key creation, such as permission issues or invalid parameters. If you don't handle these errors, the test might fail in unexpected ways.
| try { | ||
| await kmsClient.destroyCryptoKeyVersion({ | ||
| name: `${kmsKey}/cryptoKeyVersions/1`, | ||
| }); | ||
| } catch (error) { | ||
| if (error.code === 5) { | ||
| console.info(`Already destroyed: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| await kmsClient.destroyCryptoKeyVersion({ | ||
| name: `${kmsKey1}/cryptoKeyVersions/1`, | ||
| }); | ||
| } catch (error) { | ||
| if (error.code === 5) { | ||
| console.error(`Already destroyed: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| await kmsClient.destroyCryptoKeyVersion({ | ||
| name: `${regionalKmsKey}/cryptoKeyVersions/1`, | ||
| }); | ||
| } catch (error) { | ||
| if (error.code === 5) { | ||
| console.error(`Already destroyed: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| await kmsClient.destroyCryptoKeyVersion({ | ||
| name: `${regionalKmsKey1}/cryptoKeyVersions/1`, | ||
| }); | ||
| } catch (error) { | ||
| if (error.code === 5) { | ||
| console.error(`Already destroyed: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
These try...catch blocks destroy crypto key versions, but they don't destroy the crypto keys themselves. While destroying the version is good, consider also destroying the crypto keys to fully clean up resources after the tests. You can use kmsClient.deleteCryptoKey for this, but be aware of potential permission issues or other errors during deletion.
| const request = { | ||
| parameter: { | ||
| name: name, | ||
| }, |
There was a problem hiding this comment.
The request object only contains the name in the parameter field. It might be clearer to directly pass the name to the client.updateParameter function, if the API allows it.
| parameter: { | ||
| name: name, | ||
| kmsKey: kmsKey, | ||
| }, |
There was a problem hiding this comment.
The request object contains both the name and kmsKey in the parameter field. It might be clearer to separate these out if the API allows it, for better readability.
Description
Created samples for the KMS key field in the global and regional parameter manager.
Added Sample List:
Checklist
npm test(see Testing)npm run lint(see Style)us-central1GoogleCloudPlatform/nodejs-docs-samples. Not a fork.