-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
test(configgrpc|confighttp): sample test showcasing issue 10093 #10162
Conversation
@jpkrohling same test showcasing issue #10093 |
config/configgrpc/configgrpc_test.go
Outdated
ext: map[component.ID]component.Component{ | ||
mockID: auth.NewServer(auth.WithServerAuthenticate(func(ctx context.Context, headers map[string][]string) (context.Context, error) { | ||
return client.NewContext(ctx, client.Info{ | ||
Metadata: client.NewMetadata(metadata.Pairs("some-key-set-in-auth", "some-value-set-in-auth")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpkrohling does the above make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
config/configgrpc/configgrpc_test.go
Outdated
host := &mockHost{ | ||
ext: map[component.ID]component.Component{ | ||
mockID: auth.NewServer(auth.WithServerAuthenticate(func(ctx context.Context, headers map[string][]string) (context.Context, error) { | ||
return client.NewContext(ctx, client.Info{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
})),
defer cancelFunc() | ||
|
||
// test | ||
tC.tester(ctx, cl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
t.Run(tC.desc, func(t *testing.T) { | ||
host := &mockHost{ | ||
ext: map[component.ID]component.Component{ | ||
mockID: auth.NewServer(auth.WithServerAuthenticate(func(ctx context.Context, headers map[string][]string) (context.Context, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as gRPC.
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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: 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. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@grandwizard28 , do you think this should be closed? |
Description
This is a sample test showcasing issue #10093