- 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>
tlsconfig.Client() does various things, including reading certs and
checking them. So we may as well return early if we're not gonna be
able to use the config.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- don't use un-keyed structs
- use http consts where possible
- use errors.As instead of manually checking the error-type
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use Client.buildRequest instead of a local copy of the same logic so
that we're using the same logic, and there's less chance of diverging.
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 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>
The "version" header was added in c0afd9c873,
but used the wrong information to get the API version.
This issue was fixed in a9d20916c3, which switched
the API handler code to get the API version from the context. That change is part
of Docker Engine 20.10 (API v1.30 and up)
This patch updates the code to only set the header on APi v1.29 and older, as it's
not used by newer API versions.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This adds an additional interval to be used by healthchecks during the
start period.
Typically when a container is just starting you want to check if it is
ready more quickly than a typical healthcheck might run. Without this
users have to balance between running healthchecks to frequently vs
taking a very long time to mark a container as healthy for the first
time.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
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>
- 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>
None of the client will return the old error-types, so there's no need
to keep the compatibility code. We can consider deprecating this function
in favor of the errdefs equivalent this.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The 'Deprecated:' line in NewClient's doc comment was not in a new
paragraph, so GoDoc, linters, and IDEs were unaware that it was
deprecated. The package documentation also continued to reference
NewClient. Update the doc comments to finish documenting that NewClient
is deprecated.
Signed-off-by: Cory Snider <csnider@mirantis.com>
This adds a function to the client package which can be used to create a
buildkit client from our moby client.
Example:
```go
package main
import (
"context"
"github.com/moby/moby/client"
bkclient "github.com/moby/buildkit/client"
)
func main() {
c := client.NewWithOpts()
bc, _ := bkclient.New(context.Background(), ""
client.BuildkitClientOpts(c),
)
// ...
}
```
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Repro steps:
- Run Docker Desktop
- Run `docker run busybox tail -f /dev/null`
- Run `pkill "Docker Desktop"
Expected:
An error message that indicates that Docker Desktop is shutting down.
Actual:
An error message that looks like this:
```
error waiting for container: invalid character 's' looking for beginning of value
```
here's an example:
https://github.com/docker/for-mac/issues/6575#issuecomment-1324879001
After this change, you get an error message like:
```
error waiting for container: copying response body from Docker: unexpected EOF
```
which is a bit more explicit.
Signed-off-by: Nick Santos <nick.santos@docker.com>
This function was suppressing errors coming from ConfigureTransport, with the
assumption that it will only be used with the Default configuration, and only return
errors for invalid / unsupported schemes (e.g., using "npipe" on a Linux environment);
d109e429dd/vendor/github.com/docker/go-connections/sockets/sockets_unix.go (L27-L29)
Those errors won't happen when the default is passed, so this is mostly theoretical.
Let's return the error instead (which should always be nil), just to be on the save
side, and to make sure that any other use of this function will return errors that
occurred, which may also be when parsing proxy environment variables;
d109e429dd/vendor/github.com/docker/go-connections/sockets/sockets.go (L29)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This makes the `ImageList` function to add `shared-size=1` to the url
query when user caller sets the SharedSize.
SharedSize support was introduced in API version 1.42. This field was
added to the options struct, but client wasn't adjusted.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
client/events.go:19:115: empty-lines: extra empty line at the start of a block (revive)
client/events_test.go:60:31: empty-lines: extra empty line at the start of a block (revive)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Had this stashed as part of some other changes, so thought I'd push
as a PR; with this patch:
=== RUN TestNewClientWithOpsFromEnv/invalid_cert_path
=== RUN TestNewClientWithOpsFromEnv/default_api_version_with_cert_path
=== RUN TestNewClientWithOpsFromEnv/default_api_version_with_cert_path_and_tls_verify
=== RUN TestNewClientWithOpsFromEnv/default_api_version_with_cert_path_and_host
=== RUN TestNewClientWithOpsFromEnv/invalid_docker_host
=== RUN TestNewClientWithOpsFromEnv/invalid_docker_host,_with_good_format
=== RUN TestNewClientWithOpsFromEnv/override_api_version
--- PASS: TestNewClientWithOpsFromEnv (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/default_api_version (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/invalid_cert_path (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/default_api_version_with_cert_path (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/default_api_version_with_cert_path_and_tls_verify (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/default_api_version_with_cert_path_and_host (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/invalid_docker_host (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/invalid_docker_host,_with_good_format (0.00s)
--- PASS: TestNewClientWithOpsFromEnv/override_api_version (0.00s)
PASS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was deprecated in edac92409a, which
was part of 18.09 and up, so should be safe by now to remove this.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was only used in a single location, and only a "convenience" type,
not used to detect a specific error.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These were deprecated in ee230d8fdd,
which is in the 22.06 branch, so we can safely remove it from
master to have them removed in the release after that.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Update IsErrNotFound() to check for the current type before falling back to
detecting the deprecated type.
- Remove unauthorizedError and notImplementedError types, which were not used.
- IsErrPluginPermissionDenied() was added in 7c36a1af03,
but not used at the time, and still appears to be unused.
- Deprecate IsErrUnauthorized in favor of errdefs.IsUnauthorized()
- Deprecate IsErrNotImplemented in favor of errdefs,IsNotImplemented()
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Older versions of Go don't format comments, so committing this as
a separate commit, so that we can already make these changes before
we upgrade to Go 1.19.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
Now client have the possibility to set the console size of the executed
process immediately at the creation. This makes a difference for example
when executing commands that output some kind of text user interface
which is bounded by the console dimensions.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
On Linux the daemon was not respecting the HostConfig.ConsoleSize
property and relied on cli initializing the tty size after the container
was created. This caused a delay between container creation and
the tty actually being resized.
This is also a small change to the api description, because
HostConfig.ConsoleSize is no longer Windows-only.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
This commit replaces `os.Setenv` with `t.Setenv` in tests. The
environment variable is automatically restored to its original value
when the test and all its subtests complete.
Reference: https://pkg.go.dev/testing#T.Setenv
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Just a small clean-up (there's more endpoints to do this for, but
I was working on changes in this area on the CLI when I noticed we
were setting this query-parameter unconditionally.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was added in 93c3e6c91e, at which time only
some basic handling of non-succesful status codes was present;
93c3e6c91e/api/client/utils.go (L112-L121)
Given that since 38e6d474af non-successful status-
codes are already handled, and a 204 ("no content") status should not be an error,
this special case should no longer be needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
both -1 and 0 are accepted as "no limit", so don't send the
limit option if no limit was set. For simplicity, we're ignoring
values <= 0.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The wrapResponseError() utility converted some specific errors, but in
doing so, could hide the actual error message returned by the daemon.
In addition, starting with 38e6d474af,
HTTP status codes were already mapped to their corresponding errdefs
types on the client-side, making this conversion redundant.
This patch removes the wrapResponseError() utility; it's worth noting
that some error-messages will change slightly (as they now return the
error as returned by the daemon), but may cointain more details as
before, and in some cases prevents hiding the actual error.
Before this change:
docker container rm nosuchcontainer
Error: No such container: nosuchcontainer
docker container cp mycontainer:/no/such/path .
Error: No such container:path: mycontainer:/no/such/path
docker container cp ./Dockerfile mycontainer:/no/such/path
Error: No such container:path: mycontainer:/no/such
docker image rm nosuchimage
Error: No such image: nosuchimage
docker network rm nosuchnetwork
Error: No such network: nosuchnetwork
docker volume rm nosuchvolume
Error: No such volume: nosuchvolume
docker plugin rm nosuchplugin
Error: No such plugin: nosuchplugin
docker checkpoint rm nosuchcontainer nosuchcheckpoint
Error response from daemon: No such container: nosuchcontainer
docker checkpoint rm mycontainer nosuchcheckpoint
Error response from daemon: checkpoint nosuchcheckpoint does not exist for container mycontainer
docker service rm nosuchservice
Error: No such service: nosuchservice
docker node rm nosuchnode
Error: No such node: nosuchnode
docker config rm nosuschconfig
Error: No such config: nosuschconfig
docker secret rm nosuchsecret
Error: No such secret: nosuchsecret
After this change:
docker container rm nosuchcontainer
Error response from daemon: No such container: nosuchcontainer
docker container cp mycontainer:/no/such/path .
Error response from daemon: Could not find the file /no/such/path in container mycontainer
docker container cp ./Dockerfile mycontainer:/no/such/path
Error response from daemon: Could not find the file /no/such in container mycontainer
docker image rm nosuchimage
Error response from daemon: No such image: nosuchimage:latest
docker network rm nosuchnetwork
Error response from daemon: network nosuchnetwork not found
docker volume rm nosuchvolume
Error response from daemon: get nosuchvolume: no such volume
docker plugin rm nosuchplugin
Error response from daemon: plugin "nosuchplugin" not found
docker checkpoint rm nosuchcontainer nosuchcheckpoint
Error response from daemon: No such container: nosuchcontainer
docker checkpoint rm mycontainer nosuchcheckpoint
Error response from daemon: checkpoint nosuchcheckpoint does not exist for container mycontainer
docker service rm nosuchservice
Error response from daemon: service nosuchservice not found
docker node rm nosuchnode
Error response from daemon: node nosuchnode not found
docker config rm nosuchconfig
Error response from daemon: config nosuchconfig not found
docker secret rm nosuchsecret
Error response from daemon: secret nosuchsecret not found
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The API defines a default limit for searches, but when removing the
default from the cli, the client still sends "0" as a limit, which
is not allowed by existing versions of the API:
docker search --limit=0 busybox
Error response from daemon: Limit 0 is outside the range of [1, 100]
This patch changes the client so that no limit is sent if none was set ("0"),
allowing the daemon to use its (or the registry's) default.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This adds consts for the environment variables that are supported
by the client. These environment variables are unlikely to change,
or at least, unlikely to be removed, but having consts allows for
them to be documented.
I did not change all occurrences of these variables to use the const,
as they're used in various tests, and it's ok to use a fixture for
those, but it's nice to have a const available for (external) consumers
of the client package, and to have their purpose (and caveats)
documented in the code.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Improve documentation of various functions to better describe their behavior.
- Rename some variables to be more descriptive (as this is client code, used
by external consumers, it's nice to be a bit more explicit).
- Remove a redundant check in `WithVersionFromEnv()`, as `WithVersion()`
already checks for empty values.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This test was setting the non-exported `Client.basePath` directly, however,
it was setting it to a value that would never realistically happen, because
`NewClientWithOpts()` initializes the Client with the default API version:
ea5b4765d9/client/client.go (L119-L130)
Which is used by `getAPIPath()` to construct the URL/path:
ea5b4765d9/client/client.go (L176-L190)
While this didn't render the test "invalid", using a Client that's constructed
in the usual way, makes it more representative.
Given that we deprecated (but still support) the non-versioned API paths, with
the exception of the `/_ping` API endpoint, we should probably change `getAPIPath()`
to default to the "current version", instead of allowing it to use an empty string.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- avoid accessing non-exported fields where possible, and test using accessors
instead, so that we're closer to how it's actually used.
- use a variable or const for "expected" in some tests, so that "expected" is
printed as part of the test-failure output (instead of just a "value").
- swap the order of "actual" and "expected" for consistency, and to make it
easier to see what the "expected" value is in some cases ("expected" on the
right, so that it reads `val (actual) != val (expected)`).
- don't set fields in the Ping response that are not relevant to the test.
- rename some variables for consistency.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I think this was there for historic reasons (may have been goimports expected
this, and we used to have a linter that wanted it), but it's not needed, so
let's remove it (to make my IDE less complaining about unneeded aliases).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The client would always send a value, even if no `condition` was set;
Calling POST /v1.41/containers/foo/wait?condition=
This patch changes the client to not send the parameter if it's empty (and the
API default value should be used).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
As caught by gosimple:
client/client.go:138:14: S1040: type assertion to the same type: c.client.Transport already has type http.RoundTripper (gosimple)
if _, ok := c.client.Transport.(http.RoundTripper); !ok {
^
This check was originally added in dc9f5c2ca3, to
check if the passed option was a `http.Transport`, and later changed in
e345cd12f9 to check for `http.RoundTripper` instead.
Client.client is a http.Client, for which the Transport field is a RoundTripper,
so this check is redundant.
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>
Unify the NetworkInspect tests to remove some boilerplating
Before this change:
go test -v -run TestNetworkInspect ./client/
=== RUN TestNetworkInspectError
--- PASS: TestNetworkInspectError (0.00s)
=== RUN TestNetworkInspectNotFoundError
--- PASS: TestNetworkInspectNotFoundError (0.00s)
=== RUN TestNetworkInspectWithEmptyID
--- PASS: TestNetworkInspectWithEmptyID (0.00s)
=== RUN TestNetworkInspect
--- PASS: TestNetworkInspect (0.00s)
PASS
ok github.com/docker/docker/client 0.010s
With this change:
go test -v -run TestNetworkInspect ./client/
=== RUN TestNetworkInspect
=== RUN TestNetworkInspect/empty_ID
=== RUN TestNetworkInspect/no_options
=== RUN TestNetworkInspect/verbose
=== RUN TestNetworkInspect/global_scope
=== RUN TestNetworkInspect/unknown_network
=== RUN TestNetworkInspect/server_error
--- PASS: TestNetworkInspect (0.00s)
--- PASS: TestNetworkInspect/empty_ID (0.00s)
--- PASS: TestNetworkInspect/no_options (0.00s)
--- PASS: TestNetworkInspect/verbose (0.00s)
--- PASS: TestNetworkInspect/global_scope (0.00s)
--- PASS: TestNetworkInspect/unknown_network (0.00s)
--- PASS: TestNetworkInspect/server_error (0.00s)
PASS
ok github.com/docker/docker/client 0.012s
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
