Skip to content
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

[configgrpc] Handle gRPC metadata generically #12307

Closed
jade-guiton-dd opened this issue Feb 6, 2025 · 0 comments · Fixed by #12320
Closed

[configgrpc] Handle gRPC metadata generically #12307

jade-guiton-dd opened this issue Feb 6, 2025 · 0 comments · Fixed by #12320
Assignees
Labels
area:config enhancement New feature or request

Comments

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Feb 6, 2025

Problem:

configgrpc and confighttp both allow setting a headers section to set headers on client requests. However, while confighttp handles this internally, configgrpc does not, as there doesn't seem to be a way to set headers on the grpc.ClientConn struct it returns. The OTLP/gRPC exporter fills the gap by extracting the headers config, wrapping it in a metadata.MD, then later adding it to the Context passed to the gRPC call, which is then taken into account by the gRPC library.

This is not ideal, as it means the configgrpc.ClientConfig.Headers is a no-op by default, and components using a gRPC client must manually do this abstraction-leaking processing to take it into account.

Proposed solution:

I considered wrapping grpc.ClientConn to automatically add the metadata in Invoke calls, but because the code calling this method is generated by protogen and expects a grpc.ClientConn specifically, this does not seem possible.

My proposed solution is a new method on ClientConfig which adds the headers to a provided Context, and documenting that the Headers field requires calling this function and passing the resulting Context into the gRPC call.

Update: We can actually do it pretty simply with grpc.WithUnaryInterceptor.

@jade-guiton-dd jade-guiton-dd added area:config enhancement New feature or request labels Feb 6, 2025
@jade-guiton-dd jade-guiton-dd self-assigned this Feb 6, 2025
github-merge-queue bot pushed a commit that referenced this issue Feb 10, 2025
#### Description

This PR uses the `grpc.WithUnaryInterceptor` option to add the metadata
configured in `ClientConfig.Headers` automatically to requests made with
the created `grpc.ClientConn`. Currently, users of `configgrpc` must
extract the headers from the config and insert them manually in each
request.

The current implementation is meant to be backward-compatible with code
that manually inserts the headers: if a header key is already present in
the `Context` passed to the interceptor, it will not be modified and no
duplicate will be added.

#### Link to tracking issue
Fixes #12307

#### Testing
The tests in `otlpexporter` check that headers are properly inserted,
and they still pass after removing the OTLP-specific headers code. I
also added a new test in configgrpc for testing unary RPC calls.

I'm not sure how to easily test streaming RPC calls however (hence the
lacking code coverage), but since [the
docs](https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md)
imply that adding metadata to streaming RPC calls is the same as unary
calls, it should hopefully work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config enhancement New feature or request
Projects
None yet
1 participant