From b96dd1877c27711fe5638cbf9054832f5aa64b6f Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki Date: Thu, 22 Jul 2021 12:27:13 +0300 Subject: [PATCH] Folder API: optionally force deleting Grafana 8 alerts when deleting a folder (or error) (#36427) * Folder API: Add an optional query parameter for allowing deleting a folder containing rules * Update frontend - Set forceDeleteRules=true when frontend deletes a folder - Improve modal text * Update docs * Apply suggestions from code review Co-authored-by: gotjosh Co-authored-by: Nathan Rodman Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com> --- docs/sources/http_api/folder.md | 5 +- pkg/api/folder.go | 5 +- pkg/api/folder_test.go | 2 +- pkg/models/dashboards.go | 5 +- pkg/models/folders.go | 1 + pkg/services/dashboards/folder_service.go | 6 +- .../dashboards/folder_service_test.go | 4 +- pkg/services/sqlstore/dashboard.go | 27 +++-- pkg/services/sqlstore/dashboard_test.go | 105 +++++++++++++++++- .../api/alerting/api_alertmanager_test.go | 20 +++- .../features/folders/FolderSettingsPage.tsx | 3 +- public/app/features/folders/state/actions.ts | 2 +- .../manage-dashboards/state/actions.ts | 2 +- .../search/components/ConfirmDeleteModal.tsx | 4 +- 14 files changed, 164 insertions(+), 27 deletions(-) diff --git a/docs/sources/http_api/folder.md b/docs/sources/http_api/folder.md index c3a42054a0f..75389516e35 100644 --- a/docs/sources/http_api/folder.md +++ b/docs/sources/http_api/folder.md @@ -237,7 +237,9 @@ Content-Length: 97 `DELETE /api/folders/:uid` -Deletes an existing folder identified by uid together with all dashboards stored in the folder, if any. This operation cannot be reverted. +Deletes an existing folder identified by UID along with all dashboards (and their alerts) stored in the folder. This operation cannot be reverted. + +If [Grafana 8 Alerts]({{< relref "../alerting/unified-alerting/_index.md" >}}) are enabled, you can set an optional query parameter `forceDeleteRules=false` so that requests will fail with 400 (Bad Request) error if the folder contains any Grafana 8 Alerts. However, if this parameter is set to `true` then it will delete any Grafana 8 Alerts under this folder. **Example Request**: @@ -265,6 +267,7 @@ Status Codes: - **200** – Deleted - **401** – Unauthorized +- **400** – Bad Request - **403** – Access Denied - **404** – Folder not found diff --git a/pkg/api/folder.go b/pkg/api/folder.go index f89841b4691..68136cde55c 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -96,7 +96,7 @@ func (hs *HTTPServer) DeleteFolder(c *models.ReqContext) response.Response { // return ToFolderErrorResponse(err) } - f, err := s.DeleteFolder(c.Params(":uid")) + f, err := s.DeleteFolder(c.Params(":uid"), c.QueryBool("forceDeleteRules")) if err != nil { return ToFolderErrorResponse(err) } @@ -149,7 +149,8 @@ func ToFolderErrorResponse(err error) response.Response { if errors.Is(err, models.ErrFolderTitleEmpty) || errors.Is(err, models.ErrDashboardTypeMismatch) || errors.Is(err, models.ErrDashboardInvalidUid) || - errors.Is(err, models.ErrDashboardUidTooLong) { + errors.Is(err, models.ErrDashboardUidTooLong) || + errors.Is(err, models.ErrFolderContainsAlertRules) { return response.Error(400, err.Error(), nil) } diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 2b4b54d9a64..31b7021f075 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -238,7 +238,7 @@ func (s *fakeFolderService) UpdateFolder(existingUID string, cmd *models.UpdateF return s.UpdateFolderError } -func (s *fakeFolderService) DeleteFolder(uid string) (*models.Folder, error) { +func (s *fakeFolderService) DeleteFolder(uid string, forceDeleteRules bool) (*models.Folder, error) { s.DeletedFolderUids = append(s.DeletedFolderUids, uid) return s.DeleteFolderResult, s.DeleteFolderError } diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index c843e023b5c..0e829ed3d3c 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -363,8 +363,9 @@ type DashboardProvisioning struct { } type DeleteDashboardCommand struct { - Id int64 - OrgId int64 + Id int64 + OrgId int64 + ForceDeleteFolderRules bool } type DeleteOrphanedProvisionedDashboardsCommand struct { diff --git a/pkg/models/folders.go b/pkg/models/folders.go index 09ff05b657c..faa0af947fc 100644 --- a/pkg/models/folders.go +++ b/pkg/models/folders.go @@ -15,6 +15,7 @@ var ( ErrFolderSameNameExists = errors.New("a folder or dashboard in the general folder with the same name already exists") ErrFolderFailedGenerateUniqueUid = errors.New("failed to generate unique folder ID") ErrFolderAccessDenied = errors.New("access denied to folder") + ErrFolderContainsAlertRules = errors.New("folder contains alert rules") ) type Folder struct { diff --git a/pkg/services/dashboards/folder_service.go b/pkg/services/dashboards/folder_service.go index 31087485eec..662de976b8e 100644 --- a/pkg/services/dashboards/folder_service.go +++ b/pkg/services/dashboards/folder_service.go @@ -19,7 +19,7 @@ type FolderService interface { GetFolderByTitle(title string) (*models.Folder, error) CreateFolder(title, uid string) (*models.Folder, error) UpdateFolder(uid string, cmd *models.UpdateFolderCommand) error - DeleteFolder(uid string) (*models.Folder, error) + DeleteFolder(uid string, forceDeleteRules bool) (*models.Folder, error) MakeUserAdmin(orgID int64, userID, folderID int64, setViewAndEditPermissions bool) error } @@ -192,7 +192,7 @@ func (dr *dashboardServiceImpl) UpdateFolder(existingUid string, cmd *models.Upd return nil } -func (dr *dashboardServiceImpl) DeleteFolder(uid string) (*models.Folder, error) { +func (dr *dashboardServiceImpl) DeleteFolder(uid string, forceDeleteRules bool) (*models.Folder, error) { query := models.GetDashboardQuery{OrgId: dr.orgId, Uid: uid} dashFolder, err := getFolder(query) if err != nil { @@ -207,7 +207,7 @@ func (dr *dashboardServiceImpl) DeleteFolder(uid string) (*models.Folder, error) return nil, models.ErrFolderAccessDenied } - deleteCmd := models.DeleteDashboardCommand{OrgId: dr.orgId, Id: dashFolder.Id} + deleteCmd := models.DeleteDashboardCommand{OrgId: dr.orgId, Id: dashFolder.Id, ForceDeleteFolderRules: forceDeleteRules} if err := bus.Dispatch(&deleteCmd); err != nil { return nil, toFolderError(err) } diff --git a/pkg/services/dashboards/folder_service_test.go b/pkg/services/dashboards/folder_service_test.go index 49818d26806..122f0ad1cc3 100644 --- a/pkg/services/dashboards/folder_service_test.go +++ b/pkg/services/dashboards/folder_service_test.go @@ -67,7 +67,7 @@ func TestFolderService(t *testing.T) { }) t.Run("When deleting folder by uid should return access denied error", func(t *testing.T) { - _, err := service.DeleteFolder("uid") + _, err := service.DeleteFolder("uid", false) require.Error(t, err) require.Equal(t, err, models.ErrFolderAccessDenied) }) @@ -121,7 +121,7 @@ func TestFolderService(t *testing.T) { }) t.Run("When deleting folder by uid should not return access denied error", func(t *testing.T) { - _, err := service.DeleteFolder("uid") + _, err := service.DeleteFolder("uid", false) require.NoError(t, err) }) diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index aaa9c332b16..e508cadce0d 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -476,16 +476,27 @@ func deleteDashboard(cmd *models.DeleteDashboardCommand, sess *DBSession) error } } - // clean ngalert tables - ngalertDeletes := []string{ - "DELETE FROM alert_rule WHERE namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", - "DELETE FROM alert_rule_version WHERE rule_namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", + var existingRuleID int64 + exists, err := sess.Table("alert_rule").Where("namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", dashboard.Id).Cols("id").Get(&existingRuleID) + if err != nil { + return err } + if exists { + if !cmd.ForceDeleteFolderRules { + return fmt.Errorf("folder cannot be deleted: %w", models.ErrFolderContainsAlertRules) + } - for _, sql := range ngalertDeletes { - _, err := sess.Exec(sql, dashboard.Id) - if err != nil { - return err + // Delete all rules under this folder. + deleteNGAlertsByFolder := []string{ + "DELETE FROM alert_rule WHERE namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", + "DELETE FROM alert_rule_version WHERE rule_namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", + } + + for _, sql := range deleteNGAlertsByFolder { + _, err := sess.Exec(sql, dashboard.Id) + if err != nil { + return err + } } } } diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 008dea834fa..ea0e25ba6d3 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -4,6 +4,8 @@ package sqlstore import ( "context" + "encoding/json" + "errors" "fmt" "testing" "time" @@ -29,6 +31,7 @@ func TestDashboardDataAccess(t *testing.T) { savedDash := insertTestDashboard(t, sqlStore, "test dash 23", 1, savedFolder.Id, false, "prod", "webapp") insertTestDashboard(t, sqlStore, "test dash 45", 1, savedFolder.Id, false, "prod") savedDash2 := insertTestDashboard(t, sqlStore, "test dash 67", 1, 0, false, "prod") + insertTestRule(t, sqlStore, savedFolder.OrgId, savedFolder.Uid) Convey("Should return dashboard model", func() { So(savedDash.Title, ShouldEqual, "test dash 23") @@ -204,8 +207,14 @@ func TestDashboardDataAccess(t *testing.T) { So(err, ShouldBeNil) }) - Convey("Should be able to delete a dashboard folder and its children", func() { - deleteCmd := &models.DeleteDashboardCommand{Id: savedFolder.Id} + Convey("Should be not able to delete a dashboard if force delete rules is disabled", func() { + deleteCmd := &models.DeleteDashboardCommand{Id: savedFolder.Id, ForceDeleteFolderRules: false} + err := DeleteDashboard(deleteCmd) + So(errors.Is(err, models.ErrFolderContainsAlertRules), ShouldBeTrue) + }) + + Convey("Should be able to delete a dashboard folder and its children if force delete rules is enabled", func() { + deleteCmd := &models.DeleteDashboardCommand{Id: savedFolder.Id, ForceDeleteFolderRules: true} err := DeleteDashboard(deleteCmd) So(err, ShouldBeNil) @@ -219,6 +228,20 @@ func TestDashboardDataAccess(t *testing.T) { So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 0) + + sqlStore.WithDbSession(context.Background(), func(sess *DBSession) error { + var existingRuleID int64 + exists, err := sess.Table("alert_rule").Where("namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", savedFolder.Id).Cols("id").Get(&existingRuleID) + require.NoError(t, err) + So(exists, ShouldBeFalse) + + var existingRuleVersionID int64 + exists, err = sess.Table("alert_rule_version").Where("rule_namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", savedFolder.Id).Cols("id").Get(&existingRuleVersionID) + require.NoError(t, err) + So(exists, ShouldBeFalse) + + return nil + }) }) Convey("Should return error if no dashboard is found for update when dashboard id is greater than zero", func() { @@ -460,6 +483,84 @@ func insertTestDashboard(t *testing.T, sqlStore *SQLStore, title string, orgId i return dash } +func insertTestRule(t *testing.T, sqlStore *SQLStore, foderOrgID int64, folderUID string) { + sqlStore.WithDbSession(context.Background(), func(sess *DBSession) error { + + type alertQuery struct { + RefID string + DatasourceUID string + Model json.RawMessage + } + + type alertRule struct { + ID int64 `xorm:"pk autoincr 'id'"` + OrgID int64 `xorm:"org_id"` + Title string + Updated time.Time + UID string `xorm:"uid"` + NamespaceUID string `xorm:"namespace_uid"` + RuleGroup string + Condition string + Data []alertQuery + } + + rule := alertRule{ + OrgID: foderOrgID, + NamespaceUID: folderUID, + UID: "rule", + RuleGroup: "rulegroup", + Updated: time.Now(), + Condition: "A", + Data: []alertQuery{ + { + RefID: "A", + DatasourceUID: "-100", + Model: json.RawMessage(`{ + "type": "math", + "expression": "2 + 3 > 1" + }`), + }, + }, + } + _, err := sess.Insert(&rule) + require.NoError(t, err) + + type alertRuleVersion struct { + ID int64 `xorm:"pk autoincr 'id'"` + RuleOrgID int64 `xorm:"rule_org_id"` + RuleUID string `xorm:"rule_uid"` + RuleNamespaceUID string `xorm:"rule_namespace_uid"` + RuleGroup string + ParentVersion int64 + RestoredFrom int64 + Version int64 + Created time.Time + Title string + Condition string + Data []alertQuery + IntervalSeconds int64 + } + + ruleVersion := alertRuleVersion{ + RuleOrgID: rule.OrgID, + RuleUID: rule.UID, + RuleNamespaceUID: rule.NamespaceUID, + RuleGroup: rule.RuleGroup, + Created: rule.Updated, + Condition: rule.Condition, + Data: rule.Data, + ParentVersion: 0, + RestoredFrom: 0, + Version: 1, + IntervalSeconds: 60, + } + _, err = sess.Insert(&ruleVersion) + require.NoError(t, err) + + return err + }) +} + func insertTestDashboardForPlugin(t *testing.T, sqlStore *SQLStore, title string, orgId int64, folderId int64, isFolder bool, pluginId string) *models.Dashboard { t.Helper() diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 8cd8092da79..8206c6c36ff 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -748,7 +748,7 @@ func TestDeleteFolderWithRules(t *testing.T) { assert.JSONEq(t, expectedGetRulesResponseBody, string(b)) } - // Next, the editor can delete the folder. + // Next, the editor can not delete the folder because it contains Grafana 8 alerts. { u := fmt.Sprintf("http://editor:editor@%s/api/folders/%s", grafanaListedAddr, namespaceUID) req, err := http.NewRequest(http.MethodDelete, u, nil) @@ -762,6 +762,24 @@ func TestDeleteFolderWithRules(t *testing.T) { }) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.JSONEq(t, `{"message":"folder cannot be deleted: folder contains alert rules"}`, string(b)) + } + + // Next, the editor can delete the folder if forceDeleteRules is true. + { + u := fmt.Sprintf("http://editor:editor@%s/api/folders/%s?forceDeleteRules=true", grafanaListedAddr, namespaceUID) + req, err := http.NewRequest(http.MethodDelete, u, nil) + require.NoError(t, err) + client := &http.Client{} + resp, err := client.Do(req) + require.NoError(t, err) + t.Cleanup(func() { + err := resp.Body.Close() + require.NoError(t, err) + }) + b, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) require.Equal(t, 200, resp.StatusCode) require.JSONEq(t, `{"id":1,"message":"Folder default deleted","title":"default"}`, string(b)) } diff --git a/public/app/features/folders/FolderSettingsPage.tsx b/public/app/features/folders/FolderSettingsPage.tsx index 9f48da88d80..8c738fb1f33 100644 --- a/public/app/features/folders/FolderSettingsPage.tsx +++ b/public/app/features/folders/FolderSettingsPage.tsx @@ -66,10 +66,11 @@ export class FolderSettingsPage extends PureComponent { evt.stopPropagation(); evt.preventDefault(); + const confirmationText = `Do you want to delete this folder and all its dashboards and alerts?`; appEvents.publish( new ShowConfirmModalEvent({ title: 'Delete', - text: `Do you want to delete this folder and all its dashboards?`, + text: confirmationText, icon: 'trash-alt', yesText: 'Delete', onConfirm: () => { diff --git a/public/app/features/folders/state/actions.ts b/public/app/features/folders/state/actions.ts index f65918e3509..26d32b46bb7 100644 --- a/public/app/features/folders/state/actions.ts +++ b/public/app/features/folders/state/actions.ts @@ -31,7 +31,7 @@ export function saveFolder(folder: FolderState): ThunkResult { export function deleteFolder(uid: string): ThunkResult { return async (dispatch) => { - await backendSrv.delete(`/api/folders/${uid}`); + await backendSrv.delete(`/api/folders/${uid}?forceDeleteRules=true`); locationService.push('/dashboards'); }; } diff --git a/public/app/features/manage-dashboards/state/actions.ts b/public/app/features/manage-dashboards/state/actions.ts index 9d01e3eba69..476d241d382 100644 --- a/public/app/features/manage-dashboards/state/actions.ts +++ b/public/app/features/manage-dashboards/state/actions.ts @@ -213,7 +213,7 @@ export function saveDashboard(options: SaveDashboardOptions) { function deleteFolder(uid: string, showSuccessAlert: boolean) { return getBackendSrv().request({ method: 'DELETE', - url: `/api/folders/${uid}`, + url: `/api/folders/${uid}?forceDeleteRules=true`, showSuccessAlert: showSuccessAlert === true, }); } diff --git a/public/app/features/search/components/ConfirmDeleteModal.tsx b/public/app/features/search/components/ConfirmDeleteModal.tsx index e88d47b5c8e..2f2172c825d 100644 --- a/public/app/features/search/components/ConfirmDeleteModal.tsx +++ b/public/app/features/search/components/ConfirmDeleteModal.tsx @@ -30,9 +30,9 @@ export const ConfirmDeleteModal: FC = ({ results, onDeleteItems, isOpen, if (folderCount > 0 && dashCount > 0) { text += `selected folder${folderEnding} and dashboard${dashEnding}?\n`; - subtitle = `All dashboards of the selected folder${folderEnding} will also be deleted`; + subtitle = `All dashboards and alerts of the selected folder${folderEnding} will also be deleted`; } else if (folderCount > 0) { - text += `selected folder${folderEnding} and all its dashboards?`; + text += `selected folder${folderEnding} and all their dashboards and alerts?`; } else { text += `selected dashboard${dashEnding}?`; }