client/container_exec.go:8:2: import "github.com/containerd/errdefs" imported without alias but must be with alias "cerrdefs" according to config (importas)
"github.com/containerd/errdefs"
^
client/container_exec_test.go:9:2: import "github.com/containerd/errdefs" imported without alias but must be with alias "cerrdefs" according to config (importas)
"github.com/containerd/errdefs"
^
client/container_rename.go:8:2: import "github.com/containerd/errdefs" imported without alias but must be with alias "cerrdefs" according to config (importas)
"github.com/containerd/errdefs"
^
client/pkg/security/security_opts_test.go:8:2: import "gotest.tools/v3/assert/cmp" imported without alias but must be with alias "is" according to config (importas)
"gotest.tools/v3/assert/cmp"
^
client/volume_prune.go:9:2: import "github.com/containerd/errdefs" imported without alias but must be with alias "cerrdefs" according to config (importas)
"github.com/containerd/errdefs"
^
client/volume_prune_test.go:8:2: import "github.com/containerd/errdefs" imported without alias but must be with alias "cerrdefs" according to config (importas)
"github.com/containerd/errdefs"
^
client/container_exec_test.go:10:2: ST1019(related information): other import of "github.com/containerd/errdefs" (staticcheck)
cerrdefs "github.com/containerd/errdefs"
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `ExecInspectResult` type was embedding `ExecInspect`, which is also
defined by the client, so there's no need to abstract it.
While updating, also;
- Rename `ExecID` to `ID`, to match the field-name returned by the API.
- Rename `Pid` to `PID`, to be in the right casing.
- Remove `json` labels, as option-types are not (un)marshaled to JSON.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `ExecCreateResult` was embedding the `container.ExecCreateRespons`,
which in itself was an alias for `common.IDResponse`. This type has a
single field (`ID`) currently, but the embedding made it awkward to use,
for example, when mocking a `ExecCreateResult` using struct-literals:
func execCreateWithID(_ string, _ client.ExecCreateOptions) (client.ExecCreateResult, error) {
return client.ExecCreateResult{ExecCreateResponse: container.ExecCreateResponse{ID: "execid"}}, nil
}
This patch defines it as a local type with the `ID` as field.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This type was introduced in [moby@3f9f231], at which type no API response
types were defined, and the [`containerRouter.getExecByID`] would return
the daemon's internal [`exec.Config`] type from [`backend.ContainerExecInspect`].
Tracing back history about the discrepancy between the type used by the client
and the actual response type; commit [moby@2a34207] added the missing type in
the API, which was documented as part of the API swagger definition since the
start ([moby@0243936]), and updated in [moby@74cb739], so we can't use the
reduced struct as response type.
[moby@3f9f231]: 3f9f23114f
[moby@2a34207]: 2a342079c6
[`containerRouter.getExecByID`]: 3f9f23114f/api/server/router/container/exec.go (L18-L25)
[`backend.ContainerExecInspect`]: 3f9f23114f/api/server/router/container/backend.go (L18)
[`exec.Config`]: 3f9f23114f/daemon/exec/exec.go (L13-L31)
[moby@0243936]: 0243936d92
[moby@74cb739]: 74cb739766
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- move api/types/container.ExecOptions to the client
- rename api/types/container.ExecOptions to ExecCreateRequest
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- move api/types/container.ExecStartOptions to the client
- move api/types/container.ExecAttachOptions to the client
- rename api/types/container.ExecStartOptions to ExecStartRequest
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
ensureReaderClosed was designed to be usable regardless if a response
was nil (error) or non-nil (success). Some code-paths were optimized to
avoid using a defer (which used to have an overhead), but the overhead
of defer is neglectable in current versions of Go, and some of these
optimizations made the logic more complicated (and err-prone).
This patch switches to use a defer for all places.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Not perfect yet, but addressing some godoc "doc" links that needed
to be updated, and touching up some references.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The stdcopy package is used to produce and read multiplexed streams for
"attach" and "logs". It is used both by the API server (to produce), and
the client (to read / de-multiplex).
Move it to the api package, so that it can be included in the api module.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Originally I've found this datarace on a project I'm working at. I'm not
able to consistently reproduce this. But by looking at the codebase I
took a chance to fix other 2 possible function that might produce such
data race.
Original stack trace produced when running `go test -race` on GH CI:
```
WARNING: DATA RACE
Write at 0x00c0005dc688 by goroutine 43:
github.com/docker/docker/client.(*Client).negotiateAPIVersionPing()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/client.go:389 +0x12f
github.com/docker/docker/client.(*Client).checkVersion()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/client.go:298 +0x249
github.com/docker/docker/client.(*Client).getAPIPath()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/client.go:307 +0x76
github.com/docker/docker/client.(*Client).sendRequest()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/request.go:111 +0x9b
github.com/docker/docker/client.(*Client).get()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/request.go:28 +0x736
github.com/docker/docker/client.(*Client).ContainerList()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/container_list.go:47 +0x6f0
Previous read at 0x00c0005dc688 by goroutine 42:
github.com/docker/docker/client.(*Client).ContainerList()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/container_list.go:39 +0x5ef
```
Co-authored-by: Luca Rinaldi <lucarin@protonmail.com>
Signed-off-by: Alessio Perugini <alessio@perugini.xyz>
- Use doc-links for references to other types.
- Fix reference to "types.HijackedConnection", which doesn't exist.
- Use a bullet-list for the different stream formats used.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Add a mention to stdcopy.StdCopy to the documentation, as the stream returned in the HijackedResponse is multiplexed when tty is disabled.
Signed-off-by: Medhy DOHOU <52136144+PowerPixel@users.noreply.github.com>
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>
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>
Introduce a container.ExecCreateResponse type as alias for IDResponse to allow
consumers to use ContainerCommit without having to import the "types" package,
and allows us to differentiate the response for container commit separate from
other endpoints currently using IDResponse.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
In situations where an empty ID was passed, the client would construct an
invalid API endpoint URL, which either resulted in the "not found" handler
being hit (resulting in a "page not found" error), or even the wrong endpoint
being hit if the client follows redirects.
For example, `/containers/<empty id>/json` (inspect) redirects to `/containers/json`
(docker ps))
Given that empty IDs should never be expected (especially if they're part of
the API URL path), we can validate these and return early.
Its worth noting that a few methods already had an error in place; those
methods were related to the situation mentioned above, where (e.g.) an
"inspect" would redirect to a "list" endpoint. The existing errors, for
convenience, mimicked a "not found" error; this patch changes such errors
to an "Invalid Parameter" instead, which is more correct, but it could be
a breaking change for some edge cases where users parsed the output;
git grep 'objectNotFoundError{'
client/config_inspect.go: return swarm.Config{}, nil, objectNotFoundError{object: "config", id: id}
client/container_inspect.go: return container.InspectResponse{}, nil, objectNotFoundError{object: "container", id: containerID}
client/container_inspect.go: return container.InspectResponse{}, objectNotFoundError{object: "container", id: containerID}
client/distribution_inspect.go: return distributionInspect, objectNotFoundError{object: "distribution", id: imageRef}
client/image_inspect.go: return image.InspectResponse{}, nil, objectNotFoundError{object: "image", id: imageID}
client/network_inspect.go: return network.Inspect{}, nil, objectNotFoundError{object: "network", id: networkID}
client/node_inspect.go: return swarm.Node{}, nil, objectNotFoundError{object: "node", id: nodeID}
client/plugin_inspect.go: return nil, nil, objectNotFoundError{object: "plugin", id: name}
client/secret_inspect.go: return swarm.Secret{}, nil, objectNotFoundError{object: "secret", id: id}
client/service_inspect.go: return swarm.Service{}, nil, objectNotFoundError{object: "service", id: serviceID}
client/task_inspect.go: return swarm.Task{}, nil, objectNotFoundError{object: "task", id: taskID}
client/volume_inspect.go: return volume.Volume{}, nil, objectNotFoundError{object: "volume", id: volumeID}
Two such errors are still left, as "ID or name" would probably be confusing,
but perhaps we can use a more generic error to include those as well (e.g.
"invalid <object> reference: value is empty");
client/distribution_inspect.go: return distributionInspect, objectNotFoundError{object: "distribution", id: imageRef}
client/image_inspect.go: return image.InspectResponse{}, nil, objectNotFoundError{object: "image", id: imageID}
Before this patch:
docker container start ""
Error response from daemon: page not found
Error: failed to start containers:
docker container start " "
Error response from daemon: No such container:
Error: failed to start containers:
With this patch:
docker container start ""
invalid container name or ID: value is empty
Error: failed to start containers:
docker container start " "
invalid container name or ID: value is empty
Error: failed to start containers:
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This moves the type to api/types/container and creates an alias for
exec attach; ContainerExecAttach currently uses the same type as
ContainerExecStart, but does not all the same options (and some
options cannot be used).
We need to split the actual types, but lets start with aliasing.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit e6907243af applied a fix for situations
where the client was configured with API-version negotiation, but did not yet
negotiate a version.
However, the checkVersion() function that was implemented copied the semantics
of cli.NegotiateAPIVersion, which ignored connection failures with the
assumption that connection errors would still surface further down.
However, when using the result of a failed negotiation for NewVersionError,
an API version mismatch error would be produced, masking the actual connection
error.
This patch changes the signature of checkVersion to return unexpected errors,
including failures to connect to the API.
Before this patch:
docker -H unix:///no/such/socket.sock secret ls
"secret list" requires API version 1.25, but the Docker daemon API version is 1.24
With this patch applied:
docker -H unix:///no/such/socket.sock secret ls
Cannot connect to the Docker daemon at unix:///no/such/socket.sock. Is the docker daemon running?
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
We try to perform API-version negotiation as lazy as possible (and only execute
when we are about to make an API request). However, some code requires API-version
dependent handling (to set options, or remove options based on the version of the
API we're using).
Currently this code depended on the caller code to perform API negotiation (or
to configure the API version) first, which may not happen, and because of that
we may be missing options (or set options that are not supported on older API
versions).
This patch:
- splits the code that triggered API-version negotiation to a separate
Client.checkVersion() function.
- updates NewVersionError to accept a context
- updates NewVersionError to perform API-version negotiation (if enabled)
- updates various Client functions to manually trigger API-version negotiation
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>
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>
Unlike a plain `net/http/client.Do()`, requests made through client/request
use the `sendRequest` function, which parses the server response, and may
convert non-transport errors into errors (through `cli.checkResponseErr()`).
This means that we cannot assume that no reader was opened if an error is
returned.
This patch changes various locations where `ensureReaderClosed` was only
called in the non-error situation, and uses a `defer` to make sure it's
always called.
`ensureReaderClosed` itself already checks if the response's body was set,
so in situations where the error was due to a transport error, calling
`ensureReaderClosed` should be a no-op.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
ContainerExecAttach used `types.ExecConfig` instead of `types.ExecStartCheck`,
which is the type that's expected by the `/exec/execid/start` API endpoint.
Investigating when this inconsistency was introduced, I found that the client has
sent the additional properties since its first imlpementation in
c786a8ee5e.
The `postContainerExecStart()` at that time used the "jobs" package, which
only took the information from the body that was needed (`Detach` and `Tty`).
Commit 24425021d2 refactored the Exec commands
to remove the "jobs", and introduced the `ExecStartCheck` type, but failed to
update the `cli.hijack()` call with the new type.
The change in this patch should not affect compatibility with older clients,
as the additional information from the `ExecConfig` type is not used (the
API server already decodes to the `ExecStartCheck` type).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Victor Vieux <vieux@docker.com>
update cobra and use Tags
Signed-off-by: Victor Vieux <vieux@docker.com>
allow client to talk to an older server
Signed-off-by: Victor Vieux <vieux@docker.com>
