From c2ca3e1118d88bdcddc5206afc26b1125e0b5575 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 9 Apr 2022 15:18:43 +0200 Subject: [PATCH] daemon/logger/syslog: remove uses of pkg/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: - removes a redundant use of urlutil.IsTransportURL(); instead adding some code to check if the given scheme (protocol) is supported. - define a `defaultPort` const for the default port. - use `net.JoinHostPort()` instead of string concatenating, to account for possible issues with IPv6 addresses. - renames a variable that collided with an imported package. - improves test coverage, and moves an integration test. Signed-off-by: Sebastiaan van Stijn --- daemon/logger/syslog/syslog.go | 23 +++--- daemon/logger/syslog/syslog_test.go | 97 +++++++++++++++-------- integration-cli/docker_cli_daemon_test.go | 8 -- 3 files changed, 74 insertions(+), 54 deletions(-) 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..2510e6c124 100644 --- a/integration-cli/docker_cli_daemon_test.go +++ b/integration-cli/docker_cli_daemon_test.go @@ -1671,14 +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))