nginx: add --nginx-sleep-seconds (#8163)

* nginx: add --nginx-sleep-seconds

As described in #7422, reloading nginx is an asynchronous process and
Certbot does not know when it is complete. In an environment where this
reload takes a long time, the nginx plugin suffers from an issue where
it responds to and fails the ACME challenge before the nginx server is
ready to serve it.

Following the discussion in a previous PR #7740, this commit introduces
a new flag, --nginx-sleep-seconds, which may be used to increase the
duration that Certbot will wait for nginx to reload, from its previously
hard-coded value of 1s.

Fixes #7422

* update CHANGELOG

* nginx: update docstring for nginx_restart

certbot-nginx/certbot_nginx/_internal/configurator.py

@@ -77,6 +77,9 @@ class NginxConfigurator(common.Installer):
         add("ctl", default=constants.CLI_DEFAULTS["ctl"], help="Path to the "
             "'nginx' binary, used for 'configtest' and retrieving nginx "
             "version number.")
+        add("sleep-seconds", default=constants.CLI_DEFAULTS["sleep_seconds"], type=int,
+            help="Number of seconds to wait for nginx configuration changes "
+            "to apply when reloading.")
 
     @property
     def nginx_conf(self):
@@ -912,7 +915,7 @@ class NginxConfigurator(common.Installer):
         :raises .errors.MisconfigurationError: If either the reload fails.
 
         """
-        nginx_restart(self.conf('ctl'), self.nginx_conf)
+        nginx_restart(self.conf('ctl'), self.nginx_conf, self.conf('sleep-seconds'))
 
     def config_test(self):
         """Check the configuration of Nginx for errors.
@@ -1159,7 +1162,7 @@ def _redirect_block_for_domain(domain):
     return redirect_block
 
 
-def nginx_restart(nginx_ctl, nginx_conf):
+def nginx_restart(nginx_ctl, nginx_conf, sleep_duration):
     """Restarts the Nginx Server.
 
     .. todo:: Nginx restart is fatal if the configuration references
@@ -1167,6 +1170,8 @@ def nginx_restart(nginx_ctl, nginx_conf):
         before restart.
 
     :param str nginx_ctl: Path to the Nginx binary.
+    :param str nginx_conf: Path to the Nginx configuration file.
+    :param int sleep_duration: How long to sleep after sending the reload signal.
 
     """
     try:
@@ -1190,10 +1195,12 @@ def nginx_restart(nginx_ctl, nginx_conf):
 
     except (OSError, ValueError):
         raise errors.MisconfigurationError("nginx restart failed")
-    # Nginx can take a moment to recognize a newly added TLS SNI servername, so sleep
-    # for a second. TODO: Check for expected servername and loop until it
-    # appears or return an error if looping too long.
-    time.sleep(1)
+    # Nginx can take a significant duration of time to fully apply a new config, depending
+    # on size and contents (https://github.com/certbot/certbot/issues/7422). Lacking a way
+    # to reliably identify when this process is complete, we provide the user with control
+    # over how long Certbot will sleep after reloading the configuration.
+    if sleep_duration > 0:
+        time.sleep(sleep_duration)
 
 
 def _determine_default_server_root():

certbot-nginx/certbot_nginx/_internal/constants.py

@@ -1,6 +1,9 @@
 """nginx plugin constants."""
 import platform
 
+from acme.magic_typing import Any
+from acme.magic_typing import Dict
+
 FREEBSD_DARWIN_SERVER_ROOT = "/usr/local/etc/nginx"
 LINUX_SERVER_ROOT = "/etc/nginx"
 PKGSRC_SERVER_ROOT = "/usr/pkg/etc/nginx"
@@ -15,7 +18,8 @@ else:
 CLI_DEFAULTS = dict(
     server_root=server_root_tmp,
     ctl="nginx",
-)
+    sleep_seconds=1
+) # type: Dict[str, Any]
 """CLI defaults."""
 
 

certbot-nginx/tests/configurator_test.py

@@ -460,11 +460,13 @@ class NginxConfiguratorTest(util.NginxTest):
         self.assertEqual(self.config._get_openssl_version(), "")
 
     @mock.patch("certbot_nginx._internal.configurator.subprocess.Popen")
-    def test_nginx_restart(self, mock_popen):
+    @mock.patch("certbot_nginx._internal.configurator.time")
+    def test_nginx_restart(self, mock_time, mock_popen):
         mocked = mock_popen()
         mocked.communicate.return_value = ('', '')
         mocked.returncode = 0
         self.config.restart()
+        mock_time.sleep.assert_called_once_with(0.1234)
 
     @mock.patch("certbot_nginx._internal.configurator.subprocess.Popen")
     def test_nginx_restart_fail(self, mock_popen):

certbot-nginx/tests/test_util.py

@@ -56,6 +56,7 @@ class NginxTest(test_log_util.AssertLogsMixin, test_util.ConfigTestCase):
         backups = os.path.join(work_dir, "backups")
 
         self.configuration.nginx_server_root = config_path
+        self.configuration.nginx_sleep_seconds = 0.1234
         self.configuration.le_vhost_ext = "-le-ssl.conf"
         self.configuration.config_dir = config_dir
         self.configuration.work_dir = work_dir

certbot/CHANGELOG.md

@@ -9,6 +9,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Third-party plugins can be used without prefix (`plugin_name` instead of `dist_name:plugin_name`):
   this concerns the plugin name, CLI flags, and keys in credential files.
   The prefixed form is still supported but is deprecated, and will be removed in a future release.
+* Added `--nginx-sleep-seconds` (default `1`) for environments where nginx takes a long time to reload.
 
 ### Changed