Skip to content

[client] Add macOS default resolvers as fallback#5201

Merged
lixmal merged 1 commit intomainfrom
macos-fallback-dns
Jan 30, 2026
Merged

[client] Add macOS default resolvers as fallback#5201
lixmal merged 1 commit intomainfrom
macos-fallback-dns

Conversation

@lixmal
Copy link
Copy Markdown
Collaborator

@lixmal lixmal commented Jan 28, 2026

Describe your changes

This PR adds the detected default upstream nameserves as fallback handlers in the handler chain (prio -100).

This is useful if there is a dns route like test.example.org that makes macOS use the whole zone as match domain.
Now if there is no other upstream nameserver configured, requests to morespecific.test.example.org would fail since they're not matched by said DNS route.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

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

    • Improved DNS nameserver tracking and retrieval on macOS systems with thread-safe access.
    • Enhanced fallback DNS handling when original nameservers are unavailable.
  • Tests

    • Added comprehensive tests for DNS nameserver management and configuration transitions.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 28, 2026

📝 Walkthrough

Walkthrough

The changes implement concurrency-safe tracking and retrieval of original nameservers in the macOS DNS configurator. DNS parsing was expanded to accumulate all server addresses, with a new accessor method providing thread-safe read access. Fallback handler deregistration logic was added for when no original nameservers exist. Test coverage includes unit tests for the getter, system DNS loading, and nameserver persistence across configuration transitions.

Changes

Cohort / File(s) Summary
Original Nameserver Tracking
client/internal/dns/host_darwin.go
Added mu sync.RWMutex and origNameservers []netip.Addr fields to systemConfigurator. Expanded getSystemDNSSettings to accumulate all server addresses instead of stopping at first IPv4; addresses are unmapped and stored under write lock. Added getOriginalNameservers() method for thread-safe read access. Updated imports to include slices and sync.
Nameserver Tracking Tests
client/internal/dns/host_darwin_test.go
Added five new test functions: TestGetOriginalNameservers (verifies getter retrieval), TestGetOriginalNameserversFromSystem (validates system DNS loading), TestOriginalNameserversNoTransition (ensures persistence without RouteAll changes), TestOriginalNameserversRouteAllTransition (verifies persistence across config transitions). Added setupTestConfigurator helper function.
Fallback Handler Logic & Mock Utilities
client/internal/dns/server.go, client/internal/dns/test/mock.go
Modified registerFallback to deregister RootZone fallback when no original nameservers exist. Enhanced MockResponseWriter with lastResponse field and GetLastResponse() method for test response inspection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • pappz

Poem

🐰 A mutex guards the names so dear,
Original servers stored with care,
Concurrent access, safe and sound,
No race conditions to be found!
Tests ensure the config stays,
Through all its changing DNS ways. 🍀

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title directly reflects the main change: adding macOS default resolvers as fallback DNS handlers in the client.
Description check ✅ Passed The PR description adequately covers the change with context about the DNS routing scenario and justifies why documentation is not needed; all required template sections are addressed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lixmal lixmal force-pushed the macos-fallback-dns branch from 828ab9b to 0d2ff11 Compare January 28, 2026 09:01
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@client/internal/dns/host_darwin_test.go`:
- Around line 114-120: The test uses an incorrect struct field name for
systemConfigurator: change the literal field originalNameservers to the actual
field name origNameservers so the struct literal matches the systemConfigurator
definition (update the initializer that sets the nameserver slice in the test
where systemConfigurator is constructed).

In `@client/internal/dns/host_darwin.go`:
- Around line 252-256: The code only assigns dnsSettings.ServerIP for IPv4
addresses, leaving it invalid on IPv6-only systems; update the loop that parses
addresses (netip.ParseAddr) so dnsSettings.ServerIP is set to the first valid
address as a fallback but still prefer IPv4: when iterating addresses, append
ip.Unmap() to dnsSettings.ServerAddresses as before; if dnsSettings.ServerIP is
not valid, set it to ip.Unmap(); if ip.Is4() then always assign/overwrite
dnsSettings.ServerIP with ip.Unmap() so IPv4 wins when present but IPv6-only
systems still get a usable ServerIP for addLocalDNS.
- Around line 12-15: Assigning dnsSettings.ServerAddresses to s.origNameservers
creates a shared mutable reference; change the write to store a defensive copy
using slices.Clone (e.g., set s.origNameservers =
slices.Clone(dnsSettings.ServerAddresses)) to match the read-side defensive
clone in the code that uses slices.Clone when returning nameservers, and remove
any comment or conditional about Go version compatibility since slices is
supported (no need to worry about Go 1.25).

Comment thread client/internal/dns/host_darwin_test.go
Comment thread client/internal/dns/host_darwin.go
Comment thread client/internal/dns/host_darwin.go
@lixmal lixmal force-pushed the macos-fallback-dns branch from 0d2ff11 to 144700c Compare January 28, 2026 09:27
@sonarqubecloud
Copy link
Copy Markdown

@lixmal lixmal merged commit 101c813 into main Jan 30, 2026
38 of 39 checks passed
@lixmal lixmal deleted the macos-fallback-dns branch January 30, 2026 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants