Remove networking.hostConf option #74032

ckauhaus · 2019-11-24T12:45:05Z

Motivation for this change

This PR is part of the networking.* namespace cleanup. We feel that
networking.hostConf is rarely used and provides little value compared to
using environment.etc."host.conf" directly.

Things done

Ma27 · 2019-11-24T12:48:32Z

nixos/modules/config/networking.nix

@@ -41,19 +41,6 @@ in
      '';
    };

-    networking.hostConf = lib.mkOption {
-      type = types.lines;
-      default = "multi on";


Shouldn't we add an /etc/host.conf with this value by default?

Good point. Will update the PR.

Is host.conf honoured in the nsswitch.conf world?

AFAICT yes as long as you use glibc's resolver directly. It might by another story when using dnsmasq or systemd-resolvd .

This should happen inside the file part of nsswitch.conf, so before and regardless of which resolver is used to do DNS lookups.

For context, this was introduced during #19148, and seems to be the default on ArchLinux.

@flokli I've studied the man pages but got still no clue about how this should be expressed in nsswitch.conf. Could you provide an example?

flokli · 2019-11-25T00:29:24Z

nixos/modules/config/networking.nix

@@ -185,8 +172,7 @@ in
          ${cfg.extraHosts}
        '';

-        # /etc/host.conf: resolver configuration file
-        "host.conf".text = cfg.hostConf;
+        "host.conf".text = mkDefault "multi on\n";


We should add a reference to #19148 here.

ckauhaus · 2019-11-26T09:58:54Z

Sure. Gonna rework this stuff and test it.

flokli · 2019-11-28T15:46:47Z

@ckauhaus ping ;-)

ckauhaus · 2019-11-29T08:48:48Z

@flokli on my way, sorry

This PR is part of the networking.* namespace cleanup. We feel that networking.hostConf is rarely used and provides little value compared to using environment.etc."host.conf" directly. Provide sensible default: multi on

ckauhaus · 2019-11-29T11:10:46Z

Added a test. This is my first Python test, so feel free to comment.

flokli · 2019-11-29T17:52:25Z

nixos/tests/resolv.nix

+  };
+
+  testScript = let
+    getaddrinfo_py = pkgs.writeScript "getaddrinfo.py" ''


I'm not sure if we need our own getaddrinfo_py script here - Couldn't we just run getent ahosts $hostname in the testScript?

I'll give it a shot.

flokli · 2019-11-29T17:53:45Z

nixos/tests/resolv.nix

+
+  in
+  ''
+    def compare(test, should, is_):


This seems to be a pretty generic tooling, which we might want to make available in the whole test driver, instead of just here. I'd probably leave it out of scope for this PR, though.

@tfc, WDYT?

The general idea is great, but i am not sure how that should look like, yet. Also, it should maybe not be part of the driver itself but a (possibly by default included) library that can be used by users - maybe an already existing one?

However, that would best be tackled after we split up the driver into individual library parts.

Let's have it 3 times in individual tests and then extract it into a lib.

flokli · 2019-11-29T17:54:32Z

nixos/tests/resolv.nix

+        )
+    )
+
+    assert all(res) is True


I'd propose to just wrap the individual test in subtests, we automatically ensure they all need to succeed.

That's true.

Although it works fine in this test format, breaking the first test assumption often makes the next assumption pointless.
I would for example not even test for the dual stack condition if one of ipv4 or ipv6 doesn't even work. That's where the test should stop doing anything and dumping me the reason it failed onto the terminal.

The whole test would better fit the estabished workflow if it looked like this:

start_all() resolv.wait_for_unit("nscd") ipv4 = "192.0.2.1 192.0.2.2" ipv6 = "2001:db8::2:1 2001:db8::2:2" with subtest("IPv4 resolves"): assert ipv4 in resolv.succeed("${getaddrinfo} host-ipv4.example.net") with subtest("IPv6 resolves"): assert ipv6 in resolv.succeed("${getaddrinfo} host-ipv6.example.net") with subtest("Dual stack resolves"): assert ipv4 + " " + ipv6 in resolv.succeed("${getaddrinfo} host-dual.example.net")

tfc · 2019-11-30T14:01:47Z

nixos/tests/resolv.nix

+        )
+    )
+
+    assert all(res) is True


That's true.

Although it works fine in this test format, breaking the first test assumption often makes the next assumption pointless.
I would for example not even test for the dual stack condition if one of ipv4 or ipv6 doesn't even work. That's where the test should stop doing anything and dumping me the reason it failed onto the terminal.

The whole test would better fit the estabished workflow if it looked like this:

start_all() resolv.wait_for_unit("nscd") ipv4 = "192.0.2.1 192.0.2.2" ipv6 = "2001:db8::2:1 2001:db8::2:2" with subtest("IPv4 resolves"): assert ipv4 in resolv.succeed("${getaddrinfo} host-ipv4.example.net") with subtest("IPv6 resolves"): assert ipv6 in resolv.succeed("${getaddrinfo} host-ipv6.example.net") with subtest("Dual stack resolves"): assert ipv4 + " " + ipv6 in resolv.succeed("${getaddrinfo} host-dual.example.net")

tfc · 2019-11-30T14:04:47Z

nixos/tests/resolv.nix

+
+      result = set()
+      for gai in socket.getaddrinfo(sys.argv[1], 0):
+          result.add(gai[4][0])


How about this generator approach:

result = {gai[4][0] for gai in socket.getaddrinfo(sys.argv[1], 0)}

Rewrote test script based on your first suggestion. Please comment. If this version is fine, I'll squash the PR.

nixos/tests/resolv.nix

ckauhaus · 2019-11-30T17:33:29Z

I'll rewrite the test next week.

@flokli To get back to the main matter: how do I get this setting into nsswitch.conf? I wasn't able to figure it out yet.

flokli · 2019-11-30T18:05:35Z

Sorry, missed replying to #74032 (comment) The nsswitch file is fine as it is in NixOS now (we have a `file` module in there, meaning `/etc/hosts` will be used for lookups and glibc will honor a given `/etc/host.conf`. I still think we should change the test to invoke `getent ahosts $hostname` instead of providing our own python test script.

@flokli

Thanks to @flokli and @tfc

flokli · 2019-12-05T00:41:47Z

LGTM!

Ma27 reviewed Nov 24, 2019

View reviewed changes

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 24, 2019

ckauhaus force-pushed the remove-networking.hostconf branch from 6e97fb5 to 73c159d Compare November 24, 2019 13:13

flokli reviewed Nov 25, 2019

View reviewed changes

Remove networking.hostConf option

918c2ca

This PR is part of the networking.* namespace cleanup. We feel that networking.hostConf is rarely used and provides little value compared to using environment.etc."host.conf" directly. Provide sensible default: multi on

ckauhaus force-pushed the remove-networking.hostconf branch from 73c159d to 918c2ca Compare November 29, 2019 11:09

flokli reviewed Nov 29, 2019

View reviewed changes

tfc requested changes Nov 30, 2019

View reviewed changes

ckauhaus self-assigned this Nov 30, 2019

Rewrite test script

8d36536

Thanks to @flokli and @tfc

tfc approved these changes Dec 5, 2019

View reviewed changes

Ma27 approved these changes Dec 5, 2019

View reviewed changes

flokli merged commit ea9c3b9 into NixOS:master Dec 5, 2019

ckauhaus deleted the remove-networking.hostconf branch December 6, 2019 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove networking.hostConf option #74032

Remove networking.hostConf option #74032

ckauhaus commented Nov 24, 2019

Ma27 Nov 24, 2019

ckauhaus Nov 24, 2019

7c6f434c Nov 24, 2019

ckauhaus Nov 24, 2019

flokli Nov 25, 2019

ckauhaus Nov 29, 2019

flokli Nov 25, 2019

ckauhaus commented Nov 26, 2019

flokli commented Nov 28, 2019

ckauhaus commented Nov 29, 2019

ckauhaus commented Nov 29, 2019

flokli Nov 29, 2019

ckauhaus Nov 30, 2019

flokli Nov 29, 2019

tfc Nov 30, 2019

ckauhaus Nov 30, 2019

flokli Nov 29, 2019

tfc Nov 30, 2019

tfc Nov 30, 2019

tfc Nov 30, 2019

ckauhaus Dec 4, 2019

ckauhaus commented Nov 30, 2019

flokli commented Nov 30, 2019 via email

flokli commented Dec 5, 2019

Remove networking.hostConf option #74032

Remove networking.hostConf option #74032

Conversation

ckauhaus commented Nov 24, 2019

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckauhaus commented Nov 26, 2019

flokli commented Nov 28, 2019

ckauhaus commented Nov 29, 2019

ckauhaus commented Nov 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckauhaus commented Nov 30, 2019

flokli commented Nov 30, 2019 via email

flokli commented Dec 5, 2019