sockets: add TCPProxyFromEnvironment to keep pre-go1.16 behavior

thaJeztah commented 3 years ago

TCPProxyFromEnvironment wraps http.ProxyFromEnvironment, to preserve the pre-go1.16 behavior for URLs using the 'tcp://' scheme.

Prior to go1.16, https:// schemes would use HTTPS_PROXY, and any other scheme would use HTTP_PROXY. However, https://github.com/golang/net/commit/7b1cca2348c07eb09fef635269c8e01611260f9f (per a request in golang/go#40909) changed this behavior to only use HTTP_PROXY for http:// schemes, no longer using a proxy for any other scheme.

Docker uses the tcp:// scheme as a default for API connections, to indicate that the API is not "purely" HTTP. Various parts in the code also require this scheme to be used. While we could change the default and allow http(s) schemes to be used, doing so will take time, taking into account that there are many installs in existence that have tcp:// configured as DOCKER_HOST.

This function detects if the tcp:// scheme is used; if it is, it creates a shallow copy of req, containing just the URL, and overrides the scheme with 'http', which should be sufficient to perform proxy detection. For other (non-'tcp://') schemes, http.ProxyFromEnvironment is called without altering the request.

Signed-off-by: Sebastiaan van Stijn github@gone.nl

thaJeztah commented 3 years ago

relates to https://github.com/docker/cli/pull/3197

thaJeztah commented 3 years ago

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 (https://github.com/docker/cli/pull/3197) 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 https://github.com/docker/cli/pull/3203#issuecomment-884875230). Changing the test to be an integration/e2e test fixes that (https://github.com/docker/cli/pull/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.

docker / go-connections

sockets: add TCPProxyFromEnvironment to keep pre-go1.16 behavior #85