From 09ab4aea01aaf95a2a830ad48271aa6bd11eef84 Mon Sep 17 00:00:00 2001 From: alexzorin Date: Tue, 28 Jul 2020 05:52:12 +1000 Subject: [PATCH] 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/_internal/configurator.py | 19 +++++++++++++------ .../certbot_nginx/_internal/constants.py | 6 +++++- certbot-nginx/tests/configurator_test.py | 4 +++- certbot-nginx/tests/test_util.py | 1 + certbot/CHANGELOG.md | 1 + 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py index a70572c33..3da38ff2d 100644 --- a/certbot-nginx/certbot_nginx/_internal/configurator.py +++ b/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(): diff --git a/certbot-nginx/certbot_nginx/_internal/constants.py b/certbot-nginx/certbot_nginx/_internal/constants.py index c10eb5098..1f058e7ef 100644 --- a/certbot-nginx/certbot_nginx/_internal/constants.py +++ b/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.""" diff --git a/certbot-nginx/tests/configurator_test.py b/certbot-nginx/tests/configurator_test.py index 2c3264a5f..0a5adcd3f 100644 --- a/certbot-nginx/tests/configurator_test.py +++ b/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): diff --git a/certbot-nginx/tests/test_util.py b/certbot-nginx/tests/test_util.py index 4b26f7935..f545dc5bc 100644 --- a/certbot-nginx/tests/test_util.py +++ b/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 diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 4eca065af..92e89e540 100644 --- a/certbot/CHANGELOG.md +++ b/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