Merge pull request #43476 from thaJeztah/split_urlutil

daemon/logger: remove uses of pkg/urlutil, fix fluentd validation and parsing

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"
@@ -172,7 +170,7 @@ func parseConfig(cfg map[string]string) (fluent.Config, error) {
 
 	loc, err := parseAddress(cfg[addressKey])
 	if err != nil {
-		return config, err
+		return config, errors.Wrapf(err, "invalid fluentd-address (%s)", cfg[addressKey])
 	}
 
 	bufferLimit := defaultBufferLimit
@@ -277,48 +275,49 @@ func parseAddress(address string) (*location, error) {
 		}, nil
 	}
 
-	protocol := defaultProtocol
-	givenAddress := address
-	if urlutil.IsTransportURL(address) {
-		url, err := url.Parse(address)
+	if !strings.Contains(address, "://") {
+		address = defaultProtocol + "://" + address
+	}
+
+	addr, err := url.Parse(address)
+	if err != nil {
+		return nil, err
+	}
+
+	switch addr.Scheme {
+	case "unix":
+		if strings.TrimLeft(addr.Path, "/") == "" {
+			return nil, errors.New("path is empty")
+		}
+		return &location{protocol: addr.Scheme, path: addr.Path}, nil
+	case "tcp", "tls":
+		// 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.Wrapf(err, "invalid fluentd-address %s", givenAddress)
+			return nil, errors.Wrap(err, "invalid port")
 		}
-		// unix and unixgram socket
-		if url.Scheme == "unix" || url.Scheme == "unixgram" {
-			return &location{
-				protocol: url.Scheme,
-				host:     "",
-				port:     0,
-				path:     url.Path,
-			}, nil
-		}
-		// tcp|udp
-		protocol = url.Scheme
-		address = url.Host
-	}
-
-	host, port, err := net.SplitHostPort(address)
-	if err != nil {
-		if !strings.Contains(err.Error(), "missing port in address") {
-			return nil, errors.Wrapf(err, "invalid fluentd-address %s", givenAddress)
-		}
-		return &location{
-			protocol: protocol,
-			host:     host,
-			port:     defaultPort,
-			path:     "",
-		}, nil
-	}
-
-	portnum, err := strconv.Atoi(port)
-	if err != nil {
-		return nil, errors.Wrapf(err, "invalid fluentd-address %s", givenAddress)
+		port = int(portNum)
 	}
 	return &location{
-		protocol: protocol,
+		protocol: addr.Scheme,
 		host:     host,
-		port:     portnum,
+		port:     port,
 		path:     "",
 	}, nil
 }

daemon/logger/fluentd/fluentd_test.go

@@ -2,6 +2,7 @@ package fluentd // import "github.com/docker/docker/daemon/logger/fluentd"
 import (
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"gotest.tools/v3/assert"
 )
 
@@ -22,3 +23,180 @@ func TestValidateLogOptReconnectInterval(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateLogOptAddress(t *testing.T) {
+	// 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
+		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
+	}{
+		{
+			addr: "",
+			expected: location{
+				protocol: defaultProtocol,
+				host:     defaultHost,
+				port:     defaultPort,
+			},
+		},
+		{
+			addr:  "192.168.1.1",
+			ports: validPorts,
+			paths: paths,
+			expected: location{
+				protocol: defaultProtocol,
+				host:     "192.168.1.1",
+			},
+		},
+		{
+			addr:  "[::1]",
+			ports: validPorts,
+			paths: paths,
+			expected: location{
+				protocol: defaultProtocol,
+				host:     "::1",
+			},
+		},
+		{
+			addr:  "example.com",
+			ports: validPorts,
+			paths: paths,
+			expected: location{
+				protocol: defaultProtocol,
+				host:     "example.com",
+			},
+		},
+		{
+			addr:  "tcp://",
+			paths: paths,
+			expected: location{
+				protocol: "tcp",
+				host:     defaultHost,
+				port:     defaultPort,
+			},
+		},
+		{
+			addr:  "tcp://example.com",
+			ports: validPorts,
+			paths: paths,
+			expected: location{
+				protocol: "tcp",
+				host:     "example.com",
+			},
+		},
+		{
+			addr:  "tls://",
+			paths: paths,
+			expected: location{
+				protocol: "tls",
+				host:     defaultHost,
+				port:     defaultPort,
+			},
+		},
+		{
+			addr:  "tls://example.com",
+			ports: validPorts,
+			paths: paths,
+			expected: location{
+				protocol: "tls",
+				host:     "example.com",
+			},
+		},
+		{
+			addr:        "://",
+			expectedErr: "missing protocol scheme",
+		},
+		{
+			addr:        "something://",
+			expectedErr: "unsupported scheme: 'something'",
+		},
+		{
+			addr:        "udp://",
+			expectedErr: "unsupported scheme: 'udp'",
+		},
+		{
+			addr:        "unixgram://",
+			expectedErr: "unsupported scheme: 'unixgram'",
+		},
+		{
+			addr:        "tcp+tls://",
+			expectedErr: "unsupported scheme: 'tcp+tls'",
+		},
+		{
+			addr:        "corrupted:c",
+			expectedErr: "invalid port",
+		},
+		{
+			addr:        "tcp://example.com:port",
+			expectedErr: "invalid port",
+		},
+		{
+			addr:        "tcp://example.com:-1",
+			expectedErr: "invalid port",
+		},
+		{
+			addr:        "tcp://example.com:65536",
+			expectedErr: "invalid port",
+		},
+		{
+			addr:        "unix://",
+			expectedErr: "path is empty",
+		},
+		{
+			addr: "unix:///some/socket.sock",
+			expected: location{
+				protocol: "unix",
+				path:     "/some/socket.sock",
+			},
+		},
+		{
+			addr: "unix:///some/socket.sock:80", // unusual, but technically valid
+			expected: location{
+				protocol: "unix",
+				path:     "/some/socket.sock:80",
+			},
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		if len(tc.ports) == 0 {
+			tc.ports = map[string]int{"": tc.expected.port}
+		}
+
+		// 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{}))
+				})
+			}
+		}
+	}
+}

daemon/logger/gelf/gelf.go

@@ -14,7 +14,6 @@ import (
 	"github.com/Graylog2/go-gelf/gelf"
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/logger/loggerutils"
-	"github.com/docker/docker/pkg/urlutil"
 	"github.com/sirupsen/logrus"
 )
 
@@ -251,23 +250,18 @@ func parseAddress(address string) (*url.URL, error) {
 	if address == "" {
 		return nil, fmt.Errorf("gelf-address is a required parameter")
 	}
-	if !urlutil.IsTransportURL(address) {
-		return nil, fmt.Errorf("gelf-address should be in form proto://address, got %v", address)
-	}
-	url, err := url.Parse(address)
+	addr, err := url.Parse(address)
 	if err != nil {
 		return nil, err
 	}
 
-	// we support only udp
-	if url.Scheme != "udp" && url.Scheme != "tcp" {
+	if addr.Scheme != "udp" && addr.Scheme != "tcp" {
 		return nil, fmt.Errorf("gelf: endpoint needs to be TCP or UDP")
 	}
 
-	// get host and port
-	if _, _, err = net.SplitHostPort(url.Host); err != nil {
+	if _, _, err = net.SplitHostPort(addr.Host); err != nil {
 		return nil, fmt.Errorf("gelf: please provide gelf-address as proto://host:port")
 	}
 
-	return url, nil
+	return addr, nil
 }

daemon/logger/gelf/gelf_test.go

@@ -223,12 +223,12 @@ func TestNewTCP(t *testing.T) {
 		ContainerID: "12345678901234567890",
 	}
 
-	logger, err := New(info)
+	gelfLogger, err := New(info)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	err = logger.Close()
+	err = gelfLogger.Close()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -250,12 +250,12 @@ func TestNewUDP(t *testing.T) {
 		ContainerID: "12345678901234567890",
 	}
 
-	logger, err := New(info)
+	gelfLogger, err := New(info)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	err = logger.Close()
+	err = gelfLogger.Close()
 	if err != nil {
 		t.Fatal(err)
 	}

daemon/logger/splunk/splunk.go

@@ -22,7 +22,6 @@ import (
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/logger/loggerutils"
 	"github.com/docker/docker/pkg/pools"
-	"github.com/docker/docker/pkg/urlutil"
 	"github.com/google/uuid"
 	"github.com/sirupsen/logrus"
 )
@@ -607,8 +606,8 @@ func parseURL(info logger.Info) (*url.URL, error) {
 		return nil, fmt.Errorf("%s: failed to parse %s as url value in %s", driverName, splunkURLStr, splunkURLKey)
 	}
 
-	if !urlutil.IsURL(splunkURLStr) ||
-		!splunkURL.IsAbs() ||
+	if !splunkURL.IsAbs() ||
+		(splunkURL.Scheme != "http" && splunkURL.Scheme != "https") ||
 		(splunkURL.Path != "" && splunkURL.Path != "/") ||
 		splunkURL.RawQuery != "" ||
 		splunkURL.Fragment != "" {

daemon/logger/syslog/syslog.go

@@ -13,10 +13,8 @@ import (
 	"time"
 
 	syslog "github.com/RackSec/srslog"
-
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/logger/loggerutils"
-	"github.com/docker/docker/pkg/urlutil"
 	"github.com/docker/go-connections/tlsconfig"
 	"github.com/sirupsen/logrus"
 )
@@ -24,6 +22,7 @@ import (
 const (
 	name        = "syslog"
 	secureProto = "tcp+tls"
+	defaultPort = "514"
 )
 
 var facilities = map[string]syslog.Priority{
@@ -157,32 +156,32 @@ func parseAddress(address string) (string, string, error) {
 	if address == "" {
 		return "", "", nil
 	}
-	if !urlutil.IsTransportURL(address) {
-		return "", "", fmt.Errorf("syslog-address should be in form proto://address, got %v", address)
-	}
-	url, err := url.Parse(address)
+	addr, err := url.Parse(address)
 	if err != nil {
 		return "", "", err
 	}
 
 	// unix and unixgram socket validation
-	if url.Scheme == "unix" || url.Scheme == "unixgram" {
-		if _, err := os.Stat(url.Path); err != nil {
+	if addr.Scheme == "unix" || addr.Scheme == "unixgram" {
+		if _, err := os.Stat(addr.Path); err != nil {
 			return "", "", err
 		}
-		return url.Scheme, url.Path, nil
+		return addr.Scheme, addr.Path, nil
+	}
+	if addr.Scheme != "udp" && addr.Scheme != "tcp" && addr.Scheme != secureProto {
+		return "", "", fmt.Errorf("unsupported scheme: '%s'", addr.Scheme)
 	}
 
 	// here we process tcp|udp
-	host := url.Host
+	host := addr.Host
 	if _, _, err := net.SplitHostPort(host); err != nil {
 		if !strings.Contains(err.Error(), "missing port in address") {
 			return "", "", err
 		}
-		host = host + ":514"
+		host = net.JoinHostPort(host, defaultPort)
 	}
 
-	return url.Scheme, host, nil
+	return addr.Scheme, host, nil
 }
 
 // ValidateLogOpt looks for syslog specific log options

daemon/logger/syslog/syslog_test.go

@@ -1,8 +1,13 @@
 package syslog // import "github.com/docker/docker/daemon/logger/syslog"
 
 import (
+	"log"
 	"net"
+	"os"
+	"path/filepath"
 	"reflect"
+	"runtime"
+	"strings"
 	"testing"
 
 	syslog "github.com/RackSec/srslog"
@@ -63,42 +68,66 @@ func TestValidateLogOptEmpty(t *testing.T) {
 }
 
 func TestValidateSyslogAddress(t *testing.T) {
-	err := ValidateLogOpt(map[string]string{
-		"syslog-address": "this is not an uri",
-	})
-	if err == nil {
-		t.Fatal("Expected error with invalid uri")
-	}
-
-	// File exists
-	err = ValidateLogOpt(map[string]string{
-		"syslog-address": "unix:///",
-	})
+	const sockPlaceholder = "/TEMPDIR/socket.sock"
+	s, err := os.Create(filepath.Join(t.TempDir(), "socket.sock"))
 	if err != nil {
-		t.Fatal(err)
+		log.Fatal(err)
 	}
+	socketPath := s.Name()
+	_ = s.Close()
 
-	// File does not exist
-	err = ValidateLogOpt(map[string]string{
-		"syslog-address": "unix:///does_not_exist",
-	})
-	if err == nil {
-		t.Fatal("Expected error when address is non existing file")
+	tests := []struct {
+		address     string
+		expectedErr string
+		skipOn      string
+	}{
+		{
+			address:     "this is not an uri",
+			expectedErr: "unsupported scheme: ''",
+		},
+		{
+			address:     "corrupted:42",
+			expectedErr: "unsupported scheme: 'corrupted'",
+		},
+		{
+			address: "unix://" + sockPlaceholder,
+			skipOn:  "windows", // doesn't work with unix:// sockets
+		},
+		{
+			address:     "unix:///does_not_exist",
+			expectedErr: "no such file or directory",
+			skipOn:      "windows", // error message differs
+		},
+		{
+			address: "tcp://1.2.3.4",
+		},
+		{
+			address: "udp://1.2.3.4",
+		},
+		{
+			address:     "http://1.2.3.4",
+			expectedErr: "unsupported scheme: 'http'",
+		},
 	}
-
-	// accepts udp and tcp URIs
-	err = ValidateLogOpt(map[string]string{
-		"syslog-address": "udp://1.2.3.4",
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	err = ValidateLogOpt(map[string]string{
-		"syslog-address": "tcp://1.2.3.4",
-	})
-	if err != nil {
-		t.Fatal(err)
+	for _, tc := range tests {
+		tc := tc
+		if tc.skipOn == runtime.GOOS {
+			continue
+		}
+		t.Run(tc.address, func(t *testing.T) {
+			address := strings.Replace(tc.address, sockPlaceholder, socketPath, 1)
+			err := ValidateLogOpt(map[string]string{"syslog-address": address})
+			if tc.expectedErr != "" {
+				if err == nil {
+					t.Fatal("expected an error, got nil")
+				}
+				if !strings.Contains(err.Error(), tc.expectedErr) {
+					t.Fatalf("expected error to contain '%s', got: '%s'", tc.expectedErr, err)
+				}
+			} else if err != nil {
+				t.Fatalf("unexpected error: '%s'", err)
+			}
+		})
 	}
 }
 
@@ -109,8 +138,8 @@ func TestParseAddressDefaultPort(t *testing.T) {
 	}
 
 	_, port, _ := net.SplitHostPort(address)
-	if port != "514" {
-		t.Fatalf("Expected to default to port 514. It used port %s", port)
+	if port != defaultPort {
+		t.Fatalf("Expected to default to port %s. It used port %s", defaultPort, port)
 	}
 }
 

integration-cli/docker_cli_daemon_test.go

@@ -1671,22 +1671,6 @@ func (s *DockerDaemonSuite) TestDaemonRestartLocalVolumes(c *testing.T) {
 	assert.NilError(c, err, out)
 }
 
-// FIXME(vdemeester) should be a unit test
-func (s *DockerDaemonSuite) TestDaemonCorruptedLogDriverAddress(c *testing.T) {
-	d := daemon.New(c, dockerBinary, dockerdBinary, testdaemon.WithEnvironment(testEnv.Execution))
-	assert.Assert(c, d.StartWithError("--log-driver=syslog", "--log-opt", "syslog-address=corrupted:42") != nil)
-	expected := "syslog-address should be in form proto://address"
-	icmd.RunCommand("grep", expected, d.LogFileName()).Assert(c, icmd.Success)
-}
-
-// FIXME(vdemeester) should be a unit test
-func (s *DockerDaemonSuite) TestDaemonCorruptedFluentdAddress(c *testing.T) {
-	d := daemon.New(c, dockerBinary, dockerdBinary, testdaemon.WithEnvironment(testEnv.Execution))
-	assert.Assert(c, d.StartWithError("--log-driver=fluentd", "--log-opt", "fluentd-address=corrupted:c") != nil)
-	expected := "invalid fluentd-address corrupted:c: "
-	icmd.RunCommand("grep", expected, d.LogFileName()).Assert(c, icmd.Success)
-}
-
 // FIXME(vdemeester) Use a new daemon instance instead of the Suite one
 func (s *DockerDaemonSuite) TestDaemonStartWithoutHost(c *testing.T) {
 	s.d.UseDefaultHost = true