CNTRLPLANE-3789: Add e2e tests for authentication component proxy - #31446
CNTRLPLANE-3789: Add e2e tests for authentication component proxy#31446tchap wants to merge 8 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
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 Authentication component-proxy suite. The tests deploy Squid and Keycloak, validate proxied OIDC discovery and trusted-CA wiring, verify proxy traffic, and check fallback after removing ChangesAuthentication component proxy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentProxySuite
participant AuthenticationOperator
participant OAuthServer
participant Squid
participant Keycloak
ComponentProxySuite->>AuthenticationOperator: configure component proxy
AuthenticationOperator->>OAuthServer: apply proxy and trusted CA wiring
OAuthServer->>Keycloak: request OIDC discovery
OAuthServer->>Squid: send proxied traffic
Squid->>Keycloak: forward discovery request
ComponentProxySuite->>Squid: verify proxy traffic
Possibly related PRs
Suggested reviewers: 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
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
|
I am going to resolve all remaining CR review comments as these were decided to be skipped. |
092cbd3 to
3fe60fa
Compare
0c941b2 to
07369cb
Compare
|
New changes are detected. LGTM label has been removed. |
|
Scheduling required tests: |
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
1 similar comment
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
Add end-to-end tests that validate the Authentication operator's component-scoped proxy support. Three test cases cover: - OIDC IdP discovery through an HTTP forward proxy - OIDC IdP discovery through an HTTPS forward proxy with trustedCA - Fallback behavior when spec.proxy is removed (deletes the proxy namespace to prove the operator no longer depends on it) The tests deploy a Squid forward proxy and Keycloak in ephemeral namespaces, configure the Authentication operator to use the proxy, register Keycloak as an OIDC IdP, and verify: - The operator discovers the IdP and stabilizes - The OAuth server deployment has the correct proxy env vars - Proxy traffic from the operator appears in structured Squid access logs (custom logformat for precise source IP matching) - The trustedCA ConfigMap is synced (HTTPS proxy case) Test infrastructure includes helpers for deploying Squid with self-signed TLS (using library-go/pkg/crypto), deploying and configuring Keycloak, saving/restoring Authentication and OAuth state, and structured proxy log verification. Extends the keycloak client with methods for client secret regeneration, access token timeout configuration, and raw client updates needed by the proxy test setup.
|
Scheduling required tests: |
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
|
@tchap: The following test 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 247fdd6
New tests seen in this PR at sha: 247fdd6
|
Make testFallbackOnProxyRemoval robust by using deleteNamespaceSync which issues a foreground-propagation delete and polls until the namespace is fully gone, eliminating the race where the Squid proxy could still be serving traffic when the operator reconciles. Extract createTrustedCAConfigMap helper to reduce inline resource creation in the test function.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Deep-merge the attributes map in UpdateClientRaw so setting a single attribute like access.token.lifespan does not wipe unrelated client attributes. Return actual volume/mount state from trustedCAVolumeState so the diagnostic log shows what was observed rather than inferring it from the expected value.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@tchap: This pull request references CNTRLPLANE-3789 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 either version "5.0." or "openshift-5.0.", but it targets "openshift-5.1" instead. 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.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/extended/authentication/component_proxy_helpers.go (2)
369-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog-based proxy verification depends on the Squid
logformatstring.
waitForProxyTrafficFromparses positional fields that theproxytestlogformatdirective at Line 194 produces:%rm %ru %>a %Ss %>Hs. The parser requiresparts[0] == "CONNECT"andparts[3] == "TCP_TUNNEL". That contract is correct for the current format only. Add a short comment at Line 194 that links the format to this parser, so a later format change does not silently make the check unreachable.🤖 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 369 - 386, The proxy traffic parser in waitForProxyTrafficFrom depends on the positional fields emitted by the proxytest logformat directive. Add a brief comment next to that directive documenting its `%rm %ru %>a %Ss %>Hs` field order and linking it to waitForProxyTrafficFrom; do not alter the parser or log format.
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
serviceDNSvalue instead of building the host twice.Line 174 and Line 328 build the same host string. The certificate SAN and the proxy URLs must always match. One variable removes that risk.
♻️ Proposed refactor
- 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))) + httpProxyURL = "http://" + net.JoinHostPort(serviceDNS, strconv.Itoa(int(squidHTTPPort))) + httpsProxyURL = "https://" + net.JoinHostPort(serviceDNS, strconv.Itoa(int(squidHTTPSPort)))Also applies to: 328-330
🤖 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` at line 174, Reuse the existing serviceDNS value when constructing the certificate SAN and proxy URLs, rather than rebuilding the host string at the later authentication setup location. Update the relevant helper logic around serviceDNS so both consumers use the same value and remain consistent.test/extended/authentication/component_proxy.go (1)
31-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
BeforeEachdeploys Squid and Keycloak for every spec.Each of the three specs pays the full Keycloak plus Squid deployment cost, and the
AfterEachdeletes both. That is correct and isolated, but it triples the runtime of a serial suite. If the specs can share the infrastructure,BeforeAllinside anOrderedcontainer with per-spec state restore is faster. Keep the current form if isolation is the priority.🤖 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 31 - 59, Consider restructuring the authentication proxy specs into an Ordered container with BeforeAll infrastructure setup and AfterAll cleanup, while retaining per-spec authentication state restoration in BeforeEach/AfterEach. Share the Squid proxy and Keycloak deployments across specs only if their state can be reliably restored; otherwise preserve the current isolated BeforeEach setup.test/extended/authentication/operator_status_helpers.go (1)
13-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe settle check can pass before the operator observes the change.
WaitForOperatorProgressingTrueis now best-effort. If the operator has not yet started reconciling when this function runs,WaitForOperatorsToSettlesees the pre-change state and returns immediately. The callers incomponent_proxy.gothen continue against stale state. The downstream polls inverifyOAuthServerDeploymentProxyConfigandwaitForProxyTrafficFromabsorb this today, so no test asserts on the stale state directly. To make the helper self-sufficient, comparestatus.observedGenerationwithmetadata.generationonauthentication/clusterbefore the settle call.🤖 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/operator_status_helpers.go` around lines 13 - 20, The waitForOperatorToPickUpChanges helper must verify that authentication/cluster status.observedGeneration matches metadata.generation before calling operator.WaitForOperatorsToSettle. Add a poll or equivalent check using the existing context and AdminConfigClient, preserving the current best-effort Progressing handling while preventing the settle check from returning on stale pre-change state.test/extended/authentication/keycloak_client.go (2)
244-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
UpdateClientRawmutates the caller-suppliedchangesmap.Line 248 writes the merged attributes back into
changes. The caller then holds a map that contains the server state. The current caller passes a literal, so there is no defect today. A local copy keeps the function free of side effects.♻️ Proposed refactor
if changesAttrs, ok := changes["attributes"].(map[string]any); ok { if existingAttrs, ok := existing["attributes"].(map[string]any); ok { maps.Copy(existingAttrs, changesAttrs) - changes["attributes"] = existingAttrs + changes = maps.Clone(changes) + changes["attributes"] = existingAttrs } }🤖 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 244 - 252, Update UpdateClientRaw to avoid mutating the caller-supplied changes map when deep-merging attributes: copy the incoming changes data into a local map before assigning merged existing attributes, then merge that local map into existing. Preserve the current deep-merge behavior while ensuring the original changes map remains unchanged.
217-225: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDecode the credential response into a typed struct.
Keycloak 25.0 returns a
CredentialRepresentation, which can include non-string fields such ascreatedDateandtemporary. Unmarshalling it intomap[string]stringcan fail and causedeployKeycloakForProxyto retry for up to five minutes. Decode onlyvaluewith a typed struct and reject an empty value.🤖 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 217 - 225, Update the credential response handling in deployKeycloakForProxy to unmarshal into a typed struct containing only the value field, rather than map[string]string, so non-string CredentialRepresentation fields are ignored. Validate that the decoded value is non-empty and return the existing retrieval error when it is missing or empty.
🤖 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.go`:
- Line 18: Register the component-proxy suite in standard_suites.go under
openshift/auth/component-proxy, then update the Describe declaration in the
suite to use the matching suite label and include [Disruptive]. Preserve the
existing authentication and conformance classification labels.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 369-386: The proxy traffic parser in waitForProxyTrafficFrom
depends on the positional fields emitted by the proxytest logformat directive.
Add a brief comment next to that directive documenting its `%rm %ru %>a %Ss
%>Hs` field order and linking it to waitForProxyTrafficFrom; do not alter the
parser or log format.
- Line 174: Reuse the existing serviceDNS value when constructing the
certificate SAN and proxy URLs, rather than rebuilding the host string at the
later authentication setup location. Update the relevant helper logic around
serviceDNS so both consumers use the same value and remain consistent.
In `@test/extended/authentication/component_proxy.go`:
- Around line 31-59: Consider restructuring the authentication proxy specs into
an Ordered container with BeforeAll infrastructure setup and AfterAll cleanup,
while retaining per-spec authentication state restoration in
BeforeEach/AfterEach. Share the Squid proxy and Keycloak deployments across
specs only if their state can be reliably restored; otherwise preserve the
current isolated BeforeEach setup.
In `@test/extended/authentication/keycloak_client.go`:
- Around line 244-252: Update UpdateClientRaw to avoid mutating the
caller-supplied changes map when deep-merging attributes: copy the incoming
changes data into a local map before assigning merged existing attributes, then
merge that local map into existing. Preserve the current deep-merge behavior
while ensuring the original changes map remains unchanged.
- Around line 217-225: Update the credential response handling in
deployKeycloakForProxy to unmarshal into a typed struct containing only the
value field, rather than map[string]string, so non-string
CredentialRepresentation fields are ignored. Validate that the decoded value is
non-empty and return the existing retrieval error when it is missing or empty.
In `@test/extended/authentication/operator_status_helpers.go`:
- Around line 13-20: The waitForOperatorToPickUpChanges helper must verify that
authentication/cluster status.observedGeneration matches metadata.generation
before calling operator.WaitForOperatorsToSettle. Add a poll or equivalent check
using the existing context and AdminConfigClient, preserving the current
best-effort Progressing handling while preventing the settle check from
returning on stale pre-change state.
🪄 Autofix
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: 126a8e2c-170d-46b3-a0c2-ab23d27636f3
📒 Files selected for processing (4)
test/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/keycloak_client.gotest/extended/authentication/operator_status_helpers.go
| operator "github.com/openshift/origin/test/extended/util/operator" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[sig-auth][Suite:openshift/conformance/serial][OCPFeatureGate:AuthenticationComponentProxy][Serial]", func() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify package registration and the component-proxy suite definition.
set -euo pipefail
rg -n 'test/extended/authentication' test/extended/include.go || echo "authentication package NOT registered in include.go"
fd -t f 'standard_suites.go' --exec rg -n -C4 'component-proxy|AuthenticationComponentProxy' {} \;
rg -n --type=go 'OCPFeatureGate:AuthenticationComponentProxy'Repository: openshift/origin
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== include.go registration =="
rg -n -C2 'test/extended/authentication' test/extended/include.go
echo "== suite definitions =="
fd -t f 'standard_suites.go' . --exec rg -n -C8 -i 'component.proxy|authenticationcomponentproxy|openshift/auth' {} \; || true
echo "== component-proxy references =="
rg -n -C4 --type=go -i 'component.proxy|AuthenticationComponentProxy|Disruptive|Suite:openshift/conformance/serial' test/extended || true
echo "== authentication test package files =="
fd -t f . test/extended/authentication | sortRepository: openshift/origin
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== authentication suite entries =="
fd -t f 'standard_suites.go' . --exec rg -n -C6 'Name: "openshift/auth|openshift/auth/' {} \; || true
echo "== exact component-proxy suite references =="
rg -n -i --glob '*.go' --glob '*.md' 'openshift/auth/component-proxy|component-proxy' . | head -80 || true
echo "== component-proxy implementation =="
cat -n test/extended/authentication/component_proxy.go | sed -n '1,180p'
echo "== cluster mutation calls in component-proxy =="
rg -n -C3 'Authentication|OAuth|oauth|Patch|Update|Delete|Create' test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 11130
Register and classify the component-proxy suite. test/extended/authentication is already registered in test/extended/include.go. Add the openshift/auth/component-proxy entry to standard_suites.go, update the Describe label, and add [Disruptive] because the tests mutate cluster authentication and OAuth configuration.
🤖 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` at line 18, Register the
component-proxy suite in standard_suites.go under
openshift/auth/component-proxy, then update the Describe declaration in the
suite to use the matching suite label and include [Disruptive]. Preserve the
existing authentication and conformance classification labels.
Source: Coding guidelines
…g changes map Add a comment linking the squid logformat field order to its parser in waitForProxyTrafficFrom. Reuse the serviceHost variable for both the TLS SAN and proxy URLs instead of computing it twice. Rewrite UpdateClientRaw merge loop to avoid mutating the caller's changes map while preserving the deep-merge of attributes.
DoRequest unconditionally set the Bearer token header even when authenticated=false, leaking an empty/stale token on unauthenticated calls like the initial token exchange.
The mount check compared mount.Name against the ConfigMap name constant, which only worked because the operator happened to use the same string for both. Use the actual volume name from the matched volume entry.
…state is restored
Poll the oauth-openshift deployment in BeforeEach to confirm proxy env vars and trustedCA volume are clean before proceeding. Prevents test-to-test interference when the previous test's cleanup is still propagating through the operator.
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
4 participants
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