Merge pull request #16437 from runcom/invalid-logger-address

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

api/server/container.go

@@ -94,7 +94,11 @@ func (s *Server) getContainersLogs(ctx context.Context, w http.ResponseWriter, r
 		return fmt.Errorf("Missing parameter")
 	}
 
-	// Validate args here, because we can't return not StatusOK after job.Run() call
+	// Args are validated before the stream starts because when it starts we're
+	// sending HTTP 200 by writing an empty chunk of data to tell the client that
+	// daemon is going to stream. By sending this initial HTTP 200 we can't report
+	// any error after the stream starts (i.e. container not found, wrong parameters)
+	// with the appropriate status code.
 	stdout, stderr := boolValue(r, "stdout"), boolValue(r, "stderr")
 	if !(stdout || stderr) {
 		return fmt.Errorf("Bad parameters: you must choose at least one stream")

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
 }

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")
-		}
-
-		// 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 url.Host, nil
+	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
 	}
 
-	return "", nil
+	// we support only udp
+	if url.Scheme != "udp" {
+		return "", fmt.Errorf("gelf: endpoint needs to be UDP")
+	}
+
+	// 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 url.Host, nil
 }

integration-cli/docker_cli_daemon_test.go

@@ -1615,14 +1615,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) {
 	if err := s.d.StartWithBusybox("--log-driver=json-file", "--log-opt=max-size=1k"); err != nil {
 		c.Fatal(err)
@@ -1697,3 +1689,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)
+	}
+}