feat(parametermanager): Added samples for kms_key in regional parameter #5262
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 @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
This pull request introduces samples for using KMS keys with regional parameters in the Parameter Manager SDK. It includes new functions for creating, updating, and removing KMS keys associated with regional parameters. Additionally, it incorporates corresponding tests to ensure the functionality works as expected. The changes also include updates to the go.mod and go.sum files to reflect the new dependencies and versions.
Highlights
- New Samples: Added three new samples demonstrating how to create, update, and remove KMS keys for regional parameters.
- KMS Key Integration: Introduced functionality to manage KMS keys for encrypting regional parameters, enhancing security.
- Testing: Included comprehensive tests to validate the new KMS key functionalities for regional parameters.
Changelog
Click here to see the changelog
- parametermanager/go.mod
- Upgraded the go version to 1.23.0
- Added
cloud.google.com/go/kmsas a new dependency. - Updated
cloud.google.com/go/parametermanagerto v0.2.0. - Updated
google.golang.org/apito v0.228.0. - Updated
google.golang.org/genprototo v0.0.0-20250404141209-ee84b53bf3d0. - Updated
google.golang.org/grpcto v1.71.1.
- parametermanager/go.sum
- Updated checksums for all the modules in go.mod
- parametermanager/regional_samples/create_regional_param_with_kms_key.go
- Added a new function
createRegionalParamWithKmsKeyto create a regional parameter with a KMS key. - The function takes projectID, locationID, parameterID, and kmsKey as arguments.
- It uses the Parameter Manager SDK to create the parameter with the specified KMS key.
- Added a new function
- parametermanager/regional_samples/regional_parametermanager_test.go
- Added tests for creating, updating, and removing KMS keys for regional parameters.
- Includes helper functions for creating test names, retrieving test locations, creating parameters with KMS keys, and cleaning up resources after testing.
- The tests verify the functionality of the new samples by checking the output and ensuring the parameters are created, updated, and removed correctly.
- parametermanager/regional_samples/remove_regional_param_kms_key.go
- Added a new function
removeRegionalParamKmsKeyto remove the KMS key from a regional parameter. - The function takes projectID, locationID, and parameterID as arguments.
- It uses the Parameter Manager SDK to update the parameter and remove the KMS key.
- Added a new function
- parametermanager/regional_samples/update_regional_param_kms_key.go
- Added a new function
updateRegionalParamKmsKeyto update the KMS key for a regional parameter. - The function takes projectID, locationID, parameterID, and kmsKey as arguments.
- It uses the Parameter Manager SDK to update the parameter with the specified KMS key.
- Added a new function
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 regions wide, parameters reside,
With KMS keys, their secrets hide.
Create, update, then erase,
A digital, secured space,
Where data's safety is the guide.
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
The pull request introduces samples for using KMS keys with regional parameters in the Parameter Manager SDK. The changes include adding new sample files and updating the go.mod and go.sum files. Overall, the code seems well-structured and includes necessary tests. However, there are a few areas that could be improved for clarity and efficiency.
Summary of Findings
- Error message consistency: The error messages in the sample functions could be more consistent and informative. Consider adding context to the error messages to help users understand the root cause of the failure.
- KMS Key Version Cleanup: The test cleanup function
testCleanupKeyVersionsattempts to delete key versions, but this operation is not directly supported. Instead, the crypto key itself should be disabled or destroyed. The current implementation might lead to confusion or unexpected behavior. - Key Ring and Key Creation: The test functions
testCreateKeyRingandtestCreateKeyHSMcheck if the key ring and key already exist before creating them. However, the check is performed by attempting to get the resource and handling aNotFounderror. This approach is less efficient than using a dedicated "exists" check if available in the API.
Merge Readiness
The pull request is almost ready for merging. However, the issues related to error message consistency and KMS key version cleanup should be addressed before merging. I am unable to directly approve the pull request, and users should have others review and approve this code before merging. Addressing the high severity issues is recommended before merging.
| // testCleanupKeyVersions deletes the specified key version in the GCP project. | ||
| // It fails the test if the key version deletion fails. | ||
| func testCleanupKeyVersions(t *testing.T, name string) { | ||
| t.Helper() | ||
| ctx := context.Background() | ||
|
|
||
| client, err := kms.NewKeyManagementClient(ctx) | ||
| if err != nil { | ||
| t.Fatalf("failed to create client: %v", err) | ||
| } | ||
| defer client.Close() | ||
|
|
||
| if _, err := client.DestroyCryptoKeyVersion(ctx, &kmspb.DestroyCryptoKeyVersionRequest{ | ||
| Name: name, | ||
| }); err != nil { | ||
| if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound { | ||
| t.Fatalf("testCleanupKeyVersion: failed to delete key version: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The DestroyCryptoKeyVersion method is used to delete key versions, but this operation is not directly supported. Instead, the crypto key itself should be disabled or destroyed. Consider updating this function to disable or destroy the crypto key instead of attempting to delete the key version, as this is the correct way to clean up the KMS resources.
| @@ -1,40 +1,61 @@ | |||
| module github.com/GoogleCloudPlatform/golang-samples/parametermanager | |||
|
|
|||
| go 1.22.7 | |||
| go 1.23.0 | |||
There was a problem hiding this comment.
Consider using go 1.22 instead of go 1.23 to align with the go version used in other golang-samples, unless there is a specific reason to use go 1.23.
| } | ||
| parameter, err := client.CreateParameter(ctx, req) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to create parameter: %w", err) |
There was a problem hiding this comment.
Consider adding more context to the error message to indicate which parameter failed to create. This will help in debugging.
For example, you could include the parameter ID in the error message.
| return fmt.Errorf("Failed to create parameter: %w", err) | |
| return fmt.Errorf("failed to create parameter %s: %w", parameterID, err) |
| // Check if key ring already exists | ||
| req := &kmspb.GetKeyRingRequest{ | ||
| Name: parent + "/keyRings/" + keyRingId, | ||
| } | ||
| _, err = client.GetKeyRing(ctx, req) | ||
| if err != nil { | ||
| if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound { | ||
| t.Fatalf("failed to get key ring: %v", err) | ||
| } | ||
| // Key ring not found, create it | ||
| req := &kmspb.CreateKeyRingRequest{ | ||
| Parent: parent, | ||
| KeyRingId: keyRingId, | ||
| } | ||
| _, err = client.CreateKeyRing(ctx, req) | ||
| if err != nil { | ||
| t.Fatalf("failed to create key ring: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test functions testCreateKeyRing and testCreateKeyHSM check if the key ring and key already exist before creating them. However, the check is performed by attempting to get the resource and handling a NotFound error. This approach is less efficient than using a dedicated "exists" check if available in the API. Consider using a more efficient method to check for the existence of the resources before attempting to create them.
| } | ||
| parameter, err := client.UpdateParameter(ctx, req) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to update parameter: %w", err) |
There was a problem hiding this comment.
Consider adding more context to the error message to indicate which parameter failed to update. This will help in debugging.
For example, you could include the parameter ID in the error message.
| return fmt.Errorf("Failed to update parameter: %w", err) | |
| return fmt.Errorf("failed to update parameter %s: %w", parameterID, err) |
| } | ||
| parameter, err := client.UpdateParameter(ctx, req) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to update parameter: %w", err) |
There was a problem hiding this comment.
Consider adding more context to the error message to indicate which parameter failed to update. This will help in debugging.
For example, you could include the parameter ID in the error message.
| return fmt.Errorf("Failed to update parameter: %w", err) | |
| return fmt.Errorf("failed to update parameter %s: %w", parameterID, err) |
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
arpangoswami
added
kokoro:force-run
kokoro-team
removed
kokoro:run
|
Please resolve the merge conflicts |
@arpangoswami Resolved the merge conflict |
|
Please, apply the same changes that were requested in the past PRs:
|
OremGLG
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
I've applied all the requested changes, including reusing the string variable and removing the newline from the got assignment. |
|
Please, fix your PR conflicts in order to re-run the checks. |
@OremGLG Resolved the merge conflict. |
|
Still with merge conflicts 😬 |
One of the PR got merged, which caused the conflict — I’ve resolved it. |
Head branch was pushed to by a user without write access
Description
Added samples for kms_key field in regional parameter using Parameter manager SDK
Sample List (regional):
Added required Tests for the same.
Checklist
go test -v ./..(see Testing)gofmt(see Formatting)go vet(see Formatting)GOLANG_REGIONAL_SAMPLES_LOCATIONvariable to be set