🌱 tests: add unit and e2e tests for HTTPS_PROXY support#2654
Open
tmshort wants to merge 1 commit intooperator-framework:mainfrom
Open
🌱 tests: add unit and e2e tests for HTTPS_PROXY support#2654tmshort wants to merge 1 commit intooperator-framework:mainfrom
tmshort wants to merge 1 commit intooperator-framework:mainfrom
Conversation
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. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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 |
Contributor
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.
Comment on lines
+91
to
+97
| // The test overrides the transport's Proxy function with http.ProxyURL to | ||
| // avoid the loopback-address exclusion in http.ProxyFromEnvironment (which | ||
| // would silently bypass any proxy for 127.0.0.1 targets) and the sync.Once | ||
| // caching that makes HTTPS_PROXY env-var changes within the same process | ||
| // unreliable. The structural test above confirms that the Proxy field is | ||
| // wired up; this test confirms the tunnelling itself works. | ||
| func TestBuildHTTPClientProxyTunnelsConnections(t *testing.T) { |
Comment on lines
+61
to
+64
| // Read the request line and headers. | ||
| buf := make([]byte, 4096) | ||
| n, err := conn.Read(buf) | ||
| if err != nil { |
Comment on lines
+121
to
+125
| out, err := exec.Command("docker", "network", "inspect", "kind", | ||
| "--format", "{{(index .IPAM.Config 0).Gateway}}").Output() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to inspect kind docker network: %w", err) | ||
| } |
Comment on lines
+43
to
+45
| func (p *recordingProxy) port() string { | ||
| _, port, _ := net.SplitHostPort(p.addr()) | ||
| return port |
Comment on lines
+187
to
+193
| patch := []map[string]interface{}{ | ||
| { | ||
| "op": "replace", | ||
| "path": "/spec/template/spec/containers/0/env", | ||
| "value": envVars, | ||
| }, | ||
| } |
Comment on lines
+30
to
+36
| // deploymentRestore records the original state of a deployment container so | ||
| // it can be rolled back after a test that modifies deployment configuration. | ||
| type deploymentRestore struct { | ||
| name string // deployment name | ||
| namespace string | ||
| containerName string | ||
| originalEnv []string // "NAME=VALUE" pairs |
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
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.
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