Repository: certbot/certbot
Pull request: Use 3rd party plugins without prefix + set a deprecation path for the prefixed version
Fixes #4351

This PR proposes a solution to use the third party plugins with the prefix `pip_package_name:` in the plugin name, plugin specific flags and keys in dns plugin credential files.

A first solution has been proposed in #6372, and a more advanced one in #7026. In #7026 was also added a deprecation warning when the old plugin name `pip_package_name:plugin_name` was used.

However there were some limitations with #7026, in particular the fact that existing flags of type `pip_package_name:dns_plugin_option` or keys like `pip_package_name:key` in dns plugin credential files were not read anymore. This would have led to silent failures during renewals if the configuration was not explicitly updated by the user.

I tried to fix that based on #7026, but the changes needed are complex, and create new problems on their own, like unexpected erasure of values in the renewal configurations.

Instead I try in this PR a new approach: the `PluginsRegistry` in `certbot._internal.plugins.disco` module register two plugins for a given entrypoint refering to a third party plugin when `find_all()` is called:
* one plugin with the name `plugin_name`
* one plugin with the name `pip_package_name:plugin_name` (like before)

This way, every existing configuration continues to work without any change (credentials, renewal configuration, CLI flags). And new configurations can refer to the new plugin name without prefix, and use the approriate CLI flags, credentials without this prefix.

On top of it I added the deprecation path given in #7026 (thanks @coldfix!):
* the plugin named `pip_package_name:plugin_name` is hidden from `certbot plugins` output
* the help for this plugin is still displayed, and a deprecation warning is displayed in the description
* when invoked, the same deprecation warning is displayed in the terminal

Problem statement:
3rd party plugin flags
The name of 3rd party plugins is generated in `certbot/plugins/disco.py`. Currently, all third party plugin names are `<package>:<entry point>`. Because we use the plugin name to prefix the CLI flag name, this generates some really awkward flags `--certbot-foo:bar-baz`. We should do better, but make sure this doesn't break existing plugins by allowing the old form.

Hints from issue discussion:
I think we can stop adding the name of the package to the plugin flag and just use the name of the entry point which the plugin author controls. The big thing here is we don't want to break backwards compatibility. The `package_name:entry_point_name` version of the flags still need to work. This affects both adding the plugin flags and fetching  the values from the parsed config.

Perhaps a simple solution is to pass multiple flags to `add_argument` [here](https://github.com/certbot/certbot/blob/master/certbot/plugins/common.py#L79) if `option_namespace` contains the package name. The `option_namespace` should probably still comes first as that's what affects the path in the parsed config, however, plugins can access these values using [conf()](https://github.com/certbot/certbot/blob/master/certbot/plugins/common.py#L103) so they don't have to see this ugliness. Only downside with this approach I see is the `package_name:entry_point_name` version will be used in renewal configuration files, but I can't see any other problems here. I definitely could be overlooking something though so this will require some experimentation and testing.
We've made a lot of changes to Certbot since this issue was opened. If you still have this issue with an up-to-date version of Certbot, can you please add a comment letting us know? This helps us to better see what issues are still affecting our users. If there is no further activity, this issue will be automatically closed.

I still think we should do this. It's easily the most common complaint I see about writing 3rd party Certbot plugins.
Due to the colon it is not possible to provide configurations for plugins in the `cli.ini` configuration file.

Commit message: Add a changelog

Patch to review:
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index fbd603d92ff..09f58a09889 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -15,6 +15,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Added `--preferred-chain <issuer CN>`. If a CA offers multiple certificate chains,
   it may be  used to indicate to Certbot which chain should be preferred.
   * e.g. `--preferred-chain "DST Root CA X3"`
+* Third-party plugins can be used without prefix (`plugin_name` instead of `dist_name:plugin_name`):
+  this concerns the plugin name, CLI flags, and keys in credential files for DNS plugins.
+  The prefixed form is still supported but is deprecated, and will be removed in a future release.
 
 ### Changed
 
diff --git a/certbot/certbot/_internal/plugins/disco.py b/certbot/certbot/_internal/plugins/disco.py
index 4cce895d859..4538e8123bc 100644
--- a/certbot/certbot/_internal/plugins/disco.py
+++ b/certbot/certbot/_internal/plugins/disco.py
@@ -23,47 +23,58 @@
 
 logger = logging.getLogger(__name__)
 
+PREFIX_FREE_DISTRIBUTIONS = [
+    "certbot",
+    "certbot-apache",
+    "certbot-dns-cloudflare",
+    "certbot-dns-cloudxns",
+    "certbot-dns-digitalocean",
+    "certbot-dns-dnsimple",
+    "certbot-dns-dnsmadeeasy",
+    "certbot-dns-gehirn",
+    "certbot-dns-google",
+    "certbot-dns-linode",
+    "certbot-dns-luadns",
+    "certbot-dns-nsone",
+    "certbot-dns-ovh",
+    "certbot-dns-rfc2136",
+    "certbot-dns-route53",
+    "certbot-dns-sakuracloud",
+    "certbot-nginx",
+]
+"""Distributions for which prefix will be omitted."""
+
 
 class PluginEntryPoint(object):
     """Plugin entry point."""
 
-    PREFIX_FREE_DISTRIBUTIONS = [
-        "certbot",
-        "certbot-apache",
-        "certbot-dns-cloudflare",
-        "certbot-dns-cloudxns",
-        "certbot-dns-digitalocean",
-        "certbot-dns-dnsimple",
-        "certbot-dns-dnsmadeeasy",
-        "certbot-dns-gehirn",
-        "certbot-dns-google",
-        "certbot-dns-linode",
-        "certbot-dns-luadns",
-        "certbot-dns-nsone",
-        "certbot-dns-ovh",
-        "certbot-dns-rfc2136",
-        "certbot-dns-route53",
-        "certbot-dns-sakuracloud",
-        "certbot-nginx",
-    ]
-    """Distributions for which prefix will be omitted."""
-
     # this object is mutable, don't allow it to be hashed!
     __hash__ = None  # type: ignore
 
-    def __init__(self, entry_point):
-        self.name = self.entry_point_to_plugin_name(entry_point)
+    def __init__(self, entry_point, with_prefix=False):
+        self.name = self.entry_point_to_plugin_name(entry_point, with_prefix)
         self.plugin_cls = entry_point.load()
         self.entry_point = entry_point
+        self.warning_message = None
         self._initialized = None
         self._prepared = None
+        self._hidden = False
+        self._long_description = None
+
+    def check_name(self, name):
+        """Check if the name refers to this plugin."""
+        if name == self.name:
+            if self.warning_message:
+                logger.warning(self.warning_message)
+            return True
+        return False
 
     @classmethod
-    def entry_point_to_plugin_name(cls, entry_point):
+    def entry_point_to_plugin_name(cls, entry_point, with_prefix):
         """Unique plugin name for an ``entry_point``"""
-        if entry_point.dist.key in cls.PREFIX_FREE_DISTRIBUTIONS:
-            return entry_point.name
-        return entry_point.dist.key + ":" + entry_point.name
+        if with_prefix:
+            return entry_point.dist.key + ":" + entry_point.name
+        return entry_point.name
 
     @property
     def description(self):
@@ -78,15 +89,25 @@ def description_with_name(self):
     @property
     def long_description(self):
         """Long description of the plugin."""
+        if self._long_description:
+            return self._long_description
         try:
             return self.plugin_cls.long_description
         except AttributeError:
             return self.description
 
+    @long_description.setter
+    def long_description(self, description):
+        self._long_description = description
+
     @property
     def hidden(self):
         """Should this plugin be hidden from UI?"""
-        return getattr(self.plugin_cls, "hidden", False)
+        return self._hidden or getattr(self.plugin_cls, "hidden", False)
+
+    @hidden.setter
+    def hidden(self, hide):
+        self._hidden = hide
 
     def ifaces(self, *ifaces_groups):
         """Does plugin implements specified interface groups?"""
@@ -212,16 +233,34 @@ def find_all(cls):
             pkg_resources.iter_entry_points(
                 constants.OLD_SETUPTOOLS_PLUGINS_ENTRY_POINT),)
         for entry_point in entry_points:
-            plugin_ep = PluginEntryPoint(entry_point)
-            assert plugin_ep.name not in plugins, (
-                "PREFIX_FREE_DISTRIBUTIONS messed up")
-            if interfaces.IPluginFactory.providedBy(plugin_ep.plugin_cls):
-                plugins[plugin_ep.name] = plugin_ep
-            else:  # pragma: no cover
-                logger.warning(
-                    "%r does not provide IPluginFactory, skipping", plugin_ep)
+            plugin_ep = cls._load_entry_point(entry_point, plugins, with_prefix=False)
+            if entry_point.dist.key not in PREFIX_FREE_DISTRIBUTIONS:
+                prefixed_plugin_ep = cls._load_entry_point(entry_point, plugins, with_prefix=True)
+                prefixed_plugin_ep.hidden = True
+                message = (
+                    "Plugin legacy name {0} may be removed in a future version. "
+                    "Please use {1} instead.").format(prefixed_plugin_ep.name, plugin_ep.name)
+                prefixed_plugin_ep.warning_message = message
+                prefixed_plugin_ep.long_description = "(WARNING: {0}) {1}".format(
+                    message, prefixed_plugin_ep.long_description)
+
         return cls(plugins)
 
+    @classmethod
+    def _load_entry_point(cls, entry_point, plugins, with_prefix):
+        plugin_ep = PluginEntryPoint(entry_point, with_prefix)
+        if plugin_ep.name in plugins:
+            other_ep = plugins[plugin_ep.name]
+            raise Exception("Duplicate plugin name {0} from {1} and {2}.".format(
+                plugin_ep.name, plugin_ep.entry_point.dist.key, other_ep.entry_point.dist.key))
+        if interfaces.IPluginFactory.providedBy(plugin_ep.plugin_cls):
+            plugins[plugin_ep.name] = plugin_ep
+        else:  # pragma: no cover
+            logger.warning(
+                "%r does not provide IPluginFactory, skipping", plugin_ep)
+
+        return plugin_ep
+
     def __getitem__(self, name):
         return self._plugins[name]
 
diff --git a/certbot/certbot/_internal/plugins/selection.py b/certbot/certbot/_internal/plugins/selection.py
index 53cef3969f1..0b04791c6e9 100644
--- a/certbot/certbot/_internal/plugins/selection.py
+++ b/certbot/certbot/_internal/plugins/selection.py
@@ -38,6 +38,7 @@ def pick_authenticator(
     return pick_plugin(
         config, default, plugins, question, (interfaces.IAuthenticator,))
 
+
 def get_unprepared_installer(config, plugins):
     """
     Get an unprepared interfaces.IInstaller object.
@@ -53,7 +54,7 @@ def get_unprepared_installer(config, plugins):
     _, req_inst = cli_plugin_requests(config)
     if not req_inst:
         return None
-    installers = plugins.filter(lambda p_ep: p_ep.name == req_inst)
+    installers = plugins.filter(lambda p_ep: p_ep.check_name(req_inst))
     installers.init(config)
     installers = installers.verify((interfaces.IInstaller,))
     if len(installers) > 1:
@@ -67,6 +68,7 @@ def get_unprepared_installer(config, plugins):
     raise errors.PluginSelectionError(
         "Could not select or initialize the requested installer %s." % req_inst)
 
+
 def pick_plugin(config, default, plugins, question, ifaces):
     """Pick plugin.
 
@@ -84,7 +86,7 @@ def pick_plugin(config, default, plugins, question, ifaces):
     """
     if default is not None:
         # throw more UX-friendly error if default not in plugins
-        filtered = plugins.filter(lambda p_ep: p_ep.name == default)
+        filtered = plugins.filter(lambda p_ep: p_ep.check_name(default))
     else:
         if config.noninteractive_mode:
             # it's really bad to auto-select the single available plugin in
diff --git a/certbot/tests/plugins/disco_test.py b/certbot/tests/plugins/disco_test.py
index 5a0a392b0d2..ed13544deaf 100644
--- a/certbot/tests/plugins/disco_test.py
+++ b/certbot/tests/plugins/disco_test.py
@@ -5,7 +5,7 @@
 
 try:
     import mock
-except ImportError: # pragma: no cover
+except ImportError:  # pragma: no cover
     from unittest import mock
 import pkg_resources
 import six
@@ -13,6 +13,7 @@
 
 from certbot import errors
 from certbot import interfaces
+from certbot._internal.plugins import null
 from certbot._internal.plugins import standalone
 from certbot._internal.plugins import webroot
 
@@ -44,7 +45,22 @@ def setUp(self):
         from certbot._internal.plugins.disco import PluginEntryPoint
         self.plugin_ep = PluginEntryPoint(EP_SA)
 
-    def test_entry_point_to_plugin_name(self):
+    def test_entry_point_to_plugin_name_not_prefixed(self):
+        from certbot._internal.plugins.disco import PluginEntryPoint
+
+        names = {
+            self.ep1: "ep1",
+            self.ep1prim: "ep1",
+            self.ep2: "ep2",
+            self.ep3: "ep3",
+            EP_SA: "sa",
+        }
+
+        for entry_point, name in six.iteritems(names):
+            self.assertEqual(
+                name, PluginEntryPoint.entry_point_to_plugin_name(entry_point, with_prefix=False))
+
+    def test_entry_point_to_plugin_name_prefixed(self):
         from certbot._internal.plugins.disco import PluginEntryPoint
 
         names = {
@@ -52,12 +68,11 @@ def test_entry_point_to_plugin_name(self):
             self.ep1prim: "p2:ep1",
             self.ep2: "p2:ep2",
             self.ep3: "p3:ep3",
-            EP_SA: "sa",
         }
 
         for entry_point, name in six.iteritems(names):
             self.assertEqual(
-                name, PluginEntryPoint.entry_point_to_plugin_name(entry_point))
+                name, PluginEntryPoint.entry_point_to_plugin_name(entry_point, with_prefix=True))
 
     def test_description(self):
         self.assertTrue("temporary webserver" in self.plugin_ep.description)
@@ -197,17 +212,28 @@ def setUp(self):
         self.plugin_ep.__hash__.side_effect = TypeError
         self.plugins = {self.plugin_ep.name: self.plugin_ep}
         self.reg = self._create_new_registry(self.plugins)
+        self.ep1 = pkg_resources.EntryPoint(
+            "ep1", "p1.ep1", dist=mock.MagicMock(key="p1"))
 
     def test_find_all(self):
         from certbot._internal.plugins.disco import PluginsRegistry
         with mock.patch("certbot._internal.plugins.disco.pkg_resources") as mock_pkg:
-            mock_pkg.iter_entry_points.side_effect = [iter([EP_SA]),
-                                                      iter([EP_WR])]
-            plugins = PluginsRegistry.find_all()
+            mock_pkg.iter_entry_points.side_effect = [
+                iter([EP_SA]), iter([EP_WR, self.ep1])
+            ]
+            with mock.patch.object(pkg_resources.EntryPoint, 'load') as mock_load:
+                mock_load.side_effect = [
+                    standalone.Authenticator, webroot.Authenticator,
+                    null.Installer, null.Installer]
+                plugins = PluginsRegistry.find_all()
         self.assertTrue(plugins["sa"].plugin_cls is standalone.Authenticator)
         self.assertTrue(plugins["sa"].entry_point is EP_SA)
         self.assertTrue(plugins["wr"].plugin_cls is webroot.Authenticator)
         self.assertTrue(plugins["wr"].entry_point is EP_WR)
+        self.assertTrue(plugins["ep1"].plugin_cls is null.Installer)
+        self.assertTrue(plugins["ep1"].entry_point is self.ep1)
+        self.assertTrue(plugins["p1:ep1"].plugin_cls is null.Installer)
+        self.assertTrue(plugins["p1:ep1"].entry_point is self.ep1)
 
     def test_getitem(self):
         self.assertEqual(self.plugin_ep, self.reg["mock"])
diff --git a/certbot/tests/plugins/selection_test.py b/certbot/tests/plugins/selection_test.py
index e5e6db031f8..a5de99e6080 100644
--- a/certbot/tests/plugins/selection_test.py
+++ b/certbot/tests/plugins/selection_test.py
@@ -174,6 +174,7 @@ def test_new_interaction_avoidance(self, mock_util):
 
         self.assertTrue("default" in mock_util().menu.call_args[1])
 
+
 class GetUnpreparedInstallerTest(test_util.ConfigTestCase):
     """Tests for certbot._internal.plugins.selection.get_unprepared_installer."""
 
@@ -181,10 +182,10 @@ 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_fail_ep.check_name = lambda name: name == "afail"
         self.mock_apache_ep = mock.Mock(
             description_with_name="apache")
-        self.mock_apache_ep.name = "apache"
+        self.mock_apache_ep.check_name = lambda name: name == "apache"
         self.mock_apache_plugin = mock.MagicMock()
         self.mock_apache_ep.init.return_value = self.mock_apache_plugin
         self.plugins = PluginsRegistry({
@@ -213,7 +214,7 @@ def test_get_plugin(self):
     def test_multiple_installers_returned(self):
         self.config.configurator = "apache"
         # Two plugins with the same name
-        self.mock_apache_fail_ep.name = "apache"
+        self.mock_apache_fail_ep.check_name = lambda name: name == "apache"
         self.assertRaises(errors.PluginSelectionError, self._call)
 
 


Code review comments:
diff_hunk: @@ -15,6 +15,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Added `--preferred-chain <issuer CN>`. If a CA offers multiple certificate chains,
   it may be  used to indicate to Certbot which chain should be preferred.
   * e.g. `--preferred-chain "DST Root CA X3"`
+* Third-party plugins can be used without prefix (`plugin_name` instead of `dist_name:plugin_name`):
path: certbot/CHANGELOG.md
text: @user1:
I think git is going to incorrectly add this to the changelog for our 1.6.0 release rather than the 1.7.0 master section. I think we should merge master into this PR and fix this.

@author:
Done.