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
Merged

Conversation

@ckauhaus
Copy link
Contributor

@ckauhaus ckauhaus commented Nov 24, 2019

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

This comment has been minimized.

@Ma27

Ma27 Nov 24, 2019
Member

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

This comment has been minimized.

@ckauhaus

ckauhaus Nov 24, 2019
Author Contributor

Good point. Will update the PR.

This comment has been minimized.

@7c6f434c

7c6f434c Nov 24, 2019
Member

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

This comment has been minimized.

@ckauhaus

ckauhaus Nov 24, 2019
Author Contributor

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

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

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.

This comment has been minimized.

@ckauhaus

ckauhaus Nov 29, 2019
Author Contributor

@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";

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

We should add a reference to #19148 here.

@ckauhaus
Copy link
Contributor Author

@ckauhaus ckauhaus commented Nov 26, 2019

Sure. Gonna rework this stuff and test it.

@flokli
Copy link
Contributor

@flokli flokli commented Nov 28, 2019

@ckauhaus ping ;-)

@ckauhaus
Copy link
Contributor Author

@ckauhaus ckauhaus commented Nov 29, 2019

@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 ckauhaus force-pushed the ckauhaus:remove-networking.hostconf branch from 73c159d to 918c2ca Nov 29, 2019
@ckauhaus
Copy link
Contributor Author

@ckauhaus ckauhaus commented Nov 29, 2019

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

};

testScript = let
getaddrinfo_py = pkgs.writeScript "getaddrinfo.py" ''

This comment has been minimized.

@flokli

flokli Nov 29, 2019
Contributor

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

This comment has been minimized.

@ckauhaus

ckauhaus Nov 30, 2019
Author Contributor

I'll give it a shot.


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

This comment has been minimized.

@flokli

flokli Nov 29, 2019
Contributor

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?

This comment has been minimized.

@tfc

tfc Nov 30, 2019
Contributor

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.

This comment has been minimized.

@ckauhaus

ckauhaus Nov 30, 2019
Author Contributor

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

)
)
assert all(res) is True

This comment has been minimized.

@flokli

flokli Nov 29, 2019
Contributor

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

This comment has been minimized.

@tfc

tfc Nov 30, 2019
Contributor

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

This comment has been minimized.

@tfc

tfc Nov 30, 2019
Contributor

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

This comment has been minimized.

@tfc

tfc Nov 30, 2019
Contributor

How about this generator approach:

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

This comment has been minimized.

@ckauhaus

ckauhaus Dec 4, 2019
Author Contributor

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

@ckauhaus ckauhaus commented Nov 30, 2019

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 flokli commented Nov 30, 2019

Thanks to @flokli and @tfc
@flokli
Copy link
Contributor

@flokli flokli commented Dec 5, 2019

LGTM!

@tfc
tfc approved these changes Dec 5, 2019
@Ma27
Ma27 approved these changes Dec 5, 2019
@flokli flokli merged commit ea9c3b9 into NixOS:master Dec 5, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@ckauhaus ckauhaus deleted the ckauhaus:remove-networking.hostconf branch Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.