feat(tool/http): support Google access tokens from ADC - #3792
feat(tool/http): support Google access tokens from ADC#3792hugosmoreira wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for obtaining and sending Google OAuth 2.0 access tokens from Application Default Credentials (ADC) in HTTP tools, including documentation, configuration updates, and integration tests. The reviewer feedback correctly identifies a critical Go anti-pattern where context.Context is stored inside the adcTokenProvider struct. Because the initialization context is short-lived, this can lead to runtime failures due to context cancellation when the tool is invoked later. The reviewer recommends refactoring the token provider and helper functions to accept the active request context dynamically during invocation, and updating the corresponding tests.
| var googleAccessTokenProvider *adcTokenProvider | ||
| if cfg.SendGoogleAccessToken { | ||
| googleAccessTokenProvider = &adcTokenProvider{ctx: ctx} | ||
| } |
There was a problem hiding this comment.
Storing context.Context in a struct is a Go anti-pattern. Additionally, using the initialization context ctx (which is typically short-lived or cancelled after startup) for lazy credential loading during invocation will cause runtime failures (e.g., context canceled) when the tool is invoked later. Instead, we should pass the request context dynamically during Invoke.
| var googleAccessTokenProvider *adcTokenProvider | |
| if cfg.SendGoogleAccessToken { | |
| googleAccessTokenProvider = &adcTokenProvider{ctx: ctx} | |
| } | |
| var googleAccessTokenProvider *adcTokenProvider | |
| if cfg.SendGoogleAccessToken { | |
| googleAccessTokenProvider = &adcTokenProvider{} | |
| } |
| type adcTokenProvider struct { | ||
| ctx context.Context | ||
| mu sync.Mutex | ||
| tokenSource oauth2.TokenSource | ||
| } | ||
|
|
||
| func (p *adcTokenProvider) getTokenSource() (oauth2.TokenSource, error) { | ||
| if p == nil { | ||
| return nil, fmt.Errorf("google ADC token provider is not initialized") | ||
| } | ||
| if p.ctx == nil { | ||
| return nil, fmt.Errorf("google ADC token provider context is not initialized") | ||
| } | ||
|
|
||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
| if p.tokenSource == nil { | ||
| credentials, err := google.FindDefaultCredentials(p.ctx, sources.CloudPlatformScope) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to initialize Google ADC: %w", err) | ||
| } | ||
| if credentials.TokenSource == nil { | ||
| return nil, fmt.Errorf("google ADC did not provide a token source") | ||
| } | ||
| p.tokenSource = credentials.TokenSource | ||
| } | ||
| return p.tokenSource, nil | ||
| } | ||
|
|
||
| func (p *adcTokenProvider) Token() (*oauth2.Token, error) { | ||
| tokenSource, err := p.getTokenSource() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| token, err := tokenSource.Token() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to get Google access token from ADC: %w", err) | ||
| } | ||
| return token, nil | ||
| } | ||
|
|
||
| func setGoogleAccessToken( | ||
| req *http.Request, | ||
| tokenProvider *adcTokenProvider, | ||
| ) error { | ||
| token, err := tokenProvider.Token() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !token.Valid() { | ||
| return fmt.Errorf("google ADC returned an invalid or expired access token") | ||
| } | ||
| req.Header.Set("Authorization", "Bearer "+token.AccessToken) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Remove the ctx field from adcTokenProvider and update its methods to accept context.Context dynamically. This ensures that the active request context is used for credential discovery and token fetching, avoiding potential context cancellation issues.
type adcTokenProvider struct {
mu sync.Mutex
tokenSource oauth2.TokenSource
}
func (p *adcTokenProvider) getTokenSource(ctx context.Context) (oauth2.TokenSource, error) {
if p == nil {
return nil, fmt.Errorf("google ADC token provider is not initialized")
}
p.mu.Lock()
defer p.mu.Unlock()
if p.tokenSource == nil {
credentials, err := google.FindDefaultCredentials(ctx, sources.CloudPlatformScope)
if err != nil {
return nil, fmt.Errorf("unable to initialize Google ADC: %w", err)
}
if credentials.TokenSource == nil {
return nil, fmt.Errorf("google ADC did not provide a token source")
}
p.tokenSource = credentials.TokenSource
}
return p.tokenSource, nil
}
func (p *adcTokenProvider) Token(ctx context.Context) (*oauth2.Token, error) {
tokenSource, err := p.getTokenSource(ctx)
if err != nil {
return nil, err
}
token, err := tokenSource.Token()
if err != nil {
return nil, fmt.Errorf("unable to get Google access token from ADC: %w", err)
}
return token, nil
}
func setGoogleAccessToken(
ctx context.Context,
req *http.Request,
tokenProvider *adcTokenProvider,
) error {
token, err := tokenProvider.Token(ctx)
if err != nil {
return err
}
if !token.Valid() {
return fmt.Errorf("google ADC returned an invalid or expired access token")
}
req.Header.Set("Authorization", "Bearer "+token.AccessToken)
return nil
}| if t.Cfg.SendGoogleAccessToken { | ||
| if err := setGoogleAccessToken(req, t.googleAccessTokenProvider); err != nil { | ||
| return nil, util.NewClientServerError("error authenticating HTTP request with Google ADC", http.StatusInternalServerError, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Pass the active request context ctx to setGoogleAccessToken during tool invocation.
| if t.Cfg.SendGoogleAccessToken { | |
| if err := setGoogleAccessToken(req, t.googleAccessTokenProvider); err != nil { | |
| return nil, util.NewClientServerError("error authenticating HTTP request with Google ADC", http.StatusInternalServerError, err) | |
| } | |
| } | |
| if t.Cfg.SendGoogleAccessToken { | |
| if err := setGoogleAccessToken(ctx, req, t.googleAccessTokenProvider); err != nil { | |
| return nil, util.NewClientServerError("error authenticating HTTP request with Google ADC", http.StatusInternalServerError, err) | |
| } | |
| } |
| err = setGoogleAccessToken( | ||
| req, | ||
| &adcTokenProvider{ | ||
| ctx: context.Background(), | ||
| tokenSource: oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "adc-token"}), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Update the test to pass context.Background() to setGoogleAccessToken and remove the ctx field from the adcTokenProvider initialization.
| err = setGoogleAccessToken( | |
| req, | |
| &adcTokenProvider{ | |
| ctx: context.Background(), | |
| tokenSource: oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "adc-token"}), | |
| }, | |
| ) | |
| err = setGoogleAccessToken( | |
| context.Background(), | |
| req, | |
| &adcTokenProvider{ | |
| tokenSource: oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "adc-token"}), | |
| }, | |
| ) |
| err = setGoogleAccessToken( | ||
| req, | ||
| &adcTokenProvider{ | ||
| ctx: context.Background(), | ||
| tokenSource: errorTokenSource{err: errors.New("credentials unavailable")}, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Update the test to pass context.Background() to setGoogleAccessToken and remove the ctx field from the adcTokenProvider initialization.
| err = setGoogleAccessToken( | |
| req, | |
| &adcTokenProvider{ | |
| ctx: context.Background(), | |
| tokenSource: errorTokenSource{err: errors.New("credentials unavailable")}, | |
| }, | |
| ) | |
| err = setGoogleAccessToken( | |
| context.Background(), | |
| req, | |
| &adcTokenProvider{ | |
| tokenSource: errorTokenSource{err: errors.New("credentials unavailable")}, | |
| }, | |
| ) |
Description
Adds an opt-in
sendGoogleAccessTokenfield to HTTP tools. When enabled, Toolbox lazily resolves Application Default Credentials with the Google Cloud Platform scope, reuses the resulting OAuth token source, and sends the current token as a BearerAuthorizationheader.The ADC header is applied after source, static, and dynamic headers so stale or caller-supplied authorization values cannot override it. ADC discovery remains lazy, preserving existing startup behavior when credentials are unavailable, and authentication failures are returned as invocation errors. The default remains
false, so existing HTTP tools are unchanged.This also adds:
Validation
go test -race -v ./internal/tools/httpgo test -race -v ./tests/http -run '^TestHttpToolSendsGoogleAccessToken$' -count=1go vet ./internal/tools/httpgolangci-lint run --timeout 10mon the changed HTTP files (0 issues)go mod tidy(no module-file diff)git diff --checkPR Checklist
CONTRIBUTING.mdstatus: help wantedFixes #1030