OCPBUGS-56444: feat: Promote ExternalOIDCWithUIDAndExtraClaimMappings feature to GA for Hypershift #6025
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
do-not-merge/needs-area
area/api
|
Depends on #5840 merging /hold |
openshift-ci
bot
added
area/control-plane-operator
openshift-merge-robot
added
the
needs-rebase
everettraven
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
from
938e55b to
738fc19
Compare
openshift-merge-robot
removed
the
needs-rebase
everettraven
changed the title
ExternalOIDCWithUIDAndExtraClaimMappings feature to GAExternalOIDCWithUIDAndExtraClaimMappings feature to GA
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: 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
removed
the
do-not-merge/work-in-progress
|
This feature gate promotion will not pass the standard feature gate promotion criteria but is being proposed for promotion due to a critical need for this feature in HyperShift. Holding until we have the necessary discussions, with the appropriate stakeholders, to determine if the risks associated with promoting this feature to GA in HyperShift are acceptable. This also should not merge until openshift/api#2274 has merged. /hold |
everettraven
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
from
738fc19 to
1542ab3
Compare
openshift-merge-robot
added
the
needs-rebase
kevinrizza
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
from
1542ab3 to
9fe3b6a
Compare
openshift-merge-robot
removed
the
needs-rebase
kevinrizza
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
3 times, most recently
from
4acdefe to
bd55ec7
Compare
|
Pre-merge test the opening openshift/api#2274 , #6025 without techpreviewnoupgrade, no regression issue, and with byo oidc with uid and extra field function well. |
|
/retitle OCPBUGS-56444: Promote ExternalOIDCWithUIDAndExtraClaimMappings feature to GA for Hypershift |
|
/payload-job periodic-ci-openshift-hypershift-release-4.21-periodics-e2e-aws-external-oidc-techpreview |
|
@everettraven: 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/3406d740-b993-11f0-914f-cee57448abf8-0 |
everettraven
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
from
ae45352 to
78321d9
Compare
|
/payload-job periodic-ci-openshift-hypershift-release-4.21-periodics-e2e-aws-external-oidc |
|
@everettraven: 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/20541ff0-b9a2-11f0-8a7c-3e8a24cae93a-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/util/external_oidc.go (1)
44-50: Consider fixing the typo in the constant name.Line 45 defines
ExternalOIDCUIDExpressionSubfixbut the correct spelling is "Suffix" not "Subfix". Since this feature is being promoted to GA, this would be an appropriate time to correct the typo before broader adoption.Apply this diff to fix the typo:
- ExternalOIDCUIDExpressionSubfix = "-uidtest" + ExternalOIDCUIDExpressionSuffix = "-uidtest"And update the usage on line 132:
- Expression: fmt.Sprintf(`"%s" + claims.sub + "%s"`, ExternalOIDCUIDExpressionPrefix, ExternalOIDCUIDExpressionSubfix), + Expression: fmt.Sprintf(`"%s" + claims.sub + "%s"`, ExternalOIDCUIDExpressionPrefix, ExternalOIDCUIDExpressionSuffix),
📜 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)
test/e2e/external_oidc_test.go(0 hunks)test/e2e/util/external_oidc.go(2 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/external_oidc_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
🔇 Additional comments (2)
test/e2e/util/external_oidc.go (2)
131-143: LGTM! Feature gate removal is clean.The UID and Extra claim mappings are now unconditionally configured, which correctly aligns with the feature promotion to GA. The CEL expressions are syntactically correct:
- Line 132 properly quotes string literals and references claims.sub
- Line 137 wraps the string literal value in quotes
- Line 141 correctly uses an unquoted variable reference to claims.email
173-174: LGTM! Validation correctly updated.The validation logic now unconditionally expects UID and Extra mappings to be present, which is consistent with the feature being promoted to GA and the unconditional configuration in
GetAuthenticationConfig().
everettraven
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
from
78321d9 to
b4ee0cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/util/external_oidc.go (1)
131-143: LGTM! UID and Extra mappings are now unconditional.The GA promotion correctly removes feature gate checks and makes these mappings always active. The UID expression construction and Extra mappings are appropriate.
However, note the typo in the constant name at line 45:
ExternalOIDCUIDExpressionSubfixshould beExternalOIDCUIDExpressionSuffix. Consider fixing this in a follow-up to improve code quality.If you'd like to address the typo now, apply this diff:
- ExternalOIDCUIDExpressionSubfix = "-uidtest" + ExternalOIDCUIDExpressionSuffix = "-uidtest"And update the reference on line 132:
- Expression: fmt.Sprintf(`"%s" + claims.sub + "%s"`, ExternalOIDCUIDExpressionPrefix, ExternalOIDCUIDExpressionSubfix), + Expression: fmt.Sprintf(`"%s" + claims.sub + "%s"`, ExternalOIDCUIDExpressionPrefix, ExternalOIDCUIDExpressionSuffix),
📜 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)
test/e2e/external_oidc_test.go(0 hunks)test/e2e/util/external_oidc.go(2 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/external_oidc_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
🔇 Additional comments (1)
test/e2e/util/external_oidc.go (1)
173-174: LGTM! Test expectations correctly updated.The validation now properly expects UID and Extra claim mappings to always be present, which aligns with the unconditional configuration in
GetAuthenticationConfig. This is appropriate for a GA feature.
Signed-off-by: Bryce Palmer <[email protected]>
everettraven
force-pushed
the
feature/oidc-uid-extra-hcp-ga
branch
from
b4ee0cb to
8758069
Compare
|
/payload-job periodic-ci-openshift-hypershift-release-4.21-periodics-e2e-aws-external-oidc |
|
@xiuwang: 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/d2efba10-b9e9-11f0-9c46-2e2c14faaa35-0 |
|
/verified by @xiuwang |
openshift-ci-robot
added
the
verified
|
@xiuwang: 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. |
|
/retest-required |
|
/lgtm |
|
/retest-required |
|
/retest-required |
1 similar comment
|
/retest-required |
|
AKS e2e is having DNS throttling issues /override ci/prow/e2e-aks |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aks, ci/prow/e2e-aks-4-20 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. |
|
/retest-required |
|
@sjenning looks like the failing test here is also a quota-like issue?
|
|
@everettraven: 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. |
|
@everettraven: Jira Issue Verification Checks: Jira Issue OCPBUGS-56444 Jira Issue OCPBUGS-56444 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. |
9 participants
What this PR does / why we need it:
Promotes the
ExternalOIDCWithUIDAndExtraClaimMappingsfeature to GA.Feature was added as TPNU in #6073
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist