OCPBUGS-61432: fix(oidc): fix OIDC client secret lookup, validation, and condition cleanup #1067
Conversation
This commit addresses three issues related to OIDC authentication: 1. Fixed OIDC client secret lookup in oidcsetup controller to use the correct informer (configSecretsLister), namespace (openshift-config), and dynamic secret name from the Authentication CR, instead of hardcoded values. 2. Fixed secret revision validation to compare the TARGET secret (openshift-console/console-oauth-config) with the deployment annotation, following the same pattern as ConfigMap CA trust validation. This ensures proper verification of secret sync status. 3. Added condition cleanup in sync_v400 to properly clear the OIDCProviderTrustedAuthorityConfigGet degraded condition when authentication type changes from OIDC to non-OIDC (e.g., IntegratedOAuth). This prevents the Console Operator from remaining in a Degraded state indefinitely during rollback scenarios. Assisted-by: Claude Code 2.0.5, claude-sonnet-4-5@20250929 Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
devguyio
changed the title
devguyio
changed the title
devguyio
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@devguyio: This pull request references Jira Issue OCPBUGS-61432, 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
openshift-ci-robot
removed
the
jira/invalid-bug
|
@devguyio: This pull request references Jira Issue OCPBUGS-61432, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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
removed
the
do-not-merge/work-in-progress
|
/cc @jhadvig @everettraven @xiuwang @xingxingxia Previous fixes for this bug caused regression in OIDC before, can you please help review and validate? I tested it locally, verified the console works, and verified that the setting an external OIDC provider gets the console to do proper token exchange, but didn't yet reach a full login scenario due to some misconfiguration between EntraID and my cluster. However the token exchange did happen. I'll keep trying though. |
openshift-ci
bot
requested review from
everettraven,
jhadvig,
xingxingxia and
xiuwang
|
/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. |
|
@devguyio: 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. |
|
/payload-job e2e-aws-sno-external-oidc-configure |
|
@devguyio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c92a90e0-d04b-11f0-98a4-caad0f6d4aa6-0 |
|
/payload-job e2e-aws-sno-external-oidc-revertoauth |
|
@devguyio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d211ec80-d04b-11f0-8ccb-984ab60b5161-0 |
| // to compare its resource version with the deployment annotation | ||
| targetClientSecret, err := c.targetNSSecretsLister.Secrets(api.OpenShiftConsoleNamespace).Get("console-oauth-config") | ||
| if err != nil { | ||
| return false, "", err |
There was a problem hiding this comment.
Wrap the error with additional context?
There was a problem hiding this comment.
typically that's what I'd do, but this is the convention in this controller, to bubble up errors as-is and rely on the higher level to handle that 🤷🏽 you'll see that in many other places, e.g. line 278
There was a problem hiding this comment.
Just because it is convention, doesn't mean we can't do better now ;).
But this is a more of a nit so I won't block on it.
There was a problem hiding this comment.
Just because it is convention, doesn't mean we can't do better now ;).
I see your point, and I try to do "leave the code better than you found it". However, I wouldn't feel comfortable introducing such a change in convention without before:
- I get an understanding of the reasons behind that convention from the project/repo maintainers.
- I agree on a scope of the change upfront, because changing this single instance, IMHO, would leave the code in a more confusing state than how it was 😉 , with inconsistencies due to the two conventions existing.
Just my two cents, but agreed, it's a nit and I am not opposing the change, just wanna make sure that it's a change that has the project approvers buy-in, and that we've clarity around unifying that moving forward 😃 .
Thanks for the feedback!
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from one minor comment, this looks fine to me.
|
/hold I think this should go in after branch-cut |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devguyio, jhadvig 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
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
openshift-ci-robot
added
jira/invalid-bug
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-61432, 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. |
6 participants
This PR addresses three issues related to OIDC authentication:
Fixed OIDC client secret lookup in oidcsetup controller to use the correct informer (configSecretsLister), namespace (openshift-config), and dynamic secret name from the Authentication CR, instead of hardcoded values.
Fixed secret revision validation to compare the TARGET secret (openshift-console/console-oauth-config) with the deployment annotation, following the same pattern as ConfigMap CA trust validation. This ensures proper verification of secret sync status.
Added condition cleanup in sync_v400 to properly clear the OIDCProviderTrustedAuthorityConfigGet degraded condition when authentication type changes from OIDC to non-OIDC (e.g., IntegratedOAuth). This prevents the Console Operator from remaining in a Degraded state indefinitely during rollback scenarios.