CNTRLPLANE-2589: Migrate test/e2e-oidc to OTE + bug fixes(imagestream,scc,Pathological Event Monitor)#945
Open
ropatil010 wants to merge 2 commits into
Open
CNTRLPLANE-2589: Migrate test/e2e-oidc to OTE + bug fixes(imagestream,scc,Pathological Event Monitor)#945ropatil010 wants to merge 2 commits into
ropatil010 wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@ropatil010: This pull request references CNTRLPLANE-2589 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 epic 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR wires a new external OIDC e2e suite into the test binary, splits the test entrypoint into a shared helper, updates shared wait and Keycloak helpers, and adds validation for OIDC configuration, OAuth state, and real Keycloak-backed authentication. ChangesExternal OIDC e2e suite
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Main as cmd/cluster-authentication-operator-tests-ext/main.go
participant Wrapper as test/e2e-oidc/external_oidc_test.go
participant Helper as testExternalOIDCWithKeycloak
participant Keycloak as Keycloak
participant Auth as authentication/cluster
participant KAS as kube-apiserver
Main->>Main: import test/e2e-oidc
Main->>Main: exclude [Disruptive] from serial suite
Wrapper->>Helper: delegate tt.Context() and tt
Helper->>Auth: update Authentication spec
Helper->>Keycloak: configure IdP and authenticate user
Helper->>KAS: validate rollout and submit SelfSubjectReview
Helper->>Auth: rollback original spec on cleanup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ropatil010
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![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)
test/e2e-oidc/external_oidc.go (1)
188-202: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTLS verification disabled +
Getwithout context.
InsecureSkipVerify: true(Lines 191-193) disables certificate validation for the issuer/JWKS fetch, andclient.Get(Lines 198, 218) ignores the ambientcontext.Context, so these calls won't honor test cancellation/timeouts. The IdP's CA is already available (thedefault-ingress-cert-derived bundle used at Line 510), so you can populateRootCAsinstead of skipping verification, and threadctxviahttp.NewRequestWithContext.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-oidc/external_oidc.go` around lines 188 - 202, The issuer/JWKS fetch in fetchIssuerJWKS is disabling TLS verification and using client.Get without any context, so update it to trust the IdP CA by configuring tls.Config.RootCAs from the existing default-ingress-cert-derived bundle instead of InsecureSkipVerify, and switch the OpenID/JWKS HTTP calls to http.NewRequestWithContext so they honor cancellation and timeouts. Apply the same pattern to both requests in fetchIssuerJWKS (and any related JWKS fetch helper) so the issuerURL lookups use the provided context and proper certificate validation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-oidc/external_oidc.go`:
- Line 98: The `t.(*testing.T)` cast in `testExternalOIDCWithKeycloak` will
panic when the test runs through Ginkgo because `g.GinkgoTB()` is not a
`*testing.T`. Update `test.WaitForNewKASRollout` to accept `testing.TB` instead
of `*testing.T`, then pass `t` through directly at each call site in
`external_oidc.go` so both standard Go tests and Ginkgo paths work safely.
---
Nitpick comments:
In `@test/e2e-oidc/external_oidc.go`:
- Around line 188-202: The issuer/JWKS fetch in fetchIssuerJWKS is disabling TLS
verification and using client.Get without any context, so update it to trust the
IdP CA by configuring tls.Config.RootCAs from the existing
default-ingress-cert-derived bundle instead of InsecureSkipVerify, and switch
the OpenID/JWKS HTTP calls to http.NewRequestWithContext so they honor
cancellation and timeouts. Apply the same pattern to both requests in
fetchIssuerJWKS (and any related JWKS fetch helper) so the issuerURL lookups use
the provided context and proper certificate validation.
🪄 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: Enterprise
Run ID: 3abe9ac9-8754-40f0-9aa0-b88e55a0553d
📒 Files selected for processing (3)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc.go (1)
188-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate the request context into
fetchIssuerJWKS.
fetchIssuerJWKStakes nocontext.Context, so bothclient.Getcalls run with an implicitcontext.Background()and cannot be cancelled with the test context. The only caller (testOIDCAuthentication, Line 845) already hasctxavailable. Thread it through and usehttp.NewRequestWithContext+client.Do(this also clears thenoctxlinter errors on Lines 198 and 218).As per coding guidelines: "When a function accepts or has access to a
context.Context, pass it through to downstream calls that accept one."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-oidc/external_oidc.go` around lines 188 - 234, `fetchIssuerJWKS` currently ignores the test context, so its HTTP requests cannot be cancelled and trigger the `noctx` issue. Update `fetchIssuerJWKS` to accept a `context.Context` from `testOIDCAuthentication`, then replace both `client.Get` calls with context-aware requests using `http.NewRequestWithContext` and `client.Do` so cancellation and timeouts propagate correctly.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-oidc/external_oidc.go`:
- Around line 189-193: The fetchIssuerJWKS client is still bypassing TLS
verification with InsecureSkipVerify, so update that http.Client setup to trust
the staged ingress CA instead. Reuse the CA data synced into openshift-config
for this IdP, build a RootCAs cert pool, and wire it through tls.Config in
fetchIssuerJWKS so the discovery/JWKS requests validate certificates normally.
Keep the fix localized to the fetchIssuerJWKS path and the related ingress CA
handling in external_oidc.go.
---
Nitpick comments:
In `@test/e2e-oidc/external_oidc.go`:
- Around line 188-234: `fetchIssuerJWKS` currently ignores the test context, so
its HTTP requests cannot be cancelled and trigger the `noctx` issue. Update
`fetchIssuerJWKS` to accept a `context.Context` from `testOIDCAuthentication`,
then replace both `client.Get` calls with context-aware requests using
`http.NewRequestWithContext` and `client.Do` so cancellation and timeouts
propagate correctly.
🪄 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: Enterprise
Run ID: d6c703ba-a314-4200-81b9-0083aa72deed
📒 Files selected for processing (4)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/waits.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cluster-authentication-operator-tests-ext/main.go
- test/e2e-oidc/external_oidc_test.go
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-oidc/external_oidc.go`:
- Around line 1207-1215: The rollback cleanup in the authentication Get/Update
flow dereferences auth.Name even when Get returns an error and auth may be nil.
Update the error handling in the block that calls
tc.configClient.ConfigV1().Authentications().Get so it does not reference
auth.Name on failure; use a stable identifier like the cluster authentication
name directly in the fmt.Errorf messages, and keep the existing update path
unchanged.
- Around line 353-357: The retry callback in PollUntilContextTimeout for the
auth ConfigMap lookup is intentionally swallowing cmErr2 by returning nil, which
triggers nilerr; update the closure in the related wait logic to make the
retry-on-error behavior explicit by either returning cmErr2 when the error
should stop polling or adding a targeted suppression with a clear reason if
retries on transient Get failures are intended.
- Around line 399-409: The oidcAuthResponse struct is unused and should be
removed to satisfy linting. Delete the unused type definition from
external_oidc.go, and make sure no other code references oidcAuthResponse before
removing it.
- Line 111: Remove the debug log in external_oidc.go that prints
kcClient.AdminURL() from the e2e setup. The issue is the t.Logf call in the
external OIDC test path exposes Keycloak admin URL/hostname details; replace it
by either deleting the log or changing it to a non-sensitive message that does
not include the URL, keeping the rest of the test flow unchanged.
- Around line 432-463: The fetchIssuerJWKS helper currently performs blocking
HTTP discovery and JWKS retrieval without honoring test cancellation. Update
fetchIssuerJWKS to accept a ctx parameter from the caller and use it for both
the OpenID configuration request and the JWKS request via NewRequestWithContext
plus client.Do, so the test can stop these fetches when the context is canceled.
Make sure the existing fetchIssuerJWKS call sites pass the test context through
consistently.
🪄 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: Enterprise
Run ID: 2a70907d-032d-458f-81b5-46984472c74c
📒 Files selected for processing (4)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/waits.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e-oidc/external_oidc_test.go
- test/library/waits.go
- cmd/cluster-authentication-operator-tests-ext/main.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/library/keycloakidp.go (1)
265-279: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
AuthenticatePassworddiscards caller's context instead of propagating it.The function creates its own timeout context from
context.Background()rather than accepting acontext.Contextparameter. The known caller (testOIDCAuthenticationintest/e2e-oidc/external_oidc.go) already holds a livectxwhen invokingkcClient.AuthenticatePassword(...)insidewait.PollUntilContextTimeout(ctx, ...), but that context is not passed through, so cancellation/deadline of the outer context won't propagate into this request.As per coding guidelines, "Propagate
context.Context- ... Never discard a context or substitutecontext.Background()/context.TODO()when a context is already available."♻️ Proposed fix
-func (kc *KeycloakClient) AuthenticatePassword(clientID, clientSecret, name, password string) error { +func (kc *KeycloakClient) AuthenticatePassword(ctx context.Context, clientID, clientSecret, name, password string) error { data := url.Values{ "username": []string{name}, "password": []string{password}, "grant_type": []string{"password"}, "client_id": []string{clientID}, "scope": []string{"openid"}, } if len(clientSecret) > 0 { data.Add("client_secret", clientSecret) } - reqCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + reqCtx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() authReq, err := http.NewRequestWithContext(reqCtx, http.MethodPost, kc.TokenURL(), bytes.NewBufferString(data.Encode()))This requires updating call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/library/keycloakidp.go` around lines 265 - 279, `AuthenticatePassword` is dropping the caller’s `context.Context` by building a new timeout from `context.Background()`, so cancellation and deadlines from `testOIDCAuthentication` and `wait.PollUntilContextTimeout` cannot flow through. Change `KeycloakClient.AuthenticatePassword` to accept a context parameter and use it for `http.NewRequestWithContext`, then update its call sites (including the one in `testOIDCAuthentication`) to pass the existing context instead of creating a new one.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-oidc/external_oidc.go`:
- Around line 170-171: The failure message in the auth configmap deletion check
uses the wrong variable, so the timeout error from waitErr is hidden. Update the
assertion in the external_oidc test to format waitErr in the message for the
configmap deletion wait, keeping the cmErr assertion unchanged and ensuring the
message reflects the variable checked by the waitForAuthConfigMap deletion
logic.
---
Outside diff comments:
In `@test/library/keycloakidp.go`:
- Around line 265-279: `AuthenticatePassword` is dropping the caller’s
`context.Context` by building a new timeout from `context.Background()`, so
cancellation and deadlines from `testOIDCAuthentication` and
`wait.PollUntilContextTimeout` cannot flow through. Change
`KeycloakClient.AuthenticatePassword` to accept a context parameter and use it
for `http.NewRequestWithContext`, then update its call sites (including the one
in `testOIDCAuthentication`) to pass the existing context instead of creating a
new one.
🪄 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: Enterprise
Run ID: 72eee9d3-5192-4de5-af16-92f7c11ed976
📒 Files selected for processing (5)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/keycloakidp.gotest/library/waits.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/library/waits.go
- test/e2e-oidc/external_oidc_test.go
- cmd/cluster-authentication-operator-tests-ext/main.go
ropatil010
force-pushed
the
test-e2e-oidc
branch
3 times, most recently
from
0d627ad to
16070ef
Compare
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
ropatil010
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ropatil010
changed the title
Contributor
Author
|
/assign @liouk @everettraven The failure bug cases are not caused bec of OTE migrations. The failure cases were existing before migration itself. |
ropatil010
changed the title
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Contributor
|
@ropatil010, |
Contributor
|
@ropatil010, |
Contributor
Author
|
Ignore the CI profile e2e-aws-operator-encryption-serial-ote-1of2, e2e-aws-operator-encryption-serial-ote-2of2 Tested 945 with 944 PR. CI logs from this PR: |
Contributor
Author
|
/test e2e-agnostic |
liouk
suggested changes
Jul 8, 2026
liouk
left a comment
liouk
left a comment
Member
There was a problem hiding this comment.
Overall I believe that the current suite structure w.r.t OIDC tests can be improved. My suggestion would be:
- remove
[OIDC]fromTestKeycloakAsOIDCPasswordGrantCheckAndGroupSyncandTestGitLabAsOIDCPasswordGrantCheck; these two tests are testing OIDC providers over IntegratedOAuth functionality, so the tag is misleading. - Drop
[OIDC]from the serial suite qualifier entirely; also remove the Disruptive cluster stability from it. - Keep
[OIDC]only for external OIDC e2e tests, i.e.TestExternalOIDCWithKeycloak; also, give these tests their own long-running suite, similar to what has been currently defined in the Makefile.
Contributor
Author
Thanks @liouk for the inputs. I guess we need to add new CI profile for this OIDC suite. Do let me know your inputs on this |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
@ropatil010: The following tests failed, say
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. |
…piration and PSA violations This PR migrates test/e2e-oidc to OpenShift Tests Extension (OTE) framework with Ginkgo v2 integration and includes critical bug fixes from PR openshift#859. ## OTE Migration Changes: 1. **Dual-Mode Pattern Implementation** - test/e2e-oidc/external_oidc.go: New Ginkgo test registration with complete OIDC test implementation - test/e2e-oidc/external_oidc_test.go: Simplified Go test wrapper for backward compatibility - cmd/cluster-authentication-operator-tests-ext/main.go: Add e2e-oidc import and fix serial suite qualifier 2. **Complete OIDC Authentication Test Workflow** - Implements full OIDC authentication test (not just preconditions) - OAuth/OIDC state validation - auth-config.json checks - JWT/JWKS verification with kube-apiserver authentication confirmation 3. **OTE Tags** - [OIDC][Serial][Disruptive] TestExternalOIDCWithKeycloak [Timeout:3h] ## Bug Fixes from PR openshift#859: 1. **Keycloak Token Expiration (test/e2e/keycloak.go)** - Problem: Test discarded kcClient with 30-minute token, created new client with 5-minute token - Result: 401 errors in long-running tests - Fix: Reuse kcClient from AddKeycloakIDP() with extended token lifetime - Impact: Eliminates intermittent TestKeycloakAsOIDCPasswordGrantCheckAndGroupSync failures 2. **PSA Audit Violations (test/library/client.go)** - Problem: WithPSaEnforcement() only set enforce label - Result: PSA audit/warn violations logged - Fix: Set all three PSA labels (enforce, audit, warn) for consistent behavior - Impact: Prevents Pod Security Admission audit log violations ## Library Enhancements: - test/library/imagestream.go: ImportImageToImageStream() for importing external images - test/library/idpdeployment.go: Configurable security context, deployPodWithImageStream() - test/library/keycloakidp.go: ImageStream integration, restricted security context - test/library/gitlabidp.go: ImageStream integration - test/library/waits.go: Enhanced wait utilities accepting testing.TB - test/library/client.go: TestNamespaceBuilder with PSA enforcement methods ## Vendor Dependencies: - Added github.com/openshift/client-go/image/* for ImageStream client support Co-Authored-By: Rohit Patil <ropatil@redhat.com>
This commit improves the OIDC test suite organization by making tag semantics more accurate and addressing code review feedback. Changes: - Remove [OIDC] tag from IntegratedOAuth IDP tests (Keycloak/GitLab) These tests use OIDC providers as identity sources for OpenShift's IntegratedOAuth, not external OIDC authentication. - Simplify External OIDC test tags: [OIDC] only (remove [Serial][Disruptive]) This test runs in its own dedicated suite via OTE framework. - Add new OTE suite: operator-oidc/serial Dedicated suite for external OIDC tests that bypass IntegratedOAuth. - Update serial suite qualifier to properly match [Serial] tests and non-disruptive operator/template/token tests. - Fix context usage in external_oidc.go poll function Use poll-derived context instead of outer context to ensure API calls are cancelled when poll times out. - Add nsCleanup() to idpdeployment.go cleanup function Fixes resource leak by properly cleaning up test namespaces. - Remove placeholder bug reference (OCPBUGS-XXXXX) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
needs-rebase
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi Team,
This PR is about:
Part 1: OTE Migration
1.1 Test Architecture Transformation: Dual-Mode Pattern Implementation:
1.2 Test Tags & Metadata
1.3 Comprehensive Test Coverage
The test suite now includes 6 complete sub-tests (previously only precondition checks):
- Verifies ConfigMap doesn't exist when authentication type is not OIDC
- Tests CAO auto-deletion when manually created
- Tests invalid CEL expressions for uid claim mapping
- Tests invalid CEL expressions for extras claim mapping
- Verifies admission controller rejects malformed expressions
- Tests degradation with invalid CA bundle
- Tests degradation with invalid issuer URL
- Confirms operator enters Degraded state appropriately
- Tests email claim with custom prefix (oidc-test:)
- Tests email claim with no prefix
- Tests sub claim with default issuer-based prefix
- Tests email claim with NoOpinion policy (auto-prefix)
- Verifies successful KAS rollout (when old architecture enabled)
- Full OAuth/OIDC state validation including JWT/JWKS verification
- Verifies ConfigMap exists when type is OIDC
- Tests CAO auto-overwrite when manually modified
- Tests configuration with non-existent claim
- Verifies rollout succeeds but authentication fails gracefully
Part 2: Critical Bug Fixes
2.1 Keycloak Token Expiration Bug
2.2 Pod Security Admission (PSA) Violations
Part 3: Library Enhancements
3.1 ImageStream Support (NEW - Addresses Known-Image-Checker Violations)
3.2 IDP Deployment Enhancements
3.3 Keycloak/GitLab Integration Updates
3.4 Wait Utilities Refactoring