NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.44k stars 13.65k forks source link

logrotate service does not use upstream systemd unit #178867

Open erdnaxe opened 2 years ago

erdnaxe commented 2 years ago

Describe the bug

It seems that NixOS logrotate module manually defines logrotate.service: https://github.com/NixOS/nixpkgs/blob/nixos-22.05/nixos/modules/services/logging/logrotate.nix#L388

Upstream ships a logrotate.service with hardening options: https://github.com/logrotate/logrotate/blob/master/examples/logrotate.service

ArchLinux, OpenSUSE, Fedora and Debian are all using upstream logrotate.service. Is there a reason we are not following this trend and shipping a less hardened configuration?

Steps To Reproduce

Steps to reproduce the behavior:

  1. Enable logrotate service on NixOS.
  2. systemctl cat logrotate and observe that it does not contain upstream options.

Notify maintainers

@viric

Metadata

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 5.15.47, NixOS, 22.05 (Quokka), 22.05.1191.ccf8bdf7262`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.8.1`
 - channels(root): `"home-manager-22.05.tar.gz, nixos-22.05, nixos-hardware, nixos-unstable"`
 - channels(erdnaxe): `""`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`
SuperSandro2000 commented 2 years ago

ping @martinetd

martinetd commented 2 years ago

Thanks for the cc.

There's no reason except nobody did it, no. I wasn't aware of this service when I did my rework and don't personally feel strongly about using upstream service units.

We could use the upstream unit with a symlink to /etc/logrotate.conf and/or overriding just the start action whichever looks better. The hardening options probably ought to be somehow copied if the immediate check service is kept (I'd like it to), otherwise it might not fail if the hardening introduce some regressions making the check less useful, but that's not a hard requirement either.

I don't think I'll be implementing that switch in the forseeable future but would be happy to review/test if anyone does, and the tests in nixosTests.logrotate should be enough to do this fearlessly so that'd be a good first issue if we have such a label.

(EDIT: missed negation...)

aanderse commented 2 years ago

@erdnaxe looks like a good unit from upstream. Awesome!

Is there any information you require to craft up a PR to do this? It would be great if you could! And I would be more than happy to provide answer any questions if you're not sure how to do something.