diff --git a/_Log.md b/_Log.md index daa8f1b664..ef013b426b 100644 --- a/_Log.md +++ b/_Log.md @@ -45020,3 +45020,29 @@ top. pkg/flowexport/README.md #3748/#5312 sampler section. - **File(s)**: pkg/flowexport/ipfix.go, pkg/flowexport/ipfix_sampler_test.go, pkg/flowexport/README.md, _Log.md + +- **Timestamp**: 2026-07-10 +- **Action**: #5120 routing/wireguard — unbind the WG TUN from its VRF on + teardown/rebind. `applyWireguardTunLocked` bound the persistent `wgN` TUN + to a VRF only in the non-empty routing-instance case with no else/unbind + branch, and the WG config-removal prune in `Apply` only reconciled + addresses. So removing the `routing-instance` stanza from a still- + configured tunnel — or removing the whole tunnel — left `wgN` mastered to + the old `vrf-` indefinitely (traffic isolated in / leaked through + the wrong routing table). Routed WG through the SAME identity-gated claim + machinery as GRE/IPIP: the apply path now calls `reconcileVRFClaimLocked` + (records `appliedRI` on a successful bind, unbinds when the desired RI is + empty), and the persistent-link removal prune calls the new extracted + `unbindVRFClaimLocked` helper (identity-checks the current master vs the + claimed `vrf-` device, LinkSetNoMaster only when it is ours, retains the + claim + `wgConfigured` entry on transient failure for retry, clears on a + not-found device). Preserved `appliedRI` across the non-WG→WG same-name + handoff so a reused anchor TUN's prior VRF claim carries into the WG + reconcile (else an anchor→WG-no-RI device would strand its old master). + Added four fail-on-revert tests (bind+removal-unbind, RI-removal-unbind, + foreign-master-veto, transient-unbind retry); proved RED-on-revert (3/4 + fail on reverted source; the foreign-master safety guard passes both + ways by design). Updated pkg/routing/README.md WG-removal + VRF-claims + sections (removed the #1434 "VRF not unbound" residual). +- **File(s)**: pkg/routing/tunnel.go, + pkg/routing/tunnel_reconcile_test.go, pkg/routing/README.md, _Log.md diff --git a/pkg/routing/README.md b/pkg/routing/README.md index 88304acc88..73dc7243d1 100644 --- a/pkg/routing/README.md +++ b/pkg/routing/README.md @@ -443,19 +443,29 @@ when the existing kernel link is genuinely incompatible: transient list failure still retries (it does NOT reuse `reconcileLinkAddrsLocked`, whose return only records failed LL deletes). A transient `LinkByName` error (vs `isLinkNotFound`) also - retains for retry; a genuine not-found drops tracking. Residuals - deferred to #1434: removal while the daemon was DOWN is not pruned - (only tracked-applied addresses prune), and VRF membership is not - unbound (WG binds VRF directly, no `appliedRI` claim). + retains for retry; a genuine not-found drops tracking. The same removal + pass also identity-gated-unbinds the WG TUN from its VRF (#5120, + `unbindVRFClaimLocked`): the persistent `wgN` link is KEPT but is never + left enslaved to the VRF it last claimed — a transient `LinkSetNoMaster` + failure retains the `appliedRI` claim (and the `wgConfigured` entry) for + retry, and a not-found device clears the claim (the binding died with the + device). Residual deferred to #1434: removal while the daemon was DOWN is + not pruned (only tracked-applied addresses/claims reconcile). - **VRF claims** (`appliedRI`): written ONLY from a successful `BindInterfaceToVRF` or a direct observation that the link's master is `vrf-` (a step-0a routing-instance interface-list bind) — never from intent. `TunnelConfig.RIListMember` (populated by `collectAppliedTunnels` with the exact step-0a name normalization) vetoes unbinding when the config list-binds the tunnel. Unbind on - config-wants-none is identity-gated: only when the current master IS - the claimed RI's `vrf-` device; transient errors retain the claim - for retry. + config-wants-none is identity-gated (`unbindVRFClaimLocked`): only when + the current master IS the claimed RI's `vrf-` device; transient errors + retain the claim for retry. WireGuard `wgN` TUNs now use this SAME claim + machinery (#5120): `applyWireguardTunLocked` routes through + `reconcileVRFClaimLocked` so removing the `routing-instance` stanza from + a still-configured tunnel unbinds it, and the persistent-link removal + prune (above) shares `unbindVRFClaimLocked` — closing the pre-#5120 gap + where WG bound its VRF directly with no unbind branch and left `wgN` + mastered to a stale VRF after an RI change or tunnel removal. - **Keepalives** (BOTH the anchor and the legacy branch, #4071): runners are reconciled by normalized identity `(remote, source, interval, retry<=0→3)` and survive unrelated applies; `LinkSetUp` is SKIPPED diff --git a/pkg/routing/tunnel.go b/pkg/routing/tunnel.go index d1a30151f0..f7a3347c47 100644 --- a/pkg/routing/tunnel.go +++ b/pkg/routing/tunnel.go @@ -333,10 +333,16 @@ func (t *tunnelManager) Apply(tunnels []*config.TunnelConfig) error { // deletable by the WG reconcile (it is absent from the new WG // desired set), not re-classified as foreign and leaked. This // mirrors the forward WG→non-WG handoff, which also keeps - // appliedAddrs. Drop only appliedRI: the GRE/anchor VRF claim is - // no longer valid (WG binds VRF directly, never via appliedRI). + // appliedAddrs. PRESERVE appliedRI too (#5120): WG now reconciles + // its VRF through the shared claim machinery, so a prior anchor + // VRF claim must carry into applyWireguardTunLocked's reconcile — + // a reused anchor TUN keeps its kernel master across the handoff, + // and dropping the claim would leave an empty appliedRI that + // skips the identity-gated unbind, stranding an anchor→WG-no-RI + // device enslaved to the old VRF. (A GRE→WG handoff deletes and + // recreates the link, so the fresh TUN has master==0 and the + // carried claim self-clears in the reconcile.) t.stopKeepaliveLocked(name) - delete(t.appliedRI, name) continue } t.stopKeepaliveLocked(name) @@ -405,14 +411,18 @@ func (t *tunnelManager) Apply(tunnels []*config.TunnelConfig) error { if err != nil { if isLinkNotFound(err) { // Device genuinely gone (manual `ip link del`, or never - // existed). Nothing to prune; drop tracking. + // existed). Nothing to prune; drop tracking. The VRF binding + // died with the device, so clear any claim too (#5120) — + // there is nothing left to unbind. delete(t.appliedAddrs, name) + delete(t.appliedRI, name) } else { // Transient lookup error (EBUSY/netlink/timeout): we cannot // conclude the device is clean. Retain the name AND its // tracked address set so the next Apply retries the prune — // dropping here would forget a still-leaked address forever - // (#1919 r1 Codex/AGY MAJOR). + // (#1919 r1 Codex/AGY MAJOR). The appliedRI claim is left + // intact so the next Apply also retries the VRF unbind (#5120). slog.Warn("failed to look up wireguard tun for address prune", "name", name, "err", err) nextWG[name] = true @@ -420,10 +430,18 @@ func (t *tunnelManager) Apply(tunnels []*config.TunnelConfig) error { continue } failed, retry := t.pruneAppliedAddrsLocked(link, name, t.appliedAddrs[name]) - if retry { - // Could not prove the device clean (an AddrDel failed, or - // AddrList itself failed). Carry the residual set forward (it - // gates link-local deletion next pass) and retry next Apply. + // Identity-gated VRF unbind (#5120): the persistent wgN link is KEPT + // (S2a), but a WG tunnel removed from config must not linger enslaved + // to the VRF it last claimed. Mirror the RI-removal unbind in + // reconcileVRFClaimLocked — retain the claim and retry on transient + // failure. + unbindRetry := t.unbindVRFClaimLocked(name, link) + if retry || unbindRetry { + // Could not prove the device clean (an AddrDel failed, AddrList + // itself failed, or the VRF unbind hit a transient error). Carry + // the residual address set forward (it gates link-local deletion + // next pass) and retry next Apply. A retained appliedRI claim + // (unbindRetry) is revisited on that retry. t.appliedAddrs[name] = failed nextWG[name] = true continue @@ -1151,15 +1169,38 @@ func (t *tunnelManager) reconcileVRFClaimLocked(tc *config.TunnelConfig, link ne return } + // config wants no RI (step 3): identity-gated unbind of the prior + // claim, shared with the WireGuard config-removal path (#5120). + t.unbindVRFClaimLocked(name, link) +} + +// unbindVRFClaimLocked runs the identity-gated unbind half of the #1884 +// A.5 claim procedure for a tunnel whose desired routing-instance is now +// empty — either because the `routing-instance` stanza was removed from a +// still-configured tunnel (reconcileVRFClaimLocked step 3) or because the +// whole tunnel was removed and its persistent link is being reconciled +// (#5120, the WireGuard removal path — the wgN link is KEPT but must not +// linger enslaved to a VRF it should no longer be in). It clears the +// link's master to vrf- ONLY when the link's CURRENT master is +// OBSERVED to be that VRF device, so it can neither strand a master we own +// nor touch a foreign bind. +// +// The appliedRI claim clears on a successful unbind, on master==0 (already +// unbound), on an identity mismatch (master not ours), or when the VRF +// device is not-found (deleting a VRF frees its slaves). It is RETAINED — +// and the function returns retry=true — only on a TRANSIENT failure (a VRF +// lookup error other than not-found, or a LinkSetNoMaster error) so the +// caller re-runs the reconcile on the next Apply. Caller MUST hold mu. +func (t *tunnelManager) unbindVRFClaimLocked(name string, link netlink.Link) (retry bool) { claim := t.appliedRI[name] if claim == "" { - return + return false } master := link.Attrs().MasterIndex if master == 0 { // Nothing is bound — whatever we once bound is already gone. delete(t.appliedRI, name) - return + return false } vrf, err := t.ops.LinkByName("vrf-" + claim) if err != nil { @@ -1167,24 +1208,25 @@ func (t *tunnelManager) reconcileVRFClaimLocked(tc *config.TunnelConfig, link ne // The VRF device is gone; deleting a master frees its // slaves, so the current master cannot be ours. delete(t.appliedRI, name) - return + return false } // Transient lookup error: retain the claim, retry next apply. - return + return true } if vrf.Attrs().Index != master { // Master is not the VRF we bound (someone else's bind). delete(t.appliedRI, name) - return + return false } if err := t.ops.LinkSetNoMaster(link); err != nil { slog.Warn("failed to unbind tunnel from VRF", "name", name, "vrf", claim, "err", err) - return // retain claim; retry next apply + return true // retain claim; retry next apply } slog.Info("tunnel unbound from routing-instance", "name", name, "vrf", claim) delete(t.appliedRI, name) + return false } // observeListClaimLocked transfers the appliedRI claim to the @@ -1320,11 +1362,15 @@ func wgTunMTUForEndpoint(tc *config.TunnelConfig) int { // daemon was DOWN is not in wgConfigured on the next start, so it is not // pruned (restart-adoption limitation shared by the whole manager — it // only prunes what it tracked applying); (2) the wgN link and its live -// Rust-attached peer/session are kept (not torn) by design; (3) VRF -// membership is NOT unbound on removal (WG binds VRF directly, bypassing -// the appliedRI claim machinery, so there is no identity-gated unbind — -// the same root cause as the no-unbind-on-routing-instance-removal gap -// for a still-configured WG tunnel). +// Rust-attached peer/session are kept (not torn) by design. +// +// VRF membership IS reconciled through the shared appliedRI claim +// machinery (#5120): the reconcile below records an identity-gated claim +// on a successful bind, so removing the `routing-instance` stanza from a +// still-configured tunnel unbinds here, and removing the whole tunnel +// unbinds via the persistent-link prune in Apply (both use +// unbindVRFClaimLocked — the wgN link is kept but never left enslaved to a +// stale VRF). // // The Rust control thread (coordinator/wg_control.rs) attaches to this // persistent device by name. @@ -1418,12 +1464,16 @@ func (t *tunnelManager) applyWireguardTunLocked(tc *config.TunnelConfig) error { t.appliedAddrs[tc.Name] = t.reconcileLinkAddrsLocked( link, tc.Name, tc.Addresses, t.appliedAddrs[tc.Name], "wireguard tun") - if tc.RoutingInstance != "" { - if bindErr := t.vrfBinder.BindInterfaceToVRF(tc.Name, tc.RoutingInstance); bindErr != nil { - slog.Warn("failed to bind wireguard tun to VRF", - "name", tc.Name, "vrf", tc.RoutingInstance, "err", bindErr) - } - } + // VRF binding via the shared identity-gated claim machinery (#5120). + // Previously WG bound directly with no else/unbind branch, so removing + // the `routing-instance` stanza from a still-configured tunnel left wgN + // mastered to the old VRF indefinitely. reconcileVRFClaimLocked records + // appliedRI on a successful bind and, when the desired RI is now empty, + // identity-checks the current master and unbinds (unbindVRFClaimLocked). + // The whole-tunnel-removal case is handled by the persistent-link prune + // in Apply, which shares unbindVRFClaimLocked. The wgN link is kept + // (S2a) but is no longer left enslaved to a stale VRF. + t.reconcileVRFClaimLocked(tc, link) return nil } diff --git a/pkg/routing/tunnel_reconcile_test.go b/pkg/routing/tunnel_reconcile_test.go index 306d068566..4d8ebb7518 100644 --- a/pkg/routing/tunnel_reconcile_test.go +++ b/pkg/routing/tunnel_reconcile_test.go @@ -1647,3 +1647,169 @@ func TestClearTunnelsDeletesOwnershipUnion(t *testing.T) { t.Fatal("Clear left the failed-apply owned link behind (union rule)") } } + +// --- #5120: WireGuard TUN VRF bind/unbind lifecycle ------------------ +// +// The WG apply path used to bind the persistent wgN TUN to a VRF only in +// the non-empty routing-instance case, with no else/unbind branch. Removing +// the whole tunnel — or clearing its `routing-instance` while it stayed +// configured — left wgN mastered to the old VRF indefinitely. These tests +// pin the bind, the RI-removal unbind, the config-removal unbind, the +// foreign-master veto, and the transient-failure retry. They go RED if the +// unbind wiring is reverted. + +func wgTCVRF(ri string, addrs ...string) *config.TunnelConfig { + return &config.TunnelConfig{ + Name: "wg0", Mode: "wireguard", RoutingInstance: ri, Addresses: addrs, + } +} + +func wgClaim(t *testing.T, tm *tunnelManager) string { + t.Helper() + tm.mu.Lock() + defer tm.mu.Unlock() + return tm.appliedRI["wg0"] +} + +// The core fail-on-revert guard: a WG tunnel removed from config keeps its +// persistent link (S2a) but must be UNBOUND from the VRF it last claimed. +func TestWireguardVRFUnboundOnConfigRemoval(t *testing.T) { + ops := newFakeLinkOps() + tm, _ := newReconcileManager(ops) + seedVRF(ops, "red", 100) + + if err := tm.Apply([]*config.TunnelConfig{wgTCVRF("red", "172.16.0.1/30")}); err != nil { + t.Fatalf("Apply 1 (bind): %v", err) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 100 { + t.Fatalf("wg0 not bound to vrf-red: MasterIndex=%d want 100", got) + } + if got := wgClaim(t, tm); got != "red" { + t.Fatalf("appliedRI claim not recorded on WG bind: %q want red", got) + } + + // Remove the WG tunnel entirely from config. + if err := tm.Apply(nil); err != nil { + t.Fatalf("Apply 2 (removal): %v", err) + } + if _, err := ops.LinkByName("wg0"); err != nil { + t.Fatalf("persistent wg0 link gone after removal (must be kept): %v", err) + } + if len(ops.noMaster) != 1 || ops.noMaster[0] != "wg0" { + t.Fatalf("wg0 not unbound from VRF on removal (#5120): noMaster=%v", ops.noMaster) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 0 { + t.Fatalf("wg0 still enslaved after removal: MasterIndex=%d want 0", got) + } + if got := wgClaim(t, tm); got != "" { + t.Fatalf("appliedRI claim not cleared after unbind: %q", got) + } + + // Idempotent: a later Apply with the tunnel still absent must not + // re-issue the unbind (the claim is gone and there is nothing to visit). + if err := tm.Apply(nil); err != nil { + t.Fatalf("Apply 3 (still removed): %v", err) + } + if len(ops.noMaster) != 1 { + t.Fatalf("VRF unbind not idempotent on removal: %v", ops.noMaster) + } +} + +// Clearing the `routing-instance` stanza from a STILL-configured WG tunnel +// must unbind it (parity with GRE/IPIP reconcileVRFClaimLocked step 3). +func TestWireguardVRFUnboundOnRoutingInstanceRemoval(t *testing.T) { + ops := newFakeLinkOps() + tm, _ := newReconcileManager(ops) + seedVRF(ops, "red", 100) + + if err := tm.Apply([]*config.TunnelConfig{wgTCVRF("red", "172.16.0.1/30")}); err != nil { + t.Fatalf("Apply 1 (bind): %v", err) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 100 { + t.Fatalf("wg0 not bound: MasterIndex=%d want 100", got) + } + + // Same tunnel, routing-instance removed (still WireGuard, still present). + if err := tm.Apply([]*config.TunnelConfig{wgTC("172.16.0.1/30")}); err != nil { + t.Fatalf("Apply 2 (RI removed): %v", err) + } + if _, err := ops.LinkByName("wg0"); err != nil { + t.Fatalf("wg0 link gone after RI removal (must be kept): %v", err) + } + if len(ops.noMaster) != 1 || ops.noMaster[0] != "wg0" { + t.Fatalf("wg0 not unbound on routing-instance removal (#5120): noMaster=%v", ops.noMaster) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 0 { + t.Fatalf("wg0 still enslaved after RI removal: MasterIndex=%d want 0", got) + } + if got := wgClaim(t, tm); got != "" { + t.Fatalf("appliedRI claim not cleared after RI removal: %q", got) + } +} + +// A WG tunnel this manager never bound (no appliedRI claim) but which +// carries a FOREIGN master must never be unbound on config removal — the +// identity gate must reject a master we do not own. +func TestWireguardVRFForeignMasterNeverUnbound(t *testing.T) { + ops := newFakeLinkOps() + tm, _ := newReconcileManager(ops) + + // Apply with NO routing-instance ⇒ no claim recorded. + if err := tm.Apply([]*config.TunnelConfig{wgTC("172.16.0.1/30")}); err != nil { + t.Fatalf("Apply 1: %v", err) + } + if got := wgClaim(t, tm); got != "" { + t.Fatalf("unexpected claim for RI-less WG tunnel: %q", got) + } + // Someone else binds wg0 to a VRF out-of-band. + ops.links["wg0"].Attrs().MasterIndex = 200 + + if err := tm.Apply(nil); err != nil { + t.Fatalf("Apply 2 (removal): %v", err) + } + if len(ops.noMaster) != 0 { + t.Fatalf("foreign master unbound on removal (identity gate breached): %v", ops.noMaster) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 200 { + t.Fatalf("foreign master mutated: MasterIndex=%d want 200", got) + } +} + +// A TRANSIENT LinkSetNoMaster failure on removal must RETAIN the claim (and +// the wgConfigured tracking) and retry on the next Apply until it succeeds. +func TestWireguardVRFUnbindTransientRetried(t *testing.T) { + ops := newFakeLinkOps() + tm, _ := newReconcileManager(ops) + seedVRF(ops, "red", 100) + + if err := tm.Apply([]*config.TunnelConfig{wgTCVRF("red", "172.16.0.1/30")}); err != nil { + t.Fatalf("Apply 1 (bind): %v", err) + } + + // Removal, but the unbind netlink call fails transiently. + ops.noMasterErr = errors.New("EBUSY") + if err := tm.Apply(nil); err != nil { + t.Fatalf("Apply 2 (removal, unbind fails): %v", err) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 100 { + t.Fatalf("wg0 unbound despite injected LinkSetNoMaster failure: MasterIndex=%d", got) + } + if got := wgClaim(t, tm); got != "red" { + t.Fatalf("claim not retained after transient unbind failure: %q want red", got) + } + + // Retry: unbind now succeeds → wg0 freed, claim cleared. + ops.noMasterErr = nil + if err := tm.Apply(nil); err != nil { + t.Fatalf("Apply 3 (retry): %v", err) + } + if len(ops.noMaster) != 1 || ops.noMaster[0] != "wg0" { + t.Fatalf("VRF unbind not retried after transient failure (#5120): %v", ops.noMaster) + } + if got := ops.links["wg0"].Attrs().MasterIndex; got != 0 { + t.Fatalf("wg0 still enslaved after retry: MasterIndex=%d want 0", got) + } + if got := wgClaim(t, tm); got != "" { + t.Fatalf("claim not cleared after successful retry: %q", got) + } +}