feat!: Switch from min throttle delay to retry mode (#488)

The v2 AWS SDK has no equivalent setting for a minimum delay for
throttle errors. So, the delay has been replaced with the new retry mode
configuration setting that is available. The "adaptive" mode accounts
for throttle errors, although it is not configurable any further than
that, and the mode itself is still experimental.

The choice of retry mode still only applies to the SSM parameter store,
but it could be extended to other implementations very easily.

The `--min-throttle-delay` option remains in place so that existing
scripted calls don't break, but the value no longer has any effect.

The README is updated accordingly, mentioning the loss of min throttle
delay as a breaking change. The removal of the `CHAMBER_NO_PATHS`
avoiding of the path-based API for the SSM parameter store is also added
as a breaking change, although that already happened when chamber moved
to the v2 SDK.

README.md

@@ -6,6 +6,19 @@ secrets in SSM Parameter Store, an AWS service for storing secrets.
 For detailed info about using chamber, please read
 [The Right Way To Manage Secrets](https://aws.amazon.com/blogs/mt/the-right-way-to-store-secrets-using-parameter-store/)
 
+## v3.0 Breaking Changes
+
+_Version 3.0 has not yet been released. Changes described here are forward-looking._
+
+* **Use of the SSM Parameter Store's path-based API is now required.** Support
+  added in v2.0 to avoid it has been removed. The `CHAMBER_NO_PATHS` environment
+  variable no longer has any effect. You must migrate to the new storage format
+  using the instructions below.
+* **The `--min-throttle-delay` option no longer has any effect.** Support for
+  specifying a minimum throttle delay has been removed from the underlying AWS
+  SDK with no direct replacement. Instead, set the new `--retry-mode` option to
+  "adaptive" to use an experimental model that accounts for throttling errors.
+
 ## v2.0 Breaking Changes
 
 Starting with version 2.0, chamber uses parameter store's path based API by default.

cmd/root.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/aws/aws-sdk-go-v2/aws"
 	analytics "github.com/segmentio/analytics-go/v3"
 	"github.com/segmentio/chamber/v2/store"
 	"github.com/spf13/cobra"
@@ -22,9 +23,11 @@ var (
 	validServiceFormatWithLabel     = regexp.MustCompile(`^[\w\-\.\:]+$`)
 	validServicePathFormatWithLabel = regexp.MustCompile(`^[\w\-\.]+((\/[\w\-\.]+)+(\:[\w\-\.]+)*)?$`)
 
-	verbose          bool
-	numRetries       int
+	verbose    bool
+	numRetries int
+	// Deprecated: Use retryMode instead.
 	minThrottleDelay time.Duration
+	retryMode        string
 	chamberVersion   string
 	// one of *Backend consts
 	backend             string
@@ -74,7 +77,12 @@ var RootCmd = &cobra.Command{
 
 func init() {
 	RootCmd.PersistentFlags().IntVarP(&numRetries, "retries", "r", DefaultNumRetries, "For SSM or Secrets Manager, the number of retries we'll make before giving up; AKA $CHAMBER_RETRIES")
-	RootCmd.PersistentFlags().DurationVarP(&minThrottleDelay, "min-throttle-delay", "", store.DefaultMinThrottleDelay, "For SSM, minimal delay before retrying throttled requests. Default 500ms.")
+	RootCmd.PersistentFlags().DurationVarP(&minThrottleDelay, "min-throttle-delay", "", 0, "DEPRECATED and no longer has any effect. Use retry-mode instead")
+	RootCmd.PersistentFlags().StringVarP(&retryMode, "retry-mode", "", store.DefaultRetryMode.String(),
+		`For SSM, the model used to retry requests
+  `+aws.RetryModeStandard.String()+`
+  `+aws.RetryModeAdaptive.String(),
+	)
 	RootCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "", false, "Print more information to STDOUT")
 	RootCmd.PersistentFlags().StringVarP(&backendFlag, "backend", "b", "ssm",
 		`Backend to use; AKA $CHAMBER_SECRET_BACKEND
@@ -213,7 +221,11 @@ func getSecretStore() (store.Store, error) {
 			return nil, errors.New("Unable to use --kms-key-alias with this backend. Use CHAMBER_KMS_KEY_ALIAS instead.")
 		}
 
-		s, err = store.NewSSMStoreWithMinThrottleDelay(numRetries, minThrottleDelay)
+		parsedRetryMode, err := aws.ParseRetryMode(retryMode)
+		if err != nil {
+			return nil, fmt.Errorf("Invalid retry mode %s", retryMode)
+		}
+		s, err = store.NewSSMStoreWithRetryMode(numRetries, parsedRetryMode)
 	default:
 		return nil, fmt.Errorf("invalid backend `%s`", backend)
 	}

store/s3store.go

@@ -56,7 +56,7 @@ type S3Store struct {
 }
 
 func NewS3StoreWithBucket(numRetries int, bucket string) (*S3Store, error) {
-	config, _, err := getConfig(numRetries)
+	config, _, err := getConfig(numRetries, aws.RetryModeStandard)
 	if err != nil {
 		return nil, err
 	}

store/s3storeKMS.go

@@ -42,7 +42,7 @@ type S3KMSStore struct {
 }
 
 func NewS3KMSStore(numRetries int, bucket string, kmsKeyAlias string) (*S3KMSStore, error) {
-	config, _, err := getConfig(numRetries)
+	config, _, err := getConfig(numRetries, aws.RetryModeStandard)
 	if err != nil {
 		return nil, err
 	}

store/secretsmanagerstore.go

@@ -81,7 +81,7 @@ type SecretsManagerStore struct {
 
 // NewSecretsManagerStore creates a new SecretsManagerStore
 func NewSecretsManagerStore(numRetries int) (*SecretsManagerStore, error) {
-	cfg, _, err := getConfig(numRetries)
+	cfg, _, err := getConfig(numRetries, aws.RetryModeStandard)
 	if err != nil {
 		return nil, err
 	}

store/secretsmanagerstore_test.go

@@ -121,7 +121,7 @@ func mockGetCallerIdentity(_ *sts.GetCallerIdentityInput) (*sts.GetCallerIdentit
 
 func NewTestSecretsManagerStore(secrets map[string]mockSecret, outputs map[string]secretsmanager.DescribeSecretOutput) *SecretsManagerStore {
 	return &SecretsManagerStore{
-		svc:    &apiSecretsManagerMock{
+		svc: &apiSecretsManagerMock{
 			CreateSecretFunc: func(ctx context.Context, params *secretsmanager.CreateSecretInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.CreateSecretOutput, error) {
 				return mockCreateSecret(params, secrets)
 			},

store/shared.go

@@ -14,12 +14,12 @@ const (
 	CustomSSMEndpointEnvVar = "CHAMBER_AWS_SSM_ENDPOINT"
 )
 
-func getConfig(numRetries int) (aws.Config, string, error) {
+func getConfig(numRetries int, retryMode aws.RetryMode) (aws.Config, string, error) {
 	endpointResolver := func(service, region string, options ...interface{}) (aws.Endpoint, error) {
 		customSsmEndpoint, ok := os.LookupEnv(CustomSSMEndpointEnvVar)
 		if ok {
 			return aws.Endpoint{
-				URL: customSsmEndpoint,
+				URL:    customSsmEndpoint,
 				Source: aws.EndpointSourceCustom,
 			}, nil
 		}
@@ -35,6 +35,7 @@ func getConfig(numRetries int) (aws.Config, string, error) {
 	cfg, err := config.LoadDefaultConfig(context.TODO(),
 		config.WithRegion(region),
 		config.WithRetryMaxAttempts(numRetries),
+		config.WithRetryMode(retryMode),
 		config.WithEndpointResolverWithOptions(aws.EndpointResolverWithOptionsFunc(endpointResolver)),
 	)
 	if err != nil {

store/shared_test.go

@@ -0,0 +1,39 @@
+package store
+
+import (
+	"os"
+	"testing"
+
+	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetConfig(t *testing.T) {
+	originalEndpoint := os.Getenv(CustomSSMEndpointEnvVar)
+	os.Setenv(CustomSSMEndpointEnvVar, "https://example.com/custom-endpoint")
+	if originalEndpoint != "" {
+		defer os.Setenv(CustomSSMEndpointEnvVar, originalEndpoint)
+	} else {
+		defer os.Unsetenv(CustomSSMEndpointEnvVar)
+	}
+
+	originalRegion := os.Getenv(RegionEnvVar)
+	os.Setenv(RegionEnvVar, "us-west-2")
+	if originalRegion != "" {
+		defer os.Setenv(RegionEnvVar, originalRegion)
+	} else {
+		defer os.Unsetenv(RegionEnvVar)
+	}
+
+	config, region, err := getConfig(3, aws.RetryModeStandard)
+
+	assert.NoError(t, err)
+	assert.Equal(t, "us-west-2", region)
+
+	endpoint, err := config.EndpointResolverWithOptions.ResolveEndpoint("ssm", "us-west-2")
+	assert.Equal(t, "https://example.com/custom-endpoint", endpoint.URL)
+	assert.Equal(t, aws.EndpointSourceCustom, endpoint.Source)
+
+	assert.Equal(t, 3, config.RetryMaxAttempts)
+	assert.Equal(t, aws.RetryModeStandard, config.RetryMode)
+}

store/ssmstore.go

@@ -18,9 +18,8 @@ const (
 	// DefaultKeyID is the default alias for the KMS key used to encrypt/decrypt secrets
 	DefaultKeyID = "alias/parameter_store_key"
 
-	// DefaultMinThrottleDelay is the default delay before retrying throttled requests
-	// DefaultMinThrottleDelay = client.DefaultRetryerMinThrottleDelay
-	DefaultMinThrottleDelay = 0
+	// DefaultRetryMode is the default retry mode for AWS SDK configurations.
+	DefaultRetryMode = aws.RetryModeStandard
 )
 
 // validPathKeyFormat is the format that is expected for key names inside parameter store
@@ -47,28 +46,30 @@ type SSMStore struct {
 
 // NewSSMStore creates a new SSMStore
 func NewSSMStore(numRetries int) (*SSMStore, error) {
-	return ssmStoreUsingRetryer(numRetries, DefaultMinThrottleDelay)
+	return ssmStoreUsingRetryer(numRetries, DefaultRetryMode)
 }
 
 // NewSSMStoreWithMinThrottleDelay creates a new SSMStore with the aws sdk max retries and min throttle delay are configured.
+//
+// Deprecated: The AWS SDK no longer supports specifying a minimum throttle delay. Instead, use
+// NewSSMStoreWithRetryMode.
 func NewSSMStoreWithMinThrottleDelay(numRetries int, minThrottleDelay time.Duration) (*SSMStore, error) {
-	return ssmStoreUsingRetryer(numRetries, minThrottleDelay)
+	return ssmStoreUsingRetryer(numRetries, DefaultRetryMode)
 }
 
-func ssmStoreUsingRetryer(numRetries int, minThrottleDelay time.Duration) (*SSMStore, error) {
-	cfg, _, err := getConfig(numRetries)
+// NewSSMStoreWithRetryMode creates a new SSMStore, configuring the underlying AWS SDK with the
+// given maximum number of retries and retry mode.
+func NewSSMStoreWithRetryMode(numRetries int, retryMode aws.RetryMode) (*SSMStore, error) {
+	return ssmStoreUsingRetryer(numRetries, retryMode)
+}
+
+func ssmStoreUsingRetryer(numRetries int, retryMode aws.RetryMode) (*SSMStore, error) {
+	cfg, _, err := getConfig(numRetries, retryMode)
 
 	if err != nil {
 		return nil, err
 	}
 
-	// FIXME minThrottleDelay is ignored
-	// retryer := retry.NewStandard(
-	// 	func(o *retry.StandardOptions) {
-	// 		o.MaxAttempts = numRetries
-	// 	},
-	// )
-
 	usePaths := true
 	_, ok := os.LookupEnv("CHAMBER_NO_PATHS")
 	if ok {

store/ssmstore_test.go

@@ -270,19 +270,19 @@ func NewTestSSMStore(parameters map[string]mockParameter, usePaths bool) *SSMSto
 				return mockDeleteParameter(params, parameters)
 			},
 			DescribeParametersFunc: func(ctx context.Context, params *ssm.DescribeParametersInput, optFns ...func(*ssm.Options)) (*ssm.DescribeParametersOutput, error) {
-				return mockDescribeParameters(params, parameters);
+				return mockDescribeParameters(params, parameters)
 			},
 			GetParameterHistoryFunc: func(ctx context.Context, params *ssm.GetParameterHistoryInput, optFns ...func(*ssm.Options)) (*ssm.GetParameterHistoryOutput, error) {
-				return mockGetParameterHistory(params, parameters);
+				return mockGetParameterHistory(params, parameters)
 			},
 			GetParametersFunc: func(ctx context.Context, params *ssm.GetParametersInput, optFns ...func(*ssm.Options)) (*ssm.GetParametersOutput, error) {
-				return mockGetParameters(params, parameters);
+				return mockGetParameters(params, parameters)
 			},
 			GetParametersByPathFunc: func(ctx context.Context, params *ssm.GetParametersByPathInput, optFns ...func(*ssm.Options)) (*ssm.GetParametersByPathOutput, error) {
-				return mockGetParametersByPath(params, parameters);
+				return mockGetParametersByPath(params, parameters)
 			},
 			PutParameterFunc: func(ctx context.Context, params *ssm.PutParameterInput, optFns ...func(*ssm.Options)) (*ssm.PutParameterOutput, error) {
-				return mockPutParameter(params, parameters);
+				return mockPutParameter(params, parameters)
 			},
 		},
 	}
@@ -330,24 +330,22 @@ func TestNewSSMStore(t *testing.T) {
 		assert.ErrorAs(t, err, &notFoundError)
 	})
 
-	// FIXME minThrottleDelay is ignored
-	// t.Run("Should set aws sdk min throttle delay to default", func(t *testing.T) {
-	// 	s, err := NewSSMStore(1)
-	// 	assert.Nil(t, err)
-	// 	assert.Equal(t, DefaultMinThrottleDelay, s.svc.(*ssm.SSM).Config.Retryer.(client.DefaultRetryer).MinThrottleDelay)
-	// })
+	t.Run("Should set AWS SDK retry mode to default", func(t *testing.T) {
+		s, err := NewSSMStore(1)
+		assert.Nil(t, err)
+		assert.Equal(t, DefaultRetryMode, s.config.RetryMode)
+	})
 
 }
 
-// FIXME minThrottleDelay is ignored
-// func TestNewSSMStoreMinThrottleDelay(t *testing.T) {
-// 	t.Run("Should configure aws sdk retryer - num max retries and min throttle delay", func(t *testing.T) {
-// 		s, err := NewSSMStoreWithMinThrottleDelay(2, time.Duration(1000)*time.Millisecond)
-// 		assert.Nil(t, err)
-// 		assert.Equal(t, 2, s.config.Retryer().MaxAttempts())
-// 		assert.Equal(t, time.Duration(1000)*time.Millisecond, s.svc.(*ssm.SSM).Config.Retryer.(client.DefaultRetryer).MinThrottleDelay)
-// 	})
-// }
+func TestNewSSMStoreWithRetryMode(t *testing.T) {
+	t.Run("Should configure AWS SDK max attempts and retry mode", func(t *testing.T) {
+		s, err := NewSSMStoreWithRetryMode(2, aws.RetryModeAdaptive)
+		assert.Nil(t, err)
+		assert.Equal(t, 2, s.config.RetryMaxAttempts)
+		assert.Equal(t, aws.RetryModeAdaptive, s.config.RetryMode)
+	})
+}
 
 func TestWrite(t *testing.T) {
 	parameters := map[string]mockParameter{}