From 9f6b147d6f06e534351377195b4f28c61270115d Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sat, 26 May 2018 18:31:23 +0300 Subject: [PATCH] Do not call updaters and deployers when run with --dry-run (#6038) When Certbot is run with --dry-run, skip running GenericUpdater and RenewDeployer interface methods. This PR also makes the parameter order of updater.run_generic_updaters and updater.run_renewal_deployer consistent. Fixes #5927 * Do not call updaters and deployers when run with --dry-run * Use ConfigTestCase instead of mocking config objects manually --- certbot/main.py | 2 +- certbot/renewal.py | 4 ++-- certbot/tests/main_test.py | 3 ++- certbot/tests/renewupdater_test.py | 35 ++++++++++++++++++------------ certbot/updater.py | 20 ++++++++++++----- 5 files changed, 41 insertions(+), 23 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index dad1b793e..8fa6ebfc6 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -1163,7 +1163,7 @@ def renew_cert(config, plugins, lineage): # 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. - updater.run_renewal_deployer(renewed_lineage, installer, config) + 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) diff --git a/certbot/renewal.py b/certbot/renewal.py index 0a6568426..236330a85 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -431,8 +431,8 @@ def handle_renewal_request(config): renew_skipped.append("%s expires on %s" % (renewal_candidate.fullchain, expiry.strftime("%Y-%m-%d"))) # Run updater interface methods - updater.run_generic_updaters(lineage_config, plugins, - renewal_candidate) + updater.run_generic_updaters(lineage_config, renewal_candidate, + plugins) except Exception as e: # pylint: disable=broad-except # obtain_cert (presumably) encountered an unanticipated problem. diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 8c9e24354..36821cf53 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1464,7 +1464,8 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met None, None, None) with mock.patch('certbot.updater.logger.warning') as mock_log: - updater.run_generic_updaters(None, None, None) + self.config.dry_run = False + updater.run_generic_updaters(self.config, None, None) self.assertTrue(mock_log.called) self.assertTrue("Could not choose appropriate plugin for updaters" in mock_log.call_args[0][0]) diff --git a/certbot/tests/renewupdater_test.py b/certbot/tests/renewupdater_test.py index ade8db390..bd1cd891e 100644 --- a/certbot/tests/renewupdater_test.py +++ b/certbot/tests/renewupdater_test.py @@ -9,10 +9,11 @@ from certbot import updater import certbot.tests.util as test_util -class RenewUpdaterTest(unittest.TestCase): +class RenewUpdaterTest(test_util.ConfigTestCase): """Tests for interfaces.RenewDeployer and interfaces.GenericUpdater""" def setUp(self): + super(RenewUpdaterTest, self).setUp() class MockInstallerGenericUpdater(interfaces.GenericUpdater): """Mock class that implements GenericUpdater""" def __init__(self, *args, **kwargs): @@ -33,41 +34,47 @@ class RenewUpdaterTest(unittest.TestCase): self.generic_updater = MockInstallerGenericUpdater() self.renew_deployer = MockInstallerRenewDeployer() - def get_config(self, args): - """Get mock config from dict of parameters""" - config = mock.MagicMock() - for key in args.keys(): - config.__dict__[key] = args[key] - return config - @mock.patch('certbot.main._get_and_save_cert') @mock.patch('certbot.plugins.selection.choose_configurator_plugins') @test_util.patch_get_utility() def test_server_updates(self, _, mock_select, mock_getsave): - config = self.get_config({"disable_renew_updates": False}) - mock_getsave.return_value = mock.MagicMock() mock_generic_updater = self.generic_updater # Generic Updater mock_select.return_value = (mock_generic_updater, None) with mock.patch('certbot.main._init_le_client'): - main.renew_cert(config, None, mock.MagicMock()) + main.renew_cert(self.config, None, mock.MagicMock()) self.assertTrue(mock_generic_updater.restart.called) mock_generic_updater.restart.reset_mock() mock_generic_updater.callcounter.reset_mock() - updater.run_generic_updaters(config, None, mock.MagicMock()) + updater.run_generic_updaters(self.config, mock.MagicMock(), None) self.assertEqual(mock_generic_updater.callcounter.call_count, 1) self.assertFalse(mock_generic_updater.restart.called) def test_renew_deployer(self): - config = self.get_config({"disable_renew_updates": False}) lineage = mock.MagicMock() mock_deployer = self.renew_deployer - updater.run_renewal_deployer(lineage, mock_deployer, config) + updater.run_renewal_deployer(self.config, lineage, mock_deployer) self.assertTrue(mock_deployer.callcounter.called_with(lineage)) + @mock.patch("certbot.updater.logger.debug") + def test_updater_skip_dry_run(self, mock_log): + self.config.dry_run = True + updater.run_generic_updaters(self.config, None, None) + self.assertTrue(mock_log.called) + self.assertEquals(mock_log.call_args[0][0], + "Skipping updaters in dry-run mode.") + + @mock.patch("certbot.updater.logger.debug") + def test_deployer_skip_dry_run(self, mock_log): + self.config.dry_run = True + updater.run_renewal_deployer(self.config, None, None) + self.assertTrue(mock_log.called) + self.assertEquals(mock_log.call_args[0][0], + "Skipping renewal deployer in dry-run mode.") + if __name__ == '__main__': unittest.main() # pragma: no cover diff --git a/certbot/updater.py b/certbot/updater.py index 1216f38a6..112cf06ef 100644 --- a/certbot/updater.py +++ b/certbot/updater.py @@ -8,21 +8,24 @@ from certbot.plugins import selection as plug_sel logger = logging.getLogger(__name__) -def run_generic_updaters(config, plugins, lineage): +def run_generic_updaters(config, lineage, plugins): """Run updaters that the plugin supports :param config: Configuration object :type config: interfaces.IConfig - :param plugins: List of plugins - :type plugins: `list` of `str` - :param lineage: Certificate lineage object :type lineage: storage.RenewableCert + :param plugins: List of plugins + :type plugins: `list` of `str` + :returns: `None` :rtype: None """ + if config.dry_run: + logger.debug("Skipping updaters in dry-run mode.") + return try: # installers are used in auth mode to determine domain names installer, _ = plug_sel.choose_configurator_plugins(config, plugins, "certonly") @@ -31,10 +34,13 @@ def run_generic_updaters(config, plugins, lineage): return _run_updaters(lineage, installer, config) -def run_renewal_deployer(lineage, installer, config): +def run_renewal_deployer(config, lineage, installer): """Helper function to run deployer interface method if supported by the used installer plugin. + :param config: Configuration object + :type config: interfaces.IConfig + :param lineage: Certificate lineage object :type lineage: storage.RenewableCert @@ -44,6 +50,10 @@ def run_renewal_deployer(lineage, installer, config): :returns: `None` :rtype: None """ + if config.dry_run: + logger.debug("Skipping renewal deployer in dry-run mode.") + return + if not config.disable_renew_updates and isinstance(installer, interfaces.RenewDeployer): installer.renew_deploy(lineage)