mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-10 20:15:39 -04:00
## 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 -->
400 lines
11 KiB
Go
400 lines
11 KiB
Go
package dynamic
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"net"
|
|
"net/netip"
|
|
"strings"
|
|
"sync"
|
|
"time"
|
|
|
|
"github.com/hashicorp/go-multierror"
|
|
log "github.com/sirupsen/logrus"
|
|
|
|
nberrors "github.com/netbirdio/netbird/client/errors"
|
|
"github.com/netbirdio/netbird/client/internal/peer"
|
|
"github.com/netbirdio/netbird/client/internal/routemanager/common"
|
|
"github.com/netbirdio/netbird/client/internal/routemanager/iface"
|
|
"github.com/netbirdio/netbird/client/internal/routemanager/refcounter"
|
|
"github.com/netbirdio/netbird/client/internal/routemanager/util"
|
|
"github.com/netbirdio/netbird/route"
|
|
"github.com/netbirdio/netbird/shared/management/domain"
|
|
)
|
|
|
|
const (
|
|
DefaultInterval = time.Minute
|
|
|
|
minInterval = 2 * time.Second
|
|
failureInterval = 5 * time.Second
|
|
|
|
addAllowedIP = "add allowed IP %s: %w"
|
|
)
|
|
|
|
type domainMap map[domain.Domain][]netip.Prefix
|
|
|
|
type resolveResult struct {
|
|
domain domain.Domain
|
|
prefix netip.Prefix
|
|
err error
|
|
}
|
|
|
|
type Route struct {
|
|
route *route.Route
|
|
routeRefCounter *refcounter.RouteRefCounter
|
|
allowedIPsRefcounter *refcounter.AllowedIPsRefCounter
|
|
interval time.Duration
|
|
dynamicDomains domainMap
|
|
mu sync.Mutex
|
|
currentPeerKey string
|
|
cancel context.CancelFunc
|
|
statusRecorder *peer.Status
|
|
wgInterface iface.WGIface
|
|
resolverAddr netip.AddrPort
|
|
}
|
|
|
|
func NewRoute(params common.HandlerParams, resolverAddr netip.AddrPort) *Route {
|
|
return &Route{
|
|
route: params.Route,
|
|
routeRefCounter: params.RouteRefCounter,
|
|
allowedIPsRefcounter: params.AllowedIPsRefCounter,
|
|
interval: params.DnsRouterInterval,
|
|
statusRecorder: params.StatusRecorder,
|
|
wgInterface: params.WgInterface,
|
|
resolverAddr: resolverAddr,
|
|
dynamicDomains: domainMap{},
|
|
}
|
|
}
|
|
|
|
func (r *Route) String() string {
|
|
return r.route.Domains.SafeString()
|
|
}
|
|
|
|
func (r *Route) AddRoute(ctx context.Context) error {
|
|
r.mu.Lock()
|
|
defer r.mu.Unlock()
|
|
|
|
if r.cancel != nil {
|
|
r.cancel()
|
|
}
|
|
|
|
ctx, r.cancel = context.WithCancel(ctx)
|
|
|
|
go r.startResolver(ctx)
|
|
|
|
return nil
|
|
}
|
|
|
|
// RemoveRoute will stop the dynamic resolver and remove all dynamic routes.
|
|
// It doesn't touch allowed IPs, these should be removed separately and before calling this method.
|
|
func (r *Route) RemoveRoute() error {
|
|
r.mu.Lock()
|
|
defer r.mu.Unlock()
|
|
|
|
if r.cancel != nil {
|
|
r.cancel()
|
|
}
|
|
|
|
var merr *multierror.Error
|
|
for domain, prefixes := range r.dynamicDomains {
|
|
for _, prefix := range prefixes {
|
|
if _, err := r.routeRefCounter.Decrement(prefix); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf("remove dynamic route for IP %s: %w", prefix, err))
|
|
}
|
|
}
|
|
log.Debugf("Removed dynamic route(s) for [%s]: %s", domain.SafeString(), strings.ReplaceAll(fmt.Sprintf("%s", prefixes), " ", ", "))
|
|
|
|
r.statusRecorder.DeleteResolvedDomainsStates(domain)
|
|
}
|
|
|
|
r.dynamicDomains = domainMap{}
|
|
|
|
return nberrors.FormatErrorOrNil(merr)
|
|
}
|
|
|
|
func (r *Route) AddAllowedIPs(peerKey string) error {
|
|
r.mu.Lock()
|
|
defer r.mu.Unlock()
|
|
|
|
var merr *multierror.Error
|
|
for domain, domainPrefixes := range r.dynamicDomains {
|
|
for _, prefix := range domainPrefixes {
|
|
if err := r.incrementAllowedIP(domain, prefix, peerKey); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf(addAllowedIP, prefix, err))
|
|
}
|
|
}
|
|
}
|
|
r.currentPeerKey = peerKey
|
|
return nberrors.FormatErrorOrNil(merr)
|
|
}
|
|
|
|
func (r *Route) RemoveAllowedIPs() error {
|
|
r.mu.Lock()
|
|
defer r.mu.Unlock()
|
|
|
|
var merr *multierror.Error
|
|
for _, domainPrefixes := range r.dynamicDomains {
|
|
for _, prefix := range domainPrefixes {
|
|
if _, err := r.allowedIPsRefcounter.Decrement(prefix, r.currentPeerKey); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %w", prefix, err))
|
|
}
|
|
}
|
|
}
|
|
|
|
r.currentPeerKey = ""
|
|
return nberrors.FormatErrorOrNil(merr)
|
|
}
|
|
|
|
func (r *Route) startResolver(ctx context.Context) {
|
|
log.Debugf("Starting dynamic route resolver for domains [%v]", r)
|
|
|
|
interval := r.interval
|
|
if interval < minInterval {
|
|
interval = minInterval
|
|
log.Warnf("Dynamic route resolver interval %s is too low, setting to minimum value %s", r.interval, minInterval)
|
|
}
|
|
|
|
ticker := time.NewTicker(interval)
|
|
defer ticker.Stop()
|
|
|
|
if err := r.update(ctx); err != nil {
|
|
log.Errorf("Failed to resolve domains for route [%v]: %v", r, err)
|
|
if interval > failureInterval {
|
|
ticker.Reset(failureInterval)
|
|
}
|
|
}
|
|
|
|
for {
|
|
select {
|
|
case <-ctx.Done():
|
|
log.Debugf("Stopping dynamic route resolver for domains [%v]", r)
|
|
return
|
|
case <-ticker.C:
|
|
if err := r.update(ctx); err != nil {
|
|
log.Errorf("Failed to resolve domains for route [%v]: %v", r, err)
|
|
// Use a lower ticker interval if the update fails
|
|
if interval > failureInterval {
|
|
ticker.Reset(failureInterval)
|
|
}
|
|
} else if interval > failureInterval {
|
|
// Reset to the original interval if the update succeeds
|
|
ticker.Reset(interval)
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
func (r *Route) update(ctx context.Context) error {
|
|
resolved, err := r.resolveDomains(ctx)
|
|
if err != nil {
|
|
if len(resolved) == 0 {
|
|
return fmt.Errorf("resolve domains: %w", err)
|
|
}
|
|
log.Warnf("Failed to resolve domains: %v", err)
|
|
}
|
|
if err := r.updateDynamicRoutes(ctx, resolved); err != nil {
|
|
return fmt.Errorf("update dynamic routes: %w", err)
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
func (r *Route) resolveDomains(ctx context.Context) (domainMap, error) {
|
|
results := make(chan resolveResult)
|
|
go r.resolve(ctx, results)
|
|
|
|
resolved := domainMap{}
|
|
var merr *multierror.Error
|
|
|
|
for result := range results {
|
|
if result.err != nil {
|
|
merr = multierror.Append(merr, result.err)
|
|
} else {
|
|
resolved[result.domain] = append(resolved[result.domain], result.prefix)
|
|
}
|
|
}
|
|
|
|
return resolved, nberrors.FormatErrorOrNil(merr)
|
|
}
|
|
|
|
func (r *Route) resolve(ctx context.Context, results chan resolveResult) {
|
|
var wg sync.WaitGroup
|
|
|
|
for _, d := range r.route.Domains {
|
|
wg.Add(1)
|
|
go func(domain domain.Domain) {
|
|
defer wg.Done()
|
|
|
|
ips, err := r.getIPsFromResolver(ctx, domain)
|
|
if err != nil {
|
|
log.Tracef("Failed to resolve domain %s with private resolver: %v", domain.SafeString(), err)
|
|
ips, err = lookupHostIPs(ctx, domain)
|
|
if err != nil {
|
|
results <- resolveResult{domain: domain, err: fmt.Errorf("resolve d %s: %w", domain.SafeString(), err)}
|
|
return
|
|
}
|
|
}
|
|
|
|
for _, ip := range ips {
|
|
prefix, err := util.GetPrefixFromIP(ip)
|
|
if err != nil {
|
|
results <- resolveResult{domain: domain, err: fmt.Errorf("get prefix from IP %s: %w", ip.String(), err)}
|
|
return
|
|
}
|
|
results <- resolveResult{domain: domain, prefix: prefix}
|
|
}
|
|
}(d)
|
|
}
|
|
|
|
wg.Wait()
|
|
close(results)
|
|
}
|
|
|
|
func (r *Route) updateDynamicRoutes(ctx context.Context, newDomains domainMap) error {
|
|
r.mu.Lock()
|
|
defer r.mu.Unlock()
|
|
|
|
if ctx.Err() != nil {
|
|
return ctx.Err()
|
|
}
|
|
|
|
var merr *multierror.Error
|
|
|
|
for domain, newPrefixes := range newDomains {
|
|
oldPrefixes := r.dynamicDomains[domain]
|
|
toAdd, toRemove := determinePrefixChanges(oldPrefixes, newPrefixes)
|
|
|
|
addedPrefixes, err := r.addRoutes(domain, toAdd)
|
|
if err != nil {
|
|
merr = multierror.Append(merr, err)
|
|
} else if len(addedPrefixes) > 0 {
|
|
log.Debugf("Added dynamic route(s) for [%s]: %s", domain.SafeString(), strings.ReplaceAll(fmt.Sprintf("%s", addedPrefixes), " ", ", "))
|
|
}
|
|
|
|
removedPrefixes, err := r.removeRoutes(toRemove)
|
|
if err != nil {
|
|
merr = multierror.Append(merr, err)
|
|
} else if len(removedPrefixes) > 0 {
|
|
log.Debugf("Removed dynamic route(s) for [%s]: %s", domain.SafeString(), strings.ReplaceAll(fmt.Sprintf("%s", removedPrefixes), " ", ", "))
|
|
}
|
|
|
|
updatedPrefixes := combinePrefixes(oldPrefixes, removedPrefixes, addedPrefixes)
|
|
r.dynamicDomains[domain] = updatedPrefixes
|
|
|
|
r.statusRecorder.UpdateResolvedDomainsStates(domain, domain, updatedPrefixes, r.route.GetResourceID())
|
|
}
|
|
|
|
return nberrors.FormatErrorOrNil(merr)
|
|
}
|
|
|
|
func (r *Route) addRoutes(domain domain.Domain, prefixes []netip.Prefix) ([]netip.Prefix, error) {
|
|
var addedPrefixes []netip.Prefix
|
|
var merr *multierror.Error
|
|
|
|
for _, prefix := range prefixes {
|
|
if _, err := r.routeRefCounter.Increment(prefix, struct{}{}); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf("add dynamic route for IP %s: %w", prefix, err))
|
|
continue
|
|
}
|
|
if r.currentPeerKey != "" {
|
|
if err := r.incrementAllowedIP(domain, prefix, r.currentPeerKey); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf(addAllowedIP, prefix, err))
|
|
}
|
|
}
|
|
addedPrefixes = append(addedPrefixes, prefix)
|
|
}
|
|
|
|
return addedPrefixes, merr.ErrorOrNil()
|
|
}
|
|
|
|
func (r *Route) removeRoutes(prefixes []netip.Prefix) ([]netip.Prefix, error) {
|
|
if r.route.KeepRoute {
|
|
return nil, nil
|
|
}
|
|
|
|
var removedPrefixes []netip.Prefix
|
|
var merr *multierror.Error
|
|
|
|
for _, prefix := range prefixes {
|
|
if _, err := r.routeRefCounter.Decrement(prefix); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf("remove dynamic route for IP %s: %w", prefix, err))
|
|
}
|
|
if r.currentPeerKey != "" {
|
|
if _, err := r.allowedIPsRefcounter.Decrement(prefix, r.currentPeerKey); err != nil {
|
|
merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %w", prefix, err))
|
|
}
|
|
}
|
|
removedPrefixes = append(removedPrefixes, prefix)
|
|
}
|
|
|
|
return removedPrefixes, merr.ErrorOrNil()
|
|
}
|
|
|
|
func (r *Route) incrementAllowedIP(domain domain.Domain, prefix netip.Prefix, peerKey string) error {
|
|
if ref, err := r.allowedIPsRefcounter.Increment(prefix, peerKey); err != nil {
|
|
return fmt.Errorf(addAllowedIP, prefix, err)
|
|
} else if ref.Count > 1 && ref.Out != peerKey {
|
|
log.Warnf("IP [%s] for domain [%s] is already routed by peer [%s]. HA routing disabled",
|
|
prefix.Addr(),
|
|
domain.SafeString(),
|
|
ref.Out,
|
|
)
|
|
|
|
}
|
|
return nil
|
|
}
|
|
|
|
func determinePrefixChanges(oldPrefixes, newPrefixes []netip.Prefix) (toAdd, toRemove []netip.Prefix) {
|
|
prefixSet := make(map[netip.Prefix]bool)
|
|
for _, prefix := range oldPrefixes {
|
|
prefixSet[prefix] = false
|
|
}
|
|
for _, prefix := range newPrefixes {
|
|
if _, exists := prefixSet[prefix]; exists {
|
|
prefixSet[prefix] = true
|
|
} else {
|
|
toAdd = append(toAdd, prefix)
|
|
}
|
|
}
|
|
for prefix, inUse := range prefixSet {
|
|
if !inUse {
|
|
toRemove = append(toRemove, prefix)
|
|
}
|
|
}
|
|
return
|
|
}
|
|
|
|
// lookupHostIPs resolves d via the system resolver, honoring ctx cancellation.
|
|
func lookupHostIPs(ctx context.Context, d domain.Domain) ([]net.IP, error) {
|
|
addrs, err := net.DefaultResolver.LookupIPAddr(ctx, d.PunycodeString())
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
ips := make([]net.IP, 0, len(addrs))
|
|
for _, addr := range addrs {
|
|
ips = append(ips, addr.IP)
|
|
}
|
|
return ips, nil
|
|
}
|
|
|
|
func combinePrefixes(oldPrefixes, removedPrefixes, addedPrefixes []netip.Prefix) []netip.Prefix {
|
|
prefixSet := make(map[netip.Prefix]struct{})
|
|
for _, prefix := range oldPrefixes {
|
|
prefixSet[prefix] = struct{}{}
|
|
}
|
|
for _, prefix := range removedPrefixes {
|
|
delete(prefixSet, prefix)
|
|
}
|
|
for _, prefix := range addedPrefixes {
|
|
prefixSet[prefix] = struct{}{}
|
|
}
|
|
|
|
var combinedPrefixes []netip.Prefix
|
|
for prefix := range prefixSet {
|
|
combinedPrefixes = append(combinedPrefixes, prefix)
|
|
}
|
|
|
|
return combinedPrefixes
|
|
}
|