OCPBUGS-86669: fix(cli): make deleteCLISecrets non-fatal during cluster destroy - #8787
OCPBUGS-86669: fix(cli): make deleteCLISecrets non-fatal during cluster destroy#8787rutvik23 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@rutvik23: This pull request references Jira Issue OCPBUGS-86669, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
openshift-ci
Bot
added
area/cli
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rutvik23: This pull request references Jira Issue OCPBUGS-86669, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8787 +/- ##
==========================================
+ Coverage 44.96% 45.01% +0.04%
==========================================
Files 778 778
Lines 97444 97451 +7
==========================================
+ Hits 43819 43866 +47
+ Misses 50602 50556 -46
- Partials 3023 3029 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/cc @bryan-cox |
openshift-ci
Bot
added
the
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
3ebd959 to
cb503da
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@rutvik23: This pull request references Jira Issue OCPBUGS-86669, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
Overall this is a solid fix — the non-fatal approach is architecturally sound and the finalizer contract is preserved. The main ask is adding a test that actually exercises the error path (see inline comment on the test file). The rest are non-blocking nits.
One thing worth tracking as a follow-up: not all CLI-created secrets carry the safe-to-delete-with-cluster label, so reconcileCLISecrets never sets ownerRefs on them (AWS SA token issuer, proxy SSH key, Azure creds, PowerVS secrets). Namespace deletion covers this, but adding the label to those secrets would close the gap properly.
|
|
||
| // clean up CLI generated secrets | ||
| // Non-fatal: secrets with ownerRef on the HostedCluster will be | ||
| // garbage collected when the HostedCluster is deleted. |
There was a problem hiding this comment.
Nit: The comment says "secrets with ownerRef on the HostedCluster will be garbage collected," but that's only true for secrets that carry the safe-to-delete-with-cluster label — reconcileCLISecrets only sets ownerRefs on those. Platform-specific secrets (AWS SA token issuer, proxy SSH key, Azure cloud creds, PowerVS secrets) are created with auto-created-for-infra only, so they won't get ownerRefs.
They're still cleaned up when the operator deletes the control-plane namespace, so the non-fatal approach is safe — but the comment should reflect that. Something like:
// Non-fatal: qualifying secrets are garbage-collected via ownerRef when the
// HostedCluster is deleted; remaining secrets are removed when the operator
// deletes the control-plane namespace.
| // Non-fatal: secrets with ownerRef on the HostedCluster will be | ||
| // garbage collected when the HostedCluster is deleted. | ||
| if err = deleteCLISecrets(ctx, o, c); err != nil { | ||
| return err |
There was a problem hiding this comment.
Nit: There's no right or wrong answer here — Log.Error is semantically valid because something did fail. That said, the repo's established pattern for non-fatal errors that are expected and self-heal uses Log.Info with "error", err.Error() (13+ instances: create.go:802, cmd/infra/aws/destroy.go:188, cmd/oadp/backup.go:122,147,151, etc.). Just mentioning for consistency, not blocking.
| g.Expect(err).ToNot(HaveOccurred()) | ||
| g.Expect(platformSpecificsCalled).To(BeTrue()) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Blocking: This test validates setFinalizer and the happy-path flow, which is useful — but it doesn't exercise the actual behavioral change in this PR. deleteCLISecrets succeeds here because the fake client has no secrets, so both the old return err and the new log-and-continue produce the same result.
Could you add a test where deleteCLISecrets actually fails? For example:
t.Run("When deleteCLISecrets fails it should log and continue", func(t *testing.T) {
g := NewGomegaWithT(t)
t.Setenv("FAKE_CLIENT", "true")
// Inject a Forbidden error on DeleteAllOf for Secrets
// using fake.NewClientBuilder().WithInterceptorFuncs(...)
// then verify DestroyCluster still returns nil
})Without this, the bug fix (OCPBUGS-86669) has zero test coverage.
There was a problem hiding this comment.
Done. Added a test that injects a Forbidden error on DeleteAllOf for Secrets via fake.NewClientBuilder().WithInterceptorFuncs(...) and verifies DestroyCluster still returns nil. Introduced a package-level var newClient in destroy.go (test seam only) so the test can override client creation. Also addressed the comment wording and switched Log.Error to Log.Info per your other suggestions.
cb503da to
e9883b4
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by Unit Test |
openshift-ci-robot
added
the
verified
|
@rutvik23: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rutvik23: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
e9883b4 to
c2adcf2
Compare
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rutvik23 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@rutvik23: This pull request references Jira Issue OCPBUGS-86669, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
ed2edc3 to
77c47e3
Compare
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
some comments, otherwise lgtm
77c47e3 to
02ed68e
Compare
openshift-ci
Bot
added
the
needs-rebase
The deleteCLISecrets function uses DeleteAllOf which requires the deletecollection RBAC verb. When the service account lacks this permission, the function returns an error that prevents removeFinalizer from executing, leaving the openshift.io/destroy-cluster finalizer stuck indefinitely on the HostedCluster. Secrets created by the CLI already have ownerRefs set by the operator (via reconcileCLISecrets), so they are garbage collected when the HostedCluster is deleted. The explicit deletion is redundant and should not block the destroy flow. Extracts an internal destroyCluster function that accepts a client, so unit tests can inject fake clients directly without a package-level var or environment variable. Fixes: https://issues.redhat.com/browse/OCPBUGS-86669 Signed-off-by: Rutvik rkshirsa@redhat.com
02ed68e to
ed9c41c
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@rutvik23: 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. |
Test CoverageVerified non-fatal Test output (all 4 subtests pass)The |
4 participants
What this PR does / why we need it:
deleteCLISecretsnon-fatal in theDestroyClusterCLI flow to prevent theopenshift.io/destroy-clusterfinalizer from getting stuck when the service accountlacks
deletecollectionRBAC permissionhyperapi.Schemein the fake test client to enable unit tests with HyperShift typesProblem
hcp destroy cluster, theopenshift.io/destroy-clusterfinalizer is added early in the flow and removed at the very end. ThedeleteCLISecretsfunction sits betweendestroyPlatformSpecificsandremoveFinalizer:deleteCLISecretsusesDeleteAllOf, which requires thedeletecollectionRBAC verb — a more elevated permission than standard CRUD. When the calling service account lacks this permission, the function returns an error, the CLI exits, and the finalizer is never removed. The HostedCluster remains stuck withdeletionTimestampset indefinitely.Special notes for your reviewer:
Why non-fatal instead of removing the finalizer entirely?
openshift.io/destroy-clusterfinalizer keeps the HostedCluster alive while the CLIperforms platform infrastructure teardown (VPCs, IAM, DNS, etc.). If
destroyPlatformSpecificsfails, the HC staying around is valuable; it serves as the source of truth for InfraID,
Region, and BaseDomain, allowing the user to simply re-run
hypershift destroywithoutremembering those values. Removing the finalizer entirely would lose this recovery anchor.
Why is
deleteCLISecretssafe to skip?reconcileCLISecrets(hostedcluster_controller.go) sets ownerRefs onCLI-created secrets pointing to the HostedCluster. When the finalizer is removed and the HC
is fully deleted, Kubernetes garbage collection handles secret cleanup automatically. The
CLI's explicit
DeleteAllOfis redundant.Why change the fake client scheme?
GetClient()withFAKE_CLIENT=truepreviously returnedfake.NewFakeClient()using onlythe default Kubernetes scheme. This prevented any test from working with HyperShift types
(HostedCluster, NodePool, etc.), the existing test worked around this by passing
nilforthe HostedCluster. Registering
hyperapi.Schemeis a superset (includes all standard k8stypes plus HyperShift types), so existing tests are unaffected.
Which issue(s) this PR fixes:
Fixes: OCPBUGS-86669
Checklist:
When HostedCluster is nil and platform specifics providedWhen destroying a hosted cluster with platform specifics, it should set the destroy finalizer and complete successfullymake verifypassesmake testpassesSummary by CodeRabbit
Bug Fixes
Tests