Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

agent: report fs log errors as http errors #6427

Merged
merged 2 commits into from
Oct 16, 2019
Merged

agent: report fs log errors as http errors #6427

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09ce0e5
agent: report fs log errors as http errors
Oct 4, 2019
5282353
tests: avoid using unnecessary pipe
Oct 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 14 additions & 11 deletions command/agent/fs_endpoint.go
View file
Open in desktop
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be 412? Precondition Failed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

412 (Precondition Failed) seems applicable to conditional requests i.e. ones with If-Unmodified-Since [1][2][3].

[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/412
[2] https://stackoverflow.com/questions/5369480/when-is-it-appropriate-to-respond-with-a-http-412-error
[3]https://tools.ietf.org/html/rfc7232#section-4.2

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
50 changes: 37 additions & 13 deletions command/agent/fs_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,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 +190,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 +221,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 @@ -518,3 +521,24 @@ func TestHTTP_FS_Logs_Follow(t *testing.T) {
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())

p, _ := io.Pipe()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to close the PipeWriter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so; we don't seem to close it in other tests and it's an in memory struct with closed mostly using for signaling errors to other pipe.

Though after looking into it further, the pipe seems unnecessary, so I removed it from this tests and other tests in the file.

defer p.Close()

req, err := http.NewRequest("GET", path, p)
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)
})
}