From b9751a328afb910aa42e53541dc9d883ef0b18c5 Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:12:09 +1000
Subject: [PATCH 1/8] Add support for updating manifests

- Adds Client#upload() to upload docker blobs
- Adds Client#generate_manifest() to generate random
blobs for manifests.
- Adds Client#generate_dummy_image() to generate a dummy image
- Adds Client#put_tag() to replace tag manifests
---
 lib/container_registry/client.rb | 50 ++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb
index 15f40993ea3c3f..6700a73db5e206 100644
--- a/lib/container_registry/client.rb
+++ b/lib/container_registry/client.rb
@@ -2,6 +2,7 @@
 
 require 'faraday'
 require 'faraday_middleware'
+require 'digest'
 
 module ContainerRegistry
   class Client
@@ -9,6 +10,8 @@ class Client
 
     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,44 @@ def delete_repository_tag(name, reference)
       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/")
+
+      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 +86,15 @@ def delete_blob(name, digest)
       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)
-- 
GitLab


From 659bccfd32bb77831913f35ef3e15a017933a3c2 Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:09:17 +1000
Subject: [PATCH 2/8] Adds DeleteTagsService to implement smart delete

Works around docker limitation to delete
individual tags.
This essentially creates a new dummy tag and
rewrite the manifests of the tags to be deleted
to point to the new dummy tag.
Then the dummy tag is deleted removing only
the selected tags.
---
 .../delete_tags_service.rb                    | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 app/services/projects/container_repository/delete_tags_service.rb

diff --git a/app/services/projects/container_repository/delete_tags_service.rb b/app/services/projects/container_repository/delete_tags_service.rb
new file mode 100644
index 00000000000000..8a79581e83f363
--- /dev/null
+++ b/app/services/projects/container_repository/delete_tags_service.rb
@@ -0,0 +1,60 @@
+# 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
+
+        # 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
-- 
GitLab


From 759d7a504378f0956c749f6cb5869aa1c849b665 Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:11:25 +1000
Subject: [PATCH 3/8] Include tag fix changes in Changelog

---
 doc/api/container_registry.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/api/container_registry.md b/doc/api/container_registry.md
index cb1a81b97b6793..9b7f8d7dc97a28 100644
--- a/doc/api/container_registry.md
+++ b/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-ce/issues/21405).
 
 Examples:
 
-- 
GitLab


From fdda4723e326170d7e89baa273b9ae9fc27d57d0 Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:13:42 +1000
Subject: [PATCH 4/8] Adds changelog entry for tag delete

---
 changelogs/unreleased/21405-fix-registry-tag-delete.yml | 5 +++++
 doc/api/container_registry.md                           | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/21405-fix-registry-tag-delete.yml

diff --git a/changelogs/unreleased/21405-fix-registry-tag-delete.yml b/changelogs/unreleased/21405-fix-registry-tag-delete.yml
new file mode 100644
index 00000000000000..48890ec62fc6d1
--- /dev/null
+++ b/changelogs/unreleased/21405-fix-registry-tag-delete.yml
@@ -0,0 +1,5 @@
+---
+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
diff --git a/doc/api/container_registry.md b/doc/api/container_registry.md
index 9b7f8d7dc97a28..7e5e265351ec25 100644
--- a/doc/api/container_registry.md
+++ b/doc/api/container_registry.md
@@ -253,7 +253,7 @@ This action does not delete blobs. In order to delete them and recycle disk spac
 
 NOTE: **Note:**
 Since GitLab 12.4, individual tags are deleted.
-For more details, see the [discussion](https://gitlab.com/gitlab-org/gitlab-ce/issues/21405).
+For more details, see the [discussion](https://gitlab.com/gitlab-org/gitlab/issues/15737).
 
 Examples:
 
-- 
GitLab


From 6e8f1a136c00997642fbcb349c0388581a3cf57c Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:13:15 +1000
Subject: [PATCH 5/8] Move #delete method to #unsafe_delete

---
 lib/container_registry/tag.rb | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/container_registry/tag.rb b/lib/container_registry/tag.rb
index ebea84fa1ca84e..2cc4c8d8b1c5b2 100644
--- a/lib/container_registry/tag.rb
+++ b/lib/container_registry/tag.rb
@@ -98,6 +98,10 @@ def layers
       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 @@ def total_size
     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)
-- 
GitLab


From 555c1948c469e018ea838094aef04bfc6c7512e6 Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:14:32 +1000
Subject: [PATCH 6/8] Use DeleteTagService to delete tags

Update controllers and api to use the new
DeleteTagService class to remote tags.
---
 .../projects/registry/tags_controller.rb      | 35 ++++++-------------
 .../cleanup_tags_service.rb                   |  2 +-
 lib/api/project_container_repositories.rb     | 12 +++++--
 3 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/app/controllers/projects/registry/tags_controller.rb b/app/controllers/projects/registry/tags_controller.rb
index 54e2faa2dd7f0f..b5ee0ba9bebf3a 100644
--- a/app/controllers/projects/registry/tags_controller.rb
+++ b/app/controllers/projects/registry/tags_controller.rb
@@ -19,14 +19,12 @@ def index
       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 @@ def bulk_destroy
           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 @@ def image
         @image ||= project.container_repositories
           .find(params[:repository_id])
       end
-
-      def tag
-        @tag ||= image.tag(params[:id])
-      end
     end
   end
 end
diff --git a/app/services/projects/container_repository/cleanup_tags_service.rb b/app/services/projects/container_repository/cleanup_tags_service.rb
index d1d9b9f22e87ca..1b880a7aab1814 100644
--- a/app/services/projects/container_repository/cleanup_tags_service.rb
+++ b/app/services/projects/container_repository/cleanup_tags_service.rb
@@ -40,7 +40,7 @@ def delete_tag_digest(digest, tags, other_tags)
         return unless tags.count == other_tags.count
 
         # delete all tags
-        tags.map(&:delete)
+        tags.map(&:unsafe_delete)
       end
 
       def group_by_digest(tags)
diff --git a/lib/api/project_container_repositories.rb b/lib/api/project_container_repositories.rb
index c10ef96922c084..2a05974509a6c2 100644
--- a/lib/api/project_container_repositories.rb
+++ b/lib/api/project_container_repositories.rb
@@ -106,9 +106,15 @@ class ProjectContainerRepositories < Grape::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
 
-- 
GitLab


From bf16664b635a653f475308e4175ae50f6b7626fd Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Tue, 17 Sep 2019 16:15:07 +1000
Subject: [PATCH 7/8] Add specs for tag delete changes

Update registry tag removal related
specs for the new changes using
the new DeleteTagService
---
 lib/container_registry/client.rb              |   1 +
 .../projects/registry/tags_controller_spec.rb |  20 ++-
 spec/features/container_registry_spec.rb      |   4 +-
 spec/lib/container_registry/client_spec.rb    |  65 ++++++++++
 spec/lib/container_registry/tag_spec.rb       |   4 +-
 .../project_container_repositories_spec.rb    |  36 ++++--
 .../delete_tags_service_spec.rb               | 120 ++++++++++++++++++
 7 files changed, 232 insertions(+), 18 deletions(-)
 create mode 100644 spec/services/projects/container_repository/delete_tags_service_spec.rb

diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb
index 6700a73db5e206..2bd8eb65306ccf 100644
--- a/lib/container_registry/client.rb
+++ b/lib/container_registry/client.rb
@@ -49,6 +49,7 @@ def upload_raw_blob(path, blob)
 
     def upload_blob(name, content, digest)
       upload = faraday.post("/v2/#{name}/blobs/uploads/")
+      return unless upload.success?
 
       location = URI(upload.headers['location'])
 
diff --git a/spec/controllers/projects/registry/tags_controller_spec.rb b/spec/controllers/projects/registry/tags_controller_spec.rb
index c6e063d82290db..8da0d217e9eef9 100644
--- a/spec/controllers/projects/registry/tags_controller_spec.rb
+++ b/spec/controllers/projects/registry/tags_controller_spec.rb
@@ -10,6 +10,8 @@
     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 @@ def get_tags
 
       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 @@ def destroy_tag(name)
           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 @@ def bulk_destroy_tags(names)
                      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
diff --git a/spec/features/container_registry_spec.rb b/spec/features/container_registry_spec.rb
index aefdc4d6d4f622..dfd08483430658 100644
--- a/spec/features/container_registry_spec.rb
+++ b/spec/features/container_registry_spec.rb
@@ -53,7 +53,9 @@
       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'
diff --git a/spec/lib/container_registry/client_spec.rb b/spec/lib/container_registry/client_spec.rb
index 6c2b338bfcd973..3782c30e88aa84 100644
--- a/spec/lib/container_registry/client_spec.rb
+++ b/spec/lib/container_registry/client_spec.rb
@@ -73,4 +73,69 @@
       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
diff --git a/spec/lib/container_registry/tag_spec.rb b/spec/lib/container_registry/tag_spec.rb
index 110f006536bcdf..3115dfe852f6c9 100644
--- a/spec/lib/container_registry/tag_spec.rb
+++ b/spec/lib/container_registry/tag_spec.rb
@@ -179,7 +179,7 @@
       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 @@
       end
 
       it 'correctly deletes the tag' do
-        expect(tag.delete).to be_truthy
+        expect(tag.unsafe_delete).to be_truthy
       end
     end
   end
diff --git a/spec/requests/api/project_container_repositories_spec.rb b/spec/requests/api/project_container_repositories_spec.rb
index f1dc4e6f0b2d92..3ac7ff7656be02 100644
--- a/spec/requests/api/project_container_repositories_spec.rb
+++ b/spec/requests/api/project_container_repositories_spec.rb
@@ -150,7 +150,7 @@
           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 @@
   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 @@
     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
diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb
new file mode 100644
index 00000000000000..2ec5850c69e47e
--- /dev/null
+++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb
@@ -0,0 +1,120 @@
+# 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
-- 
GitLab


From 17002143807d1056d0c29767426b05d1beec16d1 Mon Sep 17 00:00:00 2001
From: Giorgenes Gelatti <ggelatti@gitlab.com>
Date: Wed, 18 Sep 2019 22:44:29 +1000
Subject: [PATCH 8/8] Safeguard unique digests

---
 .../projects/container_repository/delete_tags_service.rb    | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/services/projects/container_repository/delete_tags_service.rb b/app/services/projects/container_repository/delete_tags_service.rb
index 8a79581e83f363..21dc1621e5c5e1 100644
--- a/app/services/projects/container_repository/delete_tags_service.rb
+++ b/app/services/projects/container_repository/delete_tags_service.rb
@@ -42,6 +42,12 @@ def smart_delete(container_repository, tag_names)
           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
-- 
GitLab