-
Notifications
You must be signed in to change notification settings - Fork 37
s3: expose retry/backoff constants via environment variables #296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,9 +157,14 @@ 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() | ||
|
Comment on lines
158
to
+161
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in |
||
| }) | ||
| // 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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| package s3 | ||
|
|
||
| import ( | ||
| "context" | ||
| "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) | ||
|
Comment on lines
+44
to
+46
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in |
||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // 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) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alliasgher,
Could you try to add the new environment variables into this function
setupS3Credentialin theutil/credential.goasAWSSecretKey?Then we can allow users to set up the parameters in the Secret and pass them here.