feat(network): update sandbox egress policy in place, re-evaluating live flows

[FakeLearne]

Add PUT /sandboxes/{sandboxID}/network so a running sandbox's egress policy can be replaced without recreating it, and make the update reach traffic that already exists rather than only future connections.

The route, the status codes (204/400/404/409) and the full-replacement semantics follow E2B, so an omitted field clears rather than preserves. One divergence is deliberate: E2B's nftables chain accepts ESTABLISHED,RELATED first, so tightening a policy there has no effect on open connections and a long-lived connection can keep using a revoked destination indefinitely. We re-evaluate instead.

Datapath

Each sandbox carries a policy generation (mvm_meta.policy_version), bumped only after both planes hold the new policy. Every session caches the generation it was admitted under, so the next packet on an established flow is re-judged exactly once per update:

• same verdict: restamped and left alone, costing one policy lookup per flow;
• no longer allowed, or the verdict changed: the session pair is deleted. TCP is answered with an RST, matching how every other unreachable TCP packet is handled here, so the guest fails fast instead of stalling on retransmits; UDP and ICMP have nothing to reset and are dropped.

A verdict change retires the flow rather than migrating it. The SNAT and L7 paths disagree about both the reply tuple and which side terminates the TCP connection, so migration is not expressible; the client reconnects and is judged as any new flow.

Deleting rather than flagging keeps the retirement self-enforcing, and is what lets this stay small. A later non-SYN packet finds no session and is reset, so a revoked flow cannot resume even if a subsequent update re-allows the destination, while a SYN legitimately opens a fresh connection under the current policy. Both directions go at once, so reply-path callers need no change either, and neither does the reaper.

That property holds only once the legacy 80/443 connection drain in do_tcp_nat is removed, which this does. While that branch is present a deleted session is indistinguishable from a lost one, and the drain silently undoes the revocation. It had also lost its own justification independently: written (1b5d2c3) when L7 flows created no session and re-ran the policy lookup per packet, so a DNS-learned entry expiring broke live connections, it was made redundant by #1347, after which L7 flows cache their verdict in nat_session and an established flow never consults the policy maps again. Two of the three causes its comment cites do not hold either -- both session maps are pinned and no startup path flushes them, and they are BPF_MAP_TYPE_HASH rather than LRU, so nothing is evicted. The remaining case, a flow idle past the reaper timeout, contradicts the reaper it depends on: deleteSessions removes both directions, and the drain then revives the egress direction alone, without recreating a session, on the sole evidence that the proxy still holds a socket -- scoped by which flows happen to leave a host socket rather than by policy, which is why it only ever applied to 80/443. Consequently an L7 keepalive connection left idle past the reaper timeout is now reset rather than revived, and the application reconnects.

policy_version fits in existing padding, so mvm_meta and nat_session keep their sizes and the pinned-map layout is unchanged. The kernel verifier accepts del_session() while the lookup pointer is still held, checked against mvmtap, nodenic and localgw.

Control plane

cubevs gains UpdateTAPDevicePolicy, a third apply mode beside applyNetPolicy (additive) and replaceNetPolicy (flush then refill). Neither fits a live sandbox: flushing blanks the policy for as long as the refill takes, and swapping the inner map defeats the HashOfMaps inner cache and pays a synchronize_rcu per update. So it diffs against the installed maps and writes only what changed. DNS-learned rows are left untouched, and deny_out's desired set always includes the always-denied private and link-local ranges.

Ordering is part of the contract: CubeEgress, then CubeVS (which bumps the generation), then durable state. A failure before the bump leaves flows on their cached verdict instead of judging them against a half-applied map, and a failure before the state write replays the previous policy on restart. Nothing is rolled back; the diff is computed from the live maps, so replaying converges.

For clone, snapshot and pause/resume the new policy is written to all three places that outlive the update: the network runtime state file (restart recovery), the Cubelet cubebox store (pause packaging) and Master's sandboxspec (what clone replays). The sandboxspec write happens synchronously before the response, so update then snapshot then clone is read-your-writes; a failure there is logged and does not fail the call, following the same best-effort convention the create path already applies to that store.

DNS resolver allowance

The create path folds the sandbox's resolver addresses into allow_out so domain rules can be resolved at all, but only when the policy names a domain. An update carries just the caller's targets, so it would silently revoke DNS and black-hole every domain rule it had just installed. The resolver list is now recorded on every create -- unconditionally, because a later update may introduce the first domain -- and folded back in under the same "policy still names a domain" condition, so dropping every domain also withdraws the implicit DNS access.

Recording is separate from installing: an all-IP policy still gets no resolver access, since allow_out cannot be narrowed to port 53. The gate therefore asks whether a target is installed as a domain, not whether it looks like a DNS name. The latter accepts "10.0.0.1" -- digits are valid label characters and nothing requires a non-numeric top label -- and would fold the resolver into every policy written with bare IPv4 literals, granting one extra /32 on all ports that the caller never asked for. cubevs exports that install decision as IsAllowOutDomainTarget, mirroring splitAllowOutTargets, so the update gate cannot diverge from the create gate, which has always been IP-first.

An update that neither installs nor clears L7 rules does not contact CubeEgress at all. That is decided from the old and new rule sets, not from whether the proxy happens to be configured, so an L3-only sandbox never depends on a component it does not use.

Known limitation

Addresses already learned for a domain outlive the rule that produced them until their DNS TTL expires, so revoking a domain promptly needs a short resolver TTL. Provenance tracking for learned entries is deferred.

Testing

Go unit tests cover the map diff (revocation, expanded L7 port sets, deny_out convergence, DNS port-set replacement), the generation bump and its survival across metadata rewrites, and the controller's ordering, resolver refold, L3-only skip and "a failed update keeps the old policy" guarantees. The resolver gate is exercised over bare, masked, subnet and L7-host target forms, because only the bare literals distinguish the install decision from a name-shape check. A new BPF case drives session_policy_revoked through the existing egress-policy harness across seven verdict/generation combinations.

Ten end-to-end cases were added under a new network_dynamic_update capability. Two of them are what actually distinguish this feature: a revoked connection must be torn down, and a still-permitted one must be left alone -- the second is what stops an implementation that simply kills every session from passing. Their guest-side holder uses TCP keepalive rather than application bytes, because a peer that hangs up on unexpected data is indistinguishable from a policy reset, and it classifies RESET, ALIVE, EOF and DATA separately so only the unambiguous outcomes are asserted.

Three more cover the compatibility requirement: a clone inherits the updated policy rather than the create-time one (which also covers read-your-writes, since clone snapshots immediately after the update returns), a clone of a narrowed policy is not more permissive, and the policy survives pause/resume. These have to be end-to-end -- clone and snapshot are not network-aware, they just replay a stored create request, so nothing under templatecenter changed here and review alone cannot tell whether the spec write-back landed.

Verified on a live single-node deployment: all ten cases pass on repeated runs, and examples/network-policy/network_dynamic_update.py walks an IP allow list, a connection carried across a revoking update (observed as RESET), a domain allow list, and switching on L7 interception mid-run.

[cubesandboxbot]

High severity — paused/not-running sandbox returns HTTP 500, not the documented 409.

sandbox_not_found_or_internal only maps 130404 (is_not_found()); everything else falls through params_error_or_internal, which maps only 130400/invalid-path to 400 and everything else to internal_error → 500. When the Cubelet's updateNetworkPolicy rejects because the sandbox is paused (ErrorCode_Conflict, 130409), the master forwards 130409 verbatim (updateSandboxNetworkOnNode copies cubeRsp.GetRet().GetRetCode()), and this function turns it into a 500.

That contradicts the handler's own utoipa annotation (409: Sandbox is not running), the README ("update_network returns 409 | Sandbox is paused or already gone"), and the Go SDK docstring ("including 409 when the sandbox is not running"). A deterministic, documented client-state condition also charges against server-side error-rate SLIs.

CubeMasterError::is_conflict() already exists, and the pause/resume endpoints map 130409 → AppError::Conflict via map_update_cubemaster_err/ensure_update_result. Reuse those helpers here instead of sandbox_not_found_or_internal.

[cubesandboxbot]

Medium severity — unknown sandbox ID returns HTTP 400, not 404.

CubeMaster::UpdateNetwork responds to a missing sandbox with ErrorCode_MasterParamsError (130400), "sandbox not found" (from resolveSandboxHostIP failing) — it never returns 130404 on this path. is_not_found() matches only 130404, so this maps to AppError::BadRequest → 400.

The utoipa annotation documents 404: Sandbox not found, and the Go SDK's UpdateNetwork promises ErrSandboxNotFound (404). fetch_sandbox_detail handles this class of master response with an explicit RET_CODE_NOT_FOUND check; this endpoint has no equivalent. Either have the master return a not-found code (130404) when resolveSandboxHostIP fails here, or map the master's "sandbox not found" params error to AppError::NotFound in this function.

[cubesandboxbot]

Medium severity — this classifier disagrees with the create path and with cubevs's own install logic, contradicting the comment ("this cannot disagree").

cubevs.IsDNSAllowTarget is a thin wrapper over isDNSAllowTarget, which validates DNS labels only and never excludes IP-looking strings first. For "1.2.3.4" it returns true (labels "1","2","3","4" are all valid). But:

• cubevs's actual install logic (splitAllowOutTargets) checks isIPv4Target → net.ParseIP/isDottedDecimalLikeTarget before isDNSAllowTarget, so "1.2.3.4" is routed to the IP/CIDR map (allow_out_v3), never to dns_allow_v2.
• The create path (isAllowOutDomainTarget in Cubelet/network/plugin_policy.go) returns false for "1.2.3.4" (isIPv4NetworkTarget → net.ParseIP succeeds).

Concrete failure: create with allow_internet_access=false, allow_out=["1.2.3.4"], resolver 10.2.0.53 → shouldAppendDNSAllowOut returns false, so no resolver /32 is installed. Then update_network with a policy still naming allow_out=["1.2.3.4"] → needsDNSResolution returns true → withDNSResolverAllowOut folds 10.2.0.53/32 into AllowOut. The update silently widens egress to the DNS resolver (and persists it via DNSAllowOutCIDRs), so a later clone/pause-resume/restart replays a policy the original create never granted.

Fix: exclude IPv4/CIDR/dotted-decimal targets before consulting IsDNSAllowTarget in needsDNSResolution (mirroring splitAllowOutTargets' ordering), or make the exported classifier replicate that ordering.

[cubesandboxbot]

Low severity — an all-empty body opens internet access.

build_cube_network_config(None, None) returns Ok(None), and .unwrap_or_default() yields a CubeNetworkConfig with allow_internet_access = None. On the Cubelet side cubeVSTapRegistration treats a nil AllowInternetAccess as true (the create-time default), so PUT /sandboxes/{id}/network with body {} clears allow_out/deny_out/rules and flips the sandbox to internet-allowed.

The request-model doc says "any field left out clears what the sandbox currently has" — but omitting allowInternetAccess clears it to default-true, which is likely not what a caller sending {} means (typically "clear restrictions", not "permit everything"). This is documented in the README (update_network(network={}, allow_internet_access=False)), so it's consistent behavior, but on an update endpoint where the natural reading of an empty body is "no change", this default is a footgun — at minimum worth a note in the OpenAPI/utoipa docs that omitting allowInternetAccess means "allow", and arguably worth requiring the field explicitly on this endpoint.

[cubesandboxbot]

Review: feat(network): update sandbox egress policy in place, re-evaluating live flows (#1399)

AI-generated review — this review was produced by an automated review agent. It has not been reviewed or approved by a human. Findings are ranked by severity; inline comments accompany the highest-confidence ones.

Overview

This PR adds PUT /sandboxes/{sandboxID}/network so a running sandbox's egress policy can be replaced in place, and — the distinguishing part — makes updates reach traffic that already exists. Each sandbox carries a policy generation (mvm_meta.policy_version) that userspace bumps only after both CubeEgress and the CubeVS maps hold the new policy; every session caches the generation it was admitted under, so the next packet on an established flow is re-evaluated exactly once per update. A verdict change (SNAT ↔ L7) or a new denial retires the flow rather than migrating it.

The design is genuinely sound and the plumbing is careful: the generation model, the ordering contract (CubeEgress → CubeVS → persist; bump last; revocations before additions within each map), struct-layout compatibility across BPF/Go, the DNS-resolver fold-back for updates, and the L3-only skip of CubeEgress are all implemented coherently and backed by a strong test suite (8-verdict BPF harness, Go map-diff/ordering tests, 10 e2e cases including the distinguishing "revoked connection tears down / permitted one survives").

Four issues are worth addressing; the first is a client-visible bug.

---

Finding 1 — HIGH: update_network returns HTTP 500, not the declared 409, for paused / not-active sandboxes

CubeAPI/src/services/sandboxes.rs:522 — the new service method maps errors through sandbox_not_found_or_internal, which special-cases only 404 (is_not_found) and 400 (is_params_error) and converts everything else to internal_error → HTTP 500.

The conflict path is real and reachable:

• Cubelet returns ErrorCode_Conflict (130409) for a paused sandbox or a sandbox with no active network (Cubelet/services/cubebox/update.go — updateNetworkPolicy).
• CubeMaster copies the Cubelet ret code through verbatim (updateSandboxNetworkOnNode: result.RetCode = int(cubeRsp.GetRet().GetRetCode())).
• CubeAPI's parse_response turns ret_code 130409 into CubeMasterError::Api { ret_code: 130409, .. }, and sandbox_not_found_or_internal then classifies it as a 500.

Meanwhile every consumer-facing contract promises 409:

• utoipa annotation: (status = 409, description = "Sandbox is not running", body = ApiError);
• examples/network-policy/README.md troubleshooting table: "update_network returns 409 | Sandbox is paused or already gone";
• Go SDK docstring (sdk/go/sandbox.go): "including 409 when the sandbox is not running".

The sibling update/delete/create paths already solve this with ensure_update_result / map_update_cubemaster_err / ensure_create_result, all of which map RET_CODE_CONFLICT → AppError::Conflict. The fix is to route update_network's response through one of those (e.g. ensure_update_result) instead of sandbox_not_found_or_internal. Inline comment on sandboxes.rs:522.

---

Finding 2 — MEDIUM: removing the 80/443 drain block also resets permitted connections whose session was lost

CubeNet/src/mvmtap.bpf.c:802 — the "Legacy default-port (80/443) connection drain" block is deleted. Removing it is required for the feature: with it in place, a revoked L7 flow whose TPROXY socket is still ESTABLISHED would be kept alive by mark re-stamping, defeating revocation entirely.

But the old block was also the only safeguard for the non-revocation session-loss case its comment documented ("agent restart, map eviction, or expiry"). For a connection the current policy still allows whose session entry was lost — agent/CubeNet restart that drops session state, or reaper expiry of a silent-but-live connection — the old path re-stamped the mark and kept the connection draining; the new path answers the next packet with RST even though the proxy socket is healthy and the flow is still permitted. The PR docs describe only the revocation semantics, not this consequence.

Two ways to close the gap: document the trade-off explicitly, or narrow the drain to flows the current policy still allows by re-classifying under the new policy before deciding to reset. Inline comment on mvmtap.bpf.c:802.

---

Finding 3 — MEDIUM (doc accuracy): PR description claims a "blocked / 10s reaper timeout" that does not exist

CubeNet/cubevs/reaper.go:170 — the PR body says "the reaper gives blocked sessions a 10s timeout rather than waiting out TCP Established, which is three hours" and describes sessions as "marked blocked". None of that is implemented:

• this PolicyVersion struct-field addition is the only change to reaper.go;
• there is no blocked marker on nat_session and no 10s timeout anywhere;
• revoked sessions are deleted outright at the datapath (del_session), the guest is reset immediately, and any retransmit keeps getting reset.

The committed docs (docs/guide/network-policy.md) describe the delete-outright behavior correctly, so this is a stale PR-description issue rather than a code bug — but the "blocked / reaper" framing could mislead reviewers and future maintainers about how retirement actually works. Inline comment on reaper.go:170.

---

Finding 4 — LOW (known-limitation gap): the re-check is IP-granular, so domain revocation is imprecise when IPs are shared

CubeNet/src/session.h:227 — session_policy_revoked re-classifies with classify_egress_flow(ifindex, daddr, dport), i.e. the destination IP only. If a revoked domain's IP is still allowed because another rule — or another DNS-learned domain — covers the same IP, an established flow to that IP survives the revocation indefinitely. The docs' "known limitation" acknowledges that DNS-learned IPs outlive the rule that created them (until TTL expiry), but the shared-IP case is distinct: the flow survives beyond TTL because the IP remains allowed under a different entry. A sentence in the known-limitation note would let users plan around it. Inline comment on session.h:227.

---

Verified correct (no action needed)

• Struct layout compatibility. mvm_meta stays 128 B with policy_version at offset 76; nat_session stays 64 B with policy_version at offset 32; Go mirrors (mvmMetadata, natSession) match field-for-field including padding. Pinned-map layout is unchanged, so the upgrade path (pre-existing sessions with policy_version == 0 read as stale and re-check once) is coherent.
• NULL safety in the datapath. All callers of do_tcp_nat / do_udp_nat_inline / do_icmp_nat NULL-check the mvm_meta lookup before passing it, so session_policy_revoked's dereference is safe. create_nat_session's stamping also NULL-checks; a missing meta leaves 0, which reads as stale — the safe direction.
• Revocation is self-enforcing. del_session removes both the egress and ingress halves, so replies stop via lookup_session, the 4-tuple is immediately reusable, and a SYN reconnects under the new policy.
• Ordering contract. UpdateTAPDevicePolicy diffs against the live maps (no flush-window, no inner-map swap), bumps the generation last, and deletes before inserting within each map; NetworkController.UpdateNetworkPolicy follows CubeEgress → CubeVS → persist. setDNSPolicyFlags is a read-modify-write that preserves PolicyVersion. syncDNSAllowInner writes desired values as-is, so shrinking a domain's port set actually shrinks (the create path's union helper would not have).
• L3-only skip. syncEgressPolicy leaves CubeEgress untouched when both want and installed are nil, so an update that never touches L7 doesn't depend on the proxy.
• DNS resolver fold-back. Resolver CIDRs are recorded unconditionally at create and re-folded into an update only when the policy still names a domain, with a documented one-time fallback for legacy sandboxes.
• SYN path. Retransmitted SYNs now route through do_recheck, and the L7 no-recreate guard for terminal sessions is preserved.

Minor notes

• persistSandboxNetworkSpec (CubeMaster) is best-effort by design; a failed spec write is logged and the call still succeeds, so a clone taken right after that failure inherits the older policy. The code comment acknowledges this, and the e2e "clone inherits updated policy" test only covers the success path.
• Re-evaluation is traffic-driven: replies continue to be delivered until the guest next sends on the flow, even after a bump. Documented, and consistent with the "driven by traffic, not pushed" framing.

Overall

Well-architected change with unusually good test coverage for a datapath feature. The 409/500 mismatch (Finding 1) is the one genuine client-visible bug and should be fixed before merge; Findings 2–4 are trade-offs / doc gaps worth a decision and a sentence or two in the docs.

[cubesandboxbot]

409 never reaches clients — a paused/not-active sandbox update surfaces as HTTP 500.

update_network maps errors through sandbox_not_found_or_internal, which special-cases only 404 (is_not_found) and 400 (is_params_error) and turns everything else into internal_error → HTTP 500. A Cubelet ErrorCode_Conflict (130409) — returned here for a paused sandbox or a sandbox with no active network — passes through CubeMaster verbatim (updateSandboxNetworkOnNode copies cubeRsp.Ret.RetCode straight into the response), reaches this .map_err, and is misclassified as a 500.

But the contract promises 409:

• the utoipa annotation declares (status = 409, description = "Sandbox is not running");
• examples/network-policy/README.md troubleshooting table: "update_network returns 409 | Sandbox is paused or already gone";
• the Go SDK docstring: "including 409 when the sandbox is not running".

The sibling update/delete paths avoid exactly this by using ensure_update_result / map_update_cubemaster_err / ensure_create_result, all of which map RET_CODE_CONFLICT (130409) → AppError::Conflict. Suggest routing this call through ensure_update_result (or map_update_cubemaster_err) so paused / not-active sandboxes actually return 409.

[cubesandboxbot]

PR description misdescribes the retirement mechanism — the reaper plays no part.

The PR body claims "the reaper gives blocked sessions a 10s timeout rather than waiting out TCP Established, which is three hours" and describes sessions as "marked blocked". None of that exists in the code: this struct change (adding PolicyVersion) is the only change to reaper.go, there is no blocked marker on nat_session, and there is no 10s timeout anywhere. Revoked sessions are deleted outright at the datapath (del_session in mvmtap.bpf.c) — the flow is reset immediately, and any guest retransmit finds no session and is reset again.

The committed docs (docs/guide/network-policy.md) describe the delete-outright behavior correctly, so this is a stale description in the PR body rather than a code bug — but the "blocked / 10s reaper timeout" framing could mislead reviewers and future maintainers about how retirement actually works, and the "blocked" terminology is now inconsistent with the implementation.

[cubesandboxbot]

Behavior change beyond revocation: legitimate L7 connections whose session was lost are now reset instead of drained.

Removing the 80/443 drain block is necessary for revocation to work — with it in place, a revoked L7 flow whose TPROXY socket is still ESTABLISHED would be kept alive by mark re-stamping, defeating the whole feature. But the old block was also the only safeguard for the non-revocation session-loss case its comment documented: "agent restart, map eviction, or expiry". For a connection the current policy still allows, whose session entry was lost (agent/CubeNet restart that drops session state, or reaper expiry of a silent-but-live connection), the old path re-stamped the mark and kept the connection draining; the new path answers the next packet with RST even though the proxy socket is healthy and the flow is permitted.

This consequence isn't called out anywhere in the PR docs (which describe only the revocation semantics). Two options worth considering: document the trade-off explicitly, or narrow the drain to flows the current policy still allows by re-classifying under the new policy before deciding to reset.

[cubesandboxbot]

Re-check is IP-granular, so domain revocation is imprecise when IPs are shared.

session_policy_revoked re-classifies using only the destination IP (classify_egress_flow(ifindex, daddr, dport)). If a revoked domain's IP is still allowed because another rule — or another DNS-learned domain — covers the same IP, an established flow to that IP survives the revocation even though the specific domain was removed. The docs' "known limitation" acknowledges that DNS-learned IPs outlive the rule that created them (until TTL expiry), but the shared-IP case is a distinct consequence: the flow survives indefinitely, not just until TTL, because the IP remains allowed under a different entry. Worth one sentence in the known-limitation note so users understand that domain-level revocation is best-effort at the IP layer.