Skip to content

Commit

Permalink
Revert "nixos: add option for bind to not resolve local queries (#29503
Browse files Browse the repository at this point in the history
…)"

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)
  • Loading branch information
peti committed Sep 23, 2017
1 parent 3a58e41 commit 99f759d
Show file tree
Hide file tree
Showing 4 changed files with 3 additions and 14 deletions.
4 changes: 1 addition & 3 deletions nixos/modules/config/networking.nix
Expand Up @@ -9,9 +9,7 @@ let
cfg = config.networking;
dnsmasqResolve = config.services.dnsmasq.enable &&
config.services.dnsmasq.resolveLocalQueries;
bindResolve = config.services.bind.enable &&
config.services.bind.resolveLocalQueries;
hasLocalResolver = bindResolve || dnsmasqResolve;
hasLocalResolver = config.services.bind.enable || dnsmasqResolve;

resolvconfOptions = cfg.resolvconfOptions
++ optional cfg.dnsSingleRequest "single-request"
Expand Down
9 changes: 0 additions & 9 deletions nixos/modules/services/networking/bind.nix
Expand Up @@ -151,15 +151,6 @@ in
";
};

resolveLocalQueries = mkOption {
type = types.bool;
default = true;
description = ''
Whether bind should resolve local queries (i.e. add 127.0.0.1 to
/etc/resolv.conf, overriding networking.nameserver).
'';
};

};

};
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/services/networking/dnsmasq.nix
Expand Up @@ -42,7 +42,7 @@ in
default = true;
description = ''
Whether dnsmasq should resolve local queries (i.e. add 127.0.0.1 to
/etc/resolv.conf overriding networking.nameservers).
/etc/resolv.conf).
'';
};

Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/tasks/network-interfaces-scripted.nix
Expand Up @@ -105,7 +105,7 @@ let
''
# Set the static DNS configuration, if given.
${pkgs.openresolv}/sbin/resolvconf -m 1 -a static <<EOF
${optionalString (cfg.domain != null) ''
${optionalString (cfg.nameservers != [] && cfg.domain != null) ''
domain ${cfg.domain}
''}
${optionalString (cfg.search != []) ("search " + concatStringsSep " " cfg.search)}
Expand Down

1 comment on commit 99f759d

@gwitmond
Copy link
Contributor

@gwitmond gwitmond commented on 99f759d Sep 27, 2017

Choose a reason for hiding this comment

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

@peti,

Please don't treat my pulling back the old PR as a way to circumvent the review process. I messed it up and could not figure out how to resolve that. That's why I created a new PR. And I linked it to the new one to continue the discussion.

About the patch: I copied the existing behaviour for DNSMASQ and made sure that Bind operates the same way. It may be the wrong solution, but at least it is more consistent than before my patch. Don't let the perfect be the enemy of the good.

Please reconsider and accept my patch.

Please sign in to comment.