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

dnsmasq: Make unexpected resolvconf integration separately opt-in #67598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Shados
Copy link
Member

@Shados Shados commented Aug 28, 2019

Motivation for this change

Currently, if you set services.dnsmasq.resolveLocalQueries = true; (the default), then dnsmasq will not only resolve DNS queries for the local system, but it will also make it so that any resolvconf-provided DNS servers are injected into dnsmasq's list of upstream DNS servers.

This behaviour was introduced way back in #3745, but is problematic on several fronts:

  • It isn't documented by the option description, at all.
  • It probably should not be enabled by default (while potentially useful, using dnsmasq with DHCP-supplied upstreams is not that common a use-case).
  • It can't be separately configured -- while the user can disable use of the resolv-file directive by adding no-resolv to extraConfig, there is no way to disable reading of the resolvconf-produced /etc/dnsmasq-conf.conf file. On my system at least, this file configures dnsmasq to resolve queries for my system's search domain using the DHCP-supplied server.

This PR migrates the functionality to its own, documented, opt-in configuration option. The only downside to this is that it will change the behaviour on existing systems.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@teto
Copy link
Member

teto commented Aug 29, 2019

Makes sense to provide a new setting I believe, and I agree the default should be changed. I am a bit hesitant as to how to change it though. I would say let's keep the current behavior and wait a bit before changing it, maybe mention it in 19.09 release notes ?

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@Shados
Copy link
Member Author

Shados commented Jun 8, 2020

I'm still carrying this patch around on several systems where the previous behaviour is unwanted.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 8, 2020
@bb2020
Copy link
Member

bb2020 commented Jun 27, 2020

Another issue: resolveLocalQueries enables networking.resolvconf.useLocalResolver which prepends 127.0.0.1 to nameserver list. Dnsmasq also sets networking.nameservers here. Isn't this a duplicate?

@stale
Copy link

stale bot commented Dec 24, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 24, 2020
@Shados
Copy link
Member Author

Shados commented Dec 24, 2020

I'd still like to get this merged.

@bb2020 I suppose, but that's not related to the issue this solves.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 24, 2020
@bb2020
Copy link
Member

bb2020 commented Dec 24, 2020

Yes, right. I just wanted to mention it when I saw this issue about resolveLocalQueries.

@stale
Copy link

stale bot commented Jun 23, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 23, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 23, 2021
@stale
Copy link

stale bot commented Jan 6, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 6, 2022
@Shados
Copy link
Member Author

Shados commented Jan 6, 2022

I'd still like this to be merged...

EDIT: Also, to be clear, I do not think this needs a changelog entry. The current behaviour is both undocumented and unexpected, making it a bug, making this a bug fix.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 6, 2022
@bb2020
Copy link
Member

bb2020 commented Jan 18, 2022

  • It probably should not be enabled by default (while potentially useful, using dnsmasq with DHCP-supplied upstreams is not that common a use-case).

This is actually what most users do; using dnsmasq as DNS cache for faster DNS resolving. Thus it should be enabled by default.

@Shados
Copy link
Member Author

Shados commented Jan 19, 2022

@bb2020 the quoted line is not relevant to whether or not dnsmasq is being used as a DNS cache, only to whether or not it sources DNS servers from an upstream DHCP server (via resolvconf).

As for whether or not it is "what most users do": I have only anecdotal evidence, in that I have yet to see someone deliberately using resolv-conf supplied DNS servers in dnsmasq, and I have seen many people deliberately using explicitly-configured DNS servers in dnsmasq instead. Is your own assertion also based on anecdotal evidence, or do you have some sort of harder data backing it?

@bb2020
Copy link
Member

bb2020 commented Jan 19, 2022

I agree there is a bug here because it just merges upstream DNS with local DNS.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 31, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 15, 2022
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
Copy link
Contributor

@ibizaman ibizaman left a comment

Choose a reason for hiding this comment

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

I fully agree this change is a good idea but because of the high usage, I instead vote for making the change retro-compatible.

From a quick GitHub search, there are a lot of repositories using dnsmasq and most of them set this option to true and others to false. So they implicitly rely on this option working the way it does.

Having the DNS settings change under your feet is one of the worst possible thing to happen and is usually hard to debug.

@@ -62,6 +62,15 @@ in
'';
};

useResolvConfUpstreams = mkOption {
type = types.bool;
default = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default = false;
default = true;

@@ -62,6 +62,15 @@ in
'';
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I would update the option description here to mention this will by default resolve upstream DNS services and if that's not wanted, to set useResolvConfUpstreams to false.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants