Update federation routing logic to check .well-known before SRV

3 files changed, 27 insertions, 35 deletions

diff --git a/changelog.d/4539.misc b/changelog.d/4539.misc
new file mode 100644
index 0000000000..b222c8d23f
--- /dev/null
+++ b/changelog.d/4539.misc
@@ -0,0 +1 @@
+Update federation routing logic to check .well-known before SRV
diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py
index 26649e70be..57dcab4727 100644
--- a/synapse/http/federation/matrix_federation_agent.py
+++ b/synapse/http/federation/matrix_federation_agent.py
@@ -210,11 +210,7 @@ class MatrixFederationAgent(object):
                 target_port=parsed_uri.port,
             ))
 
-        # try a SRV lookup
-        service_name = b"_matrix._tcp.%s" % (parsed_uri.host,)
-        server_list = yield self._srv_resolver.resolve_service(service_name)
-
-        if not server_list and lookup_well_known:
+        if lookup_well_known:
             # try a .well-known lookup
             well_known_server = yield self._get_well_known(parsed_uri.host)
 
@@ -250,6 +246,10 @@ class MatrixFederationAgent(object):
                 res = yield self._route_matrix_uri(new_uri, lookup_well_known=False)
                 defer.returnValue(res)
 
+        # try a SRV lookup
+        service_name = b"_matrix._tcp.%s" % (parsed_uri.host,)
+        server_list = yield self._srv_resolver.resolve_service(service_name)
+
         if not server_list:
             target_host = parsed_uri.host
             port = 8448
diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py
index 7b2800021f..369e63ecc6 100644
--- a/tests/http/federation/test_matrix_federation_agent.py
+++ b/tests/http/federation/test_matrix_federation_agent.py
@@ -358,9 +358,8 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
+        # No SRV record lookup yet
+        self.mock_resolver.resolve_service.assert_not_called()
 
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
@@ -376,6 +375,11 @@ class MatrixFederationAgentTests(TestCase):
         # .well-known request fails.
         self.reactor.pump((0.4,))
 
+        # now there should be a SRV lookup
+        self.mock_resolver.resolve_service.assert_called_once_with(
+            b"_matrix._tcp.testserv",
+        )
+
         # we should fall back to a direct connection
         self.assertEqual(len(clients), 2)
         (host, port, client_factory, _timeout, _bindAddress) = clients[1]
@@ -403,8 +407,7 @@ class MatrixFederationAgentTests(TestCase):
         self.successResultOf(test_d)
 
     def test_get_well_known(self):
-        """Test the behaviour when the server name has no port and no SRV record, but
-        the .well-known redirects elsewhere
+        """Test the behaviour when the .well-known redirects elsewhere
         """
 
         self.mock_resolver.resolve_service.side_effect = lambda _: []
@@ -416,11 +419,6 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
-        self.mock_resolver.resolve_service.reset_mock()
-
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 1)
@@ -432,7 +430,7 @@ class MatrixFederationAgentTests(TestCase):
             client_factory, expected_sni=b"testserv", target_server=b"target-server",
         )
 
-        # there should be another SRV lookup
+        # there should be a SRV lookup
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.target-server",
         )
@@ -483,11 +481,6 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
-        self.mock_resolver.resolve_service.reset_mock()
-
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 1)
@@ -529,7 +522,7 @@ class MatrixFederationAgentTests(TestCase):
 
         self.reactor.pump((0.1, ))
 
-        # there should be another SRV lookup
+        # there should be a SRV lookup
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.target-server",
         )
@@ -581,6 +574,7 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
+        # the request for a .well-known will have failed with a DNS lookup error.
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.testserv",
         )
@@ -613,11 +607,9 @@ class MatrixFederationAgentTests(TestCase):
         self.successResultOf(test_d)
 
     def test_get_well_known_srv(self):
-        """Test the behaviour when the server name has no port and no SRV record, but
-        the .well-known redirects to a place where there is a SRV.
+        """Test the behaviour when the .well-known redirects to a place where there
+        is a SRV.
         """
-
-        self.mock_resolver.resolve_service.side_effect = lambda _: []
         self.reactor.lookups["testserv"] = "1.2.3.4"
         self.reactor.lookups["srvtarget"] = "5.6.7.8"
 
@@ -626,11 +618,6 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.testserv",
-        )
-        self.mock_resolver.resolve_service.reset_mock()
-
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 1)
@@ -646,7 +633,7 @@ class MatrixFederationAgentTests(TestCase):
             client_factory, expected_sni=b"testserv", target_server=b"target-server",
         )
 
-        # there should be another SRV lookup
+        # there should be a SRV lookup
         self.mock_resolver.resolve_service.assert_called_once_with(
             b"_matrix._tcp.target-server",
         )
@@ -691,9 +678,8 @@ class MatrixFederationAgentTests(TestCase):
         # Nothing happened yet
         self.assertNoResult(test_d)
 
-        self.mock_resolver.resolve_service.assert_called_once_with(
-            b"_matrix._tcp.xn--bcher-kva.com",
-        )
+        # No SRV record lookup yet
+        self.mock_resolver.resolve_service.assert_not_called()
 
         # there should be an attempt to connect on port 443 for the .well-known
         clients = self.reactor.tcpClients
@@ -709,6 +695,11 @@ class MatrixFederationAgentTests(TestCase):
         # .well-known request fails.
         self.reactor.pump((0.4,))
 
+        # now there should have been a SRV lookup
+        self.mock_resolver.resolve_service.assert_called_once_with(
+            b"_matrix._tcp.xn--bcher-kva.com",
+        )
+
         # We should fall back to port 8448
         clients = self.reactor.tcpClients
         self.assertEqual(len(clients), 2)