Merge pull request #5906 from thaJeztah/remove_client_warnings

fix duplicate warnings on docker run / docker create, and slight refactor

Author: thaJeztah

cli/command/container/create.go

@@ -4,8 +4,8 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"net/netip"
 	"os"
-	"regexp"
 
 	"github.com/containerd/platforms"
 	"github.com/distribution/reference"
@@ -207,9 +207,6 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c
 	hostConfig := containerCfg.HostConfig
 	networkingConfig := containerCfg.NetworkingConfig
 
-	warnOnOomKillDisable(*hostConfig, dockerCli.Err())
-	warnOnLocalhostDNS(*hostConfig, dockerCli.Err())
-
 	var (
 		trustedRef reference.Canonical
 		namedRef   reference.Named
@@ -292,40 +289,27 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c
 		}
 	}
 
+	if warn := localhostDNSWarning(*hostConfig); warn != "" {
+		response.Warnings = append(response.Warnings, warn)
+	}
 	for _, w := range response.Warnings {
 		_, _ = fmt.Fprintln(dockerCli.Err(), "WARNING:", w)
 	}
 	err = containerIDFile.Write(response.ID)
 	return response.ID, 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) {
+// they are trying to set a DNS to a localhost address.
+//
+// TODO(thaJeztah): move this to the daemon, which can make a better call if it will work or not (depending on networking mode).
+func localhostDNSWarning(hostConfig container.HostConfig) string {
 	for _, dnsIP := range hostConfig.DNS {
-		if isLocalhost(dnsIP) {
-			_, _ = fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP)
-			return
+		if addr, err := netip.ParseAddr(dnsIP); err == nil && addr.IsLoopback() {
+			return fmt.Sprintf("Localhost DNS (%s) may fail in containers.", addr)
 		}
 	}
-}
-
-// 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)
+	return ""
 }
 
 func validatePullOpt(val string) error {

cli/command/container/create_test.go

@@ -270,31 +270,24 @@ func TestNewCreateCommandWithContentTrustErrors(t *testing.T) {
 
 func TestNewCreateCommandWithWarnings(t *testing.T) {
 	testCases := []struct {
-		name    string
-		args    []string
-		warning bool
+		name     string
+		args     []string
+		warnings []string
+		warning  bool
 	}{
 		{
-			name: "container-create-without-oom-kill-disable",
+			name: "container-create-no-warnings",
 			args: []string{"image:tag"},
 		},
 		{
-			name: "container-create-oom-kill-disable-false",
-			args: []string{"--oom-kill-disable=false", "image:tag"},
+			name:     "container-create-daemon-single-warning",
+			args:     []string{"image:tag"},
+			warnings: []string{"warning from daemon"},
 		},
 		{
-			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-daemon-multiple-warnings",
+			args:     []string{"image:tag"},
+			warnings: []string{"warning from daemon", "another warning from daemon"},
 		},
 		{
 			name:    "container-create-localhost-dns",
@@ -316,15 +309,15 @@ func TestNewCreateCommandWithWarnings(t *testing.T) {
 					platform *specs.Platform,
 					containerName string,
 				) (container.CreateResponse, error) {
-					return container.CreateResponse{}, nil
+					return container.CreateResponse{Warnings: tc.warnings}, nil
 				},
 			})
 			cmd := NewCreateCommand(fakeCLI)
 			cmd.SetOut(io.Discard)
 			cmd.SetArgs(tc.args)
 			err := cmd.Execute()
 			assert.NilError(t, err)
-			if tc.warning {
+			if tc.warning || len(tc.warnings) > 0 {
 				golden.Assert(t, fakeCLI.ErrBuffer().String(), tc.name+".golden")
 			} else {
 				assert.Equal(t, fakeCLI.ErrBuffer().String(), "")

cli/command/container/testdata/container-create-daemon-multiple-warnings.golden

@@ -0,0 +1,2 @@
+WARNING: warning from daemon
+WARNING: another warning from daemon

cli/command/container/testdata/container-create-daemon-single-warning.golden

@@ -0,0 +1 @@
+WARNING: warning from daemon

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

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

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

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