Merge branch '429060-skip-copy-operation-in-generic-packages-upload' into 'master'

Avoid copy operation in object store during Generic Packages upload

See merge request !147454



Merged-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Subashis Chakraborty <schakraborty@gitlab.com>
Approved-by: Mayra Cabrera <mcabrera@gitlab.com>
Approved-by: Panos Kanellidis <pkanellidis@gitlab.com>
Approved-by: Heinrich Lee Yu <heinrich@gitlab.com>
Co-authored-by: moaz-khalifa <mkhalifa@gitlab.com>

config/feature_flags/gitlab_com_derisk/skip_copy_operation_in_generic_packages_upload.yml

+---
+name: skip_copy_operation_in_generic_packages_upload
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429060
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147454
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/450992
+milestone: '16.11'
+group: group::package registry
+type: gitlab_com_derisk
+default_enabled: false

db/migrate/20240320110318_add_file_final_path_to_packages_package_files.rb

+# frozen_string_literal: true
+
+class AddFileFinalPathToPackagesPackageFiles < Gitlab::Database::Migration[2.2]
+  disable_ddl_transaction!
+  milestone '16.11'
+
+  def up
+    with_lock_retries do
+      add_column :packages_package_files, :file_final_path, :text, if_not_exists: true
+    end
+
+    add_text_limit :packages_package_files, :file_final_path, 1024
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :packages_package_files, :file_final_path, if_exists: true
+    end
+  end
+end

db/schema_migrations/20240320110318

+3fdf05335113f176717a30d28a2e60c55b677e4518d1445706827750b7d85c0e
\ No newline at end of file

db/structure.sql

@@ -12887,6 +12887,8 @@ CREATE TABLE packages_package_files (
     verification_state smallint DEFAULT 0 NOT NULL,
     verification_started_at timestamp with time zone,
     status smallint DEFAULT 0 NOT NULL,
+    file_final_path text,
+    CONSTRAINT check_0f29938b18 CHECK ((char_length(file_final_path) <= 1024)),
     CONSTRAINT check_4c5e6bb0b3 CHECK ((file_store IS NOT NULL))
 );

lib/api/generic_packages.rb

@@ -47,9 +47,7 @@ class GenericPackages < ::API::Base
           end
 
           put 'authorize' do
-            project = authorized_user_project
-
-            authorize_workhorse!(subject: project, maximum_size: project.actual_limits.generic_packages_max_file_size)
+            authorize_workhorse!(**authorize_workhorse_params)
           end
 
           desc 'Upload package file' do
@@ -146,6 +144,21 @@ class GenericPackages < ::API::Base
       def max_file_size_exceeded?
         authorized_user_project.actual_limits.exceeded?(:generic_packages_max_file_size, params[:file].size)
       end
+
+      def authorize_workhorse_params
+        project = authorized_user_project
+
+        params = {
+          subject: project,
+          maximum_size: project.actual_limits.generic_packages_max_file_size
+        }
+
+        if ::Feature.enabled?(:skip_copy_operation_in_generic_packages_upload, project)
+          params[:use_final_store_path] = true
+        end
+
+        params
+      end
     end
   end
 end

lib/api/helpers/packages_helpers.rb

@@ -40,7 +40,12 @@ def authorize_packages_access!(subject = user_project, required_permission = :re
         end
       end
 
-      def authorize_workhorse!(subject: user_project, has_length: true, maximum_size: MAX_PACKAGE_FILE_SIZE)
+      def authorize_workhorse!(
+        subject: user_project,
+        has_length: true,
+        maximum_size: MAX_PACKAGE_FILE_SIZE,
+        use_final_store_path: false)
+
         authorize_upload!(subject)
 
         Gitlab::Workhorse.verify_api_request!(headers)
@@ -48,8 +53,9 @@ def authorize_workhorse!(subject: user_project, has_length: true, maximum_size:
         status 200
         content_type Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
 
-        params = { has_length: has_length }
+        params = { has_length: has_length, use_final_store_path: use_final_store_path }
         params[:maximum_size] = maximum_size unless has_length
+        params[:final_store_path_root_id] = subject.id if use_final_store_path
         ::Packages::PackageFileUploader.workhorse_authorize(**params)
       end
 

spec/lib/api/helpers/packages_helpers_spec.rb

@@ -104,34 +104,45 @@
 
   describe '#authorize_workhorse!' do
     let_it_be(:headers) { {} }
+    let_it_be(:params) { { subject: project } }
 
-    subject { helper.authorize_workhorse!(subject: project) }
+    subject { helper.authorize_workhorse!(**params) }
+
+    shared_examples 'workhorse authorize' do
+      it 'authorizes workhorse' do
+        expect(helper).to receive(:authorize_upload!).with(project)
+        expect(helper).to receive(:status).with(200)
+        expect(helper).to receive(:content_type).with(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+        expect(Gitlab::Workhorse).to receive(:verify_api_request!).with(headers)
+        expect(::Packages::PackageFileUploader).to receive(:workhorse_authorize).with(workhorse_authorize_params)
+
+        expect(subject).to eq nil
+      end
+    end
 
     before do
       allow(helper).to receive(:headers).and_return(headers)
     end
 
-    it 'authorizes workhorse' do
-      expect(helper).to receive(:authorize_upload!).with(project)
-      expect(helper).to receive(:status).with(200)
-      expect(helper).to receive(:content_type).with(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
-      expect(Gitlab::Workhorse).to receive(:verify_api_request!).with(headers)
-      expect(::Packages::PackageFileUploader).to receive(:workhorse_authorize).with(has_length: true)
-
-      expect(subject).to eq nil
+    it_behaves_like 'workhorse authorize' do
+      let(:workhorse_authorize_params) { { has_length: true, use_final_store_path: false } }
     end
 
     context 'without length' do
-      subject { helper.authorize_workhorse!(subject: project, has_length: false) }
+      let(:params) { super().merge(has_length: false) }
 
-      it 'authorizes workhorse' do
-        expect(helper).to receive(:authorize_upload!).with(project)
-        expect(helper).to receive(:status).with(200)
-        expect(helper).to receive(:content_type).with(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
-        expect(Gitlab::Workhorse).to receive(:verify_api_request!).with(headers)
-        expect(::Packages::PackageFileUploader).to receive(:workhorse_authorize).with(has_length: false, maximum_size: ::API::Helpers::PackagesHelpers::MAX_PACKAGE_FILE_SIZE)
+      it_behaves_like 'workhorse authorize' do
+        let(:workhorse_authorize_params) do
+          { has_length: false, maximum_size: ::API::Helpers::PackagesHelpers::MAX_PACKAGE_FILE_SIZE, use_final_store_path: false }
+        end
+      end
+    end
 
-        expect(subject).to eq nil
+    context 'when use_final_store_path is true' do
+      let(:params) { super().merge(use_final_store_path: true) }
+
+      it_behaves_like 'workhorse authorize' do
+        let(:workhorse_authorize_params) { { has_length: true, use_final_store_path: true, final_store_path_root_id: project.id } }
       end
     end
   end

spec/requests/api/generic_packages_spec.rb

@@ -170,6 +170,34 @@ def deploy_token_header(value)
       end
     end
 
+    context 'for use_final_store_path' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'sends use_final_store_path with true' do
+        expect(::Packages::PackageFileUploader).to receive(:workhorse_authorize).with(
+          hash_including(use_final_store_path: true, final_store_path_root_id: project.id)
+        ).and_call_original
+
+        authorize_upload_file(workhorse_headers.merge(personal_access_token_header))
+      end
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_feature_flags(skip_copy_operation_in_generic_packages_upload: false)
+        end
+
+        it 'sends use_final_store_path with false' do
+          expect(::Packages::PackageFileUploader).to receive(:workhorse_authorize).with(
+            hash_including(use_final_store_path: false)
+          ).and_call_original
+
+          authorize_upload_file(workhorse_headers.merge(personal_access_token_header))
+        end
+      end
+    end
+
     def authorize_upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz')
       url = "/projects/#{project.id}/packages/generic/#{package_name}/0.0.1/#{file_name}/authorize"