integration-cli/docker_cli_external_volume_driver_test.go:116:4: Error return value of `(*encoding/json.Encoder).Encode` is not checked: unsafe type `interface{}` found (errchkjson)
json.NewEncoder(w).Encode(&data)
^
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>
Now a gratuitous/unsolicted ARP is sent, there's no need to
use an IPv4-based MAC address to preserve arp-cache mappings
between an endpoint's IP addresses and its MAC addresses.
Because a random MAC address is used for the default bridge,
it no longer makes sense to derive container IPv6 addresses
from the MAC address. This "postIPv6" behaviour was needed
before IPv6 addresses could be configured, but not now. So,
IPv6 addresses will now be IPAM-allocated on the default
bridge network, just as they are for user-defined bridges.
Signed-off-by: Rob Murray <rob.murray@docker.com>
The StatsResponse type was a compatibility-wrapper introduced in
d3379946ec to differentiate responses
for API < 1.21 and API >= 1.21. API versions lower than 1.24 are
deprecated, and we should merge StatsResponse and Stats, but let's
start with using the StatsResponse in our tests.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This test was testing errors produced by runc; both the "maximum" and
"minimum" values originate from the OCI runtime;
d48d9cfefc/libcontainer/cgroups/fs/cpu.go (L66-L83)
docker run --cpu-shares=1 alpine
docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error setting cgroup config for procHooks process: the minimum allowed cpu-shares is 2: unknown.
Happy path for this setting is covered by TestRunWithCPUShares, and
various other tests, so we validate that the options take effect;
f5af46d4d5/integration-cli/docker_cli_run_unix_test.go (L494-L503)
This patch:
- removes the test and migrates it to an integration test
- removes the checks for errors that might be produced by runc
- updates our validation for invalid (negative) values to happen
when creating the contaienr; the existing check that happened when
creating the OCI spec is preserved, so that configs of existing containers
are still validated.
- updates validateResources to return the correct error-type
- updated unit-test to validate
With this patch:
make TEST_FILTER='TestCreateInvalidHostConfig' TEST_SKIP_INTEGRATION_CLI=1 test-integration
--- PASS: TestCreateInvalidHostConfig (0.00s)
--- PASS: TestCreateInvalidHostConfig/invalid_IpcMode (0.00s)
--- PASS: TestCreateInvalidHostConfig/invalid_CPUShares (0.00s)
--- PASS: TestCreateInvalidHostConfig/invalid_PidMode (0.00s)
--- PASS: TestCreateInvalidHostConfig/invalid_PidMode_without_container_ID (0.00s)
--- PASS: TestCreateInvalidHostConfig/invalid_Annotations (0.00s)
--- PASS: TestCreateInvalidHostConfig/invalid_UTSMode (0.00s)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This test originally added in 4352da7803,
and was a bit involved as it involved building an image, and had some
dubious test-cases, such as using `wrongimage:<ID of other image>` as
reference, and expecting that to produce a "not found" error. Possibly
this format was supported in the past, but currently it fails equally with
`correctimage:<ID of image>`.
This patch rewrites the test to an integration test, and removes the test
from integration-cli. It also removes TestCreate64ByteHexID, as it was
duplicated by this test.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The use of bufio for writing without flushing can lead to an incomplete
writing of the tar and subsequent unexpected EOF when importing.
Signed-off-by: Derek McGowan <derek@mcg.dev>
The netfilter module is now loaded on-demand, and no longer during daemon
startup, making these fields obsolete. These fields are now always `false`
and will be removed in the next relase.
This patch deprecates:
- the `BridgeNfIptables` field in `api/types/system.Info`
- the `BridgeNfIp6tables` field in `api/types/system.Info`
- the `BridgeNFCallIPTablesDisabled` field in `pkg/sysinfo.SysInfo`
- the `BridgeNFCallIP6TablesDisabled` field in `pkg/sysinfo.SysInfo`
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 3ec19ff62b introduced a GC goroutine to delete files where netns
were mounted. It was primarly added to work around a race in kernel
3.18-4.0.1. Since no distros we support are using such old kernels,
there's no need to keep this code around.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Non-distributable artifacts (foreign layers) were introduced in commit
05bd04350b to accommodate Windows images,
for which the EULA did not allow layers to be distributed through registries
other than those hosted by Microsoft. The concept of foreign / non-distributable
layers was adopted by the OCI distribution spec in [oci#233].
These restrictions were relaxed later to allow distributing these images
through non-public registries, for which a configuration was added in the
daemon in 67fdf574d5. In 2022, Microsoft updated
the EULA and [removed these restrictions altogether][1], and the OCI distribution
spec deprecated the functionality in [oci#965].
In 2023, Microsoft [removed the use of foreign data layers][2] for their images,
making this functionality obsolete.
This patch:
- Deprecates the `--allow-nondistributable-artifacts` daemon flag and corresponding
`allow-nondistributable-artifacts` field in `daemon.json`. Setting either
option will no longer take an effect, but a deprecation warning log is added
to raise awareness about the deprecation. This warning is planned to become
an error in the next release.
- Deprecates the `RegistryConfig.AllowNondistributableArtifactsCIDRs` and
`RegistryConfig.AllowNondistributableArtifactsHostnames` fields in the
`GET /info` API response. For API version v1.48 and lower, the fields are
still included in the response, but always `null`. In API version v1.49 and
higher, the field will be omitted entirely.
- Deprecates the `api/types/registry/ServiceConfig.AllowNondistributableArtifactsCIDRs`
field.
- Deprecates the `api/types/registry/ServiceConfig.AllowNondistributableArtifactsHostnames`
field.
- Deprecates the `registry.ServiceOptions.AllowNondistributableArtifacts` field.
[oci#233]: https://github.com/opencontainers/image-spec/pull/233
[oci#965]: https://github.com/opencontainers/image-spec/pull/965
[1]: https://techcommunity.microsoft.com/blog/containers/announcing-windows-container-base-image-redistribution-rights-change/3645201
[2]: https://techcommunity.microsoft.com/blog/containers/announcing-removal-of-foreign-layers-from-windows-container-images/3846833
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This partially reverts e440831802 ("fix and skip some tests based on
API version"), which caused the integration-cli tests to skip all
AppArmor-related tests on SUSE.
It's not really clear why this was done originally, but I have verified
that on modern SLE 12 and SLE 15 systems the AppArmor tests pass without
any adjustments needed.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Test 'TestDockerCLIRunSuite/TestRunAttachFailedNoLeak' does this ...
- start a container that exits immediately, its comment says:
- "Run a dummy container to ensure all goroutines are up and running
before we get a count"
- wait for the number of goroutines to be stable for 400ms, and remember
that number
- start a container
- start another container, expecting it to fail with a port-mapping clash
- stop the running container
- wait for up to 30s for the number of goroutines to fall back to the
remembered number.
In a CI run - hacking in some debug to count goroutines once a second,
before waiting for the number to stablilise for 400ms, showed that the
initial (dummy) container run had no immediate effect. But, three more
goroutines appeared within a few seconds. For example:
=== RUN TestDockerCLIRunSuite/TestRunAttachFailedNoLeak
docker_cli_run_test.go:3822: goroutines before container run: 47 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 47 i 0 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 48 i 1 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 48 i 2 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 48 i 3 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 48 i 4 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 50 i 5 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 50 i 6 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 50 i 7 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 50 i 8 err <nil>
docker_cli_run_test.go:3830: goroutines after container run: 50 i 9 err <nil>
That means a delay while running the rest of the test risks finding the
extra goroutines that are going to start anyway and not go away (regardless
of whether more containers are started).
So - wait for the goroutine count to be stable for 7s, rather than 400ms.
Signed-off-by: Rob Murray <rob.murray@docker.com>
If we have an error type that we're checking a substring against, we
should really be checking using ErrorContains to indicate the right
semantics to assert.
Mostly done using these transforms:
find . -type f -name "*_test.go" | \
xargs gofmt -w -r 'assert.Assert(t, is.ErrorContains(e, s)) -> assert.ErrorContains(t, e, s)'
find . -type f -name "*_test.go" | \
xargs gofmt -w -r 'assert.Assert(t, is.Contains(err.Error(), s)) -> assert.ErrorContains(t, err, s)'
find . -type f -name "*_test.go" | \
xargs gofmt -w -r 'assert.Check(t, is.Contains(err.Error(), s)) -> assert.Check(t, is.ErrorContains(err, s))'
As well as some small fixups to helpers that were doing
strings.Contains explicitly.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
If a values is non-nil when we don't expect it, it would be quite
helpful to get an error message explaining what happened.
find . -type f -name "*_test.go" | \
xargs gofmt -w -r "assert.Assert(t, a == nil) -> assert.Assert(t, is.Nil(a))"
find . -type f -name "*_test.go" | \
xargs gofmt -w -r "assert.Check(t, a == nil) -> assert.Check(t, is.Nil(a))"
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Migrated using
find . -type f -name "*_test.go" |
xargs gofmt -w \
-r "assert.Check(t, strings.Contains(a, b)) -> assert.Check(t, is.Contains(a, b))"
find . -type f -name "*_test.go" |
xargs gofmt -w \
-r "assert.Assert(t, strings.Contains(a, b)) -> assert.Assert(t, is.Contains(a, b))"
Using a boolean in assert.Assert or assert.Check results in error
messages that don't contain the actual problematic string, and when
running the integration suite on an actual machine (where the source
code parsing doesn't work) this makes it almost impossible to figure out
what the actual error is.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
integration-cli/docker_api_containers_test.go:1748:3: The copy of the 'for' variable "x" can be deleted (Go 1.22+) (copyloopvar)
x := x
^
integration-cli/docker_api_containers_test.go:1916:3: The copy of the 'for' variable "x" can be deleted (Go 1.22+) (copyloopvar)
x := x
^
integration-cli/docker_cli_build_test.go:6203:3: The copy of the 'for' variable "builder" can be deleted (Go 1.22+) (copyloopvar)
builder := builder
^
integration-cli/docker_cli_build_test.go:6227:4: 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>
integration-cli/docker_utils_test.go:492:3: shadow: declaration of "line" shadows declaration at line 491 (govet)
line := strings.TrimSpace(line)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These already had a comment, so let's make it a ignore-comment
integration-cli/docker_cli_exec_test.go:409:13: G302: Expect file permissions to be 0600 or less (gosec)
f, err := os.OpenFile(netFilePath, os.O_WRONLY|os.O_SYNC|os.O_APPEND, 0o644)
^
integration-cli/docker_cli_run_test.go:3050:12: G302: Expect file permissions to be 0600 or less (gosec)
if err := os.Chmod(filename, 0o646); err != nil {
^
integration-cli/docker_cli_run_test.go:3072:12: G302: Expect file permissions to be 0600 or less (gosec)
if err := os.Chmod(filename, 0o646); err != nil {
^
integration-cli/docker_cli_run_test.go:3094:12: G302: Expect file permissions to be 0600 or less (gosec)
if err := os.Chmod(filename, 0o646); err != nil {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
integration-cli/docker_cli_run_test.go:401:12: G302: Expect file permissions to be 0600 or less (gosec)
f, err := os.OpenFile(filepath.Join(dir, "test"), os.O_CREATE, 0o700)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
integration-cli/docker_utils_test.go:187:12: G302: Expect file permissions to be 0600 or less (gosec)
f, err := os.OpenFile(dst, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0o700)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
integration-cli/docker_utils_test.go:149:8: G601: Implicit memory aliasing in for loop. (gosec)
m = &c
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
A recent change to the vishvananda/netlink package exposes
NLM_F_DUMP_INTR in some netlink responses as an EINTR (with
no data).
Retry the requests when that happens, up to five times, before
returning the error. The limit of five is arbitrary, on most
systems a single retry will be rare but, there's no guarantee
that a retry will succeed. So, on a very busy or misbehaving
system the error may still be returned. In most cases, this
will lead to failure of the operation being attempted (which
may lead to daemon startup failure, network initialisation
failure etc).
Signed-off-by: Rob Murray <rob.murray@docker.com>
Add `Platform` parameter that allows to select a specific platform to
show the history for.
This is a breaking change to the Go client as it changes the signature
of `ImageHistory`.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Use the same error message as the graphdrivers image store backend.
It's more informative as it also includes the requested platform and
won't break clients checking doing error check with string-matching.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Also fix some unhandled errors.
integration-cli/docker_cli_swarm_test.go:697:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"Error":"failed to add veth pair: `+err.Error()+`"}`)
^
integration-cli/docker_cli_swarm_test.go:731:18: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"LocalDefaultAddressSpace":"`+lAS+`", "GlobalDefaultAddressSpace": "`+gAS+`"}`)
^
integration-cli/docker_cli_swarm_test.go:742:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"Error":"Unknown address space in pool request: `+poolRequest.AddressSpace+`"}`)
^
integration-cli/docker_cli_swarm_test.go:746:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"PoolID":"`+poolID+`", "Pool":"`+pool+`"}`)
^
integration-cli/docker_cli_swarm_test.go:763:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"Address":"`+gw+`"}`)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Also rename some variables that shadowed imports, and fix some
unhandled errors.
integration-cli/docker_cli_network_unix_test.go:102:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"Error":"failed to add veth pair: `+err.Error()+`"}`)
^
integration-cli/docker_cli_network_unix_test.go:136:18: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"LocalDefaultAddressSpace":"`+lAS+`", "GlobalDefaultAddressSpace": "`+gAS+`"}`)
^
integration-cli/docker_cli_network_unix_test.go:147:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"Error":"Unknown address space in pool request: `+poolRequest.AddressSpace+`"}`)
^
integration-cli/docker_cli_network_unix_test.go:151:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"PoolID":"`+poolID+`", "Pool":"`+pool+`"}`)
^
integration-cli/docker_cli_network_unix_test.go:168:19: printf: non-constant format string in call to fmt.Fprintf (govet)
fmt.Fprintf(w, `{"Address":"`+gw+`"}`)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
integration-cli/benchmark_test.go:49:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
integration-cli/benchmark_test.go:62:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
integration-cli/benchmark_test.go:68:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
integration-cli/benchmark_test.go:73:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
integration-cli/benchmark_test.go:78:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
integration-cli/benchmark_test.go:84:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
integration-cli/benchmark_test.go:94:27: printf: non-constant format string in call to fmt.Errorf (govet)
chErr <- fmt.Errorf(out)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This condition was added in 0215a62d5b, which
removed pkg/homedir as abstraction, but didn't consider that this test
is currently only ran on Unix.
integration-cli/docker_cli_run_unix_test.go:254:5: SA4032: due to the file's build constraints, runtime.GOOS will never equal "windows" (staticcheck)
if runtime.GOOS == "windows" {
^
integration-cli/docker_cli_run_unix_test.go:338:5: SA4032: due to the file's build constraints, runtime.GOOS will never equal "windows" (staticcheck)
if runtime.GOOS == "windows" {
^
Added a TODO, because this functionality should also be tested on Windows,
probably as part of tests in docker/cli instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before commit 4f09af6, when allocating host ports for a new
port mapping, iptables rules were set up then docker-proxy was
started. If the host port was already in-use, docker-proxy exited
with an error, and the iptables rules were removed. That could
potentially interfere with a non-docker service that was already
using the host port for something unrelated.
Commit 4f09af6 swapped that problem for a different one... in
order to check that a port was available before creating iptables
rules, it attempted to start docker-proxy first. If it failed, it
could then try a different host port, without interfering with
any other service. The problem with that is docker-proxy would
start listening before the iptables rules were in place, so it
could accept connections then become unusable because new NAT
rules diverted packets directly to the container. This would leave
the client with a broken connection, causing at-least a delay
while it figured that out and reconnected.
This change creates and binds the socket in the daemon, before
creating iptables rules. If the bind fails, it may try a different
port. When or if the bind succeeds, iptables rules are created,
then the daemon calls listen on the socket. If docker-proxy is
needed, the socket is handed over to it at that point.
In rootless mode, the ports have to be bound to an address in the
rootless network namespace (where dockerd is running). DNAT rules
now use the same address.
If docker-proxy is not needed ("--userland-proxy=false"), the daemon
still listens on TCP sockets as the old dummyProxy would have done.
This makes the socket show up in "netstat" output.
The dummyProxy is no longer needed on Linux. Its job was to bind the
host ports if docker-proxy was disabled, but that's now already
handled by binding the sockets early.
This change doesn't affect SCTP, because it's not currently possible
for docker-proxy to convert the file descriptor into an SCTPListener.
So, docker-proxy is still started early, and the window for lost
connections remains.
If the user has an old docker-proxy in their path and it's given a
listener docker with '-use-listen-fd', it'll fail because of the
unknown option. In this case, the daemon's error message suggests
checking $PATH.
Signed-off-by: Rob Murray <rob.murray@docker.com>
This moves the `ContainerJSONBase`, `ContainerJSON` and `ContainerNode`
types to the api/types/container package and deprecates the old location.
- `ContainerJSONBase` was renamed to `InspectBase`
- `ContainerJSON` was rnamed to `InspectResponse`
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This moves the `ImageInspect` and `RootFS` types to the image package,
and deprecates the old location.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This moves the `Health` and `HealthcheckResult` types to the container package,
as well as the related `NoHealthcheck`, `Starting`, `Healthy`, and `Unhealthy`
consts.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
