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{}

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
+	log       log.Logger
-	HTTPMethod string
+	ns        notifications.WebhookSender
-	MaxAlerts  int
+	images    ImageStore
-	log        log.Logger
+	tmpl      *template.Template
-	ns         notifications.WebhookSender
+	orgID     int64
-	images     ImageStore
+	maxAlerts int
-	tmpl       *template.Template
+	settings  webhookSettings
-	orgID      int64
-
-	User     string
-	Password string
-
-	AuthorizationScheme      string
-	AuthorizationCredentials string
 }
 
-type WebhookConfig struct {
+type webhookSettings struct {
-	*NotificationChannelConfig
+	URL        string      `json:"url,omitempty" yaml:"url,omitempty"`
-	URL        string
+	HTTPMethod string      `json:"httpMethod,omitempty" yaml:"httpMethod,omitempty"`
-	HTTPMethod string
+	MaxAlerts  interface{} `json:"maxAlerts,omitempty" yaml:"maxAlerts,omitempty"`
-	MaxAlerts  int
 
 	// Authorization Header.
-	AuthorizationScheme      string
+	AuthorizationScheme      string `json:"authorization_scheme,omitempty" yaml:"authorization_scheme,omitempty"`
-	AuthorizationCredentials string
+	AuthorizationCredentials string `json:"authorization_credentials,omitempty" yaml:"authorization_credentials,omitempty"`
 	// HTTP Basic Authentication.
-	User     string
+	User     string `json:"username,omitempty" yaml:"username,omitempty"`
-	Password string
+	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) {
+// buildWebhookNotifier is the constructor for
-	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
 // 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,
+			Uid:                   factoryConfig.Config.UID,
-			Name:                  config.Name,
+			Name:                  factoryConfig.Config.Name,
-			Type:                  config.Type,
+			Type:                  factoryConfig.Config.Type,
-			DisableResolveMessage: config.DisableResolveMessage,
+			DisableResolveMessage: factoryConfig.Config.DisableResolveMessage,
-			Settings:              config.Settings,
+			Settings:              factoryConfig.Config.Settings,
 		}),
-		orgID:                    config.OrgID,
+		orgID:     factoryConfig.Config.OrgID,
-		URL:                      config.URL,
+		log:       logger,
-		User:                     config.User,
+		ns:        factoryConfig.NotificationService,
-		Password:                 config.Password,
+		images:    factoryConfig.ImageStore,
-		AuthorizationScheme:      config.AuthorizationScheme,
+		tmpl:      factoryConfig.Template,
-		AuthorizationCredentials: config.AuthorizationCredentials,
+		maxAlerts: maxAlerts,
-		HTTPMethod:               config.HTTPMethod,
+		settings:  settings,
-		MaxAlerts:                config.MaxAlerts,
+	}, nil
-		log:                      log.New("alerting.notifier.webhook"),
-		ns:                       ns,
-		images:                   images,
-		tmpl:                     t,
-	}
 }
 
 // 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 != "" {
+	if wn.settings.AuthorizationScheme != "" && wn.settings.AuthorizationCredentials != "" {
-		headers["Authorization"] = fmt.Sprintf("%s %s", wn.AuthorizationScheme, wn.AuthorizationCredentials)
+		headers["Authorization"] = fmt.Sprintf("%s %s", wn.settings.AuthorizationScheme, wn.settings.AuthorizationCredentials)
 	}
 
 	cmd := &models.SendWebhookSync{
-		Url:        wn.URL,
+		Url:        wn.settings.URL,
-		User:       wn.User,
+		User:       wn.settings.User,
-		Password:   wn.Password,
+		Password:   wn.settings.Password,
 		Body:       string(body),
-		HttpMethod: wn.HTTPMethod,
+		HttpMethod: wn.settings.HTTPMethod,
 		HttpHeader: headers,
 	}
 

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)