OCPBUGS-65687: fix(cpo): prevent informer creation for inaccessible resource types - #7819
Conversation
When a CRD like SecretProviderClass is installed on a non-Azure management cluster, the CPO creates informers for it during cleanup. The informer's LIST/WATCH fails with 403 Forbidden (no RBAC), retries forever, and blocks reconciliation of the entire hosted control plane. Introduce GVKAccessChecker that probes each GVK's accessibility once using an uncached reader (no informer created), caches the result, and either skips (inaccessible) or uses the normal cached client (accessible) for all subsequent reconciles. Refs: OCPBUGS-65687 Signed-off-by: Mulham Raee <mulham.raee@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
muraee
changed the title
openshift-ci-robot
added
jira/severity-critical
WalkthroughThis PR introduces a GVK accessibility probing and caching mechanism for control plane components. A new Changes
Sequence DiagramsequenceDiagram
participant Reconciler
participant GenericAdapter
participant GVKAccessChecker
participant Reader
participant Cache
Reconciler->>GenericAdapter: reconcile(obj)
GenericAdapter->>GenericAdapter: evaluate predicate
alt Predicate is true
GenericAdapter->>GenericAdapter: proceed with adapt & apply
else Predicate is false
GenericAdapter->>GVKAccessChecker: GetOrProbe(ctx, obj)
GVKAccessChecker->>Cache: lookup GVK
alt Cache hit
Cache-->>GVKAccessChecker: cached result
else Cache miss
GVKAccessChecker->>Reader: Get(ctx, obj)
alt Reader returns success/NotFound
Reader-->>GVKAccessChecker: accessible=true
GVKAccessChecker->>Cache: store accessible
else Reader returns Forbidden/NoMatch
Reader-->>GVKAccessChecker: error
GVKAccessChecker->>Cache: store inaccessible
else Reader returns transient error
Reader-->>GVKAccessChecker: error
GVKAccessChecker-->>GenericAdapter: propagate error
end
end
GVKAccessChecker-->>GenericAdapter: (accessible, error)
alt Accessible
GenericAdapter->>GenericAdapter: check ownership, delete if needed
else Not accessible
GenericAdapter->>GenericAdapter: skip processing
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
the
jira/valid-bug
|
@muraee: This pull request references Jira Issue OCPBUGS-65687, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee 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 |
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.
🧹 Nitpick comments (1)
support/controlplane-component/generic-adapter_test.go (1)
118-130: Minor: Unused variable in test.The variable
objcreated on line 118 is assigned but never used (line 129 just suppresses the unused warning with_ = obj). Consider removing it since the test only usescmObj.♻️ Suggested cleanup
t.Run("When predicate is false and GVK checker is nil it should proceed with existing logic", func(t *testing.T) { g := NewWithT(t) // No checker — backward compatibility. cpCtx := testCPContext(t, nil) ga := &genericAdapter{ predicate: func(_ WorkloadContext) bool { return false }, } - obj := testObjWithGVK(inaccessibleGVK) - // Without a checker the code falls through to Client.Get which will + // Without a checker the code falls through to Client.Get which will // return an error or NotFound depending on the fake client setup. // Since the object doesn't exist and the GVK is registered (ConfigMap used in fake), // we use a ConfigMap to avoid scheme issues. cmObj := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "test-resource", Namespace: "test-ns", }, } - _ = obj // unused in this path err := ga.reconcile(cpCtx, cmObj) // Should succeed (object not found → no deletion needed). g.Expect(err).ToNot(HaveOccurred()) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/controlplane-component/generic-adapter_test.go` around lines 118 - 130, Remove the unused local variable to clean up the test: delete the call and assignment to obj := testObjWithGVK(inaccessibleGVK) and the no-op suppression _ = obj, since the test uses cmObj and calls ga.reconcile(cpCtx, cmObj); ensure no other references to obj or testObjWithGVK remain in generic-adapter_test.go so the test compiles cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@support/controlplane-component/generic-adapter_test.go`:
- Around line 118-130: Remove the unused local variable to clean up the test:
delete the call and assignment to obj := testObjWithGVK(inaccessibleGVK) and the
no-op suppression _ = obj, since the test uses cmObj and calls
ga.reconcile(cpCtx, cmObj); ensure no other references to obj or testObjWithGVK
remain in generic-adapter_test.go so the test compiles cleanly.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/main.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/generic-adapter.gosupport/controlplane-component/generic-adapter_test.gosupport/controlplane-component/gvk_cache.gosupport/controlplane-component/gvk_cache_test.go
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/verified by @xiuwang |
openshift-ci-robot
added
the
verified
|
@xiuwang: 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. |
|
/retest-required |
|
@muraee: 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. |
|
@muraee: An error was encountered searching for bug OCPBUGS-65687 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/OCPBUGS-65687": GET https://issues.redhat.com/rest/api/2/issue/OCPBUGS-65687 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
|
/cherry-pick release-4.21 |
|
@muraee: new pull request created: #8833 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 kubernetes-sigs/prow repository. |
6 participants
Summary
genericAdapter.reconcile()andcontrolPlaneWorkload.delete()paths callClient.Get()on the cached client to check if the resource exists before deleting it. The cached client creates an informer as a side effect ofGet(), and these informers fail permanently with 403 Forbidden when the CPO has no RBAC for the resource type. The informer retries LIST/WATCH forever, blocking reconciliation of the entire hosted control plane.GVKAccessCheckerinterface (backed bygvkAccessCache) that probes each GVK's accessibility once using an uncached reader (no informer created), caches the result, and either skips (inaccessible) or proceeds with the normal cached client (accessible) for all subsequent reconciles. The probe is inserted before both cleanup paths: the predicate-false branch ingenericAdapter.reconcile()and the manifest deletion loop incontrolPlaneWorkload.delete().Refs: OCPBUGS-65687
Test plan
GVKAccessCheckercovering all error paths (Forbidden, NoMatch, NotFound, OK, transient errors, cache hit, empty GVK)genericAdapter.reconcile()verifying skip behavior when predicate is false and GVK is inaccessiblecontrolplane-componenttests pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests