Web socket support. #19

brendandburns · 2017-05-16T13:38:56Z

Ready for review

Fixes #18

Depends on #17

brendandburns · 2017-06-02T05:31:13Z

@mbohlool this is ready for review.

brendandburns · 2017-06-10T03:57:25Z

Friendly friday evening ping on this one (feel free to wait until next week to review :)

brendandburns · 2017-06-14T05:16:19Z

@mbohlool friendly ping on this one.

Thanks!

lwander

LGTM - maybe drop a link to https://tools.ietf.org/html/rfc6455, it was helpful for reviewing this code.

lwander · 2017-06-14T14:02:48Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

@@ -0,0 +1,107 @@
+package io.kubernetes.client.util;


No license header?

Yes, please add license header to all files. we need a check for that.

mbohlool · 2017-06-14T16:24:43Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

@@ -0,0 +1,107 @@
+package io.kubernetes.client.util;


Yes, please add license header to all files. we need a check for that.

mbohlool · 2017-06-14T16:25:31Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

+import java.util.ArrayList;
+import java.util.HashMap;
+
+import okio.Buffer;


This import should be in second group I guess.

mbohlool · 2017-06-14T16:27:21Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

+    /**
+     * A simple interface for a listener on a web socket
+     */
+    public interface SocketListener {


We need a wrapper on top of this interface that understands kubernetes streams from different channels (stdout, stderr). It can be done in another PR.

Yeah, I did this in a later PR, see:

https://github.com/kubernetes-client/java/pull/24/files#diff-72ca04ac95ffb12a152f92ed801c4b5a

mbohlool · 2017-06-14T16:28:55Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

+     */
+    public static void stream(String path, String method, ApiClient client, SocketListener listener) throws ApiException, IOException {
+        HashMap<String, String> headers = new HashMap<String, String>();
+        headers.put("X-Stream-Protocol-Version", "v4.channel.k8s.io,v3.channel.k8s.io,v2.channel.k8s.io,channel.k8s.io");


Extract constants please. Also are these protocols the only protocol k8s supports? if there is a documentation somewhere in k8s repo, a link would be nice. Ideally these should be extensions in OpenAPI spec and should be populated from there.

Yeah, ideally these would be in the OpenAPI spec but I couldn't find them. They're constants in the code as far as I can tell, e.g.

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/util/wsstream/conn.go#L48

I personally lean toward extracting constant as much as possible but this is not a deal breaker for me. If you prefer to keep them this way, I can live with it :)

mbohlool · 2017-06-14T16:29:52Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

+    public static void stream(String path, String method, ApiClient client, SocketListener listener) throws ApiException, IOException {
+        HashMap<String, String> headers = new HashMap<String, String>();
+        headers.put("X-Stream-Protocol-Version", "v4.channel.k8s.io,v3.channel.k8s.io,v2.channel.k8s.io,channel.k8s.io");
+        headers.put("Connection", "Upgrade");


Do you think it is cleaner to have all string literals as constant? If you agree, we can make it an style guide. Of course there could be exceptions (that should be well defined).

For one-off things like this where values aren't often repeated, I don't really see the value, personally. I don't feel super strongly, but I don't personally find it to be useful.

mbohlool · 2017-06-14T16:32:49Z

util/src/main/java/io/kubernetes/client/util/WebSockets.java

+            listener.close();
+        }
+    }
+}


New line at the end of the file. I think this one should be an style guide too.

mbohlool · 2017-06-14T16:36:21Z

util/src/main/java/io/kubernetes/client/util/WebSocketCall.java

+ * limitations under the License.
+ */
+// This has been cloned from the okhttp-ws package so that I could remove
+// the requirement that websockets use the "GET" method


What is the difference between exec/attach get vs post? If post works, I prefer not to clone this file. Otherwise is there a way to push a feature upstream in okhttp-ws and support post when that is merged?

Actually GET is a requirement. I suggest we remove this file and only support GET. from rfc6455:

The method of the request MUST be GET, and the HTTP version MUST
be at least 1.1.

POST is defined in the API swagger/openapi specificaction.

https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json

Does that mean that we should edit the swagger first?

I think we should fix that too but we should not wait for that. I will investigate it (to see why it gets into the spec and if we actually support POST) but I don't think there is anything we can do with POST that we can't do with GET. Websocket reuse the connection and ignores http method, thus I think we should not support POST in java client (or any other client). We can always add POST back if we see any usecase for it.

I have validated that both GET and POST work.

(fwiw) the kubectl client uses POST:

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/exec.go#L312

So I don't think we can remove POST support (unless we also update kubectl)

Ah, and I think remember why it is this way... I think the theory is that exec/attach have the possibility to modify the state in the container, thus POST feels more right than GET since GET is supposed to be idempotent.

mbohlool · 2017-06-14T16:36:40Z

util/pom.xml

+		<java.version>1.7</java.version>
+		<maven.compiler.source>${java.version}</maven.compiler.source>
+		<maven.compiler.target>${java.version}</maven.compiler.target>
+	</properties>
 </project>


new line at the end of the file.

mbohlool · 2017-06-14T16:38:37Z

Sorry for the delay. I am attending a conference and have limited time. Done one round of review.

brendandburns · 2017-06-15T04:13:49Z

Comments addressed, please take another look.

Thanks!
--brendan

mbohlool · 2017-06-16T06:29:07Z

I still have concern about websocket with POST. If the reason is kubectl is doing it and if we conclude that it is wrong, do you think we should repeat the mistake?
I think it is wrong on two level, interface and implementation (and rfc as the bonus :) ).

Interface:
We use websocket connections to connect to pods or watch objects. None of them is changing the actual object (connecting to a pod can change things inside it, but the kubernetes object pod/container, the one we store in etcd, would not change by that). The interface should not be watch_get_pod_... or connect_post_pod_exec... or something like that. interface should be watch_pod..., or connect_pod_exec..., etc. The http verb doesn't mean anything to be in the interface at all.

Implementation:
Even if we disagree on interface point, the implementation still doesn't care about verb. We can keep whatever interface we desire and the implementation can still be only GET. It makes no practical difference to start connection with POST or GET because websocket is not bond to http verb rules and reuses the connection. My point is even if we have post_websocket and get_websocket interfaces (for any reason), we can still implement both of them with standard websocket library that usesGET and it will make no practical difference to clone part of library and support POST.

rfc
While we can diverse from rfc if we see good reason, I don't see one here.

What do you think?

brendandburns · 2017-06-16T15:28:49Z

Well I already ripped out the POST stuff from this PR so I'm fine going ahead with GET for now, but I'm 100% sure the POST code has better test coverage in the kubernetes repo, so we should be prepared for breaks...

…

--brendan

________________________________ From: Mehdy Bohlool <[email protected]> Sent: Thursday, June 15, 2017 11:29:08 PM To: kubernetes-client/java Cc: Brendan Burns; Author Subject: Re: [kubernetes-client/java] Web socket support. (#19) I still have concern about websocket with POST. If the reason is kubectl is doing it and if we conclude that it is wrong, do you think we should repeat the mistake? I think it is wrong on two level, interface and implementation. Interface: We use websocket connections to connect to pods or watch objects. None of them is changing the actual object (connecting to a pod can change things inside it, but the kubernetes object pod/container, the one we store in etcd, would not change by that). The interface should not be watch (get_pod_...) or connect (post_pod_exec...) or something like that. interface should be watch_pod..., or connect_pod_exec..., etc. The http verb doesn't mean anything to be in the interface at all. Implementation: Even if we disagree on interface point, the implementation still doesn't care about verb. We can keep whatever interface we desire and the implementation can still be only GET. It makes no practical difference to start connection with POST or GET because websocket is not bond to http verb rules and reuses the connection. My point is even if we have post_websocket and get_websocket interfaces (for any reason), we can still implement both of them with standard websocket library that uses get and it will make no practical difference to clone part of library and make it post. rfc While we can diverse from rfc if we see good reason, I don't see one here. What do you think? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkubernetes-client%2Fjava%2Fpull%2F19%23issuecomment-308944643&data=02%7C01%7Cbburns%40microsoft.com%7C26f9bae1620242670ddf08d4b480ff3d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636331913508228109&sdata=SCTY5zoC%2Bzyoydk0Cy1FB5GgcfzHnJnZQaY1qEcMeEA%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFfDgsysVFtY_i-2WpH63BvzjezAry28ks5sEiC0gaJpZM4Ncgtn&data=02%7C01%7Cbburns%40microsoft.com%7C26f9bae1620242670ddf08d4b480ff3d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636331913508228109&sdata=csQMOn%2FuYL9DcFKRO%2F8irCQoDdjMXPXqJXpE0QL27NU%3D&reserved=0>.

mbohlool · 2017-06-16T16:39:55Z

lgtm. can you squash commits before I merge? Thanks.

brendandburns · 2017-06-17T04:24:02Z

Squashed and ready for merge.

Thanks!

brendandburns force-pushed the ws branch 2 times, most recently from c5920a0 to eb22b06 Compare May 20, 2017 04:50

brendandburns mentioned this pull request May 28, 2017

Initial implementation of Exec. #21

Merged

brendandburns force-pushed the ws branch from eb22b06 to 2376176 Compare June 2, 2017 01:38

brendandburns changed the title ~~WIP web socket support.~~ Web socket support. Jun 2, 2017

brendandburns force-pushed the ws branch from 2376176 to 5f6c43a Compare June 2, 2017 05:31

brendandburns force-pushed the ws branch 2 times, most recently from 5ccc832 to 4e44a4f Compare June 2, 2017 05:34

brendandburns force-pushed the ws branch 4 times, most recently from 07cf307 to 6c42315 Compare June 10, 2017 03:56

lwander approved these changes Jun 14, 2017

View reviewed changes

mbohlool suggested changes Jun 14, 2017

View reviewed changes

brendandburns force-pushed the ws branch from aedc141 to 7ea44eb Compare June 15, 2017 04:16

mbohlool mentioned this pull request Jun 16, 2017

Websocket POST vs GET #30

Closed

Web socket support.

380928a

brendandburns force-pushed the ws branch from 7ea44eb to 380928a Compare June 17, 2017 04:23

mbohlool merged commit 2d418a8 into kubernetes-client:master Jun 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web socket support. #19

Web socket support. #19

brendandburns commented May 16, 2017 •

edited

Loading

brendandburns commented Jun 2, 2017

brendandburns commented Jun 10, 2017

brendandburns commented Jun 14, 2017

lwander left a comment

lwander Jun 14, 2017

mbohlool Jun 14, 2017

brendandburns Jun 15, 2017

mbohlool Jun 14, 2017

mbohlool Jun 14, 2017

brendandburns Jun 15, 2017

mbohlool Jun 14, 2017 •

edited

Loading

brendandburns Jun 15, 2017

mbohlool Jun 14, 2017

brendandburns Jun 15, 2017

mbohlool Jun 15, 2017

mbohlool Jun 14, 2017

brendandburns Jun 15, 2017

brendandburns Jun 15, 2017

mbohlool Jun 14, 2017

brendandburns Jun 15, 2017

mbohlool Jun 14, 2017

mbohlool Jun 14, 2017

brendandburns Jun 14, 2017

mbohlool Jun 14, 2017 •

edited

Loading

brendandburns Jun 15, 2017

brendandburns Jun 15, 2017

mbohlool Jun 14, 2017

brendandburns Jun 15, 2017

mbohlool commented Jun 14, 2017 •

edited

Loading

brendandburns commented Jun 15, 2017

mbohlool commented Jun 16, 2017 •

edited

Loading

brendandburns commented Jun 16, 2017 via email

mbohlool commented Jun 16, 2017 •

edited

Loading

brendandburns commented Jun 17, 2017

Web socket support. #19

Web socket support. #19

Conversation

brendandburns commented May 16, 2017 • edited Loading

brendandburns commented Jun 2, 2017

brendandburns commented Jun 10, 2017

brendandburns commented Jun 14, 2017

lwander 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

mbohlool Jun 14, 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

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 Jun 14, 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

mbohlool commented Jun 14, 2017 • edited Loading

brendandburns commented Jun 15, 2017

mbohlool commented Jun 16, 2017 • edited Loading

brendandburns commented Jun 16, 2017 via email

mbohlool commented Jun 16, 2017 • edited Loading

brendandburns commented Jun 17, 2017

brendandburns commented May 16, 2017 •

edited

Loading

mbohlool Jun 14, 2017 •

edited

Loading

mbohlool Jun 14, 2017 •

edited

Loading

mbohlool commented Jun 14, 2017 •

edited

Loading

mbohlool commented Jun 16, 2017 •

edited

Loading

mbohlool commented Jun 16, 2017 •

edited

Loading