nixos/nginx: allow adding new ACME certificates without nginx restart (#445544)

nixos/modules/services/web-servers/nginx/default.nix

@@ -1491,17 +1491,22 @@ in
       };
     };
 
-    systemd.services.nginx = {
+    systemd.services = {
+      nginx = {
         description = "Nginx Web Server";
         wantedBy = [ "multi-user.target" ];
-      wants = concatLists (map (certName: [ "acme-${certName}.service" ]) vhostCertNames);
+        wants = lib.optionals (!cfg.enableReload) (
+          concatLists (map (certName: [ "acme-${certName}.service" ]) vhostCertNames)
+        );
         after = [
           "network.target"
         ]
         # Ensure nginx runs with baseline certificates in place.
-      ++ map (certName: "acme-${certName}.service") vhostCertNames;
+        ++ lib.optionals (!cfg.enableReload) (map (certName: "acme-${certName}.service") vhostCertNames);
         # Ensure nginx runs (with current config) before the actual ACME jobs run
-      before = map (certName: "acme-order-renew-${certName}.service") vhostCertNames;
+        before = lib.optionals (!cfg.enableReload) (
+          map (certName: "acme-order-renew-${certName}.service") vhostCertNames
+        );
         stopIfChanged = false;
         preStart = ''
           ${cfg.preStart}
@@ -1589,26 +1594,25 @@ in
         };
       };
 
-    environment.etc."nginx/nginx.conf" = mkIf cfg.enableReload {
-      source = configFile;
-    };
-
       # This service waits for all certificates to be available
       # before reloading nginx configuration.
       # sslTargets are added to wantedBy + before
       # which allows the acme-order-renew-$cert.service to signify the successful updating
       # of certs end-to-end.
-    systemd.services.nginx-config-reload =
+      nginx-config-reload =
         let
+          sslServices = map (certName: "acme-${certName}.service") vhostCertNames;
           sslOrderRenewServices = map (certName: "acme-order-renew-${certName}.service") vhostCertNames;
         in
         mkIf (cfg.enableReload || vhostCertNames != [ ]) {
           wants = optionals cfg.enableReload [ "nginx.service" ];
-        wantedBy = sslOrderRenewServices ++ [ "multi-user.target" ];
-        # XXX Before the finished targets, after the renew services.
-        # This service might be needed for HTTP-01 challenges, but we only want to confirm
-        # certs are updated _after_ config has been reloaded.
-        after = sslOrderRenewServices;
+          # Reload config directly after the self-signed certificates have been requested
+          # This is required for HTTP-01 ACME challenges, as the vHost with `.well-known/acme-challenge`
+          # must already exist. Another reload with the actual certificate is triggered
+          # with `security.acme.certs.<...>.reloadServices`
+          wantedBy = [ "multi-user.target" ] ++ optionals cfg.enableReload sslServices;
+          after = optionals cfg.enableReload sslServices;
+          before = optionals cfg.enableReload sslOrderRenewServices;
           restartTriggers = optionals cfg.enableReload [ configFile ];
           # Block reloading if not all certs exist yet.
           # Happens when config changes add new vhosts/certs.
@@ -1629,15 +1633,43 @@ in
             ExecStart = "/run/current-system/systemd/bin/systemctl reload nginx.service";
           };
         };
+    }
+    # When reload is enabled, add the systemd dependency to the acme unit to prevent restarts
+    # of the nginx.service unit.
+    # This needs to be here, because of how switch-to-configuration works in the case that nginx.service
+    # is not started at the moment where new certificates are requested.
+    # Configuring this relationship in nginx.service would lead to s-t-c restarting nginx.service
+    # when a new certificate is added, as it will restart a unit when their direct unit properties,
+    # including After and Wants, change.
+    // lib.optionalAttrs cfg.enableReload (
+      lib.listToAttrs (
+        map (
+          name:
+          lib.nameValuePair "acme-${name}" {
+            before = [ "nginx.service" ];
+            wantedBy = [ "nginx.service" ];
+          }
+        ) vhostCertNames
+      )
+    );
+
+    environment.etc."nginx/nginx.conf" = mkIf cfg.enableReload {
+      source = configFile;
+    };
 
     security.acme.certs =
       let
-        acmePairs = map (
+        # Here are two cases:
+        # - when no `useACMEHost` is set, the `serverName` acme certificate is the primary name and we need to configure it
+        # - when `useACMEHost` is set, this is also the primary name and we only need to configure the reloadServices property
+        acmePairs =
+          map (
             vhostConfig:
             let
               hasRoot = vhostConfig.acmeRoot != null;
             in
             nameValuePair vhostConfig.serverName {
+              reloadServices = [ "nginx.service" ];
               group = mkDefault cfg.group;
               # if acmeRoot is null inherit config.security.acme
               # Since config.security.acme.certs.<cert>.webroot's own default value
@@ -1648,7 +1680,13 @@ in
               extraDomainNames = vhostConfig.serverAliases;
               # Filter for enableACME-only vhosts. Don't want to create dud certs
             }
-        ) (filter (vhostConfig: vhostConfig.useACMEHost == null) acmeEnabledVhosts);
+          ) (filter (vhostConfig: vhostConfig.useACMEHost == null) acmeEnabledVhosts)
+          ++ map (
+            vhostConfig:
+            nameValuePair vhostConfig.useACMEHost {
+              reloadServices = [ "nginx.service" ];
+            }
+          ) (filter (vhostConfig: vhostConfig.useACMEHost != null) acmeEnabledVhosts);
       in
       listToAttrs acmePairs;
 

nixos/tests/acme/default.nix

@@ -1,20 +1,9 @@
-{ runTest }:
+{ lib, runTest }:
 let
   domain = "example.test";
-in
-{
-  http01-builtin = runTest ./http01-builtin.nix;
-  dns01 = runTest ./dns01.nix;
-  caddy = runTest ./caddy.nix;
-  nginx = runTest (
-    import ./webserver.nix {
-      inherit domain;
-      serverName = "nginx";
-      group = "nginx";
-      baseModule = {
+  nginxBaseModule = {
     services.nginx = {
       enable = true;
-          enableReload = true;
       logError = "stderr info";
       # This tests a number of things at once:
       #   - Self-signed certs are in place before the webserver startup
@@ -32,6 +21,29 @@ in
       services.nginx.virtualHosts."nullroot.${domain}".acmeFallbackHost = "localhost:8081";
     };
   };
+in
+{
+  http01-builtin = runTest ./http01-builtin.nix;
+  dns01 = runTest ./dns01.nix;
+  caddy = runTest ./caddy.nix;
+  nginx = runTest (
+    import ./webserver.nix {
+      inherit domain;
+      serverName = "nginx";
+      group = "nginx";
+      baseModule = lib.recursiveUpdate nginxBaseModule {
+        services.nginx.enableReload = true;
+      };
+    }
+  );
+  nginx-without-reload = runTest (
+    import ./webserver.nix {
+      inherit domain;
+      serverName = "nginx";
+      group = "nginx";
+      baseModule = lib.recursiveUpdate nginxBaseModule {
+        services.nginx.enableReload = false;
+      };
     }
   );
   httpd = runTest (

nixos/tests/acme/webserver.nix

@@ -226,5 +226,18 @@
           # Ensure the timer works, due to our shenanigans with
           # RemainAfterExit=true
           webserver.wait_until_succeeds(f"journalctl --cursor-file=/tmp/cursor | grep 'Starting Order (and renew) ACME certificate for zeroconf3.{domain}...'")
+    ''
+    +
+      lib.optionalString
+        (config.nodes.webserver.services.nginx.enable && config.nodes.webserver.services.nginx.enableReload)
+        ''
+          with subtest("Ensure that adding a second vhost does not restart nginx"):
+              switch_to(webserver, "zeroconf")
+              webserver.wait_for_unit("renew-triggered.target")
+              webserver.succeed("journalctl --cursor-file=/tmp/cursor")
+              switch_to(webserver, "zeroconf3")
+              webserver.wait_for_unit("renew-triggered.target")
+              output = webserver.succeed("journalctl --cursor-file=/tmp/cursor")
+              t.assertNotIn("Stopping Nginx Web Server...", output)
         '';
 }

nixos/tests/all-tests.nix

@@ -183,7 +183,10 @@ in
   # keep-sorted start case=no numeric=no block=yes
   _3proxy = runTest ./3proxy.nix;
   aaaaxy = runTest ./aaaaxy.nix;
-  acme = import ./acme/default.nix { inherit runTest; };
+  acme = import ./acme/default.nix {
+    inherit runTest;
+    inherit (pkgs) lib;
+  };
   acme-dns = runTest ./acme-dns.nix;
   activation = pkgs.callPackage ../modules/system/activation/test.nix { };
   activation-bashless = runTest ./activation/bashless.nix;

pkgs/servers/http/nginx/generic.nix

@@ -298,6 +298,7 @@ stdenv.mkDerivation {
         ;
       variants = lib.recurseIntoAttrs nixosTests.nginx-variants;
       acme-integration = nixosTests.acme.nginx;
+      acme-integration-without-reload = nixosTests.acme.nginx-without-reload;
     }
     // passthru.tests;
   };