opentelemetry-collector
opentelemetry-collector copied to clipboard
open-telemetry
Provision to include request context(query params, headers) in AuthenticatorFunc
Is your feature request related to a problem? Please describe.
A facility to access request params such as query string in addition to already existing headers would be useful in the AuthenticatorFunc
Describe alternatives you've considered Pass the queryParams along with headers in the map passed to the above func. Probably not the best approach.
Additional context Authenticators such as apiToken validators would need to pick the token from either the header or query string. In such case, the current implementation wont be sufficient. We need a more generic way of handling this.
@jpkrohling
How about we have a list of properties that the authenticator should propagate down via the context? The config would look like this:
receivers:
myhttpreceiver:
auth:
propagate:
headers:
- x-tenant-id
query:
- access_token
Today, we'd just blindly propagate everything, which is user-friendly but potentially resource-intensive. This change would be resource efficient, but will not be as user-friendly as before. Also, if the payload changes and a new header is sent, the collector's config needs to be changed to make use of that.
Or perhaps we should propagate everything, except when this new propagate node is provided? It would provide the best of two worlds, in my opinion.
While writing an authenticator for our distribution, I actually thought of using something similar to above in the extensions.myauth section. However, I realized that the only thing which the func receives is the header map. So it didnt work out.
Or perhaps we should propagate everything, except when this new propagate node is provided? It would provide the best of two worlds, in my opinion.
This is indeed a good approach. We make propagate as an optional field in the config. This way any existing configuration wont break. Existing authenticators will continue to receive only the headers. If propagate field is present, we must also give the option of specifying all headers or query params.
There are a few things which we should make a note of-
- Do we keep the function signature same?
type AuthenticatorFunc func(ctx context.Context, headers map[string][]string) (context.Context, error) - If so, we need to figure out the possible sources where the auth data could be present. By convention, usually queryParams and headers should suffice. However, if in any uncommon case where the auth data is present in the request body, how should the func signature look like? If we change it, then there would be breaking changes.
- The above points are valid for HTTP receivers only. For gRPC receivers, any change we propose should have minimal impact.
Note that the propagation happens in the confighttp/configgrpc code, not in the authenticator itself.
Do we keep the function signature same?
Yes, the only change is to the contents of the incoming map.
However, if in any uncommon case where the auth data is present in the request body, how should the func signature look like? If we change it, then there would be breaking changes.
Do you have an example of that?
For gRPC receivers, any change we propose should have minimal impact
gRPC would be affected, as the list would restrict what we get from its metadata.
Yes I understood that this would be a part of confighttp/configgrpc
Regarding auth data in the body, only one concrete instance I have is wrt to statsd receiver. It is over UDP and accepts plaintext statsd format metrics data. I know that auth is not a part of that configuration. One possible way of sending auth(tokens) would be via tags and we would need that tag to be parsed which is present as request body itself. I Note: This may not be required as UDP and HTTP(TCP) are completely different protocols, but it is a use case that we want to explore at our organization.
gRPC would be affected, as the list would restrict what we get from its metadata.
But for gRPC auth, we wont have propagate field right?
we would need that tag to be parsed which is present as request body itself
Good point. I can see how this would become a problem down the chain, so, let's consider this in the future if we ever need it. I believe that the proposal from this issue here could be easily changed to add body propagation. We'd need a way to drop that after we used it, but that's also something that can be done in the future.
But for gRPC auth, we wont have propagate field right?
Both HTTP and gRPC share the same auth object:
https://github.com/open-telemetry/opentelemetry-collector/blob/7cdf8c7f1cbcab89721ef8576ff0a5aab2d81f1e/config/confighttp/confighttp.go#L61-L62
https://github.com/open-telemetry/opentelemetry-collector/blob/7cdf8c7f1cbcab89721ef8576ff0a5aab2d81f1e/config/configgrpc/configgrpc.go#L98-L99
https://github.com/open-telemetry/opentelemetry-collector/blob/7cdf8c7f1cbcab89721ef8576ff0a5aab2d81f1e/config/configauth/configauth.go#L31-L35
My idea is to add the Propagate option to this last struct.
Agreed. My contention was that for grpc, propagate will not be used since it has only headers. However, upon thinking I realized, it can do the same logic of picking up only those fields which are mentioned from the metadata of the request.
@jpkrohling the list of "requirements" can be embedded in the code. Something like the "AuthExtension" can have a "Requirements()" API that tells the HTTP/GRPC what infos are required?
Something like the "AuthExtension" can have a "Requirements()" API that tells the HTTP/GRPC what infos are required?
What if the requirement is coming from a processor down the line? Or perhaps even from an exporter (for routing capabilities, like described in #4814)
@jpkrohling maybe can use "Host" as the intermediary, components can "record their needs"? Not sold necessary on the "Host" but you can come up with a solution :)
Hello, friendly reminder :)
Is there any finished plan for this issue?
Not yet. It's in my queue, but I have a few things to complete before going to this one.