@lizan can you take a first pass at this?

Sure I will take first pass.

@klarose definitely want to see 1. as a separate PR so it can be merged sooner. The gdoc in the description is 404, can you update?

@lizan thanks for taking a look. I've fixed the link. Let me know if you still have issues. It's a relatively long document, covering more than the work I've done here. So, if you want to focus on this, just look at the design section near the end. If anything is unclear, or you'd like more of an explanation, let me know. I'll update the document/we can chat about it.

As for 1. I'll try to get that up in the next few hours.

@klarose @lizan Have you looked at the hashKey() method of Envoy::Network::Socket::Option? It is an existing method in Envoy that allows downstream connection metadata (options) to essentially divide upstream connection pools into sub-pools that each have a separate key (as accumulated from the hashKey() methods of Socket::Options). This feature is an extension of the method by which Envoy keeps different connection pools for each priority class, and as such added minimal additional logic to the upstream implementation.

This functionality is currently used by Cilium to separate the upstream connection pools by the SO_MARK option we set on the upstream connections, but would be similarly applicable to ensure that upstream connections are not shared across different source addresses.

I have recently prototyped multiplexed transport on top of a normal TCP listener, where the original 5-tuple is carried in the beginning of each frame. I used a Socket::Option to reset the upstream source address to be the original downstream address and to shard the upstream pools essentially per source address.

IMO this could be used to implement source transparency so that a Socket::Option instance would set the upstream Socket's localAddress (using setLocalAddress()), set the SO_TRANSPARENT on the socket, and return the bytes needed in hashKey() to keep the upstream connections separated by the source address. A listener filter can be created to add these options to the downstream connections, from where they are already carried over to the upstream connection constructors (e.g., dispatcher's createClientConnection() method).

So, it seems to me that a single-proxy version of source transparency could be implemented entirely using the existing extension mechanisms already built into Envoy. It could be one more listener filter in source/extensions/filters/listener/ and be exposed to users as a well known filter name they add to their listener configuration.

@jrajahalme That's certainly an interesting approach. I think it could work, as long as the pools are cleaned up fairly regularly. However, I have a few concerns with it that I think are addressed by the wrapped pool I have proposed:

1. We'd end up creating an extra connection pool for each new external connection all the time. There would never be any reuse of the pools, meaning that the overhead of pool creation and destruction is guaranteed.
2. There are no limits on the number of pools created this way. I worry that the map could bloat quite quickly. Obviously we could put some constraints on it, but then I'm not sure how we would work in the logic to give "pending" requests a chance without re implementing much of the pooling logic where it doesn't really belong.
3. It wouldn't necessarily help with other "partitioning" approaches such as SNI. @mattklein123 asked that I develop a solution which could be extended to allow pooling based on the SNI. I don't think it's appropriate to put the SNI into a socket option, though others could disagree with me. :)

At the surface, I think your proposed approach is nicer. It's simple, and it uses the existing mechanisms as you've suggested. However, I worry that it won't scale as well as we'd like, and that we may need to build something similar in the future anyway.

What do the others think? I'm willing to revisit the design.

@jrajahalme Out of curiosity, what are you using for the multiplexed transport? PROXY protocol?

It wouldn't necessarily help with other "partitioning" approaches such as SNI. @mattklein123 asked that I develop a solution which could be extended to allow pooling based on the SNI. I don't think it's appropriate to put the SNI into a socket option, though others could disagree with me. :)

#4973 adds TransportSocketOptions for SNI with hashKey approach to allow TcpConnectionPool connections to be partitioned by SNI, it left HTTP as a todo but similar approach based on that should work.

@lizan @jrajahalme Alright. You have me convinced. It's worth taking a look at. I'll see what I can do today to prove it out.

Hmm. I'm not sure this will cover all cases I'm interested in, at least as a listener filter. I want to be able to use the downstream remote address set by x-forwarder-for, which is set in void ConnectionManagerImpl::ActiveStream::decodeHeaders. This is a filter-chain filter, so it is invoked after the listener filters. Maybe I can make it work there...

EDIT: Looks like a network read filter may be the answer.

@klarose Http ConnPoolImpl already takes in Network::ConnectionSocket::OptionsSharedPtr& as a parameter, you should be able to use that for this purpose.

@klarose I've prototyped a multiplexed transport for kTLS, for which kernel-side eBPF program creates custom shim headers, basically just the 5-tuple and frame length in a binary representation. So, very similar to proxy protocol, but different :-)

@jrajahalme Thanks for the info. My current plan is to create a network filter which uses the ReadFilterCallbacks to get access to the socket option list. It'll add the option which sets the transparency/mark/etc as well as the source IP of the downstream.

Being a network filter, it can come after listener filters such as the proxy protocol filter, and the http connection manager (which sets the downstream from x-forwarded-for if it's trusted).

@klarose Interesting, I never realized you can have network filters after the http connection manager, I've kind of assumed that it will consume the payload, but maybe it does not matter for setting metadata like this.

@jrajahalme Yeah, I think the filters work by doing some work up front when the connection is established, on all filters, then they proceed to do work with the data. I'll make these changes at connection establishment time. We'll see if it works. :)

@jrajahalme Looking at adding the new socket option to do this... Unfortunately, Socket::Option only has access to a Socket, which does not itself capture the concept of a local address -- a subclass of it (ConnectionSocket) does.

Ways around this I can think of offhand?

1. Extend the visitor with another setOption taking a ConnectionSocket. Unfortunately, applyOptions is static, so we can't use inheritance to use the more concrete version of setOption. I guess I could make it a virtual method. I'd only have to change a few places to call socket_.applyOptions(options, ....) rather than Network::Socket::applyOptions(socket_ options,...)
2. Use dynamic_cast (ick!)
3. Add a parallel set of socket options called ConnectionSocketOptions. I'd rather avoid doing this, since it would involve a bunch of integration work, which I think we're trying to avoid here.

What do you think? I prefer 1, even though it's a little gross to pollute an interface with knowledge of its subclasses... That's the visitor pattern for you, though... I guess it's not really the interface we're polluting, since Option is a separate class. It just happens to be nested in Socket.

It looks like createClientConnection doesn't use the sourceAddress of the connection. Should be simple to change, though.

    if (source_address != nullptr) {
      const Api::SysCallIntResult result = source_address->bind(fd());
      if (result.rc_ < 0) {
        ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(),
                       strerror(result.errno_));
        bind_error_ = true;
        // Set a special error state to ensure asynchronous close to give the owner of the
        // ConnectionImpl a chance to add callbacks and detect the "disconnect".
        immediate_error_event_ = ConnectionEvent::LocalClose;

        // Trigger a write event to close this connection out-of-band.
        file_event_->activate(Event::FileReadyType::Write);
      }

sourceAddress comes from the clusterInfo. The pre-bind options are set just before this. So, I could check if the local address is not null. If it is, I'll override the cluster address.

@klarose Network::Socket has the local address, but not the method to set it. IMO we should add a setLocalAddress(const Address::InstanceConstSharedPtr& local_address) method to Network::Socket.

I had used a dynamic_cast in the prototype, but did not remember that fact, sorry.

I'd also want to see a non-null value returned by Socket::localAddress() override the cluster's source address setting.

I have it working for PROXY protocol. However, it doesn't work when I want to consume the XFF. This makes sense, now that I think about it -- the XFF is only set once all the http headers have been parsed, and that's too late. So, I think I need to make two filters:

1. Listener filter, gets the address from remoteAddress of the connection
2. An http decoder filter, gets the address from StreamInfo::downstreamRemoteAddress.

The listener filter can be used with ProxyProtocol, for tcp or http. The http decoder can only be used for http.

Sad, because it's more work than I anticipated, and I need to do some refactoring, but that's life I guess. :/

I'll submit a PR for the listener one, so we can take a look, then get on to implementing the http one.

Cancelling this as it has been subsumed by the work for another approach (#5337)