Merge branch 'sh-delete-container-registry-async' into 'master'

Delete a container registry asynchronously

Closes #51063 and #49926

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

app/assets/javascripts/registry/components/collapsible_container.vue

@@ -6,6 +6,7 @@
   import tooltip from '../../vue_shared/directives/tooltip';
   import tableRegistry from './table_registry.vue';
   import { errorMessages, errorMessagesTypes } from '../constants';
+  import { __ } from '../../locale';
 
   export default {
     name: 'CollapsibeContainerRegisty',
@@ -46,7 +47,10 @@
 
       handleDeleteRepository() {
         this.deleteRepo(this.repo)
-          .then(() => this.fetchRepos())
+          .then(() => {
+            Flash(__('This container registry has been scheduled for deletion.'), 'notice');
+            this.fetchRepos();
+          })
           .catch(() => this.showError(errorMessagesTypes.DELETE_REPO));
       },
 

app/controllers/projects/registry/repositories_controller.rb

@@ -18,14 +18,10 @@ module Projects
       end
 
       def destroy
-        if image.destroy
-          respond_to do |format|
-            format.json { head :no_content }
-          end
-        else
-          respond_to do |format|
-            format.json { head :bad_request }
-          end
+        DeleteContainerRepositoryWorker.perform_async(current_user.id, image.id)
+
+        respond_to do |format|
+          format.json { head :no_content }
         end
       end
 
@@ -41,10 +37,10 @@ module Projects
       # Needed to maintain a backwards compatibility.
       #
       def ensure_root_container_repository!
-        ContainerRegistry::Path.new(@project.full_path).tap do |path|
+        ::ContainerRegistry::Path.new(@project.full_path).tap do |path|
           break if path.has_repository?
 
-          ContainerRepository.build_from_path(path).tap do |repository|
+          ::ContainerRepository.build_from_path(path).tap do |repository|
             repository.save! if repository.has_tags?
           end
         end

app/services/projects/container_repository/destroy_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module ContainerRepository
+    class DestroyService < BaseService
+      def execute(container_repository)
+        return false unless can?(current_user, :update_container_image, project)
+
+        container_repository.destroy
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -87,6 +87,7 @@
 - authorized_projects
 - background_migration
 - create_gpg_signature
+- delete_container_repository
 - delete_merged_branches
 - delete_user
 - email_receiver

app/workers/delete_container_repository_worker.rb

+# frozen_string_literal: true
+
+class DeleteContainerRepositoryWorker
+  include ApplicationWorker
+  include ExclusiveLeaseGuard
+
+  LEASE_TIMEOUT = 1.hour
+
+  attr_reader :container_repository
+
+  def perform(current_user_id, container_repository_id)
+    current_user = User.find_by(id: current_user_id)
+    @container_repository = ContainerRepository.find_by(id: container_repository_id)
+    project = container_repository&.project
+
+    return unless current_user && container_repository && project
+
+    # If a user accidentally attempts to delete the same container registry in quick succession,
+    # this can lead to orphaned tags.
+    try_obtain_lease do
+      Projects::ContainerRepository::DestroyService.new(project, current_user).execute(container_repository)
+    end
+  end
+
+  # For ExclusiveLeaseGuard concern
+  def lease_key
+    @lease_key ||= "container_repository:delete:#{container_repository.id}"
+  end
+
+  # For ExclusiveLeaseGuard concern
+  def lease_timeout
+    LEASE_TIMEOUT
+  end
+end

changelogs/unreleased/sh-delete-container-registry-async.yml

+---
+title: Delete a container registry asynchronously
+merge_request: 21553
+author:
+type: fixed

config/sidekiq_queues.yml

@@ -46,6 +46,7 @@
   - [project_service, 1]
   - [delete_user, 1]
   - [todos_destroyer, 1]
+  - [delete_container_repository, 1]
   - [delete_merged_branches, 1]
   - [authorized_projects, 1]
   - [expire_build_instance_artifacts, 1]

locale/gitlab.pot

@@ -5934,6 +5934,9 @@ msgstr ""
 msgid "This branch has changed since you started editing. Would you like to create a new branch?"
 msgstr ""
 
+msgid "This container registry has been scheduled for deletion."
+msgstr ""
+
 msgid "This diff is collapsed."
 msgstr ""
 

spec/controllers/projects/registry/repositories_controller_spec.rb

@@ -86,9 +86,10 @@ describe Projects::Registry::RepositoriesController do
           stub_container_registry_tags(repository: :any, tags: [])
         end
 
-        it 'deletes a repository' do
-          expect { delete_repository(repository) }.to change { ContainerRepository.all.count }.by(-1)
+        it 'schedules a job to delete a repository' do
+          expect(DeleteContainerRepositoryWorker).to receive(:perform_async).with(user.id, repository.id)
 
+          delete_repository(repository)
           expect(response).to have_gitlab_http_status(:no_content)
         end
       end

spec/services/projects/container_repository/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ContainerRepository::DestroyService do
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :private) }
+
+  subject { described_class.new(project, user) }
+
+  before do
+    stub_container_registry_config(enabled: true)
+  end
+
+  context 'when user does not have access to registry' do
+    let!(:repository) { create(:container_repository, :root, project: project) }
+
+    it 'does not delete a repository' do
+      expect { subject.execute(repository) }.not_to change { ContainerRepository.all.count }
+    end
+  end
+
+  context 'when user has access to registry' do
+    before do
+      project.add_developer(user)
+    end
+
+    context 'when root container repository exists' do
+      let!(:repository) { create(:container_repository, :root, project: project) }
+
+      before do
+        stub_container_registry_tags(repository: :any, tags: [])
+      end
+
+      it 'deletes the repository' do
+        expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1)
+      end
+    end
+  end
+end

spec/workers/delete_container_repository_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DeleteContainerRepositoryWorker do
+  let(:registry) { create(:container_repository) }
+  let(:project) { registry.project }
+  let(:user) { project.owner }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    it 'executes the destroy service' do
+      service = instance_double(Projects::ContainerRepository::DestroyService)
+      expect(service).to receive(:execute)
+      expect(Projects::ContainerRepository::DestroyService).to receive(:new).with(project, user).and_return(service)
+
+      subject.perform(user.id, registry.id)
+    end
+
+    it 'does not raise error when user could not be found' do
+      expect do
+        subject.perform(-1, registry.id)
+      end.not_to raise_error
+    end
+
+    it 'does not raise error when registry could not be found' do
+      expect do
+        subject.perform(user.id, -1)
+      end.not_to raise_error
+    end
+  end
+end