From e40735288489b63f3af41244f7e3f06472179679 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Thu, 28 Jan 2021 21:38:37 +1100 Subject: [PATCH 1/6] renew: don't restart the installer for dry-runs Prevents Certbot from superfluously invoking the installer restart during dry-run renewals. (This does not affect authenticator restarts). Additionally removes some CLI output that was reporting the fullchain path of the renewed certificate. --- certbot/certbot/_internal/main.py | 11 +---------- certbot/tests/main_test.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index b9b6b16f6..262c6e861 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -1233,19 +1233,10 @@ def renew_cert(config, plugins, lineage): renewed_lineage = _get_and_save_cert(le_client, config, lineage=lineage) - notify = zope.component.getUtility(interfaces.IDisplay).notification - if installer is None: - notify("new certificate deployed without reload, fullchain is {0}".format( - lineage.fullchain), pause=False) - else: + if installer and not config.dry_run: # In case of a renewal, reload server to pick up new certificate. - # In principle we could have a configuration option to inhibit this - # from happening. - # Run deployer updater.run_renewal_deployer(config, renewed_lineage, installer) installer.restart() - notify("new certificate deployed with reload of {0} server; fullchain is {1}".format( - config.installer, lineage.fullchain), pause=False) def certonly(config, plugins): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 5471248b4..aaab5e313 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1459,6 +1459,24 @@ class MainTest(test_util.ConfigTestCase): # without installer self.assertFalse(mock_run.called) + @mock.patch('certbot._internal.main.updater.run_renewal_deployer') + @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins') + @mock.patch('certbot._internal.main._init_le_client') + @mock.patch('certbot._internal.main._get_and_save_cert') + def test_renew_doesnt_restart_on_dryrun(self, mock_get_cert, mock_init, mock_choose, + mock_run_renewal_deployer): + """A dry-run renewal shouldn't try to restart the installer""" + self.config.dry_run = True + installer = mock.MagicMock() + mock_choose.return_value = (installer, mock.MagicMock()) + + main.renew_cert(self.config, None, None) + + self.assertEqual(mock_init.call_count, 1) + self.assertEqual(mock_get_cert.call_count, 1) + installer.restart.assert_not_called() + mock_run_renewal_deployer.assert_not_called() + class UnregisterTest(unittest.TestCase): def setUp(self): From 85f284d40c1b2f7c47c6634d3662ae70963283da Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Thu, 28 Jan 2021 21:54:57 +1100 Subject: [PATCH 2/6] update CHANGELOG.md --- certbot/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 0ffed42d1..10dda7d9d 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -21,6 +21,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * Fixed the apache component on openSUSE Tumbleweed which no longer provides an apache2ctl symlink and uses apachectl instead. * Fixed a typo in `certbot/crypto_util.py` causing an error upon attempting `secp521r1` key generation +* Installers (e.g. nginx, Apache) were being restarted unnecessarily after dry-run renewals. More details about these changes can be found on our GitHub repo. From 74fd4fe794571da018e989ee563079ec83ccafb5 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Mon, 15 Feb 2021 17:54:43 +1100 Subject: [PATCH 3/6] print message when restarting server after renewal --- certbot/certbot/_internal/main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 262c6e861..1f6efc387 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -1236,6 +1236,7 @@ def renew_cert(config, plugins, lineage): if installer and not config.dry_run: # In case of a renewal, reload server to pick up new certificate. updater.run_renewal_deployer(config, renewed_lineage, installer) + display_util.notify(f"Restarting {config.installer} server after certificate renewal") installer.restart() From 936644cde5c769ff7ee5524c69ecbf6feb2de6f8 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Mon, 22 Feb 2021 10:23:51 +1100 Subject: [PATCH 4/6] fix docstring --- certbot/certbot/_internal/plugins/selection.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/certbot/certbot/_internal/plugins/selection.py b/certbot/certbot/_internal/plugins/selection.py index 0b04791c6..b4ece7b15 100644 --- a/certbot/certbot/_internal/plugins/selection.py +++ b/certbot/certbot/_internal/plugins/selection.py @@ -3,11 +3,13 @@ from __future__ import print_function import logging +from typing import Optional, Tuple import six import zope.component from certbot import errors from certbot import interfaces +from certbot._internal.plugins import disco from certbot.compat import os from certbot.display import util as display_util @@ -175,7 +177,9 @@ def record_chosen_plugins(config, plugins, auth, inst): config.authenticator, config.installer) -def choose_configurator_plugins(config, plugins, verb): +def choose_configurator_plugins(config: interfaces.IConfig, plugins: disco.PluginsRegistry, + verb: str) -> Tuple[Optional[interfaces.IInstaller], + Optional[interfaces.IAuthenticator]]: """ Figure out which configurator we're going to use, modifies config.authenticator and config.installer strings to reflect that choice if @@ -183,7 +187,7 @@ def choose_configurator_plugins(config, plugins, verb): :raises errors.PluginSelectionError if there was a problem - :returns: (an `IAuthenticator` or None, an `IInstaller` or None) + :returns: tuple of (`IInstaller` or None, `IAuthenticator` or None) :rtype: tuple """ From 1a79716583e8315485893eb8546d86b43a7a14c8 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Mon, 22 Feb 2021 10:30:32 +1100 Subject: [PATCH 5/6] s/Restarting/Reloading/ when notifying the user --- certbot/certbot/_internal/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 1f6efc387..b940c120b 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -1236,7 +1236,7 @@ def renew_cert(config, plugins, lineage): if installer and not config.dry_run: # In case of a renewal, reload server to pick up new certificate. updater.run_renewal_deployer(config, renewed_lineage, installer) - display_util.notify(f"Restarting {config.installer} server after certificate renewal") + display_util.notify(f"Reloading {config.installer} server after certificate renewal") installer.restart() From d2254e1eba8e3b779f30e5a7ba3f894d6e4884c1 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Tue, 18 May 2021 14:12:12 +1000 Subject: [PATCH 6/6] ignore type error --- certbot/certbot/_internal/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 898ef7df0..faccc68a5 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -1246,7 +1246,7 @@ def renew_cert(config, plugins, lineage): # In case of a renewal, reload server to pick up new certificate. updater.run_renewal_deployer(config, renewed_lineage, installer) display_util.notify(f"Reloading {config.installer} server after certificate renewal") - installer.restart() + installer.restart() # type: ignore def certonly(config, plugins):