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

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.

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)

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;

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           

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;

1. 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)

2. 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
   }

3. 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
   }

4. 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

5. 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"
       }
   }

6. 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.

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