per sandbox networking - #1689
Benjamin Elder (BenTheElder) wants to merge 4 commits into
Conversation
36d971a to
554332a
Compare
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review.
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 mostly AI generated, reviewed by me :)
Sorry for the double review, the agent initially didn't understand that I wanted to append the first review comment to these...
| if err := s.releaseSandboxNetwork(ctx); err != nil { | ||
| return fmt.Errorf("while releasing the previous sandbox network: %w", err) | ||
| } | ||
| session, err := ateomnet.ServeSandbox(ctx, ateomnet.SandboxNetworkConfig{ |
There was a problem hiding this comment.
Preserve the documented behavior when no egress gateway is configured.
gVisor setup and microVM setup now always redirect TCP into atunnel, but prepareActorEgress still returns nil for an absent gateway and activateActorNetworking then skips activating egress. The inactive handler closes every intercepted connection. The API server still defaults the gateway address to empty, and the protocol contract explicitly promises direct egress in that case.
Could we preserve this supported mode, or explicitly require a gateway and update the configuration contract? At present an actor can start successfully with all external TCP connectivity broken. This was traced through both runtimes; I did not run a complete boot without a gateway.
There was a problem hiding this comment.
After some side discussions: I think we want to change this to clearly state that egress is not supported without a gateway, but a gateway is not strictly required (perhaps your actor does not need egress).
If we really need it later we can add a no-capture mode, but it greatly simplifies things to always use atunnel, and atunnel will need an egress gateway for egress.
There was a problem hiding this comment.
plus 1 to ben
554332a to
096646d
Compare
096646d to
2c90bf9
Compare
| if err := root.Mkdir("etc", 0o755); err != nil && !errors.Is(err, fs.ErrExist) { | ||
| return fmt.Errorf("creating %q: %w", filepath.Join(rootfs, "etc"), err) | ||
| } | ||
| if err := root.Remove("etc/resolv.conf"); err != nil && !errors.Is(err, fs.ErrNotExist) { |
There was a problem hiding this comment.
Prefer write out file and then use atomic rename to replace file contents.
There was a problem hiding this comment.
We're doing this while nothing is reading it? I don't see the benefit. This only happens when the actor is not yet running.
2c90bf9 to
a04fc54
Compare
faf330a to
57f05ee
Compare
| // TODO: we hope gVisor can take that same single-namespace shape soon, | ||
| // once runsc can be given one interface rather than claiming every | ||
| // interface in the namespace it runs in. | ||
| GatewayNetNS netns.NsHandle |
There was a problem hiding this comment.
Naming this GatewayNetNS is very confusing, since this is for atunnel. How about this?
| Current | Proposed |
|---|---|
SandboxNetwork.GatewayNetNS |
SandboxNetwork.AtunnelNetNS |
SandboxGatewayNetNSName |
SandboxAtunnelNetNSName |
setupGatewaySide |
setupAtunnelSide |
gatewayVethName |
atunnelVethName |
There was a problem hiding this comment.
It's not just atunnel, so I'm not sure that's clearer. We're also doing the DNS listener, and possibly soon an "upward API" socket.
I do hope we can align gVisor soon so there's just one netns and no veth.
There was a problem hiding this comment.
Point is that its not "Gateway" and using the word "gateway" is confusing? What is it beyond Atunnel?
There was a problem hiding this comment.
keep in mind we will not have both of these once gvisor can adapt. RuntimeNetNS will be the only one. GatewayNetNS is only if we have to split them across the veth, we drop this one later.
I don't think Atunnel vs Runtime is clearer personally but I'll rename to a consensus.
57f05ee to
b477fbe
Compare
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
LGTM overall
| // TODO: we hope gVisor can take that same single-namespace shape soon, | ||
| // once runsc can be given one interface rather than claiming every | ||
| // interface in the namespace it runs in. | ||
| GatewayNetNS netns.NsHandle |
There was a problem hiding this comment.
Point is that its not "Gateway" and using the word "gateway" is confusing? What is it beyond Atunnel?
| if err := s.releaseSandboxNetwork(ctx); err != nil { | ||
| return fmt.Errorf("while releasing the previous sandbox network: %w", err) | ||
| } | ||
| session, err := ateomnet.ServeSandbox(ctx, ateomnet.SandboxNetworkConfig{ |
There was a problem hiding this comment.
plus 1 to ben
b477fbe to
da4ba43
Compare
Use fixed sandbox addresses in private namespaces. gVisor uses a veth pair across two namespaces; microVMs use a tap in one. Redirect TCP egress to atunnel and provide namespace-scoped listeners and dialers. The dialer takes TCP and UDP IP literals only, and restores the worker namespace once the socket exists so a pending connect does not pin a native thread.
Forward UDP and TCP DNS unchanged through the worker pod. Bound concurrency and connection lifetime, and close TCP connections on cancellation. DNS bypasses the actor egress policy.
A sandbox now lives in its own network namespace, so a readiness probe has to be dialed from there rather than from the worker's. Take the dialer as a parameter, and let a caller name the resolv.conf bound into the sandbox.
Wire both runtimes to isolated namespaces, namespace-aware ingress and readiness, and gateway DNS. Preserve fixed addresses across restore. Replace worker-wide routing and microVM TC mirroring with per-sandbox TCP redirects.
da4ba43 to
fc3897e
Compare
|
One naming bikeshed outstanding. We can revisit that monday. Resolved the others and did some more passes for bugs / minor nits. |
This is part of #1266 , opening now for discussion.
Stacked on #1682 which was slightly orthogonal.
This is loosely based on the mini proposal by John Howard (@howardjohn) as discussed in the community meeting, and feedback from Bowei Du (@bowei) Eitan Yarmush (@EItanya) Lior Lieberman (@LiorLieberman) Antonio Ojea (@aojea).
https://docs.google.com/document/d/1TycfQ3iiEpbI3rveMIj0S2PpPuLecb8I5R--yTpt9Ig/edit?resourcekey=0-kJbtEZ-KGzuL5eCjHDvBhg&tab=t.0#heading=h.ga9bfaf55ptk
Roughly:
Every actor gets the same fixed guest IP as before, which is only visible to the actor.
All inbound/outbound traffic comes from atunnel / the DNS relay.
The actor no longer has any direct use of the pod interface, so we can begin to consider ateom using the network itself.
When we add the rest of multi-actor changes, this greatly simplifies thing.
Full multi-actor requires further changes, but this diff is already large (suggest reading commit by commit) and can stand-alone. I'll file more stacked changes when we've got consensus on this one.