xDS HTTP Connect: Supported transport socket name parsing bug

[sergiitk]

There's a bug in the logic that identifies supported Http11ProxyUpstreamTransport transport sockets. Input:

{
  "versionInfo": "1769538225641032355",
  "resources": [{
    "@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
    "name": "cloud-internal-istio-staging:cloud_mp_357736519905_4795705583231421394",
    "type": "EDS",
    // ...
    "transportSocket": {
      "name": "envoy.transport_sockets.http_11_proxy",
      "typedConfig": {
        "@type": "type.googleapis.com/envoy.extensions.transport_sockets.http_11_proxy.v3.Http11ProxyUpstreamTransport"
      }
    },
  }],
  "typeUrl": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
  "nonce": "1"
}

The current logic compares TransportSocket's name (proto) with the TRANSPORT_SOCKET_NAME_HTTP11_PROXY constant:

grpc-java/xds/src/main/java/io/grpc/xds/XdsClusterResource.java

Lines 263 to 267 in 15788b7

	if (hasTransportSocket && !TRANSPORT_SOCKET_NAME_TLS.equals(transportSocket.getName())
	&& !(isEnabledXdsHttpConnect
	&& TRANSPORT_SOCKET_NAME_HTTP11_PROXY.equals(transportSocket.getName()))) {
	return StructOrError.fromError(
	"transport-socket with name " + transportSocket.getName() + " not supported.");

However, the constant itself contains the typed_config (Any) type, not the name:

grpc-java/xds/src/main/java/io/grpc/xds/XdsClusterResource.java

Lines 83 to 85 in 15788b7

	static final String TRANSPORT_SOCKET_NAME_HTTP11_PROXY =
	"type.googleapis.com/envoy.extensions.transport_sockets.http_11_proxy.v3"
	+ ".Http11ProxyUpstreamTransport";

Java expected: type.googleapis.com/envoy.extensions.transport_sockets.http_11_proxy.v3.Http11ProxyUpstreamTransport
Actual xds config: envoy.transport_sockets.http_11_proxy.'

Implementation details

While the simplest approach is to replaces the value of TRANSPORT_SOCKET_NAME_HTTP11_PROXY constant with envoy.transport_sockets.http_11_proxy, we should not be using extension names to identify supported sockets (or any other wrapped typed_configs for this matter).

The proper fix should change the logic to inspecting transportSocket.typed_config.type instead of the name.

Quoting @markdroth:

All extension points in xDS should use the protobuf type inside of the Any field to determine which extension to use, not the name.

The fact that extensions have names is essentially a historical artifact in almost all cases, because Envoy originally used the names instead of the protobuf types to identify the extensions, but they've long since switched to using the protobuf types instead.

The only case I can think of offhand where an extension name is still used for something is HTTP filters. But in that case, it's a filter instance name, not the name of the filter implementation. It's used to match up top-level filter instances with the corresponding override configs in various typed_per_filter_config fields. The name is useful here because it's totally possible to have two instances of the same filter implementation, in which case they'd both have the same protobuf message type but they'd have two different names, and those names could be used in typed_per_filter_config to indicate which of the two a given override config is associated with.

[shivaspeaks]

I see. I conveniently went with how TRANSPORT_SOCKET_NAME_TLS check is performed but it actually stores the name unlike TRANSPORT_SOCKET_NAME_HTTP11_PROXY which stores typeUrl.

grpc-java/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java

Line 46 in 15788b7

protected static final String TRANSPORT_SOCKET_NAME_TLS = "envoy.transport_sockets.tls";

I understand the fix for it but also wanted to state that it will be inconsistent with how TRANSPORT_SOCKET_NAME_TLS is getting handled. Should I alter this one as well to use typeUrl instead of transport socket name?

[markdroth]

Yes, we should make the same change for the TLS transport socket.

All xDS extensions should be chosen based on the type_url, not the name. Anywhere you see us doing something different, it's a bug.