From b457d917dcfa6d73fdb7b9317440ff5fb5ee4b8b Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 27 Feb 2022 19:06:44 +0900 Subject: [PATCH 1/7] logrotate: move mail dependency from package to service 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 #162001 --- nixos/modules/services/logging/logrotate.nix | 16 ++++++---- nixos/tests/logrotate.nix | 31 +++++++++++++------- pkgs/tools/system/logrotate/default.nix | 3 -- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix index 082cf92ff4ef..8dfece8109d3 100644 --- a/nixos/modules/services/logging/logrotate.nix +++ b/nixos/modules/services/logging/logrotate.nix @@ -97,12 +97,18 @@ let ''; paths = sortProperties (attrValues (filterAttrs (_: pathOpts: pathOpts.enable) cfg.paths)); - configFile = pkgs.writeText "logrotate.conf" ( - concatStringsSep "\n" ( + configText = concatStringsSep "\n" ( [ "missingok" "notifempty" cfg.extraConfig ] ++ (map mkConf paths) - ) - ); + ); + configFile = pkgs.writeText "logrotate.conf" configText; + mailOption = + # add mail option to service if a mail is requested in config + # this ugly match will be replaced by cleaner attribute check in + # the near future + if builtins.match "(.*[[:space:]])?mail[[:space:]].*" configText != null + then "--mail=${pkgs.mailutils}/bin/mail" + else ""; in { imports = [ @@ -172,7 +178,7 @@ in serviceConfig = { Restart = "no"; User = "root"; - ExecStart = "${pkgs.logrotate}/sbin/logrotate ${configFile}"; + ExecStart = "${pkgs.logrotate}/sbin/logrotate ${mailOption} ${configFile}"; }; }; }; diff --git a/nixos/tests/logrotate.nix b/nixos/tests/logrotate.nix index 7e3046c94272..85923465593a 100644 --- a/nixos/tests/logrotate.nix +++ b/nixos/tests/logrotate.nix @@ -1,26 +1,33 @@ # Test logrotate service works and is enabled by default -import ./make-test-python.nix ({ pkgs, ...} : rec { +import ./make-test-python.nix ({ pkgs, ... }: rec { name = "logrotate"; meta = with pkgs.lib.maintainers; { maintainers = [ martinetd ]; }; - # default machine - nodes.machine = { ... }: { + nodes = { + defaultMachine = { ... }: { }; + machine = { config, ... }: { + services.logrotate.paths = { + # using mail somewhere should add --mail to logrotate invokation + sendmail = { + extraConfig = "mail user@domain.tld"; + }; + }; + }; }; testScript = '' with subtest("whether logrotate works"): - machine.succeed( - # we must rotate once first to create logrotate stamp - "systemctl start logrotate.service") + # we must rotate once first to create logrotate stamp + defaultMachine.succeed("systemctl start logrotate.service") # we need to wait for console text once here to # clear console buffer up to this point for next wait - machine.wait_for_console_text('logrotate.service: Deactivated successfully') + defaultMachine.wait_for_console_text('logrotate.service: Deactivated successfully') - machine.succeed( + defaultMachine.succeed( # wtmp is present in default config. "rm -f /var/log/wtmp*", # we need to give it at least 1MB @@ -28,10 +35,14 @@ import ./make-test-python.nix ({ pkgs, ...} : rec { # move into the future and check rotation. "date -s 'now + 1 month + 1 day'") - machine.wait_for_console_text('logrotate.service: Deactivated successfully') - machine.succeed( + defaultMachine.wait_for_console_text('logrotate.service: Deactivated successfully') + defaultMachine.succeed( # check rotate worked "[ -e /var/log/wtmp.1 ]", ) + with subtest("default config does not have mail"): + defaultMachine.fail("systemctl cat logrotate.service | grep -- --mail") + with subtest("using mails adds mail option"): + machine.succeed("systemctl cat logrotate.service | grep -- --mail") ''; }) diff --git a/pkgs/tools/system/logrotate/default.nix b/pkgs/tools/system/logrotate/default.nix index f0ce08383359..4c9536cd7cf4 100644 --- a/pkgs/tools/system/logrotate/default.nix +++ b/pkgs/tools/system/logrotate/default.nix @@ -1,5 +1,4 @@ { lib, stdenv, fetchFromGitHub, gzip, popt, autoreconfHook -, mailutils ? null , aclSupport ? true, acl , nixosTests }: @@ -19,8 +18,6 @@ stdenv.mkDerivation rec { configureFlags = [ "--with-compress-command=${gzip}/bin/gzip" "--with-uncompress-command=${gzip}/bin/gunzip" - ] ++ lib.optionals (mailutils != null) [ - "--with-default-mail-command=${mailutils}/bin/mail" ]; nativeBuildInputs = [ autoreconfHook ]; From 3a2fa0d04928e67d20f10df75a60fa2ea0002a15 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Tue, 1 Mar 2022 06:53:15 +0900 Subject: [PATCH 2/7] logrotate: run through nixpkgs-fmt Running once now will make further patches formatting easier --- nixos/modules/services/logging/logrotate.nix | 25 +++++++++++--------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix index 8dfece8109d3..7e32296c7909 100644 --- a/nixos/modules/services/logging/logrotate.nix +++ b/nixos/modules/services/logging/logrotate.nix @@ -5,7 +5,7 @@ with lib; let cfg = config.services.logrotate; - pathOpts = { name, ... }: { + pathOpts = { name, ... }: { options = { enable = mkOption { type = types.bool; @@ -98,8 +98,8 @@ let paths = sortProperties (attrValues (filterAttrs (_: pathOpts: pathOpts.enable) cfg.paths)); configText = concatStringsSep "\n" ( - [ "missingok" "notifempty" cfg.extraConfig ] ++ (map mkConf paths) - ); + [ "missingok" "notifempty" cfg.extraConfig ] ++ (map mkConf paths) + ); configFile = pkgs.writeText "logrotate.conf" configText; mailOption = @@ -124,7 +124,7 @@ in paths = mkOption { type = with types; attrsOf (submodule pathOpts); - default = {}; + default = { }; description = '' Attribute set of paths to rotate. The order each block appears in the generated configuration file can be controlled by the priority option @@ -163,13 +163,16 @@ in }; config = mkIf cfg.enable { - assertions = mapAttrsToList (name: pathOpts: - { assertion = (pathOpts.user != null) == (pathOpts.group != null); - message = '' - If either of `services.logrotate.paths.${name}.user` or `services.logrotate.paths.${name}.group` are specified then *both* must be specified. - ''; - } - ) cfg.paths; + assertions = mapAttrsToList + (name: pathOpts: + { + assertion = (pathOpts.user != null) == (pathOpts.group != null); + message = '' + If either of `services.logrotate.paths.${name}.user` or `services.logrotate.paths.${name}.group` are specified then *both* must be specified. + ''; + } + ) + cfg.paths; systemd.services.logrotate = { description = "Logrotate Service"; From 3cc8ea28d1c20320b674d3d4131d02e4df8df5fa Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Tue, 1 Mar 2022 22:02:32 +0900 Subject: [PATCH 3/7] logrotate: add services.logrotate.configFile escape hatch --- nixos/modules/services/logging/logrotate.nix | 23 +++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix index 7e32296c7909..d16a5a571ba3 100644 --- a/nixos/modules/services/logging/logrotate.nix +++ b/nixos/modules/services/logging/logrotate.nix @@ -122,6 +122,27 @@ in defaultText = literalExpression "cfg.paths != {}"; }; + configFile = mkOption { + type = types.path; + default = configFile; + defaultText = '' + A configuration file automatically generated by NixOS. + ''; + description = '' + Override the configuration file used by MySQL. By default, + NixOS generates one automatically from . + ''; + example = literalExpression '' + pkgs.writeText "logrotate.conf" ''' + missingok + "/var/log/*.log" { + rotate 4 + weekly + } + '''; + ''; + }; + paths = mkOption { type = with types; attrsOf (submodule pathOpts); default = { }; @@ -181,7 +202,7 @@ in serviceConfig = { Restart = "no"; User = "root"; - ExecStart = "${pkgs.logrotate}/sbin/logrotate ${mailOption} ${configFile}"; + ExecStart = "${pkgs.logrotate}/sbin/logrotate ${mailOption} ${cfg.configFile}"; }; }; }; From e92c05349c6053df22cf21eb9f424251ba2b114f Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Tue, 1 Mar 2022 06:54:12 +0900 Subject: [PATCH 4/7] nixos/logrotate: convert to freeform 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 --- .../from_md/release-notes/rl-2205.section.xml | 17 +- .../manual/release-notes/rl-2205.section.md | 5 +- nixos/modules/services/logging/logrotate.nix | 192 +++++++++++++++--- nixos/modules/services/misc/gitlab.nix | 16 +- .../services/networking/lxd-image-server.nix | 18 +- .../web-servers/apache-httpd/default.nix | 21 +- .../services/web-servers/nginx/default.nix | 16 +- nixos/modules/system/boot/systemd.nix | 18 +- nixos/modules/virtualisation/azure-agent.nix | 14 +- nixos/tests/logrotate.nix | 80 +++++++- 10 files changed, 297 insertions(+), 100 deletions(-) diff --git a/nixos/doc/manual/from_md/release-notes/rl-2205.section.xml b/nixos/doc/manual/from_md/release-notes/rl-2205.section.xml index 99af050d7baf..9535d441740b 100644 --- a/nixos/doc/manual/from_md/release-notes/rl-2205.section.xml +++ b/nixos/doc/manual/from_md/release-notes/rl-2205.section.xml @@ -1655,9 +1655,20 @@ - services.logrotate.enable now defaults to - true if any rotate path has been defined, and some paths have - been added by default. + services.logrotate.enable + now defaults to true if any rotate path has been defined, and + some paths have been added by default. + + + + + The logrotate module also has been updated to freeform syntax: + services.logrotate.paths + and + services.logrotate.extraConfig + will work, but issue deprecation warnings and + services.logrotate.settings + should now be used instead. diff --git a/nixos/doc/manual/release-notes/rl-2205.section.md b/nixos/doc/manual/release-notes/rl-2205.section.md index 3e883625664e..377dd1b5cae1 100644 --- a/nixos/doc/manual/release-notes/rl-2205.section.md +++ b/nixos/doc/manual/release-notes/rl-2205.section.md @@ -578,8 +578,11 @@ In addition to numerous new and upgraded packages, this release has the followin - `services.mattermost.plugins` has been added to allow the declarative installation of Mattermost plugins. Plugins are automatically repackaged using autoPatchelf. -- `services.logrotate.enable` now defaults to true if any rotate path has +- [services.logrotate.enable](#opt-services.logrotate.enable) now defaults to true if any rotate path has been defined, and some paths have been added by default. +- The logrotate module also has been updated to freeform syntax: [services.logrotate.paths](#opt-services.logrotate.paths) + and [services.logrotate.extraConfig](#opt-services.logrotate.extraConfig) will work, but issue deprecation + warnings and [services.logrotate.settings](#opt-services.logrotate.settings) should now be used instead. - The `zrepl` package has been updated from 0.4.0 to 0.5: diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix index d16a5a571ba3..6a9ed469fd38 100644 --- a/nixos/modules/services/logging/logrotate.nix +++ b/nixos/modules/services/logging/logrotate.nix @@ -5,6 +5,9 @@ with lib; let cfg = config.services.logrotate; + # deprecated legacy compat settings + # these options will be removed before 22.11 in the following PR: + # https://github.com/NixOS/nixpkgs/pull/164169 pathOpts = { name, ... }: { options = { enable = mkOption { @@ -86,27 +89,77 @@ let config.name = name; }; - mkConf = pathOpts: '' - # generated by NixOS using the `services.logrotate.paths.${pathOpts.name}` attribute set - ${concatMapStringsSep " " (path: ''"${path}"'') (toList pathOpts.path)} { - ${optionalString (pathOpts.user != null || pathOpts.group != null) "su ${pathOpts.user} ${pathOpts.group}"} - ${pathOpts.frequency} - rotate ${toString pathOpts.keep} - ${pathOpts.extraConfig} - } - ''; - - paths = sortProperties (attrValues (filterAttrs (_: pathOpts: pathOpts.enable) cfg.paths)); - configText = concatStringsSep "\n" ( - [ "missingok" "notifempty" cfg.extraConfig ] ++ (map mkConf paths) + generateLine = n: v: + if builtins.elem n [ "files" "priority" "enable" "global" ] || v == null then null + else if builtins.elem n [ "extraConfig" "frequency" ] then "${v}\n" + else if builtins.elem n [ "firstaction" "lastaction" "prerotate" "postrotate" "preremove" ] + then "${n}\n ${v}\n endscript\n" + else if isInt v then "${n} ${toString v}\n" + else if v == true then "${n}\n" + else if v == false then "no${n}\n" + else "${n} ${v}\n"; + generateSection = indent: settings: concatStringsSep (fixedWidthString indent " " "") ( + filter (x: x != null) (mapAttrsToList generateLine settings) + ); + + # generateSection includes a final newline hence weird closing brace + mkConf = settings: + if settings.global or false then generateSection 0 settings + else '' + ${concatMapStringsSep "\n" (files: ''"${files}"'') (toList settings.files)} { + ${generateSection 2 settings}} + ''; + + # below two mapPaths are compat functions + mapPathOptToSetting = n: v: + if n == "keep" then nameValuePair "rotate" v + else if n == "path" then nameValuePair "files" v + else nameValuePair n v; + + mapPathsToSettings = path: pathOpts: + nameValuePair path ( + filterAttrs (n: v: ! builtins.elem n [ "user" "group" "name" ] && v != "") ( + (mapAttrs' mapPathOptToSetting pathOpts) // + { + su = + if pathOpts.user != null + then "${pathOpts.user} ${pathOpts.group}" + else null; + } + ) + ); + + settings = sortProperties (attrValues (filterAttrs (_: settings: settings.enable) ( + foldAttrs recursiveUpdate { } [ + { + header = { + enable = true; + missingok = true; + notifempty = true; + frequency = "weekly"; + rotate = 4; + }; + # compat section + extraConfig = { + enable = (cfg.extraConfig != ""); + global = true; + extraConfig = cfg.extraConfig; + priority = 101; + }; + } + (mapAttrs' mapPathsToSettings cfg.paths) + cfg.settings + { header = { global = true; priority = 100; }; } + ] + ))); + configFile = pkgs.writeText "logrotate.conf" ( + concatStringsSep "\n" ( + map mkConf settings + ) ); - configFile = pkgs.writeText "logrotate.conf" configText; mailOption = - # add mail option to service if a mail is requested in config - # this ugly match will be replaced by cleaner attribute check in - # the near future - if builtins.match "(.*[[:space:]])?mail[[:space:]].*" configText != null + if foldr (n: a: a || n ? mail) false (attrValues cfg.settings) then "--mail=${pkgs.mailutils}/bin/mail" else ""; in @@ -118,8 +171,68 @@ in options = { services.logrotate = { enable = mkEnableOption "the logrotate systemd service" // { - default = foldr (n: a: a || n.enable) false (attrValues cfg.paths); - defaultText = literalExpression "cfg.paths != {}"; + default = foldr (n: a: a || n.enable) false (attrValues cfg.settings); + defaultText = literalExpression "cfg.settings != {}"; + }; + + settings = mkOption { + default = { }; + description = '' + logrotate freeform settings: each attribute here will define its own section, + ordered by priority, which can either define files to rotate with their settings + or settings common to all further files settings. + Refer to for details. + ''; + type = types.attrsOf (types.submodule ({ name, ... }: { + freeformType = with types; attrsOf (nullOr (oneOf [ int bool str ])); + + options = { + enable = mkEnableOption "setting individual kill switch" // { + default = true; + }; + + global = mkOption { + type = types.bool; + default = false; + description = '' + Whether this setting is a global option or not: set to have these + settings apply to all files settings with a higher priority. + ''; + }; + files = mkOption { + type = with types; either str (listOf str); + default = name; + defaultText = '' + The attrset name if not specified + ''; + description = '' + Single or list of files for which rules are defined. + The files are quoted with double-quotes in logrotate configuration, + so globs and spaces are supported. + Note this setting is ignored if globals is true. + ''; + }; + + frequency = mkOption { + type = types.nullOr types.str; + default = null; + description = '' + How often to rotate the logs. Defaults to previously set global setting, + which itself defauts to weekly. + ''; + }; + + priority = mkOption { + type = types.int; + default = 1000; + description = '' + Order of this logrotate block in relation to the others. The semantics are + the same as with `lib.mkOrder`. Smaller values are inserted first. + ''; + }; + }; + + })); }; configFile = mkOption { @@ -130,7 +243,7 @@ in ''; description = '' Override the configuration file used by MySQL. By default, - NixOS generates one automatically from . + NixOS generates one automatically from . ''; example = literalExpression '' pkgs.writeText "logrotate.conf" ''' @@ -143,6 +256,7 @@ in ''; }; + # deprecated legacy compat settings paths = mkOption { type = with types; attrsOf (submodule pathOpts); default = { }; @@ -150,6 +264,7 @@ in Attribute set of paths to rotate. The order each block appears in the generated configuration file can be controlled by the priority option using the same semantics as `lib.mkOrder`. Smaller values have a greater priority. + This setting has been deprecated in favor of logrotate settings. ''; example = literalExpression '' { @@ -178,22 +293,37 @@ in description = '' Extra contents to append to the logrotate configuration file. Refer to for details. + This setting has been deprecated in favor of + logrotate settings. ''; }; }; }; config = mkIf cfg.enable { - assertions = mapAttrsToList - (name: pathOpts: - { - assertion = (pathOpts.user != null) == (pathOpts.group != null); - message = '' - If either of `services.logrotate.paths.${name}.user` or `services.logrotate.paths.${name}.group` are specified then *both* must be specified. - ''; - } - ) - cfg.paths; + assertions = + mapAttrsToList + (name: pathOpts: + { + assertion = (pathOpts.user != null) == (pathOpts.group != null); + message = '' + If either of `services.logrotate.paths.${name}.user` or `services.logrotate.paths.${name}.group` are specified then *both* must be specified. + ''; + }) + cfg.paths; + + warnings = + (mapAttrsToList + (name: pathOpts: '' + Using config.services.logrotate.paths.${name} is deprecated and will become unsupported in a future release. + Please use services.logrotate.settings instead. + '') + cfg.paths + ) ++ + (optional (cfg.extraConfig != "") '' + Using config.services.logrotate.extraConfig is deprecated and will become unsupported in a future release. + Please use services.logrotate.settings with globals=true instead. + ''); systemd.services.logrotate = { description = "Logrotate Service"; diff --git a/nixos/modules/services/misc/gitlab.nix b/nixos/modules/services/misc/gitlab.nix index e48444f71612..488c3be7b653 100644 --- a/nixos/modules/services/misc/gitlab.nix +++ b/nixos/modules/services/misc/gitlab.nix @@ -848,10 +848,7 @@ in { extraConfig = mkOption { type = types.lines; - default = '' - copytruncate - compress - ''; + default = ""; description = '' Extra logrotate config options for this path. Refer to for details. @@ -977,13 +974,14 @@ in { # Enable rotation of log files services.logrotate = { enable = cfg.logrotate.enable; - paths = { + settings = { gitlab = { - path = "${cfg.statePath}/log/*.log"; - user = cfg.user; - group = cfg.group; + files = "${cfg.statePath}/log/*.log"; + su = "${cfg.user} ${cfg.group}"; frequency = cfg.logrotate.frequency; - keep = cfg.logrotate.keep; + rotate = cfg.logrotate.keep; + copytruncate = true; + compress = true; extraConfig = cfg.logrotate.extraConfig; }; }; diff --git a/nixos/modules/services/networking/lxd-image-server.nix b/nixos/modules/services/networking/lxd-image-server.nix index b119ba8acf63..d326626eed44 100644 --- a/nixos/modules/services/networking/lxd-image-server.nix +++ b/nixos/modules/services/networking/lxd-image-server.nix @@ -51,18 +51,14 @@ in environment.etc."lxd-image-server/config.toml".source = format.generate "config.toml" cfg.settings; - services.logrotate.paths.lxd-image-server = { - path = "/var/log/lxd-image-server/lxd-image-server.log"; + services.logrotate.settings.lxd-image-server = { + files = "/var/log/lxd-image-server/lxd-image-server.log"; frequency = "daily"; - keep = 21; - extraConfig = '' - create 755 lxd-image-server ${cfg.group} - missingok - compress - delaycompress - copytruncate - notifempty - ''; + rotate = 21; + create = "755 lxd-image-server ${cfg.group}"; + compress = true; + delaycompress = true; + copytruncate = true; }; systemd.tmpfiles.rules = [ diff --git a/nixos/modules/services/web-servers/apache-httpd/default.nix b/nixos/modules/services/web-servers/apache-httpd/default.nix index d817ff6019a3..3099705acbe2 100644 --- a/nixos/modules/services/web-servers/apache-httpd/default.nix +++ b/nixos/modules/services/web-servers/apache-httpd/default.nix @@ -710,20 +710,15 @@ in services.logrotate = optionalAttrs (cfg.logFormat != "none") { enable = mkDefault true; - paths.httpd = { - path = "${cfg.logDir}/*.log"; - user = cfg.user; - group = cfg.group; + settings.httpd = { + files = "${cfg.logDir}/*.log"; + su = "${cfg.user} ${cfg.group}"; frequency = "daily"; - keep = 28; - extraConfig = '' - sharedscripts - compress - delaycompress - postrotate - systemctl reload httpd.service > /dev/null 2>/dev/null || true - endscript - ''; + rotate = 28; + sharedscripts = true; + compress = true; + delaycompress = true; + postrotate = "systemctl reload httpd.service > /dev/null 2>/dev/null || true"; }; }; diff --git a/nixos/modules/services/web-servers/nginx/default.nix b/nixos/modules/services/web-servers/nginx/default.nix index e046c28dd6bb..1e18956c2dc4 100644 --- a/nixos/modules/services/web-servers/nginx/default.nix +++ b/nixos/modules/services/web-servers/nginx/default.nix @@ -989,17 +989,13 @@ in nginx.gid = config.ids.gids.nginx; }; - services.logrotate.paths.nginx = mapAttrs (_: mkDefault) { - path = "/var/log/nginx/*.log"; + services.logrotate.settings.nginx = mapAttrs (_: mkDefault) { + files = "/var/log/nginx/*.log"; frequency = "weekly"; - keep = 26; - extraConfig = '' - compress - delaycompress - postrotate - [ ! -f /var/run/nginx/nginx.pid ] || kill -USR1 `cat /var/run/nginx/nginx.pid` - endscript - ''; + rotate = 26; + compress = true; + delaycompress = true; + postrotate = "[ ! -f /var/run/nginx/nginx.pid ] || kill -USR1 `cat /var/run/nginx/nginx.pid`"; }; }; } diff --git a/nixos/modules/system/boot/systemd.nix b/nixos/modules/system/boot/systemd.nix index 297a80d4681b..f69c5d3d5a64 100644 --- a/nixos/modules/system/boot/systemd.nix +++ b/nixos/modules/system/boot/systemd.nix @@ -612,22 +612,18 @@ in boot.kernelParams = optional (!cfg.enableUnifiedCgroupHierarchy) "systemd.unified_cgroup_hierarchy=0"; - services.logrotate.paths = { + services.logrotate.settings = { "/var/log/btmp" = mapAttrs (_: mkDefault) { frequency = "monthly"; - keep = 1; - extraConfig = '' - create 0660 root ${config.users.groups.utmp.name} - minsize 1M - ''; + rotate = 1; + create = "0660 root ${config.users.groups.utmp.name}"; + minsize = "1M"; }; "/var/log/wtmp" = mapAttrs (_: mkDefault) { frequency = "monthly"; - keep = 1; - extraConfig = '' - create 0664 root ${config.users.groups.utmp.name} - minsize 1M - ''; + rotate = 1; + create = "0664 root ${config.users.groups.utmp.name}"; + minsize = "1M"; }; }; }; diff --git a/nixos/modules/virtualisation/azure-agent.nix b/nixos/modules/virtualisation/azure-agent.nix index bd8c7f8c1eea..e2425b44eac4 100644 --- a/nixos/modules/virtualisation/azure-agent.nix +++ b/nixos/modules/virtualisation/azure-agent.nix @@ -146,15 +146,11 @@ in services.logrotate = { enable = true; - extraConfig = '' - /var/log/waagent.log { - compress - monthly - rotate 6 - notifempty - missingok - } - ''; + settings."/var/log/waagent.log" = { + compress = true; + frequency = "monthly"; + rotate = 6; + }; }; systemd.targets.provisioned = { diff --git a/nixos/tests/logrotate.nix b/nixos/tests/logrotate.nix index 85923465593a..3087119c339f 100644 --- a/nixos/tests/logrotate.nix +++ b/nixos/tests/logrotate.nix @@ -1,5 +1,14 @@ # Test logrotate service works and is enabled by default +let + importTest = { ... }: { + services.logrotate.settings.import = { + olddir = false; + }; + }; + +in + import ./make-test-python.nix ({ pkgs, ... }: rec { name = "logrotate"; meta = with pkgs.lib.maintainers; { @@ -9,10 +18,59 @@ import ./make-test-python.nix ({ pkgs, ... }: rec { nodes = { defaultMachine = { ... }: { }; machine = { config, ... }: { - services.logrotate.paths = { + imports = [ importTest ]; + + services.logrotate.settings = { + # remove default frequency header and add another + header = { + frequency = null; + delaycompress = true; + }; + # extra global setting... affecting nothing + last_line = { + global = true; + priority = 2000; + shred = true; + }; # using mail somewhere should add --mail to logrotate invokation sendmail = { - extraConfig = "mail user@domain.tld"; + mail = "user@domain.tld"; + }; + # postrotate should be suffixed by 'endscript' + postrotate = { + postrotate = "touch /dev/null"; + }; + # multiple paths should be aggregated + multipath = { + files = [ "file1" "file2" ]; + }; + # overriding imported path should keep existing attributes + # (e.g. olddir is still set) + import = { + notifempty = true; + }; + }; + # extraConfig compatibility - should be added to top level, early. + services.logrotate.extraConfig = '' + nomail + ''; + # paths compatibility + services.logrotate.paths = { + compat_path = { + path = "compat_test_path"; + }; + # user/group should be grouped as 'su user group' + compat_user = { + user = config.users.users.root.name; + group = "root"; + }; + # extraConfig in path should be added to block + compat_extraConfig = { + extraConfig = "dateext"; + }; + # keep -> rotate + compat_keep = { + keep = 1; }; }; }; @@ -44,5 +102,23 @@ import ./make-test-python.nix ({ pkgs, ... }: rec { defaultMachine.fail("systemctl cat logrotate.service | grep -- --mail") with subtest("using mails adds mail option"): machine.succeed("systemctl cat logrotate.service | grep -- --mail") + with subtest("check generated config matches expectation"): + machine.succeed( + # copy conf to /tmp/logrotate.conf for easy grep + "conf=$(systemctl cat logrotate | grep -oE '/nix/store[^ ]*logrotate.conf'); cp $conf /tmp/logrotate.conf", + "! grep weekly /tmp/logrotate.conf", + "grep -E '^delaycompress' /tmp/logrotate.conf", + "tail -n 1 /tmp/logrotate.conf | grep shred", + "sed -ne '/\"sendmail\" {/,/}/p' /tmp/logrotate.conf | grep 'mail user@domain.tld'", + "sed -ne '/\"postrotate\" {/,/}/p' /tmp/logrotate.conf | grep endscript", + "grep '\"file1\"\n\"file2\" {' /tmp/logrotate.conf", + "sed -ne '/\"import\" {/,/}/p' /tmp/logrotate.conf | grep noolddir", + "sed -ne '1,/^\"/p' /tmp/logrotate.conf | grep nomail", + "grep '\"compat_test_path\" {' /tmp/logrotate.conf", + "sed -ne '/\"compat_user\" {/,/}/p' /tmp/logrotate.conf | grep 'su root root'", + "sed -ne '/\"compat_extraConfig\" {/,/}/p' /tmp/logrotate.conf | grep dateext", + "[[ $(sed -ne '/\"compat_keep\" {/,/}/p' /tmp/logrotate.conf | grep -w rotate) = \" rotate 1\" ]]", + "! sed -ne '/\"compat_keep\" {/,/}/p' /tmp/logrotate.conf | grep -w keep", + ) ''; }) From 45ef5c174113c9133250dab82d629d7ab5cdc01b Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Wed, 2 Mar 2022 22:12:45 +0900 Subject: [PATCH 5/7] logrotate: add configuration check at build time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- nixos/modules/services/logging/logrotate.nix | 60 ++++++++++++++++++-- nixos/tests/logrotate.nix | 8 +++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix index 6a9ed469fd38..c552d3b059c4 100644 --- a/nixos/modules/services/logging/logrotate.nix +++ b/nixos/modules/services/logging/logrotate.nix @@ -152,11 +152,45 @@ let { header = { global = true; priority = 100; }; } ] ))); - configFile = pkgs.writeText "logrotate.conf" ( - concatStringsSep "\n" ( + configFile = pkgs.writeTextFile { + name = "logrotate.conf"; + text = concatStringsSep "\n" ( map mkConf settings - ) - ); + ); + checkPhase = optionalString cfg.checkConfig '' + # logrotate --debug also checks that users specified in config + # file exist, but we only have sandboxed users here so brown these + # out. according to man page that means su, create and createolddir. + # files required to exist also won't be present, so missingok is forced. + user=$(${pkgs.coreutils}/bin/id -un) + group=$(${pkgs.coreutils}/bin/id -gn) + sed -e "s/\bsu\s.*/su $user $group/" \ + -e "s/\b\(create\s\+[0-9]*\s*\|createolddir\s\+[0-9]*\s\+\).*/\1$user $group/" \ + -e "1imissingok" -e "s/\bnomissingok\b//" \ + $out > /tmp/logrotate.conf + # Since this makes for very verbose builds only show real error. + # 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 + set -o pipefail + if ! ${pkgs.logrotate}/sbin/logrotate --debug /tmp/logrotate.conf 2>&1 \ + | ( ! grep "error:" ) > /tmp/logrotate-error; then + echo "Logrotate configuration check failed." + echo "The failing configuration (after adjustments to pass tests in sandbox) was:" + printf "%s\n" "-------" + cat /tmp/logrotate.conf + printf "%s\n" "-------" + echo "The error reported by logrotate was as follow:" + printf "%s\n" "-------" + cat /tmp/logrotate-error + printf "%s\n" "-------" + echo "You can disable this check with services.logrotate.checkConfig = false," + echo "but if you think it should work please report this failure along with" + echo "the config file being tested!" + false + fi + ''; + }; mailOption = if foldr (n: a: a || n ? mail) false (attrValues cfg.settings) @@ -256,6 +290,24 @@ in ''; }; + checkConfig = mkOption { + type = types.bool; + default = true; + description = '' + Whether the config should be checked at build time. + + Some options are not checkable at build time because of the build sandbox: + for example, the test does not know about existing files and system users are + not known. + These limitations mean we must adjust the file for tests (missingok is forced + and users are replaced by dummy users). + + Conversely there are still things that might make this check fail incorrectly + (e.g. a file path where we don't have access to intermediate directories): + in this case you can disable the failing check with this option. + ''; + }; + # deprecated legacy compat settings paths = mkOption { type = with types; attrsOf (submodule pathOpts); diff --git a/nixos/tests/logrotate.nix b/nixos/tests/logrotate.nix index 3087119c339f..31592f0a39c5 100644 --- a/nixos/tests/logrotate.nix +++ b/nixos/tests/logrotate.nix @@ -40,6 +40,14 @@ import ./make-test-python.nix ({ pkgs, ... }: rec { postrotate = { postrotate = "touch /dev/null"; }; + # check checkConfig works as expected: there is nothing to check here + # except that the file build passes + checkConf = { + su = "root utmp"; + createolddir = "0750 root utmp"; + create = "root utmp"; + "create " = "0750 root utmp"; + }; # multiple paths should be aggregated multipath = { files = [ "file1" "file2" ]; From b0a04e41052a7abf2c0538cd2f9c97bf9c86d911 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 27 Feb 2022 23:03:24 +0900 Subject: [PATCH 6/7] nginx/logrotate: run logrotate as nginx user --- nixos/modules/services/web-servers/nginx/default.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/nixos/modules/services/web-servers/nginx/default.nix b/nixos/modules/services/web-servers/nginx/default.nix index 1e18956c2dc4..7caaf5611cc0 100644 --- a/nixos/modules/services/web-servers/nginx/default.nix +++ b/nixos/modules/services/web-servers/nginx/default.nix @@ -992,6 +992,7 @@ in services.logrotate.settings.nginx = mapAttrs (_: mkDefault) { files = "/var/log/nginx/*.log"; frequency = "weekly"; + su = "${cfg.user} ${cfg.group}"; rotate = 26; compress = true; delaycompress = true; From 829c611b489f606c0b84fd315052681e8a03b083 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 5 Mar 2022 07:50:12 +0900 Subject: [PATCH 7/7] logrotate: add logrotate-checkconf.service 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 --- nixos/modules/services/logging/logrotate.nix | 14 +++++++++++++- nixos/tests/logrotate.nix | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix index c552d3b059c4..332a2a597edc 100644 --- a/nixos/modules/services/logging/logrotate.nix +++ b/nixos/modules/services/logging/logrotate.nix @@ -300,7 +300,10 @@ in for example, the test does not know about existing files and system users are not known. These limitations mean we must adjust the file for tests (missingok is forced - and users are replaced by dummy users). + and users are replaced by dummy users), so tests are complemented by a + logrotate-checkconf service that is enabled by default. + This extra check can be disabled by disabling it at the systemd level with the + option. Conversely there are still things that might make this check fail incorrectly (e.g. a file path where we don't have access to intermediate directories): @@ -387,5 +390,14 @@ in ExecStart = "${pkgs.logrotate}/sbin/logrotate ${mailOption} ${cfg.configFile}"; }; }; + systemd.services.logrotate-checkconf = { + description = "Logrotate configuration check"; + wantedBy = [ "multi-user.target" ]; + serviceConfig = { + Type = "oneshot"; + RemainAfterExit = true; + ExecStart = "${pkgs.logrotate}/sbin/logrotate --debug ${cfg.configFile}"; + }; + }; }; } diff --git a/nixos/tests/logrotate.nix b/nixos/tests/logrotate.nix index 31592f0a39c5..b0685f3af9ff 100644 --- a/nixos/tests/logrotate.nix +++ b/nixos/tests/logrotate.nix @@ -17,6 +17,12 @@ import ./make-test-python.nix ({ pkgs, ... }: rec { nodes = { defaultMachine = { ... }: { }; + failingMachine = { ... }: { + services.logrotate.configFile = pkgs.writeText "logrotate.conf" '' + # self-written config file + su notarealuser notagroupeither + ''; + }; machine = { config, ... }: { imports = [ importTest ]; @@ -128,5 +134,19 @@ import ./make-test-python.nix ({ pkgs, ... }: rec { "[[ $(sed -ne '/\"compat_keep\" {/,/}/p' /tmp/logrotate.conf | grep -w rotate) = \" rotate 1\" ]]", "! sed -ne '/\"compat_keep\" {/,/}/p' /tmp/logrotate.conf | grep -w keep", ) + # also check configFile option + failingMachine.succeed( + "conf=$(systemctl cat logrotate | grep -oE '/nix/store[^ ]*logrotate.conf'); cp $conf /tmp/logrotate.conf", + "grep 'self-written config' /tmp/logrotate.conf", + ) + with subtest("Check logrotate-checkconf service"): + machine.wait_for_unit("logrotate-checkconf.service") + # wait_for_unit also asserts for success, so wait for + # parent target instead and check manually. + failingMachine.wait_for_unit("multi-user.target") + info = failingMachine.get_unit_info("logrotate-checkconf.service") + if info["ActiveState"] != "failed": + raise Exception('logrotate-checkconf.service was not failed') + ''; })