diff --git a/daemon/logger/fluentd/fluentd.go b/daemon/logger/fluentd/fluentd.go index 30a23293bc..b99c46e107 100644 --- a/daemon/logger/fluentd/fluentd.go +++ b/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 } diff --git a/daemon/logger/fluentd/fluentd_test.go b/daemon/logger/fluentd/fluentd_test.go index e67d63249b..8fd27b89dc 100644 --- a/daemon/logger/fluentd/fluentd_test.go +++ b/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{})) + }) + } + } + } +} diff --git a/daemon/logger/gelf/gelf.go b/daemon/logger/gelf/gelf.go index 5492e5a4ce..c7814e9207 100644 --- a/daemon/logger/gelf/gelf.go +++ b/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 } diff --git a/daemon/logger/gelf/gelf_test.go b/daemon/logger/gelf/gelf_test.go index 2b6e5f0f82..144dc544d3 100644 --- a/daemon/logger/gelf/gelf_test.go +++ b/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) } diff --git a/daemon/logger/splunk/splunk.go b/daemon/logger/splunk/splunk.go index 5d76506dcb..ebdc88b552 100644 --- a/daemon/logger/splunk/splunk.go +++ b/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 != "" { diff --git a/daemon/logger/syslog/syslog.go b/daemon/logger/syslog/syslog.go index fb20cf4def..4469af8a02 100644 --- a/daemon/logger/syslog/syslog.go +++ b/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 diff --git a/daemon/logger/syslog/syslog_test.go b/daemon/logger/syslog/syslog_test.go index fbba836e11..fbf16474d2 100644 --- a/daemon/logger/syslog/syslog_test.go +++ b/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) } } diff --git a/integration-cli/docker_cli_daemon_test.go b/integration-cli/docker_cli_daemon_test.go index 5b57c0b8a1..036aff81bc 100644 --- a/integration-cli/docker_cli_daemon_test.go +++ b/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