IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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
This commit is contained in:
Matthew Jacobson 2024-04-16 12:14:39 -04:00 committed by GitHub
parent 79eab65f94
commit 533bed6d94
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 233 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
pkg/services/ngalert/api/api_ruler_validation_test.go
View File

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

66
pkg/services/ngalert/models/notifications.go
View File

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

77
pkg/services/ngalert/models/notifications_test.go
View File

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

5
pkg/services/ngalert/notifier/autogen_alertmanager.go
View File

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

81
pkg/services/ngalert/notifier/autogen_alertmanager_test.go
View File

@ -135,16 +135,16 @@ func TestAddAutogenConfig(t *testing.T) {
}),
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),

42
pkg/tests/api/alerting/api_ruler_test.go
View File

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

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

@ -29,8 +29,8 @@
"notification_settings": {
"receiver": "rule-receiver",
"group_by": [
"alertname",
"grafana_folder",
"alertname",
"label1"
],
"group_wait": "100ms",