From 7c514a31c97cc0a4e74b53f5cff73da72e680a01 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 13 Dec 2018 12:59:35 +0100 Subject: [PATCH] Fix warnings not being printed on "create", only on "run" Previously, these errors were only printed when using `docker run`, but were omitted when using `docker container create` and `docker container start` separately. Given that these warnings apply to both situations, this patch moves generation of these warnings to `docker container create` (which is also called by `docker run`) Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create.go | 33 ++++++++++ cli/command/container/create_test.go | 65 +++++++++++++++++++ cli/command/container/run.go | 33 ---------- ...container-create-localhost-dns-ipv6.golden | 1 + .../container-create-localhost-dns.golden | 1 + ...-oom-kill-true-without-memory-limit.golden | 1 + ...reate-oom-kill-without-memory-limit.golden | 1 + 7 files changed, 102 insertions(+), 33 deletions(-) create mode 100644 cli/command/container/testdata/container-create-localhost-dns-ipv6.golden create mode 100644 cli/command/container/testdata/container-create-localhost-dns.golden create mode 100644 cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden create mode 100644 cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 62d1a088aa..a055121b27 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "regexp" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" @@ -165,6 +166,9 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig networkingConfig := containerConfig.NetworkingConfig stderr := dockerCli.Err() + warnOnOomKillDisable(*hostConfig, stderr) + warnOnLocalhostDNS(*hostConfig, stderr) + var ( trustedRef reference.Canonical namedRef reference.Named @@ -227,3 +231,32 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig err = containerIDFile.Write(response.ID) return &response, err } + +func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) { + if hostConfig.OomKillDisable != nil && *hostConfig.OomKillDisable && hostConfig.Memory == 0 { + fmt.Fprintln(stderr, "WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.") + } +} + +// check the DNS settings passed via --dns against localhost regexp to warn if +// they are trying to set a DNS to a localhost address +func warnOnLocalhostDNS(hostConfig container.HostConfig, stderr io.Writer) { + for _, dnsIP := range hostConfig.DNS { + if isLocalhost(dnsIP) { + fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP) + return + } + } +} + +// IPLocalhost is a regex pattern for IPv4 or IPv6 loopback range. +const ipLocalhost = `((127\.([0-9]{1,3}\.){2}[0-9]{1,3})|(::1)$)` + +var localhostIPRegexp = regexp.MustCompile(ipLocalhost) + +// IsLocalhost returns true if ip matches the localhost IP regular expression. +// Used for determining if nameserver settings are being passed which are +// localhost addresses +func isLocalhost(ip string) bool { + return localhostIPRegexp.MatchString(ip) +} diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index 1df09c7d6f..2e3bdd529d 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -20,6 +20,7 @@ import ( "gotest.tools/assert" is "gotest.tools/assert/cmp" "gotest.tools/fs" + "gotest.tools/golden" ) func TestCIDFileNoOPWithNoFilename(t *testing.T) { @@ -166,6 +167,70 @@ func TestNewCreateCommandWithContentTrustErrors(t *testing.T) { } } +func TestNewCreateCommandWithWarnings(t *testing.T) { + testCases := []struct { + name string + args []string + warning bool + }{ + { + name: "container-create-without-oom-kill-disable", + args: []string{"image:tag"}, + }, + { + name: "container-create-oom-kill-disable-false", + args: []string{"--oom-kill-disable=false", "image:tag"}, + }, + { + name: "container-create-oom-kill-without-memory-limit", + args: []string{"--oom-kill-disable", "image:tag"}, + warning: true, + }, + { + name: "container-create-oom-kill-true-without-memory-limit", + args: []string{"--oom-kill-disable=true", "image:tag"}, + warning: true, + }, + { + name: "container-create-oom-kill-true-with-memory-limit", + args: []string{"--oom-kill-disable=true", "--memory=100M", "image:tag"}, + }, + { + name: "container-create-localhost-dns", + args: []string{"--dns=127.0.0.11", "image:tag"}, + warning: true, + }, + { + name: "container-create-localhost-dns-ipv6", + args: []string{"--dns=::1", "image:tag"}, + warning: true, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cli := test.NewFakeCli(&fakeClient{ + createContainerFunc: func(config *container.Config, + hostConfig *container.HostConfig, + networkingConfig *network.NetworkingConfig, + containerName string, + ) (container.ContainerCreateCreatedBody, error) { + return container.ContainerCreateCreatedBody{}, nil + }, + }) + cmd := NewCreateCommand(cli) + cmd.SetOutput(ioutil.Discard) + cmd.SetArgs(tc.args) + err := cmd.Execute() + assert.NilError(t, err) + if tc.warning { + golden.Assert(t, cli.ErrBuffer().String(), tc.name+".golden") + } else { + assert.Equal(t, cli.ErrBuffer().String(), "") + } + }) + } +} + type fakeNotFound struct{} func (f fakeNotFound) NotFound() bool { return true } diff --git a/cli/command/container/run.go b/cli/command/container/run.go index d2ca58739f..7013a7167e 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -6,7 +6,6 @@ import ( "io" "net/http/httputil" "os" - "regexp" "runtime" "strings" "syscall" @@ -68,35 +67,6 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command { return cmd } -func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) { - if hostConfig.OomKillDisable != nil && *hostConfig.OomKillDisable && hostConfig.Memory == 0 { - fmt.Fprintln(stderr, "WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.") - } -} - -// check the DNS settings passed via --dns against localhost regexp to warn if -// they are trying to set a DNS to a localhost address -func warnOnLocalhostDNS(hostConfig container.HostConfig, stderr io.Writer) { - for _, dnsIP := range hostConfig.DNS { - if isLocalhost(dnsIP) { - fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP) - return - } - } -} - -// IPLocalhost is a regex pattern for IPv4 or IPv6 loopback range. -const ipLocalhost = `((127\.([0-9]{1,3}\.){2}[0-9]{1,3})|(::1)$)` - -var localhostIPRegexp = regexp.MustCompile(ipLocalhost) - -// IsLocalhost returns true if ip matches the localhost IP regular expression. -// Used for determining if nameserver settings are being passed which are -// localhost addresses -func isLocalhost(ip string) bool { - return localhostIPRegexp.MatchString(ip) -} - func runRun(dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error { proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), copts.env.GetAll()) newEnv := []string{} @@ -124,9 +94,6 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio stdout, stderr := dockerCli.Out(), dockerCli.Err() client := dockerCli.Client() - warnOnOomKillDisable(*hostConfig, stderr) - warnOnLocalhostDNS(*hostConfig, stderr) - config.ArgsEscaped = false if !opts.detach { diff --git a/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden b/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden new file mode 100644 index 0000000000..5c98b97716 --- /dev/null +++ b/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden @@ -0,0 +1 @@ +WARNING: Localhost DNS setting (--dns=::1) may fail in containers. diff --git a/cli/command/container/testdata/container-create-localhost-dns.golden b/cli/command/container/testdata/container-create-localhost-dns.golden new file mode 100644 index 0000000000..1c8b0e1f7f --- /dev/null +++ b/cli/command/container/testdata/container-create-localhost-dns.golden @@ -0,0 +1 @@ +WARNING: Localhost DNS setting (--dns=127.0.0.11) may fail in containers. diff --git a/cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden b/cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden new file mode 100644 index 0000000000..5fb6aeb429 --- /dev/null +++ b/cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden @@ -0,0 +1 @@ +WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous. diff --git a/cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden b/cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden new file mode 100644 index 0000000000..5fb6aeb429 --- /dev/null +++ b/cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden @@ -0,0 +1 @@ +WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.