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

nixos/systemd: move systemd-provided NSS modules to systemd module #86940

Merged
merged 2 commits into from May 5, 2020

Conversation

@flokli
Copy link
Contributor

flokli commented May 5, 2020

Motivation for this change

#86350

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)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-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
  • Fits CONTRIBUTING.md.
@flokli flokli requested review from florianjacob, dasJ and WilliButz May 5, 2020
# While there is already an assertion in place complaining loudly about
# having nssModules configured and nscd disabled, for some reason we still
# check for nscd being enabled before adding to nssModules.
Comment on lines +835 to +837

This comment has been minimized.

Copy link
@flokli

flokli May 5, 2020

Author Contributor

@florianjacob as the author of e370e97, can you elaborate on why we silently disable these nss modules if nscd is disabled, even though there's an assertion in https://github.com/NixOS/nixpkgs/pull/86940/files#diff-5796c52b71eee35842f408f4126430d6R126-R127 which should complain if nss modules are present, but nscd disabled (so nssModules are not respected)?

Maybe instead of silently ignoring these (and breaking dynamic user support, as well as other NSS modules), can't we ask the user to mkForce system.nssModules = [] in the assertion message if they really doesn't want any external NSS modules?

This comment has been minimized.

Copy link
@flokli

flokli May 5, 2020

Author Contributor

Also see #43607 and #86010 for context.

This comment has been minimized.

Copy link
@dasJ

dasJ May 5, 2020

Member

I really like your idea. There is no reason for the current behaviour and system.nssModules = systemd.out seems like the most elegant solution and also prevents users from accidentially breaking their systems by disabling nscd.

This comment has been minimized.

Copy link
@andir

andir May 5, 2020

Member

I hope @florianjacob can shed some light on this. I think we should never have to add vague comments like for some reason we still check for nscd being enabled before adding to nssModules. The source should be a place of truth :) If we can't figure it out and the original authors don't respond we might as well change the implementation until we encounter errors and can properly document them.

Copy link
Member

dasJ left a comment

Configuring systemd without the mkIf seems like a much better solution 👍

nixos/modules/system/boot/resolved.nix Show resolved Hide resolved
# While there is already an assertion in place complaining loudly about
# having nssModules configured and nscd disabled, for some reason we still
# check for nscd being enabled before adding to nssModules.

This comment has been minimized.

Copy link
@dasJ

dasJ May 5, 2020

Member

I really like your idea. There is no reason for the current behaviour and system.nssModules = systemd.out seems like the most elegant solution and also prevents users from accidentially breaking their systems by disabling nscd.

nixos/modules/system/boot/systemd.nix Outdated Show resolved Hide resolved
flokli added 2 commits May 5, 2020
We keep the "only add the nss module if nscd is enabled" logic for now.

The assertion never was triggered, so it can be removed.
We keep the conditional on only adding if nscd is enabled for now.
@flokli flokli force-pushed the flokli:move-nss-systemd branch from 2920001 to c0995d2 May 5, 2020
@flokli
Copy link
Contributor Author

flokli commented May 5, 2020

I adressed your suggestions. I'll reserve the nssModules assertions for a follow-up PR.

@dasJ
dasJ approved these changes May 5, 2020
@flokli flokli requested review from aanderse, andir and arianvp May 5, 2020
Copy link
Member

andir left a comment

Overall 👍 just some comments.

nixos/modules/system/boot/resolved.nix Show resolved Hide resolved
# While there is already an assertion in place complaining loudly about
# having nssModules configured and nscd disabled, for some reason we still
# check for nscd being enabled before adding to nssModules.

This comment has been minimized.

Copy link
@andir

andir May 5, 2020

Member

I hope @florianjacob can shed some light on this. I think we should never have to add vague comments like for some reason we still check for nscd being enabled before adding to nssModules. The source should be a place of truth :) If we can't figure it out and the original authors don't respond we might as well change the implementation until we encounter errors and can properly document them.

@flokli
Copy link
Contributor Author

flokli commented May 5, 2020

@andir as written in #86940 (comment), I'd really prefer to make the user explicitly force system.nssModules to [] if he really wants to disable nscd - as loading custom modules without nscd doesn't work anyways.

However, I wanted to keep the existing behaviour while moving things around, and plan to address this in a future PR.

@@ -138,6 +138,10 @@ in

users.users.resolved.group = "systemd-resolve";

# add resolve to nss hosts database if enabled and nscd enabled
# system.nssModules is configured in nixos/modules/system/boot/systemd.nix

This comment has been minimized.

Copy link
@arianvp

arianvp May 5, 2020

Member

This isn't enough. We need to mkOrder in such a way that we're sure dns follows resolve

This comment has been minimized.

Copy link
@flokli

flokli May 5, 2020

Author Contributor

dns is still added with (mkAfter [ "dns" ]) - try nix-build nixos/tests/networking.nix --arg networkd true -A dhcpOneIf.driver && result/bin/nixos-run-vms and cat /etc/nsswitch.conf on the client:

cat /etc/nsswitch.conf | grep hosts
hosts:    files mymachines resolve [!UNAVAIL=return] dns myhostname

This comment has been minimized.

Copy link
@arianvp

arianvp May 5, 2020

Member

perfect

@arianvp
arianvp approved these changes May 5, 2020
Copy link
Member

arianvp left a comment

LGTM. Let's hope we can get rid of the conditional check on nscd later.

@flokli flokli merged commit 265415f into NixOS:master May 5, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c0995d2"; rev="c0995d22eed1a19ac9442c8460c18dd6a4c389b7"; } ./pkgs/t
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
@flokli flokli deleted the flokli:move-nss-systemd branch May 5, 2020
@flokli flokli mentioned this pull request May 5, 2020
2 of 10 tasks complete
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

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