Merge pull request #621 from tahoe-lafs/3051.handle-weird-announcements

Handle weird static server "announcements"

Fixes: ticket:3051

newsfragments/3051.feature

@@ -0,0 +1 @@
+Static storage server "announcements" in ``private/servers.yaml`` are now individually logged and ignored if they cannot be interpreted.

src/allmydata/client.py

@@ -8,7 +8,6 @@ from twisted.internet import reactor, defer
 from twisted.application import service
 from twisted.application.internet import TimerService
 from twisted.python.filepath import FilePath
-from twisted.python.failure import Failure
 from pycryptopp.publickey import rsa
 
 import allmydata
@@ -207,7 +206,7 @@ def create_client(basedir=u".", _client_factory=None):
             _client_factory=_client_factory,
         )
     except Exception:
-        return Failure()
+        return defer.fail()
 
 
 def create_client_from_config(config, _client_factory=None):
@@ -261,7 +260,7 @@ def create_client_from_config(config, _client_factory=None):
         storage_broker.setServiceParent(client)
         return defer.succeed(client)
     except Exception:
-        return Failure()
+        return defer.fail()
 
 
 def _sequencer(config):

src/allmydata/storage_client.py

@@ -34,7 +34,9 @@ import attr
 from zope.interface import implementer
 from twisted.internet import defer
 from twisted.application import service
-
+from eliot import (
+    log_call,
+)
 from foolscap.api import eventually
 from allmydata.interfaces import (
     IStorageBroker,
@@ -90,18 +92,36 @@ class StorageFarmBroker(service.MultiService):
         self._threshold_listeners = [] # tuples of (threshold, Deferred)
         self._connected_high_water_mark = 0
 
+    @log_call(action_type=u"storage-client:broker:set-static-servers")
     def set_static_servers(self, servers):
-        for (server_id, server) in servers.items():
+        # Sorting the items gives us a deterministic processing order.  This
-            assert isinstance(server_id, unicode) # from YAML
+        # doesn't really matter but it makes the logging behavior more
-            server_id = server_id.encode("ascii")
+        # predictable and easier to test (and at least one test does depend on
-            self._static_server_ids.add(server_id)
+        # this sorted order).
-            handler_overrides = server.get("connections", {})
+        for (server_id, server) in sorted(servers.items()):
-            s = NativeStorageServer(server_id, server["ann"],
+            try:
-                                    self._tub_maker, handler_overrides)
+                storage_server = self._make_storage_server(server_id, server)
-            s.on_status_changed(lambda _: self._got_connection())
+            except Exception:
-            s.setServiceParent(self)
+                pass
-            self.servers[server_id] = s
+            else:
-            s.start_connecting(self._trigger_connections)
+                self._static_server_ids.add(server_id)
+                self.servers[server_id] = storage_server
+                storage_server.setServiceParent(self)
+                storage_server.start_connecting(self._trigger_connections)
+
+    @log_call(
+        action_type=u"storage-client:broker:make-storage-server",
+        include_args=["server_id"],
+        include_result=False,
+    )
+    def _make_storage_server(self, server_id, server):
+        assert isinstance(server_id, unicode) # from YAML
+        server_id = server_id.encode("ascii")
+        handler_overrides = server.get("connections", {})
+        s = NativeStorageServer(server_id, server["ann"],
+                                self._tub_maker, handler_overrides)
+        s.on_status_changed(lambda _: self._got_connection())
+        return s
 
     def when_connected_enough(self, threshold):
         """
@@ -254,6 +274,7 @@ class StubServer(object):
     def get_nickname(self):
         return "?"
 
+
 @implementer(IServer)
 class NativeStorageServer(service.MultiService):
     """I hold information about a storage server that we want to connect to.

src/allmydata/test/test_client.py

@@ -1,9 +1,30 @@
 import os, sys
 import mock
 import twisted
+from yaml import (
+    safe_dump,
+)
+from fixtures import (
+    Fixture,
+    TempDir,
+)
+from eliot.testing import (
+    capture_logging,
+    assertHasAction,
+)
 from twisted.trial import unittest
 from twisted.application import service
 from twisted.internet import defer
+from twisted.python.filepath import (
+    FilePath,
+)
+from testtools.matchers import (
+    Equals,
+    AfterPreprocessing,
+)
+from testtools.twistedsupport import (
+    succeeded,
+)
 
 import allmydata
 import allmydata.frontends.magic_folder
@@ -20,6 +41,9 @@ from allmydata.interfaces import IFilesystemNode, IFileNode, \
      IImmutableFileNode, IMutableFileNode, IDirectoryNode
 from foolscap.api import flushEventualQueue
 import allmydata.test.common_util as testutil
+from allmydata.test.common import (
+    SyncTestCase,
+)
 
 
 BASECONFIG = ("[client]\n"
@@ -666,6 +690,143 @@ class IntroducerClients(unittest.TestCase):
         )
 
 
+def get_known_server_details(a_client):
+    """
+    Get some details about known storage servers from a client.
+
+    :param _Client a_client: The client to inspect.
+
+    :return: A ``list`` of two-tuples.  Each element of the list corresponds
+        to a "known server".  The first element of each tuple is a server id.
+        The second is the server's announcement.
+    """
+    return list(
+        (s.get_serverid(), s.get_announcement())
+        for s
+        in a_client.storage_broker.get_known_servers()
+    )
+
+
+class StaticServers(Fixture):
+    """
+    Create a ``servers.yaml`` file.
+    """
+    def __init__(self, basedir, server_details):
+        super(StaticServers, self).__init__()
+        self._basedir = basedir
+        self._server_details = server_details
+
+    def _setUp(self):
+        private = self._basedir.child(u"private")
+        private.makedirs()
+        servers = private.child(u"servers.yaml")
+        servers.setContent(safe_dump({
+            u"storage": {
+                serverid: {
+                    u"ann": announcement,
+                }
+                for (serverid, announcement)
+                in self._server_details
+            },
+        }))
+
+
+class StorageClients(SyncTestCase):
+    """
+    Tests for storage-related behavior of ``_Client``.
+    """
+    def setUp(self):
+        super(StorageClients, self).setUp()
+        # Some other tests create Nodes and Node mutates tempfile.tempdir and
+        # that screws us up because we're *not* making a Node.  "Fix" it.  See
+        # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3052 for the real fix,
+        # though.
+        import tempfile
+        tempfile.tempdir = None
+
+        tempdir = TempDir()
+        self.useFixture(tempdir)
+        self.basedir = FilePath(tempdir.path)
+
+    @capture_logging(
+        lambda case, logger: assertHasAction(
+            case,
+            logger,
+            actionType=u"storage-client:broker:set-static-servers",
+            succeeded=True,
+        ),
+    )
+    def test_static_servers(self, logger):
+        """
+        Storage servers defined in ``private/servers.yaml`` are loaded into the
+        storage broker.
+        """
+        serverid = u"v0-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+        announcement = {
+            u"nickname": u"some-storage-server",
+            u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum",
+        }
+        self.useFixture(
+            StaticServers(
+                self.basedir,
+                [(serverid, announcement)],
+            ),
+        )
+        self.assertThat(
+            client.create_client(self.basedir.asTextMode().path),
+            succeeded(
+                AfterPreprocessing(
+                    get_known_server_details,
+                    Equals([(serverid, announcement)]),
+                ),
+            ),
+        )
+
+    @capture_logging(
+        lambda case, logger: assertHasAction(
+            case,
+            logger,
+            actionType=u"storage-client:broker:make-storage-server",
+            succeeded=False,
+        ),
+    )
+    def test_invalid_static_server(self, logger):
+        """
+        An invalid announcement for a static server does not prevent other static
+        servers from being loaded.
+        """
+        # Some good details
+        serverid = u"v1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+        announcement = {
+            u"nickname": u"some-storage-server",
+            u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum",
+        }
+        self.useFixture(
+            StaticServers(
+                self.basedir,
+                [(serverid, announcement),
+                 # Along with a "bad" server announcement.  Order in this list
+                 # doesn't matter, yaml serializer and Python dicts are going
+                 # to shuffle everything around kind of randomly.
+                 (u"v0-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+                  {u"nickname": u"another-storage-server",
+                   u"anonymous-storage-FURL": None,
+                  }),
+                ],
+            ),
+        )
+        self.assertThat(
+            client.create_client(self.basedir.asTextMode().path),
+            succeeded(
+                AfterPreprocessing(
+                    get_known_server_details,
+                    # It should have the good server details.
+                    Equals([(serverid, announcement)]),
+                ),
+            ),
+        )
+
+
 class Run(unittest.TestCase, testutil.StallMixin):
 
     def setUp(self):