Skip to content

[client] Fix IPv6 address formatting in DNS address construction#5603

Merged
lixmal merged 1 commit intonetbirdio:mainfrom
mango766:fix/ipv6-address-formatting-dns
Mar 17, 2026
Merged

[client] Fix IPv6 address formatting in DNS address construction#5603
lixmal merged 1 commit intonetbirdio:mainfrom
mango766:fix/ipv6-address-formatting-dns

Conversation

@mango766
Copy link
Copy Markdown
Contributor

@mango766 mango766 commented Mar 16, 2026

Summary

Fixes #5601
Related to #4074

Several places in the DNS client code construct server addresses using fmt.Sprintf("%s:%d", ip, port). This produces malformed addresses when the IP is IPv6, since IPv6 addresses contain colons and need brackets when combined with a port (e.g., [2606:4700:4700::1111]:53 instead of 2606:4700:4700::1111:53).

This causes DNS resolution to fail with dial udp: address 2606:4700:4700::1111:53: too many colons in address when IPv6 nameservers are configured.

Changes

Replaced fmt.Sprintf("%s:%d", ip, port) with net.JoinHostPort() in:

  • client/internal/routemanager/dnsinterceptor/handler.go — upstream DNS address for the DNS interceptor (most critical, directly causes query failures)
  • client/internal/dns/service_listener.go — DNS listener address and port binding probes
  • client/internal/routemanager/client/client.go — DNS address for dynamic route handlers

net.JoinHostPort() is the standard Go idiom for this — it automatically wraps IPv6 addresses in brackets. The rest of the codebase already uses this function in many places (e.g., peer/worker_ice.go, ssh/detection/detection.go).

Test plan

  • Verify existing DNS tests still pass (go test ./client/internal/dns/...)
  • Verify route manager tests pass (go test ./client/internal/routemanager/...)
  • Configure an IPv6 nameserver (e.g., Cloudflare 2606:4700:4700::1111) and verify DNS queries succeed
  • Verify IPv4 nameserver behavior is unchanged

Summary by CodeRabbit

  • Refactor
    • Enhanced internal DNS address handling with standardized formatting utilities, improving code consistency and maintainability across core networking components.

Replace fmt.Sprintf("%s:%d", ip, port) with net.JoinHostPort() to
properly handle IPv6 addresses that need bracket wrapping (e.g.,
[2606:4700:4700::1111]:53 instead of 2606:4700:4700::1111:53).

Without this fix, configuring IPv6 nameservers causes "too many colons
in address" errors because Go's net.Dial cannot parse the malformed
address string.

Fixes netbirdio#5601
Related to netbirdio#4074

Co-Authored-By: Claude (claude-opus-4-6) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 41b5c730-99d1-4fda-8908-792ff324f962

📥 Commits

Reviewing files that changed from the base of the PR and between 3e6baea and e9474f9.

📒 Files selected for processing (3)
  • client/internal/dns/service_listener.go
  • client/internal/routemanager/client/client.go
  • client/internal/routemanager/dnsinterceptor/handler.go

📝 Walkthrough

Walkthrough

This PR fixes IPv6 address formatting in DNS address construction by replacing fmt.Sprintf with net.JoinHostPort across three DNS-related files. The fix ensures IPv6 addresses are properly bracketed when combined with ports.

Changes

Cohort / File(s) Summary
DNS Service Listener
client/internal/dns/service_listener.go
Replaced fmt.Sprintf("%s:%d", ip, port) with net.JoinHostPort(ip.String(), strconv.Itoa(port)) in DNS listener address construction to support IPv6 addresses.
Route Manager DNS Handling
client/internal/routemanager/client/client.go, client/internal/routemanager/dnsinterceptor/handler.go
Replaced DNS address string formatting with net.JoinHostPort() for dynamic route handlers and upstream DNS interceptor to properly format IPv6 addresses with bracketed notation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • pappz

Poem

🐰 IPv6 brackets now in place,
No more colons in the space,
DNS queries flow with grace,
Addresses formatted just right! 🌐

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: fixing IPv6 address formatting in DNS address construction across the client code.
Description check ✅ Passed The description provides a comprehensive summary of the problem, lists all changed files, explains the rationale, and includes a test plan. However, the PR description template sections are not formally filled out per the repository template.
Linked Issues check ✅ Passed All four locations identified in issue #5601 have been addressed: dnsinterceptor/handler.go, service_listener.go (two locations), and client/client.go. The changes replace fmt.Sprintf with net.JoinHostPort, correctly fixing IPv6 address formatting.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing IPv6 address formatting in DNS address construction across the three files. No unrelated modifications are present.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

Tip

CodeRabbit can generate a title for your PR based on the changes.

Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the reviews.auto_title_placeholder setting.

@sonarqubecloud
Copy link
Copy Markdown

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


easonysliu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@lixmal
Copy link
Copy Markdown
Collaborator

lixmal commented Mar 16, 2026

Can you please stick to the PR template?
Thanks for the contribution!

@lixmal lixmal merged commit a590c38 into netbirdio:main Mar 17, 2026
37 of 42 checks passed
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.

IPv6 address formatting broken in DNS address construction (missing brackets)

3 participants