Merge branch 'ash.mckenzie/display-feedback-git-push-ssh-proxy' into 'master'

Display helpful feedback when proxying an SSH git push to secondary request

See merge request gitlab-org/gitlab-ee!7357

ee/changelogs/unreleased/ash-mckenzie-display-feedback-git-push-ssh-proxy.yml

+---
+title: 'Geo: Display helpful feedback when proxying an SSH git push to secondary request'
+merge_request: 7357
+author:
+type: changed

ee/lib/api/geo.rb

@@ -62,9 +62,9 @@ module API
           authenticate_by_gitlab_shell_token!
           params.delete(:secret_token)
 
-          resp = Gitlab::Geo::GitPushSSHProxy.new(params['data']).info_refs
-          status(resp.code.to_i)
-          { status: true, message: nil, result: Base64.encode64(resp.body.to_s) }
+          response = Gitlab::Geo::GitPushSSHProxy.new(params['data']).info_refs
+          status(response.code)
+          response.body
         end
 
         # Responsible for making HTTP POST /repo.git/git-receive-pack
@@ -82,9 +82,9 @@ module API
           authenticate_by_gitlab_shell_token!
           params.delete(:secret_token)
 
-          resp = Gitlab::Geo::GitPushSSHProxy.new(params['data']).push(Base64.decode64(params['output']))
-          status(resp.code.to_i)
-          { status: true, message: nil, result: Base64.encode64(resp.body.to_s) }
+          response = Gitlab::Geo::GitPushSSHProxy.new(params['data']).push(params['output'])
+          status(response.code)
+          response.body
         end
       end
     end

ee/lib/ee/gitlab/geo_git_access.rb

@@ -37,8 +37,9 @@ module EE
         payload = {
           'action' => 'geo_proxy_to_primary',
           'data' => {
-            'api_endpoints' => [api_v4_geo_proxy_git_push_ssh_info_refs_path, api_v4_geo_proxy_git_push_ssh_push_path],
-            'primary_repo' => geo_primary_http_url_to_repo(project_or_wiki)
+            'info_message' => proxying_to_primary_message,
+            'api_endpoints' => custom_action_api_endpoints,
+            'primary_repo' => primary_http_repo_url
           }
         }
 
@@ -63,6 +64,25 @@ module EE
           geo_primary_http_url_to_repo(project_or_wiki)
         end
       end
+
+      def primary_http_repo_url
+        geo_primary_http_url_to_repo(project_or_wiki)
+      end
+
+      def primary_ssh_url_to_repo
+        geo_primary_ssh_url_to_repo(project_or_wiki)
+      end
+
+      def proxying_to_primary_message
+        ::Gitlab::Geo::GitPushSSHProxy.inform_client_message(primary_ssh_url_to_repo)
+      end
+
+      def custom_action_api_endpoints
+        [
+          api_v4_geo_proxy_git_push_ssh_info_refs_path,
+          api_v4_geo_proxy_git_push_ssh_push_path
+        ]
+      end
     end
   end
 end

ee/lib/gitlab/geo/git_push_ssh_proxy.rb

@@ -4,46 +4,96 @@ module Gitlab
   module Geo
     class GitPushSSHProxy
       HTTP_READ_TIMEOUT = 10
-      HTTP_SUCCESS_CODE = '200'.freeze
+
+      INFO_REFS_CONTENT_TYPE = 'application/x-git-upload-pack-request'.freeze
+      PUSH_CONTENT_TYPE = 'application/x-git-receive-pack-request'.freeze
+      PUSH_ACCEPT = 'application/x-git-receive-pack-result'.freeze
 
       MustBeASecondaryNode = Class.new(StandardError)
 
+      class APIResponse
+        attr_reader :code, :body
+
+        def initialize(code, body)
+          @code = code
+          @body = body
+        end
+
+        def self.from_http_response(response, primary_repo)
+          success = response.is_a?(Net::HTTPSuccess)
+          body = response.body.to_s
+
+          if success
+            result = Base64.encode64(body)
+          else
+            message = failed_message(body, primary_repo)
+          end
+
+          new(response.code.to_i, status: success, message: message, result: result)
+        end
+
+        def self.failed_message(str, primary_repo)
+          "Failed to contact primary #{primary_repo}\nError: #{str}"
+        end
+      end
+
+      class FailedAPIResponse < APIResponse
+        def self.from_exception(ex_message, primary_repo, code: 500)
+          new(code.to_i,
+              status: false,
+              message: failed_message(ex_message, primary_repo),
+              result: nil)
+        end
+      end
+
       def initialize(data)
         @data = data
       end
 
+      def self.inform_client_message(primary_repo_ssh)
+        "You're pushing to a Geo secondary.\nWe'll help you by proxying this request to the primary: #{primary_repo_ssh}"
+      end
+
       def info_refs
         ensure_secondary!
 
         url = "#{primary_repo}/info/refs?service=git-receive-pack"
-        headers = {
-          'Content-Type' => 'application/x-git-upload-pack-request'
-        }
+        headers = { 'Content-Type' => INFO_REFS_CONTENT_TYPE }
 
         resp = get(url, headers)
-        return resp unless resp.code == HTTP_SUCCESS_CODE
+        resp.body = remove_http_service_fragment_from(resp.body) if resp.is_a?(Net::HTTPSuccess)
 
-        resp.body = remove_http_service_fragment_from(resp.body)
-
-        resp
+        APIResponse.from_http_response(resp, primary_repo)
+      rescue => e
+        handle_exception(e)
       end
 
-      def push(info_refs_response)
+      def push(encoded_info_refs_response)
         ensure_secondary!
 
         url = "#{primary_repo}/git-receive-pack"
-        headers = {
-          'Content-Type' => 'application/x-git-receive-pack-request',
-          'Accept' => 'application/x-git-receive-pack-result'
-        }
+        headers = { 'Content-Type' => PUSH_CONTENT_TYPE, 'Accept' => PUSH_ACCEPT }
+        info_refs_response = Base64.decode64(encoded_info_refs_response)
 
-        post(url, info_refs_response, headers)
+        resp = post(url, info_refs_response, headers)
+        APIResponse.from_http_response(resp, primary_repo)
+      rescue => e
+        handle_exception(e)
       end
 
       private
 
       attr_reader :data
 
+      def handle_exception(ex)
+        case ex
+        when MustBeASecondaryNode
+          raise(ex)
+        else
+          FailedAPIResponse.from_exception(ex.message, primary_repo)
+        end
+      end
+
       def primary_repo
         @primary_repo ||= data['primary_repo']
       end

ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb

@@ -15,8 +15,7 @@ describe Gitlab::Geo::GitPushSSHProxy, :geo do
   let(:base_request) { double(Gitlab::Geo::BaseRequest.new.authorization) }
 
   let(:info_refs_body_short) do
-    "008f43ba78b7912f7bf7ef1d7c3b8a0e5ae14a759dfa refs/heads/masterreport-status delete-refs side-band-64k quiet atomic ofs-delta agent=git/2.18.0
-0000"
+    "008f43ba78b7912f7bf7ef1d7c3b8a0e5ae14a759dfa refs/heads/masterreport-status delete-refs side-band-64k quiet atomic ofs-delta agent=git/2.18.0\n0000"
   end
 
   let(:base_headers) do
@@ -26,85 +25,220 @@ describe Gitlab::Geo::GitPushSSHProxy, :geo do
     }
   end
 
+  let(:primary_repo_http) { geo_primary_http_url_to_repo(project) }
+  let(:primary_repo_ssh) { geo_primary_ssh_url_to_repo(project) }
+
   let(:data) do
     {
       'gl_id' => "key-#{key.id}",
-      'primary_repo' => "#{primary_node.url}#{project.repository.full_path}.git"
+      'primary_repo' => primary_repo_http
     }
   end
 
-  subject { described_class.new(data) }
-
-  before do
-    stub_current_geo_node(current_node)
-
-    allow(Gitlab::Geo::BaseRequest).to receive(:new).and_return(base_request)
-    allow(base_request).to receive(:authorization).and_return('secret')
+  describe '.inform_client_message' do
+    it 'returns a message, with the ssh address' do
+      expect(described_class.inform_client_message(primary_repo_ssh)).to eql("You're pushing to a Geo secondary.\nWe'll help you by proxying this request to the primary: #{primary_repo_ssh}")
+    end
   end
 
-  describe '#info_refs' do
-    context 'against primary node' do
-      let(:current_node) { primary_node }
+  context 'instance methods' do
+    subject { described_class.new(data) }
 
-      it 'raises an exception' do
-        expect do
-          subject.info_refs
-        end.to raise_error(described_class::MustBeASecondaryNode, 'Node is not a secondary or there is no primary Geo node')
-      end
+    before do
+      stub_current_geo_node(current_node)
+
+      allow(Gitlab::Geo::BaseRequest).to receive(:new).and_return(base_request)
+      allow(base_request).to receive(:authorization).and_return('secret')
     end
 
-    context 'against secondary node' do
-      let(:current_node) { secondary_node }
+    describe '#info_refs' do
+      context 'against primary node' do
+        let(:current_node) { primary_node }
 
-      let(:full_info_refs_url) { "#{primary_node.url}#{project.full_path}.git/info/refs?service=git-receive-pack" }
-      let(:info_refs_headers) { base_headers.merge('Content-Type' => 'application/x-git-upload-pack-request') }
-      let(:info_refs_http_body_full) do
-        "001f# service=git-receive-pack
-0000#{info_refs_body_short}"
+        it 'raises an exception' do
+          expect do
+            subject.info_refs
+          end.to raise_error(described_class::MustBeASecondaryNode, 'Node is not a secondary or there is no primary Geo node')
+        end
       end
 
-      before do
-        stub_request(:get, full_info_refs_url).to_return(status: 200, body: info_refs_http_body_full, headers: info_refs_headers)
-      end
+      context 'against secondary node' do
+        let(:current_node) { secondary_node }
 
-      it 'returns a Net::HTTPOK' do
-        expect(subject.info_refs).to be_a(Net::HTTPOK)
-      end
+        let(:full_info_refs_url) { "#{primary_repo_http}/info/refs?service=git-receive-pack" }
+        let(:info_refs_headers) { base_headers.merge('Content-Type' => 'application/x-git-upload-pack-request') }
+        let(:info_refs_http_body_full) { "001f# service=git-receive-pack\n0000#{info_refs_body_short}" }
 
-      it 'returns a modified body' do
-        expect(subject.info_refs.body).to eql(info_refs_body_short)
-      end
-    end
-  end
+        context 'with a failed response' do
+          let(:error_msg) { 'execution expired' }
 
-  describe '#push' do
-    context 'against primary node' do
-      let(:current_node) { primary_node }
+          before do
+            stub_request(:get, full_info_refs_url).to_timeout
+          end
 
-      it 'raises an exception' do
-        expect do
-          subject.push(info_refs_body_short)
-        end.to raise_error(described_class::MustBeASecondaryNode)
-      end
-    end
+          it 'returns a Gitlab::Geo::GitPushSSHProxy::FailedAPIResponse' do
+            expect(subject.info_refs).to be_a(Gitlab::Geo::GitPushSSHProxy::FailedAPIResponse)
+          end
+
+          it 'has a code of 500' do
+            expect(subject.info_refs.code).to be(500)
+          end
+
+          it 'has a status of false' do
+            expect(subject.info_refs.body[:status]).to be_falsey
+          end
 
-    context 'against secondary node' do
-      let(:current_node) { secondary_node }
+          it 'has a messsage' do
+            expect(subject.info_refs.body[:message]).to eql("Failed to contact primary #{primary_repo_http}\nError: #{error_msg}")
+          end
 
-      let(:full_git_receive_pack_url) { "#{primary_node.url}#{project.full_path}.git/git-receive-pack" }
-      let(:push_headers) do
-        base_headers.merge(
-          'Content-Type' => 'application/x-git-receive-pack-request',
-          'Accept' => 'application/x-git-receive-pack-result'
-        )
+          it 'has no result' do
+            expect(subject.info_refs.body[:result]).to be_nil
+          end
+        end
+
+        context 'with an invalid response' do
+          let(:error_msg) { 'dial unix /Users/ash/src/gdk/gdk-ee/gitlab.socket: connect: connection refused' }
+
+          before do
+            stub_request(:get, full_info_refs_url).to_return(status: 502, body: error_msg, headers: info_refs_headers)
+          end
+
+          it 'returns a Gitlab::Geo::GitPushSSHProxy::FailedAPIResponse' do
+            expect(subject.info_refs).to be_a(Gitlab::Geo::GitPushSSHProxy::APIResponse)
+          end
+
+          it 'has a code of 502' do
+            expect(subject.info_refs.code).to be(502)
+          end
+
+          it 'has a status of false' do
+            expect(subject.info_refs.body[:status]).to be_falsey
+          end
+
+          it 'has a messsage' do
+            expect(subject.info_refs.body[:message]).to eql("Failed to contact primary #{primary_repo_http}\nError: #{error_msg}")
+          end
+
+          it 'has no result' do
+            expect(subject.info_refs.body[:result]).to be_nil
+          end
+        end
+
+        context 'with a valid response' do
+          before do
+            stub_request(:get, full_info_refs_url).to_return(status: 200, body: info_refs_http_body_full, headers: info_refs_headers)
+          end
+
+          it 'returns a Gitlab::Geo::GitPushSSHProxy::APIResponse' do
+            expect(subject.info_refs).to be_a(Gitlab::Geo::GitPushSSHProxy::APIResponse)
+          end
+
+          it 'has a code of 200' do
+            expect(subject.info_refs.code).to be(200)
+          end
+
+          it 'has a status of true' do
+            expect(subject.info_refs.body[:status]).to be_truthy
+          end
+
+          it 'has no messsage' do
+            expect(subject.info_refs.body[:message]).to be_nil
+          end
+
+          it 'returns a modified body' do
+            expect(subject.info_refs.body[:result]).to eql(Base64.encode64(info_refs_body_short))
+          end
+        end
       end
+    end
+
+    describe '#push' do
+      context 'against primary node' do
+        let(:current_node) { primary_node }
 
-      before do
-        stub_request(:post, full_git_receive_pack_url).to_return(status: 201, body: info_refs_body_short, headers: push_headers)
+        it 'raises an exception' do
+          expect do
+            subject.push(info_refs_body_short)
+          end.to raise_error(described_class::MustBeASecondaryNode)
+        end
       end
 
-      it 'returns a Net::HTTPCreated' do
-        expect(subject.push(info_refs_body_short)).to be_a(Net::HTTPCreated)
+      context 'against secondary node' do
+        let(:current_node) { secondary_node }
+
+        let(:full_git_receive_pack_url) { "#{primary_repo_http}/git-receive-pack" }
+        let(:push_headers) do
+          base_headers.merge(
+            'Content-Type' => 'application/x-git-receive-pack-request',
+            'Accept' => 'application/x-git-receive-pack-result'
+          )
+        end
+
+        context 'with a failed response' do
+          let(:error_msg) { 'execution expired' }
+
+          before do
+            stub_request(:post, full_git_receive_pack_url).to_timeout
+          end
+
+          it 'returns a Gitlab::Geo::GitPushSSHProxy::FailedAPIResponse' do
+            expect(subject.push(info_refs_body_short)).to be_a(Gitlab::Geo::GitPushSSHProxy::FailedAPIResponse)
+          end
+
+          it 'has a messsage' do
+            expect(subject.push(info_refs_body_short).body[:message]).to eql("Failed to contact primary #{primary_repo_http}\nError: #{error_msg}")
+          end
+
+          it 'has no result' do
+            expect(subject.push(info_refs_body_short).body[:result]).to be_nil
+          end
+        end
+
+        context 'with an invalid response' do
+          let(:error_msg) { 'dial unix /Users/ash/src/gdk/gdk-ee/gitlab.socket: connect: connection refused' }
+
+          before do
+            stub_request(:post, full_git_receive_pack_url).to_return(status: 502, body: error_msg, headers: push_headers)
+          end
+
+          it 'returns a Gitlab::Geo::GitPushSSHProxy::FailedAPIResponse' do
+            expect(subject.push(info_refs_body_short)).to be_a(Gitlab::Geo::GitPushSSHProxy::APIResponse)
+          end
+
+          it 'has a messsage' do
+            expect(subject.push(info_refs_body_short).body[:message]).to eql("Failed to contact primary #{primary_repo_http}\nError: #{error_msg}")
+          end
+
+          it 'has no result' do
+            expect(subject.push(info_refs_body_short).body[:result]).to be_nil
+          end
+        end
+
+        context 'with a valid response' do
+          let(:body) { '<binary content>' }
+          let(:base64_encoded_body) { Base64.encode64(body) }
+
+          before do
+            stub_request(:post, full_git_receive_pack_url).to_return(status: 201, body: body, headers: push_headers)
+          end
+
+          it 'returns a Gitlab::Geo::GitPushSSHProxy::APIResponse' do
+            expect(subject.push(info_refs_body_short)).to be_a(Gitlab::Geo::GitPushSSHProxy::APIResponse)
+          end
+
+          it 'has a code of 201' do
+            expect(subject.push(info_refs_body_short).code).to be(201)
+          end
+
+          it 'has no messsage' do
+            expect(subject.push(info_refs_body_short).body[:message]).to be_nil
+          end
+
+          it 'has a result' do
+            expect(subject.push(info_refs_body_short).body[:result]).to eql(base64_encoded_body)
+          end
+        end
       end
     end
   end

ee/spec/lib/gitlab/git_access_spec.rb

@@ -18,7 +18,8 @@ describe Gitlab::GitAccess do
       allow(Gitlab::Database).to receive(:read_only?) { true }
     end
 
-    let(:primary_repo_url) { "https://localhost:3000/gitlab/#{project.full_path}.git" }
+    let(:primary_repo_url) { geo_primary_http_url_to_repo(project) }
+    let(:primary_repo_ssh_url) { geo_primary_ssh_url_to_repo(project) }
 
     it_behaves_like 'a read-only GitLab instance'
   end

ee/spec/lib/gitlab/git_access_wiki_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::GitAccessWiki do
   let(:user) { create(:user) }
-  let(:project) { create(:project, :repository) }
+  let(:project) { create(:project, :wiki_repo) }
   let(:changes) { ['6f6d7e7ed 570e7b2ab refs/heads/master'] }
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
@@ -17,7 +17,8 @@ describe Gitlab::GitAccessWiki do
       allow(Gitlab::Database).to receive(:read_only?) { true }
     end
 
-    let(:primary_repo_url) { "https://localhost:3000/gitlab/#{project.full_path}.wiki.git" }
+    let(:primary_repo_url) { geo_primary_http_url_to_repo(project.wiki) }
+    let(:primary_repo_ssh_url) { geo_primary_ssh_url_to_repo(project.wiki) }
 
     it_behaves_like 'a read-only GitLab instance'
   end

ee/spec/requests/api/geo_spec.rb

@@ -290,7 +290,8 @@ describe API::Geo do
 
   describe '/geo/proxy_git_push_ssh' do
     let(:secret_token) { Gitlab::Shell.secret_token }
-    let(:data) { { primary_repo: 'http://localhost:3001/testuser/repo.git', gl_id: 'key-1', gl_username: 'testuser' } }
+    let(:primary_repo) { 'http://localhost:3001/testuser/repo.git' }
+    let(:data) { { primary_repo: primary_repo, gl_id: 'key-1', gl_username: 'testuser' } }
 
     before do
       stub_current_geo_node(secondary_node)
@@ -335,10 +336,17 @@ describe API::Geo do
         end
 
         context 'with a valid secret token' do
-          let(:http_response) { double(Net::HTTPResponse, code: 200, body: 'something here') }
+          let(:http_response) { double(Net::HTTPOK, code: 200, body: 'something here') }
+          let(:api_response) { Gitlab::Geo::GitPushSSHProxy::APIResponse.from_http_response(http_response, primary_repo) }
+
+          before do
+            # Mocking a real Net::HTTPSuccess is very difficult as it's not
+            # easy to instantiate the class due to the way it sets the body
+            expect(http_response).to receive(:is_a?).with(Net::HTTPSuccess).and_return(true)
+          end
 
           it 'responds with 200' do
-            expect(git_push_ssh_proxy).to receive(:info_refs).and_return(http_response)
+            expect(git_push_ssh_proxy).to receive(:info_refs).and_return(api_response)
 
             post api('/geo/proxy_git_push_ssh/info_refs'), { secret_token: secret_token, data: data }
 
@@ -360,8 +368,7 @@ describe API::Geo do
       end
 
       context 'with all required params' do
-        let(:text) { 'output text' }
-        let(:output) { Base64.encode64(text) }
+        let(:output) { Base64.encode64('info_refs content') }
         let(:git_push_ssh_proxy) { double(Gitlab::Geo::GitPushSSHProxy) }
 
         before do
@@ -389,10 +396,17 @@ describe API::Geo do
         end
 
         context 'with a valid secret token' do
-          let(:http_response) { double(Net::HTTPResponse, code: 201, body: 'something here') }
+          let(:http_response) { double(Net::HTTPCreated, code: 201, body: 'something here', class: Net::HTTPCreated) }
+          let(:api_response) { Gitlab::Geo::GitPushSSHProxy::APIResponse.from_http_response(http_response, primary_repo) }
+
+          before do
+            # Mocking a real Net::HTTPSuccess is very difficult as it's not
+            # easy to instantiate the class due to the way it sets the body
+            expect(http_response).to receive(:is_a?).with(Net::HTTPSuccess).and_return(true)
+          end
 
           it 'responds with 201' do
-            expect(git_push_ssh_proxy).to receive(:push).with(text).and_return(http_response)
+            expect(git_push_ssh_proxy).to receive(:push).with(output).and_return(api_response)
 
             post api('/geo/proxy_git_push_ssh/push'), { secret_token: secret_token, data: data, output: output }
 

ee/spec/support/shared_examples/git_access_shared_examples.rb

@@ -31,6 +31,7 @@ shared_examples 'a read-only GitLab instance' do
         {
           'action' => 'geo_proxy_to_primary',
           'data' => {
+            'info_message' => "You're pushing to a Geo secondary.\nWe'll help you by proxying this request to the primary: #{primary_repo_ssh_url}",
             'api_endpoints' => %w{/api/v4/geo/proxy_git_push_ssh/info_refs /api/v4/geo/proxy_git_push_ssh/push},
             'primary_repo' => primary_repo_url
           }