Repository navigation
feat(dns): retry requests on 408, 502, 503, 504 #1854
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: main
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 ( | |
| "io" | ||
| "math/rand/v2" | ||
| "net/http" | ||
| "slices" | ||
| "strconv" | ||
| "time" | ||
|
|
||
|
|
@@ -215,13 +216,15 @@ type DefaultClientFactory struct { | |
| ProviderDefaultRegion string | ||
| } | ||
|
|
||
| func (f *DefaultClientFactory) defaultConfigOptions(customEndpoint string) []config.ConfigurationOption { | ||
| func (f *DefaultClientFactory) defaultConfigOptions(customEndpoint string, additionalRetryStatusCodes ...int) []config.ConfigurationOption { | ||
| // Retry HTTP 429 rate limits for every service, outside the authentication transport. | ||
| // Services can opt in to retrying additional status codes. | ||
| retryRoundTripper := &RetryTransport{ | ||
| Base: f.RoundTripper, | ||
| MaxRetries: defaultMaxRetries, | ||
| BaseBackoff: defaultBaseBackoff, | ||
| MaxJitter: defaultMaxJitter, // Always added to wait time | ||
| Base: f.RoundTripper, | ||
| MaxRetries: defaultMaxRetries, | ||
| BaseBackoff: defaultBaseBackoff, | ||
| MaxJitter: defaultMaxJitter, // Always added to wait time | ||
| AdditionalRetryStatusCodes: additionalRetryStatusCodes, | ||
|
Member
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. Could we please have a note here that this is not intended for GA yet and only for selected services?
Member
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. I think we also had it that way for the HTTP 429 retries back then.
Contributor
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 |
||
| } | ||
|
|
||
| apiClientConfigOptions := []config.ConfigurationOption{ | ||
|
|
@@ -545,7 +548,13 @@ func (f *DefaultClientFactory) newAuthorizationV2Client() (authorization.Default | |
| } | ||
|
|
||
| func (f *DefaultClientFactory) newDnsV1Client() (dns.DefaultAPI, error) { | ||
| apiClientConfigOptions := f.defaultConfigOptions(f.CustomEndpoints.DnsCustomEndpoint) | ||
| apiClientConfigOptions := f.defaultConfigOptions( | ||
| f.CustomEndpoints.DnsCustomEndpoint, | ||
| http.StatusRequestTimeout, | ||
| http.StatusBadGateway, | ||
| http.StatusServiceUnavailable, | ||
| http.StatusGatewayTimeout, | ||
| ) | ||
|
|
||
| apiClient, err := dns.NewAPIClient(apiClientConfigOptions...) | ||
| if err != nil { | ||
|
|
@@ -699,12 +708,15 @@ func (f *DefaultClientFactory) newObservabilityV1Client() (observability.Default | |
| return apiClient.DefaultAPI, nil | ||
| } | ||
|
|
||
| // RetryTransport wraps an underlying RoundTripper to retry on HTTP 429 rate limit errors with jitter. | ||
| // RetryTransport wraps an underlying RoundTripper to retry on HTTP 429 and configured additional status codes with jitter. | ||
| type RetryTransport struct { | ||
| Base http.RoundTripper | ||
| MaxRetries int | ||
| BaseBackoff time.Duration | ||
| MaxJitter time.Duration | ||
| // AdditionalRetryStatusCodes are retried in addition to HTTP 429, which is always retried. | ||
| // Retrying additional status codes is not yet generally available (GA) and is intended only for selected services. | ||
| AdditionalRetryStatusCodes []int | ||
| } | ||
|
|
||
| func (t *RetryTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
|
|
@@ -746,8 +758,8 @@ func (t *RetryTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |
|
|
||
| resp, err = base.RoundTrip(reqClone) | ||
|
|
||
| // If success or non-429 error, return immediately | ||
| if err != nil || resp.StatusCode != http.StatusTooManyRequests { | ||
| // Return transport errors and responses with non-retryable status codes immediately. | ||
| if err != nil || (resp.StatusCode != http.StatusTooManyRequests && !slices.Contains(t.AdditionalRetryStatusCodes, resp.StatusCode)) { | ||
| return resp, err | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,129 @@ func TestDefaultClientFactory_RetryRequests(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestRetryTransport_AdditionalStatusCodes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| statusCode int | ||
| }{ | ||
| { | ||
| name: "request timeout", | ||
| statusCode: http.StatusRequestTimeout, | ||
| }, | ||
| { | ||
| name: "bad gateway", | ||
| statusCode: http.StatusBadGateway, | ||
| }, | ||
| { | ||
| name: "service unavailable", | ||
| statusCode: http.StatusServiceUnavailable, | ||
| }, | ||
| { | ||
| name: "gateway timeout", | ||
| statusCode: http.StatusGatewayTimeout, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| synctest.Test(t, func(t *testing.T) { | ||
| attempts := 0 | ||
| transport := expectedRetryTransport(retryTestRoundTripper(func(*http.Request) (*http.Response, error) { | ||
| attempts++ | ||
| if attempts > 2 { | ||
| t.Fatal("unexpected extra retry") | ||
| } | ||
| status := tt.statusCode | ||
| if attempts == 2 { | ||
| status = http.StatusOK | ||
| } | ||
| return &http.Response{ | ||
| StatusCode: status, | ||
| Body: http.NoBody, | ||
| }, nil | ||
| })) | ||
| transport.AdditionalRetryStatusCodes = []int{tt.statusCode} | ||
| req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://example.com", http.NoBody) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| resp, err := transport.RoundTrip(req) | ||
|
|
||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := resp.Body.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusOK) | ||
| } | ||
| if attempts != 2 { | ||
| t.Errorf("attempts = %d, want 2", attempts) | ||
| } | ||
| }) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestRetryTransport_AdditionalStatusCodes_RetryAfter(t *testing.T) { | ||
|
Contributor
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. Retry-After checked for all status codes. The standard only gives some examples: |
||
| statusCodes := []int{ | ||
| http.StatusRequestTimeout, | ||
| http.StatusBadGateway, | ||
| http.StatusServiceUnavailable, | ||
| http.StatusGatewayTimeout, | ||
| } | ||
|
|
||
| for _, statusCode := range statusCodes { | ||
|
Member
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. I guess it makes sense to have this as a table test which asserts that e.g. HTTP 200 status codes are not retried while the ones currently listed are.
Contributor
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. non-retries of other status codes is covered in Is this ok for you or should I replicate some test cases here? |
||
| t.Run(http.StatusText(statusCode), func(t *testing.T) { | ||
| synctest.Test(t, func(t *testing.T) { | ||
| attempts := 0 | ||
| transport := expectedRetryTransport(retryTestRoundTripper(func(*http.Request) (*http.Response, error) { | ||
| attempts++ | ||
| if attempts > 2 { | ||
| t.Fatal("unexpected extra retry") | ||
| } | ||
| if attempts == 2 { | ||
| return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil | ||
| } | ||
| return &http.Response{ | ||
| StatusCode: statusCode, | ||
| Header: http.Header{"Retry-After": {"30"}}, | ||
| Body: http.NoBody, | ||
| }, nil | ||
| })) | ||
| transport.AdditionalRetryStatusCodes = []int{statusCode} | ||
| transport.MaxJitter = time.Nanosecond // use exact timing | ||
| req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://example.com", http.NoBody) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| start := time.Now() | ||
|
|
||
| resp, err := transport.RoundTrip(req) | ||
|
|
||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := resp.Body.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusOK) | ||
| } | ||
| if attempts != 2 { | ||
| t.Errorf("attempts = %d, want 2", attempts) | ||
| } | ||
| if elapsed := time.Since(start); elapsed != 30*time.Second { | ||
| t.Errorf("elapsed = %s, want Retry-After delay of 30s", elapsed) | ||
| } | ||
| }) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestRetryTransport_ExponentialBackoffWithJitter(t *testing.T) { | ||
| synctest.Test(t, func(t *testing.T) { | ||
| attempts := 0 | ||
|
|
||
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.
Went for just status code varargs instead of function opts varargs, since this will probably become the default in 2 weeks