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. diff --git a/notify/sns/sns.go b/notify/sns/sns.go index 1742366334..66d25af50d 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 @@ -106,20 +146,15 @@ 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 { - // 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 +164,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)) @@ -154,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), @@ -193,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 ec559351eb..3c6a302410 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,137 @@ 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", + }, + } { + 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) + }) + } +}