[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e - #31463
[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e#31463ehearne-redhat wants to merge 18 commits into
Conversation
…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 BeforeEach to deploy Squid proxy, Keycloak, and save/restore the proxy config, with DeferCleanup for teardown. Each test only handles its own IdP registration, network policies, and verification. Three tests validate: - OIDC IdP discovery through an HTTP proxy - OIDC IdP discovery through an HTTPS proxy with trustedCA - Fallback on spec.proxy removal (deletes the proxy to prove the operator no longer routes through it)
- 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.
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.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ehearne-redhat 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 |
WalkthroughThis PR adds end-to-end test coverage for OpenShift authentication component proxy configuration. It introduces test helpers to deploy a Squid forward proxy and Keycloak OIDC provider, configure proxy settings on Authentication/OAuth resources, and verify OAuth server deployment reconciliation, trusted CA synchronization, and OIDC login behavior. ChangesComponent proxy e2e test suite
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Auth as Authentication/cluster
participant Squid as Squid Proxy
participant Operator as Auth Operator
participant OAuthServer as oauth-server Deployment
participant Keycloak as Keycloak OIDC
Test->>Squid: deploySquidProxy()
Test->>Keycloak: deployKeycloakForProxy()
Test->>Auth: updateAuthenticationProxy(httpProxy, httpsProxy, trustedCA)
Auth->>Operator: notify spec change
Operator->>OAuthServer: reconcile HTTP_PROXY/HTTPS_PROXY/NO_PROXY env, mount trusted CA
Test->>OAuthServer: verifyOAuthServerDeploymentProxyConfig()
Test->>Keycloak: assertOIDCLogin() via Squid proxy
Keycloak-->>Test: token/username response
Test->>Auth: remove proxy configuration
Auth->>Operator: notify spec change
Operator->>OAuthServer: reconcile cleared proxy env
Test->>OAuthServer: verify proxy env cleared
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 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/keycloak_helpers.go (1)
46-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the per-resource cleanups are discarded.
The service account, service, deployment, and route cleanups are dropped because the namespace cleanup removes those objects. The CA ConfigMap cleanup stays because that object lives in
openshift-config. Add one short comment so a later reader does not treat the discarded returns as a leak.As per coding guidelines: "Keep comments minimal and helpful, explaining why rather than what."
🤖 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_helpers.go` around lines 46 - 79, Add one concise comment near the cleanup initialization or before the per-resource creation calls explaining that the service account, service, deployment, and route cleanups are intentionally discarded because namespace cleanup removes those objects, while the CA ConfigMap cleanup is retained because it lives in openshift-config. Do not alter the cleanup behavior.Source: Coding guidelines
test/extended/authentication/component_proxy_helpers.go (3)
406-424: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClient selection is nondeterministic.
The master realm contains several clients with
redirectUris, for exampleaccount,account-console, andsecurity-admin-console. The loop takes the first one the API returns, so the test can configure the IdP with an unintended client. Select the client by its knownclientIDinstead.♻️ Suggested approach
- var adminClientID, passwdClientID string - for _, c := range clientList { - if c.ClientID == "admin-cli" { - adminClientID = c.ID - } else if len(c.RedirectURIs) > 0 { - passwdClientID = c.ID - setup.clientID = c.ClientID - } - if len(passwdClientID) > 0 && len(adminClientID) > 0 { - break - } - } + var adminClientID, passwdClientID string + for _, c := range clientList { + switch c.ClientID { + case "admin-cli": + adminClientID = c.ID + case keycloakTestClientID: // the client created for this test + passwdClientID = c.ID + setup.clientID = c.ClientID + } + }🤖 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 406 - 424, Update the client-selection loop to choose the password-grant client by its known clientID rather than selecting the first client with non-empty RedirectURIs. Preserve admin-cli lookup and the existing missing-client errors, and assign setup.clientID from the explicitly matched password-grant client.
333-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTraffic detection reads the full log history.
getSquidProxyLogspasses a zero time, so the check matches any earlier request as well.TCP_also appears in unrelated log lines. If a spec must prove that a specific step produced proxy traffic, pass a start timestamp togetSquidProxyLogsSince.🤖 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 333 - 347, Update waitForSquidProxyTraffic to capture the check start timestamp and use getSquidProxyLogsSince with that timestamp on each poll, limiting detection to traffic generated after the wait began. Retain the existing polling and error behavior while using a more specific proxy request pattern than the broad TCP_ match.
689-692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe debug message can report the wrong actual state.
matchTrustedCAVolumechecks both the volume and the mount. Afalseresult does not prove that presence equals!expectTrustedCAVolume. Log the two found flags instead.🤖 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 689 - 692, Update the logging in the matchTrustedCAVolume failure branch to report the actual trusted CA volume and mount presence flags returned or computed by that check, rather than deriving a single state from !expectTrustedCAVolume. Keep the existing mismatch return behavior unchanged.test/extended/authentication/crypto_helpers.go (1)
18-89: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider ECDSA P-256 and shared key/serial generation.
The path instructions prefer Ed25519 or ECDSA P-256+ for signing. These certificates are test-only and short-lived, so RSA-2048 with SHA-256 is acceptable, but ECDSA P-256 generates faster and matches the guidance. The two functions also duplicate key generation and serial-number generation; extract a small helper.
As per path instructions: "Signing: Ed25519 or ECDSA P-256+".
🤖 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/crypto_helpers.go` around lines 18 - 89, Update mustNewServerCertificate and mustNewCertificateAuthority to use ECDSA P-256 keys and the corresponding certificate signature algorithm instead of RSA-2048/SHA-256. Extract the duplicated private-key and serial-number generation into a small shared helper, then reuse it in both certificate-construction paths while preserving their existing certificate hierarchy and fields.Source: Path instructions
test/extended/authentication/component_proxy_oauth.go (3)
363-373: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the "no redeploy" assertion.
The pod-name comparison passes immediately after the CA rotation, before the operator could have rolled out a new revision. Compare pod UIDs, or assert that the
oauth-openshiftDeploymentmetadata.generationandstatus.observedGenerationdid not change, and hold the assertion witho.Consistentlyfor a short window. That distinguishes "not redeployed" from "not yet redeployed".🤖 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_oauth.go` around lines 363 - 373, Strengthen the no-redeploy check in the pod verification block by polling consistently for a short window instead of comparing names only once. Capture and compare stable pod UIDs (or the oauth-openshift Deployment generation and observedGeneration), and ensure the assertion remains unchanged throughout the window so delayed rollouts are detected.
101-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
updateAuthenticationProxyinstead of inlining Get/Update.
component_proxy.gosets the component proxy throughupdateAuthenticationProxy. This file repeats the Get/mutate/Update sequence in four specs (lines 101-110, 146-159, 174-182, 258-268, 378-391). The helper also centralizes conflict handling if it is added later. Use the helper here for consistency.🤖 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_oauth.go` around lines 101 - 110, Replace the inline Authentication Get, proxy mutation, and Update sequence in the affected specs with the existing updateAuthenticationProxy helper, passing the appropriate proxy configuration and preserving each test’s existing assertions and behavior. Apply this consistently to all repeated occurrences in the file.
204-209: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleeps before positive log assertions with
o.Eventually.Lines 205, 231, 289, 358, and 406 each sleep two minutes unconditionally. That adds about ten minutes to the suite. For the positive assertions (lines 209, 292, 361), poll
getSquidProxyLogsSincewitho.Eventuallyso the spec continues as soon as the expected log line appears. For the negative assertions (lines 235, 410), a bounded wait is still needed;o.Consistentlyexpresses that intent more clearly than a sleep.Also note that lines 193 and 225 use
logCutOffandlogCutofffor the same concept. Use one spelling.♻️ Proposed change for the positive assertion
- g.By("Waiting for squid logs to settle before checking for proxy traffic") - time.Sleep(2 * time.Minute) - - logs, err := getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff) - o.Expect(err).NotTo(o.HaveOccurred()) - o.Expect(logs).To(o.ContainSubstring(keycloakHost), "squid logs should contain keycloak traffic after proxy login") + g.By("Waiting for squid logs to show proxy traffic") + o.Eventually(func() (string, error) { + return getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff) + }).WithTimeout(3*time.Minute).WithPolling(10*time.Second). + Should(o.ContainSubstring(keycloakHost), "squid logs should contain keycloak traffic after proxy login")🤖 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_oauth.go` around lines 204 - 209, Replace the unconditional time.Sleep call before the positive assertion with o.Eventually that polls getSquidProxyLogsSince until the expected substring appears in the logs. Apply this pattern to all lines with positive assertions (checking that logs contain keycloakHost or similar expected values) by moving the getSquidProxyLogsSince call and the o.ContainSubstring check into the Eventually block. For negative assertions (checking that logs do not contain something), use o.Consistently instead to express the intent of verifying absence over time. Additionally, standardize the spelling throughout the file to use logCutOff consistently instead of mixing logCutOff and logCutoff.
🤖 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`:
- Line 194: The Replicas field assignment uses the new builtin incorrectly by
passing a value instead of a type. Add the import for "k8s.io/utils/ptr" and
replace new(int32(1)) with ptr.To(int32(1)), following the same pattern already
used in keycloak_helpers.go.
- Around line 278-284: Update the watch.Error branch in the event callback to
stop passing the runtime.Object event.Object to the %w formatting verb; use a
non-error formatting verb such as %v, or explicitly convert the object to an
error before wrapping. Preserve the existing error message context and return
behavior, then ensure go vet passes.
In `@test/extended/authentication/component_proxy_oauth.go`:
- Around line 60-69: Remove the local kcCleanups declaration in the Keycloak
setup so the assignment updates the suite-level variable used by the deferred
cleanup closure. Reset the suite-level kcCleanups slice in BeforeEach before
deploying resources, ensuring later IdP cleanup functions appended in each spec
are executed without accumulating across specs.
- Around line 405-410: Move the existing logCutOff declaration in the noProxy
test to immediately before the login call, then replace getSquidProxyLogs with
getSquidProxyLogsSince using that cutoff for the negative assertion. Preserve
the existing error and absence checks while limiting logs to entries generated
after the cutoff.
In `@test/extended/authentication/component_proxy.go`:
- Around line 34-43: Register each cleanup only after its helper error assertion
succeeds: in test/extended/authentication/component_proxy.go lines 34-43, move
the assertions before DeferCleanup(authRestore) and DeferCleanup(proxyCleanup);
in test/extended/authentication/component_proxy_oauth.go lines 88-91, move the
assertion before DeferCleanup(authRestore).
- Around line 34-61: The cleanup registrations in the setup block currently
restore authentication state after deleting Squid and Keycloak resources. Move
the g.DeferCleanup(authRestore) registration to after the proxy and Keycloak
cleanup registrations, matching the ordering used by component_proxy_oauth.go so
authentication is restored first during LIFO teardown.
In `@test/extended/authentication/keycloak_client.go`:
- Around line 230-244: Update UpdateClientRaw to merge the nested attributes map
with the existing attributes before issuing the client update, preserving all
unrelated client attributes while applying changes such as
access.token.lifespan. Keep the existing shallow merge behavior for other
top-level fields and ensure the merged attributes are included in the final
update payload.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 406-424: Update the client-selection loop to choose the
password-grant client by its known clientID rather than selecting the first
client with non-empty RedirectURIs. Preserve admin-cli lookup and the existing
missing-client errors, and assign setup.clientID from the explicitly matched
password-grant client.
- Around line 333-347: Update waitForSquidProxyTraffic to capture the check
start timestamp and use getSquidProxyLogsSince with that timestamp on each poll,
limiting detection to traffic generated after the wait began. Retain the
existing polling and error behavior while using a more specific proxy request
pattern than the broad TCP_ match.
- Around line 689-692: Update the logging in the matchTrustedCAVolume failure
branch to report the actual trusted CA volume and mount presence flags returned
or computed by that check, rather than deriving a single state from
!expectTrustedCAVolume. Keep the existing mismatch return behavior unchanged.
In `@test/extended/authentication/component_proxy_oauth.go`:
- Around line 363-373: Strengthen the no-redeploy check in the pod verification
block by polling consistently for a short window instead of comparing names only
once. Capture and compare stable pod UIDs (or the oauth-openshift Deployment
generation and observedGeneration), and ensure the assertion remains unchanged
throughout the window so delayed rollouts are detected.
- Around line 101-110: Replace the inline Authentication Get, proxy mutation,
and Update sequence in the affected specs with the existing
updateAuthenticationProxy helper, passing the appropriate proxy configuration
and preserving each test’s existing assertions and behavior. Apply this
consistently to all repeated occurrences in the file.
- Around line 204-209: Replace the unconditional time.Sleep call before the
positive assertion with o.Eventually that polls getSquidProxyLogsSince until the
expected substring appears in the logs. Apply this pattern to all lines with
positive assertions (checking that logs contain keycloakHost or similar expected
values) by moving the getSquidProxyLogsSince call and the o.ContainSubstring
check into the Eventually block. For negative assertions (checking that logs do
not contain something), use o.Consistently instead to express the intent of
verifying absence over time. Additionally, standardize the spelling throughout
the file to use logCutOff consistently instead of mixing logCutOff and
logCutoff.
In `@test/extended/authentication/crypto_helpers.go`:
- Around line 18-89: Update mustNewServerCertificate and
mustNewCertificateAuthority to use ECDSA P-256 keys and the corresponding
certificate signature algorithm instead of RSA-2048/SHA-256. Extract the
duplicated private-key and serial-number generation into a small shared helper,
then reuse it in both certificate-construction paths while preserving their
existing certificate hierarchy and fields.
In `@test/extended/authentication/keycloak_helpers.go`:
- Around line 46-79: Add one concise comment near the cleanup initialization or
before the per-resource creation calls explaining that the service account,
service, deployment, and route cleanups are intentionally discarded because
namespace cleanup removes those objects, while the CA ConfigMap cleanup is
retained because it lives in openshift-config. Do not alter the cleanup
behavior.
🪄 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: 708f097b-0dec-43c5-b832-fe6b994bdc5a
📒 Files selected for processing (7)
test/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/component_proxy_oauth.gotest/extended/authentication/crypto_helpers.gotest/extended/authentication/keycloak_client.gotest/extended/authentication/keycloak_helpers.gotest/extended/authentication/operator_status_helpers.go
| Labels: map[string]string{"app": squidServiceName}, | ||
| }, | ||
| Spec: appsv1.DeploymentSpec{ | ||
| Replicas: new(int32(1)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
new(int32(1)) does not compile.
The new builtin accepts a type, not a value. Use ptr.To as keycloak_helpers.go does at line 185.
🐛 Proposed fix
- Replicas: new(int32(1)),
+ Replicas: ptr.To(int32(1)),Add the import:
"k8s.io/utils/ptr"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Replicas: new(int32(1)), | |
| Replicas: ptr.To(int32(1)), |
🤖 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 194, The
Replicas field assignment uses the new builtin incorrectly by passing a value
instead of a type. Add the import for "k8s.io/utils/ptr" and replace
new(int32(1)) with ptr.To(int32(1)), following the same pattern already used in
keycloak_helpers.go.
| g.By("Waiting for squid logs to settle before checking for absence of proxy traffic") | ||
| time.Sleep(2 * time.Minute) | ||
|
|
||
| logs, err := getSquidProxyLogs(ctx, oc, proxyNamespace) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use getSquidProxyLogsSince with a cutoff for this negative assertion.
This check reads the full squid log for the pod lifetime. Any keycloak line logged before the noProxy configuration took effect fails the assertion. The other three log checks in this file use a cutoff. Record a cutoff before the login and use getSquidProxyLogsSince.
🐛 Proposed fix
+ logCutOff := time.Now()
+
g.By("Verifying OIDC login works after setting proxy with noProxy")
assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup)
g.By("Waiting for squid logs to settle before checking for absence of proxy traffic")
time.Sleep(2 * time.Minute)
- logs, err := getSquidProxyLogs(ctx, oc, proxyNamespace)
+ logs, err := getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff)
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect")Move the logCutOff declaration above the login call at line 403.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| g.By("Waiting for squid logs to settle before checking for absence of proxy traffic") | |
| time.Sleep(2 * time.Minute) | |
| logs, err := getSquidProxyLogs(ctx, oc, proxyNamespace) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect") | |
| logCutOff := time.Now() | |
| g.By("Verifying OIDC login works after setting proxy with noProxy") | |
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) | |
| g.By("Waiting for squid logs to settle before checking for absence of proxy traffic") | |
| time.Sleep(2 * time.Minute) | |
| logs, err := getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect") |
🤖 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_oauth.go` around lines 405 -
410, Move the existing logCutOff declaration in the noProxy test to immediately
before the login call, then replace getSquidProxyLogs with
getSquidProxyLogsSince using that cutoff for the negative assertion. Preserve
the existing error and absence checks while limiting logs to entries generated
after the cutoff.
| g.By("Saving auth state for restore after test") | ||
| authRestore, err := saveAndRestoreAuthState(ctx, oc) | ||
| g.DeferCleanup(authRestore) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| g.By("Deploying Squid forward proxy") | ||
| var proxyCleanup removalFunc | ||
| httpProxyURL, httpsProxyURL, caCertPEM, proxyNamespace, proxyCleanup, err = deploySquidProxy(ctx, oc) | ||
| g.DeferCleanup(proxyCleanup) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Register cleanup functions only after you assert the error. Both files pass a cleanup function to g.DeferCleanup before checking the error from the helper that returned it. If the helper fails and returns a nil function, g.DeferCleanup receives nil and panics, which replaces the real assertion failure.
test/extended/authentication/component_proxy.go#L34-L43: moveo.Expect(err).NotTo(o.HaveOccurred())aboveg.DeferCleanup(authRestore)and aboveg.DeferCleanup(proxyCleanup).test/extended/authentication/component_proxy_oauth.go#L88-L91: moveo.Expect(err).NotTo(o.HaveOccurred())aboveg.DeferCleanup(authRestore).
📍 Affects 2 files
test/extended/authentication/component_proxy.go#L34-L43(this comment)test/extended/authentication/component_proxy_oauth.go#L88-L91
🤖 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 34 - 43,
Register each cleanup only after its helper error assertion succeeds: in
test/extended/authentication/component_proxy.go lines 34-43, move the assertions
before DeferCleanup(authRestore) and DeferCleanup(proxyCleanup); in
test/extended/authentication/component_proxy_oauth.go lines 88-91, move the
assertion before DeferCleanup(authRestore).
| func (kc *keycloakClient) UpdateClientAccessTokenTimeout(id string, timeout int32) error { | ||
| return kc.UpdateClientRaw(id, map[string]any{ | ||
| "attributes": map[string]any{ | ||
| "access.token.lifespan": strconv.FormatInt(int64(timeout), 10), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error { | ||
| existing, err := kc.GetClientRaw(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| maps.Copy(existing, changes) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
maps.Copy drops existing nested attributes.
maps.Copy performs a shallow merge. When UpdateClientAccessTokenTimeout passes an attributes map with one key, that map replaces the client's whole existing attributes object in the PUT body. Keycloak then loses every other attribute on the client, for example post.logout.redirect.uris. Merge nested maps one level deep, or set only the single attribute key.
🐛 Proposed fix
func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error {
existing, err := kc.GetClientRaw(id)
if err != nil {
return err
}
- maps.Copy(existing, changes)
+ // Merge one level deep so callers that set a single nested key (for example
+ // "attributes") do not discard the client's other nested values.
+ for key, value := range changes {
+ newNested, newIsMap := value.(map[string]any)
+ oldNested, oldIsMap := existing[key].(map[string]any)
+ if newIsMap && oldIsMap {
+ maps.Copy(oldNested, newNested)
+ continue
+ }
+ existing[key] = value
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (kc *keycloakClient) UpdateClientAccessTokenTimeout(id string, timeout int32) error { | |
| return kc.UpdateClientRaw(id, map[string]any{ | |
| "attributes": map[string]any{ | |
| "access.token.lifespan": strconv.FormatInt(int64(timeout), 10), | |
| }, | |
| }) | |
| } | |
| func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error { | |
| existing, err := kc.GetClientRaw(id) | |
| if err != nil { | |
| return err | |
| } | |
| maps.Copy(existing, changes) | |
| func (kc *keycloakClient) UpdateClientAccessTokenTimeout(id string, timeout int32) error { | |
| return kc.UpdateClientRaw(id, map[string]any{ | |
| "attributes": map[string]any{ | |
| "access.token.lifespan": strconv.FormatInt(int64(timeout), 10), | |
| }, | |
| }) | |
| } | |
| func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error { | |
| existing, err := kc.GetClientRaw(id) | |
| if err != nil { | |
| return err | |
| } | |
| // Merge one level deep so callers that set a single nested key (for example | |
| // "attributes") do not discard the client's other nested values. | |
| for key, value := range changes { | |
| newNested, newIsMap := value.(map[string]any) | |
| oldNested, oldIsMap := existing[key].(map[string]any) | |
| if newIsMap && oldIsMap { | |
| maps.Copy(oldNested, newNested) | |
| continue | |
| } | |
| existing[key] = 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 230 - 244,
Update UpdateClientRaw to merge the nested attributes map with the existing
attributes before issuing the client update, preserving all unrelated client
attributes while applying changes such as access.token.lifespan. Keep the
existing shallow merge behavior for other top-level fields and ensure the merged
attributes are included in the final update payload.
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.
The service URL approach doesn't work because .svc is in the operator's NO_PROXY list, bypassing the proxy entirely. Revert to the route URL as the OIDC issuer and use haproxy.router.openshift.io/ip_whitelist on the Keycloak route to restrict access to only the Squid proxy pod IP. This structurally enforces proxy usage: the ingress router rejects requests from any IP other than the proxy, so the operator must route through the proxy to reach Keycloak. Also reverts to the default ingress CA (instead of service CA) since the route's TLS cert is signed by the ingress CA.
Replace network policy with haproxy.router.openshift.io/ip_whitelist on the Keycloak route, restricting access to the Squid proxy pod IP. This structurally enforces proxy usage since the ingress router rejects requests from any other source IP. Add a reachability check that verifies Keycloak is blocked without the proxy (expects EOF from the whitelisted route). Wait for trustedCA sync before registering the IdP to avoid a race where the operator tries to use the HTTPS proxy before the CA is available. Revert to route URL as issuer and ingress CA for the IdP CA configmap, since .svc addresses are in the operator's NO_PROXY list.
c2dc765 to
1a6a523
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 62-67: The removeResources function call in the AfterEach block
discards its error return value using the blank identifier, which violates Go
error handling practices and could hide cleanup failures. Capture the error
return from removeResources instead of discarding it, then assert that the error
did not occur using the same expectation pattern applied to the
WaitForOperatorsToSettle error below it, ensuring cleanup failures are properly
reported before the test completes.
- Around line 32-34: Update the test setup and cleanup flow around BeforeEach
and AfterEach to create a 30-second timeout context with cancellation for
cleanup, pass that cleanup context to removeResources instead of the root ctx,
and handle the returned error explicitly rather than ignoring it.
🪄 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: cfd3ac2e-3f0c-41ca-8904-931be371befa
📒 Files selected for processing (3)
test/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/component_proxy_oauth.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/authentication/component_proxy_oauth.go
- test/extended/authentication/component_proxy_helpers.go
| g.BeforeEach(func() { | ||
| ctx = context.Background() | ||
| cleanups = nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
cat -n test/extended/authentication/component_proxy.go | head -80Repository: openshift/origin
Length of output: 3378
🏁 Script executed:
# Search for the removeResources function to understand error handling
rg -A 10 "func removeResources" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check the full cleanup lifecycle in the file
rg -B 5 -A 5 "cleanups" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 1641
🏁 Script executed:
# Search for removeResources in the entire test/extended directory
rg -l "func removeResources" test/extended/Repository: openshift/origin
Length of output: 191
🏁 Script executed:
# Also check if removeResources is defined in this file
rg "removeResources" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 280
🏁 Script executed:
# Get the full file to understand the structure
wc -l test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 206
🏁 Script executed:
# Get the removeResources function definition from oidc.go
rg -B 3 -A 15 "func removeResources" test/extended/authentication/oidc.goRepository: openshift/origin
Length of output: 837
🏁 Script executed:
# Check the removalFunc type definition
rg "type removalFunc" test/extended/authentication/Repository: openshift/origin
Length of output: 236
🏁 Script executed:
# Check Ginkgo context best practices by looking at other test files
rg "context.Background()" test/extended/ | head -20Repository: openshift/origin
Length of output: 2525
🏁 Script executed:
# Check if there are other patterns for bounded contexts in test files
rg -A 3 "g.BeforeEach" test/extended/authentication/oidc.go | head -30Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check how other test files handle context in AfterEach/cleanup
rg -B 2 -A 5 "g.AfterEach" test/extended/authentication/oidc.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
# Look for patterns where context is created with timeout/deadline in test files
rg "context.WithTimeout\|context.WithDeadline" test/extended/ | head -20Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check the Ginkgo spec context documentation or common usage
rg "SpecContext\|context.TODO" test/extended/ | head -20Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Verify if there are any timeouts applied in removeResources or similar cleanup operations
rg -B 5 -A 5 "AfterEach" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 629
🏁 Script executed:
# Check if there's a pattern in how Ginkgo tests are structured
# Look for Ginkgo spec that has context with timeout
rg "g.It\|g.Describe" test/extended/authentication/component_proxy.go -A 3 | head -20Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check what the spec timeout is for this test
rg "Timeout\|SetDefaultEventuallyTimeout" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
# Let me check if Ginkgo provides context management
rg "context.WithTimeout" test/extended/ -c | head -5Repository: openshift/origin
Length of output: 387
🏁 Script executed:
# Check the learning context around cleanup patterns
# See if any tests use a special cleanup context
rg -B 3 -A 8 "g.DeferCleanup\|deferred.*cleanup" test/extended/ | head -40Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check component_proxy_helpers.go for context usage patterns
cat -n test/extended/authentication/component_proxy_helpers.go | head -50Repository: openshift/origin
Length of output: 1905
🏁 Script executed:
# Look for how other Go tests in the codebase handle context in BeforeEach/AfterEach
rg -B 5 -A 10 "g.BeforeEach.*func\(\)" test/extended/storage/driver_configuration.go | head -40Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Verify the exact error handling at line 63
sed -n '62,65p' test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 273
🏁 Script executed:
# Check if context.Background() is the standard pattern in Ginkgo test files
rg "ctx = context.Background()" test/extended/ -cRepository: openshift/origin
Length of output: 370
🏁 Script executed:
# Check the exact error behavior of removeResources - does it always return an error?
sed -n '1,200p' test/extended/authentication/oidc.go | grep -A 20 "func removeResources"Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Look for patterns in how removeResources errors are handled elsewhere
rg "removeResources" test/extended/authentication/ -B 2 -A 1Repository: openshift/origin
Length of output: 1967
🏁 Script executed:
# Verify the guideline interpretation: Check Go security guidelines in coding_guidelines
# Look for the exact context usage guidance
rg "context.Context" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 553
Add timeout context for cleanup operations and handle errors.
Line 33 assigns context.Background() which has no cancellation or deadline. When cleanup operations in AfterEach (line 63) reuse this context, a stalled API request can hang indefinitely. Line 63 also ignores the error return from removeResources.
In AfterEach, create a time-bounded cleanup context: cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second); defer cancel(). Use cleanupCtx instead of the root ctx for removeResources. Check the error return or document explicitly why it is safe to discard.
The coding guidelines require context with cancellation/timeout for operations and never to 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 32 - 34, Update
the test setup and cleanup flow around BeforeEach and AfterEach to create a
30-second timeout context with cancellation for cleanup, pass that cleanup
context to removeResources instead of the root ctx, and handle the returned
error explicitly rather than ignoring it.
Sources: Path instructions, Learnings
| g.AfterEach(func() { | ||
| _ = removeResources(ctx, cleanups...) | ||
|
|
||
| g.By("Waiting for operators to be stable after test") | ||
| err := operator.WaitForOperatorsToSettle(ctx, oc.AdminConfigClient(), 10) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report cleanup failures.
Line 63 discards the error from removeResources. If state restoration or resource deletion fails, later serial specs can run against leaked authentication configuration. Capture and assert this error.
As per path instructions, Go code must 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 62 - 67, The
removeResources function call in the AfterEach block discards its error return
value using the blank identifier, which violates Go error handling practices and
could hide cleanup failures. Capture the error return from removeResources
instead of discarding it, then assert that the error did not occur using the
same expectation pattern applied to the WaitForOperatorsToSettle error below it,
ensuring cleanup failures are properly reported before the test completes.
Source: Path instructions
|
Scheduling required tests: |
|
@ehearne-redhat: 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. |
3 participants
Continues work from openshift/cluster-authentication-operator#950 . Kept in separate file for now. Plan is to add tests to
test/extended/authentication/component_proxy_oauth.gointo #31446 when consensus reached on test status.Summary by CodeRabbit