Fix outbound HTTP proxy fallback for executor clients#2357
Fix outbound HTTP proxy fallback for executor clients#2357s5gan wants to merge 4 commits intorouter-for-me:devfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the outbound HTTP proxy selection logic for executor clients, making it more explicit and consistent, especially when no CPA proxy is configured. The changes formalize the proxy resolution order, ensuring that environment variables are properly considered as a fallback, thereby improving the robustness of network requests. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b04ae3e18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if transport, ok := http.DefaultTransport.(*http.Transport); ok && transport != nil { | ||
| return transport.Clone() |
There was a problem hiding this comment.
When env proxy variables are set, newEnvironmentProxyTransport() clones http.DefaultTransport for each new client, which creates a fresh connection pool every time. In this codebase, executors call newProxyAwareHTTPClient(...) per request, so proxied traffic loses shared keep-alive reuse and can accumulate extra sockets/goroutines compared with the previous Transport == nil behavior (which reused http.DefaultTransport). Use a shared transport (or leave Transport nil) for the env-fallback path instead of cloning on each call.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the newProxyAwareHTTPClient function to explicitly support environment proxy settings (HTTP_PROXY, HTTPS_PROXY, ALL_PROXY) as a fallback mechanism before using a context-provided RoundTripper. This involves adding new helper functions environmentProxyConfigured and newEnvironmentProxyTransport, and updating the proxy priority documentation. Corresponding test cases have been added to verify this new behavior, ensuring that environment proxies are used when no explicit proxy is set, and that explicit proxies still take precedence. Feedback suggests improving the robustness of newEnvironmentProxyTransport by explicitly setting Proxy = http.ProxyFromEnvironment on the cloned transport, optimizing environmentProxyConfigured by making the environment variable key slice a package-level constant, and refactoring duplicated environment variable setup/teardown logic in tests into a helper function for better maintainability.
| if transport, ok := http.DefaultTransport.(*http.Transport); ok && transport != nil { | ||
| return transport.Clone() | ||
| } |
There was a problem hiding this comment.
The comment for priority 3 states that the goal is to 'Use environment proxy settings explicitly... instead of relying on http.DefaultTransport'. However, this implementation clones http.DefaultTransport without explicitly setting the Proxy function. If http.DefaultTransport were to be modified elsewhere to not use the environment proxy, this function would return a transport that doesn't use the environment proxy, which would defeat the purpose of this change.
To make this more robust and explicit, you should set Proxy to http.ProxyFromEnvironment on the cloned transport. This pattern is used in sdk/proxyutil/proxy.go in the NewDirectTransport function, which explicitly sets Proxy = nil.
if transport, ok := http.DefaultTransport.(*http.Transport); ok && transport != nil {
clone := transport.Clone()
clone.Proxy = http.ProxyFromEnvironment
return clone
}| } | ||
|
|
||
| func environmentProxyConfigured() bool { | ||
| for _, key := range []string{"HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "all_proxy"} { |
There was a problem hiding this comment.
To improve efficiency and reduce memory allocations, this slice of environment variable keys should be defined once as a package-level variable instead of being re-created on every function call.
| oldHTTPProxy, hadHTTPProxy := os.LookupEnv("HTTP_PROXY") | ||
| if err := os.Setenv("HTTP_PROXY", "http://env-proxy.example.com:8080"); err != nil { | ||
| t.Fatalf("Setenv(HTTP_PROXY): %v", err) | ||
| } | ||
| defer func() { | ||
| if hadHTTPProxy { | ||
| _ = os.Setenv("HTTP_PROXY", oldHTTPProxy) | ||
| return | ||
| } | ||
| _ = os.Unsetenv("HTTP_PROXY") | ||
| }() | ||
|
|
||
| oldHTTPSProxy, hadHTTPSProxy := os.LookupEnv("HTTPS_PROXY") | ||
| if err := os.Setenv("HTTPS_PROXY", "http://env-proxy.example.com:8080"); err != nil { | ||
| t.Fatalf("Setenv(HTTPS_PROXY): %v", err) | ||
| } | ||
| defer func() { | ||
| if hadHTTPSProxy { | ||
| _ = os.Setenv("HTTPS_PROXY", oldHTTPSProxy) | ||
| return | ||
| } | ||
| _ = os.Unsetenv("HTTPS_PROXY") | ||
| }() |
There was a problem hiding this comment.
This block of code for setting and cleaning up environment variables is duplicated in TestNewProxyAwareHTTPClientExplicitProxyWinsOverEnvironmentProxy. To improve maintainability and reduce duplication, consider extracting this logic into a test helper function. You can use t.Cleanup for a more idiomatic way to handle test teardown.
For example:
func setProxyEnv(t *testing.T, proxyURL string) {
t.Helper()
for _, key := range []string{"HTTP_PROXY", "HTTPS_PROXY"} {
old, had := os.LookupEnv(key)
if err := os.Setenv(key, proxyURL); err != nil {
t.Fatalf("Setenv(%s): %v", key, err)
}
t.Cleanup(func() {
if had {
_ = os.Setenv(key, old)
} else {
_ = os.Unsetenv(key)
}
})
}
}Then you can call setProxyEnv(t, "http://env-proxy.example.com:8080") in your tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3f49a963d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if environmentProxyConfigured() { | ||
| httpClient.Transport = newEnvironmentProxyTransport() | ||
| return httpClient |
There was a problem hiding this comment.
Setting httpClient.Transport in the new environment-proxy branch changes downstream behavior for Antigravity: newAntigravityHTTPClient now sees a non-nil *http.Transport and clones it on every call (internal/runtime/executor/antigravity_executor.go:123-126) instead of using its shared singleton transport path (...:117-120). In env-proxy deployments (no explicit CPA proxy), this reintroduces one connection pool per request and the associated socket/goroutine growth that did not happen before this commit when Transport was left nil.
Useful? React with 👍 / 👎.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
This PR makes outbound proxy selection explicit and consistent by introducing an environment-proxy fallback before context RoundTripper, and it also avoids per-request transport cloning for Antigravity environment fallback.
Key findings:
- No blocking issues found.
- Non-blocking: one debug log message in
newProxyAwareHTTPClientstill mentions fallback to context transport, while code now prefers environment proxy first.
Validation:
go test ./internal/runtime/executor -run 'TestNewProxyAwareHTTPClient(DirectBypassesGlobalProxy|FallsBackToEnvironmentProxy|ExplicitProxyWinsOverEnvironmentProxy|ReusesEnvironmentProxyTransport)$|TestNewAntigravityHTTPClientReusesSharedEnvironmentProxyTransport'go test ./cmd/server ./sdk/cliproxy ./internal/api/...- CI status checked;
translator-path-guardintentionally ignored per review instruction.
Recommendation:
Approve. The behavior change matches the PR intent, test coverage is appropriate for the new precedence rules, and the transport reuse improvement for Antigravity reduces connection-pool churn risk.
This is an automated Codex review result and still requires manual verification by a human reviewer.
|
@luispater Hi! Thanks for the review. 🙏Just wondering if there's anything else needed from my side before merging this PR? |
|
This PR is approved and already targets |
2 participants
Summary
Make executor outbound HTTP proxy selection explicit and consistent when no CPA proxy is configured.
Proxy priority is now:
auth.ProxyURLcfg.ProxyURLHTTP_PROXY,HTTPS_PROXY,ALL_PROXY)RoundTripperChanges
newProxyAwareHTTPClient(...)to explicitly fall back to environment proxy settingsdirectbypassing proxyValidation
go test ./internal/runtime/executor -run 'TestNewProxyAwareHTTPClient(DirectBypassesGlobalProxy|FallsBackToEnvironmentProxy|ExplicitProxyWinsOverEnvironmentProxy)$'go test ./cmd/server ./sdk/cliproxy ./internal/api/...