Merge branch '21405-fix-registry-tag-delete-simple' into 'master'

Fix registry tag deletes

Closes #15737

See merge request gitlab-org/gitlab!16886

app/controllers/projects/registry/tags_controller.rb

@@ -19,14 +19,12 @@ module Projects
       end
 
       def destroy
-        if tag.delete
-          respond_to do |format|
-            format.json { head :no_content }
-          end
-        else
-          respond_to do |format|
-            format.json { head :bad_request }
-          end
+        result = Projects::ContainerRepository::DeleteTagsService
+          .new(image.project, current_user, tags: [params[:id]])
+          .execute(image)
+
+        respond_to do |format|
+          format.json { head(result[:status] == :success ? :ok : bad_request) }
         end
       end
 
@@ -42,21 +40,12 @@ module Projects
           return
         end
 
-        @tags = tag_names.map { |tag_name| image.tag(tag_name) }
-        unless @tags.all? { |tag| tag.valid_name? }
-          head :bad_request
-          return
-        end
-
-        success_count = 0
-        @tags.each do |tag|
-          if tag.delete
-            success_count += 1
-          end
-        end
+        result = Projects::ContainerRepository::DeleteTagsService
+          .new(image.project, current_user, tags: tag_names)
+          .execute(image)
 
         respond_to do |format|
-          format.json { head(success_count == @tags.size ? :no_content : :bad_request) }
+          format.json { head(result[:status] == :success ? :no_content : :bad_request) }
         end
       end
 
@@ -70,10 +59,6 @@ module Projects
         @image ||= project.container_repositories
           .find(params[:repository_id])
       end
-
-      def tag
-        @tag ||= image.tag(params[:id])
-      end
     end
   end
 end

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -40,7 +40,7 @@ module Projects
         return unless tags.count == other_tags.count
 
         # delete all tags
-        tags.map(&:delete)
+        tags.map(&:unsafe_delete)
       end
 
       def group_by_digest(tags)

app/services/projects/container_repository/delete_tags_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module ContainerRepository
+    class DeleteTagsService < BaseService
+      def execute(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?
+
+        if can_use?
+          smart_delete(container_repository, tag_names)
+        else
+          unsafe_delete(container_repository, tag_names)
+        end
+      end
+
+      private
+
+      def unsafe_delete(container_repository, tag_names)
+        deleted_tags = tag_names.select do |tag_name|
+          container_repository.tag(tag_name).unsafe_delete
+        end
+
+        return error('could not delete tags') if deleted_tags.empty?
+
+        success(deleted: deleted_tags)
+      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.
+      # See https://gitlab.com/gitlab-org/gitlab-ce/issues/21405 for a discussion
+      def smart_delete(container_repository, tag_names)
+        # generates the blobs for the dummy image
+        dummy_manifest = container_repository.client.generate_empty_manifest(container_repository.path)
+
+        # update the manifests of the tags with the new dummy image
+        tag_digests = tag_names.map do |name|
+          container_repository.client.put_tag(container_repository.path, name, dummy_manifest)
+        end
+
+        # make sure the digests are the same (it should always be)
+        tag_digests.uniq!
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        Gitlab::Sentry.track_exception(ArgumentError.new('multiple tag digests')) if tag_digests.many?
+
+        # 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 container_repository.client.delete_repository_tag(container_repository.path, tag_digests.first)
+          success(deleted: tag_names)
+        else
+          error('could not delete tags')
+        end
+      end
+
+      def can_use?
+        Feature.enabled?(:container_registry_smart_delete, project, default_enabled: true)
+      end
+    end
+  end
+end

changelogs/unreleased/21405-fix-registry-tag-delete.yml

+---
+title: 'Adds the ability to delete single tags from the docker registry. Fix the issue that caused all related tags and image to be deleted at the same time.'
+merge_request: 16886
+author:
+type: fixed

doc/api/container_registry.md

@@ -252,8 +252,8 @@ This action does not delete blobs. In order to delete them and recycle disk spac
 [run the garbage collection](https://docs.gitlab.com/omnibus/maintenance/README.html#removing-unused-layers-not-referenced-by-manifests).
 
 NOTE: **Note:**
-Due to a [Docker Distribution deficiency](https://gitlab.com/gitlab-org/gitlab-foss/issues/21405),
-it doesn't remove tags whose manifest is shared by multiple tags.
+Since GitLab 12.4, individual tags are deleted.
+For more details, see the [discussion](https://gitlab.com/gitlab-org/gitlab/issues/15737).
 
 Examples:
 

lib/api/project_container_repositories.rb

@@ -106,9 +106,15 @@ module API
         authorize_destroy_container_image!
         validate_tag!
 
-        tag.delete
-
-        status :ok
+        result = ::Projects::ContainerRepository::DeleteTagsService
+          .new(repository.project, current_user, tags: [declared_params[:tag_name]])
+          .execute(repository)
+
+        if result[:status] == :success
+          status :ok
+        else
+          status :bad_request
+        end
       end
     end
 

lib/container_registry/client.rb

@@ -2,6 +2,7 @@
 
 require 'faraday'
 require 'faraday_middleware'
+require 'digest'
 
 module ContainerRegistry
   class Client
@@ -9,6 +10,8 @@ module ContainerRegistry
 
     DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE = 'application/vnd.docker.distribution.manifest.v2+json'
     OCI_MANIFEST_V1_TYPE = 'application/vnd.oci.image.manifest.v1+json'
+    CONTAINER_IMAGE_V1_TYPE = 'application/vnd.docker.container.image.v1+json'
+
     ACCEPTED_TYPES = [DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE, OCI_MANIFEST_V1_TYPE].freeze
 
     # Taken from: FaradayMiddleware::FollowRedirects
@@ -36,6 +39,45 @@ module ContainerRegistry
       faraday.delete("/v2/#{name}/manifests/#{reference}").success?
     end
 
+    def upload_raw_blob(path, blob)
+      digest = "sha256:#{Digest::SHA256.hexdigest(blob)}"
+
+      if upload_blob(path, blob, digest).success?
+        [blob, digest]
+      end
+    end
+
+    def upload_blob(name, content, digest)
+      upload = faraday.post("/v2/#{name}/blobs/uploads/")
+      return unless upload.success?
+
+      location = URI(upload.headers['location'])
+
+      faraday.put("#{location.path}?#{location.query}") do |req|
+        req.params['digest'] = digest
+        req.headers['Content-Type'] = 'application/octet-stream'
+        req.body = content
+      end
+    end
+
+    def generate_empty_manifest(path)
+      image = {
+        config: {}
+      }
+      image, image_digest = upload_raw_blob(path, JSON.pretty_generate(image))
+      return unless image
+
+      {
+        schemaVersion: 2,
+        mediaType: DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE,
+        config: {
+          mediaType: CONTAINER_IMAGE_V1_TYPE,
+          size: image.size,
+          digest: image_digest
+        }
+      }
+    end
+
     def blob(name, digest, type = nil)
       type ||= 'application/octet-stream'
       response_body faraday_blob.get("/v2/#{name}/blobs/#{digest}", nil, 'Accept' => type), allow_redirect: true
@@ -45,6 +87,15 @@ module ContainerRegistry
       faraday.delete("/v2/#{name}/blobs/#{digest}").success?
     end
 
+    def put_tag(name, reference, manifest)
+      response = faraday.put("/v2/#{name}/manifests/#{reference}") do |req|
+        req.headers['Content-Type'] = DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE
+        req.body = JSON.pretty_generate(manifest)
+      end
+
+      response.headers['docker-content-digest'] if response.success?
+    end
+
     private
 
     def initialize_connection(conn, options)

lib/container_registry/tag.rb

@@ -98,6 +98,10 @@ module ContainerRegistry
       end
     end
 
+    def put(digests)
+      repository.client.put_tag(repository.path, name, digests)
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def total_size
       return unless layers
@@ -106,7 +110,10 @@ module ContainerRegistry
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def delete
+    # Deletes the image associated with this tag
+    # Note this will delete the image and all tags associated with it.
+    # Consider using DeleteTagsService instead.
+    def unsafe_delete
       return unless digest
 
       client.delete_repository_tag(repository.path, digest)

spec/controllers/projects/registry/tags_controller_spec.rb

@@ -10,6 +10,8 @@ describe Projects::Registry::TagsController do
     create(:container_repository, name: 'image', project: project)
   end
 
+  let(:service) { double('service') }
+
   before do
     sign_in(user)
     stub_container_registry_config(enabled: true)
@@ -84,17 +86,17 @@ describe Projects::Registry::TagsController do
 
       context 'when there is matching tag present' do
         before do
-          stub_container_registry_tags(repository: repository.path, tags: %w[rc1 test.])
+          stub_container_registry_tags(repository: repository.path, tags: %w[rc1], with_manifest: true)
         end
 
         it 'makes it possible to delete regular tag' do
-          expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete)
+          expect_delete_tags(%w[rc1])
 
           destroy_tag('rc1')
         end
 
         it 'makes it possible to delete a tag that ends with a dot' do
-          expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete)
+          expect_delete_tags(%w[test.])
 
           destroy_tag('test.')
         end
@@ -125,11 +127,12 @@ describe Projects::Registry::TagsController do
           stub_container_registry_tags(repository: repository.path, tags: %w[rc1 test.])
         end
 
+        let(:tags) { %w[tc1 test.] }
+
         it 'makes it possible to delete tags in bulk' do
-          allow_any_instance_of(ContainerRegistry::Tag).to receive(:delete) { |*args| ContainerRegistry::Tag.delete(*args) }
-          expect(ContainerRegistry::Tag).to receive(:delete).exactly(2).times
+          expect_delete_tags(tags)
 
-          bulk_destroy_tags(['rc1', 'test.'])
+          bulk_destroy_tags(tags)
         end
       end
     end
@@ -146,4 +149,9 @@ describe Projects::Registry::TagsController do
                      format: :json
     end
   end
+
+  def expect_delete_tags(tags, status = :success)
+    expect(service).to receive(:execute).with(repository) { { status: status } }
+    expect(Projects::ContainerRepository::DeleteTagsService).to receive(:new).with(repository.project, user, tags: tags) { service }
+  end
 end

spec/features/container_registry_spec.rb

@@ -53,7 +53,9 @@ describe 'Container Registry', :js do
       find('.js-toggle-repo').click
       wait_for_requests
 
-      expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete).and_return(true)
+      service = double('service')
+      expect(service).to receive(:execute).with(container_repository) { { status: :success } }
+      expect(Projects::ContainerRepository::DeleteTagsService).to receive(:new).with(container_repository.project, user, tags: ['latest']) { service }
 
       click_on(class: 'js-delete-registry-row', visible: false)
       expect(find('.modal .modal-title')).to have_content 'Remove image'

spec/lib/container_registry/client_spec.rb

@@ -73,4 +73,69 @@ describe ContainerRegistry::Client do
       expect(response).to eq('Successfully redirected')
     end
   end
+
+  def stub_upload(path, content, digest, status = 200)
+    stub_request(:post, "http://container-registry/v2/#{path}/blobs/uploads/")
+      .to_return(status: status, body: "", headers: { 'location' => 'http://container-registry/next_upload?id=someid' })
+
+    stub_request(:put, "http://container-registry/next_upload?digest=#{digest}&id=someid")
+      .with(body: content)
+      .to_return(status: status, body: "", headers: {})
+  end
+
+  describe '#upload_blob' do
+    subject { client.upload_blob('path', 'content', 'sha256:123') }
+
+    context 'with successful uploads' do
+      it 'starts the upload and posts the blob' do
+        stub_upload('path', 'content', 'sha256:123')
+
+        expect(subject).to be_success
+      end
+    end
+
+    context 'with a failed upload' do
+      before do
+        stub_upload('path', 'content', 'sha256:123', 400)
+      end
+
+      it 'returns nil' do
+        expect(subject).to be nil
+      end
+    end
+  end
+
+  describe '#generate_empty_manifest' do
+    subject { client.generate_empty_manifest('path') }
+
+    let(:result_manifest) do
+      {
+        schemaVersion: 2,
+        mediaType: 'application/vnd.docker.distribution.manifest.v2+json',
+        config: {
+          mediaType: 'application/vnd.docker.container.image.v1+json',
+          size: 21,
+          digest: 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3'
+        }
+      }
+    end
+
+    it 'uploads a random image and returns the manifest' do
+      stub_upload('path', "{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+
+      expect(subject).to eq(result_manifest)
+    end
+  end
+
+  describe '#put_tag' do
+    subject { client.put_tag('path', 'tagA', { foo: :bar }) }
+
+    it 'uploads the manifest and returns the digest' do
+      stub_request(:put, "http://container-registry/v2/path/manifests/tagA")
+        .with(body: "{\n  \"foo\": \"bar\"\n}")
+        .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:123' })
+
+      expect(subject).to eq 'sha256:123'
+    end
+  end
 end

spec/lib/container_registry/tag_spec.rb

@@ -179,7 +179,7 @@ describe ContainerRegistry::Tag do
       end
     end
 
-    describe '#delete' do
+    describe '#unsafe_delete' do
       before do
         stub_request(:delete, 'http://registry.gitlab/v2/group/test/manifests/sha256:digest')
           .with(headers: headers)
@@ -187,7 +187,7 @@ describe ContainerRegistry::Tag do
       end
 
       it 'correctly deletes the tag' do
-        expect(tag.delete).to be_truthy
+        expect(tag.unsafe_delete).to be_truthy
       end
     end
   end

spec/requests/api/project_container_repositories_spec.rb

@@ -150,7 +150,7 @@ describe API::ProjectContainerRepositories do
           expect(response).to have_gitlab_http_status(:accepted)
         end
 
-        context 'called multiple times in one hour' do
+        context 'called multiple times in one hour', :clean_gitlab_redis_shared_state do
           it 'returns 400 with an error message' do
             stub_exclusive_lease_taken(lease_key, timeout: 1.hour)
             subject
@@ -202,6 +202,8 @@ describe API::ProjectContainerRepositories do
   end
 
   describe 'DELETE /projects/:id/registry/repositories/:repository_id/tags/:tag_name' do
+    let(:service) { double('service') }
+
     subject { delete api("/projects/#{project.id}/registry/repositories/#{root_repository.id}/tags/rootA", api_user) }
 
     it_behaves_like 'rejected container repository access', :reporter, :forbidden
@@ -210,18 +212,34 @@ describe API::ProjectContainerRepositories do
     context 'for developer' do
       let(:api_user) { developer }
 
-      before do
-        stub_container_registry_tags(repository: root_repository.path, tags: %w(rootA), with_manifest: true)
+      context 'when there are multiple tags' do
+        before do
+          stub_container_registry_tags(repository: root_repository.path, tags: %w(rootA rootB), with_manifest: true)
+        end
+
+        it 'properly removes tag' do
+          expect(service).to receive(:execute).with(root_repository) { { status: :success } }
+          expect(Projects::ContainerRepository::DeleteTagsService).to receive(:new).with(root_repository.project, api_user, tags: %w[rootA]) { service }
+
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
 
-      it 'properly removes tag' do
-        expect_any_instance_of(ContainerRegistry::Client)
-          .to receive(:delete_repository_tag).with(root_repository.path,
-            'sha256:4c8e63ca4cb663ce6c688cb06f1c372b088dac5b6d7ad7d49cd620d85cf72a15')
+      context 'when there\'s only one tag' do
+        before do
+          stub_container_registry_tags(repository: root_repository.path, tags: %w(rootA), with_manifest: true)
+        end
 
-        subject
+        it 'properly removes tag' do
+          expect(service).to receive(:execute).with(root_repository) { { status: :success } }
+          expect(Projects::ContainerRepository::DeleteTagsService).to receive(:new).with(root_repository.project, api_user, tags: %w[rootA]) { service }
 
-        expect(response).to have_gitlab_http_status(:ok)
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
     end
   end

spec/services/projects/container_repository/delete_tags_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ContainerRepository::DeleteTagsService do
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :private) }
+  set(:repository) { create(:container_repository, :root, project: project) }
+
+  let(:params) { { tags: tags } }
+  let(:service) { described_class.new(project, user, params) }
+
+  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))
+
+    stub_tag_digest('latest', 'sha256:configA')
+    stub_tag_digest('A', 'sha256:configA')
+    stub_tag_digest('Ba', 'sha256:configB')
+  end
+
+  describe '#execute' do
+    let(:tags) { %w[A] }
+    subject { service.execute(repository) }
+
+    context 'without permissions' do
+      it { is_expected.to include(status: :error) }
+    end
+
+    context 'with permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      context 'when no params are specified' do
+        let(:params) { {} }
+
+        it 'does not remove anything' do
+          expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag)
+
+          is_expected.to include(status: :error)
+        end
+      end
+
+      context 'with empty tags' do
+        let(:tags) { [] }
+
+        it 'does not remove anything' do
+          expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag)
+
+          is_expected.to include(status: :error)
+        end
+      end
+
+      context 'with dummy tags disabled' do
+        let(:tags) { %w[A Ba] }
+
+        before do
+          stub_feature_flags(container_registry_smart_delete: false)
+        end
+
+        it 'deletes tags one by one' do
+          expect_delete_tag('sha256:configA')
+          expect_delete_tag('sha256:configB')
+          is_expected.to include(status: :success)
+        end
+      end
+
+      context 'with dummy tags enabled' do
+        let(:tags) { %w[A Ba] }
+
+        it 'deletes the tags using a dummy image' do
+          stub_upload("{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+
+          stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A")
+            .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' })
+
+          stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba")
+            .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' })
+
+          expect_delete_tag('sha256:dummy')
+
+          is_expected.to include(status: :success)
+        end
+      end
+    end
+  end
+
+  private
+
+  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(content, digest)
+    expect_any_instance_of(ContainerRegistry::Client)
+      .to receive(:upload_blob)
+      .with(repository.path, content, digest) { double(success?: true ) }
+  end
+
+  def expect_delete_tag(digest)
+    expect_any_instance_of(ContainerRegistry::Client)
+      .to receive(:delete_repository_tag)
+      .with(repository.path, digest) { true }
+  end
+end