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

5 files changed, 39 insertions, 34 deletions

diff --git a/app/workers/cleanup_container_repository_worker.rb b/app/workers/cleanup_container_repository_worker.rb
index 974ee8c8146..0331fc7b01c 100644
--- a/app/workers/cleanup_container_repository_worker.rb
+++ b/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,11 +13,9 @@ class CleanupContainerRepositoryWorker
 
     return unless valid?
 
-    try_obtain_lease do
-      Projects::ContainerRepository::CleanupTagsService
-        .new(project, current_user, params)
-        .execute(container_repository)
-    end
+    Projects::ContainerRepository::CleanupTagsService
+      .new(project, current_user, params)
+      .execute(container_repository)
   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
diff --git a/changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml b/changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml
new file mode 100644
index 00000000000..ce75a55efdc
--- /dev/null
+++ b/changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml
@@ -0,0 +1,5 @@
+---
+title: Return 400 when deleting tags more often than once per hour.
+merge_request: 29448
+author:
+type: changed
diff --git a/lib/api/container_registry.rb b/lib/api/container_registry.rb
index 7d9b5e1a598..7dad20a822a 100644
--- a/lib/api/container_registry.rb
+++ b/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
diff --git a/spec/requests/api/container_registry_spec.rb b/spec/requests/api/container_registry_spec.rb
index 4ad15ed6bea..b64f3ea1081 100644
--- a/spec/requests/api/container_registry_spec.rb
+++ b/spec/requests/api/container_registry_spec.rb
@@ -1,6 +1,8 @@
 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
diff --git a/spec/workers/cleanup_container_repository_worker_spec.rb b/spec/workers/cleanup_container_repository_worker_spec.rb
index 5bee7294010..9be8f882785 100644
--- a/spec/workers/cleanup_container_repository_worker_spec.rb
+++ b/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