Merge branch 'jej/lfs-object-storage' into 'master'

Can migrate LFS objects to S3 style object storage

Closes #2841

See merge request !2760

11 files changed, 379 insertions, 66 deletions

diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb
index caa63e7bd22..2bd8f8e2bfc 100644
--- a/spec/controllers/projects/artifacts_controller_spec.rb
+++ b/spec/controllers/projects/artifacts_controller_spec.rb
@@ -22,7 +22,7 @@ describe Projects::ArtifactsController do
 
   describe 'GET download' do
     it 'sends the artifacts file' do
-      expect(controller).to receive(:send_file).with(job.artifacts_file.path, disposition: 'attachment').and_call_original
+      expect(controller).to receive(:send_file).with(job.artifacts_file.path, hash_including(disposition: 'attachment')).and_call_original
 
       get :download, namespace_id: project.namespace, project_id: project, job_id: job
     end
@@ -66,19 +66,52 @@ describe Projects::ArtifactsController do
 
   describe 'GET raw' do
     context 'when the file exists' do
-      it 'serves the file using workhorse' do
-        get :raw, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
+      let(:path) { 'ci_artifacts.txt' }
+      let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline, artifacts_file_store: store, artifacts_metadata_store: store) }
 
-        send_data = response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]
+      shared_examples 'a valid file' do
+        it 'serves the file using workhorse' do
+          subject
 
-        expect(send_data).to start_with('artifacts-entry:')
+          expect(send_data).to start_with('artifacts-entry:')
 
-        base64_params = send_data.sub(/\Aartifacts\-entry:/, '')
-        params = JSON.parse(Base64.urlsafe_decode64(base64_params))
+          expect(params.keys).to eq(%w(Archive Entry))
+          expect(params['Archive']).to start_with(archive_path)
+          # On object storage, the URL can end with a query string
+          expect(params['Archive']).to match(/build_artifacts.zip(\?[^?]+)?$/)
+          expect(params['Entry']).to eq(Base64.encode64('ci_artifacts.txt'))
+        end
+
+        def send_data
+          response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]
+        end
 
-        expect(params.keys).to eq(%w(Archive Entry))
-        expect(params['Archive']).to end_with('build_artifacts.zip')
-        expect(params['Entry']).to eq(Base64.encode64('ci_artifacts.txt'))
+        def params
+          @params ||= begin
+            base64_params = send_data.sub(/\Aartifacts\-entry:/, '')
+            JSON.parse(Base64.urlsafe_decode64(base64_params))
+          end
+        end
+      end
+
+      context 'when using local file storage' do
+        it_behaves_like 'a valid file' do
+          let(:store) { ObjectStoreUploader::LOCAL_STORE }
+          let(:archive_path) { ArtifactUploader.local_store_path }
+        end
+      end
+
+      context 'when using remote file storage' do
+        before do
+          stub_artifacts_object_storage
+        end
+
+        it_behaves_like 'a valid file' do
+          let!(:artifact) { create(:ci_job_artifact, :archive, :remote_store, job: job) }
+          let!(:job) { create(:ci_build, :success, pipeline: pipeline) }
+          let(:store) { ObjectStorage::Store::REMOTE }
+          let(:archive_path) { 'https://' }
+        end
       end
     end
   end
diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb
index b4eaab29fed..7e3d6574e8d 100644
--- a/spec/controllers/projects/raw_controller_spec.rb
+++ b/spec/controllers/projects/raw_controller_spec.rb
@@ -8,10 +8,7 @@ describe Projects::RawController do
       let(:id) { 'master/README.md' }
 
       it 'delivers ASCII file' do
-        get(:show,
-            namespace_id: public_project.namespace.to_param,
-            project_id: public_project,
-            id: id)
+        get_show(public_project, id)
 
         expect(response).to have_http_status(200)
         expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
@@ -25,10 +22,7 @@ describe Projects::RawController do
       let(:id) { 'master/files/images/6049019_460s.jpg' }
 
       it 'sets image content type header' do
-        get(:show,
-            namespace_id: public_project.namespace.to_param,
-            project_id: public_project,
-            id: id)
+        get_show(public_project, id)
 
         expect(response).to have_http_status(200)
         expect(response.header['Content-Type']).to eq('image/jpeg')
@@ -54,21 +48,40 @@ describe Projects::RawController do
 
           it 'serves the file' do
             expect(controller).to receive(:send_file).with("#{Gitlab.config.shared.path}/lfs-objects/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment')
-            get(:show,
-                namespace_id: public_project.namespace.to_param,
-                project_id: public_project,
-                id: id)
+            get_show(public_project, id)
 
             expect(response).to have_http_status(200)
           end
+
+          context 'and lfs uses object storage' do
+            before do
+              lfs_object.file = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png")
+              lfs_object.save!
+              stub_lfs_object_storage
+              lfs_object.file.migrate!(LfsObjectUploader::REMOTE_STORE)
+            end
+
+            it 'responds with redirect to file' do
+              get_show(public_project, id)
+
+              expect(response).to have_gitlab_http_status(302)
+              expect(response.location).to include(lfs_object.reload.file.path)
+            end
+
+            it 'sets content disposition' do
+              get_show(public_project, id)
+
+              file_uri = URI.parse(response.location)
+              params = CGI.parse(file_uri.query)
+
+              expect(params["response-content-disposition"].first).to eq 'attachment;filename="lfs_object.iso"'
+            end
+          end
         end
 
         context 'when project does not have access' do
           it 'does not serve the file' do
-            get(:show,
-                namespace_id: public_project.namespace.to_param,
-                project_id: public_project,
-                id: id)
+            get_show(public_project, id)
 
             expect(response).to have_http_status(404)
           end
@@ -81,10 +94,7 @@ describe Projects::RawController do
         end
 
         it 'delivers ASCII file' do
-          get(:show,
-              namespace_id: public_project.namespace.to_param,
-              project_id: public_project,
-              id: id)
+          get_show(public_project, id)
 
           expect(response).to have_http_status(200)
           expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
@@ -95,4 +105,10 @@ describe Projects::RawController do
       end
     end
   end
+
+  def get_show(project, id)
+    get(:show, namespace_id: project.namespace.to_param,
+               project_id: project,
+               id: id)
+  end
 end
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 8f213dbb597..6be658a3adf 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -17,8 +17,12 @@ describe API::Jobs do
   let(:api_user) { user }
   let(:reporter) { create(:project_member, :reporter, project: project).user }
   let(:guest) { create(:project_member, :guest, project: project).user }
+  let(:cross_project_pipeline_enabled) { true }
+  let(:object_storage_enabled) { true }
 
   before do
+    stub_licensed_features(cross_project_pipelines: cross_project_pipeline_enabled,
+                           object_storage: object_storage_enabled)
     project.add_developer(user)
   end
 
@@ -319,7 +323,7 @@ describe API::Jobs do
     let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) }
 
     before do
-      stub_artifacts_object_storage
+      stub_artifacts_object_storage(licensed: :skip)
       job.success
     end
 
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 27d09b8202e..00f45e5f702 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -190,10 +190,12 @@ describe 'Git LFS API and storage' do
   describe 'when fetching lfs object' do
     let(:project) { create(:project) }
     let(:update_permissions) { }
+    let(:before_get) { }
 
     before do
       enable_lfs
       update_permissions
+      before_get
       get "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}", nil, headers
     end
 
@@ -238,6 +240,21 @@ describe 'Git LFS API and storage' do
             end
 
             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::REMOTE_STORE)
+              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
 
@@ -944,6 +961,46 @@ describe 'Git LFS API and storage' do
             end
           end
 
+          context 'and workhorse requests upload finalize for a new lfs object' do
+            before do
+              allow_any_instance_of(LfsObjectUploader).to receive(:exists?) { false }
+            end
+
+            context 'with object storage disabled' do
+              it "doesn't attempt to migrate file to object storage" do
+                expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+                put_finalize(with_tempfile: true)
+              end
+            end
+
+            context 'with object storage enabled' do
+              before do
+                stub_lfs_object_storage
+              end
+
+              it 'schedules migration of file to object storage' do
+                expect(ObjectStorageUploadWorker).to receive(:perform_async).with('LfsObjectUploader', 'LfsObject', :file, kind_of(Numeric))
+
+                put_finalize(with_tempfile: true)
+              end
+            end
+          end
+
+          context 'and project has limit enabled but will stay under the limit' do
+            before do
+              allow_any_instance_of(EE::Project).to receive_messages(
+                actual_size_limit: 200,
+                size_limit_enabled?: true)
+
+              put_finalize
+            end
+
+            it 'responds with status 200' do
+              expect(response).to have_gitlab_http_status(200)
+            end
+          end
+
           context 'invalid tempfiles' do
             it 'rejects slashes in the tempfile name (path traversal' do
               put_finalize('foo/bar')
@@ -1143,7 +1200,9 @@ describe 'Git LFS API and storage' do
       put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", nil, authorize_headers
     end
 
-    def put_finalize(lfs_tmp = lfs_tmp_file)
+    def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false)
+      setup_tempfile(lfs_tmp) if with_tempfile
+
       put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", nil,
           headers.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp).compact
     end
@@ -1151,6 +1210,13 @@ describe 'Git LFS API and storage' do
     def lfs_tmp_file
       "#{sample_oid}012345678"
     end
+
+    def setup_tempfile(lfs_tmp)
+      upload_path = "#{Gitlab.config.lfs.storage_path}/tmp/upload"
+
+      FileUtils.mkdir_p(upload_path)
+      FileUtils.touch(File.join(upload_path, lfs_tmp))
+    end
   end
 
   def enable_lfs
diff --git a/spec/support/stub_artifacts.rb b/spec/support/stub_artifacts.rb
deleted file mode 100644
index d531be5b8e7..00000000000
--- a/spec/support/stub_artifacts.rb
+++ /dev/null
@@ -1,26 +0,0 @@
-module StubConfiguration
-  def stub_artifacts_object_storage(enabled: true)
-    Fog.mock!
-    allow(Gitlab.config.artifacts.object_store).to receive_messages(
-      enabled: enabled,
-      remote_directory: 'artifacts',
-      connection: {
-        provider: 'AWS',
-        aws_access_key_id: 'AWS_ACCESS_KEY_ID',
-        aws_secret_access_key: 'AWS_SECRET_ACCESS_KEY',
-        region: 'eu-central-1'
-      }
-    )
-
-    allow_any_instance_of(ArtifactUploader).to receive(:verify_license!) { true }
-
-    return unless enabled
-
-    ::Fog::Storage.new(Gitlab.config.artifacts.object_store.connection).tap do |connection|
-      begin
-        connection.directories.create(key: 'artifacts')
-      rescue Excon::Error::Conflict
-      end
-    end
-  end
-end
diff --git a/spec/support/stub_object_storage.rb b/spec/support/stub_object_storage.rb
new file mode 100644
index 00000000000..df7e05585d2
--- /dev/null
+++ b/spec/support/stub_object_storage.rb
@@ -0,0 +1,32 @@
+module StubConfiguration
+  def stub_object_storage_uploader(config:, uploader:, remote_directory:, enabled: true, licensed: true)
+    Fog.mock!
+
+    allow(config).to receive(:enabled) { enabled }
+
+    stub_licensed_features(object_storage: licensed) unless licensed == :skip
+
+    return unless enabled
+
+    ::Fog::Storage.new(uploader.object_store_credentials).tap do |connection|
+      begin
+        connection.directories.create(key: remote_directory)
+      rescue Excon::Error::Conflict
+      end
+    end
+  end
+
+  def stub_artifacts_object_storage(**params)
+    stub_object_storage_uploader(config: Gitlab.config.artifacts.object_store,
+                                 uploader: ArtifactUploader,
+                                 remote_directory: 'artifacts',
+                                 **params)
+  end
+
+  def stub_lfs_object_storage(**params)
+    stub_object_storage_uploader(config: Gitlab.config.lfs.object_store,
+                                 uploader: LfsObjectUploader,
+                                 remote_directory: 'lfs-objects',
+                                 **params)
+  end
+end
diff --git a/spec/tasks/gitlab/lfs_rake_spec.rb b/spec/tasks/gitlab/lfs_rake_spec.rb
new file mode 100644
index 00000000000..faed24f2010
--- /dev/null
+++ b/spec/tasks/gitlab/lfs_rake_spec.rb
@@ -0,0 +1,37 @@
+require 'rake_helper'
+
+describe 'gitlab:lfs namespace rake task' do
+  before :all do
+    Rake.application.rake_require 'tasks/gitlab/lfs'
+  end
+
+  describe 'migrate' do
+    let(:local) { ObjectStoreUploader::LOCAL_STORE }
+    let(:remote) { ObjectStoreUploader::REMOTE_STORE }
+    let!(:lfs_object) { create(:lfs_object, :with_file, file_store: local) }
+
+    def lfs_migrate
+      run_rake_task('gitlab:lfs:migrate')
+    end
+
+    context 'object storage disabled' do
+      before do
+        stub_lfs_object_storage(enabled: false)
+      end
+
+      it "doesn't migrate files" do
+        expect { lfs_migrate }.not_to change { lfs_object.reload.file_store }
+      end
+    end
+
+    context 'object storage enabled' do
+      before do
+        stub_lfs_object_storage
+      end
+
+      it 'migrates local file to object storage' do
+        expect { lfs_migrate }.to change { lfs_object.reload.file_store }.from(local).to(remote)
+      end
+    end
+  end
+end
diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb
index f4ba4a8207f..88f394b2938 100644
--- a/spec/uploaders/artifact_uploader_spec.rb
+++ b/spec/uploaders/artifact_uploader_spec.rb
@@ -6,8 +6,8 @@ describe ArtifactUploader do
   let(:uploader) { described_class.new(job, :artifacts_file) }
   let(:local_path) { Gitlab.config.artifacts.path }
 
-  describe '.local_artifacts_store' do
-    subject { described_class.local_artifacts_store }
+  describe '.local_store_path' do
+    subject { described_class.local_store_path }
 
     it "delegate to artifacts path" do
       expect(Gitlab.config.artifacts).to receive(:path)
diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb
index 7088bc23334..1e09958d369 100644
--- a/spec/uploaders/lfs_object_uploader_spec.rb
+++ b/spec/uploaders/lfs_object_uploader_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe LfsObjectUploader do
   let(:lfs_object) { create(:lfs_object, :with_file) }
-  let(:uploader) { described_class.new(lfs_object) }
+  let(:uploader) { described_class.new(lfs_object, :file) }
   let(:path) { Gitlab.config.lfs.storage_path }
 
   describe '#move_to_cache' do
@@ -37,4 +37,73 @@ describe LfsObjectUploader do
     it { is_expected.to start_with(path) }
     it { is_expected.to end_with('/tmp/work') }
   end
+
+  describe 'migration to object storage' do
+    context 'with object storage disabled' do
+      it "is skipped" do
+        expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+        lfs_object
+      end
+    end
+
+    context 'with object storage enabled' do
+      before do
+        stub_lfs_object_storage
+      end
+
+      it 'is scheduled to run after creation' do
+        expect(ObjectStorageUploadWorker).to receive(:perform_async).with(described_class.name, 'LfsObject', :file, kind_of(Numeric))
+
+        lfs_object
+      end
+    end
+
+    context 'with object storage unlicenced' do
+      before do
+        stub_lfs_object_storage(licensed: false)
+      end
+
+      it 'is skipped' do
+        expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+        lfs_object
+      end
+    end
+  end
+
+  describe 'remote file' do
+    let(:remote) { described_class::REMOTE_STORE }
+    let(:lfs_object) { create(:lfs_object, file_store: remote) }
+
+    context 'with object storage enabled' do
+      before do
+        stub_lfs_object_storage
+      end
+
+      it 'can store file remotely' do
+        allow(ObjectStorageUploadWorker).to receive(:perform_async)
+
+        store_file(lfs_object)
+
+        expect(lfs_object.file_store).to eq remote
+        expect(lfs_object.file.path).not_to be_blank
+      end
+    end
+
+    context 'with object storage unlicenced' do
+      before do
+        stub_lfs_object_storage(licensed: false)
+      end
+
+      it 'can not store file remotely' do
+        expect { store_file(lfs_object) }.to raise_error('Object Storage feature is missing')
+      end
+    end
+  end
+
+  def store_file(lfs_object)
+    lfs_object.file = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png")
+    lfs_object.save!
+  end
 end
diff --git a/spec/uploaders/object_store_uploader_spec.rb b/spec/uploaders/object_store_uploader_spec.rb
index c6c7d47e703..c5554502980 100644
--- a/spec/uploaders/object_store_uploader_spec.rb
+++ b/spec/uploaders/object_store_uploader_spec.rb
@@ -198,18 +198,15 @@ describe ObjectStoreUploader do
     end
 
     context 'when using remote storage' do
-      let(:project) { double }
-
       before do
         uploader_class.storage_options double(
           object_store: double(enabled: true))
         expect(object).to receive(:artifacts_file_store) { described_class::REMOTE_STORE }
-        expect(object).to receive(:project) { project }
       end
 
       context 'feature is not available' do
         before do
-          expect(project).to receive(:feature_available?).with(:object_storage) { false }
+          expect(License).to receive(:feature_available?).with(:object_storage) { false }
         end
 
         it "does raise an error" do
@@ -219,7 +216,7 @@ describe ObjectStoreUploader do
 
       context 'feature is available' do
         before do
-          expect(project).to receive(:feature_available?).with(:object_storage) { true }
+          expect(License).to receive(:feature_available?).with(:object_storage) { true }
         end
 
         it "does not raise an error" do
diff --git a/spec/workers/object_storage_upload_worker_spec.rb b/spec/workers/object_storage_upload_worker_spec.rb
new file mode 100644
index 00000000000..8a8f7a065a0
--- /dev/null
+++ b/spec/workers/object_storage_upload_worker_spec.rb
@@ -0,0 +1,85 @@
+require 'spec_helper'
+
+describe ObjectStorageUploadWorker do
+  let(:local) { ObjectStoreUploader::LOCAL_STORE }
+  let(:remote) { ObjectStoreUploader::REMOTE_STORE }
+
+  def perform
+    described_class.perform_async(uploader_class.name, subject_class, file_field, subject_id)
+  end
+
+  context 'for LFS' do
+    let!(:lfs_object) { create(:lfs_object, :with_file, file_store: local) }
+    let(:uploader_class) { LfsObjectUploader }
+    let(:subject_class) { LfsObject }
+    let(:file_field) { :file }
+    let(:subject_id) { lfs_object.id }
+
+    context 'when object storage is enabled' do
+      before do
+        stub_lfs_object_storage
+      end
+
+      it 'uploads object to storage' do
+        expect { perform }.to change { lfs_object.reload.file_store }.from(local).to(remote)
+      end
+
+      context 'when background upload is disabled' do
+        before do
+          allow(Gitlab.config.lfs.object_store).to receive(:background_upload) { false }
+        end
+
+        it 'is skipped' do
+          expect { perform }.not_to change { lfs_object.reload.file_store }
+        end
+      end
+    end
+
+    context 'when object storage is disabled' do
+      before do
+        stub_lfs_object_storage(enabled: false)
+      end
+
+      it "doesn't migrate files" do
+        perform
+
+        expect(lfs_object.reload.file_store).to eq(local)
+      end
+    end
+  end
+
+  context 'for artifacts' do
+    let(:job) { create(:ci_build, :artifacts, artifacts_file_store: store, artifacts_metadata_store: store) }
+    let(:uploader_class) { ArtifactUploader }
+    let(:subject_class) { Ci::Build }
+    let(:file_field) { :artifacts_file }
+    let(:subject_id) { job.id }
+
+    context 'when local storage is used' do
+      let(:store) { local }
+
+      context 'and remote storage is defined' do
+        before do
+          stub_artifacts_object_storage
+          job
+        end
+
+        it "migrates file to remote storage" do
+          perform
+
+          expect(job.reload.artifacts_file_store).to eq(remote)
+        end
+
+        context 'for artifacts_metadata' do
+          let(:file_field) { :artifacts_metadata }
+
+          it 'migrates metadata to remote storage' do
+            perform
+
+            expect(job.reload.artifacts_metadata_store).to eq(remote)
+          end
+        end
+      end
+    end
+  end
+end