Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker: set proxy from env for custom transport #160

Merged
merged 1 commit into from
Nov 10, 2016

Conversation

runcom
Copy link
Member

@runcom runcom commented Nov 10, 2016

@mtrmac PTAL asap for BZ https://bugzilla.redhat.com/show_bug.cgi?id=1393816

Signed-off-by: Antonio Murdaca runcom@redhat.com

@rhatdan
Copy link
Member

rhatdan commented Nov 10, 2016

LGTM

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the http.Transport in newDockerClient?

Aside from the above, this seems reasonable; there is the question whether proxies are prevalent enough not to go through SystemContext instead (see #149), but I would lean towards saying yes, they are prevalent enough that we should just use http.ProxyFromEnvironment.

@runcom
Copy link
Member Author

runcom commented Nov 10, 2016

@mtrmac fixed, we're now following what upstream docker is doing, just to be sure we do not regress on anything.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ServerDefault is the only substantive comment (will check Docker code in parallel with you); the rest is just aesthetics which we can do as a separate PR if we are under time pressure.

proxyDialer, err := sockets.DialerFromEnvironment(direct)
if err == nil {
tr.Dial = proxyDialer.Dial
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be cleaner with a separate function to return a tr only without polluting the rest… this works though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
proxyDialer, err := sockets.DialerFromEnvironment(direct)
if err == nil {
tr.Dial = proxyDialer.Dial
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t it be far cleaner to set a dialer := &net.Dialer…; if … { dialer = proxyDialer } than to override tr.Dial?

client.Transport = tr
if tr.TLSClientConfig == nil {
var cfg = tlsconfig.ServerDefault
tr.TLSClientConfig = &cfg
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? In the other path we are happy enough to keep the empty default. Also using ServerDefault for a client seems strange (though I realize the client options may be far too restrictive for interoperability).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just following upstream here

}
client.Transport = tr
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be simpler as client := &http.Client{Transport: tr}.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Member Author

runcom commented Nov 10, 2016

lgtm

@mtrmac is this good for you to merge?

Approved with PullApprove

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 10, 2016

👍 let’s merge this, and the rest will be tracked in #161.

Approved with PullApprove

@mtrmac mtrmac merged commit e9fed84 into containers:master Nov 10, 2016
@runcom runcom deleted the fix-proxies branch November 10, 2016 17:13
giuseppe pushed a commit to giuseppe/image that referenced this pull request Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants