From 29fdbf0354c3dfae39b6d8b60ea11ec68d1e0657 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Mon, 26 Sep 2022 12:51:58 -0400 Subject: [PATCH] Alerting: Refactor webhook notifier to use encoding/json to parse settings instead of simplejson (#55517) * update webhook to use json marshaller * make maxAlerts to be interface{} --- .../ngalert/notifier/channels/webhook.go | 168 +++++++++--------- .../ngalert/notifier/channels/webhook_test.go | 19 +- 2 files changed, 101 insertions(+), 86 deletions(-) diff --git a/pkg/services/ngalert/notifier/channels/webhook.go b/pkg/services/ngalert/notifier/channels/webhook.go index 21917df1a02..4b88f10d325 100644 --- a/pkg/services/ngalert/notifier/channels/webhook.go +++ b/pkg/services/ngalert/notifier/channels/webhook.go @@ -5,6 +5,8 @@ import ( "encoding/json" "errors" "fmt" + "net/http" + "strconv" "github.com/prometheus/alertmanager/notify" "github.com/prometheus/alertmanager/template" @@ -21,98 +23,104 @@ import ( // alert notifications as webhooks. type WebhookNotifier struct { *Base - URL string - HTTPMethod string - MaxAlerts int - log log.Logger - ns notifications.WebhookSender - images ImageStore - tmpl *template.Template - orgID int64 - - User string - Password string - - AuthorizationScheme string - AuthorizationCredentials string + log log.Logger + ns notifications.WebhookSender + images ImageStore + tmpl *template.Template + orgID int64 + maxAlerts int + settings webhookSettings } -type WebhookConfig struct { - *NotificationChannelConfig - URL string - HTTPMethod string - MaxAlerts int +type webhookSettings struct { + URL string `json:"url,omitempty" yaml:"url,omitempty"` + HTTPMethod string `json:"httpMethod,omitempty" yaml:"httpMethod,omitempty"` + MaxAlerts interface{} `json:"maxAlerts,omitempty" yaml:"maxAlerts,omitempty"` // Authorization Header. - AuthorizationScheme string - AuthorizationCredentials string + AuthorizationScheme string `json:"authorization_scheme,omitempty" yaml:"authorization_scheme,omitempty"` + AuthorizationCredentials string `json:"authorization_credentials,omitempty" yaml:"authorization_credentials,omitempty"` // HTTP Basic Authentication. - User string - Password string + User string `json:"username,omitempty" yaml:"username,omitempty"` + Password string `json:"password,omitempty" yaml:"password,omitempty"` +} + +func buildWebhookSettings(factoryConfig FactoryConfig) (webhookSettings, error) { + settings := webhookSettings{} + err := factoryConfig.Config.unmarshalSettings(&settings) + if err != nil { + return settings, fmt.Errorf("failed to unmarshal settings: %w", err) + } + if settings.URL == "" { + return settings, errors.New("required field 'url' is not specified") + } + if settings.HTTPMethod == "" { + settings.HTTPMethod = http.MethodPost + } + settings.User = factoryConfig.DecryptFunc(context.Background(), factoryConfig.Config.SecureSettings, "username", settings.User) + settings.Password = factoryConfig.DecryptFunc(context.Background(), factoryConfig.Config.SecureSettings, "password", settings.Password) + settings.AuthorizationCredentials = factoryConfig.DecryptFunc(context.Background(), factoryConfig.Config.SecureSettings, "authorization_scheme", settings.AuthorizationCredentials) + if settings.AuthorizationCredentials != "" && settings.AuthorizationScheme == "" { + settings.AuthorizationScheme = "Bearer" + } + if settings.User != "" && settings.Password != "" && settings.AuthorizationScheme != "" && settings.AuthorizationCredentials != "" { + return settings, errors.New("both HTTP Basic Authentication and Authorization Header are set, only 1 is permitted") + } + return settings, err } func WebHookFactory(fc FactoryConfig) (NotificationChannel, error) { - cfg, err := NewWebHookConfig(fc.Config, fc.DecryptFunc) + notifier, err := buildWebhookNotifier(fc) if err != nil { return nil, receiverInitError{ Reason: err.Error(), Cfg: *fc.Config, } } - return NewWebHookNotifier(cfg, fc.NotificationService, fc.ImageStore, fc.Template), nil + return notifier, nil } -func NewWebHookConfig(config *NotificationChannelConfig, decryptFunc GetDecryptedValueFn) (*WebhookConfig, error) { - url := config.Settings.Get("url").MustString() - if url == "" { - return nil, errors.New("could not find url property in settings") - } - - user := config.Settings.Get("username").MustString() - password := decryptFunc(context.Background(), config.SecureSettings, "password", config.Settings.Get("password").MustString()) - authorizationScheme := config.Settings.Get("authorization_scheme").MustString("Bearer") - authorizationCredentials := decryptFunc(context.Background(), config.SecureSettings, "authorization_credentials", config.Settings.Get("authorization_credentials").MustString()) - - if user != "" && password != "" && authorizationScheme != "" && authorizationCredentials != "" { - return nil, errors.New("both HTTP Basic Authentication and Authorization Header are set, only 1 is permitted") - } - - return &WebhookConfig{ - NotificationChannelConfig: config, - URL: url, - User: user, - Password: password, - AuthorizationScheme: authorizationScheme, - AuthorizationCredentials: authorizationCredentials, - HTTPMethod: config.Settings.Get("httpMethod").MustString("POST"), - MaxAlerts: config.Settings.Get("maxAlerts").MustInt(0), - }, nil -} - -// NewWebHookNotifier is the constructor for +// buildWebhookNotifier is the constructor for // the WebHook notifier. -func NewWebHookNotifier(config *WebhookConfig, ns notifications.WebhookSender, images ImageStore, t *template.Template) *WebhookNotifier { +func buildWebhookNotifier(factoryConfig FactoryConfig) (*WebhookNotifier, error) { + settings, err := buildWebhookSettings(factoryConfig) + if err != nil { + return nil, err + } + + logger := log.New("alerting.notifier.webhook") + maxAlerts := 0 + if settings.MaxAlerts != nil { + switch value := settings.MaxAlerts.(type) { + case int: + maxAlerts = value + case string: + maxAlerts, err = strconv.Atoi(value) + if err != nil { + logger.Warn("failed to convert setting maxAlerts to integer. Using default", "err", err, "original", value) + maxAlerts = 0 + } + default: + logger.Warn("unexpected type of setting maxAlerts. Expected integer. Using default", "type", fmt.Sprintf("%T", settings.MaxAlerts)) + } + } + return &WebhookNotifier{ 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, }), - orgID: config.OrgID, - URL: config.URL, - User: config.User, - Password: config.Password, - AuthorizationScheme: config.AuthorizationScheme, - AuthorizationCredentials: config.AuthorizationCredentials, - HTTPMethod: config.HTTPMethod, - MaxAlerts: config.MaxAlerts, - log: log.New("alerting.notifier.webhook"), - ns: ns, - images: images, - tmpl: t, - } + orgID: factoryConfig.Config.OrgID, + log: logger, + ns: factoryConfig.NotificationService, + images: factoryConfig.ImageStore, + tmpl: factoryConfig.Template, + maxAlerts: maxAlerts, + settings: settings, + }, nil } // webhookMessage defines the JSON object send to webhook endpoints. @@ -125,7 +133,7 @@ type webhookMessage struct { TruncatedAlerts int `json:"truncatedAlerts"` OrgID int64 `json:"orgId"` - // Deprecated, to be removed in 8.1. + // Deprecated, to be removed in Grafana 10 // These are present to make migration a little less disruptive. Title string `json:"title"` State string `json:"state"` @@ -139,7 +147,7 @@ func (wn *WebhookNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool return false, err } - as, numTruncated := truncateAlerts(wn.MaxAlerts, as) + as, numTruncated := truncateAlerts(wn.maxAlerts, as) var tmplErr error tmpl, data := TmplText(ctx, wn.tmpl, as, wn.log, &tmplErr) @@ -178,16 +186,16 @@ func (wn *WebhookNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool } headers := make(map[string]string) - if wn.AuthorizationScheme != "" && wn.AuthorizationCredentials != "" { - headers["Authorization"] = fmt.Sprintf("%s %s", wn.AuthorizationScheme, wn.AuthorizationCredentials) + if wn.settings.AuthorizationScheme != "" && wn.settings.AuthorizationCredentials != "" { + headers["Authorization"] = fmt.Sprintf("%s %s", wn.settings.AuthorizationScheme, wn.settings.AuthorizationCredentials) } cmd := &models.SendWebhookSync{ - Url: wn.URL, - User: wn.User, - Password: wn.Password, + Url: wn.settings.URL, + User: wn.settings.User, + Password: wn.settings.Password, Body: string(body), - HttpMethod: wn.HTTPMethod, + HttpMethod: wn.settings.HTTPMethod, HttpHeader: headers, } diff --git a/pkg/services/ngalert/notifier/channels/webhook_test.go b/pkg/services/ngalert/notifier/channels/webhook_test.go index 5dd1798e8ed..28181ff7bae 100644 --- a/pkg/services/ngalert/notifier/channels/webhook_test.go +++ b/pkg/services/ngalert/notifier/channels/webhook_test.go @@ -102,7 +102,7 @@ func TestWebhookNotifier(t *testing.T) { "username": "user1", "password": "mysecret", "httpMethod": "PUT", - "maxAlerts": 2 + "maxAlerts": "2" }`, alerts: []*types.Alert{ { @@ -242,14 +242,14 @@ func TestWebhookNotifier(t *testing.T) { "password": "mysecret", "authorization_credentials": "mysecret", "httpMethod": "POST", - "maxAlerts": 2 + "maxAlerts": "2" }`, expInitError: "both HTTP Basic Authentication and Authorization Header are set, only 1 is permitted", }, { name: "Error in initing", settings: `{}`, - expInitError: `could not find url property in settings`, + expInitError: `required field 'url' is not specified`, }, } @@ -269,8 +269,16 @@ func TestWebhookNotifier(t *testing.T) { webhookSender := mockNotificationService() secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) - decryptFn := secretsService.GetDecryptedValue - cfg, err := NewWebHookConfig(m, decryptFn) + + fc := FactoryConfig{ + Config: m, + NotificationService: webhookSender, + DecryptFunc: secretsService.GetDecryptedValue, + ImageStore: &UnavailableImageStore{}, + Template: tmpl, + } + + pn, err := buildWebhookNotifier(fc) if c.expInitError != "" { require.Error(t, err) require.Equal(t, c.expInitError, err.Error()) @@ -281,7 +289,6 @@ func TestWebhookNotifier(t *testing.T) { ctx := notify.WithGroupKey(context.Background(), "alertname") ctx = notify.WithGroupLabels(ctx, model.LabelSet{"alertname": ""}) ctx = notify.WithReceiverName(ctx, "my_receiver") - pn := NewWebHookNotifier(cfg, webhookSender, &UnavailableImageStore{}, tmpl) ok, err := pn.Notify(ctx, c.alerts...) if c.expMsgError != nil { require.False(t, ok)