[v11.0.x] Alerting: Fix simplified routes '...' groupBy creating invalid routes (#86376)

Alerting: Fix simplified routes '...' groupBy creating invalid routes (#86006)

* Alerting: Fix simplified routes '...' groupBy creating invalid routes

There were a few ways to go about this fix:
1. Modifying our copy of upstream validation to allow this
2. Modify our notification settings validation to prevent this
3. Normalize group by on save
4. Normalized group by on generate

Option 4. was chosen as the others have a mix of the following cons:
- Generated routes risk being incompatible with upstream/remote AM
- Awkward FE UX when using '...'
- Rule definition changing after save and potential pitfalls with TF

With option 4. generated routes stay compatible with external/remote AMs, FE
doesn't need to change as we allow mixed '...' and custom label groupBys, and
settings we save to db are the same ones requested.

In addition, it has the slight benefit of allowing us to hide the internal
implementation details of `alertname, grafana_folder` from the user in the
future, since we don't need to send them with every FE or TF request.

* Safer use of DefaultNotificationSettingsGroupBy

* Fix missed API tests

(cherry picked from commit 533bed6d94)

Co-authored-by: Matthew Jacobson <matthew.jacobson@grafana.com>

pkg/services/ngalert/api/api_ruler_validation_test.go

@@ -827,14 +827,12 @@ func TestValidateRuleNodeNotificationSettings(t *testing.T) {
 			notificationSettings: models.CopyNotificationSettings(validNotificationSettings(), models.NSMuts.WithGroupBy(model.AlertNameLabel, models.FolderTitleLabel)),
 		},
 		{
-			name:                 "group by missing alert name label is invalid",
+			name:                 "group by missing alert name label is valid",
 			notificationSettings: models.CopyNotificationSettings(validNotificationSettings(), models.NSMuts.WithGroupBy(models.FolderTitleLabel)),
-			expErrorContains:     model.AlertNameLabel,
 		},
 		{
-			name:                 "group by missing folder name label is invalid",
+			name:                 "group by missing folder name label is valid",
 			notificationSettings: models.CopyNotificationSettings(validNotificationSettings(), models.NSMuts.WithGroupBy(model.AlertNameLabel)),
-			expErrorContains:     models.FolderTitleLabel,
 		},
 		{
 			name:                 "group wait empty is valid",

pkg/services/ngalert/models/notifications.go

@@ -3,7 +3,6 @@ package models
 import (
 	"encoding/binary"
 	"errors"
-	"fmt"
 	"hash/fnv"
 	"slices"
 	"unsafe"
@@ -12,8 +11,11 @@ import (
 	"github.com/prometheus/common/model"
 )
 
-// groupByAll is a special value defined by alertmanager that can be used in a Route's GroupBy field to aggregate by all possible labels.
-const groupByAll = "..."
+// GroupByAll is a special value defined by alertmanager that can be used in a Route's GroupBy field to aggregate by all possible labels.
+const GroupByAll = "..."
+
+// DefaultNotificationSettingsGroupBy are the default required GroupBy fields for notification settings.
+var DefaultNotificationSettingsGroupBy = []string{FolderTitleLabel, model.AlertNameLabel}
 
 type ListNotificationSettingsQuery struct {
 	OrgID        int64
@@ -32,33 +34,50 @@ type NotificationSettings struct {
 	MuteTimeIntervals []string        `json:"mute_time_intervals,omitempty"`
 }
 
+// NormalizedGroupBy returns a consistent and ordered GroupBy.
+//   - If the GroupBy is empty, it returns nil so that the parent group can be inherited.
+//   - If the GroupBy contains the special label '...', it returns only '...'.
+//   - Otherwise, it returns the default GroupBy labels followed by any custom labels in sorted order.
+//
+// To ensure consistent and valid generated routes, this should be used instead of GroupBy when generating fingerprints
+// or fingerprint-level routes.
+func (s *NotificationSettings) NormalizedGroupBy() []string {
+	if len(s.GroupBy) == 0 {
+		// Inherit group from parent.
+		return nil
+	}
+
+	defaultGroupBySet := make(map[string]struct{}, len(DefaultNotificationSettingsGroupBy))
+	for _, lbl := range DefaultNotificationSettingsGroupBy {
+		defaultGroupBySet[lbl] = struct{}{}
+	}
+
+	var customLabels []string
+	for _, lbl := range s.GroupBy {
+		if lbl == GroupByAll {
+			return []string{GroupByAll}
+		}
+		if _, ok := defaultGroupBySet[lbl]; !ok {
+			customLabels = append(customLabels, lbl)
+		}
+	}
+
+	// Sort the custom labels to ensure consistent ordering while keeping the required labels in the front.
+	slices.Sort(customLabels)
+
+	normalized := make([]string, 0, len(DefaultNotificationSettingsGroupBy)+len(customLabels))
+	normalized = append(normalized, DefaultNotificationSettingsGroupBy...)
+	return append(normalized, customLabels...)
+}
+
 // Validate checks if the NotificationSettings object is valid.
 // It returns an error if any of the validation checks fail.
 // The receiver must be specified.
-// If GroupBy is not empty, it must contain both model.AlertNameLabel and FolderTitleLabel or the special label '...'.
 // GroupWait, GroupInterval, RepeatInterval must be positive durations.
 func (s *NotificationSettings) Validate() error {
 	if s.Receiver == "" {
 		return errors.New("receiver must be specified")
 	}
-	if len(s.GroupBy) > 0 {
-		alertName, folderTitle := false, false
-		for _, lbl := range s.GroupBy {
-			if lbl == groupByAll {
-				alertName, folderTitle = true, true
-				break
-			}
-			if lbl == model.AlertNameLabel {
-				alertName = true
-			}
-			if lbl == FolderTitleLabel {
-				folderTitle = true
-			}
-		}
-		if !alertName || !folderTitle {
-			return fmt.Errorf("group by override must contain two required labels: '%s' and '%s' or '...' (group by all)", model.AlertNameLabel, FolderTitleLabel)
-		}
-	}
 	if s.GroupWait != nil && *s.GroupWait < 0 {
 		return errors.New("group wait must be a positive duration")
 	}
@@ -153,8 +172,7 @@ func (s *NotificationSettings) Fingerprint() data.Fingerprint {
 	}
 
 	writeString(s.Receiver)
-	// TODO: Should we sort the group by labels?
-	for _, gb := range s.GroupBy {
+	for _, gb := range s.NormalizedGroupBy() {
 		writeString(gb)
 	}
 	writeDuration(s.GroupWait)

pkg/services/ngalert/models/notifications_test.go

@@ -41,14 +41,12 @@ func TestValidate(t *testing.T) {
 			notificationSettings: CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(model.AlertNameLabel, FolderTitleLabel)),
 		},
 		{
-			name:                 "group by missing alert name label is invalid",
+			name:                 "group by missing alert name label is valid",
 			notificationSettings: CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(FolderTitleLabel)),
-			expErrorContains:     model.AlertNameLabel,
 		},
 		{
-			name:                 "group by missing folder name label is invalid",
+			name:                 "group by missing folder name label is valid",
 			notificationSettings: CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(model.AlertNameLabel)),
-			expErrorContains:     FolderTitleLabel,
 		},
 		{
 			name:                 "group wait empty is valid",
@@ -118,6 +116,18 @@ func TestNotificationSettingsLabels(t *testing.T) {
 				AutogeneratedRouteReceiverNameLabel: "receiver name",
 			},
 		},
+		{
+			name: "default notification settings with hardcoded default group by",
+			notificationSettings: NotificationSettings{
+				Receiver: "receiver name",
+				GroupBy:  DefaultNotificationSettingsGroupBy,
+			},
+			labels: data.Labels{
+				AutogeneratedRouteLabel:             "true",
+				AutogeneratedRouteReceiverNameLabel: "receiver name",
+				AutogeneratedRouteSettingsHashLabel: "6027cdeaff62ba3f",
+			},
+		},
 		{
 			name: "custom notification settings",
 			notificationSettings: NotificationSettings{
@@ -131,7 +141,7 @@ func TestNotificationSettingsLabels(t *testing.T) {
 			labels: data.Labels{
 				AutogeneratedRouteLabel:             "true",
 				AutogeneratedRouteReceiverNameLabel: "receiver name",
-				AutogeneratedRouteSettingsHashLabel: "f0e23250cefc4a31",
+				AutogeneratedRouteSettingsHashLabel: "47164c92f2986a35",
 			},
 		},
 	}
@@ -143,3 +153,60 @@ func TestNotificationSettingsLabels(t *testing.T) {
 		})
 	}
 }
+
+func TestNormalizedGroupBy(t *testing.T) {
+	validNotificationSettings := NotificationSettingsGen()
+
+	testCases := []struct {
+		name                      string
+		notificationSettings      NotificationSettings
+		expectedNormalizedGroupBy []string
+	}{
+		{
+			name:                      "default group by is normal",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(DefaultNotificationSettingsGroupBy...)),
+			expectedNormalizedGroupBy: DefaultNotificationSettingsGroupBy,
+		},
+		{
+			name:                      "group by ... is normal",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(GroupByAll)),
+			expectedNormalizedGroupBy: []string{GroupByAll},
+		},
+		{
+			name:                 "group by empty is normal",
+			notificationSettings: CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy()),
+		},
+		{
+			name:                      "group by with ... and other labels is not normal",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(FolderTitleLabel, model.AlertNameLabel, GroupByAll)),
+			expectedNormalizedGroupBy: []string{GroupByAll},
+		},
+		{
+			name:                      "group by missing alert name label is not normal",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(FolderTitleLabel)),
+			expectedNormalizedGroupBy: DefaultNotificationSettingsGroupBy,
+		},
+		{
+			name:                      "group by missing folder name label is not normal",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(model.AlertNameLabel)),
+			expectedNormalizedGroupBy: DefaultNotificationSettingsGroupBy,
+		},
+		{
+			name:                      "group by with all required labels plus extra is normal",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy(FolderTitleLabel, model.AlertNameLabel, "custom")),
+			expectedNormalizedGroupBy: []string{FolderTitleLabel, model.AlertNameLabel, "custom"},
+		},
+		{
+			name:                      "ensure consistent ordering, required labels in front followed by sorted custom labels ",
+			notificationSettings:      CopyNotificationSettings(validNotificationSettings(), NSMuts.WithGroupBy("custom", model.AlertNameLabel, "something", FolderTitleLabel, "other")),
+			expectedNormalizedGroupBy: []string{FolderTitleLabel, model.AlertNameLabel, "custom", "other", "something"},
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			normalized := tt.notificationSettings.NormalizedGroupBy()
+			require.Equal(t, normalized, tt.expectedNormalizedGroupBy)
+		})
+	}
+}

pkg/services/ngalert/notifier/autogen_alertmanager.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/grafana/grafana-plugin-sdk-go/data"
 	"github.com/prometheus/alertmanager/pkg/labels"
-	"github.com/prometheus/common/model"
 	"golang.org/x/exp/maps"
 
 	"github.com/grafana/grafana/pkg/infra/log"
@@ -121,7 +120,7 @@ func generateRouteFromSettings(defaultReceiver string, settings map[data.Fingerp
 				ObjectMatchers: definitions.ObjectMatchers{contactMatcher},
 				Continue:       false,
 				// Since we'll have many rules from different folders using this policy, we ensure it has these necessary groupings.
-				GroupByStr: []string{models.FolderTitleLabel, model.AlertNameLabel},
+				GroupByStr: append([]string{}, models.DefaultNotificationSettingsGroupBy...),
 			}
 			receiverRoutes[s.Receiver] = receiverRoute
 			autoGenRoot.Routes = append(autoGenRoot.Routes, receiverRoute)
@@ -140,7 +139,7 @@ func generateRouteFromSettings(defaultReceiver string, settings map[data.Fingerp
 			ObjectMatchers: definitions.ObjectMatchers{settingMatcher},
 			Continue:       false, // Only a single setting-specific route should match.
 
-			GroupByStr:        s.GroupBy, // Note: in order to pass validation at least FolderTitleLabel and AlertNameLabel are always included.
+			GroupByStr:        s.NormalizedGroupBy(),
 			MuteTimeIntervals: s.MuteTimeIntervals,
 			GroupWait:         s.GroupWait,
 			GroupInterval:     s.GroupInterval,

pkg/services/ngalert/notifier/autogen_alertmanager_test.go

@@ -134,17 +134,17 @@ func TestAddAutogenConfig(t *testing.T) {
 						MuteTimeIntervals: []string{"maintenance"},
 					}),
 					withChildRoutes(basicContactRoute("receiver1"), &definitions.Route{
-						Receiver:       "receiver1",
-						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "dde34b8127e68f31"),
-						GroupInterval:  util.Pointer(model.Duration(1 * time.Minute)),
-					}, &definitions.Route{
 						Receiver:          "receiver1",
-						ObjectMatchers:    matcher(models.AutogeneratedRouteSettingsHashLabel, "ed4038c5d6733607"),
-						GroupByStr:        []string{model.AlertNameLabel, models.FolderTitleLabel, "custom"},
+						ObjectMatchers:    matcher(models.AutogeneratedRouteSettingsHashLabel, "4f095749ddf3eeeb"),
+						GroupByStr:        []string{models.FolderTitleLabel, model.AlertNameLabel, "custom"},
 						GroupInterval:     util.Pointer(model.Duration(1 * time.Minute)),
 						GroupWait:         util.Pointer(model.Duration(2 * time.Minute)),
 						RepeatInterval:    util.Pointer(model.Duration(3 * time.Minute)),
 						MuteTimeIntervals: []string{"maintenance"},
+					}, &definitions.Route{
+						Receiver:       "receiver1",
+						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "dde34b8127e68f31"),
+						GroupInterval:  util.Pointer(model.Duration(1 * time.Minute)),
 					}),
 					withChildRoutes(basicContactRoute("receiver2"), &definitions.Route{
 						Receiver:       "receiver2",
@@ -153,8 +153,8 @@ func TestAddAutogenConfig(t *testing.T) {
 					}),
 					withChildRoutes(basicContactRoute("receiver4"), &definitions.Route{
 						Receiver:       "receiver4",
-						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "5e5ab8d592b12e86"),
-						GroupByStr:     []string{model.AlertNameLabel, models.FolderTitleLabel, "custom"},
+						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "b3a2fa5e615dcc7e"),
+						GroupByStr:     []string{models.FolderTitleLabel, model.AlertNameLabel, "custom"},
 					}),
 					withChildRoutes(basicContactRoute("receiver3"), &definitions.Route{
 						Receiver:       "receiver3",
@@ -164,6 +164,71 @@ func TestAddAutogenConfig(t *testing.T) {
 				},
 			}),
 		},
+		{
+			name:           "settings with custom options and nil groupBy, groupBy should inherit from parent",
+			existingConfig: configGen([]string{"receiver1"}, nil),
+			storeSettings: []models.NotificationSettings{
+				models.CopyNotificationSettings(models.NewDefaultNotificationSettings("receiver1"), models.NSMuts.WithGroupInterval(util.Pointer(1*time.Minute)), models.NSMuts.WithGroupBy()),
+			},
+			expRoute: withChildRoutes(rootRoute(), &definitions.Route{
+				Receiver:       "default",
+				ObjectMatchers: matcher(models.AutogeneratedRouteLabel, "true"),
+				Routes: []*definitions.Route{
+					withChildRoutes(basicContactRoute("receiver1"), &definitions.Route{
+						Receiver:       "receiver1",
+						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "dde34b8127e68f31"),
+						GroupByStr:     nil,
+						GroupInterval:  util.Pointer(model.Duration(1 * time.Minute)),
+					}),
+				},
+			}),
+		},
+		{
+			name:           "settings with nil groupBy should have different fingerprint than default groupBy",
+			existingConfig: configGen([]string{"receiver1"}, nil),
+			storeSettings: []models.NotificationSettings{
+				models.CopyNotificationSettings(models.NewDefaultNotificationSettings("receiver1"), models.NSMuts.WithGroupInterval(util.Pointer(1*time.Minute)), models.NSMuts.WithGroupBy()),
+				models.CopyNotificationSettings(models.NewDefaultNotificationSettings("receiver1"), models.NSMuts.WithGroupInterval(util.Pointer(1*time.Minute)), models.NSMuts.WithGroupBy(models.DefaultNotificationSettingsGroupBy...)),
+			},
+			expRoute: withChildRoutes(rootRoute(), &definitions.Route{
+				Receiver:       "default",
+				ObjectMatchers: matcher(models.AutogeneratedRouteLabel, "true"),
+				Routes: []*definitions.Route{
+					withChildRoutes(basicContactRoute("receiver1"), &definitions.Route{
+						Receiver:       "receiver1",
+						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "dde34b8127e68f31"),
+						GroupByStr:     nil,
+						GroupInterval:  util.Pointer(model.Duration(1 * time.Minute)),
+					}, &definitions.Route{
+						Receiver:       "receiver1",
+						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "e1f3a275a8918385"), // Different hash.
+						GroupByStr:     []string{models.FolderTitleLabel, model.AlertNameLabel},
+						GroupInterval:  util.Pointer(model.Duration(1 * time.Minute)),
+					}),
+				},
+			}),
+		},
+		{
+			name:           "settings with incomplete required groupBy labels will be completed and should have the same fingerprint",
+			existingConfig: configGen([]string{"receiver1"}, nil),
+			storeSettings: []models.NotificationSettings{
+				models.CopyNotificationSettings(models.NewDefaultNotificationSettings("receiver1"), models.NSMuts.WithGroupInterval(util.Pointer(1*time.Minute)), models.NSMuts.WithGroupBy(models.FolderTitleLabel)),
+				models.CopyNotificationSettings(models.NewDefaultNotificationSettings("receiver1"), models.NSMuts.WithGroupInterval(util.Pointer(1*time.Minute)), models.NSMuts.WithGroupBy(model.AlertNameLabel)),
+				models.CopyNotificationSettings(models.NewDefaultNotificationSettings("receiver1"), models.NSMuts.WithGroupInterval(util.Pointer(1*time.Minute)), models.NSMuts.WithGroupBy(models.DefaultNotificationSettingsGroupBy...)),
+			},
+			expRoute: withChildRoutes(rootRoute(), &definitions.Route{
+				Receiver:       "default",
+				ObjectMatchers: matcher(models.AutogeneratedRouteLabel, "true"),
+				Routes: []*definitions.Route{
+					withChildRoutes(basicContactRoute("receiver1"), &definitions.Route{
+						Receiver:       "receiver1",
+						ObjectMatchers: matcher(models.AutogeneratedRouteSettingsHashLabel, "e1f3a275a8918385"),
+						GroupByStr:     []string{models.FolderTitleLabel, model.AlertNameLabel},
+						GroupInterval:  util.Pointer(model.Duration(1 * time.Minute)),
+					}),
+				},
+			}),
+		},
 		{
 			name:           "when skipInvalid=true, invalid settings are skipped",
 			existingConfig: configGen([]string{"receiver1", "receiver2", "receiver3"}, nil),

pkg/tests/api/alerting/api_ruler_test.go

@@ -1885,7 +1885,7 @@ func TestIntegrationRuleNotificationSettings(t *testing.T) {
 		t.Log(body)
 	})
 
-	t.Run("create should fail if group_by does not contain special labels", func(t *testing.T) {
+	t.Run("create should not fail if group_by is missing required labels but they should still be used", func(t *testing.T) {
 		var copyD testData
 		err = json.Unmarshal(testDataRaw, &copyD)
 		group := copyD.RuleGroup
@@ -1893,10 +1893,48 @@ func TestIntegrationRuleNotificationSettings(t *testing.T) {
 		ns.GroupBy = []string{"label1"}
 
 		_, status, body := apiClient.PostRulesGroupWithStatus(t, folder, &group)
-		require.Equalf(t, http.StatusBadRequest, status, body)
+		require.Equalf(t, http.StatusAccepted, status, body)
+
+		cfg, status, body := apiClient.GetAlertmanagerConfigWithStatus(t)
+		if !assert.Equalf(t, http.StatusOK, status, body) {
+			return
+		}
+
+		// Ensure that the group by contains the default required labels.
+		autogenRoute := cfg.AlertmanagerConfig.Route.Routes[0]
+		receiverRoute := autogenRoute.Routes[0]
+		ruleRoute := receiverRoute.Routes[0]
+		assert.Equal(t, []model.LabelName{ngmodels.FolderTitleLabel, model.AlertNameLabel, "label1"}, ruleRoute.GroupBy)
+
 		t.Log(body)
 	})
 
+	t.Run("create with '...' groupBy followed by config post should succeed", func(t *testing.T) {
+		var copyD testData
+		err = json.Unmarshal(testDataRaw, &copyD)
+		group := copyD.RuleGroup
+		ns := group.Rules[0].GrafanaManagedAlert.NotificationSettings
+		ns.GroupBy = []string{ngmodels.FolderTitleLabel, model.AlertNameLabel, ngmodels.GroupByAll}
+
+		_, status, body := apiClient.PostRulesGroupWithStatus(t, folder, &group)
+		require.Equalf(t, http.StatusAccepted, status, body)
+
+		// Now update the config with no changes.
+		_, status, body = apiClient.GetAlertmanagerConfigWithStatus(t)
+		if !assert.Equalf(t, http.StatusOK, status, body) {
+			return
+		}
+
+		cfg := apimodels.PostableUserConfig{}
+
+		err = json.Unmarshal([]byte(body), &cfg)
+		require.NoError(t, err)
+
+		ok, err := apiClient.PostConfiguration(t, cfg)
+		require.NoError(t, err)
+		require.True(t, ok)
+	})
+
 	t.Run("should create rule and generate route", func(t *testing.T) {
 		_, status, body := apiClient.PostRulesGroupWithStatus(t, folder, &d.RuleGroup)
 		require.Equalf(t, http.StatusAccepted, status, body)

pkg/tests/api/alerting/test-data/rule-notification-settings-1-post.json

@@ -29,8 +29,8 @@
           "notification_settings": {
             "receiver": "rule-receiver",
             "group_by": [
-              "alertname",
               "grafana_folder",
+              "alertname",
               "label1"
             ],
             "group_wait": "100ms",
@@ -55,4 +55,4 @@
     "name": "rule-time-interval",
     "time_intervals":[{"times":[{"start_time":"10:00","end_time":"12:00"}]}]
   }
-}
+}