Remove deprecated options as early as possible using an explicit list (#8617)

* Remove deprecated options as early as possible using an explicit list

* add deprecated options to cli init import list

* use correct dict comprehension syntax for py3

* lint

* add test for renewal reconstitution code

* add test to ensure we're not saving deprecated values

* comment code

certbot/certbot/_internal/cli/__init__.py

@@ -28,7 +28,8 @@ from certbot._internal.cli.cli_constants import (
     ARGPARSE_PARAMS_TO_REMOVE,
     EXIT_ACTIONS,
     ZERO_ARG_ACTIONS,
-    VAR_MODIFIERS
+    VAR_MODIFIERS,
+    DEPRECATED_OPTIONS
 )
 
 from certbot._internal.cli.cli_utils import (
@@ -471,6 +472,11 @@ def set_by_cli(var):
     (CLI or config file) including if the user explicitly set it to the
     default.  Returns False if the variable was assigned a default value.
     """
+    # We should probably never actually hit this code. But if we do,
+    # a deprecated option has logically never been set by the CLI.
+    if var in DEPRECATED_OPTIONS:
+        return False
+
     detector = set_by_cli.detector  # type: ignore
     if detector is None and helpful_parser is not None:
         # Setup on first run: `detector` is a weird version of config in which
@@ -531,6 +537,9 @@ def option_was_set(option, value):
     :rtype: bool
 
     """
+    # If an option is deprecated, it was effectively not set by the user.
+    if option in DEPRECATED_OPTIONS:
+        return False
     return set_by_cli(option) or not has_default_value(option, value)
 
 

certbot/certbot/_internal/cli/cli_constants.py

@@ -105,3 +105,8 @@ VAR_MODIFIERS = {"account": {"server",},
                  "renew_hook": {"deploy_hook",},
                  "server": {"dry_run", "staging",},
                  "webroot_map": {"webroot_path",}}
+
+# This is a list of all CLI options that we have ever deprecated. It lets us
+# opt out of the default detection, which can interact strangely with option
+# deprecation. See https://github.com/certbot/certbot/issues/8540 for more info.
+DEPRECATED_OPTIONS = {"manual_public_ip_logging_ok",}

certbot/certbot/_internal/renewal.py

@@ -85,6 +85,7 @@ def _reconstitute(config, full_path):
         return None
     # Now restore specific values along with their data types, if
     # those elements are present.
+    renewalparams = _remove_deprecated_config_elements(renewalparams)
     try:
         restore_required_config_elements(config, renewalparams)
         _restore_plugin_configs(config, renewalparams)
@@ -188,6 +189,19 @@ def restore_required_config_elements(config, renewalparams):
             setattr(config, item_name, value)
 
 
+def _remove_deprecated_config_elements(renewalparams):
+    """Removes deprecated config options from the parsed renewalparams.
+
+    :param dict renewalparams: list of parsed renewalparams
+
+    :returns: list of renewalparams with deprecated config options removed
+    :rtype: dict
+
+    """
+    return {option_name: v for (option_name, v) in renewalparams.items()
+        if option_name not in cli.DEPRECATED_OPTIONS}
+
+
 def _restore_pref_challs(unused_name, value):
     """Restores preferred challenges from a renewal config file.
 

certbot/certbot/tests/testdata/sample-renewal-deprecated-option.conf

@@ -0,0 +1,14 @@
+# renew_before_expiry = 30 days
+version = 1.11.0
+archive_dir = MAGICDIR/live/sample-renewal-deprecated-option
+cert = MAGICDIR/live/sample-renewal-deprecated-option/cert.pem
+privkey = MAGICDIR/live/sample-renewal-deprecated-option/privkey.pem
+chain = MAGICDIR/live/sample-renewal-deprecated-option/chain.pem
+fullchain = MAGICDIR/live/sample-renewal-deprecated-option/fullchain.pem
+
+# Options used in the renewal process
+[renewalparams]
+account = ffffffffffffffffffffffffffffffff
+authenticator = nginx
+installer = nginx
+manual_public_ip_logging_ok = None

certbot/tests/renewal_test.py

@@ -1,4 +1,6 @@
 """Tests for certbot._internal.renewal"""
+import copy
+
 import unittest
 
 try:
@@ -98,6 +100,23 @@ class RenewalTest(test_util.ConfigTestCase):
 
         assert self.config.elliptic_curve == 'secp256r1'
 
+    @test_util.patch_get_utility()
+    @mock.patch('certbot._internal.renewal.cli.set_by_cli')
+    def test_remove_deprecated_config_elements(self, mock_set_by_cli, unused_mock_get_utility):
+        mock_set_by_cli.return_value = False
+        config = configuration.NamespaceConfig(self.config)
+        config.certname = "sample-renewal-deprecated-option"
+
+        rc_path = test_util.make_lineage(
+            self.config.config_dir, 'sample-renewal-deprecated-option.conf')
+
+        from certbot._internal import renewal
+        lineage_config = copy.deepcopy(self.config)
+        renewal_candidate = renewal._reconstitute(lineage_config, rc_path)
+        # This means that manual_public_ip_logging_ok was not modified in the config based on its
+        # value in the renewal conf file
+        self.assertTrue(isinstance(lineage_config.manual_public_ip_logging_ok, mock.MagicMock))
+
 
 class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase):
     """Tests for certbot._internal.renewal.restore_required_config_elements."""

certbot/tests/storage_test.py

@@ -77,6 +77,17 @@ class RelevantValuesTest(unittest.TestCase):
 
         self.assertEqual(self._call(self.values), expected_relevant_values)
 
+    @mock.patch("certbot._internal.cli.set_by_cli")
+    def test_deprecated_item(self, unused_mock_set_by_cli):
+        # deprecated items should never be relevant to store
+        expected_relevant_values = self.values.copy()
+        self.values["manual_public_ip_logging_ok"] = None
+        self.assertEqual(self._call(self.values), expected_relevant_values)
+        self.values["manual_public_ip_logging_ok"] = True
+        self.assertEqual(self._call(self.values), expected_relevant_values)
+        self.values["manual_public_ip_logging_ok"] = False
+        self.assertEqual(self._call(self.values), expected_relevant_values)
+
 
 class BaseRenewableCertTest(test_util.ConfigTestCase):
     """Base class for setting up Renewable Cert tests.