Skip to content

Conversation

@PaulSD
Copy link
Contributor

@PaulSD PaulSD commented Apr 16, 2019

http.Transport.Dial is deprecated. Other parts of Docker have been updated to use DialContext instead (for example: https://github.com/moby/moby/blob/master/client/options.go#L66), but go-connections is currently still using Dial.

This commit updates go-connections to use DialContext.

Copy link
Collaborator

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Timeout: defaultTimeout,
}
tr.DialContext = func(ctx context.Context, _, _ string) (net.Conn, error) {
// DialPipeContext() has been added to winio:
Copy link
Member

Choose a reason for hiding this comment

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

@jhowardmsft do you know if a new release is planned?

@thaJeztah thaJeztah merged commit ccfcbc5 into docker:master Apr 21, 2019
MashiroC added a commit to MashiroC/go-connections that referenced this pull request May 16, 2019
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)
@ijc
Copy link

ijc commented May 23, 2019

How was this change tested? For me it doesn't build:

$ go get golang.org/x/net/proxy
$ go get github.com/pkg/errors
$ go get github.com/stretchr/testify/assert
$ go test ./...
?   	github.com/docker/go-connections	[no test files]
# github.com/docker/go-connections/sockets [github.com/docker/go-connections/sockets.test]
sockets/sockets.go:34:26: dialer.DialContext undefined (type proxy.Dialer has no field or method DialContext)
sockets/sockets_unix.go:24:28: undefined: context
ok  	github.com/docker/go-connections/nat	(cached)
ok  	github.com/docker/go-connections/proxy	(cached)
FAIL	github.com/docker/go-connections/sockets [build failed]
ok  	github.com/docker/go-connections/tlsconfig	(cached)

The naive change just kicks the can a bit further down the road:

diff --git a/sockets/proxy.go b/sockets/proxy.go
index 98e9a1d..36978c8 100644
--- a/sockets/proxy.go
+++ b/sockets/proxy.go
@@ -23,7 +23,7 @@ func GetProxyEnv(key string) string {
 // DialerFromEnvironment takes in a "direct" *net.Dialer and returns a
 // proxy.Dialer which will route the connections through the proxy using the
 // given dialer.
-func DialerFromEnvironment(direct *net.Dialer) (proxy.Dialer, error) {
+func DialerFromEnvironment(direct *net.Dialer) (proxy.ContextDialer, error) {
        allProxy := GetProxyEnv("all_proxy")
        if len(allProxy) == 0 {
                return direct, nil

produces:

$ go test ./...
?   	github.com/docker/go-connections	[no test files]
ok  	github.com/docker/go-connections/nat	(cached)
# github.com/docker/go-connections/sockets [github.com/docker/go-connections/sockets.test]
sockets/proxy.go:44:3: cannot use proxyFromURL (type proxy.Dialer) as type proxy.ContextDialer in return argument:
	proxy.Dialer does not implement proxy.ContextDialer (missing DialContext method)
sockets/sockets_unix.go:24:28: undefined: context
ok  	github.com/docker/go-connections/proxy	(cached)
FAIL	github.com/docker/go-connections/sockets [build failed]
ok  	github.com/docker/go-connections/tlsconfig	(cached)

I can't see a variant of golang.org/x/net/proxy.FromURL which returns something satisfying ContextDialer directly, meaning we would have to a type cast I think and deal with possible errors from that. It also brings a dependency on the very latest x/net since golang/net@7f726ca is only from a couple of weeks ago.

@thaJeztah
Copy link
Member

How was this change tested? For me it doesn't build

That's weird, I think CI was green 🤔

@thaJeztah
Copy link
Member

Oh, wait; there's #61

Looking at that PR, it looks like CI isn't running 😞

@ijc
Copy link

ijc commented May 23, 2019

CI doesn't appear to build, just lint and fmt...

I just spotted #61 which I suspect might be the fix.

I also missed #60 reporting this too

@ijc
Copy link

ijc commented May 23, 2019

CI doesn't appear to build, just lint and fmt...

Correction: + vet which does catch the build errors (iff the CI runs)

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.

5 participants