-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix tests on Twisted trunk. #16528
Fix tests on Twisted trunk. #16528
Changes from all commits
05282b3
c6afdde
f4efd04
69d0181
15107e6
5dcaea6
79c76e6
3467806
01d1e83
6dec5c3
412d593
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix running unit tests on Twisted trunk. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,18 +29,14 @@ | |
) | ||
from twisted.internet.interfaces import IProtocol, IProtocolFactory | ||
from twisted.internet.protocol import Factory, Protocol | ||
from twisted.protocols.tls import TLSMemoryBIOFactory, TLSMemoryBIOProtocol | ||
from twisted.protocols.tls import TLSMemoryBIOProtocol | ||
from twisted.web.http import HTTPChannel | ||
|
||
from synapse.http.client import BlocklistingReactorWrapper | ||
from synapse.http.connectproxyclient import BasicProxyCredentials | ||
from synapse.http.proxyagent import ProxyAgent, parse_proxy | ||
|
||
from tests.http import ( | ||
TestServerTLSConnectionFactory, | ||
dummy_address, | ||
get_test_https_policy, | ||
) | ||
from tests.http import dummy_address, get_test_https_policy, wrap_server_factory_for_tls | ||
from tests.server import FakeTransport, ThreadedMemoryReactorClock | ||
from tests.unittest import TestCase | ||
from tests.utils import checked_cast | ||
|
@@ -251,7 +247,9 @@ def _make_connection( | |
the server Protocol returned by server_factory | ||
""" | ||
if ssl: | ||
server_factory = _wrap_server_factory_for_tls(server_factory, tls_sanlist) | ||
server_factory = wrap_server_factory_for_tls( | ||
server_factory, self.reactor, tls_sanlist or [b"DNS:test.com"] | ||
) | ||
|
||
server_protocol = server_factory.buildProtocol(dummy_address) | ||
assert server_protocol is not None | ||
|
@@ -618,8 +616,8 @@ def _do_https_request_via_proxy( | |
request.finish() | ||
|
||
# now we make another test server to act as the upstream HTTP server. | ||
server_ssl_protocol = _wrap_server_factory_for_tls( | ||
_get_test_protocol_factory() | ||
server_ssl_protocol = wrap_server_factory_for_tls( | ||
_get_test_protocol_factory(), self.reactor, sanlist=[b"DNS:test.com"] | ||
Comment on lines
-621
to
+620
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is passing in an explicit sanlist here a meaningful change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto below in the rest of this test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No: |
||
).buildProtocol(dummy_address) | ||
|
||
# Tell the HTTP server to send outgoing traffic back via the proxy's transport. | ||
|
@@ -785,7 +783,9 @@ def test_https_request_via_uppercase_proxy_with_blocklist(self) -> None: | |
request.finish() | ||
|
||
# now we can replace the proxy channel with a new, SSL-wrapped HTTP channel | ||
ssl_factory = _wrap_server_factory_for_tls(_get_test_protocol_factory()) | ||
ssl_factory = wrap_server_factory_for_tls( | ||
_get_test_protocol_factory(), self.reactor, sanlist=[b"DNS:test.com"] | ||
) | ||
ssl_protocol = ssl_factory.buildProtocol(dummy_address) | ||
assert isinstance(ssl_protocol, TLSMemoryBIOProtocol) | ||
http_server = ssl_protocol.wrappedProtocol | ||
|
@@ -849,30 +849,6 @@ def test_proxy_with_https_scheme(self) -> None: | |
self.assertEqual(proxy_ep._wrappedEndpoint._port, 8888) | ||
|
||
|
||
def _wrap_server_factory_for_tls( | ||
factory: IProtocolFactory, sanlist: Optional[List[bytes]] = None | ||
) -> TLSMemoryBIOFactory: | ||
"""Wrap an existing Protocol Factory with a test TLSMemoryBIOFactory | ||
|
||
The resultant factory will create a TLS server which presents a certificate | ||
signed by our test CA, valid for the domains in `sanlist` | ||
|
||
Args: | ||
factory: protocol factory to wrap | ||
sanlist: list of domains the cert should be valid for | ||
|
||
Returns: | ||
interfaces.IProtocolFactory | ||
""" | ||
if sanlist is None: | ||
sanlist = [b"DNS:test.com"] | ||
|
||
connection_creator = TestServerTLSConnectionFactory(sanlist=sanlist) | ||
return TLSMemoryBIOFactory( | ||
connection_creator, isClient=False, wrappedFactory=factory | ||
) | ||
|
||
|
||
def _get_test_protocol_factory() -> IProtocolFactory: | ||
"""Get a protocol Factory which will build an HTTPChannel | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,9 +43,11 @@ | |
from unittest.mock import Mock | ||
|
||
import attr | ||
from incremental import Version | ||
from typing_extensions import ParamSpec | ||
from zope.interface import implementer | ||
|
||
import twisted | ||
from twisted.internet import address, tcp, threads, udp | ||
from twisted.internet._resolver import SimpleResolverComplexifier | ||
from twisted.internet.defer import Deferred, fail, maybeDeferred, succeed | ||
|
@@ -474,6 +476,16 @@ def getHostByName( | |
return fail(DNSLookupError("OH NO: unknown %s" % (name,))) | ||
return succeed(lookups[name]) | ||
|
||
# In order for the TLS protocol tests to work, modify _get_default_clock | ||
# on newer Twisted versions to use the test reactor's clock. | ||
# | ||
# This is *super* dirty since it is never undone and relies on the next | ||
# test to overwrite it. | ||
if twisted.version > Version("Twisted", 23, 8, 0): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels janky that we check It sounds like a Twisted 23.1x.0 will be out soon-ish though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think 23.8 is fine, just a note for completeness. |
||
from twisted.protocols import tls | ||
|
||
tls._get_default_clock = lambda: self # type: ignore[attr-defined] | ||
|
||
self.nameResolver = SimpleResolverComplexifier(FakeResolver()) | ||
super().__init__() | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did Twisted make a breaking change to TLSMemoryBIOFactory here, or are we just taking advantage of a newer API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I can't see anything in https://github.com/twisted/twisted/blob/trunk/NEWS.rst#deprecations-and-removals so assuming we're using a newer API.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't a breaking change, you don't have to provide
clock
, but you do if you want our tests to pass. (Otherwise it uses the trial reactor instead of the test reactor.)