tailscale
tailscale copied to clipboard
tailscale
Add Control D support, along with DoH client metadata
This adds support for Control D in a similar fashion to how NextDNS was integrated via this merge.
Resolves: https://github.com/tailscale/tailscale/issues/7946
Control D allows for many advanced use cases, and many customers of both Control D and Tailscale have requested this integration.
This also adds support for providing the client hostname, mac address and tailscale local IP to the DNS server to facilitate client specific DNS configurations and data analysis via the Control D analytics feature. While I understand that adding Control D specific fields to the forwarder may not be ideal, I do think that these fields may become useful for other providers over time. If this is something that the tailscale team is very opposed to, I can remove this as it's not a requirement for integration, but does add value for clients that want to use a single DoH endpoint for multiple devices and manage them under a single scope.
@bradfitz Please take a look 🙏
@alexelisenko, @yegors, ping. See comments/questions above. I just saw this again when cleaning up PRs and noticed it had stalled.
Is there anything limiting this from being merged?
@alexelisenko said that @bradfitz would provide an update.
Just checking since I love both services and have been drowning in DNS mismatches with my devices.
Hey team, is there any update on this? I would love to see the integration
I'm a new Control D user (loving the service) and really need Tailscale integration.
At the risk aggravating you, @bradfitz can you cast your eye on this PR, please? I see you're on parental leave, yet I've spotted you've approved a few PRs (though you should enjoy your time off!). Is this held up due to the commit message check failing, or something else?
This PR is lacking DCO (git commit -s) but even if that's fixed and the rest of the PR is fine, there are server-side bits that would also need to be done & prioritized before this PR would be usable.
/cc @kabirsikand to think about that.
@bradfitz / @kabirsikand Hate to be annoying but any chance to get this moving along anytime soon? Anything we can do on our end?
The associated issue is now 3rd most upvoted one here.
I could merge at least this client-side portion of things but what's going on with this PR's commits? It has 744 commits in it. Looks like the rebase went poorly. There's a merge commit but then also 744 commits from main that are all re-signed by a Signed-off-by: Alex Paguis <[email protected]> (is that the same Alex as @alexelisenko?).
To answer the questions in the TODOs in the code: that's not how we should get the hostname, Tailscale IP, and MAC. Let's omit that part for now. I assume it's optional. We can add it back in a subsequent commit, plumbed the right way.
BTW, ip.String() == controlDv4One.String() || ip.String() == controlDv4Two.String() is an inefficient (+ allocating) way to compare to netip.Addrs. You can just == on them directly.
The newly added tests also fail?
--- FAIL: TestDoHIPsOfBase (0.00s)
publicdns_test.go:141: DoHIPsOfBase("https://dns.controld.com/hyq3ipr2ct") = [76.76.2.22 76.76.10.22 2a07:a8c0:0:6:7b5b:5949:35ad:0 2a07:a8c1:0:6:7b5b:5949:35ad:0]; want [76.76.2.22 76.76.10.22 2606:1a40:0:6:7b5b:5949:35ad:0 2606:1a40:1:6:7b5b:5949:35ad:0]
publicdns_test.go:141: DoHIPsOfBase("https://dns.controld.com/112233445566778899aabbcc") = [76.76.2.22 76.76.10.22 2a07:a8c0:0:ffff:ffff:ffff:ffff:0 2a07:a8c1:0:ffff:ffff:ffff:ffff:0]; want [76.76.2.22 76.76.10.22 2606:1a40:0:ffff:ffff:ffff:ffff:0 2606:1a40:1:ffff:ffff:ffff:ffff:0]
FAIL
FAIL tailscale.com/net/dns/publicdns 0.124s
FAIL
You didn't check that box that maintainers could make changes, so I ended up forking this off into #11659 to fix the git mistakes and commit message and remove the extra headers for now.
This is now mostly merged (except the parts I removed). If you want to send followup changes, please check the git result first.