Repository: certbot/certbot
Pull request: If an installer is provided to certonly, restart after cert issuance
Fixes #8912. I wasn't sure if any docs needed updating for this, since it sounded like this change was making `--nginx` and `--apache` behave closer to how they're described in the docs in the `certonly` case. Let me know if that's not the case!

## Pull Request Checklist

- [x] If the change being made is to a [distributed component](https://certbot.eff.org/docs/contributing.html#code-components-and-layout), edit the `master` section of `certbot/CHANGELOG.md` to include a description of the change being made.
- [x] Add or update any documentation as needed to support the changes in this PR.
- [x] Include your name in `AUTHORS.md` if you like.


Problem statement:
Should certonly call `restart` at the end if the installer is set? And/or if no installer is set but the authenticator is also an installer?
Based on an ad-hoc user test from a user who was upgrading to an ACMEv2-compatible version of certbot after using an old version entombed in a docker distribution.

Since the user already had their nginx config set up, they only wanted to renew an existing cert, not install it, so they used `certonly` and followed the prompts to renew the existing cert. But they had to restart nginx after getting the cert, despite using nginx for the authenticator, which was unexpected and made it seem like certbot might not have worked somehow.

They'll be fine on renewal since `--nginx` does set an installer and so will restart nginx despite having run `certonly` (is this weird and unexpected?), but given that we do know that `nginx -s reload` is successful because we literally just called it to get the cert, should we just call it anyway? To me, this doesn't make sense in theory due to our internal auth/installer splits, but if it's a common-enough use case (which I suspect it might be), it probably makes sense in practice for users who shouldn't need to fully understand our internal structure, especially if they're just following the instruction generator instructions.

Alternatively, we could just throw a line in the instruction generator steps about this, to the effect of "this will update the files but you will need to modify or reload where you have them installed to pick up the files". Although that is a little specific to this case, and would be confusing to users who haven't yet installed the certificate, since it does assume certs are already installed. We could try to catch this case early and tell users that they can just run `renew` if they already have certs from an old installation, but I'd personally prefer more branching in code and less in user-facing instructions, especially since we do try to catch all the ways users might run certbot and make them "just work."

Leaving this untagged so people see it during triage in case I forget to bring it up for discussion at the weekly meeting.

Hints from issue discussion:
> They'll be fine on renewal since `--nginx` does set an installer and so will restart nginx despite having run `certonly` (is this weird and unexpected?)

Personally, I think this is very weird. Not everyone knows the `-a` and `-i` options and it's very much possible to get a certificate for a hostname for which a VirtualHost does **not** exist. Think SMTP or IMAP servers. (At least, I can perfectly get certs with the `--apache` plugin without actually having that hostname in my Apache configuration.. Not sure if that's by design..)
So it wouldn't make sense *at all* if someone gets a certificate with `certonly`, possibly not even used in Apache, but using the `apache` authenticator with the `--apache` option, because they didn't know the `-a` option (without `-i`).
I would say: certonly? Then don't set an installer.
I'd personally be OK with having `certonly` call `restart` if the installer is set.

I understand that this is kind of weird with our mental model of Certbot, but that weirdness arguably already exists with commands like `certbot certonly --nginx` setting an installer that is used by `certbot renew`. I think us matching this behavior with `certonly` is reasonable enough and should essentially completely resolve this issue with little to no downside.

I think this issue essentially only affects our own plugins because third party plugins currently cannot have flags like `--nginx`. See https://github.com/certbot/certbot/issues/1549. Because of this, 3rd party plugins will only be affected if the user is explicitly setting an installer. If they are and it's already going to be used with `renew` anyway, calling `restart` initially with `certonly` to have consistent behavior and potentially catch renewal problems earlier seems fine to me personally.

For our own plugins, our only installer plugins are for Apache and nginx and both do graceful restarts with no downtime. In essentially all cases, I'd expect Apache or nginx to have been used as the authenticator too because we tell people to use the `--apache` and `--nginx` flags and in that case we've already restarted the web server successfully in both cases. Adding another `restart` call after saving the certificate doesn't seem to me like it should cause any problems and it'd automatically avoid issues like the one described in the initial post.
Great points, thanks, I agree. In general, between matching our mental model of certbot and the user's expectations, the latter is more important. This is a step towards making our UX more seamless.

Commit message: fix trailing whitespace

Patch to review:
diff --git a/AUTHORS.md b/AUTHORS.md
index d82ddcb7688..377ba5d0d79 100644
--- a/AUTHORS.md
+++ b/AUTHORS.md
@@ -273,6 +273,7 @@ Authors
 * [Wilfried Teiken](https://github.com/wteiken)
 * [Willem Fibbe](https://github.com/fibbers)
 * [William Budington](https://github.com/Hainish)
+* [Will Greenberg](https://github.com/wgreenberg)
 * [Will Newby](https://github.com/willnewby)
 * [Will Oller](https://github.com/willoller)
 * [Yan](https://github.com/diracdeltas)
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 4d2ae2b7002..b406fd1087d 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -45,6 +45,9 @@ More details about these changes can be found on our GitHub repo.
 
 ## 1.23.0 - 2022-02-08
 
+* When `certonly` is run with an installer specified (e.g.  `--nginx`),
+  `certonly` will now also run `restart` for that installer
+
 ### Added
 
 * Added `show_account` subcommand, which will fetch the account information
diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py
index 9d63be23acc..afddce8a797 100644
--- a/certbot/certbot/_internal/main.py
+++ b/certbot/certbot/_internal/main.py
@@ -534,12 +534,21 @@ def _report_next_steps(config: configuration.NamespaceConfig, installer_err: Opt
 
     # If the installation or enhancement raised an error, show advice on trying again
     if installer_err:
-        steps.append(
-            "The certificate was saved, but could not be installed (installer: "
-            f"{config.installer}). After fixing the error shown below, try installing it again "
-            f"by running:\n  {cli.cli_command} install --cert-name "
-            f"{_cert_name_from_config_or_lineage(config, lineage)}"
-        )
+        # Special case where either --nginx or --apache were used, causing us to
+        # run the "installer" (i.e. reloading the nginx/apache config)
+        if config.verb == 'certonly':
+            steps.append(
+                "The certificate was saved, but was not successfully loaded by the installer"
+                f"({config.installer}) due to the installer failing to reload."
+                f"After fixing the error shown below, try reloading {config.installer} manually."
+            )
+        else:
+            steps.append(
+                "The certificate was saved, but could not be installed (installer: "
+                f"{config.installer}). After fixing the error shown below, try installing it again "
+                f"by running:\n  {cli.cli_command} install --cert-name "
+                f"{_cert_name_from_config_or_lineage(config, lineage)}"
+            )
 
     # If a certificate was obtained or renewed, show applicable renewal advice
     if new_or_renewed_cert:
@@ -1559,11 +1568,21 @@ def certonly(config: configuration.NamespaceConfig, plugins: plugins_disco.Plugi
 
     lineage = _get_and_save_cert(le_client, config, domains, certname, lineage)
 
+    # If a new cert was issued and we were passed an installer, we can safely
+    # run `installer.restart()` to load the newly issued certificate
+    installer_err: Optional[errors.Error] = None
+    if lineage and installer and not config.dry_run:
+        logger.info("Reloading % server after certificate issuance", config.installer)
+        try:
+            installer.restart()
+        except errors.Error as e:
+            installer_err = e
+
     cert_path = lineage.cert_path if lineage else None
     fullchain_path = lineage.fullchain_path if lineage else None
     key_path = lineage.key_path if lineage else None
     _report_new_cert(config, cert_path, fullchain_path, key_path)
-    _report_next_steps(config, None, lineage,
+    _report_next_steps(config, installer_err, lineage,
                        new_or_renewed_cert=should_get_cert and not config.dry_run)
     _suggest_donation_if_appropriate(config)
     eff.handle_subscription(config, le_client.account)
diff --git a/certbot/certbot/_internal/plugins/selection.py b/certbot/certbot/_internal/plugins/selection.py
index f62db40896c..7404dbac94c 100644
--- a/certbot/certbot/_internal/plugins/selection.py
+++ b/certbot/certbot/_internal/plugins/selection.py
@@ -249,6 +249,13 @@ def choose_configurator_plugins(config: configuration.NamespaceConfig,
             installer = pick_installer(config, req_inst, plugins, installer_question)
         if need_auth:
             authenticator = pick_authenticator(config, req_auth, plugins)
+
+    # As a special case for certonly, if the user interactively selected nginx
+    # or apache as an authenticator, make sure we set the appropriate installer
+    # to allow us to safely reload after retrieving a cert
+    if verb == "certonly" and authenticator and not installer:
+        if authenticator.name in ("nginx", "apache"):
+            installer = cast(Optional[interfaces.Installer], authenticator)
     logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
 
     # Report on any failures
diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py
index d75b6ad58f0..fb378e87f5a 100644
--- a/certbot/tests/main_test.py
+++ b/certbot/tests/main_test.py
@@ -304,6 +304,36 @@ def test_dryrun_next_steps_no_cert_saved(self, mock_lineage, mock_csr_get_cert,
                 mock.ANY, mock.ANY, mock.ANY, new_or_renewed_cert=False)
             mock_report_next_steps.reset_mock()
 
+    @mock.patch('certbot._internal.main._report_next_steps')
+    @mock.patch('certbot._internal.main._report_new_cert')
+    @mock.patch('certbot._internal.main._find_cert')
+    @mock.patch('certbot._internal.main._get_and_save_cert')
+    @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins')
+    def test_installer_runs_restart(self, mock_sel, mock_get_cert, mock_find_cert,
+                                    unused_report_new, unused_report_next):
+        mock_installer = mock.MagicMock()
+        mock_sel.return_value = (mock_installer, None)
+        mock_get_cert.return_value = mock.MagicMock()
+        mock_find_cert.return_value = (True, None)
+
+        self._call('certonly --nginx -d example.com'.split())
+        mock_installer.restart.assert_called_once()
+
+    @mock.patch('certbot._internal.main._report_next_steps')
+    @mock.patch('certbot._internal.main._report_new_cert')
+    @mock.patch('certbot._internal.main._find_cert')
+    @mock.patch('certbot._internal.main._get_and_save_cert')
+    @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins')
+    def test_dryrun_installer_doesnt_restart(self, mock_sel, mock_get_cert, mock_find_cert,
+                                             unused_report_new, unused_report_next):
+        mock_installer = mock.MagicMock()
+        mock_sel.return_value = (mock_installer, None)
+        mock_get_cert.return_value = mock.MagicMock()
+        mock_find_cert.return_value = (True, None)
+
+        self._call('certonly --nginx -d example.com --dry-run'.split())
+        mock_installer.restart.assert_not_called()
+
 
 class FindDomainsOrCertnameTest(unittest.TestCase):
     """Tests for certbot._internal.main._find_domains_or_certname."""


Code review comments:
diff_hunk: @@ -534,12 +534,21 @@ def _report_next_steps(config: configuration.NamespaceConfig, installer_err: Opt
 
     # If the installation or enhancement raised an error, show advice on trying again
     if installer_err:
-        steps.append(
-            "The certificate was saved, but could not be installed (installer: "
-            f"{config.installer}). After fixing the error shown below, try installing it again "
-            f"by running:\n  {cli.cli_command} install --cert-name "
-            f"{_cert_name_from_config_or_lineage(config, lineage)}"
-        )
+        # Special case where either --nginx or --apache were used, causing us to
+        # run the "installer" (i.e. reloading the nginx/apache config)
+        if config.verb == 'certonly':
+            steps.append(
+                "The certificate was saved, but was not successfully loaded by the installer"
+                f"({config.installer}) due to the installer failing to reload."
path: certbot/certbot/_internal/main.py
text: @user1:
```suggestion
                "The certificate was saved, but was not successfully loaded by the installer "
                f"({config.installer}) due to the installer failing to reload. "
```

diff_hunk: @@ -249,6 +249,13 @@ def choose_configurator_plugins(config: configuration.NamespaceConfig,
             installer = pick_installer(config, req_inst, plugins, installer_question)
         if need_auth:
             authenticator = pick_authenticator(config, req_auth, plugins)
+
+    # As a special case for certonly, if the user interactively selected nginx
+    # or apache as an authenticator, make sure we set the appropriate installer
+    # to allow us to safely reload after retrieving a cert
+    if verb == "certonly" and authenticator and not installer:
+        if authenticator.name in ("nginx", "apache"):
+            installer = cast(Optional[interfaces.Installer], authenticator)
path: certbot/certbot/_internal/plugins/selection.py
text: @user1:
This also means that:

     certbot certonly -d example.com --authenticator nginx --installer none

results in an `installer` being set in ` /etc/letsencrypt/renewal/example.com.conf` anyway.

Do we want this to happen? Is there any difference between `certonly` pretending there's an `installer` set, and  forcefully setting an `installer` like this PR does?

If the latter, I think we will also need to reword the CHANGELOG to reflect the change being made here.

@user2:
Ah nice catch, unfortunately I do think we ideally wouldn't have that behavior, since I feel like specifying `-i none` would only be done when the user specifically does not want any installer stuff to happen at all, like renewing, and maybe in that case we shouldn't even install the cert in the first place. Unless there's some other reason I'm missing why someone would specify `-i none`?

That being said, detecting that case would require differentiating between the default value and `none` set on the cli, which last I checked is a pretty hacky bug-prone part of the codebase. Maybe it's gotten better since then or maybe it's sufficiently smoothed over by higher-level functions these days that hide the messiness, but I think my sense of what decision to make depends on the cost of bringing that code in.

@author:
i wonder if we could get away with handling just these two cases, which i think are easier to detect from `config`:

1. User used `--nginx-`/`--apache`/`--configurator nginx/apache` in the CLI, in which case we set the installer for certonly
2. User used `--authenticator nginx/apache` but didn't set an installer, in which case we assume they did so intentionally and leave the installer blank

@user2:
Yeah, `config` is what I meant by the bug-prone area of the codebase; it's detecting case 2 vs explicitly set it to `none` that might be tricky. It might not though! Those are in fact the relevant cases, if you want to go ahead and see how icky it'll be it's at least best to know so we can decide if it's worth doing.

@author:
ah fwiw i was considering `--authenticator nginx --installer none` and `--authenticator nginx` to both be case 2. if that's acceptable, i think this works:

```
    # As a special case for certonly, if a user selected apache or nginx, set
    # the relevant installer (unless the user specifically specified no
    # installer)
    if verb == "certonly":
        # user specified --nginx or --apache on CLI
        selected_configurator = config.nginx or config.apache
        # user didn't request an authenticator, and so interactively chose nginx
        # or apache
        interactively_selected = req_auth is None and authenticator.name in ("nginx", "apache")

        if selected_configurator or interactively_selected:
            installer = cast(Optional[interfaces.Installer], authenticator)
```

@user1:
Gave that a quick test and it seems to do just what we want. Nice!

diff_hunk: @@ -1559,11 +1568,21 @@ def certonly(config: configuration.NamespaceConfig, plugins: plugins_disco.Plugi
 
     lineage = _get_and_save_cert(le_client, config, domains, certname, lineage)
 
+    # If a new cert was issued and we were passed an installer, we can safely
+    # run `installer.restart()` to load the newly issued certificate
+    installer_err: Optional[errors.Error] = None
+    if lineage and installer and not config.dry_run:
+        logger.info("Reloading % server after certificate issuance", config.installer)
path: certbot/certbot/_internal/main.py
text: @user1:
```suggestion
        logger.info("Reloading %s server after certificate issuance", config.installer)
```