[client] Add IPv6 routing, fake IPs, and DNS bind selection#5706
Merged
lixmal merged 9 commits intoclient-ipv6-acl-uspfrom Apr 9, 2026
Merged
[client] Add IPv6 routing, fake IPs, and DNS bind selection#5706lixmal merged 9 commits intoclient-ipv6-acl-uspfrom
lixmal merged 9 commits intoclient-ipv6-acl-uspfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR modernizes address type representations across the networking stack by replacing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/internal/dns/upstream_ios.go (1)
127-129:⚠️ Potential issue | 🟡 MinorPotential nil dereference in
getInterfaceIndex.If
net.InterfaceByNamereturns an error,ifacewill be nil and accessingiface.Indexwill panic.🛡️ Proposed fix
func getInterfaceIndex(interfaceName string) (int, error) { iface, err := net.InterfaceByName(interfaceName) + if err != nil { + return 0, err + } return iface.Index, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/dns/upstream_ios.go` around lines 127 - 129, The getInterfaceIndex function risks a nil dereference because it returns iface.Index without checking err; update getInterfaceIndex to check the error from net.InterfaceByName before accessing iface (e.g., if err != nil return 0, err), and only read and return iface.Index when iface is non-nil and err == nil so the function never dereferences a nil iface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/internal/routemanager/fakeip/fakeip.go`:
- Around line 88-119: The public APIs AllocateFakeIP, GetFakeIP and GetRealIP
currently dispatch by m.pool(ip) without validating or normalizing inputs; add
validation to each method to first call ip.Unmap() and then check ip.IsValid(),
returning an error (for AllocateFakeIP) or (false) for getters if invalid, and
use the unmapped/normalized addr for subsequent calls to m.pool() so IPv4-mapped
IPv6 addresses are routed to the v4 pool and zero/invalid addrs are rejected;
update references to pool(), allocate(), p.allocated and p.fakeToReal to use the
normalized address.
In `@client/internal/routemanager/systemops/systemops_generic.go`:
- Around line 225-239: The IPv6 blackhole addition currently returns on failure
leaving IPv4 split routes in place; change this to a soft-fail: when
!r.wgInterface.Address().HasIPv6() and either
r.addToRouteTable(splitDefaultv6_1, nextHop) or
r.addToRouteTable(splitDefaultv6_2, nextHop) fails, do not return an error—log a
warning (with the error) and, if the second add fails, attempt to rollback the
first but treat rollback failures as warnings too—this keeps the IPv4 split
routes (added earlier) intact and mirrors the existing IPv6 soft-failure pattern
instead of aborting the operation.
---
Outside diff comments:
In `@client/internal/dns/upstream_ios.go`:
- Around line 127-129: The getInterfaceIndex function risks a nil dereference
because it returns iface.Index without checking err; update getInterfaceIndex to
check the error from net.InterfaceByName before accessing iface (e.g., if err !=
nil return 0, err), and only read and return iface.Index when iface is non-nil
and err == nil so the function never dereferences a nil iface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f99a061-a328-4706-bffd-c83b8f8218df
📒 Files selected for processing (18)
client/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/firewall/uspfilter/forwarder/tcp.goclient/firewall/uspfilter/forwarder/udp.goclient/internal/dns/upstream.goclient/internal/dns/upstream_android.goclient/internal/dns/upstream_general.goclient/internal/dns/upstream_ios.goclient/internal/routemanager/client/client.goclient/internal/routemanager/dnsinterceptor/handler.goclient/internal/routemanager/dynamic/route.goclient/internal/routemanager/dynamic/route_ios.goclient/internal/routemanager/fakeip/fakeip.goclient/internal/routemanager/fakeip/fakeip_test.goclient/internal/routemanager/manager.goclient/internal/routemanager/systemops/systemops_generic.goclient/internal/routemanager/systemops/systemops_linux.go
lixmal
force-pushed
the
client-ipv6-routing
branch
2 times, most recently
from
1bafb87 to
1ffe411
Compare
lixmal
force-pushed
the
client-ipv6-routing
branch
from
1ffe411 to
da6f610
Compare
This was referenced Mar 26, 2026
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
lixmal
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/internal/routemanager/fakeip/fakeip.go (1)
76-77: Considersync.RWMutexfor read-heavy lookups.
GetFakeIPandGetRealIPare read-only but currently take an exclusive lock. If these paths are hot, switching toRWMutex+RLockmay reduce contention.♻️ Optional refactor
type Manager struct { - mu sync.Mutex + mu sync.RWMutex v4 *fakeIPPool v6 *fakeIPPool } @@ func (m *Manager) GetFakeIP(realIP netip.Addr) (netip.Addr, bool) { @@ - m.mu.Lock() - defer m.mu.Unlock() + m.mu.RLock() + defer m.mu.RUnlock() @@ func (m *Manager) GetRealIP(fakeIP netip.Addr) (netip.Addr, bool) { @@ - m.mu.Lock() - defer m.mu.Unlock() + m.mu.RLock() + defer m.mu.RUnlock()Also applies to: 116-117, 130-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/routemanager/fakeip/fakeip.go` around lines 76 - 77, The mutex "mu" is currently a sync.Mutex causing exclusive locks for read-only lookup paths; change mu's type to sync.RWMutex and update read-only methods (e.g., GetFakeIP, GetRealIP and the other read-heavy lookup functions referenced) to use mu.RLock()/mu.RUnlock() while keeping write paths (where v4 is mutated) using mu.Lock()/mu.Unlock(); ensure you update all Lock/Unlock calls tied to mu to the appropriate RLock/RUnlock or Lock/Unlock in the fakeip pool methods so reads are concurrent-safe and writes remain exclusive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/internal/routemanager/dynamic/route_ios.go`:
- Around line 30-37: The loop that calls nbdns.ExchangeWithFallback for each
qtype (dns.TypeA and dns.TypeAAAA) should not abort the entire function on a
transport error for one family if the other query already produced usable
answers; modify the logic in the loop around nbdns.ExchangeWithFallback (and the
surrounding variables msg, response, err, domain.SafeString(), startTime,
r.resolverAddr) to collect/append successful answers into a single result set
and on per-query error just record the error and continue; after the loop, if
the combined answer set is non-empty return it, otherwise return an aggregated
error (or the last error) indicating both lookups failed.
---
Nitpick comments:
In `@client/internal/routemanager/fakeip/fakeip.go`:
- Around line 76-77: The mutex "mu" is currently a sync.Mutex causing exclusive
locks for read-only lookup paths; change mu's type to sync.RWMutex and update
read-only methods (e.g., GetFakeIP, GetRealIP and the other read-heavy lookup
functions referenced) to use mu.RLock()/mu.RUnlock() while keeping write paths
(where v4 is mutated) using mu.Lock()/mu.Unlock(); ensure you update all
Lock/Unlock calls tied to mu to the appropriate RLock/RUnlock or Lock/Unlock in
the fakeip pool methods so reads are concurrent-safe and writes remain
exclusive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bb62c08-34cb-48dc-bdfd-ef2c80b05d57
📒 Files selected for processing (20)
client/anonymize/anonymize.goclient/anonymize/anonymize_test.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/firewall/uspfilter/forwarder/tcp.goclient/firewall/uspfilter/forwarder/udp.goclient/internal/dns/upstream.goclient/internal/dns/upstream_android.goclient/internal/dns/upstream_general.goclient/internal/dns/upstream_ios.goclient/internal/routemanager/client/client.goclient/internal/routemanager/dnsinterceptor/handler.goclient/internal/routemanager/dynamic/route.goclient/internal/routemanager/dynamic/route_ios.goclient/internal/routemanager/fakeip/fakeip.goclient/internal/routemanager/fakeip/fakeip_test.goclient/internal/routemanager/manager.goclient/internal/routemanager/systemops/systemops_generic.goclient/internal/routemanager/systemops/systemops_linux.go
✅ Files skipped from review due to trivial changes (4)
- client/internal/dns/upstream.go
- client/anonymize/anonymize_test.go
- client/internal/routemanager/manager.go
- client/firewall/uspfilter/forwarder/tcp.go
🚧 Files skipped from review as they are similar to previous changes (4)
- client/firewall/uspfilter/forwarder/endpoint.go
- client/internal/routemanager/systemops/systemops_generic.go
- client/internal/dns/upstream_android.go
- client/internal/routemanager/fakeip/fakeip_test.go
lixmal
force-pushed
the
client-ipv6-acl-usp
branch
from
71266a5 to
ed5cfa6
Compare
# Conflicts: # client/firewall/uspfilter/forwarder/forwarder.go # client/firewall/uspfilter/forwarder/icmp.go
7 tasks
pappz
requested changes
Apr 8, 2026
| var currentMTU uint16 = iface.DefaultMTU | ||
|
|
||
| // privateClientIface is the subset of the WireGuard interface needed by GetClientPrivate. | ||
| type privateClientIface interface { |
Collaborator
There was a problem hiding this comment.
It required by ios only. Move into the _ios.go
Collaborator
Author
There was a problem hiding this comment.
Can't, the signature of the stubs for other platforms also require it.
|
This was referenced Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
fakeippackage with testsStacked on #5688.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#667