daemon: logger: error out on daemon start if invalid logger address

If an invalid logger address is provided on daemon start it will silently fail. As syslog driver is doing, this check should be done on daemon start and prevent it from starting even in other drivers. This patch also adds integration tests for this behavior. Signed-off-by: Antonio Murdaca <[email protected]>
mheon · Sep 20, 2015 · e3c4724 · e3c4724
1 parent 536353e
commit e3c4724
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 60 deletions.
diff --git a/daemon/logger/fluentd/fluentd.go b/daemon/logger/fluentd/fluentd.go
@@ -38,44 +38,20 @@ func init() {
 	}
 }
 
-func parseConfig(ctx logger.Context) (string, int, string, error) {
-	host := defaultHostName
-	port := defaultPort
-
-	config := ctx.Config
-
-	tag, err := loggerutils.ParseLogTag(ctx, "docker.{{.ID}}")
-	if err != nil {
-		return "", 0, "", err
-	}
-
-	if address := config["fluentd-address"]; address != "" {
-		if h, p, err := net.SplitHostPort(address); err != nil {
-			if !strings.Contains(err.Error(), "missing port in address") {
-				return "", 0, "", err
-			}
-			host = h
-		} else {
-			portnum, err := strconv.Atoi(p)
-			if err != nil {
-				return "", 0, "", err
-			}
-			host = h
-			port = portnum
-		}
-	}
-
-	return host, port, tag, nil
-}
-
 // New creates a fluentd logger using the configuration passed in on
 // the context. Supported context configuration variables are
 // fluentd-address & fluentd-tag.
 func New(ctx logger.Context) (logger.Logger, error) {
-	host, port, tag, err := parseConfig(ctx)
+	host, port, err := parseAddress(ctx.Config["fluentd-address"])
+	if err != nil {
+		return nil, err
+	}
+
+	tag, err := loggerutils.ParseLogTag(ctx, "docker.{{.ID}}")
 	if err != nil {
 		return nil, err
 	}
+
 	logrus.Debugf("logging driver fluentd configured for container:%s, host:%s, port:%d, tag:%s.", ctx.ContainerID, host, port, tag)
 
 	// logger tries to recoonect 2**32 - 1 times
@@ -104,6 +80,14 @@ func (f *fluentd) Log(msg *logger.Message) error {
 	return f.writer.PostWithTime(f.tag, msg.Timestamp, data)
 }
 
+func (f *fluentd) Close() error {
+	return f.writer.Close()
+}
+
+func (f *fluentd) Name() string {
+	return name
+}
+
 // ValidateLogOpt looks for fluentd specific log options fluentd-address & fluentd-tag.
 func ValidateLogOpt(cfg map[string]string) error {
 	for key := range cfg {
@@ -115,13 +99,30 @@ func ValidateLogOpt(cfg map[string]string) error {
 			return fmt.Errorf("unknown log opt '%s' for fluentd log driver", key)
 		}
 	}
+
+	if _, _, err := parseAddress(cfg["fluentd-address"]); err != nil {
+		return err
+	}
+
 	return nil
 }
 
-func (f *fluentd) Close() error {
-	return f.writer.Close()
-}
+func parseAddress(address string) (string, int, error) {
+	if address == "" {
+		return defaultHostName, defaultPort, nil
+	}
 
-func (f *fluentd) Name() string {
-	return name
+	host, port, err := net.SplitHostPort(address)
+	if err != nil {
+		if !strings.Contains(err.Error(), "missing port in address") {
+			return "", 0, fmt.Errorf("invalid fluentd-address %s: %s", address, err)
+		}
+		return host, defaultPort, nil
+	}
+
+	portnum, err := strconv.Atoi(port)
+	if err != nil {
+		return "", 0, fmt.Errorf("invalid fluentd-address %s: %s", address, err)
+	}
+	return host, portnum, nil
 }
diff --git a/daemon/logger/gelf/gelf.go b/daemon/logger/gelf/gelf.go
@@ -147,28 +147,35 @@ func ValidateLogOpt(cfg map[string]string) error {
 			return fmt.Errorf("unknown log opt '%s' for gelf log driver", key)
 		}
 	}
+
+	if _, err := parseAddress(cfg["gelf-address"]); err != nil {
+		return err
+	}
+
 	return nil
 }
 
 func parseAddress(address string) (string, error) {
-	if urlutil.IsTransportURL(address) {
-		url, err := url.Parse(address)
-		if err != nil {
-			return "", err
-		}
-
-		// we support only udp
-		if url.Scheme != "udp" {
-			return "", fmt.Errorf("gelf: endpoint needs to be UDP")
-		}
+	if address == "" {
+		return "", nil
+	}
+	if !urlutil.IsTransportURL(address) {
+		return "", fmt.Errorf("gelf-address should be in form proto://address, got %v", address)
+	}
+	url, err := url.Parse(address)
+	if err != nil {
+		return "", err
+	}
 
-		// get host and port
-		if _, _, err = net.SplitHostPort(url.Host); err != nil {
-			return "", fmt.Errorf("gelf: please provide gelf-address as udp://host:port")
-		}
+	// we support only udp
+	if url.Scheme != "udp" {
+		return "", fmt.Errorf("gelf: endpoint needs to be UDP")
+	}
 
-		return url.Host, nil
+	// get host and port
+	if _, _, err = net.SplitHostPort(url.Host); err != nil {
+		return "", fmt.Errorf("gelf: please provide gelf-address as udp://host:port")
 	}
 
-	return "", nil
+	return url.Host, nil
 }
diff --git a/integration-cli/docker_cli_daemon_test.go b/integration-cli/docker_cli_daemon_test.go
@@ -1609,14 +1609,6 @@ func (s *DockerDaemonSuite) TestDaemonRestartWithContainerWithRestartPolicyAlway
 	c.Assert(strings.TrimSpace(out), check.Equals, id[:12])
 }
 
-func (s *DockerDaemonSuite) TestDaemonCorruptedSyslogAddress(c *check.C) {
-	c.Assert(s.d.Start("--log-driver=syslog", "--log-opt", "syslog-address=corrupted:1234"), check.NotNil)
-	runCmd := exec.Command("grep", "Failed to set log opts: syslog-address should be in form proto://address", s.d.LogfileName())
-	if out, _, err := runCommandWithOutput(runCmd); err != nil {
-		c.Fatalf("Expected 'Error starting daemon' message; but doesn't exist in log: %q, err: %v", out, err)
-	}
-}
-
 func (s *DockerDaemonSuite) TestDaemonWideLogConfig(c *check.C) {
 	c.Assert(s.d.Start("--log-driver=json-file", "--log-opt=max-size=1k"), check.IsNil)
 	out, err := s.d.Cmd("run", "-d", "--name=logtest", "busybox", "top")
@@ -1689,3 +1681,27 @@ func (s *DockerDaemonSuite) TestDaemonRestartLocalVolumes(c *check.C) {
 	_, err = s.d.Cmd("volume", "inspect", "test")
 	c.Assert(err, check.IsNil)
 }
+
+func (s *DockerDaemonSuite) TestDaemonCorruptedLogDriverAddress(c *check.C) {
+	for _, driver := range []string{
+		"syslog",
+		"gelf",
+	} {
+		args := []string{"--log-driver=" + driver, "--log-opt", driver + "-address=corrupted:42"}
+		c.Assert(s.d.Start(args...), check.NotNil, check.Commentf(fmt.Sprintf("Expected daemon not to start with invalid %s-address provided", driver)))
+		expected := fmt.Sprintf("Failed to set log opts: %s-address should be in form proto://address", driver)
+		runCmd := exec.Command("grep", expected, s.d.LogfileName())
+		if out, _, err := runCommandWithOutput(runCmd); err != nil {
+			c.Fatalf("Expected %q message; but doesn't exist in log: %q, err: %v", expected, out, err)
+		}
+	}
+}
+
+func (s *DockerDaemonSuite) TestDaemonCorruptedFluentdAddress(c *check.C) {
+	c.Assert(s.d.Start("--log-driver=fluentd", "--log-opt", "fluentd-address=corrupted:c"), check.NotNil)
+	expected := "Failed to set log opts: invalid fluentd-address corrupted:c: "
+	runCmd := exec.Command("grep", expected, s.d.LogfileName())
+	if out, _, err := runCommandWithOutput(runCmd); err != nil {
+		c.Fatalf("Expected %q message; but doesn't exist in log: %q, err: %v", expected, out, err)
+	}
+}