certbot-apache: use httpd by default for CentOS/RHEL #9402

wgreenberg · 2022-09-09T00:07:10Z

CentOS and RHEL 9 now use httpd instead of apachectl, so that's now the default for that configurator. There's now a special case for older CentOS/RHEL versions to use apachectl, as well as for older Fedora (<28).

ohemorange · 2022-09-13T01:01:49Z

Code looks solid, have you already tested it on CentOS/RHEL 8/9 in practice?

notroj · 2022-09-14T10:12:03Z

This seems like it will now leave self.options.restart_cmd_alt[0] set to "httpd" in some cases, which looks wrong, since httpd restart is not a valid way to invoke httpd. (Though I am not sure how certbot uses restart_cmd_alt so may be missing something)

On all versions of CentOS, RHEL and Fedora it is fine to restart httpd using apachectl restart.

The special case needed is if you want to do something other than control the daemon, e.g. httpd -t or httpd -v in some form, you should run httpd directly.

wgreenberg · 2022-09-19T19:51:38Z

Ah, thanks for the clarification @notroj, turns out I misunderstood the RHEL changelog! Updating the PR

A change in RHEL 9 is causing apachectl to error out when used with additional arguments, resulting in certbot errors. The CentOS configurator now uses httpd instead for RHEL 9 (and later) derived distros.

bmw · 2022-09-30T01:21:09Z

certbot-apache/certbot_apache/_internal/override_centos.py

+        ]
+        new = util.parse_loose_version(os_version) >= util.parse_loose_version('9')
+        if rhel_derived and new:
+            self.options.ctl = 'httpd'


I think this sets the value of --apache-ctl (see certbot --help apache) with no way for the user to override it while continuing to list the default value as apachectl in the help output on RHEL 9+. Is that right?

If so, I think we may want to do something like what we do here and define a new override class with static defaults.

What do you think?

true, though i was hoping to leave all the finicky distro-specific details out of get_configurator() and instead handle it within CentOSConfigurator.

one issue w/ relying on static defaults is that _prepare_options() always sets options.restart_cmd's binary to options.ctl (https://github.com/certbot/certbot/blob/master/certbot-apache/certbot_apache/_internal/configurator.py#L184-L187), so even if our CentOS 9 defaults have httpd for one and apachectl for the other, they'll both end up as httpd unless we intervene. what about something like:

diff --git a/certbot-apache/certbot_apache/_internal/configurator.py b/certbot-apache/certbot_apache/_internal/configurator.py index 82b3dcaec..8d7d4eb3a 100644 --- a/certbot-apache/certbot_apache/_internal/configurator.py +++ b/certbot-apache/certbot_apache/_internal/configurator.py @@ -178,14 +178,14 @@ class ApacheConfigurator(common.Configurator): # Config options use dashes instead of underscores if self.conf(o.replace("_", "-")) is not None: setattr(self.options, o, self.conf(o.replace("_", "-"))) + # If the user overrides apachectl, update the various cmds as well + if o == "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 else: setattr(self.options, o, getattr(self.OS_DEFAULTS, o)) - # Special cases - self.options.version_cmd[0] = self.options.ctl - self.options.restart_cmd[0] = self.options.ctl - self.options.conftest_cmd[0] = self.options.ctl

this way, we can just give CentOS 9 a different set of OS_DEFAULTS and leave its _prepare_options() alone

Hmm. Digging into this further, I noticed that we also have an --apache-bin option:

$ certbot --help apache usage: certbot [SUBCOMMAND] [options] [-d DOMAIN] [-d DOMAIN] ... ... --apache-ctl APACHE_CTL Full path to Apache control script (default: apachectl) --apache-bin APACHE_BIN Full path to apache2/httpd binary (default: None)

I feel like in the long term if not now, we should make use of this here instead of telling our code that the name of apachectl is httpd. There are different ways to structure this, but I personally think we should try to keep these two options true to their intended usage and update other commands based on their values. By that I mean I'm essentially imagining logic that at least effectively does something like:

if self.options.version_cmd[0] == self.OS_DEFAULTS.ctl: self.options.version_cmd[0] = self.options.ctl else: self.options.version_cmd[0] = self.options.bin

What do you think?

Also, on RHEL 9+ and friends, are these values from higher up in this file correct? Do we want to be using apachectl at all?

restart_cmd=['apachectl', 'graceful'], restart_cmd_alt=['apachectl', 'restart'], conftest_cmd=['apachectl', 'configtest'],

I notice we leave them unmodified.

Also, on RHEL 9+ and friends, are these values from higher up in this file correct? Do we want to be using apachectl at all?

Authoritatively: yes, you should use apachectl restart or graceful to restart the daemon on all versions of RHEL, and will be supported in all future versions. This is supported as a thin wrapper around systemctl $action httpd to manipulate the systemd service. apachectl configtest should also work for all versions since it's so widely used. The thing we don't support is passing arguments in.

I went w/ @bmw's suggestion and reworked this to use two different configurator classes for the different scenarios (i.e. older CentOSy OSes vs. new RHEL-derived OSes). this ended up adding some more complexity to get_configurator(), but in the end i think it makes more sense this way.

@wgreenberg, sorry for the conflicting suggestions between my first and second comments in the thread here. What do you think about what I said in that second post though at #9402 (comment) about using the apache_bin option instead of configuring our code to believe that the name to the Apache control script is httpd?

bmw

This is getting close! Based on your answers to some of my inline questions, I think we may want to consider keeping a single CentOS override class and doing the command overriding conditionally by inspecting the OS again like you did in the initial version of this PR. I'm not tied to this specific design, although I do think it may allow us to simplify/undo some of the other changes here.

I think we should also add a changelog entry about this but other than that and my inline comments, this LGTM!

bmw · 2022-10-26T16:40:32Z

certbot-apache/certbot_apache/_internal/override_centos.py

+        As of RHEL 9, apachectl can't be passed flags like "-t -D", so instead
+        use options.bin (i.e. httpd)
+        """
+        if not self.options.bin:


Should we call super()._override_cmds() here so values like self.options.restart_cmd[0] get set based on the ctl value and then in addition set self.options.version_cmd[0] = self.options.bin below?

bmw · 2022-10-26T16:41:37Z

certbot-apache/certbot_apache/_internal/override_centos.py

+        super()._prepare_options()
+        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


Do we want to do this for RHEL 9+? If not, do you know what changed between new and old versions?

bmw · 2022-10-26T17:03:49Z

certbot-apache/certbot_apache/_internal/entrypoint.py

    "amazon": override_centos.CentOSConfigurator,
    "gentoo": override_gentoo.GentooConfigurator,
    "gentoo base system": override_gentoo.GentooConfigurator,
    "opensuse": override_suse.OpenSUSEConfigurator,
    "suse": override_suse.OpenSUSEConfigurator,
    "sles": override_suse.OpenSUSEConfigurator,
-    "scientific": override_centos.CentOSConfigurator,
-    "scientific linux": override_centos.CentOSConfigurator,


I worry about removing entries from this dict due to the code at

certbot/certbot-apache/certbot_apache/_internal/entrypoint.py

Lines 60 to 63 in 529942f

os_like = util.get_systemd_os_like()

if os_like:

for os_name in os_like:

override_class = OVERRIDE_CLASSES.get(os_name)

What do you think?

This reverts commit aecb269.

wgreenberg · 2022-10-26T21:17:58Z

@bmw i reworked this into a single class, ready for review again!

bmw · 2022-10-26T21:25:56Z

certbot-apache/certbot_apache/_internal/override_centos.py

+            self.options.get_includes_cmd[0] = self.options.bin
+            self.options.get_defines_cmd[0] = self.options.bin
+        else:
+            super()._override_cmds()


I think we may want to unconditionally call this at the top of this function so values like self.options.restart_cmd[0] get set to a possibly custom self.options.ctl value. Otherwise, this PR LGTM!

alexzorin · 2022-10-27T00:34:45Z

CHANGELOG entry needs to be moved from 1.25.0 to 1.32.0.

wgreenberg · 2022-10-27T01:47:30Z

d'oh, good catch @alexzorin. fixed here: #9444

bmw · 2022-10-27T17:09:28Z

Yeah thanks for catching this Alex. This recurring issue motivates me a bit to take another look at #8272.

wgreenberg changed the title ~~certbot-apache: use httpd by default for CentOS et al~~ certbot-apache: use httpd by default for CentOS/RHEL Sep 9, 2022

ohemorange added area: fedora / redhat / CentOS area: apache labels Sep 13, 2022

certbot-apache: use httpd for newer RHEL derived distros

6a7d876

A change in RHEL 9 is causing apachectl to error out when used with additional arguments, resulting in certbot errors. The CentOS configurator now uses httpd instead for RHEL 9 (and later) derived distros.

wgreenberg force-pushed the fix-9386 branch from 36cebb1 to 6a7d876 Compare September 22, 2022 20:27

bmw self-assigned this Sep 29, 2022

bmw reviewed Sep 30, 2022

View reviewed changes

wgreenberg added 5 commits October 7, 2022 16:25

Rework CentOS configurator into two subclasses

aecb269

Fix linting/coverage issues

7742f69

Another rework: use the apache_bin option for RHEL 9+

bd95659

mypy fixup

41edb76

exclude edge case from coverage

e207537

bmw requested changes Oct 26, 2022

View reviewed changes

wgreenberg added 3 commits October 26, 2022 13:34

Revert "Rework CentOS configurator into two subclasses"

7e6d806

This reverts commit aecb269.

Single CentOS class which uses the apache_bin option

1162fd3

soothe mypy

5a9c38a

bmw reviewed Oct 26, 2022

View reviewed changes

Always call super()._override_cmds()

b32ba26

bmw approved these changes Oct 26, 2022

View reviewed changes

wgreenberg merged commit eed1afb into master Oct 26, 2022

wgreenberg deleted the fix-9386 branch October 26, 2022 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

certbot-apache: use httpd by default for CentOS/RHEL #9402

certbot-apache: use httpd by default for CentOS/RHEL #9402

wgreenberg commented Sep 9, 2022

ohemorange commented Sep 13, 2022

notroj commented Sep 14, 2022

wgreenberg commented Sep 19, 2022

bmw Sep 30, 2022

wgreenberg Oct 5, 2022 •

edited

bmw Oct 6, 2022

notroj Oct 7, 2022

wgreenberg Oct 7, 2022 •

edited

bmw Oct 12, 2022

bmw left a comment •

edited

bmw Oct 26, 2022

bmw Oct 26, 2022

bmw Oct 26, 2022

wgreenberg commented Oct 26, 2022

bmw Oct 26, 2022

wgreenberg Oct 26, 2022

alexzorin commented Oct 27, 2022

wgreenberg commented Oct 27, 2022

bmw commented Oct 27, 2022

	os_like = util.get_systemd_os_like()
	if os_like:
	for os_name in os_like:
	override_class = OVERRIDE_CLASSES.get(os_name)

certbot-apache: use httpd by default for CentOS/RHEL #9402

certbot-apache: use httpd by default for CentOS/RHEL #9402

Conversation

wgreenberg commented Sep 9, 2022

ohemorange commented Sep 13, 2022

notroj commented Sep 14, 2022

wgreenberg commented Sep 19, 2022

Choose a reason for hiding this comment

wgreenberg Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgreenberg Oct 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgreenberg commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzorin commented Oct 27, 2022

wgreenberg commented Oct 27, 2022

bmw commented Oct 27, 2022

wgreenberg Oct 5, 2022 •

edited

wgreenberg Oct 7, 2022 •

edited

bmw left a comment •

edited