Actually set FAILED_DOMAINS and RENEWED_DOMAINS variables when renewals fail (#10347)

Fixes https://github.com/certbot/certbot/issues/10259

This PR moves post-hook execution from `main.renew` to
`renewal.handle_renewal_request` so that failed and renewed domains
actually get passed into post-hook execution as promised, even when
failures happened.

I suspect the original PR was being overly cautious by putting the whole
thing into a try/finally so that post-hooks definitely happen, but
`handle_renewal_request` is already full of exception catching. I
understand the worry about executing a pre-hook and then failing to
execute its matching post-hook, but the code really is already
structured to make sure that that won't happen. And then when we added
`FAILED_DOMAINS` and `RENEWED_DOMAINS`, we both kept that
overly-cautious hooks execution location, but also kept the error so we
have a summary at the end...which meant that if failures happened, the
env vars were never set.

If we really want to keep the `hooks.run_saved_post_hooks` call on the
outside of everything in main, we can, but then we will have to do one
of the following:
- pass in the output lists to be filled out during execution. not my
favorite pattern
- throw the output lists in the error object or make a wrapper error,
not sure, haven't looked at `errors.py` too closely
- stop raising that final error where we report failures at the very
bottom. it's a little outdated maybe but I do like it and I think people
are used to it
- raise that error in main, returning the number of parse and renewal
failures. this is my favorite of the options, but I still like it less
than what I've implemented here.

Here's the integration/regression test failing on main:

https://dev.azure.com/certbot/certbot/_build/results?buildId=9237&view=logs&j=fca58cec-e7ce-563a-f36f-5c233894d750
You can see here that that branch just has the integration test without
the fix (and removing other tests for efficiency):
https://github.com/certbot/certbot/compare/main...test-fail-env-on-main

It's the default, but just to be clear, this should definitely have two
reviewers.

certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py

@@ -442,6 +442,40 @@ def test_renew_hook_override(context: IntegrationTestsContext) -> None:
     assert_hook_execution(context.hook_probe, 'deploy_override')
 
 
+def test_renew_hook_env_vars(context: IntegrationTestsContext) -> None:
+    fail_domain = context.get_domain('fail-env')
+    context.certbot([
+        'certonly', '-d', fail_domain,
+        '--preferred-challenges', 'http-01'
+    ])
+
+    context.certbot([
+        'renew',
+        '--post-hook', f'printenv RENEWED_DOMAINS >> {context.hook_probe}'
+    ])
+
+    assert_hook_execution(context.hook_probe, fail_domain)
+
+    # Clear probe
+    with open(context.hook_probe, 'w'):
+        pass
+
+    # now renew using manual dns, which will fail on renew
+    # manual_dns_auth_hook from misc is designed to fail if the domain contains 'fail-*'.
+    with pytest.raises(subprocess.CalledProcessError):
+        context.certbot([
+            'renew', '--cert-name', fail_domain,
+            '--preferred-challenges', 'dns',
+            '--manual-auth-hook', context.manual_dns_auth_hook,
+            '--manual-cleanup-hook', context.manual_dns_cleanup_hook,
+            '-a', 'manual',
+            '--post-hook', f'printenv FAILED_DOMAINS >> {context.hook_probe}',
+            '--dry-run', # use dry run here to deactivate previous authz, or this will pass
+        ])
+
+    assert_hook_execution(context.hook_probe, fail_domain)
+
+
 def test_invalid_domain_with_dns_challenge(context: IntegrationTestsContext) -> None:
     """Test certificate issuance failure with DNS-01 challenge."""
     # Manual dns auth hooks from misc are designed to fail if the domain contains 'fail-*'.

certbot/CHANGELOG.md

@@ -14,7 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Previously, we claimed to set FAILED_DOMAINS and RENEWED_DOMAINS env variables for use by
+  post-hooks when certificate renewals fail, but we were not actually setting them. Now, we are.
 
 More details about these changes can be found on our GitHub repo.
 

certbot/src/certbot/_internal/main.py

@@ -1620,13 +1620,7 @@ def renew(config: configuration.NamespaceConfig,
     :rtype: None
 
     """
-
+    renewal.handle_renewal_request(config)
-    renewed_domains: List[str] = []
-    failed_domains: List[str] = []
-    try:
-        renewed_domains, failed_domains = renewal.handle_renewal_request(config)
-    finally:
-        hooks.run_saved_post_hooks(renewed_domains, failed_domains)
 
 
 def make_or_verify_needed_dirs(config: configuration.NamespaceConfig) -> None:

certbot/src/certbot/_internal/renewal.py

@@ -14,7 +14,6 @@ from typing import Iterable
 from typing import List
 from typing import Mapping
 from typing import Optional
-from typing import Tuple
 from typing import Union
 
 from cryptography.hazmat.backends import default_backend
@@ -553,7 +552,7 @@ def _renew_describe_results(config: configuration.NamespaceConfig, renew_success
     notify(display_obj.SIDE_FRAME)
 
 
-def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list, list]:
+def handle_renewal_request(config: configuration.NamespaceConfig) -> None:
     """Examine each lineage; renew if due and report results"""
 
     # This is trivially False if config.domains is empty
@@ -596,6 +595,7 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list,
     for renewal_file in conf_files:
         display_util.notification("Processing " + renewal_file, pause=False)
         lineage_config = copy.deepcopy(config)
+        assert renewal_file.endswith(".conf") # make sure lineagename_for_filename will not error
         lineagename = storage.lineagename_for_filename(renewal_file)
 
         # Note that this modifies config (to add back the configuration
@@ -659,14 +659,14 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list,
     _renew_describe_results(config, renew_successes, renew_failures,
                             renew_skipped, parse_failures)
 
+    hooks.run_saved_post_hooks(renewed_domains, failed_domains)
+
     if renew_failures or parse_failures:
         raise errors.Error(
             f"{len(renew_failures)} renew failure(s), {len(parse_failures)} parse failure(s)")
 
     logger.debug("no renewal failures")
 
-    return (renewed_domains, failed_domains)
-
 
 def _update_renewal_params_from_key(key_path: str, config: configuration.NamespaceConfig) -> None:
     with open(key_path, 'rb') as file_h: