diff --git a/app/controllers/projects/registry/tags_controller.rb b/app/controllers/projects/registry/tags_controller.rb index 54e2faa2dd7f0f4af77dbca86de994560c01f635..b5ee0ba9bebf3af56d37e312b87b9be6d7b2ff38 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 d1d9b9f22e87cac22f4dc2996a82c52c760cf03c..1b880a7aab1814bb11542adf2e329c88995634fa 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/app/services/projects/container_repository/delete_tags_service.rb b/app/services/projects/container_repository/delete_tags_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..21dc1621e5c5e14ae17acc2e4c2a577a4c80cd96 --- /dev/null +++ b/app/services/projects/container_repository/delete_tags_service.rb @@ -0,0 +1,66 @@ +# 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 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 0000000000000000000000000000000000000000..48890ec62fc6d1e1d546a1057177a24e4d0e1948 --- /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 cb1a81b97b67937fb07757af2cd4531ef25347c1..7e5e265351ec258768ddd5d21870df635f16e51d 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/issues/15737). Examples: diff --git a/lib/api/project_container_repositories.rb b/lib/api/project_container_repositories.rb index c10ef96922c08437ebdb00471373ba3dd5ea6fab..2a05974509a6c256072662e210b62b5cd1811a78 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 diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb index 15f40993ea3c3f4d48dfcd1f5da6f674449eb7b9..2bd8eb65306ccfff6c4d91452412fde0c3b5784f 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,45 @@ 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/") + 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 @@ 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) diff --git a/lib/container_registry/tag.rb b/lib/container_registry/tag.rb index ebea84fa1ca84e5d7debd3a3119a64d066c53f85..2cc4c8d8b1c5b2f524b81e1faff7605c5e91f3e2 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) diff --git a/spec/controllers/projects/registry/tags_controller_spec.rb b/spec/controllers/projects/registry/tags_controller_spec.rb index c6e063d82290dbdd6d251073ce64a840d0583319..8da0d217e9eef9b046f462bb3fa0295bb01784b0 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 aefdc4d6d4f622eae57fc3e6ebb8e2cadcf2a2e9..dfd084834306580944f78567f87107a212800435 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 6c2b338bfcd973e55c35d8eb69ef3486a3f1f9fb..3782c30e88aa84eadb040b8078505752d793a899 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 110f006536bcdf6b1fdcfe1261f594edbfa237b6..3115dfe852f6c93389861878813543e7200bc9dd 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 f1dc4e6f0b2d9273eda4da195994553eebfca927..3ac7ff7656be02db6c09a1f44da6bc905d9e81f4 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 0000000000000000000000000000000000000000..2ec5850c69e47e7308d0eecb669f508886d254a6 --- /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