-
Notifications
You must be signed in to change notification settings - Fork 3.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 our terms of service and privacy 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 is NOT APPROVED This pull-request has been approved by: Novelfor The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The committers listed above are authorized under a signed CLA. |
Welcome @Novelfor! |
598c0fc
to
764cc34
Compare
I think this could be improved by calling poll/select only when ssl_pending is 0. |
/assign |
@@ -193,7 +197,7 @@ def update(self, timeout=0): | |||
r, _, _ = select.select( | |||
(self.sock.sock, ), (), (), timeout) | |||
|
|||
if r: | |||
if r or ssl_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 case https://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.
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 document https://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.: