xds: xDS-based HTTP CONNECT configuration

[shivaspeaks]

Implements gRFC A86 (grpc/proposal#455).

NOTE: Making this PR go through review cycle before writing unit tests.

[ejona86]

What should the behavior be if this isn't a string? This is received from I/O, so we have to be careful processing it. I doubt it is appropriate to get a ClassCastException. Ditto below.

[ejona86]

AssertionError is much worse than ClassCastException, because you're saying it should never happen and nothing should be catching it. You may need to ask others what the behavior should be here, but I doubt this change was in the right direction.

Note that you should cast to SocketAddress, not InetSocketAddress, as HttpConnectProxiedSocketAddress is fine with SocketAddress and if we add support for Unix domain sockets or such in the future it will be a different type here.

[shivaspeaks]

The grfc (A86-xds-http-connect) says:

The proxy address for a given endpoint will be set by looking for a metadata entry of type envoy.config.core.v3.Address under the key envoy.http11_proxy_transport_socket.proxy_address. If that key is not present, or if the key is present but the value has a different type, the entry will be ignored. The transport socket will look for the key first in the endpoint-level metadata. If not found there, it will look in the locality-level metadata. If not found in either place, then HTTP CONNECT is not used (i.e., the behavior will be exactly the same as if the Http11ProxyUpstreamTransport transport socket wrapper was not present).

So should we not throw error? Just ignore.

[ejona86]

Throwing is very, very bad, as it completely kills the channel ("panic mode"). I expect we should either ignore the address or ignore the metadata. But that needs to be specified in the gRFC.

[shivaspeaks]

I see my mistake. We should not be typecasting here at all and in turn throw the error when incorrect because we have done this step already in our parse method (all kinds of validation is there). Here we will just get it, if it is not present then we will have null in it and will be ignored.

In other words, endpointMetadata can never have a different type for envoy.http11_proxy_transport_socket.proxy_address key. I did in a different way that how it was defined in grfc I believe. According to gRFC, we had to create map of <String, Any> but we have map of <String, Object> and we already do better parsing beforehand.

[ejona86]

Oh, I see it is specified in the part you copied:

If that key is not present, or if the key is present but the value has a different type, the entry will be ignored.

We do need the cast at some point. But we need to check it is the right type before the cast, and ignore it if it isn't the right type.

The parsing does its verification, but there's nothing that requires envoy.http11_proxy_transport_socket.proxy_address is the expected type, except this code.

[shivaspeaks]

I don't understand this.
Yes we need to typecast at some point from Object to Socket Address, but that should be harmless here.

The parse() method is returning SocketAddress

public java.net.SocketAddress parse(Any any) throws ResourceInvalidException {
      SocketAddress socketAddress;
      try {
        socketAddress = any.unpack(Address.class).getSocketAddress();
      } catch (InvalidProtocolBufferException ex) {
        throw new ResourceInvalidException("Invalid Resource in address proto", ex);
      }
      validateAddress(socketAddress);

      String ip = socketAddress.getAddress();
      int port = socketAddress.getPortValue();

      try {
        return new InetSocketAddress(InetAddresses.forString(ip), port);
      } catch (IllegalArgumentException e) {
        throw createException("Invalid IP address or port: " + ip + ":" + port);
      }
    }

and this SocketAddress will be stored against the key envoy.http11_proxy_transport_socket.proxy_address in the form of Object, the logic for this is in parseMetadata() in MetadataRegistry class.

So we see here, if we ever do endpointMetadata.get() for key envoy.http11_proxy_transport_socket.proxy_address then we will always get an Object which is SocketAddress.

Yes, I think while writing this I understand what you're trying to say. There could be ways from IO, where key is envoy.http11_proxy_transport_socket.proxy_address but the value has a different typeUrl and when our code base grows for metadata type parsing then it might get into some other parser and gives the incorrect type. Maybe!

[ejona86]

and this SocketAddress will be stored against the key envoy.http11_proxy_transport_socket.proxy_address in the form of Object

Why do you say that? Where in the code do we know proxy_address is an Address? Nowhere in the code is that written. That is determined by what we receive from the control plane. So I mostly agree with your last paragraph. But note that even without growing the code base, envoy.http11_proxy_transport_socket.proxy_address could have the type envoy.extensions.filters.http.gcp_authn.v3.Audience or Struct.

[ejona86]

I know there's a TODO in the comment, but I'm OOO tomorrow so I might at least share what meager comments I have. You can wait until I finisher the fuller review.

[shivaspeaks]

Please react on this comment (👍) if this looks good, and then I will proceed with unit tests. Except the class cast discussion.