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

chore: make noopManager a no-op, even if a DNS config is supplied #63

Closed
wants to merge 1 commit into from

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Nov 7, 2024

Re: coder/coder#14718

The Tailscale DNS manager that's used when you don't supply one to the wireguard engine (noopManager) returns false for whether it supports split DNS. If split DNS isn't supported, and a non-empty DNS config is supplied, the wireguard engine will try and get the base DNS config, which the no-op manager also doesn't support, and then logs an error, which fails a heap of coder/coder tests.

To address this, we can modify the default configurator to perform a no-op, even if a dns config is supplied.

We could also just set the DNS config conditionally (i.e. only for CoderVPN, or if a host has been supplied to the tailnet.Conn), but doing the plumbing for that seems not-great compared to just making this a no-op.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ethanndickson and the rest of your teammates on Graphite Graphite

@ethanndickson ethanndickson marked this pull request as ready for review November 7, 2024 03:58
Copy link

I do think its a good thing that we log errors if we try to program DNS entries into a tailnet.Conn that doesn't support it.

I'd rather set the DNS conditionally somehow. Maybe we look at the Options.DNSConfigurator to decide whether to set DNS config, and throw errors if you try to insert DNS records into a tailnet.Conn that doesn't have that option set?

@ethanndickson
Copy link
Member Author

Yep, that works 👍

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.

2 participants