-
Couldn't load subscription status.
- Fork 107
OCPBUGS-62941: (bugfix): configure status controller to remove unset versions #798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-62941: (bugfix): configure status controller to remove unset versions #798
Conversation
Signed-off-by: Bryce Palmer <[email protected]>
|
@everettraven: This pull request references Jira Issue OCPBUGS-62941, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughAppend a status-controller option in prepareOauthAPIServerOperator to enable version removal on the StatusSyncer when infrastructure topology is not SingleReplica; add an e2e test helper that validates presence/absence of operand versions in the authentication ClusterOperator; remove a public operand version entry from the ClusterOperator manifest. No exported API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc_test.go (1)
876-891: Add documentation and consider conditional validation logic.The function is well-structured, but could benefit from:
- A doc comment explaining when operand versions should be unset (e.g., "validates that oauth-apiserver and oauth-openshift operand versions are not reported when External OIDC is configured")
- Consider whether the validation should be conditional on authentication mode or topology rather than always expecting versions to be unset
The current implementation assumes operand versions should never be present, which may not hold for all test scenarios (see comment on line 730).
Example with documentation:
+// validateOperandVersionsUnset verifies that the authentication ClusterOperator +// does not report operand versions for oauth-apiserver and oauth-openshift. +// These versions should be unset when External OIDC authentication is configured +// to prevent stuck upgrades. func validateOperandVersionsUnset(ctx context.Context, cfgClient *configclient.Clientset) error { operands := sets.New("oauth-apiserver", "oauth-openshift")
📜 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 (1)
test/e2e-oidc/external_oidc_test.go(2 hunks)
|
Pre-merge tested. As private slack pasted test result, the cluster shows error: |
|
/lgtm Contains fixups, holding until squashed. |
|
Did another pre-merge test. As the results pasted in online channel, fresh env after external oidc configured has no issue. Upgrade succeeded and functions worked well. Rollback to IDP also has no regression. Only one minor issue monitored during upgrade but was transient and later gone: The Available was transiently False in above showed. Generally this should not happen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc_test.go (1)
876-901: LGTM! Logic correctly handles both OIDC and IntegratedOAuth modes.The implementation properly validates operand version presence/absence based on the authentication mode:
- When
requireMissing=true(OIDC mode): ensures operands are absent from ClusterOperator status- When
requireMissing=false(IntegratedOAuth mode): ensures both expected operands are presentThe filtering logic at line 886 prevents false positives by only checking for known operands, and the error messages clearly indicate what was expected vs. found.
Optional: Add godoc comment for better documentation.
Consider adding a comment above the function to document its purpose:
+// validateOperandVersions checks whether the expected OAuth operands are present or absent +// in the authentication ClusterOperator status based on the authentication mode. +// When requireMissing is true (OIDC mode), it verifies operands are unset. +// When requireMissing is false (IntegratedOAuth), it verifies both operands are present. func validateOperandVersions(ctx context.Context, cfgClient *configclient.Clientset, requireMissing bool) []error {
📜 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 (1)
test/e2e-oidc/external_oidc_test.go(2 hunks)
🔇 Additional comments (1)
test/e2e-oidc/external_oidc_test.go (1)
730-730: LGTM! Integration follows existing validation pattern.The integration of
validateOperandVersionsinto the validation chain is correct and consistent with the other validators. TherequireMissingparameter is properly forwarded to handle both OIDC and IntegratedOAuth modes.
|
/lgtm |
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
5b1d094 to
1b41173
Compare
|
@liouk Commits squashed. Canceling hold, but could use another LGTM when you've got a chance :). /hold cancel |
|
/retest-required |
|
@everettraven: The following test failed, say
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. |
|
The diff since last pre-merge test is only https://github.com/openshift/cluster-authentication-operator/compare/99d6050..1b4117 . So adding /verified: |
|
@xingxingxia: This PR has been marked as verified by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, 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 |
|
/jira refresh |
|
@xingxingxia: This pull request references Jira Issue OCPBUGS-62941, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
a0db9c2
into
openshift:master
|
@everettraven: Jira Issue Verification Checks: Jira Issue OCPBUGS-62941 Jira Issue OCPBUGS-62941 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
/jira backport 4.20 |
|
@xingxingxia: The following backport issues have been created: Queuing cherrypicks to the requested branches to be created after this PR merges: 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-robot: cannot checkout 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. |
|
/jira backport release-4.20 |
|
@xingxingxia: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: 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-robot: new pull request created: #802 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. |
|
Fix included in accepted release 4.21.0-0.nightly-2025-10-22-123727 |
Description
Updates the status controller options configured when setting up the OAuth API server operator to remove unset versions from the cluster operator status.
This should help prevent stuck upgrades when having configured the External OIDC authentication mode.