Reduce number of egress atunnels by multiplexing CONNECTs over H/2 by yanavlasov · Pull Request #2425 · agent-substrate/substrate

yanavlasov · 2026-10-09T19:26:12Z

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.

Tests pass
Appropriate changes to documentation are included in the PR

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Taahir Ahmed (ahmedtd) · 2026-10-09T19:51:37Z

+			return nil, err
+		}
+		if h1Conn != nil {
+			return c.connectH1(h1Conn, destination)


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

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.

Alan Grosskurth (@grosskur)

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 <yavlasov@google.com>

Lior Lieberman (LiorLieberman)

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?

yanavlasov · 2026-10-10T00:45:34Z

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)

was likely my confusion - atenet-egress uses cluster IP, which will do connection load balancing.
I have addressed important concerns in this PR - this change limits number of CONNECT tunnels per H/2 connection (Egress tunnel capture + agent identity #338 had all CONNECTs over single H/2 connection), helping to balance the load on inner connections. The complexity of the internal listener however is already here, this PR does not change that.

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 <yavlasov@google.com>

Taahir Ahmed (ahmedtd) · 2026-10-10T18:57:09Z

 	return &bufferedConn{Conn: tlsConn, reader: reader}, nil
 }

+func (c *Client) connectH2(ctx context.Context, h2Conn *h2ClientConn, wasVerified bool, destination string) (net.Conn, bool, error) {


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

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.

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.

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.

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.

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 <yavlasov@google.com>

yanavlasov · 2026-10-11T03:44:10Z

I've added some clarifying comments to the code.

However I'm more inclined to ditch H/1 support and greatly simplify this code.

Adhita Selvaraj (swiftdiaries) · 2026-10-11T10:26:48Z

I think this change can let a new CONNECT tunnel use old policy.
With HTTP/1, each downstream connection carries one tunnel. With H2, several tunnels share that connection and can select the same Envoy pool.
The pool keeps the first tunnel’s authority and policy. Ordinary envoy.string values do not affect pool selection. So a new tunnel can receive fresh policy from ext_proc but still use a pool with old rules. Even a new TCP connection from that pool can receive those old rules.
We should change factory_key to envoy.hashable_string for:

dev.ate.connect.authority
dev.ate.policy.egress
This separates pools when the destination or policy differs. Keep connection_pool_per_downstream_connection to preserve isolation between authenticated connections.

Suggested regression test to repro:

A regression test should hold a TLS passthrough tunnel open, revoke its SNI rule while retaining another valid rule, wait past policy-cache expiry, and verify that a second CONNECT on the same H2 connection cannot use the revoked rule.

Did a 🤖 review for the PR with both this repo and the envoyproxy repo in context.

Lior Lieberman (LiorLieberman) · 2026-10-11T15:12:56Z

I think this change can let a new CONNECT tunnel use old policy.

With HTTP/1, each downstream connection carries one tunnel. With H2, several tunnels share that connection and can select the same Envoy pool.

The pool keeps the first tunnel’s authority and policy. Ordinary envoy.string values do not affect pool selection. So a new tunnel can receive fresh policy from ext_proc but still use a pool with old rules. Even a new TCP connection from that pool can receive those old rules.

We should change factory_key to envoy.hashable_string for:

dev.ate.connect.authority

dev.ate.policy.egress

This separates pools when the destination or policy differs. Keep connection_pool_per_downstream_connection to preserve isolation between authenticated connections.

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

yanavlasov · 2026-10-11T18:26:03Z

I think this change can let a new CONNECT tunnel use old policy.
With HTTP/1, each downstream connection carries one tunnel. With H2, several tunnels share that connection and can select the same Envoy pool.
The pool keeps the first tunnel’s authority and policy. Ordinary envoy.string values do not affect pool selection. So a new tunnel can receive fresh policy from ext_proc but still use a pool with old rules. Even a new TCP connection from that pool can receive those old rules.
We should change factory_key to envoy.hashable_string for:

dev.ate.connect.authority

dev.ate.policy.egress

This separates pools when the destination or policy differs. Keep connection_pool_per_downstream_connection to preserve isolation between authenticated connections.

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 <yavlasov@google.com>

yanavlasov · 2026-10-12T02:23:44Z

Suggested regression test to repro:

A regression test should hold a TLS passthrough tunnel open, revoke its SNI rule while retaining another valid rule, wait past policy-cache expiry, and verify that a second CONNECT on the same H2 connection cannot use the revoked rule.

Did a 🤖 review for the PR with both this repo and the envoyproxy repo in context.

Added TestActorEgressPassthroughPolicyRefresh

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov · 2026-10-12T03:42:06Z

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 <yavlasov@google.com>

Lior Lieberman (LiorLieberman) · 2026-10-12T04:42:59Z

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?

Adhita Selvaraj (swiftdiaries) · 2026-10-12T05:43:17Z

+        # Create one pool per unique tuple, containing:
+        # actor ID
+        # CONNECT authority
+        # Egress policy


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

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.

The AGW connection pool isn't per policy, but otherwise, it looks pretty similar

yanavlasov · 2026-10-12T17:12:44Z

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?

Yes, it is to avoid using pools that may have stale egress policy attached to them. If the policy was changed to be more permissive (i.e. more SNIs were added), then reusing old policy will be a problem, since SNI match is evaluated the listener filter. And correct, the whole policy string is hashed.

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov · 2026-10-12T21:51:42Z

Benchmark results for single core egress gateway:
1000QPS of small, short lived H/1 requests.

Overall:
40% bump to QPS
7x improvement to 99.0 latency
3.x improvement to 99.9 latency

Before:

Initiation to completion (4337 samples)
  min: 0s 000ms 002us | mean: 0s 015ms 645us | max: 0s 159ms 072us | pstdev: 0s 014ms 777us

  Percentile  Count       Value          
  0.5         2169        0s 013ms 733us 
  0.75        3254        0s 015ms 028us 
  0.8         3471        0s 015ms 428us 
  0.9         3904        0s 016ms 614us 
  0.95        4121        0s 018ms 034us 
  0.990625    4297        0s 128ms 016us 
  0.99902344  4333        0s 151ms 846us 

Counter                                 Value       Per second
benchmark.http_2xx                      4238        70.63

After:

Initiation to completion (7172 samples)
  min: 0s 000ms 015us | mean: 0s 008ms 558us | max: 0s 087ms 257us | pstdev: 0s 003ms 688us

  Percentile  Count       Value          
  0.5         3587        0s 008ms 091us 
  0.75        5379        0s 008ms 967us 
  0.8         5738        0s 009ms 219us 
  0.9         6455        0s 010ms 038us 
  0.95        6815        0s 010ms 793us 
  0.990625    7105        0s 018ms 442us 
  0.99902344  7165        0s 058ms 900us 

Counter                                 Value       Per second
benchmark.http_2xx                      7073        117.88

Lior Lieberman (LiorLieberman) · 2026-10-12T23:56:15Z

Benchmarks looks good, taking another look. At this point I am not sure we need to hash the whole egress policy for connection pool. shouldnt egress policy freshess be handles with whatever caching semantics we have?

taking a look

Lior Lieberman (LiorLieberman)

Ok looks good and some nits inline

Lior Lieberman (LiorLieberman) · 2026-10-13T00:10:45Z


+// This test validates that TLS passthrough connection is denied when policy changes
+// even if there are outstanding connections that were allowed by the previous policy.
+func TestActorEgressPassthroughPolicyRefresh(t *testing.T) {


This test still sets an empty policy, and an actor with no rules gets a 403 on CONNECT so the second tunnel never reaches a pool, and the test passes with or without the policy hash.
Maybe keep one unrelated rule so it actually tests the hash.

Lior Lieberman (LiorLieberman) · 2026-10-13T00:11:53Z

+	// Allow connection to be fully established before changing the policy.
+	time.Sleep(1 * time.Second)


Should wait for the first CONNECT in the gateway access log (assertEgressGatewayConnect) rather than sleeping 1s, which can be flaky?

ditto for the other sleep 1 below

Lior Lieberman (LiorLieberman) · 2026-10-13T00:12:58Z

 	}
-	if got := str(entry, "factory_key"); got != "envoy.string" {
+	if got := str(entry, "factory_key"); got != "envoy.hashable_string" {
 		t.Errorf("%s uses factory %q, want envoy.string, the string accessor the module reads", extproc.EgressPolicyMetadataNamespace, got)


Suggested change

t.Errorf("%s uses factory %q, want envoy.string, the string accessor the module reads", extproc.EgressPolicyMetadataNamespace, got)

t.Errorf("%s uses factory %q, want envoy.hashable_string, the string accessor the module reads", extproc.EgressPolicyMetadataNamespace, got)

Lior Lieberman (LiorLieberman) · 2026-10-13T00:15:48Z

-				if ok, _ := cluster["connection_pool_per_downstream_connection"].(bool); !ok {
-					t.Errorf("internal cluster %q shares its pool across tunnels; want connection_pool_per_downstream_connection: true", str(cluster, "name"))
-				}


Can you add one instead for the new rule: every filter state object the CONNECT leg shares with upstream uses envoy.hashable_string? Otherwise the next shared object someone adds brings this bug back.

Lior Lieberman (LiorLieberman) · 2026-10-13T00:18:04Z

Can you add a todo to edit the md file to describe the new transport (h2 and alpn?)

yanavlasov added 6 commits October 9, 2026 16:06

Enable H/2 between atunnel and egress gw

5ac153e

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Add test

c34b656

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Add Envoy metric scraping

b21d8e3

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Refactor stats reader

ead4f56

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Comments

6bd0cbe

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Fix test

ac7c7ad

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Taahir Ahmed (ahmedtd) reviewed Oct 9, 2026

View reviewed changes

yanavlasov added 2 commits October 9, 2026 20:26

Close H/2 connections when actor deactivates

701354d

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Accomodate gateway specific stream concurrency

f7abdf9

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Lior Lieberman (LiorLieberman) reviewed Oct 9, 2026

View reviewed changes

yanavlasov added 3 commits October 10, 2026 02:05

Merge branch 'main' into atunnel-egress-h2

99a3977

Merge branch 'main' into atunnel-egress-h2

4477a7f

post merge fixes

32b271e

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Taahir Ahmed (ahmedtd) reviewed Oct 10, 2026

View reviewed changes

Bowei Du (bowei) self-assigned this Oct 10, 2026

Bowei Du (bowei) added the area/network label Oct 10, 2026

yanavlasov added 2 commits October 11, 2026 02:53

comments

418321f

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Set saner CONNECT listener buffer sizes

36e506e

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Test policy refresh with outstanding connections

44601f0

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Add test for pool sharing

52b1c65

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Fix tests

7e6ebfd

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Adhita Selvaraj (swiftdiaries) reviewed Oct 12, 2026

View reviewed changes

yanavlasov added 3 commits October 12, 2026 17:53

Merge branch 'main' into atunnel-egress-h2

c95426c

Address comments

968bb88

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Merge branch 'main' into atunnel-egress-h2

e877cb4

Lior Lieberman (LiorLieberman) approved these changes Oct 13, 2026

View reviewed changes

Lior Lieberman (LiorLieberman) requested review from Bowei Du (bowei), Alan Grosskurth (grosskur), Keith Mattix II (keithmattix) and Adhita Selvaraj (swiftdiaries) October 13, 2026 00:17

		// Allow connection to be fully established before changing the policy.
		time.Sleep(1 * time.Second)

	t.Errorf("%s uses factory %q, want envoy.string, the string accessor the module reads", extproc.EgressPolicyMetadataNamespace, got)
	t.Errorf("%s uses factory %q, want envoy.hashable_string, the string accessor the module reads", extproc.EgressPolicyMetadataNamespace, got)

Conversation

yanavlasov commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alan Grosskurth (grosskur) Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lior Lieberman (LiorLieberman) left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Oct 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taahir Ahmed (ahmedtd) Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Oct 11, 2026

Uh oh!

Adhita Selvaraj (swiftdiaries) commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lior Lieberman (LiorLieberman) commented Oct 11, 2026

Uh oh!

yanavlasov commented Oct 11, 2026

Uh oh!

yanavlasov commented Oct 12, 2026

Uh oh!

yanavlasov commented Oct 12, 2026

Uh oh!

Lior Lieberman (LiorLieberman) commented Oct 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Oct 12, 2026

Uh oh!

yanavlasov commented Oct 12, 2026

Uh oh!

Lior Lieberman (LiorLieberman) commented Oct 12, 2026

Uh oh!

Lior Lieberman (LiorLieberman) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lior Lieberman (LiorLieberman) commented Oct 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

yanavlasov commented Oct 9, 2026 •

edited

Loading

yanavlasov Oct 9, 2026 •

edited

Loading

Alan Grosskurth (grosskur) Oct 9, 2026 •

edited

Loading

Taahir Ahmed (ahmedtd) Oct 10, 2026 •

edited

Loading

Adhita Selvaraj (swiftdiaries) commented Oct 11, 2026 •

edited

Loading