auth fixes - #928
Merged
Merged
Conversation
plorenz
commented
Apr 29, 2026
Member
- Authenticate ListServices call in CtrlClient.GetService
- Make controller HTTP timeout configurable, raise default to 30s. Fixes Make controller http timeout configurable, with a default of 30s #924
- Broaden controller-swap error matching, fix shared errors.As target bug. Fixes Switch controllers on a broader set of errors #925
- Refresh access tokens using a C-SDK style window. Fixes Refresh OIDC token using a window to avoid race conditions and herding #926
- Apply exponential backoff to failed authentication attempts. Fixes Apply exponential backoff to auth retry attempts #927
- Skip re-authentication on 401 within the refresh race window
- Update deps and changelog
|
|
||
| if !context.lastAuthAttempt.IsZero() && context.currentWait > 0 { | ||
| if remaining := context.currentWait - time.Since(context.lastAuthAttempt); remaining > 0 { | ||
| logrus.Debugf("auth backoff: %v remaining, returning cached error", remaining) |
Member
|
Sent some private comments. |
andrewpmartinez
requested changes
May 5, 2026
| // re-auth is required. | ||
| const refreshRaceWindow = 15 * time.Second | ||
|
|
||
| func recentlyAuthenticated(context *ContextImpl) bool { |
Member
There was a problem hiding this comment.
probably should have some godoc here considering how complex this logic is.
Member
Author
There was a problem hiding this comment.
the doc is above, on refreshRaceWindow
Member
Author
|
Review comments addressed |
plorenz
force-pushed
the
auth-fixes
branch
2 times, most recently
from
May 6, 2026 22:44
459307a to
3e05ce6
Compare
andrewpmartinez
approved these changes
May 27, 2026
- passes the current API session as the authorization argument so the request is authenticated, matching the rest of the CtrlClient methods. GetService was previously calling ListServices(params, nil), which produced an unauthenticated request and could surface as an unexpected 401 on a path that should already be authenticated.
…#924 - adds Options.HttpTimeout, plumbed through ApiClientConfig and ComponentsConfig to NewHttpClient. - raises the default HTTP client timeout from 10s to 30s, since OIDC token-exchange round trips on slow controllers were exceeding 10s and surfacing as spurious failures that drove the SDK into re-auth retries. - exposes DefaultHttpTimeout for callers that want the new default.
…ug. Fixes #925 - expands errorIndicatesControllerSwap to recognize HTTP timeouts, context cancellation, EOF / ErrUnexpectedEOF, 5xx responses (via runtime.ClientResponseStatus), and url.Error timeouts so the SDK rotates to a different controller endpoint on transient transport failures instead of hammering an unhealthy one. - moves the errors.As targets (opError, urlError) from package-level vars into local declarations. errors.As mutates the target, so the shared instances were racing across concurrent callers.
- replaces the fixed expiresAt - 10s schedule with tokenRefreshTime, which picks a random point between 1/2 and 5/6 of the token's remaining lifetime. The earlier window leaves headroom for slow refreshes and the jitter avoids thundering-herd token exchanges across SDK clients. - skips the periodic service-list refresh when a token refresh is due in under 15 seconds, so the service refresh doesn't race the expiring token into an unnecessary re-auth on a 401. - tokenRefreshTime returns time.Now() when the remaining lifetime is too small to jitter against (avoids rand.Int63n(0)).
- after a failed Authenticate(), subsequent calls within the backoff window return the cached error without retrying. Prevents callers from hammering an unreachable controller. - backoff doubles per failure with jitter, capped at AuthBackoffMax. Defaults: AuthBackoffInitial=5s, AuthBackoffMax=5m. - separates the un-jittered base (authBackoff) from the per-failure wait (currentWait) so jitter doesn't compound into the doubling sequence. - exposes Options.AuthBackoffInitial / AuthBackoffMax for embedders.
- a 401 on a service-related call within 15s of a successful api-session refresh is almost always a propagation race (server saw the request before processing the new token). Retry with the current session instead of going through Authenticate(), which would needlessly drive the refresh machinery and risk churning sessions and ER connections. - converts lastSuccessfulApiSessionRefresh to concurrenz.AtomicValue so recentlyAuthenticated can read it without taking authAttemptLock; also records the refresh time after successful periodic refreshes in runRefreshes, not just on Authenticate-time refreshes.
plorenz
force-pushed
the
auth-fixes
branch
2 times, most recently
from
May 28, 2026 14:03
753da99 to
e2feb07
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.