Merge pull request #1571 from thaJeztah/warn_on_create

Fix warnings not being printed on "create", only on "run"

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)
+}

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 }

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 {

cli/command/container/testdata/container-create-localhost-dns-ipv6.golden

@@ -0,0 +1 @@
+WARNING: Localhost DNS setting (--dns=::1) may fail in containers.

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.

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.

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.