notify: small prepation changes for retry on 429 mechanism

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.

notify/sns/sns.go

@@ -99,27 +99,62 @@ 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
 		data    = notify.GetTemplateData(ctx, n.tmpl, alert, n.logger)
 		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,32 +164,15 @@ 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))
 
 	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.

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: &notify.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: &notify.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)
+		})
+	}
+}