Alerting: update rules POST API to validate query and condition only for rules that changed. (#68667)

* replace condition validation with just structural validation
* validate conditions of only new and updated rules
* add integration tests for rule update\delete API

Co-authored-by: George Robinson <george.robinson@grafana.com>

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 {
+	rules, err := validateRuleGroup(&ruleGroupConfig, c.SignedInUser.OrgID, namespace, srv.cfg)
-		return srv.conditionValidator.Validate(eval.NewContext(c.Req.Context(), c.SignedInUser), condition)
-	}, 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
+}

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

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)

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, cfg)
-		alerts, err := validateRuleGroup(&g, orgId, folder, func(condition models.Condition) error {
-			conditionValidations++
-			return nil
-		}, 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 {
+		alerts, err := validateRuleGroup(&g, orgId, folder, cfg)
-			return nil
-		}, 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 {
+		alerts, err := validateRuleGroup(&g, orgId, folder, cfg)
-			return nil
-		}, 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 {
+			_, err := validateRuleGroup(g, orgId, folder, cfg)
-				return nil
-			}, 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 {
+			alert, err := validateRuleNode(r, name, interval, orgId, folder, cfg)
-				return nil
-			}, 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 {
+		alert, err := validateRuleNode(&r, "", interval, orgId, folder, cfg)
-			return nil
-		}, 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
+		name           string
-		interval            *time.Duration
+		interval       *time.Duration
-		rule                func() *apimodels.PostableExtendedRuleNode
+		rule           func() *apimodels.PostableExtendedRuleNode
-		conditionValidation func(condition models.Condition) error
+		assert         func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error)
-		assert              func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error)
+		allowedIfNoUId bool
-		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 {
+			alert, err := validateRuleNode(r, name, interval, orgId, folder, cfg)
-				return nil
-			}, 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 {
+		alert, err := validateRuleNode(&r, "", interval, orgId, folder, cfg)
-			return nil
-		}, 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
+		name     string
-		interval            *time.Duration
+		interval *time.Duration
-		rule                func() *apimodels.PostableExtendedRuleNode
+		rule     func() *apimodels.PostableExtendedRuleNode
-		conditionValidation func(condition models.Condition) error
+		assert   func(t *testing.T, model *apimodels.PostableExtendedRuleNode, err error)
-		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 {
+			_, err := validateRuleNode(&r, util.GenerateShortUID(), testCase.interval, rand.Int63(), randFolder(), cfg)
-				return nil
-			}
-
-			_, err := validateRuleNode(&r, util.GenerateShortUID(), testCase.interval, rand.Int63(), randFolder(), f, cfg)
 			require.Error(t, err)
 		})
 	}

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 {

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",

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

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)
-
+				expectedCode := tc.expectedCode
-				assert.Equal(t, http.StatusBadRequest, status)
+				if expectedCode == 0 {
+					expectedCode = http.StatusBadRequest
+				}
+				assert.Equal(t, expectedCode, status)
 			})
 		}
 	}

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 {

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