cc @lizan @htuch

I have a general question about this PR, as it relates to the ongoing discussions around the work that @klarose is doing for source transparency. I assume the goal here is that we can make upstream connections with the downstream SNI information? Is that right?

Per discussions in the related issues (e.g. #5337), instead of passing the transport socket information around, can't we just use an HTTP filter to set the socket hash options such that SNI would be used to create a discrete connection pool in the cluster manager connection pool hash? It seems like if we do that we can largely skip this PR and not have multiple methods for doing the same thing. Thoughts?

@mattklein123 I would like to understand how socket options are used in the case of #5337. Does Envoy's Network::Socket::Option represent Unix socket options, as described for example in https://linux.die.net/man/7/socket ? If yes, which Linux socket option can represent SNI?

My assumption in this PR and in #4973 was that SNI cannot be represented by Lunux socket options, and as a result cannot be represented by Network::Socket::Option. So I added TransportSocketOptions https://github.com/envoyproxy/envoy/pull/4973/files#diff-ce02143d0382937ced28781ec174f1c1R141 which should be independent from Unix socket options and can contain information to pass from the downstream connection to the upstream. It's hash is used in connection pool selection in the same way as Network::Socket::Option is used.

So, Network::Socket::Option can be used only for passing information related to the Unix socket options. TransportSocketOptions can be used to pass arbitrary information. Alternatively, if Network::Socket::Option can pass arbitrary information, TransportSocketOptions is not needed and can be removed.

SNI cannot be represented by Linux socket options. IMO, TransportSocketOption is needed to be passed to transport sockets, while Network::Socket::Option are handled by connections and they are not passed down to transport sockets.

Per discussions in the related issues (e.g. #5337), instead of passing the transport socket information around, can't we just use an HTTP filter to set the socket hash options such that SNI would be used to create a discrete connection pool in the cluster manager connection pool hash? It seems like if we do that we can largely skip this PR and not have multiple methods for doing the same thing. Thoughts?

+1, IIUC @klarose is proposing an HTTP filter to set upstream SNI for HTTP cases (by either punting TransportSocketOptions int o LoadBalancerContext or have separate callbacks), it is more clearer than use the TCP semantics in router in this PR.

I also propose an HTTP filter that would set UpstreamServerName in the filter state of the stream info of the downstream connection. The router will get this UpstreamServerName and put it into TransportSocketOptions. The logic for passing the SNI by TransportSocketOptions, from the HTTP filter has to be implemented anyway, and it is part of this PR.

Even more so, I propose to put the UpstreamServerName in the metadata of the StreamInfo. This way, header-to-metadata could set SNI from the value of an arbitrary header.

@vadimeisenbergibm @lizan I think the technique @mattklein123 is referring to is not to set an actual socket option in the OS, but rather to use Envoy's SocketOption class to encode a unique identifier that may be used to ensure that requests to a given SNI go to a connection pool unique to that SNI.

Currently, all requests to a given upstream transit through the same connection pool, disregarding the SNI. There is no way to partition upstream connections based on the SNI within the connection pool. However, you can partition connection pools based on the hashKey() of a SocketOption. So, we could simply pack the SNI into the hashKey(). This would have the effect of ensuring that requests for a given SNI are never routed to an upstream connection used by requests for other SNIs.

Is that all you want the SNI for -- to partition upstream connections? Or, did you actually need the SNI for other purposes?

Is that all you want the SNI for -- to partition upstream connections? Or, did you actually need the SNI for other purposes?

@klarose Yes SNI is for partition upstream connections, they also need to passed down to transport socket (TLS extensions). The current TransportSocketOptions does have hashKey() in same semantics as Socket::Option has, and in #4973 they are already used for those in TCP proxy (not in HTTP yet). I don't think we should merge TransportSocketOptions and Socket::Option into one since they go to different component at the end.

I don't think we should merge TransportSocketOptions and Socket::Option into one since they go to different component at the end.

IMO we could merge them into one logical entity since I think we are using them for mostly the same things, and there is going to be a lot of duplication. With that said, I don't feel very strongly about it. Would it at least be possible to have them share some base interface as it relates to hashing for connection pools, etc.?

+1, IIUC @klarose is proposing an HTTP filter to set upstream SNI for HTTP cases (by either punting TransportSocketOptions int o LoadBalancerContext or have separate callbacks), it is more clearer than use the TCP semantics in router in this PR.

Yeah, this is my main objection to this PR. I think we need to figure out how we want to generically pass downstream information into the connection pool, either via LB context or something else. I know that @klarose has been thinking about this a bunch from some of our prior discussions. Since the upstream SNI stuff is now (I knew it was coming @klarose!) can we work together to build a common plan? @klarose @vadimeisenbergibm perhaps you can hash this out offline and come back with a proposal? Or feel free to do it here if that's easier.

@mattklein123 From my point of view as a novice in Envoy, I think we can just merge the two into one logical entity. I would, however, make clear that Network::Socket::Options are not necessarily related to Unix socket options. Either to document it, or call the class something like ConnectionOptions.

@klarose @lizan @mattklein123 @htuch Let's decide what we want to do and then decide who will perform the refactoring of #4973, I can do it.

Would it at least be possible to have them share some base interface as it relates to hashing for connection pools, etc.?

This makes sense, IMO that is the only common feature that two has. Current Socket::Option is tightly coupled with ListenSocket/ConnectionSocket. We can have a base class and separate them accordingly. Transport socket is extension and there could be more and more options for them.

I'm okay with a base class. How about "HashableOption" or something like that?

With respect to the refactoring, is the plan here only to simplify how we actually go about building the hash? I'm not sure a base class will provide much more than that, since we'll still need to pass around these objects in separate arrays; the more concrete type will be important to any consumers of them.

For a more intensive refactoring, let's think about what we're trying to do. We have two things we want to expose to the connection pool:

1. Upstream Socket Options (for my use-case)
2. The SNI (for this use-case)

Both of these need to influence hashing. However, once they make their way to the connection pool, they do different things.

Currently, the upstream socket options will be used in one way -- to set the source IP, SO_MARK and IP_TRANSPARENT on the upstream socket. The SNI is used in a different way -- to create the SSL socket.

What if, instead of the SNI being passed to createTransportSocket, we instead set it on the socket, just like we do the other socket options? Then we could just make this SNI option a normal socket option, and pass it to the connection pool as an upstream socket option. How would we do this? We'd extend the socket visitor to accept an SslSocket as well as a generic Socket. Of course, this wouldn't really work unless TransportSocket were a subclass of Socket. I suspect that refactoring would be a lot of work, so maybe it's not a good idea.

If we don't do the above, then it probably makes more sense to keep TransportSocketOptions parallel to normal SocketOptions. But, if we do that, I wouldn't mind seeing them made a little more extensible. Currently you can only have one, and it can only optionally be used to set the SNI. Further extension would likely involve adding other optional accessors, making for a somewhat polluted interface. It would probably make more sense to make the TransportSocketOptions a visitor, just like SocketOption. Then, as long as the TransportSocket to which the option is being applied has the functionality to 'set' the option, it'll work fairly nicely.

Does anyone else have thoughts on this?

Does anyone else have thoughts on this?

+1 to your suggestions. Curious what @lizan thinks also. I do think that if we were creating we could merge transport socket options and socket options, but as I said before, I don't feel very strongly about it. I think some type of trivial base class that understands hashing so that we have less code duplication in the conn pool hashing code (and dedup http/tcp) would be nice.

I like the visitor idea for transport socket options also.

If we don't do the above, then it probably makes more sense to keep TransportSocketOptions parallel to normal SocketOptions. But, if we do that, I wouldn't mind seeing them made a little more extensible. Currently you can only have one, and it can only optionally be used to set the SNI. Further extension would likely involve adding other optional accessors, making for a somewhat polluted interface. It would probably make more sense to make the TransportSocketOptions a visitor, just like SocketOption. Then, as long as the TransportSocket to which the option is being applied has the functionality to 'set' the option, it'll work fairly nicely.

+1 to this.

@lizan @mattklein123 @klarose So if we keep TransportSocketOptions, this PR is valid AS IS, in my opinion. We can merge this PR and then refactor TransportSocketOptions. The currently proposed refactoring:

1. Common base class with SocketOptions for hashing
2. Visitor pattern for TransportSocketOptions

If we agree on the current design and the refactoring, I can start working on it. Let's agree first though, no pressure, so we are all sure we are confident in the solution.

@vadimeisenbergibm I'm not sure we want to merge this as is. Didn't we discuss above not doing the changes in the router but in a filter of some type?

@mattklein123 Ah, I see, this is another point - where to put setting that TransportSocketOption. I thought that Router could be that filter - that reads UpstreamServerName from the filter state of the downstream connection and creates the TransportSocketOption. UpstreamServerName can be set by some other filter in the filter chain before the router.

For TCP, creating the TransportSocketOption is done by tcp_proxy.

Otherwise what should be the filter that creates the TransportSocketOption? Note that if some other filter, before the router in the filter chain, creates the TransportSocketOption, router will have to be changed anyway to pass the TransportSocketOption further to ConnectionPool.

@vadimeisenbergibm See #5415 (comment). It is possible to put TransportSocketOptions in LoadBalancerContext and allow HTTP filter to modify it via callbacks.

@lizan I thought we wanted the following flow:

1. Some filter sets UpstreamServerName on the StreamInfo::FilterState of the downstream connection.

2. Some other filter, for TCP it is tcp_proxy, creates TransportSocketOptions (currently the only option is SNI), according to the value of UpstreamServerName.

We have this indirection (a listener filter -> UpstreamServerName -> tcp_proxy -> TransportSocketOption), do we want to remove it, and let both listener filters and HTTP filters to manipulate TransportSocketOptions directly? Or do we want it to leave it for listener filters and but let HTTP filters to manipulate TransportSocketOptions directly?

@vadimeisenbergibm that flow is intuitive for TCP, though for HTTP there are more things to consider, in HTTP(>=1.1) world, stream (request) to connection is N:1 mapping. And the router will map each request to upstream connection pool.

In my mind the flow for HTTP is:

1. Some HTTP filter sets UpstreamServerName in the L7 request (not downstream connection). This might be achievable by setting it directly to the L7 StreamInfo (i.e. callbacks->steramInfo()) or by adding it to LoadBalancerContext, which @klarose is exploring after [filters] original source listener filter beginnings #5337. You may write a HTTP filter to populate L4 StreamInfo to L7 StreamInfo, or maybe we can have some propagation here.

2. The HTTP router (this is equivalent to tcp_proxy in TCP world), will pass the information set by the HTTP filter (either in L7 StreamInfo or LoadBalancerContext) to connection pool and use them.

Yeah +1 to what @lizan says. That is how I have been thinking about it and I think @klarose also from our previous discussions. We should have some explicit way to alter the upstream connection info (vs. downstrem) on a per-stream basis.

OK, agreed, closing this PR.