Websocket proxy worker for HTM5 consoles #7078

skateman · 2016-03-03T14:03:22Z

Parent issue: #2451

List of changes:

Allow running of a rack application specified in RACK_APPLICATION with a web worker
Create a new WebsocketWorker running a dummy rack application on port 5000
Add an UI feature to control the number of the workers
Add a PROTOCOL option into the MiqApache::Conf class for loadbalancing websocket workers
Adjust the specs of the MiqApache module
Implement the WebSocket -> Socket proxy using the websocket-driver gem
Adjust the console access ticket generation for the new proxy
Change the URL in the noVNC & SPICE client settings
Add some new tests

kbrock · 2016-03-11T14:08:13Z

If this is going to take a while to merge, Do you want to pull out the change in API that supports passing the protocol? (but we'd just pass 'http' everywhere)

Fryguy · 2016-03-31T14:11:31Z

db/migrate/20160322162318_add_opened_to_consoles.rb

@@ -0,0 +1,5 @@
+class AddOpenedToConsoles < ActiveRecord::Migration[5.0]
+  def change
+    add_column :consoles, :opened, :boolean, :default => false


Avoid default database values...prefer default_value_for in the model.

@Fryguy fixed

Since this has never been merged, can you just collapse the two migrations together, to reduce the migration churn?

Fryguy · 2016-03-31T17:33:55Z

app/models/console.rb

@@ -0,0 +1,16 @@
+class Console < ApplicationRecord


Not sure I like this class name as it can be confused with the Rails::Console. Not sure what a better name might be, though...RemoteConsole?

SystemConsole or ComputerSystemConsole?

SystemConsole sounds good to me

[skip ci]

kbrock · 2016-04-06T20:19:48Z

lib/websocket_server.rb

+  Pairing = Struct.new(:is_ws, :proxy)
+
+  def initialize
+    @pairing = {}


Do we need a concurrent hash for @pairing?

@kbrock nope, the read is guarded by the critical section of @sockets

kbrock · 2016-04-06T20:21:24Z

This is a bit big for me to wrap my head around, but it looks good.

himdel · 2016-04-07T12:09:51Z

app/views/vm_common/console_spice.html.haml

-          if (!host || !port || !password) {
-            spice_error(__("must set host, port and password"));
+          var secret = "#{@secret}";
+          var url = "#{@url}";


These two probably need to be escaped...

miq-bot · 2016-04-07T12:57:31Z

<github_pr_commenter_batch />Some comments on commits skateman/manageiq@5ee925a~...5aec6be

Gemfile changes detected. /cc @JPrause @simaishi

miq-bot · 2016-04-07T12:57:57Z

Checked commits skateman/manageiq@5ee925a~...5aec6be with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
33 files checked, 23 offenses detected

app/controllers/api_controller/vms.rb

🔶 - Line 211, Col 7 - Style/LeadingCommentSpace - Missing space after #.
🔶 - Line 390, Col 37 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/controllers/vm_common.rb

🔹 - Line 1352, Col 121 - Metrics/LineLength - Line is too long. [151/120]

app/models/manageiq/providers/redhat/infra_manager/vm/remote_console.rb

🔹 - Line 23, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for remote_console_acquire_ticket is too high. [24.1/15]

app/models/manageiq/providers/vmware/infra_manager/vm/remote_console.rb

🔹 - Line 66, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for remote_console_vnc_acquire_ticket is too high. [37.12/15]
🔹 - Line 66, Col 3 - Metrics/MethodLength - Method has too many lines. [27/25]

app/models/miq_server/role_management.rb

🔶 - Line 47, Col 37 - Style/RedundantSelf - Redundant self detected.

app/models/miq_ui_worker.rb

🔶 - Line 19, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.

app/models/miq_web_service_worker.rb

🔶 - Line 9, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.

app/models/miq_websocket_worker.rb

🔶 - Line 7, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.
🔶 - Line 8, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.
🔶 - Line 10, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.
🔶 - Line 12, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.
🔶 - Line 13, Col 32 - Style/MutableConstant - Freeze mutable objects assigned to constants.

app/models/mixins/miq_web_server_worker_mixin.rb

🔹 - Line 119, Col 5 - Metrics/AbcSize - Assignment Branch Condition size for modify_apache_ports is too high. [32.77/15]
🔹 - Line 119, Col 5 - Metrics/CyclomaticComplexity - Cyclomatic complexity for modify_apache_ports is too high. [8/6]
🔹 - Line 119, Col 5 - Metrics/PerceivedComplexity - Perceived complexity for modify_apache_ports is too high. [8/7]

gems/pending/spec/util/miq_apache/conf_spec.rb

🔹 - Line 96, Col 121 - Metrics/LineLength - Line is too long. [147/120]

gems/pending/util/miq_apache/miq_apache.rb

🔶 - Line 281, Col 47 - Style/RegexpLiteral - Use %r around regular expression.

lib/websocket_proxy.rb

🔹 - Line 4, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for initialize is too high. [29.07/15]

lib/websocket_server.rb

🔶 - Line 39, Col 7 - Performance/RedundantMerge - Use @pairing[ws] = Pairing.new(true, proxy); @pairing[sock] = Pairing.new(false, proxy) instead of @pairing.merge!(ws => Pairing.new(true, proxy), sock => Pairing.new(false, proxy)).
🔹 - Line 4, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for initialize is too high. [17.03/15]
🔹 - Line 28, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for call is too high. [18.28/15]

martinpovolny · 2016-04-07T13:05:35Z

We should revisit how the API is defined and if it should remain as it is now. But that can be done in a separate PR.

kbrock · 2016-04-08T19:54:01Z

yay!

cben · 2017-05-10T21:04:44Z

lib/websocket_proxy.rb

+    context.cert = OpenSSL::X509::Certificate.new(File.open('certs/server.cer'))
+    context.key = OpenSSL::PKey::RSA.new(File.open('certs/server.cer.key'))
+    context.ssl_version = :SSLv23
+    context.verify_depth = OpenSSL::SSL::VERIFY_NONE


@skateman accidentally came across this line, I think it's assigning apples to oranges. perhaps you meant:

context.verify_mode = OpenSSL::SSL::VERIFY_NONE

verify_depth is an integer, limits length of chain: https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_verify_depth.html
VERIFY_NONE is conceptually an enum, one of the valid values for verify_mode. It happens to be the integer 0. But verify_depth = 0 doesn't even mean "always accept" nor "never accept", because the root trust anchor and the validated cert don't count towards the depth; AFAICT it means the arbitrary "verify but reject if there are any intermediate certs in the chain"...

Oh, I thought you were going to say "Let's please avoid VERIFY_NONE.

chessbyte added wip enhancement labels Mar 3, 2016

skateman force-pushed the console-proxy-worker branch 3 times, most recently from 0b2020e to ca3c515 Compare March 3, 2016 16:50

skateman mentioned this pull request Mar 3, 2016

Including websocket ProxyPass settings file in the apache configuration ManageIQ/manageiq-appliance#61

Merged

skateman force-pushed the console-proxy-worker branch from ca3c515 to ec5dd27 Compare March 9, 2016 15:05

miq-bot added the dependencies label Mar 9, 2016

skateman mentioned this pull request Mar 15, 2016

Wrong number of arguments when starting EVM #7289

Closed

skateman force-pushed the console-proxy-worker branch 6 times, most recently from f3347d3 to a7a2d17 Compare March 23, 2016 15:24

skateman force-pushed the console-proxy-worker branch 5 times, most recently from ff48981 to 85d7778 Compare March 31, 2016 07:22

Fryguy reviewed Mar 31, 2016
View reviewed changes

skateman force-pushed the console-proxy-worker branch 2 times, most recently from a9c112f to 27680ac Compare March 31, 2016 16:26

Fryguy reviewed Mar 31, 2016
View reviewed changes

skateman changed the title ~~[WIP] Websocket proxy worker for HTML8 consoles~~ [WIP] Websocket proxy worker for HTM5 consoles Apr 4, 2016

skateman force-pushed the console-proxy-worker branch from 76aa935 to d21f697 Compare April 5, 2016 09:22

skateman changed the title ~~[WIP] Websocket proxy worker for HTM5 consoles~~ Websocket proxy worker for HTM5 consoles Apr 5, 2016

chessbyte removed the wip label Apr 5, 2016

skateman force-pushed the console-proxy-worker branch from d21f697 to ebbf3ea Compare April 6, 2016 11:30

skateman mentioned this pull request Apr 6, 2016

Fixed brakeman's console_private_method issues #7740

Merged

skateman force-pushed the console-proxy-worker branch 2 times, most recently from 905ae81 to 957d2eb Compare April 6, 2016 12:52

martinpovolny and others added 3 commits April 6, 2016 17:57

Add model for active consoles.

5ee925a

skeleton for VM request_console action

ca993c9

[skip ci]

Integrate remote consoles with the new model and the API skeleton

c8f6ec5

kbrock reviewed Apr 6, 2016
View reviewed changes

himdel reviewed Apr 7, 2016
View reviewed changes

Implemented websocket to socket proxy as a new worker

5aec6be

skateman force-pushed the console-proxy-worker branch from 957d2eb to 5aec6be Compare April 7, 2016 12:50

martinpovolny merged commit 318a31c into ManageIQ:master Apr 7, 2016

martinpovolny added this to the Sprint 39 Ending Apr 18, 2016 milestone Apr 7, 2016

skateman deleted the console-proxy-worker branch April 7, 2016 14:00

Fryguy mentioned this pull request Apr 7, 2016

Websocket proxy and consoles next steps #2451

Closed

bdunne mentioned this pull request Apr 7, 2016

Docker: Limit open ports for VNC consoles #7803

Closed

jrafanie mentioned this pull request Apr 8, 2016

Closed ports for proxies using the old console implementation ManageIQ/manageiq-appliance-build#120

Merged

cben reviewed May 10, 2017

View reviewed changes

chessbyte assigned martinpovolny Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Websocket proxy worker for HTM5 consoles #7078

Websocket proxy worker for HTM5 consoles #7078

skateman commented Mar 3, 2016

kbrock commented Mar 11, 2016

Fryguy Mar 31, 2016

skateman Mar 31, 2016

Fryguy Mar 31, 2016

Fryguy Mar 31, 2016

chessbyte Mar 31, 2016

martinpovolny Apr 1, 2016

kbrock Apr 6, 2016

skateman Apr 6, 2016

kbrock commented Apr 6, 2016

himdel Apr 7, 2016

miq-bot commented Apr 7, 2016

miq-bot commented Apr 7, 2016

martinpovolny commented Apr 7, 2016

kbrock commented Apr 8, 2016

cben May 10, 2017 •

edited

Loading

kbrock May 12, 2017

Websocket proxy worker for HTM5 consoles #7078

Websocket proxy worker for HTM5 consoles #7078

Conversation

skateman commented Mar 3, 2016

kbrock commented Mar 11, 2016

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

kbrock commented Apr 6, 2016

Choose a reason for hiding this comment

miq-bot commented Apr 7, 2016

miq-bot commented Apr 7, 2016

martinpovolny commented Apr 7, 2016

kbrock commented Apr 8, 2016

cben May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben May 10, 2017 •

edited

Loading