Add `direct_upload` setting for artifacts

3 files changed, 31 insertions, 33 deletions

diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb
index ef0f8acefd6..dd86753479d 100644
--- a/app/uploaders/job_artifact_uploader.rb
+++ b/app/uploaders/job_artifact_uploader.rb
@@ -2,6 +2,8 @@ class JobArtifactUploader < GitlabUploader
   extend Workhorse::UploadPath
   include ObjectStorage::Concern
 
+  ObjectNotReadyError = Class.new(StandardError)
+
   storage_options Gitlab.config.artifacts
 
   def size
@@ -25,6 +27,8 @@ class JobArtifactUploader < GitlabUploader
   private
 
   def dynamic_segment
+    raise ObjectNotReadyError, 'JobArtifact is not ready' unless model.id
+
     creation_date = model.created_at.utc.strftime('%Y_%m_%d')
 
     File.join(disk_hash[0..1], disk_hash[2..3], disk_hash,
diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb
index b726b053493..efb7893d153 100644
--- a/app/uploaders/legacy_artifact_uploader.rb
+++ b/app/uploaders/legacy_artifact_uploader.rb
@@ -2,6 +2,8 @@ class LegacyArtifactUploader < GitlabUploader
   extend Workhorse::UploadPath
   include ObjectStorage::Concern
 
+  ObjectNotReadyError = Class.new(StandardError)
+
   storage_options Gitlab.config.artifacts
 
   def store_dir
@@ -11,6 +13,8 @@ class LegacyArtifactUploader < GitlabUploader
   private
 
   def dynamic_segment
+    raise ObjectNotReadyError, 'Build is not ready' unless model.id
+
     File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s)
   end
 end
diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb
index 3f59ee39299..bd258e04d3f 100644
--- a/app/uploaders/object_storage.rb
+++ b/app/uploaders/object_storage.rb
@@ -156,11 +156,10 @@ module ObjectStorage
       end
 
       def workhorse_authorize
-        if options = workhorse_remote_upload_options
-          { RemoteObject: options }
-        else
-          { TempPath: workhorse_local_upload_path }
-        end
+        {
+          RemoteObject: workhorse_remote_upload_options,
+          TempPath: workhorse_local_upload_path
+        }.compact
       end
 
       def workhorse_local_upload_path
@@ -293,16 +292,14 @@ module ObjectStorage
       }
     end
 
-    def store_workhorse_file!(params, identifier)
-      filename = params["#{identifier}.name"]
-
-      if remote_object_id = params["#{identifier}.remote_id"]
-        store_remote_file!(remote_object_id, filename)
-      elsif local_path = params["#{identifier}.path"]
-        store_local_file!(local_path, filename)
-      else
-        raise RemoteStoreError, 'Bad file'
+    def cache!(new_file = sanitized_file)
+      # We intercept ::UploadedFile which might be stored on remote storage
+      # We use that for "accelerated" uploads, where we store result on remote storage
+      if new_file.is_a?(::UploadedFile) && new_file.remote_id
+        return cache_remote_file!(new_file.remote_id, new_file.original_filename)
       end
+
+      super
     end
 
     private
@@ -313,36 +310,29 @@ module ObjectStorage
         self.file_storage?
     end
 
-    def store_remote_file!(remote_object_id, filename)
-      raise RemoteStoreError, 'Missing filename' unless filename
-
+    def cache_remote_file!(remote_object_id, original_filename)
       file_path = File.join(TMP_UPLOAD_PATH, remote_object_id)
       file_path = Pathname.new(file_path).cleanpath.to_s
       raise RemoteStoreError, 'Bad file path' unless file_path.start_with?(TMP_UPLOAD_PATH + '/')
 
-      self.object_store = Store::REMOTE
-
       # TODO:
       # This should be changed to make use of `tmp/cache` mechanism
       # instead of using custom upload directory,
       # using tmp/cache makes this implementation way easier than it is today
-      CarrierWave::Storage::Fog::File.new(self, storage, file_path).tap do |file|
+      CarrierWave::Storage::Fog::File.new(self, storage_for(Store::REMOTE), file_path).tap do |file|
         raise RemoteStoreError, 'Missing file' unless file.exists?
 
-        self.filename = filename
-        self.file = storage.store!(file)
-      end
-    end
-
-    def store_local_file!(local_path, filename)
-      raise RemoteStoreError, 'Missing filename' unless filename
-
-      root_path = File.realpath(self.class.workhorse_local_upload_path)
-      file_path = File.realpath(local_path)
-      raise RemoteStoreError, 'Bad file path' unless file_path.start_with?(root_path)
+        # Remote stored file, we force to store on remote storage
+        self.object_store = Store::REMOTE
 
-      self.object_store = Store::LOCAL
-      self.store!(UploadedFile.new(file_path, filename))
+        # TODO:
+        # We store file internally and force it to be considered as `cached`
+        # This makes CarrierWave to store file in permament location (copy/delete)
+        # once this object is saved, but not sooner
+        @cache_id = "force-to-use-cache" # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        @file = file # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        @filename = original_filename # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      end
     end
 
     # this is a hack around CarrierWave. The #migrate method needs to be