Repository navigation
feat(orchestrator): nftables sandbox network datapath behind NETWORK_VERSION=2 - #3556
Closed
tomassrnka wants to merge 10 commits into
Closed
tomassrnka wants to merge 10 commits into
tomassrnka wants to merge 10 commits into
Conversation
…lint-clean The ported observability tests assumed veth-1/veth-2 already existed on the host; they now create the links they observe. Kernel-touching tests carry the repo's paralleltest nolint with the shared resource named. NETWORK_VERSION absent or empty must parse as 1, so the datapath stays opt-in.
go test ./... runs the two network packages concurrently and the ported tests hardcoded indices inside the range v1's suite allocates from, so namespace creation raced on /run/netns/ns-30001. Observer veth links are named per-test for the same reason.
Tap MAC, forward-chain posture, restart survival, teardown symmetry and the boot prerequisite all diverged from v1, so NETWORK_VERSION=2 was not a drop-in: - stamp the fixed host tap MAC, so resumed guests' cached gateway ARP stays valid - forward chain accepts by policy and accepts gw->veth unconditionally, as v1 does; the default-drop hook and ct-state match filtered traffic v1 allows, including every non-v2 forwarding path on the host - reconcile set membership at startup, and keep the table on Close while slots are still registered, so sandboxes that outlive the process keep connectivity - teardown tolerates already-absent state, runs on partial-create failure, and preserves the netns anchor when host-side teardown fails - drop the src_valid_mark prerequisite: it gates fwmark policy routing, which this datapath does not use, and nothing sets it - Populate no longer strands a slot when the pool closes mid-send
… code The egress DSCP stamp rewrote the TOS byte as a 1-byte payload write at odd offset 1. The kernel's inet checksum fixup is 16-bit-word based, so the delta landed in the wrong half and every stamped packet left with a corrupt IPv4 header checksum: with SANDBOX_EGRESS_DSCP set, all sandbox egress was dropped by the next hop. Writing the two leading header bytes, as nft itself does, keeps the fixup aligned. The wire test now validates the header checksum — an L2 capture accepts a packet the receiver would discard, which is why the original passed. Also from the same review round: - discard the shared nftables conn when a mutation fails, matching the v1 firewall: a failed buffer or Flush leaves the batch queued and the error sticky, and every slot on the host shares this conn - Get returns ErrClosed instead of a nil slot once Populate has closed the channel; CreateNetworkV2 keeps the OS thread locked when the namespace restore fails, so a poisoned thread cannot build the next slot in the wrong netns - compare set keys by trimming the ifname padding; the previous slice classified every live slot as both stale and missing - delete the fwmark policy-routing helpers, the mangle chain and SlotV2.FwMark — nothing in this datapath sets or matches an fwmark - pin the NAT, redirect and masquerade rules, the counters and the absence of iptables rules in tests that fail when the behaviour regresses
The namespace restore at the end of a slot create is deferred and can fail. Keeping the OS thread locked so it dies with its goroutine only works if the goroutine actually exits — and the only caller is the pool's Populate loop, which runs forever. A failed restore therefore pinned that loop to a thread stuck inside a sandbox namespace, and every later slot got its host-side veth, addresses and routes built in that namespace: silently, and for the rest of the process's life. On the path where the create itself succeeded the call even returned nil, handing out a slot with no host connectivity. Each create now runs on its own goroutine, so a thread that cannot get back to the host namespace is destroyed when that goroutine returns, and a failed restore fails the slot instead of reporting success. Also from the same review round: classifyVeths is split out as a pure function so a unit test can pin the padded-key comparison that decides whether reconcile deletes a live sandbox's veth — the previous assertion recomputed the answer with the same helper it was meant to check, and stayed green when that helper was bypassed.
Flush clears the queued messages even when it fails; what sticks to the conn is the serialization error, and messages queued by a mutation that returned before reaching Flush. Both still justify replacing the conn.
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit 98b8ded. Bugbot is set up for automated code reviews on this repo. Configure here. |
An nftables batch is atomic, so tearing the veth and host-CIDR entries down in one flush means an already-absent element aborts the whole thing — and because teardown treats ENOENT as success, the element that was present stayed in the set while RemoveSlot reported success. A stale veth keeps matching the forward and redirect rules. Proven with nft directly, and the regression test fails against the single-batch form.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a second sandbox-network datapath that programs nftables directly instead of writing per-sandbox iptables rules, behind
NETWORK_VERSION=2. Ported from the26-03-24-net-sbx-v2-pocbranch, datapath only — egress gateways, migration, WireGuard and the publish stub are not in this PR.With
NETWORK_VERSIONunset or1the v1 pool is constructed and its code path is untouched: the only edits to pre-existing files are a*network.Pool→network.PoolInterfacewidening (type-only, 5 lines acrosssandbox.go/server/main.go), three additive accessors, and the env field itself. Everything else is new files underpkg/sandbox/network/v2/.v2 matches v1's observable behaviour rather than the PoC's: the same fixed tap MAC (resumed guests keep their cached gateway ARP), the same two unconditional forward ACCEPTs with no default-drop hook, the same egress DSCP semantics per
EgressClassincluding theBUILD_SANDBOX_EGRESS_DSCPoverride and its nil-inherits-sandbox rule, and the same teardown ordering that preserves the netns as a reclaim anchor when host-side cleanup fails.Known limitation. v1 appends its forward ACCEPTs into the iptables
filter FORWARDchain, so they beat that chain's policy. v2's accepts live in its own table, and an accept in one table cannot override aDROPpolicy in another — measured, not assumed: on a host with-P FORWARD DROP(what Docker sets on install) a UDP probe between two namespaces is delivered with v1's rule shape and dropped with v2's. Hosts running v2 need a permissive FORWARD policy until we decide between documenting that, checking it at boot, or installing one iptables rule per slot.Rolling back to v1 needs one manual step:
Closedeliberately keeps thev2-host-firewalltable while any slot is still registered, so sandboxes survive an orchestrator restart, and nothing on the v1 path removes it —nft delete table inet v2-host-firewallonce the host has no v2 sandboxes left.Verified as root on an x86_64 box with nft + netns:
go test ./pkg/sandbox/network/v2/32 pass / 0 skip, also under-raceand-shuffle=on; the v1 network suite still green;go build ./...,go vet ./...andgolangci-lintclean. Egress DSCP is checked on the wire — the TOS byte and the IPv4 header checksum on a real capture, plus a ping round trip. The full module suite is green exceptpkg/nfsproxyandpkg/tcpfirewall, which fail identically on unmodifiedmainhere (testcontainers, no Docker on that box).Reviewed internally before opening: an adversarial multi-lens pass over the whole diff plus a verification round, which caught a checksum bug in the DSCP stamp that would have blackholed all v2 egress, a reintroduction of the poisoned-nftables-conn bug fixed in 03f10e0, and a nil-slot panic on shutdown. Draft until the metric rename is reflected in dashboards — v2 reports under
orchestrator.network.v2.*.