Bump go 1.16.6 #3197

thaJeztah · 2021-07-19T12:46:53Z

Keeping the dockerfiles/Dockerfile.cross image at 1.13, as we don't
have more current versions of that image. However, I don't think it's
still used, so we should remove it.

thaJeztah · 2021-07-19T13:33:40Z

Interesting; looks like for some reason, the test doesn't pick up the HTTP_PROXY env var. In that case transport.Proxy(request) returns nil;

=== Failed
=== FAIL: cli/command TestNewAPIClientFromFlagsWithHttpProxyEnv (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x67a14a]

goroutine 24 [running]:
testing.tRunner.func1.2(0xaa9ca0, 0xfe73a0)
	/usr/local/go/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc000083380)
	/usr/local/go/src/testing/testing.go:1146 +0x4b6
panic(0xaa9ca0, 0xfe73a0)
	/usr/local/go/src/runtime/panic.go:965 +0x1b9
net/url.(*URL).String(0x0, 0xc000083380, 0x0)
	/usr/local/go/src/net/url/url.go:813 +0x4a
github.com/docker/cli/cli/command.TestNewAPIClientFromFlagsWithHttpProxyEnv(0xc000083380)
	/go/src/github.com/docker/cli/cli/command/cli_test.go:98 +0x377
testing.tRunner(0xc000083380, 0xb98b50)
	/usr/local/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1238 +0x2b3


DONE 1568 tests, 3 skipped, 1 failure in 216.761s
make: *** [Makefile:22: test-coverage] Error 1

Exited with code exit status 2
CircleCI received exit code 2

thaJeztah · 2021-07-19T15:09:15Z

I suspect the problem is that stdlib uses a sync.Once to get the proxy; https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/net/http/transport.go#L815-L826

So if some codepath has checked if a proxy must be used, and at that moment no proxy env-vars were set, then it will no longer be called after that.

func envProxyFunc() func(*url.URL) (*url.URL, error) {
	envProxyOnce.Do(func() {
		envProxyFuncValue = httpproxy.FromEnvironment().ProxyFunc()
	})
	return envProxyFuncValue
}

// resetProxyConfig is used by tests.
func resetProxyConfig() {
	envProxyOnce = sync.Once{}
	envProxyFuncValue = nil
}

There is a resetProxyConfig() function that they used in testing, but that can't be used externally.

thaJeztah · 2021-07-19T15:23:45Z

Hmm.. actually, this commit may be the cause; golang/net@7b1cca2

We set up a HTTP(S)_PROXY, but the scheme we use to connect is tcp://. The commit above adds an extra check and if the scheme is not http:// it will return "no proxy" (nil)

thaJeztah · 2021-07-19T15:33:39Z

This is gonna be fun, because we use tcp:// by default. I'll be honest; I don't know why we used that in the first place (and not http:// and https://, but it's there, and (IIUC) it means that with go 1.16, connections specifying one of those will ignore proxy configuration;

cli/opts/hosts.go

Lines 21 to 24 in a32cd16

    
           // defaultTCPHost constant defines the default host string used by docker on Windows 
        
           defaultTCPHost = "tcp://" + defaultHTTPHost + ":" + defaultHTTPPort 
        
           // DefaultTLSHost constant defines the default host string used by docker for TLS sockets 
        
           defaultTLSHost = "tcp://" + defaultHTTPHost + ":" + defaultTLSHTTPPort

codecov-commenter · 2021-07-28T17:26:16Z

Codecov Report

Merging #3197 (ecd9beb) into master (c9f8473) will not change coverage.
The diff coverage is n/a.

❗ Current head ecd9beb differs from pull request most recent head a866978. Consider uploading reports for the commit a866978 to get more accurate results

@@           Coverage Diff           @@
##           master    #3197   +/-   ##
=======================================
  Coverage   58.58%   58.58%           
=======================================
  Files         299      299           
  Lines       21502    21502           
=======================================
  Hits        12597    12597           
  Misses       7983     7983           
  Partials      922      922

thaJeztah · 2021-07-29T08:21:34Z

This is gonna be fun, because we use tcp:// by default. I'll be honest; I don't know why we used that in the first place (and not http:// and https://, but it's there, and (IIUC) it means that with go 1.16, connections specifying one of those will ignore proxy configuration;

Update to the above:

So, it turned out that the actual code-path that the CLI uses does not (directly)
hit this problem. The issue in TestNewAPIClientFromFlagsWithHttpProxyEnv was
that;

It was a unit-test, and Golang uses a sync.Once to configure the proxy to
be used. Depending on what code was executed before that test, the proxy would
already be set (based on the first request made), and therefore potentially hit
the wrong value (see cli/command: don't use client.CustomHTTPHeaders() #3203 (comment)).
Changing the test to be an integration/e2e test fixes that (change TestNewAPIClientFromFlagsWithHttpProxyEnv to an e2e test #3221)
The test manually created a request using tcp:// as scheme.
while that revealed that the transport itself would get the incorrect proxy
(no proxy) when used with a non-http(s):// scheme, our standard code does
not use

During initialisation;

client.NewClientWithOpts() (which is used to initialize the client sets up a default HTTP client, based on the default host (unix:///var/run/docker.sock on Linux, npipe:////./pipe/docker_engine on Windows). Users can override this with a custom option, but (see 2.), this should not matter.
https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/client/client.go#L120
```
func NewClientWithOpts(ops ...Opt) (*Client, error) {
    client, err := defaultHTTPClient(DefaultDockerHost)
```

client.defaultClient() parses the host, and gets the scheme from it. It passes that scheme to sockets.ConfigureTransport https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/client/client.go#L158-L169

func defaultHTTPClient(host string) (*http.Client, error) {
    url, err := ParseHostURL(host)
    if err != nil {
        return nil, err
    }
    transport := new(http.Transport)
    sockets.ConfigureTransport(transport, url.Scheme, url.Host)
    return &http.Client{
        Transport:     transport,
        CheckRedirect: CheckRedirect,
    }, nil
}

sockets.ConfigureTransport only uses the scheme to switch between setting up unix, npipe, or "other" transports https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/vendor/github.com/docker/go-connections/sockets/sockets.go#L21-L37

func ConfigureTransport(tr *http.Transport, proto, addr string) error {
    switch proto {
    case "unix":
        return configureUnixTransport(tr, proto, addr)
    case "npipe":
        return configureNpipeTransport(tr, proto, addr)
    default:
        tr.Proxy = http.ProxyFromEnvironment
        dialer, err := DialerFromEnvironment(&net.Dialer{
            Timeout: defaultTimeout,
        })
        if err != nil {
            return err
        }
        tr.Dial = dialer.Dial
    }
    return nil
}

while it sets transport.Proxy to http.ProxyFromEnvironment, that function is not called until a request is made https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/vendor/github.com/docker/go-connections/sockets/sockets.go#L28
```
default:
    tr.Proxy = http.ProxyFromEnvironment
```

the client's scheme is not set based on DOCKER_HOST. The tcp:// scheme is simply ignored, but depending on whether or not TLS is configured, is overwritten with http:// or https:// https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/client/client.go#L141-L153

if c.scheme == "" {
    c.scheme = "http"

    tlsConfig := resolveTLSConfig(c.client.Transport)
    if tlsConfig != nil {
        // TODO(stevvooe): This isn't really the right way to write clients in Go.
        // `NewClient` should probably only take an `*http.Client` and work from there.
        // Unfortunately, the model of having a host-ish/url-thingy as the connection
        // string has us confusing protocol and transport layers. We continue doing
        // this to avoid breaking existing clients but this should be addressed.
        c.scheme = "https"
    }
}

Whenever the client makes a request, it does not use the Transport's scheme, but sets the scheme for the request to what's configured on the client, which (see above) would be unix://, npipe://, or http(s):// (not sure what happens with ssh://) https://github.com/moby/moby/blob/b9ad7b96bd86e5f632e5fb4f36f98dcc145014fc/client/request.go#L99-L100
```
req.URL.Host = cli.addr
req.URL.Scheme = cli.scheme
```

So, although there still are some bits to look into, for example, if the client is initialized with a custom scheme, using the WithScheme() option, the regular code-path won't be affected by the Golang change.

thaJeztah · 2021-07-29T09:34:03Z

@silvin-lubecki @StefanScherer PTAL - this is ready for review now 🤗

silvin-lubecki · 2021-07-29T09:39:48Z

dockerfiles/Dockerfile.e2e

@@ -39,6 +39,7 @@ ARG GITCOMMIT
 ENV VERSION=${VERSION}
 ENV GITCOMMIT=${GITCOMMIT}
 ENV DOCKER_BUILDKIT=1
+#ENV GO111MODULE=off


question: Should we keep this commented line?

Arf, nope, my mistake. Let me fix

Keeping the dockerfiles/Dockerfile.cross image at 1.13, as we don't have more current versions of that image. However, I don't think it's still used, so we should remove it. Signed-off-by: Sebastiaan van Stijn <[email protected]>

silvin-lubecki

LGTM

thaJeztah added status/2-code-review process/cherry-pick labels Jul 19, 2021

thaJeztah added this to the 21.xx milestone Jul 19, 2021

thaJeztah force-pushed the bump_go_1.16.6 branch 2 times, most recently from 6e06ceb to 59d2077 Compare July 19, 2021 12:51

thaJeztah mentioned this pull request Jul 21, 2021

sockets: add TCPProxyFromEnvironment to keep pre-go1.16 behavior docker/go-connections#85

Draft

thaJeztah force-pushed the bump_go_1.16.6 branch 12 times, most recently from 59296b8 to 6f75653 Compare July 28, 2021 15:39

thaJeztah mentioned this pull request Jul 28, 2021

change TestNewAPIClientFromFlagsWithHttpProxyEnv to an e2e test #3221

Merged

thaJeztah force-pushed the bump_go_1.16.6 branch 4 times, most recently from 02cb88a to 94d1e60 Compare July 28, 2021 17:01

thaJeztah force-pushed the bump_go_1.16.6 branch 2 times, most recently from f9067f9 to ecd9beb Compare July 28, 2021 18:00

thaJeztah force-pushed the bump_go_1.16.6 branch 2 times, most recently from 5a5e4de to a866978 Compare July 29, 2021 07:38

silvin-lubecki reviewed Jul 29, 2021

View reviewed changes

thaJeztah force-pushed the bump_go_1.16.6 branch from a866978 to ab12131 Compare July 29, 2021 09:42

Update to go1.16.6

a477a72

Keeping the dockerfiles/Dockerfile.cross image at 1.13, as we don't have more current versions of that image. However, I don't think it's still used, so we should remove it. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the bump_go_1.16.6 branch from ab12131 to a477a72 Compare July 29, 2021 09:52

silvin-lubecki approved these changes Jul 29, 2021

View reviewed changes

thaJeztah merged commit 1ba95f2 into docker:master Jul 29, 2021

thaJeztah deleted the bump_go_1.16.6 branch July 29, 2021 10:06

thaJeztah mentioned this pull request Jul 29, 2021

[20.10 backport] Bump go 1.16.6 #3224

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump go 1.16.6 #3197

Bump go 1.16.6 #3197

thaJeztah commented Jul 19, 2021 •

edited

Loading

thaJeztah commented Jul 19, 2021

thaJeztah commented Jul 19, 2021

thaJeztah commented Jul 19, 2021

thaJeztah commented Jul 19, 2021

codecov-commenter commented Jul 28, 2021 •

edited

Loading

thaJeztah commented Jul 29, 2021 •

edited

Loading

thaJeztah commented Jul 29, 2021

silvin-lubecki Jul 29, 2021

thaJeztah Jul 29, 2021

silvin-lubecki left a comment

Bump go 1.16.6 #3197

Bump go 1.16.6 #3197

Conversation

thaJeztah commented Jul 19, 2021 • edited Loading

thaJeztah commented Jul 19, 2021

thaJeztah commented Jul 19, 2021

thaJeztah commented Jul 19, 2021

thaJeztah commented Jul 19, 2021

codecov-commenter commented Jul 28, 2021 • edited Loading

Codecov Report

thaJeztah commented Jul 29, 2021 • edited Loading

thaJeztah commented Jul 29, 2021

silvin-lubecki Jul 29, 2021

Choose a reason for hiding this comment

thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

thaJeztah commented Jul 19, 2021 •

edited

Loading

codecov-commenter commented Jul 28, 2021 •

edited

Loading

thaJeztah commented Jul 29, 2021 •

edited

Loading