Skip to content

Commit

Permalink
ConfigureTransport: enable compression for non unix/npipe protos
Browse files Browse the repository at this point in the history
`ConfigureTransport` is used by the official Docker client to set up its
`http.Transport`:

* [`NewClientWithOpts()`][1] starts by calling `defaultHTTPClient()`
  which creates a new `http.Transport` and call `ConfigureTransport`
  with either the default unix socket address, or the default npipe
  socket address (see [here][2]). This first call to
  `ConfigureTransport` will set `DisableCompression` to false.
* Then, when [`WithHost()`][3] is called, `ConfigureTransport` is called
  once again with the same `http.Transport` instance. If that hostURL
  uses tcp proto, we fall in `ConfigureTransport`'s default switch case,
  which doesn't reset `DisableCompression`. This effectively means that
  **the Docker client has (presumably) never been using HTTP compression
  for TCP hosts.**

Note that `WithHost` is called whenever the `WithHostFromEnv` or
`FromEnv` option funcs are used.

We think it _does_ make sense to keep compression disabled for unix /
npipe sockets (it would just make the CPU needlessly spin more if we
enable it), and we think it generally also makes sense to enable it for
HTTP over TCP.

We discussed creating a new `http.Transport` in Docker client codebase
whenever `ConfigureTransport` is called, but we can't do that without
introducing major side-effects (see [here][4]). So we still have to
leave in a world where multiple calls to `ConfigureTransport` might be
made for the same `http.Transport`.

As such, this commit make sure compression is enabled for non unix /
npipe protos.

If `ConfigureTransport` callers really want to make sure compression is
disabled, they should set `DisableCompression` _only after_ all
subsequent calls to `ConfigureTransport` have been made. The comment on
that function has also been updated to reflect that.

For the record, `DisableCompression = true` was first introduced in
this repo by 88d5fb2, which can be tracked down to this initial change
in moby/moby: moby/moby@fb7ceeb

[1]: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/client.go#L181-L197
[2]: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/client.go#L237-L238
[3]: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/options.go#L71-L73
[4]: docker#103 (comment)

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
  • Loading branch information
akerouanton committed Oct 31, 2023
1 parent 12f72d4 commit 2573a15
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions sockets/sockets.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ const defaultTimeout = 10 * time.Second
// ErrProtocolNotAvailable is returned when a given transport protocol is not provided by the operating system.
var ErrProtocolNotAvailable = errors.New("protocol not available")

// ConfigureTransport configures the specified Transport according to the
// specified proto and addr.
// If the proto is unix (using a unix socket to communicate) or npipe the
// compression is disabled.
// ConfigureTransport configures the specified [http.Transport] according to the specified proto
// and addr.
//
// If the proto is unix (using a unix socket to communicate) or npipe the compression is disabled.
// For other protos, compression is enabled. If you want to manually enable/disable compression,
// make sure you do it _after_ any subsequent calls to ConfigureTransport is made against the same
// [http.Transport].
func ConfigureTransport(tr *http.Transport, proto, addr string) error {
switch proto {
case "unix":
Expand All @@ -25,6 +28,7 @@ func ConfigureTransport(tr *http.Transport, proto, addr string) error {
return configureNpipeTransport(tr, proto, addr)
default:
tr.Proxy = http.ProxyFromEnvironment
tr.DisableCompression = false
tr.DialContext = (&net.Dialer{
Timeout: defaultTimeout,
}).DialContext
Expand Down

0 comments on commit 2573a15

Please sign in to comment.