From 9bbfeedadfa04f6f184d8844373098eec2fe7c0a Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Fri, 17 Jun 2022 10:19:22 -0500 Subject: [PATCH] Alerting: Create algorithm to process receiver changes and keep them consistent internally (#50738) * Algorithm to fix up receivers * Extract for tests * Add tests, fix bug * Add test which demonstrates how it fixes up broken groups * Fix package prefix --- .../ngalert/provisioning/contactpoints.go | 89 +++- .../provisioning/contactpoints_test.go | 405 ++++++++++++++++++ 2 files changed, 480 insertions(+), 14 deletions(-) diff --git a/pkg/services/ngalert/provisioning/contactpoints.go b/pkg/services/ngalert/provisioning/contactpoints.go index 1023948171d..17774b9ea64 100644 --- a/pkg/services/ngalert/provisioning/contactpoints.go +++ b/pkg/services/ngalert/provisioning/contactpoints.go @@ -252,21 +252,12 @@ func (ecp *ContactPointService) UpdateContactPoint(ctx context.Context, orgID in if err != nil { return err } - for _, receiver := range revision.cfg.AlertmanagerConfig.Receivers { - if receiver.Name == contactPoint.Name { - receiverNotFound := true - for i, grafanaReceiver := range receiver.GrafanaManagedReceivers { - if grafanaReceiver.UID == mergedReceiver.UID { - receiverNotFound = false - receiver.GrafanaManagedReceivers[i] = mergedReceiver - break - } - } - if receiverNotFound { - return fmt.Errorf("contact point with uid '%s' not found", mergedReceiver.UID) - } - } + + configModified := stitchReceiver(revision.cfg, mergedReceiver) + if !configModified { + return fmt.Errorf("contact point with uid '%s' not found", mergedReceiver.UID) } + data, err := json.Marshal(revision.cfg) if err != nil { return err @@ -378,3 +369,73 @@ func (ecp *ContactPointService) encryptValue(value string) (string, error) { } return base64.StdEncoding.EncodeToString(encryptedData), nil } + +// stitchReceiver modifies a receiver, target, in an alertmanager config. It modifies the given config in-place. +// Returns true if the config was altered in any way, and false otherwise. +func stitchReceiver(cfg *apimodels.PostableUserConfig, target *apimodels.PostableGrafanaReceiver) bool { + // Algorithm to fix up receivers. Receivers are very complex and depend heavily on internal consistency. + // All receivers in a given receiver group have the same name. We must maintain this across renames. + configModified := false +groupLoop: + for _, receiverGroup := range cfg.AlertmanagerConfig.Receivers { + // Does the current group contain the grafana receiver we're interested in? + for i, grafanaReceiver := range receiverGroup.GrafanaManagedReceivers { + if grafanaReceiver.UID == target.UID { + // If it's a basic field change, simply replace it. Done! + // + // NOTE: + // In a "normal" database, receiverGroup.Name should always == grafanaReceiver.Name. + // Check it regardless. + // If these values are out of sync due to some bug elsewhere in the code, let's fix it up. + // Our receiver group fixing logic below will handle it. + if grafanaReceiver.Name == target.Name && receiverGroup.Name == grafanaReceiver.Name { + receiverGroup.GrafanaManagedReceivers[i] = target + configModified = true + break groupLoop + } + + // If we're renaming, we'll need to fix up the macro receiver group for consistency. + // Firstly, if we're the only receiver in the group, simply rename the group to match. Done! + if len(receiverGroup.GrafanaManagedReceivers) == 1 { + receiverGroup.Name = target.Name + receiverGroup.GrafanaManagedReceivers[i] = target + configModified = true + break groupLoop + } + + // Otherwise, we only want to rename the receiver we are touching... NOT all of them. + // Check to see whether a different group with the name we want already exists. + for i, candidateExistingGroup := range cfg.AlertmanagerConfig.Receivers { + // If so, put our modified receiver into that group. Done! + if candidateExistingGroup.Name == target.Name { + // Drop it from the old group... + receiverGroup.GrafanaManagedReceivers = append(receiverGroup.GrafanaManagedReceivers[:i], receiverGroup.GrafanaManagedReceivers[i+1:]...) + // Add the modified receiver to the new group... + candidateExistingGroup.GrafanaManagedReceivers = append(candidateExistingGroup.GrafanaManagedReceivers, target) + configModified = true + break groupLoop + } + } + + // Doesn't exist? Create a new group just for the receiver. + newGroup := &apimodels.PostableApiReceiver{ + Receiver: config.Receiver{ + Name: target.Name, + }, + PostableGrafanaReceivers: apimodels.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*apimodels.PostableGrafanaReceiver{ + target, + }, + }, + } + cfg.AlertmanagerConfig.Receivers = append(cfg.AlertmanagerConfig.Receivers, newGroup) + // Drop it from the old spot. + receiverGroup.GrafanaManagedReceivers = append(receiverGroup.GrafanaManagedReceivers[:i], receiverGroup.GrafanaManagedReceivers[i+1:]...) + configModified = true + break groupLoop + } + } + } + + return configModified +} diff --git a/pkg/services/ngalert/provisioning/contactpoints_test.go b/pkg/services/ngalert/provisioning/contactpoints_test.go index f3b2527e0c0..730408b7df0 100644 --- a/pkg/services/ngalert/provisioning/contactpoints_test.go +++ b/pkg/services/ngalert/provisioning/contactpoints_test.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/services/secrets/database" "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/prometheus/alertmanager/config" "github.com/stretchr/testify/require" ) @@ -254,3 +255,407 @@ func createTestContactPoint() definitions.EmbeddedContactPoint { Settings: settings, } } + +func TestStitchReceivers(t *testing.T) { + type testCase struct { + name string + initial *definitions.PostableUserConfig + new *definitions.PostableGrafanaReceiver + expModified bool + expCfg definitions.PostableApiAlertingConfig + } + + cases := []testCase{ + { + name: "non matching receiver by UID, no change", + new: &definitions.PostableGrafanaReceiver{ + UID: "does not exist", + }, + expModified: false, + expCfg: createTestConfigWithReceivers().AlertmanagerConfig, + }, + { + name: "matching receiver with unchanged name, replaces", + new: &definitions.PostableGrafanaReceiver{ + UID: "ghi", + Name: "receiver-2", + Type: "teams", + }, + expModified: true, + expCfg: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "receiver-1", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "receiver-1", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "def", + Name: "receiver-2", + Type: "slack", + }, + { + UID: "ghi", + Name: "receiver-2", + Type: "teams", + }, + { + UID: "jkl", + Name: "receiver-2", + Type: "discord", + }, + }, + }, + }, + }, + }, + }, + { + name: "rename with only one receiver in group, renames group", + new: &definitions.PostableGrafanaReceiver{ + UID: "abc", + Name: "new-receiver", + Type: "slack", + }, + expModified: true, + expCfg: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "new-receiver", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "new-receiver", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "def", + Name: "receiver-2", + Type: "slack", + }, + { + UID: "ghi", + Name: "receiver-2", + Type: "email", + }, + { + UID: "jkl", + Name: "receiver-2", + Type: "discord", + }, + }, + }, + }, + }, + }, + }, + { + name: "rename to another existing group, moves receiver", + new: &definitions.PostableGrafanaReceiver{ + UID: "def", + Name: "receiver-1", + Type: "slack", + }, + expModified: true, + expCfg: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "receiver-1", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "receiver-1", + Type: "slack", + }, + { + UID: "def", + Name: "receiver-1", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "ghi", + Name: "receiver-2", + Type: "email", + }, + { + UID: "jkl", + Name: "receiver-2", + Type: "discord", + }, + }, + }, + }, + }, + }, + }, + { + name: "rename to a name that doesn't exist, creates new group and moves", + new: &definitions.PostableGrafanaReceiver{ + UID: "jkl", + Name: "brand-new-group", + Type: "opsgenie", + }, + expModified: true, + expCfg: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "receiver-1", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "receiver-1", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "def", + Name: "receiver-2", + Type: "slack", + }, + { + UID: "ghi", + Name: "receiver-2", + Type: "email", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "brand-new-group", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "jkl", + Name: "brand-new-group", + Type: "opsgenie", + }, + }, + }, + }, + }, + }, + }, + { + name: "rename an inconsistent group in the database, algorithm fixes it", + initial: createInconsistentTestConfigWithReceivers(), + new: &definitions.PostableGrafanaReceiver{ + UID: "ghi", + Name: "brand-new-group", + Type: "opsgenie", + }, + expModified: true, + expCfg: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "receiver-1", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "receiver-1", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "def", + Name: "receiver-2", + Type: "slack", + }, + { + UID: "jkl", + Name: "receiver-2", + Type: "discord", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "brand-new-group", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "ghi", + Name: "brand-new-group", + Type: "opsgenie", + }, + }, + }, + }, + }, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cfg := createTestConfigWithReceivers() + if c.initial != nil { + cfg = c.initial + } + + modified := stitchReceiver(cfg, c.new) + + require.Equal(t, c.expModified, modified) + require.Equal(t, c.expCfg, cfg.AlertmanagerConfig) + }) + } +} + +func createTestConfigWithReceivers() *definitions.PostableUserConfig { + return &definitions.PostableUserConfig{ + AlertmanagerConfig: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "receiver-1", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "receiver-1", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "def", + Name: "receiver-2", + Type: "slack", + }, + { + UID: "ghi", + Name: "receiver-2", + Type: "email", + }, + { + UID: "jkl", + Name: "receiver-2", + Type: "discord", + }, + }, + }, + }, + }, + }, + } +} + +// This is an invalid config, with inconsistently named receivers (intentionally). +func createInconsistentTestConfigWithReceivers() *definitions.PostableUserConfig { + return &definitions.PostableUserConfig{ + AlertmanagerConfig: definitions.PostableApiAlertingConfig{ + Receivers: []*definitions.PostableApiReceiver{ + { + Receiver: config.Receiver{ + Name: "receiver-1", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "abc", + Name: "receiver-1", + Type: "slack", + }, + }, + }, + }, + { + Receiver: config.Receiver{ + Name: "receiver-2", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: "def", + Name: "receiver-2", + Type: "slack", + }, + { + UID: "ghi", + Name: "receiver-3", + Type: "email", + }, + { + UID: "jkl", + Name: "receiver-2", + Type: "discord", + }, + }, + }, + }, + }, + }, + } +}