Clean up renew_before_expiry default behavior (#10306)

[Recent changes](https://github.com/certbot/certbot/pull/10272/) to
`renewal.should_autorenew` assumed that if
`RenewableCert.configuration.renew_before_expiry` was set, that means
the user set it. That's wasn't true; we were throwing in a default value
if the user didn't set it. But there's no reason for that, especially
since we now set the default renewal time dynamically. Also, we were
writing out a commented `# renew_before_expiry = 30 days` without any
further documentation, in a file that we tell users they [shouldn't
really be
editing](https://eff-certbot.readthedocs.io/en/latest/using.html#modifying-the-renewal-configuration-file).
We now do neither of those things.

certbot/src/certbot/_internal/constants.py

@@ -153,11 +153,6 @@ QUIET_LOGGING_LEVEL = logging.ERROR
 DEFAULT_LOGGING_LEVEL = logging.WARNING
 """Default logging level to use when not in quiet mode."""
 
-RENEWER_DEFAULTS = {
-    "renew_before_expiry": "30 days",
-}
-"""Defaults for `certbot renew`."""
-
 ARCHIVE_DIR = "archive"
 """Archive directory, relative to `certbot.configuration.NamespaceConfig.config_dir`."""
 

certbot/src/certbot/_internal/storage.py

@@ -31,7 +31,6 @@ from certbot import errors
 from certbot import interfaces
 from certbot import ocsp
 from certbot import util
-from certbot._internal import constants
 from certbot._internal import error_handler
 from certbot._internal.plugins import disco as plugins_disco
 from certbot.compat import filesystem
@@ -84,16 +83,6 @@ def cert_path_for_cert_name(config: configuration.NamespaceConfig, cert_name: st
         cert_name_implied_conf, encoding='utf-8', default_encoding='utf-8')["fullchain"]
 
 
-def config_with_defaults(config: Optional[configuration.NamespaceConfig] = None
-                         ) -> configobj.ConfigObj:
-    """Merge supplied config, if provided, on top of builtin defaults."""
-    defaults_copy = configobj.ConfigObj(
-        constants.RENEWER_DEFAULTS, encoding='utf-8', default_encoding='utf-8')
-    defaults_copy.merge(config if config is not None else configobj.ConfigObj(
-        encoding='utf-8', default_encoding='utf-8'))
-    return defaults_copy
-
-
 def add_time_interval(base_time: datetime.datetime, interval: str,
                       textparser: parsedatetime.Calendar = parsedatetime.Calendar()
                       ) -> datetime.datetime:
@@ -153,10 +142,6 @@ def write_renewal_config(o_filename: str, n_filename: str, archive_dir: str,
         if k not in relevant_data:
             del config["renewalparams"][k]
 
-    if "renew_before_expiry" not in config:
-        default_interval = constants.RENEWER_DEFAULTS["renew_before_expiry"]
-        config.initial_comment = ["renew_before_expiry = " + default_interval]
-
     # TODO: add human-readable comments explaining other available
     #       parameters
     logger.debug("Writing new config %s.", n_filename)
@@ -469,20 +454,19 @@ class RenewableCert(interfaces.RenewableCert):
         self.cli_config = cli_config
         self._lineagename = lineagename_for_filename(config_filename)
 
-        # self.configuration should be used to read parameters that
-        # may have been chosen based on default values from the
-        # systemwide renewal configuration; self.configfile should be
-        # used to make and save changes.
         try:
             self.configfile = configobj.ConfigObj(
                 config_filename, encoding='utf-8', default_encoding='utf-8')
         except configobj.ConfigObjError:
             raise errors.CertStorageError(
                 "error parsing {0}".format(config_filename))
-        # TODO: Do we actually use anything from defaults and do we want to
+
-        #       read further defaults from the systemwide renewal configuration
+        # These are equivalent. Previously we were adding the unused default
-        #       file at this stage?
+        # value of renew_before_expiry. Keeping both names because cleaning
-        self.configuration = config_with_defaults(self.configfile)
+        # out the variables from callers is annoying. Ideally new code should
+        # use self.configfile so we can remove self.configuration at some point,
+        # but either should work currently.
+        self.configuration = self.configfile
 
         if not all(x in self.configuration for x in ALL_FOUR):
             raise errors.CertStorageError(
@@ -1189,7 +1173,7 @@ class RenewableCert(interfaces.RenewableCert):
         # Update renewal config file
         self.configfile = update_configuration(
             self.lineagename, self.archive_dir, symlinks, cli_config)
-        self.configuration = config_with_defaults(self.configfile)
+        self.configuration = self.configfile
 
         return target_version
 
@@ -1204,7 +1188,7 @@ class RenewableCert(interfaces.RenewableCert):
         # Update renewal config file
         self.configfile = update_configuration(
             self.lineagename, self.archive_dir, symlinks, cli_config)
-        self.configuration = config_with_defaults(self.configfile)
+        self.configuration = self.configfile
 
     def truncate(self, num_prior_certs_to_keep: int = 5) -> None:
         """Delete unused historical certificate, chain and key items from the lineage.