Merge pull request #1457 from letsencrypt/apache_rollback

Apache rollback

letsencrypt-apache/letsencrypt_apache/augeas_configurator.py

@@ -73,7 +73,8 @@ class AugeasConfigurator(common.Plugin):
 
         This function first checks for save errors, if none are found,
         all configuration changes made will be saved. According to the
-        function parameters.
+        function parameters. If an exception is raised, a new checkpoint
+        was not created.
 
         :param str title: The title of the save. If a title is given, the
             configuration will be saved as a new checkpoint and put in a
@@ -82,8 +83,9 @@ class AugeasConfigurator(common.Plugin):
         :param bool temporary: Indicates whether the changes made will
             be quickly reversed in the future (ie. challenges)
 
-        :raises .errors.PluginError: If there was an error in Augeas, in an
-            attempt to save the configuration, or an error creating a checkpoint
+        :raises .errors.PluginError: If there was an error in Augeas, in
+            an attempt to save the configuration, or an error creating a
+            checkpoint
 
         """
         save_state = self.aug.get("/augeas/save")
@@ -122,16 +124,16 @@ class AugeasConfigurator(common.Plugin):
             except errors.ReverterError as err:
                 raise errors.PluginError(str(err))
 
+        self.aug.set("/augeas/save", save_state)
+        self.save_notes = ""
+        self.aug.save()
+
         if title and not temporary:
             try:
                 self.reverter.finalize_checkpoint(title)
             except errors.ReverterError as err:
                 raise errors.PluginError(str(err))
 
-        self.aug.set("/augeas/save", save_state)
-        self.save_notes = ""
-        self.aug.save()
-
     def _log_save_errors(self, ex_errs):
         """Log errors due to bad Augeas save.
 

letsencrypt/client.py

@@ -5,6 +5,7 @@ import os
 from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives.asymmetric import rsa
 import OpenSSL
+import zope.component
 
 from acme import client as acme_client
 from acme import jose
@@ -18,6 +19,7 @@ from letsencrypt import continuity_auth
 from letsencrypt import crypto_util
 from letsencrypt import errors
 from letsencrypt import error_handler
+from letsencrypt import interfaces
 from letsencrypt import le_util
 from letsencrypt import reverter
 from letsencrypt import storage
@@ -330,26 +332,13 @@ class Client(object):
 
             self.installer.save("Deployed Let's Encrypt Certificate")
 
-        # sites may have been enabled / final cleanup
-        with error_handler.ErrorHandler(self._rollback_and_restart):
+        msg = ("We were unable to install your certificate, "
+               "however, we successfully restored your "
+               "server to its prior configuration.")
+        with error_handler.ErrorHandler(self._rollback_and_restart, msg):
+            # sites may have been enabled / final cleanup
             self.installer.restart()
 
-    def _rollback_and_restart(self):
-        """Rollback the most recent checkpoint and restart the webserver"""
-        logger.critical("Rolling back to previous server configuration...")
-        try:
-            self.installer.rollback_checkpoints()
-            self.installer.restart()
-        except:
-            # TODO: suggest letshelp-letsencypt here
-            logger.critical("Failure to rollback config "
-                            "changes and restart your server")
-            logger.critical("Please submit a bug report to "
-                            "https://github.com/letsencrypt/letsencrypt")
-            raise
-        logger.critical("Rollback successful; your server has "
-                        "been restarted with your old configuration")
-
     def enhance_config(self, domains, redirect=None):
         """Enhance the configuration.
 
@@ -386,7 +375,9 @@ class Client(object):
         :type vhost: :class:`letsencrypt.interfaces.IInstaller`
 
         """
-        with error_handler.ErrorHandler(self.installer.recovery_routine):
+        msg = ("We were unable to set up a redirect for your server, "
+               "however, we successfully installed your certificate.")
+        with error_handler.ErrorHandler(self._recovery_routine_with_msg, msg):
             for dom in domains:
                 try:
                     self.installer.enhance(dom, "redirect")
@@ -395,8 +386,41 @@ class Client(object):
                     raise
 
             self.installer.save("Add Redirects")
+
+        with error_handler.ErrorHandler(self._rollback_and_restart, msg):
             self.installer.restart()
 
+    def _recovery_routine_with_msg(self, success_msg):
+        """Calls the installer's recovery routine and prints success_msg
+
+        :param str success_msg: message to show on successful recovery
+
+        """
+        self.installer.recovery_routine()
+        reporter = zope.component.getUtility(interfaces.IReporter)
+        reporter.add_message(success_msg, reporter.HIGH_PRIORITY)
+
+    def _rollback_and_restart(self, success_msg):
+        """Rollback the most recent checkpoint and restart the webserver
+
+        :param str success_msg: message to show on successful rollback
+
+        """
+        logger.critical("Rolling back to previous server configuration...")
+        reporter = zope.component.getUtility(interfaces.IReporter)
+        try:
+            self.installer.rollback_checkpoints()
+            self.installer.restart()
+        except:
+            # TODO: suggest letshelp-letsencypt here
+            reporter.add_message(
+                "An error occured and we failed to restore your config and "
+                "restart your server. Please submit a bug report to "
+                "https://github.com/letsencrypt/letsencrypt",
+                reporter.HIGH_PRIORITY)
+            raise
+        reporter.add_message(success_msg, reporter.HIGH_PRIORITY)
+
 
 def validate_key_csr(privkey, csr=None):
     """Validate Key and CSR files.

letsencrypt/error_handler.py

@@ -1,4 +1,5 @@
 """Registers functions to be called if an exception or signal occurs."""
+import functools
 import logging
 import os
 import signal
@@ -40,11 +41,11 @@ class ErrorHandler(object):
     to be called again by the next signal handler.
 
     """
-    def __init__(self, func=None):
+    def __init__(self, func=None, *args, **kwargs):
         self.funcs = []
         self.prev_handlers = {}
         if func is not None:
-            self.register(func)
+            self.register(func, *args, **kwargs)
 
     def __enter__(self):
         self.set_signal_handlers()
@@ -57,9 +58,13 @@ class ErrorHandler(object):
             self.call_registered()
         self.reset_signal_handlers()
 
-    def register(self, func):
-        """Registers func to be called if an error occurs."""
-        self.funcs.append(func)
+    def register(self, func, *args, **kwargs):
+        """Sets func to be called with *args and **kwargs during cleanup
+
+        :param function func: function to be called in case of an error
+
+        """
+        self.funcs.append(functools.partial(func, *args, **kwargs))
 
     def call_registered(self):
         """Calls all registered functions"""

letsencrypt/interfaces.py

@@ -298,7 +298,8 @@ class IInstaller(IPlugin):
 
         Both title and temporary are needed because a save may be
         intended to be permanent, but the save is not ready to be a full
-        checkpoint
+        checkpoint. If an exception is raised, it is assumed a new
+        checkpoint was not created.
 
         :param str title: The title of the save. If a title is given, the
             configuration will be saved as a new checkpoint and put in a

letsencrypt/tests/client_test.py

@@ -148,7 +148,7 @@ class ClientTest(unittest.TestCase):
 
         shutil.rmtree(tmp_path)
 
-    def test_deploy_certificate(self):
+    def test_deploy_certificate_success(self):
         self.assertRaises(errors.Error, self.client.deploy_certificate,
                           ["foo.bar"], "key", "cert", "chain", "fullchain")
 
@@ -166,17 +166,38 @@ class ClientTest(unittest.TestCase):
         self.assertEqual(installer.save.call_count, 2)
         installer.restart.assert_called_once_with()
 
-    def test_deploy_certificate_restart_failure_with_recovery(self):
+    def test_deploy_certificate_failure(self):
+        installer = mock.MagicMock()
+        self.client.installer = installer
+
+        installer.deploy_cert.side_effect = errors.PluginError
+        self.assertRaises(errors.PluginError, self.client.deploy_certificate,
+                          ["foo.bar"], "key", "cert", "chain", "fullchain")
+        installer.recovery_routine.assert_called_once_with()
+
+    def test_deploy_certificate_save_failure(self):
+        installer = mock.MagicMock()
+        self.client.installer = installer
+
+        installer.save.side_effect = errors.PluginError
+        self.assertRaises(errors.PluginError, self.client.deploy_certificate,
+                          ["foo.bar"], "key", "cert", "chain", "fullchain")
+        installer.recovery_routine.assert_called_once_with()
+
+    @mock.patch("letsencrypt.client.zope.component.getUtility")
+    def test_deploy_certificate_restart_failure(self, mock_get_utility):
         installer = mock.MagicMock()
         installer.restart.side_effect = [errors.PluginError, None]
         self.client.installer = installer
 
         self.assertRaises(errors.PluginError, self.client.deploy_certificate,
                           ["foo.bar"], "key", "cert", "chain", "fullchain")
+        self.assertEqual(mock_get_utility().add_message.call_count, 1)
         installer.rollback_checkpoints.assert_called_once_with()
         self.assertEqual(installer.restart.call_count, 2)
 
-    def test_deploy_certificate_restart_failure_without_recovery(self):
+    @mock.patch("letsencrypt.client.zope.component.getUtility")
+    def test_deploy_certificate_restart_failure2(self, mock_get_utility):
         installer = mock.MagicMock()
         installer.restart.side_effect = errors.PluginError
         installer.rollback_checkpoints.side_effect = errors.ReverterError
@@ -184,6 +205,7 @@ class ClientTest(unittest.TestCase):
 
         self.assertRaises(errors.PluginError, self.client.deploy_certificate,
                           ["foo.bar"], "key", "cert", "chain", "fullchain")
+        self.assertEqual(mock_get_utility().add_message.call_count, 1)
         installer.rollback_checkpoints.assert_called_once_with()
         self.assertEqual(installer.restart.call_count, 1)
 
@@ -201,10 +223,68 @@ class ClientTest(unittest.TestCase):
         self.assertEqual(installer.save.call_count, 1)
         installer.restart.assert_called_once_with()
 
+    def test_enhance_config_no_installer(self):
+        self.assertRaises(errors.Error,
+                          self.client.enhance_config, ["foo.bar"])
+
+    @mock.patch("letsencrypt.client.zope.component.getUtility")
+    @mock.patch("letsencrypt.client.enhancements")
+    def test_enhance_config_enhance_failure(self, mock_enhancements,
+                                            mock_get_utility):
+        mock_enhancements.ask.return_value = True
+        installer = mock.MagicMock()
+        self.client.installer = installer
         installer.enhance.side_effect = errors.PluginError
+
         self.assertRaises(errors.PluginError,
                           self.client.enhance_config, ["foo.bar"], True)
         installer.recovery_routine.assert_called_once_with()
+        self.assertEqual(mock_get_utility().add_message.call_count, 1)
+
+    @mock.patch("letsencrypt.client.zope.component.getUtility")
+    @mock.patch("letsencrypt.client.enhancements")
+    def test_enhance_config_save_failure(self, mock_enhancements,
+                                         mock_get_utility):
+        mock_enhancements.ask.return_value = True
+        installer = mock.MagicMock()
+        self.client.installer = installer
+        installer.save.side_effect = errors.PluginError
+
+        self.assertRaises(errors.PluginError,
+                          self.client.enhance_config, ["foo.bar"], True)
+        installer.recovery_routine.assert_called_once_with()
+        self.assertEqual(mock_get_utility().add_message.call_count, 1)
+
+    @mock.patch("letsencrypt.client.zope.component.getUtility")
+    @mock.patch("letsencrypt.client.enhancements")
+    def test_enhance_config_restart_failure(self, mock_enhancements,
+                                            mock_get_utility):
+        mock_enhancements.ask.return_value = True
+        installer = mock.MagicMock()
+        self.client.installer = installer
+        installer.restart.side_effect = [errors.PluginError, None]
+
+        self.assertRaises(errors.PluginError,
+                          self.client.enhance_config, ["foo.bar"], True)
+        self.assertEqual(mock_get_utility().add_message.call_count, 1)
+        installer.rollback_checkpoints.assert_called_once_with()
+        self.assertEqual(installer.restart.call_count, 2)
+
+    @mock.patch("letsencrypt.client.zope.component.getUtility")
+    @mock.patch("letsencrypt.client.enhancements")
+    def test_enhance_config_restart_failure2(self, mock_enhancements,
+                                             mock_get_utility):
+        mock_enhancements.ask.return_value = True
+        installer = mock.MagicMock()
+        self.client.installer = installer
+        installer.restart.side_effect = errors.PluginError
+        installer.rollback_checkpoints.side_effect = errors.ReverterError
+
+        self.assertRaises(errors.PluginError,
+                          self.client.enhance_config, ["foo.bar"], True)
+        self.assertEqual(mock_get_utility().add_message.call_count, 1)
+        installer.rollback_checkpoints.assert_called_once_with()
+        self.assertEqual(installer.restart.call_count, 1)
 
 
 class RollbackTest(unittest.TestCase):

letsencrypt/tests/error_handler_test.py

@@ -13,7 +13,11 @@ class ErrorHandlerTest(unittest.TestCase):
         from letsencrypt import error_handler
 
         self.init_func = mock.MagicMock()
-        self.handler = error_handler.ErrorHandler(self.init_func)
+        self.init_args = set((42,))
+        self.init_kwargs = {'foo': 'bar'}
+        self.handler = error_handler.ErrorHandler(self.init_func,
+                                                  *self.init_args,
+                                                  **self.init_kwargs)
         # pylint: disable=protected-access
         self.signals = error_handler._SIGNALS
 
@@ -23,7 +27,8 @@ class ErrorHandlerTest(unittest.TestCase):
                 raise ValueError
         except ValueError:
             pass
-        self.init_func.assert_called_once_with()
+        self.init_func.assert_called_once_with(*self.init_args,
+                                               **self.init_kwargs)
 
     @mock.patch('letsencrypt.error_handler.os')
     @mock.patch('letsencrypt.error_handler.signal')
@@ -37,7 +42,8 @@ class ErrorHandlerTest(unittest.TestCase):
 
         signum = self.signals[0]
         signal_handler(signum, None)
-        self.init_func.assert_called_once_with()
+        self.init_func.assert_called_once_with(*self.init_args,
+                                               **self.init_kwargs)
         mock_os.kill.assert_called_once_with(mock_os.getpid(), signum)
 
         self.handler.reset_signal_handlers()
@@ -48,7 +54,8 @@ class ErrorHandlerTest(unittest.TestCase):
         bad_func = mock.MagicMock(side_effect=[ValueError])
         self.handler.register(bad_func)
         self.handler.call_registered()
-        self.init_func.assert_called_once_with()
+        self.init_func.assert_called_once_with(*self.init_args,
+                                               **self.init_kwargs)
         bad_func.assert_called_once_with()
 
     def test_sysexit_ignored(self):