Silently ignored networking.nameservers setting #29202 #29205
Conversation
NixOS@f9f9749#commitcomment-24028305 (cherry picked from commit 22d4630)
…th git break. (cherry picked from commit c651a0c)
(cherry picked from commit 8d5fc1b)
Also, fix the xcb plugin error (NixOS#24256) and add service-identity which is a required dependency now.
It's better to break with a comment than to silently ignore a user's setting.
|
@gwitmond, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @regellosigkeitsaxiom and @Mic92 to be potential reviewers. |
When the user specifies the networking.nameservers setting in the configuration file, it must take precedence over automatically derived settings. This patch prevents the service.bind and dnsmasq.resolveLocalQueries settings from overriding the users' settings. Also, when the user specifies a domain to search, it must be set in the resolver configuration, even if the user does not specify any nameservers.
gwitmond
force-pushed
the
1709-hackathon
branch
from
82c10cd
to
4b8c706
Compare
gwitmond
requested review from
copumpkin,
edolstra,
FRidh,
LnL7 and
peti
as code owners
nixos/modules/config/networking.nix
Outdated
| @@ -9,7 +9,7 @@ let | |||
| cfg = config.networking; | |||
| dnsmasqResolve = config.services.dnsmasq.enable && | |||
| config.services.dnsmasq.resolveLocalQueries; | |||
| hasLocalResolver = config.services.bind.enable || dnsmasqResolve; | |||
| hasLocalResolver = cfg.nameservers == [] && ( config.services.bind.enable || dnsmasqResolve ); | |||
There was a problem hiding this comment.
What exactly happens here when nameservers is not empty?
There was a problem hiding this comment.
The intention of this PR (apparently): only use nameservers from the explicit nameservers list and not 127.0.0.1.
Side note: I added another resolver months ago and didn't realize we had this magic; I should amend hasLocalResolver for that.
There was a problem hiding this comment.
@peti: When 'nameservers' is empty, it follows the old behaviour. That is: it sets 'nameserver = 127.0.0.1' in networking.nix when either services.bind.enable == true or services.dnsmasq.resolveLocalQueries == true at https://github.com/gwitmond/nixpkgs/blob/4b8c7065e2d9f2961855e7ed2cd51b4bbb6d5f11/nixos/modules/config/networking.nix#L241
If neither networking.nameservers is set nor either of those services, it defaults to standard behaviour.
@vcunat: Indeed, my goal is to give the user specified nameservers priority over any automatic ones.
There was a problem hiding this comment.
I actually have a machine that has nameservers configured which aren't used because there is a local BIND running, and I do rely on the fact that the local bind overrides the other setting. If we'd merge this PR, then this would break my server's setup. Therefore, I'm 👎 on this PR.
I think it would be okay to add an assertion that catches such an arguably inconsistent configuration, but silently changing the behavior of NixOS sounds like a bad idea to me.
The culprit was services.bind that made the resolver set to 127.0.0.1 and ignore the nameserver setting. This patch adds a flag to services.bind to override the nameserver to localhost. It defaults to true. There is also a warning when a user sets the nameserver and this setting. I added an explanation to the dnsmasq documentation to specify this behaviour.
gwitmond
force-pushed
the
1709-hackathon
branch
from
4b8c706
to
fdde0fa
Compare
|
Ignore this PR for now, as I added a bunch of garbage... |
|
I've created a new PR at #29503 |
…)" This reverts commit 670b4e2. The change added in this commit was controversial when it was originally suggested in #29205. Then that PR was closed and a new one opened, #29503, effectively circumventing the review process. I don't agree with this modification. Adding an option 'resolveLocalQueries' to tell the locally running name server that it should resolve local DNS queries feels outright nuts. I agree that the current state is unsatisfactory and that it should be improved, but this is not the right way.
…)" This reverts commit 670b4e2. The change added in this commit was controversial when it was originally suggested in #29205. Then that PR was closed and a new one opened, #29503, effectively circumventing the review process. I don't agree with this modification. Adding an option 'resolveLocalQueries' to tell the locally running name server that it should resolve local DNS queries feels outright nuts. I agree that the current state is unsatisfactory and that it should be improved, but this is not the right way. (cherry picked from commit 23a021d)
It's better to break with a comment than to silently ignore a user's setting.
Motivation for this change
The networking configuration set the resolver configuration (/etc/resolv.conf) to use 127.0.0.1 as the nameserver when the services.bind.enable is set to true. Or when services.dnsmasq.resolveLocalQueries is enabled.
This PR adds an assertion to verify if the user set the networking.nameservers options when either service is enabled. This makes it clear that the user cannot set the nameservers for the resolver.
Things done
Built on platform(s)
Tested according to 'steps to reproduce' in issue silently ignored networking.nameservers setting #29202.`