From 7ccb022c03bd10124d1a92b69c8a6a94358a22cc Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki Date: Wed, 28 Apr 2021 11:31:51 +0300 Subject: [PATCH] Alerting: validate condition before updating rulegroup (#33367) * Alerting: validate condition before updating rulegroup * Apply suggestions from code review --- pkg/services/ngalert/api/api.go | 2 +- pkg/services/ngalert/api/api_ruler.go | 18 ++++- pkg/services/ngalert/api/api_testing.go | 3 +- pkg/services/ngalert/api/util.go | 50 +++++-------- .../api/alerting/api_alertmanager_test.go | 70 +++++++++++++++++-- 5 files changed, 102 insertions(+), 41 deletions(-) diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 47de6e7dbed..0d6e1c83f14 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -84,7 +84,7 @@ func (api *API) RegisterAPIEndpoints() { api.RegisterRulerApiEndpoints(NewForkedRuler( api.DatasourceCache, NewLotexRuler(proxy, logger), - RulerSrv{store: api.RuleStore, log: logger}, + RulerSrv{DatasourceCache: api.DatasourceCache, store: api.RuleStore, log: logger}, reg, )) api.RegisterTestingApiEndpoints(TestingApiSrv{ diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 8df9894f5cd..97804c7387a 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -2,9 +2,11 @@ package api import ( "errors" + "fmt" "net/http" "time" + "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/ngalert/store" coreapi "github.com/grafana/grafana/pkg/api" @@ -18,8 +20,9 @@ import ( ) type RulerSrv struct { - store store.RuleStore - log log.Logger + store store.RuleStore + DatasourceCache datasources.CacheService + log log.Logger } func (srv RulerSrv) RouteDeleteNamespaceRulesConfig(c *models.ReqContext) response.Response { @@ -196,6 +199,17 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *models.ReqContext, ruleGroupConf return response.Error(http.StatusBadRequest, "rule group name is not valid", nil) } + for _, r := range ruleGroupConfig.Rules { + cond := ngmodels.Condition{ + Condition: r.GrafanaManagedAlert.Condition, + OrgID: c.SignedInUser.OrgId, + Data: r.GrafanaManagedAlert.Data, + } + if err := validateCondition(cond, c.SignedInUser, c.SkipCache, srv.DatasourceCache); err != nil { + return response.Error(http.StatusBadRequest, fmt.Sprintf("failed to validate alert rule %s", r.GrafanaManagedAlert.Title), err) + } + } + if err := srv.store.UpdateRuleGroup(store.UpdateRuleGroupCmd{ OrgID: c.SignedInUser.OrgId, NamespaceUID: namespace.Uid, diff --git a/pkg/services/ngalert/api/api_testing.go b/pkg/services/ngalert/api/api_testing.go index e00ddbc27be..15f44609098 100644 --- a/pkg/services/ngalert/api/api_testing.go +++ b/pkg/services/ngalert/api/api_testing.go @@ -84,7 +84,8 @@ func (srv TestingApiSrv) RouteEvalQueries(c *models.ReqContext, cmd apimodels.Ev if now.IsZero() { now = timeNow() } - if err := validateQueriesAndExpressions(cmd.Data, c.SignedInUser, c.SkipCache, srv.DatasourceCache); err != nil { + + if _, err := validateQueriesAndExpressions(cmd.Data, c.SignedInUser, c.SkipCache, srv.DatasourceCache); err != nil { return response.Error(http.StatusBadRequest, "invalid queries or expressions", err) } diff --git a/pkg/services/ngalert/api/util.go b/pkg/services/ngalert/api/util.go index b1c2c7a4b0e..7cfc104a418 100644 --- a/pkg/services/ngalert/api/util.go +++ b/pkg/services/ngalert/api/util.go @@ -158,67 +158,53 @@ func messageExtractor(b []byte) (interface{}, error) { } func validateCondition(c ngmodels.Condition, user *models.SignedInUser, skipCache bool, datasourceCache datasources.CacheService) error { - var refID string - if len(c.Data) == 0 { return nil } - for _, query := range c.Data { - if c.Condition == query.RefID { - refID = c.Condition - } - - datasourceUID, err := query.GetDatasource() - if err != nil { - return err - } - - isExpression, err := query.IsExpression() - if err != nil { - return err - } - if isExpression { - continue - } - - _, err = datasourceCache.GetDatasourceByUID(datasourceUID, user, skipCache) - if err != nil { - return fmt.Errorf("failed to get datasource: %s: %w", datasourceUID, err) - } + refIDs, err := validateQueriesAndExpressions(c.Data, user, skipCache, datasourceCache) + if err != nil { + return err } - if refID == "" { - return fmt.Errorf("condition %s not found in any query or expression", c.Condition) + t := make([]string, 0, len(refIDs)) + for refID := range refIDs { + t = append(t, refID) + } + if _, ok := refIDs[c.Condition]; !ok { + return fmt.Errorf("condition %s not found in any query or expression: it should be one of: [%s]", c.Condition, strings.Join(t, ",")) } return nil } -func validateQueriesAndExpressions(data []ngmodels.AlertQuery, user *models.SignedInUser, skipCache bool, datasourceCache datasources.CacheService) error { +func validateQueriesAndExpressions(data []ngmodels.AlertQuery, user *models.SignedInUser, skipCache bool, datasourceCache datasources.CacheService) (map[string]struct{}, error) { + refIDs := make(map[string]struct{}) if len(data) == 0 { - return nil + return nil, nil } for _, query := range data { datasourceUID, err := query.GetDatasource() if err != nil { - return err + return nil, err } isExpression, err := query.IsExpression() if err != nil { - return err + return nil, err } if isExpression { + refIDs[query.RefID] = struct{}{} continue } _, err = datasourceCache.GetDatasourceByUID(datasourceUID, user, skipCache) if err != nil { - return fmt.Errorf("failed to get datasource: %s: %w", datasourceUID, err) + return nil, fmt.Errorf("invalid query %s: %w: %s", query.RefID, err, datasourceUID) } + refIDs[query.RefID] = struct{}{} } - return nil + return refIDs, nil } func conditionEval(c *models.ReqContext, cmd ngmodels.EvalAlertConditionCommand, datasourceCache datasources.CacheService, dataService *tsdb.Service, cfg *setting.Cfg) response.Response { diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 0f2e4492fdd..ccc2018330b 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -183,8 +183,8 @@ func TestAlertRuleCRUD(t *testing.T) { Annotations: map[string]string{"annotation1": "val1"}, }, GrafanaManagedAlert: &apimodels.PostableGrafanaRule{ - Title: "AlwaysFiring", - Condition: "A", + Title: "AlwaysFiring", + Data: []ngmodels.AlertQuery{}, }, }, expectedResponse: `{"error":"invalid alert rule: no queries or expressions are found", "message":"failed to update rule group"}`, @@ -310,6 +310,66 @@ func TestAlertRuleCRUD(t *testing.T) { }, expectedResponse: `{"error":"invalid alert rule: interval (1s) should be divided exactly by scheduler interval: 10s", "message":"failed to update rule group"}`, }, + { + desc: "alert rule with unknown datasource", + rulegroup: "arulegroup", + rule: apimodels.PostableExtendedRuleNode{ + ApiRuleNode: &apimodels.ApiRuleNode{ + For: interval, + Labels: map[string]string{"label1": "val1"}, + Annotations: map[string]string{"annotation1": "val1"}, + }, + GrafanaManagedAlert: &apimodels.PostableGrafanaRule{ + Title: "AlwaysFiring", + Condition: "A", + Data: []ngmodels.AlertQuery{ + { + RefID: "A", + RelativeTimeRange: ngmodels.RelativeTimeRange{ + From: ngmodels.Duration(time.Duration(5) * time.Hour), + To: ngmodels.Duration(time.Duration(3) * time.Hour), + }, + DatasourceUID: "unknown", + Model: json.RawMessage(`{ + "type": "math", + "expression": "2 + 3 > 1" + }`), + }, + }, + }, + }, + expectedResponse: `{"error":"invalid query A: data source not found: unknown", "message":"failed to validate alert rule AlwaysFiring"}`, + }, + { + desc: "alert rule with invalid condition", + rulegroup: "arulegroup", + rule: apimodels.PostableExtendedRuleNode{ + ApiRuleNode: &apimodels.ApiRuleNode{ + For: interval, + Labels: map[string]string{"label1": "val1"}, + Annotations: map[string]string{"annotation1": "val1"}, + }, + GrafanaManagedAlert: &apimodels.PostableGrafanaRule{ + Title: "AlwaysFiring", + Condition: "B", + Data: []ngmodels.AlertQuery{ + { + RefID: "A", + RelativeTimeRange: ngmodels.RelativeTimeRange{ + From: ngmodels.Duration(time.Duration(5) * time.Hour), + To: ngmodels.Duration(time.Duration(3) * time.Hour), + }, + DatasourceUID: "-100", + Model: json.RawMessage(`{ + "type": "math", + "expression": "2 + 3 > 1" + }`), + }, + }, + }, + }, + expectedResponse: `{"error":"condition B not found in any query or expression: it should be one of: [A]", "message":"failed to validate alert rule AlwaysFiring"}`, + }, } for _, tc := range testCases { @@ -931,7 +991,7 @@ func TestAlertRuleCRUD(t *testing.T) { } `, expectedStatusCode: http.StatusBadRequest, - expectedResponse: `{"error":"condition B not found in any query or expression","message":"invalid condition"}`, + expectedResponse: `{"error":"condition B not found in any query or expression: it should be one of: [A]","message":"invalid condition"}`, }, { desc: "unknown query datasource", @@ -956,7 +1016,7 @@ func TestAlertRuleCRUD(t *testing.T) { } `, expectedStatusCode: http.StatusBadRequest, - expectedResponse: `{"error":"failed to get datasource: unknown: data source not found","message":"invalid condition"}`, + expectedResponse: `{"error":"invalid query A: data source not found: unknown","message":"invalid condition"}`, }, } @@ -1112,7 +1172,7 @@ func TestAlertRuleCRUD(t *testing.T) { } `, expectedStatusCode: http.StatusBadRequest, - expectedResponse: `{"error":"failed to get datasource: unknown: data source not found","message":"invalid queries or expressions"}`, + expectedResponse: `{"error":"invalid query A: data source not found: unknown","message":"invalid queries or expressions"}`, }, }