- Notifications
You must be signed in to change notification settings - Fork3.4k
fix stream bug when use ssl#2422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR isNOT APPROVED This pull-request has been approved by:Novelfor The full list of commands accepted by this bot can be foundhere. Needs approval from an approver in each of these files: Approvers can indicate their approval by writing |
linux-foundation-easyclabot commentedJul 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The committers listed above are authorized under a signed CLA. |
Welcome@Novelfor! |
ivaradi commentedJul 30, 2025
I think this could be improved by calling poll/select only when ssl_pending is 0. |
/assign |
| (self.sock.sock, ), (), (),timeout) | ||
| ifr: | ||
| ifrorssl_pending>0: |
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.
Could you add a test?
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.
Current e2e test has stream exec test. but i think it's not easy to find ssl blocking by current test code.
- The blocking is not clearly observable in the program (if the data is sent continuously, once the next chunk is sent, it will also carry over any previously blocked data).
- Setting up SSL in minikube requires additional work.
I can think about designing a reliable testing approach later, it's not easy to write a work test (current e2e casehttps://github.com/kubernetes-client/python/blob/master/kubernetes/e2e_test/test_client.py#L130 contain stream exec test, but it hard to reflect blocking). But I hope we can merge this first so that I don't have to keep using my own forked version on my cluster.
k8s-triage-robot commentedOct 29, 2025
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience atkubernetes/community. /lifecycle stale |
Uh oh!
There was an error while loading.Please reload this page.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Stream api will block data when use ssl socket.
Which issue(s) this PR fixes:
Fixes#2414
Special notes for your reviewer:
According to this documenthttps://docs.python.org/3/library/ssl.html#ssl-nonblocking, select() may lose some data, it only happen when use ssl socket
I test the code in my self-host kubernetes server
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: