diff --git a/client/internal/networkmonitor/check_change_bsd.go b/client/internal/networkmonitor/check_change_bsd.go index bb327a877..f5eb2c739 100644 --- a/client/internal/networkmonitor/check_change_bsd.go +++ b/client/internal/networkmonitor/check_change_bsd.go @@ -19,7 +19,7 @@ import ( func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) error { fd, err := unix.Socket(syscall.AF_ROUTE, syscall.SOCK_RAW, syscall.AF_UNSPEC) if err != nil { - return fmt.Errorf("failed to open routing socket: %v", err) + return fmt.Errorf("open routing socket: %v", err) } defer func() { err := unix.Close(fd) diff --git a/client/internal/networkmonitor/check_change_windows.go b/client/internal/networkmonitor/check_change_windows.go index 582865738..814584863 100644 --- a/client/internal/networkmonitor/check_change_windows.go +++ b/client/internal/networkmonitor/check_change_windows.go @@ -13,7 +13,7 @@ import ( func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) error { routeMonitor, err := systemops.NewRouteMonitor(ctx) if err != nil { - return fmt.Errorf("failed to create route monitor: %w", err) + return fmt.Errorf("create route monitor: %w", err) } defer func() { if err := routeMonitor.Stop(); err != nil { @@ -38,35 +38,49 @@ func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) er } func routeChanged(route systemops.RouteUpdate, nexthopv4, nexthopv6 systemops.Nexthop) bool { - intf := "" - if route.Interface != nil { - intf = route.Interface.Name - if isSoftInterface(intf) { - log.Debugf("Network monitor: ignoring default route change for soft interface %s", intf) - return false - } + if intf := route.NextHop.Intf; intf != nil && isSoftInterface(intf.Name) { + log.Debugf("Network monitor: ignoring default route change for next hop with soft interface %s", route.NextHop) + return false + } + + // TODO: for the empty nexthop ip (on-link), determine the family differently + nexthop := nexthopv4 + if route.NextHop.IP.Is6() { + nexthop = nexthopv6 } switch route.Type { - case systemops.RouteModified: - // TODO: get routing table to figure out if our route is affected for modified routes - log.Infof("Network monitor: default route changed: via %s, interface %s", route.NextHop, intf) - return true - case systemops.RouteAdded: - if route.NextHop.Is4() && route.NextHop != nexthopv4.IP || route.NextHop.Is6() && route.NextHop != nexthopv6.IP { - log.Infof("Network monitor: default route added: via %s, interface %s", route.NextHop, intf) - return true - } + case systemops.RouteModified, systemops.RouteAdded: + return handleRouteAddedOrModified(route, nexthop) case systemops.RouteDeleted: - if nexthopv4.Intf != nil && route.NextHop == nexthopv4.IP || nexthopv6.Intf != nil && route.NextHop == nexthopv6.IP { - log.Infof("Network monitor: default route removed: via %s, interface %s", route.NextHop, intf) - return true - } + return handleRouteDeleted(route, nexthop) } return false } +func handleRouteAddedOrModified(route systemops.RouteUpdate, nexthop systemops.Nexthop) bool { + // For added/modified routes, we care about different next hops + if !nexthop.Equal(route.NextHop) { + action := "changed" + if route.Type == systemops.RouteAdded { + action = "added" + } + log.Infof("Network monitor: default route %s: via %s", action, route.NextHop) + return true + } + return false +} + +func handleRouteDeleted(route systemops.RouteUpdate, nexthop systemops.Nexthop) bool { + // For deleted routes, we care about our tracked next hop being deleted + if nexthop.Equal(route.NextHop) { + log.Infof("Network monitor: default route removed: via %s", route.NextHop) + return true + } + return false +} + func isSoftInterface(name string) bool { return strings.Contains(strings.ToLower(name), "isatap") || strings.Contains(strings.ToLower(name), "teredo") } diff --git a/client/internal/networkmonitor/check_change_windows_test.go b/client/internal/networkmonitor/check_change_windows_test.go new file mode 100644 index 000000000..29ff34dca --- /dev/null +++ b/client/internal/networkmonitor/check_change_windows_test.go @@ -0,0 +1,404 @@ +package networkmonitor + +import ( + "net" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/client/internal/routemanager/systemops" +) + +func TestRouteChanged(t *testing.T) { + tests := []struct { + name string + route systemops.RouteUpdate + nexthopv4 systemops.Nexthop + nexthopv6 systemops.Nexthop + expected bool + }{ + { + name: "soft interface should be ignored", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Name: "ISATAP-Interface", // isSoftInterface checks name + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.2"), + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: false, + }, + { + name: "modified route with different v4 nexthop IP should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.2"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: true, + }, + { + name: "modified route with same v4 nexthop (IP and Intf Index) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: false, + }, + { + name: "added route with different v6 nexthop IP should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteAdded, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::2"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + expected: true, + }, + { + name: "added route with same v6 nexthop (IP and Intf Index) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteAdded, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + expected: false, + }, + { + name: "deleted route matching tracked v4 nexthop (IP and Intf Index) should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: true, + }, + { + name: "deleted route not matching tracked v4 nexthop (different IP) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.3"), // Different IP + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: false, + }, + { + name: "modified v4 route with same IP, different Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "modified v4 route with same IP, one Intf nil, other non-nil should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: nil, // Intf is nil + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, // Tracked Intf is not nil + }, + expected: true, + }, + { + name: "added v4 route with same IP, different Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteAdded, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "deleted v4 route with same IP, different Intf Index should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ // This is the route being deleted + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv4: systemops.Nexthop{ // This is our tracked nexthop + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + expected: false, // Because nexthopv4.Equal(route.NextHop) will be false + }, + { + name: "modified v6 route with different IP, same Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::3"), // Different IP + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "modified v6 route with same IP, different Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "modified v6 route with same IP, same Intf Index should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: false, + }, + { + name: "deleted v6 route matching tracked nexthop (IP and Intf Index) should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "deleted v6 route not matching tracked nexthop (different IP) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::3"), // Different IP + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: false, + }, + { + name: "deleted v6 route not matching tracked nexthop (same IP, different Intf Index) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ // This is the route being deleted + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ // This is our tracked nexthop + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + expected: false, + }, + { + name: "unknown route type should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteUpdateType(99), // Unknown type + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.2"), // Different from route.NextHop + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := routeChanged(tt.route, tt.nexthopv4, tt.nexthopv6) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestIsSoftInterface(t *testing.T) { + tests := []struct { + name string + ifname string + expected bool + }{ + { + name: "ISATAP interface should be detected", + ifname: "ISATAP tunnel adapter", + expected: true, + }, + { + name: "lowercase soft interface should be detected", + ifname: "isatap.{14A5CF17-CA72-43EC-B4EA-B4B093641B7D}", + expected: true, + }, + { + name: "Teredo interface should be detected", + ifname: "Teredo Tunneling Pseudo-Interface", + expected: true, + }, + { + name: "regular interface should not be detected as soft", + ifname: "eth0", + expected: false, + }, + { + name: "another regular interface should not be detected as soft", + ifname: "wlan0", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isSoftInterface(tt.ifname) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/client/internal/networkmonitor/monitor.go b/client/internal/networkmonitor/monitor.go index 5896b66b6..accdd9c9d 100644 --- a/client/internal/networkmonitor/monitor.go +++ b/client/internal/networkmonitor/monitor.go @@ -118,9 +118,12 @@ func (nw *NetworkMonitor) Stop() { } func (nw *NetworkMonitor) checkChanges(ctx context.Context, event chan struct{}, nexthop4 systemops.Nexthop, nexthop6 systemops.Nexthop) { + defer close(event) for { if err := checkChangeFn(ctx, nexthop4, nexthop6); err != nil { - close(event) + if !errors.Is(err, context.Canceled) { + log.Errorf("Network monitor: failed to check for changes: %v", err) + } return } // prevent blocking diff --git a/client/internal/routemanager/systemops/systemops.go b/client/internal/routemanager/systemops/systemops.go index 5c117b94d..fd511fc20 100644 --- a/client/internal/routemanager/systemops/systemops.go +++ b/client/internal/routemanager/systemops/systemops.go @@ -1,6 +1,7 @@ package systemops import ( + "fmt" "net" "net/netip" "sync" @@ -15,6 +16,20 @@ type Nexthop struct { Intf *net.Interface } +// Equal checks if two nexthops are equal. +func (n Nexthop) Equal(other Nexthop) bool { + return n.IP == other.IP && (n.Intf == nil && other.Intf == nil || + n.Intf != nil && other.Intf != nil && n.Intf.Index == other.Intf.Index) +} + +// String returns a string representation of the nexthop. +func (n Nexthop) String() string { + if n.Intf == nil { + return n.IP.String() + } + return fmt.Sprintf("%s @ %d (%s)", n.IP.String(), n.Intf.Index, n.Intf.Name) +} + type ExclusionCounter = refcounter.Counter[netip.Prefix, struct{}, Nexthop] type SysOps struct { diff --git a/client/internal/routemanager/systemops/systemops_windows.go b/client/internal/routemanager/systemops/systemops_windows.go index ad325e123..f66161595 100644 --- a/client/internal/routemanager/systemops/systemops_windows.go +++ b/client/internal/routemanager/systemops/systemops_windows.go @@ -33,8 +33,7 @@ type RouteUpdateType int type RouteUpdate struct { Type RouteUpdateType Destination netip.Prefix - NextHop netip.Addr - Interface *net.Interface + NextHop Nexthop } // RouteMonitor provides a way to monitor changes in the routing table. @@ -231,15 +230,15 @@ func (rm *RouteMonitor) parseUpdate(row *MIB_IPFORWARD_ROW2, notificationType MI intf, err := net.InterfaceByIndex(idx) if err != nil { log.Warnf("failed to get interface name for index %d: %v", idx, err) - update.Interface = &net.Interface{ + update.NextHop.Intf = &net.Interface{ Index: idx, } } else { - update.Interface = intf + update.NextHop.Intf = intf } } - log.Tracef("Received route update with destination %v, next hop %v, interface %v", row.DestinationPrefix, row.NextHop, update.Interface) + log.Tracef("Received route update with destination %v, next hop %v, interface %v", row.DestinationPrefix, row.NextHop, update.NextHop.Intf) dest := parseIPPrefix(row.DestinationPrefix, idx) if !dest.Addr().IsValid() { return RouteUpdate{}, fmt.Errorf("invalid destination: %v", row) @@ -262,7 +261,7 @@ func (rm *RouteMonitor) parseUpdate(row *MIB_IPFORWARD_ROW2, notificationType MI update.Type = updateType update.Destination = dest - update.NextHop = nexthop + update.NextHop.IP = nexthop return update, nil }