Merge branch '10io-split-container-registry-delete-tags-service' into 'master'

Split the DeleteTagsService into two smaller services

Closes #208193

See merge request gitlab-org/gitlab!36337

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -42,8 +42,8 @@ module Projects
         # Technical Debt: https://gitlab.com/gitlab-org/gitlab/issues/207267
         # name_regex to be removed when container_expiration_policies is updated
         # to have both regex columns
-        regex_delete = Gitlab::UntrustedRegexp.new("\\A#{params['name_regex_delete'] || params['name_regex']}\\z")
-        regex_retain = Gitlab::UntrustedRegexp.new("\\A#{params['name_regex_keep']}\\z")
+        regex_delete = ::Gitlab::UntrustedRegexp.new("\\A#{params['name_regex_delete'] || params['name_regex']}\\z")
+        regex_retain = ::Gitlab::UntrustedRegexp.new("\\A#{params['name_regex_keep']}\\z")
 
         tags.select do |tag|
           # regex_retain will override any overlapping matches by regex_delete
@@ -81,11 +81,11 @@ module Projects
       def valid_regex?
         %w(name_regex_delete name_regex name_regex_keep).each do |param_name|
           regex = params[param_name]
-          Gitlab::UntrustedRegexp.new(regex) unless regex.blank?
+          ::Gitlab::UntrustedRegexp.new(regex) unless regex.blank?
         end
         true
       rescue RegexpError => e
-        Gitlab::ErrorTracking.log_exception(e, project_id: project.id)
+        ::Gitlab::ErrorTracking.log_exception(e, project_id: project.id)
         false
       end
     end

app/services/projects/container_repository/delete_tags_service.rb

@@ -6,65 +6,35 @@ module Projects
       LOG_DATA_BASE = { service_class: self.to_s }.freeze
 
       def execute(container_repository)
+        @container_repository = container_repository
         return error('access denied') unless can?(current_user, :destroy_container_image, project)
 
-        tag_names = params[:tags]
-        return error('not tags specified') if tag_names.blank?
+        @tag_names = params[:tags]
+        return error('not tags specified') if @tag_names.blank?
 
-        smart_delete(container_repository, tag_names)
+        delete_tags
       end
 
       private
 
-      # Delete tags by name with a single DELETE request. This is only supported
-      # by the GitLab Container Registry fork. See
-      # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/23325 for details.
-      def fast_delete(container_repository, tag_names)
-        deleted_tags = tag_names.select do |name|
-          container_repository.delete_tag_by_name(name)
-        end
-
-        deleted_tags.any? ? success(deleted: deleted_tags) : error('could not delete tags')
+      def delete_tags
+        delete_service.execute
+                      .tap(&method(:log_response))
       end
 
-      # Replace a tag on the registry with a dummy tag.
-      # This is a hack as the registry doesn't support deleting individual
-      # tags. This code effectively pushes a dummy image and assigns the tag to it.
-      # This way when the tag is deleted only the dummy image is affected.
-      # This is used to preverse compatibility with third-party registries that
-      # don't support fast delete.
-      # See https://gitlab.com/gitlab-org/gitlab/issues/15737 for a discussion
-      def slow_delete(container_repository, tag_names)
-        # generates the blobs for the dummy image
-        dummy_manifest = container_repository.client.generate_empty_manifest(container_repository.path)
-        return error('could not generate manifest') if dummy_manifest.nil?
-
-        deleted_tags = replace_tag_manifests(container_repository, dummy_manifest, tag_names)
+      def delete_service
+        fast_delete_enabled = Feature.enabled?(:container_registry_fast_tag_delete, default_enabled: true)
 
-        # Deletes the dummy image
-        # All created tag digests are the same since they all have the same dummy image.
-        # a single delete is sufficient to remove all tags with it
-        if deleted_tags.any? && container_repository.delete_tag_by_digest(deleted_tags.each_value.first)
-          success(deleted: deleted_tags.keys)
+        if fast_delete_enabled && @container_repository.client.supports_tag_delete?
+          ::Projects::ContainerRepository::Gitlab::DeleteTagsService.new(@container_repository, @tag_names)
         else
-          error('could not delete tags')
+          ::Projects::ContainerRepository::ThirdParty::DeleteTagsService.new(@container_repository, @tag_names)
         end
       end
 
-      def smart_delete(container_repository, tag_names)
-        fast_delete_enabled = Feature.enabled?(:container_registry_fast_tag_delete, default_enabled: true)
-        response = if fast_delete_enabled && container_repository.client.supports_tag_delete?
-                     fast_delete(container_repository, tag_names)
-                   else
-                     slow_delete(container_repository, tag_names)
-                   end
-
-        response.tap { |r| log_response(r, container_repository) }
-      end
-
-      def log_response(response, container_repository)
+      def log_response(response)
         log_data = LOG_DATA_BASE.merge(
-          container_repository_id: container_repository.id,
+          container_repository_id: @container_repository.id,
           message: 'deleted tags'
         )
 
@@ -76,26 +46,6 @@ module Projects
           log_error(log_data)
         end
       end
-
-      # update the manifests of the tags with the new dummy image
-      def replace_tag_manifests(container_repository, dummy_manifest, tag_names)
-        deleted_tags = {}
-
-        tag_names.each do |name|
-          digest = container_repository.client.put_tag(container_repository.path, name, dummy_manifest)
-          next unless digest
-
-          deleted_tags[name] = digest
-        end
-
-        # make sure the digests are the same (it should always be)
-        digests = deleted_tags.values.uniq
-
-        # rubocop: disable CodeReuse/ActiveRecord
-        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(ArgumentError.new('multiple tag digests')) if digests.many?
-
-        deleted_tags
-      end
     end
   end
 end

app/services/projects/container_repository/gitlab/delete_tags_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module ContainerRepository
+    module Gitlab
+      class DeleteTagsService
+        include BaseServiceUtility
+
+        def initialize(container_repository, tag_names)
+          @container_repository = container_repository
+          @tag_names = tag_names
+        end
+
+        # Delete tags by name with a single DELETE request. This is only supported
+        # by the GitLab Container Registry fork. See
+        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/23325 for details.
+        def execute
+          return success(deleted: []) if @tag_names.empty?
+
+          deleted_tags = @tag_names.select do |name|
+            @container_repository.delete_tag_by_name(name)
+          end
+
+          deleted_tags.any? ? success(deleted: deleted_tags) : error('could not delete tags')
+        end
+      end
+    end
+  end
+end

app/services/projects/container_repository/third_party/delete_tags_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module ContainerRepository
+    module ThirdParty
+      class DeleteTagsService
+        include BaseServiceUtility
+
+        def initialize(container_repository, tag_names)
+          @container_repository = container_repository
+          @tag_names = tag_names
+        end
+
+        # Replace a tag on the registry with a dummy tag.
+        # This is a hack as the registry doesn't support deleting individual
+        # tags. This code effectively pushes a dummy image and assigns the tag to it.
+        # This way when the tag is deleted only the dummy image is affected.
+        # This is used to preverse compatibility with third-party registries that
+        # don't support fast delete.
+        # See https://gitlab.com/gitlab-org/gitlab/issues/15737 for a discussion
+        def execute
+          return success(deleted: []) if @tag_names.empty?
+
+          # generates the blobs for the dummy image
+          dummy_manifest = @container_repository.client.generate_empty_manifest(@container_repository.path)
+          return error('could not generate manifest') if dummy_manifest.nil?
+
+          deleted_tags = replace_tag_manifests(dummy_manifest)
+
+          # Deletes the dummy image
+          # All created tag digests are the same since they all have the same dummy image.
+          # a single delete is sufficient to remove all tags with it
+          if deleted_tags.any? && @container_repository.delete_tag_by_digest(deleted_tags.each_value.first)
+            success(deleted: deleted_tags.keys)
+          else
+            error('could not delete tags')
+          end
+        end
+
+        private
+
+        # update the manifests of the tags with the new dummy image
+        def replace_tag_manifests(dummy_manifest)
+          deleted_tags = {}
+
+          @tag_names.each do |name|
+            digest = @container_repository.client.put_tag(@container_repository.path, name, dummy_manifest)
+            next unless digest
+
+            deleted_tags[name] = digest
+          end
+
+          # make sure the digests are the same (it should always be)
+          digests = deleted_tags.values.uniq
+
+          # rubocop: disable CodeReuse/ActiveRecord
+          Gitlab::ErrorTracking.track_and_raise_for_dev_exception(ArgumentError.new('multiple tag digests')) if digests.many?
+
+          deleted_tags
+        end
+      end
+    end
+  end
+end

spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do
+  include_context 'container repository delete tags service shared context'
+
+  let(:service) { described_class.new(repository, tags) }
+
+  describe '#execute' do
+    let(:tags) { %w[A Ba] }
+
+    subject { service.execute }
+
+    context 'with tags to delete' do
+      it 'deletes the tags by name' do
+        stub_delete_reference_requests(tags)
+        expect_delete_tag_by_names(tags)
+
+        is_expected.to eq(status: :success, deleted: tags)
+      end
+
+      it 'succeeds when tag delete returns 404' do
+        stub_delete_reference_requests('A' => 200, 'Ba' => 404)
+
+        is_expected.to eq(status: :success, deleted: tags)
+      end
+
+      it 'succeeds when a tag delete returns 500' do
+        stub_delete_reference_requests('A' => 200, 'Ba' => 500)
+
+        is_expected.to eq(status: :success, deleted: ['A'])
+      end
+
+      context 'with failures' do
+        context 'when the delete request fails' do
+          before do
+            stub_delete_reference_requests('A' => 500, 'Ba' => 500)
+          end
+
+          it { is_expected.to eq(status: :error, message: 'could not delete tags') }
+        end
+      end
+    end
+
+    context 'with empty tags' do
+      let_it_be(:tags) { [] }
+
+      it 'does not remove anything' do
+        expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name)
+
+        is_expected.to eq(status: :success, deleted: [])
+      end
+    end
+  end
+end

spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::ContainerRepository::ThirdParty::DeleteTagsService do
+  include_context 'container repository delete tags service shared context'
+
+  let(:service) { described_class.new(repository, tags) }
+
+  describe '#execute' do
+    let(:tags) { %w[A Ba] }
+
+    subject { service.execute }
+
+    context 'with tags to delete' do
+      it 'deletes the tags by name' do
+        stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+
+        tags.each { |tag| stub_put_manifest_request(tag) }
+
+        expect_delete_tag_by_digest('sha256:dummy')
+
+        is_expected.to eq(status: :success, deleted: tags)
+      end
+
+      it 'succeeds when tag delete returns 404' do
+        stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+
+        stub_put_manifest_request('A')
+        stub_put_manifest_request('Ba')
+
+        stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy")
+          .to_return(status: 404, body: '', headers: {})
+
+        is_expected.to eq(status: :success, deleted: tags)
+      end
+
+      context 'with failures' do
+        context 'when the dummy manifest generation fails' do
+          before do
+            stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false)
+          end
+
+          it { is_expected.to eq(status: :error, message: 'could not generate manifest') }
+        end
+
+        context 'when updating tags fails' do
+          before do
+            stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+
+            stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3")
+              .to_return(status: 200, body: '', headers: {})
+          end
+
+          context 'all tag updates fail' do
+            before do
+              stub_put_manifest_request('A', 500, {})
+              stub_put_manifest_request('Ba', 500, {})
+            end
+
+            it { is_expected.to eq(status: :error, message: 'could not delete tags') }
+          end
+
+          context 'a single tag update fails' do
+            before do
+              stub_put_manifest_request('A')
+              stub_put_manifest_request('Ba', 500, {})
+
+              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy")
+                .to_return(status: 404, body: '', headers: {})
+            end
+
+            it { is_expected.to eq(status: :success, deleted: ['A']) }
+          end
+        end
+      end
+    end
+
+    context 'with empty tags' do
+      let_it_be(:tags) { [] }
+
+      it 'does not remove anything' do
+        expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name)
+
+        is_expected.to eq(status: :success, deleted: [])
+      end
+    end
+  end
+end

spec/support/shared_contexts/services/projects/container_repository/delete_tags_service_shared_context.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'container repository delete tags service shared context' do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project, reload: true) { create(:project, :private) }
+  let_it_be(:repository) { create(:container_repository, :root, project: project) }
+
+  let(:params) { { tags: tags } }
+
+  before do
+    stub_container_registry_config(enabled: true,
+                                   api_url: 'http://registry.gitlab',
+                                   host_port: 'registry.gitlab')
+
+    stub_container_registry_tags(
+      repository: repository.path,
+      tags: %w(latest A Ba Bb C D E))
+  end
+
+  def stub_delete_reference_request(tag, status = 200)
+    stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/#{tag}")
+      .to_return(status: status, body: '')
+  end
+
+  def stub_delete_reference_requests(tags)
+    tags = Hash[Array.wrap(tags).map { |tag| [tag, 200] }] unless tags.is_a?(Hash)
+
+    tags.each do |tag, status|
+      stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/#{tag}")
+      .to_return(status: status, body: '')
+    end
+  end
+
+  def stub_put_manifest_request(tag, status = 200, headers = { 'docker-content-digest' => 'sha256:dummy' })
+    stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}")
+      .to_return(status: status, body: '', headers: headers)
+  end
+
+  def stub_tag_digest(tag, digest)
+    stub_request(:head, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}")
+      .to_return(status: 200, body: '', headers: { 'docker-content-digest' => digest })
+  end
+
+  def stub_digest_config(digest, created_at)
+    allow_any_instance_of(ContainerRegistry::Client)
+      .to receive(:blob)
+      .with(repository.path, digest, nil) do
+      { 'created' => created_at.to_datetime.rfc3339 }.to_json if created_at
+    end
+  end
+
+  def stub_upload(digest, success: true)
+    content = "{\n  \"config\": {\n  }\n}"
+    expect_any_instance_of(ContainerRegistry::Client)
+      .to receive(:upload_blob)
+      .with(repository.path, content, digest) { double(success?: success ) }
+  end
+
+  def expect_delete_tag_by_digest(digest)
+    expect_any_instance_of(ContainerRegistry::Client)
+      .to receive(:delete_repository_tag_by_digest)
+      .with(repository.path, digest) { true }
+
+    expect_any_instance_of(ContainerRegistry::Client)
+      .not_to receive(:delete_repository_tag_by_name)
+  end
+
+  def expect_delete_tag_by_names(names)
+    Array.wrap(names).each do |name|
+      expect_any_instance_of(ContainerRegistry::Client)
+        .to receive(:delete_repository_tag_by_name)
+        .with(repository.path, name) { true }
+
+      expect_any_instance_of(ContainerRegistry::Client)
+        .not_to receive(:delete_repository_tag_by_digest)
+    end
+  end
+end