Merge pull request #2282 from olabini/nginx_reverter_guarding

Guard reverter invocations and wrap in correct exceptions

letsencrypt-nginx/letsencrypt_nginx/configurator.py

@@ -190,6 +190,12 @@ class NginxConfigurator(common.Plugin):
                              ", ".join(str(addr) for addr in vhost.addrs)))
         self.save_notes += "\tssl_certificate %s\n" % fullchain_path
         self.save_notes += "\tssl_certificate_key %s\n" % key_path
+        if len(stapling_directives) > 0:
+            self.save_notes += "\tssl_trusted_certificate %s\n" % chain_path
+            self.save_notes += "\tssl_stapling on\n"
+            self.save_notes += "\tssl_stapling_verify on\n"
+
+
 
     #######################
     # Vhost parsing methods
@@ -512,21 +518,33 @@ class NginxConfigurator(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
+            an attempt to save the configuration, or an error creating a
+            checkpoint
+
         """
         save_files = set(self.parser.parsed.keys())
 
-        # Create Checkpoint
-        if temporary:
-            self.reverter.add_to_temp_checkpoint(
-                save_files, self.save_notes)
-        else:
-            self.reverter.add_to_checkpoint(save_files,
+        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.save_notes = ""
 
         # Change 'ext' to something else to not override existing conf files
         self.parser.filedump(ext='')
         if title and not temporary:
-            self.reverter.finalize_checkpoint(title)
+            try:
+                self.reverter.finalize_checkpoint(title)
+            except errors.ReverterError as err:
+                raise errors.PluginError(str(err))
 
         return True
 
@@ -535,13 +553,25 @@ class NginxConfigurator(common.Plugin):
 
         Reverts all modified files that have not been saved as a checkpoint
 
+        :raises .errors.PluginError: If unable to recover the configuration
+
         """
-        self.reverter.recovery_routine()
+        try:
+            self.reverter.recovery_routine()
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
         self.parser.load()
 
     def revert_challenge_config(self):
-        """Used to cleanup challenge configurations."""
-        self.reverter.revert_temporary_config()
+        """Used to cleanup challenge configurations.
+
+        :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.parser.load()
 
     def rollback_checkpoints(self, rollback=1):
@@ -549,13 +579,27 @@ class NginxConfigurator(common.Plugin):
 
         :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
+
         """
-        self.reverter.rollback_checkpoints(rollback)
+        try:
+            self.reverter.rollback_checkpoints(rollback)
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
         self.parser.load()
 
     def view_config_changes(self):
-        """Show all of the configuration changes that have taken place."""
-        self.reverter.view_config_changes()
+        """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

letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py

@@ -371,6 +371,31 @@ class NginxConfiguratorTest(util.NginxTest):
         mock_run_script.side_effect = errors.SubprocessError
         self.assertRaises(errors.MisconfigurationError, self.config.config_test)
 
+    @mock.patch("letsencrypt.reverter.Reverter.recovery_routine")
+    def test_recovery_routine_throws_error_from_reverter(self, mock_recovery_routine):
+        mock_recovery_routine.side_effect = errors.ReverterError("foo")
+        self.assertRaises(errors.PluginError, self.config.recovery_routine)
+
+    @mock.patch("letsencrypt.reverter.Reverter.view_config_changes")
+    def test_view_config_changes_throws_error_from_reverter(self, mock_view_config_changes):
+        mock_view_config_changes.side_effect = errors.ReverterError("foo")
+        self.assertRaises(errors.PluginError, self.config.view_config_changes)
+
+    @mock.patch("letsencrypt.reverter.Reverter.rollback_checkpoints")
+    def test_rollback_checkpoints_throws_error_from_reverter(self, mock_rollback_checkpoints):
+        mock_rollback_checkpoints.side_effect = errors.ReverterError("foo")
+        self.assertRaises(errors.PluginError, self.config.rollback_checkpoints)
+
+    @mock.patch("letsencrypt.reverter.Reverter.revert_temporary_config")
+    def test_revert_challenge_config_throws_error_from_reverter(self, mock_revert_temporary_config):
+        mock_revert_temporary_config.side_effect = errors.ReverterError("foo")
+        self.assertRaises(errors.PluginError, self.config.revert_challenge_config)
+
+    @mock.patch("letsencrypt.reverter.Reverter.add_to_checkpoint")
+    def test_save_throws_error_from_reverter(self, mock_add_to_checkpoint):
+        mock_add_to_checkpoint.side_effect = errors.ReverterError("foo")
+        self.assertRaises(errors.PluginError, self.config.save)
+
     def test_get_snakeoil_paths(self):
         # pylint: disable=protected-access
         cert, key = self.config._get_snakeoil_paths()