Skip to content

Fix WSClient.update dropping frames buffered in the SSL socket - #2650

Merged
kubernetes-prow[bot] merged 1 commit into
kubernetes-client:masterfrom
jojinkb:fix-ws-continuation-frame
Aug 10, 2026
Merged

Fix WSClient.update dropping frames buffered in the SSL socket#2650
kubernetes-prow[bot] merged 1 commit into
kubernetes-client:masterfrom
jojinkb:fix-ws-continuation-frame

Conversation

@jojinkb

@jojinkb jojinkb commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

WSClient.update() gates every read on poll()/select() against the raw socket fd. SSL sockets decrypt an entire TLS record at a time, so when several websocket frames arrive in the same TLS record, the first recv_data_frame() call consumes the whole record from the socket and the remaining frames sit decrypted inside the SSLSocket's internal buffer — where poll()/select() cannot see them (see the note on non-blocking SSL sockets in the Python docs: "an SSL socket may still have data available for reading without select() being aware of it", https://docs.python.org/3/library/ssl.html#ssl-nonblocking). Those frames are only delivered if new bytes later arrive on the wire, and are lost when the caller gives up first.

All real API-server connections use wss://, and whether frames share a TLS record depends on timing and message sizes, so this surfaces as random truncation of large pod_exec outputs — exactly the symptom in #2375 (Airflow xcom sidecar JSON truncated mid-document) and #2226 (stdout truncated at exactly 32768 chars; the TLS max plaintext record size is 16384 bytes). I reproduced it deterministically with a real TLS connection over a socketpair: with three websocket messages sent in a single TLS record, current master delivers only the first message, and every subsequent update() call blocks for its full poll timeout while SSLSocket.pending() reports the remaining frames sitting in the buffer forever.

The fix checks SSLSocket.pending() before polling and reads a frame directly when decrypted data is already buffered. This mirrors the pending-data handling that PortForward._proxy() in the same file already has (which is why port-forward does not suffer from this bug), and follows the review suggestion made on the earlier attempt #2422 to skip poll/select entirely when pending data exists. Credit to @Novelfor (#2414, #2422) for the original diagnosis of the SSL interaction.

About the OPCODE_CONT handling proposed in #2375: it is not needed and is not the cause of the truncation. With the library-default fire_cont_frame=False (which create_websocket() uses), websocket-client reassembles continuation frames inside recv_data_frame() and returns the opcode of the initial frame (continuous_frame.extract() returns data[0]), so update() never observes OPCODE_CONT — only frame.opcode, which update() does not use, can be CONT. This matches the reporter's observation that adding OPCODE_CONT to the condition did not solve the issue. A regression test (test_update_receives_fragmented_message, real websocket.WebSocket over a socketpair fed BINARY fin=0 + CONT fin=1 frames) now pins the reassembly behavior. Note that handling OPCODE_CONT per-fragment would actually be harmful if it ever fired, since continuation fragments do not repeat the 1-byte channel prefix.

Which issue(s) this PR fixes:

Fixes #2375
Fixes #2414

Most likely also resolves #2226 (same starvation mechanism; truncation there occurs at exact multiples of the 16384-byte TLS record plaintext limit).

Special notes for your reviewer:

  • test_update_reads_frames_pending_in_ssl_buffer fails on current master (AssertionError: None != b'hello') and passes with the fix; the other two new tests guard the poll fallback path and the continuation-frame reassembly contract.
  • When SSL data is pending, recv_data_frame() could block if the buffered bytes are a partial frame; this is not a new class of blocking — the same happens today whenever poll() fires on a partially received frame.
  • The asyncio client (kubernetes/aio/stream) is aiohttp-based and does not select on raw fds, so it is not affected.
  • Test run: python -m pytest kubernetes/base/stream/ -q → 21 passed (16 at HEAD before this change).

Does this PR introduce a user-facing change?

Fixed random truncation and delayed delivery of stream (exec/attach) data over TLS connections: WSClient.update() now consumes frames that were already decrypted into the SSL socket's internal buffer instead of waiting for the underlying socket to poll as readable.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


This change was developed with AI assistance (Claude Code); I have reviewed and tested it.

Sibling PR: #2651 (fixes #2226, same module — merge-compatible in either order, combined tests pass).

WSClient.update() waits for the underlying socket to become readable
via poll()/select() before reading a frame. SSL sockets decrypt an
entire TLS record at a time, so when several websocket frames arrive
in one TLS record, the first recv_data_frame() call consumes the whole
record from the socket and the remaining frames sit decrypted inside
the SSLSocket's internal buffer, where poll()/select() cannot see
them. Those frames are only delivered once new data arrives on the
connection and are lost if it never does, which randomly truncates
large outputs read through the stream API.

Check SSLSocket.pending() before polling, mirroring what
PortForward._proxy() already does, so buffered frames are consumed
without waiting for socket readability.

The OPCODE_CONT handling suggested in the issue is not needed:
websocket-client reassembles continuation frames inside
recv_data_frame() and returns the opcode of the initial frame, so
update() never observes OPCODE_CONT. A regression test documents that
fragmented messages are delivered in full.

Signed-off-by: Jojin <jojin.kb@gmail.com>
@kubernetes-prow kubernetes-prow Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 25, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

Welcome @jojinkb!

It looks like this is your first PR to kubernetes-client/python 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/python has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@kubernetes-prow
kubernetes-prow Bot requested review from fabianvf and yliaog July 25, 2026 09:54
@kubernetes-prow kubernetes-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2026
@yliaog

yliaog commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

@aojea could you take a look at this PR?

@yliaog

yliaog commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

#2646 has a fix for the same problem, could you please review?

i think the code logic fix is similar, but the tests in this PR could be helpful to add. what do you think?

@kubernetes-prow kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2026
@yliaog

yliaog commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@jojinkb please rebase the PR

@yliaog

yliaog commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

/close

@kubernetes-prow kubernetes-prow Bot closed this Aug 10, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

@yliaog: Closed this PR.

Details

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@yliaog

yliaog commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

/reopen

@kubernetes-prow kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2026
@kubernetes-prow kubernetes-prow Bot reopened this Aug 10, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

@yliaog: Reopened this PR.

Details

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@yliaog

yliaog commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

thanks for the PR

/lgtm
/approve

@kubernetes-prow kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jojinkb, yliaog

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2026
@kubernetes-prow
kubernetes-prow Bot merged commit 5a8b21c into kubernetes-client:master Aug 10, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

2 participants