From ae0be73b5386b18a45dc60eefdba4c26bfeb393e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Aug 2017 17:06:09 -0700 Subject: [PATCH] Make common Installer base class (#5055) * Add installer class * Add wrapped reverter methods to common.Installer. * Use Installer class in Apache plugin * Use Installer class in Nginx plugin * Don't create reverter in Apache and Nginx plugins --- .../certbot_apache/augeas_configurator.py | 57 +++-------- certbot-nginx/certbot_nginx/configurator.py | 54 ++-------- certbot/plugins/common.py | 98 +++++++++++++++++++ certbot/plugins/common_test.py | 87 ++++++++++++++++ 4 files changed, 202 insertions(+), 94 deletions(-) diff --git a/certbot-apache/certbot_apache/augeas_configurator.py b/certbot-apache/certbot_apache/augeas_configurator.py index 444fbb763..40a3d84df 100644 --- a/certbot-apache/certbot_apache/augeas_configurator.py +++ b/certbot-apache/certbot_apache/augeas_configurator.py @@ -3,7 +3,6 @@ import logging from certbot import errors -from certbot import reverter from certbot.plugins import common from certbot_apache import constants @@ -11,7 +10,7 @@ from certbot_apache import constants logger = logging.getLogger(__name__) -class AugeasConfigurator(common.Plugin): +class AugeasConfigurator(common.Installer): """Base Augeas Configurator class. :ivar config: Configuration. @@ -33,11 +32,6 @@ class AugeasConfigurator(common.Plugin): self.save_notes = "" - # See if any temporary changes need to be recovered - # This needs to occur before VirtualHost objects are setup... - # because this will change the underlying configuration and potential - # vhosts - self.reverter = reverter.Reverter(self.config) def init_augeas(self): """ Initialize the actual Augeas instance """ @@ -50,6 +44,10 @@ class AugeasConfigurator(common.Plugin): flags=(augeas.Augeas.NONE | augeas.Augeas.NO_MODL_AUTOLOAD | augeas.Augeas.ENABLE_SPAN)) + # See if any temporary changes need to be recovered + # This needs to occur before VirtualHost objects are setup... + # because this will change the underlying configuration and potential + # vhosts self.recovery_routine() def check_parsing_errors(self, lens): @@ -124,17 +122,8 @@ class AugeasConfigurator(common.Plugin): if save_paths: for path in save_paths: save_files.add(self.aug.get(path)[6:]) - - try: - # Create Checkpoint - if temporary: - self.reverter.add_to_temp_checkpoint( - save_files, self.save_notes) - else: - self.reverter.add_to_checkpoint(save_files, - self.save_notes) - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + self.add_to_checkpoint(save_files, + self.save_notes, temporary=temporary) self.aug.set("/augeas/save", save_state) self.save_notes = "" @@ -147,10 +136,7 @@ class AugeasConfigurator(common.Plugin): self.aug.remove("/files/"+sf) self.aug.load() if title and not temporary: - try: - self.reverter.finalize_checkpoint(title) - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + self.finalize_checkpoint(title) def _log_save_errors(self, ex_errs): """Log errors due to bad Augeas save. @@ -175,10 +161,7 @@ class AugeasConfigurator(common.Plugin): :raises .errors.PluginError: If unable to recover the configuration """ - try: - self.reverter.recovery_routine() - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + super(AugeasConfigurator, self).recovery_routine() # Need to reload configuration after these changes take effect self.aug.load() @@ -188,10 +171,7 @@ class AugeasConfigurator(common.Plugin): :raises .errors.PluginError: If unable to revert the challenge config. """ - try: - self.reverter.revert_temporary_config() - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + self.revert_temporary_config() self.aug.load() def rollback_checkpoints(self, rollback=1): @@ -203,20 +183,5 @@ class AugeasConfigurator(common.Plugin): the function is unable to correctly revert the configuration """ - try: - self.reverter.rollback_checkpoints(rollback) - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + super(AugeasConfigurator, self).rollback_checkpoints(rollback) self.aug.load() - - def view_config_changes(self): - """Show all of the configuration changes that have taken place. - - :raises .errors.PluginError: If there is a problem while processing - the checkpoints directories. - - """ - try: - self.reverter.view_config_changes() - except errors.ReverterError as err: - raise errors.PluginError(str(err)) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 0579c8d52..aac8c1237 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -19,7 +19,6 @@ from certbot import crypto_util from certbot import errors from certbot import interfaces from certbot import util -from certbot import reverter from certbot.plugins import common @@ -63,7 +62,7 @@ TEST_REDIRECT_COMMENT_BLOCK = [ @zope.interface.implementer(interfaces.IAuthenticator, interfaces.IInstaller) @zope.interface.provider(interfaces.IPluginFactory) -class NginxConfigurator(common.Plugin): +class NginxConfigurator(common.Installer): # pylint: disable=too-many-instance-attributes,too-many-public-methods """Nginx configurator. @@ -127,8 +126,6 @@ class NginxConfigurator(common.Plugin): self._enhance_func = {"redirect": self._enable_redirect, "staple-ocsp": self._enable_ocsp_stapling} - # Set up reverter - self.reverter = reverter.Reverter(self.config) self.reverter.recovery_routine() @property @@ -697,31 +694,13 @@ class NginxConfigurator(common.Plugin): """ save_files = set(self.parser.parsed.keys()) - - try: # TODO: make a common base for Apache and Nginx plugins - # Create Checkpoint - if temporary: - self.reverter.add_to_temp_checkpoint( - save_files, self.save_notes) - # how many comments does it take - else: - self.reverter.add_to_checkpoint(save_files, - self.save_notes) - # to confuse a linter? - except errors.ReverterError as err: - raise errors.PluginError(str(err)) - + self.add_to_checkpoint(save_files, self.save_notes, temporary) self.save_notes = "" # Change 'ext' to something else to not override existing conf files self.parser.filedump(ext='') if title and not temporary: - try: - self.reverter.finalize_checkpoint(title) - except errors.ReverterError as err: - raise errors.PluginError(str(err)) - - return True + self.finalize_checkpoint(title) def recovery_routine(self): """Revert all previously modified files. @@ -731,10 +710,7 @@ class NginxConfigurator(common.Plugin): :raises .errors.PluginError: If unable to recover the configuration """ - try: - self.reverter.recovery_routine() - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + super(NginxConfigurator, self).recovery_routine() self.parser.load() def revert_challenge_config(self): @@ -743,10 +719,7 @@ class NginxConfigurator(common.Plugin): :raises .errors.PluginError: If unable to revert the challenge config. """ - try: - self.reverter.revert_temporary_config() - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + self.revert_temporary_config() self.parser.load() def rollback_checkpoints(self, rollback=1): @@ -758,24 +731,9 @@ class NginxConfigurator(common.Plugin): the function is unable to correctly revert the configuration """ - try: - self.reverter.rollback_checkpoints(rollback) - except errors.ReverterError as err: - raise errors.PluginError(str(err)) + super(NginxConfigurator, self).rollback_checkpoints(rollback) self.parser.load() - def view_config_changes(self): - """Show all of the configuration changes that have taken place. - - :raises .errors.PluginError: If there is a problem while processing - the checkpoints directories. - - """ - try: - self.reverter.view_config_changes() - except errors.ReverterError as err: - raise errors.PluginError(str(err)) - ########################################################################### # Challenges Section for IAuthenticator ########################################################################### diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 255563bb6..7bb019163 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -13,7 +13,9 @@ from acme.jose import util as jose_util from certbot import constants from certbot import crypto_util +from certbot import errors from certbot import interfaces +from certbot import reverter from certbot import util logger = logging.getLogger(__name__) @@ -100,6 +102,102 @@ class Plugin(object): # other +class Installer(Plugin): + """An installer base class with reverter methods defined. + + Installer plugins do not have to inherit from this class. + + """ + def __init__(self, *args, **kwargs): + super(Installer, self).__init__(*args, **kwargs) + self.reverter = reverter.Reverter(self.config) + + def add_to_checkpoint(self, save_files, save_notes, temporary=False): + """Add files to a checkpoint. + + :param set save_files: set of filepaths to save + :param str save_notes: notes about changes during the save + :param bool temporary: True if the files should be added to a + temporary checkpoint rather than a permanent one. This is + usually used for changes that will soon be reverted. + + :raises .errors.PluginError: when unable to add to checkpoint + + """ + if temporary: + checkpoint_func = self.reverter.add_to_temp_checkpoint + else: + checkpoint_func = self.reverter.add_to_checkpoint + + try: + checkpoint_func(save_files, save_notes) + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + def finalize_checkpoint(self, title): + """Timestamp and save changes made through the reverter. + + :param str title: Title describing checkpoint + + :raises .errors.PluginError: when an error occurs + + """ + try: + self.reverter.finalize_checkpoint(title) + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + def recovery_routine(self): + """Revert all previously modified files. + + Reverts all modified files that have not been saved as a checkpoint + + :raises .errors.PluginError: If unable to recover the configuration + + """ + try: + self.reverter.recovery_routine() + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + def revert_temporary_config(self): + """Rollback temporary checkpoint. + + :raises .errors.PluginError: when unable to revert config + + """ + try: + self.reverter.revert_temporary_config() + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + def rollback_checkpoints(self, rollback=1): + """Rollback saved checkpoints. + + :param int rollback: Number of checkpoints to revert + + :raises .errors.PluginError: If there is a problem with the input or + the function is unable to correctly revert the configuration + + """ + try: + self.reverter.rollback_checkpoints(rollback) + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + def view_config_changes(self): + """Show all of the configuration changes that have taken place. + + :raises .errors.PluginError: If there is a problem while processing + the checkpoints directories. + + """ + try: + self.reverter.view_config_changes() + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + class Addr(object): r"""Represents an virtual host address. diff --git a/certbot/plugins/common_test.py b/certbot/plugins/common_test.py index 409c05946..cece2c5f9 100644 --- a/certbot/plugins/common_test.py +++ b/certbot/plugins/common_test.py @@ -1,4 +1,5 @@ """Tests for certbot.plugins.common.""" +import functools import os import shutil import tempfile @@ -12,6 +13,7 @@ from acme import jose from certbot import achallenges from certbot import crypto_util +from certbot import errors from certbot.tests import acme_util from certbot.tests import util as test_util @@ -77,6 +79,91 @@ class PluginTest(unittest.TestCase): "--mock-foo-bar", dest="different_to_foo_bar", x=1, y=None) +class InstallerTest(unittest.TestCase): + """Tests for certbot.plugins.common.Installer.""" + + def setUp(self): + from certbot.plugins.common import Installer + + with mock.patch("certbot.plugins.common.reverter.Reverter"): + self.installer = Installer(config=mock.MagicMock(), + name="Installer") + self.reverter = self.installer.reverter + + def test_add_to_real_checkpoint(self): + files = set(("foo.bar", "baz.qux",)) + save_notes = "foo bar baz qux" + self._test_wrapped_method("add_to_checkpoint", files, save_notes) + + def test_add_to_real_checkpoint2(self): + self._test_add_to_checkpoint_common(False) + + def test_add_to_temporary_checkpoint(self): + self._test_add_to_checkpoint_common(True) + + def _test_add_to_checkpoint_common(self, temporary): + files = set(("foo.bar", "baz.qux",)) + save_notes = "foo bar baz qux" + + installer_func = functools.partial(self.installer.add_to_checkpoint, + temporary=temporary) + + if temporary: + reverter_func = self.reverter.add_to_temp_checkpoint + else: + reverter_func = self.reverter.add_to_checkpoint + + self._test_adapted_method( + installer_func, reverter_func, files, save_notes) + + def test_finalize_checkpoint(self): + self._test_wrapped_method("finalize_checkpoint", "foo") + + def test_recovery_routine(self): + self._test_wrapped_method("recovery_routine") + + def test_revert_temporary_config(self): + self._test_wrapped_method("revert_temporary_config") + + def test_rollback_checkpoints(self): + self._test_wrapped_method("rollback_checkpoints", 42) + + def test_view_config_changes(self): + self._test_wrapped_method("view_config_changes") + + def _test_wrapped_method(self, name, *args, **kwargs): + """Test a wrapped reverter method. + + :param str name: name of the method to test + :param tuple args: position arguments to method + :param dict kwargs: keyword arguments to method + + """ + installer_func = getattr(self.installer, name) + reverter_func = getattr(self.reverter, name) + self._test_adapted_method( + installer_func, reverter_func, *args, **kwargs) + + def _test_adapted_method(self, installer_func, + reverter_func, *passed_args, **passed_kwargs): + """Test an adapted reverter method + + :param callable installer_func: installer method to test + :param mock.MagicMock reverter_func: mocked adapated + reverter method + :param tuple passed_args: positional arguments passed from + installer method to the reverter method + :param dict passed_kargs: keyword arguments passed from + installer method to the reverter method + + """ + installer_func(*passed_args, **passed_kwargs) + reverter_func.assert_called_once_with(*passed_args, **passed_kwargs) + reverter_func.side_effect = errors.ReverterError + self.assertRaises( + errors.PluginError, installer_func, *passed_args, **passed_kwargs) + + class AddrTest(unittest.TestCase): """Tests for certbot.client.plugins.common.Addr."""