From 3469d0ce77035db2ac31b8fab8ba4b4b97da4fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Pa=C3=9F?= <22845248+mpass99@users.noreply.github.com> Date: Tue, 30 Aug 2022 14:14:24 +0200 Subject: [PATCH] Specify http not found exit code by replacing it with StatusGone (410) for a missing runner and StatusFailedDependency (424) for missing or not accessible files. --- api/swagger.yaml | 76 ++++++++++++++++++++++----------- internal/api/environments.go | 12 +++--- internal/api/helpers.go | 10 ++--- internal/api/runners.go | 14 +++--- internal/api/runners_test.go | 14 +++--- internal/api/websocket.go | 2 +- internal/runner/nomad_runner.go | 2 +- tests/e2e/runners_test.go | 14 +++--- 8 files changed, 81 insertions(+), 63 deletions(-) diff --git a/api/swagger.yaml b/api/swagger.yaml index d1b5c45..52f7712 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -78,6 +78,16 @@ components: - size - modificationTime additionalProperties: false + ClientError: + type: object + properties: + message: + description: Explanation on why the request could not be handled + type: string + example: Nomad server unreachable + required: + - message + additionalProperties: false responses: BadRequest: @@ -94,26 +104,36 @@ components: description: Client could not be authenticated NotFound: description: The entity with the given identifier does not exist. + RunnerGone: + description: The runner is not available any longer. + content: + application/json: + schema: + $ref: "#/components/schemas/ClientError" + FailedFileDependency: + description: The file is not available. + content: + application/json: + schema: + $ref: "#/components/schemas/ClientError" InternalServerError: description: Request could not be handled content: application/json: schema: - type: object - properties: - message: - description: Explanation on why the request could not be handled - type: string - example: Nomad server unreachable - errorCode: - description: Machine readable error description - type: string - enum: - - NOMAD_UNREACHABLE - - NOMAD_OVERLOAD - - NOMAD_INTERNAL_SERVER_ERROR - - UNKNOWN - example: NOMAD_UNREACHABLE + allOf: + - $ref: "#/components/schemas/ClientError" + - type: object + properties: + errorCode: + description: Machine readable error description + type: string + enum: + - NOMAD_UNREACHABLE + - NOMAD_OVERLOAD + - NOMAD_INTERNAL_SERVER_ERROR + - UNKNOWN + example: NOMAD_UNREACHABLE tags: - name: runner @@ -275,8 +295,8 @@ paths: description: Success "401": $ref: "#/components/responses/Unauthorized" - "404": - $ref: "#/components/responses/NotFound" + "410": + $ref: "#/components/responses/RunnerGone" "500": $ref: "#/components/responses/InternalServerError" @@ -307,6 +327,7 @@ paths: description: Specify the directory from where the filesystem is listed. schema: type: string + format: pct-encoded # rfc 3986 default: "./" required: false responses: @@ -324,8 +345,10 @@ paths: $ref: "#/components/schemas/FileHeader" "401": $ref: "#/components/responses/Unauthorized" - "404": - $ref: "#/components/responses/NotFound" + "410": + $ref: "#/components/responses/RunnerGone" + "424": + $ref: "#/components/responses/FailedFileDependency" "500": $ref: "#/components/responses/InternalServerError" patch: @@ -373,8 +396,8 @@ paths: $ref: "#/components/responses/BadRequest" "401": $ref: "#/components/responses/Unauthorized" - "404": - $ref: "#/components/responses/NotFound" + "410": + $ref: "#/components/responses/RunnerGone" "500": $ref: "#/components/responses/InternalServerError" @@ -397,6 +420,7 @@ paths: description: Specify the file that should be returned by its filename including its path and extension. schema: type: string + format: pct-encoded # rfc 3986 example: "./flag.txt" required: true responses: @@ -409,8 +433,10 @@ paths: format: binary "401": $ref: "#/components/responses/Unauthorized" - "404": - $ref: "#/components/responses/NotFound" + "410": + $ref: "#/components/responses/RunnerGone" + "424": + $ref: "#/components/responses/FailedFileDependency" /runners/{runnerId}/execute: post: @@ -475,8 +501,8 @@ paths: $ref: "#/components/responses/BadRequest" "401": $ref: "#/components/responses/Unauthorized" - "404": - $ref: "#/components/responses/NotFound" + "410": + $ref: "#/components/responses/RunnerGone" "500": $ref: "#/components/responses/InternalServerError" diff --git a/internal/api/environments.go b/internal/api/environments.go index 11ec34f..14abcab 100644 --- a/internal/api/environments.go +++ b/internal/api/environments.go @@ -45,7 +45,7 @@ func (e *EnvironmentController) ConfigureRoutes(router *mux.Router) { func (e *EnvironmentController) list(writer http.ResponseWriter, request *http.Request) { fetch, err := parseFetchParameter(request) if err != nil { - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return } @@ -63,12 +63,12 @@ func (e *EnvironmentController) get(writer http.ResponseWriter, request *http.Re environmentID, err := parseEnvironmentID(request) if err != nil { // This case is never used as the router validates the id format - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return } fetch, err := parseFetchParameter(request) if err != nil { - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return } @@ -89,7 +89,7 @@ func (e *EnvironmentController) delete(writer http.ResponseWriter, request *http environmentID, err := parseEnvironmentID(request) if err != nil { // This case is never used as the router validates the id format - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return } @@ -109,12 +109,12 @@ func (e *EnvironmentController) delete(writer http.ResponseWriter, request *http func (e *EnvironmentController) createOrUpdate(writer http.ResponseWriter, request *http.Request) { req := new(dto.ExecutionEnvironmentRequest) if err := json.NewDecoder(request.Body).Decode(req); err != nil { - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return } environmentID, err := parseEnvironmentID(request) if err != nil { - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return } diff --git a/internal/api/helpers.go b/internal/api/helpers.go index 7349145..fb540ca 100644 --- a/internal/api/helpers.go +++ b/internal/api/helpers.go @@ -11,12 +11,8 @@ func writeInternalServerError(writer http.ResponseWriter, err error, errorCode d sendJSON(writer, &dto.InternalServerError{Message: err.Error(), ErrorCode: errorCode}, http.StatusInternalServerError) } -func writeBadRequest(writer http.ResponseWriter, err error) { - sendJSON(writer, &dto.ClientError{Message: err.Error()}, http.StatusBadRequest) -} - -func writeNotFound(writer http.ResponseWriter, err error) { - sendJSON(writer, &dto.ClientError{Message: err.Error()}, http.StatusNotFound) +func writeClientError(writer http.ResponseWriter, err error, status uint16) { + sendJSON(writer, &dto.ClientError{Message: err.Error()}, int(status)) } func sendJSON(writer http.ResponseWriter, content interface{}, httpStatusCode int) { @@ -36,7 +32,7 @@ func sendJSON(writer http.ResponseWriter, content interface{}, httpStatusCode in func parseJSONRequestBody(writer http.ResponseWriter, request *http.Request, structure interface{}) error { if err := json.NewDecoder(request.Body).Decode(structure); err != nil { - writeBadRequest(writer, err) + writeClientError(writer, err, http.StatusBadRequest) return fmt.Errorf("error parsing JSON request body: %w", err) } return nil diff --git a/internal/api/runners.go b/internal/api/runners.go index 8e8bc4c..874951c 100644 --- a/internal/api/runners.go +++ b/internal/api/runners.go @@ -65,7 +65,7 @@ func (r *RunnerController) provide(writer http.ResponseWriter, request *http.Req if err != nil { switch { case errors.Is(err, runner.ErrUnknownExecutionEnvironment): - writeNotFound(writer, err) + writeClientError(writer, err, http.StatusNotFound) case errors.Is(err, runner.ErrNoRunnersAvailable): log.WithField("environment", logging.RemoveNewlineSymbol(strconv.Itoa(int(environmentID)))). Warn("No runners available") @@ -97,7 +97,7 @@ func (r *RunnerController) listFileSystem(writer http.ResponseWriter, request *h writer.Header().Set("Content-Type", "application/json") err = targetRunner.ListFileSystem(path, recursive, writer, request.Context()) if errors.Is(err, runner.ErrFileNotFound) { - writeNotFound(writer, err) + writeClientError(writer, err, http.StatusFailedDependency) return } else if err != nil { log.WithError(err).Error("Could not perform the requested listFileSystem.") @@ -133,7 +133,7 @@ func (r *RunnerController) fileContent(writer http.ResponseWriter, request *http writer.Header().Set("Content-Type", "application/octet-stream") err := targetRunner.GetFileContent(path, writer, request.Context()) if errors.Is(err, runner.ErrFileNotFound) { - writeNotFound(writer, err) + writeClientError(writer, err, http.StatusFailedDependency) return } else if err != nil { log.WithError(err).Error("Could not retrieve the requested file.") @@ -197,7 +197,7 @@ func (r *RunnerController) findRunnerMiddleware(next http.Handler) http.Handler if readErr != nil { log.WithError(readErr).Warn("Failed to discard the request body") } - writeNotFound(writer, err) + writeClientError(writer, err, http.StatusGone) return } ctx := runner.NewContext(request.Context(), targetRunner) @@ -214,11 +214,7 @@ func (r *RunnerController) delete(writer http.ResponseWriter, request *http.Requ err := r.manager.Return(targetRunner) if err != nil { - if errors.Is(err, runner.ErrUnknownExecutionEnvironment) { - writeNotFound(writer, err) - } else { - writeInternalServerError(writer, err, dto.ErrorNomadInternalServerError) - } + writeInternalServerError(writer, err, dto.ErrorNomadInternalServerError) return } diff --git a/internal/api/runners_test.go b/internal/api/runners_test.go index 9aec028..257fdcc 100644 --- a/internal/api/runners_test.go +++ b/internal/api/runners_test.go @@ -76,7 +76,7 @@ func (s *MiddlewareTestSuite) TestFindRunnerMiddlewareIfRunnerDoesNotExist() { recorder := httptest.NewRecorder() s.router.ServeHTTP(recorder, s.runnerRequest(invalidID)) - s.Equal(http.StatusNotFound, recorder.Code) + s.Equal(http.StatusGone, recorder.Code) } func (s *MiddlewareTestSuite) TestFindRunnerMiddlewareDoesNotEarlyRespond() { @@ -91,7 +91,7 @@ func (s *MiddlewareTestSuite) TestFindRunnerMiddlewareDoesNotEarlyRespond() { recorder := httptest.NewRecorder() s.router.ServeHTTP(recorder, request) - s.Equal(http.StatusNotFound, recorder.Code) + s.Equal(http.StatusGone, recorder.Code) s.Equal(0, body.Len()) // No data should be unread } @@ -283,7 +283,7 @@ func (s *UpdateFileSystemRouteTestSuite) TestUpdateFileSystemReturnsBadRequestOn s.Equal(http.StatusBadRequest, s.recorder.Code) } -func (s *UpdateFileSystemRouteTestSuite) TestUpdateFileSystemToNonExistingRunnerReturnsNotFound() { +func (s *UpdateFileSystemRouteTestSuite) TestUpdateFileSystemToNonExistingRunnerReturnsGone() { s.runnerManager.On("Get", invalidID).Return(nil, runner.ErrRunnerNotFound) path, err := s.router.Get(UpdateFileSystemPath).URL(RunnerIDKey, invalidID) s.Require().NoError(err) @@ -294,7 +294,7 @@ func (s *UpdateFileSystemRouteTestSuite) TestUpdateFileSystemToNonExistingRunner s.Require().NoError(err) s.router.ServeHTTP(s.recorder, request) - s.Equal(http.StatusNotFound, s.recorder.Code) + s.Equal(http.StatusGone, s.recorder.Code) } func (s *UpdateFileSystemRouteTestSuite) TestUpdateFileSystemReturnsInternalServerErrorWhenCopyFailed() { @@ -383,7 +383,7 @@ func (s *UpdateFileSystemRouteTestSuite) TestFileContent() { request, err := http.NewRequest(http.MethodGet, routeURL.String(), strings.NewReader("")) s.Require().NoError(err) s.router.ServeHTTP(s.recorder, request) - s.Equal(http.StatusNotFound, s.recorder.Code) + s.Equal(http.StatusFailedDependency, s.recorder.Code) }) s.recorder = httptest.NewRecorder() @@ -449,7 +449,7 @@ func (s *DeleteRunnerRouteTestSuite) TestReturnInternalServerErrorWhenApiCallToN s.Equal(http.StatusInternalServerError, recorder.Code) } -func (s *DeleteRunnerRouteTestSuite) TestDeleteInvalidRunnerIdReturnsNotFound() { +func (s *DeleteRunnerRouteTestSuite) TestDeleteInvalidRunnerIdReturnsGone() { s.runnerManager.On("Get", mock.AnythingOfType("string")).Return(nil, tests.ErrDefault) deleteURL, err := s.router.Get(DeleteRoute).URL(RunnerIDKey, "1nv4l1dID") s.Require().NoError(err) @@ -461,5 +461,5 @@ func (s *DeleteRunnerRouteTestSuite) TestDeleteInvalidRunnerIdReturnsNotFound() s.router.ServeHTTP(recorder, request) - s.Equal(http.StatusNotFound, recorder.Code) + s.Equal(http.StatusGone, recorder.Code) } diff --git a/internal/api/websocket.go b/internal/api/websocket.go index 4d67737..f31f1c2 100644 --- a/internal/api/websocket.go +++ b/internal/api/websocket.go @@ -80,7 +80,7 @@ func (r *RunnerController) connectToRunner(writer http.ResponseWriter, request * executionID := request.URL.Query().Get(ExecutionIDKey) if !targetRunner.ExecutionExists(executionID) { - writeNotFound(writer, ErrUnknownExecutionID) + writeClientError(writer, ErrUnknownExecutionID, http.StatusNotFound) return } diff --git a/internal/runner/nomad_runner.go b/internal/runner/nomad_runner.go index 4db3014..d7d3442 100644 --- a/internal/runner/nomad_runner.go +++ b/internal/runner/nomad_runner.go @@ -32,7 +32,7 @@ const ( var ( ErrorUnknownExecution = errors.New("unknown execution") ErrorFileCopyFailed = errors.New("file copy failed") - ErrFileNotFound = errors.New("file not found") + ErrFileNotFound = errors.New("file not found or insufficient permissions") ) // NomadJob is an abstraction to communicate with Nomad environments. diff --git a/tests/e2e/runners_test.go b/tests/e2e/runners_test.go index 9dde5e4..acd72ad 100644 --- a/tests/e2e/runners_test.go +++ b/tests/e2e/runners_test.go @@ -104,16 +104,16 @@ func (s *E2ETestSuite) TestDeleteRunnerRoute() { s.Equal(http.StatusNoContent, resp.StatusCode) }) - s.Run("Deleting it again returns NotFound", func() { + s.Run("Deleting it again returns Gone", func() { resp, err := helpers.HTTPDelete(helpers.BuildURL(api.BasePath, api.RunnersPath, runnerID), nil) s.NoError(err) - s.Equal(http.StatusNotFound, resp.StatusCode) + s.Equal(http.StatusGone, resp.StatusCode) }) - s.Run("Deleting non-existing runner returns NotFound", func() { + s.Run("Deleting non-existing runner returns Gone", func() { resp, err := helpers.HTTPDelete(helpers.BuildURL(api.BasePath, api.RunnersPath, tests.NonExistingStringID), nil) s.NoError(err) - s.Equal(http.StatusNotFound, resp.StatusCode) + s.Equal(http.StatusGone, resp.StatusCode) }) }) } @@ -249,10 +249,10 @@ func (s *E2ETestSuite) TestCopyFilesRoute() { s.Equal(http.StatusBadRequest, resp.StatusCode) }) - s.Run("Copying to non-existing runner returns NotFound", func() { + s.Run("Copying to non-existing runner returns Gone", func() { resp, err := CopyFiles(tests.NonExistingStringID, request) s.NoError(err) - s.Equal(http.StatusNotFound, resp.StatusCode) + s.Equal(http.StatusGone, resp.StatusCode) }) }) } @@ -333,7 +333,7 @@ func (s *E2ETestSuite) TestGetFileContent_Nomad() { getFileURL.RawQuery = fmt.Sprintf("%s=%s", api.PathKey, tests.DefaultFileName) response, err := http.Get(getFileURL.String()) s.Require().NoError(err) - s.Equal(http.StatusNotFound, response.StatusCode) + s.Equal(http.StatusFailedDependency, response.StatusCode) }) s.Run("Ok", func() {