Skip to content

Commit eedcd37

Browse files
Add retry logic to token acquisition for auth credentials (#1602)
## Summary Token acquisition for OIDC, M2M OAuth, and Azure client secret credentials now retries on transient failures with Retry-After header support. Retries happen at the token source level (application layer) rather than the HTTP transport level, sidestepping the body-rewind problem that caused #1398. ## Why The SDK's `ApiClient.RoundTrip` retries failed requests at the transport level, violating the `http.RoundTripper` contract. This is what led to #1398: the `oauth2` library wraps request bodies in a non-seekable reader, and when the transport retry tries to rewind the body, it fails with "cannot reset reader". A related problem is #1072: OIDC token endpoint errors like `TEMPORARILY_UNAVAILABLE` were never retried because token acquisition bypasses the SDK's retry middleware entirely. By retrying at the application level, each attempt calls `Token()` on the underlying source, creating a fresh HTTP request from scratch. No body to rewind. ### Retriable status codes | Code | Name | Rationale | |------|------|-----------| | 429 | Too Many Requests | Rate limiting from the IdP. | | 502 | Bad Gateway | Gateway could not reach the upstream token service. Common during rolling deployments. | | 503 | Service Unavailable | Token service temporarily overloaded or down. | | 504 | Gateway Timeout | Gateway timed out waiting for the upstream. Addresses #1102. | Transient network errors are also retried: ECONNRESET, ECONNREFUSED, and timeouts. ### Retry-After For retriable HTTP errors, the `Retry-After` header is used as a hint: the delay is `max(backoff, Retry-After)`, so we never retry sooner than the server asked. No upper cap is applied -- the context timeout already bounds total retry duration. Retry-After is intentionally NOT treated as a retriability signal: a 400 with Retry-After is still not retried. ## What changed ### Interface changes - **`auth.NewRetryingTokenSource(inner, opts...)`** -- wraps any `auth.TokenSource` with retry logic. Exponential backoff, 1-minute timeout by default. - **`api.ExecuteWithResult[T](ctx, fn, opts...)`** -- generic retry executor in `experimental/api/`. - **`(*httpclient.HttpError).HTTPStatusCode()`** -- exposes the status code for error classification without import cycles. - **`(*httpclient.HttpError).Header()`** -- exposes headers for Retry-After parsing without import cycles. ### Behavioral changes - OIDC, M2M OAuth, and Azure client secret token acquisition now retry on 429, 502, 503, 504, and transient network errors. - Retry-After header is respected on retriable responses (#767). ### Internal changes - `httpRetrier` in `config/experimental/auth/retry.go` classifies errors and determines retry delay, including Retry-After parsing. - `httpResponseError` interface unifies status code and header extraction from errors, avoiding import cycles with `httpclient`. - `httpResponse()` extracts metadata from both `oauth2.RetrieveError` and SDK `HttpError` types. - All three credential providers wrap their token sources with `NewRetryingTokenSource`. ## How is this tested? - `TestRetryingTokenSource` -- integration test: retry on 429/503/network errors, no retry on 400/401. - `TestHTTPRetrier_IsRetriable` -- exhaustive retriability classification for all status codes and error types. - `TestHTTPRetrier_RetryAfterDelay` -- Retry-After raises delay above backoff, not capped by backoff maximum. - `TestParseRetryAfter` -- seconds, zero, negative, non-numeric, HTTP-date in the past. - `TestExecuteWithResult` -- timeout, retry, and success paths. ## Issues Fixes #1398 -- OIDC auth retries fail with "cannot reset reader" Fixes #1072 -- OIDC token endpoint TEMPORARILY_UNAVAILABLE not retried Partially addresses #767 -- Retry-After header respected for token acquisition Related: #1102 (504 now retriable) NO_CHANGELOG=true --------- Signed-off-by: Ubuntu <renaud.hartert@databricks.com>
1 parent aa1af2f commit eedcd37

10 files changed

Lines changed: 588 additions & 19 deletions

File tree

NEXT_CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
### Bug Fixes
1616

17+
* Add retry logic to token acquisition for OIDC, M2M, and Azure client secret credentials ([#1398](https://github.com/databricks/databricks-sdk-go/issues/1398), [#1072](https://github.com/databricks/databricks-sdk-go/issues/1072)).
1718
* Fix double-caching of OAuth tokens in Azure client secret credentials ([#1549](https://github.com/databricks/databricks-sdk-go/issues/1549)).
1819
* Disable async token refresh for GCP credential providers to avoid wasted refresh attempts caused by double-caching with Google's internal `oauth2.ReuseTokenSource` ([#1549](https://github.com/databricks/databricks-sdk-go/issues/1549)).
1920
* Fixed double-caching in M2M OAuth that prevented the proactive async token refresh from reaching the HTTP endpoint until ~10s before expiry, causing bursts of 401 errors at token rotation boundaries ([#1549](https://github.com/databricks/databricks-sdk-go/issues/1549)).

config/auth_azure_client_secret.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ func (c AzureClientSecretCredentials) Configure(ctx context.Context, cfg *Config
6464
aadEndpoint := env.AzureActiveDirectoryEndpoint()
6565
managementEndpoint := env.AzureServiceManagementEndpoint()
6666
opts := cacheOptions(cfg)
67-
inner := azureReuseTokenSource(nil, c.tokenSourceFor(ctx, cfg, aadEndpoint, env.AzureApplicationID), opts...)
68-
management := azureReuseTokenSource(nil, c.tokenSourceFor(ctx, cfg, aadEndpoint, managementEndpoint), opts...)
67+
inner := azureReuseTokenSource(nil, auth.NewRetryingTokenSource(c.tokenSourceFor(ctx, cfg, aadEndpoint, env.AzureApplicationID)), opts...)
68+
management := azureReuseTokenSource(nil, auth.NewRetryingTokenSource(c.tokenSourceFor(ctx, cfg, aadEndpoint, managementEndpoint)), opts...)
6969
visitor := azureVisitor(cfg, serviceToServiceVisitor(inner, management, xDatabricksAzureSpManagementToken, false, opts...))
7070
return newVisitorOAuthCredentials(visitor, inner), nil
7171
}

config/auth_m2m.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,29 +27,32 @@ func (c M2mCredentials) Configure(ctx context.Context, cfg *Config) (credentials
2727
return nil, fmt.Errorf("oidc: %w", err)
2828
}
2929
logger.Debugf(ctx, "Generating Databricks OAuth token for Service Principal (%s)", cfg.ClientID)
30+
31+
// IMPORTANT: do not use Config.TokenSource, which returns an
32+
// oauth2.TokenSource already wrapped in a cache. This leads to
33+
// double-caching that defeats the proactive async refresh provided by
34+
// NewCachedTokenSource (see [issue1549] for context).
35+
//
36+
// [issue1549]: https://github.com/databricks/databricks-sdk-go/issues/1549
3037
ccfg := &clientcredentials.Config{
3138
ClientID: cfg.ClientID,
3239
ClientSecret: cfg.ClientSecret,
3340
AuthStyle: oauth2.AuthStyleInHeader,
3441
TokenURL: endpoints.TokenEndpoint,
3542
Scopes: cfg.GetScopes(),
3643
}
37-
38-
// Use a direct (non-caching) token source so that cachedTokenSource is the
39-
// single cache layer. clientcredentials.Config.TokenSource returns an
40-
// oauth2.ReuseTokenSource which adds a second cache with a 10 s expiryDelta.
41-
// With double-caching the async refresh in cachedTokenSource calls through to
42-
// ReuseTokenSource, which returns its own cached token without making an HTTP
43-
// request until only ~10 s remain — defeating the proactive 20-min refresh
44-
// window and causing a burst of 401s at token expiry.
45-
// See https://github.com/databricks/databricks-sdk-go/issues/1549.
46-
//
47-
// ctx is captured from Configure; the oauth2 HTTP client is bound to it via
48-
// InContextForOAuth2 and must be used for all token requests.
49-
directTS := auth.TokenSourceFn(func(_ context.Context) (*oauth2.Token, error) {
44+
ts := auth.TokenSourceFn(func(ctx context.Context) (*oauth2.Token, error) {
45+
// Callers like CredentialsProvider.SetHeaders pass context.Background()
46+
// rather than the actual context from the HTTP client. Because of this,
47+
// the request would bypass the configured transport.
48+
//
49+
// The following is a workaround to ensure the refresh client's
50+
// transport is always used.
51+
ctx = cfg.refreshClient.InContextForOAuth2(ctx)
5052
return ccfg.Token(ctx)
5153
})
54+
5255
return credentials.NewOAuthCredentialsProviderFromTokenSource(
53-
auth.NewCachedTokenSource(directTS, cacheOptions(cfg)...),
56+
auth.NewCachedTokenSource(auth.NewRetryingTokenSource(ts), cacheOptions(cfg)...),
5457
), nil
5558
}

config/auth_oidc.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55

66
"github.com/databricks/databricks-sdk-go/config/credentials"
7+
"github.com/databricks/databricks-sdk-go/config/experimental/auth"
78
"github.com/databricks/databricks-sdk-go/config/experimental/auth/oidc"
89
)
910

@@ -100,7 +101,7 @@ func oidcStrategy(cfg *Config, name string, ts oidc.IDTokenSource) CredentialsSt
100101
}
101102
oidcConfig.SetScopes(cfg.GetScopes())
102103
tokenSource := oidc.NewDatabricksOIDCTokenSource(oidcConfig)
103-
return NewTokenSourceStrategy(name, tokenSource)
104+
return NewTokenSourceStrategy(name, auth.NewRetryingTokenSource(tokenSource))
104105
}
105106

106107
// failedStrategy is a CredentialsStrategy that always fails.

config/experimental/auth/retry.go

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
package auth
2+
3+
import (
4+
"context"
5+
"errors"
6+
"net"
7+
"net/http"
8+
"slices"
9+
"strconv"
10+
"syscall"
11+
"time"
12+
13+
"github.com/databricks/databricks-sdk-go/experimental/api"
14+
"golang.org/x/oauth2"
15+
)
16+
17+
// retryingTokenSource wraps a TokenSource with retry logic for transient
18+
// failures during token acquisition. Each retry calls the underlying Token()
19+
// method, which creates a fresh HTTP request.
20+
type retryingTokenSource struct {
21+
inner TokenSource
22+
opts []api.Option
23+
}
24+
25+
// NewRetryingTokenSource wraps inner with retry logic for transient failures.
26+
// The provided options are applied after the default options, allowing callers
27+
// to override the default timeout and retry behavior.
28+
func NewRetryingTokenSource(inner TokenSource, opts ...api.Option) TokenSource {
29+
defaultOptions := []api.Option{
30+
api.WithTimeout(1 * time.Minute),
31+
api.WithRetrier(func() api.Retrier { return &httpRetrier{} }),
32+
}
33+
return &retryingTokenSource{
34+
inner: inner,
35+
opts: append(defaultOptions, opts...),
36+
}
37+
}
38+
39+
// Token returns a token from the underlying source, retrying on transient
40+
// errors.
41+
func (r *retryingTokenSource) Token(ctx context.Context) (*oauth2.Token, error) {
42+
return api.ExecuteWithResult(ctx, r.inner.Token, r.opts...)
43+
}
44+
45+
// httpRetrier classifies errors from HTTP-based token endpoints and determines
46+
// whether they should be retried and with what delay.
47+
//
48+
// An error is retriable if it carries an HTTP status code in retriableCodes
49+
// (429, 502, 503, 504) or is a transient network error (ECONNRESET,
50+
// ECONNREFUSED, timeout). All other errors, including 4xx client errors, are
51+
// not retriable regardless of headers.
52+
//
53+
// For retriable HTTP errors, the Retry-After header is used as a hint: the
54+
// delay is the maximum of the exponential backoff and the Retry-After value,
55+
// so we never retry sooner than the server asked. No upper cap is applied,
56+
// the context timeout already bounds total retry duration.
57+
//
58+
// TODO: this retrier encodes logic (retriable status codes, Retry-After
59+
// parsing, network error classification) that is also needed by the main HTTP
60+
// retry loop in httpclient/. Consider moving it to a shared location so both
61+
// token acquisition and regular API calls use the same retry classification.
62+
type httpRetrier struct {
63+
backoff api.BackoffPolicy
64+
}
65+
66+
// IsRetriable reports whether err is a transient failure that should be
67+
// retried, and if so, how long the caller should wait before retrying.
68+
func (r *httpRetrier) IsRetriable(err error) (time.Duration, bool) {
69+
if code, header := httpResponse(err); code != 0 {
70+
if !slices.Contains(retriableCodes, code) {
71+
return 0, false
72+
}
73+
delay := r.backoff.Delay()
74+
if d, ok := parseRetryAfter(header); ok && d > delay {
75+
delay = d // context timeout already bounds total retry duration
76+
}
77+
return delay, true
78+
}
79+
if isTransientNetworkError(err) {
80+
return r.backoff.Delay(), true
81+
}
82+
return 0, false
83+
}
84+
85+
// parseRetryAfter parses a Retry-After header value as either a delay in
86+
// seconds or an HTTP-date.
87+
func parseRetryAfter(header http.Header) (time.Duration, bool) {
88+
if header == nil {
89+
return 0, false
90+
}
91+
v := header.Get("Retry-After")
92+
if v == "" {
93+
return 0, false
94+
}
95+
if seconds, parseErr := strconv.Atoi(v); parseErr == nil && seconds >= 0 {
96+
return time.Duration(seconds) * time.Second, true
97+
}
98+
if t, parseErr := http.ParseTime(v); parseErr == nil {
99+
if d := time.Until(t); d > 0 {
100+
return d, true
101+
}
102+
}
103+
return 0, false
104+
}
105+
106+
// httpResponseError is implemented by errors that carry HTTP response
107+
// metadata. This interface avoids importing httpclient (which would create a
108+
// cycle) while still allowing classification of httpclient.HttpError by status
109+
// code and headers.
110+
//
111+
// TODO: This is meant to be temporary and should move this to a shared location
112+
// so both token acquisition and regular API calls use the same classification.
113+
type httpResponseError interface {
114+
HTTPStatusCode() int
115+
Header() http.Header
116+
}
117+
118+
var retriableCodes = []int{
119+
http.StatusTooManyRequests, // 429
120+
http.StatusBadGateway, // 502
121+
http.StatusServiceUnavailable, // 503
122+
http.StatusGatewayTimeout, // 504
123+
}
124+
125+
// httpResponse extracts HTTP response metadata from an error, if available.
126+
func httpResponse(err error) (statusCode int, header http.Header) {
127+
var retrieveErr *oauth2.RetrieveError
128+
if errors.As(err, &retrieveErr) && retrieveErr.Response != nil {
129+
return retrieveErr.Response.StatusCode, retrieveErr.Response.Header
130+
}
131+
var re httpResponseError
132+
if errors.As(err, &re) {
133+
return re.HTTPStatusCode(), re.Header()
134+
}
135+
return 0, nil
136+
}
137+
138+
// isTransientNetworkError reports whether err represents a transient network
139+
// condition that is likely to resolve on retry.
140+
func isTransientNetworkError(err error) bool {
141+
if errors.Is(err, syscall.ECONNRESET) {
142+
return true
143+
}
144+
if errors.Is(err, syscall.ECONNREFUSED) {
145+
return true
146+
}
147+
var netErr net.Error
148+
if errors.As(err, &netErr) && netErr.Timeout() {
149+
return true
150+
}
151+
return false
152+
}

0 commit comments

Comments
 (0)