Alerting: validate condition before updating rulegroup (#33367)

* Alerting: validate condition before updating rulegroup

* Apply suggestions from code review

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{

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
+	store           store.RuleStore
-	log   log.Logger
+	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,

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)
 	}
 

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 {
+	refIDs, err := validateQueriesAndExpressions(c.Data, user, skipCache, datasourceCache)
-		if c.Condition == query.RefID {
+	if err != nil {
-			refID = c.Condition
+		return err
-		}
-
-		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)
-		}
 	}
 
-	if refID == "" {
+	t := make([]string, 0, len(refIDs))
-		return fmt.Errorf("condition %s not found in any query or expression", c.Condition)
+	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 {

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",
+						Title: "AlwaysFiring",
-						Condition: "A",
+						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"}`,
 		},
 	}