diff --git a/client/internal/routemanager/refcounter/allowedips.go b/client/internal/routemanager/refcounter/allowedips.go index 3b92c2018..6f23162f6 100644 --- a/client/internal/routemanager/refcounter/allowedips.go +++ b/client/internal/routemanager/refcounter/allowedips.go @@ -106,45 +106,45 @@ func (rm *AllowedIPsRefCounter) Decrement(prefix netip.Prefix, peerKey string) ( return Ref[string]{}, nil } - if e.peers[peerKey] == 0 { + if e.peers[peerKey] > 0 { + logCallerF("Decreasing allowed IP ref count for prefix %v peer %s [peer %d -> %d, total %d -> %d, active %q]", + prefix, peerKey, e.peers[peerKey], e.peers[peerKey]-1, e.total, e.total-1, e.active) + e.peers[peerKey]-- + e.total-- + if e.peers[peerKey] == 0 { + delete(e.peers, peerKey) + } + } else { logCallerF("No allowed IP reference found for prefix %v peer %s", prefix, peerKey) + } + + // If the peer currently installed in WireGuard still holds references, nothing to reprogram. + // Keying the check on the active peer (not the one just released) makes this self-healing: + // a prior swap whose remove/add failed leaves e.active pointing at a peer with no references, + // and this retries the hand-off on the next Decrement instead of getting stuck. + if e.active != "" && e.peers[e.active] > 0 { return Ref[string]{Count: e.total, Out: e.active}, nil } - logCallerF("Decreasing allowed IP ref count for prefix %v peer %s [peer %d -> %d, total %d -> %d, active %q]", - prefix, peerKey, e.peers[peerKey], e.peers[peerKey]-1, e.total, e.total-1, e.active) - - e.peers[peerKey]-- - e.total-- - if e.peers[peerKey] == 0 { - delete(e.peers, peerKey) - } - - // Nothing to reprogram unless the peer we just released is the one installed in WireGuard - // and it no longer holds any reference. - if peerKey != e.active || e.peers[peerKey] > 0 { - return Ref[string]{Count: e.total, Out: e.active}, nil - } - - // The active peer is gone. Hand the prefix over to a surviving peer, or remove it entirely. - if survivor, ok := pickSurvivor(e.peers); ok { + // Detach the stale/gone active peer from WireGuard before reprogramming. + if e.active != "" { if err := rm.remove(prefix, e.active); err != nil { return Ref[string]{Count: e.total, Out: e.active}, fmt.Errorf("remove allowed IP %v for peer %s: %w", prefix, e.active, err) } + e.active = "" + } + + // Hand the prefix over to a surviving peer, or drop the entry when none remain. + if survivor, ok := pickSurvivor(e.peers); ok { out, err := rm.add(prefix, survivor) if err != nil { - e.active = "" - return Ref[string]{Count: e.total, Out: e.active}, fmt.Errorf("swap allowed IP %v to peer %s: %w", prefix, survivor, err) + return Ref[string]{Count: e.total, Out: ""}, fmt.Errorf("swap allowed IP %v to peer %s: %w", prefix, survivor, err) } e.active = out return Ref[string]{Count: e.total, Out: e.active}, nil } - if err := rm.remove(prefix, e.active); err != nil { - return Ref[string]{}, fmt.Errorf("remove allowed IP %v for peer %s: %w", prefix, e.active, err) - } delete(rm.entries, prefix) - return Ref[string]{Count: 0, Out: ""}, nil } diff --git a/client/internal/routemanager/refcounter/allowedips_test.go b/client/internal/routemanager/refcounter/allowedips_test.go index 1d4b58143..835142083 100644 --- a/client/internal/routemanager/refcounter/allowedips_test.go +++ b/client/internal/routemanager/refcounter/allowedips_test.go @@ -1,15 +1,19 @@ package refcounter import ( + "errors" "net/netip" "testing" ) // fakeWG models WireGuard's cryptokey routing: a prefix can be installed on exactly one peer. +// failAdd/failRemove make the next add/remove fail once, to exercise the self-healing error paths. type fakeWG struct { - installed map[netip.Prefix]string - adds int - removes int + installed map[netip.Prefix]string + adds int + removes int + failAdd bool + failRemove bool } func newFakeWG() *fakeWG { @@ -19,11 +23,19 @@ func newFakeWG() *fakeWG { func (f *fakeWG) counter() *AllowedIPsRefCounter { return NewAllowedIPs( func(prefix netip.Prefix, peerKey string) (string, error) { + if f.failAdd { + f.failAdd = false + return "", errors.New("add failed") + } f.adds++ f.installed[prefix] = peerKey return peerKey, nil }, func(prefix netip.Prefix, peerKey string) error { + if f.failRemove { + f.failRemove = false + return errors.New("remove failed") + } f.removes++ // only clear if this peer is the one installed, mirroring wg semantics if f.installed[prefix] == peerKey { @@ -43,6 +55,24 @@ func mustPrefix(t *testing.T, s string) netip.Prefix { return p } +func mustIncrement(t *testing.T, c *AllowedIPsRefCounter, p netip.Prefix, peer string) Ref[string] { + t.Helper() + ref, err := c.Increment(p, peer) + if err != nil { + t.Fatalf("Increment(%v, %s): %v", p, peer, err) + } + return ref +} + +func mustDecrement(t *testing.T, c *AllowedIPsRefCounter, p netip.Prefix, peer string) Ref[string] { + t.Helper() + ref, err := c.Decrement(p, peer) + if err != nil { + t.Fatalf("Decrement(%v, %s): %v", p, peer, err) + } + return ref +} + // TestAllowedIPs_SwapOnActivePeerRemoval reproduces the reported bug: two networks with the same // prefix routed by different peers. Removing the network whose peer is installed must hand the // prefix over to the surviving peer instead of leaving it on the removed one. @@ -51,29 +81,21 @@ func TestAllowedIPs_SwapOnActivePeerRemoval(t *testing.T) { c := f.counter() p := mustPrefix(t, "10.44.8.0/24") - if _, err := c.Increment(p, "peerA"); err != nil { - t.Fatal(err) - } - if _, err := c.Increment(p, "peerB"); err != nil { - t.Fatal(err) - } + mustIncrement(t, c, p, "peerA") + mustIncrement(t, c, p, "peerB") // First peer wins while both are present. if got := f.installed[p]; got != "peerA" { t.Fatalf("expected peerA installed, got %q", got) } // Remove the active peer's network -> must swap to peerB. - if _, err := c.Decrement(p, "peerA"); err != nil { - t.Fatal(err) - } + mustDecrement(t, c, p, "peerA") if got := f.installed[p]; got != "peerB" { t.Fatalf("BUG: prefix stuck on removed peer, want peerB got %q", got) } // Remove the last one -> prefix gone. - if _, err := c.Decrement(p, "peerB"); err != nil { - t.Fatal(err) - } + mustDecrement(t, c, p, "peerB") if _, ok := f.installed[p]; ok { t.Fatalf("expected prefix removed, still installed on %q", f.installed[p]) } @@ -85,13 +107,11 @@ func TestAllowedIPs_RemoveNonActivePeer(t *testing.T) { c := f.counter() p := mustPrefix(t, "10.44.8.0/24") - _, _ = c.Increment(p, "peerA") - _, _ = c.Increment(p, "peerB") + mustIncrement(t, c, p, "peerA") + mustIncrement(t, c, p, "peerB") removesBefore := f.removes - if _, err := c.Decrement(p, "peerB"); err != nil { - t.Fatal(err) - } + mustDecrement(t, c, p, "peerB") if f.installed[p] != "peerA" { t.Fatalf("active peer must stay peerA, got %q", f.installed[p]) } @@ -107,15 +127,13 @@ func TestAllowedIPs_SamePeerMultipleRefs(t *testing.T) { c := f.counter() p := mustPrefix(t, "10.44.8.0/24") - _, _ = c.Increment(p, "peerA") - _, _ = c.Increment(p, "peerA") + mustIncrement(t, c, p, "peerA") + mustIncrement(t, c, p, "peerA") if f.adds != 1 { t.Fatalf("expected a single wg add for the same peer, got %d", f.adds) } - if _, err := c.Decrement(p, "peerA"); err != nil { - t.Fatal(err) - } + mustDecrement(t, c, p, "peerA") if f.installed[p] != "peerA" { t.Fatalf("prefix must stay while a reference remains, got %q", f.installed[p]) } @@ -123,9 +141,7 @@ func TestAllowedIPs_SamePeerMultipleRefs(t *testing.T) { t.Fatalf("no wg remove expected while a reference remains, got %d", f.removes) } - if _, err := c.Decrement(p, "peerA"); err != nil { - t.Fatal(err) - } + mustDecrement(t, c, p, "peerA") if _, ok := f.installed[p]; ok { t.Fatalf("prefix must be removed after last reference") } @@ -137,11 +153,11 @@ func TestAllowedIPs_RefCountAndActive(t *testing.T) { c := f.counter() p := mustPrefix(t, "10.44.8.0/24") - ref, _ := c.Increment(p, "peerA") + ref := mustIncrement(t, c, p, "peerA") if ref.Count != 1 || ref.Out != "peerA" { t.Fatalf("want {1, peerA}, got {%d, %q}", ref.Count, ref.Out) } - ref, _ = c.Increment(p, "peerB") + ref = mustIncrement(t, c, p, "peerB") if ref.Count != 2 || ref.Out != "peerA" { t.Fatalf("want {2, peerA}, got {%d, %q}", ref.Count, ref.Out) } @@ -154,8 +170,8 @@ func TestAllowedIPs_Flush(t *testing.T) { p1 := mustPrefix(t, "10.44.8.0/24") p2 := mustPrefix(t, "10.44.9.0/24") - _, _ = c.Increment(p1, "peerA") - _, _ = c.Increment(p2, "peerB") + mustIncrement(t, c, p1, "peerA") + mustIncrement(t, c, p2, "peerB") if err := c.Flush(); err != nil { t.Fatal(err) @@ -164,8 +180,62 @@ func TestAllowedIPs_Flush(t *testing.T) { t.Fatalf("expected all prefixes removed, got %v", f.installed) } // After flush, a fresh increment must add again. - _, _ = c.Increment(p1, "peerC") + mustIncrement(t, c, p1, "peerC") if f.installed[p1] != "peerC" { t.Fatalf("counter not reset after flush") } } + +// TestAllowedIPs_SelfHealAfterSwapAddError ensures a failed add during a swap does not permanently +// strand the prefix: the next Decrement (or Increment) must retry and install a surviving peer. +func TestAllowedIPs_SelfHealAfterSwapAddError(t *testing.T) { + f := newFakeWG() + c := f.counter() + p := mustPrefix(t, "10.44.8.0/24") + + mustIncrement(t, c, p, "peerA") + mustIncrement(t, c, p, "peerB") + mustIncrement(t, c, p, "peerC") + + // Removing the active peerA triggers a swap to a survivor; make the add fail once. + f.failAdd = true + if _, err := c.Decrement(p, "peerA"); err == nil { + t.Fatalf("expected error from failed swap add") + } + if _, ok := f.installed[p]; ok { + t.Fatalf("nothing should be installed after a failed swap add, got %q", f.installed[p]) + } + + // A later Decrement of a non-active survivor must retry the hand-off (self-heal), not stay stuck. + ref := mustDecrement(t, c, p, "peerC") + if got := f.installed[p]; got == "" { + t.Fatalf("self-heal failed: prefix left unrouted after add recovered") + } + if ref.Out == "" { + t.Fatalf("expected an active peer after self-heal, got empty") + } +} + +// TestAllowedIPs_SelfHealAfterRemoveError ensures a failed remove during a swap is retried instead +// of leaving e.active stuck on a peer that no longer holds references. +func TestAllowedIPs_SelfHealAfterRemoveError(t *testing.T) { + f := newFakeWG() + c := f.counter() + p := mustPrefix(t, "10.44.8.0/24") + + mustIncrement(t, c, p, "peerA") + mustIncrement(t, c, p, "peerB") + + // Releasing active peerA must detach it (remove) then add peerB; fail the remove once. + f.failRemove = true + if _, err := c.Decrement(p, "peerA"); err == nil { + t.Fatalf("expected error from failed remove") + } + + // Next Decrement of the non-active survivor retries: removes stale peerA, installs peerB. + mustDecrement(t, c, p, "peerB") + // peerB had only one ref, so after retry the prefix is fully released. + if _, ok := f.installed[p]; ok { + t.Fatalf("expected prefix released after self-heal, still on %q", f.installed[p]) + } +} diff --git a/client/internal/routemanager/static/route.go b/client/internal/routemanager/static/route.go index 7c45c3efa..8ba03d090 100644 --- a/client/internal/routemanager/static/route.go +++ b/client/internal/routemanager/static/route.go @@ -62,9 +62,10 @@ func (r *Route) AddAllowedIPs(peerKey string) error { } func (r *Route) RemoveAllowedIPs() error { - if _, err := r.allowedIPsRefcounter.Decrement(r.route.Network, r.currentPeerKey); err != nil { - return err + var err error + if _, decErr := r.allowedIPsRefcounter.Decrement(r.route.Network, r.currentPeerKey); decErr != nil { + err = fmt.Errorf("remove allowed IP %s: %w", r.route.Network, decErr) } r.currentPeerKey = "" - return nil + return err }