After discussing in the maintainers meeting, we concluded that Slowloris attacks
are not a real risk other than potentially having some additional goroutines
lingering around, so setting a long timeout to satisfy the linter, and to at
least have "some" timeout.
libnetwork/diagnostic/server.go:96:10: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)
srv := &http.Server{
Addr: net.JoinHostPort(ip, strconv.Itoa(port)),
Handler: s,
}
api/server/server.go:60:10: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)
srv: &http.Server{
Addr: addr,
},
daemon/metrics_unix.go:34:13: G114: Use of net/http serve function that has no support for setting timeouts (gosec)
if err := http.Serve(l, mux); err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
^
cmd/dockerd/metrics.go:27:13: G114: Use of net/http serve function that has no support for setting timeouts (gosec)
if err := http.Serve(l, mux); err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `-g` / `--graph` options were soft deprecated in favor of `--data-root` in
261ef1fa27 (v17.05.0) and at the time considered
to not be removed. However, with the move towards containerd snapshotters, having
these options around adds additional complexity to handle fallbacks for deprecated
(and hidden) flags, so completing the deprecation.
With this patch:
dockerd --graph=/var/lib/docker --validate
Flag --graph has been deprecated, Use --data-root instead
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: the "graph" config file option is deprecated; use "data-root" instead
mkdir -p /etc/docker
echo '{"graph":"/var/lib/docker"}' > /etc/docker/daemon.json
dockerd --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: the "graph" config file option is deprecated; use "data-root" instead
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Makes sure that tests use a config struct that's more representative
to how it's used in the code.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This centralizes more defaults, to be part of the config struct that's
created, instead of interweaving the defaults with other code in various
places.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- return early if we're expecting a system-containerd
- rename `initContainerD` to `initContainerd` ':)
- remove .Config to reduce verbosity
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Change the log-level for messages about starting the managed containerd instance
to be the same as for the main API. And remove a redundant debug-log.
With this patch:
dockerd
INFO[2022-08-11T11:46:32.573299176Z] Starting up
INFO[2022-08-11T11:46:32.574304409Z] containerd not running, starting managed containerd
INFO[2022-08-11T11:46:32.575289181Z] started new containerd process address=/var/run/docker/containerd/containerd.sock module=libcontainerd pid=5370
cmd/dockerd: initContainerD(): clean-up some logs
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
the `--log-level` flag overrides whatever is in the containerd configuration file;
f033f6ff85/cmd/containerd/command/main.go (L339-L352)
Given that we set that flag when we start the containerd binary, there is no need
to write it both to the generated config-file and pass it as flag.
This patch also slightly changes the behavior; as both dockerd and containerd use
"info" as default log-level, don't set the log-level if it's the default.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Containerd, like dockerd has a OOMScore configuration option to adjust its own
OOM score. In dockerd, this option was added when default installations were not
yet running the daemon as a systemd unit, which made it more complicated to set
the score, and adding a daemon option was convenient.
A binary adjusting its own score has been frowned upon, as it's more logical to
make that the responsibility of the process manager _starting_ the daemon, which
is what we did for dockerd in 21578530d7.
There have been discussions on deprecating the daemon flag for dockerd, and
similar discussions have been happening for containerd.
This patch changes how we set the OOM score for the containerd child process,
and to have dockerd (supervisor) set the OOM score, as it's acting as process
manager in this case (performing a role similar to systemd otherwise).
With this patch, the score is still adjusted as usual, but not written to the
containerd configuration file;
dockerd --oom-score-adjust=-123
cat /proc/$(pidof containerd)/oom_score_adj
-123
As a follow-up, we may consider to adjust the containerd OOM score based on the
daemon's own score instead of on the `cli.OOMScoreAdjust` configuration so that
we will also adjust the score in situations where dockerd's OOM score was set
through other ways (systemd or manually adjusting the cgroup). A TODO was added
for this.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- properly use filepath.Join() for Windows paths
- use getDaemonConfDir() to get the path for storing daemon.json
- return error instead of logrus.Fatal(). The logrus.Fatal() was a left-over
from when Cobra was not used, and appears to have been missed in commit
fb83394714, which did the conversion to Cobra.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The existing implementation used a `nil` value for the CRI plugin's configuration
to indicate that the plugin had to be disabled. Effectively, the `Plugins` value
was only used as an intermediate step, only to be removed later on, and to instead
add the given plugin to `DisabledPlugins` in the containerd configuration.
This patch removes the intermediate step; as a result we also don't need to mask
the containerd `Plugins` field, which was added to allow serializing the toml.
A code comment was added as well to explain why we're (currently) disabling the
CRI plugin by default, which may help future visitors of the code to determin
if that default is still needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Initial pull/ls works
Build is deactivated if the feature is active
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
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>
Set the defaults when constructing the config, instead of setting them
indirectly through the command-line flags.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function depends on flags having been parsed before it's used;
add a safety-net in case this function would be called before that.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This prevents creating a socket and touching the filesystem before
trying to use a port that was already in use by a container.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The LoopkupImage method is only used by the inspect image route and
returns an api/type struct. The depenency to api/types of the
daemon/images package is wrong, the daemon doesn't need to know about
the api types.
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Use the default (0) value to indicate "not set", which simplifies
working with these configuration options, preventing the need to
use intermediate variables etc.
While changing this code, also making some small cleanups, such
as replacing "fmt.Sprintf()" for "strconv" variants.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
spf13/pflag now provides this out of the box, so no need to implement
and use our own value-type for this.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This file was originally part of the work to support Solaris, and
there's nothing "not common unix" anymmore, so merging the files.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
installConfigFlags already has separate implementations for Linux and
Windows, so no need to further differentiate.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The installCommonConfigFlags() function is meant for flags that are
supported by all platforms, so removing it from that function.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Previously, the API server configuration would be initialized and
validated when starting the API. Because of this, invalid configuration
(e.g. missing or invalid TLS certificates) would not be detected
when using `dockerd --validate`.
This patch moves creation of the validation earlier, so that it's
validated as part of `dockerd --validate`.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Previously, hosts were de-duplicated and normalized when starting
the API server (in `loadListeners()`), which meant that errors could
occur in that step (but not detected when using `dockerd --validate`),
as well as the list of hosts in the config not matching what would
actually be used (i.e., if duplicates were present).
This patch extracts the de-duplicating to a separate function, and
executes it as part of loading the daemon configuration, so that we
can fail early.
Moving this code also showed that some of this validation depended
on `newAPIServerConfig()` modifying the configuration (adding an
empty host if none was set) in order to have the parsing set a
default. This code was moved elsewhere, but a TODO comment added
as this logic is somewhat sketchy.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- un-export `daemonOptions.InstallFlags()`; `daemonOptions` itself isn't exported,
not exported, and `InstallFlags()` isn't matching any interface and only used
internally.
- un-export `daemonOptions.SetDefaultOptions()` and remove the `flags` argument
as we were passing `daemonOptions.flags` as argument on a method attached to
`daemonOptions`, which was somewhat backwards. While at it, also removing an
intermediate variable that wasn't needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Log-level validation was previously performed when configuring the daemon-logs;
this moves the validation to config.Validate() so that we can catch invalid
settings when running dockerd --validate.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was introduced in 85572cac14, where I
probably forgot to remove this code from an earlier iteration (I decided
that having an explicit `configureCertsDir()` function call for this would
make it more transparent that we're re-configuring a default).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The Logging boolean was unconditionally set to true and ignored in all locations,
except for enabling the debugging middleware, which was also gated by the active
logrus logging level.
While it could make sense to have a Loglevel option configured on the API server,
we don't have this currently, and to make that actually useful, that config would
need to be tollerated by all locations that produce logs (which isn't the case
either).
Looking at the history of this option; a boolean to disable logging was originally
added in commit c423a790d6, which hard-coded it to
"disabled" in a test, and "enabled" for the API server outside of tests (before
that commit, logging was always enabled).
02ddaad5d9 and 5c42b2b512
changed the hard-coded values to be configurable through a `Logging` env-var (env-
vars were used _internally_ at the time to pass on options), which later became
a configuration struct in a0bf80fe03.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
