From 1031d1b36ea6eaab8db9dac996a1b674deccb7c7 Mon Sep 17 00:00:00 2001 From: Ali Date: Fri, 24 Apr 2026 00:04:11 +0500 Subject: [PATCH 1/2] s3: expose retry/backoff constants via environment variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The S3 backupstore service currently hardcodes three retry-related constants (`AWSRetryMaxAttempts`, `AWSRetryMaximumAttempts`, `AWSRetryMaximumBackoff`) so operators running against S3-compatible endpoints with different latency/reliability characteristics have no way to tune them without recompiling the binary. Add three optional environment-variable overrides, following the existing `AWS_ENDPOINTS` / `VIRTUAL_HOSTED_STYLE` env-var pattern: * `AWS_RETRY_MAX_ATTEMPTS` — integer, overrides AWSRetryMaxAttempts * `AWS_RETRY_MAXIMUM_ATTEMPTS` — integer, overrides AWSRetryMaximumAttempts * `AWS_RETRY_MAXIMUM_BACKOFF` — Go duration (e.g. "60s", "5m"), overrides AWSRetryMaximumBackoff Empty / missing / malformed values silently fall back to the existing defaults, so there's no behaviour change for users that don't set them. Refs longhorn/longhorn#12155 Signed-off-by: Ali Signed-off-by: Ali --- s3/s3_service.go | 49 +++++++++++++++++++++++-- s3/s3_service_retry_test.go | 71 +++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 s3/s3_service_retry_test.go diff --git a/s3/s3_service.go b/s3/s3_service.go index 213bb45a..af394635 100644 --- a/s3/s3_service.go +++ b/s3/s3_service.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "os" + "strconv" "strings" "time" @@ -40,6 +41,13 @@ const ( // AWSRetryMaximumBackoff specifies the maximum duration between retried attempts. AWSRetryMaximumBackoff = 300 * time.Second + // EnvAWSRetryMaxAttempts overrides AWSRetryMaxAttempts when set to a positive integer. + EnvAWSRetryMaxAttempts = "AWS_RETRY_MAX_ATTEMPTS" + // EnvAWSRetryMaximumAttempts overrides AWSRetryMaximumAttempts when set to a positive integer. + EnvAWSRetryMaximumAttempts = "AWS_RETRY_MAXIMUM_ATTEMPTS" + // EnvAWSRetryMaximumBackoff overrides AWSRetryMaximumBackoff when set to a Go duration string (e.g. "60s", "5m"). + EnvAWSRetryMaximumBackoff = "AWS_RETRY_MAXIMUM_BACKOFF" + // InvalidRequestErrorMsg is the error message returned by S3 Compatible services when the authorization mechanism is not supported, // which can be caused by using AWS Signature Version 2 for signing requests to AWS S3 regions that require AWS Signature Version 4. InvalidRequestErrorMsg = "The authorization mechanism you have provided is not supported. Please use AWS4-HMAC-SHA256." @@ -52,6 +60,41 @@ const ( maxSinglePutObjectSize int64 = 5 * 1024 * 1024 * 1024 ) +// retryMaxAttempts returns the configured retry max attempts, falling back to +// AWSRetryMaxAttempts when the env var is unset, empty, or malformed. +func retryMaxAttempts() int { + if v := os.Getenv(EnvAWSRetryMaxAttempts); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 { + return n + } + } + return AWSRetryMaxAttempts +} + +// retryMaximumAttempts returns the configured retry maximum attempts, falling +// back to AWSRetryMaximumAttempts when the env var is unset, empty, or +// malformed. +func retryMaximumAttempts() int { + if v := os.Getenv(EnvAWSRetryMaximumAttempts); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 { + return n + } + } + return AWSRetryMaximumAttempts +} + +// retryMaximumBackoff returns the configured retry maximum backoff, falling +// back to AWSRetryMaximumBackoff when the env var is unset, empty, or +// malformed. +func retryMaximumBackoff() time.Duration { + if v := os.Getenv(EnvAWSRetryMaximumBackoff); v != "" { + if d, err := time.ParseDuration(v); err == nil && d > 0 { + return d + } + } + return AWSRetryMaximumBackoff +} + func newService(u *url.URL) (*service, error) { s := service{} if u.User != nil { @@ -82,7 +125,7 @@ func (s *service) newInstance(ctx context.Context, retryBackoff bool) (*s3.Clien // Load AWS configuration cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(s.Region), - config.WithRetryMaxAttempts(AWSRetryMaxAttempts), + config.WithRetryMaxAttempts(retryMaxAttempts()), config.WithRequestChecksumCalculation(aws.RequestChecksumCalculationWhenRequired), config.WithResponseChecksumValidation(aws.ResponseChecksumValidationWhenRequired), ) @@ -114,8 +157,8 @@ func (s *service) newInstance(ctx context.Context, retryBackoff bool) (*s3.Clien o.UsePathStyle = usePathStyle if retryBackoff { o.Retryer = retry.NewStandard(func(so *retry.StandardOptions) { - so.MaxAttempts = AWSRetryMaximumAttempts - so.MaxBackoff = AWSRetryMaximumBackoff + so.MaxAttempts = retryMaximumAttempts() + so.MaxBackoff = retryMaximumBackoff() }) } // Google Cloud Storage alters the `Accept-Encoding` header (GCS might changes the header on its way to GCS by appending gzip(gfe) as accepted encoding), diff --git a/s3/s3_service_retry_test.go b/s3/s3_service_retry_test.go new file mode 100644 index 00000000..6d74bbc6 --- /dev/null +++ b/s3/s3_service_retry_test.go @@ -0,0 +1,71 @@ +package s3 + +import ( + "testing" + "time" +) + +func TestRetryMaxAttempts_EnvOverride(t *testing.T) { + cases := []struct { + name string + env string + want int + }{ + {"unset", "", AWSRetryMaxAttempts}, + {"valid", "7", 7}, + {"zero falls back", "0", AWSRetryMaxAttempts}, + {"negative falls back", "-1", AWSRetryMaxAttempts}, + {"garbage falls back", "abc", AWSRetryMaxAttempts}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(EnvAWSRetryMaxAttempts, tc.env) + if got := retryMaxAttempts(); got != tc.want { + t.Fatalf("retryMaxAttempts() = %d, want %d", got, tc.want) + } + }) + } +} + +func TestRetryMaximumAttempts_EnvOverride(t *testing.T) { + cases := []struct { + name string + env string + want int + }{ + {"unset", "", AWSRetryMaximumAttempts}, + {"valid", "20", 20}, + {"zero falls back", "0", AWSRetryMaximumAttempts}, + {"garbage falls back", "x", AWSRetryMaximumAttempts}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(EnvAWSRetryMaximumAttempts, tc.env) + if got := retryMaximumAttempts(); got != tc.want { + t.Fatalf("retryMaximumAttempts() = %d, want %d", got, tc.want) + } + }) + } +} + +func TestRetryMaximumBackoff_EnvOverride(t *testing.T) { + cases := []struct { + name string + env string + want time.Duration + }{ + {"unset", "", AWSRetryMaximumBackoff}, + {"valid seconds", "60s", 60 * time.Second}, + {"valid minutes", "5m", 5 * time.Minute}, + {"zero falls back", "0s", AWSRetryMaximumBackoff}, + {"garbage falls back", "nope", AWSRetryMaximumBackoff}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(EnvAWSRetryMaximumBackoff, tc.env) + if got := retryMaximumBackoff(); got != tc.want { + t.Fatalf("retryMaximumBackoff() = %v, want %v", got, tc.want) + } + }) + } +} From 7fd2623274d5b3b7269b64632926e5e1f61ccbcb Mon Sep 17 00:00:00 2001 From: alliasgher Date: Tue, 18 Aug 2026 05:14:57 +0500 Subject: [PATCH 2/2] s3: keep the custom retryer's maximum attempts effective NewFromConfig calls finalizeRetryMaxAttempts after the option callback. When o.RetryMaxAttempts is nonzero, which it is because newInstance passes config.WithRetryMaxAttempts, it wraps the retryer in retry.AddWithMaxAttempts. That capped the custom retryer at AWS_RETRY_MAX_ATTEMPTS, so AWS_RETRY_MAXIMUM_ATTEMPTS had no effect above 5. Clear o.RetryMaxAttempts when installing the custom retryer. The non-backoff path is unchanged and still honours AWS_RETRY_MAX_ATTEMPTS. The existing tests only cover the env parsing helpers and passed either way, so add one that asserts Retryer.MaxAttempts() on the constructed client. Signed-off-by: alliasgher --- s3/s3_service.go | 5 +++++ s3/s3_service_retry_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/s3/s3_service.go b/s3/s3_service.go index af394635..2edf6b0c 100644 --- a/s3/s3_service.go +++ b/s3/s3_service.go @@ -160,6 +160,11 @@ func (s *service) newInstance(ctx context.Context, retryBackoff bool) (*s3.Clien so.MaxAttempts = retryMaximumAttempts() so.MaxBackoff = retryMaximumBackoff() }) + // NewFromConfig runs finalizeRetryMaxAttempts after this callback, which + // wraps the retryer above in retry.AddWithMaxAttempts(o.RetryMaxAttempts) + // and would cap it at AWS_RETRY_MAX_ATTEMPTS. Clear it so the retryer's + // own AWS_RETRY_MAXIMUM_ATTEMPTS stays effective. + o.RetryMaxAttempts = 0 } // Google Cloud Storage alters the `Accept-Encoding` header (GCS might changes the header on its way to GCS by appending gzip(gfe) as accepted encoding), // which causing signature mismatches and breaks the v2 request signature verification. diff --git a/s3/s3_service_retry_test.go b/s3/s3_service_retry_test.go index 6d74bbc6..1327ef8b 100644 --- a/s3/s3_service_retry_test.go +++ b/s3/s3_service_retry_test.go @@ -1,6 +1,7 @@ package s3 import ( + "context" "testing" "time" ) @@ -69,3 +70,35 @@ func TestRetryMaximumBackoff_EnvOverride(t *testing.T) { }) } } + +// The env-parsing tests above pass whether or not the retryer is actually wired +// into the SDK, so assert the effective value on the constructed client. +// s3.NewFromConfig runs finalizeRetryMaxAttempts after the option callback and +// caps a custom retryer with o.RetryMaxAttempts, which would silently limit +// AWS_RETRY_MAXIMUM_ATTEMPTS to AWS_RETRY_MAX_ATTEMPTS. +func TestRetryMaximumAttempts_AppliedToClient(t *testing.T) { + t.Setenv(EnvAWSRetryMaxAttempts, "5") + t.Setenv(EnvAWSRetryMaximumAttempts, "20") + t.Setenv("AWS_REGION", "us-east-1") + t.Setenv("AWS_ACCESS_KEY_ID", "test") + t.Setenv("AWS_SECRET_ACCESS_KEY", "test") + + s := &service{Region: "us-east-1"} + + client, err := s.newInstance(context.Background(), true) + if err != nil { + t.Fatalf("newInstance() error = %v", err) + } + if got := client.Options().Retryer.MaxAttempts(); got != 20 { + t.Errorf("with retry backoff, Retryer.MaxAttempts() = %d, want 20", got) + } + + // Without the custom retryer, AWS_RETRY_MAX_ATTEMPTS still governs. + client, err = s.newInstance(context.Background(), false) + if err != nil { + t.Fatalf("newInstance() error = %v", err) + } + if got := client.Options().Retryer.MaxAttempts(); got != 5 { + t.Errorf("without retry backoff, Retryer.MaxAttempts() = %d, want 5", got) + } +}