test(configgrpc|confighttp): sample test showcasing issue 10093

[grandwizard28]

Description

This is a sample test showcasing issue #10093

[grandwizard28]

@jpkrohling same test showcasing issue #10093

[jpkrohling]

I see what you mean now. Authenticators are supposed to only provide data via the AuthData, not Metadata. Metadata is only supposed to be set by the parts that deal with the connection with the client, such as gRPC and HTTP servers (like those in receivers). Your downstream consumer of this information would have to call AuthData.GetAttribute(...).

opentelemetry-collector/client/client.go

Lines 26 to 30 in 3d0189d

	// Authenticators are responsible for obtaining a client.Info from the current
	// context, enhancing the client.Info with an implementation of client.AuthData,
	// and storing a new client.Info into the context that it passes down. The
	// attribute names should be documented with their return types and considered
	// part of the public API for the authenticator.

Here's an implementation example:

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/5cc9e03d054728b8f97d9214aa58910aa493c502/extension/basicauthextension/extension.go#L105-L107

[grandwizard28]

Yeah, but consider the following:

• This doesnt happen for oltp/http
• If I try putting in the AuthData, the batch processor will drop it. See this: feat(processor): preserve auth in batch processor #10002

I feel the behaviours should be similar for oltp/http and oltp/grpc. Wouldn't you agree?

[jpkrohling]

Can you add a test case here for otlp/http? There should be no difference between the receivers, and if the authenticator is overriding the metadata, none should see the original headers after the authenticator runs.

[grandwizard28]

Table depicting what happens to metadata set in Auth:

Protocol/Config	Include Metadata = True	Include Metadata = False
GRPC	overriden	not overriden
HTTP	not overriden	not overriden

[grandwizard28]

@jpkrohling does the above make sense?

[jpkrohling]

I realize that perhaps the API isn't very intuitive at the moment, and we either need better documentation or to prevent the API from being misused.

I found a couple of inconsistencies with the test, and I'm sending a new commit to this PR to show how we expect the APIs to be used.

[jpkrohling]

The right thing here would be to obtain the client from the context, set only the Auth attributes, and wrap it in the context that is being returned, like this:

mockID: auth.NewServer(auth.WithServerAuthenticate(func(ctx context.Context, headers map[string][]string) (context.Context, error) {
	cl := client.FromContext(ctx)
	cl.Auth = &mockAuthData{
		Attributes: map[string]string{"some-key-set-in-auth": "some-value-set-in-auth"},
	}

	return client.NewContext(ctx, cl), nil
})),

[jpkrohling]

This is counter-intuitive, as we set the headers on the ClientSettings, but the client making use of the construct is responsible for using the headers. So, the headers you are setting at ClientConfig aren't being sent to the actual service. You'll need something like this:

// this is what we expect clients to do before making a gRPC call
headers := map[string]string{}
for k, v := range gcs.Headers {
	headers[k] = string(v)
}
md := metadata.New(headers)
ctx = metadata.NewOutgoingContext(ctx, md)

[jpkrohling]

Same comments as gRPC.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.48%. Comparing base (3d0189d) to head (8bae1d0).
Report is 54 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10162      +/-   ##
==========================================
+ Coverage   91.65%   92.48%   +0.82%     
==========================================
  Files         356      387      +31     
  Lines       16847    18264    +1417     
==========================================
+ Hits        15441    16891    +1450     
+ Misses       1063     1027      -36     
- Partials      343      346       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[grandwizard28]

Right! The intended usage is to set only the AuthData in custom authenticators and not touch metadata at all.

If someone tries to set the metadata in a custom authenticator (as I did), http would allow it to propagate but grpc wont. (With IncludeMetadata: true) Even though this is not the intended usage, the two protocols behave differently in this instance.

My Reason for doing so:
The whole reason I tried to set Metadata in the Authenticator was because the batch processor was dropping the AuthData altogether.

All I want is to be able to propagate Authdata across the collector.

[jpkrohling]

All I want is to be able to propagate Authdata across the collector

You can do that today already, except if you use the batching processor: that one will group data from potentially different connections into one single batch. Those connections came with potentially different auth data.

The proper solution would be to enhance the batch processor, so that it can also group data by auth data in addition to metadata.