Add and use APIResponse/FailedAPIResponse classes

Instead of returning a Net::HTTPOK / Net::HTTPCreated, return an APIResponse/FailedAPIResponse instance

296aa96d

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/gitlab/geo/git_push_ssh_proxy.rb

@@ -61,29 +61,39 @@ module Gitlab
         headers = { 'Content-Type' => INFO_REFS_CONTENT_TYPE }
 
         resp = get(url, headers)
-        return resp unless resp.is_a?(Net::HTTPSuccess)
+        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' => PUSH_CONTENT_TYPE,
-          'Accept' => PUSH_ACCEPT
-        }
+        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
@@ -70,16 +69,86 @@ describe Gitlab::Geo::GitPushSSHProxy, :geo do
         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}" }
 
-        before do
-          stub_request(:get, full_info_refs_url).to_return(status: 200, body: info_refs_http_body_full, headers: info_refs_headers)
+        context 'with a failed response' do
+          let(:error_msg) { 'execution expired' }
+
+          before do
+            stub_request(:get, full_info_refs_url).to_timeout
+          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
+
+          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
 
-        it 'returns a Net::HTTPOK' do
-          expect(subject.info_refs).to be_a(Net::HTTPOK)
+        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
 
-        it 'returns a modified body' do
-          expect(subject.info_refs.body).to eql(info_refs_body_short)
+        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
@@ -106,12 +175,69 @@ describe Gitlab::Geo::GitPushSSHProxy, :geo do
           )
         end
 
-        before do
-          stub_request(:post, full_git_receive_pack_url).to_return(status: 201, body: info_refs_body_short, headers: push_headers)
+        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
 
-        it 'returns a Net::HTTPCreated' do
-          expect(subject.push(info_refs_body_short)).to be_a(Net::HTTPCreated)
+        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

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

@@ -18,6 +18,7 @@ describe Gitlab::GitAccessWiki do
     end
 
     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
           }