Improvements on ws_client #125

mbohlool · 2017-02-15T20:40:20Z

The ws_client should be able to return an object so the caller can interact with server. This will happen if _preload_content is not True. In that case, the client will return an object that can interact with stdin/stdout/stderr of the remote container.

fixes #58

codecov-io · 2017-02-16T00:59:58Z

Codecov Report

Merging #125 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   94.46%   94.46%           
=======================================
  Files           9        9           
  Lines         668      668           
=======================================
  Hits          631      631           
  Misses         37       37

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1635150...88563a6. Read the comment docs.

mrmcmuffinz · 2017-02-16T01:22:59Z

kubernetes/client/ws_client.py

+
+ # TODO: This method does not seem to work.
+ def write_stdin(self, data):
+ self.sock.send(data)


Is the connection still open to even send the data? How does the underlining api url call look like when you try to tack on more to exec commands to an already successful call?

The connection stays open until explicitly closed by either side. My example code does not close the connection before sending data.

mrmcmuffinz · 2017-02-16T01:23:11Z

examples/exec.py

+ print("STDERR: %s" % resp.read_stderr())
+
+exit(1)
+# This part does not work yet. resp.write_stdin does not work.


Could it be that you need to set the websocket to lingering, so that the connection stays open on both ends till the action you are trying to execute completes and forwards back the result?

In the example bellow, I only run sh without closing the connection. _preload_content is False so the call will stays open. Then on line 94 inside the loop I send commands one by one. This example code will not end by itself because the connection stays open.

mbohlool · 2017-02-17T04:39:20Z

@mrmcmuffinz @dims this is ready for review.

mrmcmuffinz · 2017-02-17T04:40:42Z

If you don't mind I can review it tomorrow, I'm on the east coast and it's close to midnight here. I want to get some sleep.

mbohlool · 2017-02-17T04:41:25Z

@mrmcmuffinz no rush. thanks.

dims · 2017-02-17T11:39:30Z

examples/exec.py

+from kubernetes.client.rest import ApiException
+
+config.load_kube_config(
+ context="gke_cloud-kubernetes-dev_us-central1-f_mehdy-cluster")


have a more generic name here?

dims · 2017-02-17T11:43:19Z

@mbohlool : looks good, i haven't tried it yet. Though that reminds me that we should add a e2e test especially for the new code path where we pull data on demand.

mbohlool · 2017-02-17T17:20:18Z

@dims. Will add an e2e test.

mrmcmuffinz · 2017-02-17T19:55:12Z

examples/exec.py

+ command=exec_command,
+ stderr=True, stdin=False,
+ stdout=True, tty=False)
+print "Response: " + resp


Do you think it would help to demonstrate here how to access the stdout and stderr?
What happens when you print to stderr but when you call exec function with stderr=False and vice versa with stdout?

There are many cases that we can cover. We can add more examples later (and feel free to add more).

mrmcmuffinz · 2017-02-17T19:57:27Z

examples/exec.py

+exec_command = ['/bin/sh']
+resp = api.connect_get_namespaced_pod_exec(name, 'default',
+ command=exec_command,
+ stderr=True, stdin=True,


You mentioned in the comments that stdin is not working, can we get rid of that argument here?

stdin is part of auto-generated arguments that we never touch.

stdin is working now :)

mrmcmuffinz · 2017-02-17T19:58:43Z

kubernetes/client/ws_client.py

@@ -23,22 +25,17 @@

 class WSClient:
 def __init__(self, configuration, url, headers):


Can we add some documentation to this?

I will add some documentation for methods in this class. init specifically does not need docs in my opinion as this class is only created inside this file.

mrmcmuffinz · 2017-02-17T20:00:00Z

kubernetes/client/ws_client.py

+ self._connected = False
+ self._stdout = ""
+ self._stderr = ""
+ self._all = ""

 # We just need to pass the Authorization, ignore all the other
 # http headers we get from the generated code
 if 'Authorization' in headers:


Do we need to be concerned that headers is None or that there is no Authorization in it?

if check to see if there is authorization in it. if there is none, we don't need to pass any header. Fixed the case that headers is None.

mrmcmuffinz · 2017-02-17T20:02:39Z

kubernetes/client/ws_client.py

+ self.update()
+ return self._stderr
+
+ def read_stderr(self):


Where was this in the example?

end of the example, I've used this method.

mrmcmuffinz · 2017-02-17T20:04:47Z

kubernetes/client/ws_client.py

 import collections
-import websocket
+from websocket import WebSocket, ABNF, enableTrace


Just a suggestion however can we organize these, import first then from?

we have pep8 and sort_import running automatically and checking these. We cannot organize them the way we want unless we want to remove those checks.

mrmcmuffinz · 2017-02-17T20:05:27Z

kubernetes/client/ws_client.py

+ def is_open(self):
+ return self._connected
+
+ # TODO: This method does not seem to work.


If this does not work, do it make sense to keep it in the code or remove it?

old comment. removed.

mrmcmuffinz · 2017-02-17T20:07:20Z

kubernetes/client/ws_client.py

+ if op_code == ABNF.OPCODE_CLOSE:
+ self._connected = False
+ return
+ elif op_code == ABNF.OPCODE_BINARY or op_code == ABNF.OPCODE_TEXT:


Is there any other op_code that needs to be considered other than the ones you caught here and above?

I don't think so.

I don't know much about sockets however I see that there are more codes https://github.com/websocket-client/websocket-client/blob/master/websocket/_abnf.py#L102 where I thought we may have to cover our bases but I would like to leave that up to someone else who is more experienced in the area.

I've modeled this from WebSocketApp here: https://github.com/websocket-client/websocket-client/blob/master/websocket/_app.py#L205. Ping and Pong are optional (and we can support it later). Cont can be ignored and it is only useful if you want to get notified on an incomplete frame. however, we may improve this later by doing something on "Cont" (like running update again).

mrmcmuffinz · 2017-02-17T20:07:40Z

kubernetes/client/ws_client.py



 WSResponse = collections.namedtuple('WSResponse', ['data'])


-def GET(configuration, url, query_params, _request_timeout, headers):
+def GET(configuration, url, query_params, _request_timeout, _preload_content,
+ headers):


Documentation?

GET is internal, but above methods need docs that will be added.

mrmcmuffinz · 2017-02-17T20:18:16Z

kubernetes/client/ws_client.py

+ self._stdout += data
+
+ def run_forever(self, timeout=None):
+ if timeout:


Can we get an explanation on what run_forever means and are there any gotchas(what are the assumptions you are making)?

will add documentation.

mbohlool · 2017-02-17T22:48:48Z

@mrmcmuffinz, @dims addressed comments and also make the implementation more general to support any channel number (0,1,2 for std in,out,err). We may want to reuse this again in port forwarding api calls (they use websocket too). PTAL.

mrmcmuffinz · 2017-02-18T00:01:38Z

General comment, was your intention to expose the channel constants for the end user like myself or just for the purpose of the websocket client?

mbohlool · 2017-02-18T00:27:35Z

@mrmcmuffinz Channel numbering is a kubernetes thing as far as I can tell and I think it is not bad to let user know about it. But the helper function like read_stdout can hide that if user don't want to care about them. The whole channels expose can be useful later for port forwarding api implementation.

chekolyn · 2017-02-19T19:30:27Z

kubernetes/client/ws_client.py

- on_close=self.on_close,
- header=[header] if header else None)
- self.ws.on_open = self.on_open
+ if headers and 'Authorization' in headers:


I'm running into a small issue when running the exec.py example using header authentication.
Looks like is a small case condition; it appears that the header key is "authorization" instead of "Authorization" and then we pass an empty header to the WS call.

(kubernetes-local) PAOC02S79NJG8WP:examples sa$ python exec.py {'Content-Type': 'application/json', 'authorization': 'Bearer CLSMM1PCEbKNOgy2fz5runETCrf-yzV1ThPXAiJ4aLw', 'Accept': '*/*', 'User-Agent': 'Swagger-Codegen/1.0.0-snapshot/python'} Traceback (most recent call last): File "exec.py", line 61, in <module> stdout=True, tty=False) File "/Users/sagui10/git/kubernetes/client-python/kubernetes/client/apis/core_v1_api.py", line 907, in connect_get_namespaced_pod_exec (data) = self.connect_get_namespaced_pod_exec_with_http_info(name, namespace, **kwargs) File "/Users/sagui10/git/kubernetes/client-python/kubernetes/client/apis/core_v1_api.py", line 1012, in connect_get_namespaced_pod_exec_with_http_info collection_formats=collection_formats) File "/Users/sagui10/git/kubernetes/client-python/kubernetes/client/api_client.py", line 329, in call_api _return_http_data_only, collection_formats, _preload_content, _request_timeout) File "/Users/sagui10/git/kubernetes/client-python/kubernetes/client/api_client.py", line 153, in __call_api _request_timeout=_request_timeout) File "/Users/sagui10/git/kubernetes/client-python/kubernetes/client/api_client.py", line 355, in request headers=headers) File "/Users/sagui10/git/kubernetes/client-python/kubernetes/client/ws_client.py", line 239, in websocket_call raise ApiException(status=0, reason=str(e)) kubernetes.client.rest.ApiException: (0) Reason: Handshake status 403

I added this to debug and make it work:

# We just need to pass the Authorization, ignore all the other # http headers we get from the generated code if headers and 'Authorization' in headers: header.append("Authorization: %s" % headers['Authorization']) elif headers and 'authorization' in headers: header.append("Authorization: %s" % headers['authorization']) print headers

@chekolyn brings up a good point, however instead of doing the elif headers and 'authorization' in headers: we just need to do a case insensitive search. There may be a case where authorization can be all caps as well, what do you think @mbohlool ?

This should be always authorization with all lower case. I wonder why it worked for me before, I am investigating it. But I am agains making the check case insensitive as it will result in being inconsistent in different part of the code while we know this should be always lowecase.

I've fixed this to be lower case authorization to be consistent with the rest of the client, also created #128 to further investigate this.

…teract with websocket server and reach each channel separately

dims · 2017-02-21T21:32:59Z

kubernetes/client/ws_client.py

- on_close=self.on_close,
- header=[header] if header else None)
- self.ws.on_open = self.on_open
+ if headers and 'authorization' in headers:


Should we support both Authorization and authorization?

I think we should be consistent. We used lowercase authorization everywhere else in our client. Based on the discussion in #128, I think we should have capital Authorization everywhere and case-insensitive checks. I will let another PR fixing #128 to do it consistently for whole client and keep this here to be consistent meanwhile.

dims · 2017-02-21T21:34:37Z

One question inline, LGTM otherwise. thanks @mbohlool

mbohlool · 2017-02-21T21:45:57Z

thanks @dims @mrmcmuffinz @chekolyn

mrmcmuffinz · 2017-02-22T02:28:01Z

Hey @mbohlool to be clear I can not set in my pip requirements.txt v1.0.0b2 for kubernetes client-python right?

mrmcmuffinz · 2017-02-22T02:28:27Z

Btw, thanks to all for being patient and following through on the comments. Awesome work!

chekolyn · 2017-02-22T04:22:03Z

Second that, awesome work!
@mbohlool with your consent I would like to port your WSClient implementation to pykube. Both projects have the same Apache License, would that be OK?

mbohlool · 2017-02-22T22:19:20Z

@chekolyn That would be fine. But I think it is better for pykube to add a dependency on client-python. It could simplify its config and transport layers too (by delegating them to client-python) and get any patch/update for free.

mbohlool · 2017-02-22T22:20:12Z

@mrmcmuffinz you're welcome. v1.0.0b2 has a dependency problem that @dims is fixing. Was your question related to that?

mbohlool added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 15, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2017

mbohlool force-pushed the exec branch 3 times, most recently from f26ae30 to df4c73b Compare February 16, 2017 00:54

mrmcmuffinz reviewed Feb 16, 2017

View reviewed changes

mbohlool force-pushed the exec branch from df4c73b to 8651983 Compare February 17, 2017 04:38

mbohlool assigned dims Feb 17, 2017

mbohlool requested a review from dims February 17, 2017 04:38

mbohlool removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 17, 2017

mbohlool added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 17, 2017

mbohlool changed the title ~~WIP: Improvements on ws_client~~ Improvements on ws_client Feb 17, 2017

mbohlool force-pushed the exec branch from 3a372af to b0a4aeb Compare February 17, 2017 08:13

dims reviewed Feb 17, 2017

View reviewed changes

mrmcmuffinz reviewed Feb 17, 2017

View reviewed changes

mbohlool force-pushed the exec branch 2 times, most recently from fd956cd to c20d9a8 Compare February 17, 2017 22:49

chekolyn reviewed Feb 19, 2017

View reviewed changes

mbohlool added 2 commits February 21, 2017 12:55

Improvements on ws_client. Now the client can returns an object to in…

4e593a7

…teract with websocket server and reach each channel separately

Update CHANGELOG

88563a6

mbohlool force-pushed the exec branch from c20d9a8 to 88563a6 Compare February 21, 2017 20:55

dims reviewed Feb 21, 2017

View reviewed changes

mbohlool merged commit 436351b into kubernetes-client:master Feb 21, 2017

roycaihw mentioned this pull request Feb 4, 2019

decode add a replace option kubernetes-client/python-base#104

Merged

		@@ -23,22 +25,17 @@

		class WSClient:
		def __init__(self, configuration, url, headers):

Improvements on ws_client #125

Improvements on ws_client #125

Conversation

mbohlool commented Feb 15, 2017 • edited Loading

codecov-io commented Feb 16, 2017 • edited Loading

Codecov Report

mrmcmuffinz Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented Feb 17, 2017

mrmcmuffinz commented Feb 17, 2017

mbohlool commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Feb 17, 2017

mbohlool commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented Feb 17, 2017

mrmcmuffinz commented Feb 18, 2017

mbohlool commented Feb 18, 2017

Choose a reason for hiding this comment

mrmcmuffinz Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Feb 21, 2017

mbohlool commented Feb 21, 2017

mrmcmuffinz commented Feb 22, 2017

mrmcmuffinz commented Feb 22, 2017 • edited Loading

chekolyn commented Feb 22, 2017

mbohlool commented Feb 22, 2017 • edited Loading

mbohlool commented Feb 22, 2017

mbohlool commented Feb 15, 2017 •

edited

Loading

codecov-io commented Feb 16, 2017 •

edited

Loading

mrmcmuffinz Feb 16, 2017 •

edited

Loading

mrmcmuffinz Feb 19, 2017 •

edited

Loading

mrmcmuffinz commented Feb 22, 2017 •

edited

Loading

mbohlool commented Feb 22, 2017 •

edited

Loading