diff options
author | Adrien Ferrand <adferrand@users.noreply.github.com> | 2021-04-13 21:18:49 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-04-13 21:18:49 +0300 |
commit | 0dbe17bbd48b97d41754b9618c302ab3c4aa1421 (patch) | |
tree | 33589e50d39d33e2506024543859ac9bf5aa0241 /certbot-apache | |
parent | e33090f2827558962984e0fa6cd16e653307f412 (diff) |
Define OS options by a dedicated object in Apache configurator (#8778)
In https://github.com/certbot/certbot/pull/8748#discussion_r605457670 we discussed about changing the dict used to set OS options for Apache configurators into a dedicated object.
* Create _OsOptions class to configure the os specific options of the Apache configurators
* Fix tests
* Clean imports
* Fix naming
* Fix compatibility tests
* Rename a class
* Ensure restart_cmd_alt is set for specific OSes.
* Add docstring
* Fix override
* Fix coverage
Diffstat (limited to 'certbot-apache')
12 files changed, 127 insertions, 158 deletions
diff --git a/certbot-apache/certbot_apache/_internal/configurator.py b/certbot-apache/certbot_apache/_internal/configurator.py index 5ef9083e6..abf199793 100644 --- a/certbot-apache/certbot_apache/_internal/configurator.py +++ b/certbot-apache/certbot_apache/_internal/configurator.py @@ -8,7 +8,6 @@ import logging import re import socket import time -from typing import cast from typing import DefaultDict from typing import Dict from typing import List @@ -51,6 +50,47 @@ except ImportError: # pragma: no cover logger = logging.getLogger(__name__) +class OsOptions: + """ + Dedicated class to describe the OS specificities (eg. paths, binary names) + that the Apache configurator needs to be aware to operate properly. + """ + def __init__(self, + server_root="/etc/apache2", + vhost_root="/etc/apache2/sites-available", + vhost_files="*", + logs_root="/var/log/apache2", + ctl="apache2ctl", + version_cmd: Optional[List[str]] = None, + restart_cmd: Optional[List[str]] = None, + restart_cmd_alt: Optional[List[str]] = None, + conftest_cmd: Optional[List[str]] = None, + enmod: Optional[str] = None, + dismod: Optional[str] = None, + le_vhost_ext="-le-ssl.conf", + handle_modules=False, + handle_sites=False, + challenge_location="/etc/apache2", + apache_bin: Optional[str] = None, + ): + self.server_root = server_root + self.vhost_root = vhost_root + self.vhost_files = vhost_files + self.logs_root = logs_root + self.ctl = ctl + self.version_cmd = ['apache2ctl', '-v'] if not version_cmd else version_cmd + self.restart_cmd = ['apache2ctl', 'graceful'] if not restart_cmd else restart_cmd + self.restart_cmd_alt = restart_cmd_alt + self.conftest_cmd = ['apache2ctl', 'configtest'] if not conftest_cmd else conftest_cmd + self.enmod = enmod + self.dismod = dismod + self.le_vhost_ext = le_vhost_ext + self.handle_modules = handle_modules + self.handle_sites = handle_sites + self.challenge_location = challenge_location + self.bin = apache_bin + + # TODO: Augeas sections ie. <VirtualHost>, <IfModule> beginning and closing # tags need to be the same case, otherwise Augeas doesn't recognize them. # This is not able to be completely remedied by regular expressions because @@ -106,27 +146,7 @@ class ApacheConfigurator(common.Installer): " change depending on the operating system Certbot is run on.)" ) - OS_DEFAULTS = dict( - server_root="/etc/apache2", - vhost_root="/etc/apache2/sites-available", - vhost_files="*", - logs_root="/var/log/apache2", - ctl="apache2ctl", - version_cmd=['apache2ctl', '-v'], - restart_cmd=['apache2ctl', 'graceful'], - conftest_cmd=['apache2ctl', 'configtest'], - enmod=None, - dismod=None, - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, - challenge_location="/etc/apache2", - bin=None - ) - - def option(self, key): - """Get a value from options""" - return self.options.get(key) + OS_DEFAULTS = OsOptions() def pick_apache_config(self, warn_on_no_mod_ssl=True): """ @@ -156,14 +176,14 @@ class ApacheConfigurator(common.Installer): for o in opts: # Config options use dashes instead of underscores if self.conf(o.replace("_", "-")) is not None: - self.options[o] = self.conf(o.replace("_", "-")) + setattr(self.options, o, self.conf(o.replace("_", "-"))) else: - self.options[o] = self.OS_DEFAULTS[o] + setattr(self.options, o, getattr(self.OS_DEFAULTS, o)) # Special cases - cast(List[str], self.options["version_cmd"])[0] = self.option("ctl") - cast(List[str], self.options["restart_cmd"])[0] = self.option("ctl") - cast(List[str], self.options["conftest_cmd"])[0] = self.option("ctl") + self.options.version_cmd[0] = self.options.ctl + self.options.restart_cmd[0] = self.options.ctl + self.options.conftest_cmd[0] = self.options.ctl @classmethod def add_parser_arguments(cls, add): @@ -178,30 +198,30 @@ class ApacheConfigurator(common.Installer): else: # cls.OS_DEFAULTS can be distribution specific, see override classes DEFAULTS = cls.OS_DEFAULTS - add("enmod", default=DEFAULTS["enmod"], + add("enmod", default=DEFAULTS.enmod, help="Path to the Apache 'a2enmod' binary") - add("dismod", default=DEFAULTS["dismod"], + add("dismod", default=DEFAULTS.dismod, help="Path to the Apache 'a2dismod' binary") - add("le-vhost-ext", default=DEFAULTS["le_vhost_ext"], + add("le-vhost-ext", default=DEFAULTS.le_vhost_ext, help="SSL vhost configuration extension") - add("server-root", default=DEFAULTS["server_root"], + add("server-root", default=DEFAULTS.server_root, help="Apache server root directory") add("vhost-root", default=None, help="Apache server VirtualHost configuration root") - add("logs-root", default=DEFAULTS["logs_root"], + add("logs-root", default=DEFAULTS.logs_root, help="Apache server logs directory") add("challenge-location", - default=DEFAULTS["challenge_location"], + default=DEFAULTS.challenge_location, help="Directory path for challenge configuration") - add("handle-modules", default=DEFAULTS["handle_modules"], + add("handle-modules", default=DEFAULTS.handle_modules, help="Let installer handle enabling required modules for you " + "(Only Ubuntu/Debian currently)") - add("handle-sites", default=DEFAULTS["handle_sites"], + add("handle-sites", default=DEFAULTS.handle_sites, help="Let installer handle enabling sites for you " + "(Only Ubuntu/Debian currently)") - add("ctl", default=DEFAULTS["ctl"], + add("ctl", default=DEFAULTS.ctl, help="Full path to Apache control script") - add("bin", default=DEFAULTS["bin"], + add("bin", default=DEFAULTS.bin, help="Full path to apache2/httpd binary") def __init__(self, *args, **kwargs): @@ -290,8 +310,8 @@ class ApacheConfigurator(common.Installer): ssl_module_location = self.parser.standard_path_from_server_root(ssl_module_location) else: # Possibility B: ssl_module is statically linked into Apache - if self.option("bin"): - ssl_module_location = self.option("bin") + if self.options.bin: + ssl_module_location = self.options.bin else: logger.warning("ssl_module is statically linked but --apache-bin is " "missing; not disabling session tickets.") @@ -321,7 +341,7 @@ class ApacheConfigurator(common.Installer): self._prepare_options() # Verify Apache is installed - self._verify_exe_availability(self.option("ctl")) + self._verify_exe_availability(self.options.ctl) # Make sure configuration is valid self.config_test() @@ -361,20 +381,20 @@ class ApacheConfigurator(common.Installer): # We may try to enable mod_ssl later. If so, we shouldn't warn if we can't find it now. # This is currently only true for debian/ubuntu. - warn_on_no_mod_ssl = not self.option("handle_modules") + warn_on_no_mod_ssl = not self.options.handle_modules self.install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest, warn_on_no_mod_ssl) # Prevent two Apache plugins from modifying a config at once try: - util.lock_dir_until_exit(self.option("server_root")) + util.lock_dir_until_exit(self.options.server_root) except (OSError, errors.LockError): logger.debug("Encountered error:", exc_info=True) raise errors.PluginError( "Unable to create a lock file in {0}. Are you running" " Certbot with sufficient privileges to modify your" - " Apache configuration?".format(self.option("server_root"))) + " Apache configuration?".format(self.options.server_root)) self._prepared = True def save(self, title=None, temporary=False): @@ -449,7 +469,7 @@ class ApacheConfigurator(common.Installer): """Initializes the ApacheParser""" # If user provided vhost_root value in command line, use it return parser.ApacheParser( - self.option("server_root"), self.conf("vhost-root"), + self.options.server_root, self.conf("vhost-root"), self.version, configurator=self) def get_parsernode_root(self, metadata): @@ -457,9 +477,9 @@ class ApacheConfigurator(common.Installer): if HAS_APACHECONFIG: apache_vars = {} - 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")) + apache_vars["defines"] = apache_util.parse_defines(self.options.ctl) + apache_vars["includes"] = apache_util.parse_includes(self.options.ctl) + apache_vars["modules"] = apache_util.parse_modules(self.options.ctl) metadata["apache_vars"] = apache_vars with open(self.parser.loc["root"]) as f: @@ -1311,7 +1331,7 @@ class ApacheConfigurator(common.Installer): :param boolean temp: If the change is temporary """ - if self.option("handle_modules"): + if self.options.handle_modules: if self.version >= (2, 4) and ("socache_shmcb_module" not in self.parser.modules): self.enable_mod("socache_shmcb", temp=temp) @@ -1331,7 +1351,7 @@ class ApacheConfigurator(common.Installer): Duplicates vhost and adds default ssl options New vhost will reside as (nonssl_vhost.path) + - ``self.option("le_vhost_ext")`` + ``self.options.le_vhost_ext`` .. note:: This function saves the configuration @@ -1430,15 +1450,15 @@ class ApacheConfigurator(common.Installer): """ if self.conf("vhost-root") and os.path.exists(self.conf("vhost-root")): - fp = os.path.join(filesystem.realpath(self.option("vhost_root")), + fp = os.path.join(filesystem.realpath(self.options.vhost_root), os.path.basename(non_ssl_vh_fp)) else: # Use non-ssl filepath fp = filesystem.realpath(non_ssl_vh_fp) if fp.endswith(".conf"): - return fp[:-(len(".conf"))] + self.option("le_vhost_ext") - return fp + self.option("le_vhost_ext") + return fp[:-(len(".conf"))] + self.options.le_vhost_ext + return fp + self.options.le_vhost_ext def _sift_rewrite_rule(self, line): """Decides whether a line should be copied to a SSL vhost. @@ -2282,7 +2302,7 @@ class ApacheConfigurator(common.Installer): addr in self._get_proposed_addrs(ssl_vhost)), servername, serveralias, " ".join(rewrite_rule_args), - self.option("logs_root"))) + self.options.logs_root)) def _write_out_redirect(self, ssl_vhost, text): # This is the default name @@ -2294,7 +2314,7 @@ class ApacheConfigurator(common.Installer): if len(ssl_vhost.name) < (255 - (len(redirect_filename) + 1)): redirect_filename = "le-redirect-%s.conf" % ssl_vhost.name - redirect_filepath = os.path.join(self.option("vhost_root"), + redirect_filepath = os.path.join(self.options.vhost_root, redirect_filename) # Register the new file that will be created @@ -2414,19 +2434,18 @@ class ApacheConfigurator(common.Installer): """ try: - util.run_script(self.option("restart_cmd")) + util.run_script(self.options.restart_cmd) except errors.SubprocessError as err: logger.info("Unable to restart apache using %s", - self.option("restart_cmd")) - alt_restart = self.option("restart_cmd_alt") + self.options.restart_cmd) + alt_restart = self.options.restart_cmd_alt if alt_restart: logger.debug("Trying alternative restart command: %s", alt_restart) # There is an alternative restart command available # This usually is "restart" verb while original is "graceful" try: - util.run_script(self.option( - "restart_cmd_alt")) + util.run_script(self.options.restart_cmd_alt) return except errors.SubprocessError as secerr: error = str(secerr) @@ -2441,7 +2460,7 @@ class ApacheConfigurator(common.Installer): """ try: - util.run_script(self.option("conftest_cmd")) + util.run_script(self.options.conftest_cmd) except errors.SubprocessError as err: raise errors.MisconfigurationError(str(err)) @@ -2457,11 +2476,11 @@ class ApacheConfigurator(common.Installer): """ try: - stdout, _ = util.run_script(self.option("version_cmd")) + stdout, _ = util.run_script(self.options.version_cmd) except errors.SubprocessError: raise errors.PluginError( "Unable to run %s -v" % - self.option("version_cmd")) + self.options.version_cmd) regex = re.compile(r"Apache/([0-9\.]*)", re.IGNORECASE) matches = regex.findall(stdout) diff --git a/certbot-apache/certbot_apache/_internal/override_arch.py b/certbot-apache/certbot_apache/_internal/override_arch.py index 1c3aed1dc..30d161a4e 100644 --- a/certbot-apache/certbot_apache/_internal/override_arch.py +++ b/certbot-apache/certbot_apache/_internal/override_arch.py @@ -3,13 +3,14 @@ import zope.interface from certbot import interfaces from certbot_apache._internal import configurator +from certbot_apache._internal.configurator import OsOptions @zope.interface.provider(interfaces.IPluginFactory) class ArchConfigurator(configurator.ApacheConfigurator): """Arch Linux specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( + OS_DEFAULTS = OsOptions( server_root="/etc/httpd", vhost_root="/etc/httpd/conf", vhost_files="*.conf", @@ -18,11 +19,5 @@ class ArchConfigurator(configurator.ApacheConfigurator): version_cmd=['apachectl', '-v'], restart_cmd=['apachectl', 'graceful'], conftest_cmd=['apachectl', 'configtest'], - enmod=None, - dismod=None, - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, challenge_location="/etc/httpd/conf", - bin=None, ) diff --git a/certbot-apache/certbot_apache/_internal/override_centos.py b/certbot-apache/certbot_apache/_internal/override_centos.py index 98dc80e0b..c1a69885c 100644 --- a/certbot-apache/certbot_apache/_internal/override_centos.py +++ b/certbot-apache/certbot_apache/_internal/override_centos.py @@ -12,6 +12,7 @@ from certbot.errors import MisconfigurationError from certbot_apache._internal import apache_util from certbot_apache._internal import configurator from certbot_apache._internal import parser +from certbot_apache._internal.configurator import OsOptions logger = logging.getLogger(__name__) @@ -20,7 +21,7 @@ logger = logging.getLogger(__name__) class CentOSConfigurator(configurator.ApacheConfigurator): """CentOS specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( + OS_DEFAULTS = OsOptions( server_root="/etc/httpd", vhost_root="/etc/httpd/conf.d", vhost_files="*.conf", @@ -30,13 +31,7 @@ class CentOSConfigurator(configurator.ApacheConfigurator): restart_cmd=['apachectl', 'graceful'], restart_cmd_alt=['apachectl', 'restart'], conftest_cmd=['apachectl', 'configtest'], - enmod=None, - dismod=None, - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, challenge_location="/etc/httpd/conf.d", - bin=None, ) def config_test(self): @@ -77,12 +72,14 @@ class CentOSConfigurator(configurator.ApacheConfigurator): alternative restart cmd used in CentOS. """ super()._prepare_options() - cast(List[str], self.options["restart_cmd_alt"])[0] = self.option("ctl") + if not self.options.restart_cmd_alt: # pragma: no cover + raise ValueError("OS option restart_cmd_alt must be set for CentOS.") + self.options.restart_cmd_alt[0] = self.options.ctl def get_parser(self): """Initializes the ApacheParser""" return CentOSParser( - self.option("server_root"), self.option("vhost_root"), + self.options.server_root, self.options.vhost_root, self.version, configurator=self) def _deploy_cert(self, *args, **kwargs): # pylint: disable=arguments-differ diff --git a/certbot-apache/certbot_apache/_internal/override_darwin.py b/certbot-apache/certbot_apache/_internal/override_darwin.py index 106f5fbab..e1dca7f5e 100644 --- a/certbot-apache/certbot_apache/_internal/override_darwin.py +++ b/certbot-apache/certbot_apache/_internal/override_darwin.py @@ -3,26 +3,19 @@ import zope.interface from certbot import interfaces from certbot_apache._internal import configurator +from certbot_apache._internal.configurator import OsOptions @zope.interface.provider(interfaces.IPluginFactory) class DarwinConfigurator(configurator.ApacheConfigurator): """macOS specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( - server_root="/etc/apache2", + OS_DEFAULTS = OsOptions( vhost_root="/etc/apache2/other", vhost_files="*.conf", - logs_root="/var/log/apache2", ctl="apachectl", version_cmd=['apachectl', '-v'], restart_cmd=['apachectl', 'graceful'], conftest_cmd=['apachectl', 'configtest'], - enmod=None, - dismod=None, - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, challenge_location="/etc/apache2/other", - bin=None, ) diff --git a/certbot-apache/certbot_apache/_internal/override_debian.py b/certbot-apache/certbot_apache/_internal/override_debian.py index c5f3b6e1a..7f12f4bbc 100644 --- a/certbot-apache/certbot_apache/_internal/override_debian.py +++ b/certbot-apache/certbot_apache/_internal/override_debian.py @@ -10,6 +10,7 @@ from certbot.compat import filesystem from certbot.compat import os from certbot_apache._internal import apache_util from certbot_apache._internal import configurator +from certbot_apache._internal.configurator import OsOptions logger = logging.getLogger(__name__) @@ -18,22 +19,11 @@ logger = logging.getLogger(__name__) class DebianConfigurator(configurator.ApacheConfigurator): """Debian specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( - server_root="/etc/apache2", - vhost_root="/etc/apache2/sites-available", - vhost_files="*", - logs_root="/var/log/apache2", - ctl="apache2ctl", - version_cmd=['apache2ctl', '-v'], - restart_cmd=['apache2ctl', 'graceful'], - conftest_cmd=['apache2ctl', 'configtest'], + OS_DEFAULTS = OsOptions( enmod="a2enmod", dismod="a2dismod", - le_vhost_ext="-le-ssl.conf", handle_modules=True, handle_sites=True, - challenge_location="/etc/apache2", - bin=None, ) def enable_site(self, vhost): @@ -132,11 +122,11 @@ class DebianConfigurator(configurator.ApacheConfigurator): # Generate reversal command. # Try to be safe here... check that we can probably reverse before # applying enmod command - if not util.exe_exists(self.option("dismod")): + if not util.exe_exists(self.options.dismod): raise errors.MisconfigurationError( "Unable to find a2dismod, please make sure a2enmod and " "a2dismod are configured correctly for certbot.") self.reverter.register_undo_command( - temp, [self.option("dismod"), "-f", mod_name]) - util.run_script([self.option("enmod"), mod_name]) + temp, [self.options.dismod, "-f", mod_name]) + util.run_script([self.options.enmod, mod_name]) diff --git a/certbot-apache/certbot_apache/_internal/override_fedora.py b/certbot-apache/certbot_apache/_internal/override_fedora.py index 0f7970460..3b947a823 100644 --- a/certbot-apache/certbot_apache/_internal/override_fedora.py +++ b/certbot-apache/certbot_apache/_internal/override_fedora.py @@ -1,7 +1,4 @@ """ Distribution specific override class for Fedora 29+ """ -from typing import cast -from typing import List - import zope.interface from certbot import errors @@ -10,13 +7,14 @@ from certbot import util from certbot_apache._internal import apache_util from certbot_apache._internal import configurator from certbot_apache._internal import parser +from certbot_apache._internal.configurator import OsOptions @zope.interface.provider(interfaces.IPluginFactory) class FedoraConfigurator(configurator.ApacheConfigurator): """Fedora 29+ specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( + OS_DEFAULTS = OsOptions( server_root="/etc/httpd", vhost_root="/etc/httpd/conf.d", vhost_files="*.conf", @@ -26,13 +24,7 @@ class FedoraConfigurator(configurator.ApacheConfigurator): restart_cmd=['apachectl', 'graceful'], restart_cmd_alt=['apachectl', 'restart'], conftest_cmd=['apachectl', 'configtest'], - enmod=None, - dismod=None, - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, challenge_location="/etc/httpd/conf.d", - bin=None, ) def config_test(self): @@ -50,7 +42,7 @@ class FedoraConfigurator(configurator.ApacheConfigurator): def get_parser(self): """Initializes the ApacheParser""" return FedoraParser( - self.option("server_root"), self.option("vhost_root"), + self.options.server_root, self.options.vhost_root, self.version, configurator=self) def _try_restart_fedora(self): @@ -72,9 +64,11 @@ class FedoraConfigurator(configurator.ApacheConfigurator): of Fedora to restart httpd. """ super()._prepare_options() - cast(List[str], self.options["restart_cmd"])[0] = 'apachectl' - cast(List[str], self.options["restart_cmd_alt"])[0] = 'apachectl' - cast(List[str], self.options["conftest_cmd"])[0] = 'apachectl' + self.options.restart_cmd[0] = 'apachectl' + if not self.options.restart_cmd_alt: # pragma: no cover + raise ValueError("OS option restart_cmd_alt must be set for Fedora.") + self.options.restart_cmd_alt[0] = 'apachectl' + self.options.conftest_cmd[0] = 'apachectl' class FedoraParser(parser.ApacheParser): diff --git a/certbot-apache/certbot_apache/_internal/override_gentoo.py b/certbot-apache/certbot_apache/_internal/override_gentoo.py index 484e15532..1b86c925e 100644 --- a/certbot-apache/certbot_apache/_internal/override_gentoo.py +++ b/certbot-apache/certbot_apache/_internal/override_gentoo.py @@ -1,36 +1,23 @@ """ Distribution specific override class for Gentoo Linux """ -from typing import cast -from typing import List - import zope.interface from certbot import interfaces from certbot_apache._internal import apache_util from certbot_apache._internal import configurator from certbot_apache._internal import parser +from certbot_apache._internal.configurator import OsOptions @zope.interface.provider(interfaces.IPluginFactory) class GentooConfigurator(configurator.ApacheConfigurator): """Gentoo specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( + OS_DEFAULTS = OsOptions( server_root="/etc/apache2", vhost_root="/etc/apache2/vhosts.d", vhost_files="*.conf", - logs_root="/var/log/apache2", - ctl="apache2ctl", - version_cmd=['apache2ctl', '-v'], - restart_cmd=['apache2ctl', 'graceful'], restart_cmd_alt=['apache2ctl', 'restart'], - conftest_cmd=['apache2ctl', 'configtest'], - enmod=None, - dismod=None, - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, challenge_location="/etc/apache2/vhosts.d", - bin=None, ) def _prepare_options(self): @@ -39,12 +26,14 @@ class GentooConfigurator(configurator.ApacheConfigurator): alternative restart cmd used in Gentoo. """ super()._prepare_options() - cast(List[str], self.options["restart_cmd_alt"])[0] = self.option("ctl") + if not self.options.restart_cmd_alt: # pragma: no cover + raise ValueError("OS option restart_cmd_alt must be set for Gentoo.") + self.options.restart_cmd_alt[0] = self.options.ctl def get_parser(self): """Initializes the ApacheParser""" return GentooParser( - self.option("server_root"), self.option("vhost_root"), + self.options.server_root, self.options.vhost_root, self.version, configurator=self) @@ -69,7 +58,7 @@ 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"] + mod_cmd = [self.configurator.options.ctl, "modules"] matches = apache_util.parse_from_subprocess(mod_cmd, r"(.*)_module") for mod in matches: self.add_mod(mod.strip()) diff --git a/certbot-apache/certbot_apache/_internal/override_suse.py b/certbot-apache/certbot_apache/_internal/override_suse.py index afce98dfa..d692fd239 100644 --- a/certbot-apache/certbot_apache/_internal/override_suse.py +++ b/certbot-apache/certbot_apache/_internal/override_suse.py @@ -3,26 +3,21 @@ import zope.interface from certbot import interfaces from certbot_apache._internal import configurator +from certbot_apache._internal.configurator import OsOptions @zope.interface.provider(interfaces.IPluginFactory) class OpenSUSEConfigurator(configurator.ApacheConfigurator): """OpenSUSE specific ApacheConfigurator override class""" - OS_DEFAULTS = dict( - server_root="/etc/apache2", + OS_DEFAULTS = OsOptions( vhost_root="/etc/apache2/vhosts.d", vhost_files="*.conf", - logs_root="/var/log/apache2", ctl="apachectl", version_cmd=['apachectl', '-v'], restart_cmd=['apachectl', 'graceful'], conftest_cmd=['apachectl', 'configtest'], enmod="a2enmod", dismod="a2dismod", - le_vhost_ext="-le-ssl.conf", - handle_modules=False, - handle_sites=False, challenge_location="/etc/apache2/vhosts.d", - bin=None, ) diff --git a/certbot-apache/certbot_apache/_internal/parser.py b/certbot-apache/certbot_apache/_internal/parser.py index ff7e90f3b..141991ccc 100644 --- a/certbot-apache/certbot_apache/_internal/parser.py +++ b/certbot-apache/certbot_apache/_internal/parser.py @@ -81,7 +81,7 @@ class ApacheParser: # Must also attempt to parse additional virtual host root if vhostroot: self.parse_file(os.path.abspath(vhostroot) + "/" + - self.configurator.option("vhost_files")) + self.configurator.options.vhost_files) # check to see if there were unparsed define statements if version < (2, 4): @@ -282,7 +282,7 @@ class ApacheParser: def update_defines(self): """Updates the dictionary of known variables in the configuration""" - self.variables = apache_util.parse_defines(self.configurator.option("ctl")) + self.variables = apache_util.parse_defines(self.configurator.options.ctl) def update_includes(self): """Get includes from httpd process, and add them to DOM if needed""" @@ -292,7 +292,7 @@ class ApacheParser: # configuration files _ = self.find_dir("Include") - matches = apache_util.parse_includes(self.configurator.option("ctl")) + matches = apache_util.parse_includes(self.configurator.options.ctl) if matches: for i in matches: if not self.parsed_in_current(i): @@ -301,7 +301,7 @@ class ApacheParser: def update_modules(self): """Get loaded modules from httpd process, and add them to DOM""" - matches = apache_util.parse_modules(self.configurator.option("ctl")) + matches = apache_util.parse_modules(self.configurator.options.ctl) for mod in matches: self.add_mod(mod.strip()) diff --git a/certbot-apache/tests/configurator_test.py b/certbot-apache/tests/configurator_test.py index ad1f5f04d..8620955aa 100644 --- a/certbot-apache/tests/configurator_test.py +++ b/certbot-apache/tests/configurator_test.py @@ -103,9 +103,9 @@ class MultipleVhostsTest(util.ApacheTest): "handle_modules", "handle_sites", "ctl"] exp = {} - for k in ApacheConfigurator.OS_DEFAULTS: + for k in ApacheConfigurator.OS_DEFAULTS.__dict__.keys(): if k in parserargs: - exp[k.replace("_", "-")] = ApacheConfigurator.OS_DEFAULTS[k] + exp[k.replace("_", "-")] = getattr(ApacheConfigurator.OS_DEFAULTS, k) # Special cases exp["vhost-root"] = None @@ -128,14 +128,13 @@ class MultipleVhostsTest(util.ApacheTest): def test_all_configurators_defaults_defined(self): from certbot_apache._internal.entrypoint import OVERRIDE_CLASSES from certbot_apache._internal.configurator import ApacheConfigurator - parameters = set(ApacheConfigurator.OS_DEFAULTS.keys()) + parameters = set(ApacheConfigurator.OS_DEFAULTS.__dict__.keys()) for cls in OVERRIDE_CLASSES.values(): - self.assertTrue(parameters.issubset(set(cls.OS_DEFAULTS.keys()))) + self.assertTrue(parameters.issubset(set(cls.OS_DEFAULTS.__dict__.keys()))) def test_constant(self): self.assertTrue("debian_apache_2_4/multiple_vhosts/apache" in - self.config.option("server_root")) - self.assertEqual(self.config.option("nonexistent"), None) + self.config.options.server_root) @certbot_util.patch_get_utility() def test_get_all_names(self, mock_getutility): @@ -1774,7 +1773,7 @@ class InstallSslOptionsConfTest(util.ApacheTest): # ssl_module statically linked self.config._openssl_version = None self.config.parser.modules['ssl_module'] = None - self.config.options['bin'] = '/fake/path/to/httpd' + self.config.options.bin = '/fake/path/to/httpd' with mock.patch("certbot_apache._internal.configurator." "ApacheConfigurator._open_module_file") as mock_omf: mock_omf.return_value = some_string_contents @@ -1810,7 +1809,7 @@ class InstallSslOptionsConfTest(util.ApacheTest): # When ssl_module is statically linked but --apache-bin not provided self.config._openssl_version = None - self.config.options['bin'] = None + self.config.options.bin = None self.config.parser.modules['ssl_module'] = None with mock.patch("certbot_apache._internal.configurator.logger.warning") as mock_log: self.assertEqual(self.config.openssl_version(), None) diff --git a/certbot-apache/tests/parser_test.py b/certbot-apache/tests/parser_test.py index 37d4eb782..e30eca153 100644 --- a/certbot-apache/tests/parser_test.py +++ b/certbot-apache/tests/parser_test.py @@ -305,11 +305,9 @@ class BasicParserTest(util.ParserTest): self.assertRaises( errors.PluginError, self.parser.update_runtime_variables) - @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.option") @mock.patch("certbot_apache._internal.apache_util.subprocess.Popen") - def test_update_runtime_vars_bad_ctl(self, mock_popen, mock_opt): + def test_update_runtime_vars_bad_ctl(self, mock_popen): mock_popen.side_effect = OSError - mock_opt.return_value = "nonexistent" self.assertRaises( errors.MisconfigurationError, self.parser.update_runtime_variables) diff --git a/certbot-apache/tests/util.py b/certbot-apache/tests/util.py index bd522d736..a0b44d188 100644 --- a/certbot-apache/tests/util.py +++ b/certbot-apache/tests/util.py @@ -123,11 +123,11 @@ def get_apache_configurator( version=version, use_parsernode=use_parsernode, openssl_version=openssl_version) if not conf_vhost_path: - config_class.OS_DEFAULTS["vhost_root"] = 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.config.apache_ctl = config_class.OS_DEFAULTS.ctl config.prepare() return config |