Merge pull request #43524 from thaJeztah/daemon_fix_hosts_validation_step2

opts: ParseTCPAddr(): extract parsing logic, consistent errors

daemon/cluster/cluster.go

@@ -68,7 +68,7 @@ const (
 	swarmConnectTimeout            = 20 * time.Second
 	swarmRequestTimeout            = 20 * time.Second
 	stateFile                      = "docker-state.json"
-	defaultAddr                    = "0.0.0.0:2377"
+	defaultAddr                    = "tcp://0.0.0.0:2377"
 	isWindows                      = runtime.GOOS == "windows"
 	initialReconnectDelay          = 100 * time.Millisecond
 	maxReconnectDelay              = 30 * time.Second

daemon/cluster/listen_addr.go

@@ -20,7 +20,7 @@ const (
 func resolveListenAddr(specifiedAddr string) (string, string, error) {
 	specifiedHost, specifiedPort, err := net.SplitHostPort(specifiedAddr)
 	if err != nil {
-		return "", "", fmt.Errorf("could not parse listen address %s", specifiedAddr)
+		return "", "", configError("could not parse listen address " + specifiedAddr)
 	}
 	// Does the host component match any of the interface names on the
 	// system? If so, use the address from that interface.

daemon/cluster/swarm.go

@@ -52,7 +52,7 @@ func (c *Cluster) Init(req types.InitRequest) (string, error) {
 
 	listenHost, listenPort, err := resolveListenAddr(req.ListenAddr)
 	if err != nil {
-		return "", err
+		return "", errdefs.InvalidParameter(err)
 	}
 
 	advertiseHost, advertisePort, err := c.resolveAdvertiseAddr(req.AdvertiseAddr, listenPort)
@@ -549,6 +549,7 @@ func validateAddr(addr string) (string, error) {
 	}
 	newaddr, err := opts.ParseTCPAddr(addr, defaultAddr)
 	if err != nil {
+		// TODO(thaJeztah) why are we ignoring the error here? Is this to allow "non-tcp" addresses?
 		return addr, nil
 	}
 	return strings.TrimPrefix(newaddr, "tcp://"), nil

opts/hosts.go

@@ -1,7 +1,6 @@
 package opts // import "github.com/docker/docker/opts"
 
 import (
-	"fmt"
 	"net"
 	"net/url"
 	"path/filepath"
@@ -77,7 +76,7 @@ func ParseHost(defaultToTLS, defaultToUnixXDG bool, val string) (string, error)
 }
 
 // parseDaemonHost parses the specified address and returns an address that will be used as the host.
-// Depending of the address specified, this may return one of the global Default* strings defined in hosts.go.
+// Depending on the address specified, this may return one of the global Default* strings defined in hosts.go.
 func parseDaemonHost(addr string) (string, error) {
 	addrParts := strings.SplitN(addr, "://", 2)
 	if len(addrParts) == 1 && addrParts[0] != "" {
@@ -86,7 +85,7 @@ func parseDaemonHost(addr string) (string, error) {
 
 	switch addrParts[0] {
 	case "tcp":
-		return ParseTCPAddr(addrParts[1], DefaultTCPHost)
+		return ParseTCPAddr(addr, DefaultTCPHost)
 	case "unix":
 		return parseSimpleProtoAddr("unix", addrParts[1], DefaultUnixSocket)
 	case "npipe":
@@ -94,7 +93,7 @@ func parseDaemonHost(addr string) (string, error) {
 	case "fd":
 		return addr, nil
 	default:
-		return "", fmt.Errorf("Invalid bind address format: %s", addr)
+		return "", errors.Errorf("invalid bind address (%s): unsupported proto '%s'", addr, addrParts[0])
 	}
 }
 
@@ -105,12 +104,12 @@ func parseDaemonHost(addr string) (string, error) {
 func parseSimpleProtoAddr(proto, addr, defaultAddr string) (string, error) {
 	addr = strings.TrimPrefix(addr, proto+"://")
 	if strings.Contains(addr, "://") {
-		return "", fmt.Errorf("Invalid proto, expected %s: %s", proto, addr)
+		return "", errors.Errorf("invalid proto, expected %s: %s", proto, addr)
 	}
 	if addr == "" {
 		addr = defaultAddr
 	}
-	return fmt.Sprintf("%s://%s", proto, addr), nil
+	return proto + "://" + addr, nil
 }
 
 // ParseTCPAddr parses and validates that the specified address is a valid TCP
@@ -119,48 +118,54 @@ func parseSimpleProtoAddr(proto, addr, defaultAddr string) (string, error) {
 // tryAddr is expected to have already been Trim()'d
 // defaultAddr must be in the full `tcp://host:port` form
 func ParseTCPAddr(tryAddr string, defaultAddr string) (string, error) {
-	if tryAddr == "" || tryAddr == "tcp://" {
-		return defaultAddr, nil
-	}
-	addr := strings.TrimPrefix(tryAddr, "tcp://")
-	if strings.Contains(addr, "://") || addr == "" {
-		return "", fmt.Errorf("Invalid proto, expected tcp: %s", tryAddr)
+	def, err := parseTCPAddr(defaultAddr, true)
+	if err != nil {
+		return "", errors.Wrapf(err, "invalid default address (%s)", defaultAddr)
 	}
 
-	defaultAddr = strings.TrimPrefix(defaultAddr, "tcp://")
-	defaultHost, defaultPort, err := net.SplitHostPort(defaultAddr)
+	addr, err := parseTCPAddr(tryAddr, false)
 	if err != nil {
-		return "", err
+		return "", errors.Wrapf(err, "invalid bind address (%s)", tryAddr)
 	}
-	u, err := url.Parse("tcp://" + addr)
-	if err != nil {
-		return "", err
+
+	host := addr.Hostname()
+	if host == "" {
+		host = def.Hostname()
 	}
-	host, port, err := net.SplitHostPort(u.Host)
-	if err != nil {
-		// try port addition once
-		host, port, err = net.SplitHostPort(net.JoinHostPort(u.Host, defaultPort))
+	port := addr.Port()
+	if port == "" {
+		port = def.Port()
 	}
+
+	return "tcp://" + net.JoinHostPort(host, port), nil
+}
+
+// parseTCPAddr parses the given addr and validates if it is in the expected
+// format. If strict is enabled, the address must contain a scheme (tcp://),
+// a host (or IP-address) and a port number.
+func parseTCPAddr(address string, strict bool) (*url.URL, error) {
+	if !strict && !strings.Contains(address, "://") {
+		address = "tcp://" + address
+	}
+	parsedURL, err := url.Parse(address)
 	if err != nil {
-		return "", fmt.Errorf("Invalid bind address format: %s", tryAddr)
+		return nil, err
 	}
-
-	if host == "" {
-		host = defaultHost
+	if parsedURL.Scheme != "tcp" {
+		return nil, errors.Errorf("unsupported proto '%s'", parsedURL.Scheme)
 	}
-	if port == "" {
-		port = defaultPort
+	if parsedURL.Path != "" {
+		return nil, errors.New("should not contain a path element")
 	}
-	p, err := strconv.Atoi(port)
-	if err != nil && p == 0 {
-		return "", fmt.Errorf("Invalid bind address format: %s", tryAddr)
+	if strict && parsedURL.Host == "" {
+		return nil, errors.New("no host or IP address")
 	}
-
-	if u.Path != "" {
-		return "", errors.Errorf("invalid bind address (%s): should not contain a path element", tryAddr)
+	if parsedURL.Port() != "" || strict {
+		if p, err := strconv.Atoi(parsedURL.Port()); err != nil || p == 0 {
+			return nil, errors.Errorf("invalid port: %s", parsedURL.Port())
+		}
 	}
-
-	return "tcp://" + net.JoinHostPort(host, port), nil
+	return parsedURL, nil
 }
 
 // ValidateExtraHost validates that the specified string is a valid extrahost and returns it.
@@ -169,12 +174,12 @@ func ValidateExtraHost(val string) (string, error) {
 	// allow for IPv6 addresses in extra hosts by only splitting on first ":"
 	arr := strings.SplitN(val, ":", 2)
 	if len(arr) != 2 || len(arr[0]) == 0 {
-		return "", fmt.Errorf("bad format for add-host: %q", val)
+		return "", errors.Errorf("bad format for add-host: %q", val)
 	}
 	// Skip IPaddr validation for special "host-gateway" string
 	if arr[1] != HostGatewayName {
 		if _, err := ValidateIPAddress(arr[1]); err != nil {
-			return "", fmt.Errorf("invalid IP address in add-host: %q", arr[1])
+			return "", errors.Errorf("invalid IP address in add-host: %q", arr[1])
 		}
 	}
 	return val, nil

opts/hosts_test.go

@@ -8,19 +8,19 @@ import (
 
 func TestParseHost(t *testing.T) {
 	invalid := map[string]string{
-		"something with spaces": `parse "tcp://something with spaces": invalid character " " in host name`,
-		"://":                   `Invalid bind address format: ://`,
-		"unknown://":            `Invalid bind address format: unknown://`,
-		"tcp://:port":           `parse "tcp://:port": invalid port ":port" after host`,
-		"tcp://invalid:port":    `parse "tcp://invalid:port": invalid port ":port" after host`,
-		"tcp://:5555/":          `invalid bind address (:5555/): should not contain a path element`,
-		"tcp://:5555/p":         `invalid bind address (:5555/p): should not contain a path element`,
-		"tcp://0.0.0.0:5555/":   `invalid bind address (0.0.0.0:5555/): should not contain a path element`,
-		"tcp://0.0.0.0:5555/p":  `invalid bind address (0.0.0.0:5555/p): should not contain a path element`,
-		"tcp://[::1]:/":         `invalid bind address ([::1]:/): should not contain a path element`,
-		"tcp://[::1]:5555/":     `invalid bind address ([::1]:5555/): should not contain a path element`,
-		"tcp://[::1]:5555/p":    `invalid bind address ([::1]:5555/p): should not contain a path element`,
-		" tcp://:5555/path ":    `invalid bind address (:5555/path): should not contain a path element`,
+		"something with spaces": `invalid bind address (something with spaces): parse "tcp://something with spaces": invalid character " " in host name`,
+		"://":                   `invalid bind address (://): unsupported proto ''`,
+		"unknown://":            `invalid bind address (unknown://): unsupported proto 'unknown'`,
+		"tcp://:port":           `invalid bind address (tcp://:port): parse "tcp://:port": invalid port ":port" after host`,
+		"tcp://invalid:port":    `invalid bind address (tcp://invalid:port): parse "tcp://invalid:port": invalid port ":port" after host`,
+		"tcp://:5555/":          `invalid bind address (tcp://:5555/): should not contain a path element`,
+		"tcp://:5555/p":         `invalid bind address (tcp://:5555/p): should not contain a path element`,
+		"tcp://0.0.0.0:5555/":   `invalid bind address (tcp://0.0.0.0:5555/): should not contain a path element`,
+		"tcp://0.0.0.0:5555/p":  `invalid bind address (tcp://0.0.0.0:5555/p): should not contain a path element`,
+		"tcp://[::1]:/":         `invalid bind address (tcp://[::1]:/): should not contain a path element`,
+		"tcp://[::1]:5555/":     `invalid bind address (tcp://[::1]:5555/): should not contain a path element`,
+		"tcp://[::1]:5555/p":    `invalid bind address (tcp://[::1]:5555/p): should not contain a path element`,
+		" tcp://:5555/path ":    `invalid bind address (tcp://:5555/path): should not contain a path element`,
 	}
 
 	valid := map[string]string{
@@ -33,6 +33,7 @@ func TestParseHost(t *testing.T) {
 		"tcp://":                   DefaultTCPHost,
 		"tcp://:":                  DefaultTCPHost,
 		"tcp://:5555":              fmt.Sprintf("tcp://%s:5555", DefaultHTTPHost),
+		"tcp://[::1]":              fmt.Sprintf(`tcp://[::1]:%d`, DefaultHTTPPort),
 		"tcp://[::1]:":             fmt.Sprintf(`tcp://[::1]:%d`, DefaultHTTPPort),
 		"tcp://[::1]:5555":         `tcp://[::1]:5555`,
 		"tcp://0.0.0.0:5555":       "tcp://0.0.0.0:5555",
@@ -70,18 +71,19 @@ func TestParseHost(t *testing.T) {
 
 func TestParseDockerDaemonHost(t *testing.T) {
 	invalids := map[string]string{
-		"tcp:a.b.c.d":                   `parse "tcp://tcp:a.b.c.d": invalid port ":a.b.c.d" after host`,
-		"tcp:a.b.c.d/path":              `parse "tcp://tcp:a.b.c.d/path": invalid port ":a.b.c.d" after host`,
-		"udp://127.0.0.1":               "Invalid bind address format: udp://127.0.0.1",
-		"udp://127.0.0.1:5555":          "Invalid bind address format: udp://127.0.0.1:5555",
-		"tcp://unix:///run/docker.sock": "Invalid proto, expected tcp: unix:///run/docker.sock",
-		" tcp://:5555/path ":            "Invalid bind address format:  tcp://:5555/path ",
-		"":                              "Invalid bind address format: ",
+		"tcp:a.b.c.d":                   `invalid bind address (tcp:a.b.c.d): parse "tcp://tcp:a.b.c.d": invalid port ":a.b.c.d" after host`,
+		"tcp:a.b.c.d/path":              `invalid bind address (tcp:a.b.c.d/path): parse "tcp://tcp:a.b.c.d/path": invalid port ":a.b.c.d" after host`,
+		"tcp://127.0.0.1/":              "invalid bind address (tcp://127.0.0.1/): should not contain a path element",
+		"udp://127.0.0.1":               "invalid bind address (udp://127.0.0.1): unsupported proto 'udp'",
+		"udp://127.0.0.1:5555":          "invalid bind address (udp://127.0.0.1:5555): unsupported proto 'udp'",
+		"tcp://unix:///run/docker.sock": "invalid bind address (tcp://unix:///run/docker.sock): should not contain a path element",
+		" tcp://:5555/path ":            "invalid bind address ( tcp://:5555/path ): unsupported proto ' tcp'",
+		"":                              "invalid bind address (): unsupported proto ''",
 		":5555/path":                    "invalid bind address (:5555/path): should not contain a path element",
 		"0.0.0.1:5555/path":             "invalid bind address (0.0.0.1:5555/path): should not contain a path element",
 		"[::1]:5555/path":               "invalid bind address ([::1]:5555/path): should not contain a path element",
 		"[0:0:0:0:0:0:0:1]:5555/path":   "invalid bind address ([0:0:0:0:0:0:0:1]:5555/path): should not contain a path element",
-		"tcp://:5555/path":              "invalid bind address (:5555/path): should not contain a path element",
+		"tcp://:5555/path":              "invalid bind address (tcp://:5555/path): should not contain a path element",
 		"localhost:5555/path":           "invalid bind address (localhost:5555/path): should not contain a path element",
 	}
 	valids := map[string]string{
@@ -89,8 +91,10 @@ func TestParseDockerDaemonHost(t *testing.T) {
 		":5555":                   fmt.Sprintf("tcp://%s:5555", DefaultHTTPHost),
 		"0.0.0.1:":                fmt.Sprintf("tcp://0.0.0.1:%d", DefaultHTTPPort),
 		"0.0.0.1:5555":            "tcp://0.0.0.1:5555",
+		"[::1]":                   fmt.Sprintf("tcp://[::1]:%d", DefaultHTTPPort),
 		"[::1]:":                  fmt.Sprintf("tcp://[::1]:%d", DefaultHTTPPort),
 		"[::1]:5555":              "tcp://[::1]:5555",
+		"[0:0:0:0:0:0:0:1]":       fmt.Sprintf("tcp://[0:0:0:0:0:0:0:1]:%d", DefaultHTTPPort),
 		"[0:0:0:0:0:0:0:1]:":      fmt.Sprintf("tcp://[0:0:0:0:0:0:0:1]:%d", DefaultHTTPPort),
 		"[0:0:0:0:0:0:0:1]:5555":  "tcp://[0:0:0:0:0:0:0:1]:5555",
 		"localhost":               fmt.Sprintf("tcp://localhost:%d", DefaultHTTPPort),
@@ -102,6 +106,7 @@ func TestParseDockerDaemonHost(t *testing.T) {
 		"npipe:////./pipe/foo":    "npipe:////./pipe/foo",
 		"tcp://":                  DefaultTCPHost,
 		"tcp://:5555":             fmt.Sprintf("tcp://%s:5555", DefaultHTTPHost),
+		"tcp://[::1]":             fmt.Sprintf("tcp://[::1]:%d", DefaultHTTPPort),
 		"tcp://[::1]:":            fmt.Sprintf("tcp://[::1]:%d", DefaultHTTPPort),
 		"tcp://[::1]:5555":        "tcp://[::1]:5555",
 		"unix://":                 "unix://" + DefaultUnixSocket,
@@ -136,10 +141,10 @@ func TestParseTCP(t *testing.T) {
 		defaultHTTPHost = "tcp://127.0.0.1:8888"
 	)
 	invalids := map[string]string{
-		"tcp:a.b.c.d":                 `parse "tcp://tcp:a.b.c.d": invalid port ":a.b.c.d" after host`,
-		"tcp:a.b.c.d/path":            `parse "tcp://tcp:a.b.c.d/path": invalid port ":a.b.c.d" after host`,
-		"udp://127.0.0.1":             "Invalid proto, expected tcp: udp://127.0.0.1",
-		"udp://127.0.0.1:5555":        "Invalid proto, expected tcp: udp://127.0.0.1:5555",
+		"tcp:a.b.c.d":                 `invalid bind address (tcp:a.b.c.d): parse "tcp://tcp:a.b.c.d": invalid port ":a.b.c.d" after host`,
+		"tcp:a.b.c.d/path":            `invalid bind address (tcp:a.b.c.d/path): parse "tcp://tcp:a.b.c.d/path": invalid port ":a.b.c.d" after host`,
+		"udp://127.0.0.1":             "invalid bind address (udp://127.0.0.1): unsupported proto 'udp'",
+		"udp://127.0.0.1:5555":        "invalid bind address (udp://127.0.0.1:5555): unsupported proto 'udp'",
 		":5555/path":                  "invalid bind address (:5555/path): should not contain a path element",
 		"0.0.0.1:5555/path":           "invalid bind address (0.0.0.1:5555/path): should not contain a path element",
 		"[::1]:5555/path":             "invalid bind address ([::1]:5555/path): should not contain a path element",
@@ -147,6 +152,12 @@ func TestParseTCP(t *testing.T) {
 		"tcp://:5555/path":            "invalid bind address (tcp://:5555/path): should not contain a path element",
 		"localhost:5555/path":         "invalid bind address (localhost:5555/path): should not contain a path element",
 	}
+	invalidDefaults := map[string]string{
+		"localhost":                 `invalid default address (localhost): unsupported proto ''`,
+		"udp://localhost":           `invalid default address (udp://localhost): unsupported proto 'udp'`,
+		"tcp://localhost:noport":    `invalid default address (tcp://localhost:noport): parse "tcp://localhost:noport": invalid port ":noport" after host`,
+		"tcp://localhost:5555/path": `invalid default address (tcp://localhost:5555/path): should not contain a path element`,
+	}
 	valids := map[string]string{
 		"":                       defaultHTTPHost,
 		"0.0.0.1":                "tcp://0.0.0.1:8888",
@@ -154,9 +165,10 @@ func TestParseTCP(t *testing.T) {
 		"0.0.0.1:5555":           "tcp://0.0.0.1:5555",
 		":":                      "tcp://127.0.0.1:8888",
 		":5555":                  "tcp://127.0.0.1:5555",
-		"::1":                    "tcp://[::1]:8888",
+		"[::1]":                  "tcp://[::1]:8888",
 		"[::1]:":                 "tcp://[::1]:8888",
 		"[::1]:5555":             "tcp://[::1]:5555",
+		"[0:0:0:0:0:0:0:1]":      "tcp://[0:0:0:0:0:0:0:1]:8888",
 		"[0:0:0:0:0:0:0:1]:":     "tcp://[0:0:0:0:0:0:0:1]:8888",
 		"[0:0:0:0:0:0:0:1]:5555": "tcp://[0:0:0:0:0:0:0:1]:5555",
 		"localhost":              "tcp://localhost:8888",
@@ -177,6 +189,14 @@ func TestParseTCP(t *testing.T) {
 			}
 		})
 	}
+	for invalidAddr, expectedError := range invalidDefaults {
+		t.Run("default "+invalidAddr, func(t *testing.T) {
+			_, err := ParseTCPAddr("tcp://0.0.0.0:2375", invalidAddr)
+			if err == nil || err.Error() != expectedError {
+				t.Errorf(`expected error "%s", got "%v"`, expectedError, err)
+			}
+		})
+	}
 	for validAddr, expectedAddr := range valids {
 		t.Run(validAddr, func(t *testing.T) {
 			addr, err := ParseTCPAddr(validAddr, defaultHTTPHost)
@@ -191,10 +211,10 @@ func TestParseTCP(t *testing.T) {
 }
 
 func TestParseInvalidUnixAddrInvalid(t *testing.T) {
-	if _, err := parseSimpleProtoAddr("unix", "tcp://127.0.0.1", "unix:///var/run/docker.sock"); err == nil || err.Error() != "Invalid proto, expected unix: tcp://127.0.0.1" {
+	if _, err := parseSimpleProtoAddr("unix", "tcp://127.0.0.1", "unix:///var/run/docker.sock"); err == nil || err.Error() != "invalid proto, expected unix: tcp://127.0.0.1" {
 		t.Fatalf("Expected an error, got %v", err)
 	}
-	if _, err := parseSimpleProtoAddr("unix", "unix://tcp://127.0.0.1", "/var/run/docker.sock"); err == nil || err.Error() != "Invalid proto, expected unix: tcp://127.0.0.1" {
+	if _, err := parseSimpleProtoAddr("unix", "unix://tcp://127.0.0.1", "/var/run/docker.sock"); err == nil || err.Error() != "invalid proto, expected unix: tcp://127.0.0.1" {
 		t.Fatalf("Expected an error, got %v", err)
 	}
 	if v, err := parseSimpleProtoAddr("unix", "", "/var/run/docker.sock"); err != nil || v != "unix:///var/run/docker.sock" {