[Apache v2] Move the apachectl parsing to apache_util (#7569)

* Move the Apache CLI parsing to apache_util

* Fix test mocks

* Address review comments

* Fix the parsernode metadata dictionary

certbot-apache/certbot_apache/apache_util.py

@@ -1,9 +1,16 @@
 """ Utility functions for certbot-apache plugin """
 import binascii
+import logging
+import re
+import subprocess
 
+from certbot import errors
 from certbot import util
+
 from certbot.compat import os
 
+logger = logging.getLogger(__name__)
+
 
 def get_mod_deps(mod_name):
     """Get known module dependencies.
@@ -105,3 +112,115 @@ def parse_define_file(filepath, varname):
 def unique_id():
     """ Returns an unique id to be used as a VirtualHost identifier"""
     return binascii.hexlify(os.urandom(16)).decode("utf-8")
+
+
+def parse_defines(apachectl):
+    """
+    Gets Defines from httpd process and returns a dictionary of
+    the defined variables.
+
+    :param str apachectl: Path to apachectl executable
+
+    :returns: dictionary of defined variables
+    :rtype: dict
+    """
+
+    variables = dict()
+    define_cmd = [apachectl, "-t", "-D",
+                  "DUMP_RUN_CFG"]
+    matches = parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)")
+    try:
+        matches.remove("DUMP_RUN_CFG")
+    except ValueError:
+        return {}
+
+    for match in matches:
+        if match.count("=") > 1:
+            logger.error("Unexpected number of equal signs in "
+                         "runtime config dump.")
+            raise errors.PluginError(
+                "Error parsing Apache runtime variables")
+        parts = match.partition("=")
+        variables[parts[0]] = parts[2]
+
+    return variables
+
+
+def parse_includes(apachectl):
+    """
+    Gets Include directives from httpd process and returns a list of
+    their values.
+
+    :param str apachectl: Path to apachectl executable
+
+    :returns: list of found Include directive values
+    :rtype: list of str
+    """
+
+    inc_cmd = [apachectl, "-t", "-D",
+               "DUMP_INCLUDES"]
+    return parse_from_subprocess(inc_cmd, r"\(.*\) (.*)")
+
+
+def parse_modules(apachectl):
+    """
+    Get loaded modules from httpd process, and return the list
+    of loaded module names.
+
+    :param str apachectl: Path to apachectl executable
+
+    :returns: list of found LoadModule module names
+    :rtype: list of str
+    """
+
+    mod_cmd = [apachectl, "-t", "-D",
+               "DUMP_MODULES"]
+    return parse_from_subprocess(mod_cmd, r"(.*)_module")
+
+
+def parse_from_subprocess(command, regexp):
+    """Get values from stdout of subprocess command
+
+    :param list command: Command to run
+    :param str regexp: Regexp for parsing
+
+    :returns: list parsed from command output
+    :rtype: list
+
+    """
+    stdout = _get_runtime_cfg(command)
+    return re.compile(regexp).findall(stdout)
+
+
+def _get_runtime_cfg(command):
+    """
+    Get runtime configuration info.
+
+    :param command: Command to run
+
+    :returns: stdout from command
+
+    """
+    try:
+        proc = subprocess.Popen(
+            command,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            universal_newlines=True)
+        stdout, stderr = proc.communicate()
+
+    except (OSError, ValueError):
+        logger.error(
+            "Error running command %s for runtime parameters!%s",
+            command, os.linesep)
+        raise errors.MisconfigurationError(
+            "Error accessing loaded Apache parameters: {0}".format(
+                command))
+    # Small errors that do not impede
+    if proc.returncode != 0:
+        logger.warning("Error in checking parameter list: %s", stderr)
+        raise errors.MisconfigurationError(
+            "Apache is unable to check whether or not the module is "
+            "loaded because Apache is misconfigured.")
+
+    return stdout

certbot-apache/certbot_apache/configurator.py

@@ -359,6 +359,13 @@ class ApacheConfigurator(common.Installer):
 
     def get_parsernode_root(self, metadata):
         """Initializes the ParserNode parser root instance."""
+
+        apache_vars = dict()
+        apache_vars["defines"] = apache_util.parse_defines(self.option("ctl"))
+        apache_vars["includes"] = apache_util.parse_includes(self.option("ctl"))
+        apache_vars["modules"] = apache_util.parse_modules(self.option("ctl"))
+        metadata["apache_vars"] = apache_vars
+
         return dualparser.DualBlockNode(
             name=assertions.PASS,
             ancestor=None,

certbot-apache/certbot_apache/override_gentoo.py

@@ -70,6 +70,6 @@ class GentooParser(parser.ApacheParser):
     def update_modules(self):
         """Get loaded modules from httpd process, and add them to DOM"""
         mod_cmd = [self.configurator.option("ctl"), "modules"]
-        matches = self.parse_from_subprocess(mod_cmd, r"(.*)_module")
+        matches = apache_util.parse_from_subprocess(mod_cmd, r"(.*)_module")
         for mod in matches:
             self.add_mod(mod.strip())

certbot-apache/certbot_apache/parser.py

@@ -3,7 +3,6 @@ import copy
 import fnmatch
 import logging
 import re
-import subprocess
 import sys
 
 import six
@@ -13,6 +12,7 @@ from acme.magic_typing import Dict, List, Set  # pylint: disable=unused-import,
 from certbot import errors
 from certbot.compat import os
 
+from certbot_apache import apache_util
 from certbot_apache import constants
 
 logger = logging.getLogger(__name__)
@@ -291,32 +291,15 @@ class ApacheParser(object):
 
     def update_runtime_variables(self):
         """Update Includes, Defines and Includes from httpd config dump data"""
+
         self.update_defines()
         self.update_includes()
         self.update_modules()
 
     def update_defines(self):
-        """Get Defines from httpd process"""
+        """Updates the dictionary of known variables in the configuration"""
 
-        variables = dict()
-        define_cmd = [self.configurator.option("ctl"), "-t", "-D",
-                      "DUMP_RUN_CFG"]
-        matches = self.parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)")
-        try:
-            matches.remove("DUMP_RUN_CFG")
-        except ValueError:
-            return
-
-        for match in matches:
-            if match.count("=") > 1:
-                logger.error("Unexpected number of equal signs in "
-                             "runtime config dump.")
-                raise errors.PluginError(
-                    "Error parsing Apache runtime variables")
-            parts = match.partition("=")
-            variables[parts[0]] = parts[2]
-
-        self.variables = variables
+        self.variables = apache_util.parse_defines(self.configurator.option("ctl"))
 
     def update_includes(self):
         """Get includes from httpd process, and add them to DOM if needed"""
@@ -326,9 +309,7 @@ class ApacheParser(object):
         # configuration files
         _ = self.find_dir("Include")
 
-        inc_cmd = [self.configurator.option("ctl"), "-t", "-D",
-                   "DUMP_INCLUDES"]
-        matches = self.parse_from_subprocess(inc_cmd, r"\(.*\) (.*)")
+        matches = apache_util.parse_includes(self.configurator.option("ctl"))
         if matches:
             for i in matches:
                 if not self.parsed_in_current(i):
@@ -337,56 +318,10 @@ class ApacheParser(object):
     def update_modules(self):
         """Get loaded modules from httpd process, and add them to DOM"""
 
-        mod_cmd = [self.configurator.option("ctl"), "-t", "-D",
-                       "DUMP_MODULES"]
-        matches = self.parse_from_subprocess(mod_cmd, r"(.*)_module")
+        matches = apache_util.parse_modules(self.configurator.option("ctl"))
         for mod in matches:
             self.add_mod(mod.strip())
 
-    def parse_from_subprocess(self, command, regexp):
-        """Get values from stdout of subprocess command
-
-        :param list command: Command to run
-        :param str regexp: Regexp for parsing
-
-        :returns: list parsed from command output
-        :rtype: list
-
-        """
-        stdout = self._get_runtime_cfg(command)
-        return re.compile(regexp).findall(stdout)
-
-    def _get_runtime_cfg(self, command):  # pylint: disable=no-self-use
-        """Get runtime configuration info.
-        :param command: Command to run
-
-        :returns: stdout from command
-
-        """
-        try:
-            proc = subprocess.Popen(
-                command,
-                stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE,
-                universal_newlines=True)
-            stdout, stderr = proc.communicate()
-
-        except (OSError, ValueError):
-            logger.error(
-                "Error running command %s for runtime parameters!%s",
-                command, os.linesep)
-            raise errors.MisconfigurationError(
-                "Error accessing loaded Apache parameters: {0}".format(
-                command))
-        # Small errors that do not impede
-        if proc.returncode != 0:
-            logger.warning("Error in checking parameter list: %s", stderr)
-            raise errors.MisconfigurationError(
-                "Apache is unable to check whether or not the module is "
-                "loaded because Apache is misconfigured.")
-
-        return stdout
-
     def filter_args_num(self, matches, args):  # pylint: disable=no-self-use
         """Filter out directives with specific number of arguments.
 

certbot-apache/certbot_apache/tests/centos_test.py

@@ -107,7 +107,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest):
     def test_get_parser(self):
         self.assertIsInstance(self.config.parser, override_centos.CentOSParser)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_opportunistic_httpd_runtime_parsing(self, mock_get):
         define_val = (
             'Define: TEST1\n'
@@ -156,7 +156,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest):
                 raise Exception("Missed: %s" % vhost)  # pragma: no cover
         self.assertEqual(found, 2)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_get_sysconfig_vars(self, mock_cfg):
         """Make sure we read the sysconfig OPTIONS variable correctly"""
         # Return nothing for the process calls

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -807,7 +807,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertEqual(mock_restart.call_count, 1)
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_cleanup(self, mock_cfg, mock_restart):
         mock_cfg.return_value = ""
         _, achalls = self.get_key_and_achalls()
@@ -823,7 +823,7 @@ class MultipleVhostsTest(util.ApacheTest):
                 self.assertFalse(mock_restart.called)
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_cleanup_no_errors(self, mock_cfg, mock_restart):
         mock_cfg.return_value = ""
         _, achalls = self.get_key_and_achalls()

certbot-apache/certbot_apache/tests/debian_test.py

@@ -47,7 +47,7 @@ class MultipleVhostsTestDebian(util.ApacheTest):
 
     @mock.patch("certbot.util.run_script")
     @mock.patch("certbot.util.exe_exists")
-    @mock.patch("certbot_apache.parser.subprocess.Popen")
+    @mock.patch("certbot_apache.apache_util.subprocess.Popen")
     def test_enable_mod(self, mock_popen, mock_exe_exists, mock_run_script):
         mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "")
         mock_popen().returncode = 0

certbot-apache/certbot_apache/tests/fedora_test.py

@@ -101,7 +101,7 @@ class MultipleVhostsTestFedora(util.ApacheTest):
     def test_get_parser(self):
         self.assertIsInstance(self.config.parser, override_fedora.FedoraParser)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_opportunistic_httpd_runtime_parsing(self, mock_get):
         define_val = (
             'Define: TEST1\n'
@@ -156,7 +156,7 @@ class MultipleVhostsTestFedora(util.ApacheTest):
                 raise Exception("Missed: %s" % vhost)  # pragma: no cover
         self.assertEqual(found, 2)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_get_sysconfig_vars(self, mock_cfg):
         """Make sure we read the sysconfig OPTIONS variable correctly"""
         # Return nothing for the process calls

certbot-apache/certbot_apache/tests/gentoo_test.py

@@ -90,7 +90,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest):
         for define in defines:
             self.assertTrue(define in self.config.parser.variables.keys())
 
-    @mock.patch("certbot_apache.parser.ApacheParser.parse_from_subprocess")
+    @mock.patch("certbot_apache.apache_util.parse_from_subprocess")
     def test_no_binary_configdump(self, mock_subprocess):
         """Make sure we don't call binary dumps other than modules from Apache
         as this is not supported in Gentoo currently"""
@@ -104,7 +104,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest):
         self.config.parser.reset_modules()
         self.assertTrue(mock_subprocess.called)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_opportunistic_httpd_runtime_parsing(self, mock_get):
         mod_val = (
             'Loaded Modules:\n'

certbot-apache/certbot_apache/tests/parser_test.py

@@ -167,7 +167,7 @@ class BasicParserTest(util.ParserTest):
             self.assertTrue(mock_logger.debug.called)
 
     @mock.patch("certbot_apache.parser.ApacheParser.find_dir")
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_update_runtime_variables(self, mock_cfg, _):
         define_val = (
             'ServerRoot: "/etc/apache2"\n'
@@ -273,7 +273,7 @@ class BasicParserTest(util.ParserTest):
             self.assertEqual(mock_parse.call_count, 25)
 
     @mock.patch("certbot_apache.parser.ApacheParser.find_dir")
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_update_runtime_variables_alt_values(self, mock_cfg, _):
         inc_val = (
             'Included configuration files:\n'
@@ -295,7 +295,7 @@ class BasicParserTest(util.ParserTest):
             # path derived from root configuration Include statements
             self.assertEqual(mock_parse.call_count, 1)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_update_runtime_vars_bad_output(self, mock_cfg):
         mock_cfg.return_value = "Define: TLS=443=24"
         self.parser.update_runtime_variables()
@@ -305,7 +305,7 @@ class BasicParserTest(util.ParserTest):
             errors.PluginError, self.parser.update_runtime_variables)
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.option")
-    @mock.patch("certbot_apache.parser.subprocess.Popen")
+    @mock.patch("certbot_apache.apache_util.subprocess.Popen")
     def test_update_runtime_vars_bad_ctl(self, mock_popen, mock_opt):
         mock_popen.side_effect = OSError
         mock_opt.return_value = "nonexistent"
@@ -313,7 +313,7 @@ class BasicParserTest(util.ParserTest):
             errors.MisconfigurationError,
             self.parser.update_runtime_variables)
 
-    @mock.patch("certbot_apache.parser.subprocess.Popen")
+    @mock.patch("certbot_apache.apache_util.subprocess.Popen")
     def test_update_runtime_vars_bad_exit(self, mock_popen):
         mock_popen().communicate.return_value = ("", "")
         mock_popen.returncode = -1
@@ -357,7 +357,7 @@ class ParserInitTest(util.ApacheTest):
                 ApacheParser, os.path.relpath(self.config_path),
                 "/dummy/vhostpath", version=(2, 4, 22), configurator=self.config)
 
-    @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
+    @mock.patch("certbot_apache.apache_util._get_runtime_cfg")
     def test_unparseable(self, mock_cfg):
         from certbot_apache.parser import ApacheParser
         mock_cfg.return_value = ('Define: TEST')

certbot-apache/certbot_apache/tests/util.py

@@ -111,19 +111,21 @@ def get_apache_configurator(  # pylint: disable=too-many-arguments, too-many-loc
             mock_exe_exists.return_value = True
             with mock.patch("certbot_apache.parser.ApacheParser."
                             "update_runtime_variables"):
-                try:
-                    config_class = entrypoint.OVERRIDE_CLASSES[os_info]
-                except KeyError:
-                    config_class = configurator.ApacheConfigurator
-                config = config_class(config=mock_le_config, name="apache",
-                    version=version)
-                if not conf_vhost_path:
-                    config_class.OS_DEFAULTS["vhost_root"] = vhost_path
-                else:
-                    # Custom virtualhost path was requested
-                    config.config.apache_vhost_root = conf_vhost_path
-                config.config.apache_ctl = config_class.OS_DEFAULTS["ctl"]
-                config.prepare()
+                with mock.patch("certbot_apache.apache_util.parse_from_subprocess") as mock_sp:
+                    mock_sp.return_value = []
+                    try:
+                        config_class = entrypoint.OVERRIDE_CLASSES[os_info]
+                    except KeyError:
+                        config_class = configurator.ApacheConfigurator
+                    config = config_class(config=mock_le_config, name="apache",
+                                          version=version)
+                    if not conf_vhost_path:
+                        config_class.OS_DEFAULTS["vhost_root"] = vhost_path
+                    else:
+                        # Custom virtualhost path was requested
+                        config.config.apache_vhost_root = conf_vhost_path
+                    config.config.apache_ctl = config_class.OS_DEFAULTS["ctl"]
+                    config.prepare()
     return config