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/nscd: start in early boot #154928

Closed
wants to merge 1 commit into from
Closed

nixos/nscd: start in early boot #154928

wants to merge 1 commit into from

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Jan 13, 2022

Services that have dynamic users require nscd to resolve users
via pam_systemd. Those services might not even create
their own dynamic users itself i.e. iptables.
To make sure nscd is always started when this is happening we move
nscd to sysinit.target and make sure that it is always started before
starting/reloading/restarting any other service.

Motivation for this change

This pr was original opened here: #106336

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@arcnmx
Copy link
Member

arcnmx commented Jan 13, 2022

Thanks, I can confirm this fixes the issue with DynamicUser services on switch!

Before:

restarting the following units: ddclient.timer, home-manager-arc.service, home-manager-root.service, polkit.service
starting the following units: accounts-daemon.service, nscd.service
warning: the following units failed: ddclient.service
...
3jifr0sab601i8l2ka07c2c2cak5csg2-ddclient-prestart[430095]: install: invalid user ‘ddclient’

After:

starting nscd
restarting the following units: ddclient.timer, home-manager-arc.service, home-manager-root.service, polkit.service
starting the following units: accounts-daemon.service

@dasJ
Copy link
Member

dasJ commented Jan 13, 2022

I highly recommend stopIfChanged = false. While the default of true is already crappy in most cases, it's even worst for nscd because stc can stop nscd, restart a service, and then only start nscd afterwards, causing the service that is restarted in between to be started without a working nscd. This way we don't need to modify stc

@dasJ
Copy link
Member

dasJ commented Jan 13, 2022

Source: We have been doing so in our downstream repo for ~1 year and have had no issues since then

@@ -474,6 +476,13 @@ sub filterUnits {
print STDERR "setting up tmpfiles\n";
system("@systemd@/bin/systemd-tmpfiles", "--create", "--remove", "--exclude-prefix=/dev") == 0 or $res = 3;

# We need to start nscd before any other service, since they might need
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of yet another special case

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a way around it without that does not involve getting rid of nscd.

@dasJ
Copy link
Member

dasJ commented Jan 13, 2022

Also it may be helpful to just fix nscd.service instead:

$ systemctl show nscd | grep WantedBy
WantedBy=nss-user-lookup.target nss-lookup.target

$ systemctl show nss-lookup.target | grep By
$ systemctl show nss-user-lookup.target | grep By

Of course stc will not start nscd if nobody wants the service…

@Mic92
Copy link
Member Author

Mic92 commented Jan 15, 2022

I highly recommend stopIfChanged = false. While the default of true is already crappy in most cases, it's even worst for nscd because stc can stop nscd, restart a service, and then only start nscd afterwards, causing the service that is restarted in between to be started without a working nscd. This way we don't need to modify stc

This also has the big downside that nss changes no longer are applied until reboot.

@Mic92
Copy link
Member Author

Mic92 commented Jan 15, 2022

Also it may be helpful to just fix nscd.service instead:

$ systemctl show nscd | grep WantedBy
WantedBy=nss-user-lookup.target nss-lookup.target

$ systemctl show nss-lookup.target | grep By
$ systemctl show nss-user-lookup.target | grep By

Of course stc will not start nscd if nobody wants the service…

This is a foot gun at best. Non one expects a dependency on nscd when using DynamicUser.

@Mic92
Copy link
Member Author

Mic92 commented Jan 15, 2022

If we don't fix this I would consider DynamicUser not as fit for purpose in NixOS.

Mic92 added a commit to Mic92/nixpkgs that referenced this pull request Jan 15, 2022
@arcnmx
Copy link
Member

arcnmx commented Jan 15, 2022

This is a foot gun at best. Non one expects a dependency on nscd when using DynamicUser.

Would it be a footgun if the module (systemd unit generator) automatically did it for you? That wouldn't work for units coming from systemd.packages, but it could probably assert it at build time and request that it be added?

@Mic92
Copy link
Member Author

Mic92 commented Jan 16, 2022

This is a foot gun at best. Non one expects a dependency on nscd when using DynamicUser.

Would it be a footgun if the module (systemd unit generator) automatically did it for you? That wouldn't work for units coming from systemd.packages, but it could probably assert it at build time and request that it be added?

Maybe but I feel this is more code/complexity in all of nixpkgs than just having them in the activation script. Also we can no longer use some of the upstream systemd units as is.

@flokli
Copy link
Contributor

flokli commented Jan 19, 2022

Can we drop this in favor of #155655?

With this in, requests before nscd is up should still work.

@erikarvstedt
Copy link
Member

erikarvstedt commented Jan 20, 2022

@flokli: It's common practice to use the stable NixOS release for the system modules and run services with binaries from unstable on top of that. When the glibc minor release differs between stable and unstable, #155655 won't work for these services.
So +1 for this PR, I don't see a simpler way to achieve this.

@flokli
Copy link
Contributor

flokli commented Jan 20, 2022

But I'd assume we can revert this after #155655 is merged, or in a followup PR, to get closer with upstream again?

@arianvp
Copy link
Member

arianvp commented Jan 20, 2022

Given that #155655 still leaves nscd enabled by default this still seems a useful change no? Or would during early boot DynamicUser still work due to the fallback built in #155655 ?

@erikarvstedt
Copy link
Member

erikarvstedt commented Jan 20, 2022

For systems that have services using different glibc minor versions, #155655 is no replacement for this PR.
For all other systems, it is.
(#155655 only provides access to NSS modules for services using the system glibc.)
But we have to support the multi-glibc use case when nscd is enabled, so this PR is still needed, even with #155655.

@Mic92
Copy link
Member Author

Mic92 commented Jan 27, 2022

rebased.

Copy link
Contributor

@nrdxp nrdxp left a comment

Choose a reason for hiding this comment

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

Everything appears in order. @Mic92 is it ready for merge? I can pull the trigger if so.

@dasJ
Copy link
Member

dasJ commented Jan 27, 2022

Still not a fan at all. Adding yet another special case to stc that could also be implemented by native systemd functionality.
#154620 would immensely improve the current situation, combined with stopIfChanged there almost no issues.

A fix that would involve only systemd logic would be to add the After=s in the systemd builder when DynamicUser is set. This way there is no additional NixOS-specific tinkering in a language most people dislike.

But do what you think is best.

@nrdxp
Copy link
Contributor

nrdxp commented Jan 27, 2022

You suggestion sounds reasonable to me.I'll leave open for a bit to give time for consideration.

Services that have dynamic users require nscd to resolve users
via pam_systemd. Those services might not even create
their own dynamic users itself i.e. iptables.
To make sure nscd is always started when this is happening we move
nscd to sysinit.target and make sure that it is always started before
starting/reloading/restarting any other service.
@Mic92
Copy link
Member Author

Mic92 commented Apr 4, 2022

Rebased.

@Mic92
Copy link
Member Author

Mic92 commented Apr 4, 2022

Also I will check if #154620 is enough.

@symphorien
Copy link
Member

Still not a fan at all. Adding yet another special case to stc that could also be implemented by native systemd functionality. #154620 would immensely improve the current situation, combined with stopIfChanged there almost no issues.

A fix that would involve only systemd logic would be to add the After=s in the systemd builder when DynamicUser is set. This way there is no additional NixOS-specific tinkering in a language most people dislike.

But do what you think is best.

this is what I proposed in #105354, but it was rejected.

@tilpner
Copy link
Member

tilpner commented Jun 5, 2022

Isn't this already fixed by #154320?

@symphorien
Copy link
Member

I had the issue on 2022-05-12 on a system updated daily.

@flokli
Copy link
Contributor

flokli commented Oct 10, 2022

@dasJ @ajs124 is this still necessary, with #186785 merged?

@dasJ
Copy link
Member

dasJ commented Oct 10, 2022

I highly doubt it

@dasJ dasJ closed this Oct 20, 2022
@Mic92 Mic92 deleted the nscd-fix branch November 20, 2022 19:15
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

10 participants