Merge pull request #693 from letsencrypt/fix_cleanup_enable_mod

Fix enable_mod for "run" command

letsencrypt-apache/letsencrypt_apache/configurator.py

@@ -493,7 +493,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """
         if "ssl_module" not in self.parser.modules:
             logger.info("Loading mod_ssl into Apache Server")
-            self.enable_mod("ssl", temp)
+            self.enable_mod("ssl", temp=temp)
 
         # Check for Listen <port>
         # Note: This could be made to also look for ip:443 combo
@@ -952,6 +952,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
     def enable_site(self, vhost):
         """Enables an available site, Apache restart required.
 
+        .. note:: Does not make sure that the site correctly works or that all
+        modules are enabled appropriately.
+
         .. todo:: This function should number subdomains before the domain vhost
         .. todo:: Make sure link is not broken...
 
@@ -965,12 +968,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         if self.is_site_enabled(vhost.filep):
             return
 
-        if vhost.ssl:
-            # TODO: Make this based on addresses
-            self.prepare_server_https("443")
-            if self.save_notes:
-                self.save()
-
         if "/sites-available/" in vhost.filep:
             enabled_path = ("%s/sites-enabled/%s" %
                             (self.parser.root, os.path.basename(vhost.filep)))
@@ -1138,6 +1135,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         if not self._chall_out:
             self.revert_challenge_config()
             self.restart()
+            self.parser.init_modules()
 
 
 def apache_restart(apache_init_script):

letsencrypt-apache/letsencrypt_apache/dvsni.py

@@ -53,7 +53,7 @@ class ApacheDvsni(common.Dvsni):
             "le_dvsni_cert_challenge.conf")
 
     def perform(self):
-        """Peform a DVSNI challenge."""
+        """Perform a DVSNI challenge."""
         if not self.achalls:
             return []
         # Save any changes to the configuration as a precaution

letsencrypt-apache/letsencrypt_apache/parser.py

@@ -51,7 +51,7 @@ class ApacheParser(object):
         # https://httpd.apache.org/docs/2.4/mod/core.html#ifmodule
         # This needs to come before locations are set.
         self.modules = set()
-        self._init_modules()
+        self.init_modules()
 
         # Set up rest of locations
         self.loc.update(self._set_locations())
@@ -60,13 +60,15 @@ class ApacheParser(object):
         # Sites-available is not included naturally in configuration
         self._parse_file(os.path.join(self.root, "sites-available") + "/*")
 
-    def _init_modules(self):
+    def init_modules(self):
         """Iterates on the configuration until no new modules are loaded.
 
         ..todo:: This should be attempted to be done with a binary to avoid
             the iteration issue.  Else... parse and enable mods at same time.
 
         """
+        # Since modules are being initiated... clear existing set.
+        self.modules = set()
         matches = self.find_dir("LoadModule")
 
         iterator = iter(matches)

letsencrypt-apache/letsencrypt_apache/tests/complex_parsing_test.py

@@ -18,7 +18,7 @@ class ComplexParserTest(util.ParserTest):
         self.setup_variables()
         # This needs to happen after due to setup_variables not being run
         # until after
-        self.parser._init_modules()  # pylint: disable=protected-access
+        self.parser.init_modules()  # pylint: disable=protected-access
 
     def tearDown(self):
         shutil.rmtree(self.temp_dir)

letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py

@@ -207,20 +207,11 @@ class TwoVhost80Test(util.ApacheTest):
         self.assertRaises(
             errors.MisconfigurationError, self.config.enable_mod, "ssl")
 
-    @mock.patch("letsencrypt.le_util.run_script")
+    def test_enable_site(self):
-    @mock.patch("letsencrypt.le_util.exe_exists")
-    @mock.patch("letsencrypt_apache.parser.subprocess.Popen")
-    def test_enable_site(self, mock_popen, mock_exe_exists, mock_run_script):
-        mock_popen().returncode = 0
-        mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "")
-        mock_exe_exists.return_value = True
-
         # Default 443 vhost
         self.assertFalse(self.vh_truth[1].enabled)
         self.config.enable_site(self.vh_truth[1])
         self.assertTrue(self.vh_truth[1].enabled)
-        # Mod enabled
-        self.assertTrue(mock_run_script.called)
 
         # Go again to make sure nothing fails
         self.config.enable_site(self.vh_truth[1])
@@ -302,7 +293,9 @@ class TwoVhost80Test(util.ApacheTest):
             "NameVirtualHost", "*:80"))
 
     def test_prepare_server_https(self):
-        self.config.parser.modules.add("ssl_module")
+        mock_enable = mock.Mock()
+        self.config.enable_mod = mock_enable
+
         mock_find = mock.Mock()
         mock_add_dir = mock.Mock()
         mock_find.return_value = []
@@ -312,7 +305,12 @@ class TwoVhost80Test(util.ApacheTest):
         self.config.parser.add_dir_to_ifmodssl = mock_add_dir
 
         self.config.prepare_server_https("443")
-        self.config.prepare_server_https("8080")
+        self.assertEqual(mock_enable.call_args[1], {"temp": False})
+
+        self.config.prepare_server_https("8080", temp=True)
+        # Enable mod is temporary
+        self.assertEqual(mock_enable.call_args[1], {"temp": True})
+
         self.assertEqual(mock_add_dir.call_count, 2)
 
     def test_make_vhost_ssl(self):

letsencrypt/plugins/disco.py

@@ -215,7 +215,7 @@ class PluginsRegistry(collections.Mapping):
         Returns ``None`` if ``plugin`` is not found in the registry.
 
         """
-        # use list instead of set beacse PluginEntryPoint is not hashable
+        # use list instead of set because PluginEntryPoint is not hashable
         candidates = [plugin_ep for plugin_ep in self._plugins.itervalues()
                       if plugin_ep.initialized and plugin_ep.init() is plugin]
         assert len(candidates) <= 1

letsencrypt/reverter.py

@@ -75,7 +75,11 @@ class Reverter(object):
         backups = os.listdir(self.config.backup_dir)
         backups.sort()
 
-        if len(backups) < rollback:
+        if not backups:
+            logger.warning(
+                "Let's Encrypt hasn't modified your configuration, so rollback "
+                "isn't available.")
+        elif len(backups) < rollback:
             logger.warning("Unable to rollback %d checkpoints, only %d exist",
                            rollback, len(backups))
 

letsencrypt/tests/reverter_test.py

@@ -343,9 +343,16 @@ class TestFullCheckpointsReverter(unittest.TestCase):
 
     @mock.patch("letsencrypt.reverter.logger")
     def test_rollback_too_many(self, mock_logger):
+        # Test no exist warning...
         self.reverter.rollback_checkpoints(1)
         self.assertEqual(mock_logger.warning.call_count, 1)
 
+        # Test Generic warning
+        mock_logger.warning.call_count = 0
+        self._setup_three_checkpoints()
+        self.reverter.rollback_checkpoints(4)
+        self.assertEqual(mock_logger.warning.call_count, 1)
+
     def test_multi_rollback(self):
         config3 = self._setup_three_checkpoints()
         self.reverter.rollback_checkpoints(3)