gRPC API: Add support to control the internal request headers #34179
Conversation
Signed-off-by: Tianyu Xia <tyxia@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Tianyu Xia <tyxia@google.com>
| google.protobuf.BoolValue add_internal_header = 5; | ||
|
|
||
| // [#not-implemented-hide:] | ||
| // If true, x-forwarded-for header will be added to Envoy-gRPC request header. |
There was a problem hiding this comment.
I realize that we're preserving backwards compatible behavior here by making the default true, but I think XFF for async client makes no sense. Async requests are sidecalls, we're not forwarding to the service on behalf of some client, we're making an RPC to some service mid-way through request processing. For gRPC in particular, I think it's a confusion to include XFF by default in the "gRPC metadata" - @markdroth.
There was a problem hiding this comment.
Make sense. xff is to provide a client IP address to the server which is not applicable to sidecar case.
We probably should use runtime guard approach to make xff removal default. I removed this config from this PR.
PTAL, Thanks
There was a problem hiding this comment.
I think this makes sense based on my understanding of the original intent from https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers.html#x-envoy-internal, although, again it's a bit weird to be attaching something like x-envoy-internal to gRPC - the destination service should know that any call to it is coming from a set of internal trusted clients.
@mattklein123 for a second opinion on this.
There was a problem hiding this comment.
Friendly ping @mattklein123 's opinion.
I think the fundamental question here is :
For envoy-gRPC : Whether (1) we want to remove x-enovy-internal by default. OR (2) we want to keep x-envoy-internal but enable gRPC client's capability of removing it.
From strictly backwards-compatibility perspective, (2) is better. But if we are in agreement with end-state of removing this header from envoy-gRPC, (1) is good and its impact can be mitigated by runtime guard.
Signed-off-by: Tianyu Xia <tyxia@google.com>
|
/wait (for CI to be made green) |
This is the follow-up of #34125.
#34125 provides the way to remove
x-envoy-internalandx-forwarded-forin http asyncClient path.This API PR provides the config options for envoy-gRPC (which is on top of http asyncClient) to remove
x-envoy-internalandx-forwarded-for.By default, those two config options are TRUE, which keeps the existing behavior. And next in the impl, envoy-gRPC client can config these two options(setting them to FALSE) to not add those two headers.