API/CLI discrepancy on hostname validation (#21595).

This fix tries to fix the discrepancy between API and CLI on hostname
validation. Previously, the hostname validation was handled at the
CLI interface in runconfig/opts/parse.go and return an error if the
hostname is invalid. However, if an end user use the remote API to
pass the hostname, the error will not be returned immediately.
Instead the error will only be thrown out when the container creation
fails. This creates behavior discrepancy between API and CLI.

In this fix, the hostname validation was moved to
verifyContainerSettings so the behavior will be the same for API and
CLI.

After the change, since CLI does not handle the hostname validation
any more, the previous unit tests about hostname validation on CLI
in runconfig/opts/parse_test.go has to be updated as well because
there is no validation at this stage. All those unit tests are moved
to integration test TestRunTooLongHostname so that the hostname
validation is still properly covered as before.

Note: Since the hostname validation moved to API, the error message
changes from `invalid hostname format for --hostname:` to
`invalid hostname format:` as well because `--hostname` is passed
to CLI only.

This fix fixes #21595.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

daemon/daemon.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 	"sync"
@@ -1338,6 +1339,18 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostCon
 				return nil, err
 			}
 		}
+
+		// Validate if the given hostname is RFC 1123 (https://tools.ietf.org/html/rfc1123) compliant.
+		if len(config.Hostname) > 0 {
+			// RFC1123 specifies that 63 bytes is the maximium length
+			// Windows has the limitation of 63 bytes in length
+			// Linux hostname is limited to HOST_NAME_MAX=64, not not including the terminating null byte.
+			// We limit the length to 63 bytes here to match RFC1035 and RFC1123.
+			matched, _ := regexp.MatchString("^(([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])\\.)*([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])$", config.Hostname)
+			if len(config.Hostname) > 63 || !matched {
+				return nil, fmt.Errorf("invalid hostname format: %s", config.Hostname)
+			}
+		}
 	}
 
 	if hostConfig == nil {

integration-cli/docker_cli_run_test.go

@@ -4295,15 +4295,33 @@ func (s *DockerSuite) TestRunTooLongHostname(c *check.C) {
 	hostname1 := "this-is-a-way-too-long-hostname-but-it-should-give-a-nice-error.local"
 	out, _, err := dockerCmdWithError("run", "--hostname", hostname1, "busybox", "echo", "test")
 	c.Assert(err, checker.NotNil, check.Commentf("Expected docker run to fail!"))
-	c.Assert(out, checker.Contains, "invalid hostname format for --hostname:", check.Commentf("Expected to have 'invalid hostname format for --hostname:' in the output, get: %s!", out))
+	c.Assert(out, checker.Contains, "invalid hostname format:", check.Commentf("Expected to have 'invalid hostname format:' in the output, get: %s!", out))
 
-	// HOST_NAME_MAX=64 so 65 bytes will fail
-	hostname2 := "this-is-a-hostname-with-65-bytes-so-it-should-give-an-error.local"
-	out, _, err = dockerCmdWithError("run", "--hostname", hostname2, "busybox", "echo", "test")
-	c.Assert(err, checker.NotNil, check.Commentf("Expected docker run to fail!"))
-	c.Assert(out, checker.Contains, "invalid hostname format for --hostname:", check.Commentf("Expected to have 'invalid hostname format for --hostname:' in the output, get: %s!", out))
+	// Addtional test cases
+	validHostnames := map[string]string{
+		"hostname":    "hostname",
+		"host-name":   "host-name",
+		"hostname123": "hostname123",
+		"123hostname": "123hostname",
+		"hostname-of-63-bytes-long-should-be-valid-and-without-any-error": "hostname-of-63-bytes-long-should-be-valid-and-without-any-error",
+	}
+	for hostname := range validHostnames {
+		dockerCmd(c, "run", "--hostname", hostname, "busybox", "echo", "test")
+	}
 
-	// 64 bytes will be OK
-	hostname3 := "this-is-a-hostname-with-64-bytes-so-will-not-give-an-error.local"
-	dockerCmd(c, "run", "--hostname", hostname3, "busybox", "echo", "test")
+	invalidHostnames := map[string]string{
+		"^hostname": "invalid hostname format: ^hostname",
+		"hostname%": "invalid hostname format: hostname%",
+		"host&name": "invalid hostname format: host&name",
+		"-hostname": "invalid hostname format: -hostname",
+		"host_name": "invalid hostname format: host_name",
+		"hostname-of-64-bytes-long-should-be-invalid-and-be-with-an-error": "invalid hostname format: hostname-of-64-bytes-long-should-be-invalid-and-be-with-an-error",
+	}
+
+	for hostname, expectedError := range invalidHostnames {
+		out, _, err = dockerCmdWithError("run", "--hostname", hostname, "busybox", "echo", "test")
+		c.Assert(err, checker.NotNil, check.Commentf("Expected docker run to fail!"))
+		c.Assert(out, checker.Contains, expectedError, check.Commentf("Expected to have '%s' in the output, get: %s!", expectedError, out))
+
+	}
 }

runconfig/opts/parse.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"path"
-	"regexp"
 	"strconv"
 	"strings"
 
@@ -243,15 +242,6 @@ func Parse(cmd *flag.FlagSet, args []string) (*container.Config, *container.Host
 	if *flEntrypoint != "" {
 		entrypoint = strslice.StrSlice{*flEntrypoint}
 	}
-	// Validate if the given hostname is RFC 1123 (https://tools.ietf.org/html/rfc1123) compliant.
-	hostname := *flHostname
-	if hostname != "" {
-		// Linux hostname is limited to HOST_NAME_MAX=64, not including the terminating null byte.
-		matched, _ := regexp.MatchString("^(([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])\\.)*([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])$", hostname)
-		if len(hostname) > 64 || !matched {
-			return nil, nil, nil, cmd, fmt.Errorf("invalid hostname format for --hostname: %s", hostname)
-		}
-	}
 
 	ports, portBindings, err := nat.ParsePortSpecs(flPublish.GetAll())
 	if err != nil {

runconfig/opts/parse_test.go

@@ -390,15 +390,7 @@ func TestParseHostname(t *testing.T) {
 		"host-name":   "host-name",
 		"hostname123": "hostname123",
 		"123hostname": "123hostname",
-		"hostname-of-64-bytes-long-should-be-valid-and-without-any-errors": "hostname-of-64-bytes-long-should-be-valid-and-without-any-errors",
-	}
-	invalidHostnames := map[string]string{
-		"^hostname": "invalid hostname format for --hostname: ^hostname",
-		"hostname%": "invalid hostname format for --hostname: hostname%",
-		"host&name": "invalid hostname format for --hostname: host&name",
-		"-hostname": "invalid hostname format for --hostname: -hostname",
-		"host_name": "invalid hostname format for --hostname: host_name",
-		"hostname-of-65-bytes-long-should-be-invalid-and-be-given-an-error": "invalid hostname format for --hostname: hostname-of-65-bytes-long-should-be-invalid-and-be-given-an-error",
+		"hostname-of-63-bytes-long-should-be-valid-and-without-any-error": "hostname-of-63-bytes-long-should-be-valid-and-without-any-error",
 	}
 	hostnameWithDomain := "--hostname=hostname.domainname"
 	hostnameWithDomainTld := "--hostname=hostname.domainname.tld"
@@ -407,11 +399,6 @@ func TestParseHostname(t *testing.T) {
 			t.Fatalf("Expected the config to have 'hostname' as hostname, got '%v'", config.Hostname)
 		}
 	}
-	for hostname, expectedError := range invalidHostnames {
-		if _, _, err := parse(t, fmt.Sprintf("--hostname=%s", hostname)); err == nil || err.Error() != expectedError {
-			t.Fatalf("Expected error '%v' with '--hostname=%s', got '%s'", expectedError, hostname, err)
-		}
-	}
 	if config, _ := mustParse(t, hostnameWithDomain); config.Hostname != "hostname.domainname" && config.Domainname != "" {
 		t.Fatalf("Expected the config to have 'hostname' as hostname.domainname, got '%v'", config.Hostname)
 	}