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

This option works for me.

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.

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?

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.

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.

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.

I did the following patch:

From f694fc3d1003d1343dcb32e10dcb9e7a3eddf700 Mon Sep 17 00:00:00 2001
From: Florian Klink <flokli@flokli.de>
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.

Feel to make a PR.