daemon/logger/fluentd: fix missing host, remove urlutil.IsTransportURL()

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic
utility to handle URLs, and should only be used by the builder to handle (remote)
build contexts.

This patch:

- fix some cases where the host was ignored for valid addresses.
- removes a redundant use of urlutil.IsTransportURL(); instead adding code to
  check if the given scheme (protocol) is supported.
- improve port validation for out of range ports.
- fix some missing validation: the driver was silently ignoring path elements,
  but expected a host (not an URL)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/logger/fluentd/fluentd.go

@@ -4,7 +4,6 @@ package fluentd // import "github.com/docker/docker/daemon/logger/fluentd"
 
 import (
 	"math"
-	"net"
 	"net/url"
 	"strconv"
 	"strings"
@@ -13,7 +12,6 @@ import (
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/logger/loggerutils"
 	"github.com/docker/docker/errdefs"
-	"github.com/docker/docker/pkg/urlutil"
 	units "github.com/docker/go-units"
 	"github.com/fluent/fluent-logger-golang/fluent"
 	"github.com/pkg/errors"
@@ -281,54 +279,45 @@ func parseAddress(address string) (*location, error) {
 		address = defaultProtocol + "://" + address
 	}
 
-	protocol := defaultProtocol
-	if urlutil.IsTransportURL(address) {
-		addr, err := url.Parse(address)
-		if err != nil {
-			return nil, err
-		}
-		// unix and unixgram socket
-		if addr.Scheme == "unix" || addr.Scheme == "unixgram" {
-			if strings.TrimLeft(addr.Path, "/") == "" {
-				return nil, errors.New("path is empty")
-			}
-			return &location{
-				protocol: addr.Scheme,
-				host:     "",
-				port:     0,
-				path:     addr.Path,
-			}, nil
-		}
-		// tcp|udp
-		protocol = addr.Scheme
-		address = addr.Host
-
-		if addr.Path != "" {
-			return nil, errors.New("should not contain a path element")
-		}
-	}
-
-	host, port, err := net.SplitHostPort(address)
-	if err != nil {
-		if !strings.Contains(err.Error(), "missing port in address") {
-			return nil, err
-		}
-		return &location{
-			protocol: protocol,
-			host:     host,
-			port:     defaultPort,
-			path:     "",
-		}, nil
-	}
-
-	portnum, err := strconv.Atoi(port)
+	addr, err := url.Parse(address)
 	if err != nil {
 		return nil, err
 	}
+
+	switch addr.Scheme {
+	case "unix", "unixgram":
+		if strings.TrimLeft(addr.Path, "/") == "" {
+			return nil, errors.New("path is empty")
+		}
+		return &location{protocol: addr.Scheme, path: addr.Path}, nil
+	case "tcp", "tcp+tls", "udp":
+		// continue processing below
+	default:
+		return nil, errors.Errorf("unsupported scheme: '%s'", addr.Scheme)
+	}
+
+	if addr.Path != "" {
+		return nil, errors.New("should not contain a path element")
+	}
+
+	host := defaultHost
+	port := defaultPort
+
+	if h := addr.Hostname(); h != "" {
+		host = h
+	}
+	if p := addr.Port(); p != "" {
+		// Port numbers are 16 bit: https://www.ietf.org/rfc/rfc793.html#section-3.1
+		portNum, err := strconv.ParseUint(p, 10, 16)
+		if err != nil {
+			return nil, errors.Wrap(err, "invalid port")
+		}
+		port = int(portNum)
+	}
 	return &location{
-		protocol: protocol,
+		protocol: addr.Scheme,
 		host:     host,
-		port:     portnum,
+		port:     port,
 		path:     "",
 	}, nil
 }

daemon/logger/fluentd/fluentd_test.go

@@ -25,13 +25,20 @@ func TestValidateLogOptReconnectInterval(t *testing.T) {
 }
 
 func TestValidateLogOptAddress(t *testing.T) {
-
-	// paths to try
+	// ports to try, and their results
+	validPorts := map[string]int{
+		"":       defaultPort,
+		":":      defaultPort,
+		":123":   123,
+		":65535": 65535,
+	}
+	// paths to try, which should result in an error
 	paths := []string{"/", "/some-path"}
 
 	tests := []struct {
 		addr        string
-		paths       []string // paths to append to addr, should be an error for tcp/udp
+		ports       map[string]int // combinations of addr + port -> expected port
+		paths       []string       // paths to append to addr, should be an error for tcp/udp
 		expected    location
 		expectedErr string
 	}{
@@ -45,26 +52,29 @@ func TestValidateLogOptAddress(t *testing.T) {
 		},
 		{
 			addr:  "192.168.1.1",
+			ports: validPorts,
 			paths: paths,
 			expected: location{
-				port:     defaultPort,
 				protocol: defaultProtocol,
+				host:     "192.168.1.1",
 			},
 		},
 		{
 			addr:  "[::1]",
+			ports: validPorts,
 			paths: paths,
 			expected: location{
-				port:     defaultPort,
 				protocol: defaultProtocol,
+				host:     "::1",
 			},
 		},
 		{
 			addr:  "example.com",
+			ports: validPorts,
 			paths: paths,
 			expected: location{
-				port:     defaultPort,
 				protocol: defaultProtocol,
+				host:     "example.com",
 			},
 		},
 		{
@@ -72,33 +82,26 @@ func TestValidateLogOptAddress(t *testing.T) {
 			paths: paths,
 			expected: location{
 				protocol: "tcp",
+				host:     defaultHost,
 				port:     defaultPort,
 			},
 		},
 		{
 			addr:  "tcp://example.com",
-			paths: paths,
-			expected: location{
-				protocol: "tcp",
-				port:     defaultPort,
-			},
-		},
-		{
-			addr:  "tcp://example.com:65535",
+			ports: validPorts,
 			paths: paths,
 			expected: location{
 				protocol: "tcp",
 				host:     "example.com",
-				port:     65535,
 			},
 		},
 		{
 			addr:        "://",
-			expectedErr: "invalid syntax",
+			expectedErr: "missing protocol scheme",
 		},
 		{
 			addr:        "something://",
-			expectedErr: "invalid syntax",
+			expectedErr: "unsupported scheme: 'something'",
 		},
 		{
 			addr:        "corrupted:c",
@@ -112,6 +115,10 @@ func TestValidateLogOptAddress(t *testing.T) {
 			addr:        "tcp://example.com:-1",
 			expectedErr: "invalid port",
 		},
+		{
+			addr:        "tcp://example.com:65536",
+			expectedErr: "invalid port",
+		},
 		{
 			addr:        "unix://",
 			expectedErr: "path is empty",
@@ -133,26 +140,33 @@ func TestValidateLogOptAddress(t *testing.T) {
 	}
 	for _, tc := range tests {
 		tc := tc
-		if len(tc.paths) == 0 {
-			tc.paths = []string{""}
+		if len(tc.ports) == 0 {
+			tc.ports = map[string]int{"": tc.expected.port}
 		}
-		for _, path := range tc.paths {
-			address := tc.addr + path
-			t.Run(address, func(t *testing.T) {
-				err := ValidateLogOpt(map[string]string{addressKey: address})
-				if path != "" {
-					assert.ErrorContains(t, err, "should not contain a path element")
-					return
-				}
-				if tc.expectedErr != "" {
-					assert.ErrorContains(t, err, tc.expectedErr)
-					return
-				}
 
-				assert.NilError(t, err)
-				addr, _ := parseAddress(address)
-				assert.DeepEqual(t, tc.expected, *addr, cmp.AllowUnexported(location{}))
-			})
+		// always try empty paths; add paths to try if the test specifies it
+		tc.paths = append([]string{""}, tc.paths...)
+		for port, expectedPort := range tc.ports {
+			for _, path := range tc.paths {
+				address := tc.addr + port + path
+				expected := tc.expected
+				expected.port = expectedPort
+				t.Run(address, func(t *testing.T) {
+					err := ValidateLogOpt(map[string]string{addressKey: address})
+					if path != "" {
+						assert.ErrorContains(t, err, "should not contain a path element")
+						return
+					}
+					if tc.expectedErr != "" {
+						assert.ErrorContains(t, err, tc.expectedErr)
+						return
+					}
+
+					assert.NilError(t, err)
+					addr, _ := parseAddress(address)
+					assert.DeepEqual(t, expected, *addr, cmp.AllowUnexported(location{}))
+				})
+			}
 		}
 	}
 }