Google secret manager store implementation #1034
Conversation
📝 WalkthroughWalkthroughThe pull request adds support for Google Secret Manager as a new store. It updates dependency management in Changes
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
pkg/store/google_secret_manager_store.go (4)
23-36: Consider environment-based credential management
Storing credentials as a JSON string in configuration risks accidental exposure. Adopting environment variables or secret references can enhance security.
38-76: Consider allowing caller-supplied context
Currently, a background context is used to initialize the store. Passing a custom context would allow more flexibility for cancellation and timeouts.
99-190: Refactor creation & versioning flow
- Consider splitting secret creation and version addition into separate methods for clarity and improved testability.
- Restricting values to strings might be limiting if you need to store JSON or other data.
192-237: Support specifying secret versions
Always retrieving "latest" can be restrictive. Consider allowing a version parameter for more advanced workflows.pkg/store/google_secret_manager_store_test.go (3)
51-55: Remove or utilize the unused helper
Static analysis flags “newMockGSMClient” as unused. Consider removing or integrating it in tests.🧰 Tools
🪛 golangci-lint (1.62.2)
52-52: func
newMockGSMClientis unused(unused)
56-272: Comprehensive sub-tests
“TestGSMStore_Set” covers multiple paths effectively. If concurrency is a concern, consider adding parallel or race tests.
376-429: Handling environment credentials
Providing credentials from the environment is pragmatic. You might mock environment variables for consistent CI runs.website/docs/core-concepts/stacks/yaml-functions/store.mdx (1)
68-74: Effective Google Secret Manager Example Addition
This set of changes demonstrates the usage of Google Secret Manager for various scenarios—including direct retrieval, default values, and cross-stack access. The examples are concise and correctly use templating for dynamic stack names. One minor suggestion would be to include a brief inline note (or reference) on when to prefer using the alias “gsm” compared to “google-secret-manager” for added clarity.website/docs/core-concepts/projects/configuration/stores.mdx (1)
204-248: Detailed GSM Options and Authentication Instructions
The expanded documentation following the configuration sample thoroughly explains the options for GSM—covering the required project_id, optional prefix and credentials, as well as the stack_delimiter. The authentication section is particularly useful, outlining Application Default Credentials, Direct Credentials, and Workload Identity methods. This level of detail greatly aids users. A small suggestion: consider emphasizing best practices for secure handling of credentials (especially when embedding JSON strings), which can further educate users on avoiding potential pitfalls.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(3 hunks)pkg/store/google_secret_manager_store.go(1 hunks)pkg/store/google_secret_manager_store_test.go(1 hunks)pkg/store/registry.go(1 hunks)website/docs/core-concepts/projects/configuration/stores.mdx(2 hunks)website/docs/core-concepts/stacks/yaml-functions/store.mdx(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/store/google_secret_manager_store_test.go
52-52: func newMockGSMClient is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (13)
pkg/store/google_secret_manager_store.go (3)
1-13: Imports are well-organized
Everything looks good here.
15-21: Interface definition is clean
No immediate concerns with concurrency usage or naming in the GSMClient interface.
78-97: Key transformation collisions
Replacing slashes with underscores may cause collisions (e.g., multiple slash segments). Confirm that potential collisions are acceptable or consider a unique delimiter.pkg/store/registry.go (1)
37-48: Registration logic is consistent
Storing the parsed GSM options in the registry matches patterns used for other store types.pkg/store/google_secret_manager_store_test.go (3)
1-15: Imports and initial setup
All included packages appear necessary and relevant.
17-49: Mock implementation looks solid
Neatly uses the testify/mock framework for mocking GSM client methods.
274-374: Sufficient coverage for Get method
Tests multiple outcome scenarios, including error handling. Implementation looks thorough.website/docs/core-concepts/stacks/yaml-functions/store.mdx (1)
63-67: Clear AWS SSM Example in YAML Block
The newly added lines neatly illustrate how to retrieve security group IDs via AWS SSM Parameter Store. The indentation and labeling are clear, adding value to the documentation.website/docs/core-concepts/projects/configuration/stores.mdx (2)
15-15: Support for Google Secret Manager Added to Stores List
Including Google Secret Manager in the list of supported stores is a welcome update. The link provided directs users to the official documentation, ensuring they can obtain more details if needed.
184-203: Comprehensive GSM YAML Configuration Sample
The YAML configuration block for the Google Secret Manager is clear and well-structured. It illustrates both the direct type (“google-secret-manager”) and its alias (“gsm”) for production configurations, along with key options such as project_id, prefix, and credentials. This practical example will help users correctly integrate GSM with their atmos.yaml configurations.go.mod (3)
6-6: New Dependency for GSM Functionality
The addition of the cloud.google.com/go/secretmanager v1.11.5 dependency is well justified by the new GSM store implementation. It’s important to ensure that this version is compatible with other Google Cloud dependencies in the project.
31-31: Direct Requirement for GAX Library
Noting the appearance of github.com/googleapis/gax-go/v2 v2.12.3 as a direct dependency now—this update signals a cleaner dependency graph. Please verify that this change doesn’t introduce any conflicts with transitive dependencies elsewhere in the codebase.
63-63: Explicit Inclusion of google.golang.org/api
The explicit requirement of google.golang.org/api v0.171.0 ensures stability for GSM interactions. Like the GAX update, confirm that making this dependency direct meets your project’s overall dependency management strategy.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/store/google_secret_manager_store.go (1)
99-190: Add context timeout for GSM operations.Using context.Background() without a timeout could lead to hanging operations. Consider adding a reasonable timeout.
Apply this diff:
func (s *GSMStore) Set(stack string, component string, key string, value interface{}) error { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() - ctx := context.Background()pkg/store/google_secret_manager_store_test.go (1)
376-429: Add test case for client cleanup.Consider adding a test case to verify that the client is properly closed when an error occurs during initialization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/google_secret_manager_store.go(1 hunks)pkg/store/google_secret_manager_store_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/store/google_secret_manager_store_test.go
52-52: func newMockGSMClient is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/store/google_secret_manager_store.go (2)
15-36: LGTM! Well-structured interface and struct declarations.The GSMClient interface is well-designed for mocking, and the GSMStore struct with its options provides a clean configuration structure.
192-237: Add context timeout for GSM operations.Similar to the Set method, add a timeout to prevent hanging operations.
pkg/store/google_secret_manager_store_test.go (2)
56-272: LGTM! Comprehensive test coverage.The test cases thoroughly cover success scenarios, error handling, and input validation.
274-374: LGTM! Well-structured test cases.The test cases provide good coverage of both success and error scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/store/google_secret_manager_store.go (1)
108-200: Potential concurrency considerations
If multiple calls toSet()operate on the same key concurrently, consider version sprawl and race conditions. Also, storing large secrets might require additional checks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(3 hunks)pkg/store/google_secret_manager_store.go(1 hunks)pkg/store/google_secret_manager_store_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/store/google_secret_manager_store_test.go
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/store/google_secret_manager_store.go (7)
1-19: Great package structure
The imports and thegsmOperationTimeoutconstant form a clean foundation. The 30-second timeout is a sensible default.
20-26: Neat interface
TheGSMClientinterface neatly captures the required Secret Manager methods.
28-34: Straightforward store struct
All essential fields (client, project ID, prefix, delimiter) are logically grouped.
36-41: Flexible options
Allowing optional credentials, prefix, and delimiter is convenient for diverse configurations.
46-85: Robust constructor
Resource cleanup on error is carefully handled, preventing leaks. Everything else looks solid.
87-106: Comprehensive key transformation
Stripping slashes and extraneous underscores ensures secret IDs remain valid in Google Secret Manager.
202-248: String-based retrieval
Returning string data is fine but limits storing binary or structured data. This is acceptable for most use cases.
There was a problem hiding this comment.
Thanks for this contribution, @shirkevich! Overall, it looks really good, just left a few small comments.
|
💥 This pull request now has conflicts. Could you fix it @shirkevich? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/docs/core-concepts/projects/configuration/stores.mdx(2 hunks)website/docs/core-concepts/stacks/yaml-functions/store.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/docs/core-concepts/stacks/yaml-functions/store.mdx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
website/docs/core-concepts/projects/configuration/stores.mdx (3)
18-21: LGTM! The list is already alphabetized.The list of supported stores is properly organized in alphabetical order, which aligns with the documentation's structure and improves readability.
135-150: LGTM! Comprehensive examples covering key configurations.The examples effectively demonstrate:
- Both store type identifiers (
google-secret-managerandgsm)- Different authentication methods
- Optional configuration parameters
180-197: LGTM! Comprehensive authentication documentation.The authentication section effectively covers all supported methods with clear setup instructions and security best practices.
mergify
bot
added
triage
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
go.mod (2)
6-6: LGTM! Dependencies for Google Secret Manager integration.The added dependencies are appropriate for implementing the Google Secret Manager store:
- cloud.google.com/go/secretmanager
- github.com/googleapis/gax-go/v2
- google.golang.org/api
Also applies to: 31-31, 64-64
63-63:✅ Verification successful
Verify the downgrade of golang.org/x/text.
The downgrade from v0.22.0 to v0.21.0 might introduce compatibility issues.
🌐 Web query:
What is the latest stable version of golang.org/x/text?💡 Result:
The latest stable version of
golang.org/x/textis v0.21.0, released on December 4, 2024[5][8]. This version includes updates to support Unicode standards aligned with Go compiler versions up to Go 1.23.x[1][4].For compatibility:
x/textsynchronizes its Unicode/CLDR versions with those used by your Go toolchain[1][4]- To update dependencies, use
go get golang.org/x/[email protected][5]The package maintains backward compatibility only within major versions (v0.x), with breaking changes possible between minor versions[4].
Citations:
- 1: https://github.com/golang/text
- 2: https://go.dev/doc/devel/release
- 3: https://tip.golang.org/doc/go1.21
- 4: https://pkg.go.dev/golang.org/x/text
- 5: https://deps.dev/go/golang.org%2Fx%2Ftext/v0.3.3
- 6: https://tip.golang.org/doc/go1.19
- 7: https://github.com/golang/tools/releases
- 8: https://pkg.go.dev/golang.org/x/text/date
- 9: https://go.dev/dl
- 10: https://tip.golang.org/doc/go1.18
Dependency Verified: No Action Required
The code in
go.modis usinggolang.org/x/text v0.21.0, which is confirmed as the latest stable version. There is no compatibility issue from this change.
- File: go.mod (Line 63)
|
@shirkevich looks like there's a failure in the tests: |
Fixed |
|
Still looks like we've got a failing test: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/store/google_secret_manager_store_test.go (3)
10-10: Consider removing AWS SDK dependency.The AWS SDK is only used for the
String()helper function. Consider using a simple string pointer constructor instead:-"github.com/aws/aws-sdk-go-v2/aws"Replace
aws.String()usage with:func StringPtr(s string) *string { return &s }
379-379: Replace aws.String with local StringPtr function.Following the earlier suggestion to remove the AWS SDK dependency:
- stackDelimiter: aws.String(testDelimiter), + stackDelimiter: StringPtr(testDelimiter),
395-448: Consider adding more test cases for NewGSMStore.The current test suite covers basic scenarios. Consider adding test cases for:
- Empty prefix
- Empty stack delimiter (should use default)
- Invalid project ID format
Example test cases:
{ name: "empty prefix", options: GSMStoreOptions{ ProjectID: "test-project", StackDelimiter: StringPtr("-"), }, expectError: false, }, { name: "empty stack delimiter uses default", options: GSMStoreOptions{ ProjectID: "test-project", Prefix: StringPtr("test-prefix"), }, expectError: false, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/store/google_secret_manager_store_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/store/google_secret_manager_store_test.go (1)
41-73: Well-structured mock implementation!The mock client implementation is thorough and handles all edge cases properly, including nil checks and error propagation.
| return nil, fmt.Errorf("failed to access secret version: %w", err) | ||
| } | ||
|
|
||
| log.Debug("successfully accessed secret", |
There was a problem hiding this comment.
| log.Debug("successfully accessed secret", | |
| log.Debug("successfully retrieved secret", |
| Name: name, | ||
| }) | ||
| if err != nil { | ||
| log.Debug("failed to access secret", |
There was a problem hiding this comment.
| log.Debug("failed to access secret", | |
| log.Debug("failed to retrieve secret", |
| // Build the resource name for the latest version | ||
| name := fmt.Sprintf("projects/%s/secrets/%s/versions/latest", s.projectID, secretID) | ||
|
|
||
| log.Debug("accessing Google Secret Manager secret", |
There was a problem hiding this comment.
| log.Debug("accessing Google Secret Manager secret", | |
| log.Debug("retrieving Google Secret Manager secret", |
| return args.Get(0).(*secretmanagerpb.SecretVersion), args.Error(1) | ||
| } | ||
|
|
||
| func (m *MockGSMClient) AccessSecretVersion(ctx context.Context, req *secretmanagerpb.AccessSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.AccessSecretVersionResponse, error) { |
There was a problem hiding this comment.
Under the hood, we're calling Get, so let's keep the verbs consistent.
| func (m *MockGSMClient) AccessSecretVersion(ctx context.Context, req *secretmanagerpb.AccessSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.AccessSecretVersionResponse, error) { | |
| func (m *MockGSMClient) GetSecretVersion(ctx context.Context, req *secretmanagerpb.AccessSecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.AccessSecretVersionResponse, error) { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
63-63: Version Adjustment for golang.org/x/text:
Downgrading togolang.org/x/text v0.21.0is noted. Please verify that this change does not trigger any compatibility issues in parts of Atmos that rely on text processing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
go.mod (1)
Learnt from: mcalhoun
PR: cloudposse/atmos#1034
File: go.mod:3-3
Timestamp: 2025-02-14T16:16:36.328Z
Learning: Go 1.23.5 was released on January 16, 2025, and is a valid Go version. Always verify Go version information with official sources before suggesting version changes.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
go.mod (4)
3-3: Go Version Check:
The specified Go version1.23.5is valid and aligns with the latest releases as confirmed by recent documentation.
6-6: Dependency Addition for GSM:
Addingcloud.google.com/go/secretmanager v1.11.5properly supports the integration of Google Secret Manager as outlined in the PR objectives.
31-31: New Dependency for GAX-Go:
The inclusion ofgithub.com/googleapis/gax-go/v2 v2.12.3is appropriate for enhanced handling of Google API interactions within the new store implementation.
64-64: New Dependency for Google API Client:
The addition ofgoogle.golang.org/api v0.171.0is well-aligned with the GSM integration requirements, ensuring the necessary support for interacting with Google APIs.
what
google-secret-managerand its aliasgsmwhy
references
Code changes:
pkg/store/google_secret_manager_store.go: Main GSM store implementationpkg/store/google_secret_manager_store_test.go: Test coveragepkg/store/registry.go: Added GSM store type registrationwebsite/docs/core-concepts/projects/configuration/stores.mdx: Added GSM documentationRelated GCP documentation:
Summary by CodeRabbit
New Features
Documentation
stacks_delimitertostack_delimiter.