Merge branch '212373-fix-triggering-multiple-pipeline-with-same-artifact' into 'master'

Fix triggering multiple children pipeline with the same artifact

See merge request gitlab-org/gitlab!40268

app/uploaders/object_storage.rb

@@ -210,6 +210,20 @@ module ObjectStorage
       end
     end
 
+    class OpenFile
+      extend Forwardable
+
+      # Explicitly exclude :path, because rubyzip uses that to detect "real" files.
+      def_delegators :@file, *(Zip::File::IO_METHODS - [:path])
+
+      # Even though :size is not in IO_METHODS, we do need it.
+      def_delegators :@file, :size
+
+      def initialize(file)
+        @file = file
+      end
+    end
+
     # allow to configure and overwrite the filename
     def filename
       @filename || super || file&.filename # rubocop:disable Gitlab/ModuleWithInstanceVariables
@@ -259,6 +273,24 @@ module ObjectStorage
       end
     end
 
+    def use_open_file(&blk)
+      Tempfile.open(path) do |file|
+        file.unlink
+        file.binmode
+
+        if file_storage?
+          IO.copy_stream(path, file)
+        else
+          streamer = lambda { |chunk, _, _| file.write(chunk) }
+          Excon.get(url, response_block: streamer)
+        end
+
+        file.seek(0, IO::SEEK_SET)
+
+        yield OpenFile.new(file)
+      end
+    end
+
     #
     # Move the file to another store
     #

config/feature_flags/development/ci_new_artifact_file_reader.yml

+---
+name: ci_new_artifact_file_reader
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40268
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/249588
+group: group::pipeline authoring
+type: development
+default_enabled: false

lib/gitlab/ci/artifact_file_reader.rb

@@ -45,6 +45,31 @@ module Gitlab
       end
 
       def read_zip_file!(file_path)
+        if ::Gitlab::Ci::Features.new_artifact_file_reader_enabled?(job.project)
+          read_with_new_artifact_file_reader(file_path)
+        else
+          read_with_legacy_artifact_file_reader(file_path)
+        end
+      end
+
+      def read_with_new_artifact_file_reader(file_path)
+        job.artifacts_file.use_open_file do |file|
+          zip_file = Zip::File.new(file, false, true)
+          entry = zip_file.find_entry(file_path)
+
+          unless entry
+            raise Error, "Path `#{file_path}` does not exist inside the `#{job.name}` artifacts archive!"
+          end
+
+          if entry.name_is_directory?
+            raise Error, "Path `#{file_path}` was expected to be a file but it was a directory!"
+          end
+
+          zip_file.read(entry)
+        end
+      end
+
+      def read_with_legacy_artifact_file_reader(file_path)
         job.artifacts_file.use_file do |archive_path|
           Zip::File.open(archive_path) do |zip_file|
             entry = zip_file.find_entry(file_path)

lib/gitlab/ci/features.rb

@@ -78,6 +78,10 @@ module Gitlab
         ::Feature.enabled?(:ci_enable_live_trace, project) &&
           ::Feature.enabled?(:ci_accept_trace, project, type: :ops, default_enabled: false)
       end
+
+      def self.new_artifact_file_reader_enabled?(project)
+        ::Feature.enabled?(:ci_new_artifact_file_reader, project, default_enabled: false)
+      end
     end
   end
 end

spec/lib/gitlab/ci/artifact_file_reader_spec.rb

@@ -18,6 +18,17 @@ RSpec.describe Gitlab::Ci::ArtifactFileReader do
         expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom')
       end
 
+      context 'when FF ci_new_artifact_file_reader is disabled' do
+        before do
+          stub_feature_flags(ci_new_artifact_file_reader: false)
+        end
+
+        it 'returns the content at the path' do
+          is_expected.to be_present
+          expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom')
+        end
+      end
+
       context 'when path does not exist' do
         let(:path) { 'file/does/not/exist.txt' }
         let(:expected_error) do

spec/uploaders/object_storage_spec.rb

@@ -210,6 +210,27 @@ RSpec.describe ObjectStorage do
         end
       end
 
+      describe '#use_open_file' do
+        context 'when file is stored locally' do
+          it "returns the file" do
+            expect { |b| uploader.use_open_file(&b) }.to yield_with_args(an_instance_of(ObjectStorage::Concern::OpenFile))
+          end
+        end
+
+        context 'when file is stored remotely' do
+          let(:store) { described_class::Store::REMOTE }
+
+          before do
+            stub_artifacts_object_storage
+            stub_request(:get, %r{s3.amazonaws.com/#{uploader.path}}).to_return(status: 200, body: '')
+          end
+
+          it "returns the file" do
+            expect { |b| uploader.use_open_file(&b) }.to yield_with_args(an_instance_of(ObjectStorage::Concern::OpenFile))
+          end
+        end
+      end
+
       describe '#migrate!' do
         subject { uploader.migrate!(new_store) }
 
@@ -844,4 +865,19 @@ RSpec.describe ObjectStorage do
       end
     end
   end
+
+  describe 'OpenFile' do
+    subject { ObjectStorage::Concern::OpenFile.new(file) }
+
+    let(:file) { double(read: true, size: true, path: true) }
+
+    it 'delegates read and size methods' do
+      expect(subject.read).to eq(true)
+      expect(subject.size).to eq(true)
+    end
+
+    it 'does not delegate path method' do
+      expect { subject.path }.to raise_error(NoMethodError)
+    end
+  end
 end