CNTRLPLANE-2554: Add ExternalOIDCWithUpstreamParity e2e tests#31028
CNTRLPLANE-2554: Add ExternalOIDCWithUpstreamParity e2e tests#31028ShazaAldawamneh wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2554 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughThe changes enhance Keycloak client HTTP status handling for group creation, refactor user creation to accept custom email addresses, and add a comprehensive OIDC feature test suite validating claim-based and CEL expression-based username/groups mapping with multiple validation rule scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2554 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/authentication/oidc.go`:
- Around line 451-467: Add a negative test case that verifies the
TokenValidationRuleTypeRequiredClaim rule fails when the required claim value
doesn't match: modify the test that sets provider.ClaimValidationRules (using
TokenValidationRuleTypeRequiredClaim and TokenRequiredClaim for "aud" →
"admin-cli" alongside the CEL rule) to also exercise a token whose aud is
different (or missing) and assert that authentication is rejected; ensure you
create or reuse a token generator to emit a token with a mismatched audience and
add an explicit assertion that the request fails due to the required-claim check
so both TokenValidationRuleTypeRequiredClaim and TokenValidationRuleTypeCEL
paths are covered.
- Around line 422-423: The test currently sets provider.Issuer.DiscoveryURL to
the same document as the issuer-derived endpoint so it won't detect regressions;
change provider.Issuer.DiscoveryURL to a clearly different path (e.g., a
"/custom-discovery/.well-known/openid-configuration" under idpUrl) so it does
not equal issuerURL + "/.well-known/openid-configuration", and update the test
to assert the cluster/user-agent actually used that configured DiscoveryURL (for
example by making the IDP serve distinct content at that custom path and
verifying the cluster fetched/consumed that distinct document). Apply the same
change where DiscoveryURL is set in the other block (lines around 489-500) so
both cases prove the feature gate is honored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 83cf6ac0-5157-42d9-9bad-1c53e3b3436b
📒 Files selected for processing (2)
test/extended/authentication/keycloak_client.gotest/extended/authentication/oidc.go
ShazaAldawamneh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2554 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/extended/authentication/oidc.go (2)
451-459:⚠️ Potential issue | 🟠 Major
RequiredClaimstill has no failing-path assertion.The suite validates CEL rejection but does not include a token case where
aud != admin-cli(or missing), so Line 454–458 enforcement could regress without being caught. Add a negative case specifically for theTokenValidationRuleTypeRequiredClaimpath.Also applies to: 503-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/oidc.go` around lines 451 - 459, The test adds provider.ClaimValidationRules including a TokenClaimValidationRule of Type TokenValidationRuleTypeRequiredClaim with a TokenRequiredClaim for claim "aud" == "admin-cli", but it lacks a negative test verifying rejection when aud is different or absent; add an explicit failing-case token test that uses the same provider.ClaimValidationRules (the TokenClaimValidationRule/TokenRequiredClaim combo) but sets the token's "aud" to a non-matching value (or omits it) and assert the OIDC token exchange/validation returns an error or denies access (the same assertion style used for CEL rejection). Apply the same negative-case addition pattern for the other occurrence around the TokenClaimValidationRule block at the later lines.
422-423:⚠️ Potential issue | 🟠 MajorDiscoveryURL test path is still equivalent to issuer-derived default, so parity isn’t actually proven.
Line 423 and Line 575 set
DiscoveryURLto the sameissuerURL + "/.well-known/openid-configuration"location. IfDiscoveryURLwere ignored, these tests could still pass. Use a distinct discovery endpoint and assert behavior that only succeeds when that configured URL is actually consumed.Also applies to: 575-576
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/oidc.go` around lines 422 - 423, The test currently sets provider.Issuer.DiscoveryURL to the same path derived from issuerURL so the custom discovery setting isn’t actually validated; change provider.Issuer.DiscoveryURL to a distinct path (e.g., issuerURL + "/custom-discovery" or "/realms/master/custom-well-known") and update the test harness to serve a different discovery document and JWKS at that distinct endpoint, then assert behavior that only succeeds when that configured URL is fetched (for example, ensure the client reads the custom discovery endpoints/JWKS and that token validation uses those values). Make the same change for the second occurrence (the other provider.Issuer.DiscoveryURL use) so both tests exercise a non-default discovery URL.
🧹 Nitpick comments (1)
Dockerfile.tests (1)
1-3: Prefer an explicit non-root runtime user for the test image.This image does not set
USER, so it inherits root unless the base image overrides it. For CI/test containers, it’s still better to declare a non-root user explicitly (or document why root is required) to avoid policy and hardening drift.Proposed hardening tweak
FROM registry.ci.openshift.org/ocp/4.22:tests -COPY openshift-tests /usr/bin/openshift-tests -RUN chmod +x /usr/bin/openshift-tests +COPY --chmod=0755 openshift-tests /usr/bin/openshift-tests +# Use a non-root UID if compatible with the test runtime/SCC +USER 1001🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.tests` around lines 1 - 3, The Dockerfile currently inherits root; add an explicit non-root runtime user and adjust ownership before switching: create a user/group (e.g., TEST_USER/TEST_GID), chown the copied binary (/usr/bin/openshift-tests) to that user, and set USER to run as that non-root account so tests run without root privileges; ensure any steps that require root (install/chown) remain before the USER directive and reference the symbols `/usr/bin/openshift-tests` and USER in the Dockerfile to locate where to apply changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/authentication/oidc.go`:
- Around line 451-459: The test adds provider.ClaimValidationRules including a
TokenClaimValidationRule of Type TokenValidationRuleTypeRequiredClaim with a
TokenRequiredClaim for claim "aud" == "admin-cli", but it lacks a negative test
verifying rejection when aud is different or absent; add an explicit
failing-case token test that uses the same provider.ClaimValidationRules (the
TokenClaimValidationRule/TokenRequiredClaim combo) but sets the token's "aud" to
a non-matching value (or omits it) and assert the OIDC token exchange/validation
returns an error or denies access (the same assertion style used for CEL
rejection). Apply the same negative-case addition pattern for the other
occurrence around the TokenClaimValidationRule block at the later lines.
- Around line 422-423: The test currently sets provider.Issuer.DiscoveryURL to
the same path derived from issuerURL so the custom discovery setting isn’t
actually validated; change provider.Issuer.DiscoveryURL to a distinct path
(e.g., issuerURL + "/custom-discovery" or "/realms/master/custom-well-known")
and update the test harness to serve a different discovery document and JWKS at
that distinct endpoint, then assert behavior that only succeeds when that
configured URL is fetched (for example, ensure the client reads the custom
discovery endpoints/JWKS and that token validation uses those values). Make the
same change for the second occurrence (the other provider.Issuer.DiscoveryURL
use) so both tests exercise a non-default discovery URL.
---
Nitpick comments:
In `@Dockerfile.tests`:
- Around line 1-3: The Dockerfile currently inherits root; add an explicit
non-root runtime user and adjust ownership before switching: create a user/group
(e.g., TEST_USER/TEST_GID), chown the copied binary (/usr/bin/openshift-tests)
to that user, and set USER to run as that non-root account so tests run without
root privileges; ensure any steps that require root (install/chown) remain
before the USER directive and reference the symbols `/usr/bin/openshift-tests`
and USER in the Dockerfile to locate where to apply changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44059da4-e46a-4d4b-996d-707300f1bc46
📒 Files selected for processing (2)
Dockerfile.teststest/extended/authentication/oidc.go
|
Scheduling required tests: |
| }) | ||
| }) | ||
|
|
||
| // TODO: this test relies on https://github.com/openshift/kubernetes/pull/2627 |
There was a problem hiding this comment.
Just a note - I'd like to go ahead and try to get this openshift/kubernetes PR in so we can run this test spec. It will help us get to the 5 test cases that we need for feature promotion.
There was a problem hiding this comment.
I will leave this comment as unresolved to get back to it when the kube PR get merged, I made changes to it today so hopefully will get merged soon
|
So far, I think it looks good, and it's great that you highlighted a problem with the I do agree with @everettraven - we should definitely run all tests where possible. I will hold off on LGTM for now until we see those tests in action. :) Is there a WIP PR or a place where the solution to |
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2554
branch
from
7a9276e to
5bc6b8a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ShazaAldawamneh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/extended/authentication/oidc.go (1)
422-423:⚠️ Potential issue | 🟠 MajorDiscoveryURL parity is not actually exercised.
DiscoveryURLis set to the same endpoint derived fromIssuer.URL, so a regression that ignoresDiscoveryURLwould still pass. The success assertion therefore does not prove custom discovery behavior.🔧 Suggested direction
- provider.Issuer.DiscoveryURL = fmt.Sprintf("%s/realms/master/.well-known/openid-configuration", idpUrl) + provider.Issuer.DiscoveryURL = fmt.Sprintf("%s/custom-discovery/.well-known/openid-configuration", idpUrl)Then serve distinct discovery content at that custom path (or otherwise verify fetches hit it), and keep assertions tied to that distinct behavior.
Also applies to: 482-483, 567-569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/oidc.go` around lines 422 - 423, The test currently sets provider.Issuer.DiscoveryURL to the same endpoint as Issuer.URL so it doesn't validate custom-discovery behavior; change the test to point provider.Issuer.DiscoveryURL to a different path than provider.Issuer.URL (e.g., add a distinct suffix) and arrange the test HTTP server to return different discovery JSON at that custom path (or instrument the handler to record requests and assert the discovery path was requested); update assertions to verify the distinct discovery content or that the custom DiscoveryURL handler was hit, referencing provider.Issuer.DiscoveryURL and provider.Issuer.URL in the test.
🧹 Nitpick comments (1)
test/extended/authentication/oidc.go (1)
690-742: Prefer skipping over keeping a large commented-out test body.Please convert this to a real
g.Itwithg.Skip(...)(including the dependency link), or move it to a tracked follow-up issue. Keeping it commented makes maintenance harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/oidc.go` around lines 690 - 742, The large commented test block should be converted into an actual skipped test so it's discoverable and trackable: replace the commented-out g.It("should reject invalid CEL expressions in admission", func() { ... }) block with a real g.It that immediately calls g.Skip("skipped until https://github.com/openshift/kubernetes/pull/2627") (or g.Skipf) and retains the original test body for future re-enabling; keep the same assertions and calls (configureOIDCAuthentication, provider.UserValidationRules, provider.ClaimValidationRules, provider.ClaimMappings.Username.Expression, provider.ClaimMappings.Groups.TokenClaimMapping.Expression, and o.Expect checks) so the test logic is preserved but skipped and thus not left commented out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/authentication/oidc.go`:
- Around line 492-493: The inline comment above the assertion incorrectly states
"both RequiredClaim and CEL types" while the test setup only configures CEL
claim-validation rules; update the comment to accurately reflect the configured
rule types (e.g., mention only CEL claim-validation rules) or modify the test
configuration to actually include a RequiredClaim rule if you intended to cover
both; locate the assertion chain using
WithTimeout(...).WithPolling(...).Should(o.Succeed()) to find the surrounding
comment and then either edit the comment text or add the missing RequiredClaim
rule to the claim validation configuration so the comment and test behavior
match.
---
Duplicate comments:
In `@test/extended/authentication/oidc.go`:
- Around line 422-423: The test currently sets provider.Issuer.DiscoveryURL to
the same endpoint as Issuer.URL so it doesn't validate custom-discovery
behavior; change the test to point provider.Issuer.DiscoveryURL to a different
path than provider.Issuer.URL (e.g., add a distinct suffix) and arrange the test
HTTP server to return different discovery JSON at that custom path (or
instrument the handler to record requests and assert the discovery path was
requested); update assertions to verify the distinct discovery content or that
the custom DiscoveryURL handler was hit, referencing
provider.Issuer.DiscoveryURL and provider.Issuer.URL in the test.
---
Nitpick comments:
In `@test/extended/authentication/oidc.go`:
- Around line 690-742: The large commented test block should be converted into
an actual skipped test so it's discoverable and trackable: replace the
commented-out g.It("should reject invalid CEL expressions in admission", func()
{ ... }) block with a real g.It that immediately calls g.Skip("skipped until
https://github.com/openshift/kubernetes/pull/2627") (or g.Skipf) and retains the
original test body for future re-enabling; keep the same assertions and calls
(configureOIDCAuthentication, provider.UserValidationRules,
provider.ClaimValidationRules, provider.ClaimMappings.Username.Expression,
provider.ClaimMappings.Groups.TokenClaimMapping.Expression, and o.Expect checks)
so the test logic is preserved but skipped and thus not left commented out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26c17cac-b394-464f-93a4-45508e361a33
📒 Files selected for processing (2)
test/extended/authentication/keycloak_client.gotest/extended/authentication/oidc.go
|
/retest |
|
Scheduling required tests: |
|
/retest-required |
|
Job Failure Risk Analysis for sha: 5bc6b8a
|
|
/retest-required |
|
@ShazaAldawamneh: 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. |
5 participants
This PR adds comprehensive end-to-end tests for the
ExternalOIDCWithUpstreamParityfeature gate, enabling the feature to be tested in TechPreview periodic jobs as a requirement for promotion.This replaces #30906 which is being closed due to the original author's departure.
New Tests Added
ExternalOIDCWithUpstreamParity
Supporting Changes
Modified
keycloak_client.go:CreateGroup()idempotent (handles existing groups)CreateUserWithEmail()for custom email testingNotes
g.By()for clear step documentation instead of immediately invoked functionsSummary by CodeRabbit
Release Notes