Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

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, golang/net@7b1cca2
(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
Copy link
Member Author

relates to docker/cli#3197

@thaJeztah thaJeztah force-pushed the go116_proxy_detect branch 4 times, most recently from 3475981 to ce64c61 Compare July 22, 2021 11:26
TCPProxyFromEnvironment wraps http.ProxyFromEnvironment, to preserve the
pre-go1.16 behavior for URLs using the 'tcp://' scheme. For other schemes,
golang's standard behavior is preserved (and depends on the Go version used).

Prior to go1.16, `https://` schemes would use HTTPS_PROXY, and any other
scheme would use HTTP_PROXY. However, golang/net@7b1cca2
(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
Copy link
Member Author

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 (docker/cli#3197) was
that;

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.

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.

None yet

1 participant