diff --git a/certbot-apache/certbot_apache/augeas_configurator.py b/certbot-apache/certbot_apache/augeas_configurator.py index 40a3d84df..a32c65c41 100644 --- a/certbot-apache/certbot_apache/augeas_configurator.py +++ b/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() diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index b5929b95e..c630e243f 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/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") diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index 2a3830964..8929f5ecb 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/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: diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 1709be4b7..c04c88f5f 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/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")