WIP: NO-JIRA: Update authentication ClusterOperator manifest versions #799
Conversation
This will prevent the CVO to consider it an expected version at all times, which breaks when OIDC is configured and that version does not exist anymore.
openshift-ci-robot
added
the
jira/valid-reference
|
@liouk: This pull request explicitly references no jira issue. In 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
|
For testing purposes, I have manually pulled the fix from #798. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liouk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughRemoved the oauth-openshift version entry from the ClusterOperator manifest and enabled version removal in the OAuth API server status syncer by adding an option during operator startup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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
🧹 Nitpick comments (1)
pkg/operator/starter.go (1)
479-481: Add inline documentation explaining the version removal.Consider adding a comment above this code block to explain:
- Why version removal is needed (OIDC scenario causing oauth-openshift version absence)
- Which version(s) are being removed
- Reference to the related PR objectives or documentation
Apply this diff to add explanatory comments:
+ // Enable version removal for oauth-openshift to prevent CVO from waiting + // when the oauth-openshift version doesn't exist in OIDC scenarios. + // See: related changes in manifests/08_clusteroperator.yaml statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer { return ss.WithVersionRemoval() })
📜 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
📒 Files selected for processing (2)
manifests/08_clusteroperator.yaml(0 hunks)pkg/operator/starter.go(1 hunks)
💤 Files with no reviewable changes (1)
- manifests/08_clusteroperator.yaml
| statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer { | ||
| return ss.WithVersionRemoval() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that version removal targets only the oauth-openshift version.
The addition of WithVersionRemoval() enables version removal functionality on the StatusSyncer, but it's unclear from this code which versions will be removed and under what conditions. Given the PR objective to remove the oauth-openshift version in OIDC scenarios, please verify that:
- The
WithVersionRemoval()method implementation correctly identifies and removes only the oauth-openshift version - The removal logic properly handles both OIDC and non-OIDC scenarios
- Other versions (e.g., for different components) are not inadvertently affected
Since the PR is marked WIP while investigating repercussions, it would be helpful to understand what concerns or edge cases are being investigated.
Run the following script to examine the WithVersionRemoval() implementation and verify its behavior:
🏁 Script executed:
#!/bin/bash
# Description: Examine the WithVersionRemoval implementation and its usage
# Find the WithVersionRemoval method definition
echo "=== WithVersionRemoval method definition ==="
ast-grep --pattern $'func ($_ *StatusSyncer) WithVersionRemoval() $_ {
$$$
}'
echo ""
echo "=== Usage of WithVersionRemoval in codebase ==="
rg -n 'WithVersionRemoval' -A 3 -B 3
echo ""
echo "=== Related manifest changes ==="
# Check if manifests/08_clusteroperator.yaml exists and contains relevant changes
fd -t f '08_clusteroperator.yaml' --exec cat {}Length of output: 3628
Guard version removal behind OIDC mode
Unconditionally enabling WithVersionRemoval() will strip all missing versions, not just oauth-openshift. Wrap this option in the OIDC-specific path (or equivalent flag) to avoid unintended removal elsewhere, add an inline comment clarifying the scope, and ensure tests cover both OIDC and non-OIDC scenarios.
🤖 Prompt for AI Agents
In pkg/operator/starter.go around lines 479–481, the call that unconditionally
appends ss.WithVersionRemoval() must be guarded so version removal only happens
in OIDC mode; change the code to append that option only when the
OIDC/oauth-openshift mode flag or configuration is true (e.g., if oIDCEnabled {
statusControllerOptions = append(...)}), add an inline comment explaining this
is OIDC-specific behavior to avoid stripping missing versions for other auth
modes, and update/add unit/integration tests to assert WithVersionRemoval is
applied when OIDC is enabled and not applied when OIDC is disabled.
|
@liouk: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Closing in favor of #798. /close |
|
@liouk: Closed this PR. In 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. |
2 participants
The CVO looks at the auth ClusterOperator's manifest and determines which versions are expected in the operator's status; if any of those are missing from the operator's status, it enters a waiting state where it waits for the operator to update that version.
However, in OIDC this breaks currently, because the defined
oauth-openshiftversion will not exist. This PR removes that version from the operator's manifest.This PR is currently WIP in order to investigate the repercussions of removing that version from the manifest.