From b7bbc5058f7d9ed25ac0a2093375d9a82bccc240 Mon Sep 17 00:00:00 2001
From: William Wernert <william.wernert@grafana.com>
Date: Thu, 15 Feb 2024 12:03:28 -0500
Subject: [PATCH] Alerting: Don't validate rules on group update if they've
 only been reordered (#81841)

---------

Co-authored-by: Yuri Tseretyan <yuriy.tseretyan@grafana.com>
---
 pkg/services/ngalert/api/api_ruler.go      | 19 ++++++++++++++++
 pkg/services/ngalert/api/api_ruler_test.go | 26 +++++++++++++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go
index c1688241153..d551a9e9f2b 100644
--- a/pkg/services/ngalert/api/api_ruler.go
+++ b/pkg/services/ngalert/api/api_ruler.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"slices"
 	"strings"
 	"time"
 
@@ -62,6 +63,9 @@ var (
 	errProvisionedResource = errors.New("request affects resources created via provisioning API")
 )
 
+// ignore fields that are not part of the rule definition
+var ignoreFieldsForValidate = [...]string{"RuleGroupIndex"}
+
 // 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.StatusForbidden if user does not have access to any of the rules that match the filter.
@@ -558,6 +562,9 @@ func validateQueries(ctx context.Context, groupChanges *store.GroupDelta, valida
 	}
 	if len(groupChanges.Update) > 0 {
 		for _, upd := range groupChanges.Update {
+			if !shouldValidate(upd) {
+				continue
+			}
 			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())
@@ -567,6 +574,18 @@ func validateQueries(ctx context.Context, groupChanges *store.GroupDelta, valida
 	return nil
 }
 
+// shouldValidate returns true if the rule is not paused and there are changes in the rule that are not ignored
+func shouldValidate(delta store.RuleDelta) bool {
+	for _, diff := range delta.Diff {
+		if !slices.Contains(ignoreFieldsForValidate[:], diff.Path) {
+			return true
+		}
+	}
+
+	// TODO: consider also checking if rule will be paused after the update
+	return false
+}
+
 // getAuthorizedRuleByUid fetches all rules in group to which the specified rule belongs, and checks whether the user is authorized to access the group.
 // A user is authorized to access a group of rules only when it has permission to query all data sources used by all rules in this group.
 // Returns rule identified by provided UID or ErrAuthorization if user is not authorized to access the rule.
diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go
index 326fa6102b3..7069a20e4ac 100644
--- a/pkg/services/ngalert/api/api_ruler_test.go
+++ b/pkg/services/ngalert/api/api_ruler_test.go
@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"slices"
 	"testing"
 	"time"
 
@@ -31,6 +32,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
+	"github.com/grafana/grafana/pkg/util/cmputil"
 	"github.com/grafana/grafana/pkg/web"
 )
 
@@ -538,7 +540,24 @@ func TestValidateQueries(t *testing.T) {
 				New: models.AlertRuleGen(func(rule *models.AlertRule) {
 					rule.Condition = "Update_New"
 				})(),
-				Diff: nil,
+				Diff: cmputil.DiffReport{
+					cmputil.Diff{
+						Path: "SomeField",
+					},
+				},
+			},
+			{
+				Existing: models.AlertRuleGen(func(rule *models.AlertRule) {
+					rule.Condition = "Update_Index_Existing"
+				})(),
+				New: models.AlertRuleGen(func(rule *models.AlertRule) {
+					rule.Condition = "Update_Index_New"
+				})(),
+				Diff: cmputil.DiffReport{
+					cmputil.Diff{
+						Path: "RuleGroupIndex",
+					},
+				},
 			},
 		},
 		Delete: []*models.AlertRule{
@@ -548,12 +567,13 @@ func TestValidateQueries(t *testing.T) {
 		},
 	}
 
-	t.Run("should validate New and Updated only", func(t *testing.T) {
+	t.Run("should not validate deleted rules or updated rules with ignored fields", func(t *testing.T) {
 		validator := &recordingConditionValidator{}
 		err := validateQueries(context.Background(), &delta, validator, nil)
 		require.NoError(t, err)
+		noValidate := []string{"Deleted", "Update_Index_New"}
 		for _, condition := range validator.recorded {
-			if condition.Condition == "New" || condition.Condition == "Update_New" {
+			if !slices.Contains(noValidate, condition.Condition) {
 				continue
 			}
 			assert.Failf(t, "validated unexpected condition", "condition '%s' was validated but should not", condition.Condition)