[PR #100] [MERGED] fix(network): periodic reconcile of published-port DNAT (self-heal drift) #101

New issue

Closed

opened 2026-10-15 12:52:01 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 12:52:01 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/games-on-whales/LXC2Docker/pull/100
Author: @JBailes
Created: 6/29/2026
Status: ✅ Merged
Merged: 6/29/2026
Merged by: @JBailes

Base: main ← Head: fix/dnat-periodic-reconcile

📝 Commits (1)

15794a6 fix(network): periodic reconcile of published-port DNAT (self-heal drift)

📊 Changes

3 files changed (+343 additions, -0 deletions)

View changed files

📝 internal/lxc/manager.go (+147 -0)
📝 internal/lxc/network.go (+142 -0)
📝 internal/lxc/network_portforward_test.go (+54 -0)

📄 Description

Problem

Published-port DNAT rules (<proto> dport <hostPort> dnat to <containerIP>:<containerPort> in table veth_nat, chains prerouting + output) are keyed to a container's bridge IP, which is allocated dynamically and changes on recreation. Event-driven publishing (StartContainer) plus the startup reconcile() cover the normal paths, but any drift between events went unhealed until a daemon restart:

an external nft flush / table rebuild,
a rule that was never (re)applied on some start path,
a container that came back on a new IP.

This is the recurring host-port DNAT fragility seen in production (e.g. the gateway -> host:8741 forward that kept breaking whenever the dependent container was recreated, requiring a manual refresh-containers).

Fix

Add a declarative port-forward reconcile to the existing 30s network-reconciler tick (StartNetworkReconciler), alongside the bridge/syncHosts re-apply it already does.

CurrentPortForwards reads both the prerouting and output chains (AddPortForward writes both) and counts a forward "correct" only when it agrees across both chains with no duplicate rules. Any disagreement or duplicate is treated as drift and re-applied. It returns an error so a transient nft read failure skips the tick rather than churning the whole table; a genuinely missing table is treated as empty (heal).
reconcilePortForwards heals missing/stale/duplicated rules (mismatch-only — a correct, active rule is never delete+re-added, so live streams aren't dropped) and prunes orphan host ports that no running container claims, so a rule resurrected by a stop/reconcile race can't linger pointing at a freed (possibly reused) IP.
Prune uses 2-tick hysteresis and protects containers whose state can't be read, so the brief starting window or a transient State() error can never delete a live rule. A host port claimed by two containers is logged and left alone rather than driven into evict/re-add churn.
nftMu serializes the multi-step DNAT mutations (AddPortForward / RemovePortForwards / RemovePortForwardForHostPort) so the reconciler and the API lifecycle handlers can't interleave their phases.

Testing

go build ./..., go vet ./..., go test ./... all green.
New unit tests: TestParseDNATChain (dual-rule duplicate detection across the chain) and TestParseHostPortKey.
Reviewed across three adversarial review rounds; the dual-chain coverage, duplicate handling, mutation serialization, and the prune-during-starting/State()-error TOCTOU were each surfaced and addressed before this PR.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/games-on-whales/LXC2Docker/pull/100 **Author:** [@JBailes](https://github.com/JBailes) **Created:** 6/29/2026 **Status:** ✅ Merged **Merged:** 6/29/2026 **Merged by:** [@JBailes](https://github.com/JBailes) **Base:** `main` ← **Head:** `fix/dnat-periodic-reconcile` --- ### 📝 Commits (1) - [`15794a6`](https://github.com/games-on-whales/LXC2Docker/commit/15794a6837137e973898cf94ee28f0491c923038) fix(network): periodic reconcile of published-port DNAT (self-heal drift) ### 📊 Changes **3 files changed** (+343 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `internal/lxc/manager.go` (+147 -0) 📝 `internal/lxc/network.go` (+142 -0) 📝 `internal/lxc/network_portforward_test.go` (+54 -0) </details> ### 📄 Description ## Problem Published-port DNAT rules (`<proto> dport <hostPort> dnat to <containerIP>:<containerPort>` in table `veth_nat`, chains `prerouting` + `output`) are keyed to a container's **bridge IP**, which is allocated dynamically and **changes on recreation**. Event-driven publishing (`StartContainer`) plus the startup `reconcile()` cover the normal paths, but any **drift between events** went unhealed until a daemon restart: - an external `nft flush` / table rebuild, - a rule that was never (re)applied on some start path, - a container that came back on a new IP. This is the recurring host-port DNAT fragility seen in production (e.g. the `gateway -> host:8741` forward that kept breaking whenever the dependent container was recreated, requiring a manual `refresh-containers`). ## Fix Add a **declarative** port-forward reconcile to the existing 30s network-reconciler tick (`StartNetworkReconciler`), alongside the bridge/`syncHosts` re-apply it already does. - **`CurrentPortForwards`** reads **both** the `prerouting` *and* `output` chains (`AddPortForward` writes both) and counts a forward "correct" only when it agrees across both chains with **no duplicate rules**. Any disagreement or duplicate is treated as drift and re-applied. It returns an error so a *transient* nft read failure **skips the tick** rather than churning the whole table; a genuinely missing table is treated as empty (heal). - **`reconcilePortForwards`** heals missing/stale/duplicated rules (**mismatch-only** — a correct, active rule is never delete+re-added, so live streams aren't dropped) and **prunes orphan** host ports that no running container claims, so a rule resurrected by a stop/reconcile race can't linger pointing at a freed (possibly reused) IP. - Prune uses **2-tick hysteresis** and **protects containers whose state can't be read**, so the brief `starting` window or a transient `State()` error can never delete a live rule. A host port claimed by two containers is logged and left alone rather than driven into evict/re-add churn. - **`nftMu`** serializes the multi-step DNAT mutations (`AddPortForward` / `RemovePortForwards` / `RemovePortForwardForHostPort`) so the reconciler and the API lifecycle handlers can't interleave their phases. ## Testing - `go build ./...`, `go vet ./...`, `go test ./...` all green. - New unit tests: `TestParseDNATChain` (dual-rule duplicate detection across the chain) and `TestParseHostPortKey`. - Reviewed across three adversarial review rounds; the dual-chain coverage, duplicate handling, mutation serialization, and the prune-during-`starting`/`State()`-error TOCTOU were each surfaced and addressed before this PR. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-15 12:52:01 +00:00