Support CONNECT in atenet router - #715
Conversation
Keith Mattix II (keithmattix)
changed the title
05577ee to
ab56e6e
Compare
Max Smythe (maxsmythe)
added
kind/feature
|
Bowei Du (@bowei) to route networking PR |
|
Can you put the vendor change in its own commit (make it obvious) |
168bdd4 to
d6ce656
Compare
|
Bowei Du (@bowei) good call - done |
Lior Lieberman (LiorLieberman)
left a comment
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Why do we need that network ext_proc? Can we resolve instead on the CONNECT itself. connect_terminate listener is already an HCM with no ext_proc on it - we can add the http ExtProc there.
What are the concerns with that?
| // TODO(router): authority is always empty today (see SubstrateMetadataNamespace's | ||
| // doc comment) -- dynamic metadata doesn't survive the connect_terminate -> | ||
| // main_internal internal-listener hop, and NetworkExternalProcessor has no | ||
| // filter-state-reading mechanism (no request_attributes) to fall back on the | ||
| // way the HTTP leg's handleRequestHeaders does. Until Envoy adds one, every | ||
| // CONNECT scenario that reaches this raw TCP leg -- non-HTTP payloads, and any | ||
| // TLS-wrapped payload per buildMainInternalListener's transport-protocol | ||
| // match -- fails here. |
There was a problem hiding this comment.
not sure I understand that. Can you elaborate?
AFAIU the metadata fails to cross the hop likely because passthrough_metadata only forwards static Host/Cluster metadata and never carried the value in the first place. I dont think its a NetworkExternalProcessor gap. Unless I am missing something
There was a problem hiding this comment.
I call it a NetworkExternalProcessor gap because its HTTP equivalent has a way to pass filter state to the extproc proc server and we use that to get the original CONNECT authority over there. Because request level metadata isn't passed through the internal listener transport socket; we can't use metadata to propagate it; we have to use filter state. So the requirements for the Envoy team is to be either allow internal listeners to propagate request metadata OR to allow network extproc to pass filter state via connection attributes. The same limitation applies to network ext auth btw
|
Lior Lieberman (@LiorLieberman) at connect_terminate, we don't know whether the CONNECT is tunneling HTTP or not and we want different behavior depending on the inner protocol. If it's HTTP, we want to hit the ext proc on every request so that we Resume. If it's just TCP, we only want to hit the extproc once (on connection establishment). But we don't know which one we want until we get to main_internal and sniff the protocol of the CONNECT tunnel's contents. |
569a7a5 to
172f73d
Compare
|
Documenting some of the things Keith and I discussed just now;
To answer this we need to agree on one thing -- we want/need extProc to call resumeActor on **every ** (inner) http request. With this as a given, we can use extProc on the outer connect, but we could loose the ability to reuse a persistent connection (likely has to be h2) between the harness and atenet-ingress and just stream new inner http requests within it as they come. We would loose it cause if we do extProc on outer CONNECT we wont trigger extProc on inner new http req. Long way to say that we DO need inner http ext procs. If I try to put a visual representation, I think that this PR currently attempts to do CONNECT -> HTTP ext_proc (for httptraffic) To avoid using NetworkExtProc I think we can do; CONNECT ext_proc -> HTTP ext_proc (yes - we will have two extProc calls one when the CONNECT is established, the second one in the inner http - will return early though) |
|
Also botengyao just added a PR (merged) for filter_state propagation yesterday.. Keith - take a look - envoyproxy/envoy#46551 |
|
Agree with the summary except for one thing (emphasis mine):
Not in the common case right? Again, assuming we want per request extproc triggers, the second extproc will be triggered onRequestHeaders like it does now. If anything, the first one would need to return early for HTTP, but we can't even do that because we don't know what's inside the CONNECT tunnel |
I messed up my language earlier - fixed the language.
why? Becuase we dont know whats the inside in CONNECT my point was -- outer -- always extproc (since we need it both for TCP and HTTP to call resume), then for the inner, for TCP we dont need again (cause we know the outer did it), for HTTP we do need again, cause it may be the second http stream and not the first one. So the only "cost" we paid is that we did two extProcs for http request in the first CONNECT initiation. |
|
I'm still not sure I understand: what does "return early" mean here practically? Does that mean the extproc hop is quicker than normal or that we won't call Resume() for the actor? If it's the latter, that still doesn't work if we assume that we want to call extproc for every inner http request; the second ext proc would need to be a fully fledged call to Resume so we can wake up the actor. Even if we just scope it down to the very first CONNECT establishment where we'll have double extproc, you'd need to have some sort of really small cache or other coordination mechanism if you wanted to return early (honestly may not be worth the hassle if ResumeActor() is idempotent) |
Exactly, its just a super minor optimizations I suggested (say to keep a 30 sec cache or something and if you just called resume actor dont call again for that actor). I agree its not necessary to have that optimization for now. We may be proved wrong if client harnesses wont maintain persistent connections too often as we think they will. |
a24be6b to
2b6ac54
Compare
Consolidates the vendor/, go.mod, and go.sum changes needed for arbitrary-port CONNECT ingress support into a single commit.
Adds arbitrary-port ingress via HTTP CONNECT: a connect_terminate
listener (plaintext and TLS variants) terminates the client's CONNECT
and reinjects the tunneled bytes into a main_internal internal
listener. There, a filter-chain matcher keyed on transport protocol
then application protocol splits the traffic:
- HTTP-tunneled requests go through the normal HCM + ext_proc path,
so each inner request still triggers ext_proc/Resume exactly like
plain ingress traffic -- this is what lets a long-lived CONNECT
tunnel keep working across an actor moving to a new worker pod.
- Raw TCP (or anything else) falls through to a tcp_proxy filter
chain fed by a new network (L4) ext_proc server, resolved once per
connection rather than per request.
The ORIGINAL_DST cluster moves from header-mutation-based routing to
metadata-based (OriginalDstMetadataKey/OriginalDstAddressKey): a
header only works for HTTP traffic, and the network ext_proc leg has
none, so one metadata-based mechanism now serves both legs.
Also includes: the atunnel-side CONNECT ingress handling
(internal/atunnel/ingress.go), the e2e RouterClient CONNECT support
and arbitrary-port test suite, and the counter demo's extra listening
port used to prove traffic actually reached a non-default port.
Adds arbitrary-port ingress via CONNECT for the agentgateway
dataplane, mirroring the Envoy implementation's design:
- Dedicated CONNECT binds (plaintext and TLS, matching Envoy's
connect_terminate/connect_terminate_tls) terminate the tunnel in
Tunnel mode and re-enter it into an internal wildcard bind.
- The wildcard bind's route resolves the actor from the original
CONNECT authority via source.connectHeaders, rather than its own
(re-entered, unrelated) request Host -- the same per-inner-request
ext_proc/Resume behavior as the Envoy main_internal path.
- ext_proc header mutations set both Value and RawValue: Envoy and
agentgateway differ on which field they read, so both dataplanes
need both set.
Both dataplanes can resolve the worker atunnel address without any
header mutation at all, superseding the "set both Value and RawValue"
header-based approach from the previous commit:
- Envoy's ORIGINAL_DST cluster already resolves via dynamic metadata
(OriginalDstMetadataKey/OriginalDstAddressKey) -- the
x-ate-original-dst header mutation and its UseHttpHeader-based
reading were dead code once that migration landed.
- Agentgateway's dynamic backend now supports a CEL target
expression, evaluated with the same ext_proc dynamic metadata
already surfaced as `extproc.*`. Its config backend becomes
`dynamic: { target: extproc["envoy.filters.listener.original_dst"]["local"] }`
instead of rewriting :authority and restoring it via
X-Ate-Original-Host on the atunnel side.
:authority/Host is now never touched by ext_proc on either dataplane,
so atunnel authorizes the actor by its own, unmodified Host with no
restore step needed.
2b6ac54 to
075d3e2
Compare
|
I've done another pass to clean up the commits to make this a bit more reviewable. If we're all on the same page regarding the points in #715 (comment), this should be ready for review. |
|
Re: caching of the resume -- we really need to discuss the semantics. Ideally, the server tells the consumer how long to cache. I want to avoid baking timeouts as constants into the client -- this presents problems for flexibility in how the caching works, including disabling it in certain cases. #593 |
|
I see that the code has the Network ext_proc, but there was a lot of discussion above how we don't need it at the moment. Is it possible to get a conclusive comment here on whether or not we have settled on the approach? It's not clear at the moment from what I can tell from the discussion. |
If we take the approach currently in the PR, we don't need to cache any resumes; we only call it once. Lior Lieberman (@LiorLieberman) was suggesting alternative approaches that would reduce the need for network extproc.
Lior Lieberman (@LiorLieberman) correct me if I'm wrong, but my understanding from our discussion (above and synchronously) is that we're aligned on using network ext proc for now for a couple of reasons:
|
4 participants
Fixes #689, #265 and starts to address #484. Modifies the ate router to:
#484 is currently blocked because Envoy's implementation of NetworkExternalProcessor doesn't support passing filter state to the extproc server. Dynamic metadata doesn't work because only Host (i.e. endpoint) and cluster metadata can propagate across internal listeners (a necessary implementation detail of doing CONNECT in Envoy). This PR still adds the xDS for it; the extproc calls just fail because there's no metadata.
Open questions:
We're assuming that non HTTP actors don't get suspended during the lifetime of a TCP connection; we only run Resume on the initial connection establishment. We could potentially do network ext authz instead (it actually on gets invoked once during conn establishment vs. we hit extproc on every client write), but wanted to discuss. I will say that I highly doubt that we'll need ANYTHING in the TCP body.Per Lior Lieberman (@LiorLieberman)'s comments, there's Envoy work being done on network ext_proc, so I assume that's what we're doing.Tested 6 scenarios locally:
Once #652 merges, I can add the agentgateway implementationAdded and tested agentgateway implementation as well