These tests were made parallel to speed up the execution, but this
turned out to be flaky, because they mutate some shared state.
The tests use shared `storage` variable without any synchronization.
However, adding synchronization is not enough in all cases, some tests
register the same plugin, so they can't be run in parallel to each
other.
This commit adds the synchronization around `storage` variable
modification and removes parallel from the tests where it's not enough.
Before:
```
$ go test -race -v . -count 1
...
--- FAIL: TestGet (15.02s)
--- FAIL: TestGet/not_implemented (0.00s)
testing.go:1446: race detected during execution of test
testing.go:1446: race detected during execution of test
FAIL
FAIL github.com/docker/docker/pkg/plugins 17.655s
FAIL
```
After:
```
$ go test -race -v . -count 1
ok github.com/docker/docker/pkg/plugins 32.702s
```
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
This variable was only accessed from within LocalRegistry methods, but
due to being a package-level variable, tests had to deal with setting
and resetting it.
Move it to be a field scoped to the LocalRegistry. This simplifies the
tests, and to make this more transparent, also removing the "Setup()"
helper (which, wasn't marked as a t.Helper()).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The client's transport can only be set by newClientWithTransport, which
is not exported, and always uses a transport.HTTPTransport.
However, requestFactory is mocked in one of the tests, so keep the interface,
but make it a local, non-exported one.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The interface is not consumed anywhere, and only non-exported functions
produced one, so we can remove it.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This field was exported, but never mutated outside of the package, and
effectively a rather "creative" way to define a method on LocalRegistry.
While un-exporting also store these paths in a field, instead of constructing
them on every call, as the results won't change during the lifecycle of the
LocalRegistry.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Split the exported SpecsPaths from the platform-specific implementations,
so that documentation can be maintained in a single location.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Since 0e5eaf8ee3, these implementations
were fully identical, so removing the duplicate, and move it to a
platform-agnostic file.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- remove gotest.tools dependency as it was only used in one test,
and only for a trivial check
- use t.TempDir()
- rename vars that collided with package types
- don't use un-keyed structs
- explicitly ignore some errors to please linters
- use iotest.ErrReader
- TestReadCloserWrapperClose: verify reading works before closing :)
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 socket path as hostname, 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.
Before this patch, tests would fail on go1.20.6:
=== FAIL: pkg/authorization TestAuthZRequestPlugin (15.01s)
time="2023-07-12T12:53:45Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 1s"
time="2023-07-12T12:53:46Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 2s"
time="2023-07-12T12:53:48Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 4s"
time="2023-07-12T12:53:52Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 8s"
authz_unix_test.go:82: Failed to authorize request Post "http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq": http: invalid Host header
[1]: https://github.com/advisories/GHSA-f8f7-69v5-w4vx
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Some tests are testing timeouts and take a long time to run. Run the tests
in parallel, so that the test-suite takes shorter to run.
Before:
ok github.com/docker/docker/pkg/plugins 34.013s
After:
ok github.com/docker/docker/pkg/plugins 17.945s
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Refactor setupRemotePluginServer() to be a helper, and to spin up a test-
server for each test instead of sharing the same instance between tests.
This allows the tests to be run in parallel without stepping on each-other's
toes (tearing down the server).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This patch contains some optimizations I still had stashed when working
on eaa9494b71.
- Use the bytes package for handling the output of "lsof", instead of
converting to a string.
- Count the number of newlines in the output, instead of splitting the
output into a slice of strings. We're only interested in the number
of lines in the output.
- Use lsof's -F option to only print the file-descriptor for each line,
as we don't need other information.
- Use the -l, -n, and -P options to omit converting usernames, host names,
and port numbers.
From the [LSOF(8)][1] man-page:
-l This option inhibits the conversion of user ID numbers to
login names. It is also useful when login name lookup is
working improperly or slowly.
-n This option inhibits the conversion of network numbers to host
names for network files. Inhibiting conversion can make lsof run faster.
It is also useful when host name lookup is not working properly.
-P This option inhibits the conversion of port numbers to port names for network files.
Inhibiting the conversion can make lsof run a little faster.
It is also useful when host name lookup is not working properly.
Output looks something like;
lsof -lnP -Ff -p 39849
p39849
fcwd
ftxt
ftxt
f0
f1
f2
f3
f4
f5
f6
f7
f8
f9
f10
f11
Before/After:
BenchmarkGetTotalUsedFds-10 122 9479384 ns/op 10816 B/op 63 allocs/op
BenchmarkGetTotalUsedFds-10 154 7814697 ns/op 7257 B/op 60 allocs/op
[1]: https://opensource.apple.com/source/lsof/lsof-49/lsof/lsof.man.auto.html
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
TestClientWithRequestTimeout has been observed to flake in CI. The
timing in the test is quite tight, only giving the client a 10ms window
to time out, which could potentially be missed if the host is under
load and the goroutine scheduling is unlucky. Give the client a full
five seconds of grace to time out before failing the test.
Signed-off-by: Cory Snider <csnider@mirantis.com>
They're not used anywhere, so let's remove them; better to have
a compile error than a panic at runtime.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Linux 6.2 and up (commit [f1f1f2569901ec5b9d425f2e91c09a0e320768f3][1])
provides a fast path for the number of open files for the process.
From the [Linux docs][2]:
> The number of open files for the process is stored in 'size' member of
> `stat()` output for /proc/<pid>/fd for fast access.
[1]: f1f1f25699
[2]: https://docs.kernel.org/filesystems/proc.html#proc-pid-fd-list-of-symlinks-to-open-files
This patch adds a fast-path for Kernels that support this, and falls back
to the slow path if the Size fields is zero.
Comparing on a Fedora 38 (kernel 6.2.9-300.fc38.x86_64):
Before/After:
go test -bench ^BenchmarkGetTotalUsedFds$ -run ^$ ./pkg/fileutils/
BenchmarkGetTotalUsedFds 57264 18595 ns/op 408 B/op 10 allocs/op
BenchmarkGetTotalUsedFds 370392 3271 ns/op 40 B/op 3 allocs/op
Note that the slow path has 1 more file-descriptor, due to the open
file-handle for /proc/<pid>/fd during the calculation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use File.Readdirnames instead of os.ReadDir, as we're only interested in
the number of files, and results don't have to be sorted.
Before:
BenchmarkGetTotalUsedFds-5 149272 7896 ns/op 945 B/op 20 allocs/op
After:
BenchmarkGetTotalUsedFds-5 153517 7644 ns/op 408 B/op 10 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 8d56108ffb moved this function from
the generic (no build-tags) fileutils.go to a unix file, adding "freebsd"
to the build-tags.
This likely was a wrong assumption (as other files had freebsd build-tags).
FreeBSD's procfs does not mention `/proc/<pid>/fd` in the manpage, and
we don't test FreeBSD in CI, so let's drop it, and make this a Linux-only
file.
While updating also dropping the import-tag, as we're planning to move
this file internal to the daemon.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This utility was only used in tests, and internally, and no longer
used since we switch to using os.UserHomeDir() from stdlib.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was last used in the pkg/mflag package, which was removed
in 14712f9ff0, and is no longer used in
libnetwork code since e6de8aec2f
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This utility was only used in a single location (as part of `docker info`),
but the `pkg/rootless` package is imported in various locations, causing
rootlesskit to be a dependency for consumers of that package.
Move GetRootlessKitClient to the daemon code, which is the only location
it was used.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Audit the OCI spec options used for Linux containers to ensure they are
less order-dependent. Ensure they don't assume that any pointer fields
are non-nil and that they don't unintentionally clobber mutations to the
spec applied by other options.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Extended attributes are set on files in container images for a reason.
Fail to unpack if extended attributes are present in a layer and setting
the attributes on the unpacked files fails for any reason.
Add an option to the vfs graph driver to opt into the old behaviour
where ENOTSUPP and EPERM errors encountered when setting extended
attributes are ignored. Make it abundantly clear to users and anyone
triaging their bug reports that they are shooting themselves in the
foot by enabling this option.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Now that most uses of reexec have been replaced with non-reexec
solutions, most of the reexec.Init() calls peppered throughout the test
suites are unnecessary. Furthermore, most of the reexec.Init() calls in
test code neglects to check the return value to determine whether to
exit, which would result in the reexec'ed subprocesses proceeding to run
the tests, which would reexec another subprocess which would proceed to
run the tests, recursively. (That would explain why every reexec
callback used to unconditionally call os.Exit() instead of returning...)
Remove unneeded reexec.Init() calls from test and example code which no
longer needs it, and fix the reexec.Init() calls which are not inert to
exit after a reexec callback is invoked.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Our templates no longer contain version-specific rules, so this function
is no longer used. This patch deprecates it.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These were deprecated in 9d5e754caa, which
is part of the v24.0.0 release, so we can remove it from master.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was deprecated in 9f3e5eead5, which
is part of the v24.0.0 release, so we can remove it from master.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These were deprecated in 2d49080056, which
is part of the v24.0.0 release, so we can remove it from master.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was deprecated in c63ea32a17, which
is part of the v24.0.0 release, so we can remove it from master.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This const was deprecated in 5c78cbd3be, which
is part of the v24.0.0 release, so we can remove it from master.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This special case was added in 3043c26419 as
a sentinel error (`AuthRequiredError`) to check whether authentication
is required (and to prompt the users to authenticate). A later refactor
(946bbee39a) removed the `AuthRequiredError`,
but kept the error-message and logic.
Starting with fcee6056dc, it looks like we
no longer depend on this specific error, so we can return the registry's
error message instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The man page for sched_setaffinity(2) states the following about the pid
argument [1]:
> If pid is zero, then the mask of the calling thread is returned.
Thus the additional call to unix.Getpid can be omitted and pid = 0
passed to unix.SchedGetaffinity.
[1] https://man7.org/linux/man-pages/man2/sched_setaffinity.2.html#DESCRIPTION
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
This const looks to only be there for "convenience", or _possibly_ was created
with future normalization or special handling in mind.
In either case, currently it is just a direct copy (alias) for runtime.GOOS,
and defining our own type for this gives the impression that it's more than
that. It's only used in a single place, and there's no external consumers, so
let's deprecate this const, and use runtime.GOOS instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These consts are only used internally, and never returned to the user.
Un-export to make it clear these are not for external consumption.
While looking at the code, I also noticed that we may be using the wrong
Windows API to collect this information (and found an implementation elsewhere
that does use the correct API). I did not yet update the code, in cases there
are specific reasons.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was not immediately clear why we were not using runtime.GOARCH for
these (with a conversion to other formats, such as x86_64). These docs
are based on comments that were posted when implementing this package;
- https://github.com/moby/moby/pull/13921#issuecomment-130106474
- https://github.com/moby/moby/pull/13921#issuecomment-140270124
Some links were now redirecting to a new location, so updated them to
not depend on the redirect.
While at it, also updated a call to logrus to use structured formatting
(WithError()).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
commit 3c69b9f2c5 replaced these functions
and types with github.com/moby/patternmatcher. That commit has shipped with
docker 23.0, and BuildKit v0.11 no longer uses the old functions, so we can
remove these.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 6a516acb2e moved the MemInfo type and
ReadMemInfo() function into the pkg/sysinfo package. In an attempt to assist
consumers of these to migrate to the new location, an alias was added.
Unfortunately, the side effect of this alias is that pkg/system now depends
on pkg/sysinfo, which means that consumers of this (such as docker/cli) now
get all (indirect) dependencies of that package as dependency, which includes
many dependencies that should only be needed for the daemon / runtime;
- github.com/cilium/ebpf
- github.com/containerd/cgroups
- github.com/coreos/go-systemd/v22
- github.com/godbus/dbus/v5
- github.com/moby/sys/mountinfo
- github.com/opencontainers/runtime-spec
This patch moves the MemInfo related code to its own package. As the previous move
was not yet part of a release, we're not adding new aliases in pkg/sysinfo.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
"math/rand".Seed
- Migrate to using local RNG instances.
"archive/tar".TypeRegA
- The deprecated constant tar.TypeRegA is the same value as
tar.TypeReg and so is not needed at all.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Disables user.Lookup() and net.LookupHost() in the init() function on Windows.
Any package that simply imports pkg/chrootarchive will panic on Windows
Nano Server, due to missing netapi32.dll. While docker itself is not
meant to run on Nano Server, binaries that may import this package and
run on Nano server, will fail even if they don't really use any of the
functionality in this package while running on Nano.
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This is a squashed version of various PRs (or related code-changes)
to implement image inspect with the containerd-integration;
- add support for image inspect
- introduce GetImageOpts to manage image inspect data in backend
- GetImage to return image tags with details
- list images matching digest to discover all tags
- Add ExposedPorts and Volumes to the image returned
- Refactor resolving/getting images
- Return the image ID on inspect
- consider digest and ignore tag when both are set
- docker run --platform
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
We're looking for a specific prefix, so remove the prefix instead. Also remove
redundant error-wrapping, as `os.Open()` already provides details in the error
returned;
open /no/such/file: no such file or directory
open /etc/os-release: permission denied
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
