A86: xDS-Based HTTP CONNECT #455
Conversation
| as a wrapper around whatever transport socket is actually used. | ||
|
|
||
| Note that the only transport socket implementation that gRPC currently | ||
| supports is `UpstreamTlsContext`, which is used to configure TLS |
There was a problem hiding this comment.
To be clear, this means you can't use this feature with plaintext, right? Http11ProxyUpstreamTransport.transport_socket is a required field, so if you use Http11ProxyUpstreamTransport then you must also be using UpstreamTlsContext (until we add more).
There was a problem hiding this comment.
Good catch! I think we do want this to work with plaintext. I hadn't noticed that Http11ProxyUpstreamTransport.transport_socket is a required field, so I was assuming it could be left empty, just as the Cluster.transport_socket field is.
@tonya11en, is there a reason that Http11ProxyUpstreamTransport.transport_socket is a required field? Given that the top-level Cluster.transport_socket field is optional and defaults to plaintext, why doesn't the one inside of Http11ProxyUpstreamTransport work the same way? Is that something we can just fix in xDS and Envoy?
The alternative is for gRPC to add support for the RawBuffer transport socket, which I believe is used to explicitly configure plaintext in xDS. But I'm honestly not sure why that proto exists in the first place; it seems like somewhat pointless bloat on the wire, given that we default to plaintext anyway if the top-level field is unset.
There was a problem hiding this comment.
It's was required because RawBuffer exists, but I see no reason why it has to be. We could just have it default to the raw buffer transport, make the field optional, and call it a day if that works for you guys? I'm assuming this is okay, since it's not considered stable yet.
There was a problem hiding this comment.
Yeah, that sounds like the best approach to me. Can you please go ahead and make that change in the xDS protos and associated docs? Thanks!
A86-xds-http-connect.md
Outdated
| and at the locality level (in the [`LocalityLbEndpoints.metadata` | ||
| field](https://github.com/envoyproxy/envoy/blob/d6120f3c769e70c988ddcc5c7e9cbc2737b5f63c/api/envoy/config/endpoint/v3/endpoint_components.proto#L165)). | ||
|
|
||
| To support this, we will use the metadata mechanism described for CDS in |
There was a problem hiding this comment.
I don't know what this means. This is different metadata than CDS. So we need to say what the "mechanism" is.
There was a problem hiding this comment.
It's actually not different metadata. In Envoy, there's a single envoy.config.core.v3.Metadata type that is used in a bunch of places -- including both the CDS metadata field described in A83 and the two EDS metadata fields described here. Envoy has a single registry of supported protobuf types for metadata that it uses everywhere, so the set of values it accepts is the same in any field of this type, regardless of which resource type that field is in.
It's true that it wouldn't actually be useful to store the GCP auth audience in EDS metadata, nor would it be useful to store the proxy address in CDS metadata -- but that's because the things that looks for those metadata values is looking for them in a different metadata field, not because Envoy would reject the metadata if it was present in the wrong place.
I've attempted to clarify the wording to indicate that it's the metadata registry mechanism that we're reusing here.
No description provided.