Implementation for /exec using websocket #120

dims · 2017-02-01T18:01:24Z

inspired by the POC from @chekolyn

Adds a new requirement on websocket-client
Add a new class WSClient that uses WebSocketApp from the websocket-client.
Make sure we pass Authorization header
Make sure we honor the SSL settings in configuration
Some of the code will get overwritten when we generate fresh classes from swagger definition. To remind us added a e2e test so we don't lose the changes

Reference #58 (by mbohlool: Removed Fix. This PR is definitely a huge step toward the right direction, but I don't call 58 done yet.)

mbohlool

Thank you for taking care of this. It is great to see progress on exec support. We just need to make sure it supports all authentication methods and also let a way to interactively communicate. More info at each individual comments.

mbohlool · 2017-02-01T18:15:37Z

kubernetes/e2e_test/test_client.py

+                                                   command=exec_command,
+                                                   stderr=False, stdin=False,
+                                                   stdout=True, tty=False)
+        print('EXEC response : %s' % resp)


Can we do better instead of printing out the output? If we know what output we expect, we can check that or even part of the output.

ack. let's make sure we get 3 lines back

mbohlool · 2017-02-01T18:20:02Z

kubernetes/client/api_client.py

@@ -343,6 +344,13 @@ def request(self, method, url, query_params=None, headers=None,
        """
        Makes the HTTP request using RESTClient.
        """
+        if url.endswith('/exec') and method == "GET":
+            return ws_client.GET(self.config,


I maybe not familiar enough with exec command, but does it let interactive connection with the host? I mean can a script send something, then read the result, then decide what next command to send, etc.? In that case, should we return some kind of client allows that instead of command result?

Nope. the "/exec" is one shot only.

mbohlool · 2017-02-01T18:21:17Z

kubernetes/client/ws_client.py

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


what does configuration used for?

will add code for getting the ssl configuration options shortly

mbohlool · 2017-02-01T18:21:36Z

kubernetes/client/ws_client.py

+                                         on_message=self.on_message,
+                                         on_error=self.on_error,
+                                         on_close=self.on_close,
+                                         header=[])


should we send authorization token?

yep for sure :)

mbohlool · 2017-02-01T18:22:17Z

kubernetes/client/ws_client.py

+        self.ws.on_open = self.on_open
+        sslopt = None
+        if url.startswith('wss://'):
+            sslopt = {"cert_reqs": ssl.CERT_NONE}


Should we use ssl cert (and client cert and key) from configuration?

right. yes.

mbohlool · 2017-02-01T18:23:19Z

kubernetes/client/ws_client.py

+from six.moves.urllib.parse import quote_plus
+
+
+class WSClient:


I would suggest testing this (even manually) on GCE, GCP and minikube to make sure we cover a good range of authentication methods.

@mbohlool i do not have any kind of access to GCE or GCP. so i'll rely on your help with testing these

mbohlool · 2017-02-01T18:23:47Z

kubernetes/e2e_test/test_client.py

+            else:
+                time.sleep(1)
+
+        # exec_command = 'uptime'


remove the commented out codes.

mbohlool · 2017-02-01T18:30:18Z

@dims Sorry, I missed [WIP] in the title. Please ignore my comments until you are done with this.

dims · 2017-02-02T22:33:35Z

Tested by adding a TLS proxy in front of the K8S API

sudo make-ssl-cert /usr/share/ssl-cert/ssleay.cnf example.pem
hitch --frontend=[*]:8443 --backend=[localhost]:8080 example.pem

mbohlool · 2017-02-02T22:35:04Z

I think I had a comment about interactiveness of this solution but I cannot find it. The solution does not let the script to interactive with the server. Like open connection, run command, get output/error, run another command, etc... Is it possible to support that?

mbohlool · 2017-02-02T22:35:36Z

kubernetes/client/apis/core_v1_api.py

@@ -1007,7 +1007,7 @@ def connect_get_namespaced_pod_exec_with_http_info(self, name, namespace, **kwar
                                        auth_settings=auth_settings,
                                        callback=params.get('callback'),
                                        _return_http_data_only=params.get('_return_http_data_only'),
-                                        _preload_content=params.get('_preload_content', True),


What is the purpose of this change? I mean if we need the flag to be false, we should be able to do it in the client, where we override /exec path, right? This is the only place that can be overwritten by the code generation.

the _preload_content tries to coerce the response into a json. would be great to find another spot to put this.

api_client.py, if you move your changes out of request method into __call_api itself, you have access to preload_content flag. You can even branch before calling request to make sure the preload_content flag does not have any effect. e.g. if exec, do stuff and return, else do normal stuff of calling request and preload_content stuff. We should not change generated files as much as possible.

i figured out a way to avoid this change, please see latest update

chekolyn · 2017-02-02T23:03:57Z

@dims This is awesome work!

dims · 2017-02-03T00:20:46Z

@chekolyn kudos to you for finding a way :)

dims · 2017-02-03T00:38:02Z

@mbohlool : the API definition does not allow for interactivity

mbohlool · 2017-02-03T01:49:01Z

@mbohlool : the API definition does not allow for interactivity

you can return an object instead of an string, right? the object can have methods to work interactively with the open connection. We do that if _preload_content flag is false (returning a connection object). Having said that, instead of checking for "/exec", do you think it maybe better to pass a parameter like preload_content? something like _upgrade_connection, or _use_websocket. Then user can decide if they want to use websocket for an api call, and also we don't need to change client if anything other than /exec uses websocket (in future).

dims · 2017-02-03T10:46:18Z

@mbohlool i am happy with what we have here in this PR now. i am probably missing something about interactivity idea that you have brought up a few times. I hope we can merge this and then iterate on it. I have added a FIXME for the "/exec" check in one spot, so we can deal with that later. Hope this is good enough.

mbohlool · 2017-02-03T18:45:42Z

kubernetes/client/ws_client.py

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


I like the idea of WSResponse. make it easier to add interactivity later (which I am fine with).

Does WSResponse needs an 'status'? and 'reason' in case of an error?

mbohlool · 2017-02-03T18:48:04Z

kubernetes/e2e_test/test_client.py

+                                                   stderr=False, stdin=False,
+                                                   stdout=True, tty=False)
+        print('EXEC response : %s' % resp)
+        self.assertEqual(3, len(resp.splitlines()))


resp is a WSResponse, right? should we check it's data field?

@mbohlool the WSResponse is already unwrapped and the resp is just the string. If it were WSResponse the assert would fail :)

That was my problem too, why the assert didn't fail. I am just trying to run it locally (just curious to see it), then I will merge. Thanks.

I think I got the place that WSResponse is unwrapped, now I have more questions about that, e.g. why we don't have 'status' in WSResponse? rest.py checks for an status and raise an exception if status is not in OK range.

mbohlool · 2017-02-03T18:50:14Z

@dims OK. I am fine with this change (except one comment above) and we can merge and iterate on it later. It is in a pretty good shape.

@chekolyn

inspired by the POC from @chekolyn * Adds a new requirement on websocket-client * Add a new class WSClient that uses WebSocketApp from the websocket-client. * Make sure we pass Authorization header * Make sure we honor the SSL settings in configuration * Some of the code will get overwritten when we generate fresh classes from swagger definition. To remind us added a e2e test so we don't lose the changes * Added a new configuration option to enable/disable failures when hostnames in certificates don't match Fixes kubernetes-client#58

codecov-io · 2017-02-07T22:02:21Z

Codecov Report

Merging #120 into master will not impact coverage.

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  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 192b67c...066bba1. Read the comment docs.

dims · 2017-02-07T22:24:49Z

@mbohlool : there is no rest.py in the call stack. api_client.py calls ws_client.py directly so we don't need the "status" field.

https://www.evernote.com/shard/s412/sh/3a91433b-7125-4393-8506-2794b4565d2e/7ac292e184053c6a/res/ef38c1d4-214a-454b-b869-b2816c18087e/skitch.png

mbohlool · 2017-02-07T22:41:04Z

@dims Where we unwrap WSResponse then?

dims · 2017-02-07T22:44:08Z

@mbohlool - we deserialize here - https://github.com/kubernetes-incubator/client-python/blob/master/kubernetes/client/api_client.py#L160

@chekolyn

This commit reapplies PR kubernetes-client#120 that was removed by kubernetes-client#353. Below is the original commit message. inspired by the POC from @chekolyn * Adds a new requirement on websocket-client * Add a new class WSClient that uses WebSocketApp from the websocket-client. * Make sure we pass Authorization header * Make sure we honor the SSL settings in configuration * Some of the code will get overwritten when we generate fresh classes from swagger definition. To remind us added a e2e test so we don't lose the changes * Added a new configuration option to enable/disable failures when hostnames in certificates don't match Fixes kubernetes-client#58 Fixes kubernetes-client#409

@chekolyn

This commit reapplies PR kubernetes-client#120 that was removed by kubernetes-client#353. Below is the original commit message. inspired by the POC from @chekolyn * Adds a new requirement on websocket-client * Add a new class WSClient that uses WebSocketApp from the websocket-client. * Make sure we pass Authorization header * Make sure we honor the SSL settings in configuration * Some of the code will get overwritten when we generate fresh classes from swagger definition. To remind us added a e2e test so we don't lose the changes * Added a new configuration option to enable/disable failures when hostnames in certificates don't match Fixes kubernetes-client#58 Fixes kubernetes-client#409

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

mbohlool suggested changes Feb 1, 2017

View reviewed changes

dims force-pushed the support-exec-calls branch 2 times, most recently from 50919ab to aec77d5 Compare February 2, 2017 15:05

dims changed the title ~~[WIP] implementation for /exec using websocket~~ Implementation for /exec using websocket Feb 2, 2017

dims force-pushed the support-exec-calls branch 2 times, most recently from 7e9adec to df1f320 Compare February 2, 2017 22:32

mbohlool reviewed Feb 2, 2017

View reviewed changes

dims force-pushed the support-exec-calls branch 2 times, most recently from c787a71 to 36e95fe Compare February 3, 2017 03:17

mbohlool reviewed Feb 3, 2017

View reviewed changes

mbohlool self-assigned this Feb 3, 2017

dims force-pushed the support-exec-calls branch from 36e95fe to 066bba1 Compare February 7, 2017 21:50

mbohlool approved these changes Feb 7, 2017

View reviewed changes

mbohlool merged commit cfd34ef into kubernetes-client:master Feb 7, 2017

mbohlool mentioned this pull request Feb 9, 2017

Exec calls #58

Closed

chekolyn mentioned this pull request Feb 11, 2017

Exec on Pod? kelproject/pykube#64

Open

dulek mentioned this pull request Dec 5, 2017

Upgrade request required when running exec command with python-kubernetes client #409

Closed

dulek mentioned this pull request Dec 5, 2017

Reapply "Implementation for /exec using websocket" #410

Closed

		from six.moves.urllib.parse import quote_plus


		class WSClient:

		pass


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

Implementation for /exec using websocket #120

Implementation for /exec using websocket #120

Conversation

dims commented Feb 1, 2017 • edited by mbohlool Loading

mbohlool left a 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 1, 2017 • edited Loading

dims commented Feb 2, 2017

mbohlool commented Feb 2, 2017

mbohlool Feb 2, 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

chekolyn commented Feb 2, 2017

dims commented Feb 3, 2017

dims commented Feb 3, 2017

mbohlool commented Feb 3, 2017 • edited Loading

dims commented Feb 3, 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

mbohlool commented Feb 3, 2017

codecov-io commented Feb 7, 2017

Codecov Report

dims commented Feb 7, 2017 • edited Loading

mbohlool commented Feb 7, 2017

dims commented Feb 7, 2017

dims commented Feb 1, 2017 •

edited by mbohlool

Loading

mbohlool commented Feb 1, 2017 •

edited

Loading

mbohlool Feb 2, 2017 •

edited

Loading

mbohlool commented Feb 3, 2017 •

edited

Loading

dims commented Feb 7, 2017 •

edited

Loading