sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-07-18 04:29:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

[client] make AllowedIPs swap self-healing on WireGuard errors

Browse Source 
Address CodeRabbit review on #6799.

Decrement now decides whether to reprogram based on whether the currently
installed peer still holds references (e.peers[e.active] > 0) instead of
whether the released peer was the active one. If a prior swap's remove/add
failed, e.active was left pointing at a peer with zero references and every
later Decrement returned early, permanently stranding the prefix and leaking
the entry. The active peer is now detached only when e.active != "", and a
failed hand-off is retried on the next Decrement/Increment.

Also clear currentPeerKey unconditionally in the static handler's
RemoveAllowedIPs (matching dynamic/dnsinterceptor) and assert Increment
errors in the tests. Added self-heal tests for the failed remove/add paths.
This commit is contained in:
riccardom
2026-07-16 16:07:52 +02:00
parent f6a756c962
commit aa4257f1ac
3 changed files with 131 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
client/internal/routemanager/refcounter/allowedips.go
View File

@@ -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
}

136
client/internal/routemanager/refcounter/allowedips_test.go
View File

@@ -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])
}
}

7
client/internal/routemanager/static/route.go
View File

@@ -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
}