From 41bd36eb9747f52e52c4c7feb4fb2f8be32889dd Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Wed, 24 Aug 2022 15:33:33 -0400 Subject: [PATCH] Alerting: Update rules delete endpoint to handle rules in group (#53790) * update RouteDeleteAlertRules rules to update as a group * remove expecter from scheduler mock to support variadic function * create function to check for provisioning status + tests Co-authored-by: Alexander Weaver --- pkg/services/ngalert/api/api_ruler.go | 123 ++++----- pkg/services/ngalert/api/api_ruler_test.go | 257 +++++++++--------- pkg/services/ngalert/api/util.go | 13 + pkg/services/ngalert/api/util_test.go | 19 ++ pkg/services/ngalert/models/testing.go | 5 + pkg/services/ngalert/ngalert_test.go | 2 +- pkg/services/ngalert/schedule/schedule.go | 38 +-- .../ngalert/schedule/schedule_mock.go | 154 +---------- 8 files changed, 245 insertions(+), 366 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index e884c3e1a66..6ded48c03c1 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -44,8 +44,10 @@ var ( errProvisionedResource = errors.New("request affects resources created via provisioning API") ) -// RouteDeleteAlertRules deletes all alert rules user is authorized to access in the given namespace -// or, if non-empty, a specific group of rules in the namespace +// RouteDeleteAlertRules deletes all alert rules the user is authorized to access in the given namespace +// or, if non-empty, a specific group of rules in the namespace. +// Returns http.StatusUnauthorized if user does not have access to any of the rules that match the filter. +// Returns http.StatusBadRequest if all rules that match the filter and the user is authorized to delete are provisioned. func (srv RulerSrv) RouteDeleteAlertRules(c *models.ReqContext, namespaceTitle string, group string) response.Response { namespace, err := srv.store.GetNamespaceByTitle(c.Req.Context(), namespaceTitle, c.SignedInUser.OrgID, c.SignedInUser, true) if err != nil { @@ -71,8 +73,9 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *models.ReqContext, namespaceTitle s return ErrResp(http.StatusInternalServerError, err, "failed to fetch provenances of alert rules") } - var deletableRules []string + deletedGroups := make(map[ngmodels.AlertRuleGroupKey][]ngmodels.AlertRuleKey) err = srv.xactManager.InTransaction(c.Req.Context(), func(ctx context.Context) error { + unauthz, provisioned := false, false q := ngmodels.ListAlertRulesQuery{ OrgID: c.SignedInUser.OrgID, NamespaceUIDs: []string{namespace.Uid}, @@ -87,68 +90,60 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *models.ReqContext, namespaceTitle s return nil } - var canDelete []*ngmodels.AlertRule - var cannotDelete []string - - // partition will partation the given rules in two, one partition - // being the rules that fulfill the predicate the other partation being - // the ruleIDs not fulfilling it. - partition := func(alerts []*ngmodels.AlertRule, predicate func(rule *ngmodels.AlertRule) bool) ([]*ngmodels.AlertRule, []string) { - positive, negative := make([]*ngmodels.AlertRule, 0, len(alerts)), make([]string, 0, len(alerts)) - for _, rule := range alerts { - if predicate(rule) { - positive = append(positive, rule) - continue - } - negative = append(negative, rule.UID) - } - return positive, negative - } - - canDelete, cannotDelete = partition(q.Result, func(rule *ngmodels.AlertRule) bool { - return authorizeDatasourceAccessForRule(rule, hasAccess) - }) - if len(canDelete) == 0 { - return fmt.Errorf("%w to delete rules because user is not authorized to access data sources used by the rules", ErrAuthorization) + var deletionCandidates = make(map[ngmodels.AlertRuleGroupKey][]*ngmodels.AlertRule) + for _, rule := range q.Result { + key := rule.GetGroupKey() + deletionCandidates[key] = append(deletionCandidates[key], rule) } - if len(cannotDelete) > 0 { - logger.Info("user cannot delete one or many alert rules because it does not have access to data sources. Those rules will be skipped", "expected", len(q.Result), "authorized", len(canDelete), "unauthorized", cannotDelete) + rulesToDelete := make([]string, 0, len(q.Result)) + for groupKey, rules := range deletionCandidates { + if !authorizeAccessToRuleGroup(rules, hasAccess) { + unauthz = true + continue + } + if containsProvisionedAlerts(provenances, rules) { + provisioned = true + continue + } + uid := make([]string, 0, len(rules)) + keys := make([]ngmodels.AlertRuleKey, 0, len(rules)) + for _, rule := range rules { + uid = append(uid, rule.UID) + keys = append(keys, rule.GetKey()) + } + rulesToDelete = append(rulesToDelete, uid...) + deletedGroups[groupKey] = keys } - - canDelete, cannotDelete = partition(canDelete, func(rule *ngmodels.AlertRule) bool { - provenance, exists := provenances[rule.UID] - return (exists && provenance == ngmodels.ProvenanceNone) || !exists - }) - - if len(canDelete) == 0 { - return fmt.Errorf("all rules have been provisioned and cannot be deleted through this api") + if len(rulesToDelete) > 0 { + return srv.store.DeleteAlertRulesByUID(ctx, c.SignedInUser.OrgID, rulesToDelete...) } - - if len(cannotDelete) > 0 { - logger.Info("user cannot delete one or many alert rules because it does have a provenance set. Those rules will be skipped", "expected", len(q.Result), "provenance_none", len(canDelete), "provenance_set", cannotDelete) + // if none rules were deleted return an error. + // Check whether provisioned check failed first because if it is true, then all rules that the user can access (actually read via GET API) are provisioned. + if provisioned { + return errProvisionedResource } - - for _, rule := range canDelete { - deletableRules = append(deletableRules, rule.UID) + if unauthz { + if group == "" { + return fmt.Errorf("%w to delete any existing rules in the namespace", ErrAuthorization) + } + return fmt.Errorf("%w to delete group of the rules", ErrAuthorization) } - - return srv.store.DeleteAlertRulesByUID(ctx, c.SignedInUser.OrgID, deletableRules...) + return nil }) if err != nil { if errors.Is(err, ErrAuthorization) { - return ErrResp(http.StatusUnauthorized, err, "") + return ErrResp(http.StatusUnauthorized, err, "failed to delete rule group") + } + if errors.Is(err, errProvisionedResource) { + return ErrResp(http.StatusBadRequest, err, "failed to delete rule group") } return ErrResp(http.StatusInternalServerError, err, "failed to delete rule group") } logger.Debug("rules have been deleted from the store. updating scheduler") - - for _, uid := range deletableRules { - srv.scheduleService.DeleteAlertRule(ngmodels.AlertRuleKey{ - OrgID: c.SignedInUser.OrgID, - UID: uid, - }) + for _, ruleKeys := range deletedGroups { + srv.scheduleService.DeleteAlertRule(ruleKeys...) } return response.JSON(http.StatusAccepted, util.DynMap{"message": "rules deleted"}) @@ -427,11 +422,12 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, groupKey ngmod }, rule.Existing.Version+1) } - for _, rule := range finalChanges.Delete { - srv.scheduleService.DeleteAlertRule(ngmodels.AlertRuleKey{ - OrgID: c.SignedInUser.OrgID, - UID: rule.UID, - }) + if len(finalChanges.Delete) > 0 { + keys := make([]ngmodels.AlertRuleKey, 0, len(finalChanges.Delete)) + for _, rule := range finalChanges.Delete { + keys = append(keys, rule.GetKey()) + } + srv.scheduleService.DeleteAlertRule(keys...) } if finalChanges.IsEmpty() { @@ -510,16 +506,13 @@ func verifyProvisionedRulesNotAffected(ctx context.Context, provenanceStore prov } errorMsg := strings.Builder{} for group, alertRules := range ch.AffectedGroups { - for _, rule := range alertRules { - if provenance, exists := provenances[rule.UID]; (exists && provenance == ngmodels.ProvenanceNone) || !exists { - continue - } - if errorMsg.Len() > 0 { - errorMsg.WriteRune(',') - } - errorMsg.WriteString(group.String()) - break + if !containsProvisionedAlerts(provenances, alertRules) { + continue + } + if errorMsg.Len() > 0 { + errorMsg.WriteRune(',') } + errorMsg.WriteString(group.String()) } if errorMsg.Len() == 0 { return nil diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index b0c07eca0f7..082ad2f57d8 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -55,217 +55,208 @@ func TestRouteDeleteAlertRules(t *testing.T) { require.Containsf(t, actualUIDs, rule.UID, "Rule %s was expected to be deleted but it wasn't", rule.UID) } - require.Len(t, scheduler.Calls, len(expectedRules)) + notDeletedRules := make(map[models.AlertRuleKey]struct{}, len(expectedRules)) + for _, rule := range expectedRules { + notDeletedRules[rule.GetKey()] = struct{}{} + } for _, call := range scheduler.Calls { require.Equal(t, "DeleteAlertRule", call.Method) - key, ok := call.Arguments.Get(0).(models.AlertRuleKey) + keys, ok := call.Arguments.Get(0).([]models.AlertRuleKey) require.Truef(t, ok, "Expected AlertRuleKey but got something else") - found := false - for _, rule := range expectedRules { - if rule.GetKey() == key { - found = true - break - } + for _, key := range keys { + delete(notDeletedRules, key) } - require.Truef(t, found, "Key %v was not expected to be submitted to scheduler", key) } + require.Emptyf(t, notDeletedRules, "Not all rules were deleted") + } + + orgID := rand.Int63() + folder := randFolder() + + initFakeRuleStore := func(t *testing.T) *store.FakeRuleStore { + ruleStore := store.NewFakeRuleStore(t) + ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) + // add random data + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID)))...) + return ruleStore } t.Run("when fine-grained access is disabled", func(t *testing.T) { + ac := acMock.New().WithDisabled() t.Run("viewer should not be authorized", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) + ruleStore := initFakeRuleStore(t) + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) scheduler := &schedule.FakeScheduleService{} - scheduler.On("DeleteAlertRule", mock.Anything).Panic("should not be called") + scheduler.On("DeleteAlertRule", mock.Anything) - ac := acMock.New().WithDisabled() request := createRequestContext(orgID, org.RoleViewer, nil) response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "") - require.Equalf(t, 401, response.Status(), "Expected 403 but got %d: %v", response.Status(), string(response.Body())) + require.Equalf(t, 401, response.Status(), "Expected 401 but got %d: %v", response.Status(), string(response.Body())) scheduler.AssertNotCalled(t, "DeleteAlertRule") require.Empty(t, getRecordedCommand(ruleStore)) }) - t.Run("editor should be able to delete all rules in folder", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - rulesInFolder := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder))) + t.Run("editor should be able to delete all non-provisioned rules in folder", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + rulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder))) ruleStore.PutRule(context.Background(), rulesInFolder...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) scheduler := &schedule.FakeScheduleService{} scheduler.On("DeleteAlertRule", mock.Anything) - ac := acMock.New().WithDisabled() request := createRequestContext(orgID, org.RoleEditor, nil) response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "") + require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) assertRulesDeleted(t, rulesInFolder, ruleStore, scheduler) }) - t.Run("editor should be able to delete rules in a group in a folder", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() + t.Run("editor should be able to delete rules group if it is not provisioned", func(t *testing.T) { groupName := util.GenerateShortUID() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - rulesInFolderInGroup := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName))) + rulesInFolderInGroup := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName))) + + ruleStore := initFakeRuleStore(t) ruleStore.PutRule(context.Background(), rulesInFolderInGroup...) // rules in different groups but in the same namespace - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) // rules in the same group but different folder - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withGroup(groupName)))...) + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withGroup(groupName)))...) scheduler := &schedule.FakeScheduleService{} - scheduler.On("DeleteAlertRule", mock.Anything) + scheduler.On("DeleteAlertRule", mock.Anything).Return() - ac := acMock.New().WithDisabled() request := createRequestContext(orgID, org.RoleEditor, nil) response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, groupName) + require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) assertRulesDeleted(t, rulesInFolderInGroup, ruleStore, scheduler) }) - t.Run("editor shouldn't be able to delete provisioned rules", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - rulesInFolder := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder))) - ruleStore.PutRule(context.Background(), rulesInFolder...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) + t.Run("should return 202 if folder is empty", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) scheduler := &schedule.FakeScheduleService{} scheduler.On("DeleteAlertRule", mock.Anything) - ac := acMock.New().WithDisabled() - - svc := createService(ac, ruleStore, scheduler) + requestCtx := createRequestContext(orgID, org.RoleEditor, nil) + response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(requestCtx, folder.Title, "") - err := svc.provenanceStore.SetProvenance(context.Background(), rulesInFolder[0], orgID, models.ProvenanceAPI) - require.NoError(t, err) - - request := createRequestContext(orgID, org.RoleEditor, nil) - response := svc.RouteDeleteAlertRules(request, folder.Title, "") require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) - assertRulesDeleted(t, rulesInFolder[1:], ruleStore, scheduler) + scheduler.AssertNotCalled(t, "DeleteAlertRule") + require.Empty(t, getRecordedCommand(ruleStore)) }) }) t.Run("when fine-grained access is enabled", func(t *testing.T) { - t.Run("and user does not have access to any of data sources used by alert rules", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) - - scheduler := &schedule.FakeScheduleService{} - scheduler.On("DeleteAlertRule", mock.Anything).Panic("should not be called") + requestCtx := createRequestContext(orgID, "None", nil) - ac := acMock.New() - request := createRequestContext(orgID, "None", nil) - response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "") - require.Equalf(t, 401, response.Status(), "Expected 403 but got %d: %v", response.Status(), string(response.Body())) - - scheduler.AssertNotCalled(t, "DeleteAlertRule") - require.Empty(t, getRecordedCommand(ruleStore)) - }) - t.Run("and user has access to all alert rules", func(t *testing.T) { - t.Run("should delete all rules", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - rulesInFolder := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder))) - ruleStore.PutRule(context.Background(), rulesInFolder...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) + t.Run("and group argument is empty", func(t *testing.T) { + t.Run("return 401 if user is not authorized to access any group in the folder", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) scheduler := &schedule.FakeScheduleService{} - scheduler.On("DeleteAlertRule", mock.Anything) + scheduler.On("DeleteAlertRule", mock.Anything).Panic("should not be called") - ac := acMock.New().WithPermissions(createPermissionsForRules(rulesInFolder)) + ac := acMock.New() request := createRequestContext(orgID, "None", nil) response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "") - require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) - assertRulesDeleted(t, rulesInFolder, ruleStore, scheduler) + require.Equalf(t, 401, response.Status(), "Expected 401 but got %d: %v", response.Status(), string(response.Body())) + + scheduler.AssertNotCalled(t, "DeleteAlertRule") + require.Empty(t, getRecordedCommand(ruleStore)) }) - t.Run("shouldn't be able to delete provisioned rules", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - rulesInFolder := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder))) - ruleStore.PutRule(context.Background(), rulesInFolder...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) + t.Run("delete only non-provisioned groups that user is authorized", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + provisioningStore := provisioning.NewFakeProvisioningStore() scheduler := &schedule.FakeScheduleService{} scheduler.On("DeleteAlertRule", mock.Anything) - ac := acMock.New().WithPermissions(createPermissionsForRules(rulesInFolder)) - svc := createService(ac, ruleStore, scheduler) + authorizedRulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup("authz_"+util.GenerateShortUID()))) - err := svc.provenanceStore.SetProvenance(context.Background(), rulesInFolder[0], orgID, models.ProvenanceAPI) + provisionedRulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup("provisioned_"+util.GenerateShortUID()))) + err := provisioningStore.SetProvenance(context.Background(), provisionedRulesInFolder[0], orgID, models.ProvenanceAPI) require.NoError(t, err) - request := createRequestContext(orgID, "None", nil) + ruleStore.PutRule(context.Background(), authorizedRulesInFolder...) + ruleStore.PutRule(context.Background(), provisionedRulesInFolder...) + // more rules in the same namespace but user does not have access to them + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup("unauthz"+util.GenerateShortUID())))...) + + ac := acMock.New().WithPermissions(createPermissionsForRules(append(authorizedRulesInFolder, provisionedRulesInFolder...))) + + response := createServiceWithProvenanceStore(ac, ruleStore, scheduler, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, "") - response := svc.RouteDeleteAlertRules(request, folder.Title, "") require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) - assertRulesDeleted(t, rulesInFolder[1:], ruleStore, scheduler) + assertRulesDeleted(t, authorizedRulesInFolder, ruleStore, scheduler) }) - }) - t.Run("and user has access to data sources of some of alert rules", func(t *testing.T) { - t.Run("should delete only those that are accessible in folder", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - authorizedRulesInFolder := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder))) - ruleStore.PutRule(context.Background(), authorizedRulesInFolder...) + t.Run("return 400 if all rules user can access are provisioned", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + provisioningStore := provisioning.NewFakeProvisioningStore() + + provisionedRulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(util.GenerateShortUID()))) + err := provisioningStore.SetProvenance(context.Background(), provisionedRulesInFolder[0], orgID, models.ProvenanceAPI) + require.NoError(t, err) + + ruleStore.PutRule(context.Background(), provisionedRulesInFolder...) // more rules in the same namespace but user does not have access to them - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID)))...) + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(util.GenerateShortUID())))...) scheduler := &schedule.FakeScheduleService{} scheduler.On("DeleteAlertRule", mock.Anything) - ac := acMock.New().WithPermissions(createPermissionsForRules(authorizedRulesInFolder)) - request := createRequestContext(orgID, "None", nil) + ac := acMock.New().WithPermissions(createPermissionsForRules(provisionedRulesInFolder)) - response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "") - require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) - assertRulesDeleted(t, authorizedRulesInFolder, ruleStore, scheduler) + response := createServiceWithProvenanceStore(ac, ruleStore, scheduler, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, "") + + require.Equalf(t, 400, response.Status(), "Expected 400 but got %d: %v", response.Status(), string(response.Body())) + scheduler.AssertNotCalled(t, "DeleteAlertRule") + require.Empty(t, getRecordedCommand(ruleStore)) }) - t.Run("should delete only rules in a group that are authorized", func(t *testing.T) { - ruleStore := store.NewFakeRuleStore(t) - orgID := rand.Int63() - groupName := util.GenerateShortUID() - folder := randFolder() - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - authorizedRulesInGroup := models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName))) + }) + t.Run("and group argument is not empty", func(t *testing.T) { + groupName := util.GenerateShortUID() + t.Run("return 401 if user is not authorized to access the group", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + + authorizedRulesInGroup := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName))) ruleStore.PutRule(context.Background(), authorizedRulesInGroup...) // more rules in the same group but user is not authorized to access them - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName)))...) - // rules in different groups but in the same namespace - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...) - // rules in the same group but different folder - ruleStore.PutRule(context.Background(), models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withOrgID(orgID), withGroup(groupName)))...) + ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName)))...) scheduler := &schedule.FakeScheduleService{} scheduler.On("DeleteAlertRule", mock.Anything) ac := acMock.New().WithPermissions(createPermissionsForRules(authorizedRulesInGroup)) - request := createRequestContext(orgID, "None", nil) - response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, groupName) - require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) - assertRulesDeleted(t, authorizedRulesInGroup, ruleStore, scheduler) + + response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(requestCtx, folder.Title, groupName) + + require.Equalf(t, 401, response.Status(), "Expected 401 but got %d: %v", response.Status(), string(response.Body())) + scheduler.AssertNotCalled(t, "DeleteAlertRule", mock.Anything) + deleteCommands := getRecordedCommand(ruleStore) + require.Empty(t, deleteCommands) + }) + t.Run("return 400 if group is provisioned", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + provisioningStore := provisioning.NewFakeProvisioningStore() + + provisionedRulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName))) + err := provisioningStore.SetProvenance(context.Background(), provisionedRulesInFolder[0], orgID, models.ProvenanceAPI) + require.NoError(t, err) + + ruleStore.PutRule(context.Background(), provisionedRulesInFolder...) + + scheduler := &schedule.FakeScheduleService{} + scheduler.On("DeleteAlertRule", mock.Anything) + + ac := acMock.New().WithPermissions(createPermissionsForRules(provisionedRulesInFolder)) + + response := createServiceWithProvenanceStore(ac, ruleStore, scheduler, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, groupName) + + require.Equalf(t, 400, response.Status(), "Expected 400 but got %d: %v", response.Status(), string(response.Body())) + scheduler.AssertNotCalled(t, "DeleteAlertRule", mock.Anything) + deleteCommands := getRecordedCommand(ruleStore) + require.Empty(t, deleteCommands) }) }) }) @@ -645,6 +636,12 @@ func TestVerifyProvisionedRulesNotAffected(t *testing.T) { }) } +func createServiceWithProvenanceStore(ac *acMock.Mock, store *store.FakeRuleStore, scheduler schedule.ScheduleService, provenanceStore provisioning.ProvisioningStore) *RulerSrv { + svc := createService(ac, store, scheduler) + svc.provenanceStore = provenanceStore + return svc +} + func createService(ac *acMock.Mock, store *store.FakeRuleStore, scheduler schedule.ScheduleService) *RulerSrv { return &RulerSrv{ xactManager: store, diff --git a/pkg/services/ngalert/api/util.go b/pkg/services/ngalert/api/util.go index 3f4ff195c4f..543c08c7811 100644 --- a/pkg/services/ngalert/api/util.go +++ b/pkg/services/ngalert/api/util.go @@ -276,3 +276,16 @@ func ErrResp(status int, err error, msg string, args ...interface{}) *response.N func accessForbiddenResp() response.Response { return ErrResp(http.StatusForbidden, errors.New("Permission denied"), "") } + +func containsProvisionedAlerts(provenances map[string]ngmodels.Provenance, rules []*ngmodels.AlertRule) bool { + if len(provenances) == 0 { + return false + } + for _, rule := range rules { + provenance, ok := provenances[rule.UID] + if ok && provenance != ngmodels.ProvenanceNone { + return true + } + } + return false +} diff --git a/pkg/services/ngalert/api/util_test.go b/pkg/services/ngalert/api/util_test.go index a5acc9fb61e..34f4deb08e2 100644 --- a/pkg/services/ngalert/api/util_test.go +++ b/pkg/services/ngalert/api/util_test.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + models2 "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" @@ -156,3 +157,21 @@ func TestAlertingProxy_createProxyContext(t *testing.T) { }) }) } + +func Test_containsProvisionedAlerts(t *testing.T) { + t.Run("should return true if at least one rule is provisioned", func(t *testing.T) { + _, rules := models2.GenerateUniqueAlertRules(rand.Intn(4)+2, models2.AlertRuleGen()) + provenance := map[string]models2.Provenance{ + rules[rand.Intn(len(rules)-1)].UID: []models2.Provenance{models2.ProvenanceAPI, models2.ProvenanceFile}[rand.Intn(2)], + } + require.Truef(t, containsProvisionedAlerts(provenance, rules), "the group of rules is expected to be considered as provisioned but it isn't. Provenances: %v", provenance) + }) + t.Run("should return false if map does not contain or has ProvenanceNone", func(t *testing.T) { + _, rules := models2.GenerateUniqueAlertRules(rand.Intn(5)+1, models2.AlertRuleGen()) + provenance := make(map[string]models2.Provenance) + for i := 0; i < rand.Intn(len(rules)); i++ { + provenance[rules[i].UID] = models2.ProvenanceNone + } + require.Falsef(t, containsProvisionedAlerts(provenance, rules), "the group of rules is not expected to be provisioned but it is. Provenances: %v", provenance) + }) +} diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index f73380f51e1..33988cd9a76 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -188,6 +188,11 @@ func GenerateUniqueAlertRules(count int, f func() *AlertRule) (map[string]*Alert return uIDs, result } +// GenerateAlertRulesSmallNonEmpty generates 1 to 5 rules using the provided generator +func GenerateAlertRulesSmallNonEmpty(f func() *AlertRule) []*AlertRule { + return GenerateAlertRules(rand.Intn(4)+1, f) +} + // GenerateAlertRules generates many random alert rules. Does not guarantee that rules are unique (by UID) func GenerateAlertRules(count int, f func() *AlertRule) []*AlertRule { result := make([]*AlertRule, 0, count) diff --git a/pkg/services/ngalert/ngalert_test.go b/pkg/services/ngalert/ngalert_test.go index c76696471ec..a1911eb25b4 100644 --- a/pkg/services/ngalert/ngalert_test.go +++ b/pkg/services/ngalert/ngalert_test.go @@ -34,7 +34,7 @@ func Test_subscribeToFolderChanges(t *testing.T) { db.PutRule(context.Background(), rules...) scheduler := &schedule.FakeScheduleService{} - scheduler.EXPECT().UpdateAlertRule(mock.Anything, mock.Anything).Return() + scheduler.On("UpdateAlertRule", mock.Anything, mock.Anything).Return() subscribeToFolderChanges(log.New("test"), bus, db, scheduler) diff --git a/pkg/services/ngalert/schedule/schedule.go b/pkg/services/ngalert/schedule/schedule.go index 185ee91f66a..e7f8c6844dc 100644 --- a/pkg/services/ngalert/schedule/schedule.go +++ b/pkg/services/ngalert/schedule/schedule.go @@ -26,15 +26,15 @@ import ( // ScheduleService is an interface for a service that schedules the evaluation // of alert rules. -//go:generate mockery --name ScheduleService --structname FakeScheduleService --inpackage --filename schedule_mock.go --with-expecter +//go:generate mockery --name ScheduleService --structname FakeScheduleService --inpackage --filename schedule_mock.go --unroll-variadic=False type ScheduleService interface { // Run the scheduler until the context is canceled or the scheduler returns // an error. The scheduler is terminated when this function returns. Run(context.Context) error // UpdateAlertRule notifies scheduler that a rule has been changed UpdateAlertRule(key ngmodels.AlertRuleKey, lastVersion int64) - // DeleteAlertRule notifies scheduler that a rule has been changed - DeleteAlertRule(key ngmodels.AlertRuleKey) + // DeleteAlertRule notifies scheduler that rules have been deleted + DeleteAlertRule(keys ...ngmodels.AlertRuleKey) // the following are used by tests only used for tests evalApplied(ngmodels.AlertRuleKey, time.Time) stopApplied(ngmodels.AlertRuleKey) @@ -153,23 +153,23 @@ func (sch *schedule) UpdateAlertRule(key ngmodels.AlertRuleKey, lastVersion int6 } // DeleteAlertRule stops evaluation of the rule, deletes it from active rules, and cleans up state cache. -func (sch *schedule) DeleteAlertRule(key ngmodels.AlertRuleKey) { - // It can happen that the scheduler has deleted the alert rule before the - // Ruler API has called DeleteAlertRule. This can happen as requests to - // the Ruler API do not hold an exclusive lock over all scheduler operations. - if _, ok := sch.schedulableAlertRules.del(key); !ok { - sch.log.Info("alert rule cannot be removed from the scheduler as it is not scheduled", "uid", key.UID, "org_id", key.OrgID) - } - - // Delete the rule routine - ruleInfo, ok := sch.registry.del(key) - if !ok { - sch.log.Info("alert rule cannot be stopped as it is not running", "uid", key.UID, "org_id", key.OrgID) - return +func (sch *schedule) DeleteAlertRule(keys ...ngmodels.AlertRuleKey) { + for _, key := range keys { + // It can happen that the scheduler has deleted the alert rule before the + // Ruler API has called DeleteAlertRule. This can happen as requests to + // the Ruler API do not hold an exclusive lock over all scheduler operations. + if _, ok := sch.schedulableAlertRules.del(key); !ok { + sch.log.Info("alert rule cannot be removed from the scheduler as it is not scheduled", "uid", key.UID, "org_id", key.OrgID) + } + // Delete the rule routine + ruleInfo, ok := sch.registry.del(key) + if !ok { + sch.log.Info("alert rule cannot be stopped as it is not running", "uid", key.UID, "org_id", key.OrgID) + continue + } + // stop rule evaluation + ruleInfo.stop() } - // stop rule evaluation - ruleInfo.stop() - // Our best bet at this point is that we update the metrics with what we hope to schedule in the next tick. alertRules := sch.schedulableAlertRules.all() sch.metrics.SchedulableAlertRules.Set(float64(len(alertRules))) diff --git a/pkg/services/ngalert/schedule/schedule_mock.go b/pkg/services/ngalert/schedule/schedule_mock.go index 0c2c3839b45..720a7366813 100644 --- a/pkg/services/ngalert/schedule/schedule_mock.go +++ b/pkg/services/ngalert/schedule/schedule_mock.go @@ -16,40 +16,9 @@ type FakeScheduleService struct { mock.Mock } -type FakeScheduleService_Expecter struct { - mock *mock.Mock -} - -func (_m *FakeScheduleService) EXPECT() *FakeScheduleService_Expecter { - return &FakeScheduleService_Expecter{mock: &_m.Mock} -} - -// DeleteAlertRule provides a mock function with given fields: key -func (_m *FakeScheduleService) DeleteAlertRule(key models.AlertRuleKey) { - _m.Called(key) -} - -// FakeScheduleService_DeleteAlertRule_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteAlertRule' -type FakeScheduleService_DeleteAlertRule_Call struct { - *mock.Call -} - -// DeleteAlertRule is a helper method to define mock.On call -// - key models.AlertRuleKey -func (_e *FakeScheduleService_Expecter) DeleteAlertRule(key interface{}) *FakeScheduleService_DeleteAlertRule_Call { - return &FakeScheduleService_DeleteAlertRule_Call{Call: _e.mock.On("DeleteAlertRule", key)} -} - -func (_c *FakeScheduleService_DeleteAlertRule_Call) Run(run func(key models.AlertRuleKey)) *FakeScheduleService_DeleteAlertRule_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(models.AlertRuleKey)) - }) - return _c -} - -func (_c *FakeScheduleService_DeleteAlertRule_Call) Return() *FakeScheduleService_DeleteAlertRule_Call { - _c.Call.Return() - return _c +// DeleteAlertRule provides a mock function with given fields: keys +func (_m *FakeScheduleService) DeleteAlertRule(keys ...models.AlertRuleKey) { + _m.Called(keys) } // Run provides a mock function with given fields: _a0 @@ -66,139 +35,22 @@ func (_m *FakeScheduleService) Run(_a0 context.Context) error { return r0 } -// FakeScheduleService_Run_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Run' -type FakeScheduleService_Run_Call struct { - *mock.Call -} - -// Run is a helper method to define mock.On call -// - _a0 context.Context -func (_e *FakeScheduleService_Expecter) Run(_a0 interface{}) *FakeScheduleService_Run_Call { - return &FakeScheduleService_Run_Call{Call: _e.mock.On("Run", _a0)} -} - -func (_c *FakeScheduleService_Run_Call) Run(run func(_a0 context.Context)) *FakeScheduleService_Run_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context)) - }) - return _c -} - -func (_c *FakeScheduleService_Run_Call) Return(_a0 error) *FakeScheduleService_Run_Call { - _c.Call.Return(_a0) - return _c -} - // UpdateAlertRule provides a mock function with given fields: key, lastVersion func (_m *FakeScheduleService) UpdateAlertRule(key models.AlertRuleKey, lastVersion int64) { _m.Called(key, lastVersion) } -// FakeScheduleService_UpdateAlertRule_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'UpdateAlertRule' -type FakeScheduleService_UpdateAlertRule_Call struct { - *mock.Call -} - -// UpdateAlertRule is a helper method to define mock.On call -// - key models.AlertRuleKey -// - lastVersion int64 -func (_e *FakeScheduleService_Expecter) UpdateAlertRule(key interface{}, lastVersion interface{}) *FakeScheduleService_UpdateAlertRule_Call { - return &FakeScheduleService_UpdateAlertRule_Call{Call: _e.mock.On("UpdateAlertRule", key, lastVersion)} -} - -func (_c *FakeScheduleService_UpdateAlertRule_Call) Run(run func(key models.AlertRuleKey, lastVersion int64)) *FakeScheduleService_UpdateAlertRule_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(models.AlertRuleKey), args[1].(int64)) - }) - return _c -} - -func (_c *FakeScheduleService_UpdateAlertRule_Call) Return() *FakeScheduleService_UpdateAlertRule_Call { - _c.Call.Return() - return _c -} - // evalApplied provides a mock function with given fields: _a0, _a1 func (_m *FakeScheduleService) evalApplied(_a0 models.AlertRuleKey, _a1 time.Time) { _m.Called(_a0, _a1) } -// FakeScheduleService_evalApplied_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'evalApplied' -type FakeScheduleService_evalApplied_Call struct { - *mock.Call -} - -// evalApplied is a helper method to define mock.On call -// - _a0 models.AlertRuleKey -// - _a1 time.Time -func (_e *FakeScheduleService_Expecter) evalApplied(_a0 interface{}, _a1 interface{}) *FakeScheduleService_evalApplied_Call { - return &FakeScheduleService_evalApplied_Call{Call: _e.mock.On("evalApplied", _a0, _a1)} -} - -func (_c *FakeScheduleService_evalApplied_Call) Run(run func(_a0 models.AlertRuleKey, _a1 time.Time)) *FakeScheduleService_evalApplied_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(models.AlertRuleKey), args[1].(time.Time)) - }) - return _c -} - -func (_c *FakeScheduleService_evalApplied_Call) Return() *FakeScheduleService_evalApplied_Call { - _c.Call.Return() - return _c -} - // overrideCfg provides a mock function with given fields: cfg func (_m *FakeScheduleService) overrideCfg(cfg SchedulerCfg) { _m.Called(cfg) } -// FakeScheduleService_overrideCfg_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'overrideCfg' -type FakeScheduleService_overrideCfg_Call struct { - *mock.Call -} - -// overrideCfg is a helper method to define mock.On call -// - cfg SchedulerCfg -func (_e *FakeScheduleService_Expecter) overrideCfg(cfg interface{}) *FakeScheduleService_overrideCfg_Call { - return &FakeScheduleService_overrideCfg_Call{Call: _e.mock.On("overrideCfg", cfg)} -} - -func (_c *FakeScheduleService_overrideCfg_Call) Run(run func(cfg SchedulerCfg)) *FakeScheduleService_overrideCfg_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(SchedulerCfg)) - }) - return _c -} - -func (_c *FakeScheduleService_overrideCfg_Call) Return() *FakeScheduleService_overrideCfg_Call { - _c.Call.Return() - return _c -} - // stopApplied provides a mock function with given fields: _a0 func (_m *FakeScheduleService) stopApplied(_a0 models.AlertRuleKey) { _m.Called(_a0) } - -// FakeScheduleService_stopApplied_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'stopApplied' -type FakeScheduleService_stopApplied_Call struct { - *mock.Call -} - -// stopApplied is a helper method to define mock.On call -// - _a0 models.AlertRuleKey -func (_e *FakeScheduleService_Expecter) stopApplied(_a0 interface{}) *FakeScheduleService_stopApplied_Call { - return &FakeScheduleService_stopApplied_Call{Call: _e.mock.On("stopApplied", _a0)} -} - -func (_c *FakeScheduleService_stopApplied_Call) Run(run func(_a0 models.AlertRuleKey)) *FakeScheduleService_stopApplied_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(models.AlertRuleKey)) - }) - return _c -} - -func (_c *FakeScheduleService_stopApplied_Call) Return() *FakeScheduleService_stopApplied_Call { - _c.Call.Return() - return _c -}