Add destroy previous secret versions functionality when updating a secret#5194
Add destroy previous secret versions functionality when updating a secret#5194psalajova wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughCreateOrUpdateSecret gains a boolean to optionally destroy previously enabled GSM secret versions after adding a new version. The SecretManagerClient interface, mocks, and test fakes are updated to include ListSecretVersions and DestroySecretVersion. Two call sites enable destruction; CreateSecrets preserves prior behavior (destruction disabled). ChangesGSM Secret Version Cleanup
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gsm-secrets/execution.go`:
- Around line 72-98: The cleanup currently deletes all enabled versions except
currentVersionName, which can remove newer versions created concurrently; update
destroyPreviousSecretVersions to first retrieve the current version's creation
timestamp (call GetSecretVersion for currentVersionName) and then only destroy
versions whose CreateTime is strictly before that timestamp (keep skipping when
v.Name == currentVersionName or when v.CreateTime is nil or not before current's
CreateTime); use the created timestamp comparison instead of deleting every
enabled version and still log errors from DestroySecretVersion as before
(references: destroyPreviousSecretVersions, currentVersionName, v.Name,
client.GetSecretVersion, client.DestroySecretVersion,
ListSecretVersionsRequest).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: add70176-a6c7-429e-979d-5d6292846f2b
📒 Files selected for processing (5)
cmd/ci-secret-bootstrap/main_test.gopkg/gsm-secrets/execution.gopkg/gsm-secrets/execution_mock.gopkg/secrets/gsm.gopkg/steps/multi_stage/gsm_bundle_resolver_test.go
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hector-vido, Prucek, psalajova The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // If labels or annotations are nil, they won't be set on the secret. | ||
| func CreateOrUpdateSecret(ctx context.Context, client SecretManagerClient, projectIdNumber, secretName string, payload []byte, labels, annotations map[string]string) error { | ||
| // If destroyPreviousVersions is true, all previously enabled versions are destroyed after adding the new one. | ||
| func CreateOrUpdateSecret(ctx context.Context, client SecretManagerClient, projectIdNumber, secretName string, payload []byte, labels, annotations map[string]string, destroyPreviousVersions bool) error { |
There was a problem hiding this comment.
It seems that you always want to destroy the previous versions, so the bool doesn't make any difference here.
There was a problem hiding this comment.
There are 3 call sites:
- gsm.go:68 (SetFieldOnItem): passes
true-- ci-secret-generator updating existing secrets - gsm.go:81 (UpdateIndexSecret): passes
true-- ci-secret-generator updating index - execution.go:299 (CreateSecrets): passes
false-- initial secret creation by gsm-secret-sync
The false at execution.go:299 is intentional: when gsm-secret-sync creates secrets for the first time, there are no previous versions to destroy. Additionally, the migration code I'm working on also calls CreateOrUpdateSecret with false (initial migration, no previous versions). I agree that maybe we could set it to always destroy previous versions, but it feels better to be able to control it.
There was a problem hiding this comment.
So, you have two explicit behaviors. You shouldn't flip the switch with a boolean. Try to split the logic correctly and use the corresponding function in the correct places.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
We are discussing if this is the right approach in https://redhat-internal.slack.com/archives/GB7NB0CUC/p1779274337330229 , waiting for the conclusion there before I move forward with this |
psalajova
force-pushed
the
gsm-destroy-past-versions-upon-update
branch
from
5ae92dd to
46e8fdc
Compare
|
New changes are detected. LGTM label has been removed. |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/steps/multi_stage/gsm_bundle_resolver_test.go (1)
52-54: ⚡ Quick winPrefer returning an error instead of panicking in the test fake.
Use an explicit error return here so failures stay assertion-driven and consistent with non-panic error paths.
Suggested fix
func (f *fakeGSMClient) DestroySecretVersion(ctx context.Context, req *secretmanagerpb.DestroySecretVersionRequest, opts ...gax.CallOption) (*secretmanagerpb.SecretVersion, error) { - panic("DestroySecretVersion not implemented in test") + return nil, errors.New("DestroySecretVersion not implemented in test") }As per coding guidelines,
Avoid panic() in Go except in init() or fatal conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go` around lines 52 - 54, The test fake method DestroySecretVersion on fakeGSMClient currently panics; change it to return an explicit error instead (e.g., return nil and an error describing the unimplemented method) so tests can handle failures via assertions; locate the DestroySecretVersion method on the fakeGSMClient type and replace the panic with a nil *secretmanagerpb.SecretVersion and a descriptive error value.pkg/gsm-secrets/execution.go (1)
31-31: Coordinate downstream update for this public API change.
CreateOrUpdateSecretnow has a new parameter; linked-repo findings showopenshift/ci-chat-botstill vendors the old signature/interface. Please revendor/update in lockstep to avoid downstream compile breaks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gsm-secrets/execution.go` at line 31, CreateOrUpdateSecret now adds a new parameter (destroyPreviousVersions bool), so update downstream consumers and vendor copies that still call the old signature (notably openshift/ci-chat-bot) to use the new function signature; change calls to pass the new destroyPreviousVersions argument, update the vendored pkg/gsm-secrets to this version, run go mod tidy / vendor refresh and rebuild downstream to ensure the interface/signature change is synchronized and avoids compile errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gsm-secrets/execution.go`:
- Around line 103-110: The interface methods in SecretManagerClient reference
gax.CallOption but execution.go is missing the gax package import; add the
import "github.com/googleapis/gax-go/v2" to the imports block in
pkg/gsm-secrets/execution.go so ListSecrets, ListSecretVersions, GetSecret,
DeleteSecret, CreateSecret, AddSecretVersion, DestroySecretVersion and
AccessSecretVersion compile and use gax.CallOption correctly, then run go build
to verify.
---
Nitpick comments:
In `@pkg/gsm-secrets/execution.go`:
- Line 31: CreateOrUpdateSecret now adds a new parameter
(destroyPreviousVersions bool), so update downstream consumers and vendor copies
that still call the old signature (notably openshift/ci-chat-bot) to use the new
function signature; change calls to pass the new destroyPreviousVersions
argument, update the vendored pkg/gsm-secrets to this version, run go mod tidy /
vendor refresh and rebuild downstream to ensure the interface/signature change
is synchronized and avoids compile errors.
In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go`:
- Around line 52-54: The test fake method DestroySecretVersion on fakeGSMClient
currently panics; change it to return an explicit error instead (e.g., return
nil and an error describing the unimplemented method) so tests can handle
failures via assertions; locate the DestroySecretVersion method on the
fakeGSMClient type and replace the panic with a nil
*secretmanagerpb.SecretVersion and a descriptive error value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c20eb856-1ef4-487d-9bba-dc0c5d30eeb9
📒 Files selected for processing (5)
cmd/ci-secret-bootstrap/main_test.gopkg/gsm-secrets/execution.gopkg/gsm-secrets/execution_mock.gopkg/secrets/gsm.gopkg/steps/multi_stage/gsm_bundle_resolver_test.go
psalajova
force-pushed
the
gsm-destroy-past-versions-upon-update
branch
from
46e8fdc to
ff62178
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/steps/multi_stage/gsm_bundle_resolver_test.go (1)
48-50: ⚡ Quick winMake
ListSecretVersionsfail-fast in this “should never be called” fake.Line 48 currently returns
nil, which can hide accidental usage. This fake’s contract is explicit; panic here too for consistency and stronger test signaling.Proposed change
func (f *fakeGSMClient) ListSecretVersions(ctx context.Context, req *secretmanagerpb.ListSecretVersionsRequest, opts ...gax.CallOption) *secretmanager.SecretVersionIterator { - return nil + panic("ListSecretVersions should not be called when cache is populated") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go` around lines 48 - 50, The fake method ListSecretVersions on fakeGSMClient currently returns nil and should fail-fast; replace the nil return with an immediate panic (or equivalent immediate test-failing behavior) so any accidental call to fakeGSMClient.ListSecretVersions(...) surfaces immediately (use a clear message like "ListSecretVersions should not be called" to aid debugging).cmd/ci-secret-bootstrap/main_test.go (1)
2326-2328: ⚡ Quick winFail fast on unexpected
ListSecretVersionscalls in the fake.Returning
nilhere can silently pass if code unexpectedly calls this method (the production path treats nil iterator as a no-op). Panicking (or returning a deterministic failing iterator path) makes regressions visible in tests.Proposed change
func (f *fakeGSMClient) ListSecretVersions(ctx context.Context, req *secretmanagerpb.ListSecretVersionsRequest, opts ...gax.CallOption) *secretmanager.SecretVersionIterator { - return nil + panic("ListSecretVersions not implemented in test") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ci-secret-bootstrap/main_test.go` around lines 2326 - 2328, The fakeGSMClient's ListSecretVersions currently returns nil which can hide unexpected calls; change fakeGSMClient.ListSecretVersions to fail fast by either panicking with a clear message (e.g., "unexpected ListSecretVersions call in test") or returning a deterministic iterator that immediately yields a non-nil error so tests fail fast; update the implementation referenced by fakeGSMClient.ListSecretVersions to use one of these approaches so any accidental invocation becomes an explicit test failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gsm-secrets/execution.go`:
- Line 32: CreateOrUpdateSecret signature was changed which breaks downstream
vendored consumers (e.g., openshift/ci-chat-bot); coordinate a compatible
rollout by either restoring a backwards-compatible wrapper with the old
signature that calls the new CreateOrUpdateSecret(ctx, client, projectIdNumber,
secretName, payload, labels, annotations, destroyPreviousVersions) or
communicate and land a coordinated vendor bump: notify the downstream repo,
update their vendor snapshot to include the new API, and regenerate any mocks
(mock for CreateOrUpdateSecret) so tests compile against the updated signature.
---
Nitpick comments:
In `@cmd/ci-secret-bootstrap/main_test.go`:
- Around line 2326-2328: The fakeGSMClient's ListSecretVersions currently
returns nil which can hide unexpected calls; change
fakeGSMClient.ListSecretVersions to fail fast by either panicking with a clear
message (e.g., "unexpected ListSecretVersions call in test") or returning a
deterministic iterator that immediately yields a non-nil error so tests fail
fast; update the implementation referenced by fakeGSMClient.ListSecretVersions
to use one of these approaches so any accidental invocation becomes an explicit
test failure.
In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go`:
- Around line 48-50: The fake method ListSecretVersions on fakeGSMClient
currently returns nil and should fail-fast; replace the nil return with an
immediate panic (or equivalent immediate test-failing behavior) so any
accidental call to fakeGSMClient.ListSecretVersions(...) surfaces immediately
(use a clear message like "ListSecretVersions should not be called" to aid
debugging).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c0b58b58-59ce-450e-b290-efe369c730ab
📒 Files selected for processing (5)
cmd/ci-secret-bootstrap/main_test.gopkg/gsm-secrets/execution.gopkg/gsm-secrets/execution_mock.gopkg/secrets/gsm.gopkg/steps/multi_stage/gsm_bundle_resolver_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/gsm-secrets/execution_mock.go
| // If labels or annotations are nil, they won't be set on the secret. | ||
| func CreateOrUpdateSecret(ctx context.Context, client SecretManagerClient, projectIdNumber, secretName string, payload []byte, labels, annotations map[string]string) error { | ||
| // destroyPreviousVersions controls whether old enabled versions are cleaned up after adding the new one. | ||
| func CreateOrUpdateSecret(ctx context.Context, client SecretManagerClient, projectIdNumber, secretName string, payload []byte, labels, annotations map[string]string, destroyPreviousVersions bool) error { |
There was a problem hiding this comment.
Coordinate downstream vendored consumers before merging this API change.
Line 32 changes CreateOrUpdateSecret’s signature, and linked-repo findings show openshift/ci-chat-bot vendors the old pkg/gsm-secrets API and mock. That downstream will break until its vendor snapshot is updated in lockstep.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/gsm-secrets/execution.go` at line 32, CreateOrUpdateSecret signature was
changed which breaks downstream vendored consumers (e.g.,
openshift/ci-chat-bot); coordinate a compatible rollout by either restoring a
backwards-compatible wrapper with the old signature that calls the new
CreateOrUpdateSecret(ctx, client, projectIdNumber, secretName, payload, labels,
annotations, destroyPreviousVersions) or communicate and land a coordinated
vendor bump: notify the downstream repo, update their vendor snapshot to include
the new API, and regenerate any mocks (mock for CreateOrUpdateSecret) so tests
compile against the updated signature.
|
Scheduling tests matching the |
|
@psalajova: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
I identified that GSM costs were getting high by accumulating secret versions (~150 per secret across ~550 secrets in the
test-platform-infracollection), all using automatic replication. I ran a one-time cleanup script to destroy all non-latest versions, and modified CreateOrUpdateSecret to accept a destroyPreviousVersions flag — enabled for ci-secret-generator — so old versions are automatically destroyed after each update, preventing future cost buildup.This PR adds an option to destroy previous Google Secret Manager (GSM) secret versions when a secret is updated and enables that option for the ci-secret-generator, preventing future accumulation of old secret versions and related billing growth.
Changes and practical impact
Core behavior (pkg/gsm-secrets/execution.go)
Integration into CI tooling (pkg/secrets/gsm.go)
Tests and mocks
Who is affected / operational notes
Compatibility