Add support for non-binary WebMKS websocket #17200

miha-plesko · 2018-03-26T12:22:08Z

Until now, 'binary' protocol for websockets was assumed. But VMware vCloud console access only works via legacy 'uint8utf8' protocol. To make it even more interesting, vCloud's JavaScript SDK actually thinks it supports 'binary', but in fact it doesn't. Well 'binary' protocol works when connecting to vCenter (i.e. Vmware::InfraManager), but not for vCloud Director (Vmware::CloudManager).

With this commit we extend original (binary) websocket proxy to implement our own uint8utf8 socket proxying.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1560517

@miq-bot add_label enhancement,gaprindashvili/yes
@miq-bot assign @skateman

Followup PRs:
ManageIQ/manageiq-ui-classic#3679
ManageIQ/manageiq-providers-vmware#218

/cc @bmclaughlin @gberginc

skateman

🎉 👍 🍻

skateman · 2018-03-26T13:37:01Z

lib/websocket_proxy.rb

+    # able to use it :) We workaround this by only forcing the 'uint8utf8' protocol which actually works.
+    force_uint8 = console.protocol.to_s.end_with?('uint8utf8')
+    protocols = force_uint8 ? ['uint8utf8'] : ['binary']
+    @driver = WebSocket::Driver.rack(self, :protocols => protocols)


I see no point of having the condition, is it OK for the client to always send both protocols?

Well, it's a bit complicated. JavaScript SDK (webmks/wmks.min.js) is eager to grab binary protocol if offered, even if it doesn't know how to handle it. It took me days to figure, but binary protocol is simply not working with vCloud, for some reason. It seems to be working with vCenter (InfraManager), but not with vCloud.

That being said, it would be better to patch the JavaScript SDK to use text socket even if binary is offered, but:

a) only minified version of the SDK is available, so the patching is not trivial
b) InfraManager uses the same SDK and actually uses binary sockets, so we're in danger to spoil it there

I don't like it either, but wasn't able to connect otherwise.

BTW: isn't it actually dangerous to list multiple protocols here, since it's just proxy socket?

USER ---> proxy socket ---> MIQ ---> actual socket ---> PROVIDER

If PROVIDER (actual socket) would only support uint8utf8 protocol, but MIQ (proxy socket) would advertise [binary, uint8utf8], then binary wouldn't work anyway I think.

Oh, then just let's increase the readability:

protocol = console.protocol.to_s.end_with?('uint8utf8') ? 'uint8utf8' : 'binary' @driver = WebSocket::Driver.rack(self, :protocols => [protocol])

skateman · 2018-03-26T13:37:50Z

lib/websocket_proxy.rb

@@ -51,7 +63,7 @@ def transmit(sockets, is_ws)
      @driver.parse(data)
    else
      @right.fetch(64.kilobytes) do |data|
-        @driver.binary(data)
+        @driver.frame(data)


Can you please explain this?

@driver.binary(data)

forcibly threats data obtained as :binary (source code) while

@driver.frame(data)

lets underlying library, websocket-driver, probe the type (see this line). In the first case our uint8utf8 data gets spoiled because it's intepreted incorrectly - should be interpreted as :text but is forcibly interpreted as :binary.

skateman · 2018-03-26T13:38:26Z

lib/websocket_webmks_uint8utf8.rb

@@ -0,0 +1,31 @@
+class WebsocketWebmksUint8utf8 < WebsocketSSLSocket


Wouldn't be better to inherit from the WebsocketWebmks?

It would BUT then we can't invoke WebsocketSSLSocket's constructor to setup SSL stuff for us. On the other hand, we must not invoke WebsocketWebmks's constructor because it'll start the wrong driver for us, so...

Can you feel the pain? 😄

In other words:

WebsocketWebmksUint8utf8 <--- WebsocketWebmks <--- WebsocketSSLSocket |------------- super.super ------------------^

From what I know, Ruby does not support invoking super.super methods, therefore we need to do the ugly copy-pasting here... But maybe I missed something?

Super (pun intended), keep it like this...

miha-plesko

Thanks @skateman for a fast reply, you ask very good questions. Please see my answers, looking forward to discuss further. Also, I do know we need unit tests for all this, will try to come up with something.

miha-plesko · 2018-03-26T13:56:17Z

lib/websocket_proxy.rb

+    # able to use it :) We workaround this by only forcing the 'uint8utf8' protocol which actually works.
+    force_uint8 = console.protocol.to_s.end_with?('uint8utf8')
+    protocols = force_uint8 ? ['uint8utf8'] : ['binary']
+    @driver = WebSocket::Driver.rack(self, :protocols => protocols)


Well, it's a bit complicated. JavaScript SDK (webmks/wmks.min.js) is eager to grab binary protocol if offered, even if it doesn't know how to handle it. It took me days to figure, but binary protocol is simply not working with vCloud, for some reason. It seems to be working with vCenter (InfraManager), but not with vCloud.

That being said, it would be better to patch the JavaScript SDK to use text socket even if binary is offered, but:

a) only minified version of the SDK is available, so the patching is not trivial
b) InfraManager uses the same SDK and actually uses binary sockets, so we're in danger to spoil it there

I don't like it either, but wasn't able to connect otherwise.

miha-plesko · 2018-03-26T13:58:52Z

lib/websocket_proxy.rb

+    # able to use it :) We workaround this by only forcing the 'uint8utf8' protocol which actually works.
+    force_uint8 = console.protocol.to_s.end_with?('uint8utf8')
+    protocols = force_uint8 ? ['uint8utf8'] : ['binary']
+    @driver = WebSocket::Driver.rack(self, :protocols => protocols)


BTW: isn't it actually dangerous to list multiple protocols here, since it's just proxy socket?

USER ---> proxy socket ---> MIQ ---> actual socket ---> PROVIDER

If PROVIDER (actual socket) would only support uint8utf8 protocol, but MIQ (proxy socket) would advertise [binary, uint8utf8], then binary wouldn't work anyway I think.

miha-plesko · 2018-03-26T14:05:48Z

lib/websocket_proxy.rb

@@ -51,7 +63,7 @@ def transmit(sockets, is_ws)
      @driver.parse(data)
    else
      @right.fetch(64.kilobytes) do |data|
-        @driver.binary(data)
+        @driver.frame(data)


@driver.binary(data)

forcibly threats data obtained as :binary (source code) while

@driver.frame(data)

lets underlying library, websocket-driver, probe the type (see this line). In the first case our uint8utf8 data gets spoiled because it's intepreted incorrectly - should be interpreted as :text but is forcibly interpreted as :binary.

miha-plesko · 2018-03-26T14:13:07Z

lib/websocket_webmks_uint8utf8.rb

@@ -0,0 +1,31 @@
+class WebsocketWebmksUint8utf8 < WebsocketSSLSocket


It would BUT then we can't invoke WebsocketSSLSocket's constructor to setup SSL stuff for us. On the other hand, we must not invoke WebsocketWebmks's constructor because it'll start the wrong driver for us, so...

Can you feel the pain? 😄

In other words:

WebsocketWebmksUint8utf8 <--- WebsocketWebmks <--- WebsocketSSLSocket |------------- super.super ------------------^

From what I know, Ruby does not support invoking super.super methods, therefore we need to do the ugly copy-pasting here... But maybe I missed something?

miha-plesko · 2018-03-28T13:41:59Z

@skateman This one is now green. Please let me know if you'd like to have any additional rspec tests.

skateman · 2018-03-28T13:45:22Z

spec/lib/websocket_proxy_spec.rb

+      it_behaves_like('picking protocol', 'spice', WebsocketSocket, ['binary'], true)
+      it_behaves_like('picking protocol', 'webmks', WebsocketWebmks, ['binary'], true)
+      it_behaves_like('picking protocol', 'webmks-uint8utf8', WebsocketWebmksUint8utf8, ['uint8utf8'], false)
+    end


I'd rather prefer include_examples over it_behaves_like.

Also having a hash as arguments can be much more descriptive here...

Agreed. Done.

miha-plesko · 2018-03-29T09:07:42Z

@skateman examining some specs on UI repo I think I figured what was bothering you with my approach: single shared example with many arguments vs. multiple small shared examples.

I've updated the test to now introduce multiple small shared examples and I think it's quite more readable now. WDYT?

skateman

This breaks the VNC console support, I'm suspecting the frame call on the websocket driver.

Until now, 'binary' protocol for websockets was assumed. But VMware vCloud console access only works via legacy 'uint8utf8' protocol. To make it even more interesting, vCloud's JavaScript SDK actually thinks it supports 'binary', but in fact it doesn't. Well 'binary' protocol works when connecting to vCenter (i.e. `Vmware::InfraManager`), but not for vCloud (`Vmware::CloudManager`). With this commit we implement websocket proxy client for 'uint8utf8' protocol and remove 'binary' from list of offered protocols when 'uint8utf8' is used to make sure JavaScript SDK doesn't overestimate itself (and then fail). Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miha-plesko · 2018-03-29T15:11:54Z

@skateman I've quickly refactored the code so that we use custom WebsocketProxyUintUtf8 rather than messing with existing WebsocketProxy. I think there are just too many ugly IF statements needed otherwise.

Haven't bothered with unit tests yet, just wanted to ask your opinion. Also, I'd kinly ask you to test against vCenter.

Looking forward to be hearing your opinion.

PS: Naturally, it would be even better to also refactor the existing WebsocketProxy implementation so that we could reuse as much code as possible, but I'm not sure you guys will want to merge it then.

miha-plesko · 2018-03-30T10:13:35Z

@skateman I'm sorry to push you, but is there any chance we try to converge with this PR today? We're catching CFME 4.6.2 code freeze and time is not our friend...

So I've just pushed a much nicer implementation of WebsocketProxy than before - I'm quite confident that we're not breaking any exisiting functionality for that reason. TBH I like this solution much more than what we had here before, but am looking forward to be hearing your opinion.

In unit tests I've fallen back to allow_any_instance_of because I just can't seem to figure any other way to prevent real sockets from being open (which fails hard due to missing SSL certs...). Looking forward to hear your opinion on this topic as well.

skateman · 2018-03-30T12:37:35Z

@miha-plesko I just tested your solution from yesterday and it worked, why did you change it? Introducing a new proxy class is IMO not a good idea.

miq-bot · 2018-03-30T13:41:52Z

Checked commit miha-plesko@061037d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

lib/websocket_proxy.rb

⚠️ - Line 70, Col 38 - Lint/ShadowingOuterLocalVariable - Shadowing outer local variable - data.

miha-plesko · 2018-03-30T13:43:05Z

Sorry @skateman, I've pushed back the same code as yesterday. If it works then we're good to go 😄

skateman

Add support for non-binary WebMKS websocket (cherry picked from commit 85754f5) https://bugzilla.redhat.com/show_bug.cgi?id=1563364

simaishi · 2018-04-03T17:43:39Z

Gaprindashvili backport details:

$ git log -1
commit d56944ce84ae94a5f655db852d7619f2eb2bfdd8
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Tue Apr 3 08:50:44 2018 +0200

    Merge pull request #17200 from miha-plesko/vcloud-webmks
    
    Add support for non-binary WebMKS websocket
    (cherry picked from commit 85754f5598797bf9b8cf7e1301483c3d14210af5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1563364

miq-bot added enhancement gaprindashvili/yes labels Mar 26, 2018

miq-bot assigned skateman Mar 26, 2018

This was referenced Mar 26, 2018

Enable WebMKS console access for VMware vCloud's VMs ManageIQ/manageiq-ui-classic#3679

Merged

Add support for vCloud console access via WebMKS ManageIQ/manageiq-providers-vmware#218

Merged

skateman suggested changes Mar 26, 2018

View reviewed changes

miha-plesko commented Mar 26, 2018

View reviewed changes

miha-plesko force-pushed the vcloud-webmks branch from f44b84b to 05d8575 Compare March 28, 2018 13:11

skateman reviewed Mar 28, 2018

View reviewed changes

miha-plesko force-pushed the vcloud-webmks branch 3 times, most recently from 6833c86 to 8bec11d Compare March 29, 2018 09:01

miha-plesko force-pushed the vcloud-webmks branch from 8bec11d to f0dd280 Compare March 29, 2018 09:53

skateman suggested changes Mar 29, 2018

View reviewed changes

miha-plesko force-pushed the vcloud-webmks branch 2 times, most recently from 061037d to 374b873 Compare March 29, 2018 15:05

miha-plesko force-pushed the vcloud-webmks branch 2 times, most recently from 6d22c44 to 89b9aa3 Compare March 30, 2018 10:06

miha-plesko force-pushed the vcloud-webmks branch from 89b9aa3 to 061037d Compare March 30, 2018 13:38

skateman approved these changes Mar 30, 2018

View reviewed changes

martinpovolny merged commit 85754f5 into ManageIQ:master Apr 3, 2018

martinpovolny added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 3, 2018

simaishi pushed a commit that referenced this pull request Apr 3, 2018

Merge pull request #17200 from miha-plesko/vcloud-webmks

d56944c

Add support for non-binary WebMKS websocket (cherry picked from commit 85754f5) https://bugzilla.redhat.com/show_bug.cgi?id=1563364

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 3, 2018

miha-plesko deleted the vcloud-webmks branch January 7, 2019 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for non-binary WebMKS websocket #17200

Add support for non-binary WebMKS websocket #17200

miha-plesko commented Mar 26, 2018 •

edited

Loading

skateman left a comment

skateman Mar 26, 2018

miha-plesko Mar 26, 2018

miha-plesko Mar 26, 2018

skateman Mar 26, 2018

skateman Mar 26, 2018

miha-plesko Mar 26, 2018

skateman Mar 26, 2018

skateman Mar 26, 2018

miha-plesko Mar 26, 2018

skateman Mar 26, 2018

miha-plesko left a comment

miha-plesko Mar 26, 2018

miha-plesko Mar 26, 2018

miha-plesko Mar 26, 2018

miha-plesko Mar 26, 2018

miha-plesko commented Mar 28, 2018

skateman Mar 28, 2018

skateman Mar 28, 2018

miha-plesko Mar 28, 2018

miha-plesko commented Mar 29, 2018

skateman left a comment

miha-plesko commented Mar 29, 2018

miha-plesko commented Mar 30, 2018

skateman commented Mar 30, 2018

miq-bot commented Mar 30, 2018

miha-plesko commented Mar 30, 2018

skateman left a comment

simaishi commented Apr 3, 2018

		@@ -0,0 +1,31 @@
		class WebsocketWebmksUint8utf8 < WebsocketSSLSocket

Add support for non-binary WebMKS websocket #17200

Add support for non-binary WebMKS websocket #17200

Conversation

miha-plesko commented Mar 26, 2018 • edited Loading

skateman 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

miha-plesko 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

miha-plesko commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Mar 29, 2018

skateman left a comment

Choose a reason for hiding this comment

miha-plesko commented Mar 29, 2018

miha-plesko commented Mar 30, 2018

skateman commented Mar 30, 2018

miq-bot commented Mar 30, 2018

miha-plesko commented Mar 30, 2018

skateman left a comment

Choose a reason for hiding this comment

simaishi commented Apr 3, 2018

miha-plesko commented Mar 26, 2018 •

edited

Loading