Do not call IPlugin.prepare() for updaters when running renew (#6167)

interfaces.GenericUpdater and new enhancement interface updater functions get run on every invocation of Certbot with "renew" verb for every lineage. This causes performance problems for users with large configurations, because of plugin plumbing and preparsing happening in prepare() method of installer plugins. This PR moves the responsibility to call prepare() to the plugin (possibly) implementing a new style enhancement interface.

Fixes: #6153

* Do not call IPlugin.prepare() for updaters when running renew

* Check prepare called in tests

* Refine pydoc and make the function name more informative

* Verify the plugin type

certbot-apache/certbot_apache/configurator.py

@@ -165,6 +165,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self._autohsts = {}  # type: Dict[str, Dict[str, Union[int, float]]]
 
         # These will be set in the prepare function
+        self._prepared = False
         self.parser = None
         self.version = version
         self.vhosts = None
@@ -249,6 +250,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             logger.debug("Encountered error:", exc_info=True)
             raise errors.PluginError(
                 "Unable to lock %s", self.conf("server-root"))
+        self._prepared = True
 
     def _check_aug_version(self):
         """ Checks that we have recent enough version of libaugeas.
@@ -2394,6 +2396,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 continue
             nextstep = config["laststep"] + 1
             if nextstep < len(constants.AUTOHSTS_STEPS):
+                # If installer hasn't been prepared yet, do it now
+                if not self._prepared:
+                    self.prepare()
                 # Have not reached the max value yet
                 try:
                     vhost = self.find_vhost_by_id(id_str)

certbot-apache/certbot_apache/tests/autohsts_test.py

@@ -55,7 +55,9 @@ class AutoHSTSTest(util.ApacheTest):
 
     @mock.patch("certbot_apache.constants.AUTOHSTS_FREQ", 0)
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")
-    def test_autohsts_increase(self, _mock_restart):
+    @mock.patch("certbot_apache.configurator.ApacheConfigurator.prepare")
+    def test_autohsts_increase(self, mock_prepare, _mock_restart):
+        self.config._prepared = False
         maxage = "\"max-age={0}\""
         initial_val = maxage.format(constants.AUTOHSTS_STEPS[0])
         inc_val = maxage.format(constants.AUTOHSTS_STEPS[1])
@@ -69,6 +71,7 @@ class AutoHSTSTest(util.ApacheTest):
         # Verify increased value
         self.assertEquals(self.get_autohsts_value(self.vh_truth[7].path),
                           inc_val)
+        self.assertTrue(mock_prepare.called)
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")
     @mock.patch("certbot_apache.configurator.ApacheConfigurator._autohsts_increase")

certbot/interfaces.py

@@ -620,6 +620,9 @@ class GenericUpdater(object):
     methods, and interfaces.GenericUpdater.register(InstallerClass) should
     be called from the installer code.
 
+    The plugins implementing this enhancement are responsible of handling
+    the saving of configuration checkpoints as well as other calls to
+    interface methods of `interfaces.IInstaller` such as prepare() and restart()
     """
 
     @abc.abstractmethod

certbot/main.py

@@ -1199,11 +1199,11 @@ def renew_cert(config, plugins, lineage):
         # In case of a renewal, reload server to pick up new certificate.
         # In principle we could have a configuration option to inhibit this
         # from happening.
+        # Run deployer
         updater.run_renewal_deployer(config, renewed_lineage, installer)
         installer.restart()
         notify("new certificate deployed with reload of {0} server; fullchain is {1}".format(
                config.installer, lineage.fullchain), pause=False)
-        # Run deployer
 
 def certonly(config, plugins):
     """Authenticate & obtain cert, but do not install it.

certbot/plugins/enhancements.py

@@ -88,7 +88,8 @@ class AutoHSTSEnhancement(object):
 
     The plugins implementing new style enhancements are responsible of handling
     the saving of configuration checkpoints as well as calling possible restarts
-    of managed software themselves.
+    of managed software themselves. For update_autohsts method, the installer may
+    have to call prepare() to finalize the plugin initialization.
 
     Methods:
         enable_autohsts is called when the header is initially installed using a
@@ -112,6 +113,10 @@ class AutoHSTSEnhancement(object):
 
         :param lineage: Certificate lineage object
         :type lineage: certbot.storage.RenewableCert
+
+        .. note:: prepare() method inherited from `interfaces.IPlugin` might need
+            to be called manually within implementation of this interface method
+            to finalize the plugin initialization.
         """
 
     @abc.abstractmethod

certbot/plugins/selection.py

@@ -39,6 +39,35 @@ def pick_authenticator(
     return pick_plugin(
         config, default, plugins, question, (interfaces.IAuthenticator,))
 
+def get_unprepared_installer(config, plugins):
+    """
+    Get an unprepared interfaces.IInstaller object.
+
+    :param certbot.interfaces.IConfig config: Configuration
+    :param certbot.plugins.disco.PluginsRegistry plugins:
+        All plugins registered as entry points.
+
+    :returns: Unprepared installer plugin or None
+    :rtype: IPlugin or None
+    """
+
+    _, req_inst = cli_plugin_requests(config)
+    if not req_inst:
+        return None
+    installers = plugins.filter(lambda p_ep: p_ep.name == req_inst)
+    installers.init(config)
+    installers = installers.verify((interfaces.IInstaller,))
+    if len(installers) > 1:
+        raise errors.PluginSelectionError(
+            "Found multiple installers with the name %s, Certbot is unable to "
+            "determine which one to use. Skipping." % req_inst)
+    if installers:
+        inst = list(installers.values())[0]
+        logger.debug("Selecting plugin: %s", inst)
+        return inst.init(config)
+    else:
+        raise errors.PluginSelectionError(
+            "Could not select or initialize the requested installer %s." % req_inst)
 
 def pick_plugin(config, default, plugins, question, ifaces):
     """Pick plugin.

certbot/plugins/selection_test.py

@@ -6,10 +6,13 @@ import unittest
 import mock
 import zope.component
 
+from certbot import errors
+from certbot import interfaces
+
 from acme.magic_typing import List  # pylint: disable=unused-import, no-name-in-module
 from certbot.display import util as display_util
+from certbot.plugins.disco import PluginsRegistry
 from certbot.tests import util as test_util
-from certbot import interfaces
 
 
 class ConveniencePickPluginTest(unittest.TestCase):
@@ -170,5 +173,48 @@ class ChoosePluginTest(unittest.TestCase):
 
         self.assertTrue("default" in mock_util().menu.call_args[1])
 
+class GetUnpreparedInstallerTest(test_util.ConfigTestCase):
+    """Tests for certbot.plugins.selection.get_unprepared_installer."""
+
+    def setUp(self):
+        super(GetUnpreparedInstallerTest, self).setUp()
+        self.mock_apache_fail_ep = mock.Mock(
+            description_with_name="afail")
+        self.mock_apache_fail_ep.name = "afail"
+        self.mock_apache_ep = mock.Mock(
+            description_with_name="apache")
+        self.mock_apache_ep.name = "apache"
+        self.mock_apache_plugin = mock.MagicMock()
+        self.mock_apache_ep.init.return_value = self.mock_apache_plugin
+        self.plugins = PluginsRegistry({
+            "afail": self.mock_apache_fail_ep,
+            "apache": self.mock_apache_ep,
+        })
+
+    def _call(self):
+        from certbot.plugins.selection import get_unprepared_installer
+        return get_unprepared_installer(self.config, self.plugins)
+
+    def test_no_installer_defined(self):
+        self.config.configurator = None
+        self.assertEquals(self._call(), None)
+
+    def test_no_available_installers(self):
+        self.config.configurator = "apache"
+        self.plugins = PluginsRegistry({})
+        self.assertRaises(errors.PluginSelectionError, self._call)
+
+    def test_get_plugin(self):
+        self.config.configurator = "apache"
+        installer = self._call()
+        self.assertTrue(installer is self.mock_apache_plugin)
+
+    def test_multiple_installers_returned(self):
+        self.config.configurator = "apache"
+        # Two plugins with the same name
+        self.mock_apache_fail_ep.name = "apache"
+        self.assertRaises(errors.PluginSelectionError, self._call)
+
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

certbot/tests/main_test.py

@@ -1493,17 +1493,17 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
                         self.assertTrue(mock_handle.called)
 
     @mock.patch('certbot.plugins.selection.choose_configurator_plugins')
-    def test_plugin_selection_error(self, mock_choose):
+    @mock.patch('certbot.updater._run_updaters')
+    def test_plugin_selection_error(self, mock_run, mock_choose):
         mock_choose.side_effect = errors.PluginSelectionError
         self.assertRaises(errors.PluginSelectionError, main.renew_cert,
                           None, None, None)
 
-        with mock.patch('certbot.updater.logger.warning') as mock_log:
-            self.config.dry_run = False
-            updater.run_generic_updaters(self.config, None, None)
-            self.assertTrue(mock_log.called)
-            self.assertTrue("Could not choose appropriate plugin for updaters"
-                            in mock_log.call_args[0][0])
+        self.config.dry_run = False
+        updater.run_generic_updaters(self.config, None, None)
+        # Make sure we're returning None, and hence not trying to run the
+        # without installer
+        self.assertFalse(mock_run.called)
 
 
 class UnregisterTest(unittest.TestCase):

certbot/tests/renewupdater_test.py

@@ -23,13 +23,15 @@ class RenewUpdaterTest(test_util.ConfigTestCase):
 
     @mock.patch('certbot.main._get_and_save_cert')
     @mock.patch('certbot.plugins.selection.choose_configurator_plugins')
+    @mock.patch('certbot.plugins.selection.get_unprepared_installer')
     @test_util.patch_get_utility()
-    def test_server_updates(self, _, mock_select, mock_getsave):
+    def test_server_updates(self, _, mock_geti, mock_select, mock_getsave):
         mock_getsave.return_value = mock.MagicMock()
         mock_generic_updater = self.generic_updater
 
         # Generic Updater
         mock_select.return_value = (mock_generic_updater, None)
+        mock_geti.return_value = mock_generic_updater
         with mock.patch('certbot.main._init_le_client'):
             main.renew_cert(self.config, None, mock.MagicMock())
         self.assertTrue(mock_generic_updater.restart.called)
@@ -62,9 +64,9 @@ class RenewUpdaterTest(test_util.ConfigTestCase):
         self.assertEquals(mock_log.call_args[0][0],
                           "Skipping renewal deployer in dry-run mode.")
 
-    @mock.patch('certbot.plugins.selection.choose_configurator_plugins')
-    def test_enhancement_updates(self, mock_select):
-        mock_select.return_value = (self.mockinstaller, None)
+    @mock.patch('certbot.plugins.selection.get_unprepared_installer')
+    def test_enhancement_updates(self, mock_geti):
+        mock_geti.return_value = self.mockinstaller
         updater.run_generic_updaters(self.config, mock.MagicMock(), None)
         self.assertTrue(self.mockinstaller.update_autohsts.called)
         self.assertEqual(self.mockinstaller.update_autohsts.call_count, 1)
@@ -74,10 +76,10 @@ class RenewUpdaterTest(test_util.ConfigTestCase):
                                      self.mockinstaller)
         self.assertTrue(self.mockinstaller.deploy_autohsts.called)
 
-    @mock.patch('certbot.plugins.selection.choose_configurator_plugins')
-    def test_enhancement_updates_not_called(self, mock_select):
+    @mock.patch('certbot.plugins.selection.get_unprepared_installer')
+    def test_enhancement_updates_not_called(self, mock_geti):
         self.config.disable_renew_updates = True
-        mock_select.return_value = (self.mockinstaller, None)
+        mock_geti.return_value = self.mockinstaller
         updater.run_generic_updaters(self.config, mock.MagicMock(), None)
         self.assertFalse(self.mockinstaller.update_autohsts.called)
 
@@ -87,8 +89,8 @@ class RenewUpdaterTest(test_util.ConfigTestCase):
                                      self.mockinstaller)
         self.assertFalse(self.mockinstaller.deploy_autohsts.called)
 
-    @mock.patch('certbot.plugins.selection.choose_configurator_plugins')
-    def test_enhancement_no_updater(self, mock_select):
+    @mock.patch('certbot.plugins.selection.get_unprepared_installer')
+    def test_enhancement_no_updater(self, mock_geti):
         FAKEINDEX = [
             {
                 "name": "Test",
@@ -98,7 +100,7 @@ class RenewUpdaterTest(test_util.ConfigTestCase):
                 "enable_function": "enable_autohsts"
             }
         ]
-        mock_select.return_value = (self.mockinstaller, None)
+        mock_geti.return_value = self.mockinstaller
         with mock.patch("certbot.plugins.enhancements._INDEX", FAKEINDEX):
             updater.run_generic_updaters(self.config, mock.MagicMock(), None)
         self.assertFalse(self.mockinstaller.update_autohsts.called)

certbot/updater.py

@@ -28,13 +28,13 @@ def run_generic_updaters(config, lineage, plugins):
         logger.debug("Skipping updaters in dry-run mode.")
         return
     try:
-        # installers are used in auth mode to determine domain names
-        installer, _ = plug_sel.choose_configurator_plugins(config, plugins, "certonly")
-    except errors.PluginSelectionError as e:
+        installer = plug_sel.get_unprepared_installer(config, plugins)
+    except errors.Error as e:
         logger.warning("Could not choose appropriate plugin for updaters: %s", e)
         return
-    _run_updaters(lineage, installer, config)
-    _run_enhancement_updaters(lineage, installer, config)
+    if installer:
+        _run_updaters(lineage, installer, config)
+        _run_enhancement_updaters(lineage, installer, config)
 
 def run_renewal_deployer(config, lineage, installer):
     """Helper function to run deployer interface method if supported by the used