Fix problems introduced by #58 #61

PaulSD · 2019-04-22T15:29:12Z

So apparently I missed an import line in #58 (See #60 🤦‍♂️). I've also since discovered problems with current versions of Go, which dropped support for golang.org/x/net/proxy. This PR fixes both of those issues.

sockets/proxy.go

PaulSD · 2019-04-24T01:21:59Z

I put GetProxyEnv() back (there is no harm in continuing to use it).

I replaced DialerFromEnvironment() with a no-op with a deprecation notice. Note that it is no longer necessary to set http.Transport.Dial from DialerFromEnvironment(), so code such as https://github.com/moby/moby/blob/b0e6eedcf4252d56e62fc3de6de5d01e75a3dc44/distribution/registry.go#L82 can simply be deleted.

wtibbitts · 2019-04-24T15:04:15Z

Is there an ETA on this fix?

PaulSD · 2019-04-29T21:00:51Z

Ping @thaJeztah

thomasmodeneis · 2019-05-01T13:35:36Z

up!

thomasmodeneis · 2019-05-02T14:16:27Z

Sorry, but this PR wont compile:
go-connections/sockets/proxy.go:5:2: imported and not used: "net/url"

disafronov · 2019-05-13T16:05:44Z

up!

olljanat · 2019-05-13T20:36:49Z

@PaulSD what @thomasmodeneis says is valid. Can you plz remove that not used import so we can get this one merged?

I find there has a problem in docker#61(/sockets/sockets_windows) /sockets/sockets_windows no longer need to declare `dialer` (sorry my English is poor, Translate to google translate)

olljanat · 2019-05-22T16:29:22Z

@thaJeztah @AkihiroSuda can we get this one merged with #62 ?
It is bit boring that code on master is broken...

AkihiroSuda

please sign the commit with git commit -s and fix the compilation failure reported above

AkihiroSuda · 2019-05-22T20:32:15Z

#62 seems to lack the DCO as well

PaulSD · 2019-05-22T20:32:26Z

One moment, working on it...

PaulSD · 2019-05-22T20:55:49Z

Should be good now. Anything else I missed?

AkihiroSuda

thanks

wanyvic · 2019-05-27T03:46:11Z

work fine

Signed-off-by: Paul Donohue <[email protected]>

avii-wix · 2019-05-28T12:47:46Z

Any ETA on merging this PR?

AkihiroSuda · 2019-05-28T13:23:05Z

ping @thaJeztah

olljanat · 2019-05-28T16:22:57Z

Should be good now. Anything else I missed?

@PaulSD One minor thing. Can you modify comment line back on way that it starts with text DialerFromEnvironment other why it does not pass golint (noticed when I was creating #64)

You can test it locally with command:

golint -min_confidence 0.81 ./...

* Add missing import of "context" (Fixes docker#60) * Drop configuration of the http.Transport.DialContext based on proxy settings (This was originally needed for SOCKS support, but has not been required since https://go-review.googlesource.com/c/go/+/35488/ in Go 1.9, and is no longer supported as of https://go-review.googlesource.com/c/go/+/41031/ in Go 1.11) Signed-off-by: Paul Donohue <[email protected]>

PaulSD · 2019-05-31T22:22:50Z

@olljanat Fixed, thanks!

Innectic · 2019-06-08T16:46:43Z

What is currently preventing this from being merged?

AkihiroSuda · 2019-06-08T17:05:04Z

merging. I assume single LGTM is enough in this case

AkihiroSuda · 2019-06-08T17:06:11Z

cc @dnephin @thaJeztah @vdemeester

thaJeztah · 2019-06-12T10:58:28Z

argh, sorry; yes, SGTM

jamesmoessis · 2022-07-19T02:14:18Z

sockets/sockets.go

-		if err != nil {
-			return err
-		}
-		tr.DialContext = dialer.DialContext


This seems to have caused a problem for me because now the unix transport is used when I have specified an HTTP endpoint.

The dial context is used and for an http endpoint it tries to connect to a unix socket instead of a tcp socket.

I'm not following you ...

The code you highlighted here was only used to configure the proxy on Go <1.9, and was removed because it is redundant and no longer necessary in Go >=1.9.

Removal of this code should not impact transport selection. Transport selection occurs a few lines earlier, and was not modified by this change: https://github.com/docker/go-connections/blob/master/sockets/sockets.go#L17

Maybe you have a proxy configured using a UNIX socket?

Since docker#61, there's no more DialContext defined in the default switch case of ConfigureTransport. As a consequence, if ConfigureTransport was already called with a npipe or unix proto (ie. Docker client does that to initialize its HTTP client with the default DOCKER_HOST), the DialContext function defined by these protocols will be used. As the DialContext functions defined by unix and npipe protos totally ignore DialContext's 2nd and 3rd argument (ie. network and addr) and instead use the equivalent arguments passed to configureUnixTransport and configureNpipeTransport when they were called, this results in ConfigureTransport being totally ineffective. In addition, DisableCompression is also reset to false. Signed-off-by: Albin Kerouanton <[email protected]>

Since docker#61, there's no more DialContext defined in the default switch case of ConfigureTransport. As a consequence, if ConfigureTransport was already called with a npipe or unix proto (ie. Docker client does that to initialize its HTTP client with the default DOCKER_HOST), the DialContext function defined by these protocols will be used. As the DialContext functions defined by unix and npipe protos totally ignore DialContext's 2nd and 3rd argument (ie. network and addr) and instead use the equivalent arguments passed to configureUnixTransport and configureNpipeTransport when they were called, this results in ConfigureTransport being totally ineffective. Signed-off-by: Albin Kerouanton <[email protected]>

PaulSD force-pushed the master branch from 80ab42c to 29ef0ca Compare April 22, 2019 15:30

ssmuch approved these changes Apr 23, 2019

View reviewed changes

thaJeztah reviewed Apr 23, 2019

View reviewed changes

sockets/proxy.go Show resolved Hide resolved

thaJeztah reviewed Apr 23, 2019

View reviewed changes

sockets/proxy.go Show resolved Hide resolved

PaulSD force-pushed the master branch from 29ef0ca to c98bc50 Compare April 24, 2019 01:15

goddamit-io mentioned this pull request Apr 28, 2019

Error thrown at go build / make containers/docker-lvm-plugin#73

Closed

MashiroC mentioned this pull request May 16, 2019

Fix problems introduced by #58 & #61 #62

Closed

AkihiroSuda requested changes May 22, 2019

View reviewed changes

PaulSD force-pushed the master branch from c98bc50 to 4a3911e Compare May 22, 2019 20:45

AkihiroSuda approved these changes May 22, 2019

View reviewed changes

thaJeztah mentioned this pull request May 23, 2019

Replace deprecated Transport.Dial with Transport.DialContext #58

Merged

serhiiz mentioned this pull request May 23, 2019

Enabling plugin throws "no such file or directory" error on boot2docker vm trajano/docker-volume-plugins#11

Open

PaulSD referenced this pull request May 27, 2019

Replace deprecated Transport.Dial with Transport.DialContext

61039d0

Signed-off-by: Paul Donohue <[email protected]>

PaulSD force-pushed the master branch from 4a3911e to fb772cf Compare May 31, 2019 22:22

AkihiroSuda merged commit 0f3f48b into docker:master Jun 8, 2019

thaJeztah mentioned this pull request Jun 9, 2020

Bump go-connections for TLS 1.3 support moby/moby#41084

Merged

thaJeztah mentioned this pull request Mar 15, 2021

[do not merge] testing reverting some changes #80

Closed

thaJeztah mentioned this pull request Jul 22, 2021

socket: restore deprecated Transport.Dial (for now) #90

Draft

jamesmoessis reviewed Jul 19, 2022

View reviewed changes

akerouanton mentioned this pull request Oct 27, 2023

ConfigureTransport: make sure a clean DialContext is used for tcp #103

Merged

Fix problems introduced by #58 #61

Fix problems introduced by #58 #61

Uh oh!

Conversation

PaulSD commented Apr 22, 2019

Uh oh!

Uh oh!

Uh oh!

PaulSD commented Apr 24, 2019

Uh oh!

wtibbitts commented Apr 24, 2019

Uh oh!

PaulSD commented Apr 29, 2019

Uh oh!

thomasmodeneis commented May 1, 2019

Uh oh!

thomasmodeneis commented May 2, 2019

Uh oh!

disafronov commented May 13, 2019

Uh oh!

olljanat commented May 13, 2019

Uh oh!

olljanat commented May 22, 2019

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 22, 2019

Uh oh!

PaulSD commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulSD commented May 22, 2019

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

wanyvic commented May 27, 2019

Uh oh!

avii-wix commented May 28, 2019

Uh oh!

AkihiroSuda commented May 28, 2019

Uh oh!

olljanat commented May 28, 2019

Uh oh!

PaulSD commented May 31, 2019

Uh oh!

Innectic commented Jun 8, 2019

Uh oh!

AkihiroSuda commented Jun 8, 2019

Uh oh!

AkihiroSuda commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jun 12, 2019

Uh oh!

jamesmoessis Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulSD Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

PaulSD commented May 22, 2019 •

edited

Loading

AkihiroSuda commented Jun 8, 2019 •

edited

Loading

jamesmoessis Jul 19, 2022 •

edited

Loading