Rewrite CLI container exec websocket client #2599

SpComb · 2017-07-20T14:39:20Z

Fixes #2500 CLI SSL certificate verification
Mitigates #2605 to exit with an 5s websocket open timeout error instead of hanging indefinitely

See #2560 for the agent websocket client

Replaces the CLI Kontena::Websocket::Client with the improved kontena-websocket-client, and rewrite the Kontena::Cli::Helpers::ExecHelper, Kontena::Cli::Containers::ExecComamnd, Kontena::Cli::Services::ExecCommand.

SpComb · 2017-07-20T14:41:19Z

cli/kontena-cli.gemspec

@@ -33,5 +33,5 @@ Gem::Specification.new do |spec|
  spec.add_runtime_dependency "semantic", "~> 1.5"
  spec.add_runtime_dependency "liquid", "~> 4.0.0"
  spec.add_runtime_dependency "tty-table", "~> 0.8.0"
-  spec.add_runtime_dependency "websocket-driver-kontena", "0.6.5"
+  spec.add_runtime_dependency "kontena-websocket-client", "~> 0.1.0"


Needs some trickery to avoid the websocket-extensions dependency...

How can we do that trickery?

s/websocket-extensions/native websocket_mask extension/

The websocket-driver-ruby already handles the require 'websocket_mask' as an optional dependency with a fallback to a pure-ruby implementation... the kontena-websocket-client should probably depend on the pure-ruby websocket-driver-kontena, and then the agent could also depend on some optional gem that provides the optional websocket_mask extension for better performance.

Sounds like a plan.

See kontena/kontena-websocket-client#11 kontena/kontena-websocket-client#12

SpComb · 2017-07-20T14:43:03Z

cli/spec/kontena/cli/helpers/exec_helper_spec.rb

+      it 'connects and sends messages from stdin' do
+        stdin_eol = false
+
+        expect(subject).to receive(:read_stdin).with(tty: true) do |&block|


1) Kontena::Cli::Helpers::ExecHelper#websocket_exec with interactive tty connects and sends messages from stdin Failure/Error: expect(subject).to receive(:read_stdin).with(tty: true) do |&block| block.call 'f' block.call 'oo' block.call "\n" sleep 1 end (#<#<Class:0x00000002c4e058>:0x00000002c4df68>).read_stdin({:tty=>true}) expected: 1 time with arguments: ({:tty=>true}) received: 0 times # ./spec/kontena/cli/helpers/exec_helper_spec.rb:192:in `block (4 levels) in <top (required)>' # ./spec/spec_helper.rb:50:in `block (3 levels) in <top (required)>' # ./spec/spec_helper.rb:48:in `catch' # ./spec/spec_helper.rb:48:in `block (2 levels) in <top (required)>'

…sl_subject_cn for Kontena::Client

jakolehm · 2017-07-21T14:28:58Z

cli/Gemfile

@@ -1,7 +1,10 @@
 source 'https://rubygems.org'

+gem 'kontena-websocket-client', git: 'https://github.com/kontena/kontena-websocket-client.git'


We need to get rid of this git reference.

jakolehm · 2017-07-21T14:29:26Z

cli/kontena-cli.gemspec

@@ -33,5 +33,5 @@ Gem::Specification.new do |spec|
  spec.add_runtime_dependency "semantic", "~> 1.5"
  spec.add_runtime_dependency "liquid", "~> 4.0.0"
  spec.add_runtime_dependency "tty-table", "~> 0.8.0"
-  spec.add_runtime_dependency "websocket-driver-kontena", "0.6.5"
+  spec.add_runtime_dependency "kontena-websocket-client", "~> 0.1.0"


How can we do that trickery?

SpComb · 2017-07-24T08:08:10Z

Needs something to deal with oauth2 token refresh... running kontena container exec after the token expires fails with HTTP 403, because it doesn't do any normal API requests that would refresh the token:

 [error] Error during WebSocket handshake: Unexpected response code: 403

SpComb · 2017-07-24T08:11:36Z

cli/lib/kontena/cli/helpers/exec_helper.rb

+
+      # we do not expect CloseError, because the server will send an 'exit' message first,
+      # and we return before seeing the close frame
+      # TODO: handle HTTP 404 errors


The old code had some special handling of HTTP 404 responses, which this is missing. ATM something like a kontena container exec core-01/weavex looks like this:

[error] Error during WebSocket handshake: Unexpected response code: 404

Compared to other commands:

[error] 404 : Not found (/v1/services/development/null/test)

Probably acceptable... otherwise the kontena-websocket-client would need to trap the ProtocolError raised by the websocket-driver and reraise it as some kind of HTTPError with a @status_code.

This is ok.

Updated the e2e spec to match.

jakolehm · 2017-07-24T14:46:56Z

cli/kontena-cli.gemspec

@@ -33,5 +33,5 @@ Gem::Specification.new do |spec|
  spec.add_runtime_dependency "semantic", "~> 1.5"
  spec.add_runtime_dependency "liquid", "~> 4.0.0"
  spec.add_runtime_dependency "tty-table", "~> 0.8.0"
-  spec.add_runtime_dependency "websocket-driver-kontena", "0.6.5"
+  spec.add_runtime_dependency "kontena-websocket-client", "~> 0.1.0"


Sounds like a plan.

jakolehm · 2017-07-24T14:47:50Z

cli/lib/kontena/cli/helpers/exec_helper.rb

@@ -1,87 +1,190 @@
-require_relative '../../websocket/client'
+require 'io/console'


why did we move this outside? (this was inside a method for lazy loading)

The entire exec_helper.rb is only loaded if you run an exec command:

lib/kontena/cli/containers/exec_command.rb:require_relative '../helpers/exec_helper' lib/kontena/cli/services/exec_command.rb:require_relative '../helpers/exec_helper'

Or was there some even more specific purpose in only loading it when using --interactive?

jakolehm · 2017-07-24T14:48:28Z

cli/lib/kontena/cli/helpers/exec_helper.rb

+
+      # we do not expect CloseError, because the server will send an 'exit' message first,
+      # and we return before seeing the close frame
+      # TODO: handle HTTP 404 errors


This is ok.

SpComb · 2017-07-26T12:38:24Z

Added e2e specs for the error exit statuses.

Tero Marttila added 17 commits July 19, 2017 17:51

remove bundled Kontena::Websocket::Client

eeb61bc

require kontena-websocket-client from github

2a6d8d8

rewrite cli exec helper using Kontena::Websocket::Client

877121f

rewrite cli container exec command using container_exec()

2bd08e1

fix container exec missing cmd_list

d25e645

refactor websocket exec helper connect/read/write logging

46bd45d

websocket logging with DEBUG=websocket

b51abff

tweak

da82caf

fix container id

e415a82

rewrite service exec command

965f4f9

update service exec spec to just spec container_exec()

d08dc85

cli container exec command spec

9519c8b

cli spec client helpers: make master_url customizable

b3e144d

cli exec helper: spec websocket_url, container_exec

cfd720a

fix and spec read_stdin

3d8ed31

fix non-interactive exec to not send, spec websocket_exec

515c383

spec SSL_IGNORE_ERRORS -> verify_mode

dea0e3f

SpComb added bug cli labels Jul 20, 2017

SpComb changed the title ~~[WiP] Rewrite CLI websocket exec to use kontena-websocket-client~~ [WiP] Rewrite CLI websocket container exec helpers Jul 20, 2017

SpComb commented Jul 20, 2017

View reviewed changes

Tero Marttila added 5 commits July 21, 2017 14:57

join write thread to fix specs

246d59f

cli: refactor Kontena::Cli::Config::Server ssl_cert_path, ssl_cert, s…

a30ecbf

…sl_subject_cn for Kontena::Client

cli websocket exec: use server SSL cert, subject cn

f1e1689

cli websocket exec: rescue websocket errors to exit_with_error

cc0eb50

cli websocket exec client timeouts

6683afe

SpComb changed the title ~~[WiP] Rewrite CLI websocket container exec helpers~~ Rewrite CLI websocket container exec helpers Jul 21, 2017

SpComb requested a review from jakolehm July 21, 2017 13:45

SpComb added the status/code-review label Jul 21, 2017

SpComb mentioned this pull request Jul 21, 2017

Actual usage example kontena/kontena-websocket-client#8

Open

jakolehm reviewed Jul 22, 2017

View reviewed changes

SpComb commented Jul 24, 2017

View reviewed changes

drop gemfile kontena-websocket-client from git, use the 0.1.x releases

2ed0cc3

SpComb force-pushed the fix/cli-websocket-client branch from ba54e20 to 2ed0cc3 Compare July 24, 2017 08:21

comment services ExecCommand#maybe_spinner

c927516

SpComb requested a review from jakolehm July 24, 2017 08:34

jakolehm reviewed Jul 24, 2017

View reviewed changes

Tero Marttila added 3 commits July 25, 2017 12:25

fix e2e container exec 404 spec

36bf20a

e2e spec container exec exit errors

d31b000

e2e spec service exec error exits

ed344bf

SpComb requested a review from jakolehm July 26, 2017 12:39

SpComb changed the title ~~Rewrite CLI websocket container exec helpers~~ Rewrite CLI container exec websocket client Jul 26, 2017

SpComb mentioned this pull request Jul 26, 2017

e2e kontena service exec hangs with HTTP 500 errors #2537

Closed

jakolehm approved these changes Jul 26, 2017

View reviewed changes

SpComb merged commit a7e05ed into master Jul 27, 2017

SpComb deleted the fix/cli-websocket-client branch July 27, 2017 07:32

SpComb added this to the 1.4.0 milestone Jul 27, 2017

SpComb mentioned this pull request Jul 31, 2017

CLI validation of wss:// SSL certs for exec commands is broken #2603

Closed

SpComb mentioned this pull request Aug 31, 2017

Server container exec websocket API hangs connnection without any HTTP response on exceptions raised from the r.websocket block #2605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite CLI container exec websocket client #2599

Rewrite CLI container exec websocket client #2599

SpComb commented Jul 20, 2017 •

edited

SpComb Jul 20, 2017

jakolehm Jul 21, 2017

SpComb Jul 24, 2017 •

edited

jakolehm Jul 24, 2017

SpComb Jul 25, 2017 •

edited

SpComb Jul 20, 2017

jakolehm Jul 21, 2017

jakolehm Jul 21, 2017

SpComb commented Jul 24, 2017

SpComb Jul 24, 2017

jakolehm Jul 24, 2017

SpComb Jul 25, 2017

jakolehm Jul 24, 2017

jakolehm Jul 24, 2017

SpComb Jul 25, 2017

jakolehm Jul 24, 2017

SpComb commented Jul 26, 2017

		@@ -1,7 +1,10 @@
		source 'https://rubygems.org'

		gem 'kontena-websocket-client', git: 'https://github.com/kontena/kontena-websocket-client.git'

		@@ -1,87 +1,190 @@
		require_relative '../../websocket/client'
		require 'io/console'

Rewrite CLI container exec websocket client #2599

Rewrite CLI container exec websocket client #2599

Conversation

SpComb commented Jul 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpComb Jul 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpComb Jul 25, 2017 • edited

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

SpComb commented Jul 24, 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

SpComb commented Jul 26, 2017

SpComb commented Jul 20, 2017 •

edited

SpComb Jul 24, 2017 •

edited

SpComb Jul 25, 2017 •

edited