From 216b4c9cf6cbdc9cd620de4edfbcd754a2184559 Mon Sep 17 00:00:00 2001 From: LK4D4 Date: Wed, 18 Jun 2014 23:11:04 +0400 Subject: [PATCH] Validate that one of streams choosen in logs on api side Fixes #6506 There is the bug, that very hard to fix: When we return job.Errorf in "logs" job it writes to job.Stderr, to which connected ResponseWriter and on this write w.WriteHeader(http.StatusOK) is called. So, we get 200 on error from "logs" job. Docker-DCO-1.1-Signed-off-by: Alexandr Morozov (github: LK4D4) --- api/server/server.go | 27 ++++++---- api/server/server_unit_test.go | 99 +++++++++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 13 deletions(-) diff --git a/api/server/server.go b/api/server/server.go index ce1bdbd39e..5feeaf9229 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -370,13 +370,23 @@ func getContainersLogs(eng *engine.Engine, version version.Version, w http.Respo } var ( - job = eng.Job("container_inspect", vars["name"]) - c, err = job.Stdout.AddEnv() + inspectJob = eng.Job("container_inspect", vars["name"]) + logsJob = eng.Job("logs", vars["name"]) + c, err = inspectJob.Stdout.AddEnv() ) if err != nil { return err } - if err = job.Run(); err != nil { + logsJob.Setenv("follow", r.Form.Get("follow")) + logsJob.Setenv("stdout", r.Form.Get("stdout")) + logsJob.Setenv("stderr", r.Form.Get("stderr")) + logsJob.Setenv("timestamps", r.Form.Get("timestamps")) + // Validate args here, because we can't return not StatusOK after job.Run() call + stdout, stderr := logsJob.GetenvBool("stdout"), logsJob.GetenvBool("stderr") + if !(stdout || stderr) { + return fmt.Errorf("Bad parameters: you must choose at least one stream") + } + if err = inspectJob.Run(); err != nil { return err } @@ -390,14 +400,9 @@ func getContainersLogs(eng *engine.Engine, version version.Version, w http.Respo errStream = outStream } - job = eng.Job("logs", vars["name"]) - job.Setenv("follow", r.Form.Get("follow")) - job.Setenv("stdout", r.Form.Get("stdout")) - job.Setenv("stderr", r.Form.Get("stderr")) - job.Setenv("timestamps", r.Form.Get("timestamps")) - job.Stdout.Add(outStream) - job.Stderr.Set(errStream) - if err := job.Run(); err != nil { + logsJob.Stdout.Add(outStream) + logsJob.Stderr.Set(errStream) + if err := logsJob.Run(); err != nil { fmt.Fprintf(outStream, "Error running logs job: %s\n", err) } return nil diff --git a/api/server/server_unit_test.go b/api/server/server_unit_test.go index 32f8e42b18..1116a0e562 100644 --- a/api/server/server_unit_test.go +++ b/api/server/server_unit_test.go @@ -4,12 +4,14 @@ import ( "bytes" "encoding/json" "fmt" - "github.com/dotcloud/docker/api" - "github.com/dotcloud/docker/engine" "io" "net/http" "net/http/httptest" + "strings" "testing" + + "github.com/dotcloud/docker/api" + "github.com/dotcloud/docker/engine" ) func TestGetBoolParam(t *testing.T) { @@ -151,6 +153,99 @@ func TestGetContainersByName(t *testing.T) { } } +func TestLogs(t *testing.T) { + eng := engine.New() + var inspect bool + var logs bool + eng.Register("container_inspect", func(job *engine.Job) engine.Status { + inspect = true + if len(job.Args) == 0 { + t.Fatal("Job arguments is empty") + } + if job.Args[0] != "test" { + t.Fatalf("Container name %s, must be test", job.Args[0]) + } + return engine.StatusOK + }) + expected := "logs" + eng.Register("logs", func(job *engine.Job) engine.Status { + logs = true + if len(job.Args) == 0 { + t.Fatal("Job arguments is empty") + } + if job.Args[0] != "test" { + t.Fatalf("Container name %s, must be test", job.Args[0]) + } + follow := job.Getenv("follow") + if follow != "1" { + t.Fatalf("follow: %s, must be 1", follow) + } + stdout := job.Getenv("stdout") + if stdout != "1" { + t.Fatalf("stdout %s, must be 1", stdout) + } + stderr := job.Getenv("stderr") + if stderr != "" { + t.Fatalf("stderr %s, must be empty", stderr) + } + timestamps := job.Getenv("timestamps") + if timestamps != "1" { + t.Fatalf("timestamps %s, must be 1", timestamps) + } + job.Stdout.Write([]byte(expected)) + return engine.StatusOK + }) + r := serveRequest("GET", "/containers/test/logs?follow=1&stdout=1×tamps=1", nil, eng, t) + if r.Code != http.StatusOK { + t.Fatalf("Got status %d, expected %d", r.Code, http.StatusOK) + } + if !inspect { + t.Fatal("container_inspect job was not called") + } + if !logs { + t.Fatal("logs job was not called") + } + res := r.Body.String() + if res != expected { + t.Fatalf("Output %s, expected %s", res, expected) + } +} + +func TestLogsNoStreams(t *testing.T) { + eng := engine.New() + var inspect bool + var logs bool + eng.Register("container_inspect", func(job *engine.Job) engine.Status { + inspect = true + if len(job.Args) == 0 { + t.Fatal("Job arguments is empty") + } + if job.Args[0] != "test" { + t.Fatalf("Container name %s, must be test", job.Args[0]) + } + return engine.StatusOK + }) + eng.Register("logs", func(job *engine.Job) engine.Status { + logs = true + return engine.StatusOK + }) + r := serveRequest("GET", "/containers/test/logs", nil, eng, t) + if r.Code != http.StatusBadRequest { + t.Fatalf("Got status %d, expected %d", r.Code, http.StatusBadRequest) + } + if inspect { + t.Fatal("container_inspect job was called, but it shouldn't") + } + if logs { + t.Fatal("logs job was called, but it shouldn't") + } + res := strings.TrimSpace(r.Body.String()) + expected := "Bad parameters: you must choose at least one stream" + if !strings.Contains(res, expected) { + t.Fatalf("Output %s, expected %s in it", res, expected) + } +} + func serveRequest(method, target string, body io.Reader, eng *engine.Engine, t *testing.T) *httptest.ResponseRecorder { r := httptest.NewRecorder() req, err := http.NewRequest(method, target, body)