Repository navigation
Reduce number of egress atunnels by multiplexing CONNECTs over H/2 - #2425
Lior Lieberman (LiorLieberman) merged 27 commits into
Conversation
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
| return nil, err | ||
| } | ||
| if h1Conn != nil { | ||
| return c.connectH1(h1Conn, destination) |
There was a problem hiding this comment.
Do we need to support http 1.1? http2 seems strictly superior for the CONNECT use case. And this traffic is internal to the install --- it's perfectly fine for us to say "If you want to be a drop-in agent substrate egress gateway, you must support CONNECT over http/2
There was a problem hiding this comment.
I can remove H/1 support. agentgateway folks have any opinions? I was keeping it as a fallback in case there are gremlins in my H/2 changes. Since this is a simple config change on gateway.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not an agentgateway maintainer, but if we make atunnel use H/2 CONNECT then IMO that puts a limitation on any drop-in replacement for atunnel in the future because it requires those replacements also be L7 proxies with H/2 support. Whereas H/1 CONNECT is simple enough that it can be implemented by an L4-only proxy.
On the server side, I agree with Taahir Ahmed (@ahmedtd) that terminating H/2 CONNECT shouldn't be an issue for egress gateways, since AIUI any egress gateway will need to have L7 functionality anyway. My concern is about forcing an H/2 CONNECT requirement on the client side. For that reason, it might be beneficial to keep the H/1 CONNECT support for a little while longer until decisions are made about (a) whether atunnel intends to be replaceable or not, and (b) if so, whether such a replacement is required to implement an H/2 CONNECT client.
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Lior Lieberman (LiorLieberman)
left a comment
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
I like the h2 for performance obviously but I think we need to understand how this impacts policy freshness and correctness when connection is reused (e.g like the bug that led to #2213)
yanavlasov you had a comment about other complications with multiplexing tcp streams over h2 earlier in July - #338 (comment). Are those concerns resolved?
If you mean freshness of egress policy, then this PR does not change anything. What is changes is that CONNECT tunnel instead of having its own dedicated TCP and TLS transport is sharing it with up to 100 others. Processing of CONNECT requests on egress proxy however does not change. I do not know what #2213 was for, so can not answer that. Is it possible that it was a hallucinated "fix"? Regarding #338 (comment)
This change is an improvement for the highly asymmetric cost of opening new actor connections. For a single SYN packet from actor, atunnel + egress open mTLS connection, roundtrip CONNECT and make ext_proc callout. An actor aggressively retrying denied connections can DoS the dataplane. This and #2200 are means of bringing this asymmetry down. |
Signed-off-by: Yan Avlasov <[email protected]>
| return &bufferedConn{Conn: tlsConn, reader: reader}, nil | ||
| } | ||
|
|
||
| func (c *Client) connectH2(ctx context.Context, h2Conn *h2ClientConn, wasVerified bool, destination string) (net.Conn, bool, error) { |
There was a problem hiding this comment.
OK, I think I'm getting a handle on the overall implementation strategy here:
- Create our own tls.Conn(s) and remember them.
- Ask the net/http library to create a new http2 client connection over the tls.Conn, represented with a net/http.ClientConn.
- Whenever the actor opens a new TCP connection, we need to run the http2 connect process on a new http 2 stream.
- We ask the net/http.ClientConn to send an http.Request with method CONNECT. This allocates a new stream 123, and sends the CONNECT request over it. Assuming the server supports CONNECT, the response will be 200 OK. The http2 stream 123 still exists.
- When we have new TCP data from the actor to forward:
a) We somehow trick the http.ClientConn into sending a frame header for stream 123, or write the frame header ourselves (I don't yet see where this is happening).
b) We lock the tls.Conn, and write the data bytes directly to it. - We somehow run the reverse process for response data (I also don't yet see how this is happening).
Overall feedback and questions:
- Obviously it would be a lot better if net/http support connect in some coherent way. It looks like there is work being done on this, at least for http2 [1] [2]. We should track that work and use it as soon as it is usable for us.
- Where do we send END_STREAM / RST_STREAM frames? Are we just leaking stream IDs until we run out? (Answered for myself after reading the RFC --- http/2 just leaks stream IDs by design. If you run out, you have to open a new connection). However, is envoy going to keep some state open if it doesn't get END_STREAM? Also, it seems like we will exceed SETTINGS_MAX_CONCURRENT_STREAMS.
- We should extract all of the connection management into a sub-package that just exposes an io.ReadWriter or net.Conn interface for atunnel to push data into and out of.
[1] golang/go#53208
[2] golang/go#49918
There was a problem hiding this comment.
Also, are you sure this code is actually getting exercised, and the e2e tests aren't just falling back to HTTP 1 Connect? I would expect to see a call to foo.raw.Write(...) or io.Copy(foo.raw, ...) somewhere in the added code, but I don't see it.
There was a problem hiding this comment.
This whole complexity is to support both H/1 and H/2 with ALPN. If we ditch H/1 CONNECT from egress atunnel 3/4 of this code will go away.
Specifically:
- we need raw TLS connection just to see what ALPN server offered.
- client.DialContext returns net.Conn interface, which is convenient for H/1, but cumbersome for H/2. If we use only H/2 an interface convenient for H/2 could be used, and a lot of code could be removed.
- The h2StreamConn bridges net.Conn and body reader/writer channels provided to the http.Request when CONNECT was "roundtripped". This is how actor's connection is bound to H/2 stream. This answers 5a.
- We do not directly write into TLS connection in H/2 case. We write into HTTP request body writer, which codec converts to H/2 frames and writes into TLS connection. Answer for 5b.
- h2StreamConn Read reads from the body reader channel. This is how net.Conn Read method is bridged to H/2 stream reader. Answer for 6.
- We not send END_STREAM explicitly. H/2 codec does this for us when body writer channel is closed. Without END_STREAM Envoy would not complete requests. TestActorEgressMultiplexing test reads stats at the end and verifies that all requests have completed.
I think it may be better to just ditch H/1 as it will remove most of this cruft.
There was a problem hiding this comment.
Also, are you sure this code is actually getting exercised, and the e2e tests aren't just falling back to HTTP 1 Connect? I would expect to see a call to
foo.raw.Write(...)orio.Copy(foo.raw, ...)somewhere in the added code, but I don't see it.
Yes. TestActorEgressMultiplexing specifically reads the stat for the number of H/2 connections and checks that it is more than 1.
See h2StreamConn Read and Write for how actors's TCP connection is bridged to H/2 stream.
There was a problem hiding this comment.
h2StreamConn bridges net.Conn and body reader/writer channels
Ah, OK, I missed this. That makes sense, and it's pretty interesting that it works. So from the perspective of net/http it's one long request.
I agree, it seems like this could be much simpler if we only supported HTTP 2.
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
|
I've added some clarifying comments to the code. However I'm more inclined to ditch H/1 support and greatly simplify this code. |
|
I think this change can let a new CONNECT tunnel use old policy.
Suggested regression test to repro:
Did a 🤖 review for the PR with both this repo and the envoyproxy repo in context. |
Right this is what was fixed in #2213. I am not sure the fix was a good long term solution but I looks like thus pr as it is introduce this problem again After we solve this, in any case we we should not remove h1 at the same pr, we should keep it for some period of time and discuss removal separately |
Why does this look like this PR introduces this problem again? This PR changes nothing about how pooling is configured or works. No existing tests fail. Can you suggest an e2e test to validate your concern? |
Signed-off-by: Yan Avlasov <[email protected]>
Added TestActorEgressPassthroughPolicyRefresh |
Signed-off-by: Yan Avlasov <[email protected]>
|
Thanks Lior Lieberman (@LiorLieberman) and Adhita Selvaraj (@swiftdiaries) for pointing the issue with the filter state being captured on creation of connection pool to the internal listener. I have addressed this issue by making pool selection based on hash of actor cert, CONNECT authority (which includes port) and egress policy. I have also added TestActorEgressPendingPlaintextHeadersDoNotBlockMITM test to validate that pools for different policies are not shared. |
Signed-off-by: Yan Avlasov <[email protected]>
|
Thanks Yan! Nit: why are we hashing the egress policy? To avoid staleness? I wonder whether hashing only the authority and actor cert is enough For egress policy - are you hashing the whole object at realtime? |
| # Create one pool per unique tuple, containing: | ||
| # actor ID | ||
| # CONNECT authority | ||
| # Egress policy |
There was a problem hiding this comment.
nit: can we also add the object keys associated with each of these entities (dev.ate.policy.egress) in the comment?
And separately, not super sure how this translates across gateways. I understand that might not be a blocker for V1. But just surfacing this up.
cc Eitan Yarmush (@EItanya) Keith Mattix II (@keithmattix) for agentgateway
There was a problem hiding this comment.
It definitely is, we have an egress contract and this is changing it. I'll defer to John Howard (@howardjohn) as he'll know the answer offhand for sure as to whether agentgateway already supports this, my guess is yes.
There was a problem hiding this comment.
The AGW connection pool isn't per policy, but otherwise, it looks pretty similar
There was a problem hiding this comment.
agentgateway can offer just H/1 in ALPN to atunnel connection. This is will cause atunnel to use old behavior
Not PR-specific but re:credential/cert rotation, I feel like we need a safe way to surface this to an admin/operator persona. |
Adhita Selvaraj (swiftdiaries)
left a comment
Adhita Selvaraj (swiftdiaries)
left a comment
There was a problem hiding this comment.
^^ comments are not blocking. It'd be nice if we can get the test changes in.
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Done in internal/atunnel/client.go |
I have fixed e2e test so it uses TLS policy with non matching SNI. I agree credential rotation needs to be accounted for. Once credentials have expired atunnel needs to stop re-using old H/2 connections and start opening new ones. I've added TODO for this work. |
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Head branch was pushed to by a user without write access
Atunnel was hardcoded to use CONNECT over H/1. This causes atunnel to open a new mTLS connection to egress gw for each new atunnel connection, causing elevated memory and CPU usage in egress gateway.