WIP: CNTRLPLANE-3789: Add e2e tests for component-scoped proxy - #949
Open
tchap wants to merge 2 commits into
Open
WIP: CNTRLPLANE-3789: Add e2e tests for component-scoped proxy#949tchap wants to merge 2 commits into
tchap wants to merge 2 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe operator resolves feature-gated component proxy settings and trusted CA references, applies them to transports, controller checks, OAuth deployments, and configuration observation, and adds serial component-proxy end-to-end coverage. ChangesComponent proxy integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticationCR
participant ProxyConfigController
participant OAuthDeploymentController
participant OAuthServer
AuthenticationCR->>ProxyConfigController: update component proxy
ProxyConfigController->>ProxyConfigController: resolve proxy and validate IdP connectivity
AuthenticationCR->>OAuthDeploymentController: provide proxy and TrustedCAName
OAuthDeploymentController->>OAuthServer: update proxy environment and mounted CA
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings, 1 inconclusive)
✅ Passed checks (9 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controllers/configobservation/oauth/idp_conversions.go (1)
318-332: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound both OIDC probes with request deadlines.
A proxy that accepts the connection but never responds can indefinitely pin the configuration-observer worker.
pkg/controllers/configobservation/oauth/idp_conversions.go#L318-L332: create the discovery request with a timeout context before callingRoundTrip.pkg/controllers/configobservation/oauth/idp_conversions.go#L376-L424: apply a timeout context to the token request, optionally backed by anhttp.Client.Timeout.As per path instructions, use “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 `@pkg/controllers/configobservation/oauth/idp_conversions.go` around lines 318 - 332, Bound both OIDC HTTP probes using context.Context cancellation and timeouts: in discoverOpenIDURLs, create the discovery request with a timeout context before rt.RoundTrip; in the token-request flow covering lines 376-424, apply an equivalent timeout context and optionally configure http.Client.Timeout. Update both affected sites in pkg/controllers/configobservation/oauth/idp_conversions.go (318-332 and 376-424), preserving existing request behavior while ensuring unresponsive proxies cannot block indefinitely.Source: Path instructions
pkg/controllers/proxyconfig/proxyconfig_controller.go (1)
59-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWatch every lister-backed input used by validation.
OAuth changes and updates to the component proxy’s dynamic
openshift-config/<trustedCAName>ConfigMap do not enqueue this controller, leaving validation stale until the 60-minute resync. Accept the OAuth informer and theopenshift-configConfigMap informer, then register both withWithInformers; update thestarter.gowiring accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controllers/proxyconfig/proxyconfig_controller.go` around lines 59 - 90, The proxyConfigChecker controller currently watches only routes, auth configuration, operator authentication, and selected CA ConfigMaps, so validation inputs from OAuth and the dynamic openshift-config trusted-CA ConfigMap can remain stale. Update the controller factory and its starter.go wiring to accept the OAuth informer and the openshift-config ConfigMap informer, register both with WithInformers, and preserve the existing filtered ConfigMap watches.
🤖 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 `@cmd/cluster-authentication-operator-tests-ext/main.go`:
- Around line 89-97: The existing operator serial suite selector also matches
component-proxy tests, causing duplicate execution. Update its qualifier in the
existing serial suite registration to exclude names containing
“[ComponentProxy]”, while leaving the dedicated component-proxy suite selector
unchanged.
In `@pkg/controllers/common/proxy.go`:
- Around line 101-104: Update mergeNoProxy to sort the combined entries
deterministically before joining them, replacing the unordered UnsortedList flow
with an ordered list while preserving all existing entries and comma-separated
output.
In `@pkg/controllers/configobservation/oauth/observe_proxy_trusted_ca_test.go`:
- Around line 188-190: Update the test assertion around the observed
configuration to compare tt.expected directly with observed, removing the
NestedString extraction and ignored error returns. Preserve the test’s existing
assertion framework while validating the complete configuration, including
fallback fields.
In `@pkg/controllers/customroute/custom_route_conditions.go`:
- Around line 165-171: Update the trusted CA handling around
transport.LoadCAData in the custom route conditions flow to check the boolean
result from rootCAs.AppendCertsFromPEM(caData). When no certificate is parsed,
return a configuration error for the invalid trusted proxy CA bundle instead of
continuing; preserve the existing load error propagation and successful append
behavior.
In `@pkg/controllers/deployment/deployment_controller.go`:
- Around line 382-392: Update the ConfigMap construction in the trusted CA copy
flow to preserve both sourceCM.Data and sourceCM.BinaryData when creating
targetCM. Keep the existing namespace, name, and apply behavior in the
surrounding deployment controller logic unchanged.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Around line 143-150: Update the no-IdP branch in the validation flow around
extractIdPURLs to clear p.lastIdPValidationHash before returning. Keep the
existing early return, and preserve hash comparison behavior when at least one
IdP remains.
In `@pkg/transport/transport_test.go`:
- Around line 303-308: Do not ignore test fixture insertion errors: in
pkg/transport/transport_test.go:303-308, update newConfigMapLister to accept
*testing.T and assert each indexer.Add result; in
pkg/controllers/common/proxy_test.go:352-357, replace the discarded
Authentication fixture insertion error with an assertion using the test handle.
Preserve the existing fixture setup and lister behavior.
In `@pkg/transport/transport.go`:
- Around line 51-58: Update the CA aggregation loop around LoadCAData so each
ConfigMap value is separated from the next PEM bundle by an explicit newline,
including when the loaded data lacks a trailing newline. Preserve the existing
error propagation and append all certificate data into caData.
- Around line 60-62: Update the no-component-proxy branch in TransportFor to
return a transport with proxy resolution disabled, rather than delegating to the
default environment-aware transport. Preserve the existing behavior for
configurations that provide CA data or HTTP/HTTPS proxy settings.
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 63-65: Update the error branch in the authentication GET flow
within the component proxy test to propagate the returned API error instead of
returning a nil error. Preserve the existing false result while passing err
through to the caller so cleanup failures are reported immediately.
---
Outside diff comments:
In `@pkg/controllers/configobservation/oauth/idp_conversions.go`:
- Around line 318-332: Bound both OIDC HTTP probes using context.Context
cancellation and timeouts: in discoverOpenIDURLs, create the discovery request
with a timeout context before rt.RoundTrip; in the token-request flow covering
lines 376-424, apply an equivalent timeout context and optionally configure
http.Client.Timeout. Update both affected sites in
pkg/controllers/configobservation/oauth/idp_conversions.go (318-332 and
376-424), preserving existing request behavior while ensuring unresponsive
proxies cannot block indefinitely.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Around line 59-90: The proxyConfigChecker controller currently watches only
routes, auth configuration, operator authentication, and selected CA ConfigMaps,
so validation inputs from OAuth and the dynamic openshift-config trusted-CA
ConfigMap can remain stale. Update the controller factory and its starter.go
wiring to accept the OAuth informer and the openshift-config ConfigMap informer,
register both with WithInformers, and preserve the existing filtered ConfigMap
watches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35b76f36-3286-4f09-8779-6c1a053e7f71
⛔ Files ignored due to path filters (55)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/project/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (31)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/common/proxy.gopkg/controllers/common/proxy_test.gopkg/controllers/configobservation/configobservercontroller/observe_config_controller.gopkg/controllers/configobservation/interfaces.gopkg/controllers/configobservation/oauth/idp_conversions.gopkg/controllers/configobservation/oauth/idp_conversions_test.gopkg/controllers/configobservation/oauth/observe_idps.gopkg/controllers/configobservation/oauth/observe_idps_test.gopkg/controllers/configobservation/oauth/observe_proxy_trusted_ca.gopkg/controllers/configobservation/oauth/observe_proxy_trusted_ca_test.gopkg/controllers/customroute/custom_route_conditions.gopkg/controllers/customroute/custom_route_controller.gopkg/controllers/deployment/default_deployment.gopkg/controllers/deployment/deployment_controller.gopkg/controllers/deployment/deployment_controller_test.gopkg/controllers/oauthendpoints/oauth_endpoints_controller.gopkg/controllers/proxyconfig/proxyconfig_controller.gopkg/controllers/proxyconfig/proxyconfig_controller_test.gopkg/internal/transporttest/transporttest.gopkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/operator/replacement_starter.gopkg/operator/starter.gopkg/transport/transport.gopkg/transport/transport_test.gotest-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-body-system-COLON-openshift-COLON-openshift-authenticator-.yamltest-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-metadata-system-COLON-openshift-COLON-openshift-authenticator-.yamltest-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-body-oauth-openshift.yamltest-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-metadata-oauth-openshift.yamltest/e2e-component-proxy/component_proxy.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-component-proxy/component_proxy.go`:
- Line 119: Remove the g.GinkgoWriter.Printf call that logs proxyURL, or replace
its value with a non-sensitive redacted indicator so the in-cluster proxy
hostname is never written to test artifacts.
- Around line 135-136: Handle and propagate or explicitly fail on the error
returned by the Secrets(...).Delete call in the cleanup flow, rather than
discarding it with “_”. Ensure cleanup reports the deletion failure so leftover
cluster state cannot contaminate subsequent tests.
In `@test/library/proxy.go`:
- Around line 14-16: The proxy helper functions in test/library/proxy.go are
still placeholders and cause the e2e suite to panic. Implement DeploySquidProxy
and every other exported helper in the file according to their declared
contracts, including proxy setup, returned namespace/URL values, and cleanup
behavior, so the tests can execute without panics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33c9dd03-651b-4433-a7b0-db01e6b8c2bc
📒 Files selected for processing (2)
test/e2e-component-proxy/component_proxy.gotest/library/proxy.go
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-component-proxy/component_proxy.go (1)
229-231: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn event-list failures from the polling callback.
Line 231 converts an API failure into another retry, hiding the cause for up to ten minutes. Return
errso the test fails immediately.Proposed fix
if err != nil { g.GinkgoWriter.Printf("failed to list events: %v\n", err) - return false, nil + return false, err }As per path instructions, “Never ignore error returns.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-component-proxy/component_proxy.go` around lines 229 - 231, Update the event-list error branch in the polling callback to return the encountered err instead of returning false, nil. Preserve the existing failure log and successful polling behavior so API failures propagate immediately and stop retries.Source: Path instructions
♻️ Duplicate comments (1)
test/library/proxy.go (1)
52-93: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftImplement the proxy helpers before these specs run.
The helpers still panic.
CheckFeatureGateEnabledOrSkipruns before setup in every new spec, so all registered Component Proxy tests abort immediately; the Squid, NetworkPolicy, deployment-verification, and traffic helpers block the remaining assertions too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/library/proxy.go` around lines 52 - 93, Implement all proxy helper functions in test/library/proxy.go instead of leaving them as panics, including CheckFeatureGateEnabledOrSkip before test setup and the Squid deployment, NetworkPolicy, OAuth environment, logs, traffic-wait, deployment-verification, and trusted-CA sync helpers. Use the provided Kubernetes clients to perform the documented operations, assertions, cleanup, and feature-gate skip behavior so all Component Proxy specs can execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-component-proxy/component_proxy.go`:
- Line 55: Remove or redact the internal endpoint values logged by the component
proxy test, including proxyURL in the GinkgoWriter output and
kcClient.IssuerURL() at the additionally affected log site. Preserve useful
diagnostic context without exposing in-cluster hostnames.
In `@test/library/proxy.go`:
- Around line 20-21: Update SaveAndRestoreProxyConfig to replace context.TODO()
with separate deadline-bound contexts for the initial configuration snapshot and
the cleanup restore path. Apply each timeout context to its corresponding
GET/UPDATE calls, ensure cancellation is handled, and keep cleanup bounded even
if the API stalls.
- Around line 31-47: Update the cleanup logic around the Authentication proxy
restoration to call t.Errorf instead of t.Logf when fetching the resource,
updating its proxy, or waiting via WaitForOperatorToPickUpChanges fails.
Preserve the existing early returns, but ensure each failure marks the spec as
failed so proxy restoration or reconciliation errors cannot leave later tests
poisoned.
---
Outside diff comments:
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 229-231: Update the event-list error branch in the polling
callback to return the encountered err instead of returning false, nil. Preserve
the existing failure log and successful polling behavior so API failures
propagate immediately and stop retries.
---
Duplicate comments:
In `@test/library/proxy.go`:
- Around line 52-93: Implement all proxy helper functions in
test/library/proxy.go instead of leaving them as panics, including
CheckFeatureGateEnabledOrSkip before test setup and the Squid deployment,
NetworkPolicy, OAuth environment, logs, traffic-wait, deployment-verification,
and trusted-CA sync helpers. Use the provided Kubernetes clients to perform the
documented operations, assertions, cleanup, and feature-gate skip behavior so
all Component Proxy specs can execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4288bd75-d622-46e5-a4a2-38bd1f58545a
📒 Files selected for processing (2)
test/e2e-component-proxy/component_proxy.gotest/library/proxy.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-component-proxy/component_proxy.go (1)
265-288: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
proxyCleanupis reassigned, so the Squid proxy is never cleaned up.Line 265 binds
proxyCleanupto the Squid cleanup, and theDeferCleanupat Lines 266-269 closes over that variable. Line 273 uses:=withoperatorAuth(the only new name) in the same block scope, so it reassignsproxyCleanupto theSaveAndRestoreProxyConfigcleanup rather than declaring a new variable. As a result the "removing Squid proxy" deferred closure invokes the restore function instead, the Squid deployment is never torn down (resource leak that can contaminate subsequent serial tests), and the restore cleanup runs twice (Lines 269 and 287).🐛 Proposed fix: use a distinct name for the restore cleanup
g.By("Saving original proxy config for cleanup") - operatorAuth, proxyCleanup := test.SaveAndRestoreProxyConfig(t, clients.OperatorClient, clients.ConfigClient) + operatorAuth, proxyRestore := test.SaveAndRestoreProxyConfig(t, clients.OperatorClient, clients.ConfigClient) @@ g.GinkgoWriter.Println("cleaning up: deleting fake IdP secret") _ = clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{}) - proxyCleanup() + proxyRestore() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-component-proxy/component_proxy.go` around lines 265 - 288, Rename the cleanup returned by SaveAndRestoreProxyConfig in the operatorAuth setup to a distinct symbol, such as a proxy-config restore cleanup, so it does not reassign the Squid proxy cleanup captured by the first DeferCleanup. Update the later deferred cleanup to invoke the renamed restore function, while leaving the original proxyCleanup closure responsible only for deleting the Squid proxy.
🤖 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.
Outside diff comments:
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 265-288: Rename the cleanup returned by SaveAndRestoreProxyConfig
in the operatorAuth setup to a distinct symbol, such as a proxy-config restore
cleanup, so it does not reassign the Squid proxy cleanup captured by the first
DeferCleanup. Update the later deferred cleanup to invoke the renamed restore
function, while leaving the original proxyCleanup closure responsible only for
deleting the Squid proxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44fdc303-375b-4320-a04d-9ddb59cb0e85
📒 Files selected for processing (3)
test/e2e-component-proxy/component_proxy.gotest/library/keycloakidp.gotest/library/proxy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/proxy.go
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 21, 2026 10:31
1575d36 to
ce6e2da
Compare
openshift-ci
Bot
removed
the
needs-rebase
tchap
force-pushed
the
disconnected-env-e2e
branch
8 times, most recently
from
July 29, 2026 10:34
cadb1d6 to
24d0588
Compare
Contributor
Author
|
/retitle CNTRLPLANE-3789: Add e2e tests for component-scoped proxy |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
@tchap: This pull request references CNTRLPLANE-3789 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 29, 2026 10:41
24d0588 to
f46122e
Compare
tchap
marked this pull request as draft
July 29, 2026 10:41
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 29, 2026 10:54
f46122e to
b58806f
Compare
Introduce a new test/e2e-component-proxy suite gated behind the AuthenticationComponentProxy feature gate with five Serial tests: - A1: OIDC IdP validation through component proxy (HTTP and HTTPS with trustedCA variants) - A2: graceful fallback when spec.proxy is removed - C1: Degraded condition when spec.proxy points to an unreachable host - C2: IdPEndpointUnreachable warning event when an IdP issuer is unreachable through the proxy (without going Degraded) Add test/library/proxy.go with shared helpers: DeploySquidProxy (RHEL 10 Squid image, HTTP+HTTPS, self-signed TLS), NetworkPolicy deployment, proxy log retrieval with time filtering, SaveAndRestoreProxyConfig, VerifyOAuthServerDeploymentProxyConfig, VerifyTrustedCAConfigMapSynced, and CheckFeatureGateEnabledOrSkip. Split AddKeycloakIDP into DeployKeycloak and AddKeycloakOIDCIdP so tests can control ordering — deploy Keycloak, apply NetworkPolicy, set the proxy, then register the IdP. AddKeycloakIDP remains as a convenience wrapper.
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 29, 2026 11:11
417f1c9 to
7bafc12
Compare
When all IdPs were removed from the OAuth config, the stale lastIdPValidationHash was preserved. Re-adding the same IdPs with the same proxy config produced a matching hash, causing validateIdPConnectivity to skip validation entirely. Clear the hash in the no-IdP early return so re-added IdPs are always validated.
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 29, 2026 11:34
7bafc12 to
9bd8a65
Compare
tchap
marked this pull request as ready for review
July 29, 2026 12:29
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/retitle WIP: CNTRLPLANE-3789: Add e2e tests for component-scoped proxy |
openshift-ci
Bot
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a new test/e2e-component-proxy suite gated behind the
AuthenticationComponentProxy feature gate with five Serial tests:
with trustedCA variants)
unreachable through the proxy (without going Degraded)
Add test/library/proxy.go with shared helpers: DeploySquidProxy
(RHEL 10 Squid image, HTTP+HTTPS, self-signed TLS), NetworkPolicy
deployment, proxy log retrieval with time filtering,
SaveAndRestoreProxyConfig, VerifyOAuthServerDeploymentProxyConfig,
VerifyTrustedCAConfigMapSynced, and CheckFeatureGateEnabledOrSkip.
Split AddKeycloakIDP into DeployKeycloak and AddKeycloakOIDCIdP so
tests can control ordering — deploy Keycloak, apply NetworkPolicy,
set the proxy, then register the IdP. AddKeycloakIDP remains as a
convenience wrapper.
Summary by CodeRabbit
NO_PROXYhandling.