Revert change to NamespaceConfig's constructor (#9709)

* Revert change to NamespaceConfig's constructor

NamespaceConfig's argument sources dict is now set with a method,
and raises a runtime error if one isn't set when set_by_user() is
called.

* Actually update CHANGELOG to reflect the set_by_user changes

* linter appeasement

* configuration: update docs, add test

This test ensures that calling `set_by_user` without an initialized
sources dict raises a RuntimeError.

certbot-compatibility-test/certbot_compatibility_test/configurators/apache/common.py

@@ -58,7 +58,7 @@ def _prepare_configurator(self) -> None:
                     getattr(entrypoint.ENTRYPOINT.OS_DEFAULTS, k))
 
         self._configurator = entrypoint.ENTRYPOINT(
-            config=configuration.NamespaceConfig(self.le_config, {}),
+            config=configuration.NamespaceConfig(self.le_config),
             name="apache")
         self._configurator.prepare()
 

certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py

@@ -44,7 +44,7 @@ def _prepare_configurator(self) -> None:
         for k in constants.CLI_DEFAULTS:
             setattr(self.le_config, "nginx_" + k, constants.os_constant(k))
 
-        conf = configuration.NamespaceConfig(self.le_config, {})
+        conf = configuration.NamespaceConfig(self.le_config)
         self._configurator = configurator.NginxConfigurator(config=conf, name="nginx")
         self._configurator.prepare()
 

certbot/CHANGELOG.md

@@ -10,7 +10,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Changed
 
-*
+* `NamespaceConfig` now tracks how its arguments were set via a dictionary, allowing us to remove a bunch
+  of global state previously needed to inspect whether a user set an argument or not.
 
 ### Fixed
 

certbot/certbot/_internal/cli/helpful.py

@@ -224,7 +224,8 @@ def parse_args(self) -> NamespaceConfig:
         parsed_args = self.parser.parse_args(self.args)
         parsed_args.func = self.VERBS[self.verb]
         parsed_args.verb = self.verb
-        config = NamespaceConfig(parsed_args, self._build_sources_dict())
+        config = NamespaceConfig(parsed_args)
+        config.set_argument_sources(self._build_sources_dict())
 
         self.remove_config_file_domains_for_renewal(config)
 

certbot/certbot/_internal/tests/cert_manager_test.py

@@ -236,7 +236,7 @@ def test_certificates_no_files(self, mock_utility, mock_logger):
             work_dir=os.path.join(empty_tempdir, "work"),
             logs_dir=os.path.join(empty_tempdir, "logs"),
             quiet=False
-        ), {})
+        ))
 
         filesystem.makedirs(empty_config.renewal_configs_dir)
         self._certificates(empty_config)

certbot/certbot/_internal/tests/configuration_test.py

@@ -29,7 +29,7 @@ def test_init_same_ports(self):
         self.config.https_port = 4321
         from certbot.configuration import NamespaceConfig
         with pytest.raises(errors.Error):
-            NamespaceConfig(self.config.namespace, {})
+            NamespaceConfig(self.config.namespace)
 
     def test_proxy_getattr(self):
         assert self.config.foo == 'bar'
@@ -87,7 +87,7 @@ def test_absolute_paths(self):
         mock_namespace.work_dir = work_base
         mock_namespace.logs_dir = logs_base
         mock_namespace.server = server
-        config = NamespaceConfig(mock_namespace, {})
+        config = NamespaceConfig(mock_namespace)
 
         assert os.path.isabs(config.config_dir)
         assert config.config_dir == \
@@ -132,7 +132,7 @@ def test_renewal_absolute_paths(self):
         mock_namespace.config_dir = config_base
         mock_namespace.work_dir = work_base
         mock_namespace.logs_dir = logs_base
-        config = NamespaceConfig(mock_namespace, {})
+        config = NamespaceConfig(mock_namespace)
 
         assert os.path.isabs(config.default_archive_dir)
         assert os.path.isabs(config.live_dir)
@@ -158,6 +158,24 @@ def test_hook_directories(self):
                          os.path.join(self.config.renewal_hooks_dir,
                                       constants.RENEWAL_POST_HOOKS_DIR)
 
+    def test_set_by_user_runtime_overrides(self):
+        assert not self.config.set_by_user('something')
+        self.config.something = 'a value'
+        assert self.config.set_by_user('something')
+
+    def test_set_by_user_exception(self):
+        from certbot.configuration import NamespaceConfig
+
+        # a newly created NamespaceConfig has no argument sources dict, so an
+        # exception is raised
+        config = NamespaceConfig(self.config.namespace)
+        with pytest.raises(RuntimeError):
+            config.set_by_user('whatever')
+
+        # now set an argument sources dict
+        config.set_argument_sources({})
+        assert not config.set_by_user('whatever')
+
 
 if __name__ == '__main__':
     sys.exit(pytest.main(sys.argv[1:] + [__file__]))  # pragma: no cover

certbot/certbot/_internal/tests/renewal_test.py

@@ -22,7 +22,7 @@ def test_ancient_webroot_renewal_conf(self, mock_set_by_user):
         self.config.account = None
         self.config.email = None
         self.config.webroot_path = None
-        config = configuration.NamespaceConfig(self.config, {})
+        config = configuration.NamespaceConfig(self.config)
         lineage = storage.RenewableCert(rc_path, config)
         renewalparams = lineage.configuration['renewalparams']
         # pylint: disable=protected-access
@@ -59,7 +59,7 @@ def test_reuse_key_renewal_params(self, unused_mock_avoid_reuse_conflicts):
         self.config.elliptic_curve = 'INVALID_VALUE'
         self.config.reuse_key = True
         self.config.dry_run = True
-        config = configuration.NamespaceConfig(self.config, {})
+        config = configuration.NamespaceConfig(self.config)
 
         rc_path = test_util.make_lineage(
             self.config.config_dir, 'sample-renewal.conf')
@@ -81,7 +81,7 @@ def test_reuse_ec_key_renewal_params(self, unused_mock_avoid_reuse_conflicts):
         self.config.reuse_key = True
         self.config.dry_run = True
         self.config.key_type = 'ecdsa'
-        config = configuration.NamespaceConfig(self.config, {})
+        config = configuration.NamespaceConfig(self.config)
 
         rc_path = test_util.make_lineage(
             self.config.config_dir,
@@ -108,7 +108,7 @@ def test_new_key(self, mock_set_by_user):
         self.config.reuse_key = True
         self.config.new_key = True
         self.config.dry_run = True
-        config = configuration.NamespaceConfig(self.config, {})
+        config = configuration.NamespaceConfig(self.config)
 
         rc_path = test_util.make_lineage(
             self.config.config_dir, 'sample-renewal.conf')
@@ -140,7 +140,7 @@ def test_reuse_key_conflicts(self, mock_set_by_user, unused_mock_renew_hook):
         self.config.rsa_key_size = 4096
         self.config.dry_run = True
 
-        config = configuration.NamespaceConfig(self.config, {})
+        config = configuration.NamespaceConfig(self.config)
 
         rc_path = test_util.make_lineage(
             self.config.config_dir, 'sample-renewal.conf')
@@ -164,7 +164,7 @@ def test_reuse_key_conflicts(self, mock_set_by_user, unused_mock_renew_hook):
     @mock.patch.object(configuration.NamespaceConfig, 'set_by_user')
     def test_remove_deprecated_config_elements(self, mock_set_by_user, unused_mock_get_utility):
         mock_set_by_user.return_value = False
-        config = configuration.NamespaceConfig(self.config, {})
+        config = configuration.NamespaceConfig(self.config)
         config.certname = "sample-renewal-deprecated-option"
 
         rc_path = test_util.make_lineage(

certbot/certbot/configuration.py

@@ -59,17 +59,14 @@ class NamespaceConfig:
     :ivar namespace: Namespace typically produced by
         :meth:`argparse.ArgumentParser.parse_args`.
     :type namespace: :class:`argparse.Namespace`
-    :ivar argument_sources: dictionary of argument names to their :class:`ArgumentSource`
-    :type argument_sources: :class:`Dict[str, ArgumentSource]`
 
     """
 
-    def __init__(self, namespace: argparse.Namespace,
-                 argument_sources: Dict[str, ArgumentSource]) -> None:
+    def __init__(self, namespace: argparse.Namespace) -> None:
         self.namespace: argparse.Namespace
         # Avoid recursion loop because of the delegation defined in __setattr__
         object.__setattr__(self, 'namespace', namespace)
-        object.__setattr__(self, 'argument_sources', argument_sources)
+        object.__setattr__(self, 'argument_sources', None)
 
         self.namespace.config_dir = os.path.abspath(self.namespace.config_dir)
         self.namespace.work_dir = os.path.abspath(self.namespace.work_dir)
@@ -78,17 +75,42 @@ def __init__(self, namespace: argparse.Namespace,
         # Check command line parameters sanity, and error out in case of problem.
         _check_config_sanity(self)
 
+    def set_argument_sources(self, argument_sources: Dict[str, ArgumentSource]) -> None:
+        """
+        Associate the NamespaceConfig with a dictionary describing where each of
+        its arguments came from, e.g. `{ 'email': ArgumentSource.CONFIG_FILE }`.
+        This is necessary for making runtime evaluations on whether an argument
+        was specified by the user or not (see `set_by_user`).
+
+        For an example of how to build such a dictionary, see
+        `certbot._internal.cli.helpful.HelpfulArgumentParser._build_sources_dict`
+
+        :ivar argument_sources: dictionary of argument names to their :class:`ArgumentSource`
+        :type argument_sources: :class:`Dict[str, ArgumentSource]`
+        """
+
+        # Avoid recursion loop because of the delegation defined in __setattr__
+        object.__setattr__(self, 'argument_sources', argument_sources)
+
+
     def set_by_user(self, var: str) -> bool:
         """
         Return True if a particular config variable has been set by the user
         (via CLI or config file) including if the user explicitly set it to the
         default, or if it was dynamically set at runtime.  Returns False if the
         variable was assigned a default value.
+
+        Raises an exception if `argument_sources` is not set.
         """
         from certbot._internal.cli.cli_constants import DEPRECATED_OPTIONS
         from certbot._internal.cli.cli_constants import VAR_MODIFIERS
         from certbot._internal.plugins import selection
 
+        if self.argument_sources is None:
+            raise RuntimeError(
+                "NamespaceConfig.set_by_user called without an ArgumentSources dict. "
+                "See NamespaceConfig.set_argument_sources().")
+
         # 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:
@@ -121,10 +143,12 @@ def to_dict(self) -> Dict[str, Any]:
 
     def _mark_runtime_override(self, name: str) -> None:
         """
-        Overwrites an argument's source to be ArgumentSource.RUNTIME. Used when certbot sets an
-        argument's values at runtime.
+        If an argument_sources dict was set, overwrites an argument's source to
+        be ArgumentSource.RUNTIME. Used when certbot sets an argument's values
+        at runtime.
         """
-        self.argument_sources[name] = ArgumentSource.RUNTIME
+        if self.argument_sources is not None:
+            self.argument_sources[name] = ArgumentSource.RUNTIME
 
     # Delegate any attribute not explicitly defined to the underlying namespace object.
 
@@ -401,8 +425,11 @@ def __deepcopy__(self, _memo: Any) -> 'NamespaceConfig':
         # Work around https://bugs.python.org/issue1515 for py26 tests :( :(
         # https://travis-ci.org/letsencrypt/letsencrypt/jobs/106900743#L3276
         new_ns = copy.deepcopy(self.namespace)
-        new_sources = copy.deepcopy(self.argument_sources)
-        return type(self)(new_ns, new_sources)
+        new_config = type(self)(new_ns)
+        if self.set_argument_sources is not None:
+            new_sources = copy.deepcopy(self.argument_sources)
+            new_config.set_argument_sources(new_sources)
+        return new_config
 
 
 def _check_config_sanity(config: NamespaceConfig) -> None:

certbot/certbot/tests/util.py

@@ -396,8 +396,8 @@ def setUp(self) -> None:
         super().setUp()
         self.config = configuration.NamespaceConfig(
             mock.MagicMock(**constants.CLI_DEFAULTS),
-            {},
         )
+        self.config.set_argument_sources({})
         self.config.namespace.verb = "certonly"
         self.config.namespace.config_dir = os.path.join(self.tempdir, 'config')
         self.config.namespace.work_dir = os.path.join(self.tempdir, 'work')