Merge branch '62183-update-response-code-for-bulk-delete-api-for-container-registry' into 'master'

Update response code for container registry api bulk delete

Closes #62183

See merge request gitlab-org/gitlab-ce!29448

app/workers/cleanup_container_repository_worker.rb

@@ -2,12 +2,9 @@
 
 class CleanupContainerRepositoryWorker
   include ApplicationWorker
-  include ExclusiveLeaseGuard
 
   queue_namespace :container_repository
 
-  LEASE_TIMEOUT = 1.hour
-
   attr_reader :container_repository, :current_user
 
   def perform(current_user_id, container_repository_id, params)
@@ -16,12 +13,10 @@ class CleanupContainerRepositoryWorker
 
     return unless valid?
 
-    try_obtain_lease do
     Projects::ContainerRepository::CleanupTagsService
       .new(project, current_user, params)
       .execute(container_repository)
   end
-  end
 
   private
 
@@ -32,22 +27,4 @@ class CleanupContainerRepositoryWorker
   def project
     container_repository&.project
   end
-
-  # For ExclusiveLeaseGuard concern
-  def lease_key
-    @lease_key ||= "container_repository:cleanup_tags:#{container_repository.id}"
-  end
-
-  # For ExclusiveLeaseGuard concern
-  def lease_timeout
-    LEASE_TIMEOUT
-  end
-
-  # For ExclusiveLeaseGuard concern
-  def lease_release?
-    # we don't allow to execute this worker
-    # more often than LEASE_TIMEOUT
-    # for given container repository
-    false
-  end
 end

changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml

+---
+title: Return 400 when deleting tags more often than once per hour.
+merge_request: 29448
+author:
+type: changed

lib/api/container_registry.rb

@@ -68,6 +68,9 @@ module API
       delete ':id/registry/repositories/:repository_id/tags', requirements: REGISTRY_ENDPOINT_REQUIREMENTS do
         authorize_admin_container_image!
 
+        message = 'This request has already been made. You can run this at most once an hour for a given container repository'
+        render_api_error!(message, 400) unless obtain_new_cleanup_container_lease
+
         CleanupContainerRepositoryWorker.perform_async(current_user.id, repository.id,
           declared_params.except(:repository_id)) # rubocop: disable CodeReuse/ActiveRecord
 
@@ -123,6 +126,13 @@ module API
         authorize! :admin_container_image, repository
       end
 
+      def obtain_new_cleanup_container_lease
+        Gitlab::ExclusiveLease
+          .new("container_repository:cleanup_tags:#{repository.id}",
+               timeout: 1.hour)
+          .try_obtain
+      end
+
       def repository
         @repository ||= user_project.container_repositories.find(params[:repository_id])
       end

spec/requests/api/container_registry_spec.rb

 require 'spec_helper'
 
 describe API::ContainerRegistry do
+  include ExclusiveLeaseHelpers
+
   set(:project) { create(:project, :private) }
   set(:maintainer) { create(:user) }
   set(:developer) { create(:user) }
@@ -155,7 +157,10 @@ describe API::ContainerRegistry do
             older_than: '1 day' }
         end
 
+        let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" }
+
         it 'schedules cleanup of tags repository' do
+          stub_exclusive_lease(lease_key, timeout: 1.hour)
           expect(CleanupContainerRepositoryWorker).to receive(:perform_async)
             .with(maintainer.id, root_repository.id, worker_params)
 
@@ -163,6 +168,22 @@ describe API::ContainerRegistry do
 
           expect(response).to have_gitlab_http_status(:accepted)
         end
+
+        context 'called multiple times in one hour' do
+          it 'returns 400 with an error message' do
+            stub_exclusive_lease_taken(lease_key, timeout: 1.hour)
+            subject
+
+            expect(response).to have_gitlab_http_status(400)
+            expect(response.body).to include('This request has already been made.')
+          end
+
+          it 'executes service only for the first time' do
+            expect(CleanupContainerRepositoryWorker).to receive(:perform_async).once
+
+            2.times { subject }
+          end
+        end
       end
     end
   end

spec/workers/cleanup_container_repository_worker_spec.rb

@@ -35,13 +35,5 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do
         subject.perform(user.id, -1, params)
       end.not_to raise_error
     end
-
-    context 'when executed twice in short period' do
-      it 'executes service only for the first time' do
-        expect(service).to receive(:execute).once
-
-        2.times { subject.perform(user.id, repository.id, params) }
-      end
-    end
   end
 end