Skip to content

Commit

Permalink
Merge pull request #6427 from hashicorp/b-fs-endpoint-errors
Browse files Browse the repository at this point in the history 
agent: report fs log errors as http errors
  • Loading branch information
Mahmood Ali committed Oct 16, 2019
2 parents bf91e83 + 5282353 commit 31da091
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 41 deletions.
25 changes: 14 additions & 11 deletions command/agent/fs_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
}
}

Expand All @@ -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))
}
}

Expand Down Expand Up @@ -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 {
Expand Down
67 changes: 37 additions & 30 deletions command/agent/fs_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package agent
import (
"encoding/base64"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
Expand All @@ -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"
Expand Down Expand Up @@ -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")
})
}

Expand All @@ -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")
})
}

Expand Down Expand Up @@ -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{})
Expand Down Expand Up @@ -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()
})
}

Expand All @@ -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{})
Expand Down Expand Up @@ -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()
})
}

Expand All @@ -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() {
Expand All @@ -469,8 +463,6 @@ func TestHTTP_FS_Logs(t *testing.T) {
}, func(err error) {
t.Fatal(err)
})

p.Close()
})
}

Expand All @@ -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)
Expand All @@ -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):
}
})
}

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)

p.Close()
_, ok := err.(HTTPCodedError)
require.Truef(t, ok, "expected a coded error but found: %#+v", err)
})
}

0 comments on commit 31da091

Please sign in to comment.