tailscale / tailscale/tailscale

util/linuxfw: nftables DNATWithLoadBalancer leaks stale PREROUTING DNAT rules

Open
#21,260 0 comments 0 reactions 0 assignees View on GitHub
bug kubernetes
Dominant language
Go
Stars
36.5k
Forks
3.2k
Avg merge
1d 23h
Merged PRs (30d)
132

Description

### What is the issue?

The iptables and nftables `NetfilterRunner` implementations of `DNATWithLoadBalancer` behave differently when called repeatedly for the same original destination. containerboot does exactly that every time a `TS_EXPERIMENTAL_DEST_DNS_NAME` target (operator `ExternalName` Service proxies) re-resolves to a different set of IPs.

**iptables** clears `nat PREROUTING` first and then appends the new rules, so the chain always reflects the current backends:
https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/util/linuxfw/iptables_runner.go#L316-L333

```go
if err := table.ClearChain("nat", "PREROUTING"); err != nil && !isNotExistError(err) {
```

**nftables** just delegates to `AddDNATRule`, which `InsertRule`s a new untagged rule and never deletes anything:
https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/util/linuxfw/nftables_runner.go#L104-L112
https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/util/linuxfw/nftables_runner.go#L162-L164

```go
func (n *nftablesRunner) DNATWithLoadBalancer(origDst netip.Addr, dsts []netip.Addr) error {
return n.AddDNATRule(origDst, dsts[0])
}
```

The caller re-installs the rules on every backend change from the periodic re-resolve timer
(https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/cmd/containerboot/main.go#L740-L757)
and again on netmap changes
(https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/cmd/containerboot/main.go#L827-L845),
via `installIngressForwardingRuleForDNSTarget`
(https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/cmd/containerboot/forwarding.go#L231-L233).

Because `InsertRule` prepends and `dnat` is a terminal statement, the newest rule wins and traffic keeps flowing. But every DNS change leaves one more dead `ip daddr dnat to ` rule in `nat PREROUTING`. On a long-lived proxy whose target changes regularly (a Service behind rolling deployments, a name whose A/AAAA records rotate), the chain grows without bound, and `nft list ruleset` becomes misleading when debugging. The `NetfilterRunner` interface doc describes the same behaviour for both modes, and `EnsureSNATForDst` already implements the correct idempotent pattern in nftables mode using rule `UserData` tags (#13658):
https://github.com/tailscale/tailscale/blob/e2ed432399c9b0fda7aa14e9eb27784d2d893c55/util/linuxfw/nftables_runner.go#L203-L259

The DNAT path was not covered by that fix.

The operator Helm chart defaults to `proxyConfig.firewallMode: auto`, which picks nftables in a fresh container network namespace when nft is available, so operator-managed proxies are on the affected path.

Note: the existing TODO about only using `dsts[0]` in nftables mode (nftables_runner.go L157-L161, forwarding.go L210-L211) is a separate, known limitation; this issue is only about the leaked rules.

### Steps to reproduce

Unit test in `util/linuxfw/nftables_runner_test.go`, using the existing `newSysConn(t)` (L522, netns + `tstest.RequireRoot`, runs in the `privileged` CI job) and `chainRuleCount` (nftables_for_svcs_test.go L264) helpers, same shape as `TestEnsureSNATForDst_nftables` (L1079):

```go
func TestDNATWithLoadBalancer_nftables(t *testing.T) {
conn := newSysConn(t)
runner := newFakeNftablesRunnerWithConn(t, conn, true)
dst := netip.MustParseAddr("100.99.99.99")
a, b := netip.MustParseAddr("10.0.0.1"), netip.MustParseAddr("10.0.0.2")

if err := runner.DNATWithLoadBalancer(dst, []netip.Addr{a}); err != nil {
t.Fatal(err)
}
chainRuleCount(t, "PREROUTING", 1, conn, nftables.TableFamilyIPv4)

// Same backend again: must be a no-op.
if err := runner.DNATWithLoadBalancer(dst, []netip.Addr{a}); err != nil {
t.Fatal(err)
}
chainRuleCount(t, "PREROUTING", 1, conn, nftables.TableFamilyIPv4) // fails today: 2

// Backend changed: old rule must be replaced, not shadowed.
if err := runner.DNATWithLoadBalancer(dst, []netip.Addr{b}); err != nil {
t.Fatal(err)
}
chainRuleCount(t, "PREROUTING", 1, conn, nftables.TableFamilyIPv4) // fails today: 3
}
```

Equivalent manual reproduction: run containerboot with `TS_EXPERIMENTAL_DEST_DNS_NAME` pointing at a name whose IPs change, in nftables mode (`TS_DEBUG_FIREWALL_MODE=nftables`), and watch `nft list chain ip nat PREROUTING` gain one `dnat` rule per change. The iptables equivalent (`TS_DEBUG_FIREWALL_MODE=iptables`, `iptables -t nat -L PREROUTING`) stays at one rule.

### Proposed fix

Mirror `EnsureSNATForDst`: tag DNAT rules created by `AddDNATRule`/`DNATWithLoadBalancer` with `UserData` such as `dnat:,dst:` (the `dnatRuleForChain` helper at nftables_runner.go L114-L155 already accepts a `meta []byte` argument for exactly this), then before inserting, `GetRules(nat, PREROUTING)` and delete any rule whose `UserData` has the `dnat:,` prefix but a different target; return early if the exact rule already exists. Optionally also emit the log line the existing TODO asks for when `len(dsts) > 1` in nftables mode.

### Are there any recent changes that introduced the issue?

No. The behaviour dates from the introduction of `DNATWithLoadBalancer` in #11802; #13658 fixed the analogous leak for SNAT rules only.

### OS

Linux

### OS version

Any (containerboot in a Kubernetes pod)

### Tailscale version

main @ e2ed432399c9b0fda7aa14e9eb27784d2d893c55

### Other software

nftables (netfilter) via containerboot; Kubernetes operator proxies with `firewallMode: auto`/`nftables`

### Bug report

N/A (code-level issue, reproducible with the unit test above)

Analysis prepared with an AI agent operated by KR-Ravindra, who verified the code paths.

Contributor guide

Open the contributing guide

Research direction

Start in util/linuxfw/nftables_runner.go, especially AddDNATRule, dnatRuleForChain, and EnsureSNATForDst, then review util/linuxfw/nftables_runner_test.go and the existing TestEnsureSNATForDst_nftables pattern. Add coverage for repeated DNATWithLoadBalancer calls using newSysConn and chainRuleCount: identical backends should leave one PREROUTING rule, and a changed backend should replace the old rule. Run the nftables test in the privileged CI environment and confirm the rule count stays one.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.