atunnel: ingress CONNECT support - #993
Conversation
f9e4121 to
d216b54
Compare
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
d85d3f0 to
165454a
Compare
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
| type conflictStore struct { | ||
| store.Interface | ||
| conflictTriggered atomic.Bool | ||
| shouldInject func(worker *ateapipb.Worker) bool |
There was a problem hiding this comment.
This was added to try to reduce some flakiness with OnUpdate being written to/read across tests
|
Heads up: It also collides textually with #978 and #1042: all three edit the same flag var blocks in |
|
Yuan Gao (@ygao-g) I'm not sure I understand; if 0.0.0.0:443 binds dualstack by default, then an ipv6 cluster should still work with it right? The main difference is that #943 was on the egress path and required IP6T_SO_ORIGINAL_DST. The status quo should work with ipv6 only clusters |
|
You're right, and my heads-up was wrong.
One thing still applies: #1042 changes |
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
b9e42db
into
agent-substrate:main
|
I see this was merged, but looking at some of the commits, they look like intermediary commits. In the future, it would be useful to squash this right before the merge to keep the commits meaningful for the overall history... |
|
Alternatively: We have squash merge enabled as the default, if the PR doesn't have clean commit history then you can just squash it on merge. |
According to the commit history this was squashed and merged already? https://github.com/agent-substrate/substrate/commits/main/ |
|
Ok, sorry, was looking at this PR itself. No worries. We should make the guidance more clear -- I've been looking at some of the other changes that have been going in. |
Thanks Eitan Yarmush (@EItanya), and the recent the log looks clean to me ... even passed tests after merge this time 🤞
Yeah, it should be relatively uncommon that we have a PR large enough to warrant multiple commits at this point, wondering if we even want any other mode ... |
| secondFileCounterDirectory := pflag.String("second-file-counter-directory", "", "Directory for a second file counter; empty disables it. Used to exercise an Actor with more than one durable volume") | ||
| validateExistingFilePath := pflag.String("validate-existing-file-path", "", "Path to existing file to validate reading") | ||
| extraPort := pflag.Int("extra-port", 0, "Additional port to listen on, for exercising atenet-router's arbitrary-port ingress support; 0 disables it") | ||
| tcpPort := pflag.Int("tcp-port", 0, "Plain TCP echo port for exercising atunnel CONNECT ingress; 0 disables it") |
There was a problem hiding this comment.
not sure I follow what is it for?
There was a problem hiding this comment.
Oh sorry I was testing a branch for adding TCP support and this snuck into the final version. I can remove if we'd like, but it's probably nice to keep around
| const ( | ||
| // DefaultConnectPort is the worker port on which atunnel accepts inbound | ||
| // mTLS CONNECT tunnels from the ingress router. | ||
| DefaultConnectPort = 444 |
There was a problem hiding this comment.
nit: any reason 444 is used? e.g why not 8443? I understand this is from ingress --> atunnel but IIRC what what we used in #715 was 8443 for client --> ingress for CONNECT?
Is the plan to migrate this to be served on the same 443 long term or leave it as two distinct listeners?
There was a problem hiding this comment.
The plan long term will be to migrate to a single port and only support connect, but AFAIU envoy will need some changes to make that happen.
There was a problem hiding this comment.
its still a nit but why 444 and not same connect protocol as we are doing for client-->atunnel? just simpler to remember
There was a problem hiding this comment.
Do we want to reuse a port from the lower 0-1023 range?
https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers#Well-known_ports
This is currently occupied by an actual protocol assignment.
There was a problem hiding this comment.
Pagers on Kubernetes!
Seriously, good find though; I can change this to the router connect port
| return err | ||
| } | ||
|
|
||
| // ServeConnectHTTP accepts a router-authenticated CONNECT request and relays |
There was a problem hiding this comment.
what does router-authenticated mean?
There was a problem hiding this comment.
The router accepts the request and forwards it securely. We don't have authn/authz in the router today so maybe a tad misleading
There was a problem hiding this comment.
Ah sorry, I missed this in a rename. There was a function called authorize() but all it did was reserve/allocate a worker. Must've added this comment before the rename
|
|
||
| _, port, err := net.SplitHostPort(r.Host) | ||
| if err != nil { | ||
| http.Error(w, "CONNECT authority must include a port", http.StatusBadRequest) |
There was a problem hiding this comment.
so we are taking the port from authority, and we are not using any connect metadata?
There was a problem hiding this comment.
Not sure what you mean by CONNECT metadata? This is a new TCP connection/HTTP request to atunnel on a worker, so we have no envoy metadata. We could add extra headers, but :authority is built to solve this problem, so I used that
There was a problem hiding this comment.
yeah I meant CONNECT headers. the requests goes to one port on atunnel, (I assumed it is through authority port) but then needs to go to a different port on the actor (which is taken from the originally dialed dns)? how does this actor port persisted?
There was a problem hiding this comment.
Might be helpful to walk through the full flow:
- Request gets to atenet router
- atenet router inspects the request to figure out what actor to use (currently via Host header) as well as what port to go to (also via host header)
- atenet router sends CONNECT to atunnel with the port communicated via :authority
- atunnel forwards traffic to the actor based on the port in that authority
The key thing about getting that port from the atenet router is that it gives us a future where, after we stop using Host header for actor identification, we can still know exactly what port on the actor we intend to talk to without having to parse the original request (assuming that's possible; it might be TCP). In that scenario (i.e. non-HTTP) CONNECT to atenet router tells us what the port should be and we propagate that to the CONNECT to atunnel.
| value: | ||
| name: agentgateway | ||
| image: cr.agentgateway.dev/agentgateway:v1.4.1 | ||
| image: cr.agentgateway.dev/agentgateway:v0.0.0-alpha.a6c0e366 |
There was a problem hiding this comment.
Whats is the expectation here wrt to pre-released versions? (given that its in the main project). Ideally we avoid putting non stable versions here. We have been trying to avoid doing this with envoy.
There was a problem hiding this comment.
Honestly I don't think that's a realistic expectation right now. This project is so unstable itself, and is moving so quickly that I think it's ok. Especially given that we have promised an immediate turnaround on all agentgateway fixes
There was a problem hiding this comment.
I am open for thoughts. Also fine saying this is something that will be changed moving forward when substrate is more stable but we need to think about that.
We avoided doing it with envoy - we probably need to be opinionated whether we should embrace un-released versions of the proxies (and other dependencies) or no
There was a problem hiding this comment.
I think it's fair game for everything; if we're going to iterate quickly before we're stable, prerelease seems expected IMO
|
Bowei Du (@bowei) - good call on the squashed commits. I typically try to be good about doing another push after and approval, but I'll be honest in saying that flaky CI made me hesitant to push again 😅 I'll do better about this |
|
The flakes are really bad, standing offer to prioritize reviewing flake fixes... |
Finishes up the vision from #715 to have atunnel serve CONNECT on the ingress path. This will give us the option to hit actors on other ports besides 80. I haven't wired up atenet router yet because it's nontrivial; we should do that in a second step so we can have a baseline for performance