-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
gRPC API: Add support to control the internal request headers #34179
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-internal
andx-forwarded-for
in http asyncClient path.This API PR provides the config options for envoy-gRPC (which is on top of http asyncClient) to remove
x-envoy-internal
andx-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.