From 3b9c966157962787ec4e03bb3ca3c7093870b48b Mon Sep 17 00:00:00 2001 From: Peter Bacsko Date: Mon, 27 Nov 2023 16:47:25 +1100 Subject: [PATCH] [YUNIKORN-2163] Fix HTTP status codes in REST handlers (#729) An object does not exist: 400 Bad Request --> 404 Not Found Internal metrics is disabled: 501 Not Implemented --> 500 Internal Server Error Event tracking disabled: 400 Bad Request --> 500 Internal Server Error removal of dead code for writing a new config Closes: #729 Signed-off-by: Wilfred Spiegelenburg --- pkg/webservice/handlers.go | 47 ++++++++++---------------- pkg/webservice/handlers_test.go | 59 +++++++++++---------------------- 2 files changed, 37 insertions(+), 69 deletions(-) diff --git a/pkg/webservice/handlers.go b/pkg/webservice/handlers.go index 382dd230a..ee4ecf22b 100644 --- a/pkg/webservice/handlers.go +++ b/pkg/webservice/handlers.go @@ -358,7 +358,7 @@ func getNodeUtilisation(w http.ResponseWriter, r *http.Request) { writeHeaders(w) partitionContext := schedulerContext.GetPartitionWithoutClusterID(configs.DefaultPartition) if partitionContext == nil { - buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusInternalServerError) return } // calculate the dominant resource based on root queue usage and size @@ -422,7 +422,7 @@ func getApplicationHistory(w http.ResponseWriter, r *http.Request) { // There is nothing to return but we did not really encounter a problem if imHistory == nil { - buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusNotImplemented) + buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusInternalServerError) return } // get a copy of the records: if the array contains nil values they will always be at the @@ -439,7 +439,7 @@ func getContainerHistory(w http.ResponseWriter, r *http.Request) { // There is nothing to return but we did not really encounter a problem if imHistory == nil { - buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusNotImplemented) + buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusInternalServerError) return } // get a copy of the records: if the array contains nil values they will always be at the @@ -507,20 +507,7 @@ func checkHealthStatus(w http.ResponseWriter, r *http.Request) { } } -func buildUpdateResponse(err error, w http.ResponseWriter) { - if err == nil { - w.WriteHeader(http.StatusOK) - if _, err = w.Write([]byte("Configuration updated successfully")); err != nil { - buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError) - } - } else { - log.Log(log.REST).Info("Configuration update failed with errors", - zap.Error(err)) - buildJSONErrorResponse(w, err.Error(), http.StatusConflict) - } -} - -func getPartitions(w http.ResponseWriter, r *http.Request) { +func getPartitions(w http.ResponseWriter, _ *http.Request) { writeHeaders(w) lists := schedulerContext.GetPartitionMapClone() @@ -543,7 +530,7 @@ func getPartitionQueues(w http.ResponseWriter, r *http.Request) { if partition != nil { partitionQueuesDAOInfo = partition.GetPartitionQueues() } else { - buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound) return } if err := json.NewEncoder(w).Encode(partitionQueuesDAOInfo); err != nil { @@ -566,7 +553,7 @@ func getPartitionNodes(w http.ResponseWriter, r *http.Request) { buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError) } } else { - buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound) } } @@ -583,7 +570,7 @@ func getPartitionNode(w http.ResponseWriter, r *http.Request) { nodeID := vars.ByName("node") node := partitionContext.GetNode(nodeID) if node == nil { - buildJSONErrorResponse(w, NodeDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, NodeDoesNotExists, http.StatusNotFound) return } nodeDao := getNodeDAO(node) @@ -591,7 +578,7 @@ func getPartitionNode(w http.ResponseWriter, r *http.Request) { buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError) } } else { - buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound) } } @@ -611,12 +598,12 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) { } partitionContext := schedulerContext.GetPartitionWithoutClusterID(partition) if partitionContext == nil { - buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound) return } queue := partitionContext.GetQueue(queueName) if queue == nil { - buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusNotFound) return } @@ -647,17 +634,17 @@ func getApplication(w http.ResponseWriter, r *http.Request) { } partitionContext := schedulerContext.GetPartitionWithoutClusterID(partition) if partitionContext == nil { - buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound) return } queue := partitionContext.GetQueue(queueName) if queue == nil { - buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusNotFound) return } app := queue.GetApplication(application) if app == nil { - buildJSONErrorResponse(w, ApplicationDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, ApplicationDoesNotExists, http.StatusNotFound) return } appDao := getApplicationDAO(app) @@ -830,7 +817,7 @@ func getMetrics(w http.ResponseWriter, r *http.Request) { promhttp.Handler().ServeHTTP(w, r) } -func getUsersResourceUsage(w http.ResponseWriter, r *http.Request) { +func getUsersResourceUsage(w http.ResponseWriter, _ *http.Request) { writeHeaders(w) userManager := ugm.GetUserManager() usersResources := userManager.GetUsersResources() @@ -857,7 +844,7 @@ func getUserResourceUsage(w http.ResponseWriter, r *http.Request) { } userTracker := ugm.GetUserManager().GetUserTracker(user) if userTracker == nil { - buildJSONErrorResponse(w, UserDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, UserDoesNotExists, http.StatusNotFound) return } var result = userTracker.GetUserResourceUsageDAOInfo() @@ -893,7 +880,7 @@ func getGroupResourceUsage(w http.ResponseWriter, r *http.Request) { } groupTracker := ugm.GetUserManager().GetGroupTracker(group) if groupTracker == nil { - buildJSONErrorResponse(w, GroupDoesNotExists, http.StatusBadRequest) + buildJSONErrorResponse(w, GroupDoesNotExists, http.StatusNotFound) return } var result = groupTracker.GetGroupResourceUsageDAOInfo() @@ -906,7 +893,7 @@ func getEvents(w http.ResponseWriter, r *http.Request) { writeHeaders(w) eventSystem := events.GetEventSystem() if !eventSystem.IsEventTrackingEnabled() { - buildJSONErrorResponse(w, "Event tracking is disabled", http.StatusBadRequest) + buildJSONErrorResponse(w, "Event tracking is disabled", http.StatusInternalServerError) return } diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index d67c63e0a..0d9766873 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -386,9 +386,9 @@ func TestApplicationHistory(t *testing.T) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusNotImplemented, resp.statusCode, "app history handler returned wrong status") + assert.Equal(t, http.StatusInternalServerError, resp.statusCode, "app history handler returned wrong status") assert.Equal(t, errInfo.Message, "Internal metrics collection is not enabled.", jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusNotImplemented) + assert.Equal(t, errInfo.StatusCode, http.StatusInternalServerError) // init should return null and thus no records imHistory = history.NewInternalMetricsHistory(5) @@ -441,9 +441,9 @@ func TestContainerHistory(t *testing.T) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusNotImplemented, resp.statusCode, "container history handler returned wrong status") + assert.Equal(t, http.StatusInternalServerError, resp.statusCode, "container history handler returned wrong status") assert.Equal(t, errInfo.Message, "Internal metrics collection is not enabled.", jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusNotImplemented) + assert.Equal(t, errInfo.StatusCode, http.StatusInternalServerError) // init should return null and thus no records imHistory = history.NewInternalMetricsHistory(5) @@ -550,25 +550,6 @@ func TestGetConfigJSON(t *testing.T) { configs.SetConfigMap(map[string]string{}) } -func TestBuildUpdateResponseSuccess(t *testing.T) { - resp := &MockResponseWriter{} - buildUpdateResponse(nil, resp) - assert.Equal(t, http.StatusOK, resp.statusCode, "Response should be OK") -} - -func TestBuildUpdateResponseFailure(t *testing.T) { - resp := &MockResponseWriter{} - err := fmt.Errorf("ConfigMapUpdate failed") - buildUpdateResponse(err, resp) - - var errInfo dao.YAPIError - err1 := json.Unmarshal(resp.outputBytes, &errInfo) - assert.NilError(t, err1, unmarshalError) - assert.Equal(t, http.StatusConflict, resp.statusCode, "Status code is wrong") - assert.Assert(t, strings.Contains(string(errInfo.Message), err.Error()), "Error message should contain the reason") - assert.Equal(t, errInfo.StatusCode, http.StatusConflict) -} - func TestGetClusterUtilJSON(t *testing.T) { setup(t, configDefault, 1) @@ -1308,36 +1289,36 @@ func assertPartitionExists(t *testing.T, resp *MockResponseWriter) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError) + assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError) assert.Equal(t, errInfo.Message, PartitionDoesNotExists, jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest) + assert.Equal(t, errInfo.StatusCode, http.StatusNotFound) } func assertQueueExists(t *testing.T, resp *MockResponseWriter) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError) + assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError) assert.Equal(t, errInfo.Message, QueueDoesNotExists, jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest) + assert.Equal(t, errInfo.StatusCode, http.StatusNotFound) } func assertApplicationExists(t *testing.T, resp *MockResponseWriter) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError) + assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError) assert.Equal(t, errInfo.Message, ApplicationDoesNotExists, jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest) + assert.Equal(t, errInfo.StatusCode, http.StatusNotFound) } func assertUserExists(t *testing.T, resp *MockResponseWriter) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError) + assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError) assert.Equal(t, errInfo.Message, UserDoesNotExists, jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest) + assert.Equal(t, errInfo.StatusCode, http.StatusNotFound) } func assertUserNameExists(t *testing.T, resp *MockResponseWriter) { @@ -1353,9 +1334,9 @@ func assertGroupExists(t *testing.T, resp *MockResponseWriter) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError) + assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError) assert.Equal(t, errInfo.Message, GroupDoesNotExists, jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest) + assert.Equal(t, errInfo.StatusCode, http.StatusNotFound) } func assertGroupNameExists(t *testing.T, resp *MockResponseWriter) { @@ -1371,9 +1352,9 @@ func assertNodeIDExists(t *testing.T, resp *MockResponseWriter) { var errInfo dao.YAPIError err := json.Unmarshal(resp.outputBytes, &errInfo) assert.NilError(t, err, unmarshalError) - assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError) + assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError) assert.Equal(t, errInfo.Message, NodeDoesNotExists, jsonMessageError) - assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest) + assert.Equal(t, errInfo.StatusCode, http.StatusNotFound) } func TestValidateQueue(t *testing.T) { @@ -1561,7 +1542,7 @@ func TestGetEventsWhenTrackingDisabled(t *testing.T) { req, err := http.NewRequest("GET", "/ws/v1/events/batch", strings.NewReader("")) assert.NilError(t, err) - readIllegalRequest(t, req, "Event tracking is disabled") + readIllegalRequest(t, req, http.StatusInternalServerError, "Event tracking is disabled") } func addEvents(t *testing.T) (appEvent, nodeEvent, queueEvent *si.EventRecord) { @@ -1627,15 +1608,15 @@ func checkIllegalBatchRequest(t *testing.T, query, msg string) { t.Helper() req, err := http.NewRequest("GET", "/ws/v1/events/batch?"+query, strings.NewReader("")) assert.NilError(t, err) - readIllegalRequest(t, req, msg) + readIllegalRequest(t, req, http.StatusBadRequest, msg) } -func readIllegalRequest(t *testing.T, req *http.Request, errMsg string) { +func readIllegalRequest(t *testing.T, req *http.Request, statusCode int, errMsg string) { t.Helper() rr := httptest.NewRecorder() handler := http.HandlerFunc(getEvents) handler.ServeHTTP(rr, req) - assert.Equal(t, http.StatusBadRequest, rr.Code) + assert.Equal(t, statusCode, rr.Code) jsonBytes := make([]byte, 256) n, err := rr.Body.Read(jsonBytes) assert.NilError(t, err, "cannot read response body")