Skip to content

add client headers provider func #6362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kevingentile
Copy link

if cfg.headersProvider != nil {
headers := cfg.headersProvider.Value()
for k, v := range headers {
req.Header.Set(k, v)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we concerned with Key collisions? Should this provider have the highest priority over header control?

Copy link
Author

@kevingentile kevingentile Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted to give the HeadersProvider priority over static Headers. This approach removes an edge case in newRequest.

@kevingentile kevingentile force-pushed the headers-provider-4536 branch 3 times, most recently from aaaa075 to 147ba21 Compare February 25, 2025 04:40
@kevingentile kevingentile marked this pull request as ready for review February 28, 2025 23:00
@kevingentile kevingentile force-pushed the headers-provider-4536 branch 4 times, most recently from 7d332ad to 0ecbe60 Compare March 2, 2025 18:53
@kevingentile kevingentile force-pushed the headers-provider-4536 branch from 0ecbe60 to f14defc Compare March 2, 2025 19:10
@kevingentile kevingentile force-pushed the headers-provider-4536 branch from d95fc5b to c40e5a3 Compare March 2, 2025 22:44
@dmathieu
Copy link
Member

dmathieu commented Mar 3, 2025

Please see #5129 for the discussion about this.
I'm very dubious about introducing a new API method when the specification group may be looking into this, and provide another solution (which means then having to deprecate things).

Also, side note: this PR only does logs, it ignores traces and metrics.

type HeadersProviderFunc func() (map[string]string, error)

// WithHeadersProvider will be called to set the provided headers with each HTTP requests.
func WithHeadersProvider(providerFunc HeadersProviderFunc) Option {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not allow to e.g. to refresh token when getting a 401 error.

Copy link
Author

@kevingentile kevingentile Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HeadersProvider is executed with every call to newRequest. This should enable implementers to configure a HeadersProviderFunc that can hook into required token lifecycle components for each request. See oauth2/clientcredentials.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the access token is revoked? Do not we want to give the m the possibility to get a new one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s important to point out a constraint in the scope of this effort. These changes are motivated by the need to provide dynamically generated headers with requests, namely an Authorization header whose value may change over time as access tokens are renewed. The implementer is responsible for any scaffolding required to produce the desired headers (or error).
Here is a reference on the client credentials flow that you or others may find useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s important to point out a constraint in the scope of this effort.

What do you mean? Not gracefully supporting one of the possible oauth scenarios/flows?

I am not sure if it would not be more flexible and easier if we simply add an WithHTTPClient or WithHTTPTransport option (similarly to https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc#WithGRPCConn). Then you would be able to use https://pkg.go.dev/golang.org/x/oauth2/clientcredentials#Config.Client. WDYT? I think there was some prior art for it... Probably worth digging into some older issues and PRs

Copy link
Member

@pellared pellared Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

@kevingentile kevingentile Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal here is to provide a way to set dynamic headers, not implement graceful OAuth2 support. Implementers can bring their own. For example:

package main

import (
	"context"
	"fmt"
	"net/url"
	"os"

	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp"
	"go.opentelemetry.io/otel/sdk/metric"
	"golang.org/x/oauth2/clientcredentials"
)

// NOTE: replace in go.mod:
// replace go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp => github.com/kevingentile/opentelemetry-go/exporters/otlp/otlpmetric/otlpmetrichttp v0.0.0-20250302224340-c40e5a3c2e22


func main() {
	ctx := context.Background()

	conf := clientcredentials.Config{
		ClientID:       os.Getenv("CLIENT_ID"),
		ClientSecret:   os.Getenv("CLIENT_SECRET"),
		TokenURL:       os.Getenv("TOKEN_URL"),
		EndpointParams: url.Values{"audience": {os.Getenv("AUDIENCE")}},
	}

	exp, err := otlpmetrichttp.New(ctx, otlpmetrichttp.WithHeadersProvider(func() (map[string]string, error) {
		token, err := conf.Token(ctx)
		if err != nil {
			return nil, fmt.Errorf("failed to get token: %w", err)
		}

		return map[string]string{
			"Authorization":       "Bearer " + token.AccessToken,
			"Some-Dynamic-Header": fmt.Sprint(os.Getpid()),
		}, nil
	}))
	if err != nil {
		panic(err)
	}

	meterProvider := metric.NewMeterProvider(metric.WithReader(metric.NewPeriodicReader(exp)))
	defer func() {
		if err := meterProvider.Shutdown(ctx); err != nil {
			panic(err)
		}
	}()
	otel.SetMeterProvider(meterProvider)
}

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal here is to provide a way to set dynamic headers, not implement graceful OAuth2 support.

This PR does not seem to be a proper way to resolve the mentioned issue.

I will try to look into other ways of fixing this issue (see: https://github.com/open-telemetry/opentelemetry-go/pull/6362/files#r1978191352).

@kevingentile, thanks a lot for taking the time to create the PR and discuss it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants