Revert "OCPBUGS-65675: externaloidc: return errors when node statuses cannot be used to determine oidc state"

[neisw]

Reverts #801

Testing revert for TRT-2460

[openshift-ci-robot]

@neisw: This pull request references Jira Issue OCPBUGS-65675, which is invalid:

• expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is Verified instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Reverts #801

Testing revert for TRT-2460

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]

Walkthrough

This pull request refactors the OIDC availability checking logic by removing informer synchronization requirements and error conditions, then propagates corresponding changes across controller initialization code. A test helper for synchronized informers is removed and replaced with a simpler generic wrapper in specific tests.

Changes

Cohort / File(s)	Change Summary
OIDC Availability Logic pkg/controllers/common/external_oidc.go	Removed informer sync checks for auth, kubeapiservers, and configmaps informers. Removed error when no node statuses exist. Removed validation requiring valid CurrentRevision per node. Added early return when no revisions observed. Retained ConfigMap and OIDC settings validation.
OIDC Test Suite pkg/controllers/common/external_oidc_test.go	Simplified test fixture by collapsing informer-sync flags into single generic fakeInformer[T] wrapper. Removed multiple test cases relying on informer sync behavior. Updated test matrix and assertions to match new early-return logic.
Controller Informer Wiring Simplification pkg/controllers/deployment/deployment_controller.go, pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go, pkg/controllers/ingressstate/ingress_state_controller.go, pkg/controllers/oauthendpoints/oauth_endpoints_controller.go, pkg/controllers/proxyconfig/proxyconfig_controller.go, pkg/controllers/readiness/wellknown_ready_controller.go	Removed AuthConfigCheckerInformers wiring from controller initialization chains. Replaced dynamic informer assembly with explicit inline informer slices where applicable.
Operator Starter Wiring pkg/operator/starter.go	Removed combined informer construction using AuthConfigCheckerInformers variadic expansion. Replaced with single authentication operator client informer in WithWorkloadController call.
Router Certs Test Update pkg/controllers/routercerts/controller_test.go	Replaced test.NewFakeSharedIndexInformerWithSync calls with new generic newFakeInformer[T] wrapper helper. Added local fakeInformer[T] type implementing Informer() and Lister() methods.
Test Helper Removal test/library/informer.go	Deleted entire file containing FakeSharedIndexInformerWithSync[T] generic type and related constructor, methods (Informer(), Lister(), HasSynced()), and supporting fakeSharedIndexInformer struct.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• OIDC logic simplification (external_oidc.go): Verify that removing informer sync checks and node status validation does not introduce race conditions or unhandled scenarios
• Public test API removal (test/library/informer.go): Confirm all usages of FakeSharedIndexInformerWithSync have been migrated (may affect callers outside this PR)
• Cross-controller consistency: Ensure informer removals across multiple controller files are intentional and do not leave orphaned initialization code
• Test coverage implications: Validate that test simplification (removed test cases) maintains adequate coverage for edge cases previously tested with informer sync flags

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: neisw
Once this PR has been reviewed and has the lgtm label, please assign liouk for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[neisw]

/payload-aggregate periodic-ci-openshift-release-master-ci-4.21-e2e-gcp-ovn-upgrade 3

[openshift-ci]

@neisw: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

• periodic-ci-openshift-release-master-ci-4.21-e2e-gcp-ovn-upgrade

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3f52a2f0-d040-11f0-91e1-f79c7859c533-0

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/controllers/common/external_oidc_test.go (1)

321-331: Duplicate fakeInformer helper across test files.

This generic fakeInformer type is identical to the one in controller_test.go. Consider extracting it to a shared test utility (e.g., test/library/informer.go) to avoid duplication.

+// Consider moving fakeInformer to test/library/informer.go
+// and importing it in both test files to reduce duplication

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between e6c52f8 and 4476174.

📒 Files selected for processing (11)

• pkg/controllers/common/external_oidc.go (1 hunks)
• pkg/controllers/common/external_oidc_test.go (17 hunks)
• pkg/controllers/deployment/deployment_controller.go (1 hunks)
• pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go (0 hunks)
• pkg/controllers/ingressstate/ingress_state_controller.go (0 hunks)
• pkg/controllers/oauthendpoints/oauth_endpoints_controller.go (3 hunks)
• pkg/controllers/proxyconfig/proxyconfig_controller.go (0 hunks)
• pkg/controllers/readiness/wellknown_ready_controller.go (0 hunks)
• pkg/controllers/routercerts/controller_test.go (2 hunks)
• pkg/operator/starter.go (1 hunks)
• test/library/informer.go (0 hunks)

💤 Files with no reviewable changes (5)

• pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go
• pkg/controllers/readiness/wellknown_ready_controller.go
• pkg/controllers/ingressstate/ingress_state_controller.go
• test/library/informer.go
• pkg/controllers/proxyconfig/proxyconfig_controller.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/controllers/common/external_oidc.go
• pkg/controllers/deployment/deployment_controller.go
• pkg/controllers/routercerts/controller_test.go
• pkg/controllers/oauthendpoints/oauth_endpoints_controller.go
• pkg/operator/starter.go
• pkg/controllers/common/external_oidc_test.go

🔇 Additional comments (9)

pkg/controllers/deployment/deployment_controller.go (1)

126-130: Informer wiring change looks correct for the revert.

The inline informer slice correctly includes Ingresses, Proxies, and Node informers. The removal of AuthConfigCheckerInformers aligns with reverting to the previous behavior where the controller didn't react to changes in authentication configuration resources.

pkg/operator/starter.go (1)

518-518: Simplified informer wiring aligns with the revert.

The change removes the aggregated informer set and uses only the operator client informer, consistent with the broader PR pattern of removing AuthConfigCheckerInformers from controller initialization.

pkg/controllers/routercerts/controller_test.go (2)

266-269: Test helper change is appropriate.

The newFakeInformer wrapper correctly provides the lister functionality needed for the test. Passing nil for KubeAPIServerLister and ConfigMapLister is acceptable since this test focuses on the IntegratedOAuth authentication type path, which returns early before accessing those listers.

---

483-497: Generic fakeInformer helper is well-designed.

The helper provides a minimal implementation suitable for testing. Note that Informer() returning nil means any code path that calls HasSynced() or similar methods on the informer will panic. This is acceptable for these tests since the reverted code removes sync checks.

pkg/controllers/oauthendpoints/oauth_endpoints_controller.go (3)

65-70: Inline informer list correctly captures data dependencies.

The informer list includes cmInformer, secretInformer, routeInformer, and ingressInformer which are all used by endpointListFunc and getTLSConfigFunc. This is consistent with the revert pattern.

---

95-98: Informer list matches controller's data dependencies.

The ConfigMaps and Services informers align with what endpointsListFunc and getTLSConfigFunc access.

---

124-127: Informer list is appropriate for the endpoint check controller.

The Endpoints and ConfigMaps informers match the resources accessed by the controller's functions.

pkg/controllers/common/external_oidc.go (1)

75-78: Behavioral change: Empty node statuses now return false instead of an error.

This reverts to treating the absence of node statuses as "OIDC not available" rather than an error condition. This is less strict but may be intentional for the testing scenario (TRT-2460).

Be aware that during cluster bootstrap or if KAS hasn't reported any node statuses, this will silently return false rather than surfacing the condition as an error. Ensure this behavior is acceptable for your use case.

pkg/controllers/common/external_oidc_test.go (1)

32-36: Test case validates the reverted behavior.

The "no node statuses observed" case correctly expects (false, nil) instead of an error, matching the code change in external_oidc.go at lines 75-78.

[neisw]

/hold
not sure this is related, hold while testing

[neisw]

We suspect the issue we are investigating might be fixed by openshift/operator-framework-operator-controller#574. Still holding for analysis but will close on verification.

[neisw]

/close

openshift/operator-framework-operator-controller#574 appears to be the fix

[openshift-ci]

@neisw: Closed this PR.

In response to this:

/close

openshift/operator-framework-operator-controller#574 appears to be the fix

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.

[openshift-ci-robot]

@neisw: This pull request references Jira Issue OCPBUGS-65675. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

Reverts #801

Testing revert for TRT-2460

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.