Make sure the Augeas DOM is written to disk before loading new files

certbot-apache/certbot_apache/augeas_configurator.py

@@ -76,26 +76,26 @@ class AugeasConfigurator(common.Installer):
                     self.aug.get(path + "/message")))
                 raise errors.PluginError(msg)
 
-    # TODO: Cleanup this function
-    def save(self, title=None, temporary=False):
-        """Saves all changes to the configuration files.
+    def ensure_augeas_state(self):
+        """Makes sure that all Augeas dom changes are written to files to avoid
+        loss of configuration directives when doing additional augeas parsing,
+        causing a possible augeas.load() resulting dom reset
+        """
 
-        This function first checks for save errors, if none are found,
-        all configuration changes made will be saved. According to the
-        function parameters. If an exception is raised, a new checkpoint
-        was not created.
+        if self.unsaved_files():
+            self.save_notes += "(autosave)"
+            self.save()
 
-        :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
-            timestamped directory.
-
-        :param bool temporary: Indicates whether the changes made will
-            be quickly reversed in the future (ie. challenges)
+    def unsaved_files(self):
+        """Lists files that have modified Augeas DOM but the changes have not
+        been written to the filesystem yet, used by `self.save()` and
+        ApacheConfigurator to check the file state.
 
         :raises .errors.PluginError: If there was an error in Augeas, in
             an attempt to save the configuration, or an error creating a
             checkpoint
 
+        :returns: `set` of unsaved files
         """
         save_state = self.aug.get("/augeas/save")
         self.aug.set("/augeas/save", "noop")
@@ -111,21 +111,41 @@ class AugeasConfigurator(common.Installer):
             raise errors.PluginError(
                 "Error saving files, check logs for more info.")
 
+        # Return the original save method
+        self.aug.set("/augeas/save", save_state)
+
         # Retrieve list of modified files
         # Note: Noop saves can cause the file to be listed twice, I used a
         # set to remove this possibility. This is a known augeas 0.10 error.
         save_paths = self.aug.match("/augeas/events/saved")
 
-        # If the augeas tree didn't change, no files were saved and a backup
-        # should not be created
         save_files = set()
         if save_paths:
             for path in save_paths:
                 save_files.add(self.aug.get(path)[6:])
+        return save_files
+
+    def save(self, title=None, temporary=False):
+        """Saves all changes to the configuration files.
+
+        This function first checks for save errors, if none are found,
+        all configuration changes made will be saved. According to the
+        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
+            timestamped directory.
+
+        :param bool temporary: Indicates whether the changes made will
+            be quickly reversed in the future (ie. challenges)
+
+        """
+        save_files = self.unsaved_files()
+        if save_files:
             self.add_to_checkpoint(save_files,
                                    self.save_notes, temporary=temporary)
 
-        self.aug.set("/augeas/save", save_state)
         self.save_notes = ""
         self.aug.save()
 

certbot-apache/certbot_apache/configurator.py

@@ -197,7 +197,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             self.vhostroot = os.path.abspath(self.conf("vhost-root"))
 
         self.parser = parser.ApacheParser(
-            self.aug, self.conf("server-root"), self.conf("vhost-root"), self.version)
+            self.aug, self.conf("server-root"), self.conf("vhost-root"),
+            self.version, configurator=self)
         # Check for errors in parsing files with Augeas
         self.check_parsing_errors("httpd.aug")
 

certbot-apache/certbot_apache/parser.py

@@ -30,9 +30,15 @@ class ApacheParser(object):
     arg_var_interpreter = re.compile(r"\$\{[^ \}]*}")
     fnmatch_chars = set(["*", "?", "\\", "[", "]"])
 
-    def __init__(self, aug, root, vhostroot=None, version=(2, 4)):
+    def __init__(self, aug, root, vhostroot=None, version=(2, 4),
+                 configurator=None):
         # Note: Order is important here.
 
+        # Needed for calling save() with reverter functionality that resides in
+        # AugeasConfigurator superclass of ApacheConfigurator. This resolves
+        # issues with aug.load() after adding new files / defines to parse tree
+        self.configurator = configurator
+
         # This uses the binary, so it can be done first.
         # https://httpd.apache.org/docs/2.4/mod/core.html#define
         # https://httpd.apache.org/docs/2.4/mod/core.html#ifdefine
@@ -479,6 +485,10 @@ class ApacheParser(object):
 
         """
         use_new, remove_old = self._check_path_actions(filepath)
+        # Ensure that we have the latest Augeas DOM state on disk before
+        # calling aug.load() which reloads the state from disk
+        if self.configurator:
+            self.configurator.ensure_augeas_state()
         # Test if augeas included file for Httpd.lens
         # Note: This works for augeas globs, ie. *.conf
         if use_new:

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -776,10 +776,10 @@ class MultipleVhostsTest(util.ApacheTest):
 
     def test_add_name_vhost_if_necessary(self):
         # pylint: disable=protected-access
-        self.config.save = mock.Mock()
+        self.config.add_name_vhost = mock.Mock()
         self.config.version = (2, 2)
         self.config._add_name_vhost_if_necessary(self.vh_truth[0])
-        self.assertTrue(self.config.save.called)
+        self.assertTrue(self.config.add_name_vhost.called)
 
         new_addrs = set()
         for addr in self.vh_truth[0].addrs:
@@ -787,7 +787,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         self.vh_truth[0].addrs = new_addrs
         self.config._add_name_vhost_if_necessary(self.vh_truth[0])
-        self.assertEqual(self.config.save.call_count, 2)
+        self.assertEqual(self.config.add_name_vhost.call_count, 2)
 
     @mock.patch("certbot_apache.configurator.tls_sni_01.ApacheTlsSni01.perform")
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")