Commit 34a283f9 authored by Steve Abrams's avatar Steve Abrams Committed by Mayra Cabrera

Add 2nd response for container api bulk delete

The bulk delete api endpoint for container registries can
only be called once per hour. If a user calls the endpoint more
than once per hour, they will now receive a 400 error with a
descriptive message.
parent 6fa90054
...@@ -2,12 +2,9 @@ ...@@ -2,12 +2,9 @@
class CleanupContainerRepositoryWorker class CleanupContainerRepositoryWorker
include ApplicationWorker include ApplicationWorker
include ExclusiveLeaseGuard
queue_namespace :container_repository queue_namespace :container_repository
LEASE_TIMEOUT = 1.hour
attr_reader :container_repository, :current_user attr_reader :container_repository, :current_user
def perform(current_user_id, container_repository_id, params) def perform(current_user_id, container_repository_id, params)
...@@ -16,11 +13,9 @@ class CleanupContainerRepositoryWorker ...@@ -16,11 +13,9 @@ class CleanupContainerRepositoryWorker
return unless valid? return unless valid?
try_obtain_lease do Projects::ContainerRepository::CleanupTagsService
Projects::ContainerRepository::CleanupTagsService .new(project, current_user, params)
.new(project, current_user, params) .execute(container_repository)
.execute(container_repository)
end
end end
private private
...@@ -32,22 +27,4 @@ class CleanupContainerRepositoryWorker ...@@ -32,22 +27,4 @@ class CleanupContainerRepositoryWorker
def project def project
container_repository&.project container_repository&.project
end 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 end
---
title: Return 400 when deleting tags more often than once per hour.
merge_request: 29448
author:
type: changed
...@@ -68,6 +68,9 @@ module API ...@@ -68,6 +68,9 @@ module API
delete ':id/registry/repositories/:repository_id/tags', requirements: REGISTRY_ENDPOINT_REQUIREMENTS do delete ':id/registry/repositories/:repository_id/tags', requirements: REGISTRY_ENDPOINT_REQUIREMENTS do
authorize_admin_container_image! 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, CleanupContainerRepositoryWorker.perform_async(current_user.id, repository.id,
declared_params.except(:repository_id)) # rubocop: disable CodeReuse/ActiveRecord declared_params.except(:repository_id)) # rubocop: disable CodeReuse/ActiveRecord
...@@ -123,6 +126,13 @@ module API ...@@ -123,6 +126,13 @@ module API
authorize! :admin_container_image, repository authorize! :admin_container_image, repository
end end
def obtain_new_cleanup_container_lease
Gitlab::ExclusiveLease
.new("container_repository:cleanup_tags:#{repository.id}",
timeout: 1.hour)
.try_obtain
end
def repository def repository
@repository ||= user_project.container_repositories.find(params[:repository_id]) @repository ||= user_project.container_repositories.find(params[:repository_id])
end end
......
require 'spec_helper' require 'spec_helper'
describe API::ContainerRegistry do describe API::ContainerRegistry do
include ExclusiveLeaseHelpers
set(:project) { create(:project, :private) } set(:project) { create(:project, :private) }
set(:maintainer) { create(:user) } set(:maintainer) { create(:user) }
set(:developer) { create(:user) } set(:developer) { create(:user) }
...@@ -155,7 +157,10 @@ describe API::ContainerRegistry do ...@@ -155,7 +157,10 @@ describe API::ContainerRegistry do
older_than: '1 day' } older_than: '1 day' }
end end
let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" }
it 'schedules cleanup of tags repository' do it 'schedules cleanup of tags repository' do
stub_exclusive_lease(lease_key, timeout: 1.hour)
expect(CleanupContainerRepositoryWorker).to receive(:perform_async) expect(CleanupContainerRepositoryWorker).to receive(:perform_async)
.with(maintainer.id, root_repository.id, worker_params) .with(maintainer.id, root_repository.id, worker_params)
...@@ -163,6 +168,22 @@ describe API::ContainerRegistry do ...@@ -163,6 +168,22 @@ describe API::ContainerRegistry do
expect(response).to have_gitlab_http_status(:accepted) expect(response).to have_gitlab_http_status(:accepted)
end 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 end
end end
......
...@@ -35,13 +35,5 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do ...@@ -35,13 +35,5 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do
subject.perform(user.id, -1, params) subject.perform(user.id, -1, params)
end.not_to raise_error end.not_to raise_error
end 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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment