fix: use configured STUN hostnames - #130
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns STUN server usage with Config.stun_servers by resolving host:port entries at runtime (DNS A lookup or IPv4 literal), while preserving deterministic numeric IPv4 fallbacks when resolution fails. This removes reliance on embedded provider IPs as the primary runtime source and updates documentation accordingly.
Changes:
- Add parsing + resolution utilities in
nat/stun.zigto turn configuredhost:portstrings intoStunServerendpoints with numeric fallback. - Wire
upandconnectSTUN discovery through the configured-or-default server selection. - Update NAT/config documentation to describe runtime hostname resolution and fallback behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/nat/stun.zig | Adds parsing/resolution of configured host:port STUN specs and fallback selection logic, plus tests. |
| src/main.zig | Routes STUN discovery in up/connect through the configured STUN selector. |
| src/lib.zig | Ensures STUN module declarations are referenced in the library test root. |
| docs/guide/configuration.md | Updates configuration docs to reflect runtime resolution + numeric fallback. |
| docs/concepts/nat-traversal.md | Updates NAT traversal docs to reflect runtime resolution + numeric fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn resolveServerSpec(spec: []const u8) ?StunServer { | ||
| const parsed = parseServerSpec(spec) orelse return null; | ||
| const host = parseIpv4(parsed.host) orelse Dns.resolveA(parsed.host) orelse return null; | ||
| return .{ .host = host, .port = parsed.port }; | ||
| } |
| // Pick STUN server used (for token) | ||
| const stun_server = Stun.DEFAULT_STUN_SERVERS[0]; | ||
| const stun_server = stun_servers[0]; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 583f5b9602
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| pub fn resolveServerSpec(spec: []const u8) ?StunServer { | ||
| const parsed = parseServerSpec(spec) orelse return null; | ||
| const host = parseIpv4(parsed.host) orelse Dns.resolveA(parsed.host) orelse return null; |
There was a problem hiding this comment.
In environments whose /etc/resolv.conf nameserver is not an all-same-octet address (for example systemd-resolved 127.0.0.53 or Docker 127.0.0.11), this call currently times out because Dns.sendQuery builds its sockaddr.in with the nameserver bytes reversed. resolveConfiguredServers then drops every hostname and configuredOrDefault silently falls back to the embedded IPs, so the default stun.l.google.com/stun.cloudflare.com hostnames are not actually used; please fix or bypass that resolver before depending on Dns.resolveA here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd14e27871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parsed = parseServerSpec(spec) orelse return null; | ||
| const host = parseIpv4(parsed.host) orelse blk: { | ||
| if (!shouldResolveHostname(parsed.host)) return null; | ||
| break :blk Dns.resolveA(parsed.host) orelse return null; |
There was a problem hiding this comment.
On macOS and FreeBSD, Dns.resolveA always returns null because getNameservers only handles Linux/Windows and returns 0 otherwise; this call therefore drops every hostname in Config.stun_servers and configuredOrDefault falls back to the embedded IPs. That means the new runtime hostname resolution does not work on the supported BSD builds, so provider IP changes or hostname-only custom lists are still ignored there unless entries are numeric IPs.
Useful? React with 👍 / 👎.
Summary
host:portentries fromConfig.stun_serversat runtime instead of using embedded provider IPs first.upandconnectthrough the configured STUN selector and update NAT/config docs.Closes #127
Validation
zig build test --summary all(103/103 tests passed)zig build --summary allgit diff --checkmeshguard up --gossip-only --openwithout--announcediscovered a public STUN endpoint,statussucceeded, anddownstopped the daemon with exit 0zig build -Dtarget=x86_64-linux-gnu -Dno-sodium=true --summary allzig build -Dtarget=x86_64-macos -Doptimize=ReleaseFast --summary allzig build -Dtarget=x86_64-freebsd -Doptimize=ReleaseFast --summary all