Fix add deprecated argument (#8500) (#8501)

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

To further explain the problem here, `modify_kwargs_for_default_detection` as called in `add` is simplistic and doesn't always work. See https://github.com/certbot/certbot/issues/6164 for one other example.

In this case, were bitten by the code d1e7404358/certbot/certbot/_internal/cli/helpful.py (L393-L395)

The action used for deprecated arguments isn't in `ZERO_ARG_ACTIONS` so it assumes that all deprecated flags take one parameter.

Rather than trying to fix this function (which I think can only realistically be fixed by https://github.com/certbot/certbot/issues/4493), I took the approach that was previously used in `HelpfulArgumentParser.add_deprecated_argument` of bypassing this extra logic entirely. I adapted that function to now call `HelpfulArgumentParser.add` as well for consistency and to make testing easier.

* Rename deprecated arg action class

* Skip extra parsing for deprecated arguments

* Add back test of --manual-public-ip-logging-ok

* Add changelog entry

(cherry picked from commit 5f73274390)

certbot-ci/certbot_integration_tests/utils/certbot_call.py

@@ -92,6 +92,7 @@ def _prepare_args_env(certbot_args, directory_url, http_01_port, tls_alpn_01_por
         '--no-verify-ssl',
         '--http-01-port', str(http_01_port),
         '--https-port', str(tls_alpn_01_port),
+        '--manual-public-ip-logging-ok',
         '--config-dir', config_dir,
         '--work-dir', os.path.join(workspace, 'work'),
         '--logs-dir', os.path.join(workspace, 'logs'),

certbot/CHANGELOG.md

@@ -2,6 +2,16 @@
 
 Certbot adheres to [Semantic Versioning](https://semver.org/).
 
+## 1.10.1 - master
+
+### Fixed
+
+* Fixed a bug in `certbot.util.add_deprecated_argument` that caused the
+  deprecated `--manual-public-ip-logging-ok` flag to crash Certbot in some
+  scenarios.
+
+More details about these changes can be found on our GitHub repo.
+
 ## 1.10.0 - 2020-12-01
 
 ### Added

certbot/certbot/_internal/cli/helpful.py

@@ -2,8 +2,10 @@
 from __future__ import print_function
 import argparse
 import copy
+import functools
 import glob
 import sys
+
 import configargparse
 import six
 import zope.component
@@ -356,6 +358,18 @@ class HelpfulArgumentParser(object):
         :param dict **kwargs: various argparse settings for this argument
 
         """
+        action = kwargs.get("action")
+        if action is util.DeprecatedArgumentAction:
+            # If the argument is deprecated through
+            # certbot.util.add_deprecated_argument, it is not shown in the help
+            # output and any value given to the argument is thrown away during
+            # argument parsing. Because of this, we handle this case early
+            # skipping putting the argument in different help topics and
+            # handling default detection since these actions aren't needed and
+            # can cause bugs like
+            # https://github.com/certbot/certbot/issues/8495.
+            self.parser.add_argument(*args, **kwargs)
+            return
 
         if isinstance(topics, list):
             # if this flag can be listed in multiple sections, try to pick the one
@@ -410,8 +424,22 @@ class HelpfulArgumentParser(object):
         :param int nargs: Number of arguments the option takes.
 
         """
-        util.add_deprecated_argument(
-            self.parser.add_argument, argument_name, num_args)
+        # certbot.util.add_deprecated_argument expects the normal add_argument
+        # interface provided by argparse. This is what is given including when
+        # certbot.util.add_deprecated_argument is used by plugins, however, in
+        # that case the first argument to certbot.util.add_deprecated_argument
+        # is certbot._internal.cli.HelpfulArgumentGroup.add_argument which
+        # internally calls the add method of this class.
+        #
+        # The difference between the add method of this class and the standard
+        # argparse add_argument method caused a bug in the past (see
+        # https://github.com/certbot/certbot/issues/8495) so we use the same
+        # code path here for consistency and to ensure it works. To do that, we
+        # wrap the add method in a similar way to
+        # HelpfulArgumentGroup.add_argument by providing a help topic (which in
+        # this case is set to None).
+        add_func = functools.partial(self.add, None)
+        util.add_deprecated_argument(add_func, argument_name, num_args)
 
     def add_group(self, topic, verbs=(), **kwargs):
         """Create a new argument group.

certbot/certbot/util.py

@@ -439,7 +439,7 @@ def safe_email(email):
     return False
 
 
-class _ShowWarning(argparse.Action):
+class DeprecatedArgumentAction(argparse.Action):
     """Action to log a warning when an argument is used."""
     def __call__(self, unused1, unused2, unused3, option_string=None):
         logger.warning("Use of %s is deprecated.", option_string)
@@ -458,16 +458,16 @@ def add_deprecated_argument(add_argument, argument_name, nargs):
     :param nargs: Value for nargs when adding the argument to argparse.
 
     """
-    if _ShowWarning not in configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE:
+    if DeprecatedArgumentAction not in configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE:
         # In version 0.12.0 ACTION_TYPES_THAT_DONT_NEED_A_VALUE was
         # changed from a set to a tuple.
         if isinstance(configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE, set):
             configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE.add(
-                _ShowWarning)
+                DeprecatedArgumentAction)
         else:
             configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE += (
-                _ShowWarning,)
-    add_argument(argument_name, action=_ShowWarning,
+                DeprecatedArgumentAction,)
+    add_argument(argument_name, action=DeprecatedArgumentAction,
                  help=argparse.SUPPRESS, nargs=nargs)
 
 

certbot/tests/helpful_test.py

@@ -1,6 +1,11 @@
 """Tests for certbot.helpful_parser"""
 import unittest
 
+try:
+    import mock
+except ImportError: # pragma: no cover
+    from unittest import mock
+
 from certbot import errors
 from certbot._internal.cli import HelpfulArgumentParser
 from certbot._internal.cli import _DomainsAction
@@ -189,5 +194,16 @@ class TestParseArgsErrors(unittest.TestCase):
             arg_parser.parse_args()
 
 
+class TestAddDeprecatedArgument(unittest.TestCase):
+    """Tests for add_deprecated_argument method of HelpfulArgumentParser"""
+
+    @mock.patch.object(HelpfulArgumentParser, "modify_kwargs_for_default_detection")
+    def test_no_default_detection_modifications(self, mock_modify):
+        arg_parser = HelpfulArgumentParser(["run"], {}, detect_defaults=True)
+        arg_parser.add_deprecated_argument("--foo", 0)
+        arg_parser.parse_args()
+        mock_modify.assert_not_called()
+
+
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover