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

Remove networking.hostConf option #74032

Merged
merged 2 commits into from Dec 5, 2019

Conversation

ckauhaus
Copy link
Contributor

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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)
    • nixos/tests/network.nix with networkd=false
  • 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
    • no appearances outside options.html
  • Fits CONTRIBUTING.md.

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

networking.hostConf = lib.mkOption {
type = types.lines;
default = "multi on";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Will update the PR.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

@@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a reference to #19148 here.

@ckauhaus
Copy link
Contributor Author

Sure. Gonna rework this stuff and test it.

@flokli
Copy link
Contributor

flokli commented Nov 28, 2019

@ckauhaus ping ;-)

@ckauhaus
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

};

testScript = let
getaddrinfo_py = pkgs.writeScript "getaddrinfo.py" ''
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give it a shot.


in
''
def compare(test, should, is_):
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
)

assert all(res) is True
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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")

)
)

assert all(res) is True
Copy link
Contributor

Choose a reason for hiding this comment

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

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")


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

Choose a reason for hiding this comment

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

How about this generator approach:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

nixos/tests/resolv.nix Outdated Show resolved Hide resolved
@ckauhaus ckauhaus self-assigned this Nov 30, 2019
@ckauhaus
Copy link
Contributor Author

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
Copy link
Contributor

flokli commented Nov 30, 2019 via email

@flokli
Copy link
Contributor

flokli commented Dec 5, 2019

LGTM!

@flokli flokli merged commit ea9c3b9 into NixOS:master Dec 5, 2019
@ckauhaus ckauhaus deleted the remove-networking.hostconf branch December 6, 2019 09:47
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