logrotate: update to freeform by martinetd · Pull Request #162063 · NixOS/nixpkgs

martinetd · 2022-02-27T10:19:54Z

Motivation for this change

logrotate is much bigger than intended through its mailutils dependency as reported in #162001

This attempts to resolve the problem by making the dependency only appear when required.

Notes:

also included first commit for logrotate: do not enable logrotate.service itself #161929 that is a release blocker and will (should) get merged first
this isn't finished as illustrated by PR being a draft, in particular:
- ~~setting paths user/group doesn't work and was commented~~
- the compat renamedoptions from services.logrotate.config to extraConfig was removed as non-working, extraConfig would need to be readded as an explicit option for it to work (not difficult) but it was suggested that extraConfg be removed in Logrotate adds bloat to installation media closure #162001 (comment) so I'm not quite sure what to do?
- coupled with logrotate: do not enable logrotate.service itself #161929 since the service is no longer immediately started on system switch, it's very likely that a configuration error will slip unnoticed. I'd like to add a validation step (logrotate --debug config) somehow, ideally at build time, but I'm not sure how to do that.
  (EDIT: looking around I quite like what nixos/modules/services/networking/bird.nix is doing with a checkPhase on a file that can be disabled through cfg.checkConfig in pkgs.writeTextFile. We can do this easily, I'm mostly done with that)
  (EDIT2: done, but doesn't check users -- if someone knows how to do that it'd be great...)
- some of it is quite ugly and I'll be happy to take any improvement, I've favored compatibility over style but there's definitely ways to make this better...
- ~~EDIT: I have no idea why the manual update check fails -- updating an existing section is not allowed?~~

Things done

Built on platform(s)
- x86_64-linux
Tested, as applicable:
- NixOS test(s) (look inside nixos/tests)
Tested basic functionality of all binary files (usually in ./result/bin/)
22.05 Release Notes (or backporting 21.11 Release notes)
- (Module updates) Added a release notes entry if the change is significant
Fits CONTRIBUTING.md.

ju1m · 2022-02-27T12:32:32Z

@martinetd regarding the build error for the manual:

Check DocBook files generated from Markdown are consistent

this is related to a new todo item in the PRs' template:

(Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes

martinetd · 2022-02-27T14:37:16Z

Mostly fixed what I wanted, still need to figure out why the manual check fails now (+ stray tab but that one is easy and isn't related)... out of time for today, later!

Izorkin · 2022-02-27T20:08:38Z

Is it possible to add the ability to specify several different files?

  services.logrotate = {
    paths = {
      files = [ "file1" "file2" ];
      extraConfig = ''
        daily
      '';
    };
  };

Result:

file1
file2
{
  daily
}

martinetd · 2022-02-27T21:16:17Z

Is it possible to add the ability to specify several different files?

It was meant to be possible yes! I don't think I changed this part and didn't test but this obviously doesn't work as you've pointed out...
I'll add a test and fix it.

martinetd · 2022-02-27T22:13:51Z

I'm finishing this up but for your question this actually works, the problem is the usage -- I'm actually curious nix doesn't error on this for you, when I try I get this error as appropriate:

error: A definition for option `services.logrotate.paths.extraConfig' is not of type `attribute set of null or boolean or signed integer or strings'. Definition values:
       - In `/home/asmadeus/nixpkgs/nixos/lib/build-vms.nix':
           ''
             daily
           ''
(use '--show-trace' to show detailed location information)

There are two errors:

paths is an attrset of paths, so you don't define a section directly but need to name it
the special attribute key to specify files is 'path', not 'files', this must be kept for compatibility
This appears to work:

      services.logrotate.paths = {
        multipath = {
          path = [ "file1" "file2" ];
          #whatever attributes you want
        };
      };

And generates the following, which should work (I think):

"file1" "file2" {
daily
rotate 20
}

(daily and rotate 20 come from default values of frequency and keep; the current version will also have plenty of empty lines that I've trimmed in an update I'm planning to push as soon as I understand why manual stopped building...)

martinetd · 2022-02-27T22:34:22Z

Okay, I think we're good this time. Style aside I see no more big problem.
There are a couple of caveats:

the check added for logrotate isn't perfect, logrotate --debug is very picky (users that don't exist will generate an error (I'm stripping users for test), when running as normal user like now su as other users fail (also dealt with by stripping users), files missing when there is no missingok fail the check (enabled by default so not a big problem), if a permission denied happens because an intermediate directory isn't readable to our builder the check will fail (thanksfully there the sandbox is helpful, file will not not exist..))
overall it feels better than no check but it might be surprising to some and probably could deserve some improvement, although I don't see how lest of patching logrotate or writing our own verifier....
as said in original message I've favored backwards compatibility over style, there's much to be left desired. I didn't bother checking for 'mail' attribute in extraConfig though so these users will need adjusting, that might be worth a warning...

Happy to adjust things as requested, please have a look @ius @Artturin @aanderse @dasJ ! thanks!

ju1m

For a consistent code source style, you may run nixpkgs-fmt on the .nix files (in a dedicated commit to facilitate reviewing).
Great work @martined! Thanks for having taken time to revamp this service with so much attention to corner cases. :)

ju1m · 2022-02-28T03:00:26Z

nixos/modules/services/logging/logrotate.nix

Since user and group have default values this should be equivalent to settings.user != null and settings.group != null. If you ever need to see if a module option has been defined, you may add options to the { config, lib, pkgs, ... }: and use options.services.logrotate.settings.foo.isDefined.

Yes and no, that would be true for paths attrsets but generateSection is also used with services.logrotate.settings which doesn't have default values there.

ju1m · 2022-02-28T03:02:44Z

nixos/modules/services/logging/logrotate.nix

I don't think it's possible to use the real /etc/passwd and /etc/groups at build time, especially when mutableUsers == true.

ju1m · 2022-02-28T03:05:58Z

nixos/tests/logrotate.nix

defaultMachine would be clearer and needs no comment.

ju1m · 2022-02-28T03:16:14Z

nixos/modules/services/logging/logrotate.nix

If that settings is to be deprecated I would use mkRemovedOptionModule with a message indicating to use su, because a simple change in the description goes easily unnoticed.
And simplify the module and test by removing the new code to support the backward compatibility.

I wanted to leave a window with the setting deprecated but still working, it's possible by adding warnings by setting config.warnings conditionally (plenty of examples about) but I didn't get around to it yet.

I agree the description isn't enough in itself, but I think warnings can be added in a subsequent PR so as not to slow this down -- I don't really like PRs growing in scope all the time and prefer to keep goals somewhat constrainted... But if I get around to it fast enough I'll update this PR when I'm done.

(that being said, if the consensus is to deprecate settings the hard way, it'll definitely make this module a bit simpler and I don't particularly mind for myself. We can easily remove everywhere I've marked deprecated and most of the exceptions I added to generateConfig)

ju1m · 2022-02-28T03:25:46Z

nixos/modules/services/logging/logrotate.nix

For a prettier rendering, you may use: <xref linkend="opt-services.logrotate.settings"/>

martinetd

Thanks for the review! I'll fix the two suggestions I didn't reply to tonight, and check nixpkgs-fmt as well.

martinetd · 2022-02-28T03:45:50Z

nixos/modules/services/logging/logrotate.nix

I wanted to leave a window with the setting deprecated but still working, it's possible by adding warnings by setting config.warnings conditionally (plenty of examples about) but I didn't get around to it yet.

I agree the description isn't enough in itself, but I think warnings can be added in a subsequent PR so as not to slow this down -- I don't really like PRs growing in scope all the time and prefer to keep goals somewhat constrainted... But if I get around to it fast enough I'll update this PR when I'm done.

martinetd · 2022-02-28T03:46:52Z

nixos/modules/services/logging/logrotate.nix

Yes and no, that would be true for paths attrsets but generateSection is also used with services.logrotate.settings which doesn't have default values there.

Izorkin · 2022-02-28T05:15:31Z

@martinetd i am currently writing the logrotate configuration with extraConf. The resulting file looks like this:

notifempty
olddir all_log
daily
nocompress
rotate 30
missingok
dateext
dateformat -%Y-%m-%d
nomail
ifempty
sharedscripts

/var/log/sshd/auth.log
{
  postrotate
    systemctl reload syslog-ng
  endscript
}
olddir all_log
createolddir 0755 root root
daily
nocompress
rotate 30
missingok
dateext
dateformat _%Y-%m-%d
dateyesterday
datehourago
nomail
ifempty
sharedscripts

/var/log/nginx/site1_*.log
{
  create 0640 web-nginx www-user1
  postrotate
    [ ! -f /run/nginx/nginx.pid ] || kill -USR1 `cat /run/nginx/nginx.pid`
  endscript
}

...
/var/log/nginx/access.log
/var/log/nginx/error.log
/var/data/web/all/logs/nginx/access.log
/var/data/web/all/logs/nginx/error.log
{
  create 0640 web-nginx web-nginx
  postrotate
    [ ! -f /run/nginx/nginx.pid ] || kill -USR1 `cat /run/nginx/nginx.pid`
  endscript
}

Instead of this variant:

"file1" "file2" {
daily
rotate 20
}

It is possible to generate this variant:

"file1"
"file2"
{
daily
rotate 20
}

Will work too.

Izorkin · 2022-02-28T05:18:02Z

Another option would be to add the option of writing all the parameters to the very top of the configuration, which are used by default. The parameters specified below in the file overwrite the upper parameters.

Izorkin · 2022-02-28T05:20:12Z

  services.logrotate = {
    extraConfigDefault = ''
      daily
      nocompress
      rotate 30
      missingok
      dateext
      dateformat -%Y-%m-%d
      nomail
      ifempty
      sharedscripts
    '';
  };

martinetd · 2022-03-14T21:59:18Z

add a comment with a link to this pr and that the deprecated options should be removed before 22.11

Thanks for your review!

I've opened a PR with option removal (as draft) and added a comment with a link to it.

nixos/modules/services/logging/logrotate.nix

nixos/modules/virtualisation/azure-agent.nix

Artturin

Seems fine

Built a vm and compared the new generated file to the old one and it was ok

Artturin · 2022-03-15T17:36:18Z

squashed locally to preserve commit messages

martinetd · 2022-03-15T23:28:54Z

Thanks! I prefer to keep things split slightly though to illustrate logical changes, I've fixed the history up so all fixes commits are squashed with their appropriate counterpart, and we can just e.g. revert the logrotate-checkconf service commit if this becomes needed later, or simply for easier history reading while keeping the system bisectable (well, at least I've checked that nixosTests.logrotate passes for each commit individually, so logrotate is "instantiable" at all time)

You can check this results in no code change:

$ git diff 4a466cb 1647fd2
<empty>

I appreciate your effort of manually squashing though, and have kept your squashed commit around in a local branch if you strongly prefer a single commit for merging (just push it back or tell me to and I will)

having pkgs.logrotate depend on mailutils brings in quite a bit of dependencies through mailutil itself and recursive dependency to guile when most people do not need it. Remove mailutils dependency from the package, and conditionally add it to the service if the user specify the mail option either at top level or in a path Fixes NixOS#162001

Running once now will make further patches formatting easier

using freeform is the new standard way of using modules and should replace extraConfig. In particular, this will allow us to place a condition on mails

Now the service no longer starts immediately, check if the config we generated makes sense as soon as possible. The check isn't perfect because logrotate --debug wants to check users required, there are two problems: - /etc/passwd and /etc/group are sandboxed and we don't have visibility of system users - the check phase runs as nixbld which cannot su to other users and logrotate fails on this Until these two problems can be addressed, users-related checks are filtered out, it's still much better than no check. The check can be disabled with services.logrotate.checkConfig if required (bird also has a preCheck param, to prepare the environment before check, but we can add it if it becomes necessary) Since this makes for very verbose builds, we only show errors: There is no way to control log level, but logrotate hardcodes 'error:' at common log level, so we can use grep, taking care to keep error codes Some manual tests: ───────┬────────────────────────────────────────── │ File: valid-config.conf ───────┼────────────────────────────────────────── 1 │ missingok ───────┴────────────────────────────────────────── logrotate --debug ok grep ok ───────┬────────────────────────────────────────── │ File: postrotate-no-end.conf ───────┼────────────────────────────────────────── 1 │ missingok 2 │ /file { 3 │ postrotate 4 │ test 5 │ } ───────┴────────────────────────────────────────── error: postrotate-no-end.conf:prerotate, postrotate or preremove without endscript ───────┬────────────────────────────────────────── │ File: missing-file.conf ───────┼────────────────────────────────────────── 1 │ "test" { daily } ───────┴────────────────────────────────────────── error: stat of test failed: No such file or directory ───────┬────────────────────────────────────────── │ File: unknown-option.conf ───────┼────────────────────────────────────────── 1 │ some syntax error ───────┴────────────────────────────────────────── logrotate --debug ok error: unknown-option.conf:1 unknown option 'some' -- ignoring line ───────┬────────────────────────────────────────── │ File: unknown-user.conf ───────┼────────────────────────────────────────── 1 │ su notauser notagroup ───────┴────────────────────────────────────────── error: unknown-user.conf:1 unknown user 'notauser' In particular note that logrotate would not error on unknown option (it just ignores the line) but this change makes the check fail.

the build-time check is not safe (e.g. doesn't protect from bad users or nomissingok paths missing), so add a new unit for configuration switch time check

martinetd · 2022-03-31T23:09:20Z

aa0f27a introduced a small conflict so I've rebased the branch to today's master.

@aanderse my suggestion to review the final form in #164169 instead still stands if you
can find a moment (should be less convoluted than this as compatibility has been removed),
otherwise I think we're getting stuck -- this has had two proper reviews (from ju1m and
Artturin), should I try to find one more?

Artturin · 2022-04-01T02:48:07Z

@ofborg test logrotate gitlab nginx systemd

You have written tests so if these pass I think ill merge

martinetd · 2022-04-01T03:16:13Z

You have written tests so if these pass I think ill merge

The tests passed! thanks!

Artturin · 2022-04-01T03:21:23Z

Thanks for your work

aanderse · 2022-04-01T03:25:06Z

🚀

wizeman · 2022-06-09T03:28:26Z

nixos/modules/services/logging/logrotate.nix

+  };
+
+  mailOption =
+    if foldr (n: a: a || n ? mail) false (attrValues cfg.settings)


Sorry for the late comment, but this doesn't seem to be accounting for when the user sets mail = false (so that nomail appears in the appropriate section in logrotate.conf).

In other words, if the user sets mail = false in one of the sections in the settings, the --mail option will get added even though no section in the settings actually sets mail = true.

good point, I'll update it to a || n.mail or false in a new PR (EDIT: when I get home, so now + ~7 hours)

Done in #177106

wizeman · 2022-06-09T03:31:54Z

nixos/modules/services/logging/logrotate.nix

+
+  mailOption =
+    if foldr (n: a: a || n ? mail) false (attrValues cfg.settings)
+    then "--mail=${pkgs.mailutils}/bin/mail"


Again, sorry for the late comment, but shouldn't this be something like:

if /* ... */ then "\"--mail=${pkgs.mailutils}/bin/mail -s\"" else ""

i.e., shouldn't the -s argument be specified?

The logrotate man page says (about the --mail option):

This command should accept two arguments: 1) the subject of the message, and 2) the recipient.
(...)
The default mail command is /bin/mail -s.

Please note that I haven't actually tested sending mail, though (neither with nor without the -s option).

hm, this comes straight from what we had in the package, from the recent diff:

- ] ++ lib.optionals (mailutils != null) [ - "--with-default-mail-command=${mailutils}/bin/mail" ];

So if this doesn't work then it's likely sending mails from logrotate never worked...

I'll admit I never tested either myself, I'll give it a try this weekend unless someone beats me to it.

from logrotate changelog:

106: - remove `-s` from `DEFAULT_MAIL_COMMAND` and improve its documentation (#152)

You might be looking at old man page? that dates 3.13.0 (2017)....

looking at the code and not testing I think we're fine, my setup is half broken so it's a pain to test and I'll leave it at code inspection...

Yes, I think you are correct.
I was looking at https://linux.die.net/man/8/logrotate, which is what man configuration.nix pointed me at, but it seems that this man page is not up-to-date.

martinetd mentioned this pull request Feb 27, 2022

Logrotate adds bloat to installation media closure #162001

Closed

ofborg bot requested a review from viric February 27, 2022 10:34

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 27, 2022

martinetd force-pushed the logrotate_size branch 2 times, most recently from b7c0fed to 8a64038 Compare February 27, 2022 14:29

martinetd marked this pull request as ready for review February 27, 2022 14:30

martinetd requested a review from dasJ as a code owner February 27, 2022 14:30

martinetd marked this pull request as draft February 27, 2022 14:36

martinetd force-pushed the logrotate_size branch from 8a64038 to 2097eba Compare February 27, 2022 22:26

martinetd marked this pull request as ready for review February 27, 2022 22:26

martinetd force-pushed the logrotate_size branch from 2097eba to d5f547e Compare February 27, 2022 22:33

martinetd force-pushed the logrotate_size branch 2 times, most recently from 3058cc8 to d3c9e54 Compare February 27, 2022 22:37

ju1m reviewed Feb 28, 2022

View reviewed changes

martinetd commented Feb 28, 2022

View reviewed changes

Artturin reviewed Mar 14, 2022

View reviewed changes

nixos/modules/services/logging/logrotate.nix Outdated Show resolved Hide resolved

martinetd force-pushed the logrotate_size branch from 8e6c6a2 to a279768 Compare March 14, 2022 22:36

martinetd mentioned this pull request Mar 14, 2022

logrotate: cleanup legacy options #164169

Merged

13 tasks

Artturin reviewed Mar 14, 2022

View reviewed changes

nixos/modules/services/logging/logrotate.nix Outdated Show resolved Hide resolved

Artturin reviewed Mar 14, 2022

View reviewed changes

nixos/modules/virtualisation/azure-agent.nix Outdated Show resolved Hide resolved

Artturin approved these changes Mar 15, 2022

View reviewed changes

Artturin force-pushed the logrotate_size branch 2 times, most recently from 21c9129 to 4a466cb Compare March 15, 2022 17:35

martinetd force-pushed the logrotate_size branch from 4a466cb to 1647fd2 Compare March 15, 2022 23:20

martinetd added 7 commits April 1, 2022 07:09

logrotate: run through nixpkgs-fmt

3a2fa0d

Running once now will make further patches formatting easier

logrotate: add services.logrotate.configFile escape hatch

3cc8ea2

nixos/logrotate: convert to freeform

e92c053

using freeform is the new standard way of using modules and should replace extraConfig. In particular, this will allow us to place a condition on mails

nginx/logrotate: run logrotate as nginx user

b0a04e4

logrotate: add logrotate-checkconf.service

829c611

the build-time check is not safe (e.g. doesn't protect from bad users or nomissingok paths missing), so add a new unit for configuration switch time check

martinetd force-pushed the logrotate_size branch from 1647fd2 to 829c611 Compare March 31, 2022 22:10

martinetd requested a review from a team as a code owner March 31, 2022 22:10

Artturin merged commit c7ac6ff into NixOS:master Apr 1, 2022

wizeman reviewed Jun 9, 2022

View reviewed changes

martinetd deleted the logrotate_size branch October 5, 2022 07:17

Uh oh!

Conversation

martinetd commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

ju1m commented Feb 27, 2022

Uh oh!

martinetd commented Feb 27, 2022

Uh oh!

Izorkin commented Feb 27, 2022

Uh oh!

martinetd commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinetd commented Feb 27, 2022

Uh oh!

martinetd commented Feb 27, 2022

Uh oh!

ju1m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinetd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Izorkin commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Izorkin commented Feb 28, 2022

Uh oh!

Izorkin commented Feb 28, 2022

Uh oh!

martinetd commented Mar 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Artturin left a comment

Choose a reason for hiding this comment

Uh oh!

Artturin commented Mar 15, 2022

Uh oh!

martinetd commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinetd commented Mar 31, 2022

Uh oh!

Artturin commented Apr 1, 2022

Uh oh!

martinetd commented Apr 1, 2022

Uh oh!

Artturin commented Apr 1, 2022

Uh oh!

aanderse commented Apr 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinetd Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

martinetd commented Feb 27, 2022 •

edited

Loading

martinetd commented Feb 27, 2022 •

edited

Loading

ju1m left a comment •

edited

Loading

Izorkin commented Feb 28, 2022 •

edited

Loading

martinetd commented Mar 15, 2022 •

edited

Loading

martinetd Jun 9, 2022 •

edited

Loading

wizeman Jun 9, 2022 •

edited

Loading

wizeman Jun 13, 2022 •

edited

Loading