From 4df41972f11456537994320f2ae39124ecf55fea Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Mon, 19 Sep 2022 17:30:16 -0500 Subject: [PATCH] Alerting: Refactor one notifier to use encoding/json to parse settings instead of simplejson (#55271) * Create struct for Slack's receiver settings * Remove one layer of indirection when building slack notifier * Delete un-used struct * Validate against settings struct instead of simplejson object * Genericize settings marshalling * Remove repetition between fields on notifier and fields on settings struct * Rename unmarshal settings wrapper * Handle comma separated strings at marshalling time rather than validation time * Address misc review feedback --- .../ngalert/notifier/channels/slack.go | 187 +++++++----------- .../ngalert/notifier/channels/slack_test.go | 4 +- .../ngalert/notifier/channels/util.go | 68 +++++++ 3 files changed, 144 insertions(+), 115 deletions(-) diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index c5956f841d4..af300fbed2a 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -35,132 +35,93 @@ type SlackNotifier struct { tmpl *template.Template images ImageStore webhookSender notifications.WebhookSender - - URL *url.URL - Username string - IconEmoji string - IconURL string - Recipient string - Text string - Title string - MentionUsers []string - MentionGroups []string - MentionChannel string - Token string + settings slackSettings } -type SlackConfig struct { - *NotificationChannelConfig - URL *url.URL - Username string - IconEmoji string - IconURL string - Recipient string - Text string - Title string - MentionUsers []string - MentionGroups []string - MentionChannel string - Token string +type slackSettings struct { + EndpointURL string `json:"endpointUrl,omitempty" yaml:"endpointUrl,omitempty"` + URL string `json:"url,omitempty" yaml:"url,omitempty"` + Token string `json:"token,omitempty" yaml:"token,omitempty"` + Recipient string `json:"recipient,omitempty" yaml:"recipient,omitempty"` + Text string `json:"text,omitempty" yaml:"text,omitempty"` + Title string `json:"title,omitempty" yaml:"title,omitempty"` + Username string `json:"username,omitempty" yaml:"username,omitempty"` + IconEmoji string `json:"icon_emoji,omitempty" yaml:"icon_emoji,omitempty"` + IconURL string `json:"icon_url,omitempty" yaml:"icon_url,omitempty"` + MentionChannel string `json:"mentionChannel,omitempty" yaml:"mentionChannel,omitempty"` + MentionUsers CommaSeparatedStrings `json:"mentionUsers,omitempty" yaml:"mentionUsers,omitempty"` + MentionGroups CommaSeparatedStrings `json:"mentionGroups,omitempty" yaml:"mentionGroups,omitempty"` } +// SlackFactory creates a new NotificationChannel that sends notifications to Slack. func SlackFactory(fc FactoryConfig) (NotificationChannel, error) { - cfg, err := NewSlackConfig(fc) + ch, err := buildSlackNotifier(fc) if err != nil { return nil, receiverInitError{ Reason: err.Error(), Cfg: *fc.Config, } } - return NewSlackNotifier(cfg, fc.ImageStore, fc.NotificationService, fc.Template), nil + return ch, nil } -func NewSlackConfig(factoryConfig FactoryConfig) (*SlackConfig, error) { - channelConfig := factoryConfig.Config +func buildSlackNotifier(factoryConfig FactoryConfig) (*SlackNotifier, error) { decryptFunc := factoryConfig.DecryptFunc - endpointURL := channelConfig.Settings.Get("endpointUrl").MustString(SlackAPIEndpoint) - slackURL := decryptFunc(context.Background(), channelConfig.SecureSettings, "url", channelConfig.Settings.Get("url").MustString()) + var settings slackSettings + err := factoryConfig.Config.unmarshalSettings(&settings) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal settings: %w", err) + } + + if settings.EndpointURL == "" { + settings.EndpointURL = SlackAPIEndpoint + } + slackURL := decryptFunc(context.Background(), factoryConfig.Config.SecureSettings, "url", settings.URL) if slackURL == "" { - slackURL = endpointURL + slackURL = settings.EndpointURL } apiURL, err := url.Parse(slackURL) if err != nil { return nil, fmt.Errorf("invalid URL %q", slackURL) } - recipient := strings.TrimSpace(channelConfig.Settings.Get("recipient").MustString()) - if recipient == "" && apiURL.String() == SlackAPIEndpoint { + settings.URL = apiURL.String() + + settings.Recipient = strings.TrimSpace(settings.Recipient) + if settings.Recipient == "" && settings.URL == SlackAPIEndpoint { return nil, errors.New("recipient must be specified when using the Slack chat API") } - mentionChannel := channelConfig.Settings.Get("mentionChannel").MustString() - if mentionChannel != "" && mentionChannel != "here" && mentionChannel != "channel" { - return nil, fmt.Errorf("invalid value for mentionChannel: %q", mentionChannel) + if settings.MentionChannel != "" && settings.MentionChannel != "here" && settings.MentionChannel != "channel" { + return nil, fmt.Errorf("invalid value for mentionChannel: %q", settings.MentionChannel) } - token := decryptFunc(context.Background(), channelConfig.SecureSettings, "token", channelConfig.Settings.Get("token").MustString()) - if token == "" && apiURL.String() == SlackAPIEndpoint { + settings.Token = decryptFunc(context.Background(), factoryConfig.Config.SecureSettings, "token", settings.Token) + if settings.Token == "" && settings.URL == SlackAPIEndpoint { return nil, errors.New("token must be specified when using the Slack chat API") } - mentionUsersStr := channelConfig.Settings.Get("mentionUsers").MustString() - mentionUsers := []string{} - for _, u := range strings.Split(mentionUsersStr, ",") { - u = strings.TrimSpace(u) - if u != "" { - mentionUsers = append(mentionUsers, u) - } + if settings.Username == "" { + settings.Username = "Grafana" } - mentionGroupsStr := channelConfig.Settings.Get("mentionGroups").MustString() - mentionGroups := []string{} - for _, g := range strings.Split(mentionGroupsStr, ",") { - g = strings.TrimSpace(g) - if g != "" { - mentionGroups = append(mentionGroups, g) - } + if settings.Text == "" { + settings.Text = `{{ template "default.message" . }}` + } + if settings.Title == "" { + settings.Title = DefaultMessageTitleEmbed } - return &SlackConfig{ - NotificationChannelConfig: channelConfig, - Recipient: strings.TrimSpace(channelConfig.Settings.Get("recipient").MustString()), - MentionChannel: channelConfig.Settings.Get("mentionChannel").MustString(), - MentionUsers: mentionUsers, - MentionGroups: mentionGroups, - URL: apiURL, - Username: channelConfig.Settings.Get("username").MustString("Grafana"), - IconEmoji: channelConfig.Settings.Get("icon_emoji").MustString(), - IconURL: channelConfig.Settings.Get("icon_url").MustString(), - Token: token, - Text: channelConfig.Settings.Get("text").MustString(`{{ template "default.message" . }}`), - Title: channelConfig.Settings.Get("title").MustString(DefaultMessageTitleEmbed), - }, nil -} -// NewSlackNotifier is the constructor for the Slack notifier -func NewSlackNotifier(config *SlackConfig, - images ImageStore, - webhookSender notifications.WebhookSender, - t *template.Template, -) *SlackNotifier { return &SlackNotifier{ Base: NewBase(&models.AlertNotification{ - Uid: config.UID, - Name: config.Name, - Type: config.Type, - DisableResolveMessage: config.DisableResolveMessage, - Settings: config.Settings, + Uid: factoryConfig.Config.UID, + Name: factoryConfig.Config.Name, + Type: factoryConfig.Config.Type, + DisableResolveMessage: factoryConfig.Config.DisableResolveMessage, + Settings: factoryConfig.Config.Settings, }), - URL: config.URL, - Recipient: config.Recipient, - MentionUsers: config.MentionUsers, - MentionGroups: config.MentionGroups, - MentionChannel: config.MentionChannel, - Username: config.Username, - IconEmoji: config.IconEmoji, - IconURL: config.IconURL, - Token: config.Token, - Text: config.Text, - Title: config.Title, - images: images, - webhookSender: webhookSender, - log: log.New("alerting.notifier.slack"), - tmpl: t, - } + settings: settings, + + images: factoryConfig.ImageStore, + webhookSender: factoryConfig.NotificationService, + log: log.New("alerting.notifier.slack"), + tmpl: factoryConfig.Template, + }, nil } // slackMessage is the slackMessage for sending a slack notification. @@ -201,21 +162,21 @@ func (sn *SlackNotifier) Notify(ctx context.Context, alerts ...*types.Alert) (bo return false, fmt.Errorf("marshal json: %w", err) } - sn.log.Debug("sending Slack API request", "url", sn.URL.String(), "data", string(b)) - request, err := http.NewRequestWithContext(ctx, http.MethodPost, sn.URL.String(), bytes.NewReader(b)) + sn.log.Debug("sending Slack API request", "url", sn.settings.URL, "data", string(b)) + request, err := http.NewRequestWithContext(ctx, http.MethodPost, sn.settings.URL, bytes.NewReader(b)) if err != nil { return false, fmt.Errorf("failed to create HTTP request: %w", err) } request.Header.Set("Content-Type", "application/json") request.Header.Set("User-Agent", "Grafana") - if sn.Token == "" { - if sn.URL.String() == SlackAPIEndpoint { + if sn.settings.Token == "" { + if sn.settings.URL == SlackAPIEndpoint { panic("Token should be set when using the Slack chat API") } } else { sn.log.Debug("adding authorization header to HTTP request") - request.Header.Set("Authorization", fmt.Sprintf("Bearer %s", sn.Token)) + request.Header.Set("Authorization", fmt.Sprintf("Bearer %s", sn.settings.Token)) } if err := sendSlackRequest(request, sn.log); err != nil { @@ -299,23 +260,23 @@ func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, alrts []*types.A ruleURL := joinUrlPath(sn.tmpl.ExternalURL.String(), "/alerting/list", sn.log) req := &slackMessage{ - Channel: tmpl(sn.Recipient), - Text: tmpl(sn.Title), - Username: tmpl(sn.Username), - IconEmoji: tmpl(sn.IconEmoji), - IconURL: tmpl(sn.IconURL), + Channel: tmpl(sn.settings.Recipient), + Text: tmpl(sn.settings.Title), + Username: tmpl(sn.settings.Username), + IconEmoji: tmpl(sn.settings.IconEmoji), + IconURL: tmpl(sn.settings.IconURL), // TODO: We should use the Block Kit API instead: // https://api.slack.com/messaging/composing/layouts#when-to-use-attachments Attachments: []attachment{ { Color: getAlertStatusColor(alerts.Status()), - Title: tmpl(sn.Title), - Fallback: tmpl(sn.Title), + Title: tmpl(sn.settings.Title), + Fallback: tmpl(sn.settings.Title), Footer: "Grafana v" + setting.BuildVersion, FooterIcon: FooterIconURL, Ts: time.Now().Unix(), TitleLink: ruleURL, - Text: tmpl(sn.Text), + Text: tmpl(sn.settings.Text), Fields: nil, // TODO. Should be a config. }, }, @@ -336,19 +297,19 @@ func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, alrts []*types.A mentionsBuilder.WriteString(" ") } } - mentionChannel := strings.TrimSpace(sn.MentionChannel) + mentionChannel := strings.TrimSpace(sn.settings.MentionChannel) if mentionChannel != "" { mentionsBuilder.WriteString(fmt.Sprintf("", mentionChannel, mentionChannel)) } - if len(sn.MentionGroups) > 0 { + if len(sn.settings.MentionGroups) > 0 { appendSpace() - for _, g := range sn.MentionGroups { + for _, g := range sn.settings.MentionGroups { mentionsBuilder.WriteString(fmt.Sprintf("", tmpl(g))) } } - if len(sn.MentionUsers) > 0 { + if len(sn.settings.MentionUsers) > 0 { appendSpace() - for _, u := range sn.MentionUsers { + for _, u := range sn.settings.MentionUsers { mentionsBuilder.WriteString(fmt.Sprintf("<@%s>", tmpl(u))) } } diff --git a/pkg/services/ngalert/notifier/channels/slack_test.go b/pkg/services/ngalert/notifier/channels/slack_test.go index 2f34ca3dae9..f8405c7b646 100644 --- a/pkg/services/ngalert/notifier/channels/slack_test.go +++ b/pkg/services/ngalert/notifier/channels/slack_test.go @@ -271,9 +271,10 @@ func TestSlackNotifier(t *testing.T) { // TODO: allow changing the associated values for different tests. NotificationService: notificationService, DecryptFunc: decryptFn, + Template: tmpl, } - cfg, err := NewSlackConfig(fc) + pn, err := buildSlackNotifier(fc) if c.expInitError != "" { require.Error(t, err) require.Equal(t, c.expInitError, err.Error()) @@ -306,7 +307,6 @@ func TestSlackNotifier(t *testing.T) { ctx := notify.WithGroupKey(context.Background(), "alertname") ctx = notify.WithGroupLabels(ctx, model.LabelSet{"alertname": ""}) - pn := NewSlackNotifier(cfg, fc.ImageStore, fc.NotificationService, tmpl) ok, err := pn.Notify(ctx, c.alerts...) if c.expMsgError != nil { require.Error(t, err) diff --git a/pkg/services/ngalert/notifier/channels/util.go b/pkg/services/ngalert/notifier/channels/util.go index 1333e4ab4ed..ad3c3da8c13 100644 --- a/pkg/services/ngalert/notifier/channels/util.go +++ b/pkg/services/ngalert/notifier/channels/util.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "crypto/tls" + "encoding/json" "errors" "fmt" "io" @@ -13,11 +14,13 @@ import ( "os" "path" "path/filepath" + "strings" "time" "github.com/prometheus/alertmanager/notify" "github.com/prometheus/alertmanager/types" "github.com/prometheus/common/model" + "gopkg.in/yaml.v2" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ngalert/models" @@ -170,6 +173,18 @@ type NotificationChannelConfig struct { SecureSettings map[string][]byte `json:"secureSettings"` } +func (c NotificationChannelConfig) unmarshalSettings(v interface{}) error { + ser, err := c.Settings.Encode() + if err != nil { + return err + } + err = json.Unmarshal(ser, v) + if err != nil { + return err + } + return nil +} + type httpCfg struct { body []byte user string @@ -249,3 +264,56 @@ func joinUrlPath(base, additionalPath string, logger log.Logger) string { var GetBoundary = func() string { return "" } + +type CommaSeparatedStrings []string + +func (r *CommaSeparatedStrings) UnmarshalJSON(b []byte) error { + var str string + if err := json.Unmarshal(b, &str); err != nil { + return err + } + if len(str) > 0 { + res := CommaSeparatedStrings(splitCommaDelimitedString(str)) + *r = res + } + return nil +} + +func (r *CommaSeparatedStrings) MarshalJSON() ([]byte, error) { + if r == nil { + return nil, nil + } + str := strings.Join(*r, ",") + return json.Marshal(str) +} + +func (r *CommaSeparatedStrings) UnmarshalYAML(b []byte) error { + var str string + if err := yaml.Unmarshal(b, &str); err != nil { + return err + } + if len(str) > 0 { + res := CommaSeparatedStrings(splitCommaDelimitedString(str)) + *r = res + } + return nil +} + +func (r *CommaSeparatedStrings) MarshalYAML() ([]byte, error) { + if r == nil { + return nil, nil + } + str := strings.Join(*r, ",") + return yaml.Marshal(str) +} + +func splitCommaDelimitedString(str string) []string { + split := strings.Split(str, ",") + res := make([]string, 0, len(split)) + for _, s := range split { + if tr := strings.TrimSpace(s); tr != "" { + res = append(res, tr) + } + } + return res +}