From fa24047a9351cf47bf984ba6cc8ff8e13449bf5b Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Sat, 29 Aug 2026 10:45:48 +0200 Subject: [PATCH 1/3] opsgenie: don't return retry on success `retry_stage.go` will ignore the retry, if the notification was delivered successfully. This change is technically a change in behaviour, since it affects the tracing attributes. Signed-off-by: Solomon Jacobs --- notify/opsgenie/opsgenie.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notify/opsgenie/opsgenie.go b/notify/opsgenie/opsgenie.go index a1bbba7043..efb096a8ac 100644 --- a/notify/opsgenie/opsgenie.go +++ b/notify/opsgenie/opsgenie.go @@ -111,7 +111,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err) } } - return true, nil + return false, nil } // Like Split but filter out empty strings. From 14b595d3c28541bc8a1b634a237300c8e05e5f98 Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Sat, 29 Aug 2026 15:10:13 +0200 Subject: [PATCH 2/3] sns: document SDK errors This part of our notification retry logic is especially tricky. Thus, I used an httptest server to generate errors from the SDK. This allows to safely make changes to `ErrorWithReason`. Signed-off-by: Solomon Jacobs --- notify/sns/sns.go | 72 ++++++++++++-------- notify/sns/sns_test.go | 151 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 30 deletions(-) diff --git a/notify/sns/sns.go b/notify/sns/sns.go index 1742366334..5a2bf77b73 100644 --- a/notify/sns/sns.go +++ b/notify/sns/sns.go @@ -99,6 +99,46 @@ func newAWSBuildableClient(c *SNSConfig) (*awshttp.BuildableClient, error) { }), nil } +// classifyClientError turns a failure to build the SNS client into a retry +// decision and a failure reason. +func (n *Notifier) classifyClientError(err error) (bool, error) { + // V2 error handling is different. We don't have awserr.RequestFailure. + // We can check for a generic smithy.APIError to see if it's a service error. + if apiErr, ok := errors.AsType[smithy.APIError](err); ok { + // To maintain compatibility with the retrier, we attempt to get an HTTP status code. + var respErr *smithyhttp.ResponseError + if errors.As(err, &respErr) && respErr.Response != nil { + return n.retrier.Check(respErr.Response.StatusCode, strings.NewReader(apiErr.ErrorMessage())) + } + // Fallback if we can't get a status code. + return true, fmt.Errorf("failed to create SNS client: %s: %s", apiErr.ErrorCode(), apiErr.ErrorMessage()) + } + return true, err +} + +// classifyPublishError turns a failed Publish into a retry decision and a +// failure reason. +func (n *Notifier) classifyPublishError(err error) (bool, error) { + // V2 error handling uses errors.As to inspect the error chain. + if apiErr, ok := errors.AsType[smithy.APIError](err); ok { + var statusCode int + var respErr *smithyhttp.ResponseError + // Try to extract the HTTP status code for the retrier. + if errors.As(err, &respErr) && respErr.Response != nil { + statusCode = respErr.Response.StatusCode + } + + // If we got a status code, use the retrier logic. + if statusCode != 0 { + retryable, checkErr := n.retrier.Check(statusCode, strings.NewReader(apiErr.ErrorMessage())) + reasonErr := notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(statusCode), checkErr) + return retryable, reasonErr + } + } + // Fallback for non-API errors or if status code extraction fails. + return true, err +} + func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, error) { var ( tmplErr error @@ -108,18 +148,7 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err client, err := n.createSNSClient(ctx, tmpl, &tmplErr) if err != nil { - // V2 error handling is different. We don't have awserr.RequestFailure. - // We can check for a generic smithy.APIError to see if it's a service error. - if apiErr, ok := errors.AsType[smithy.APIError](err); ok { - // To maintain compatibility with the retrier, we attempt to get an HTTP status code. - var respErr *smithyhttp.ResponseError - if errors.As(err, &respErr) && respErr.Response != nil { - return n.retrier.Check(respErr.Response.StatusCode, strings.NewReader(apiErr.ErrorMessage())) - } - // Fallback if we can't get a status code. - return true, fmt.Errorf("failed to create SNS client: %s: %s", apiErr.ErrorCode(), apiErr.ErrorMessage()) - } - return true, err + return n.classifyClientError(err) } publishInput, err := n.createPublishInput(ctx, tmpl, &tmplErr) @@ -129,24 +158,7 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err publishOutput, err := client.Publish(ctx, publishInput) if err != nil { - // V2 error handling uses errors.As to inspect the error chain. - if apiErr, ok := errors.AsType[smithy.APIError](err); ok { - var statusCode int - var respErr *smithyhttp.ResponseError - // Try to extract the HTTP status code for the retrier. - if errors.As(err, &respErr) && respErr.Response != nil { - statusCode = respErr.Response.StatusCode - } - - // If we got a status code, use the retrier logic. - if statusCode != 0 { - retryable, checkErr := n.retrier.Check(statusCode, strings.NewReader(apiErr.ErrorMessage())) - reasonErr := notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(statusCode), checkErr) - return retryable, reasonErr - } - } - // Fallback for non-API errors or if status code extraction fails. - return true, err + return n.classifyPublishError(err) } n.logger.Debug("SNS message successfully published", "message_id", aws.ToString(publishOutput.MessageId), "sequence_number", aws.ToString(publishOutput.SequenceNumber)) diff --git a/notify/sns/sns_test.go b/notify/sns/sns_test.go index ec559351eb..0d9d9cddeb 100644 --- a/notify/sns/sns_test.go +++ b/notify/sns/sns_test.go @@ -15,14 +15,24 @@ package sns import ( "context" + "errors" + "fmt" + "net/http" "net/url" "testing" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/aws/retry" + awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" + snstypes "github.com/aws/aws-sdk-go-v2/service/sns/types" + "github.com/aws/smithy-go" + smithyhttp "github.com/aws/smithy-go/transport/http" commoncfg "github.com/prometheus/common/config" "github.com/prometheus/common/promslog" "github.com/prometheus/sigv4" "github.com/stretchr/testify/require" + "github.com/prometheus/alertmanager/notify" "github.com/prometheus/alertmanager/template" "github.com/prometheus/alertmanager/types" ) @@ -147,3 +157,144 @@ func createTmpl(t *testing.T) *template.Template { tmpl.ExternalURL, _ = url.Parse("http://am") return tmpl } + +// sdkErr creates errors faithful to what the SDK would create. +func sdkErr(statusCode int, requestID string, deserErr error) error { + var err error = &awshttp.ResponseError{ + ResponseError: &smithyhttp.ResponseError{ + Response: &smithyhttp.Response{Response: &http.Response{StatusCode: statusCode}}, + Err: deserErr, + }, + RequestID: requestID, + } + if statusCode >= 500 { + err = &retry.MaxAttemptsError{Attempt: 1, Err: err} + } + return &smithy.OperationError{ + ServiceID: "SNS", + OperationName: "Publish", + Err: err, + } +} + +func TestClassifyClientError(t *testing.T) { + for _, tc := range []struct { + title string + err error + + retry bool + reason notify.Reason + errMsg string + }{ + { + title: "client build, client error", + err: sdkErr(http.StatusBadRequest, "req-1", &snstypes.InvalidParameterException{Message: aws.String("bogus")}), + retry: false, + reason: notify.DefaultReason, + errMsg: "unexpected status code 400", + }, + { + title: "client build, server error", + err: sdkErr(http.StatusInternalServerError, "req-1", &snstypes.InternalErrorException{Message: aws.String("bogus")}), + retry: true, + reason: notify.DefaultReason, + errMsg: "unexpected status code 500", + }, + { + title: "template error", + err: notify.NewErrorWithReason(notify.ClientErrorReason, errors.New("execute 'api_url' template")), + retry: true, + reason: notify.ClientErrorReason, + errMsg: "execute 'api_url' template", + }, + } { + t.Run(tc.title, func(t *testing.T) { + classifyNotifier := &Notifier{retrier: ¬ify.Retrier{}} + retry, err := classifyNotifier.classifyClientError(tc.err) + require.Error(t, err) + require.Equal(t, tc.retry, retry) + reason := notify.DefaultReason + if e, ok := errors.AsType[*notify.ErrorWithReason](err); ok { + reason = e.Reason + } + require.Equal(t, tc.reason, reason) + require.Contains(t, err.Error(), tc.errMsg) + }) + } +} + +func TestClassifyPublishError(t *testing.T) { + for _, tc := range []struct { + title string + err error + + retry bool + reason notify.Reason + errMsg string + }{ + { + title: "publish, client error", + err: sdkErr(http.StatusBadRequest, "req-1", &snstypes.InvalidParameterException{Message: aws.String("bogus")}), + retry: false, + reason: notify.ClientErrorReason, + errMsg: "unexpected status code 400", + }, + { + title: "publish, auth error", + err: sdkErr(http.StatusForbidden, "req-1", &snstypes.AuthorizationErrorException{Message: aws.String("bogus")}), + retry: false, + reason: notify.AuthErrorReason, + errMsg: "unexpected status code 403", + }, + { + // Surprisingly, the Publish deserializer has no case for the + // "Throttled" error code (unlike e.g. PublishBatch), so it never + // builds snstypes.ThrottledException. + title: "publish, rate limited", + err: sdkErr(http.StatusTooManyRequests, "req-1", &smithy.GenericAPIError{Code: "Throttled", Message: "bogus"}), + retry: false, + reason: notify.RateLimitedReason, + errMsg: "unexpected status code 429", + }, + { + title: "publish, server error", + err: sdkErr(http.StatusInternalServerError, "req-1", &snstypes.InternalErrorException{Message: aws.String("bogus")}), + retry: true, + reason: notify.ServerErrorReason, + errMsg: "unexpected status code 500", + }, + { + // The AWS HTTP client refuses to follow 301/302, and the deserializer + // builds a GenericAPIError from the empty redirect body. + title: "publish, redirect status", + err: sdkErr(http.StatusMovedPermanently, "", &smithy.GenericAPIError{Code: "UnknownError", Message: "UnknownError"}), + retry: false, + reason: notify.DefaultReason, + errMsg: "unexpected status code 301", + }, + { + // The SDK only builds an APIError for a non-2xx response, so a 2xx + // carries a DeserializationError instead and lands here. + title: "publish, no API error in the chain", + err: sdkErr(http.StatusOK, "", &smithy.DeserializationError{ + Err: fmt.Errorf("failed to decode response body, %w", errors.New("PublishResult node not found")), + }), + retry: true, + reason: notify.DefaultReason, + errMsg: "deserialization failed", + }, + } { + t.Run(tc.title, func(t *testing.T) { + classifyNotifier := &Notifier{retrier: ¬ify.Retrier{}} + retry, err := classifyNotifier.classifyPublishError(tc.err) + require.Error(t, err) + require.Equal(t, tc.retry, retry) + reason := notify.DefaultReason + if e, ok := errors.AsType[*notify.ErrorWithReason](err); ok { + reason = e.Reason + } + require.Equal(t, tc.reason, reason) + require.Contains(t, err.Error(), tc.errMsg) + }) + } +} From 5d63e9c46a71df6cd3fc19662b001b4fe4339f42 Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Sat, 29 Aug 2026 17:05:00 +0200 Subject: [PATCH 3/3] sns: derive apiUrl before client construction This change simplifies the number errors handled by `TestClassifyClientError`. No test coverage is lost, since `TestNotifyWithInvalidTemplate` already covers the behaviour in question. Small change in behaviour: `template` errors are now always surfaced before client errors. Signed-off-by: Solomon Jacobs --- notify/sns/sns.go | 19 ++++++++++--------- notify/sns/sns_test.go | 7 ------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/notify/sns/sns.go b/notify/sns/sns.go index 5a2bf77b73..66d25af50d 100644 --- a/notify/sns/sns.go +++ b/notify/sns/sns.go @@ -146,7 +146,13 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err tmpl = notify.TmplText(n.tmpl, data, &tmplErr) ) - client, err := n.createSNSClient(ctx, tmpl, &tmplErr) + // Resolve the API URL from the template. + apiURL := tmpl(n.conf.APIUrl) + if tmplErr != nil { + return true, notify.NewErrorWithReason(notify.ClientErrorReason, fmt.Errorf("execute 'api_url' template: %w", tmplErr)) + } + + client, err := n.createSNSClient(ctx, apiURL) if err != nil { return n.classifyClientError(err) } @@ -166,7 +172,7 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err return false, nil } -func (n *Notifier) createSNSClient(ctx context.Context, tmpl func(string) string, tmplErr *error) (*sns.Client, error) { +func (n *Notifier) createSNSClient(ctx context.Context, tmplApiURL string) (*sns.Client, error) { // Base configuration options that apply to both STS (if used) and the final SNS client. baseCfgOpts := []func(*awsconfig.LoadOptions) error{ awsconfig.WithHTTPClient(n.client), @@ -205,13 +211,8 @@ func (n *Notifier) createSNSClient(ctx context.Context, tmpl func(string) string snsCfgOpts = append(snsCfgOpts, awsconfig.WithCredentialsProvider(aws.NewCredentialsCache(stsProvider))) } - // Resolve the API URL from the template. - apiURL := tmpl(n.conf.APIUrl) - if *tmplErr != nil { - return nil, notify.NewErrorWithReason(notify.ClientErrorReason, fmt.Errorf("execute 'api_url' template: %w", *tmplErr)) - } - if apiURL != "" { - snsCfgOpts = append(snsCfgOpts, awsconfig.WithBaseEndpoint(apiURL)) + if tmplApiURL != "" { + snsCfgOpts = append(snsCfgOpts, awsconfig.WithBaseEndpoint(tmplApiURL)) } // Load the final configuration for the SNS client. diff --git a/notify/sns/sns_test.go b/notify/sns/sns_test.go index 0d9d9cddeb..3c6a302410 100644 --- a/notify/sns/sns_test.go +++ b/notify/sns/sns_test.go @@ -200,13 +200,6 @@ func TestClassifyClientError(t *testing.T) { reason: notify.DefaultReason, errMsg: "unexpected status code 500", }, - { - title: "template error", - err: notify.NewErrorWithReason(notify.ClientErrorReason, errors.New("execute 'api_url' template")), - retry: true, - reason: notify.ClientErrorReason, - errMsg: "execute 'api_url' template", - }, } { t.Run(tc.title, func(t *testing.T) { classifyNotifier := &Notifier{retrier: ¬ify.Retrier{}}