Merge pull request #2663 from letsencrypt/no_irrelevant_items

Don't save irrelevant items into renewal configuration

letsencrypt/storage.py

@@ -50,13 +50,12 @@ def add_time_interval(base_time, interval, textparser=parsedatetime.Calendar()):
     return textparser.parseDT(interval, base_time, tzinfo=tzinfo)[0]
 
 
-def write_renewal_config(filename, target, cli_config):
+def write_renewal_config(filename, target, relevant_data):
     """Writes a renewal config file with the specified name and values.
 
     :param str filename: Absolute path to the config file
     :param dict target: Maps ALL_FOUR to their symlink paths
-    :param .RenewerConfiguration cli_config: parsed command line
+    :param dict relevant_data: Renewal configuration options to save
-        arguments
 
     :returns: Configuration object for the new config file
     :rtype: configobj.ConfigObj
@@ -67,18 +66,11 @@ def write_renewal_config(filename, target, cli_config):
     for kind in ALL_FOUR:
         config[kind] = target[kind]
 
-    # XXX: We clearly need a more general and correct way of getting
+    if relevant_data:
-    # options into the configobj for the RenewableCert instance.
+        config["renewalparams"] = relevant_data
-    # This is a quick-and-dirty way to do it to allow integration
-    # testing to start.  (Note that the config parameter to new_lineage
-    # ideally should be a ConfigObj, but in this case a dict will be
-    # accepted in practice.)
-    renewalparams = vars(cli_config.namespace)
-    if renewalparams:
-        config["renewalparams"] = renewalparams
         config.comments["renewalparams"] = ["",
-                                            "Options and defaults used"
+                                            "Options used in "
-                                            " in the renewal process"]
+                                            "the renewal process"]
 
     # TODO: add human-readable comments explaining other available
     #       parameters
@@ -106,7 +98,10 @@ def update_configuration(lineagename, target, cli_config):
     # If an existing tempfile exists, delete it
     if os.path.exists(temp_filename):
         os.unlink(temp_filename)
-    write_renewal_config(temp_filename, target, cli_config)
+
+    # Save only the config items that are relevant to renewal
+    values = relevant_values(vars(cli_config.namespace))
+    write_renewal_config(temp_filename, target, values)
     os.rename(temp_filename, config_filename)
 
     return configobj.ConfigObj(config_filename)
@@ -127,6 +122,60 @@ def get_link_target(link):
     return os.path.abspath(target)
 
 
+def _relevant(option):
+    """
+    Is this option one that could be restored for future renewal purposes?
+    :param str option: the name of the option
+
+    :rtype: bool
+    """
+    # The list() here produces a list of the plugin names as strings.
+    from letsencrypt import renewal
+    from letsencrypt.plugins import disco as plugins_disco
+    plugins = list(plugins_disco.PluginsRegistry.find_all())
+    return (option in renewal.STR_CONFIG_ITEMS
+            or option in renewal.INT_CONFIG_ITEMS
+            or any(option.startswith(x + "_") for x in plugins))
+
+
+def relevant_values(all_values):
+    """Return a new dict containing only items relevant for renewal.
+
+    :param dict all_values: The original values.
+
+    :returns: A new dictionary containing items that can be used in renewal.
+    :rtype dict:"""
+
+    from letsencrypt import cli
+
+    def _is_cli_default(option, value):
+        # Look through the CLI parser defaults and see if this option is
+        # both present and equal to the specified value. If not, return
+        # False.
+        # pylint: disable=protected-access
+        for x in cli.helpful_parser.parser._actions:
+            if x.dest == option:
+                if x.default == value:
+                    return True
+                else:
+                    break
+        return False
+
+    values = dict()
+    for option, value in all_values.iteritems():
+        # Try to find reasons to store this item in the
+        # renewal config.  It can be stored if it is relevant and
+        # (it is set_by_cli() or flag_default() is different
+        # from the value or flag_default() doesn't exist).
+        if _relevant(option):
+            if (cli.set_by_cli(option)
+                or not _is_cli_default(option, value)):
+#                or option not in constants.CLI_DEFAULTS
+#                or constants.CLI_DEFAULTS[option] != value):
+                values[option] = value
+    return values
+
+
 class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     """Renewable certificate.
 
@@ -690,6 +739,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         :rtype: :class:`storage.renewableCert`
 
         """
+
         # Examine the configuration and find the new lineage's name
         for i in (cli_config.renewal_configs_dir, cli_config.archive_dir,
                   cli_config.live_dir):
@@ -744,7 +794,11 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         # Document what we've done in a new renewal config file
         config_file.close()
-        new_config = write_renewal_config(config_filename, target, cli_config)
+
+        # Save only the config items that are relevant to renewal
+        values = relevant_values(vars(cli_config.namespace))
+
+        new_config = write_renewal_config(config_filename, target, values)
         return cls(new_config.filename, cli_config)
 
     def save_successor(self, prior_version, new_cert,

letsencrypt/tests/storage_test.py

@@ -493,7 +493,12 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertTrue(self.test_rc.should_autorenew())
         mock_ocsp.return_value = False
 
-    def test_save_successor(self):
+    @mock.patch("letsencrypt.storage.relevant_values")
+    def test_save_successor(self, mock_rv):
+        # Mock relevant_values() to claim that all values are relevant here
+        # (to avoid instantiating parser)
+        mock_rv.side_effect = lambda x: x
+
         for ver in xrange(1, 6):
             for kind in ALL_FOUR:
                 where = getattr(self.test_rc, kind)
@@ -557,8 +562,47 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.exists(temp_config_file))
 
-    def test_new_lineage(self):
+    @mock.patch("letsencrypt.cli.helpful_parser")
+    def test_relevant_values(self, mock_parser):
+        """Test that relevant_values() can reject an irrelevant value."""
+        # pylint: disable=protected-access
+        from letsencrypt import storage
+        mock_parser.verb = "certonly"
+        mock_parser.args = ["--standalone"]
+        mock_action = mock.Mock(dest="rsa_key_size", default=2048)
+        mock_parser.parser._actions = [mock_action]
+        self.assertEqual(storage.relevant_values({"hello": "there"}), {})
+
+    @mock.patch("letsencrypt.cli.helpful_parser")
+    def test_relevant_values_default(self, mock_parser):
+        """Test that relevant_values() can reject a default value."""
+        # pylint: disable=protected-access
+        from letsencrypt import storage
+        mock_parser.verb = "certonly"
+        mock_parser.args = ["--standalone"]
+        mock_action = mock.Mock(dest="rsa_key_size", default=2048)
+        mock_parser.parser._actions = [mock_action]
+        self.assertEqual(storage.relevant_values({"rsa_key_size": 2048}), {})
+
+    @mock.patch("letsencrypt.cli.helpful_parser")
+    def test_relevant_values_nondefault(self, mock_parser):
+        """Test that relevant_values() can retain a non-default value."""
+        # pylint: disable=protected-access
+        from letsencrypt import storage
+        mock_parser.verb = "certonly"
+        mock_parser.args = ["--standalone"]
+        mock_action = mock.Mock(dest="rsa_key_size", default=2048)
+        mock_parser.parser._actions = [mock_action]
+        self.assertEqual(storage.relevant_values({"rsa_key_size": 12}),
+                         {"rsa_key_size": 12})
+
+    @mock.patch("letsencrypt.storage.relevant_values")
+    def test_new_lineage(self, mock_rv):
         """Test for new_lineage() class method."""
+        # Mock relevant_values to say everything is relevant here (so we
+        # don't have to mock the parser to help it decide!)
+        mock_rv.side_effect = lambda x: x
+
         from letsencrypt import storage
         result = storage.RenewableCert.new_lineage(
             "the-lineage.com", "cert", "privkey", "chain", self.cli_config)
@@ -592,8 +636,13 @@ class RenewableCertTests(BaseRenewableCertTest):
         # TODO: Conceivably we could test that the renewal parameters actually
         #       got saved
 
-    def test_new_lineage_nonexistent_dirs(self):
+    @mock.patch("letsencrypt.storage.relevant_values")
+    def test_new_lineage_nonexistent_dirs(self, mock_rv):
         """Test that directories can be created if they don't exist."""
+        # Mock relevant_values to say everything is relevant here (so we
+        # don't have to mock the parser to help it decide!)
+        mock_rv.side_effect = lambda x: x
+
         from letsencrypt import storage
         shutil.rmtree(self.cli_config.renewal_configs_dir)
         shutil.rmtree(self.cli_config.archive_dir)