fix(network-agent): retain taps until cleanup and pool prep complete#801
fix(network-agent): retain taps until cleanup and pool prep complete#801FakeLearne wants to merge 3 commits into
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
| } | ||
| } | ||
| s.stageTapForPoolLocked(restoredTap, "recover") | ||
| if err := s.prepareAndStageTapForPool(context.Background(), restoredTap, "recover"); err != nil { |
There was a problem hiding this comment.
Stale eBPF allow entries may survive when restoring taps not in CubeVS on restart
When inCubeVS is false, recover() skips the cleanup block and calls prepareAndStageTapForPool directly, which only calls PrepareTAPDevicePolicy — it does not flush existing eBPF map entries. If the agent crashed mid-cleanup (after some entries were flushed but before cubevsDelTAPDevice completed), stale allow entries from the previous sandbox survive in allow_out_v2. A new sandbox using this ifindex via the non-replace applyNetPolicy path will inherit them, potentially allowing traffic that its policy intended to block.
Consider calling cleanupNetPolicy(ifindex) (or flushing both maps) before re-preparing taps that lack CubeVS metadata on restart.
| return | ||
| } | ||
| closeTapFile(tap.File) | ||
| _ = destroyTapFunc(tap.Index) |
There was a problem hiding this comment.
destroyTapFunc error silently discarded on non-pool tap release
In a PR that otherwise tightens cleanup rigor, _ = destroyTapFunc(tap.Index) silently discards the destruction error. If the host-side tap device cannot be destroyed, the IP is still released and could be re-allocated for a new tap while the old device still exists on the host. Consider logging this error and/or returning it so the caller can decide whether to retry.
| return cubevsPrepareTAPPolicy(uint32(tap.Index)) | ||
| } | ||
|
|
||
| func (s *localService) prepareAndStageTapForPool(ctx context.Context, tap *tapDevice, reason string) error { |
There was a problem hiding this comment.
TOCTOU window: lock gap between prepare and stage
s.mu is released between prepareTapForPool (which does eBPF syscalls) and stageTapForPoolLocked. Under concurrent sandbox teardown+creation, another goroutine calling acquireTap can find the pool temporarily empty and create a fresh tap from scratch, defeating pool efficiency and potentially wasting the preparation work done here.
Consider staging the tap into the pool first (under lock, which is fast) and then doing the eBPF preparation asynchronously via the abnormal pool — or document this intentional trade-off.
| allowOutEntries []allowOutPolicyEntry | ||
| dnsAllowRules []dnsAllowRule | ||
| denyOutEntries []denyOutPolicyEntry | ||
| includeDefaultDenyOut bool |
There was a problem hiding this comment.
No need for this bool flag.
| } | ||
| } | ||
| s.stageTapForPoolLocked(restoredTap, "recover") | ||
| if err := s.prepareAndStageTapForPool(context.Background(), restoredTap, "recover"); err != nil { |
There was a problem hiding this comment.
Potential IP resource leak when quarantine is reached
When recover() fails to stage a tap (line 790) or handleAbnormalTaps reaches the quarantine threshold (line 361-363), the IP allocated by s.allocator.Assign at line 741 is never released. After maxAbnormalRecoveryAttempts (3), the tap moves to quarantinedTaps and the IP leaks for the process lifetime.
A persistent eBPF failure on a single tap can slowly exhaust the sandbox IP pool. Consider releasing the IP before quarantining, e.g. s.allocator.Release(tap.IP) at lines 362-363 and at line 793 before the requeue call.
| s.clearPortMappings(staleTap), | ||
| s.cleanupCubeVSTap(device.Ifindex, staleTap.IP), | ||
| ) | ||
| if cleanupErr != nil { |
There was a problem hiding this comment.
Hard failure on stale state cleanup can block node startup
If cleanup of a stale persisted state fails (transient eBPF error, etc.), recover() returns the error to NewLocalService, preventing the agent from starting at all. Every other cleanup failure in this function correctly enqueues to the abnormal pool and continues.
Consider making this best-effort: log the error, quarantine the ifindex, and continue recovery of other states. Otherwise a single corrupted eBPF entry can indefinitely wedge the node.
| HostPort: int32(hostPort), | ||
| ContainerPort: mapping.ContainerPort, | ||
| }) | ||
| tap.PortMappings = append([]PortMapping(nil), actualMappings...) |
There was a problem hiding this comment.
O(N²) copy: tap.PortMappings re-allocated on every loop iteration
This creates a new backing array and copies the entire actualMappings slice once per port mapping. For N mappings, N(N+1)/2 elements are copied (e.g., 10 ports → 55 elements). Consider moving this assignment to after the loop when all mappings have been built successfully.
| } | ||
| denyOutEntries = appendDenyOutPolicyEntries(denyOutEntries, alwaysDeniedSandboxEntries) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Hard to trace err reuse across conditional branches
err is written in one branch (AllowInternetAccess=false → line 338) and checked after the conditional (line 347). If AllowInternetAccess is nil, err retains whatever value it had from line 328, which should be nil after the check at line 329 but requires tracing three possible sources to verify. Using local-scoped err variables or an explicit err = nil before the outer if would make this self-documenting.
Review: PR #801 — fix(network-agent): retain taps until cleanup and pool prep completeOverviewThis PR addresses a real correctness issue: TAP devices with incomplete cleanup should not be recycled, as stale eBPF allow/deny state could leak into a different sandbox that reuses the same ifindex. The architectural change is sound — moving The code is well-structured, the concurrency tests are thorough, and the error-sentinel pattern ( Medium: IP resource leak when taps are quarantinedFile: When a tap fails recovery and hits the quarantine threshold ( On a node with persistently failing taps (e.g., hardware issues), this slowly exhausts the sandbox IP pool. Add Inline comment posted. Medium: Recover() hard-fails on stale-state cleanup, risking startup failureFile: The stale-state cleanup loop in Make this best-effort: log the error, quarantine the ifindex, and continue recovering other states. A transient eBPF failure should not prevent the agent from starting. Inline comment posted. Medium: O(N²) copy in
|
| HostPort: int32(hostPort), | ||
| ContainerPort: mapping.ContainerPort, | ||
| }) | ||
| tap.PortMappings = append([]PortMapping(nil), actualMappings...) |
There was a problem hiding this comment.
O(N²) copy: tap.PortMappings re-allocated on every loop iteration
This creates a new backing array and copies the entire actualMappings slice once per port mapping. For N mappings, N(N+1)/2 elements are copied (e.g., 10 ports → 55 elements). Consider moving the assignment to after the loop when all mappings have been built successfully.
Keep TAP devices out of the reusable pool when sandbox cleanup fails. Port mappings and CubeVS policy metadata must be cleaned successfully before a TAP can be recycled, so stale allow/deny state cannot leak into a later sandbox that reuses the same ifindex. Move the default private/link-local deny ranges out of the sandbox creation hot path. TAPs now receive those invariant deny_out entries when they are initialized or asynchronously prepared for the pool, while replace/recover paths still replay them after flushing policy maps. Signed-off-by: yarrischen <yarrischen@tencent.com>
Remove the includeDefaultDenyOut branch and treat the private/link-local deny ranges as baseline TAP policy in all paths. Replace/flush now always replays those default deny entries, including deny-all sandboxes, keeping create and replace policy shapes consistent while simplifying the policy plan logic. Signed-off-by: yarrischen <yarrischen@tencent.com>
Make TAP recycling safer by ensuring a TAP is cleaned and prepared before it is returned to the available pool. On the normal sandbox delete path, prepare the TAP synchronously and only move it to the abnormal retry queue if preparation fails. Also clear per-sandbox net policy and DNS allow state before replaying default deny entries during TAP pool preparation, so restarted network-agent instances do not reuse TAPs with stale policy residue. Restore the HostPort ownership check when rolling back failed port map installation, so only dynamically allocated host ports are released back to the allocator. Signed-off-by: yarrischen <yarrischen@tencent.com>
FakeLearne
force-pushed
the
fix/tap-cleanup-retain-on-failure
branch
from
ff4a5ed to
f5d301d
Compare
3 participants
Keep TAP devices out of the reusable pool when sandbox cleanup fails. Port mappings and CubeVS policy metadata must be cleaned successfully before a TAP can be recycled, so stale allow/deny state cannot leak into a later sandbox that reuses the same ifindex.
Move the default private/link-local deny ranges out of the sandbox creation hot path. TAPs now receive those invariant deny_out entries when they are initialized or asynchronously prepared for the pool, while replace/recover paths still replay them after flushing policy maps.