nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch with reload enabled (#76179)

nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch
with reload enabled

Closes https://github.com/NixOS/nixpkgs/issues/73455

2 files changed, 32 insertions, 5 deletions

diff --git a/nixos/modules/services/web-servers/nginx/default.nix b/nixos/modules/services/web-servers/nginx/default.nix
index ada7a25604c40..60a5b503def9c 100644
--- a/nixos/modules/services/web-servers/nginx/default.nix
+++ b/nixos/modules/services/web-servers/nginx/default.nix
@@ -178,6 +178,8 @@ let
     then "/etc/nginx/nginx.conf"
     else configFile;
 
+  execCommand = "${cfg.package}/bin/nginx -c '${configPath}' -p '${cfg.stateDir}'";
+
   vhosts = concatStringsSep "\n" (mapAttrsToList (vhostName: vhost:
     let
         onlySSL = vhost.onlySSL || vhost.enableSSL;
@@ -682,10 +684,10 @@ in
       stopIfChanged = false;
       preStart = ''
         ${cfg.preStart}
-        ${cfg.package}/bin/nginx -c '${configPath}' -p '${cfg.stateDir}' -t
+        ${execCommand} -t
       '';
       serviceConfig = {
-        ExecStart = "${cfg.package}/bin/nginx -c '${configPath}' -p '${cfg.stateDir}'";
+        ExecStart = execCommand;
         ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
         Restart = "always";
         RestartSec = "10s";
@@ -706,11 +708,18 @@ in
     };
 
     systemd.services.nginx-config-reload = mkIf cfg.enableReload {
-      wantedBy = [ "nginx.service" ];
+      wants = [ "nginx.service" ];
+      wantedBy = [ "multi-user.target" ];
       restartTriggers = [ configFile ];
+      # commented, because can cause extra delays during activate for this config:
+      #      services.nginx.virtualHosts."_".locations."/".proxyPass = "http://blabla:3000";
+      # stopIfChanged = false;
+      serviceConfig.Type = "oneshot";
+      serviceConfig.TimeoutSec = 60;
       script = ''
         if ${pkgs.systemd}/bin/systemctl -q is-active nginx.service ; then
-          ${pkgs.systemd}/bin/systemctl reload nginx.service
+          ${execCommand} -t && \
+            ${pkgs.systemd}/bin/systemctl reload nginx.service
         fi
       '';
       serviceConfig.RemainAfterExit = true;
diff --git a/nixos/tests/nginx.nix b/nixos/tests/nginx.nix
index 55d2c9309084a..7358800a67633 100644
--- a/nixos/tests/nginx.nix
+++ b/nixos/tests/nginx.nix
@@ -7,7 +7,7 @@
 import ./make-test-python.nix ({ pkgs, ... }: {
   name = "nginx";
   meta = with pkgs.stdenv.lib.maintainers; {
-    maintainers = [ mbbx6spp ];
+    maintainers = [ mbbx6spp danbst ];
   };
 
   nodes = {
@@ -59,6 +59,11 @@ import ./make-test-python.nix ({ pkgs, ... }: {
         {
           services.nginx.package = pkgs.nginxUnstable;
         }
+
+        {
+          services.nginx.package = pkgs.nginxUnstable;
+          services.nginx.virtualHosts."!@$$(#*%".locations."~@#*$*!)".proxyPass = ";;;";
+        }
       ];
     };
 
@@ -68,6 +73,7 @@ import ./make-test-python.nix ({ pkgs, ... }: {
     etagSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-1";
     justReloadSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-2";
     reloadRestartSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-3";
+    reloadWithErrorsSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-4";
   in ''
     url = "http://localhost/index.html"
 
@@ -110,5 +116,17 @@ import ./make-test-python.nix ({ pkgs, ... }: {
         )
         webserver.wait_for_unit("nginx")
         webserver.succeed("journalctl -u nginx | grep -q -i stopped")
+
+    with subtest("nixos-rebuild --switch should fail when there are configuration errors"):
+        webserver.fail(
+            "${reloadWithErrorsSystem}/bin/switch-to-configuration test >&2"
+        )
+        webserver.succeed("[[ $(systemctl is-failed nginx-config-reload) == failed ]]")
+        webserver.succeed("[[ $(systemctl is-failed nginx) == active ]]")
+        # just to make sure operation is idempotent. During development I had a situation
+        # when first time it shows error, but stops showing it on subsequent rebuilds
+        webserver.fail(
+            "${reloadWithErrorsSystem}/bin/switch-to-configuration test >&2"
+        )
   '';
 })