per sandbox networking - #1689
Benjamin Elder (BenTheElder) wants to merge 4 commits into
Conversation
36d971a to
554332a
Compare
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review.
Eitan Yarmush (EItanya)
left a comment
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...
554332a to
096646d
Compare
Bowei Du (bowei)
added
the
kind/feature
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.
There was a problem hiding this comment.
keep in mind we will not have both of these once gvisor can adapt
I agree with Benjamin Elder (@BenTheElder) here that there isn't really a great name for this thing since it's a temporary solution. FWIW gateway means something different in this context, and is correct, we're just thinking of it as an L7 gateway.
Can we just leave it for now since it's an internal impl detail that will be removed?
57f05ee to
b477fbe
Compare
Lior Lieberman (LiorLieberman)
left a comment
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?
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. |
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.
fc3897e to
7057452
Compare
|
[trivial rebase on main] |
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
I'm giving this my LGTM, holding off on the official approval because there are open comments from other folks.
5 participants
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.