From 8318e45452b31f44b4538d85ba5896f0199249eb Mon Sep 17 00:00:00 2001 From: George Robinson Date: Mon, 11 Oct 2021 14:50:50 +0100 Subject: [PATCH] Alerting: Fix error message in ngalert when notifications cannot be sent to alertmanager (#40158) --- .../ngalert/notifier/channels/alertmanager.go | 12 +- .../notifier/channels/alertmanager_test.go | 107 ++++++++++++++---- 2 files changed, 90 insertions(+), 29 deletions(-) diff --git a/pkg/services/ngalert/notifier/channels/alertmanager.go b/pkg/services/ngalert/notifier/channels/alertmanager.go index d5d46de6015..14c131fa878 100644 --- a/pkg/services/ngalert/notifier/channels/alertmanager.go +++ b/pkg/services/ngalert/notifier/channels/alertmanager.go @@ -84,7 +84,10 @@ func (n *AlertmanagerNotifier) Notify(ctx context.Context, as ...*types.Alert) ( return false, err } - errCnt := 0 + var ( + lastErr error + numErrs int + ) for _, u := range n.urls { if _, err := sendHTTPRequest(ctx, u, httpCfg{ user: n.basicAuthUser, @@ -92,14 +95,15 @@ func (n *AlertmanagerNotifier) Notify(ctx context.Context, as ...*types.Alert) ( body: body, }, n.logger); err != nil { n.logger.Warn("Failed to send to Alertmanager", "error", err, "alertmanager", n.Name, "url", u.String()) - errCnt++ + lastErr = err + numErrs++ } } - if errCnt == len(n.urls) { + if numErrs == len(n.urls) { // All attempts to send alerts have failed n.logger.Warn("All attempts to send to Alertmanager failed", "alertmanager", n.Name) - return false, fmt.Errorf("failed to send alert to Alertmanager") + return false, fmt.Errorf("failed to send alert to Alertmanager: %w", lastErr) } return true, nil diff --git a/pkg/services/ngalert/notifier/channels/alertmanager_test.go b/pkg/services/ngalert/notifier/channels/alertmanager_test.go index 1a05808d1b7..17211ee6a68 100644 --- a/pkg/services/ngalert/notifier/channels/alertmanager_test.go +++ b/pkg/services/ngalert/notifier/channels/alertmanager_test.go @@ -3,6 +3,7 @@ package channels import ( "context" "encoding/json" + "errors" "net/url" "testing" @@ -17,7 +18,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" ) -func TestAlertmanagerNotifier(t *testing.T) { +func TestNewAlertmanagerNotifier(t *testing.T) { tmpl := templateForTests(t) externalURL, err := url.Parse("http://localhost") @@ -25,11 +26,62 @@ func TestAlertmanagerNotifier(t *testing.T) { tmpl.ExternalURL = externalURL cases := []struct { - name string - settings string - alerts []*types.Alert - expInitError string - receiverName string + name string + settings string + alerts []*types.Alert + expectedInitError string + receiverName string + }{ + { + name: "Error in initing: missing URL", + settings: `{}`, + expectedInitError: `failed to validate receiver of type "alertmanager": could not find url property in settings`, + }, { + name: "Error in initing: invalid URL", + settings: `{ + "url": "://alertmanager.com" + }`, + expectedInitError: `failed to validate receiver "Alertmanager" of type "alertmanager": invalid url property in settings: parse "://alertmanager.com/api/v1/alerts": missing protocol scheme`, + receiverName: "Alertmanager", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + settingsJSON, err := simplejson.NewJson([]byte(c.settings)) + require.NoError(t, err) + + m := &NotificationChannelConfig{ + Name: c.receiverName, + Type: "alertmanager", + Settings: settingsJSON, + } + + decryptFn := ossencryption.ProvideService().GetDecryptedValue + sn, err := NewAlertmanagerNotifier(m, tmpl, decryptFn) + if c.expectedInitError != "" { + require.Equal(t, c.expectedInitError, err.Error()) + return + } + require.NoError(t, err) + require.NotNil(t, sn) + }) + } +} + +func TestAlertmanagerNotifier_Notify(t *testing.T) { + tmpl := templateForTests(t) + + externalURL, err := url.Parse("http://localhost") + require.NoError(t, err) + tmpl.ExternalURL = externalURL + + cases := []struct { + name string + settings string + alerts []*types.Alert + expectedError string + sendHTTPRequestError error + receiverName string }{ { name: "Default config with one alert", @@ -56,16 +108,21 @@ func TestAlertmanagerNotifier(t *testing.T) { }, }, }, { - name: "Error in initing: missing URL", - settings: `{}`, - expInitError: `failed to validate receiver of type "alertmanager": could not find url property in settings`, - }, { - name: "Error in initing: invalid URL", + name: "Error sending to Alertmanager", settings: `{ - "url": "://alertmanager.com" + "url": "https://alertmanager.com" }`, - expInitError: `failed to validate receiver "Alertmanager" of type "alertmanager": invalid url property in settings: parse "://alertmanager.com/api/v1/alerts": missing protocol scheme`, - receiverName: "Alertmanager", + alerts: []*types.Alert{ + { + Alert: model.Alert{ + Labels: model.LabelSet{"__alert_rule_uid__": "rule uid", "alertname": "alert1", "lbl1": "val1"}, + Annotations: model.LabelSet{"ann1": "annv1"}, + }, + }, + }, + expectedError: "failed to send alert to Alertmanager: expected error", + sendHTTPRequestError: errors.New("expected error"), + receiverName: "Alertmanager", }, } for _, c := range cases { @@ -81,10 +138,6 @@ func TestAlertmanagerNotifier(t *testing.T) { decryptFn := ossencryption.ProvideService().GetDecryptedValue sn, err := NewAlertmanagerNotifier(m, tmpl, decryptFn) - if c.expInitError != "" { - require.Equal(t, c.expInitError, err.Error()) - return - } require.NoError(t, err) var body []byte @@ -94,19 +147,23 @@ func TestAlertmanagerNotifier(t *testing.T) { }) sendHTTPRequest = func(ctx context.Context, url *url.URL, cfg httpCfg, logger log.Logger) ([]byte, error) { body = cfg.body - return nil, nil + return nil, c.sendHTTPRequestError } ctx := notify.WithGroupKey(context.Background(), "alertname") ctx = notify.WithGroupLabels(ctx, model.LabelSet{"alertname": ""}) ok, err := sn.Notify(ctx, c.alerts...) - require.NoError(t, err) - require.True(t, ok) - expBody, err := json.Marshal(c.alerts) - require.NoError(t, err) - - require.JSONEq(t, string(expBody), string(body)) + if c.sendHTTPRequestError != nil { + require.EqualError(t, err, c.expectedError) + require.False(t, ok) + } else { + require.NoError(t, err) + require.True(t, ok) + expBody, err := json.Marshal(c.alerts) + require.NoError(t, err) + require.JSONEq(t, string(expBody), string(body)) + } }) } }