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

Add support for NextDNS resolver #940

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Add support for NextDNS resolver #940

merged 1 commit into from
Nov 18, 2022

Conversation

arnarg
Copy link
Contributor

@arnarg arnarg commented Nov 7, 2022

This adds support for easily using NextDNS as a DoH resolver in tailscale network controlled by Headscale. More info in #939

To enable you would put the following in config:

dns_config:
  nameservers:
    - nextdns:abcdef # Where abcdef is your NextDNS ID
  • read the CONTRIBUTING guidelines
  • raised a GitHub issue or discussed it on the projects chat beforehand
  • added unit tests
  • added integration tests
  • updated documentation if needed
  • updated CHANGELOG.md

Fixes #939

config.go Outdated Show resolved Hide resolved
dns.go Outdated
Comment on lines 190 to 196
resolver.Addr = fmt.Sprintf(
"%s?device_name=%s&device_model=%s&device_ip=%s",
resolver.Addr,
machine.GivenName,
machine.HostInfo.OS,
machine.IPAddresses[0],
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just adds a nice machine identifier in the request log in the NextDNS dashboard.
Machine identifier example

@arnarg
Copy link
Contributor Author

arnarg commented Nov 8, 2022

I updated some of the code as I wasn't happy with everything and I realized that in getMapResponseDNSConfig the dnsConfigOrig is only cloned when if magic dns is enabled. I believe NextDNS resolver can be enabled without magic DNS but as is it will overwrite the resolver in the global config, appending machine specific config over and over again.

What do you think would be more appropriate to resolve that.

  • Only do the machine config appending to the nextdns resolver when magic DNS is enabled
  • Always clone the dnsConfigOrig in case there is a nextdns resolver in there that needs to be modified for the machine

I personally think the second would be better as the DoH resolvers don't have much to do with magic DNS.

@iSchluff
Copy link
Contributor

iSchluff commented Nov 9, 2022

As far as I understand the code in tailscale you should just be able to provide a nexdns ipv6 ip in the headscale dns configuration, and it should work without any changes

https://github.com/tailscale/tailscale/blob/bb7be74756e23d4385a91a9608fd68eddd5a249a/net/dns/publicdns/publicdns.go#L42

@arnarg
Copy link
Contributor Author

arnarg commented Nov 9, 2022

Yeah you are correct. I had not seen this. This will at least make it possible to use NextDNS DoH resolver as is, good to know!

However, from a usability point of view I would still argue that nextdns:abcdef would be nicer to specify than 2a07:a8c0::ab:cdef. But more importantly the ipv6 way does not support adding the metadata so that hosts are identifiable in the request log.

Like this:
Screenshot from 2022-11-09 19-59-31

Do you think you'd be open for adding this feature at all or not?

config.go Outdated Show resolved Hide resolved
dns.go Outdated Show resolved Hide resolved
kradalby
kradalby previously approved these changes Nov 15, 2022
@kradalby
Copy link
Collaborator

@arnarg looks good now, can you add a CHANGELOG entry and some docs?

@arnarg
Copy link
Contributor Author

arnarg commented Nov 16, 2022

@arnarg looks good now, can you add a CHANGELOG entry and some docs?

I have added to the CHANGELOG and I couldn't find any good place for this in docs except as comments in the example config. Is this acceptable?

@kradalby kradalby merged commit 6d3ede1 into juanfont:main Nov 18, 2022
@mrbluecoat mrbluecoat mentioned this pull request Jul 25, 2024
2 tasks
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.

Support NextDNS DoH resolver
3 participants