NM-277: Skip egress routes that conflict with local network interfaces#1265
NM-277: Skip egress routes that conflict with local network interfaces#1265abhishek9686 wants to merge 3 commits intodevelopfrom
Conversation
Filter out egress routes whose network range overlaps with addresses already assigned to the device's network interfaces, preventing routing conflicts when an egress range matches a locally-connected subnet.
|
Tenki Code Review - Complete Files Reviewed: 1 By Severity:
This PR adds egress route conflict detection to skip routes that overlap with local network interfaces. The approach is sound, but there are two correctness issues: the cache stores already-filtered routes which creates a subtle inconsistency for future comparisons, and the overlap detection only checks one direction of subnet containment. Files Reviewed (1 files) |
There was a problem hiding this comment.
Overview
PR #1265 introduces conflict detection for egress routes that would clash with addresses already assigned to local network interfaces (excluding the netmaker WG interface). Two new functions are added to wireguard/types.go: isNetworkPresentOnLocalInterface and filterConflictingRoutes. These are called in both SetEgressRoutes and SetRoutesFromCache.
Issues Found
Cache Stores Filtered Routes — Inconsistency with Future Re-evaluation
In SetEgressRoutes, filterConflictingRoutes is applied to addrs at line 257, and then the filtered result is stored into EgressRouteCache (lines 266, 272). In SetRoutesFromCache (line 279), filterConflictingRoutes is applied again to these already-filtered cached values.
This creates a behavioral inconsistency: if a conflicting local interface is later removed, the next call to SetEgressRoutes will produce fresh (unfiltered) routes that differ from the previously stored filtered routes, causing checkEgressRoutes to report a change and trigger an unnecessary route removal + re-add cycle. The cache should ideally hold the canonical unfiltered route set, with filtering applied transiently at the point of SetRoutes calls — which is what SetRoutesFromCache correctly does, but SetEgressRoutes does not.
Unidirectional Subnet Containment Check
In isNetworkPresentOnLocalInterface (line 151), the check network.Contains(ipNet.IP) only detects conflicts where the local interface IP falls inside the egress CIDR. It misses the reverse case: a broad local interface subnet that contains the egress network (e.g., local interface 10.0.0.0/8, egress route 10.1.2.0/24). In this case, installing the egress route would shadow traffic that currently routes through the local interface. Bidirectional overlap detection (network.Contains(ipNet.IP) || ipNet.Contains(network.IP)) would catch both cases.
Positive Aspects
- The general design of filtering routes before applying them is correct and addresses a real routing conflict scenario.
- Reuse of
filterConflictingRoutesinSetRoutesFromCacheis appropriate (re-evaluating conflicts at install time). - The WG interface is correctly excluded from conflict checks via
ncifaceNamecomparison. - The
slog.Warnlogging provides useful operational visibility.
Co-authored-by: tenki-reviewer[bot] <262613592+tenki-reviewer[bot]@users.noreply.github.com>
Filter out egress routes whose network range overlaps with addresses already assigned to the device's network interfaces, preventing routing conflicts when an egress range matches a locally-connected subnet.
Describe your changes
Provide Issue ticket number if applicable/not in title
Provide link to Netmaker PR if required
Provide testing steps
Checklist before requesting a review