From 72ffa0436677694e7ec6b30110ca7f59875eab14 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sun, 20 Sep 2015 13:03:09 +0200 Subject: [PATCH 1/2] 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 Upstream-commit: e3c472426ff1dccf083e41dbbfdf5935921bf330 Component: engine --- .../engine/daemon/logger/fluentd/fluentd.go | 73 ++++++++++--------- components/engine/daemon/logger/gelf/gelf.go | 43 ++++++----- .../integration-cli/docker_cli_daemon_test.go | 32 ++++++-- 3 files changed, 86 insertions(+), 62 deletions(-) diff --git a/components/engine/daemon/logger/fluentd/fluentd.go b/components/engine/daemon/logger/fluentd/fluentd.go index 3ce0747473..974c2e21d9 100644 --- a/components/engine/daemon/logger/fluentd/fluentd.go +++ b/components/engine/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/components/engine/daemon/logger/gelf/gelf.go b/components/engine/daemon/logger/gelf/gelf.go index e980a6ce94..6a62672d72 100644 --- a/components/engine/daemon/logger/gelf/gelf.go +++ b/components/engine/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 } diff --git a/components/engine/integration-cli/docker_cli_daemon_test.go b/components/engine/integration-cli/docker_cli_daemon_test.go index b3083b4461..f53400e66b 100644 --- a/components/engine/integration-cli/docker_cli_daemon_test.go +++ b/components/engine/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) + } +} From 7020653250c54f05849095d2d200c21b7f0e57dc Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sun, 20 Sep 2015 15:32:47 +0200 Subject: [PATCH 2/2] api: server: container: fix wrong comment This comment was wrongly referring to the old job mechanism and it wasn't clear what it was trying to document. Signed-off-by: Antonio Murdaca Upstream-commit: 76b3b5431475709b27f0c113f9319321388a49d1 Component: engine --- components/engine/api/server/container.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/components/engine/api/server/container.go b/components/engine/api/server/container.go index f74ae89356..15817e0089 100644 --- a/components/engine/api/server/container.go +++ b/components/engine/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")