From dbbc0564e38d3a48053c651baf5a7d6d37cc6f7c Mon Sep 17 00:00:00 2001 From: chen21019 Date: Sat, 12 Sep 2026 21:28:59 +0800 Subject: [PATCH] Keep overlay NAT inside the network manager across xtables backends --- COMPATIBILITY.md | 7 +++++ README.md | 12 +++++--- hostnat/firewall_test.go | 10 +++++++ hostnat/iptables_vm_test.go | 47 ++++++++++++++++++++++++++---- hostnat/watcher.go | 8 +++-- hostports/iptables_vm_test.go | 55 +++++++++++++++++++++++++---------- 6 files changed, 111 insertions(+), 28 deletions(-) diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 627f48f..b632cd3 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -45,6 +45,13 @@ under their own change control before enabling Docker native nftables; this component never auto-imports or silently deletes such rules. The retained metadata network schema and host-port rules are IPv4-only. +The host NAT and host-port `CATTLE_*` chains are owned exclusively by this +manager. Its general masquerade rules exclude the managed overlay subnet on +native nftables, iptables-nft, and iptables-legacy. The IPsec host-XFRM router +owns routes and XFRM state, not these chains. Upgrade this manager first, +confirm its reconciliation and source-preserving egress, then upgrade the +router; do not rely on a second plugin to patch a manager-owned chain. + For the v0.8.13 upgrade case where Docker uses `iptables-nft` but a previous manager left `CATTLE_*` hooks in a loaded legacy NAT table, use the dedicated `iptables-legacy` frontend for inspection; the generic `iptables` alternative diff --git a/README.md b/README.md index 47e5160..35ceeef 100644 --- a/README.md +++ b/README.md @@ -84,10 +84,14 @@ hook or an uninspectable loaded table. The manager writes only to Docker's selected backend and never silently removes old hooks. See the bounded cleanup steps in [COMPATIBILITY.md](COMPATIBILITY.md). -Native host NAT excludes destinations within each network's configured -`bridgeSubnet` from its general masquerade rules, preserving overlay source -addresses for same-subnet peer traffic. The IKE SNAT and legacy xtables rules -are unchanged; cross-host behavior still needs deployment-level validation. +Host NAT in all three backends excludes destinations within each network's +configured `bridgeSubnet` from its general masquerade rules, preserving the +source address for same-subnet overlay traffic. This component alone owns its +host NAT and host-port chains; the IPsec router must not insert bypasses or +forwarding rules into them. IKE SNAT and container-namespace compatibility +rules are separate. Upgrade this manager and verify it is healthy before +upgrading the IPsec router that no longer writes a compensating host bypass. +Cross-host behavior still requires deployment-level validation. The image healthcheck waits until both the host NAT and host-port watchers have successfully reconciled current metadata. A transient metadata delay diff --git a/hostnat/firewall_test.go b/hostnat/firewall_test.go index 269fbd5..de9e6b5 100644 --- a/hostnat/firewall_test.go +++ b/hostnat/firewall_test.go @@ -11,6 +11,16 @@ import ( "github.com/PastureStack/network-plugin-manager/internal/firewall" ) +func TestXTMasqueradePreservesSameSubnetSource(t *testing.T) { + script := string((MASQRule{Subnet: "10.42.0.0/16", Bridge: "pst0"}).iptables()) + if got := strings.Count(script, "-s 10.42.0.0/16 ! -d 10.42.0.0/16 ! -o pst0"); got != 3 { + t.Fatalf("expected all three xtables masquerade rules to exclude same-subnet destinations, got %d: %s", got, script) + } + if strings.Count(script, "-j MASQUERADE") != 4 { + t.Fatalf("expected three egress and one local-routing masquerade rule: %s", script) + } +} + func TestNATRejectsMalformedMetadataBeforeMutation(t *testing.T) { cases := map[string]ruleSet{ "global-subnet": {MASQ: map[string]MASQRule{"n": {Subnet: "0.0.0.0/0", Bridge: "pst0"}}}, diff --git a/hostnat/iptables_vm_test.go b/hostnat/iptables_vm_test.go index 71bbe39..f83be06 100644 --- a/hostnat/iptables_vm_test.go +++ b/hostnat/iptables_vm_test.go @@ -14,19 +14,41 @@ import ( // Opt-in root test for an isolated VM. It refuses to run when the production // chain/hook already exists and removes only what this test creates. func TestIptablesNFTBatchOnVM(t *testing.T) { + testIptablesBatchOnVM(t, firewall.IptablesNFT) +} + +func TestIptablesLegacyBatchOnVM(t *testing.T) { + testIptablesBatchOnVM(t, firewall.IptablesLegacy) +} + +func testIptablesBatchOnVM(t *testing.T, mode firewall.Mode) { if os.Getenv("PASTURESTACK_IPTABLES_VM_TEST") != "1" { t.Skip("set PASTURESTACK_IPTABLES_VM_TEST=1 on an isolated root VM") } if os.Geteuid() != 0 { t.Fatal("iptables integration test requires root") } - for _, name := range []string{"iptables-nft", "iptables-nft-restore"} { + command, restore := string(mode), string(mode)+"-restore" + for _, name := range []string{command, restore} { if _, err := exec.LookPath(name); err != nil { t.Fatal(err) } } + if out, err := exec.Command("docker", "info", "--format", "{{.FirewallBackend.Driver}}").CombinedOutput(); err != nil || strings.TrimSpace(string(out)) != "iptables" { + t.Fatalf("requires Docker iptables backend: %v: %s", err, out) + } + if out, err := exec.Command(command, "-t", "nat", "-S", "DOCKER").CombinedOutput(); err != nil { + t.Fatalf("requires Docker-owned NAT chain in %s: %v: %s", mode, err, out) + } + other := "iptables-nft" + if mode == firewall.IptablesNFT { + other = "iptables-legacy" + } + if out, err := exec.Command(other, "-t", "nat", "-S", "DOCKER").CombinedOutput(); err == nil { + t.Fatalf("refusing dual Docker backends; %s also owns NAT: %s", other, out) + } iptables := func(args ...string) ([]byte, error) { - return exec.Command("iptables-nft", args...).CombinedOutput() + return exec.Command(command, args...).CombinedOutput() } postrouting, err := iptables("-t", "nat", "-S", "POSTROUTING") if err != nil { @@ -38,6 +60,17 @@ func TestIptablesNFTBatchOnVM(t *testing.T) { if bytes.Contains(postrouting, []byte("-j "+natChain)) { t.Fatalf("refusing to modify pre-existing %s hook", natChain) } + const bridge = "docker0" + setting := "net.ipv4.conf." + bridge + ".route_localnet" + previous, err := exec.Command("sysctl", "-n", setting).CombinedOutput() + if err != nil { + t.Fatalf("read %s before test: %v: %s", setting, err, previous) + } + t.Cleanup(func() { + if out, err := exec.Command("sysctl", "-w", setting+"="+strings.TrimSpace(string(previous))).CombinedOutput(); err != nil { + t.Errorf("restore %s: %v: %s", setting, err, out) + } + }) owned := false t.Cleanup(func() { @@ -70,9 +103,9 @@ func TestIptablesNFTBatchOnVM(t *testing.T) { iteration := 0 w := watcher{ - backend: firewall.Backend{Mode: firewall.IptablesNFT, Command: "iptables-nft", Restore: "iptables-nft-restore"}, + backend: firewall.Backend{Mode: mode, Command: command, Restore: restore}, restoreRules: func(name string, script []byte, check bool) error { - if name != "iptables-nft-restore" { + if name != restore { return fmt.Errorf("unexpected restore binary %s", name) } if iteration > 0 { @@ -100,9 +133,13 @@ func TestIptablesNFTBatchOnVM(t *testing.T) { } for iteration = 0; iteration < 2; iteration++ { owned = true // cleanup also covers a partial failure after this point - if err := w.apply(ruleSet{}); err != nil { + if err := w.apply(ruleSet{MASQ: map[string]MASQRule{"qa": {Subnet: "198.18.250.0/24", Bridge: bridge}}}); err != nil { t.Fatalf("apply iteration %d: %v", iteration, err) } + masq, err := iptables("-t", "nat", "-S", natChain) + if err != nil || strings.Count(string(masq), "! -d 198.18.250.0/24") != 3 { + t.Fatalf("same-subnet egress exclusion missing after apply %d: %v: %s", iteration, err, masq) + } out, err := iptables("-t", "nat", "-S", "POSTROUTING") if err != nil { t.Fatalf("read POSTROUTING after iteration %d: %v: %s", iteration, err, out) diff --git a/hostnat/watcher.go b/hostnat/watcher.go index 7932d2a..0d9fd73 100644 --- a/hostnat/watcher.go +++ b/hostnat/watcher.go @@ -88,9 +88,11 @@ type IKEPortSNATRule struct { func (p MASQRule) iptables() []byte { buf := &bytes.Buffer{} - buf.WriteString(fmt.Sprintf("-A %s -p tcp -s %s ! -o %s -j MASQUERADE --to-ports 1024-65535\n", natChain, p.Subnet, p.Bridge)) - buf.WriteString(fmt.Sprintf("-A %s -p udp -s %s ! -o %s -j MASQUERADE --to-ports 1024-65535\n", natChain, p.Subnet, p.Bridge)) - buf.WriteString(fmt.Sprintf("-A %s -s %s ! -o %s -j MASQUERADE\n", natChain, p.Subnet, p.Bridge)) + // Keep same-subnet overlay traffic's original source IP. This must live in + // the manager-owned NAT rule, not in a second plugin's chain mutation. + buf.WriteString(fmt.Sprintf("-A %s -p tcp -s %s ! -d %s ! -o %s -j MASQUERADE --to-ports 1024-65535\n", natChain, p.Subnet, p.Subnet, p.Bridge)) + buf.WriteString(fmt.Sprintf("-A %s -p udp -s %s ! -d %s ! -o %s -j MASQUERADE --to-ports 1024-65535\n", natChain, p.Subnet, p.Subnet, p.Bridge)) + buf.WriteString(fmt.Sprintf("-A %s -s %s ! -d %s ! -o %s -j MASQUERADE\n", natChain, p.Subnet, p.Subnet, p.Bridge)) // LOCAL src buf.WriteString(fmt.Sprintf("-A %s -o %s -m addrtype --src-type LOCAL --dst-type UNICAST -j MASQUERADE", natChain, p.Bridge)) diff --git a/hostports/iptables_vm_test.go b/hostports/iptables_vm_test.go index b0ec6df..c775e97 100644 --- a/hostports/iptables_vm_test.go +++ b/hostports/iptables_vm_test.go @@ -14,19 +14,32 @@ import ( // This is deliberately absent from normal CI. Run only in a disposable VM // with Docker's iptables firewall backend; never against a production host. func TestIptablesNFTOnDisposableVM(t *testing.T) { + testIptablesOnDisposableVM(t, firewall.IptablesNFT) +} + +func TestIptablesLegacyOnDisposableVM(t *testing.T) { + testIptablesOnDisposableVM(t, firewall.IptablesLegacy) +} + +func testIptablesOnDisposableVM(t *testing.T, mode firewall.Mode) { if os.Getenv("PASTURESTACK_IPTABLES_VM_TEST") != "1" { t.Skip("requires explicit isolated VM opt-in") } if os.Geteuid() != 0 { t.Fatal("xtables integration test requires root in the disposable VM") } - for _, name := range []string{"iptables-nft", "iptables-nft-restore"} { + command, restore := string(mode), string(mode)+"-restore" + for _, name := range []string{command, restore} { if _, err := exec.LookPath(name); err != nil { t.Fatal(err) } out, err := exec.Command(name, "--version").CombinedOutput() - if err != nil || !strings.Contains(string(out), "nf_tables") { - t.Fatalf("%s is not an nf_tables frontend: %v: %s", name, err, out) + marker := "nf_tables" + if mode == firewall.IptablesLegacy { + marker = "legacy" + } + if err != nil || !strings.Contains(string(out), marker) { + t.Fatalf("%s is not the %s frontend: %v: %s", name, mode, err, out) } } dc, err := client.New(client.FromEnv) @@ -41,8 +54,18 @@ func TestIptablesNFTOnDisposableVM(t *testing.T) { if info.Info.FirewallBackend == nil || info.Info.FirewallBackend.Driver != "iptables" { t.Fatalf("refusing xtables test outside Docker iptables mode: %#v", info.Info.FirewallBackend) } + if out, err := exec.Command(command, "-t", "nat", "-S", "DOCKER").CombinedOutput(); err != nil { + t.Fatalf("requires Docker-owned NAT chain in %s: %v: %s", mode, err, out) + } + other := "iptables-nft" + if mode == firewall.IptablesNFT { + other = "iptables-legacy" + } + if out, err := exec.Command(other, "-t", "nat", "-S", "DOCKER").CombinedOutput(); err == nil { + t.Fatalf("refusing dual Docker backends; %s also owns NAT: %s", other, out) + } for _, table := range []string{"nat", "filter"} { - out, err := xtVMCommand("-t", table, "-S") + out, err := xtVMCommand(command, "-t", table, "-S") if err != nil { t.Fatalf("inspect existing %s rules: %v: %s", table, err, out) } @@ -50,14 +73,14 @@ func TestIptablesNFTOnDisposableVM(t *testing.T) { t.Fatalf("refusing to touch existing CATTLE chains in %s: %s", table, out) } } - t.Cleanup(func() { cleanupXTTestRules(t) }) + t.Cleanup(func() { cleanupXTTestRules(t, command) }) rules := ruleSet{ Ports: map[string]PortRule{ "isolated": {Bridge: "pstest0", SourceIP: "198.51.100.2", SourcePort: "55555", TargetIP: "10.254.250.2", TargetPort: "55556", Protocol: "tcp"}, }, ForwardSubnets: map[string]string{"isolated": "10.254.250.0/24"}, } - w := &watcher{backend: firewall.Backend{Mode: firewall.IptablesNFT, Command: "iptables-nft", Restore: "iptables-nft-restore"}} + w := &watcher{backend: firewall.Backend{Mode: mode, Command: command, Restore: restore}} for attempt := 1; attempt <= 2; attempt++ { if err := w.apply(rules); err != nil { t.Fatalf("iptables-nft apply %d (includes --test -n): %v", attempt, err) @@ -68,7 +91,7 @@ func TestIptablesNFTOnDisposableVM(t *testing.T) { {"nat", "POSTROUTING", hostPortsPostRoutingChain}, {"filter", "FORWARD", "CATTLE_FORWARD"}, } { - out, err := xtVMCommand("-t", hook.table, "-S", hook.chain) + out, err := xtVMCommand(command, "-t", hook.table, "-S", hook.chain) if err != nil { t.Fatalf("inspect %s/%s after apply %d: %v: %s", hook.table, hook.chain, attempt, err, out) } @@ -79,11 +102,11 @@ func TestIptablesNFTOnDisposableVM(t *testing.T) { } } -func xtVMCommand(args ...string) ([]byte, error) { - return exec.Command("iptables-nft", append([]string{"-w"}, args...)...).CombinedOutput() +func xtVMCommand(command string, args ...string) ([]byte, error) { + return exec.Command(command, append([]string{"-w"}, args...)...).CombinedOutput() } -func cleanupXTTestRules(t *testing.T) { +func cleanupXTTestRules(t *testing.T, command string) { for _, hook := range []struct { table, chain string spec []string @@ -96,10 +119,10 @@ func cleanupXTTestRules(t *testing.T) { check := append([]string{"-t", hook.table, "-C", hook.chain}, hook.spec...) deleteArgs := append([]string{"-t", hook.table, "-D", hook.chain}, hook.spec...) for { - if _, err := xtVMCommand(check...); err != nil { + if _, err := xtVMCommand(command, check...); err != nil { break } - if out, err := xtVMCommand(deleteArgs...); err != nil { + if out, err := xtVMCommand(command, deleteArgs...); err != nil { t.Errorf("remove own hook %s/%s: %v: %s", hook.table, hook.chain, err, out) break } @@ -112,19 +135,19 @@ func cleanupXTTestRules(t *testing.T) { {"nat", hostPortsPostRoutingChain}, {"filter", "CATTLE_FORWARD"}, } { - if _, err := xtVMCommand("-t", entry.table, "-S", entry.chain); err != nil { + if _, err := xtVMCommand(command, "-t", entry.table, "-S", entry.chain); err != nil { continue // A failed apply may never have created this chain. } - if out, err := xtVMCommand("-t", entry.table, "-F", entry.chain); err != nil { + if out, err := xtVMCommand(command, "-t", entry.table, "-F", entry.chain); err != nil { t.Errorf("flush own chain %s/%s: %v: %s", entry.table, entry.chain, err, out) continue } - if out, err := xtVMCommand("-t", entry.table, "-X", entry.chain); err != nil { + if out, err := xtVMCommand(command, "-t", entry.table, "-X", entry.chain); err != nil { t.Errorf("delete own chain %s/%s: %v: %s", entry.table, entry.chain, err, out) } } for _, table := range []string{"nat", "filter"} { - out, err := xtVMCommand("-t", table, "-S") + out, err := xtVMCommand(command, "-t", table, "-S") if err != nil { t.Errorf("verify %s cleanup: %v: %s", table, err, out) } else if strings.Contains(string(out), "CATTLE_") {