When proxying iptables captured traffic support proxying to arbitrary destinations

[louiscryan]

When acting as a side-car Envoy can receive all outbound socket connections via iptables capture. In situations where there is no configured route to the DST_IP Envoy will currently just drop the connection. This makes it hard to use Envoy in situations where the set of possible upstream clusters is hard or impossible to know and configure for in advance.

Requested feature is to allow Envoy to open outbound connections to the observed DST_IP when operating in this deployment.

See istio/old_pilot_repo#515 for context, may have overlap with #527

Bonus points for being able to use this mechanism in conjunction with L7 functionality like logging, stats and health-checking.

[louiscryan]

@PiotrSikora @rshriram @ayj @kyessenov

[mattklein123]

In thinking about this more, I could go either way on whether to just add this functionality to tcp_proxy or to make a new filter and reuse most of the code. I guess at this point I'm leaning towards just adding this directly to tcp_proxy and having a routing rule that points to "pass-through via original destination" vs. cluster. We will need to store the original destination in the connection interface, and then slightly refactor the tcp_proxy code such that we don't always assume the existence of a real cluster for stats purposes (or just use a synthetic NOP one). Either way, I don't think this is very difficult. @PiotrSikora we can chat offline when you are getting started on this.

[jrajahalme]

We have similar requirement, but we have to be able to use original_dst for HTTP traffic that may be subject to additional HTTP filtering, so a TCP proxy does not seem like a good fit.

I have implemented this on my development branch as follows:

1. Fix use_original_dst code to correctly determine if redirection took place or not, and store this fact as part of the Connection, so that later stages can act accordingly.
2. Add a new "original_dst_cluster" that has no real upstream host configuration, but that opens upstream connections to the original_dst from step 1, when requests are routed to it.

The main design to get original_dst information to flow from 1. (Listener) to 2. (upstream) via the HTTP filter chain is to pass a pointer to the downstream connection via the LoadBalancerContext. This context is already used as the sole parameter in cluster host selection and seemed like the least invasive choice to me.

To enable this the following steps are needed:

• Allow Connection to know if it's local address is a redirected address or not, so that we can avoid looping back to the same downstream listener from the new upstream connection.
• Make the downstream connection available to HTTP Filters via a new connection() callback similar to connectionId(), so that the HTTP router code can pass this on.
• Extend the LoadBalancerContext with a "downstreamConnection()" member.
• Add a "getRealHost()" member to Host interface, allowing a logical host to be selected and then a real host, with the actual destination address from original_dst be created. This can be neatly packaged into a new ThreadLocalCluster chooseHost() member so that the numerous load balancer implementations need not be changed with repetitive code.

This looks a lot like the dynamic_dns cluster, but instead of using the currently resolved address, the address is passed in from the downstream side via the filter callbacks and the load balancing context.

Alternatively to extending the load balancing context we could add new "upstream context" parameters to the functions involved in the call path. However, it seems plausible that a load balancing decision could be also made based on the downstream connection information, so extending the load balancing context in this manner may be a more future proof choice as well.

Currently the real hosts are not visible as (dynamically added) cluster members, as I have the sense that the interfaces involved in dynamic host set membership changes have not been designed to be used on the "fast path", but asynchronously w.r.t. request forwarding. I'm likely wrong, though, so please educate me :-)

[mattklein123]

@jrajahalme thanks for the writeup. Much more clear what you are trying to do. A few questions/comments.

We have similar requirement, but we have to be able to use original_dst for HTTP traffic that may be subject to additional HTTP filtering, so a TCP proxy does not seem like a good fit

Yup makes sense.

Make the downstream connection available to HTTP Filters via a new connection() callback similar to connectionId(), so that the HTTP router code can pass this on.

Originally, the downstream connection was not exposed to filters directly on purpose. When you do this work, let's deprecate connectionId() and switch to connection() but make you sure return a const Network::Connection and deal with whatever fixes are required to get at the info you need. Filters should not be able to modify the connection.

Add a "getRealHost()" member to Host interface, allowing a logical host to be selected and then a real host, with the actual destination address from original_dst be created. This can be neatly packaged into a new ThreadLocalCluster chooseHost() member so that the numerous load balancer implementations need not be changed with repetitive code.

You've lost me a little bit here. Can you expand? (I think have an idea what you mean, but want to make sure that we are on the same page).

Currently the real hosts are not visible as (dynamically added) cluster members, as I have the sense that the interfaces involved in dynamic host set membership changes have not been designed to be used on the "fast path", but asynchronously w.r.t. request forwarding. I'm likely wrong, though, so please educate me :-)

Yes, that's true. Everything has been designed for async fetching so we don't do any blocking operations in the fast path. However, I'm not totally sure how this is relevant to what you are proposing to do (which sounds reasonable). Can you expand?

[rshriram]

We have similar requirement, but we have to be able to use original_dst for HTTP traffic that may be subject to additional HTTP filtering, so a TCP proxy does not seem like a good fit. I have implemented this on my development branch as follows: 1. Fix use_original_dst code to correctly determine if redirection took place or not, and store this fact as part of the Connection, so that later stages can act accordingly. 2. Add a new "original_dst_cluster" that has no real upstream host configuration, but that opens upstream connections to the original_dst from step 1, when requests are routed to it. Just to make sure I understand, you want to use http filter's

capabilities, while still routing the request to the requested IP address? In this case, I guess you no longer require the ability to pick a specific cluster (as the destination address has already been decided by the client). And your intent is to just take advantage of things like rewrites, HTTP/2, retries/timeouts, etc. Out of curiosity, what would be the use case for this?

- Add a "getRealHost()" member to Host interface, allowing a logical host to be selected and then a real host, with the actual destination address from original_dst be created. This can be neatly packaged into a new ThreadLocalCluster chooseHost() member so that the numerous load balancer implementations need not be changed with repetitive code. This looks a lot like the dynamic_dns cluster, but instead of using the currently resolved address, the address is passed in from the downstream side via the filter callbacks and the load balancing context.

Bit lost here. Correct me if I am misunderstanding this: won't it be cleaner to just add a new LB implementation where you can do away with DNS lookups, and use the connection()'s destination address (original_dst) as your target? And this would be a good segue to a LB filter implementation, that just passes the request and other details, and expects an IP/port as output.

--

~shriram

[mattklein123]

Bit lost here. Correct me if I am misunderstanding this: won't it be
cleaner to just add a new LB implementation where you can do away with DNS
lookups, and use the connection()'s destination address (original_dst) as
your target? And this would be a good segue to a LB filter implementation, that just
passes the request and other details, and expects an IP/port as output.

This is not architecturally possible. There must always be a host and a cluster (of some type). They can be potentially synthetic, but the details will need to be worked out.

[jrajahalme]

Add a "getRealHost()" member to Host interface, allowing a logical host to be selected and then a real host, with the actual destination address from original_dst be created. This can be neatly packaged into a new ThreadLocalCluster chooseHost() member so that the numerous load balancer implementations need not be changed with repetitive code.

You've lost me a little bit here. Can you expand? (I think have an idea what you mean, but want to make sure that we are on the same page).

Given that a "logical host" can return a "real host" that has the destination address to which a c connection can be opened, I see two possible callers for the new getRealHost() call. It could be done by the load balancer as part to the chooseHost() call, or art can be done by the router right after having done the load balancer's chooseHost() call. If we do this in the load balancer, then all the different load balancers end up repeating similar code, or at the least calling a shared helper function to call the getRealHost() and figuring out it the return value represents another host or not. Race there than doing this, I'm opting to adding a new chooseHost() call to ThreadLocalCluster, that calls the load balancer's chooseHost() and then calls the getRealHost() of the Host returned by the load balancer. This way the new functionality is abstracted away from both the load balancers and the router. I'm planning to post a "request for comments" PR to show this and get your input on this design.

Currently the real hosts are not visible as (dynamically added) cluster members, as I have the sense that the interfaces involved in dynamic host set membership changes have not been designed to be used on the "fast path", but asynchronously w.r.t. request forwarding. I'm likely wrong, though, so please educate me :-)

Yes, that's true. Everything has been designed for async fetching so we don't do any blocking operations in the fast path. However, I'm not totally sure how this is relevant to what you are proposing to do (which sounds reasonable). Can you expand?

I haven't looked into this very deeply, but the current logical_dns cluster chooses to not represent the "real hosts" as cluster members, so I figured that maybe there is something about updating the user's of clusters about cluster membership changes that is undesirable at the request processing time. Currently it seems that cluster membership change callbacks are always asynchronous to request processing, so I'm not sure if it is feasible to change cluster membership in the midst of a request being processed or not. IMO representing real hosts as cluster members would be needed if we want stats specific to the real hosts, rather than aggregating all stats to the logical host. The set of destination hosts could also be unpredictably large, and if they are added dynamically, they also need to be removed at some point (maybe when the last connection to them dies off). As said, my current implementation replicates the pattern of the logical_dns cluster with the difference that the "real host" is a Host rather than a HostDescription, as it needs to hold a member variable for the destination address, so that the connection can then be opened to the right place. I think also this will become much clearer once you see the RFC code.

[mattklein123]

@jrajahalme cool, I think we are on the same page enough that it will be easier to discuss in the context of the RFC PR. I will look for that.

[jrajahalme]

Posted an RFC PR at #1246

[jrajahalme]

Posted PR #1314 proposing a solution to this issue.

[mattklein123]

I'm closing this issue as fixed as TCP case is already handled via original DST cluster. We can open a new issue for the DNS based unknown destination support.