CXP-1085 uhttp: classify OAuth2 token-exchange failures - #1132
CXP-1085 uhttp: classify OAuth2 token-exchange failures#1132agustin-conductor wants to merge 2 commits into
Conversation
x/oauth2 token failures reached the syncer as codes.Unknown, which retry.Retryer treats as terminal (pkg/retry/retry.go:61 waits only on Unavailable/DeadlineExceeded). A token endpoint that answered 500 or timed out therefore aborted the sync action, and IsSyncPreservable discarded the artifact, so a sub-second blip cost hours of completed grant walking. The classification existed but was unreachable. wrapTransientNetworkError's *oauth2.RetrieveError branch only ran from Transport.RoundTrip and wrapper.Do, and a non-2xx from a token endpoint is a successful RoundTrip: x/oauth2 builds the RetrieveError above the transport. This adds ClassifyOAuth2TokenError and NewClassifyingTokenSource in pkg/uhttp/oauth2.go, wires the three OAuth helpers in authcredentials.go through the token source wrapper, and moves the RetrieveError helpers out of errors.go so the transport path and the token-source path share one implementation. Two x/oauth2 behaviors had to be designed around, both verified against the pinned v0.36.0. jwt.Config's token source builds RetrieveError from the response alone (jwt/jwt.go:143), leaving ErrorCode and ErrorDescription empty, so oauth2ErrorParamsFromBody recovers the RFC 6749 params from the body, JSON or form-encoded. RFC 6749 5.2 defines "error" as a string; a body that puts something else there (Google's token endpoint nests a code/message/status object) carries no param to act on, so the code falls back to the HTTP status and the bounded body becomes the description. Nothing keys on a vendor's field names. jwt/jwt.go:135, :140 and :154 wrap transport failures with %v, not %w, so the status the uhttp transport attached is destroyed before any caller sees it and errors.As cannot recover it. No dependency bump fixes that. classifyFlattenedOAuth2TokenError therefore reads message text, gated on the "oauth2: cannot fetch token" prefix x/oauth2 puts on exactly these failures. It first recovers a grpc status out of the flattened text, since that code was decided from the typed error, and falls back to a substring table whose codes must agree with wrapTransientNetworkError's; the same failure takes either path depending only on whether the caller installed this package's transport under oauth2.HTTPClient. pkg/lambda/grpc/util.go:178 already classifies serialized errors this way for the same reason. Definitive rejections stay terminal: invalid_client/invalid_grant are Unauthenticated, unauthorized_client/access_denied are PermissionDenied, and pkg/retry's retryable set is untouched, per the CXE-1113 anti-goal that a disabled credential must fail fast rather than spin in a retryer with no attempt limit. server_error and temporarily_unavailable now map to Unavailable: RFC 6749 4.1.2.1 defines both as transient, and a 400 carrying one previously landed on InvalidArgument and stayed terminal. Two consequences for existing connectors. Nothing breaks at compile time or in error matching, because the status is attached by errors.Join rather than substitution: errors.As still recovers *oauth2.RetrieveError, and err.Error() still carries x/oauth2's own text and the response body. But IsSyncPreservable (pkg/sync/syncer.go:91) now returns true for these errors where it returned false, since an unclassified error fails its status.FromError check while Unavailable, Unauthenticated and PermissionDenied are all in its preserve list. That is the intended direction, and it is a retention change; the InvalidArgument cases still discard. Second, retry.Retryer's default has no attempt cap, so a permanently failing token endpoint now backs off instead of failing fast, bounded by the caller's RetryConfig and run duration. Tests cover the cross product of source shape (typed RetrieveError, %v-flattened text, raw transport error), param location (on the error, in a JSON body, in a form body, absent, non-string) and disposition (transient vs. terminal), each asserted against retry.Retryer rather than the code alone. Three tests drive a real jwt.Config against httptest to prove the flattening and empty-param premises hold in v0.36.0, so a future bump that fixes x/oauth2 fails the test rather than leaving dead code behind, and one drives OAuth2JWT.GetClient end to end to pin what connectors can still match on. Every branch in oauth2.go is covered. Ten planted mutants were each caught: dropped body recovery, dropped flattened fallback, dropped grpc status recovery, server_error made terminal, dropped truncation, dropped existing-status guard, non-string error member used as a code, NXDOMAIN made retryable, dropped body snippet, and lost error_description precedence. GetMetadata still has no retryer around it; correct classification cannot save that phase, and CXP-1085 files it as a follow-up. Deleting baton-google-cloud-platform's interim classifyingTokenSource waits on an SDK release; the swap is uhttp.NewClassifyingTokenSource(cfg.TokenSource(ctx)). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var flattenedTokenFailures = []struct { | ||
| substr string | ||
| code codes.Code | ||
| msg string | ||
| }{ | ||
| {substr: "no such host", code: codes.InvalidArgument, msg: "dns lookup failed: NXDOMAIN"}, | ||
| {substr: "server misbehaving", code: codes.Unavailable, msg: "temporary dns lookup failure"}, | ||
| {substr: "context deadline exceeded", code: codes.DeadlineExceeded, msg: "request timeout"}, | ||
| {substr: "Client.Timeout exceeded", code: codes.DeadlineExceeded, msg: "request timeout"}, | ||
| {substr: "TLS handshake timeout", code: codes.DeadlineExceeded, msg: "request timeout"}, | ||
| {substr: "i/o timeout", code: codes.DeadlineExceeded, msg: "request timeout"}, | ||
| {substr: "connection reset by peer", code: codes.Unavailable, msg: "connection reset"}, | ||
| {substr: "connection refused", code: codes.Unavailable, msg: "connection refused"}, | ||
| {substr: "broken pipe", code: codes.Unavailable, msg: "broken pipe"}, | ||
| {substr: "network is unreachable", code: codes.Unavailable, msg: "network unreachable"}, | ||
| {substr: "no route to host", code: codes.Unavailable, msg: "network unreachable"}, | ||
| {substr: "http2: client connection lost", code: codes.Unavailable, msg: "http2 client connection lost"}, | ||
| {substr: "EOF", code: codes.Unavailable, msg: "connection closed before response"}, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the table omits conditions wrapTransientNetworkError classifies as retryable by identity, so the two classifiers disagree where the header comment says they must agree. Missing: syscall.ETIMEDOUT → "connection timed out" (typed path reaches DeadlineExceeded via netErr.Timeout()), syscall.ECONNABORTED → "software caused connection abort" (isConnectionReset), syscall.ENETDOWN → "network is down" (isNetworkUnreachable), and every Winsock spelling in errors_windows.go. A jwt-path token fetch whose TCP connect times out flattens to oauth2: cannot fetch token: Post "…": dial tcp …: connect: connection timed out; nothing matches, classifyFlattenedOAuth2TokenError returns nil, and wrapTransientNetworkError on text-only evidence finds nothing either — so it stays codes.Unknown and the sync is discarded, the exact failure this PR fixes.
TestFlattenedAndTypedClassificationAgree enumerates only 7 conditions, so the invariant isn't enforced. Worth driving both the table and that test from one shared list of every condition the errors_unix.go/errors_windows.go predicates match. Note also that internal/token.go:266 flattens with %v too, so this path covers the clientcredentials/refresh flows, not just jwt.
| func oauth2TokenErrorFrom(retrieveErr *oauth2.RetrieveError) oauth2TokenError { | ||
| tokenErr := oauth2TokenError{ | ||
| resp: retrieveErr.Response, | ||
| code: retrieveErr.ErrorCode, | ||
| description: retrieveErr.ErrorDescription, | ||
| } | ||
| if tokenErr.code != "" { | ||
| return tokenErr | ||
| } | ||
|
|
||
| code, description := oauth2ErrorParamsFromBody(retrieveErr.Response, retrieveErr.Body) | ||
| tokenErr.code = code | ||
| if tokenErr.description == "" { | ||
| tokenErr.description = description | ||
| } | ||
| return tokenErr | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the maxRecoveredDescription bound only covers values recovered from the body — retrieveErr.ErrorDescription and retrieveErr.ErrorCode pass through untruncated, and so does an error string recovered from the body (oauth2ErrorParamsFromJSONBody truncates only the description). On the clientcredentials/oauth2.Config paths x/oauth2 populates ErrorDescription from up to 1 MiB of body (internal/token.go), which is the case the bound exists for, and it lands whole in message() → the grpc status message → logs and grpc trailers. Truncating all four sources in oauth2TokenErrorFrom (or inside message()) would close it.
| // Config.Client is Config.TokenSource plus oauth2.NewClient, spelled out | ||
| // here so the refresh goes through NewClassifyingTokenSource. | ||
| ts := NewClassifyingTokenSource(o.cfg.TokenSource(ctx, token)) | ||
| httpClient = oauth2.NewClient(ctx, ts) |
There was a problem hiding this comment.
🟡 Suggestion: this rewrite is behavior-preserving but untested on its compatibility-sensitive half. Because the wrapper is not a *oauth2.reuseTokenSource, oauth2.NewClient's ReuseTokenSource(nil, src) no longer short-circuits (vendor/golang.org/x/oauth2/oauth2.go:385) and adds a second reuse layer around the seeded token. The seeded token still wins — the inner reuseTokenSource holds it and Token.Valid() is true for a non-empty AccessToken with a zero Expiry — but TestOAuth2RefreshToken_GetClientClassifiesRefreshFailure passes accessToken: "", so nothing pins that a caller-supplied access token is still used without hitting the token endpoint. A case with a non-empty accessToken asserting the token server is never called would cover it.
General PR Review: CXP-1085 uhttp: classify OAuth2 token-exchange failuresBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit derives the flattened text classifier from Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
…ion list Review of #1132 found two gaps in the new classifier. The text-only table in oauth2.go was hand-written, so it missed conditions wrapTransientNetworkError classifies from error identity: a flattened ETIMEDOUT ("connection timed out"), ECONNABORTED ("software caused connection abort") and ENETDOWN ("network is down") each stayed codes.Unknown while the typed path returned DeadlineExceeded or Unavailable, as did every Winsock spelling in errors_windows.go. A jwt-path token fetch whose TCP connect timed out was therefore still discarded, which is the failure the PR exists to fix. The header comment claimed the two classifiers agree and TestFlattenedAndTypedClassificationAgree only checked seven pairs chosen by hand, so nothing enforced it. Both classifiers now come off one list. errors.go declares socketClass, socketCondition and socketClassifications; errors_other.go and errors_windows.go each list the errnos that platform spells a class with, and the four predicates are now wrappers over hasSocketClass rather than parallel errors.Is chains. flattenedTokenFailures builds its socket entries from that list with each errno's own Error() string as the text, so the Winsock spellings are right on Windows without being transcribed by hand, and the table cannot drift from the predicates. The agreement test walks the list instead of a hand-picked sample and also requires every declared class to have a spelling on the platform. Second, maxRecoveredDescription bounded only params parsed out of a response body. x/oauth2 populates RetrieveError.ErrorDescription itself from up to 1 MiB of body on the clientcredentials and oauth2.Config paths, and that landed whole in the grpc status message on both the terminal and the transient path: a 5,000 character description produced a 5,015 byte message. oauth2TokenErrorFrom now bounds both params once, after resolving them from whichever source, and the constant and helper are renamed to maxErrorParamLength and truncateErrorParam since they no longer cover only descriptions. One hole surfaced while validating: with the list as the single source of truth, deleting an entry removes the condition from both classifiers and no agreement test can notice - dropping ECONNABORTED failed nothing. The new errors_other_test.go is the POSIX floor, asserting each spelling is still on the list and still classifies, mirroring the enumeration TestWrapTransientNetworkError_Winsock already provided for Winsock. After it, dropping ECONNABORTED, ENETDOWN or EPIPE fails 3, 5 and 8 tests. Also adds TestFlattenedTokenFailures_OrderIsUnambiguous, since the table is first-match-wins and an entry whose text contains an earlier entry's text with a different code is unreachable, and a dedicated flattened-ETIMEDOUT test: it is the one condition whose typed classification does not come off the list, arriving through net.Error.Timeout() before any predicate runs, so it is the one that could silently drift again. Six planted mutants were each caught: socket entries dropped from the table, ETIMEDOUT removed from the platform list, the timeout class mislabeled Unavailable, truncation of error-borne params removed, the EOF entry moved ahead of the socket entries, and each errno deletion above. GOOS=windows go vet compiles the Windows path; oauth2.go stays fully covered and errors.go's four uncovered blocks are unchanged from before this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| func classifyFlattenedOAuth2TokenError(err error) error { | ||
| msg := err.Error() | ||
| if !strings.Contains(msg, oauth2FetchTokenPrefix) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: msg here can include a server-controlled body. *oauth2.RetrieveError.Error() formats as "oauth2: cannot fetch token: %v\nResponse: %s" (vendor/golang.org/x/oauth2/token.go:212), so any caller that %v-flattens a RetrieveError before calling the exported ClassifyOAuth2TokenError — the case this function exists for — hands both grpcStatusInText and the substring table up to 1 MiB of token-endpoint output. A 400 invalid_grant body containing EOF (three characters), no route to host, or the literal rpc error: code = Unavailable desc = x then classifies as Unavailable: retryable in a retryer with no attempt limit at its defaults, and preservable in IsSyncPreservable, which is the anti-goal the PR body calls out. Cutting the scanned text at the \nResponse: boundary before matching keeps the x/oauth2-authored prefix and drops the body:
msg := err.Error()
if !strings.Contains(msg, oauth2FetchTokenPrefix) {
return nil
}
// RetrieveError.Error() appends the raw response body after this
// marker; only the text x/oauth2 wrote is evidence.
if idx := strings.Index(msg, "\nResponse:"); idx >= 0 {
msg = msg[:idx]
}3 participants
Classifies
golang.org/x/oauth2token-exchange failures soretry.Retryercanact on them: a token endpoint that answered 500 or timed out is now
Unavailable/DeadlineExceededand gets backed off through, while a rejectedcredential stays
Unauthenticated/PermissionDeniedand still fails fast.Fixes CXP-1085, which surfaced
under CXP-890.
Why
Token failures reached the syncer as
codes.Unknown, whichretry.Retryertreats as terminal —
pkg/retry/retry.go:61waits only onUnavailableandDeadlineExceeded. The sync action aborted andIsSyncPreservablediscarded theartifact, so a sub-second blip at
oauth2.googleapis.com/tokencost hours ofcompleted grant walking. GCP re-exchanges a service-account JWT roughly hourly,
so longer syncs failed more often.
The classification already existed and was unreachable.
wrapTransientNetworkError's*oauth2.RetrieveErrorbranch (added in #1106) only ran fromTransport.RoundTripand
wrapper.Do— and a non-2xx from a token endpoint is a successfulRoundTrip, so x/oauth2 builds the
RetrieveErrorabove the transport, where thatbranch never sees it. This is not GCP-specific: every helper in
pkg/uhttp/authcredentials.gohad the same gap.What changed
pkg/uhttp/oauth2.go(new) exportsClassifyOAuth2TokenErrorandNewClassifyingTokenSource, for connectors that build their ownjwt.Configrather than going through the SDK helpers (baton-google-cloud-platform does).
OAuth2ClientCredentials,OAuth2JWTandOAuth2RefreshTokennow build theirtoken source through the wrapper.
OAuth2RefreshTokenspells outcfg.TokenSource+oauth2.NewClient, which is whatcfg.Clientdoes.errors.go'sRetrieveErrorbranch delegates to the same function, so thetransport path and the token-source path share one implementation.
Two x/oauth2 behaviors had to be worked around, both pinned by tests that drive a
real
jwt.Configagainsthttptest— if a future bump fixes either, the testfails rather than leaving dead code:
jwt.Config's token source buildsRetrieveErrorfrom the response alone (jwt/jwt.go:143), soerroranderror_descriptionare recovered from the body, JSON or form-encoded. RFC 6749§5.2 defines
erroras a string; a body that puts something else there(Google's endpoint nests a
code/message/statusobject) carries no param toact on, so the code falls back to the HTTP status and the bounded body becomes
the description. Nothing keys on a vendor's field names.
%vflattening.jwt/jwt.go:135,:140and:154wrap transport failureswith
%v, not%w, destroying the status the uhttp transport attached beforeany caller sees it;
errors.Ascannot recover it and no dependency bump fixesit.
classifyFlattenedOAuth2TokenErrortherefore reads message text, gated onthe
oauth2: cannot fetch tokenprefix. It first recovers a grpc status out ofthe flattened text — that code was decided from the typed error — then falls
back to a substring table whose codes must agree with
wrapTransientNetworkError's, asserted case by case, since the same failuretakes either path depending only on whether the caller installed this package's
transport under
oauth2.HTTPClient.pkg/lambda/grpc/util.go:178alreadyclassifies serialized errors this way for the same reason.
pkg/retry's retryable set is untouched, per the CXE-1113 anti-goal: that retryerhas no attempt limit at its defaults, so a disabled credential must fail fast
rather than spin for hours.
Effect on existing connectors
Nothing breaks at compile time — the exported surface only grew — and nothing
breaks in error matching, because the status is attached by
errors.Joinratherthan substitution.
errors.Asstill recovers*oauth2.RetrieveError, anderr.Error()still carries x/oauth2's own text and the response body, soexisting
errors.Is/errors.Aschecks and message matches keep working.TestOAuth2JWT_GetClientPreservesErrorIdentitypins that end to end.Three behavior changes worth a reviewer's attention:
IsSyncPreservable(pkg/sync/syncer.go:91) now returns true for theseerrors where it returned false. An unclassified error fails its
status.FromErrorcheck, whileUnavailable,UnauthenticatedandPermissionDeniedare all in its preserve list — so a partial artifactsurvives a token failure instead of being discarded. That is the direction
CXP-890 wants, and it is a retention change, called out here rather than left
to be discovered. The
InvalidArgumentcases (invalid_scope,invalid_request, NXDOMAIN) still discard, as before.server_errorandtemporarily_unavailableflip from terminal toretryable. RFC 6749 §4.1.2.1 defines both as transient; a 400 carrying one
previously landed on
InvalidArgument.fast, bounded by the caller's
RetryConfigand run duration. Same exposureany upstream 503 already has.
Testing
go test ./pkg/...passes;golangci-lint run pkg/uhttp/...is clean (the sixfindings the repo-wide run reports are pre-existing on
main, in packages thisPR does not touch). Tests cover the cross product of source shape (typed
RetrieveError,%v-flattened text, raw transport error), param location (onthe error, JSON body, form body, absent, non-string) and disposition, each
asserted against
retry.Retryerrather than the grpc code alone. Every branch inoauth2.gois covered. Ten planted mutants were each caught: dropped bodyrecovery, dropped flattened fallback, dropped grpc status recovery,
server_errormade terminal, dropped truncation, dropped existing-status guard, non-string
errormember used as a code, NXDOMAIN made retryable, dropped body snippet, andlost
error_descriptionprecedence.🤖 Generated with Claude Code