diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index fa6d6cde63f..b8df359a58d 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -22,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/provisioning" "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/quota" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -302,9 +303,7 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *contextmodel.ReqContext, ruleGro return toNamespaceErrorResponse(err) } - rules, err := validateRuleGroup(&ruleGroupConfig, c.SignedInUser.OrgID, namespace, func(condition ngmodels.Condition) error { - return srv.conditionValidator.Validate(eval.NewContext(c.Req.Context(), c.SignedInUser), condition) - }, srv.cfg) + rules, err := validateRuleGroup(&ruleGroupConfig, c.SignedInUser.OrgID, namespace, srv.cfg) if err != nil { return ErrResp(http.StatusBadRequest, err, "") } @@ -343,6 +342,10 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *contextmodel.ReqContext, groupKey return err } + if err := validateQueries(c.Req.Context(), groupChanges, srv.conditionValidator, c.SignedInUser); err != nil { + return err + } + if err := verifyProvisionedRulesNotAffected(c.Req.Context(), srv.provenanceStore, c.OrgID, groupChanges); err != nil { return err } @@ -508,3 +511,23 @@ func verifyProvisionedRulesNotAffected(ctx context.Context, provenanceStore prov } return fmt.Errorf("%w: alert rule group [%s]", errProvisionedResource, errorMsg.String()) } + +func validateQueries(ctx context.Context, groupChanges *store.GroupDelta, validator ConditionValidator, user *user.SignedInUser) error { + if len(groupChanges.New) > 0 { + for _, rule := range groupChanges.New { + err := validator.Validate(eval.NewContext(ctx, user), rule.GetEvalCondition()) + if err != nil { + return fmt.Errorf("%w '%s': %s", ngmodels.ErrAlertRuleFailedValidation, rule.Title, err.Error()) + } + } + } + if len(groupChanges.Update) > 0 { + for _, upd := range groupChanges.Update { + err := validator.Validate(eval.NewContext(ctx, user), upd.New.GetEvalCondition()) + if err != nil { + return fmt.Errorf("%w '%s' (UID: %s): %s", ngmodels.ErrAlertRuleFailedValidation, upd.New.Title, upd.New.UID, err.Error()) + } + } + } + return nil +} diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index c906c6d82b3..6256f75207c 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -3,6 +3,7 @@ package api import ( "context" "encoding/json" + "errors" "fmt" "math/rand" "net/http" @@ -518,6 +519,71 @@ func TestVerifyProvisionedRulesNotAffected(t *testing.T) { }) } +func TestValidateQueries(t *testing.T) { + delta := store.GroupDelta{ + New: []*models.AlertRule{ + models.AlertRuleGen(func(rule *models.AlertRule) { + rule.Condition = "New" + })(), + }, + Update: []store.RuleDelta{ + { + Existing: models.AlertRuleGen(func(rule *models.AlertRule) { + rule.Condition = "Update_Existing" + })(), + New: models.AlertRuleGen(func(rule *models.AlertRule) { + rule.Condition = "Update_New" + })(), + Diff: nil, + }, + }, + Delete: []*models.AlertRule{ + models.AlertRuleGen(func(rule *models.AlertRule) { + rule.Condition = "Deleted" + })(), + }, + } + + t.Run("should validate New and Updated only", func(t *testing.T) { + validator := &recordingConditionValidator{} + err := validateQueries(context.Background(), &delta, validator, nil) + require.NoError(t, err) + for _, condition := range validator.recorded { + if condition.Condition == "New" || condition.Condition == "Update_New" { + continue + } + assert.Failf(t, "validated unexpected condition", "condition '%s' was validated but should not", condition.Condition) + } + }) + t.Run("should return rule validate error if fails on new rule", func(t *testing.T) { + validator := &recordingConditionValidator{ + hook: func(c models.Condition) error { + if c.Condition == "New" { + return errors.New("test") + } + return nil + }, + } + err := validateQueries(context.Background(), &delta, validator, nil) + require.Error(t, err) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + }) + t.Run("should return rule validate error with UID if fails on updated rule", func(t *testing.T) { + validator := &recordingConditionValidator{ + hook: func(c models.Condition) error { + if c.Condition == "Update_New" { + return errors.New("test") + } + return nil + }, + } + err := validateQueries(context.Background(), &delta, validator, nil) + require.Error(t, err) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, delta.Update[0].New.UID) + }) +} + func createServiceWithProvenanceStore(store *fakes.RuleStore, provenanceStore provisioning.ProvisioningStore) *RulerSrv { svc := createService(store) svc.provenanceStore = provenanceStore diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index f6969bddf0a..aaed3f3faf7 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -3,6 +3,7 @@ package api import ( "errors" "fmt" + "strings" "time" "github.com/grafana/grafana/pkg/services/folder" @@ -19,7 +20,6 @@ func validateRuleNode( interval time.Duration, orgId int64, namespace *folder.Folder, - conditionValidator func(ngmodels.Condition) error, cfg *setting.UnifiedAlertingSettings) (*ngmodels.AlertRule, error) { intervalSeconds, err := validateInterval(cfg, interval) if err != nil { @@ -74,18 +74,14 @@ func validateRuleNode( } else { return nil, fmt.Errorf("%w: no queries or expressions are found", ngmodels.ErrAlertRuleFailedValidation) } + } else { + err = validateCondition(ruleNode.GrafanaManagedAlert.Condition, ruleNode.GrafanaManagedAlert.Data) + if err != nil { + return nil, fmt.Errorf("%w: %s", ngmodels.ErrAlertRuleFailedValidation, err.Error()) + } } queries := AlertQueriesFromApiAlertQueries(ruleNode.GrafanaManagedAlert.Data) - if len(queries) != 0 { - cond := ngmodels.Condition{ - Condition: ruleNode.GrafanaManagedAlert.Condition, - Data: queries, - } - if err = conditionValidator(cond); err != nil { - return nil, fmt.Errorf("failed to validate condition of alert rule %s: %w", ruleNode.GrafanaManagedAlert.Title, err) - } - } newAlertRule := ngmodels.AlertRule{ OrgID: orgId, @@ -117,6 +113,33 @@ func validateRuleNode( return &newAlertRule, nil } +func validateCondition(condition string, queries []apimodels.AlertQuery) error { + if condition == "" { + return errors.New("condition cannot be empty") + } + if len(queries) == 0 { + return errors.New("no query/expressions specified") + } + refIDs := make(map[string]int, len(queries)) + for idx, query := range queries { + if query.RefID == "" { + return fmt.Errorf("refID is not specified for data query/expression at index %d", idx) + } + if usedIdx, ok := refIDs[query.RefID]; ok { + return fmt.Errorf("refID '%s' is already used by query/expression at index %d", query.RefID, usedIdx) + } + refIDs[query.RefID] = idx + } + if _, ok := refIDs[condition]; !ok { + ids := make([]string, 0, len(refIDs)) + for id := range refIDs { + ids = append(ids, id) + } + return fmt.Errorf("condition %s does not exist, must be one of [%s]", condition, strings.Join(ids, ",")) + } + return nil +} + func validateInterval(cfg *setting.UnifiedAlertingSettings, interval time.Duration) (int64, error) { intervalSeconds := int64(interval.Seconds()) @@ -155,7 +178,6 @@ func validateRuleGroup( ruleGroupConfig *apimodels.PostableRuleGroupConfig, orgId int64, namespace *folder.Folder, - conditionValidator func(ngmodels.Condition) error, cfg *setting.UnifiedAlertingSettings) ([]*ngmodels.AlertRuleWithOptionals, error) { if ruleGroupConfig.Name == "" { return nil, errors.New("rule group name cannot be empty") @@ -180,7 +202,7 @@ func validateRuleGroup( result := make([]*ngmodels.AlertRuleWithOptionals, 0, len(ruleGroupConfig.Rules)) uids := make(map[string]int, cap(result)) for idx := range ruleGroupConfig.Rules { - rule, err := validateRuleNode(&ruleGroupConfig.Rules[idx], ruleGroupConfig.Name, interval, orgId, namespace, conditionValidator, cfg) + rule, err := validateRuleNode(&ruleGroupConfig.Rules[idx], ruleGroupConfig.Name, interval, orgId, namespace, cfg) // TODO do not stop on the first failure but return all failures if err != nil { return nil, fmt.Errorf("invalid rule specification at index [%d]: %w", idx, err) diff --git a/pkg/services/ngalert/api/api_ruler_validation_test.go b/pkg/services/ngalert/api/api_ruler_validation_test.go index b957877ec4d..03e38b78326 100644 --- a/pkg/services/ngalert/api/api_ruler_validation_test.go +++ b/pkg/services/ngalert/api/api_ruler_validation_test.go @@ -1,12 +1,12 @@ package api import ( - "errors" "fmt" "strconv" "testing" "time" + "github.com/google/uuid" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" "golang.org/x/exp/rand" @@ -98,6 +98,90 @@ func randFolder() *folder.Folder { } } +func TestValidateCondition(t *testing.T) { + testcases := []struct { + name string + condition string + data []apimodels.AlertQuery + errorMsg string + }{ + { + name: "error when condition is empty", + condition: "", + data: []apimodels.AlertQuery{}, + errorMsg: "condition cannot be empty", + }, + { + name: "error when data is empty", + condition: "A", + data: []apimodels.AlertQuery{}, + errorMsg: "no query/expressions specified", + }, + { + name: "error when condition does not exist", + condition: "A", + data: []apimodels.AlertQuery{ + { + RefID: "B", + }, + { + RefID: "C", + }, + }, + errorMsg: "condition A does not exist, must be one of [B,C]", + }, + { + name: "error when duplicated refId", + condition: "A", + data: []apimodels.AlertQuery{ + { + RefID: "A", + }, + { + RefID: "A", + }, + }, + errorMsg: "refID 'A' is already used by query/expression at index 0", + }, + { + name: "error when refId is empty", + condition: "A", + data: []apimodels.AlertQuery{ + { + RefID: "", + }, + { + RefID: "A", + }, + }, + errorMsg: "refID is not specified for data query/expression at index 0", + }, + { + name: "valid case", + condition: "B", + data: []apimodels.AlertQuery{ + { + RefID: "A", + }, + { + RefID: "B", + }, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := validateCondition(tc.condition, tc.data) + if tc.errorMsg == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tc.errorMsg) + } + }) + } +} + func TestValidateRuleGroup(t *testing.T) { orgId := rand.Int63() folder := randFolder() @@ -110,22 +194,15 @@ func TestValidateRuleGroup(t *testing.T) { t.Run("should validate struct and rules", func(t *testing.T) { g := validGroup(cfg, rules...) - conditionValidations := 0 - alerts, err := validateRuleGroup(&g, orgId, folder, func(condition models.Condition) error { - conditionValidations++ - return nil - }, cfg) + alerts, err := validateRuleGroup(&g, orgId, folder, cfg) require.NoError(t, err) require.Len(t, alerts, len(rules)) - require.Equal(t, len(rules), conditionValidations) }) t.Run("should default to default interval from config if group interval is 0", func(t *testing.T) { g := validGroup(cfg, rules...) g.Interval = 0 - alerts, err := validateRuleGroup(&g, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + alerts, err := validateRuleGroup(&g, orgId, folder, cfg) require.NoError(t, err) for _, alert := range alerts { require.Equal(t, int64(cfg.DefaultRuleEvaluationInterval.Seconds()), alert.IntervalSeconds) @@ -140,9 +217,7 @@ func TestValidateRuleGroup(t *testing.T) { isPaused = !(isPaused) } g := validGroup(cfg, rules...) - alerts, err := validateRuleGroup(&g, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + alerts, err := validateRuleGroup(&g, orgId, folder, cfg) require.NoError(t, err) for _, alert := range alerts { require.True(t, alert.HasPause) @@ -214,9 +289,7 @@ func TestValidateRuleGroupFailures(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { g := testCase.group() - _, err := validateRuleGroup(g, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + _, err := validateRuleGroup(g, orgId, folder, cfg) require.Error(t, err) if testCase.assert != nil { testCase.assert(t, g, err) @@ -323,9 +396,7 @@ func TestValidateRuleNode_NoUID(t *testing.T) { r := testCase.rule() r.GrafanaManagedAlert.UID = "" - alert, err := validateRuleNode(r, name, interval, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + alert, err := validateRuleNode(r, name, interval, orgId, folder, cfg) require.NoError(t, err) testCase.assert(t, r, alert) }) @@ -333,9 +404,7 @@ func TestValidateRuleNode_NoUID(t *testing.T) { t.Run("accepts empty group name", func(t *testing.T) { r := validRule() - alert, err := validateRuleNode(&r, "", interval, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + alert, err := validateRuleNode(&r, "", interval, orgId, folder, cfg) require.NoError(t, err) require.Equal(t, "", alert.RuleGroup) }) @@ -345,17 +414,13 @@ func TestValidateRuleNodeFailures_NoUID(t *testing.T) { orgId := rand.Int63() folder := randFolder() cfg := config(t) - successValidation := func(condition models.Condition) error { - return nil - } testCases := []struct { - name string - interval *time.Duration - rule func() *apimodels.PostableExtendedRuleNode - conditionValidation func(condition models.Condition) error - assert func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error) - allowedIfNoUId bool + name string + interval *time.Duration + rule func() *apimodels.PostableExtendedRuleNode + assert func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error) + allowedIfNoUId bool }{ { name: "fail if GrafanaManagedAlert is not specified", @@ -415,16 +480,6 @@ func TestValidateRuleNodeFailures_NoUID(t *testing.T) { return &r }, }, - { - name: "fail if validator function returns error", - rule: func() *apimodels.PostableExtendedRuleNode { - r := validRule() - return &r - }, - conditionValidation: func(condition models.Condition) error { - return errors.New("BAD alert condition") - }, - }, { name: "fail if Dashboard UID is specified but not Panel ID", rule: func() *apimodels.PostableExtendedRuleNode { @@ -456,6 +511,38 @@ func TestValidateRuleNodeFailures_NoUID(t *testing.T) { return &r }, }, + { + name: "fail if Condition is empty", + rule: func() *apimodels.PostableExtendedRuleNode { + r := validRule() + r.GrafanaManagedAlert.Condition = "" + return &r + }, + }, + { + name: "fail if Data is empty", + rule: func() *apimodels.PostableExtendedRuleNode { + r := validRule() + r.GrafanaManagedAlert.Data = nil + return &r + }, + }, + { + name: "fail if Condition does not exist", + rule: func() *apimodels.PostableExtendedRuleNode { + r := validRule() + r.GrafanaManagedAlert.Condition = uuid.NewString() + return &r + }, + }, + { + name: "fail if Data has duplicate ref ID", + rule: func() *apimodels.PostableExtendedRuleNode { + r := validRule() + r.GrafanaManagedAlert.Data = append(r.GrafanaManagedAlert.Data, r.GrafanaManagedAlert.Data...) + return &r + }, + }, } for _, testCase := range testCases { @@ -464,17 +551,13 @@ func TestValidateRuleNodeFailures_NoUID(t *testing.T) { if r.GrafanaManagedAlert != nil { r.GrafanaManagedAlert.UID = "" } - f := successValidation - if testCase.conditionValidation != nil { - f = testCase.conditionValidation - } interval := cfg.BaseInterval if testCase.interval != nil { interval = *testCase.interval } - _, err := validateRuleNode(r, "", interval, orgId, folder, f, cfg) + _, err := validateRuleNode(r, "", interval, orgId, folder, cfg) require.Error(t, err) if testCase.assert != nil { testCase.assert(t, r, err) @@ -566,9 +649,7 @@ func TestValidateRuleNode_UID(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { r := testCase.rule() - alert, err := validateRuleNode(r, name, interval, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + alert, err := validateRuleNode(r, name, interval, orgId, folder, cfg) require.NoError(t, err) testCase.assert(t, r, alert) }) @@ -576,9 +657,7 @@ func TestValidateRuleNode_UID(t *testing.T) { t.Run("accepts empty group name", func(t *testing.T) { r := validRule() - alert, err := validateRuleNode(&r, "", interval, orgId, folder, func(condition models.Condition) error { - return nil - }, cfg) + alert, err := validateRuleNode(&r, "", interval, orgId, folder, cfg) require.NoError(t, err) require.Equal(t, "", alert.RuleGroup) }) @@ -588,16 +667,12 @@ func TestValidateRuleNodeFailures_UID(t *testing.T) { orgId := rand.Int63() folder := randFolder() cfg := config(t) - successValidation := func(condition models.Condition) error { - return nil - } testCases := []struct { - name string - interval *time.Duration - rule func() *apimodels.PostableExtendedRuleNode - conditionValidation func(condition models.Condition) error - assert func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error) + name string + interval *time.Duration + rule func() *apimodels.PostableExtendedRuleNode + assert func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error) }{ { name: "fail if GrafanaManagedAlert is not specified", @@ -635,16 +710,6 @@ func TestValidateRuleNodeFailures_UID(t *testing.T) { return &r }, }, - { - name: "fail if validator function returns error", - rule: func() *apimodels.PostableExtendedRuleNode { - r := validRule() - return &r - }, - conditionValidation: func(condition models.Condition) error { - return errors.New("BAD alert condition") - }, - }, { name: "fail if Dashboard UID is specified but not Panel ID", rule: func() *apimodels.PostableExtendedRuleNode { @@ -681,17 +746,13 @@ func TestValidateRuleNodeFailures_UID(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { r := testCase.rule() - f := successValidation - if testCase.conditionValidation != nil { - f = testCase.conditionValidation - } interval := cfg.BaseInterval if testCase.interval != nil { interval = *testCase.interval } - _, err := validateRuleNode(r, "", interval, orgId, folder, f, cfg) + _, err := validateRuleNode(r, "", interval, orgId, folder, cfg) require.Error(t, err) if testCase.assert != nil { testCase.assert(t, r, err) @@ -724,11 +785,7 @@ func TestValidateRuleNodeIntervalFailures(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { r := validRule() - f := func(condition models.Condition) error { - return nil - } - - _, err := validateRuleNode(&r, util.GenerateShortUID(), testCase.interval, rand.Int63(), randFolder(), f, cfg) + _, err := validateRuleNode(&r, util.GenerateShortUID(), testCase.interval, rand.Int63(), randFolder(), cfg) require.Error(t, err) }) } diff --git a/pkg/services/ngalert/api/api_testing.go b/pkg/services/ngalert/api/api_testing.go index 9acbc01d07b..e82506a52f6 100644 --- a/pkg/services/ngalert/api/api_testing.go +++ b/pkg/services/ngalert/api/api_testing.go @@ -55,9 +55,6 @@ func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *contextmodel.ReqContext, UID: body.NamespaceUID, Title: body.NamespaceTitle, }, - func(condition ngmodels.Condition) error { - return srv.evaluator.Validate(eval.NewContext(c.Req.Context(), c.SignedInUser), condition) - }, srv.cfg, ) if err != nil { diff --git a/pkg/services/ngalert/api/api_testing_test.go b/pkg/services/ngalert/api/api_testing_test.go index dde667a7690..40d16bb4ea3 100644 --- a/pkg/services/ngalert/api/api_testing_test.go +++ b/pkg/services/ngalert/api/api_testing_test.go @@ -147,6 +147,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { rule := validRule() rule.GrafanaManagedAlert.Data = ApiAlertQueriesFromAlertQueries([]models.AlertQuery{data1, data2}) + rule.GrafanaManagedAlert.Condition = data2.RefID response := srv.RouteTestGrafanaRuleConfig(rc, definitions.PostableExtendedRuleNodeExtended{ Rule: rule, NamespaceUID: "test-folder", @@ -180,6 +181,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { rule := validRule() rule.GrafanaManagedAlert.Data = ApiAlertQueriesFromAlertQueries([]models.AlertQuery{data1, data2}) + rule.GrafanaManagedAlert.Condition = data2.RefID response := srv.RouteTestGrafanaRuleConfig(rc, definitions.PostableExtendedRuleNodeExtended{ Rule: rule, NamespaceUID: "test-folder", diff --git a/pkg/services/ngalert/api/util_test.go b/pkg/services/ngalert/api/util_test.go index 7fea2c987f4..8933cc60164 100644 --- a/pkg/services/ngalert/api/util_test.go +++ b/pkg/services/ngalert/api/util_test.go @@ -13,6 +13,7 @@ import ( accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/auth" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/services/ngalert/eval" models2 "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" @@ -159,3 +160,18 @@ func Test_containsProvisionedAlerts(t *testing.T) { require.Falsef(t, containsProvisionedAlerts(provenance, rules), "the group of rules is not expected to be provisioned but it is. Provenances: %v", provenance) }) } + +type recordingConditionValidator struct { + recorded []models2.Condition + hook func(c models2.Condition) error +} + +func (r *recordingConditionValidator) Validate(_ eval.EvaluationContext, condition models2.Condition) error { + r.recorded = append(r.recorded, condition) + if r.hook != nil { + return r.hook(condition) + } + return nil +} + +var _ ConditionValidator = &recordingConditionValidator{} diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 5602c9d4670..8aa221b1946 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -875,6 +875,7 @@ func TestIntegrationAlertRuleCRUD(t *testing.T) { rulegroup string interval model.Duration rule apimodels.PostableExtendedRuleNode + expectedCode int expectedMessage string }{ { @@ -1042,7 +1043,18 @@ func TestIntegrationAlertRuleCRUD(t *testing.T) { }, }, }, - expectedMessage: "invalid rule specification at index [0]: failed to validate condition of alert rule AlwaysFiring: failed to build query 'A': data source not found", + expectedCode: func() int { + if setting.IsEnterprise { + return http.StatusUnauthorized + } + return http.StatusBadRequest + }(), + expectedMessage: func() string { + if setting.IsEnterprise { + return "user is not authorized to create a new alert rule 'AlwaysFiring' because the user does not have read permissions for one or many datasources the rule uses" + } + return "failed to update rule group: invalid alert rule 'AlwaysFiring': failed to build query 'A': data source not found" + }(), }, { desc: "alert rule with invalid condition", @@ -1072,7 +1084,7 @@ func TestIntegrationAlertRuleCRUD(t *testing.T) { }, }, }, - expectedMessage: "invalid rule specification at index [0]: failed to validate condition of alert rule AlwaysFiring: condition B does not exist, must be one of [A]", + expectedMessage: "invalid rule specification at index [0]: invalid alert rule: condition B does not exist, must be one of [A]", }, } @@ -1091,8 +1103,11 @@ func TestIntegrationAlertRuleCRUD(t *testing.T) { require.NoError(t, err) assert.Equal(t, tc.expectedMessage, res.Message) - - assert.Equal(t, http.StatusBadRequest, status) + expectedCode := tc.expectedCode + if expectedCode == 0 { + expectedCode = http.StatusBadRequest + } + assert.Equal(t, expectedCode, status) }) } } diff --git a/pkg/tests/api/alerting/api_ruler_test.go b/pkg/tests/api/alerting/api_ruler_test.go index e774a08068d..e4c492d07bb 100644 --- a/pkg/tests/api/alerting/api_ruler_test.go +++ b/pkg/tests/api/alerting/api_ruler_test.go @@ -1,6 +1,7 @@ package alerting import ( + "context" "encoding/json" "fmt" "io" @@ -9,16 +10,21 @@ import ( "testing" "time" - "github.com/grafana/grafana/pkg/expr" + "github.com/google/uuid" "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/expr" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" + "github.com/grafana/grafana/pkg/services/datasources" + datasourceService "github.com/grafana/grafana/pkg/services/datasources/service" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" ngmodels "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/setting" "github.com/grafana/grafana/pkg/tests/testinfra" "github.com/grafana/grafana/pkg/util" ) @@ -862,14 +868,40 @@ func TestIntegrationRuleUpdate(t *testing.T) { AppModeProduction: true, }) grafanaListedAddr, store := testinfra.StartGrafana(t, dir, path) + permissionsStore := resourcepermissions.NewStore(store) // Create a user to make authenticated requests - createUser(t, store, user.CreateUserCommand{ + userID := createUser(t, store, user.CreateUserCommand{ DefaultOrgRole: string(org.RoleEditor), Password: "password", Login: "grafana", }) + if setting.IsEnterprise { + // add blanket access to data sources. + _, err := permissionsStore.SetUserResourcePermission(context.Background(), + 1, + accesscontrol.User{ID: userID}, + resourcepermissions.SetResourcePermissionCommand{ + Actions: []string{ + datasources.ActionQuery, + }, + Resource: datasources.ScopeRoot, + ResourceID: "*", + ResourceAttribute: "uid", + }, nil) + require.NoError(t, err) + } + + // Create a user to make authenticated requests + createUser(t, store, user.CreateUserCommand{ + DefaultOrgRole: string(org.RoleAdmin), + Password: "admin", + Login: "admin", + }) + + adminClient := newAlertingApiClient(grafanaListedAddr, "admin", "admin") + client := newAlertingApiClient(grafanaListedAddr, "grafana", "password") folder1Title := "folder1" client.CreateFolder(t, util.GenerateShortUID(), folder1Title) @@ -893,6 +925,80 @@ func TestIntegrationRuleUpdate(t *testing.T) { getGroup = client.GetRulesGroup(t, folder1Title, group.Name) require.Equal(t, expected, *getGroup.Rules[0].ApiRuleNode.For) }) + t.Run("when data source missing", func(t *testing.T) { + var groupName string + { + ds1 := adminClient.CreateTestDatasource(t) + group := generateAlertRuleGroup(3, alertRuleGen(withDatasourceQuery(ds1.Body.Datasource.UID))) + + status, body := client.PostRulesGroup(t, folder1Title, &group) + require.Equalf(t, http.StatusAccepted, status, "failed to post rule group. Response: %s", body) + + getGroup := client.GetRulesGroup(t, folder1Title, group.Name) + group = convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + + require.Len(t, group.Rules, 3) + + adminClient.DeleteDatasource(t, ds1.Body.Datasource.UID) + + // expire datasource caching + <-time.After(datasourceService.DefaultCacheTTL + 1*time.Second) // TODO delete when TTL could be configured + + groupName = group.Name + } + + t.Run("noop should not fail", func(t *testing.T) { + getGroup := client.GetRulesGroup(t, folder1Title, groupName) + group := convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + + status, body := client.PostRulesGroup(t, folder1Title, &group) + require.Equalf(t, http.StatusAccepted, status, "failed to post noop rule group. Response: %s", body) + }) + t.Run("should not let update rule if it does not fix datasource", func(t *testing.T) { + getGroup := client.GetRulesGroup(t, folder1Title, groupName) + group := convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + + group.Rules[0].GrafanaManagedAlert.Title = uuid.NewString() + status, body := client.PostRulesGroup(t, folder1Title, &group) + + if status == http.StatusAccepted { + getGroup = client.GetRulesGroup(t, folder1Title, group.Name) + assert.NotEqualf(t, group.Rules[0].GrafanaManagedAlert.Title, getGroup.Rules[0].GrafanaManagedAlert.Title, "group was updated") + } + require.Equalf(t, http.StatusBadRequest, status, "expected BadRequest. Response: %s", body) + assert.Contains(t, body, "data source not found") + }) + t.Run("should let delete broken rule", func(t *testing.T) { + getGroup := client.GetRulesGroup(t, folder1Title, groupName) + group := convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + + // remove the last rule. + group.Rules = group.Rules[0 : len(group.Rules)-1] + status, body := client.PostRulesGroup(t, folder1Title, &group) + require.Equalf(t, http.StatusAccepted, status, "failed to delete last rule from group. Response: %s", body) + + getGroup = client.GetRulesGroup(t, folder1Title, group.Name) + group = convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + require.Len(t, group.Rules, 2) + }) + t.Run("should let fix single rule", func(t *testing.T) { + getGroup := client.GetRulesGroup(t, folder1Title, groupName) + group := convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + + ds2 := adminClient.CreateTestDatasource(t) + withDatasourceQuery(ds2.Body.Datasource.UID)(&group.Rules[0]) + status, body := client.PostRulesGroup(t, folder1Title, &group) + require.Equalf(t, http.StatusAccepted, status, "failed to post noop rule group. Response: %s", body) + + getGroup = client.GetRulesGroup(t, folder1Title, group.Name) + group = convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig) + require.Equal(t, ds2.Body.Datasource.UID, group.Rules[0].GrafanaManagedAlert.Data[0].DatasourceUID) + }) + t.Run("should let delete group", func(t *testing.T) { + status, body := client.DeleteRulesGroup(t, folder1Title, groupName) + require.Equalf(t, http.StatusAccepted, status, "failed to post noop rule group. Response: %s", body) + }) + }) } func newTestingRuleConfig(t *testing.T) apimodels.PostableRuleGroupConfig { diff --git a/pkg/tests/api/alerting/testing.go b/pkg/tests/api/alerting/testing.go index a06053eac08..5d517a022ef 100644 --- a/pkg/tests/api/alerting/testing.go +++ b/pkg/tests/api/alerting/testing.go @@ -10,12 +10,13 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/api" "github.com/grafana/grafana/pkg/expr" - apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/quota" @@ -89,10 +90,12 @@ func getBody(t *testing.T, body io.ReadCloser) string { return string(b) } -func alertRuleGen() func() apimodels.PostableExtendedRuleNode { +type ruleMutator func(r *apimodels.PostableExtendedRuleNode) + +func alertRuleGen(mutators ...ruleMutator) func() apimodels.PostableExtendedRuleNode { return func() apimodels.PostableExtendedRuleNode { forDuration := model.Duration(10 * time.Second) - return apimodels.PostableExtendedRuleNode{ + rule := apimodels.PostableExtendedRuleNode{ ApiRuleNode: &apimodels.ApiRuleNode{ For: &forDuration, Labels: map[string]string{"label1": "val1"}, @@ -117,6 +120,69 @@ func alertRuleGen() func() apimodels.PostableExtendedRuleNode { }, }, } + + for _, mutator := range mutators { + mutator(&rule) + } + return rule + } +} + +func withDatasourceQuery(uid string) func(r *apimodels.PostableExtendedRuleNode) { + data := []apimodels.AlertQuery{ + { + RefID: "A", + RelativeTimeRange: apimodels.RelativeTimeRange{ + From: apimodels.Duration(600 * time.Second), + To: 0, + }, + DatasourceUID: uid, + Model: json.RawMessage(fmt.Sprintf(`{ + "refId": "A", + "hide": false, + "datasource": { + "type": "testdata", + "uid": "%s" + }, + "scenarioId": "random_walk", + "seriesCount": 5, + "labels": "series=series-$seriesIndex" + }`, uid)), + }, + { + RefID: "B", + DatasourceUID: expr.DatasourceType, + Model: json.RawMessage(`{ + "type": "reduce", + "reducer": "last", + "expression": "A" + }`), + }, + { + RefID: "C", + DatasourceUID: expr.DatasourceType, + Model: json.RawMessage(`{ + "refId": "C", + "type": "threshold", + "conditions": [ + { + "type": "query", + "evaluator": { + "params": [ + 0 + ], + "type": "gt" + } + } + ], + "expression": "B" + }`), + }, + } + + return func(r *apimodels.PostableExtendedRuleNode) { + r.GrafanaManagedAlert.Data = data + r.GrafanaManagedAlert.Condition = "C" } } @@ -126,7 +192,7 @@ func generateAlertRuleGroup(rulesCount int, gen func() apimodels.PostableExtende rules = append(rules, gen()) } return apimodels.PostableRuleGroupConfig{ - Name: "arulegroup-" + util.GenerateShortUID(), + Name: "arulegroup-" + uuid.NewString(), Interval: model.Duration(10 * time.Second), Rules: rules, } @@ -280,6 +346,24 @@ func (a apiClient) PostRulesGroup(t *testing.T, folder string, group *apimodels. return resp.StatusCode, string(b) } +func (a apiClient) DeleteRulesGroup(t *testing.T, folder string, group string) (int, string) { + t.Helper() + + u := fmt.Sprintf("%s/api/ruler/grafana/api/v1/rules/%s/%s", a.url, folder, group) + req, err := http.NewRequest(http.MethodDelete, u, nil) + require.NoError(t, err) + client := &http.Client{} + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { + _ = resp.Body.Close() + }() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + return resp.StatusCode, string(b) +} + func (a apiClient) GetRulesGroup(t *testing.T, folder string, group string) apimodels.RuleGroupConfigResponse { t.Helper() u := fmt.Sprintf("%s/api/ruler/grafana/api/v1/rules/%s/%s", a.url, folder, group) @@ -353,3 +437,49 @@ func (a apiClient) SubmitRuleForTesting(t *testing.T, config apimodels.PostableE require.NoError(t, err) return resp.StatusCode, string(b) } + +func (a apiClient) CreateTestDatasource(t *testing.T) (result api.CreateOrUpdateDatasourceResponse) { + t.Helper() + + payload := fmt.Sprintf(`{"name":"TestData-%s","type":"testdata","access":"proxy","isDefault":false}`, uuid.NewString()) + buf := bytes.Buffer{} + buf.Write([]byte(payload)) + + u := fmt.Sprintf("%s/api/datasources", a.url) + + // nolint:gosec + resp, err := http.Post(u, "application/json", &buf) + require.NoError(t, err) + defer func() { + _ = resp.Body.Close() + }() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + if resp.StatusCode != 200 { + require.Failf(t, "failed to create data source", "API request to create a datasource failed. Status code: %d, response: %s", resp.StatusCode, string(b)) + } + require.NoError(t, json.Unmarshal([]byte(fmt.Sprintf(`{ "body": %s }`, string(b))), &result)) + return result +} + +func (a apiClient) DeleteDatasource(t *testing.T, uid string) { + t.Helper() + + u := fmt.Sprintf("%s/api/datasources/uid/%s", a.url, uid) + + req, err := http.NewRequest(http.MethodDelete, u, nil) + require.NoError(t, err) + client := &http.Client{} + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { + _ = resp.Body.Close() + }() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + if resp.StatusCode != 200 { + require.Failf(t, "failed to create data source", "API request to create a datasource failed. Status code: %d, response: %s", resp.StatusCode, string(b)) + } +}