Envoy: Proxy proto header generation

[htuch]

Today, Envoy supports consuming the http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt PROXY header in new connections in the Listener. It would also be useful (we'll need it at Google) to be able to generate the PROXY header when performing TCP proxying.

[louiscryan]

@PiotrSikora I suspect we're going to need this in combination with routing based on SNI sniffing otherwise we'll lose the original IP

[alyssawilk]

FWIW I'm not convinced the PROXY header will be sufficient for our (GFE's) needs. The linked doc mentions intentional limited extensibility and I believe we're going to want terminating cluster, p0f information, rtt etc. I suspect we'll get that from #2394 as we'll be TCP proxying over H2 for port reduction and connection reuse.

[PiotrSikora]

@alyssawilk yes, HTTP/2 CONNECT + METADATA combo is superior, but it only works for traffic between proxies that support it. We'd use PROXY protocol on the last mile, when talking to remote 3rd-party TCP application servers (https://github.com/envoyproxy/envoy/issues/2659 might be a better alternative when talking with AFEs on localhost).

[ggreenway]

We'll need this soon as well, for tcp proxying.

[ggreenway]

Let's start discussing implementation.

First question: is this ever needed for a non-tcpproxy use case?

If not, then the implementation can go into tcpproxy and it will be pretty simple.

If yes, then my first thought would be to make it a network filter on the on the upstream connection (See #173). Or I suppose it could also be implemented by a network filter on the downstream side, as the last filter before TcpProxy, but that seems less elegant.

Thoughts and opinions?

[mattklein123]

IMO we should do this as an upstream connection network filter, especially since that work has already started?

[ggreenway]

Oh, I didn't see that the work had started (I'm pretty far behind on stuff this week). In that case, I agree.

[ggreenway]

We'll need to figure out a nice way of passing the downstream remote address to the upstream connection. Possibly pass an optional RequestInfo when creating a ClientConnection?

[mattklein123]

@ggreenway that seems reasonable to me. @alanconway WDYT?

[alanconway]

I am always translating to/from HTTP and letting envoy's existing HTTP router in the middle decide which upstream connection to use - it has the usual HTTP info to do that.
I don't have an objection to passing up information from downstream out-of-band, but presently everything I need is in the message, be it in HTTP or AMQP form.

[kylebevans]

I saw from other issues that it looks like upstream network filters are finished now. Is there active work on an upstream network filter for the proxy protocol? No pressure or anything; I'm just curious.

[mattklein123]

I don't think anyone is working on this. Help wanted!

[ggreenway]

@wez470 Was asking about this recently on slack: https://envoyproxy.slack.com/archives/C78HA81DH/p1578696402042700?thread_ts=1578696402.042700. Not sure what the status of that work is though.

[wez470]

I've been looking into it and would like to take up this work. I was actually just about to post more questions about it in slack. I'll link the discussion here after it happens, or at least add a summary.

edit - link to slack message: https://envoyproxy.slack.com/archives/C78HA81DH/p1580149986114100

[kylebevans]

If it helps, here is a link to the pull request that adds support for upstream network filters: https://github.com/envoyproxy/envoy/commit/0f892c2385e7051d325d22b3f603cc60facedd6e

And here is a link directly to the "polite filter" integration test added by that commit, which may help as an example of how to create an upstream network filter: https://github.com/envoyproxy/envoy/blob/master/test/integration/cluster_filter_integration_test.cc

[wez470]

@mattklein123 @ggreenway I wanted to have a discussion about how to best pass the stream info when creating a new connection.

To get things working so I could start building the filter, I just added a raw pointer. https://github.com/envoyproxy/envoy/compare/master...wez470:proxy-protocol-filter?expand=1#diff-0e411ba198bbc88e25d450f78141647fR207. At first I was thinking of absl::optional<StreamInfo>, but that doesn't work since it's a pure class. Potentially could be a shared pointer? Would mean updating a bunch of downstream side classes to hold the shared pointer I think. Do you have any other ideas?
Should this update be done in a separate PR?

Code is still a work in progress for sure. Let me know if I should include anyone else in discussions. I just tagged you two since you've been active on this issue.

[mattklein123]

IIRC in very recent changes all the streamInfo() accessors are moving to shared_ptr, so I think you are probably fine? Check current master?

[wez470]

Oh! Okay, I'll check that out.

[wez470]

Hmm, not seeing this on master, at least for tcp_proxy (https://github.com/envoyproxy/envoy/blob/master/source/common/tcp_proxy/tcp_proxy.h#L305). Took a quick glance at PRs and didn't see anything obviously related.

[mattklein123]

See https://github.com/envoyproxy/envoy/pull/9949 and recent PR by @gargnupur