OCPBUGS-100179: fix forward controller FilterFunc and finalizer removal - #450
OCPBUGS-100179: fix forward controller FilterFunc and finalizer removal#450emmahone wants to merge 2 commits into
Conversation
The legacyImagePullSecretController has two related defects that together cause dockercfg secrets to remain stuck with an openshift.io/legacy-token finalizer after their namespace's deletionTimestamp is set, leaving namespaces permanently in Terminating. **Defect 1 — FilterFunc silently drops transitioning secrets (new fix)** The informer FilterFunc required the openshift.io/token-secret.name annotation to be present. Secrets that have already transitioned to the "bound" auth type (annotation removed, openshift.io/internal-registry-auth-token.binding: bound) but still carry the legacy-token finalizer were therefore never queued for reconciliation. Once a namespace is deleted and deletionTimestamp is set on these secrets, sync() is never invoked, the finalizer is never cleared, and the namespace hangs indefinitely. Fix: extend the FilterFunc to also pass secrets whose deletionTimestamp is set and that still carry the openshift.io/legacy-token finalizer, regardless of the token-secret.name annotation. The existing sync() deletion path already handles the absent-annotation case correctly (len(t)==0 skips token deletion and proceeds straight to finalizer removal). **Defect 2 — Apply with nil finalizers does not clear the field (cleanup fix)** The deletion path built a filtered finalizers slice and called Apply with it. When openshift.io/legacy-token was the only finalizer, the slice was nil; the applyconfigurations field is tagged omitempty, so nil serialises as absent from the patch body. SSA therefore does not touch the finalizers field and the finalizer persists. Fix: use a JSON Patch (identical to the rollback controller's approach) which directly removes the specific finalizer by index and is not subject to omitempty serialisation. The "test" op before the "remove" ensures safe concurrent writes by failing fast if the cache is stale. Fixes: https://issues.redhat.com/browse/OCPBUGS-100179
openshift-ci-robot
added
jira/valid-reference
|
@emmahone: This pull request references Jira Issue OCPBUGS-100179, 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. |
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe controller now enqueues deleting Dockercfg secrets that retain the legacy-token finalizer and removes that finalizer with a JSON Patch. Tests cover normal, transitioned, completed, and stale-cache deletion paths. ChangesLegacy finalizer cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SecretEventFilter
participant LegacyImagePullSecretController
participant SecretLister
participant KubernetesAPI
SecretEventFilter->>LegacyImagePullSecretController: Enqueue deleting Dockercfg secret
LegacyImagePullSecretController->>SecretLister: Read cached secret
LegacyImagePullSecretController->>KubernetesAPI: Apply test/remove JSON Patch
KubernetesAPI-->>LegacyImagePullSecretController: Return patch result
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
|
@emmahone: This pull request references Jira Issue OCPBUGS-100179, which is invalid:
Comment 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
openshift-ci-robot
added
the
jira/valid-bug
|
@emmahone: This pull request references Jira Issue OCPBUGS-100179, 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/jira refresh |
|
@emmahone: This pull request references Jira Issue OCPBUGS-100179, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go (1)
97-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the informer filter path.
These cases call
syncdirectly. They do not verify that the changedFilterFuncenqueues a deleting Dockercfg secret with the legacy finalizer and withoutopenshift.io/token-secret.name.Add a controller-level event test. Send that secret through the informer. Assert that the queue receives its namespace key.
🤖 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/internalregistry/controllers/legacy_image_pull_secret_controller_test.go` around lines 97 - 101, Extend the tests around legacyImagePullSecretController with a controller-level informer event case that sends a deleting Dockercfg secret carrying the legacy finalizer and no openshift.io/token-secret.name through the informer, then assert the work queue receives its namespace/name key. Keep the existing direct sync cases unchanged and exercise the changed FilterFunc rather than invoking sync directly.
🤖 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.
Nitpick comments:
In
`@pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go`:
- Around line 97-101: Extend the tests around legacyImagePullSecretController
with a controller-level informer event case that sends a deleting Dockercfg
secret carrying the legacy finalizer and no openshift.io/token-secret.name
through the informer, then assert the work queue receives its namespace/name
key. Keep the existing direct sync cases unchanged and exercise the changed
FilterFunc rather than invoking sync directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3325f097-e87e-4712-abd3-b76ccb378c63
📒 Files selected for processing (2)
pkg/internalregistry/controllers/legacy_image_pull_secret_controller.gopkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go
|
@emmahone: 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. |
2 participants
Summary
Two related defects in
legacyImagePullSecretControllerleave dockercfgsecrets permanently stuck with an
openshift.io/legacy-tokenfinalizer afternamespace deletion, causing namespaces to hang in Terminating forever.
This is distinct from OCPBUGS-52193 / PR #380 (which fixed the
managementState: Removedscenario): the customer's cluster hasmanagementState: Managedso the rollback controller added by #380 neveractivates, and the forward controller's own FilterFunc is the defect.
Defect 1 — FilterFunc silently drops transitioning secrets (new fix)
The informer
FilterFuncrequiredopenshift.io/token-secret.nameto bepresent. Secrets that transitioned to the "bound" auth type have this
annotation removed (they carry
openshift.io/internal-registry-auth-token.binding: boundinstead), so they are silently dropped before ever reaching the workqueue.
Once namespace deletion sets
deletionTimestampon these secrets,sync()isnever called, the finalizer is never cleared, and the namespace hangs.
Fix: extend the
FilterFuncto also pass secrets whosedeletionTimestampis set and that still carry the
openshift.io/legacy-tokenfinalizer,regardless of the annotation. The existing deletion path in
sync()alreadyhandles the absent-annotation case correctly — it skips token-secret deletion
(len(t)==0) and proceeds straight to finalizer removal.
Defect 2 — Apply with nil finalizers does not clear the field (cleanup fix)
The deletion path built a filtered finalizers slice and called
Applywithit. When
openshift.io/legacy-tokenwas the only finalizer, the slice wasnil; the applyconfigurations field is taggedomitempty, sonilserialisesas absent from the patch body. SSA therefore does not touch the finalizers
field and the finalizer persists.
Fix: use a JSON Patch (identical to the rollback controller's approach)
which directly removes the specific finalizer by index and is not subject to
omitemptyserialisation. The"test"op before the"remove"ensures safeconcurrent writes by failing fast if the cache is stale.
Fixes: https://issues.redhat.com/browse/OCPBUGS-100179
Summary by CodeRabbit