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 <github@gone.nl>

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,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))