zeroshade commented on code in PR #1655:
URL: https://github.com/apache/iceberg-go/pull/1655#discussion_r3752124105
##########
catalog/rest/rest.go:
##########
@@ -880,11 +905,12 @@ func setupOAuthManager(r *Catalog, cl *http.Client, opts
*options) (AuthManager,
// Add skip oauth so we don't get in cycles trying to refresh the token
ctx := context.WithValue(context.Background(), skipOAuth, true)
- // If a separate TLS config is provided for the OAuth2 server, create a
- // dedicated HTTP client for token requests instead of reusing the
catalog
- // client. This is needed when the OAuth2 server is a different host
with
- // different TLS requirements.
- oauthClient := cl
+ // Token refresh uses a client with a Timeout so a stalled token
endpoint
+ // cannot block auth forever (oauth2's Token() takes no context). By
default
+ // this reuses the catalog client's transport — preserving its TLS,
proxy and
+ // header behavior — but as a distinct *http.Client so the Timeout
applies to
+ // refresh alone and not to ordinary catalog requests.
+ oauthClient := &http.Client{Transport: cl.Transport, Timeout:
defaultOAuthTimeout}
Review Comment:
Non-blocking: this 60s refresh timeout now applies to every REST catalog
user, not just metrics opt-ins. It's the right fix (oauth2's `Token()` takes no
context), but since it's a behavior change landing inside a metrics PR, please
add a line to the PR description calling it out so it surfaces in release notes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
