Merge branch '208193-add-logs-to-container-repository-delete-tags-service' into 'master'

Add logging to DeleteTagsService

See merge request gitlab-org/gitlab!35539

app/services/projects/container_repository/delete_tags_service.rb

@@ -3,6 +3,8 @@
 module Projects
   module ContainerRepository
     class DeleteTagsService < BaseService
+      LOG_DATA_BASE = { service_class: self.to_s }.freeze
+
       def execute(container_repository)
         return error('access denied') unless can?(current_user, :destroy_container_image, project)
 
@@ -51,10 +53,27 @@ module Projects
 
       def smart_delete(container_repository, tag_names)
         fast_delete_enabled = Feature.enabled?(:container_registry_fast_tag_delete, default_enabled: true)
-        if fast_delete_enabled && container_repository.client.supports_tag_delete?
-          fast_delete(container_repository, tag_names)
+        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)
+        log_data = LOG_DATA_BASE.merge(
+          container_repository_id: container_repository.id,
+          message: 'deleted tags'
+        )
+
+        if response[:status] == :success
+          log_data[:deleted_tags_count] = response[:deleted].size
+          log_info(log_data)
         else
-          slow_delete(container_repository, tag_names)
+          log_data[:message] = response[:message]
+          log_error(log_data)
         end
       end
 

changelogs/unreleased/208193-add-logs-to-container-repository-delete-tags-service.yml

+---
+title: Add log statements to Projects::ContainerRepository::DeleteTagsService
+merge_request: 35539
+author:
+type: added

spec/services/projects/container_repository/delete_tags_service_spec.rb

@@ -20,6 +20,31 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
       tags: %w(latest A Ba Bb C D E))
   end
 
+  RSpec.shared_examples 'logging a success response' do
+    it 'logs an info message' do
+      expect(service).to receive(:log_info).with(
+        service_class: 'Projects::ContainerRepository::DeleteTagsService',
+        message: 'deleted tags',
+        container_repository_id: repository.id,
+        deleted_tags_count: tags.size
+      )
+
+      subject
+    end
+  end
+
+  RSpec.shared_examples 'logging an error response' do |message: 'could not delete tags'|
+    it 'logs an error message' do
+      expect(service).to receive(:log_error).with(
+        service_class: 'Projects::ContainerRepository::DeleteTagsService',
+        message: message,
+        container_repository_id: repository.id
+      )
+
+      subject
+    end
+  end
+
   describe '#execute' do
     let(:tags) { %w[A] }
 
@@ -47,11 +72,8 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
             let_it_be(:tags) { %w[A Ba] }
 
             it 'deletes the tags by name' do
-              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A")
-                .to_return(status: 200, body: "")
-
-              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba")
-                .to_return(status: 200, body: "")
+              stub_delete_reference_request('A')
+              stub_delete_reference_request('Ba')
 
               expect_delete_tag_by_name('A')
               expect_delete_tag_by_name('Ba')
@@ -60,26 +82,29 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
             end
 
             it 'succeeds when tag delete returns 404' do
-              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A")
-                .to_return(status: 200, body: "")
-
-              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba")
-                .to_return(status: 404, body: "")
+              stub_delete_reference_request('A')
+              stub_delete_reference_request('Ba', 404)
 
               is_expected.to include(status: :success)
             end
 
+            it_behaves_like 'logging a success response' do
+              before do
+                stub_delete_reference_request('A')
+                stub_delete_reference_request('Ba')
+              end
+            end
+
             context 'with failures' do
               context 'when the delete request fails' do
                 before do
-                  stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A")
-                  .to_return(status: 500, body: "")
-
-                  stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba")
-                  .to_return(status: 500, body: "")
+                  stub_delete_reference_request('A', 500)
+                  stub_delete_reference_request('Ba', 500)
                 end
 
                 it { is_expected.to include(status: :error) }
+
+                it_behaves_like 'logging an error response'
               end
             end
           end
@@ -104,19 +129,35 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
             end
           end
         end
+
         context 'and the feature is disabled' do
+          let_it_be(:tags) { %w[A Ba] }
+
           before do
             stub_feature_flags(container_registry_fast_tag_delete: false)
+            stub_upload("{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+            stub_put_manifest_request('A')
+            stub_put_manifest_request('Ba')
           end
 
           it 'fallbacks to slow delete' do
             expect(service).not_to receive(:fast_delete)
-            expect(service).to receive(:slow_delete).with(repository, tags)
+            expect(service).to receive(:slow_delete).with(repository, tags).and_call_original
+
+            expect_delete_tag_by_digest('sha256:dummy')
 
             subject
           end
+
+          it_behaves_like 'logging a success response' do
+            before do
+              allow(service).to receive(:slow_delete).and_call_original
+              expect_delete_tag_by_digest('sha256:dummy')
+            end
+          end
         end
       end
+
       context 'when the registry does not support fast delete' do
         let_it_be(:project) { create(:project, :private) }
         let_it_be(:repository) { create(:container_repository, :root, project: project) }
@@ -155,11 +196,8 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
           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' })
+            stub_put_manifest_request('A')
+            stub_put_manifest_request('Ba')
 
             expect_delete_tag_by_digest('sha256:dummy')
 
@@ -169,11 +207,8 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
           it 'succeeds when tag delete returns 404' 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' })
+            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: {})
@@ -181,6 +216,15 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
             is_expected.to include(status: :success)
           end
 
+          it_behaves_like 'logging a success response' do
+            before do
+              stub_upload("{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
+              stub_put_manifest_request('A')
+              stub_put_manifest_request('Ba')
+              expect_delete_tag_by_digest('sha256:dummy')
+            end
+          end
+
           context 'with failures' do
             context 'when the dummy manifest generation fails' do
               before do
@@ -188,23 +232,23 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
               end
 
               it { is_expected.to include(status: :error) }
+
+              it_behaves_like 'logging an error response', message: 'could not generate manifest'
             end
 
             context 'when updating the tags fails' do
               before do
                 stub_upload("{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
 
-                stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A")
-                  .to_return(status: 500, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' })
-
-                stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba")
-                  .to_return(status: 500, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' })
+                stub_put_manifest_request('A', 500)
+                stub_put_manifest_request('Ba', 500)
 
                 stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3")
                   .to_return(status: 200, body: "", headers: {})
               end
 
               it { is_expected.to include(status: :error) }
+              it_behaves_like 'logging an error response'
             end
           end
         end
@@ -214,6 +258,16 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
 
   private
 
+  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_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 })