Merge pull request #1197 from meejah/3899.failed-server

Produce better user feedback when there are misconfigured plugins

newsfragments/3899.bugfix

@@ -0,0 +1,4 @@
+Provide better feedback from plugin configuration errors
+
+Local errors now print a useful message and exit.
+Announcements that only contain invalid / unusable plugins now show a message in the Welcome page.

setup.py

@@ -118,10 +118,10 @@ install_requires = [
     "pyrsistent",
 
     # A great way to define types of values.
-    "attrs >= 18.2.0",
+    "attrs >= 20.1.0",
 
     # WebSocket library for twisted and asyncio
-    "autobahn",
+    "autobahn >= 22.4.3",
 
     # Support for Python 3 transition
     "future >= 0.18.2",

src/allmydata/client.py

@@ -483,6 +483,11 @@ def create_storage_farm_broker(config: _Config, default_connection_handlers, foo
     storage_client_config = storage_client.StorageClientConfig.from_node_config(
         config,
     )
+    # ensure that we can at least load all plugins that the
+    # configuration mentions; doing this early (i.e. before creating
+    # storage-clients themselves) allows us to exit in case of a
+    # problem.
+    storage_client_config.get_configured_storage_plugins()
 
     def tub_creator(handler_overrides=None, **kwargs):
         return node.create_tub(

src/allmydata/scripts/tahoe_run.py

@@ -42,6 +42,9 @@ from allmydata.util.pid import (
 from allmydata.storage.crawler import (
     MigratePickleFileError,
 )
+from allmydata.storage_client import (
+    MissingPlugin,
+)
 from allmydata.node import (
     PortAssignmentRequired,
     PrivacyError,
@@ -197,6 +200,17 @@ class DaemonizeTheRealService(Service, HookMixin):
                             self.basedir,
                         )
                     )
+                elif reason.check(MissingPlugin):
+                    self.stderr.write(
+                        "Missing Plugin\n"
+                        "The configuration requests a plugin:\n"
+                        "\n    {}\n\n"
+                        "...which cannot be found.\n"
+                        "This typically means that some software hasn't been installed or the plugin couldn't be instantiated.\n\n"
+                        .format(
+                            reason.value.plugin_name,
+                        )
+                    )
                 else:
                     self.stderr.write("\nUnknown error, here's the traceback:\n")
                     reason.printTraceback(self.stderr)

src/allmydata/storage_client.py

@@ -33,7 +33,7 @@ Ported to Python 3.
 from __future__ import annotations
 
 from six import ensure_text
-from typing import Union, Callable, Any, Optional, cast
+from typing import Union, Callable, Any, Optional, cast, Dict
 from os import urandom
 import re
 import time
@@ -43,6 +43,7 @@ from configparser import NoSectionError
 import json
 
 import attr
+from attr import define
 from hyperlink import DecodedURL
 from twisted.web.client import HTTPConnectionPool
 from zope.interface import (
@@ -56,12 +57,14 @@ from twisted.internet.task import LoopingCall
 from twisted.internet import defer, reactor
 from twisted.internet.interfaces import IReactorTime
 from twisted.application import service
+from twisted.logger import Logger
 from twisted.plugin import (
     getPlugins,
 )
 from eliot import (
     log_call,
 )
+from foolscap.ipb import IRemoteReference
 from foolscap.api import eventually, RemoteException
 from foolscap.reconnector import (
     ReconnectionInfo,
@@ -97,6 +100,8 @@ from allmydata.storage.http_client import (
 )
 from .node import _Config
 
+_log = Logger()
+
 ANONYMOUS_STORAGE_NURLS = "anonymous-storage-NURLs"
 
 
@@ -182,6 +187,31 @@ class StorageClientConfig(object):
             grid_manager_keys,
         )
 
+    def get_configured_storage_plugins(self) -> dict[str, IFoolscapStoragePlugin]:
+        """
+        :returns: a mapping from names to instances for all available
+            plugins
+
+        :raises MissingPlugin: if the configuration asks for a plugin
+            for which there is no corresponding instance (e.g. it is
+            not installed).
+        """
+        plugins = {
+            plugin.name: plugin
+            for plugin
+            in getPlugins(IFoolscapStoragePlugin)
+        }
+
+        # mypy doesn't like "str" in place of Any ...
+        configured: Dict[Any, IFoolscapStoragePlugin] = dict()
+        for plugin_name in self.storage_plugins:
+            try:
+                plugin = plugins[plugin_name]
+            except KeyError:
+                raise MissingPlugin(plugin_name)
+            configured[plugin_name] = plugin
+        return configured
+
 
 @implementer(IStorageBroker)
 class StorageFarmBroker(service.MultiService):
@@ -710,6 +740,7 @@ class _FoolscapStorage(object):
 
 
 @implementer(IFoolscapStorageServer)
+@define
 class _NullStorage(object):
     """
     Abstraction for *not* communicating with a storage server of a type with
@@ -723,7 +754,7 @@ class _NullStorage(object):
     lease_seed = hashlib.sha256(b"").digest()
 
     name = "<unsupported>"
-    longname = "<storage with unsupported protocol>"
+    longname: str = "<storage with unsupported protocol>"
 
     def connect_to(self, tub, got_connection):
         return NonReconnector()
@@ -742,8 +773,6 @@ class NonReconnector(object):
     def getReconnectionInfo(self):
         return ReconnectionInfo()
 
-_null_storage = _NullStorage()
-
 
 class AnnouncementNotMatched(Exception):
     """
@@ -752,6 +781,18 @@ class AnnouncementNotMatched(Exception):
     """
 
 
+@attr.s(auto_exc=True)
+class MissingPlugin(Exception):
+    """
+    A particular plugin was requested but is missing
+    """
+
+    plugin_name = attr.ib()
+
+    def __str__(self):
+        return "Missing plugin '{}'".format(self.plugin_name)
+
+
 def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref):
     """
     Construct an ``IStorageServer`` from the most locally-preferred plugin
@@ -759,27 +800,37 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref):
 
     :param allmydata.node._Config node_config: The node configuration to
         pass to the plugin.
+
+    :param dict announcement: The storage announcement for the storage
+        server we should build
     """
-    plugins = {
-        plugin.name: plugin
-        for plugin
-        in getPlugins(IFoolscapStoragePlugin)
-    }
     storage_options = announcement.get(u"storage-options", [])
-    for plugin_name, plugin_config in list(config.storage_plugins.items()):
+    plugins = config.get_configured_storage_plugins()
+
+    # for every storage-option that we have enabled locally (in order
+    # of preference), see if the announcement asks for such a thing.
+    # if it does, great: we return that storage-client
+    # otherwise we've run out of options...
+
+    for options in storage_options:
         try:
-            plugin = plugins[plugin_name]
+            plugin = plugins[options[u"name"]]
         except KeyError:
-            raise ValueError("{} not installed".format(plugin_name))
+            # we didn't configure this kind of plugin locally, so
-        for option in storage_options:
+            # consider the next announced option
-            if plugin_name == option[u"name"]:
+            continue
-                furl = option[u"storage-server-FURL"]
+
-                return furl, plugin.get_storage_client(
+        furl = options[u"storage-server-FURL"]
-                    node_config,
+        return furl, plugin.get_storage_client(
-                    option,
+            node_config,
-                    get_rref,
+            options,
-                )
+            get_rref,
-    raise AnnouncementNotMatched()
+        )
+
+    # none of the storage options in the announcement are configured
+    # locally; we can't make a storage-client.
+    plugin_names = ", ".join(sorted(option["name"] for option in storage_options))
+    raise AnnouncementNotMatched(plugin_names)
 
 
 def _available_space_from_version(version):
@@ -792,6 +843,83 @@ def _available_space_from_version(version):
     return available_space
 
 
+def _make_storage_system(
+        node_config: _Config,
+        config: StorageClientConfig,
+        ann: dict,
+        server_id: bytes,
+        get_rref: Callable[[], Optional[IRemoteReference]],
+) -> IFoolscapStorageServer:
+    """
+    Create an object for interacting with the storage server described by
+    the given announcement.
+
+    :param node_config: The node configuration to pass to any configured
+        storage plugins.
+
+    :param config: Configuration specifying desired storage client behavior.
+
+    :param ann: The storage announcement from the storage server we are meant
+        to communicate with.
+
+    :param server_id: The unique identifier for the server.
+
+    :param get_rref: A function which returns a remote reference to the
+        server-side object which implements this storage system, if one is
+        available (otherwise None).
+
+    :return: An object enabling communication via Foolscap with the server
+        which generated the announcement.
+    """
+    unmatched = None
+    # Try to match the announcement against a plugin.
+    try:
+        furl, storage_server = _storage_from_foolscap_plugin(
+            node_config,
+            config,
+            ann,
+            # Pass in an accessor for our _rref attribute.  The value of
+            # the attribute may change over time as connections are lost
+            # and re-established.  The _StorageServer should always be
+            # able to get the most up-to-date value.
+            get_rref,
+        )
+    except AnnouncementNotMatched as e:
+        # show a more-specific error to the user for this server
+        # (Note this will only be shown if the server _doesn't_ offer
+        # anonymous service, which will match below)
+        unmatched = _NullStorage('{}: missing plugin "{}"'.format(server_id.decode("utf8"), str(e)))
+    else:
+        return _FoolscapStorage.from_announcement(
+            server_id,
+            furl,
+            ann,
+            storage_server,
+        )
+
+    # Try to match the announcement against the anonymous access scheme.
+    try:
+        furl = ann[u"anonymous-storage-FURL"]
+    except KeyError:
+        # Nope
+        pass
+    else:
+        # See comment above for the _storage_from_foolscap_plugin case
+        # about passing in get_rref.
+        storage_server = _StorageServer(get_rref=get_rref)
+        return _FoolscapStorage.from_announcement(
+            server_id,
+            furl,
+            ann,
+            storage_server,
+        )
+
+    # Nothing matched so we can't talk to this server. (There should
+    # not be a way to get here without this local being valid)
+    assert unmatched is not None, "Expected unmatched plugin error"
+    return unmatched
+
+
 @implementer(IServer)
 class NativeStorageServer(service.MultiService):
     """I hold information about a storage server that we want to connect to.
@@ -833,7 +961,7 @@ class NativeStorageServer(service.MultiService):
 
         self._grid_manager_verifier = grid_manager_verifier
 
-        self._storage = self._make_storage_system(node_config, config, ann)
+        self._storage = _make_storage_system(node_config, config, ann, self._server_id, self.get_rref)
 
         self.last_connect_time = None
         self.last_loss_time = None
@@ -858,63 +986,6 @@ class NativeStorageServer(service.MultiService):
             return True
         return self._grid_manager_verifier()
 
-    def _make_storage_system(self, node_config, config, ann):
-        """
-        :param allmydata.node._Config node_config: The node configuration to pass
-            to any configured storage plugins.
-
-        :param StorageClientConfig config: Configuration specifying desired
-            storage client behavior.
-
-        :param dict ann: The storage announcement from the storage server we
-            are meant to communicate with.
-
-        :return IFoolscapStorageServer: An object enabling communication via
-            Foolscap with the server which generated the announcement.
-        """
-        # Try to match the announcement against a plugin.
-        try:
-            furl, storage_server = _storage_from_foolscap_plugin(
-                node_config,
-                config,
-                ann,
-                # Pass in an accessor for our _rref attribute.  The value of
-                # the attribute may change over time as connections are lost
-                # and re-established.  The _StorageServer should always be
-                # able to get the most up-to-date value.
-                self.get_rref,
-            )
-        except AnnouncementNotMatched:
-            # Nope.
-            pass
-        else:
-            return _FoolscapStorage.from_announcement(
-                self._server_id,
-                furl,
-                ann,
-                storage_server,
-            )
-
-        # Try to match the announcement against the anonymous access scheme.
-        try:
-            furl = ann[u"anonymous-storage-FURL"]
-        except KeyError:
-            # Nope
-            pass
-        else:
-            # See comment above for the _storage_from_foolscap_plugin case
-            # about passing in get_rref.
-            storage_server = _StorageServer(get_rref=self.get_rref)
-            return _FoolscapStorage.from_announcement(
-                self._server_id,
-                furl,
-                ann,
-                storage_server,
-            )
-
-        # Nothing matched so we can't talk to this server.
-        return _null_storage
-
     def get_permutation_seed(self):
         return self._storage.permutation_seed
     def get_name(self): # keep methodname short

src/allmydata/test/cli/test_run.py

@@ -264,7 +264,7 @@ class RunTests(SyncTestCase):
         self.assertThat(runs, Equals([]))
         self.assertThat(result_code, Equals(1))
 
-    good_file_content_re = re.compile(r"\w[0-9]*\w[0-9]*\w")
+    good_file_content_re = re.compile(r"\s[0-9]*\s[0-9]*\s", re.M)
 
     @given(text())
     def test_pidfile_contents(self, content):

src/allmydata/test/common.py

@@ -307,13 +307,17 @@ class UseNode(object):
         if self.plugin_config is None:
             plugin_config_section = ""
         else:
-            plugin_config_section = """
+            plugin_config_section = (
-[storageclient.plugins.{storage_plugin}]
+                "[storageclient.plugins.{storage_plugin}]\n"
-{config}
+                "{config}\n").format(
-""".format(
+                    storage_plugin=self.storage_plugin,
-    storage_plugin=self.storage_plugin,
+                    config=format_config_items(self.plugin_config),
-    config=format_config_items(self.plugin_config),
+                )
-)
+
+        if self.storage_plugin is None:
+            plugins = ""
+        else:
+            plugins = "storage.plugins = {}".format(self.storage_plugin)
 
         write_introducer(
             self.basedir,
@@ -340,18 +344,17 @@ class UseNode(object):
         self.config = config_from_string(
             self.basedir.asTextMode().path,
             "tub.port",
-"""
+            "[node]\n"
-[node]
+            "{node_config}\n"
-{node_config}
+            "\n"
-
+            "[client]\n"
-[client]
+            "{plugins}\n"
-storage.plugins = {storage_plugin}
+            "{plugin_config_section}\n"
-{plugin_config_section}
+            .format(
-""".format(
+                plugins=plugins,
-    storage_plugin=self.storage_plugin,
+                node_config=format_config_items(node_config),
-    node_config=format_config_items(node_config),
+                plugin_config_section=plugin_config_section,
-    plugin_config_section=plugin_config_section,
+            )
-)
         )
 
     def create_node(self):

src/allmydata/test/test_storage_client.py

@@ -8,7 +8,7 @@ from json import (
     loads,
 )
 import hashlib
-from typing import Union, Any
+from typing import Union, Any, Optional
 
 from hyperlink import DecodedURL
 from fixtures import (
@@ -89,6 +89,8 @@ from allmydata.storage_client import (
     IFoolscapStorageServer,
     NativeStorageServer,
     StorageFarmBroker,
+    StorageClientConfig,
+    MissingPlugin,
     _FoolscapStorage,
     _NullStorage,
     _pick_a_http_server,
@@ -170,16 +172,21 @@ class UnrecognizedAnnouncement(unittest.TestCase):
     an announcement generated by a storage server plugin which is not loaded
     in the client.
     """
+    plugin_name = u"tahoe-lafs-testing-v1"
     ann = {
-        u"name": u"tahoe-lafs-testing-v1",
+        u"storage-options": [
-        u"any-parameter": 12345,
+            {
+                u"name": plugin_name,
+                u"any-parameter": 12345,
+            },
+        ],
     }
     server_id = b"abc"
 
     def _tub_maker(self, overrides):
         return Service()
 
-    def native_storage_server(self):
+    def native_storage_server(self, config: Optional[StorageClientConfig] = None) -> NativeStorageServer:
         """
         Make a ``NativeStorageServer`` out of an unrecognizable announcement.
         """
@@ -188,7 +195,8 @@ class UnrecognizedAnnouncement(unittest.TestCase):
             self.ann,
             self._tub_maker,
             {},
-            EMPTY_CLIENT_CONFIG,
+            node_config=EMPTY_CLIENT_CONFIG,
+            config=config if config is not None else StorageClientConfig(),
         )
 
     def test_no_exceptions(self):
@@ -235,6 +243,18 @@ class UnrecognizedAnnouncement(unittest.TestCase):
         server.get_foolscap_write_enabler_seed()
         server.get_nickname()
 
+    def test_missing_plugin(self) -> None:
+        """
+        An exception is produced if the plugin is missing
+        """
+        with self.assertRaises(MissingPlugin):
+            self.native_storage_server(
+                StorageClientConfig(
+                    storage_plugins={
+                        "missing-plugin-name": {}
+                    }
+                )
+            )
 
 
 class PluginMatchedAnnouncement(SyncTestCase):