Wrap channel in an asyncio.Transport to eliminate loop back connection#303
Wrap channel in an asyncio.Transport to eliminate loop back connection#303bdraco wants to merge 106 commits intoNabuCasa:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Last step is to replace all the connector tests with ones that test connecting to the handler |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
=======================================
Coverage ? 84.59%
=======================================
Files ? 20
Lines ? 1298
Branches ? 127
=======================================
Hits ? 1098
Misses ? 168
Partials ? 32 ☔ View full report in Codecov by Sentry. |
51befd7 to
8757bea
Compare
27a149b to
04993e8
Compare
31128d2 to
ad98e36
Compare
8bcc0f4 to
ba155d2
Compare
ba155d2 to
3cc126a
Compare
|
Whatever changed in pytest-asyncio broke the test, and it's not immediately apparent what's wrong |
|
Almost working again except the test_connector tests |
|
we use abort in newer aiohttp .. and I didn't implement that |
There was a problem hiding this comment.
Pull request overview
This PR removes the loopback TCP connection previously used to bridge SniTun channels into aiohttp, replacing it with an asyncio.Transport wrapper (ChannelTransport) so TLS (loop.start_tls) and the aiohttp request handler can be layered directly on top of a MultiplexerChannel.
Changes:
- Introduces
ChannelTransportand updatesConnector/ConnectorHandlerto wire channels directly into TLS + protocol handling (no localhost socket). - Deprecates
endpoint_connection_error_callbackinSniTunClientAioHttp.start()and updates the aiohttp client implementation accordingly. - Adds/updates end-to-end and transport-level tests (including TLS sanity checks via
trustme) to validate backpressure/pause-resume and TLS behaviors.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
snitun/multiplexer/transport.py |
New asyncio.Transport wrapper around MultiplexerChannel used for direct TLS + protocol integration. |
snitun/client/connector.py |
Refactors connector to use ChannelTransport + start_tls and connect protocols directly (no loopback). |
snitun/utils/aiohttp_client.py |
Removes loopback SockSite usage; deprecates error callback; constructs Connector from aiohttp runner server factory. |
snitun/multiplexer/channel.py |
Adds write_no_wait() and refactors message creation to support transport writes. |
snitun/multiplexer/core.py |
Uses create_eager_task() for channel tasks. |
tests/client/test_connector.py |
Reworks connector tests into end-to-end TLS/http flows via a custom aiohttp connector over multiplexer channels. |
tests/client/helpers.py |
Adds a test-only aiohttp connector that routes requests over ChannelTransport + TLS for e2e testing. |
tests/multiplexer/test_transport.py |
Adds focused tests for ChannelTransport reader lifecycle, pause/resume, and exception behavior. |
tests/multiplexer/test_channel.py |
Adds coverage for queue-full behavior when writing without waiting. |
tests/test_trustme_configuration.py |
Adds TLS loopback sanity test to validate trustme cert setup. |
tests/conftest.py |
Adds TLS fixtures, aiohttp server fixtures, and end-to-end loopback wiring helpers. |
tests/utils/test_aiohttp_client.py |
Updates tests for new aiohttp client behavior and deprecation warning. |
tests/utils/test_server.py |
Removes unused test_endpoint dependency from fernet token tests. |
tests/client/test_client_peer.py |
Removes test_endpoint dependency and switches to connector fixture. |
pyproject.toml |
Adds trustme to test dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data = data[chunk_size:] | ||
| if data and self._pause_future: | ||
| await self._pause_future | ||
| except (SystemExit, KeyboardInterrupt): |
There was a problem hiding this comment.
Should asyncio.CancelledError be caught here as well, so it is properly caught and handled later, or is it also considered a fatal error?
Breaking change
SniTunClientAioHttp.start()no longer usesendpoint_connection_error_callback. Passing it will now generate aDeprecationWarningthat it will be removed in a future release. This failure condition is no longer possible since there is no longer a loop back connection, and the protocol is connected directly to theaiohttpRequestHandler(server).hass_nabucasacurrently uses this callback in https://github.com/NabuCasa/hass-nabucasa/blob/cb14a2cad747b882f407e72e69f539f5d8fa20bb/hass_nabucasa/remote.py#L321Its unclear if
hass_nabucasacan simply remove passing the callback or another change will be needed.Technically Breaking change
The signature of
snitun.client.connector.Connectorhas changed since it no longer connects to loop-back. Instead ofend_hostandend_portit takes thessl_contextandprotocol_factory(anasyncio.Protocolfactory, in Home Assistant's case this is anaiohttpRequestHandler)This doesn't appear to be used externally as
SniTunClientAioHttpis the entry point that creates these.Deployment considerations
The new
ChannelTransportis hard coded to pass the remote IP as127.0.0.1for compatibility with the currently deployed Cloud servers.https://github.com/NabuCasa/snitun/pull/303/files#diff-962be76744351102b2907e3aab99d5a45bc091cac1cc65f53f0fe2db9033c47fR30 should be adjusted to
Truewhenchannel.ip_addressis the actual remote IP address of the client that connected to the cloud server and not the IP Address of the internal forwarder.Proposed change
The concept is that the
SelectorTransportis replaced with the newChannelTransportthat is a wrapper around the channel so we don't need to connect back to localhost since we can create the request handler, connect up the transport and protocol andstart_tlsSince
aiohttpgets the ip address from the transport it can be set to whatever is desired in a future up. See Deployment considerationsTesting TODO: