This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix tests on Twisted trunk. #16528
Merged
Merged
Fix tests on Twisted trunk. #16528
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
05282b3
Combine duplicated code.
clokep c6afdde
Fix tests for Twisted > 23.8.0.
clokep f4efd04
Newsfragment
clokep 69d0181
Clarify comment.
clokep 15107e6
Merge remote-tracking branch 'origin/develop' into clokep/fix-twisted…
clokep 5dcaea6
TEMP: Test w/o patch.
clokep 79c76e6
Revert "TEMP: Test w/o patch."
clokep 3467806
Fix tests?
clokep 01d1e83
Another try.
clokep 6dec5c3
Lint
clokep 412d593
Lint
clokep File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix running unit tests on Twisted trunk. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.)