[server] add interface name to DNS state on Darwin#4633
[server] add interface name to DNS state on Darwin#4633siriobalmelli wants to merge 0 commit intonetbirdio:mainfrom
Conversation
|
|
|
|
@nazarewk you might be interested in this; combined with nix-darwin/nix-darwin#1610 it allows participating in multiple independent overlay networks on a single nix-darwin host. |
📝 WalkthroughWalkthroughPer-interface DNS state was added for Darwin: interface names are threaded into DNS state keys, host manager initialization, shutdown state, and DNS apply/cleanup logic so DNS settings and keys are scoped by network interface. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_darwin.go(10 hunks)client/internal/dns/server_darwin.go(1 hunks)client/internal/dns/unclean_shutdown_darwin.go(1 hunks)
🔇 Additional comments (7)
client/internal/dns/host_darwin.go (4)
58-62: LGTM! Early state initialization is a good practice.Writing the initial state with just the interface name at the beginning of
applyDNSConfigensures that the shutdown state is recorded even if subsequent operations fail. Subsequent calls toupdateStatewill populateCreatedKeysas DNS configuration progresses.
124-129: LGTM! State updates now include interface scoping.The shutdown state correctly includes both the interface name and created keys, enabling proper per-interface cleanup and restoration.
192-200: Improved logic with explicit validation.The refactored conditional now only adds local DNS when both the server IP is valid AND domains exist, making the requirements explicit. The early return with a clear log message improves code clarity.
373-375: LGTM! Helper function signature matches the updated key format.The updated signature correctly accepts format, interface name, and key suffix, aligning with the two-placeholder format of
netbirdDNSStateKeyFormat.client/internal/dns/unclean_shutdown_darwin.go (2)
10-11: LGTM! Proper state persistence with JSON tags.The addition of JSON tags ensures that both
InterfaceNameandCreatedKeysare correctly marshaled and persisted for unclean shutdown recovery.
19-26: LGTM! Cleanup now scoped to the specific interface.The host manager is correctly initialized with the stored interface name, and the created keys are properly restored from the shutdown state before performing DNS restoration. This ensures cleanup only affects DNS entries for the specific interface.
client/internal/dns/server_darwin.go (1)
6-6: [rewritten comment]
[classification tag]
|
2 participants
Describe your changes
Fix bug on Darwin where different instances of netbird clobber each other's DNS entries:
disambiguate 'netbirdDNSStateKeyFormat' by including the interface name.
Issue ticket number and link
Related to #446: allows proper DNS resolution when multiple server instances are run on Darwin/MacOS, each instance with its own interface and state.
Stack
Checklist
Documentation
Select exactly one:
It is reasonable to assume that multiple instances of
netbird servercan run on a Darwin machine, as long as they bind to different interfaces and have separate runtime and state directories; in this case each should push its own independent resolver record to the resolver.This is already the case on Linux, and this bugfix brings feature parity to Darwin.
Summary by CodeRabbit