CNTRLPLANE-3789: Add e2e tests for authentication component proxy - #31446
Open
tchap wants to merge 14 commits into
Open
CNTRLPLANE-3789: Add e2e tests for authentication component proxy#31446tchap wants to merge 14 commits into
tchap wants to merge 14 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
Skipping CI for Draft Pull Request. |
|
@tchap: This pull request references CNTRLPLANE-3789 which is a valid jira issue. 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
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughAdds a serial, disruptive authentication component-proxy suite with Squid, Keycloak/OIDC, certificate, network-policy, proxy-configuration, operator-status, and event-validation helpers covering routing, fallback, degradation, and unreachable IdP behavior. ChangesAuthentication component-proxy testing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant component_proxy_tests
participant authentication_cluster_operator
participant oauth_openshift
participant squid_proxy
participant keycloak_oidc
component_proxy_tests->>authentication_cluster_operator: Configure proxy and OpenID provider
authentication_cluster_operator->>oauth_openshift: Reconcile proxy and trusted CA settings
oauth_openshift->>squid_proxy: Send IdP request
squid_proxy->>keycloak_oidc: Forward OIDC request
component_proxy_tests->>authentication_cluster_operator: Verify status and warning events
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
test/extended/authentication/component_proxy_helpers.go (4)
237-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLint gate: use
net.JoinHostPortfor the proxy URLs.
golangci-lint(nosprintfhostport) errors on both lines, somake verifywill fail.As per coding guidelines: "Run `make verify` for lint and generated-file checks".♻️ Proposed fix
- serviceHost := fmt.Sprintf("%s.%s.svc.cluster.local", squidServiceName, namespace) - httpProxyURL = fmt.Sprintf("http://%s:%d", serviceHost, squidHTTPPort) - httpsProxyURL = fmt.Sprintf("https://%s:%d", serviceHost, squidHTTPSPort) + serviceHost := fmt.Sprintf("%s.%s.svc.cluster.local", squidServiceName, namespace) + httpProxyURL = "http://" + net.JoinHostPort(serviceHost, strconv.Itoa(int(squidHTTPPort))) + httpsProxyURL = "https://" + net.JoinHostPort(serviceHost, strconv.Itoa(int(squidHTTPSPort)))Add
netandstrconvimports.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 237 - 239, Add net and strconv imports, then update the httpProxyURL and httpsProxyURL construction to use net.JoinHostPort with strconv.Itoa for the respective ports instead of fmt.Sprintf host-port formatting; preserve the existing serviceHost and URL schemes.Sources: Coding guidelines, Linters/SAST tools
450-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
configMapNameparameter is ignored.The function always reads
componentProxyCAConfigMapName, so the caller-supplied name is silently discarded. Drop the parameter (and update the call site incomponent_proxy.goLine 128) to avoid implying it is honored.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 450 - 460, Remove the unused configMapName parameter from verifyTrustedCAConfigMapSynced and update its call site in component_proxy.go to match the new signature. Keep the function’s existing lookup of componentProxyCAConfigMapName unchanged.
527-556: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retrying the oauth/cluster update on conflict.
A single
Updatecan fail with a conflict and the IdP is then left registered for the rest of the run. Wrapping the get/modify/update inretry.RetryOnConflict(or a short poll) makes cleanup reliable in this disruptive suite.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 527 - 556, Update cleanIDPConfigByName to wrap the oauth/cluster get, identity-provider removal, and update sequence in retry.RetryOnConflict. Re-fetch the current OAuth configuration on each retry, recompute the matching provider index from that fresh object, preserve the no-match early-return behavior, and keep logging non-conflict failures while allowing conflicts to be retried.
711-716: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent cleanup contract: cleanups are executed by the
deferand also returned to the caller.On failure the deferred
removeResourcesruns the accumulated cleanups, yet the error returns still hand the same non-empty slice back, so a caller that also drains it performs double deletion. Returnnilon the error paths (as done at Line 729) now that the defer owns teardown.Also applies to: 736-783
🤖 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/extended/authentication/component_proxy_helpers.go` around lines 711 - 716, Update the failure return paths in the function using success and deferred removeResources so they return nil for the cleanup slice after the defer performs teardown. Apply this consistently to the error paths through the referenced range, matching the existing nil-return behavior at the later failure path; preserve cleanup returns on successful completion.test/extended/authentication/component_proxy.go (3)
342-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
corev1.EventTypeWarninginstead of the"Warning"literal.
corev1is already imported.🤖 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/extended/authentication/component_proxy.go` around lines 342 - 363, The event type check in the polling callback should use the imported corev1.EventTypeWarning constant instead of the literal "Warning"; update the condition within the IdPEndpointUnreachable event scan while preserving the existing timestamp and event matching behavior.
42-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer Ginkgo's
SpecContextovercontext.Background().Each helper creates an uncancellable root context, so a timed-out/interrupted spec keeps polling (up to 10 minutes per wait) and cleanup can't observe cancellation. Passing the spec context through gives the suite proper cancellation semantics.
As per path instructions: "context.Context for cancellation and timeouts".♻️ Proposed change
- g.It("should validate OIDC IdP through component proxy", func() { - testOIDCIdPThroughComponentProxy(oc, false) - }) + g.It("should validate OIDC IdP through component proxy", func(ctx g.SpecContext) { + testOIDCIdPThroughComponentProxy(ctx, oc, false) + })-func testOIDCIdPThroughComponentProxy(oc *exutil.CLI, withTrustedCA bool) { - ctx := context.Background() +func testOIDCIdPThroughComponentProxy(ctx context.Context, oc *exutil.CLI, withTrustedCA bool) {🤖 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/extended/authentication/component_proxy.go` around lines 42 - 47, Update testOIDCIdPThroughComponentProxy to accept a context.Context parameter supplied by the Ginkgo SpecContext instead of creating context.Background(). Pass that context through to waitForClusterOperatorAvailableNotProgressingNotDegraded and any other helper calls in the function so spec cancellation and timeouts propagate.Source: Path instructions
276-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegister the proxy restore cleanup immediately after obtaining it.
Between Line 277 and Line 285 the cleanup isn't registered, and the combined cleanup also swallows
proxyRestore's error. RegisteringproxyRestorewith its owng.DeferCleanupright away (Ginkgo runs them LIFO, so the IdP/secret cleanup registered afterwards still runs first) keeps the ordering while removing the manual wrapper.🤖 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/extended/authentication/component_proxy.go` around lines 276 - 294, The cleanup registration in the setup around saveAndRestoreProxyConfig must be split so proxyRestore is registered immediately after it is obtained, preserving its error return. Remove the later combined g.DeferCleanup wrapper’s proxyRestore call, while keeping the fake IdP and secret cleanup in its own deferred cleanup so Ginkgo’s LIFO ordering runs those removals first.test/extended/authentication/keycloak_client.go (1)
267-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ListClientsRawduplicatesListClients(Line 469) except for the decode target.Consider having
ListClientsdelegate, or drop the typed variant if the raw form now covers the callers, to avoid two parallel implementations drifting.🤖 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/extended/authentication/keycloak_client.go` around lines 267 - 311, Consolidate the duplicated client-listing implementation between ListClientsRaw and ListClients by making the typed variant delegate to the raw method, or remove it if no callers require it. Preserve the existing decode target and public behavior while leaving only one HTTP request and response-handling path to maintain.
🤖 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/extended/authentication/component_proxy_helpers.go`:
- Around line 633-651: The client discovery logic in the setup flow must fail
fast when either required client ID is missing. After the loop over clientList,
validate both adminClientID and passwdClientID before subsequent calls; return a
descriptive error identifying the missing discovered client instead of
proceeding with empty IDs.
- Around line 222-235: Update the UntilWithSync event handler around the
deployment readiness watch to safely inspect event types before accessing
Deployment fields. Handle watch.Error events by returning the event’s
error/status, ignore or safely handle Bookmark events, and only evaluate
ReadyReplicas after confirming the object is an *appsv1.Deployment, preventing
type-assertion panics while preserving the readiness condition.
- Around line 346-389: Update the cleanup flow around sync.OnceFunc and the
returned removal function so cleanup uses the removal function’s context rather
than the captured setup ctx. Detach cancellation with context.WithoutCancel and
apply an explicit timeout for the polling and stabilization work, ensuring
cleanup can complete after the spec context is canceled.
- Around line 597-631: Update deployKeycloakForProxy so every error path after
deployKeycloak succeeds executes setup.cleanups before returning, or returns the
partially initialized setup with the error so callers can perform cleanup. Apply
this to failures from admittedURLForRoute, keycloakClientFor, and
wait.PollUntilContextTimeout while preserving the existing wrapped error
messages.
- Around line 509-513: Update checkClusterOperatorStatus to return the Get error
instead of discarding it, preserving the failure details for one-shot callers
such as component_proxy.go. In waitForClusterOperatorStatus, handle and log
retriable errors while continuing to poll, while allowing non-retriable errors
to propagate; ensure no error return is ignored.
In `@test/extended/authentication/component_proxy.go`:
- Around line 369-374: Replace the one-shot Degraded=False assertion following
the IdP/proxy churn with a stable-window check using Consistently, or wait via
waitForClusterOperatorAvailableNotProgressingNotDegraded before asserting.
Update the check around checkClusterOperatorStatus while preserving the existing
failure context and ensuring transient rollout states do not fail the test.
In `@test/extended/authentication/keycloak_client.go`:
- Around line 228-244: Update UpdateClientAccessTokenTimeout to serialize
timeout with strconv.FormatInt before placing it in the client attributes map.
In UpdateClientRaw, deep-merge the nested attributes map with existing
attributes so unrelated entries are preserved while applying changes.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 237-239: Add net and strconv imports, then update the httpProxyURL
and httpsProxyURL construction to use net.JoinHostPort with strconv.Itoa for the
respective ports instead of fmt.Sprintf host-port formatting; preserve the
existing serviceHost and URL schemes.
- Around line 450-460: Remove the unused configMapName parameter from
verifyTrustedCAConfigMapSynced and update its call site in component_proxy.go to
match the new signature. Keep the function’s existing lookup of
componentProxyCAConfigMapName unchanged.
- Around line 527-556: Update cleanIDPConfigByName to wrap the oauth/cluster
get, identity-provider removal, and update sequence in retry.RetryOnConflict.
Re-fetch the current OAuth configuration on each retry, recompute the matching
provider index from that fresh object, preserve the no-match early-return
behavior, and keep logging non-conflict failures while allowing conflicts to be
retried.
- Around line 711-716: Update the failure return paths in the function using
success and deferred removeResources so they return nil for the cleanup slice
after the defer performs teardown. Apply this consistently to the error paths
through the referenced range, matching the existing nil-return behavior at the
later failure path; preserve cleanup returns on successful completion.
In `@test/extended/authentication/component_proxy.go`:
- Around line 342-363: The event type check in the polling callback should use
the imported corev1.EventTypeWarning constant instead of the literal "Warning";
update the condition within the IdPEndpointUnreachable event scan while
preserving the existing timestamp and event matching behavior.
- Around line 42-47: Update testOIDCIdPThroughComponentProxy to accept a
context.Context parameter supplied by the Ginkgo SpecContext instead of creating
context.Background(). Pass that context through to
waitForClusterOperatorAvailableNotProgressingNotDegraded and any other helper
calls in the function so spec cancellation and timeouts propagate.
- Around line 276-294: The cleanup registration in the setup around
saveAndRestoreProxyConfig must be split so proxyRestore is registered
immediately after it is obtained, preserving its error return. Remove the later
combined g.DeferCleanup wrapper’s proxyRestore call, while keeping the fake IdP
and secret cleanup in its own deferred cleanup so Ginkgo’s LIFO ordering runs
those removals first.
In `@test/extended/authentication/keycloak_client.go`:
- Around line 267-311: Consolidate the duplicated client-listing implementation
between ListClientsRaw and ListClients by making the typed variant delegate to
the raw method, or remove it if no callers require it. Preserve the existing
decode target and public behavior while leaving only one HTTP request and
response-handling path to maintain.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6955bc00-ea55-4b8a-b4dc-7f37563c4ce5
📒 Files selected for processing (5)
pkg/testsuites/standard_suites.gotest/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/crypto_helpers.gotest/extended/authentication/keycloak_client.go
tchap
force-pushed
the
auth-proxy-e2e
branch
2 times, most recently
from
July 29, 2026 13:46
c7cfe06 to
86006a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
test/extended/authentication/component_proxy_helpers.go (1)
300-344: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
getSquidProxyLogsSince'ssinceparameter is never used with a non-zero time.
waitForSquidProxyTrafficreads the full log, so traffic recorded before the proxy config was applied can satisfy the assertion. Passing the config-change timestamp through would make the check meaningful and remove the currently dead code path.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 300 - 344, Update waitForSquidProxyTraffic and its callers to capture the proxy configuration-application timestamp and pass it to getSquidProxyLogsSince, rather than calling getSquidProxyLogs with a zero time. Preserve polling behavior while ensuring each log read only considers traffic occurring after that timestamp.
🤖 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/extended/authentication/component_proxy_helpers.go`:
- Around line 730-810: Update addKeycloakOIDCIdPForProxy so failed paths handled
by its deferred removeResources cleanup return nil cleanup functions instead of
the already-executed cleanups slice; apply this consistently to each error
return after cleanup registration while preserving the successful return and
cleanup ownership semantics.
- Around line 254-298: Update deployProxyNetworkPolicies so keycloakPolicy
permits ingress only from proxyNamespace, removing the
policy-group.network.openshift.io/ingress NamespaceSelector unless the test
explicitly requires direct router access. Ensure the policy enforces operator
traffic through the proxy rather than relying solely on
waitForSquidProxyTraffic.
- Around line 476-481: Update waitForOperatorToPickUpChanges so
WaitForOperatorProgressingTrue is best-effort rather than a required failure
condition. Rely on generation/observedGeneration synchronization and
waitForClusterOperatorAvailableNotProgressingNotDegraded to confirm the change
was picked up and settled, while preserving the existing error handling for the
final settle check.
- Around line 246-248: Update the proxy URL construction in the relevant helper
to use net.JoinHostPort with serviceHost and strconv.Itoa for both squidHTTPPort
and squidHTTPSPort, replacing the fmt.Sprintf host-port formatting. Add the
required net and strconv imports while preserving the existing HTTP and HTTPS
schemes.
- Around line 812-826: Update addKeycloakIDPForProxy to preserve cleanup
functions from a partially populated setup when deployKeycloakForProxy returns
an error: initialize cleanups from setup before handling the error, then return
those cleanups instead of nil. Keep the existing error return behavior and
subsequent OIDC cleanup handling unchanged.
In `@test/extended/authentication/component_proxy.go`:
- Around line 341-370: Move the startTime assignment in the authentication proxy
test to before the fake IdP is written to oauth/cluster, so
IdPEndpointUnreachable events emitted during the OAuth update are included in
the wait.PollUntilContextTimeout lookup. Preserve the existing event timestamp
checks and warning-event validation.
- Around line 86-88: Update both cleanup callbacks passed to idpCleanupWrapper
in the affected test to capture the error returned by removeResources and log it
through the test/spec logger, while preserving the existing cleanup arguments
and wrapper behavior. Ensure neither call site silently discards the teardown
error.
- Around line 188-190: Update the OAuth server proxy verification call in
verifyOAuthServerDeploymentProxyConfig usage to assert that proxy environment
variables, including NO_PROXY, are absent rather than passing empty strings
through the expected-value path. Adjust the helper or its invocation so empty
expectations trigger an explicit emptiness check and cannot accept arbitrary
NO_PROXY values.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 300-344: Update waitForSquidProxyTraffic and its callers to
capture the proxy configuration-application timestamp and pass it to
getSquidProxyLogsSince, rather than calling getSquidProxyLogs with a zero time.
Preserve polling behavior while ensuring each log read only considers traffic
occurring after that timestamp.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7839b38e-48b3-4eb0-9d99-cb21a245d49a
📒 Files selected for processing (5)
pkg/testsuites/standard_suites.gotest/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/crypto_helpers.gotest/extended/authentication/keycloak_client.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/testsuites/standard_suites.go
- test/extended/authentication/crypto_helpers.go
- test/extended/authentication/keycloak_client.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/extended/authentication/component_proxy_helpers.go (2)
545-574: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the IdP removal with
slices.DeleteFunc.The manual index walk plus re-slice/append works but aliases the original backing array; a single
slices.DeleteFuncon the copy is clearer.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 545 - 574, Update cleanIDPConfigByName to remove the manual idpIndex search and re-slice/append logic. Copy config.Spec.IdentityProviders, then use slices.DeleteFunc to remove entries whose Name matches idpName, and assign the result back before updating the OAuth configuration.
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
componentProxyTestLabels()for thee2e-testlabel.Duplicated literal here (and in the Squid ConfigMap/Secret/Deployment/Service, which carry no test labels at all — inconsistent for cleanup/debug tooling).
🤖 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/extended/authentication/component_proxy_helpers.go` around lines 68 - 77, Update the namespace metadata and the Squid ConfigMap, Secret, Deployment, and Service definitions to reuse componentProxyTestLabels() instead of duplicating the e2e-test label or omitting test labels. Preserve the existing Kubernetes labels while ensuring all component-proxy resources receive the shared test labels for cleanup and debugging.
🤖 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/extended/authentication/component_proxy_helpers.go`:
- Around line 427-431: Handle an empty expectedNoProxy in the NO_PROXY
validation within the helper at
test/extended/authentication/component_proxy_helpers.go lines 427-431 by
asserting the NO_PROXY environment variable is absent instead of constructing a
set and performing the superset comparison; retain the existing comparison for
non-empty expectations. Keep the empty expectations in
test/extended/authentication/component_proxy.go lines 188-190 unchanged so proxy
removal validates that no proxy environment variables remain.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 545-574: Update cleanIDPConfigByName to remove the manual idpIndex
search and re-slice/append logic. Copy config.Spec.IdentityProviders, then use
slices.DeleteFunc to remove entries whose Name matches idpName, and assign the
result back before updating the OAuth configuration.
- Around line 68-77: Update the namespace metadata and the Squid ConfigMap,
Secret, Deployment, and Service definitions to reuse componentProxyTestLabels()
instead of duplicating the e2e-test label or omitting test labels. Preserve the
existing Kubernetes labels while ensuring all component-proxy resources receive
the shared test labels for cleanup and debugging.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 89e85667-42b9-41de-bf68-4d0823bf6d25
📒 Files selected for processing (5)
pkg/testsuites/standard_suites.gotest/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/crypto_helpers.gotest/extended/authentication/keycloak_client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/testsuites/standard_suites.go
- test/extended/authentication/keycloak_client.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/extended/authentication/component_proxy_helpers.go (2)
246-248: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
make verifywill still fail here: usenet.JoinHostPort.golangci-lint
nosprintfhostporterrors on both Lines 247 and 248.🛠️ Proposed fix
serviceHost := fmt.Sprintf("%s.%s.svc.cluster.local", squidServiceName, namespace) - httpProxyURL = fmt.Sprintf("http://%s:%d", serviceHost, squidHTTPPort) - httpsProxyURL = fmt.Sprintf("https://%s:%d", serviceHost, squidHTTPSPort) + httpProxyURL = "http://" + net.JoinHostPort(serviceHost, strconv.Itoa(int(squidHTTPPort))) + httpsProxyURL = "https://" + net.JoinHostPort(serviceHost, strconv.Itoa(int(squidHTTPSPort)))Add
"net"and"strconv"imports.As per coding guidelines: "Run
make verifyfor lint and generated-file checks".🤖 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/extended/authentication/component_proxy_helpers.go` around lines 246 - 248, Update the proxy URL construction in the helper containing serviceHost to use net.JoinHostPort with strconv.Itoa for both squidHTTPPort and squidHTTPSPort instead of fmt.Sprintf host-port formatting; add the required net and strconv imports and preserve the existing HTTP/HTTPS schemes and host values.Sources: Coding guidelines, Linters/SAST tools
266-286: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNetworkPolicy still permits non-proxied operator→route traffic.
The second peer allows the ingress-controller namespace, and the operator reaches Keycloak via its route (through the router), so direct non-proxied requests remain allowed. The test therefore relies solely on
waitForSquidProxyTrafficfor evidence — drop the ingress peer or add a comment explaining why it must stay.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 266 - 286, The NetworkPolicy ingress rules shown in the helper still allow operator-to-route traffic through the ingress-controller namespace, undermining the proxy-only test. Update the Ingress peer list in the relevant policy construction to remove the `"policy-group.network.openshift.io/ingress"` namespace selector, unless this peer is required; if retained, document its necessity inline and ensure the test explicitly validates proxy traffic.test/extended/authentication/component_proxy.go (1)
86-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCleanup still discards
removeResourceserrors.Both
idpCleanupWrappercall sites (Lines 87 and 115) drop the returned error, so a failed teardown in a[Disruptive]suite is invisible.🛡️ Proposed fix
g.DeferCleanup(idpCleanupWrapper(func() { - removeResources(ctx, kcSetup.cleanups...) + if err := removeResources(ctx, kcSetup.cleanups...); err != nil { + g.GinkgoWriter.Printf("cleanup: failed to remove keycloak resources: %v\n", err) + } }))Also applies to Lines 114-116, and to
testFallbackOnProxyRemoval(Lines 169-171) andtestDegradedOnBadProxyURL(Lines 224-226).As per path instructions: "Never ignore error returns".
🤖 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/extended/authentication/component_proxy.go` around lines 86 - 88, Update every idpCleanupWrapper cleanup callback in testFallbackOnProxyRemoval, testDegradedOnBadProxyURL, and the other two call sites to propagate the error returned by removeResources instead of discarding it. Ensure each deferred cleanup returns the teardown error so failed cleanup remains visible, consistent with the path requirement to never ignore error returns.Source: Path instructions
🧹 Nitpick comments (2)
test/extended/authentication/component_proxy_helpers.go (2)
576-579: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup update is not conflict-tolerant.
A single
Updateonoauth/clustercan fail with a conflict, leaving the test IdP behind for the rest of the run. Wrapping inretry.RetryOnConflict(re-fetching inside) makes teardown reliable.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 576 - 579, Make the cleanup update in the oauth/cluster teardown conflict-tolerant by wrapping it with retry.RetryOnConflict and re-fetching the latest configuration inside each retry before assigning providers and updating via oauthClient. Preserve the existing cleanup failure logging after retries are exhausted.
424-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the observed values on mismatch.
On timeout the caller only sees
context deadline exceeded; printing the actualHTTP_PROXY/HTTPS_PROXY/NO_PROXYand volume state on the last iteration would make failures diagnosable without re-running the suite.🤖 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/extended/authentication/component_proxy_helpers.go` around lines 424 - 439, The proxy environment validation should log the observed HTTP_PROXY, HTTPS_PROXY, NO_PROXY, and trusted CA volume state when a mismatch occurs, especially on the final timeout iteration. Update the surrounding helper to emit these actual values before returning false, while preserving the existing validation and return behavior.
🤖 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.
Duplicate comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 246-248: Update the proxy URL construction in the helper
containing serviceHost to use net.JoinHostPort with strconv.Itoa for both
squidHTTPPort and squidHTTPSPort instead of fmt.Sprintf host-port formatting;
add the required net and strconv imports and preserve the existing HTTP/HTTPS
schemes and host values.
- Around line 266-286: The NetworkPolicy ingress rules shown in the helper still
allow operator-to-route traffic through the ingress-controller namespace,
undermining the proxy-only test. Update the Ingress peer list in the relevant
policy construction to remove the `"policy-group.network.openshift.io/ingress"`
namespace selector, unless this peer is required; if retained, document its
necessity inline and ensure the test explicitly validates proxy traffic.
In `@test/extended/authentication/component_proxy.go`:
- Around line 86-88: Update every idpCleanupWrapper cleanup callback in
testFallbackOnProxyRemoval, testDegradedOnBadProxyURL, and the other two call
sites to propagate the error returned by removeResources instead of discarding
it. Ensure each deferred cleanup returns the teardown error so failed cleanup
remains visible, consistent with the path requirement to never ignore error
returns.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 576-579: Make the cleanup update in the oauth/cluster teardown
conflict-tolerant by wrapping it with retry.RetryOnConflict and re-fetching the
latest configuration inside each retry before assigning providers and updating
via oauthClient. Preserve the existing cleanup failure logging after retries are
exhausted.
- Around line 424-439: The proxy environment validation should log the observed
HTTP_PROXY, HTTPS_PROXY, NO_PROXY, and trusted CA volume state when a mismatch
occurs, especially on the final timeout iteration. Update the surrounding helper
to emit these actual values before returning false, while preserving the
existing validation and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eead1351-f928-457d-a419-cf23dfe54cb5
📒 Files selected for processing (5)
pkg/testsuites/standard_suites.gotest/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/crypto_helpers.gotest/extended/authentication/keycloak_client.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/extended/authentication/crypto_helpers.go
- pkg/testsuites/standard_suites.go
- test/extended/authentication/keycloak_client.go
Contributor
Author
|
I am going to resolve all remaining CR review comments as these were decided to be skipped. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap 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 |
tchap
force-pushed
the
auth-proxy-e2e
branch
9 times, most recently
from
July 30, 2026 15:39
092cbd3 to
3fe60fa
Compare
tchap
commented
Jul 30, 2026
tchap
force-pushed
the
auth-proxy-e2e
branch
2 times, most recently
from
July 30, 2026 17:18
b14d5ed to
0c941b2
Compare
…sions Add helper functions for deploying a Squid forward proxy, managing proxy-scoped network policies, configuring component-scoped proxy on the Authentication CR, and verifying OAuth server deployment alignment. Extend the keycloak client with group/audience mapper creation, client configuration, and client lookup by clientID. Add crypto helpers for generating self-signed CA and server certificates used by the Squid proxy's HTTPS listener. Refactor keycloak deployment cleanup to rely on namespace cascading, reducing cleanup API calls from 6 to 2 (namespace + CA configmap in openshift-config). All deploy helpers self-clean on error and return nil cleanups, preventing resource leaks when callers use Expect before DeferCleanup.
- Use Keycloak service URL as OIDC issuer instead of route URL so the network policy structurally enforces proxy usage (only the proxy namespace can reach Keycloak pods directly). - Remove ingress router rule from network policy since the service URL bypasses the router. - Use service CA for IdP CA configmap instead of ingress CA, matching the service serving cert signer. - Replace saveAndRestoreProxyConfig and saveAndRestoreIdPs with a single saveAndRestoreAuthState that snapshots both the Authentication operator CR and oauth/cluster, restoring both on cleanup and waiting for stabilization only if changes were made. - Remove ListClientsRaw in favor of typed ListClients with RedirectURIs field; fix clientId JSON tag; simplify UpdateClientRaw to flat merge. - Add must prefix to crypto helpers to make panic-on-error explicit. - Remove dead code: idpCleanupWrapper, cleanIdentityProviderByName, resetComponentProxyState.
Deploy helpers no longer self-clean on error. Instead they always return accumulated cleanups, and callers register DeferCleanup before calling Expect. This prevents resource leaks when BeforeEach aborts mid-setup.
Fixes nosprintfhostport linter warning.
tchap
marked this pull request as draft
July 31, 2026 11:42
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Replace waitForClusterOperatorAvailableNotProgressingNotDegraded and its supporting functions with the existing operator.WaitForOperatorsToSettle utility. This checks all operators for the same three conditions (Available, NotProgressing, NotDegraded), which is appropriate for serial conformance tests.
tchap
marked this pull request as ready for review
July 31, 2026 12:03
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
everettraven
left a comment
everettraven
left a comment
Contributor
There was a problem hiding this comment.
Just a few more comments. Other than these, this looks fine to me.
tchap
marked this pull request as draft
July 31, 2026 13:56
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Replace scattered DeferCleanup calls in BeforeEach with a shared cleanups slice and explicit AfterEach. Drop the network policy cleanup since the keycloak namespace deletion cascades to it.
Replace custom crypto_helpers.go with library-go's MakeSelfSignedCAConfigForDuration and CA.MakeServerCert. This reuses existing, well-tested crypto utilities instead of maintaining a separate implementation.
The network policy structurally enforces proxy usage — only the proxy namespace can reach Keycloak pods. If the operator successfully discovers the OIDC issuer, it must have gone through the proxy. The log check was redundant.
tchap
marked this pull request as ready for review
July 31, 2026 14:47
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
Scheduling required tests: |
|
|
||
| // Use the in-cluster service URL as the OIDC issuer so the operator must | ||
| // go through the proxy to reach Keycloak (enforced by network policy). | ||
| serviceHost := fmt.Sprintf("%s.%s.svc", keycloakResourceName, namespace) |
Contributor
There was a problem hiding this comment.
We cannot use .svc for the issuerURL as this blocks oauth-server from authenticating with Keycloak through the proxy.
NO_PROXY contains .svc as a default value.
@everettraven what are your thoughts on this?
Contributor
|
@tchap: 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. |
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.
Add 5 Serial/Disruptive test specs gated behind the
AuthenticationComponentProxy feature gate:
Register the openshift/auth/component-proxy suite in standard_suites.go.
Summary by CodeRabbit
openshift/auth/component-proxywith a 120-minute timeout and single-threaded execution.