Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix not able to query IPv6 DNS servers because of too many colons in address #30

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

mmourick
Copy link
Collaborator

@mmourick mmourick commented Dec 12, 2023

What

  • Adds square brackets to dnsServerIP when using IPv6 addresses.
  • Adds the dns-server-port flag to let users specify a port other than the default of 53.

Why

  • The :53 port specification of the URI creates ambiguity when used in combination with IPv6 addresses. The square bracket notation is used to resolve this. Refer to RFC3986 for more details.

References

pkg/core/core.go Outdated
}
// If the server IP is IPv6, wrap it in square brackets to remove ambiguity from port number and address.
if net.ParseIP(dnsServerIP).To4() == nil {
dnsServerIP = "[" + dnsServerIP + "]"
Copy link
Owner

@bschaatsbergen bschaatsbergen Jan 9, 2024

Choose a reason for hiding this comment

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

Recommendation: I suggest checking whether the parsed dnsServerIP includes a trailing port. If it does, we should use the IP along with the trailing port. If there's no trailing port, consider appending :53 to the dnsServerIP.

Additionally, we should address another issue by allowing the user to pass a custom port through the --dns-server-ip flag. This would ensure support for a broader range of possibilities when wanting to query your own DNS server, folks tend to typically do so because they have either a custom port or use IPv6 for instance.

I'm curious to hear what you think. Also, I have doubts about the bracket notation; it seems a bit unclear, vague and not that 'clean' to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the PR to address your comment regarding the custom ports.
Besides that, I've cleaned up the implementation. JoinHostPort(..) does the exact same validation as I implemented before, but all in a single line 😄

@bschaatsbergen bschaatsbergen added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 9, 2024
Copy link
Owner

@bschaatsbergen bschaatsbergen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bschaatsbergen
Copy link
Owner

Very nice fix @mmourick 👍🏼

@bschaatsbergen bschaatsbergen merged commit 6a4d758 into main Jan 10, 2024
5 checks passed
@bschaatsbergen bschaatsbergen deleted the fix/querying-ipv6-addresses branch January 10, 2024 20:52
@bschaatsbergen bschaatsbergen removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants