fix(gcpsm): Improve SecretExists method in GCP secret manager provider#5610
Conversation
f1a86e4 to
d8d189e
Compare
58f3a63 to
17ed6fb
Compare
|
/ok-to-test sha=17ed6fbb41c17747b64da40b8065d0aba565fc56 |
|
/ok-to-test-managed sha=17ed6fbb41c17747b64da40b8065d0aba565fc56 provider=gcp |
1448d1e to
a4e20c9
Compare
|
/ok-to-test sha=17ed6fbb41c17747b64da40b8065d0aba565fc56 |
f7bdfd2 to
ad4acc7
Compare
| // This is simplified to only support what our tests actually need: | ||
| // - Return 0 or 1 versions | ||
| // - Immediately return iterator.Done when exhausted | ||
| func NewSecretVersionIterator(versions []*secretmanagerpb.SecretVersion) *secretmanager.SecretVersionIterator { |
There was a problem hiding this comment.
disclaimer: I used Claude to help me generate this SecretVersionIterator mock
|
@gusfcarvalho could you fire another e2e? I ended up making a few extra changes. I was trying to avoid the |
|
/ok-to-test sha=ad4acc7463f54be2a09a92cb79396617c01aa531 |
|
/ok-to-test-managed sha=ad4acc7463f54be2a09a92cb79396617c01aa531 |
|
@tosih I'm afraid you aren't allowed to fire the e2e test. :) Only maintainers ccan. Also, please take care of the sonar issues before proceeding. :) Thanks! |
|
/ok-to-test sha=ad4acc7463f54be2a09a92cb79396617c01aa531 |
ad4acc7 to
3d40256
Compare
WalkthroughSecretExists now obtains the latest enabled secret version via ListSecretVersions and AccessSecretVersion (using new helpers getName/getParentName/getLatestEnabledVersion) instead of GetSecret. Tests were made location-aware and use per-test contexts. Fake client gained a mock SecretVersionIterator generator for tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Provider Client
participant SM as Secret Manager API
participant Metrics as metrics
Client->>SM: ListSecretVersions(parent or secret name)
SM-->>Client: SecretVersion iterator
Client->>Client: iterate -> pick newest ENABLED by CreateTime
alt enabled version found
Client->>Metrics: ObserveAPICall(start)
Client->>SM: AccessSecretVersion(version name)
SM-->>Client: Secret payload / error
Client->>Metrics: ObserveAPICall(end)
else none enabled
Client->>Metrics: ObserveAPICall(start)
Client->>SM: AccessSecretVersion(.../versions/latest)
SM-->>Client: Secret payload / error
Client->>Metrics: ObserveAPICall(end)
end
Client-->>Caller: return exists? (true/false) or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/v1/gcp/secretmanager/client_test.go (1)
1189-1264: Test may not be validating the newSecretExistsbehavior.
TestSecretExistsWithRegionalEndpointmocksGetSecretFnandAccessSecretVersionFn, but the refactoredSecretExistsmethod now usesgetLatestEnabledVersion, which callsListSecretVersionsbeforeAccessSecretVersion.Since
ListSecretVersionsFnis not mocked in these test cases, the test may either:
- Panic with a nil function call, or
- Pass accidentally if there's a default behavior
Consider updating this test to use the same
setupMockpattern asTestSecretExiststo properly mockListSecretVersionsFn.func TestSecretExistsWithRegionalEndpoint(t *testing.T) { tests := []struct { name string location string ref esv1.PushSecretRemoteRef - getSecretMockReturn fakesm.SecretMockReturn - accessSecretVersionMockReturn fakesm.AccessSecretVersionMockReturn + setupMock func(*fakesm.MockSMClient) expectedSecret bool expectedErr func(t *testing.T, err error) }{
🧹 Nitpick comments (1)
providers/v1/gcp/secretmanager/client.go (1)
710-730: Consider edge case when multiple versions have the same CreateTime.The loop selects the version with the latest
CreateTimeusingAfter(). If two versions have identical timestamps (unlikely but possible), the selection depends on iteration order, which may not be deterministic.This is a minor edge case, but worth noting. If deterministic behavior is needed, consider a secondary sort key (e.g., version name/number).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
providers/v1/gcp/secretmanager/client.go(3 hunks)providers/v1/gcp/secretmanager/client_test.go(8 hunks)providers/v1/gcp/secretmanager/fake/fake.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
providers/v1/gcp/secretmanager/client_test.go (1)
providers/v1/gcp/secretmanager/fake/fake.go (5)
AddSecretVersionMockReturn(60-63)MockSMClient(34-47)ListSecretVersionsMockReturn(65-67)NewSecretVersionIterator(73-139)AccessSecretVersionMockReturn(55-58)
🔇 Additional comments (7)
providers/v1/gcp/secretmanager/fake/fake.go (3)
69-139: Acknowledge the reflection-based mock approach.This is a sophisticated mock implementation that uses reflection and
unsafeto populate unexported fields ofsecretmanager.SecretVersionIterator. The approach is well-documented and appropriately scoped for test-only usage.A few observations:
The
setFieldhelper silently ignores invalid field names (when!field.IsValid()). Consider logging or panicking in tests if a field name is incorrect, as this could mask test setup bugs.The
donevariable is captured in theInternalFetchclosure, meaning the iterator cannot be reused after exhaustion. This is fine for the current test patterns but worth noting.
65-67: LGTM - consistent mock pattern.The
ListSecretVersionsMockReturnstruct correctly only containsRessinceListSecretVersionsreturns an iterator directly (errors are surfaced throughiter.Next()), unlike other methods that return(result, error).
292-296: LGTM - standard mock setter.The
NewListSecretVersionsFnfollows the established pattern used by other mock setters in this file.providers/v1/gcp/secretmanager/client_test.go (2)
1145-1164: Test case behavior may not match the new implementation.This test case "secret version does not exist - latest version is destroyed/disabled" sets up:
- Empty iterator (no enabled versions)
- AccessSecretVersion returning
FailedPreconditionwith "DESTROYED state"The test expects
expectedExists: falseand an error containing "DESTROYED state".However, looking at the implementation in
client.go,getLatestEnabledVersionwill:
- Find no enabled versions (empty iterator)
- Fall through with
versionName = "latest"- Call
AccessSecretVersionwith/versions/latest- Return the FailedPrecondition error
Since
SecretExistsonly treatscodes.NotFoundas "secret doesn't exist", this error will propagate as an error (not "secret exists = false"). The test expectation seems correct, but consider whether this behavior is intended—a secret with all versions destroyed/disabled returns an error rather than(false, nil).Is this the intended behavior? If a secret exists but has no enabled versions (all destroyed/disabled), should
SecretExistsreturn:
(false, nil)- treating it as "doesn't exist for practical purposes", or(false, err)- indicating the secret exists but is in an unusable state?The current implementation returns
(false, err), which may be confusing for callers.
1016-1021: LGTM - improved test structure.The refactored
TestSecretExistsuses thesetupMockpattern which provides better flexibility and clarity. The test cases cover important scenarios including regional secrets, not-found errors, and permission denied errors.providers/v1/gcp/secretmanager/client.go (2)
149-160: LGTM - cleaner existence check using enabled versions.The refactored
SecretExistsnow correctly delegates togetLatestEnabledVersion, which checks for accessible (enabled) secret versions rather than just secret existence. This aligns with the PR objective of fixing the issue where the latest version could be disabled.The
NotFounderror handling correctly returns(false, nil)indicating the secret doesn't exist from a practical standpoint.
192-196: LGTM - minor style change.Changing
var replication =toreplication :=is a stylistic improvement with identical semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
providers/v1/gcp/secretmanager/fake/fake.go (1)
69-140: Consider adding defensive checks for field reflection failures.The reflection-based approach is reasonable for test mocking but has maintenance implications:
Silent failures: The
setFieldhelper (lines 77-85) silently does nothing if a field is not found. If the underlying library changes field names, tests may pass with invalid mocks, hiding failures.Brittleness: The mock assumes specific unexported field names (
pageInfo,nextFunc,items) insecretmanager.SecretVersionIterator. Library updates could break this without warning.Simplified scope: The implementation only supports 0-1 versions (documented on lines 70-72), which may require updates if test coverage expands.
Consider these improvements:
// Simple helper to set an unexported field using reflection setField := func(name string, value any) { v := reflect.ValueOf(it).Elem() field := v.FieldByName(name) - if field.IsValid() { + if !field.IsValid() { + // In tests, failing to set a field likely means the library structure changed + panic(fmt.Sprintf("mock iterator: field %q not found in SecretVersionIterator", name)) + } + if field.CanAddr() { //#nosec G103 -- Audited this use and it's only for mocking in testing reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())). Elem().Set(reflect.ValueOf(value)) + } else { + panic(fmt.Sprintf("mock iterator: field %q is not addressable", name)) } }This will fail tests immediately if the library structure changes, rather than silently producing incorrect behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
providers/v1/gcp/secretmanager/fake/fake.go(5 hunks)
🔇 Additional comments (3)
providers/v1/gcp/secretmanager/fake/fake.go (3)
65-67: LGTM!The
ListSecretVersionsMockReturntype follows the established pattern of other mock return types in this file and provides a clean wrapper for the mock iterator.
293-297: LGTM!The
NewListSecretVersionsFnmethod follows the established pattern of other mock setup methods in this file and correctly wires the mock iterator.
23-24: The reflection-based mock approach relies on unstable internal iterator structure. Verify compatibility and add error handling.The
InternalFetchfield inSecretVersionIteratoris explicitly documented as not part of the stable interface of the package. Using reflection and unsafe pointers to set unexported fields in a struct with internal implementation details creates brittleness. If Google's implementation changes internal field names or structure in future library versions, this mock will silently fail.Consider adding defensive error handling to the
setFieldhelper to detect and report failures, and add comments documenting the specific library version this mock targets to make future compatibility breaks more discoverable.
|
I added a fix that CodeRabbitAI was suggesting and it also matched another PR: #5678 |
9fa5abb to
98987a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
providers/v1/gcp/secretmanager/client_test.go (1)
1028-1047: Consider settingCreateTimein mock versions to fully exercise the comparison logic.The mocked
SecretVersionobjects don't setCreateTime. While this works (protobuf handles nil safely), it means the "latest by creation time" comparison logic ingetLatestEnabledVersionisn't fully tested. Consider addingCreateTimevalues to properly validate the version selection:mc.NewListSecretVersionsFn(fakesm.ListSecretVersionsMockReturn{ Res: fakesm.NewSecretVersionIterator([]*secretmanagerpb.SecretVersion{ { Name: "projects/foo/secrets/bar/versions/1", State: secretmanagerpb.SecretVersion_ENABLED, + CreateTime: timestamppb.Now(), }, }), })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
providers/v1/gcp/secretmanager/client.go(3 hunks)providers/v1/gcp/secretmanager/client_test.go(20 hunks)providers/v1/gcp/secretmanager/fake/fake.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- providers/v1/gcp/secretmanager/fake/fake.go
🧰 Additional context used
🧬 Code graph analysis (1)
providers/v1/gcp/secretmanager/client_test.go (4)
providers/v1/gcp/secretmanager/fake/fake.go (4)
AddSecretVersionMockReturn(60-63)MockSMClient(34-47)ListSecretVersionsMockReturn(65-67)NewSecretVersionIterator(73-140)apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/gcp/secretmanager/client.go (1)
Client(88-97)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)
🔇 Additional comments (8)
providers/v1/gcp/secretmanager/client.go (3)
149-160: LGTM: SecretExists now correctly checks for accessible versions.The refactored implementation properly determines secret existence by checking for an enabled/accessible version rather than just checking if the secret resource exists. This addresses the issue where a secret could exist but have only disabled/destroyed versions.
192-196: Minor style improvement noted.The change from
var replication = ...toreplication := ...is a minor idiomatic improvement.
710-746: Path construction issue resolved correctly.The previous critical issue about double-prefixed paths has been properly addressed. The implementation now:
- Initializes
versionNameas empty string- Sets it to the full path from
version.Namewhen enabled versions are found- Only constructs the fallback path when no enabled versions exist (line 734-736)
- Uses
versionNamedirectly in the requestThis ensures valid resource paths in all scenarios.
providers/v1/gcp/secretmanager/client_test.go (5)
82-82: Context usage is appropriate for test setup.Using
context.TODO()in the test setup helper is acceptable since the actual test methods uset.Context()which provides proper test-scoped context handling.
601-619: Minor style improvements noted.The changes from
var x = ...tox := ...are idiomatic Go style improvements with no functional impact.
1189-1280: Regional endpoint tests provide good coverage.The
TestSecretExistsWithRegionalEndpointtest cases properly exercise the regional secret behavior, including existence checks and NotFound scenarios with the updated mock infrastructure.
1145-1164: Good test case for destroyed/disabled state handling.This test case properly validates the behavior when the latest version is destroyed/disabled - the function correctly returns
falsewith an error containing the state information, ensuring callers can distinguish between "secret doesn't exist" and "secret exists but is inaccessible."
217-217: Correct the Go version reference in t.Context() usage explanation.Using
t.Context()(Go 1.24+) returns a context that's canceled after the test completes, improving test isolation.Likely an incorrect or invalid review comment.
98987a3 to
d464236
Compare
Update SecretExists to call getLatestEnabledVersion instead of directly checking for the latest version. This provides better handling of secret versions in different states. Changes: - Refactor SecretExists to use getLatestEnabledVersion helper function - Add comprehensive tests for SecretExists with various scenarios - Add mock support for ListSecretVersions in fake client - Improve error handling for destroyed/disabled secret versions - Add support for regional secrets in SecretExists tests - Fix linter issues in fake client Signed-off-by: Sohail Ahmed <sohail@example.com> Signed-off-by: tosih <somuchhail@gmail.com>
The version.Name field from GCP's SecretVersion returns the full resource name (e.g., projects/*/secrets/*/versions/*), not just the version number. Previously, the code was incorrectly appending this full path to another path construction, resulting in malformed paths like: projects/.../secrets/.../versions/projects/.../secrets/.../versions/1 This fix changes getLatestEnabledVersion to: - Use version.Name directly when an enabled version is found - Only construct the path when falling back to "latest" This resolves issues when SecretVersionSelectionPolicy is set to LatestOrFetch and the latest version is destroyed or disabled. Signed-off-by: tosih <somuchhail@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
providers/v1/gcp/secretmanager/client.go (1)
716-730: Consider edge case: multiple versions with identical CreateTime.The logic finds the version with the latest
CreateTime, but if two versions have identical timestamps, the result depends on iteration order which may not be deterministic. While unlikely in practice, consider using version number as a tiebreaker for deterministic behavior.providers/v1/gcp/secretmanager/fake/fake.go (1)
76-85: Consider adding error handling for invalid field names in setField.The
setFieldhelper silently ignores invalid field names. While this works for the current use case, if theSecretVersionIteratorstruct changes in a future library update, the mock could silently break. Consider logging a warning or panicking in tests when a field isn't found.setField := func(name string, value any) { v := reflect.ValueOf(it).Elem() field := v.FieldByName(name) if field.IsValid() { //#nosec G103 -- Audited this use and it's only for mocking in testing reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())). Elem().Set(reflect.ValueOf(value)) + } else { + panic(fmt.Sprintf("SecretVersionIterator field %q not found - mock may be incompatible with library version", name)) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
providers/v1/gcp/secretmanager/client.go(3 hunks)providers/v1/gcp/secretmanager/client_test.go(20 hunks)providers/v1/gcp/secretmanager/fake/fake.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
providers/v1/gcp/secretmanager/client_test.go (1)
providers/v1/gcp/secretmanager/fake/fake.go (5)
AddSecretVersionMockReturn(60-63)MockSMClient(34-47)ListSecretVersionsMockReturn(65-67)NewSecretVersionIterator(73-140)AccessSecretVersionMockReturn(55-58)
🔇 Additional comments (8)
providers/v1/gcp/secretmanager/client.go (2)
149-160: LGTM! SecretExists now correctly uses getLatestEnabledVersion.The refactored implementation properly determines secret existence by checking for accessible versions rather than just the secret metadata. This addresses the issue where a secret could exist but have all versions disabled/destroyed.
710-746: Previous path construction issue has been addressed.The implementation now correctly handles the version name:
- When an enabled version is found,
versionNameis set toversion.Name(already a full resource path)- When no enabled versions exist, the fallback constructs the full path:
fmt.Sprintf("%s/versions/latest", name)- Line 738 uses
versionNamedirectly without wrapping it in anotherfmt.SprintfThis resolves the double-prefixed path issue raised in previous reviews.
providers/v1/gcp/secretmanager/fake/fake.go (2)
69-140: Reflection-based mock iterator is correctly implemented for testing purposes.The use of
reflectandunsafeto manipulate unexported fields is an accepted pattern when mocking Google Cloud iterators. The#nosec G103annotation appropriately acknowledges the audited use of unsafe pointers.
293-297: LGTM! NewListSecretVersionsFn provides clean mock injection.The method follows the established pattern used by other mock setters in this file (e.g.,
NewAccessSecretVersionFn,NewGetSecretFn).providers/v1/gcp/secretmanager/client_test.go (4)
1014-1165: LGTM! Comprehensive test coverage for SecretExists.The test cases properly cover:
- Secret with accessible enabled versions (global and regional)
- NotFound scenarios (secret doesn't exist)
- PermissionDenied error propagation
- DESTROYED/disabled state handling
- Regional endpoint behavior
The use of
setupMockfunction pattern improves test readability and maintainability.
1146-1164: Good test case for destroyed/disabled secret version edge case.This test case correctly validates that when all versions are destroyed/disabled, the error is propagated rather than returning
exists=true. This aligns with the PR objective to properly handle disabled secret versions.
1189-1268: LGTM! Regional endpoint tests are well structured.Tests correctly validate the regional secret path format (
projects/{project}/locations/{location}/secrets/{secret}/versions/{version}) and both exists/not-exists scenarios with regional endpoints.
82-82: Consistent use of context.TODO() in test setup helpers.Using
context.TODO()inmakeValidSecretManagerTestCaseandmakeValidSecretManagerTestCaseCustomis appropriate since these helpers run before the test context is available.Also applies to: 112-112
d464236 to
2cc7fc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
providers/v1/gcp/secretmanager/fake/fake.go (1)
80-88: Consider adding error handling for missing fields.The
setFieldhelper silently ignores cases where the field is not valid. While this works for the current use case, consider logging or returning an error if a field is not found, to catch potential issues if the underlying library changes its field names.setField := func(name string, value any) { v := reflect.ValueOf(it).Elem() field := v.FieldByName(name) - if field.IsValid() { - //#nosec G103 -- Audited this use and it's only for mocking in testing - reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())). - Elem().Set(reflect.ValueOf(value)) + if !field.IsValid() { + panic(fmt.Sprintf("SecretVersionIterator field %q not found - library may have changed", name)) } + //#nosec G103 -- Audited this use and it's only for mocking in testing + reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())). + Elem().Set(reflect.ValueOf(value)) }providers/v1/gcp/secretmanager/client.go (1)
192-196: Minor: Unused variable initialization when location is set.When
c.store.Locationis set (regional secrets), thereplicationvariable is initialized but never used since regional secrets don't set replication (as per the check on line 230). This is harmless but slightly wasteful.+ var replication *secretmanagerpb.Replication + if c.store.Location == "" { - replication := &secretmanagerpb.Replication{ + replication = &secretmanagerpb.Replication{ - Replication: &secretmanagerpb.Replication_Automatic_{ - Automatic: &secretmanagerpb.Replication_Automatic{}, - }, + Replication: &secretmanagerpb.Replication_Automatic_{ + Automatic: &secretmanagerpb.Replication_Automatic{}, + }, + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
providers/v1/gcp/secretmanager/client.go(3 hunks)providers/v1/gcp/secretmanager/client_test.go(22 hunks)providers/v1/gcp/secretmanager/fake/fake.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
providers/v1/gcp/secretmanager/client.go (1)
providers/v1/oracle/oracle.go (1)
SecretExists(110-110)
🔇 Additional comments (11)
providers/v1/gcp/secretmanager/client_test.go (4)
26-35: LGTM: New imports for time-based testing.The additions of
timeandtimestamppbimports are correctly added to support the new timestamp-based version selection tests.
1016-1233: Well-structured test cases for SecretExists.The test cases comprehensively cover:
- Secret with accessible version exists
- Multiple versions selecting latest by create time
- Secret not found (NotFound error)
- Permission denied error propagation
- Regional secret existence
- Regional secret not found
- Destroyed/disabled version handling
The use of
setupMockfunction for per-test mock injection is a clean pattern.
1235-1315: LGTM: Regional endpoint tests properly updated.The
TestSecretExistsWithRegionalEndpointtests correctly use the newListSecretVersionsMockReturnand properly construct regional secret paths likeprojects/foo/locations/us-east1/secrets/bar/versions/1.
1191-1210: Test expectations are correct and align with implementation.The test case correctly validates that when a secret's latest enabled version is destroyed/disabled,
SecretExistsreturns(false, error)rather than silently returningfalse.The implementation flow confirms this:
SecretExistscallsgetLatestEnabledVersion, which falls back to accessing "latest" when no enabled versions exist. WhenAccessSecretVersionreturns aFailedPreconditionerror for a destroyed version, this error is propagated back to the caller. The implementation only treatsNotFoundas a special case (returningfalse, nil), while other errors—includingFailedPrecondition—are returned as-is. This is the intended behavior: distinguishing between "secret doesn't exist" (NotFound) and "secret exists but cannot be accessed" (FailedPrecondition). The test expectations are valid.providers/v1/gcp/secretmanager/fake/fake.go (3)
23-24: Acknowledge: Reflection and unsafe usage for test mocking.The use of
reflectandunsafepackages is necessary here to mock the Google CloudSecretVersionIteratorwhich has unexported fields. The#nosec G103annotation appropriately documents that this unsafe usage has been audited. Since this is test-only code, this approach is acceptable.
69-143: Well-documented mock iterator implementation.The
NewSecretVersionIteratorfunction is well-documented with clear comments explaining:
- The purpose of the function
- How the iterator works (returns all versions on first fetch, then
iterator.Done)- Why reflection is needed (unexported fields)
The implementation correctly uses
iterator.NewPageInfoto wire up the pagination mechanics, making it compatible with the Google iterator package.
296-303: LGTM: Clean mock configuration method.The
NewListSecretVersionsFnmethod provides a clean API for configuring the mock'sListSecretVersionsbehavior in tests.providers/v1/gcp/secretmanager/client.go (4)
149-160: LGTM: SecretExists implementation is clean and correct.The implementation correctly:
- Constructs the secret name using
getName- Delegates to
getLatestEnabledVersionto check for accessible versions- Returns
false, nilforNotFounderrors (secret doesn't exist)- Returns
false, errfor other errors (propagates the error)- Returns
true, nilwhen a version is accessible
690-706: LGTM: Well-documented helper functions.The
getNameandgetParentNamehelper functions are well-documented and correctly handle both global and regional secret paths.
708-715: LGTM: Error detection for destroyed/disabled states.The
isErrSecretDestroyedOrDisabledfunction correctly checks forFailedPreconditionstatus code and message content to detect destroyed or disabled secret versions.
717-760: Path construction issue has been addressed.The previous review comments flagged a critical path construction bug where
version.Name(a full resource path) was being incorrectly wrapped in another path template. This has been fixed:
versionNameis now initialized as empty string (Line 731)- When an enabled version is found,
versionNameis set toversion.Namedirectly (Line 742) - which is already a full path- Only when no enabled versions are found,
versionNameis constructed with the fallback path (Lines 748-750)versionNameis used directly in the request (Line 752)This correctly avoids the double-prefixed path issue.
|
please let me know what else is needed to get this merged in, thanks! |
|
/ok-to-test sha=2cc7fc612b1a1d00e0598c98bb439a795c466617 |
|
/ok-to-test-managed sha=2cc7fc612b1a1d00e0598c98bb439a795c466617 provider=gcp |
|
LGTM. Lets just wait if e2e will actually pass 😄 |
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
|
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [external-secrets](https://github.com/external-secrets/external-secrets) | minor | `1.1.1` -> `1.2.0` | --- ### Release Notes <details> <summary>external-secrets/external-secrets (external-secrets)</summary> ### [`v1.2.0`](https://github.com/external-secrets/external-secrets/releases/tag/v1.2.0) [Compare Source](external-secrets/external-secrets@v1.1.1...v1.2.0) Image: `ghcr.io/external-secrets/external-secrets:v1.2.0` Image: `ghcr.io/external-secrets/external-secrets:v1.2.0-ubi` Image: `ghcr.io/external-secrets/external-secrets:v1.2.0-ubi-boringssl` <!-- Release notes generated using configuration in .github/release.yml at main --> #### What's Changed ##### General - chore: bump 1.1.1 by [@​gusfcarvalho](https://github.com/gusfcarvalho) in [#​5687](external-secrets/external-secrets#5687) - chore: fix the argocd e2e test case by [@​Skarlso](https://github.com/Skarlso) in [#​5688](external-secrets/external-secrets#5688) - feat(provider): add Barbican provider support by [@​rkferreira](https://github.com/rkferreira) in [#​5398](external-secrets/external-secrets#5398) - docs(secretserver): promote secretserver provider to beta by [@​DelineaSahilWankhede](https://github.com/DelineaSahilWankhede) in [#​5668](external-secrets/external-secrets#5668) - feat(controller): add flag to enable/disable secretstore reconcile by [@​Ilhan-Personal](https://github.com/Ilhan-Personal) in [#​5653](external-secrets/external-secrets#5653) - fix(aws-secrets-manager): Apply filtering based on both name and tags if provided by [@​iypetrov](https://github.com/iypetrov) in [#​5685](external-secrets/external-secrets#5685) - fix(gcpsm): SecretExists should check for regional secrets when store location is specified by [@​tokiwong](https://github.com/tokiwong) in [#​5708](external-secrets/external-secrets#5708) - feat: introduce store deprecation by [@​gusfcarvalho](https://github.com/gusfcarvalho) in [#​5711](external-secrets/external-secrets#5711) - feat(charts): add global values for common deployment configurations by [@​Gabryel8818](https://github.com/Gabryel8818) in [#​5652](external-secrets/external-secrets#5652) - feat: add Doppler OIDC-based authentication by [@​mikesellitto](https://github.com/mikesellitto) in [#​5475](external-secrets/external-secrets#5475) - fix: make custom configuration available regardless of environment by [@​Skarlso](https://github.com/Skarlso) in [#​5713](external-secrets/external-secrets#5713) - chore(chart): update bitwarden dependency to v0.5.2 by [@​Skarlso](https://github.com/Skarlso) in [#​5719](external-secrets/external-secrets#5719) - docs(templating): update rbac for generic targets by [@​lostick](https://github.com/lostick) in [#​5736](external-secrets/external-secrets#5736) - fix(testing): Breaking changes should not break ci by [@​evrardjp](https://github.com/evrardjp) in [#​5739](external-secrets/external-secrets#5739) - fix(security): Get rid of getSecretKey by [@​evrardjp](https://github.com/evrardjp) in [#​5738](external-secrets/external-secrets#5738) - fix(aws): parse resource policies into canonical JSON (sorted) before comparing by [@​cmoscofian](https://github.com/cmoscofian) in [#​5622](external-secrets/external-secrets#5622) - docs: Fix example in GCP documentation by [@​headcr4sh](https://github.com/headcr4sh) in [#​5745](external-secrets/external-secrets#5745) - chore(secretserver): update dependencies to accept new DelineaXPM/tss-sdk-go by [@​DelineaSahilWankhede](https://github.com/DelineaSahilWankhede) in [#​5742](external-secrets/external-secrets#5742) - fix(gcpsm): Improve SecretExists method in GCP secret manager provider by [@​tosih](https://github.com/tosih) in [#​5610](external-secrets/external-secrets#5610) - chore(docs): add clarification to helm values being disabled by [@​Skarlso](https://github.com/Skarlso) in [#​5746](external-secrets/external-secrets#5746) - fix(release): apply [`64dc681`](external-secrets/external-secrets@64dc681) to release by [@​jakobmoellerdev](https://github.com/jakobmoellerdev) in [#​5749](external-secrets/external-secrets#5749) - docs(release): 1.2 stability-support.md by [@​jakobmoellerdev](https://github.com/jakobmoellerdev) in [#​5750](external-secrets/external-secrets#5750) ##### Dependencies - chore(deps): bump golang from 1.25.4 to 1.25.5 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5693](external-secrets/external-secrets#5693) - chore(deps): bump golang from 1.25.4-bookworm to 1.25.5-bookworm in /e2e by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5702](external-secrets/external-secrets#5702) - chore(deps): bump ubi9/ubi from `dcd8128` to `75937d9` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5655](external-secrets/external-secrets#5655) - chore(deps): bump peter-evans/slash-command-dispatch from 5.0.0 to 5.0.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5695](external-secrets/external-secrets#5695) - chore(deps): bump github/codeql-action from 4.31.5 to 4.31.7 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5696](external-secrets/external-secrets#5696) - chore(deps): bump actions/stale from 10.1.0 to 10.1.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5697](external-secrets/external-secrets#5697) - chore(deps): bump actions/create-github-app-token from 2.2.0 to 2.2.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5700](external-secrets/external-secrets#5700) - chore(deps): bump step-security/harden-runner from 2.13.2 to 2.13.3 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5698](external-secrets/external-secrets#5698) - chore(deps): bump actions/checkout from 6.0.0 to 6.0.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5699](external-secrets/external-secrets#5699) - chore(deps): bump platformdirs from 4.5.0 to 4.5.1 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5705](external-secrets/external-secrets#5705) - chore(deps): bump distroless/static from `87bce11` to `4b2a093` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5692](external-secrets/external-secrets#5692) - chore(deps): bump alpine from 3.22 to 3.23 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5703](external-secrets/external-secrets#5703) - chore(deps): bump urllib3 from 2.5.0 to 2.6.0 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5704](external-secrets/external-secrets#5704) - chore(deps): bump pymdown-extensions from 10.17.2 to 10.18 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5706](external-secrets/external-secrets#5706) - chore(deps): bump alpine from 3.22.2 to 3.23.0 in /e2e by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5701](external-secrets/external-secrets#5701) - chore(deps): bump golang from `2611181` to `2611181` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5721](external-secrets/external-secrets#5721) - chore(deps): bump codecov/codecov-action from 5.5.1 to 5.5.2 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5725](external-secrets/external-secrets#5725) - chore(deps): bump urllib3 from 2.6.0 to 2.6.2 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5730](external-secrets/external-secrets#5730) - chore(deps): bump github/codeql-action from 4.31.7 to 4.31.8 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5726](external-secrets/external-secrets#5726) - chore(deps): bump anchore/sbom-action from 0.20.10 to 0.20.11 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5724](external-secrets/external-secrets#5724) - chore(deps): bump tornado from 6.5.2 to 6.5.3 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5732](external-secrets/external-secrets#5732) - chore(deps): bump ubi9/ubi from `75937d9` to `d4feb57` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5722](external-secrets/external-secrets#5722) - chore(deps): bump golang from `5117d68` to `09f53de` in /e2e by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5729](external-secrets/external-secrets#5729) - chore(deps): bump alpine from `4b7ce07` to `51183f2` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5694](external-secrets/external-secrets#5694) - chore(deps): bump hashicorp/setup-terraform from [`712b439`](external-secrets/external-secrets@712b439) to [`071811a`](external-secrets/external-secrets@071811a) by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5727](external-secrets/external-secrets#5727) - chore(deps): bump pymdown-extensions from 10.18 to 10.19.1 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5731](external-secrets/external-secrets#5731) - chore(deps): bump step-security/harden-runner from 2.13.3 to 2.14.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5728](external-secrets/external-secrets#5728) - chore(deps): bump actions/cache from 4.3.0 to 5.0.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5723](external-secrets/external-secrets#5723) #### New Contributors - [@​iypetrov](https://github.com/iypetrov) made their first contribution in [#​5685](external-secrets/external-secrets#5685) - [@​tokiwong](https://github.com/tokiwong) made their first contribution in [#​5708](external-secrets/external-secrets#5708) - [@​Gabryel8818](https://github.com/Gabryel8818) made their first contribution in [#​5652](external-secrets/external-secrets#5652) - [@​mikesellitto](https://github.com/mikesellitto) made their first contribution in [#​5475](external-secrets/external-secrets#5475) - [@​lostick](https://github.com/lostick) made their first contribution in [#​5736](external-secrets/external-secrets#5736) - [@​cmoscofian](https://github.com/cmoscofian) made their first contribution in [#​5622](external-secrets/external-secrets#5622) - [@​headcr4sh](https://github.com/headcr4sh) made their first contribution in [#​5745](external-secrets/external-secrets#5745) - [@​tosih](https://github.com/tosih) made their first contribution in [#​5610](external-secrets/external-secrets#5610) **Full Changelog**: <external-secrets/external-secrets@v1.1.1...v1.2.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi4zOS4xIiwidXBkYXRlZEluVmVyIjoiNDIuMzkuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiY2hhcnQiXX0=--> Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/2737 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>
external-secrets#5610) Co-authored-by: Gergely Brautigam <skarlso777@gmail.com> Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Problem Statement
This issue is related to a recent change that addressed this issue: #5584. The PR: #5593 does address the issue but does not cover the case where the latest secret version returned by GCP secret manager could be disabled.
Related Issue
Relates to: #5584
Addresses: #5609
Proposed Changes
SecretExists should check for latest enabled secret version. It does not need to call
GetSecret, as looking for just the version should suffice. Instead we can use the already available private functiongetLatestEnabledVersion().Checklist
git commit --signoffmake testmake reviewableSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.