asyncio: fix SSL connections by using native TLS transport#884
Conversation
c4b4b48 to
99e7245
Compare
|
@roydahan , please rebase |
|
Rebased. |
|
Can anyone review this PR? |
dkropachev
left a comment
There was a problem hiding this comment.
This looks like the right direction for asyncio TLS: using asyncio's native transport/protocol layer avoids the ssl.SSLSocket incompatibility with sock_recv() / sock_sendall().
I think it would be worth tightening the design before merge. Right now TLS and plain CQL use different I/O models, and TLS writes do not appear to account for asyncio transport backpressure. My main suggestion would be to consider using one transport/protocol path for both TLS and non-TLS asyncio connections in this PR.
The reason I think it belongs here is that the hard part is not only "make TLS work"; it is introducing asyncio transports into this reactor correctly. Once the reactor has transport lifecycle, protocol callbacks, write flow control, close handling, and tests for those pieces, keeping plain CQL on the old socket-coroutine path leaves two separate implementations of the same connection state machine. That makes future behavior harder to reason about and increases the chance that fixes land in one path but not the other.
CI already gives broad connectivity coverage through the asyncio integration matrix, including an SSL scenario via TestSslThroughNlb, so I would focus new tests on the new transport mechanics and compatibility edge cases rather than adding another generic SSL integration test.
Python 3.8+ rejects ssl.SSLSocket in asyncio's sock_sendall/sock_recv with TypeError. This caused the driver to fail connecting to ScyllaDB clusters requiring TLS, manifesting as 'protocol version 21 not supported' errors (0x15 = TLS Alert byte misread as protocol version). Fix by using asyncio's native TLS transport (loop.create_connection with ssl= parameter) instead of wrapping sockets with ssl.SSLContext.wrap_socket(). This preserves shard-aware port binding done during _initiate_connection(). Add _AsyncioProtocol to bridge asyncio's transport/protocol API back to Connection.process_io_buffer() for SSL data reads. Non-SSL connections continue using the existing sock_recv path. Fixes scylladb#330
|
All comments were addressed. |
Summary
SimpleStrategywithNetworkTopologyStrategyacross the codebase (ScyllaDB dropped SimpleStrategy support)loop.create_connection(ssl=)) instead ofssl.SSLContext.wrap_socket(), which Python 3.8+ rejects in asyncio'ssock_sendall/sock_recv0x15was misread as protocol version 21)Details
SimpleStrategy → NetworkTopologyStrategy
All CQL
CREATE KEYSPACEstatements, Python strategy objects, and test assertions updated. TheSimpleStrategyclass definition incassandra/metadata.pyis preserved for backward compatibility.AsyncioConnection SSL Fix
Problem: Python 3.8+ explicitly rejects
ssl.SSLSocketinasyncio.loop.sock_sendall()andsock_recv()withTypeError("ssl.SSLSocket is not supported"). The baseConnection._connect_socket()wraps the socket withssl.SSLContext.wrap_socket(), producing anSSLSocketthat asyncio refuses.Solution: Override
_connect_socket()inAsyncioConnectionto skip SSL wrapping, keeping a plain TCP socket. After the socket connects (preserving shard-aware port binding), upgrade to TLS asynchronously vialoop.create_connection(sock=, ssl=). A new_AsyncioProtocolclass bridges asyncio's transport/protocol API back toConnection.process_io_buffer()for reading. Writes usetransport.write()for SSL connections. Non-SSL connections are unchanged.Edge cases handled: SSL handshake failure properly unblocks the write coroutine and defuncts the connection.
Fixes #330