Merge branch 'fj-rename-terminal-to-channel' into 'master'

Rename Workhorse terminal_specification to channel_specification

See merge request gitlab-org/gitlab-ce!26939

GITLAB_WORKHORSE_VERSION

-8.4.0
+8.5.0

app/controllers/projects/environments_controller.rb

@@ -117,7 +117,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
     terminal = environment.terminals.try(:first)
     if terminal
       set_workhorse_internal_api_content_type
-      render json: Gitlab::Workhorse.terminal_websocket(terminal)
+      render json: Gitlab::Workhorse.channel_websocket(terminal)
     else
       render html: 'Not found', status: :not_found
     end

app/controllers/projects/jobs_controller.rb

@@ -157,7 +157,7 @@ class Projects::JobsController < Projects::ApplicationController
   # GET .../terminal.ws : implemented in gitlab-workhorse
   def terminal_websocket_authorize
     set_workhorse_internal_api_content_type
-    render json: Gitlab::Workhorse.terminal_websocket(@build.terminal_specification)
+    render json: Gitlab::Workhorse.channel_websocket(@build.terminal_specification)
   end
 
   private

app/models/ci/build_runner_session.rb

@@ -6,6 +6,8 @@ module Ci
   class BuildRunnerSession < ApplicationRecord
     extend Gitlab::Ci::Model
 
+    TERMINAL_SUBPROTOCOL = 'terminal.gitlab.com'.freeze
+
     self.table_name = 'ci_builds_runner_session'
 
     belongs_to :build, class_name: 'Ci::Build', inverse_of: :runner_session
@@ -14,11 +16,21 @@ module Ci
     validates :url, url: { protocols: %w(https) }
 
     def terminal_specification
-      return {} unless url.present?
+      wss_url = Gitlab::UrlHelpers.as_wss(self.url)
+      return {} unless wss_url.present?
+
+      wss_url = "#{wss_url}/exec"
+      channel_specification(wss_url, TERMINAL_SUBPROTOCOL)
+    end
+
+    private
+
+    def channel_specification(url, subprotocol)
+      return {} if subprotocol.blank? || url.blank?
 
       {
-        subprotocols: ['terminal.gitlab.com'].freeze,
-        url: "#{url}/exec".sub("https://", "wss://"),
+        subprotocols: Array(subprotocol),
+        url: url,
         headers: { Authorization: [authorization.presence] }.compact,
         ca_pem: certificate.presence
       }

lib/gitlab/url_helpers.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class UrlHelpers
+    WSS_PROTOCOL = "wss".freeze
+    def self.as_wss(url)
+      return unless url.present?
+
+      URI.parse(url).tap do |uri|
+        uri.scheme = WSS_PROTOCOL
+      end.to_s
+    rescue URI::InvalidURIError
+      nil
+    end
+  end
+end

lib/gitlab/workhorse.rb

@@ -162,16 +162,16 @@ module Gitlab
         ]
       end
 
-      def terminal_websocket(terminal)
+      def channel_websocket(channel)
         details = {
-          'Terminal' => {
-            'Subprotocols' => terminal[:subprotocols],
-            'Url' => terminal[:url],
-            'Header' => terminal[:headers],
-            'MaxSessionTime' => terminal[:max_session_time]
+          'Channel' => {
+            'Subprotocols' => channel[:subprotocols],
+            'Url' => channel[:url],
+            'Header' => channel[:headers],
+            'MaxSessionTime' => channel[:max_session_time]
           }
         }
-        details['Terminal']['CAPem'] = terminal[:ca_pem] if terminal.key?(:ca_pem)
+        details['Channel']['CAPem'] = channel[:ca_pem] if channel.key?(:ca_pem)
 
         details
       end

spec/controllers/projects/environments_controller_spec.rb

@@ -283,7 +283,7 @@ describe Projects::EnvironmentsController do
             .and_return([:fake_terminal])
 
           expect(Gitlab::Workhorse)
-            .to receive(:terminal_websocket)
+            .to receive(:channel_websocket)
             .with(:fake_terminal)
             .and_return(workhorse: :response)
 

spec/controllers/projects/jobs_controller_spec.rb

@@ -989,7 +989,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
       context 'and valid id' do
         it 'returns the terminal for the job' do
           expect(Gitlab::Workhorse)
-            .to receive(:terminal_websocket)
+            .to receive(:channel_websocket)
             .and_return(workhorse: :response)
 
           get_terminal_websocket(id: job.id)

spec/lib/gitlab/workhorse_spec.rb

@@ -94,7 +94,7 @@ describe Gitlab::Workhorse do
     end
   end
 
-  describe '.terminal_websocket' do
+  describe '.channel_websocket' do
     def terminal(ca_pem: nil)
       out = {
         subprotocols: ['foo'],
@@ -108,25 +108,25 @@ describe Gitlab::Workhorse do
 
     def workhorse(ca_pem: nil)
       out = {
-        'Terminal' => {
+        'Channel' => {
           'Subprotocols' => ['foo'],
           'Url' => 'wss://example.com/terminal.ws',
           'Header' => { 'Authorization' => ['Token x'] },
           'MaxSessionTime' => 600
         }
       }
-      out['Terminal']['CAPem'] = ca_pem if ca_pem
+      out['Channel']['CAPem'] = ca_pem if ca_pem
       out
     end
 
     context 'without ca_pem' do
-      subject { described_class.terminal_websocket(terminal) }
+      subject { described_class.channel_websocket(terminal) }
 
       it { is_expected.to eq(workhorse) }
     end
 
     context 'with ca_pem' do
-      subject { described_class.terminal_websocket(terminal(ca_pem: "foo")) }
+      subject { described_class.channel_websocket(terminal(ca_pem: "foo")) }
 
       it { is_expected.to eq(workhorse(ca_pem: "foo")) }
     end

spec/models/ci/build_runner_session_spec.rb

@@ -13,25 +13,33 @@ describe Ci::BuildRunnerSession, model: true do
   it { is_expected.to validate_presence_of(:url).with_message('must be a valid URL') }
 
   describe '#terminal_specification' do
-    let(:terminal_specification) { subject.terminal_specification }
+    let(:specification) { subject.terminal_specification }
+
+    it 'returns terminal.gitlab.com protocol' do
+      expect(specification[:subprotocols]).to eq ['terminal.gitlab.com']
+    end
+
+    it 'returns a wss url' do
+      expect(specification[:url]).to start_with('wss://')
+    end
 
     it 'returns empty hash if no url' do
       subject.url = ''
 
-      expect(terminal_specification).to be_empty
+      expect(specification).to be_empty
     end
 
     context 'when url is present' do
       it 'returns ca_pem nil if empty certificate' do
         subject.certificate = ''
 
-        expect(terminal_specification[:ca_pem]).to be_nil
+        expect(specification[:ca_pem]).to be_nil
       end
 
       it 'adds Authorization header if authorization is present' do
         subject.authorization = 'whatever'
 
-        expect(terminal_specification[:headers]).to include(Authorization: ['whatever'])
+        expect(specification[:headers]).to include(Authorization: ['whatever'])
       end
     end
   end