Add Darwin multi-server support for local DNS settings #4837
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSystem DNS handling was changed to support multiple resolver addresses: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR enhances DNS configuration on macOS to support multiple DNS servers instead of a single server. The change allows collecting and configuring all available DNS servers (both IPv4 and IPv6) from the system.
- Renamed
ServerIPtoServerIPsto reflect the change from single to multiple server support - Modified DNS settings parser to collect all DNS server addresses instead of stopping at the first IPv4
- Added domain deduplication logic to prevent duplicate entries in search domains
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client/internal/dns/host.go | Changed SystemDNSSettings struct field from ServerIP to ServerIPs slice |
| client/internal/dns/host_darwin.go | Updated DNS configuration logic to handle multiple servers, added deduplication for domains, and modified all functions to accept/process server IP slices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| searchDomains = append(searchDomains, strings.TrimSuffix(""+dConf.Domain, ".")) | ||
| } | ||
|
|
||
| configServerIPs := []netip.Addr{config.ServerIP} |
There was a problem hiding this comment.
This line attempts to access config.ServerIP which no longer exists after the API change. The HostDNSConfig struct should have a ServerIP or ServerIPs field that matches the new API. Verify that HostDNSConfig has been updated accordingly, or this should use the correct field name.
| configServerIPs := []netip.Addr{config.ServerIP} | |
| configServerIPs := config.ServerIPs |
There was a problem hiding this comment.
This is completely wrong on AI's part
| if ip.IsValid() { | ||
| dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap()) | ||
| } |
There was a problem hiding this comment.
The ip.IsValid() check is redundant here. If netip.ParseAddr(address) succeeds (err == nil), the resulting IP is guaranteed to be valid. The check can be removed to simplify the code.
| if ip.IsValid() { | |
| dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap()) | |
| } | |
| dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap()) |
There was a problem hiding this comment.
checking twice falls inline with what was previously done
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/internal/dns/host_darwin.go (2)
255-259: Consider removing redundant validation check.The parsing logic correctly collects all DNS server addresses. However, the
ip.IsValid()check on line 256 is redundant sincenetip.ParseAddronly returns a valid address whenerr == nil.The use of
ip.Unmap()to normalize IPv4-mapped IPv6 addresses is good practice.Apply this diff to simplify the validation:
} else if inServerAddressesArray { address := strings.Split(line, " : ")[1] if ip, err := netip.ParseAddr(address); err == nil { - if ip.IsValid() { - dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap()) - } + dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap()) } }
299-323: Consider validating non-empty dnsServers slice.The implementation correctly converts the
dnsServersslice to a space-separated string format for scutil. However, if an empty slice is passed, this would result in an emptyserversStr, which might not be the intended behavior.Consider adding validation at the start of the function:
func (s *systemConfigurator) addDNSState(state, domains string, dnsServers []netip.Addr, port int, enableSearch bool) error { if len(dnsServers) == 0 { return fmt.Errorf("dnsServers cannot be empty") } noSearch := "1" if enableSearch { noSearch = "0" } // ... rest of implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/internal/dns/host.go(1 hunks)client/internal/dns/host_darwin.go(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
🔇 Additional comments (5)
client/internal/dns/host_darwin.go (4)
82-82: LGTM: Correct bridge from single server to slice-based functions.Creating a slice with the single NetBird DNS server IP correctly adapts the single-server
HostDNSConfigto the multi-server function signatures.
172-190: LGTM: Correct usage of ServerIPs slice for local DNS configuration.The empty slice checks and usage of
s.systemDNSSettings.ServerIPsare correct. This properly leverages the system's existing DNS servers for local domain resolution.
222-252: Good improvement: Domain deduplication prevents duplicates in search domains.The use of
localDomainsMapto deduplicate bothDomainNameandSearchDomainsentries is a solid enhancement that prevents duplicate entries in the final configuration.
273-297: LGTM: Function signatures correctly updated for multi-server support.Both
addSearchDomainsandaddMatchDomainsnow accept[]netip.Addrparameters and properly forward them toaddDNSState. The signature updates are consistent and correct.client/internal/dns/host.go (1)
28-28: No issues found. All references toSystemDNSSettings.ServerIPhave been properly updated toServerIPs.The verification confirms the field rename is complete and correct. All remaining
ServerIPreferences in the codebase are toHostDNSConfig(a different struct with typenetip.Addr), not toSystemDNSSettings. This is intentional—HostDNSConfig.ServerIPis singular and represents the single DNS server configuration at that layer, whileSystemDNSSettings.ServerIPsis a slice supporting multi-server capability.
hurricanehrndz
force-pushed
the
fix/darwin-dns-multi-server-support
branch
from
7a78e27 to
2d7b9a0
Compare
hurricanehrndz
force-pushed
the
fix/darwin-dns-multi-server-support
branch
from
2d7b9a0 to
b1c6513
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/internal/dns/host_darwin.go (1)
234-247: Consider defensive bounds checking on string splits.The parsing logic splits on
" : "and directly accesses index[1]at lines 235 and 241. While system output format is stable and tests validate the expected structure, adding bounds checking would make the code more resilient to unexpected formats.Example defensive approach:
if inSearchDomainsArray { - searchDomain := strings.Split(line, " : ")[1] + parts := strings.Split(line, " : ") + if len(parts) < 2 { + continue + } + searchDomain := parts[1] if _, exists := localDomainsMap[searchDomain]; !exists { localDomainsMap[searchDomain] = struct{}{} dnsSettings.Domains = append(dnsSettings.Domains, searchDomain) } } else if inServerAddressesArray { - address := strings.Split(line, " : ")[1] + parts := strings.Split(line, " : ") + if len(parts) < 2 { + continue + } + address := parts[1] if ip, err := netip.ParseAddr(address); err == nil { if ip.IsValid() { dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap()) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/internal/dns/host.go(1 hunks)client/internal/dns/host_darwin.go(9 hunks)client/internal/dns/host_darwin_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/internal/dns/host.go (1)
shared/relay/client/dialer/quic/conn.go (1)
Addr(20-22)
client/internal/dns/host_darwin.go (1)
client/internal/dns/host.go (1)
SystemDNSSettings(21-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: Windows
- GitHub Check: release
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: JS / Lint
🔇 Additional comments (6)
client/internal/dns/host.go (1)
21-25: LGTM! Clean API evolution for multi-server support.The change from
ServerIP(singular) toServerIPs(slice) correctly enables SystemDNSSettings to represent multiple DNS servers. The implementation in host_darwin.go properly leverages this updated data model.client/internal/dns/host_darwin_test.go (1)
113-255: Excellent test coverage for the new parsing logic.The test suite comprehensively validates parseSystemConfigOutput across multiple scenarios:
- Complete and partial configurations
- IPv4 and IPv6 server addresses
- Domain deduplication
- Invalid input handling (empty output, malformed IPs)
- Nested arrays and additional fields
The tests provide strong confidence in the parsing implementation.
client/internal/dns/host_darwin.go (4)
82-82: LGTM! Correct handling of NetBird DNS server.Wrapping
config.ServerIPinto a slice is appropriate here, as the updated functions accept multiple DNS servers, but HostDNSConfig represents NetBird's single DNS server configuration.
172-195: LGTM! Consistent migration to slice-based checks.The updated checks using
len(s.systemDNSSettings.ServerIPs)correctly replace the previousServerIP.IsValid()pattern, and the slice is properly propagated toaddSearchDomains.
276-300: LGTM! Function signatures correctly updated.Both
addSearchDomainsandaddMatchDomainsare properly updated to accept[]netip.Addrinstead of single addresses, and correctly propagate the slices toaddDNSState.
302-326: LGTM! Clean implementation of multi-server state configuration.The conversion of the DNS server slice to a space-separated string (lines 308-312) correctly formats the data for scutil commands, enabling multiple servers to be configured in the DNS state.
Multi-Server Support (lines 304-308): - Converts slice of IPs to space-separated string DNS Server Address Collection (lines 253-257): - Collects all DNS server addresses (not just first) - Proper validation with ip.IsValid()
Extracted scutil output parsing logic from getSystemDNSSettings() into a standalone parseSystemConfigOutput() helper function. This improves code maintainability by separating parsing logic from command execution and enables comprehensive unit testing. Added 7 unit tests covering: - Complete output parsing with domains and servers - Multiple IPv4/IPv6 server handling - Domain deduplication - Empty output edge case - Invalid IP filtering - Minimal output scenarios - Array boundary handling
hurricanehrndz
force-pushed
the
fix/darwin-dns-multi-server-support
branch
from
b1c6513 to
92c85dd
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/internal/dns/host.go(1 hunks)client/internal/dns/host_darwin.go(9 hunks)client/internal/dns/host_darwin_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/internal/dns/host_darwin.go (1)
client/internal/dns/host.go (1)
SystemDNSSettings(21-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Cache
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Android / Build
🔇 Additional comments (9)
client/internal/dns/host.go (1)
23-23: LGTM! Clean API change to support multiple DNS servers.The change from a single
ServerIPto a sliceServerIPsis well-aligned with the PR objectives and consistently implemented across the Darwin-specific code.client/internal/dns/host_darwin_test.go (1)
113-255: Excellent test coverage for the new parsing logic.The test suite comprehensively covers various scenarios including:
- Multiple IPv4 and IPv6 servers
- Domain deduplication
- Invalid IP filtering
- Edge cases (empty output, domain-only configs)
This provides strong confidence in the robustness of the
parseSystemConfigOutputfunction.client/internal/dns/host_darwin.go (7)
82-82: LGTM! Correct conversion of NetBird's DNS server to slice format.This wraps the single NetBird DNS resolver IP from
HostDNSConfiginto a slice for compatibility with the updated function signatures that accept[]netip.Addr.
86-86: LGTM! Correct usage of updated function signatures.The calls to
addMatchDomainsandaddSearchDomainscorrectly pass the[]netip.Addrslice as required by their updated signatures.Also applies to: 98-98
172-185: LGTM! Proper handling of system DNS settings.The code correctly checks for empty
ServerIPsslice and passes the system DNS servers when enabling local DNS. The logic properly handles the multi-server scenario.
194-194: LGTM! Correct update for multi-server API.The length check properly replaces the previous
IsValid()check for the slice-basedServerIPsfield.
259-274: LGTM! Clean refactoring improves testability.Extracting the parsing logic into
parseSystemConfigOutputseparates concerns and makes the code more testable, as evidenced by the comprehensive test suite.
276-300: LGTM! Function signatures correctly updated for multi-server support.Both
addSearchDomainsandaddMatchDomainsnow accept[]netip.Addrslices and properly delegate toaddDNSState.
302-326: LGTM! Correct implementation of multi-server DNS state configuration.The function properly converts the
[]netip.Addrslice to a space-separated string for thescutilcommand, enabling multiple DNS servers to be configured.
| case strings.HasPrefix(line, "DomainName :"): | ||
| domainName := strings.TrimSpace(strings.Split(line, ":")[1]) | ||
| dnsSettings.Domains = append(dnsSettings.Domains, domainName) | ||
| if _, exists := localDomainsMap[domainName]; !exists { | ||
| localDomainsMap[domainName] = struct{}{} | ||
| dnsSettings.Domains = append(dnsSettings.Domains, domainName) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add case-insensitive domain deduplication.
The current deduplication logic is case-sensitive, but DNS domain names are case-insensitive. For example, "Example.com" and "example.com" should be treated as the same domain.
Apply this diff to normalize domains to lowercase before deduplication:
case strings.HasPrefix(line, "DomainName :"):
- domainName := strings.TrimSpace(strings.Split(line, ":")[1])
+ domainName := strings.ToLower(strings.TrimSpace(strings.Split(line, ":")[1]))
if _, exists := localDomainsMap[domainName]; !exists {
localDomainsMap[domainName] = struct{}{}
dnsSettings.Domains = append(dnsSettings.Domains, domainName)
} if inSearchDomainsArray {
- searchDomain := strings.Split(line, " : ")[1]
+ searchDomain := strings.ToLower(strings.Split(line, " : ")[1])
if _, exists := localDomainsMap[searchDomain]; !exists {
localDomainsMap[searchDomain] = struct{}{}
dnsSettings.Domains = append(dnsSettings.Domains, searchDomain)
}Also applies to: 234-239
🤖 Prompt for AI Agents
In client/internal/dns/host_darwin.go around lines 217-222 (and also apply same
change to lines 234-239), the deduplication uses the raw DomainName string so
"Example.com" and "example.com" are treated as distinct; normalize the domain to
lowercase before checking and inserting into localDomainsMap and before
appending to dnsSettings.Domains (use the lowered value as the deduplication key
and append the normalized lowercase domain).
| } | ||
|
|
||
| if inSearchDomainsArray { | ||
| searchDomain := strings.Split(line, " : ")[1] |
There was a problem hiding this comment.
Add bounds checking to prevent potential panics.
Lines 235 and 241 use strings.Split(line, " : ")[1] without verifying that the split produced at least 2 elements. If the system config output format is unexpected, this will cause a panic.
Apply this diff to add defensive bounds checking:
if inSearchDomainsArray {
- searchDomain := strings.Split(line, " : ")[1]
+ parts := strings.Split(line, " : ")
+ if len(parts) < 2 {
+ continue
+ }
+ searchDomain := parts[1]
if _, exists := localDomainsMap[searchDomain]; !exists {
localDomainsMap[searchDomain] = struct{}{}
dnsSettings.Domains = append(dnsSettings.Domains, searchDomain)
}
} else if inServerAddressesArray {
- address := strings.Split(line, " : ")[1]
+ parts := strings.Split(line, " : ")
+ if len(parts) < 2 {
+ continue
+ }
+ address := parts[1]
if ip, err := netip.ParseAddr(address); err == nil {
if ip.IsValid() {
dnsSettings.ServerIPs = append(dnsSettings.ServerIPs, ip.Unmap())
}
}Also applies to: 241-241
🤖 Prompt for AI Agents
In client/internal/dns/host_darwin.go around lines 235 and 241, the code calls
strings.Split(line, " : ")[1] without checking the split length which can panic;
update both locations to perform bounds checking by splitting into a slice,
verifying len(parts) >= 2 before indexing, and only assign parts[1] to
searchDomain (or other variable) when present, otherwise handle the missing
value gracefully (skip the line, continue, or set a default/empty value) and
optionally log a debug warning to aid troubleshooting.
1 participant
Describe your changes
This commit enhances Darwin (macOS) DNS configuration to support multiple DNS servers instead of just a single server:
Key Changes:
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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.