Fix pending SSL data handling in stream client - #2646
Conversation
|
|
|
Welcome @mihirduvedi! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mihirduvedi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign @mihirduvedi Can you also check if the asyncio client needs the same fix? https://github.com/kubernetes-client/python/tree/master/kubernetes/aio |
|
Thanks for the review. I checked the asyncio client carefully. The reported issue is fixed in the synchronous client, and the asyncio client does not require the same change. kubernetes/aio/stream/ws_client.py delegates WebSocket reads to aiohttp through async for msg in ws rather than polling the raw SSL socket. aiohttp drains decrypted data into its WebSocket parser and queues each complete frame. I also tested the equivalent edge case over WSS by sending two channel frames in a single TLS transport write through the Kubernetes WsApiClient; both frames were received correctly. Validation:
Therefore, no asyncio source change is necessary. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
WSClient.update()can wait onpoll()orselect()even when an SSL socket already has decrypted data buffered internally. This can delay stream output indefinitely when no additional network event arrives.This change checks
SSLSocket.pending()before polling and immediately processes a WebSocket frame when buffered SSL data is available. It also adds a deterministic regression test usingtimeout=Nonethat verifies neither polling path is entered.Which issue(s) this PR fixes:
Fixes #2414
Special notes for your reviewer:
This addresses the unit-test request from #2422 and the review feedback to avoid calling
poll()orselect()when SSL data is already pending.Tests:
python -m pytest -q kubernetes/base/stream/ws_client_test.py— 17 passedpython -m pytest -q kubernetes/base --ignore=kubernetes/base/dynamic/test_client.py --ignore=kubernetes/base/dynamic/test_discovery.py— 143 passedThe two excluded dynamic-client modules require a live Kubernetes cluster.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: