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

10 files changed, 119 insertions, 26 deletions

diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py
index ab83a5332..bb77e2e41 100644
--- a/certbot-apache/certbot_apache/configurator.py
+++ b/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)
diff --git a/certbot-apache/certbot_apache/tests/autohsts_test.py b/certbot-apache/certbot_apache/tests/autohsts_test.py
index 86d985079..73da33f15 100644
--- a/certbot-apache/certbot_apache/tests/autohsts_test.py
+++ b/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")
diff --git a/certbot/interfaces.py b/certbot/interfaces.py
index a5fb426e6..2e837d1d2 100644
--- a/certbot/interfaces.py
+++ b/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
diff --git a/certbot/main.py b/certbot/main.py
index 6078f87a6..556722104 100644
--- a/certbot/main.py
+++ b/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.
diff --git a/certbot/plugins/enhancements.py b/certbot/plugins/enhancements.py
index 506abe433..7ca096610 100644
--- a/certbot/plugins/enhancements.py
+++ b/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
diff --git a/certbot/plugins/selection.py b/certbot/plugins/selection.py
index 030d5b6db..95f123a46 100644
--- a/certbot/plugins/selection.py
+++ b/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.
diff --git a/certbot/plugins/selection_test.py b/certbot/plugins/selection_test.py
index ab480544a..44d64ab8e 100644
--- a/certbot/plugins/selection_test.py
+++ b/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
diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py
index a2115a486..cc4e6c293 100644
--- a/certbot/tests/main_test.py
+++ b/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):
diff --git a/certbot/tests/renewupdater_test.py b/certbot/tests/renewupdater_test.py
index c1b97843e..5a362072c 100644
--- a/certbot/tests/renewupdater_test.py
+++ b/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)
diff --git a/certbot/updater.py b/certbot/updater.py
index fb7c52f77..58df6fcb4 100644
--- a/certbot/updater.py
+++ b/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