CNTRLPLANE-3851: Oauth server proxy config e2e - #31463
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@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 |
|
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 an end-to-end Authentication component proxy suite and helpers. The tests deploy Squid and Keycloak, configure proxies, trusted CAs, and ChangesAuthentication component proxy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticationTest
participant AuthenticationOperator
participant OAuthServer
participant Squid
participant Keycloak
AuthenticationTest->>AuthenticationOperator: configure proxy and OIDC provider
AuthenticationOperator->>OAuthServer: reconcile proxy variables and trusted CA mounts
OAuthServer->>Squid: send proxied OIDC request
Squid->>Keycloak: forward OIDC request
Keycloak-->>OAuthServer: return OIDC response
OAuthServer-->>AuthenticationTest: complete login
AuthenticationTest->>AuthenticationOperator: remove proxy configuration
AuthenticationOperator->>OAuthServer: remove proxy variables
OAuthServer->>Keycloak: send direct OIDC request
Keycloak-->>OAuthServer: return direct response
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
| 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).
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: |
1a6a523 to
b742e6b
Compare
|
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. |
ehearne-redhat
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 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 task 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: 3
♻️ Duplicate comments (3)
test/extended/authentication/component_proxy_helpers.go (1)
201-201: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
new(int32(1))does not compile.The
newbuiltin accepts a type, not a value. Useptr.To(int32(1)).🐛 Proposed fix
- Replicas: new(int32(1)), + Replicas: ptr.To(int32(1)),Add the import:
"k8s.io/utils/ptr"🤖 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 201, Fix the Replicas assignment in the affected component proxy helper by replacing the invalid new(int32(1)) expression with ptr.To(int32(1)), and add the k8s.io/utils/ptr import.test/extended/authentication/component_proxy.go (2)
81-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport cleanup failures and bound the cleanup context.
Line 82 discards the error from
removeResources. A failed restore then stays invisible, and later serial specs run against leaked authentication configuration. The sharedctxfrom line 45 has no deadline, so a stalled API call in cleanup can hang the suite.Assert the error and use a time-bounded cleanup context.
🐛 Proposed fix
g.AfterEach(func() { - _ = removeResources(ctx, cleanups...) + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Minute) + defer cancel() + err := removeResources(cleanupCtx, cleanups...) + o.Expect(err).NotTo(o.HaveOccurred(), "cleanup should succeed") g.By("Waiting for operators to be stable after test") - err := operator.WaitForOperatorsToSettle(ctx, oc.AdminConfigClient(), 10) + err = operator.WaitForOperatorsToSettle(cleanupCtx, oc.AdminConfigClient(), 10) o.Expect(err).NotTo(o.HaveOccurred()) })As per path instructions: "Never ignore error returns" and "context.Context for cancellation and timeouts".
🤖 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 81 - 87, Update the g.AfterEach cleanup around removeResources to create and use a time-bounded context, ensuring stalled cleanup calls are cancelled; stop discarding removeResources errors and assert/report them before waiting for operators to settle. Preserve the existing operator stabilization check and cleanup ordering.Source: Path instructions
49-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAppend cleanup functions only after you assert the error.
saveAndRestoreAuthStateanddeploySquidProxyreturn a nilremovalFuncon some failure paths. Each call appends the returned function tocleanupsbefore the assertion. When the helper fails, Gomega aborts the spec,AfterEachruns, andremoveResourcesinvokes a nil function. The resulting nil-call panic replaces the real assertion failure. Move eacho.Expect(err)above the correspondingappend, or skip nil entries.#!/bin/bash # Check whether removeResources guards against nil removalFunc entries. rg -n 'func removeResources' -A 20 test/extended/authentication rg -n 'type removalFunc' -C2 test/extended/authentication🤖 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 49 - 63, In the setup flow, update saveAndRestoreAuthState and deploySquidProxy so each returned cleanup is appended to cleanups only after its corresponding o.Expect(err).NotTo(o.HaveOccurred()) assertion succeeds; alternatively, ensure removeResources safely skips nil removalFunc entries. Preserve cleanup registration for successful helper calls and prevent failed calls from causing a nil-function panic.
🧹 Nitpick comments (6)
test/extended/authentication/component_proxy.go (4)
329-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not reuse the operator-managed ConfigMap name for the user-supplied trustedCA.
componentProxyCAConfigMapNameis the name that the operator creates inopenshift-authenticationfor the synchronized bundle.verifyTrustedCAConfigMapSyncedchecks that exact name in that namespace. Creating a source ConfigMap with the same name inopenshift-configmakes the two roles indistinguishable in the test, and the cleanup on line 342 deletes only theopenshift-configcopy. Use a distinct source name, astestPartialFullEnvVarsdoes withe2e-proxy-trusted-ca.🤖 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 329 - 337, Use a distinct user-supplied trustedCA ConfigMap name in the ConfigMap definition near caConfigMap instead of componentProxyCAConfigMapName, matching the separate source-name pattern used by testPartialFullEnvVars (for example, e2e-proxy-trusted-ca). Keep componentProxyCAConfigMapName reserved for the operator-managed synchronized bundle.
430-436: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant string conversion.
Output()already returns astring.string(output)is an unnecessary conversion, andunconvert-style linters inmake verifycan report it.♻️ Proposed change
- o.Expect(err).NotTo(o.HaveOccurred(), "squid reconfigure failed: %s", string(output)) + o.Expect(err).NotTo(o.HaveOccurred(), "squid reconfigure failed: %s", output)🤖 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 430 - 436, In the squid reconfigure assertion, update the error message in the `oc.AsAdmin().Run("exec").Args(...).Output()` flow to use `output` directly instead of converting it with `string(output)`. Preserve the existing command execution and error assertion behavior.
257-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the expected
NO_PROXYvalue fromnoProxyHost.Line 258 repeats the literal
noproxy.example.com. The value on line 241 and the expectation can drift apart. Compose the expected string from the variable.♻️ Proposed change
- err = verifyOAuthServerDeploymentProxyConfig(ctx, oc, httpProxyURL, httpsProxyURL, ".cluster.local,.svc,127.0.0.1,localhost,noproxy.example.com", true) + err = verifyOAuthServerDeploymentProxyConfig(ctx, oc, httpProxyURL, httpsProxyURL, + ".cluster.local,.svc,127.0.0.1,localhost,"+noProxyHost, true)🤖 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 257 - 259, Update the expected NO_PROXY argument in the test around verifyOAuthServerDeploymentProxyConfig to compose its custom host entry from the existing noProxyHost variable instead of repeating the noproxy.example.com literal, while preserving the surrounding default entries and verification behavior.
318-323: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTwo negative traffic assertions use a fixed two-minute sleep. Both sites wait a fixed period and then assert that the Squid access log contains no Keycloak host. The shared root cause is the fixed delay used as a settle window; a bounded poll gives the same guarantee with less fixed runtime in this serial suite.
test/extended/authentication/component_proxy.go#L318-L323: replacetime.Sleep(2 * time.Minute)with a bounded poll of the access log after you confirm the direct login succeeded.test/extended/authentication/component_proxy.go#L493-L498: apply the same bounded poll for thenoProxycase.🤖 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 318 - 323, The negative Squid traffic checks use an unnecessary fixed two-minute delay. In test/extended/authentication/component_proxy.go#L318-L323 and `#L493-L498`, replace each time.Sleep-based settle window with the existing bounded polling mechanism that repeatedly reads the access log after direct login succeeds, then preserve the assertions that the logs do not contain keycloakHost.test/extended/authentication/component_proxy_helpers.go (2)
576-588: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail early if
ca-bundle.crtis absent.If
default-ingress-certdoes not containca-bundle.crt, this helper creates a ConfigMap with an emptyca.crt. The IdP then fails TLS verification, and the failure appears much later as a login timeout. Check the key and return an error.♻️ Proposed change
+ caBundle, ok := ca.Data["ca-bundle.crt"] + if !ok || len(caBundle) == 0 { + return nil, fmt.Errorf("openshift-config-managed/default-ingress-cert has no ca-bundle.crt") + } + _, err = kubeClient.CoreV1().ConfigMaps("openshift-config").Create(ctx, &corev1.ConfigMap{ @@ Data: map[string]string{ - "ca.crt": ca.Data["ca-bundle.crt"], + "ca.crt": caBundle, },🤖 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 - 588, Update the helper around the retrieved ConfigMap and ca.Data["ca-bundle.crt"] to validate that the ca-bundle.crt key exists before creating the new ConfigMap. If absent, return a descriptive error immediately; otherwise preserve the existing ca.crt assignment and creation flow.
313-328: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead logs from all Squid pods, not only the first list entry.
pods.Items[0]can select a terminating or newly created pod. The access-log assertions then miss entries and the polling helpers time out. Filter for a running pod, or concatenate the logs of all matching pods.🤖 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 313 - 328, Update the log retrieval flow using the listed Squid pods so it does not rely on pods.Items[0]. Select an appropriate running pod or aggregate logs from every matching pod, while preserving the existing container and since-time options and returning the combined access logs to the polling helpers.
🤖 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 664-666: Update matchTrustedCAVolume to return the two observed
volume and mount-state booleans, then use those values in the GinkgoWriter
message instead of !expectTrustedCAVolume. Preserve the existing match result
and expected-value reporting.
- Around line 84-112: Update the cleanup function to collect restore errors from
both the authentication/cluster and oauth/cluster RetryOnConflict calls, while
continuing to attempt both restores. Preserve the existing logging and
operator-stabilization behavior, then return the combined failure using the
errors package instead of always returning nil.
- Around line 436-454: The client discovery loop should not select an arbitrary
client based on non-empty RedirectURIs. Update the logic around adminClientID,
passwdClientID, and setup.clientID to match the intended built-in Keycloak
client by its exact ClientID, or create and select a dedicated client, while
preserving the existing missing-client errors and secret/provider setup.
---
Duplicate comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Line 201: Fix the Replicas assignment in the affected component proxy helper
by replacing the invalid new(int32(1)) expression with ptr.To(int32(1)), and add
the k8s.io/utils/ptr import.
In `@test/extended/authentication/component_proxy.go`:
- Around line 81-87: Update the g.AfterEach cleanup around removeResources to
create and use a time-bounded context, ensuring stalled cleanup calls are
cancelled; stop discarding removeResources errors and assert/report them before
waiting for operators to settle. Preserve the existing operator stabilization
check and cleanup ordering.
- Around line 49-63: In the setup flow, update saveAndRestoreAuthState and
deploySquidProxy so each returned cleanup is appended to cleanups only after its
corresponding o.Expect(err).NotTo(o.HaveOccurred()) assertion succeeds;
alternatively, ensure removeResources safely skips nil removalFunc entries.
Preserve cleanup registration for successful helper calls and prevent failed
calls from causing a nil-function panic.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 576-588: Update the helper around the retrieved ConfigMap and
ca.Data["ca-bundle.crt"] to validate that the ca-bundle.crt key exists before
creating the new ConfigMap. If absent, return a descriptive error immediately;
otherwise preserve the existing ca.crt assignment and creation flow.
- Around line 313-328: Update the log retrieval flow using the listed Squid pods
so it does not rely on pods.Items[0]. Select an appropriate running pod or
aggregate logs from every matching pod, while preserving the existing container
and since-time options and returning the combined access logs to the polling
helpers.
In `@test/extended/authentication/component_proxy.go`:
- Around line 329-337: Use a distinct user-supplied trustedCA ConfigMap name in
the ConfigMap definition near caConfigMap instead of
componentProxyCAConfigMapName, matching the separate source-name pattern used by
testPartialFullEnvVars (for example, e2e-proxy-trusted-ca). Keep
componentProxyCAConfigMapName reserved for the operator-managed synchronized
bundle.
- Around line 430-436: In the squid reconfigure assertion, update the error
message in the `oc.AsAdmin().Run("exec").Args(...).Output()` flow to use
`output` directly instead of converting it with `string(output)`. Preserve the
existing command execution and error assertion behavior.
- Around line 257-259: Update the expected NO_PROXY argument in the test around
verifyOAuthServerDeploymentProxyConfig to compose its custom host entry from the
existing noProxyHost variable instead of repeating the noproxy.example.com
literal, while preserving the surrounding default entries and verification
behavior.
- Around line 318-323: The negative Squid traffic checks use an unnecessary
fixed two-minute delay. In
test/extended/authentication/component_proxy.go#L318-L323 and `#L493-L498`,
replace each time.Sleep-based settle window with the existing bounded polling
mechanism that repeatedly reads the access log after direct login succeeds, then
preserve the assertions that the logs do not contain keycloakHost.
🪄 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: 8fcdcb62-5ef7-4ae2-8b27-b271d8d252cb
📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/authentication/operator_status_helpers.go
- test/extended/authentication/keycloak_client.go
| }); err != nil { | ||
| g.GinkgoWriter.Printf("cleanup: failed to restore Authentication CR: %v\n", err) | ||
| } | ||
|
|
||
| g.GinkgoWriter.Println("cleanup: restoring oauth/cluster") | ||
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| fresh, err := oauthClient.Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if reflect.DeepEqual(fresh.Spec, *originalOAuthSpec) { | ||
| return nil | ||
| } | ||
| changed = true | ||
| fresh.Spec = *originalOAuthSpec | ||
| _, err = oauthClient.Update(ctx, fresh, metav1.UpdateOptions{}) | ||
| return err | ||
| }); err != nil { | ||
| g.GinkgoWriter.Printf("cleanup: failed to restore oauth/cluster: %v\n", err) | ||
| } | ||
|
|
||
| if changed { | ||
| g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize") | ||
| if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil { | ||
| g.GinkgoWriter.Printf("cleanup: operator did not recover: %v\n", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return restore failures instead of only logging them.
The cleanup function always returns nil. If the restore of authentication/cluster or oauth/cluster fails, the caller cannot detect it. This suite is [Serial], so leaked proxy or IdP configuration affects later specs. Collect the failures and return them, while still attempting both restores.
♻️ Proposed change
return func(ctx context.Context) error {
var changed bool
+ var errs []error
g.GinkgoWriter.Println("cleanup: restoring authentication/cluster")
if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
@@
}); err != nil {
- g.GinkgoWriter.Printf("cleanup: failed to restore Authentication CR: %v\n", err)
+ errs = append(errs, fmt.Errorf("restoring authentication/cluster: %w", err))
}
@@
}); err != nil {
- g.GinkgoWriter.Printf("cleanup: failed to restore oauth/cluster: %v\n", err)
+ errs = append(errs, fmt.Errorf("restoring oauth/cluster: %w", err))
}
if changed {
g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize")
if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil {
- g.GinkgoWriter.Printf("cleanup: operator did not recover: %v\n", err)
+ errs = append(errs, fmt.Errorf("operator did not recover: %w", err))
}
}
- return nil
+ return errors.Join(errs...)
}, nilAdd the errors import.
📝 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.
| }); err != nil { | |
| g.GinkgoWriter.Printf("cleanup: failed to restore Authentication CR: %v\n", err) | |
| } | |
| g.GinkgoWriter.Println("cleanup: restoring oauth/cluster") | |
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| fresh, err := oauthClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| if reflect.DeepEqual(fresh.Spec, *originalOAuthSpec) { | |
| return nil | |
| } | |
| changed = true | |
| fresh.Spec = *originalOAuthSpec | |
| _, err = oauthClient.Update(ctx, fresh, metav1.UpdateOptions{}) | |
| return err | |
| }); err != nil { | |
| g.GinkgoWriter.Printf("cleanup: failed to restore oauth/cluster: %v\n", err) | |
| } | |
| if changed { | |
| g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize") | |
| if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil { | |
| g.GinkgoWriter.Printf("cleanup: operator did not recover: %v\n", err) | |
| } | |
| } | |
| return nil | |
| }, nil | |
| return func(ctx context.Context) error { | |
| var changed bool | |
| var errs []error | |
| g.GinkgoWriter.Println("cleanup: restoring authentication/cluster") | |
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| fresh, err := authenticationClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| if reflect.DeepEqual(fresh.Spec, *originalAuthenticationSpec) { | |
| return nil | |
| } | |
| changed = true | |
| fresh.Spec = *originalAuthenticationSpec | |
| _, err = authenticationClient.Update(ctx, fresh, metav1.UpdateOptions{}) | |
| return err | |
| }); err != nil { | |
| errs = append(errs, fmt.Errorf("restoring authentication/cluster: %w", err)) | |
| } | |
| g.GinkgoWriter.Println("cleanup: restoring oauth/cluster") | |
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| fresh, err := oauthClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| if reflect.DeepEqual(fresh.Spec, *originalOAuthSpec) { | |
| return nil | |
| } | |
| changed = true | |
| fresh.Spec = *originalOAuthSpec | |
| _, err = oauthClient.Update(ctx, fresh, metav1.UpdateOptions{}) | |
| return err | |
| }); err != nil { | |
| errs = append(errs, fmt.Errorf("restoring oauth/cluster: %w", err)) | |
| } | |
| if changed { | |
| g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize") | |
| if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil { | |
| errs = append(errs, fmt.Errorf("operator did not recover: %w", err)) | |
| } | |
| } | |
| return errors.Join(errs...) | |
| }, nil |
🤖 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 84 -
112, Update the cleanup function to collect restore errors from both the
authentication/cluster and oauth/cluster RetryOnConflict calls, while continuing
to attempt both restores. Preserve the existing logging and
operator-stabilization behavior, then return the combined failure using the
errors package instead of always returning nil.
|
Scheduling required tests: |
| "directAccessGrantsEnabled": true, | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
There was a problem hiding this comment.
Since this is not needed in all the tests, it would be better to keep it in the relevant tests, even though it's mild code repetition.
| g.It("should fall back on spec.proxy removal", func() { | ||
| testFallbackOnProxyRemoval(ctx, oc, kcSetup, httpProxyURL, proxyNamespace) | ||
| }) | ||
| g.It("should set partial and full env vars when configured", func() { |
There was a problem hiding this comment.
We should improve test names TBH. When I read this without knowing the test really, I have no idea what the test is testing. I have to improve this in my PR as well, I think.
You can check full names with
$ ./openshift-tests list tests | grep -i componentproxyThere was a problem hiding this comment.
If you're going to make modifications to your tests again, I believe it is best if I move my tests into their own file again until you're happy with them.
No worries - I will try to improve the names of the tests.
| challengehandlers.NewBasicChallengeHandler(oauthServerURL, "", nil, io.Discard, nil, username, password), | ||
| ) | ||
| if err != nil { | ||
| t.Logf("failed to create challenge handler: %v", err) |
There was a problem hiding this comment.
I think that we usually use g.GinkgoWriter.Printf in these tests, perhaps we should be consistent with that?
| o.Expect(err).NotTo(o.HaveOccurred(), "should be able to delete user %q", username) | ||
| } | ||
|
|
||
| func testPartialFullEnvVars(ctx context.Context, oc *exutil.CLI, caCertPEM []byte, httpProxyURL, httpsProxyURL string) { |
There was a problem hiding this comment.
This is gonna take a long time to just check the env vars because of all the re-deployments... We could merge this into some other test, which would be more efficient, but also more ugly. Just leaving as a note for now...
| var oauthPodIPs []string | ||
| for _, p := range oauthPods.Items { | ||
| oauthPodIPs = append(oauthPodIPs, p.Status.PodIP) | ||
| } |
There was a problem hiding this comment.
This block could be a tiny helper. It's used again in another test as well.
|
|
||
| func testHotReloadCAFileChange(ctx context.Context, oc *exutil.CLI, caCertPEM []byte, kcSetup *keycloakProxySetup, httpsProxyURL, proxyNamespace string) { | ||
| kcUser, kcPass, kcGroup := createKeycloakUserPasswordGroup(kcSetup) | ||
| g.By("Creating config map with trustedCA") |
There was a problem hiding this comment.
Could we pls prepend a newline before each By?
| err = waitForOperatorToPickUpChanges(ctx, oc, "authentication") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| logCutOff := time.Now() |
There was a problem hiding this comment.
Again, actually just remove the proxy namespace already.
| if err := kubeClient.CoreV1().ConfigMaps("openshift-config").Delete(ctx, componentProxyCAConfigMapName, metav1.DeleteOptions{}); err != nil { | ||
| g.GinkgoWriter.Printf("failed to clean up ConfigMap %s: %v\n", componentProxyCAConfigMapName, err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Also I am doing the same thing in my trustedCA test, so we could refactor to use a helper. You can pick the config map name. You use an existing constant that is not related to this configmap, which is a bit confusing IMO.
|
|
||
| g.By("Verifying trustedCA ConfigMap is synced to openshift-authentication namespace") | ||
| err = verifyTrustedCAConfigMapSynced(ctx, oc) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
Shouldn't this call be way more up? I mean, at this point you can be sure it's sync'd, otherwise the login flow would not work, I think.
| deleteOIDCUserAndIdentities(ctx, oc, kcUser) | ||
|
|
||
| g.By("Verifying OIDC login works after CA rotation") | ||
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) |
There was a problem hiding this comment.
I am wondering whether this can fail in case trustedCA is somehow not synchronized in time. But I am also not sure what to do about this. I guess you would need to keep checking the actual file in the pod to contain the right trusted CA...
There was a problem hiding this comment.
Yeah that's a valid point - I also thought maybe it wouldn't work if it didn't get sync'd. We do know the exact location of the file https://github.com/openshift/cluster-authentication-operator/blob/master/pkg/controllers/configobservation/oauth/observe_proxy_trusted_ca.go#L15 so it would be a case of confirming that this file contains the new ca before proceeding with oidc login.
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.
b742e6b to
36afc82
Compare
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/extended/authentication/component_proxy.go (3)
228-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the direct access grant setup into a helper.
Lines 228-234 repeat verbatim in
testHotReloadCAFileChange(lines 294-300) andtestBypassProxyNoProxyHost(lines 463-469). Move the block into one helper, for exampleenableDirectAccessGrants(kcSetup). A previous reviewer requested the same extraction.🤖 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 228 - 234, Extract the repeated direct-access-grant setup into an `enableDirectAccessGrants` helper near the existing test helpers, including client lookup, error assertions, and `UpdateClientRaw`. Replace the duplicated blocks in the current test and `testHotReloadCAFileChange` and `testBypassProxyNoProxyHost` with calls to this helper.
402-409: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry the Squid reconfigure exec.
The cert propagation check on lines 383-399 polls, but this
execruns once.squidPodswas listed on line 376, before the poll. If the Squid pod restarts or the API server drops the exec stream, line 409 fails the whole spec on a transient error. Wrap the reconfigure in the samewait.PollUntilContextTimeoutpattern, and re-list the pod name inside the poll.🤖 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 402 - 409, Update the Squid reconfiguration block after “Reconfiguring Squid to pick up new cert” to use the existing wait.PollUntilContextTimeout pattern, retrying transient exec failures until the timeout. Re-list the Squid pod inside each poll attempt and use the current pod name for oc.AsAdmin().Run("exec"), preserving the existing failure message after polling exhausts.
193-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the two trusted CA ConfigMap helpers.
This helper creates
e2e-proxy-trusted-cawithoutcomponentProxyTestLabels(). Lines 519-527 createe2e-proxy-cawith those labels and a different cleanup style. The two paths do the same work and will drift. MakecreateTrustedCAConfigMapaccept the ConfigMap name, applycomponentProxyTestLabels(), and call it fromtestOIDCIdPThroughComponentProxy. A previous reviewer requested the same consolidation for thetrustedCAtest.♻️ Proposed refactor
-func createTrustedCAConfigMap(ctx context.Context, oc *exutil.CLI, caCertPEM []byte) string { +func createTrustedCAConfigMap(ctx context.Context, oc *exutil.CLI, configMapName string, caCertPEM []byte) string { g.GinkgoHelper() - configMapName := "e2e-proxy-trusted-ca" kubeClient := oc.AdminKubeClient() _, err := kubeClient.CoreV1().ConfigMaps("openshift-config").Create(ctx, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: configMapName, Namespace: "openshift-config", + Labels: componentProxyTestLabels(), },🤖 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 193 - 213, Consolidate the trusted CA ConfigMap setup by updating createTrustedCAConfigMap to accept the ConfigMap name, include componentProxyTestLabels() in its ObjectMeta, and retain the shared cleanup behavior. Replace the separate e2e-proxy-ca creation in testOIDCIdPThroughComponentProxy and the trustedCA test path with calls to this helper, passing each required name.
🤖 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 50-64: Move each matching o.Expect(err).NotTo(o.HaveOccurred())
assertion before registering the returned cleanup: authRestore, proxyCleanup,
and kcCleanups in test/extended/authentication/component_proxy.go lines 50-64;
the idpCleanups captured by g.DeferCleanup at lines 251-253, 317-319, 492-494,
and 554-556. No other changes are needed.
- Around line 215-225: Update getOAuthServerPodIPs to append only non-empty
p.Status.PodIP values, then require the resulting ips slice to be non-empty
before returning it, preserving the existing pod-list error and empty-pod
validations.
- Around line 505-510: After deleting proxyNamespace in the test flow before
assertOIDCLogin, wait until the Squid pods are gone or the namespace no longer
exists so the bypass assertion cannot pass through a still-serving proxy. Add
the apierrors import and use the existing polling/wait conventions, applying the
same readiness gap fix to testFallbackOnProxyRemoval before its login assertion.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy.go`:
- Around line 228-234: Extract the repeated direct-access-grant setup into an
`enableDirectAccessGrants` helper near the existing test helpers, including
client lookup, error assertions, and `UpdateClientRaw`. Replace the duplicated
blocks in the current test and `testHotReloadCAFileChange` and
`testBypassProxyNoProxyHost` with calls to this helper.
- Around line 402-409: Update the Squid reconfiguration block after
“Reconfiguring Squid to pick up new cert” to use the existing
wait.PollUntilContextTimeout pattern, retrying transient exec failures until the
timeout. Re-list the Squid pod inside each poll attempt and use the current pod
name for oc.AsAdmin().Run("exec"), preserving the existing failure message after
polling exhausts.
- Around line 193-213: Consolidate the trusted CA ConfigMap setup by updating
createTrustedCAConfigMap to accept the ConfigMap name, include
componentProxyTestLabels() in its ObjectMeta, and retain the shared cleanup
behavior. Replace the separate e2e-proxy-ca creation in
testOIDCIdPThroughComponentProxy and the trustedCA test path with calls to this
helper, passing each required name.
🪄 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: 3eef59ee-0762-4f41-adbc-177fe27a1111
📒 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
🚧 Files skipped from review as they are similar to previous changes (3)
- test/extended/authentication/operator_status_helpers.go
- test/extended/authentication/keycloak_client.go
- test/extended/authentication/component_proxy_helpers.go
| authRestore, err := saveAndRestoreAuthState(ctx, oc) | ||
| cleanups = append(cleanups, 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) | ||
| cleanups = append(cleanups, proxyCleanup) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| g.By("Deploying Keycloak (without registering IdP yet)") | ||
| var kcCleanups []removalFunc | ||
| kcSetup, kcCleanups, err = deployKeycloakForProxy(ctx, oc) | ||
| cleanups = append(cleanups, kcCleanups...) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Assert the helper error before you register its cleanup. Every site stores or registers a cleanup value that the helper returned, and only then asserts the helper error. If the helper fails, the cleanup value can be nil or partially populated, and the cleanup path can panic and hide the real assertion failure. A previous review raised the same concern.
test/extended/authentication/component_proxy.go#L50-L64: move eacho.Expect(err).NotTo(o.HaveOccurred())above the matchingcleanups = append(...)call forauthRestore,proxyCleanup, andkcCleanups.test/extended/authentication/component_proxy.go#L251-L253: move theo.Expect(err).NotTo(o.HaveOccurred())on line 254 above theg.DeferCleanupthat capturesidpCleanups.test/extended/authentication/component_proxy.go#L317-L319: move theo.Expect(err).NotTo(o.HaveOccurred())on line 320 above theg.DeferCleanupthat capturesidpCleanups.test/extended/authentication/component_proxy.go#L492-L494: move theo.Expect(err).NotTo(o.HaveOccurred())on line 495 above theg.DeferCleanupthat capturesidpCleanups.test/extended/authentication/component_proxy.go#L554-L556: move theo.Expect(err).NotTo(o.HaveOccurred())on line 557 above theg.DeferCleanupthat capturesidpCleanups.
📍 Affects 1 file
test/extended/authentication/component_proxy.go#L50-L64(this comment)test/extended/authentication/component_proxy.go#L251-L253test/extended/authentication/component_proxy.go#L317-L319test/extended/authentication/component_proxy.go#L492-L494test/extended/authentication/component_proxy.go#L554-L556
🤖 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 50 - 64, Move
each matching o.Expect(err).NotTo(o.HaveOccurred()) assertion before registering
the returned cleanup: authRestore, proxyCleanup, and kcCleanups in
test/extended/authentication/component_proxy.go lines 50-64; the idpCleanups
captured by g.DeferCleanup at lines 251-253, 317-319, 492-494, and 554-556. No
other changes are needed.
| func getOAuthServerPodIPs(ctx context.Context, oc *exutil.CLI) []string { | ||
| g.GinkgoHelper() | ||
| oauthPods, err := oc.AdminKubeClient().CoreV1().Pods("openshift-authentication").List(ctx, metav1.ListOptions{LabelSelector: "app=oauth-openshift"}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(oauthPods.Items).NotTo(o.BeEmpty()) | ||
| var ips []string | ||
| for _, p := range oauthPods.Items { | ||
| ips = append(ips, p.Status.PodIP) | ||
| } | ||
| return ips | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip pods without an assigned IP.
p.Status.PodIP is empty for a pod that is not yet scheduled or running. The function then returns an empty string, and waitForProxyTrafficFromTo matches proxy log lines against that empty value. The traffic check becomes unreliable. Filter empty IPs and require at least one result.
🛠️ Proposed fix
var ips []string
for _, p := range oauthPods.Items {
- ips = append(ips, p.Status.PodIP)
+ if p.Status.PodIP == "" {
+ continue
+ }
+ ips = append(ips, p.Status.PodIP)
}
+ o.Expect(ips).NotTo(o.BeEmpty(), "at least one oauth-server pod should have an IP")
return ips📝 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 getOAuthServerPodIPs(ctx context.Context, oc *exutil.CLI) []string { | |
| g.GinkgoHelper() | |
| oauthPods, err := oc.AdminKubeClient().CoreV1().Pods("openshift-authentication").List(ctx, metav1.ListOptions{LabelSelector: "app=oauth-openshift"}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(oauthPods.Items).NotTo(o.BeEmpty()) | |
| var ips []string | |
| for _, p := range oauthPods.Items { | |
| ips = append(ips, p.Status.PodIP) | |
| } | |
| return ips | |
| } | |
| func getOAuthServerPodIPs(ctx context.Context, oc *exutil.CLI) []string { | |
| g.GinkgoHelper() | |
| oauthPods, err := oc.AdminKubeClient().CoreV1().Pods("openshift-authentication").List(ctx, metav1.ListOptions{LabelSelector: "app=oauth-openshift"}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(oauthPods.Items).NotTo(o.BeEmpty()) | |
| var ips []string | |
| for _, p := range oauthPods.Items { | |
| if p.Status.PodIP == "" { | |
| continue | |
| } | |
| ips = append(ips, p.Status.PodIP) | |
| } | |
| o.Expect(ips).NotTo(o.BeEmpty(), "at least one oauth-server pod should have an IP") | |
| return ips | |
| } |
🤖 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 215 - 225,
Update getOAuthServerPodIPs to append only non-empty p.Status.PodIP values, then
require the resulting ips slice to be non-empty before returning it, preserving
the existing pod-list error and empty-pod validations.
| g.By("Deleting Squid proxy namespace to prove noProxy bypasses it") | ||
| err = oc.AdminKubeClient().CoreV1().Namespaces().Delete(ctx, proxyNamespace, metav1.DeleteOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| g.By("Verifying OIDC login works after setting proxy with noProxy") | ||
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wait for Squid to become unreachable before asserting the bypass login.
Namespace deletion is asynchronous. The Squid pod can keep serving requests while the namespace terminates. The login on line 510 can then succeed through the proxy, and the test passes without proving that noProxy bypassed it. Poll until the Squid pods are gone, or until the namespace no longer exists, before line 510. testFallbackOnProxyRemoval has the same gap at lines 604-612, but it waits for the operator to reconcile first.
🛠️ Proposed fix
g.By("Deleting Squid proxy namespace to prove noProxy bypasses it")
err = oc.AdminKubeClient().CoreV1().Namespaces().Delete(ctx, proxyNamespace, metav1.DeleteOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
+ g.By("Waiting for the Squid proxy pods to disappear")
+ err = wait.PollUntilContextTimeout(ctx, 10*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {
+ pods, listErr := oc.AdminKubeClient().CoreV1().Pods(proxyNamespace).List(ctx, metav1.ListOptions{LabelSelector: "app=squid-proxy"})
+ if listErr != nil {
+ return apierrors.IsNotFound(listErr), nil
+ }
+ return len(pods.Items) == 0, nil
+ })
+ o.Expect(err).NotTo(o.HaveOccurred(), "Squid proxy pods should be gone")
+
g.By("Verifying OIDC login works after setting proxy with noProxy")
assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup)This diff needs an import of apierrors "k8s.io/apimachinery/pkg/api/errors".
📝 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("Deleting Squid proxy namespace to prove noProxy bypasses it") | |
| err = oc.AdminKubeClient().CoreV1().Namespaces().Delete(ctx, proxyNamespace, metav1.DeleteOptions{}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| g.By("Verifying OIDC login works after setting proxy with noProxy") | |
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) | |
| g.By("Deleting Squid proxy namespace to prove noProxy bypasses it") | |
| err = oc.AdminKubeClient().CoreV1().Namespaces().Delete(ctx, proxyNamespace, metav1.DeleteOptions{}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| g.By("Waiting for the Squid proxy pods to disappear") | |
| err = wait.PollUntilContextTimeout(ctx, 10*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { | |
| pods, listErr := oc.AdminKubeClient().CoreV1().Pods(proxyNamespace).List(ctx, metav1.ListOptions{LabelSelector: "app=squid-proxy"}) | |
| if listErr != nil { | |
| return apierrors.IsNotFound(listErr), nil | |
| } | |
| return len(pods.Items) == 0, nil | |
| }) | |
| o.Expect(err).NotTo(o.HaveOccurred(), "Squid proxy pods should be gone") | |
| g.By("Verifying OIDC login works after setting proxy with noProxy") | |
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) |
🤖 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 505 - 510,
After deleting proxyNamespace in the test flow before assertOIDCLogin, wait
until the Squid pods are gone or the namespace no longer exists so the bypass
assertion cannot pass through a still-serving proxy. Add the apierrors import
and use the existing polling/wait conventions, applying the same readiness gap
fix to testFallbackOnProxyRemoval before its login assertion.
There was a problem hiding this comment.
This is actually an issue in my test as well, I need to add a helper for waiting for the namespace to be truly deleted. I will add that helper, test it and let you know.
There was a problem hiding this comment.
@tchap, acknowledged. A helper that waits for namespace deletion will make both test paths deterministic. Please update the bypass and fallback assertions, test the helper, and let me know when the change is ready.
You are interacting with an AI system.
36afc82 to
6d53cd8
Compare
|
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. |
6d53cd8 to
3d05573
Compare
|
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. |
|
Scheduling required tests: |
| "k8s.io/apimachinery/pkg/util/sets" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
| "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/rest" |
There was a problem hiding this comment.
I think that we usually put k8s import block before OCP imports, but no big deal. I mean that the original ordering was correct.
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.
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.
…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.
3d05573 to
4f11d67
Compare
this change migrate tests over + add helper for to/from traffic check
4f11d67 to
2db333f
Compare
|
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