Merge branch 'use-send-url-for-incompatible-runners' into 'master'

Support SendURL for performing indirect download of artifacts if clients does…

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

GITLAB_WORKHORSE_VERSION

-3.3.1
+3.6.0

changelogs/unreleased-ee/use-send-url-for-incompatible-runners.yml

+---
+title: Support SendURL for performing indirect download of artifacts if clients does
+  not specify that it supports that
+merge_request:
+author:
+type: fixed

lib/api/helpers.rb

@@ -418,13 +418,17 @@ module API
       end
     end
 
-    def present_artifacts!(artifacts_file)
+    def present_artifacts!(artifacts_file, direct_download: true)
       return not_found! unless artifacts_file.exists?
 
       if artifacts_file.file_storage?
         present_file!(artifacts_file.path, artifacts_file.filename)
-      else
+      elsif direct_download
         redirect(artifacts_file.url)
+      else
+        header(*Gitlab::Workhorse.send_url(artifacts_file.url))
+        status :ok
+        body
       end
     end
 

lib/api/runner.rb

@@ -244,11 +244,12 @@ module API
       params do
         requires :id, type: Integer, desc: %q(Job's ID)
         optional :token, type: String, desc: %q(Job's authentication token)
+        optional :direct_download, default: false, type: Boolean, desc: %q(Perform direct download from remote storage instead of proxying artifacts)
       end
       get '/:id/artifacts' do
         job = authenticate_job!
 
-        present_artifacts!(job.artifacts_file)
+        present_artifacts!(job.artifacts_file, direct_download: params[:direct_download])
       end
     end
   end

lib/gitlab/workhorse.rb

@@ -151,6 +151,18 @@ module Gitlab
         ]
       end
 
+      def send_url(url, allow_redirects: false)
+        params = {
+          'URL' => url,
+          'AllowRedirects' => allow_redirects
+        }
+
+        [
+          SEND_DATA_HEADER,
+          "send-url:#{encode(params)}"
+        ]
+      end
+
       def terminal_websocket(terminal)
         details = {
           'Terminal' => {

spec/lib/gitlab/workhorse_spec.rb

@@ -451,4 +451,21 @@ describe Gitlab::Workhorse do
       end
     end
   end
+
+  describe '.send_url' do
+    let(:url) { 'http://example.com' }
+
+    subject { described_class.send_url(url) }
+
+    it 'sets the header correctly' do
+      key, command, params = decode_workhorse_header(subject)
+
+      expect(key).to eq("Gitlab-Workhorse-Send-Data")
+      expect(command).to eq("send-url")
+      expect(params).to eq({
+        'URL' => url,
+        'AllowRedirects' => false
+      }.deep_stringify_keys)
+    end
+  end
 end

spec/requests/api/runner_spec.rb

@@ -1157,8 +1157,6 @@ describe API::Runner do
 
           before do
             create(:ci_job_artifact, :archive, file_store: store, job: job)
-
-            download_artifact
           end
 
           context 'when using job token' do
@@ -1168,6 +1166,10 @@ describe API::Runner do
                   'Content-Disposition' => 'attachment; filename=ci_build_artifacts.zip' }
               end
 
+              before do
+                download_artifact
+              end
+
               it 'download artifacts' do
                 expect(response).to have_http_status(200)
                 expect(response.headers).to include download_headers
@@ -1178,8 +1180,27 @@ describe API::Runner do
               let(:store) { JobArtifactUploader::Store::REMOTE }
               let!(:job) { create(:ci_build) }
 
-              it 'download artifacts' do
-                expect(response).to have_http_status(302)
+              context 'when proxy download is being used' do
+                before do
+                  download_artifact(direct_download: false)
+                end
+
+                it 'uses workhorse send-url' do
+                  expect(response).to have_gitlab_http_status(200)
+                  expect(response.headers).to include(
+                    'Gitlab-Workhorse-Send-Data' => /send-url:/)
+                end
+              end
+
+              context 'when direct download is being used' do
+                before do
+                  download_artifact(direct_download: true)
+                end
+
+                it 'receive redirect for downloading artifacts' do
+                  expect(response).to have_gitlab_http_status(302)
+                  expect(response.headers).to include('Location')
+                end
               end
             end
           end
@@ -1187,6 +1208,10 @@ describe API::Runner do
           context 'when using runnners token' do
             let(:token) { job.project.runners_token }
 
+            before do
+              download_artifact
+            end
+
             it 'responds with forbidden' do
               expect(response).to have_gitlab_http_status(403)
             end