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 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.

Comment on lines +835 to +837
# 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.
Copy link
Contributor Author

@flokli flokli May 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also see #43607 and #86010 for context.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 dasJ left a comment

Choose a reason for hiding this comment

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

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

nixos/modules/system/boot/resolved.nix Show resolved Hide resolved
Comment on lines +835 to +837
# 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.
Copy link
Member

Choose a reason for hiding this comment

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

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 15:59
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
Copy link
Contributor Author

flokli commented May 5, 2020

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

@flokli flokli requested review from aanderse, andir and arianvp May 5, 2020 14:26
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

Overall 👍 just some comments.

nixos/modules/system/boot/resolved.nix Show resolved Hide resolved
Comment on lines +835 to +837
# 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

perfect

Copy link
Member

@arianvp arianvp left a comment

Choose a reason for hiding this comment

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

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
@flokli flokli deleted the move-nss-systemd branch May 5, 2020 21:17
@flokli flokli mentioned this pull request May 5, 2020
10 tasks
@lopsided98 lopsided98 mentioned this pull request Oct 4, 2020
10 tasks
@florianjacob
Copy link
Contributor

Finally, here comes the truth regarding optional config.services.nscd.enable systemd.out;: ‼️
In 2017, disabling nscd did not break your system that much, the systemd nss modules were not that crucial as they are today - my motivation behind the additional check was indeed to make nscd.enable = false; not result in the assertion error, not knowing whether I can dare to make users who want to disable nscd jump through this additional hoop.
Historical context PR: #26967

From today's perspective, this is indeed another silent source of errors, which I was actually trying to prevent with the assertion. Thank you @flokli for not waiting two years until I shed light on the reasons, and continuing in #87016 😅

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