Make the mocked responses match the API closer;
- Add headers as returned by the daemon's VersionMiddleware
- By default handle "/_ping" requests to allow the client to
perform API-version negotiation as part of tests.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- 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>
Add WithAPIVersion and WithAPIVersionFromEnv to be more clear on
the intent, and to align with other related options and fields.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use a more idiomatic name so that it can be used as `client.New()`.
We should look if we want `New()` to have different / updated defaults
i.e., enable `WithEnv` as default, and have an opt-out and have API-
version negotiation enabled by default (with an opt-out option).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Add a `mockResponse` utility, and slightly enhance it to also include
the request Headers and Status message, to be more closely to actual
responses.
- Add a `mockJSONResponse` utility, implemented using `mockResponse`
- Remove `plainTextErrorMock` in favor of `mockResponse`
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The API does not produce these as a response; the fields in the Ping
struct, including the Swarm status are propagated from headers returned
by the /_ping endpoint.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The WithMockClient option was explicitly resetting the client's API
version (see [1]), which differs from the regular client, which is
initialized with the current API version used by the client (see [2]).
This patch:
- reduces the `WithMockClient` to only set the custom HTTP client, leaving
other fields un-touched.
- adds a test utility and updates tests to handle the API-version prefix
- removes redundant uses of `WithVersion()` in tests; for most test-cases
it was used to make sure a current API version is used that supports the
feature being tested, but there was no test to verify the behavior for
lower API versions, so we may as well test against "latest".
[1]: 5a582729d8/client/client_mock_test.go (L22-L36)
[2]: 5a582729d8/client/client.go (L167-L190)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Move the option-types to the client and in some cases create a
copy for the backend. These types are used to construct query-
args, and not marshaled to JSON, and can be replaced with functional
options in the client.
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>
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>
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>
There were a handful of direct checks against errors.Is that can be
translated to assert.ErrorIs without too much thought. Unfortunately
there are a load of other examples where ErrorIs probably makes sense
but would require testing whether this subtly breaks the test.
These transformations were done by hand.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
client/client_test.go:91:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
client/client_test.go:326:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
client/client_test.go:481:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
client/image_list_test.go:183:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
client/image_push_test.go:163:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
client/image_tag_test.go:50:3: The copy of the 'for' variable "repo" can be deleted (Go 1.22+) (copyloopvar)
repo := repo
^
client/image_tag_test.go:61:3: The copy of the 'for' variable "repotag" can be deleted (Go 1.22+) (copyloopvar)
repotag := repotag
^
client/ping_test.go:114:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
client/request_test.go:53:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
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 assert.Check() where possible to not fail early
- improve checks for error-types
- rename "testURL" var to be more descriptive, and use a const
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- use is.ErrorType
- replace uses of client.IsErrNotFound for errdefs.IsNotFound, as
the client no longer returns the old error-type.
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>
These checks were redundant, as we were not expecting
a specific string, just that a server-error or authentication
error was returned.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
client.checkResponseErr() was hanging and consuming infinite memory
when the serverResp.Body io.Reader returns infinite stream.
This commit prohibits reading more than 1MiB.
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Since Go 1.7, context is a standard package. Since Go 1.9, everything
that is provided by "x/net/context" is a couple of type aliases to
types in "context".
Many vendored packages still use x/net/context, so vendor entry remains
for now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This package doesn't really seem to do anything of real interest.
Removing it and replacing with a few helper functions. Most of this was
maintaining a fork of ctxhttp to support a mock that was unnecessary.
We could probably do with a further refactor of the client interface.
There is a lot of confusion of between transport, http layer and
application layer that makes for some awkward code. This change
improves the situation to the point where no breaking changes are
introduced.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
