Merge branch 'fix-proxy-downloads' into 'master'

Fix proxy_download support for lfs controller

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

doc/administration/job_artifacts.md

@@ -108,7 +108,7 @@ For source installations the following settings are nested under `artifacts:` an
 | `enabled` | Enable/disable object storage | `false` |
 | `remote_directory` | The bucket name where Artfacts will be stored| |
 | `background_upload` | Set to false to disable automatic upload. Option may be removed once upload is direct to S3 | `true` |
-| `proxy_download` | Set to false to disable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
+| `proxy_download` | Set to true to enable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
 | `connection` | Various connection options described below | |
 
 #### S3 compatible connection settings

doc/administration/uploads.md

@@ -66,7 +66,7 @@ For source installations the following settings are nested under `uploads:` and
 | `enabled` | Enable/disable object storage | `false` |
 | `remote_directory` | The bucket name where Uploads will be stored| |
 | `background_upload` | Set to false to disable automatic upload. Option may be removed once upload is direct to S3 | `true` |
-| `proxy_download` | Set to false to disable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
+| `proxy_download` | Set to true to enable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
 | `connection` | Various connection options described below | |
 
 #### S3 compatible connection settings

doc/workflow/lfs/lfs_administration.md

@@ -65,7 +65,7 @@ For source installations the following settings are nested under `lfs:` and then
 | `remote_directory` | The bucket name where LFS objects will be stored| |
 | `direct_upload` | Set to true to enable direct upload of LFS without the need of local shared storage. Option may be removed once we decide to support only single storage for all files. | `false` |
 | `background_upload` | Set to false to disable automatic upload. Option may be removed once upload is direct to S3 | `true` |
-| `proxy_download` | Set to false to disable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
+| `proxy_download` | Set to true to enable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` |
 | `connection` | Various connection options described below | |
 
 #### S3 compatible connection settings

ee/app/controllers/concerns/send_file_upload.rb

@@ -8,7 +8,8 @@ module SendFileUpload
     if file_upload.file_storage?
       send_file file_upload.path, send_params
     elsif file_upload.class.proxy_download_enabled?
-      Gitlab::Workhorse.send_url(file_upload.url(**redirect_params))
+      headers.store(*Gitlab::Workhorse.send_url(file_upload.url(**redirect_params)))
+      head :ok
     else
       redirect_to file_upload.url(**redirect_params)
     end

ee/changelogs/unreleased/fix-proxy-downloads.yml

+---
+title: Fix proxy_download support for lfs controller
+merge_request:
+author:
+type: fixed

ee/spec/controllers/concerns/send_file_upload_spec.rb

@@ -64,10 +64,12 @@ describe SendFileUpload do
         end
 
         it 'sends a file' do
-          subject
+          headers = double
+          expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-url:/)
+          expect(controller).to receive(:headers) { headers }
+          expect(controller).to receive(:head).with(:ok)
 
-          is_expected.to start_with(Gitlab::Workhorse::SEND_DATA_HEADER)
-          is_expected.to end_with(/^send-url:/)
+          subject
         end
       end
 

ee/spec/requests/api/jobs_spec.rb

@@ -39,19 +39,25 @@ describe API::Jobs do
       end
     end
 
-    context 'normal authentication' do
+    context 'for normal authentication when job with artifacts are stored remotely' do
       before do
-        stub_artifacts_object_storage
-      end
+        stub_artifacts_object_storage(proxy_download: proxy_download)
 
-      context 'when job with artifacts are stored remotely' do
-        let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: job) }
+        create(:ci_job_artifact, :archive, :remote_store, job: job)
 
-        before do
-          job.reload
+        get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
+      end
 
-          get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
+      context 'when proxy download is enabled' do
+        let(:proxy_download) { true }
+
+        it 'responds with the workhorse send-url' do
+          expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("send-url:")
         end
+      end
+
+      context 'when proxy download is disabled' do
+        let(:proxy_download) { false }
 
         it 'returns location redirect' do
           expect(response).to have_gitlab_http_status(302)

ee/spec/support/stub_object_storage.rb

 module StubConfiguration
   def stub_object_storage_uploader(
     config:, uploader:, remote_directory:, enabled: true, licensed: true,
+    proxy_download: false,
     background_upload: false, direct_upload: false
   )
     allow(config).to receive(:enabled) { enabled }
+    allow(config).to receive(:proxy_download) { proxy_download }
     allow(config).to receive(:background_upload) { background_upload }
     allow(config).to receive(:direct_upload) { direct_upload }
 

spec/requests/lfs_http_spec.rb

@@ -243,17 +243,34 @@ describe 'Git LFS API and storage' do
             it_behaves_like 'responds with a file'
 
             context 'when LFS uses object storage' do
-              let(:before_get) do
-                stub_lfs_object_storage
-                lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
-              end
+              context 'when proxy download is enabled' do
+                let(:before_get) do
+                  stub_lfs_object_storage(proxy_download: true)
+                  lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+                end
+
+                it 'responds with redirect' do
+                  expect(response).to have_gitlab_http_status(200)
+                end
 
-              it 'responds with redirect' do
-                expect(response).to have_gitlab_http_status(302)
+                it 'responds with the workhorse send-url' do
+                  expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("send-url:")
+                end
               end
 
-              it 'responds with the file location' do
-                expect(response.location).to include(lfs_object.reload.file.path)
+              context 'when proxy download is disabled' do
+                let(:before_get) do
+                  stub_lfs_object_storage(proxy_download: false)
+                  lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+                end
+
+                it 'responds with redirect' do
+                  expect(response).to have_gitlab_http_status(302)
+                end
+
+                it 'responds with the file location' do
+                  expect(response.location).to include(lfs_object.reload.file.path)
+                end
               end
             end
           end
@@ -1380,9 +1397,6 @@ describe 'Git LFS API and storage' do
     def lfs_tmp_file
       "#{sample_oid}012345678"
     end
-
-    def setup_tempfile(lfs_tmp)
-    end
   end
 
   def enable_lfs