mirror of
https://github.com/netbirdio/netbird.git
synced 2026-07-30 09:53:19 -04:00
[client] fix stale routing peer on overlapping-prefix network removal (#6799)
## Describe your changes The AllowedIPs reference counter ([refcounter/types.go#L9](https://github.com/netbirdio/netbird/blob/e1a24376a/client/internal/routemanager/refcounter/types.go#L9)) was keyed only by prefix and stored a single active peer set by the first registrar, never swapped. When two networks advertised the same prefix via different routing peers, removing the one whose peer was installed in WireGuard left the prefix pointing at the removed peer instead of the surviving one — traffic kept flowing to the old peer until a manual `netbird down/up`. Made the AllowedIPs counter peer-aware: it tracks a per-peer reference count per prefix plus the installed peer, and swaps WireGuard to a surviving peer when the active one releases its last reference (removes the prefix when none remain). `Decrement` now takes the peer key so the exact incremented peer is released; the static handler records its selected routing peer like the dynamic and DNS handlers already did. The generic `Counter` (routes, exclusion, ipset) is unchanged. ## Issue ticket number and link No public issue — reported internally (routes not updating without `netbird down/up` when two networks share a subnet). Root cause is the prefix-only key at [refcounter/types.go#L9](https://github.com/netbirdio/netbird/blob/e1a24376a/client/internal/routemanager/refcounter/types.go#L9). ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client-side routing fix. No public API, CLI, or config change — only the WireGuard AllowedIPs hand-off when overlapping-prefix networks are removed. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: N/A <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6799"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1786796229&installation_id=146802194&pr_number=6799&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6799&signature=322d6950b4f664b1cb3421f2efa039fbbb7946de9b79f2a1b1b131dc182ada2c"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>/codesmith</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved routing behavior when multiple peers share the same Allowed IP by making Allowed IP reference tracking peer-aware. * Allowed IPs now correctly decrement using the active peer key and transfer to another surviving active peer when the current peer is removed. * Prevented stale routing and incorrect reference cleanup during route and DNS-driven teardown. * **Tests** * Added/extended coverage for peer handoffs, repeated references, non-active peer removal, flushing behavior, and self-healing after swap add/remove failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -95,7 +95,7 @@ func (d *DnsInterceptor) RemoveRoute() error {
|
||||
|
||||
// AllowedIPs should use real IPs
|
||||
if d.currentPeerKey != "" {
|
||||
if _, err := d.allowedIPsRefcounter.Decrement(prefix); err != nil {
|
||||
if _, err := d.allowedIPsRefcounter.Decrement(prefix, d.currentPeerKey); err != nil {
|
||||
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %v", prefix, err))
|
||||
}
|
||||
}
|
||||
@@ -172,7 +172,7 @@ func (d *DnsInterceptor) removeAllowedIP(realPrefix netip.Prefix) error {
|
||||
}
|
||||
|
||||
// AllowedIPs use real IPs
|
||||
if _, err := d.allowedIPsRefcounter.Decrement(realPrefix); err != nil {
|
||||
if _, err := d.allowedIPsRefcounter.Decrement(realPrefix, d.currentPeerKey); err != nil {
|
||||
return fmt.Errorf("remove allowed IP %s: %v", realPrefix, err)
|
||||
}
|
||||
|
||||
@@ -205,7 +205,7 @@ func (d *DnsInterceptor) RemoveAllowedIPs() error {
|
||||
for _, prefixes := range d.interceptedDomains {
|
||||
for _, prefix := range prefixes {
|
||||
// AllowedIPs use real IPs
|
||||
if _, err := d.allowedIPsRefcounter.Decrement(prefix); err != nil {
|
||||
if _, err := d.allowedIPsRefcounter.Decrement(prefix, d.currentPeerKey); err != nil {
|
||||
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %v", prefix, err))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -135,7 +135,7 @@ func (r *Route) RemoveAllowedIPs() error {
|
||||
var merr *multierror.Error
|
||||
for _, domainPrefixes := range r.dynamicDomains {
|
||||
for _, prefix := range domainPrefixes {
|
||||
if _, err := r.allowedIPsRefcounter.Decrement(prefix); err != nil {
|
||||
if _, err := r.allowedIPsRefcounter.Decrement(prefix, r.currentPeerKey); err != nil {
|
||||
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %w", prefix, err))
|
||||
}
|
||||
}
|
||||
@@ -320,7 +320,7 @@ func (r *Route) removeRoutes(prefixes []netip.Prefix) ([]netip.Prefix, error) {
|
||||
merr = multierror.Append(merr, fmt.Errorf("remove dynamic route for IP %s: %w", prefix, err))
|
||||
}
|
||||
if r.currentPeerKey != "" {
|
||||
if _, err := r.allowedIPsRefcounter.Decrement(prefix); err != nil {
|
||||
if _, err := r.allowedIPsRefcounter.Decrement(prefix, r.currentPeerKey); err != nil {
|
||||
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %w", prefix, err))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -216,7 +216,7 @@ func (m *DefaultManager) setupRefCounters(useNoop bool) {
|
||||
)
|
||||
}
|
||||
|
||||
m.allowedIPsRefCounter = refcounter.New(
|
||||
m.allowedIPsRefCounter = refcounter.NewAllowedIPs(
|
||||
func(prefix netip.Prefix, peerKey string) (string, error) {
|
||||
// save peerKey to use it in the remove function
|
||||
return peerKey, m.wgInterface.AddAllowedIP(peerKey, prefix)
|
||||
|
||||
185
client/internal/routemanager/refcounter/allowedips.go
Normal file
185
client/internal/routemanager/refcounter/allowedips.go
Normal file
@@ -0,0 +1,185 @@
|
||||
package refcounter
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/netip"
|
||||
"sort"
|
||||
"sync"
|
||||
|
||||
"github.com/hashicorp/go-multierror"
|
||||
|
||||
nberrors "github.com/netbirdio/netbird/client/errors"
|
||||
)
|
||||
|
||||
// allowedIPsEntry holds the per-peer reference counts for a single prefix and which peer is
|
||||
// currently installed in WireGuard. WireGuard allows a prefix on exactly one peer, so at most
|
||||
// one peer is active at a time even when several peers reference the prefix.
|
||||
type allowedIPsEntry struct {
|
||||
// peers maps a peerKey to the number of references holding the prefix for that peer.
|
||||
peers map[string]int
|
||||
// active is the peerKey currently installed in WireGuard for this prefix ("" if none).
|
||||
active string
|
||||
// total is the sum of all per-peer reference counts (kept in sync with peers).
|
||||
total int
|
||||
}
|
||||
|
||||
// AllowedIPsRefCounter is a peer-aware reference counter for WireGuard AllowedIPs.
|
||||
//
|
||||
// The generic Counter keys only by prefix and remembers a single Out value set by the first
|
||||
// caller, which it never changes. That is wrong for AllowedIPs: two independent watchers (or
|
||||
// multiple resolved domains) can reference the same prefix through different peers, and when the
|
||||
// peer currently installed in WireGuard releases its last reference the prefix must be handed over
|
||||
// to a surviving peer instead of being left pointing at the released one.
|
||||
//
|
||||
// It calls add/remove (which program WireGuard) only on the transitions that matter:
|
||||
// - add on the first reference for a prefix, or when swapping the active peer;
|
||||
// - remove on the last reference for a prefix, or on the old peer during a swap.
|
||||
type AllowedIPsRefCounter struct {
|
||||
mu sync.Mutex
|
||||
entries map[netip.Prefix]*allowedIPsEntry
|
||||
add AddFunc[netip.Prefix, string, string]
|
||||
remove RemoveFunc[netip.Prefix, string]
|
||||
}
|
||||
|
||||
// NewAllowedIPs creates a new peer-aware AllowedIPs reference counter.
|
||||
// add programs a prefix on a peer in WireGuard and returns the peerKey to store as the active peer.
|
||||
// remove unprograms the prefix from the given peer.
|
||||
func NewAllowedIPs(add AddFunc[netip.Prefix, string, string], remove RemoveFunc[netip.Prefix, string]) *AllowedIPsRefCounter {
|
||||
return &AllowedIPsRefCounter{
|
||||
entries: map[netip.Prefix]*allowedIPsEntry{},
|
||||
add: add,
|
||||
remove: remove,
|
||||
}
|
||||
}
|
||||
|
||||
// Increment adds a reference to prefix for peerKey. WireGuard is programmed only for the first
|
||||
// reference to a prefix; while a different peer is already installed the prefix is left with it
|
||||
// (first peer wins, HA at the WireGuard layer is not possible) and only the reference count is kept.
|
||||
func (rm *AllowedIPsRefCounter) Increment(prefix netip.Prefix, peerKey string) (Ref[string], error) {
|
||||
rm.mu.Lock()
|
||||
defer rm.mu.Unlock()
|
||||
|
||||
e, ok := rm.entries[prefix]
|
||||
if !ok {
|
||||
e = &allowedIPsEntry{peers: map[string]int{}}
|
||||
rm.entries[prefix] = e
|
||||
}
|
||||
|
||||
logCallerF("Increasing 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)
|
||||
|
||||
// Program WireGuard only when nothing is installed yet for this prefix.
|
||||
if e.active == "" {
|
||||
out, err := rm.add(prefix, peerKey)
|
||||
if errors.Is(err, ErrIgnore) {
|
||||
if e.total == 0 {
|
||||
delete(rm.entries, prefix)
|
||||
}
|
||||
return Ref[string]{Count: e.total, Out: e.active}, nil
|
||||
}
|
||||
if err != nil {
|
||||
if e.total == 0 {
|
||||
delete(rm.entries, prefix)
|
||||
}
|
||||
return Ref[string]{}, fmt.Errorf("failed to add allowed IP %v for peer %s: %w", prefix, peerKey, err)
|
||||
}
|
||||
e.active = out
|
||||
}
|
||||
|
||||
e.peers[peerKey]++
|
||||
e.total++
|
||||
|
||||
return Ref[string]{Count: e.total, Out: e.active}, nil
|
||||
}
|
||||
|
||||
// Decrement removes a reference to prefix for peerKey. When the peer currently installed in
|
||||
// WireGuard releases its last reference, the prefix is swapped to a surviving peer if one exists,
|
||||
// otherwise it is removed from WireGuard.
|
||||
func (rm *AllowedIPsRefCounter) Decrement(prefix netip.Prefix, peerKey string) (Ref[string], error) {
|
||||
rm.mu.Lock()
|
||||
defer rm.mu.Unlock()
|
||||
|
||||
e, ok := rm.entries[prefix]
|
||||
if !ok {
|
||||
logCallerF("No allowed IP reference found for prefix %v", prefix)
|
||||
return Ref[string]{}, nil
|
||||
}
|
||||
|
||||
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
|
||||
}
|
||||
|
||||
// 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 {
|
||||
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
|
||||
}
|
||||
|
||||
delete(rm.entries, prefix)
|
||||
return Ref[string]{Count: 0, Out: ""}, nil
|
||||
}
|
||||
|
||||
// Flush removes all prefixes from WireGuard and clears the counter.
|
||||
func (rm *AllowedIPsRefCounter) Flush() error {
|
||||
rm.mu.Lock()
|
||||
defer rm.mu.Unlock()
|
||||
|
||||
var merr *multierror.Error
|
||||
for prefix, e := range rm.entries {
|
||||
if e.active == "" {
|
||||
continue
|
||||
}
|
||||
logCallerF("Flushing allowed IP for prefix %v peer %s", prefix, e.active)
|
||||
if err := rm.remove(prefix, e.active); err != nil {
|
||||
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %v for peer %s: %w", prefix, e.active, err))
|
||||
}
|
||||
}
|
||||
|
||||
clear(rm.entries)
|
||||
|
||||
return nberrors.FormatErrorOrNil(merr)
|
||||
}
|
||||
|
||||
// pickSurvivor deterministically selects a peer still referencing the prefix. WireGuard cannot do
|
||||
// multipath for a single prefix, so any surviving peer is a valid winner; the choice is made stable
|
||||
// (lowest peerKey) for predictable behavior and testability.
|
||||
func pickSurvivor(peers map[string]int) (string, bool) {
|
||||
if len(peers) == 0 {
|
||||
return "", false
|
||||
}
|
||||
keys := make([]string, 0, len(peers))
|
||||
for k := range peers {
|
||||
keys = append(keys, k)
|
||||
}
|
||||
sort.Strings(keys)
|
||||
return keys[0], true
|
||||
}
|
||||
241
client/internal/routemanager/refcounter/allowedips_test.go
Normal file
241
client/internal/routemanager/refcounter/allowedips_test.go
Normal file
@@ -0,0 +1,241 @@
|
||||
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
|
||||
failAdd bool
|
||||
failRemove bool
|
||||
}
|
||||
|
||||
func newFakeWG() *fakeWG {
|
||||
return &fakeWG{installed: map[netip.Prefix]string{}}
|
||||
}
|
||||
|
||||
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 {
|
||||
delete(f.installed, prefix)
|
||||
}
|
||||
return nil
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
func mustPrefix(t *testing.T, s string) netip.Prefix {
|
||||
t.Helper()
|
||||
p, err := netip.ParsePrefix(s)
|
||||
if err != nil {
|
||||
t.Fatalf("parse prefix %q: %v", s, err)
|
||||
}
|
||||
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.
|
||||
func TestAllowedIPs_SwapOnActivePeerRemoval(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")
|
||||
// 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.
|
||||
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.
|
||||
mustDecrement(t, c, p, "peerB")
|
||||
if _, ok := f.installed[p]; ok {
|
||||
t.Fatalf("expected prefix removed, still installed on %q", f.installed[p])
|
||||
}
|
||||
}
|
||||
|
||||
// TestAllowedIPs_RemoveNonActivePeer removing a non-installed peer must not touch WireGuard.
|
||||
func TestAllowedIPs_RemoveNonActivePeer(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")
|
||||
removesBefore := f.removes
|
||||
|
||||
mustDecrement(t, c, p, "peerB")
|
||||
if f.installed[p] != "peerA" {
|
||||
t.Fatalf("active peer must stay peerA, got %q", f.installed[p])
|
||||
}
|
||||
if f.removes != removesBefore {
|
||||
t.Fatalf("removing a non-active peer must not call wg remove")
|
||||
}
|
||||
}
|
||||
|
||||
// TestAllowedIPs_SamePeerMultipleRefs two references via the same peer must keep the prefix until
|
||||
// the last reference is released (the reason the per-peer count must be an int, not a set).
|
||||
func TestAllowedIPs_SamePeerMultipleRefs(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, "peerA")
|
||||
if f.adds != 1 {
|
||||
t.Fatalf("expected a single wg add for the same peer, got %d", f.adds)
|
||||
}
|
||||
|
||||
mustDecrement(t, c, p, "peerA")
|
||||
if f.installed[p] != "peerA" {
|
||||
t.Fatalf("prefix must stay while a reference remains, got %q", f.installed[p])
|
||||
}
|
||||
if f.removes != 0 {
|
||||
t.Fatalf("no wg remove expected while a reference remains, got %d", f.removes)
|
||||
}
|
||||
|
||||
mustDecrement(t, c, p, "peerA")
|
||||
if _, ok := f.installed[p]; ok {
|
||||
t.Fatalf("prefix must be removed after last reference")
|
||||
}
|
||||
}
|
||||
|
||||
// TestAllowedIPs_RefCountAndActive checks the Ref returned to callers (used for the HA-disabled log).
|
||||
func TestAllowedIPs_RefCountAndActive(t *testing.T) {
|
||||
f := newFakeWG()
|
||||
c := f.counter()
|
||||
p := mustPrefix(t, "10.44.8.0/24")
|
||||
|
||||
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 = mustIncrement(t, c, p, "peerB")
|
||||
if ref.Count != 2 || ref.Out != "peerA" {
|
||||
t.Fatalf("want {2, peerA}, got {%d, %q}", ref.Count, ref.Out)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAllowedIPs_Flush removes everything installed and clears the counter.
|
||||
func TestAllowedIPs_Flush(t *testing.T) {
|
||||
f := newFakeWG()
|
||||
c := f.counter()
|
||||
p1 := mustPrefix(t, "10.44.8.0/24")
|
||||
p2 := mustPrefix(t, "10.44.9.0/24")
|
||||
|
||||
mustIncrement(t, c, p1, "peerA")
|
||||
mustIncrement(t, c, p2, "peerB")
|
||||
|
||||
if err := c.Flush(); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(f.installed) != 0 {
|
||||
t.Fatalf("expected all prefixes removed, got %v", f.installed)
|
||||
}
|
||||
// After flush, a fresh increment must add again.
|
||||
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])
|
||||
}
|
||||
}
|
||||
@@ -5,5 +5,7 @@ import "net/netip"
|
||||
// RouteRefCounter is a Counter for Route, it doesn't take any input on Increment and doesn't use any output on Decrement
|
||||
type RouteRefCounter = Counter[netip.Prefix, struct{}, struct{}]
|
||||
|
||||
// AllowedIPsRefCounter is a Counter for AllowedIPs, it takes a peer key on Increment and passes it back to Decrement
|
||||
type AllowedIPsRefCounter = Counter[netip.Prefix, string, string]
|
||||
// AllowedIPsRefCounter tracks WireGuard AllowedIPs per prefix. Unlike the generic Counter it is peer-aware:
|
||||
// a prefix can be claimed by several peers at once and WireGuard allows a given prefix on exactly one peer,
|
||||
// so the counter records the per-peer reference count and swaps the installed peer when the active one is released.
|
||||
// See allowedips.go.
|
||||
|
||||
@@ -15,6 +15,11 @@ type Route struct {
|
||||
route *route.Route
|
||||
routeRefCounter *refcounter.RouteRefCounter
|
||||
allowedIPsRefcounter *refcounter.AllowedIPsRefCounter
|
||||
// currentPeerKey is the routing peer this watcher currently has the prefix installed on
|
||||
// (the HA winner elected by the watcher). It can differ from route.Peer and change on
|
||||
// failover, so it is recorded on AddAllowedIPs and used on RemoveAllowedIPs to decrement
|
||||
// the exact peer that was incremented.
|
||||
currentPeerKey string
|
||||
}
|
||||
|
||||
func NewRoute(params common.HandlerParams) *Route {
|
||||
@@ -52,12 +57,15 @@ func (r *Route) AddAllowedIPs(peerKey string) error {
|
||||
ref.Out,
|
||||
)
|
||||
}
|
||||
r.currentPeerKey = peerKey
|
||||
return nil
|
||||
}
|
||||
|
||||
func (r *Route) RemoveAllowedIPs() error {
|
||||
if _, err := r.allowedIPsRefcounter.Decrement(r.route.Network); 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)
|
||||
}
|
||||
return nil
|
||||
r.currentPeerKey = ""
|
||||
return err
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user