nixos/nginx: run config test as nginx user

[Mic92]

Otherwise this changes ownership of /var/cache/nginx/ to nobody
which makes the normal execution fail.

Motivation for this change

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.

[flokli]

Can we move the check command into an additional ExecReload line of the main unit (which should already run as the right user)?

[Izorkin]

This option works for me.

[Mic92]

Can we move the check command into an additional ExecReload line of the main unit (which should already run as the right user)?

No because than I cannot add it nginx to the keys group and still have the needed privileges. This solution is simpler.

[flokli]

No because than I cannot add it nginx to the keys group and still have the needed privileges. This solution is simpler.

I don't understand. The configuration check would need to run with the same permissions, groups etc. as the main unit, so having reload logic inside the main unit should be exactly what we need, shouldn't it?

[Mic92]

No because than I cannot add it nginx to the keys group and still have the needed privileges. This solution is simpler.

I don't understand. The configuration check would need to run with the same permissions, groups etc. as the main unit, so having reload logic inside the main unit should be exactly what we need, shouldn't it?

There is already nginx -t in the main unit. However the purpose of having it in here is to have better error messages when the reload failed.

[flokli]

No because than I cannot add it nginx to the keys group and still have the needed privileges. This solution is simpler.

I don't understand. The configuration check would need to run with the same permissions, groups etc. as the main unit, so having reload logic inside the main unit should be exactly what we need, shouldn't it?

There is already nginx -t in the main unit. However the purpose of having it in here is to have better error messages when the reload failed.

Not for the reload command, only the start command. I'd assume if we add it to the reload commands, we'd get the same (better) error messages.

[Mic92]

No because than I cannot add it nginx to the keys group and still have the needed privileges. This solution is simpler.

I don't understand. The configuration check would need to run with the same permissions, groups etc. as the main unit, so having reload logic inside the main unit should be exactly what we need, shouldn't it?

There is already nginx -t in the main unit. However the purpose of having it in here is to have better error messages when the reload failed.

Not for the reload command, only the start command. I'd assume if we add it to the reload commands, we'd get the same (better) error messages.

I don't think the error is shown because the nginx.service is not reloaded from the activation script point of view.

[flokli]

I did the following patch:

From f694fc3d1003d1343dcb32e10dcb9e7a3eddf700 Mon Sep 17 00:00:00 2001
From: Florian Klink <[email protected]>
Date: Wed, 12 Aug 2020 15:37:20 +0200
Subject: [PATCH] nixos/nginx: move configuration testing script into reload
 command

We want to check the configuration also when running
`systemctl reload nginx`, not only when restarting the dummy
`nginx-config-reload` unit (which is mostly a workaround for missing
features in our activation script anyways).
---
 nixos/modules/services/web-servers/nginx/default.nix | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/nixos/modules/services/web-servers/nginx/default.nix b/nixos/modules/services/web-servers/nginx/default.nix
index 6e0d60a110d..461888c4cc4 100644
--- a/nixos/modules/services/web-servers/nginx/default.nix
+++ b/nixos/modules/services/web-servers/nginx/default.nix
@@ -704,7 +704,10 @@ in
       '';
       serviceConfig = {
         ExecStart = execCommand;
-        ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
+        ExecReload = [
+          "${execCommand} -t"
+          "${pkgs.coreutils}/bin/kill -HUP $MAINPID"
+        ];
         Restart = "always";
         RestartSec = "10s";
         StartLimitInterval = "1min";
@@ -761,8 +764,7 @@ in
       serviceConfig.TimeoutSec = 60;
       script = ''
         if /run/current-system/systemd/bin/systemctl -q is-active nginx.service ; then
-          ${execCommand} -g "user ${cfg.user} ${cfg.group};" -t && \
-            /run/current-system/systemd/bin/systemctl reload nginx.service
+          /run/current-system/systemd/bin/systemctl reload nginx.service
         fi
       '';
       serviceConfig.RemainAfterExit = true;
-- 
2.28.0

And re-ran the nginx VM test, which also tries to switch to a configuration with a broken nginx config. The failing ExecReload command propagates to the systemctl reload, which propagates to the nginx-config-reload unit, which I think would be preferrable.

[Mic92]

Feel to make a PR.