From e729133134472dd01ffbf13d6917d91cc4b97d7f Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 17 Nov 2021 11:15:07 -0500 Subject: [PATCH] api: return 404 for alloc FS list/stat endpoints (#11482) * api: return 404 for alloc FS list/stat endpoints If the alloc filesystem doesn't have a file requested by the List Files or Stat File API, we currently return a HTTP 500 error with the expected "file not found" error message. Return a HTTP 404 error instead. * update FS Handler Previously the FS handler would interpret a 500 status as a 404 in the adapter layer by checking if the response body contained the text or is the response status was 500 and then throw an error code for 404. Co-authored-by: Jai Bhagat --- .changelog/11482.txt | 3 +++ command/agent/fs_endpoint.go | 4 ++-- nomad/structs/errors.go | 4 ++++ ui/app/adapters/allocation.js | 8 +------- ui/tests/acceptance/behaviors/fs.js | 6 ++++-- 5 files changed, 14 insertions(+), 11 deletions(-) create mode 100644 .changelog/11482.txt diff --git a/.changelog/11482.txt b/.changelog/11482.txt new file mode 100644 index 000000000..923eb9956 --- /dev/null +++ b/.changelog/11482.txt @@ -0,0 +1,3 @@ +```release-note:improvement +api: Return a HTTP 404 instead of a HTTP 500 from the Stat File and List Files API endpoints when a file or directory is not found. +``` diff --git a/command/agent/fs_endpoint.go b/command/agent/fs_endpoint.go index a186b7da9..d45a7ce85 100644 --- a/command/agent/fs_endpoint.go +++ b/command/agent/fs_endpoint.go @@ -80,7 +80,7 @@ func (s *HTTPServer) DirectoryListRequest(resp http.ResponseWriter, req *http.Re } if rpcErr != nil { - if structs.IsErrNoNodeConn(rpcErr) || structs.IsErrUnknownAllocation(rpcErr) { + if structs.IsErrNoNodeConn(rpcErr) || structs.IsErrUnknownAllocation(rpcErr) || structs.IsErrNoSuchFileOrDirectory(rpcErr) { rpcErr = CodedError(404, rpcErr.Error()) } @@ -120,7 +120,7 @@ func (s *HTTPServer) FileStatRequest(resp http.ResponseWriter, req *http.Request } if rpcErr != nil { - if structs.IsErrNoNodeConn(rpcErr) || structs.IsErrUnknownAllocation(rpcErr) { + if structs.IsErrNoNodeConn(rpcErr) || structs.IsErrUnknownAllocation(rpcErr) || structs.IsErrNoSuchFileOrDirectory(rpcErr) { rpcErr = CodedError(404, rpcErr.Error()) } diff --git a/nomad/structs/errors.go b/nomad/structs/errors.go index a8815f757..7a71708f9 100644 --- a/nomad/structs/errors.go +++ b/nomad/structs/errors.go @@ -177,6 +177,10 @@ func IsErrNodeLacksRpc(err error) bool { return err != nil && strings.Contains(err.Error(), errNodeLacksRpc) } +func IsErrNoSuchFileOrDirectory(err error) bool { + return err != nil && strings.Contains(err.Error(), "no such file or directory") +} + // NewErrRPCCoded wraps an RPC error with a code to be converted to HTTP status // code func NewErrRPCCoded(code int, msg string) error { diff --git a/ui/app/adapters/allocation.js b/ui/app/adapters/allocation.js index 20cca3c29..3fcfb6128 100644 --- a/ui/app/adapters/allocation.js +++ b/ui/app/adapters/allocation.js @@ -31,14 +31,8 @@ async function handleFSResponse(response) { } else { const body = await response.text(); - // TODO update this if/when endpoint returns 404 as expected - const statusIs500 = response.status === 500; - const bodyIncludes404Text = body.includes('no such file or directory'); - - const translatedCode = statusIs500 && bodyIncludes404Text ? 404 : response.status; - throw { - code: translatedCode, + code: response.status, toString: () => body, }; } diff --git a/ui/tests/acceptance/behaviors/fs.js b/ui/tests/acceptance/behaviors/fs.js index 114078261..6a1665075 100644 --- a/ui/tests/acceptance/behaviors/fs.js +++ b/ui/tests/acceptance/behaviors/fs.js @@ -361,7 +361,8 @@ export default function browseFilesystem({ ...visitSegments({ allocation: this.allocation, task: this.task }), path: '/what-is-this', }); - assert.equal(FS.error.title, 'Not Found', '500 is interpreted as 404'); + assert.notEqual(FS.error.title, 'Not Found', '500 is not interpreted as 404'); + assert.equal(FS.error.title, 'Server Error', '500 is not interpreted as 500'); await visit('/'); @@ -385,7 +386,8 @@ export default function browseFilesystem({ ...visitSegments({ allocation: this.allocation, task: this.task }), path: this.directory.name, }); - assert.equal(FS.error.title, 'Not Found', '500 is interpreted as 404'); + assert.notEqual(FS.error.title, 'Not Found', '500 is not interpreted as 404'); + assert.equal(FS.error.title, 'Server Error', '500 is not interpreted as 404'); await visit('/');