Add proxy support for model pulling behind firewall#263
Conversation
Reviewer's GuideAdds proxy support for model pulling by injecting proxy env vars into Docker containers and configuring HTTP transports to use ProxyFromEnvironment, with accompanying tests to verify both container setup and transport configuration. Sequence diagram for proxy environment variable injection during container creationsequenceDiagram
participant "CLI"
participant "OS Environment"
participant "Docker Container"
CLI->>"OS Environment": Read proxy env vars (HTTP_PROXY, HTTPS_PROXY, NO_PROXY, etc.)
alt Proxy env vars set
CLI->>"Docker Container": Pass proxy env vars in container config
end
Class diagram for updated model manager HTTP transport configurationclassDiagram
class ClientConfig {
+StoreRootPath string
+Logger logrus.FieldLogger
+Transport http.RoundTripper
}
class Manager {
+ClientConfig config
+memEstimator Estimator
}
class resumable {
+New(baseTransport http.RoundTripper)
}
ClientConfig <|-- Manager
Manager o-- ClientConfig
ClientConfig o-- resumable: uses Transport
resumable : New(baseTransport)
class http.Transport {
+Proxy func(*Request) (*url.URL, error)
}
http.Transport <|-- baseTransport
baseTransport : Proxy = http.ProxyFromEnvironment
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, 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 enhances the application's network capabilities by introducing robust proxy support. It ensures that all components involved in model pulling, including the Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the proxyEnvVars slice into a shared constant or helper to avoid duplicating the list across main.go, containers.go, and tests.
- In your tests, you can simplify environment setup and teardown by using t.Setenv (Go 1.17+) instead of manually saving and restoring os.Environ values.
- Since http.DefaultTransport already uses ProxyFromEnvironment by default, double-check whether cloning and re-setting the Proxy field is necessary or if you can just pass the default transport directly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the proxyEnvVars slice into a shared constant or helper to avoid duplicating the list across main.go, containers.go, and tests.
- In your tests, you can simplify environment setup and teardown by using t.Setenv (Go 1.17+) instead of manually saving and restoring os.Environ values.
- Since http.DefaultTransport already uses ProxyFromEnvironment by default, double-check whether cloning and re-setting the Proxy field is necessary or if you can just pass the default transport directly.
## Individual Comments
### Comment 1
<location> `main_test.go:113` </location>
<code_context>
+
+// TestProxyTransportConfiguration verifies that the HTTP transport
+// is configured to use proxy settings from environment variables.
+func TestProxyTransportConfiguration(t *testing.T) {
+ tests := []struct {
+ name string
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions that the Proxy function actually uses the environment variables to set up the proxy.
The test could be improved by verifying that the proxy URL is determined from the environment variables, either by making a dummy HTTP request or inspecting baseTransport.Proxy with a sample request.
Suggested implementation:
```golang
func TestProxyTransportConfiguration(t *testing.T) {
tests := []struct {
name string
envVars map[string]string
expected string
}{
{
name: "HTTP_PROXY set",
envVars: map[string]string{
"HTTP_PROXY": "http://proxy.example.com:8080",
},
expected: "http://proxy.example.com:8080",
},
{
name: "HTTPS_PROXY set",
```
```golang
// Add more test cases as needed
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Save old env vars and set new ones
oldEnv := make(map[string]string)
for k, v := range tt.envVars {
oldEnv[k] = os.Getenv(k)
os.Setenv(k, v)
}
// Create transport using ProxyFromEnvironment
transport := &http.Transport{
Proxy: http.ProxyFromEnvironment,
}
// Create dummy request
req, err := http.NewRequest("GET", "http://example.com", nil)
if err != nil {
t.Fatalf("failed to create request: %v", err)
}
// Get proxy URL
proxyURL, err := transport.Proxy(req)
if err != nil {
t.Fatalf("Proxy function returned error: %v", err)
}
if tt.expected == "" {
if proxyURL != nil {
t.Errorf("expected no proxy, got %v", proxyURL)
}
} else {
if proxyURL == nil {
t.Errorf("expected proxy %v, got nil", tt.expected)
} else if proxyURL.String() != tt.expected {
t.Errorf("expected proxy %v, got %v", tt.expected, proxyURL.String())
}
}
// Restore old env vars
for k, v := range oldEnv {
os.Setenv(k, v)
}
})
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
Adds proxy support for model pulling behind firewalls by wiring proxy environment variables into the container runtime and configuring HTTP transport to respect proxy settings.
- Configure HTTP transport to use ProxyFromEnvironment for model pulls
- Pass HTTP(S)_PROXY and NO_PROXY (both cases) into the docker-model-runner container
- Add tests for transport configuration and proxy env propagation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| main.go | Clones and configures an HTTP transport with ProxyFromEnvironment and injects it into the models manager. |
| main_test.go | Adds TestProxyTransportConfiguration to assert proxy function presence on the HTTP transport. |
| cmd/cli/pkg/standalone/containers.go | Appends proxy-related environment variables (upper/lower case) to the container env. |
| cmd/cli/pkg/standalone/containers_test.go | Adds tests that simulate collecting proxy env vars for container configuration. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@holger-stenzhorn, @xenoscopic should this close: ? |
![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 adds support for using an HTTP/S proxy when pulling models, which is a great feature for users behind corporate firewalls. The changes correctly pass proxy environment variables to the container and configure the HTTP transport. The accompanying tests are a good start, but I have some suggestions to make them more robust and meaningful. I've also pointed out a small redundancy in the transport configuration.
ericcurtin
force-pushed
the
fix-model-pulling-behind-firewall
branch
from
1589964 to
4f3e212
Compare
@ericcurtin: Yes, that should do the trick. I will test it as soon as I can today. Thanks a lot for your support. |
| // Pass proxy environment variables to the container if they are set | ||
| proxyEnvVars := []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "http_proxy", "https_proxy", "no_proxy"} | ||
| for _, proxyVar := range proxyEnvVars { | ||
| if value := os.Getenv(proxyVar); value != "" { |
There was a problem hiding this comment.
nit: LookupEnv may be a bit more accurate. Though I assume none of those variables make sense with an empty value.
There was a problem hiding this comment.
@p1-0tr could you further explain? It's unclear to be what advantage this would bring, or maybe something else is being suggested?
- if value := os.Getenv(proxyVar); value != "" {
+ if value, ok := os.LookupEnv(proxyVar); ok && value != "" {
There was a problem hiding this comment.
I meant that:
- if value := os.Getenv(proxyVar); value != "" {
+ if value, ok := os.LookupEnv(proxyVar); ok {
would be "more correct" in terms of how env variables work in general. It probably does not change much in the context of the variables we are forwarding (they likely are never used with an empty value), but I can't say for sure OTOH (so I would lean toward the more generic logic by default).
There was a problem hiding this comment.
The way we did it is the curl_getenv way.
We need this PR unblocked, lets change
ericcurtin
force-pushed
the
fix-model-pulling-behind-firewall
branch
from
4f3e212 to
cc10cf7
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@doringeman test removed, please re-review |
ericcurtin
force-pushed
the
fix-model-pulling-behind-firewall
branch
from
cc10cf7 to
d799fbb
Compare
ericcurtin
force-pushed
the
fix-model-pulling-behind-firewall
branch
2 times, most recently
from
87b0fdb to
40e1935
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Pass proxy environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) to docker-model-runner container - Configure HTTP transport to use ProxyFromEnvironment for model pulling - Add tests for proxy configuration in both container creation and transport Signed-off-by: Eric Curtin <eric.curtin@docker.com>
ericcurtin
force-pushed
the
fix-model-pulling-behind-firewall
branch
from
8f6c082 to
4fa1954
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary by Sourcery
Enable HTTP(S) proxy support for model pulling behind a firewall by configuring the HTTP transport with ProxyFromEnvironment and passing proxy environment variables into spawned containers, with accompanying unit tests.
New Features:
Enhancements:
Tests: