Skip to content

fix: resolve run-path conflist and sweep leaked masquerade chains on CNI purge - #1327

Merged
eminwux merged 1 commit into
mainfrom
fix/cni-purge-conflist-runpath-fallback
Jun 16, 2026
Merged

fix: resolve run-path conflist and sweep leaked masquerade chains on CNI purge#1327
eminwux merged 1 commit into
mainfrom
fix/cni-purge-conflist-runpath-fallback

Conversation

@eminwux

@eminwux eminwux commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

Purging a CNI-networked cell (directly or via cascade realm purge) left the bridge plugin's per-container CNI-<hash> masquerade chain and its POSTROUTING -s <podIP> jump stranded in the host nat table, accumulating on every networked-cell purge (issue #1324). This PR fixes it in three parts:

  1. findCNIConfigPath run-path fallback (helpers.go). The documented-but-unimplemented fallback now resolves the per-space conflist at <RunPath>/<metadata>/<realm>/<space>/network.conflist. Space conflists are never written as <networkName>.conflist under CniConfigDir, so the old lookup always missed for space networks and the CNI DEL was silently skipped. Resolution enumerates realm/space dirs and matches by the recomputed network name (robust to - in realm/space names, unlike reverse-splitting <realm>-<space>). Keeps the signature unchanged so all ~15 purgeCNIForContainer callers benefit at once — lowest blast radius.

  2. Surface skipped/failed CNI DEL (runCNIDel in helpers.go). Every non-success DEL path (conflist not found, manager build error, config-list load error, DEL failure) now logs at Warn (was Debug or fully swallowed) and records its outcome (cni-del-skipped:* / cni-del-failed) in the purge result instead of reporting a clean purge.

  3. Belt-and-suspenders masquerade-chain sweep (sweepCNIMasqChain in helpers.go). After the DEL, directly remove the deterministic CNI-<FormatChainName(network, containerID)> chain and its POSTROUTING jumps. See the premise-rot note below — this turned out to be required, not optional.

Premise-rot finding (documented per dev CLAUDE.md)

The issue diagnosed the leak as purely a conflist-lookup miss (fix #1) and listed the chain sweep (#3) as an optional "consider". Validating fix #1 on-device surfaced a deeper root cause: even when the DEL runs and returns success (purged=[cni-del]), the masquerade chain still leaks on the purge path. The bridge plugin's cmdDel (containernetworking/[email protected] bridge.go:788-789) early-returns after the IPAM release when invoked with an empty netns — never reaching its TeardownIPMasqForNetworks call. On the purge path the container task is already gone, so the DEL always runs with an empty netns. I confirmed live: a live-netns DEL (kuke stop) cleans the chain; the empty-netns DEL (kuke purge) does not. So fix #1+#2 satisfy AC #2 but cannot satisfy AC #1 alone — the sweep (#3) is the only thing that reclaims the chain when there is no live netns. The underlying intent (no leaked nat rules after purge) is unchanged, so I implemented #3 as a core part of the fix rather than stalling.

Design calls (for reviewer pushback)

  • Shell out to iptables rather than import containernetworking/plugins. ip.TeardownIPMasqForNetworks would be the exact teardown, but plugins is not a current dependency and would pull a new closure (plugins + coreos/go-iptables) for three commands. The project's established convention (internal/netpolicy.IptablesEnforcer) shells out to iptables via exec.Command; the sweep matches that. Adopting a new dependency is a call I avoided making unilaterally.
  • Deterministic chain-name replica. cniMasqChainName reproduces utils.FormatChainName ("CNI-" + hex(sha512(network+containerID))[:24]). Verified byte-for-byte against two live-created chains in TestCNIMasqChainName (golden values), including the issue's exact CNI-ec12256cec4057b1122e3662.
  • Jump removal by line number, parsed from iptables -L POSTROUTING --line-numbers -n and deleted descending — robust to the rule's source IP and comment text, no IP capture or comment-string matching required.

Test plan

  • make test (full CI suite: test-root + test-kukebuild) — pass
  • pre-commit run --files <changed> — pass (golangci-lint incl. gochecknoglobals nolint, gofmt, addlicense)
  • New unit tests (internal/controller/runner/helpers_test.go):
    • TestFindCNIConfigPath_ResolvesRunPathConflist — run-path fallback resolves the per-space conflist (realm name with - exercises no-reverse-split)
    • TestPurgeCNIForContainer_RunPathConflistRunsDEL — DEL fires via the run-path conflist and records cni-del (fake CNI plugin)
    • TestPurgeCNIForContainer_SurfacesSkippedDEL — skipped DEL surfaces at Warn + cni-del-skipped:config-not-found
    • TestCNIMasqChainName — chain-name replica matches live chains (golden)
    • TestSweepCNIMasqChain / _NoChainNoMarker — sweep deletes jumps (descending) + flush/delete chain; no false marker when chain absent
    • TestMain default-stubs the iptables shell-out so no unit test touches the host firewall
  • On-device end-to-end (KUKEON_PROFILE=dev make dev-init, exit 0):
    • Clean cell create→start→purge (natleak3): chain CNI-fa5b2f0a... + POSTROUTING jump fully reclaimed; daemon log: purged="[cni-del cni-masq-swept:CNI-fa5b2f0a6a8eeb62a3411ecd]"
    • Cascade realm purge (dev-init attach-smoke dev-init-attach/ds/dks/cattach): the issue's exact persistent leak CNI-ec12256cec4057b1122e3662 swept; iptables -t nat -S shows no dev-init-attach residue
    • Daemon-parity tail + attach smoke pass

Acceptance criteria

  • After kuke purge (cell, and cascade realm) of a CNI-networked cell, iptables -t nat -S contains no residual CNI-<hash> chain or POSTROUTING -s <podIP> -j CNI-<hash> rule for the purged cell — validated on-device for both paths.
  • The purge log/result reflects a real CNI DEL (cni-del in the purged list) for networked cells, and a failed/skipped DEL is surfaced above Debug (Warn).
  • A regression test covers the resolution + sweep (iptables-free unit tests asserting findCNIConfigPath resolves the run-path conflist, purgeCNIForContainer records cni-del, and the masquerade-chain sweep issues the expected delete/flush/delete sequence), plus on-device validation of the full create→purge→no-leaked-nat-rule cycle.

Out of scope

Closes #1324

@eminwux eminwux added ready-for-review PR is handed off to the reviewer agent in-review Reviewer agent is actively reading this PR and removed ready-for-review PR is handed off to the reviewer agent labels Jun 16, 2026
@eminwux

eminwux commented Jun 16, 2026

Copy link
Copy Markdown
Owner Author

PR #1327 Review — fix: resolve run-path conflist and sweep leaked masquerade chains on CNI purge

Correct and well-tested. The three-part fix (run-path conflist fallback → surfaced DEL outcomes → masquerade-chain sweep) is sound: the per-container chain name hashes network+containerID, so a sweep can never touch a sibling container's chain or jump; descending line-number deletion avoids renumbering; the Atoi guard correctly skips header rows; and cniMasqChainName is byte-for-byte utils.FormatChainName, pinned with golden values. The premise-rot escalation to make the sweep mandatory is well-justified and validated on-device for both the cell-purge and cascade-realm-purge paths. dev-init smoke is in the test plan, all commits are signature-verified, no dead code, no leaks.

One non-blocking nit:

  • Stale comment internal/controller/runner/helpers.go:194-197: the retained runCNIDel comment still claims the netns-optional DEL "reconstructs teardown (including the bridge plugin's ipMasq iptables chains/rules) from the cached CNI result" — but this PR's premise-rot finding establishes the bridge plugin early-returns before ipMasq teardown on an empty netns, which is exactly why the sweep exists. Worth a one-line correction so a future reader isn't re-misled. The code is correct as-is.

LGTM to proceed — the nit is non-blocking.

@eminwux eminwux added ready-to-merge Reviewed and ready to merge and removed in-review Reviewer agent is actively reading this PR labels Jun 16, 2026
@eminwux
eminwux merged commit b773fd2 into main Jun 16, 2026
5 checks passed
@eminwux
eminwux deleted the fix/cni-purge-conflist-runpath-fallback branch June 16, 2026 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Reviewed and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cni: per-cell masquerade nat rules leak on cell/realm purge — findCNIConfigPath resolves the wrong conflist path, CNI DEL is silently skipped

1 participant