-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[client] Change Soft Interface detection to include Interface Description #5156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[client] Change Soft Interface detection to include Interface Description #5156
Conversation
📝 WalkthroughWalkthroughAdds a Windows-specific soft interface checker that enumerates adapters via GetAdaptersAddresses, flags "soft" adapters (e.g., GlobalProtect/PANGP), and uses that checker in route change handling to ignore route changes originating from soft interfaces. Changes
Sequence DiagramsequenceDiagram
participant RM as Route Monitor
participant CH as Route Change Handler
participant SC as softInterfaceChecker
participant WinAPI as Windows API
RM->>CH: notify route change
CH->>SC: newSoftInterfaceChecker()
SC->>WinAPI: GetAdaptersAddresses()
WinAPI-->>SC: adapter list
SC->>SC: build softByIndex map
SC-->>CH: checker ready
CH->>SC: IsSoft(route.NextHop)
SC-->>CH: true / false
alt soft interface
CH->>CH: log debug and ignore route change
else not soft
CH->>CH: process route change
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/internal/networkmonitor/check_change_windows.go`:
- Around line 47-56: The nil-check on checker in routeChanged causes
soft-interface filtering to be skipped when the checker is nil; remove the
conditional guard so you always call checker.IsSoft and instead make
softInterfaceChecker.IsSoft tolerate a nil receiver (handle nil inside IsSoft
and return false when no match), and apply the same change to the other similar
call site (the block at 151-157) so name-based soft-interface detection remains
active even if GetAdaptersAddresses or the caller yields a nil checker.
🧹 Nitpick comments (1)
client/internal/networkmonitor/check_change_windows.go (1)
35-40: Consider caching the soft-interface checker outside the loop.Line 35 rebuilds the adapter index on every default-route update;
GetAdaptersAddressescan be relatively expensive and may stall the monitor during bursts. Building once and refreshing on error/interval keeps the hot path lighter.♻️ Suggested refactor
- for { + checker, err := newSoftInterfaceChecker() + if err != nil { + log.Warnf("Network monitor: failed to build soft interface checker: %v", err) + } + + for { select { case <-ctx.Done(): return ctx.Err() case route := <-routeMonitor.RouteUpdates(): if route.Destination.Bits() != 0 { continue } - checker, err := newSoftInterfaceChecker() - if err != nil { - log.Warnf("Network monitor: failed to build soft interface checker: %v", err) - } + if checker == nil { + checker, err = newSoftInterfaceChecker() + if err != nil { + log.Warnf("Network monitor: failed to build soft interface checker: %v", err) + } + } if routeChanged(route, nexthopv4, nexthopv6, checker) { return nil } } }
|



Describe your changes
Changed the Soft Interface detection on Windows to include looking at the Interface Description as well as the Name. Also added support to detect a PANGP (Palo Alto Networks GlobalProtect) as a Soft Interface.
Issue ticket number and link
This fixes #5077 on Windows
Stack
Checklist
Documentation
Select exactly one:
No changes to anything that's configurable.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.