[Stores] Add user-managed replication support to GSM #1260
Conversation
📝 WalkthroughWalkthroughReplication configuration for Google Secret Manager secrets is now supported. The store and its options were updated to allow specifying replication locations, with logic to handle both automatic and user-managed replication. Corresponding tests and mocks were updated to verify replication behavior and add coverage for these new options. Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
pkg/store/google_secret_manager_store_test.go (1)
505-505: Consider removing unused locations field.The
locationsfield was added to the test struct but isn't used in the test cases.Since this field isn't being used in the test cases, consider removing it to avoid confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/google_secret_manager_store.go(3 hunks)pkg/store/google_secret_manager_store_test.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/store/google_secret_manager_store.go (5)
37-37: Good addition of replication field to GSMStore struct.The
replicationfield with type*secretmanagerpb.Replicationenables storage of replication configuration in the GSM store instance.
45-46: Well-structured Locations field addition to options.Good implementation of the optional
Locationsfield as a pointer slice of strings, making it optional and providing a clear purpose through the comment.
90-91: Appropriate initialization of replication field.The store's replication field is properly initialized by calling the new helper function with the locations from options.
95-118: Helper function logic looks robust.The
createReplicationFromLocationsfunction correctly handles both automatic replication (default case) and user-managed replication when locations are provided.
147-147: Good use of store's replication field in createSecret.The function now uses the store's replication configuration instead of hard-coding automatic replication.
pkg/store/google_secret_manager_store_test.go (4)
77-77: Proper initialization of replication in test helper.The test helper correctly initializes the store's replication field using the same helper function as the main code.
82-90: Good default handling in mock function.The mock function correctly handles the case when replication is nil by defaulting to automatic replication.
211-219: Good test case for automatic replication.This test case verifies that automatic replication works correctly.
564-564: Good initialization of replication for getKey tests.Properly initializes the replication field for consistency with the updated GSMStore structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/store/google_secret_manager_store_test.go (2)
7-7: Fix the import formatting.The import statement is not properly formatted according to gofumpt standards. It should be properly positioned within the import block.
- "github.com/google/go-cmp/cmp" + + "github.com/google/go-cmp/cmp"🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 7-7: File is not properly formatted
(gofumpt)
101-114: Simplify the replication comparison logic.The current implementation compares locations by extracting them into slices and using
cmp.Diff. Consider using a more direct comparison to make the code more readable and less error-prone.- var expectedLocations []string - var receivedLocations []string - for _, replica := range replication.GetUserManaged().Replicas { - expectedLocations = append(expectedLocations, replica.Location) - } - for _, replica := range req.Secret.GetReplication().GetUserManaged().Replicas { - receivedLocations = append(receivedLocations, replica.Location) - } - replicationMatched = cmp.Diff(expectedLocations, receivedLocations) == "" + expectedReplicas := replication.GetUserManaged().Replicas + receivedReplicas := req.Secret.GetReplication().GetUserManaged().Replicas + + // Check if both slices have the same length + if len(expectedReplicas) != len(receivedReplicas) { + return false + } + + // Create maps of locations for efficient comparison (handles different ordering) + expectedLocMap := make(map[string]bool) + for _, r := range expectedReplicas { + expectedLocMap[r.Location] = true + } + + // Verify all received locations are in the expected set + for _, r := range receivedReplicas { + if !expectedLocMap[r.Location] { + return false + } + } + + replicationMatched = trueThis approach handles different ordering of locations and avoids unnecessary string conversions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/store/google_secret_manager_store_test.go(9 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
pkg/store/google_secret_manager_store_test.go
[error] 7-7: File is not properly formatted
(gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/store/google_secret_manager_store_test.go (4)
78-78: LGTM: Good addition of replication configuration.The store initialization now properly sets the replication configuration based on the provided locations.
83-91: LGTM: Well-implemented default replication behavior.This modification correctly handles both user-managed and automatic replication by defaulting to automatic replication when no specific locations are provided. This maintains backward compatibility.
227-254: LGTM: Comprehensive test cases for replication configurations.The test cases properly cover both automatic replication (empty locations) and user-managed replication (specific locations), ensuring that the new functionality works correctly.
522-522: LGTM: Consistent updates to other tests.You've properly updated the test struct to include the locations field and initialized the replication in the
GSMStoreobject, maintaining consistency with your changes.Also applies to: 581-581
aknysh
added
the
no-release
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/store/google_secret_manager_store_test.go (3)
102-115: Consider improving the location comparison logic.The current implementation might have issues if the order of locations differs between expected and received. Consider sorting both arrays before comparison to ensure order-independent matching.
var expectedLocations []string var receivedLocations []string for _, replica := range replication.GetUserManaged().Replicas { expectedLocations = append(expectedLocations, replica.Location) } for _, replica := range req.Secret.GetReplication().GetUserManaged().Replicas { receivedLocations = append(receivedLocations, replica.Location) } +sort.Strings(expectedLocations) +sort.Strings(receivedLocations) replicationMatched = cmp.Diff(expectedLocations, receivedLocations) == ""Don't forget to add the import:
+import "sort"
523-523: Remove unused field from TestGSMStore_GetKey.The
locationsfield is added to the test struct but isn't used in any of the test cases, making it unnecessary.struct { name string prefix string stackDelimiter *string stack string component string key string expected string - locations []string wantErr bool }
84-84: Consider parameterizing the parent project ID.The static analysis tool identified that
parentalways receives"projects/test-project". Consider parameterizing this value to make the mock function more flexible for different test scenarios.-func gsmClientSecretCreationMock(parent string, secretId string, secretPayload string, replication *secretmanagerpb.Replication, err error) func(m *MockGSMClient) { +func gsmClientSecretCreationMock(projectID string, secretId string, secretPayload string, replication *secretmanagerpb.Replication, err error) func(m *MockGSMClient) { + parent := fmt.Sprintf("projects/%s", projectID) return func(m *MockGSMClient) {Then update all calls to this function to pass "test-project" as the first parameter.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 84-84:
gsmClientSecretCreationMock-parentalways receives"projects/test-project"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go.mod(1 hunks)pkg/store/google_secret_manager_store_test.go(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🧰 Additional context used
🪛 GitHub Check: golangci-lint
pkg/store/google_secret_manager_store_test.go
[failure] 84-84:
gsmClientSecretCreationMock - parent always receives "projects/test-project"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/store/google_secret_manager_store_test.go (8)
10-10: New dependency adds support for advanced comparisons.The
github.com/google/go-cmp/cmppackage is now used for comparing location slices, providing more robust equality checking than simple string comparison.
79-79: Good initialization of the replication configuration.The store's replication field is now properly initialized from the provided locations, supporting the new user-managed replication feature.
84-93: Default replication configuration properly set.The mock function now provides a default automatic replication when none is specified, matching Google Secret Manager's behavior.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 84-84:
gsmClientSecretCreationMock-parentalways receives"projects/test-project"
146-153: Great addition of the locations field to test cases.The test struct now includes a
locationsfield to test various replication configurations, which aligns with the PR objective of adding user-managed replication support.
229-236: Good test case for automatic replication.This test case properly verifies the default behavior with automatic replication when no locations are specified.
238-255: Comprehensive test for user-managed replication.The test case for user-managed replication includes appropriate locations and verifies the correct configuration is applied.
296-296: Proper propagation of locations to store initialization.The test now correctly passes the locations from the test case to the store options, ensuring the replication configuration is properly set.
582-582: Ensure replication is consistently initialized.The test initializes replication with nil locations, providing consistent behavior with automatic replication.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1260 +/- ##
==========================================
+ Coverage 48.17% 48.60% +0.42%
==========================================
Files 233 233
Lines 25461 25480 +19
==========================================
+ Hits 12267 12385 +118
+ Misses 11608 11491 -117
- Partials 1586 1604 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
💥 This pull request now has conflicts. Could you fix it @ohaibbq? 🙏 |
ohaibbq
force-pushed
the
dan/gsm-replication
branch
from
5ac1f06 to
a40b943
Compare
what
Adds support for user-managed replication locations to the Google Secret Manager store
why
Cloud projects often have policies that constrain where resources are physically located
Summary by CodeRabbit
New Features
Tests