gRFC: host rewrite literal#1
Conversation
3a4651a to
6b78f71
Compare
a13e0d0 to
5962ed6
Compare
| Title | ||
| ---- | ||
| * Author(s): Zhiyan Foo | ||
| * Approver: a11r |
|
|
||
| ## Proposal | ||
|
|
||
| Implement `host_rewrite_literal` feature in gRPC xDS client, enabling explicit authority header |
There was a problem hiding this comment.
A link to the envoy documentation for this field would be useful to readers.
| ## Proposal | ||
|
|
||
| Implement `host_rewrite_literal` feature in gRPC xDS client, enabling explicit authority header | ||
| rewrites. This should be conditioned on `trusted_xds_server`. |
There was a problem hiding this comment.
| rewrites. This should be conditioned on `trusted_xds_server`. | |
| rewrites. This will be conditioned on `trusted_xds_server`. |
I'd also add a link to gRFC A81 which describes this feature.
|
|
||
| Implement `host_rewrite_literal` feature in gRPC xDS client, enabling explicit authority header | ||
| rewrites. This should be conditioned on `trusted_xds_server`. | ||
|
|
There was a problem hiding this comment.
You'll need to explain exactly which component of gRPC xDS are affected by this change: is setting the authority done in the config selector? Where is the authority currently set? Is there a chance that there are bad interops between the other places where we write an authority header? How will this interact with other settings such as auto_host_rewrite? We need to validate the config to make sure both are not set I think.
I would map those paragraphs the same way A81 does:
xDS Resource Validation
xDS ConfigSelector Changes
xds_cluster_impl LB Policy Changes
There was a problem hiding this comment.
This has good guidance on how/why this is important: grpc#414 (comment)
6e14a06 to
10a8d35
Compare
80aeec1 to
300c596
Compare
300c596 to
288af8a
Compare
| ## Abstract | ||
|
|
||
| Implement the | ||
| [`host_rewrite_literal`](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-routeaction-host-rewrite-literal) |
There was a problem hiding this comment.
nit: use a footnote link for nicer plain text formatting, like you did for gRFC links:
[`host_rewrite_literal`][envoy-host_rewrite_literal]
And then at the end of the doc:
[envoy-host_rewrite_literal]: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-routeaction-host-rewrite-literal
| be ignored. | ||
|
|
||
| - xDS ConfigSelector: We will propagate the `host_rewrite_literal` configuration to the | ||
| configSeletor where it would be stored in the rpc context. |
There was a problem hiding this comment.
I don't think RPC context is a cross-language feature, unfortunately. I see two solutions for this:
-
Do the same as
auto_host_rewrite, where it's done in the picker (but in our case as you pointed out there's no need to involve the picker if we have another way to communicate between picker -> transport). -
Do the same as channel credentials (TLS identity) which is passed via "channel args": https://github.com/grpc/grpc/blob/master/src/core/load_balancing/lb_policy.h#L377. Perhaps C-core can use this mechanism here.
-
sounds cleaner to me because it doesn't involve the picker (as you pointed out since the balancer is not involved for
host_rewrite_literal).
So I think a better phrasing would be that of gRFC 29:
gRPC will pass down the hostname to use to the child policies via channel arguments, or a similar mechanism depending on the language.
It looks like Go does the following for TLS credentials (A69): it wraps the clientconn to intercept subchannel creation, and override the credentials for it. The code is at https://github.com/grpc/grpc-go/blob/68205d5d0a21ca0bf2fb849aeaa4f9bfa6292533/xds/internal/balancer/cdsbalancer/cdsbalancer.go#L689-L701. We could technically do the same for overriding the host used for the subconn. I wonder if this mechanism could also be used to support SNI setting via the transport socket configuration, something we've identified as lacking support in grpc?
For SNI I think we should start with a bug report on https://github.com/grpc/grpc/, but this seems quite related, since SNI is presumably taken from the ServerName field of the subconn, the same that you want to override in https://github.com/DataDog/grpc-go-private/pull/19/files#diff-5334018444bcb34d15bcb3c8a3050cdf9c8a40fe41305c3a602ea4bfeb0cd178L744-L749. This would remove the special logic in the stream creation in the transport, which is perhaps a bit cleaner. WDYT?
Co-authored-by: Antoine Tollenaere <atollena@gmail.com>
75c967a to
8c2a7e9
Compare
No description provided.