feat(parametermanager): Added regional samples for kms_key field #5241
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 introduces regional samples for the kms_key field within the Parameter Manager SDK. It includes the implementation of three new samples: create_regional_param_with_kms_key, update_regional_param_kms_key, and remove_regional_param_kms_key. The changes also include the addition of corresponding tests to validate the functionality of these new samples.
Highlights
- New Samples: Three new regional samples have been added for managing
kms_keyin Parameter Manager: creating, updating, and removing. - Testing: The pull request includes tests to ensure the correct behavior of the new regional samples.
- Dependencies: The
go.modandgo.sumfiles have been updated to include the necessary dependencies for the Parameter Manager SDK and KMS.
Changelog
Click here to see the changelog
- go.work
- Added the
parametermanagerdirectory to thego.workfile to include it in the workspace.
- Added the
- parametermanager/go.mod
- Created
go.modfile for theparametermanagermodule, specifying dependencies such ascloud.google.com/go/kms,cloud.google.com/go/parametermanager, and others. - Specifies the go version and toolchain version.
- Created
- parametermanager/go.sum
- Created
go.sumfile for theparametermanagermodule, containing the cryptographic hashes of the module's dependencies to ensure reproducible builds.
- Created
- parametermanager/regional_samples/create_regional_param_with_kms_key.go
- Created a new sample for creating a regional parameter with a KMS key.
- The sample uses the Parameter Manager SDK to create a parameter with the specified KMS key for encryption.
- Includes error handling and output to the provided io.Writer.
- parametermanager/regional_samples/regional_parametermanager_test.go
- Created a new test file for the regional Parameter Manager samples.
- Includes tests for creating, updating, and removing KMS keys from regional parameters.
- Utilizes KMS API to create keyrings and keys for testing purposes.
- Includes helper functions for creating test names, retrieving the test location, creating parameters with KMS keys, and cleaning up resources after testing.
- parametermanager/regional_samples/remove_regional_param_kms_key.go
- Created a new sample for removing a KMS key from a regional parameter.
- The sample uses the Parameter Manager SDK to update a parameter and remove the KMS key.
- Includes error handling and output to the provided io.Writer.
- parametermanager/regional_samples/update_regional_param_kms_key.go
- Created a new sample for updating a KMS key for a regional parameter.
- The sample uses the Parameter Manager SDK to update a parameter with a new KMS key.
- Includes error handling and output to the provided io.Writer.
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, secure space,
Where data's safety is applied.
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 regional samples for the kms_key field using the Parameter Manager SDK. It includes samples for creating, updating, and removing the kms_key for regional parameters, along with corresponding tests. Overall, the code seems well-structured and addresses the intended functionality.
Summary of Findings
- Missing Error Handling: The
testCleanupKeyVersionsfunction inregional_parametermanager_test.godoes not handle the error returned byclient.DestroyCryptoKeyVersionproperly. Specifically, it checks forNotFoundbut doesn't log or handle other potential errors, which could lead to silent failures during testing. - Inconsistent Error Message: In
remove_regional_param_kms_key.go, the error message in theremoveRegionalParamKmsKeyfunction states "Failed to update parameter" even though the function is intended to remove the KMS key. This can be misleading. - Potential Resource Leak: In
testCreateKeyRingandtestCreateKeyHSMinregional_parametermanager_test.go, the code checks if the key ring or crypto key already exists, but if an error occurs during theGetKeyRingorGetCryptoKeycalls (other thanNotFound), the function immediately terminates without closing the KMS client. This could lead to resource leaks if these functions are called repeatedly in a short period.
Merge Readiness
The pull request introduces important functionality for managing regional parameters with KMS keys. However, there are a few issues that need to be addressed before merging. Specifically, the error handling in testCleanupKeyVersions should be improved, the error message in remove_regional_param_kms_key.go should be corrected, and the potential resource leak in testCreateKeyRing and testCreateKeyHSM should be fixed. I am unable to approve this pull request, and recommend that these issues be addressed before merging. Other reviewers should also review and approve this code before merging.
| } | ||
| 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.
The error message "Failed to update parameter" is misleading since the function is removing the KMS key. It would be more accurate to say "Failed to remove KMS key for parameter".
| return fmt.Errorf("Failed to update parameter: %w", err) | |
| return fmt.Errorf("Failed to remove KMS key for parameter: %w", err) |
| 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 code only checks for grpcstatus.FromError(err).Code() != grpccodes.NotFound. It should also handle other potential errors during the DestroyCryptoKeyVersion call. Consider logging the error or returning it to prevent silent failures.
if err != nil {
terr, ok := grpcstatus.FromError(err)
if !ok || terr.Code() != grpccodes.NotFound {
t.Fatalf("testCleanupKeyVersion: failed to delete key version: %v", err)
}
t.Logf("testCleanupKeyVersion: error destroying key version: %v", err) // Log the error
}| // parameterID: The ID of the parameter to be updated. | ||
| // | ||
| // The function returns an error if the parameter creation fails. | ||
| func removeRegionalParamKmsKey(w io.Writer, projectID, locationID, parameterID string) error { |
There was a problem hiding this comment.
The function description mentions 'parameter creation fails', but this function is for removing the KMS key, not creating a parameter. Consider updating the description to accurately reflect the function's purpose.
| _, 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) | ||
| } |
There was a problem hiding this comment.
If client.GetKeyRing returns an error other than NotFound, the function terminates without closing the KMS client. This could lead to resource leaks. Consider closing the client before returning in case of an error.
_, err = client.GetKeyRing(ctx, req)
if err != nil {
if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound {
client.Close() // Close the client before returning
t.Fatalf("failed to get key ring: %v", err)
}
}| _, err = client.GetCryptoKey(ctx, req) | ||
| if err != nil { | ||
| if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound { | ||
| t.Fatalf("failed to get crypto key: %v", err) | ||
| } |
There was a problem hiding this comment.
Similar to testCreateKeyRing, if client.GetCryptoKey returns an error other than NotFound, the function terminates without closing the KMS client. Consider closing the client before returning in case of an error.
_, err = client.GetCryptoKey(ctx, req)
if err != nil {
if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound {
client.Close() // Close the client before returning
t.Fatalf("failed to get crypto key: %v", err)
}
}
|
Raised a duplicate of this PR #5262 because Vatsal's CLA has expired, which is blocking the merge. Once the new PR is merged, will close the original one. |
Description
Created regional samples for kms_key field using Parameter Manager SDK
Sample List:
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