🌱 OPRUN-4573: tests: add unit and e2e tests for HTTPS_PROXY support#2654
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds unit + e2e test coverage around HTTPS proxying for catalog traffic, including new godog steps to reconfigure controller deployments and a lightweight “recording proxy” used to assert CONNECT tunneling.
Changes:
- Register new proxy-related godog steps and add scenario cleanup to revert modified deployments / stop the proxy.
- Add new e2e feature scenarios for “dead proxy blocks fetch” and “recording proxy observes CONNECT to catalogd”.
- Add unit tests intended to validate HTTPS CONNECT proxy tunneling behavior for
BuildHTTPClient.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Registers proxy step definitions in the e2e step registry. |
| test/e2e/steps/proxy_steps.go | Implements deployment env patching helpers and an in-process CONNECT recording proxy. |
| test/e2e/steps/hooks.go | Adds scenario context fields and cleanup to stop proxy + restore deployments. |
| test/e2e/features/proxy.feature | Adds two e2e scenarios covering dead proxy and recording proxy routing assertions. |
| internal/shared/util/http/httputil_test.go | Adds unit tests around proxy CONNECT tunneling behavior for BuildHTTPClient. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tmshort
force-pushed
the
proxy-tests
branch
2 times, most recently
from
43f7355 to
699c428
Compare
There was a problem hiding this comment.
Pull request overview
Adds new unit and end-to-end coverage intended to validate HTTPS proxy (CONNECT tunneling) behavior for outbound catalog fetches, including scenarios where the proxy is unreachable or where traffic must be observed flowing through a proxy.
Changes:
- Registers new Godog step definitions for proxy-related E2E scenarios and adds per-scenario cleanup to restore modified deployments.
- Adds a new E2E feature file covering “dead proxy” and “recording proxy” scenarios for operator-controller catalog requests.
- Adds unit tests around
BuildHTTPClientproxy CONNECT tunneling behavior using an in-test CONNECT proxy.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Registers the new proxy-related step set. |
| test/e2e/steps/proxy_steps.go | Implements an in-process CONNECT proxy + deployment env patching helpers and steps. |
| test/e2e/steps/hooks.go | Tracks/restores deployment env changes and stops the in-process proxy during scenario cleanup. |
| test/e2e/features/proxy.feature | Adds E2E scenarios validating failure via dead proxy and success/verification via recording proxy. |
| internal/shared/util/http/httputil_test.go | Adds unit tests for HTTPS CONNECT tunneling through a proxy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds automated coverage for outbound HTTPS proxying of operator-controller’s catalog fetches, using both unit-level CONNECT proxy tunneling tests and end-to-end scenarios that configure HTTPS_PROXY on the running deployment.
Changes:
- Register new e2e proxy-related Godog steps and add feature scenarios validating proxy failure/success cases.
- Add e2e infrastructure: an in-process recording CONNECT proxy plus deployment env patch/restore helpers.
- Add unit tests that exercise HTTPS-over-CONNECT tunneling behavior for the HTTP client used for catalog traffic.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Registers proxy steps with the existing Godog scenario context. |
| test/e2e/steps/proxy_steps.go | Implements recording CONNECT proxy + deployment patch/restore helpers + step definitions. |
| test/e2e/steps/hooks.go | Extends scenario context and cleanup to restore patched deployments and stop proxy. |
| test/e2e/features/proxy.feature | Adds two e2e scenarios for dead-proxy failure and recording-proxy success. |
| internal/shared/util/http/httputil_test.go | Adds unit tests for HTTPS CONNECT tunneling and proxy rejection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR adds coverage for HTTPS proxy behavior used by the operator-controller when fetching catalog content from catalogd, and ensures the shared HTTP client respects proxy environment variables.
Changes:
- Update
BuildHTTPClientto honorHTTPS_PROXY/NO_PROXYviahttp.ProxyFromEnvironment. - Add unit tests validating CONNECT tunneling through a proxy and failure behavior when CONNECT is rejected.
- Add new e2e feature + step implementations to exercise proxy behavior scenarios (dead proxy + recording proxy).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/shared/util/http/httputil.go |
Sets Transport.Proxy so runtime proxy env vars are honored. |
internal/shared/util/http/httputil_test.go |
Adds unit tests for CONNECT proxy tunneling and rejection behavior. |
test/e2e/features/proxy.feature |
Introduces e2e scenarios for HTTPS proxy behavior. |
test/e2e/steps/proxy_steps.go |
Implements e2e steps including a recording CONNECT proxy and deployment env patching helpers. |
test/e2e/steps/hooks.go |
Adds scenario cleanup for stopping the proxy and restoring patched deployments. |
test/e2e/steps/steps.go |
Registers the new proxy-related steps. |
Makefile |
Increases E2E timeout to accommodate added scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2654 +/- ##
=======================================
Coverage 67.99% 67.99%
=======================================
Files 144 144
Lines 10573 10573
=======================================
Hits 7189 7189
Misses 2865 2865
Partials 519 519
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds HTTPS proxy support validation across unit and end-to-end tests, and updates the shared HTTP client builder to honor HTTPS_PROXY/NO_PROXY at runtime.
Changes:
- Update
BuildHTTPClientto setTransport.Proxy = http.ProxyFromEnvironment. - Add unit tests that validate HTTPS CONNECT proxy tunneling behavior and failure modes.
- Add e2e proxy scenarios plus step implementations (including an in-process recording CONNECT proxy) and Makefile support for passing Godog args.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Registers the new proxy-related Godog steps. |
| test/e2e/steps/proxy_steps.go | Implements proxy configuration steps, an in-process recording CONNECT proxy, and deployment patch/restore helpers. |
| test/e2e/steps/hooks.go | Adds scenario cleanup for stopping the recording proxy and restoring patched deployments. |
| test/e2e/features/proxy.feature | Adds e2e scenarios that assert proxy behavior (dead proxy + recording proxy). |
| internal/shared/util/http/httputil_test.go | Adds unit tests for proxy configuration, tunneling through CONNECT, and rejection handling. |
| internal/shared/util/http/httputil.go | Ensures the built transport honors proxy env vars. |
| Makefile | Adds GODOG_ARGS passthrough for e2e runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tmshort
changed the title
There was a problem hiding this comment.
Pull request overview
Adds unit + e2e coverage to ensure outbound HTTPS catalog traffic honors HTTPS_PROXY/NO_PROXY, validating both the transport configuration in BuildHTTPClient and the operator behavior when a proxy is unreachable or when a proxy observes CONNECT tunneling.
Changes:
- Set
http.Transport.Proxy = http.ProxyFromEnvironmentinBuildHTTPClient. - Add unit tests for CONNECT tunneling through a proxy and failure when CONNECT is rejected.
- Add e2e feature/steps that (a) verify a dead proxy blocks catalog fetches and (b) verify CONNECT attempts are routed via a recording proxy.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/shared/util/http/httputil.go |
Ensures custom HTTP transport honors HTTPS_PROXY/NO_PROXY. |
internal/shared/util/http/httputil_test.go |
Adds unit tests around proxy CONNECT tunneling and rejection behavior. |
test/e2e/features/proxy.feature |
Introduces e2e scenarios for HTTPS proxy behavior. |
test/e2e/steps/proxy_steps.go |
Implements e2e steps + in-process recording CONNECT proxy and deployment env patching. |
test/e2e/steps/steps.go |
Registers new proxy-related Godog steps. |
test/e2e/steps/hooks.go |
Extends scenario cleanup to stop proxy and restore deployments (args/env). |
test/e2e/steps/tls_steps.go |
Updates deployment restore tracking fields for TLS arg patching. |
Makefile |
Extends e2e timeouts and adds GODOG_ARGS passthrough to go test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds unit and e2e coverage to ensure operator-controller’s outbound HTTPS calls (notably catalogd fetches) honor HTTPS_PROXY/NO_PROXY, including negative-path behavior when the proxy is unreachable or rejects CONNECT.
Changes:
- Set
BuildHTTPClient’s transportProxytohttp.ProxyFromEnvironment. - Add unit tests validating HTTPS CONNECT tunneling via a proxy and failure on rejected CONNECT.
- Add e2e feature + steps to (1) confirm failures through a dead proxy and (2) assert catalog traffic is routed via CONNECT through a recording proxy.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/shared/util/http/httputil.go |
Ensures custom transport honors HTTPS_PROXY/NO_PROXY via ProxyFromEnvironment. |
internal/shared/util/http/httputil_test.go |
Adds proxy tunneling/rejection unit tests for the HTTP client transport behavior. |
test/e2e/features/proxy.feature |
Introduces e2e scenarios covering dead-proxy and recording-proxy CONNECT verification. |
test/e2e/steps/proxy_steps.go |
Implements deployment env patching for proxy vars and an in-process recording CONNECT proxy. |
test/e2e/steps/steps.go |
Registers new Godog steps for proxy scenarios. |
test/e2e/steps/hooks.go |
Extends scenario cleanup to restore deployment env/args and stop the recording proxy. |
test/e2e/steps/tls_steps.go |
Updates deployment restore bookkeeping to align with the expanded restore struct. |
Makefile |
Extends e2e execution to accept optional GODOG_ARGS and adjusts timeouts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds unit and end-to-end coverage to ensure operator-controller’s outbound HTTPS requests (catalog fetches) honor HTTPS_PROXY, including both failure behavior (dead/rejecting proxy) and positive signal that requests are routed via CONNECT.
Changes:
- Update
BuildHTTPClientto opt into proxy env handling by settingTransport.Proxy = http.ProxyFromEnvironment. - Add unit tests validating HTTPS tunneling via an HTTP
CONNECTproxy and failure whenCONNECTis rejected. - Add e2e proxy scenarios plus step implementations, including an in-process recording proxy and deployment env patch/restore plumbing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/tls_steps.go | Updates deployment restore bookkeeping to new deploymentRestore shape for TLS arg patches. |
| test/e2e/steps/steps.go | Registers new Godog steps for proxy configuration and recording-proxy assertions. |
| test/e2e/steps/proxy_steps.go | Implements deployment env patching for HTTPS_PROXY/NO_PROXY and a host-side recording CONNECT proxy. |
| test/e2e/steps/hooks.go | Extends scenario context + cleanup to stop recording proxy and restore deployments (args/env). |
| test/e2e/features/proxy.feature | Adds e2e feature coverage for dead proxy behavior and CONNECT routing verification. |
| internal/shared/util/http/httputil_test.go | Adds unit tests for proxy tunneling and proxy rejection behavior. |
| internal/shared/util/http/httputil.go | Ensures BuildHTTPClient respects HTTPS_PROXY/NO_PROXY via ProxyFromEnvironment. |
| Makefile | Extends e2e runtime/args plumbing (E2E_TIMEOUT, GODOG_ARGS). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds automated coverage for outbound HTTPS proxy behavior so operator-controller honors HTTPS_PROXY/NO_PROXY when fetching catalog content from catalogd, with both unit-level transport verification and end-to-end behavior checks in the e2e suite.
Changes:
- Wire
BuildHTTPClient’s transport to usehttp.ProxyFromEnvironmentand add unit tests that verify CONNECT tunneling and proxy rejection behavior. - Add new e2e feature + step implementations for configuring
HTTPS_PROXYand validating catalog traffic routes via an HTTP CONNECT proxy. - Extend e2e deployment rollback tracking to restore either patched args (TLS) or patched env (proxy), and adjust Makefile e2e timeout/argument passthrough.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/tls_steps.go | Updates deployment restore bookkeeping for TLS arg patching. |
| test/e2e/steps/steps.go | Registers new HTTPS proxy-related godog steps. |
| test/e2e/steps/proxy_steps.go | Adds e2e proxy configuration helpers and an in-process recording CONNECT proxy. |
| test/e2e/steps/hooks.go | Extends scenario cleanup to stop proxy and restore args/env changes. |
| test/e2e/features/proxy.feature | Adds e2e scenarios asserting failure via dead proxy and routing via recording proxy. |
| internal/shared/util/http/httputil_test.go | Adds unit tests validating proxy wiring, tunneling, and failure on CONNECT rejection. |
| internal/shared/util/http/httputil.go | Sets Transport.Proxy = http.ProxyFromEnvironment in BuildHTTPClient. |
| Makefile | Increases e2e timeouts and adds optional GODOG_ARGS passthrough. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds coverage to ensure outbound HTTPS catalog traffic respects HTTPS_PROXY both in the shared HTTP client utility and in end-to-end operator-controller behavior.
Changes:
- Wire
http.Transport.Proxytohttp.ProxyFromEnvironmentinBuildHTTPClient. - Add unit tests validating proxy wiring, CONNECT tunneling, and failure behavior when CONNECT is rejected.
- Add E2E scenarios + step implementations to verify proxy failures are surfaced and that catalog requests route via CONNECT through a recording proxy.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/shared/util/http/httputil.go |
Ensures the shared HTTP client honors HTTPS_PROXY/NO_PROXY via ProxyFromEnvironment. |
internal/shared/util/http/httputil_test.go |
Adds unit tests covering proxy wiring and CONNECT proxy tunneling/rejection behavior. |
test/e2e/features/proxy.feature |
Introduces E2E scenarios validating HTTPS proxy behavior for catalog fetches. |
test/e2e/steps/proxy_steps.go |
Implements deployment env patching for proxy vars and an in-process recording CONNECT proxy for E2E verification. |
test/e2e/steps/steps.go |
Registers new proxy-related Gherkin steps. |
test/e2e/steps/hooks.go |
Extends scenario cleanup to restore env/args patches and stop the recording proxy. |
test/e2e/steps/tls_steps.go |
Updates deployment restore bookkeeping to match the new restore structure. |
Makefile |
Extends E2E timeout and allows passing GODOG_ARGS through go test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
camilamacedo86
left a comment
camilamacedo86
left a comment
There was a problem hiding this comment.
It seems great to move now
/approve
openshift-ci
Bot
added
the
approved
- Set Proxy: http.ProxyFromEnvironment on the custom http.Transport in BuildHTTPClient so HTTPS_PROXY/NO_PROXY env vars are honoured - Add unit tests verifying the transport uses env-based proxy, tunnels connections through an HTTP CONNECT proxy, and fails when the proxy rejects the tunnel - Add an in-process recording proxy and deployment patch helpers to the e2e step library - Add two @httpproxy e2e scenarios: one verifying operator-controller blocks catalog fetches when the proxy is unreachable, one verifying CONNECT requests are routed through a configured proxy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds HTTPS proxy support coverage by wiring Go HTTP clients to respect HTTPS_PROXY/NO_PROXY and introducing unit + e2e tests to validate CONNECT proxy tunneling behavior.
Changes:
- Set
BuildHTTPClienttransport proxy tohttp.ProxyFromEnvironmentand add unit tests for tunneling + proxy rejection. - Add e2e steps + a new feature validating proxy behavior (dead proxy blocks; recording proxy observes CONNECT).
- Enhance e2e cleanup/restore plumbing (deployment restore records for args/env) and allow passing Godog args via Makefile.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/tls_steps.go | Updates deployment restore bookkeeping fields used by TLS patching steps. |
| test/e2e/steps/steps.go | Registers new Godog steps for configuring HTTPS proxy and asserting CONNECT traffic. |
| test/e2e/steps/proxy_steps.go | Implements recording CONNECT proxy + deployment env patch/restore helpers for e2e scenarios. |
| test/e2e/steps/hooks.go | Extends scenario context/cleanup to stop proxy and restore env/args changes. |
| test/e2e/features/proxy.feature | Adds e2e scenarios covering dead proxy and recording proxy CONNECT verification. |
| internal/shared/util/http/httputil_test.go | Adds unit tests validating proxy wiring, tunneling, and rejection behavior. |
| internal/shared/util/http/httputil.go | Ensures custom transport honors HTTPS_PROXY/NO_PROXY via ProxyFromEnvironment. |
| Makefile | Extends e2e test invocation with configurable timeout and optional Godog args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // getDeploymentContainerEnv returns the environment variables for the named | ||
| // container in the given deployment, as a slice of "NAME=VALUE" strings. | ||
| func getDeploymentContainerEnv(deploymentName, namespace, containerName string) ([]string, error) { | ||
| raw, err := k8sClient("get", "deployment", deploymentName, "-n", namespace, "-o", "json") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get deployment %s/%s: %w", namespace, deploymentName, err) | ||
| } | ||
|
|
||
| var dep appsv1.Deployment | ||
| if err := json.Unmarshal([]byte(raw), &dep); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal deployment: %w", err) | ||
| } | ||
|
|
||
| for _, c := range dep.Spec.Template.Spec.Containers { | ||
| if c.Name == containerName { | ||
| env := make([]string, 0, len(c.Env)) | ||
| for _, e := range c.Env { | ||
| env = append(env, e.Name+"="+e.Value) | ||
| } | ||
| return env, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("container %q not found in deployment %s/%s", containerName, namespace, deploymentName) | ||
| } | ||
|
|
||
| // setDeploymentEnvVars replaces the environment of the named container with | ||
| // the provided "NAME=VALUE" pairs and waits for the rollout to complete. | ||
| // It locates the container by name (rather than assuming index 0) and uses | ||
| // the JSON Patch "add" operation, which creates the env field if absent. | ||
| func setDeploymentEnvVars(deploymentName, namespace, containerName string, env []string) error { | ||
| // Fetch the deployment to find the container index. | ||
| raw, err := k8sClient("get", "deployment", deploymentName, "-n", namespace, "-o", "json") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get deployment %s/%s: %w", namespace, deploymentName, err) | ||
| } | ||
| var dep appsv1.Deployment | ||
| if err := json.Unmarshal([]byte(raw), &dep); err != nil { | ||
| return fmt.Errorf("failed to unmarshal deployment: %w", err) | ||
| } |
| type envVar struct { | ||
| Name string `json:"name"` | ||
| Value string `json:"value"` | ||
| } | ||
| envVars := make([]envVar, 0, len(env)) | ||
| for _, kv := range env { | ||
| parts := strings.SplitN(kv, "=", 2) | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid env var %q: must be NAME=VALUE", kv) | ||
| } | ||
| envVars = append(envVars, envVar{Name: parts[0], Value: parts[1]}) | ||
| } | ||
|
|
| // Exclude the Kubernetes API server from proxying so the controller can | ||
| // still reconcile resources. client-go connects to KUBERNETES_SERVICE_HOST | ||
| // which is the cluster IP of the "kubernetes" service — a plain IP, not a | ||
| // DNS name, so DNS-wildcard NO_PROXY entries won't match it. | ||
| k8sIP, err := kubernetesClusterIP() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| newEnv := addOrReplaceEnvVar(origEnv, "HTTPS_PROXY", proxyURL) | ||
| newEnv = addOrReplaceEnvVar(newEnv, "NO_PROXY", k8sIP) | ||
| return setDeploymentEnvVars(deployName, olmNamespace, "manager", newEnv) |
| dst, err := net.Dial("tcp", r.Host) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadGateway) | ||
| return | ||
| } |
| p.hosts = append(p.hosts, target) | ||
| p.mu.Unlock() | ||
|
|
||
| dst, err := (&net.Dialer{Timeout: 30 * time.Second}).Dial("tcp", target) |
| done := make(chan struct{}, 2) | ||
| tunnel := func(dst io.Writer, src io.Reader) { | ||
| defer func() { done <- struct{}{} }() | ||
| _, _ = io.Copy(dst, src) | ||
| // Half-close the write side so the other direction sees EOF and | ||
| // its io.Copy returns, preventing the goroutine from hanging. | ||
| if cw, ok := dst.(interface{ CloseWrite() error }); ok { | ||
| _ = cw.CloseWrite() | ||
| } | ||
| } |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, rashmigottipati The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 967be14
into
operator-framework:main
Unit tests verify that BuildHTTPClient correctly tunnels HTTPS connections through a proxy and fails when the proxy rejects the CONNECT request.
E2e tests cover two scenarios: a dead proxy that blocks catalog fetches (asserted via "proxyconnect" in the Retrying condition), and a live recording proxy that asserts catalog traffic actually routes through it via CONNECT.
Description
Reviewer Checklist