diff --git a/command/agent/fs_endpoint.go b/command/agent/fs_endpoint.go index f36f3be8e..4a99a2531 100644 --- a/command/agent/fs_endpoint.go +++ b/command/agent/fs_endpoint.go @@ -17,12 +17,12 @@ import ( ) var ( - allocIDNotPresentErr = fmt.Errorf("must provide a valid alloc id") - fileNameNotPresentErr = fmt.Errorf("must provide a file name") - taskNotPresentErr = fmt.Errorf("must provide task name") - logTypeNotPresentErr = fmt.Errorf("must provide log type (stdout/stderr)") - clientNotRunning = fmt.Errorf("node is not running a Nomad Client") - invalidOrigin = fmt.Errorf("origin must be start or end") + allocIDNotPresentErr = CodedError(400, "must provide a valid alloc id") + fileNameNotPresentErr = CodedError(400, "must provide a file name") + taskNotPresentErr = CodedError(400, "must provide task name") + logTypeNotPresentErr = CodedError(400, "must provide log type (stdout/stderr)") + clientNotRunning = CodedError(400, "node is not running a Nomad Client") + invalidOrigin = CodedError(400, "origin must be start or end") ) func (s *HTTPServer) FsRequest(resp http.ResponseWriter, req *http.Request) (interface{}, error) { @@ -273,13 +273,13 @@ func (s *HTTPServer) Logs(resp http.ResponseWriter, req *http.Request) (interfac if followStr := q.Get("follow"); followStr != "" { if follow, err = strconv.ParseBool(followStr); err != nil { - return nil, fmt.Errorf("failed to parse follow field to boolean: %v", err) + return nil, CodedError(400, fmt.Sprintf("failed to parse follow field to boolean: %v", err)) } } if plainStr := q.Get("plain"); plainStr != "" { if plain, err = strconv.ParseBool(plainStr); err != nil { - return nil, fmt.Errorf("failed to parse plain field to boolean: %v", err) + return nil, CodedError(400, fmt.Sprintf("failed to parse plain field to boolean: %v", err)) } } @@ -295,7 +295,7 @@ func (s *HTTPServer) Logs(resp http.ResponseWriter, req *http.Request) (interfac if offsetString != "" { var err error if offset, err = strconv.ParseInt(offsetString, 10, 64); err != nil { - return nil, fmt.Errorf("error parsing offset: %v", err) + return nil, CodedError(400, fmt.Sprintf("error parsing offset: %v", err)) } } @@ -388,10 +388,13 @@ func (s *HTTPServer) fsStreamImpl(resp http.ResponseWriter, decoder.Reset(httpPipe) if err := res.Error; err != nil { + code := 500 if err.Code != nil { - errCh <- CodedError(int(*err.Code), err.Error()) - return + code = int(*err.Code) } + + errCh <- CodedError(code, err.Error()) + return } if _, err := io.Copy(output, bytes.NewReader(res.Payload)); err != nil { diff --git a/command/agent/fs_endpoint_test.go b/command/agent/fs_endpoint_test.go index e131c990a..67cda129f 100644 --- a/command/agent/fs_endpoint_test.go +++ b/command/agent/fs_endpoint_test.go @@ -3,7 +3,6 @@ package agent import ( "encoding/base64" "fmt" - "io" "io/ioutil" "net/http" "net/http/httptest" @@ -12,6 +11,7 @@ import ( "time" cstructs "github.com/hashicorp/nomad/client/structs" + "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" @@ -189,25 +189,26 @@ func TestHTTP_FS_Stream_MissingParams(t *testing.T) { require := require.New(t) httpTest(t, nil, func(s *TestAgent) { req, err := http.NewRequest("GET", "/v1/client/fs/stream/", nil) - require.Nil(err) + require.NoError(err) respW := httptest.NewRecorder() _, err = s.Server.Stream(respW, req) require.EqualError(err, allocIDNotPresentErr.Error()) req, err = http.NewRequest("GET", "/v1/client/fs/stream/foo", nil) - require.Nil(err) + require.NoError(err) respW = httptest.NewRecorder() _, err = s.Server.Stream(respW, req) require.EqualError(err, fileNameNotPresentErr.Error()) req, err = http.NewRequest("GET", "/v1/client/fs/stream/foo?path=/path/to/file", nil) - require.Nil(err) + require.NoError(err) respW = httptest.NewRecorder() _, err = s.Server.Stream(respW, req) - require.Nil(err) + require.Error(err) + require.Contains(err.Error(), "alloc lookup failed") }) } @@ -219,38 +220,39 @@ func TestHTTP_FS_Logs_MissingParams(t *testing.T) { httpTest(t, nil, func(s *TestAgent) { // AllocID Not Present req, err := http.NewRequest("GET", "/v1/client/fs/logs/", nil) - require.Nil(err) + require.NoError(err) respW := httptest.NewRecorder() s.Server.mux.ServeHTTP(respW, req) require.Equal(respW.Body.String(), allocIDNotPresentErr.Error()) - require.Equal(500, respW.Code) // 500 for backward compat + require.Equal(400, respW.Code) // Task Not Present req, err = http.NewRequest("GET", "/v1/client/fs/logs/foo", nil) - require.Nil(err) + require.NoError(err) respW = httptest.NewRecorder() s.Server.mux.ServeHTTP(respW, req) require.Equal(respW.Body.String(), taskNotPresentErr.Error()) - require.Equal(500, respW.Code) // 500 for backward compat + require.Equal(400, respW.Code) // Log Type Not Present req, err = http.NewRequest("GET", "/v1/client/fs/logs/foo?task=foo", nil) - require.Nil(err) + require.NoError(err) respW = httptest.NewRecorder() s.Server.mux.ServeHTTP(respW, req) require.Equal(respW.Body.String(), logTypeNotPresentErr.Error()) - require.Equal(500, respW.Code) // 500 for backward compat + require.Equal(400, respW.Code) - // Ok + // case where all parameters are set but alloc isn't found req, err = http.NewRequest("GET", "/v1/client/fs/logs/foo?task=foo&type=stdout", nil) - require.Nil(err) + require.NoError(err) respW = httptest.NewRecorder() s.Server.mux.ServeHTTP(respW, req) - require.Equal(200, respW.Code) + require.Equal(500, respW.Code) + require.Contains(respW.Body.String(), "alloc lookup failed") }) } @@ -354,8 +356,7 @@ func TestHTTP_FS_Stream_NoFollow(t *testing.T) { path := fmt.Sprintf("/v1/client/fs/stream/%s?path=alloc/logs/web.stdout.0&offset=%d&origin=end&follow=false", a.ID, offset) - p, _ := io.Pipe() - req, err := http.NewRequest("GET", path, p) + req, err := http.NewRequest("GET", path, nil) require.Nil(err) respW := testutil.NewResponseRecorder() doneCh := make(chan struct{}) @@ -383,8 +384,6 @@ func TestHTTP_FS_Stream_NoFollow(t *testing.T) { case <-time.After(1 * time.Second): t.Fatal("should close but did not") } - - p.Close() }) } @@ -401,9 +400,7 @@ func TestHTTP_FS_Stream_Follow(t *testing.T) { path := fmt.Sprintf("/v1/client/fs/stream/%s?path=alloc/logs/web.stdout.0&offset=%d&origin=end", a.ID, offset) - p, _ := io.Pipe() - - req, err := http.NewRequest("GET", path, p) + req, err := http.NewRequest("GET", path, nil) require.Nil(err) respW := httptest.NewRecorder() doneCh := make(chan struct{}) @@ -431,8 +428,6 @@ func TestHTTP_FS_Stream_Follow(t *testing.T) { t.Fatal("shouldn't close") case <-time.After(1 * time.Second): } - - p.Close() }) } @@ -448,8 +443,7 @@ func TestHTTP_FS_Logs(t *testing.T) { path := fmt.Sprintf("/v1/client/fs/logs/%s?type=stdout&task=web&offset=%d&origin=end&plain=true", a.ID, offset) - p, _ := io.Pipe() - req, err := http.NewRequest("GET", path, p) + req, err := http.NewRequest("GET", path, nil) require.Nil(err) respW := testutil.NewResponseRecorder() go func() { @@ -469,8 +463,6 @@ func TestHTTP_FS_Logs(t *testing.T) { }, func(err error) { t.Fatal(err) }) - - p.Close() }) } @@ -486,8 +478,7 @@ func TestHTTP_FS_Logs_Follow(t *testing.T) { path := fmt.Sprintf("/v1/client/fs/logs/%s?type=stdout&task=web&offset=%d&origin=end&plain=true&follow=true", a.ID, offset) - p, _ := io.Pipe() - req, err := http.NewRequest("GET", path, p) + req, err := http.NewRequest("GET", path, nil) require.Nil(err) respW := testutil.NewResponseRecorder() errCh := make(chan error) @@ -514,7 +505,23 @@ func TestHTTP_FS_Logs_Follow(t *testing.T) { t.Fatalf("shouldn't exit: %v", err) case <-time.After(1 * time.Second): } - - p.Close() + }) +} + +func TestHTTP_FS_Logs_PropagatesErrors(t *testing.T) { + t.Parallel() + httpTest(t, nil, func(s *TestAgent) { + path := fmt.Sprintf("/v1/client/fs/logs/%s?type=stdout&task=web&offset=0&origin=end&plain=true", + uuid.Generate()) + + req, err := http.NewRequest("GET", path, nil) + require.NoError(t, err) + respW := testutil.NewResponseRecorder() + + _, err = s.Server.Logs(respW, req) + require.Error(t, err) + + _, ok := err.(HTTPCodedError) + require.Truef(t, ok, "expected a coded error but found: %#+v", err) }) }