Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions stackit/internal/core/clientutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"math/rand/v2"
"net/http"
"slices"
"strconv"
"time"

Expand Down Expand Up @@ -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 {

Copy link
Copy Markdown
Contributor Author

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

// 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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

}

apiClientConfigOptions := []config.ConfigurationOption{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}

Expand Down
19 changes: 14 additions & 5 deletions stackit/internal/core/clientutils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,10 @@ func TestDefaultClientFactory_defaultConfigOptions(t *testing.T) {
}

type testHelper[T any] struct {
factoryMethod func(*DefaultClientFactory) (T, error)
clientInitFunc func(opts ...config.ConfigurationOption) T
customEndpointSetter func(endpointConfig *CustomEndpointConfig)
factoryMethod func(*DefaultClientFactory) (T, error)
clientInitFunc func(opts ...config.ConfigurationOption) T
customEndpointSetter func(endpointConfig *CustomEndpointConfig)
expectedAdditionalRetryStatusCodes []int
}

func (h *testHelper[T]) run(t *testing.T) {
Expand All @@ -222,6 +223,8 @@ func (h *testHelper[T]) run(t *testing.T) {
var testRoundTripper http.RoundTripper = &http.Transport{
TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS13},
}
retryTransport := expectedRetryTransport(testRoundTripper)
retryTransport.AdditionalRetryStatusCodes = h.expectedAdditionalRetryStatusCodes

type fields struct {
RoundTripper http.RoundTripper
Expand All @@ -248,7 +251,7 @@ func (h *testHelper[T]) run(t *testing.T) {
},
want: h.clientInitFunc(
config.WithUserAgent("stackit-terraform-provider/1.2.3"),
config.WithCustomAuth(expectedRetryTransport(testRoundTripper)),
config.WithCustomAuth(retryTransport),
),
},
{
Expand All @@ -266,7 +269,7 @@ func (h *testHelper[T]) run(t *testing.T) {
want: h.clientInitFunc(
config.WithUserAgent("stackit-terraform-provider/1.2.3"),
config.WithEndpoint(testCustomEndpoint),
config.WithCustomAuth(expectedRetryTransport(testRoundTripper)),
config.WithCustomAuth(retryTransport),
),
},
}
Expand Down Expand Up @@ -838,6 +841,12 @@ func TestDefaultClientFactory_newDnsV1Client(t *testing.T) {
customEndpointSetter: func(cfg *CustomEndpointConfig) {
cfg.DnsCustomEndpoint = testCustomEndpoint
},
expectedAdditionalRetryStatusCodes: []int{
http.StatusRequestTimeout,
http.StatusBadGateway,
http.StatusServiceUnavailable,
http.StatusGatewayTimeout,
},
}

test.run(t)
Expand Down
123 changes: 123 additions & 0 deletions stackit/internal/core/retrytransport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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:
https://httpwg.org/specs/rfc9110.html#field.retry-after

statusCodes := []int{
http.StatusRequestTimeout,
http.StatusBadGateway,
http.StatusServiceUnavailable,
http.StatusGatewayTimeout,
}

for _, statusCode := range statusCodes {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-retries of other status codes is covered in TestDefaultClientFactory_RetryRequests. Since this state is rather short lived, I've opted to only test the additional behaviour here in separate tests.
When this retry behaviour gets applied to the default middlewares, we can delete the tests added in this PR and extend the ones for the default middleware stack.

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
Expand Down
Loading