ConfigureTransport: make sure a clean DialContext is used for tcp #103
Conversation
|
Ci is doing funny things 
|
|
DOH! And I'm guessing we have a cert as fixture somewhere, which has expired and must be regenerated; |
|
golangci-lint errors should go away once #104 is merged. |
akerouanton
force-pushed
the
fix-tcp-DialContext
branch
from
96bb442 to
9d26c90
Compare
|
Based on CI error seen in moby/moby#46739, I added a commit reverting fcf9eb7. |
This reverts commit fcf9eb7. Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
fix-tcp-DialContext
branch
from
9d26c90 to
3314934
Compare
sockets/sockets.go
Outdated
| @@ -24,6 +25,10 @@ func ConfigureTransport(tr *http.Transport, proto, addr string) error { | |||
| return configureNpipeTransport(tr, proto, addr) | |||
| default: | |||
| tr.Proxy = http.ProxyFromEnvironment | |||
| tr.DisableCompression = false | |||
There was a problem hiding this comment.
Still looking at this one, but I don't have a good answer. The general issue here is that as you described in the commit message (e.g.) configureUnixTransport mutates this option
go-connections/sockets/sockets_unix.go
Lines 19 to 24 in 5cc4da5
configureUnixTransport having been called before we reach this (and have implicitly mutated that property).
Effectively, that could also be considered a bug in the caller code, i.e., the caller code has re-used a transport, and tried to configure it multiple times. However, I don't know if there's code-paths where this happens implicitly; do you know if those are present?
If we unconditionally enable compression here, that would also mean that there's no option for a HTTP transport to disable compression, which could be a legitimate case, and may also be unexpected 🤔
But honestly, I have no good suggestion what would be best here (other than suggesting; don't re-use transport (or more factually: don't call ConfigureTransport on the same transport multiple times) if it should not be re-used.
There was a problem hiding this comment.
I guess the alternative could be to remove that part from configureUnixTransport, and somehow make sure that when the transport is used, the compression is disabled, but that would require changes elsewhere to keep the existing behavior.
That part was added in 88d5fb2, but it's possible that code was moved from elsewhere 🤔
There was a problem hiding this comment.
Effectively, that could also be considered a bug in the caller code, i.e., the caller code has re-used a transport, and tried to configure it multiple times. However, I don't know if there's code-paths where this happens implicitly; do you know if those are present?
Yeah, there's one major user of that pattern: the official Docker client 🙈
defaultHTTPClientinitializes a new transport: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/client.go#L237-L238- Then,
WithHostcallsConfigureTransporton that same transport: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/options.go#L71-L73 - Both are called from
NewClientWithOpts: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/client.go#L181-L197
I think we can't / shouldn't create a new transport in WithHost (instead of mutating the existing one), as there're at least two other option funcs that mutate the same transport:
WithTLSClientConfig: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/options.go#L136-L155WithDialContext: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/options.go#L50-L58
That part was added in 88d5fb2, but it's possible that code was moved from elsewhere 🤔
It was! It comes from this commit moby/moby@fb7ceeb1. I think it makes a lot of sense to disable compression on UNIX socket, as it's only going to make the CPU spin needlessly more.
IMO it's fine to reconfigure compression when the transport protocol is changed as it's somewhat tied to it, ie. you generally want compression for HTTP over TCP, and you don't want it for HTTP over UNIX / npipe sockets.
Currently, and presumably since forever, compression has been disabled for HTTP over TCP due to the transport being initialized by defaultHTTPClient with a UNIX / npipe transport.
which means it's now ambiguous if it was the caller's intent to have compression disabled, or if it was a side-effect of configureUnixTransport having been called before we reach this (and have implicitly mutated that property).
I think it's OK to warn ConfigureTransport and WithHost callers through a doc comment that both functions will toggle compression based on what protocol is passed. If users really want to enforce that value, they should set it after these functions are called.
There was a problem hiding this comment.
If we agree on that ⬆️, I need to update the doc comment on ConfigureTransport (and on WithHost in moby/moby).
There was a problem hiding this comment.
Yeah, there's one major user of that pattern: the official Docker client 🙈
Yes, that's what I was afraid of 😞
I think it makes a lot of sense to disable compression on UNIX socket, as it's only going to make the CPU spin needlessly more.
Yup, I definitely agree with that; it does make sense to not have compression there. The tricky bit is indeed that (per the other comment) changing that option also affects other paths 😞
IMO it's fine to reconfigure compression when the transport protocol is changed as it's somewhat tied to it, ie. you generally want compression for HTTP over TCP, and you don't want it for HTTP over UNIX / npipe sockets.
It's probably fine to keep the status quo for now (at least until we cleaned up all of this). I had feature requests, such as moby/moby#1266 in mind, where it may be desirable to disable compression (for local connections). Admitted; in that case it's mostly about compressing the layers (which would have a much bigger impact for those cases)
I think it's OK to warn ConfigureTransport and WithHost callers through a doc comment that both functions will toggle compression based on what protocol is passed. If users really want to enforce that value, they should set it after these functions are called.
Documenting definitely won't hurt. At least we shouldn't try to hide that information.
Perhaps (in future) we should look at making this all more atomic somehow (but we'd have to look what the best approach is)
There was a problem hiding this comment.
After discussing privately this change with @thaJeztah, I'll move this change to another commit/PR to make it clear why it's changed.
Since docker#61, there's no more DialContext defined in the default switch case of ConfigureTransport. As a consequence, if ConfigureTransport was already called with a npipe or unix proto (ie. Docker client does that to initialize its HTTP client with the default DOCKER_HOST), the DialContext function defined by these protocols will be used. As the DialContext functions defined by unix and npipe protos totally ignore DialContext's 2nd and 3rd argument (ie. network and addr) and instead use the equivalent arguments passed to configureUnixTransport and configureNpipeTransport when they were called, this results in ConfigureTransport being totally ineffective. Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
fix-tcp-DialContext
branch
from
3314934 to
ffe640f
Compare
|
Thanks all! Let me bring this one in |
`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 is currently never using HTTP compression. Note that `WithHost` is called whenever the `WithHostFromEnv` or `FromEnv` option funcs are used. 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]). 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. As such, this commit make sure compression is enabled in such case. 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 = false` 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 <[email protected]>
`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 (presumably) **the Docker client has 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 = false` 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 <[email protected]>
`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 = false` 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 <[email protected]>
`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 <[email protected]>
Since #61, there's no more DialContext defined in the default switch case of ConfigureTransport. As a consequence, if ConfigureTransport was already called with a npipe or unix proto (ie. Docker client does that to initialize its HTTP client with the default DOCKER_HOST), the DialContext function defined by these protocols will be used.
As the DialContext functions defined by unix and npipe protos totally ignore DialContext's 2nd and 3rd argument (ie. network and addr) and instead use the equivalent arguments passed to configureUnixTransport and configureNpipeTransport when they were called, this results in ConfigureTransport being totally ineffective.
In addition, DisableCompression is also reset to false.