From 4a13580a2fbbb5c7783f6fd544312b2689273304 Mon Sep 17 00:00:00 2001 From: "Arati R." <33031346+suntala@users.noreply.github.com> Date: Mon, 28 Oct 2024 12:33:56 +0100 Subject: [PATCH] K8s/Folders: Fix folder status error message (#95464) * Fix folder status error message * Add test for folder creation response message * Add TestFoldersCreateAPIEndpointK8S fixes * Fix message returned when user has no permissions --- pkg/api/apierrors/folder.go | 31 ++++++++++++++++----- pkg/api/folder.go | 9 ++++++- pkg/tests/apis/folder/folders_test.go | 39 ++++++++++++++++++++------- 3 files changed, 61 insertions(+), 18 deletions(-) diff --git a/pkg/api/apierrors/folder.go b/pkg/api/apierrors/folder.go index f05e35b6241..a5d544f1a4a 100644 --- a/pkg/api/apierrors/folder.go +++ b/pkg/api/apierrors/folder.go @@ -1,6 +1,7 @@ package apierrors import ( + "encoding/json" "errors" "net/http" @@ -48,20 +49,36 @@ func ToFolderErrorResponse(err error) response.Response { func ToFolderStatusError(err error) k8sErrors.StatusError { resp := ToFolderErrorResponse(err) + defaultErr := k8sErrors.StatusError{ + ErrStatus: metav1.Status{ + Message: "Folder API error", + Code: http.StatusInternalServerError, + }, + } normResp, ok := resp.(*response.NormalResponse) if !ok { - return k8sErrors.StatusError{ - ErrStatus: metav1.Status{ - Message: "Folder API error", - Code: http.StatusInternalServerError, - }, - } + return defaultErr + } + + var dat map[string]interface{} + if err := json.Unmarshal(normResp.Body(), &dat); err != nil { + return defaultErr + } + + m, ok := dat["message"] + if !ok { + return defaultErr + } + + message, ok := m.(string) + if !ok { + return defaultErr } return k8sErrors.StatusError{ ErrStatus: metav1.Status{ - Message: normResp.ErrMessage(), + Message: message, Code: int32(normResp.Status()), }, } diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 6506a859127..f387cfe64f5 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -807,7 +807,14 @@ func (fk8s *folderK8sHandler) writeError(c *contextmodel.ReqContext, err error) //nolint:errorlint statusError, ok := err.(*k8sErrors.StatusError) if ok { - c.JsonApiErr(int(statusError.Status().Code), statusError.Status().Message, err) + message := statusError.Status().Message + // #TODO: Is there a better way to set the correct meesage? Instead of "access denied to folder", currently we are + // returning something like `folders.folder.grafana.app is forbidden: User "" cannot create resource "folders" in + // API group "folder.grafana.app" in the namespace "default": folder`` + if statusError.Status().Code == http.StatusForbidden { + message = dashboards.ErrFolderAccessDenied.Error() + } + c.JsonApiErr(int(statusError.Status().Code), message, err) return } errhttp.Write(c.Req.Context(), err, c.Resp) diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index 47062e57b86..0c5d3248bf8 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -657,13 +657,14 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { folderWithoutParentInput := "{ \"uid\": \"uid\", \"title\": \"Folder\"}" folderWithoutUID := "{ \"title\": \"Folder without UID\"}" folderWithTitleEmpty := "{ \"title\": \"\"}" - folderWithInvalidUid := "{ \"uid\": \"------------\"}" - folderWithUIDTooLong := "{ \"uid\": \"asdfghjklqwertyuiopzxcvbnmasdfghjklqwertyuiopzxcvbnmasdfghjklqwertyuiopzxcvbnm\"}" + folderWithInvalidUid := "{ \"uid\": \"::::::::::::\", \"title\": \"Another folder\"}" + folderWithUIDTooLong := "{ \"uid\": \"asdfghjklqwertyuiopzxcvbnmasdfghjklqwertyuiopzxcvbnmasdfghjklqwertyuiopzxcvbnm\", \"title\": \"Third folder\"}" folderWithSameName := "{\"title\": \"same name\"}" type testCase struct { description string expectedCode int + expectedMessage string expectedFolderSvcError error permissions []resourcepermissions.SetResourcePermissionCommand input string @@ -688,15 +689,19 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { permissions: folderCreatePermission, }, { - description: "folder creation fails without permissions to create a folder", - input: folderWithoutParentInput, - expectedCode: http.StatusForbidden, - permissions: []resourcepermissions.SetResourcePermissionCommand{}, + description: "folder creation fails without permissions to create a folder", + input: folderWithoutParentInput, + expectedCode: http.StatusForbidden, + expectedMessage: dashboards.ErrFolderAccessDenied.Error(), + permissions: []resourcepermissions.SetResourcePermissionCommand{}, }, { - description: "folder creation fails given folder service error %s", - input: folderWithoutUID, - expectedCode: http.StatusConflict, + // #TODO This test case doesn't set up the conditions it describes. We should have created a folder with the same UID before + // creating a second one and failing to do so successfully. + description: "folder creation fails given folder service error %s", + input: folderWithoutUID, + expectedCode: http.StatusConflict, + // expectedMessage: dashboards.ErrFolderWithSameUIDExists.Error(), expectedFolderSvcError: dashboards.ErrFolderWithSameUIDExists, createSecondRecord: true, permissions: folderCreatePermission, @@ -705,6 +710,7 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { description: "folder creation fails given folder service error %s", input: folderWithTitleEmpty, expectedCode: http.StatusBadRequest, + expectedMessage: dashboards.ErrFolderTitleEmpty.Error(), expectedFolderSvcError: dashboards.ErrFolderTitleEmpty, permissions: folderCreatePermission, }, @@ -712,6 +718,7 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { description: "folder creation fails given folder service error %s", input: folderWithInvalidUid, expectedCode: http.StatusBadRequest, + expectedMessage: dashboards.ErrDashboardInvalidUid.Error(), expectedFolderSvcError: dashboards.ErrDashboardInvalidUid, permissions: folderCreatePermission, }, @@ -719,6 +726,7 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { description: "folder creation fails given folder service error %s", input: folderWithUIDTooLong, expectedCode: http.StatusBadRequest, + expectedMessage: dashboards.ErrDashboardUidTooLong.Error(), expectedFolderSvcError: dashboards.ErrDashboardUidTooLong, permissions: folderCreatePermission, }, @@ -726,6 +734,7 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { description: "folder creation fails given folder service error %s", input: folderWithSameName, expectedCode: http.StatusConflict, + expectedMessage: dashboards.ErrFolderSameNameExists.Error(), expectedFolderSvcError: dashboards.ErrFolderSameNameExists, createSecondRecord: true, permissions: folderCreatePermission, @@ -734,6 +743,7 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { description: "folder creation fails given folder service error %s", input: folderWithoutParentInput, expectedCode: http.StatusPreconditionFailed, + expectedMessage: dashboards.ErrFolderVersionMismatch.Error(), expectedFolderSvcError: dashboards.ErrFolderVersionMismatch, createSecondRecord: true, permissions: folderCreatePermission, @@ -792,7 +802,12 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { require.NotNil(t, resp) require.Equal(t, tc.expectedCode, resp.StatusCode) - folder := dtos.Folder{} + type folderWithMessage struct { + dtos.Folder + Message string `json:"message"` + } + + folder := folderWithMessage{} err = json.NewDecoder(resp.Body).Decode(&folder) require.NoError(t, err) require.NoError(t, resp.Body.Close()) @@ -801,6 +816,10 @@ func TestFoldersCreateAPIEndpointK8S(t *testing.T) { require.Equal(t, "uid", folder.UID) require.Equal(t, "Folder", folder.Title) } + + if tc.expectedMessage != "" { + require.Equal(t, tc.expectedMessage, folder.Message) + } }) } }