- add early returns for `nil` body, `http.NoBody`, and `json.RawMessage`
- use `http.NoBody` instead of `nil` for empty bodies; it's more clear
on intent.
- use json.Encode instead of json.Encoder.Encode(), as we're marshaling
a single JSON document; this also avoid adding a trailing newline.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was setting `text/plain` as default content-type for any
request that had a non-nil body.
However, this would also set the content-type if (e.g.) `http.NoBody` was set,
or if an empty reader was used, which would result in the daemon potentialy
rejecting the request, as it validates request to be using `application/json`;
d9ee22d1ab/daemon/server/httputils/httputils.go (L47-L58)
=== RUN TestCommitInheritsEnv
commit_test.go:30: assertion failed: error is not nil: Error response from daemon: unsupported Content-Type header (text/plain): must be 'application/json'
--- FAIL: TestCommitInheritsEnv (0.02s)
This patch removes setting the default content-type.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Go 1.25 / TLS 1.3 may produce a generic "handshake failure" whereas
TLS 1.2 may produce a "bad certificate" TLS alert.
See https://github.com/golang/go/issues/56371
> https://tip.golang.org/doc/go1.12#tls_1_3
>
> In TLS 1.3 the client is the last one to speak in the handshake, so if
> it causes an error to occur on the server, it will be returned on the
> client by the first Read, not by Handshake. For example, that will be
> the case if the server rejects the client certificate.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Only use checkResponseErr if `client.doRequest` did not return an error;
any error returned by `client.doRequest` means there was an error connecting,
so there's no response to handle (including errors in the response).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Outline that any error returned is a connectivity error and a nil-error
requires the response to be handled (including errors returned in the
response).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was implemented as a method on Client, but the receiver was not used;
make it a regular function to prevent passing around the Client where
not needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before this patch:
DOCKER_HOST=tcp://example.invalid/docker docker version
error during connect: Get "http://example.invalid:2375/docker/v1.51/version": dial tcp: lookup example.invalid: no such host
With this patch:
DOCKER_HOST=tcp://example.invalid/docker docker version
failed to connect to the docker API at tcp://example.invalid:2375/docker: lookup example.invalid: no such host
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before this change, a generic "Cannot connect to the docker daemon" error
was produced which, while helpful, instructed the user to check if the daemon
was running, but didn't provide context on the reason we failed (i.e., the
socket was not found).
This patch adds a dedicated check for cases where the socket was not found,
and preserves the original error.
Before this patch:
DOCKER_HOST=unix:///var/run/no.sock docker version
Cannot connect to the Docker daemon at unix:///var/run/no.sock. Is the docker daemon running?
With this patch:
DOCKER_HOST=unix:///var/run/no.sock docker version
failed to connect to the docker API at unix:///var/run/no.sock; check if the path is correct and the daemon is running: dial unix /var/run/no.sock: connect: no such file or directory
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Previously, we were using os.IsPermission, which doesn't unwrap errors;
change to use `errors.Is` to detect permission errors, and unwrap the
error to remove information about the request, which is irrelevant if
we weren't able to connect in the first place.
Also tweak the error slightly to not assume "docker socket", instead
mentioning "docker API".
Before this;
permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Get "http://%2Fvar%2Frun%2Fdocker.sock/v1.51/version": dial unix /var/run/docker.sock: connect: permission denied
With this patch applied:
permission denied while trying to connect to the docker API at unix:///var/run/docker.sock: dial unix /var/run/docker.sock: connect: permission denied
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This code has various other issue, for which TODOs were added; this
commit only does some initial cleaning up, and improves docs and
test-coverage.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit e98e4a7111 introduced functionality
to hide experimental commands, and hide commands based on API version
negotiation. Before that commit, the user-agent header was used to detect
version-mismatches between the daemon and client based on their binary
version;
3975d648b7/api/server/middleware/user_agent.go (L32-L44)
Because of the above, a check was added to prevent custom headers from
modifying the User-Agent, but given that the user-agent header changed
formatting, and api < 1.25 is long deprecated, it's not very meaningful
to add this check, so let's remove it.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These comments were added to enforce using the correct import path for
our packages ("github.com/docker/docker", not "github.com/moby/moby").
However, when working in go module mode (not GOPATH / vendor), they have
no effect, so their impact is limited.
Remove these imports in preparation of migrating our code to become an
actual go module.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Previously, we were using our own `FromStatusCode` function to map HTTP
status codes to Docker error types. Switch to the containerd code.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
JSON errors were introduced in API 1.24, and daemons running older versions of
the API would return errors as plain-text. However, such API versions would
also send the corresponding content-type header (text/plain), so we don't
really need to make the code version-dependent; there's already fallbacks
in place to handle JSON-responses that don't use the expected format, in
which case we produce a generic status-code error.
Before this patch, the client would print JSON-responses as-is when the
daemon returned an "API version too old" error;
DOCKER_API_VERSION=v1.10 docker info --format '{{.ID}}'
Error response from daemon: {"message":"client version 1.10 is too old. Minimum supported API version is 1.24, please upgrade your client to a newer version"}
With this patch, the client detects that the response is JSON, and prints
a friendlier error-message to help the user discover their client is too
old;
DOCKER_API_VERSION=v1.10 docker info --format '{{.ID}}'
Error response from daemon: client version 1.10 is too old. Minimum supported API version is 1.24, please upgrade your client to a newer version
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Looking in history to learn why this struct existed, shows that this type
was mostly the result of tech-debt accumulating over time;
- originally ([moby@1aa7f13]) most of the request handling was internal;
the [`call()` function][1] would make a request, read the `response.Body`,
and return it as a `[]byte` (or an error if one happened).
- some features needed the statuscode, so [moby@a4bcf7e] added an extra
output variable to return the `response.StatusCode`.
- some new features required streaming, so [moby@fdd8d4b] changed the
function to return the `response.Body` as a `io.ReadCloser`, instead
of a `[]byte`.
- some features needed access to the content-type header, so a new
`clientRequest` method was introduced in [moby@6b2eeaf] to read the
`Content-Type` header from `response.Headers` and return it as a string.
- of course, `Content-Type` may not be the only header needed, so [moby@0cdc3b7]
changed the signature to return `response.Headers` as a whole as a
`http.Header`
- things became a bit unwieldy now, with the function having four (4) output
variables, so [moby@126529c] chose to refactor this code, introducing a
`serverResponse` struct to wrap them all, not realizing that all these
values were effectively deconstructed from the `url.Response`, so now
re-assembling them into our own "URL response", only preserving a subset
of the information available.
- now that we had a custom struct, it was possible to add more information
to it without changing the signature. When there was a need to know the
URL of the request that initiated the response, [moby@27ef09a] introduced
a `reqURL` field to hold the `request.URL` which notably also is available
in `response.Request.URL`.
In short;
- The original implementation tried to (pre-maturely) abstract the underlying
response to provide a simplified interface.
- While initially not needed, abstracting caused relevant information from
the response (and request) to be unavailable to callers.
- As a result, we ended up in a situation where we are deconstructing the
original `url.Response`, only to re-assemble it into our own, custom struct
(`serverResponsee`) with only a subset of the information preserved.
This patch removes the `serverResponse` struct, instead returning the
`url.Response` as-is, so that all information is preserved, allowing callers
to use the information they need.
There is one follow-up change to consider; commit [moby@589df17] introduced
a `ensureReaderClosed` utility. Before that commit, the response body would
be closed in a more idiomatic way through a [`defer serverResp.body.Close()`][2].
A later change in [docker/engine-api@5dd6452] added an optimization to that
utility, draining the response to allow connections to be reused. While
skipping that utility (and not draining the response) would not be a critical
issue, it may be easy to overlook that utility, and to close the response
body in the "idiomatic" way, resulting in a possible performance regression.
We need to check if that optimization is still relevant or if later changes
in Go itself already take care of this; we should also look if context
cancellation is handled correctly for these. If it's still relevant, we could
- Wrap the the `url.Response` in a custom struct ("drainCloser") to provide
a `Close()` function handling the draining and closing; this would re-
introduce a custom type to be returned, so perhaps not what we want.
- Wrap the `url.Response.Body` in the response returned (so, calling)
`response.Body.Close()` would call the wrapped closer.
- Change the signature of `Client.sendRequest()` (and related) to return
a `close()` func to handle this; doing so would more strongly encourage
callers to close the response body.
[1]: 1aa7f1392d/commands.go (L1008-L1027)
[2]: 589df17a1a/api/client/ps.go (L84-L89)
[moby@1aa7f13]: 1aa7f1392d
[moby@a4bcf7e]: a4bcf7e1ac
[moby@fdd8d4b]: fdd8d4b7d9
[moby@6b2eeaf]: 6b2eeaf896
[moby@0cdc3b7]: 0cdc3b7539
[moby@126529c]: 126529c6d0
[moby@27ef09a]: 27ef09a46f
[moby@589df17]: 589df17a1a
[docker/engine-api@5dd6452]: 5dd6452d4d
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before this patch, an API response that's valid JSON, but not the right
schema would be silently discarded by the CLI. For example, due to a bug
in Docker Desktop's API proxy, the "normal" (not JSON error) response
would be returned together with a non-200 status code when using an
unsupported API version;
curl -s -w 'STATUS: %{http_code}\n' --unix-socket /var/run/docker.sock 'http://localhost/v1.99/version'
{"Platform":{"Name":"Docker Desktop 4.38.0 (181016)"},"Version":"","ApiVersion":"","GitCommit":"","GoVersion":"","Os":"","Arch":""}
STATUS: 400
Before this patch, this resulted in no output being shown;
DOCKER_API_VERSION=1.99 docker version
Client:
Version: 27.5.1
API version: 1.99 (downgraded from 1.47)
Go version: go1.22.11
Git commit: 9f9e405
Built: Wed Jan 22 13:37:19 2025
OS/Arch: darwin/arm64
Context: desktop-linux
Error response from daemon:
With this patch, an error is generated based on the status:
DOCKER_API_VERSION=1.99 docker version
Client:
Version: 27.5.1
API version: 1.99 (downgraded from 1.47)
Go version: go1.22.11
Git commit: 9f9e405
Built: Wed Jan 22 13:37:19 2025
OS/Arch: darwin/arm64
Context: desktop-linux
Error response from daemon: API returned a 400 (Bad Request) but provided no error-message
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
commit 1a5dafb31e improved the error messages
produced by adding a check if the client is using as an elevated user. For
this, it attempts to open `\\.\PHYSICALDRIVE0`.
However, it looks like closing the file landed in the wrong branch of the
condition, so the file-handle would not be closed when the os.Open succeeded.
Looking further into this check, it appears the conditions were reversed;
if the check _fails_, it means the user is not running with elevated
permissions, but the check would use elevatedErr == nil.
Fix both by changing the condition to `elevatedErr != nil`.
While at it, also changing the string to use a string-literal, to reduce
the amount of escaping needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function has various errors that are returned when failing to make a
connection (due to permission issues, TLS mis-configuration, or failing to
resolve the TCP address).
The errConnectionFailed error is currently used as a special case when
processing Ping responses. The current code did not consistently treat
connection errors, and because of that could either absorb the error,
or process the empty response.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Attach the context to the request while we're creating it, instead of
creating the context first, and adding the context later.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- remove some intermediate variables
- explicitly return "nil" if there's no error
- remove redundant check for response-headers being nil
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
For local communications (npipe://, unix://), the hostname is not used,
but we need valid and meaningful hostname.
The current code used the client's `addr` as hostname in some cases, which
could contain the path for the unix-socket (`/var/run/docker.sock`), which
gets rejected by go1.20.6 and go1.19.11 because of a security fix for
[CVE-2023-29406 ][1], which was implemented in https://go.dev/issue/60374.
Prior versions go Go would clean the host header, and strip slashes in the
process, but go1.20.6 and go1.19.11 no longer do, and reject the host
header.
This patch introduces a `DummyHost` const, and uses this dummy host for
cases where we don't need an actual hostname.
Before this patch (using go1.20.6):
make GO_VERSION=1.20.6 TEST_FILTER=TestAttach test-integration
=== RUN TestAttachWithTTY
attach_test.go:46: assertion failed: error is not nil: http: invalid Host header
--- FAIL: TestAttachWithTTY (0.11s)
=== RUN TestAttachWithoutTTy
attach_test.go:46: assertion failed: error is not nil: http: invalid Host header
--- FAIL: TestAttachWithoutTTy (0.02s)
FAIL
With this patch applied:
make GO_VERSION=1.20.6 TEST_FILTER=TestAttach test-integration
INFO: Testing against a local daemon
=== RUN TestAttachWithTTY
--- PASS: TestAttachWithTTY (0.12s)
=== RUN TestAttachWithoutTTy
--- PASS: TestAttachWithoutTTy (0.02s)
PASS
[1]: https://github.com/advisories/GHSA-f8f7-69v5-w4vx
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use http.Header, which is more descriptive on intent, and we're already
importing the package in the client. Removing the "header" type also fixes
various locations where the type was shadowed by local variables named
"headers".
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
When constructing the client, and setting the User-Agent, care must be
taken to apply the header in the right location, as custom headers can
be set in the CLI configuration, and merging these custom headers should
not override the User-Agent header.
This patch adds a dedicated `WithUserAgent()` option, which stores the
user-agent separate from other headers, centralizing the merging of
other headers, so that other parts of the (CLI) code don't have to be
concerned with merging them in the right order.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The internal Client request methods which accept an object as a body use
nil to signal that the request should not have a body. But it is easy to
accidentally pass a typed-nil value as the object, e.g. if the object
comes from a function argument or struct field of a concrete type. The
result is that these requests will, surprisingly, have a JSON body of
`null`. Treat typed-nil pointers the same as untyped nils for the
purposes of determining whether or not the request should include a
body.
Stop assuming that POST requests should always have a body. POST /commit
does not require a body, for example.
Signed-off-by: Cory Snider <csnider@mirantis.com>
client/request.go:183:28: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
err = errors.Wrap(err, "In the default daemon configuration on Windows, the docker client must be run with elevated privileges to connect.")
^
client/request.go:186:28: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
err = errors.Wrap(err, "This error may indicate that the docker daemon is not running.")
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's deprecated in Go 1.18:
client/request.go:157:8: SA1019: err.Temporary is deprecated: Temporary errors are not well-defined. Most "temporary" errors are timeouts, and the few exceptions are surprising. Do not use this method. (staticcheck)
if !err.Temporary() {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The io/ioutil package has been deprecated in Go 1.16. This commit
replaces the existing io/ioutil functions with their new definitions in
io and os packages.
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
client/request.go:245:2: S1031: unnecessary nil check around range (gosimple)
if headers != nil {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
