Merge pull request #2741 from letsencrypt/reset_by_cli

Fix plugin default detection

letsencrypt/cli.py

@@ -87,6 +87,48 @@ More detailed help:
 """
 
 
+# These argparse parameters should be removed when detecting defaults.
+ARGPARSE_PARAMS_TO_REMOVE = ("const", "nargs", "type",)
+
+
+# These sets are used when to help detect options set by the user.
+EXIT_ACTIONS = set(("help", "version",))
+
+
+ZERO_ARG_ACTIONS = set(("store_const", "store_true",
+                        "store_false", "append_const", "count",))
+
+
+# Maps a config option to a set of config options that may have modified it.
+# This dictionary is used recursively, so if A modifies B and B modifies C,
+# it is determined that C was modified by the user if A was modified.
+VAR_MODIFIERS = {"account": set(("server",)),
+                 "server": set(("dry_run", "staging",)),
+                 "webroot_map": set(("webroot_path",))}
+
+
+def report_config_interaction(modified, modifiers):
+    """Registers config option interaction to be checked by set_by_cli.
+
+    This function can be called by during the __init__ or
+    add_parser_arguments methods of plugins to register interactions
+    between config options.
+
+    :param modified: config options that can be modified by modifiers
+    :type modified: iterable or str
+    :param modifiers: config options that modify modified
+    :type modifiers: iterable or str
+
+    """
+    if isinstance(modified, str):
+        modified = (modified,)
+    if isinstance(modifiers, str):
+        modifiers = (modifiers,)
+
+    for var in modified:
+        VAR_MODIFIERS.setdefault(var, set()).update(modifiers)
+
+
 def usage_strings(plugins):
     """Make usage strings late so that plugins can be initialised late"""
     if "nginx" in plugins:
@@ -100,6 +142,22 @@ def usage_strings(plugins):
     return USAGE % (apache_doc, nginx_doc), SHORT_USAGE
 
 
+class _Default(object):
+    """A class to use as a default to detect if a value is set by a user"""
+
+    def __bool__(self):
+        return False
+
+    def __eq__(self, other):
+        return isinstance(other, _Default)
+
+    def __hash__(self):
+        return id(_Default)
+
+    def __nonzero__(self):
+        return self.__bool__()
+
+
 def set_by_cli(var):
     """
     Return True if a particular config variable has been set by the user
@@ -116,30 +174,18 @@ def set_by_cli(var):
         detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
-        auth, inst = plugin_selection.cli_plugin_requests(detector)
+        detector.authenticator, detector.installer = (
-        detector.authenticator = auth if auth else ""
+            plugin_selection.cli_plugin_requests(detector))
-        detector.installer = inst if inst else ""
         logger.debug("Default Detector is %r", detector)
 
-    try:
+    if not isinstance(getattr(detector, var), _Default):
-        # Is detector.var something that isn't false?
+        return True
-        change_detected = getattr(detector, var)
-    except AttributeError:
-        logger.warning("Missing default analysis for %r", var)
-        return False
 
-    if change_detected:
+    for modifier in VAR_MODIFIERS.get(var, []):
-        return True
+        if set_by_cli(modifier):
-    # Special case: we actually want account to be set to "" if the server
+            return True
-    # the account was on has changed
+
-    elif var == "account" and (detector.server or detector.dry_run or detector.staging):
+    return False
-        return True
-    # Special case: vars like --no-redirect that get set True -> False
-    # default to None; False means they were set
-    elif var in detector.store_false_vars and change_detected is not None:
-        return True
-    else:
-        return False
 # static housekeeping var
 set_by_cli.detector = None
 
@@ -188,21 +234,23 @@ def config_help(name, hidden=False):
         return interfaces.IConfig[name].__doc__
 
 
-class SilentParser(object):  # pylint: disable=too-few-public-methods
+class HelpfulArgumentGroup(object):
-    """Silent wrapper around argparse.
+    """Emulates an argparse group for use with HelpfulArgumentParser.
 
-    A mini parser wrapper that doesn't print help for its
+    This class is used in the add_group method of HelpfulArgumentParser.
-    arguments. This is needed for the use of callbacks to define
+    Command line arguments can be added to the group, but help
-    arguments within plugins.
+    suppression and default detection is applied by
+    HelpfulArgumentParser when necessary.
 
     """
-    def __init__(self, parser):
+    def __init__(self, helpful_arg_parser, topic):
-        self.parser = parser
+        self._parser = helpful_arg_parser
+        self._topic = topic
 
     def add_argument(self, *args, **kwargs):
-        """Wrap, but silence help"""
+        """Add a new command line argument to the argument group."""
-        kwargs["help"] = argparse.SUPPRESS
+        self._parser.add(self._topic, *args, **kwargs)
-        self.parser.add_argument(*args, **kwargs)
+
 
 class HelpfulArgumentParser(object):
     """Argparse Wrapper.
@@ -235,15 +283,8 @@ class HelpfulArgumentParser(object):
 
         # This is the only way to turn off overly verbose config flag documentation
         self.parser._add_config_file_help = False  # pylint: disable=protected-access
-        self.silent_parser = SilentParser(self.parser)
 
-        # This setting attempts to force all default values to things that are
-        # pythonically false; it is used to detect when values have been
-        # explicitly set by the user, including when they are set to their
-        # normal default value
         self.detect_defaults = detect_defaults
-        if detect_defaults:
-            self.store_false_vars = {}  # vars that use "store_false"
 
         self.args = args
         self.determine_verb()
@@ -269,6 +310,9 @@ class HelpfulArgumentParser(object):
         parsed_args.func = self.VERBS[self.verb]
         parsed_args.verb = self.verb
 
+        if self.detect_defaults:
+            return parsed_args
+
         # Do any post-parsing homework here
 
         # we get domains from -d, but also from the webroot map...
@@ -304,9 +348,6 @@ class HelpfulArgumentParser(object):
                                    "cannot be used with --csr")
             self.handle_csr(parsed_args)
 
-        if self.detect_defaults:  # plumbing
-            parsed_args.store_false_vars = self.store_false_vars
-
         hooks.validate_hooks(parsed_args)
 
         return parsed_args
@@ -415,7 +456,7 @@ class HelpfulArgumentParser(object):
         """
 
         if self.detect_defaults:
-            kwargs = self.modify_arg_for_default_detection(self, *args, **kwargs)
+            kwargs = self.modify_kwargs_for_default_detection(**kwargs)
 
         if self.visible_topics[topic]:
             if topic in self.groups:
@@ -427,39 +468,28 @@ class HelpfulArgumentParser(object):
             kwargs["help"] = argparse.SUPPRESS
             self.parser.add_argument(*args, **kwargs)
 
+    def modify_kwargs_for_default_detection(self, **kwargs):
+        """Modify an arg so we can check if it was set by the user.
 
-    def modify_arg_for_default_detection(self, *args, **kwargs):
+        Changes the parameters given to argparse when adding an argument
-        """
+        so we can properly detect if the value was set by the user.
-        Adding an arg, but ensure that it has a default that evaluates to false,
-        so that set_by_cli can tell if it was set.  Only called if detect_defaults==True.
 
-        :param list *args: the names of this argument flag
+        :param dict kwargs: various argparse settings for this argument
-        :param dict **kwargs: various argparse settings for this argument
 
         :returns: a modified versions of kwargs
+        :rtype: dict
+
         """
-        # argument either doesn't have a default, or the default doesn't
+        action = kwargs.get("action", None)
-        # isn't Pythonically false
+        if action not in EXIT_ACTIONS:
-        if kwargs.get("default", True):
+            kwargs["action"] = ("store_true" if action in ZERO_ARG_ACTIONS else
-            arg_type = kwargs.get("type", None)
+                                "store")
-            if arg_type == int or kwargs.get("action", "") == "count":
+            kwargs["default"] = _Default()
-                kwargs["default"] = 0
+            for param in ARGPARSE_PARAMS_TO_REMOVE:
-            elif arg_type == read_file or "-c" in args:
+                kwargs.pop(param, None)
-                kwargs["default"] = ""
-                kwargs["type"] = str
-            else:
-                kwargs["default"] = ""
-            # This doesn't matter at present (none of the store_false args
-            # are renewal-relevant), but implement it for future sanity:
-            # detect the setting of args whose presence causes True -> False
-        if kwargs.get("action", "") == "store_false":
-            kwargs["default"] = None
-            for var in args:
-                self.store_false_vars[var] = True
 
         return kwargs
 
-
     def add_deprecated_argument(self, argument_name, num_args):
         """Adds a deprecated argument with the name argument_name.
 
@@ -475,22 +505,22 @@ class HelpfulArgumentParser(object):
             self.parser.add_argument, argument_name, num_args)
 
     def add_group(self, topic, **kwargs):
-        """
+        """Create a new argument group.
 
-        This has to be called once for every topic; but we leave those calls
+        This method must be called once for every topic, however, calls
-        next to the argument definitions for clarity. Return something
+        to this function are left next to the argument definitions for
-        arguments can be added to if necessary, either the parser or an argument
+        clarity.
-        group.
+
+        :param str topic: Name of the new argument group.
+
+        :returns: The new argument group.
+        :rtype: `HelpfulArgumentGroup`
 
         """
         if self.visible_topics[topic]:
-            #print("Adding visible group " + topic)
+            self.groups[topic] = self.parser.add_argument_group(topic, **kwargs)
-            group = self.parser.add_argument_group(topic, **kwargs)
+
-            self.groups[topic] = group
+        return HelpfulArgumentGroup(self, topic)
-            return group
-        else:
-            #print("Invisible group " + topic)
-            return self.silent_parser
 
     def add_plugin_args(self, plugins):
         """
@@ -501,7 +531,6 @@ class HelpfulArgumentParser(object):
         """
         for name, plugin_ep in six.iteritems(plugins):
             parser_or_group = self.add_group(name, description=plugin_ep.description)
-            #print(parser_or_group)
             plugin_ep.plugin_cls.inject_parser_options(parser_or_group, name)
 
     def determine_help_topics(self, chosen_topic):

letsencrypt/plugins/common.py

@@ -45,15 +45,14 @@ class Plugin(object):
     def add_parser_arguments(cls, add):
         """Add plugin arguments to the CLI argument parser.
 
+        NOTE: If some of your flags interact with others, you can
+        use cli.report_config_interaction to register this to ensure
+        values are correctly saved/overridable during renewal.
+
         :param callable add: Function that proxies calls to
             `argparse.ArgumentParser.add_argument` prepending options
             with unique plugin name prefix.
 
-        NOTE: if you add argpase arguments such that users setting them can
-        create a config entry that python's bool() would consider false (ie,
-        the use might set the variable to "", [], 0, etc), please ensure that
-        cli.set_by_cli() works for your variable.
-
         """
 
     @classmethod

letsencrypt/renewal.py

@@ -102,16 +102,14 @@ def _restore_webroot_config(config, renewalparams):
     form.
     """
     if "webroot_map" in renewalparams:
-        # if the user does anything that would create a new webroot map on the
+        if not cli.set_by_cli("webroot_map"):
-        # CLI, don't use the old one
+            config.namespace.webroot_map = renewalparams["webroot_map"]
-        if not (cli.set_by_cli("webroot_map") or cli.set_by_cli("webroot_path")):
-            setattr(config.namespace, "webroot_map", renewalparams["webroot_map"])
     elif "webroot_path" in renewalparams:
         logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path")
         wp = renewalparams["webroot_path"]
         if isinstance(wp, str):  # prior to 0.1.0, webroot_path was a string
             wp = [wp]
-        setattr(config.namespace, "webroot_path", wp)
+        config.namespace.webroot_path = wp
 
 
 def _restore_plugin_configs(config, renewalparams):

letsencrypt/tests/cli_test.py

@@ -12,6 +12,7 @@ import unittest
 
 import mock
 import six
+from six.moves import reload_module  # pylint: disable=import-error
 
 from acme import jose
 
@@ -1023,5 +1024,79 @@ class DuplicativeCertsTest(storage_test.BaseRenewableCertTest):
         self.assertEqual(result, (None, None))
 
 
+class DefaultTest(unittest.TestCase):
+    """Tests for letsencrypt.cli._Default."""
+
+    def setUp(self):
+        # pylint: disable=protected-access
+        self.default1 = cli._Default()
+        self.default2 = cli._Default()
+
+    def test_boolean(self):
+        self.assertFalse(self.default1)
+        self.assertFalse(self.default2)
+
+    def test_equality(self):
+        self.assertEqual(self.default1, self.default2)
+
+    def test_hash(self):
+        self.assertEqual(hash(self.default1), hash(self.default2))
+
+
+class SetByCliTest(unittest.TestCase):
+    """Tests for letsencrypt.set_by_cli and related functions."""
+
+    def setUp(self):
+        reload_module(cli)
+
+    def test_webroot_map(self):
+        args = '-w /var/www/html -d example.com'.split()
+        verb = 'renew'
+        self.assertTrue(_call_set_by_cli('webroot_map', args, verb))
+
+    def test_report_config_interaction_str(self):
+        cli.report_config_interaction('manual_public_ip_logging_ok',
+                                      'manual_test_mode')
+        cli.report_config_interaction('manual_test_mode', 'manual')
+
+        self._test_report_config_interaction_common()
+
+    def test_report_config_interaction_iterable(self):
+        cli.report_config_interaction(('manual_public_ip_logging_ok',),
+                                      ('manual_test_mode',))
+        cli.report_config_interaction(('manual_test_mode',), ('manual',))
+
+        self._test_report_config_interaction_common()
+
+    def _test_report_config_interaction_common(self):
+        """Tests implied interaction between manual flags.
+
+        --manual implies --manual-test-mode which implies
+        --manual-public-ip-logging-ok. These interactions don't actually
+        exist in the client, but are used here for testing purposes.
+
+        """
+
+        args = ['--manual']
+        verb = 'renew'
+        for v in ('manual', 'manual_test_mode', 'manual_public_ip_logging_ok'):
+            self.assertTrue(_call_set_by_cli(v, args, verb))
+
+        cli.set_by_cli.detector = None
+
+        args = ['--manual-test-mode']
+        for v in ('manual_test_mode', 'manual_public_ip_logging_ok'):
+            self.assertTrue(_call_set_by_cli(v, args, verb))
+
+        self.assertFalse(_call_set_by_cli('manual', args, verb))
+
+
+def _call_set_by_cli(var, args, verb):
+    with mock.patch('letsencrypt.cli.helpful_parser') as mock_parser:
+        mock_parser.args = args
+        mock_parser.verb = verb
+        return cli.set_by_cli(var)
+
+
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover