Merge branch 'ab-205166-status-page-unpublish-images' into 'master'

Add status page unpublish logic for images

See merge request gitlab-org/gitlab!30298

ee/app/services/status_page/publish_base_service.rb

@@ -53,10 +53,12 @@ module StatusPage
       success(object_key: key)
     end
 
-    def delete(key)
+    def delete_object(key)
       storage_client.delete_object(key)
+    end
 
-      success(object_key: key)
+    def recursive_delete(prefix)
+      storage_client.recursive_delete(prefix)
     end
 
     def limit_exceeded?(json)

ee/app/services/status_page/unpublish_details_service.rb

@@ -13,12 +13,21 @@ module StatusPage
     private
 
     def process(issue)
-      key = object_key(issue)
+      # Delete the incident prior to deleting images to avoid broken links
+      json_key = json_object_key(issue)
+      delete_object(json_key)
 
-      delete(key)
+      upload_keys_prefix = uploads_path(issue)
+      recursive_delete(upload_keys_prefix)
+
+      success(object_key: json_key)
+    end
+
+    def uploads_path(issue)
+      StatusPage::Storage.uploads_path(issue.iid)
     end
 
-    def object_key(issue)
+    def json_object_key(issue)
       StatusPage::Storage.details_path(issue.iid)
     end
   end

ee/lib/status_page/storage/s3_client.rb

@@ -45,7 +45,7 @@ module StatusPage
 
             objects = response.contents.map { |obj| { key: obj.key } }
             # Batch delete in sets determined by default max_key argument that can be passed to list_objects_v2
-            client.delete_objects({ bucket: bucket_name, delete: { objects: objects } })
+            client.delete_objects({ bucket: bucket_name, delete: { objects: objects } }) unless objects.empty?
           end
         end
 

ee/spec/lib/status_page/storage/s3_client_spec.rb

@@ -96,6 +96,16 @@ describe StatusPage::Storage::S3Client, :aws_s3 do
       end
     end
 
+    context 'when list_object returns no objects' do
+      include_context 'no objects list_objects_v2 result'
+
+      it 'does not attempt to delete' do
+        expect(aws_client).not_to receive(:delete_objects).with(delete_objects_data(key_list_no_objects))
+
+        result
+      end
+    end
+
     context 'when failed' do
       let(:aws_error) { 'SomeError' }
 
@@ -129,6 +139,15 @@ describe StatusPage::Storage::S3Client, :aws_s3 do
       end
     end
 
+    context 'when list_object returns no objects' do
+      include_context 'no objects list_objects_v2 result'
+
+      it 'returns an empty set' do
+        expect(result).to be_an_instance_of(Set)
+        expect(result.empty?).to be(true)
+      end
+    end
+
     context 'when failed' do
       let(:aws_error) { 'SomeError' }
 

ee/spec/services/status_page/publish_service_spec.rb

@@ -45,20 +45,20 @@ describe StatusPage::PublishService do
     describe 'unpublish details' do
       let_it_be(:issue) { create(:issue, :confidential, project: project) }
 
-      context 'when deletion succeeds' do
-        it 'deletes incident details and upload list' do
-          expect_to_delete_details(issue)
+      context 'when unpublish succeeds' do
+        it 'unpublishes incident details and uploads incident list' do
+          expect_to_unpublish(error?: false)
           expect_to_upload_list
 
           expect(result).to be_success
         end
       end
 
-      context 'when deletion fails' do
-        it 'propagates the exception' do
-          expect_to_delete_details(issue, status: 403)
+      context 'when unpublish service responses with error' do
+        it 'returns the response' do
+          response = expect_to_unpublish(error?: true)
 
-          expect { result }.to raise_error(StatusPage::Storage::Error)
+          expect(result).to be(response)
         end
       end
     end
@@ -95,6 +95,15 @@ describe StatusPage::PublishService do
 
   private
 
+  def expect_to_unpublish(**response_kwargs)
+    service_response = double(**response_kwargs)
+    expect_next_instance_of(StatusPage::UnpublishDetailsService) do |service|
+      expect(service).to receive(:execute).and_return(service_response)
+    end
+
+    service_response
+  end
+
   def expect_to_upload_details(issue, **kwargs)
     stub_aws_request(:put, StatusPage::Storage.details_path(issue.iid), **kwargs)
   end

ee/spec/services/status_page/unpublish_details_service_spec.rb

@@ -7,6 +7,7 @@ describe StatusPage::UnpublishDetailsService do
   let(:issue) { instance_double(Issue, iid: incident_id) }
   let(:incident_id) { 1 }
   let(:key) { StatusPage::Storage.details_path(incident_id) }
+  let(:image_uploads_path) { StatusPage::Storage.uploads_path(issue.iid) }
 
   let(:service) { described_class.new(project: project) }
 
@@ -31,15 +32,23 @@ describe StatusPage::UnpublishDetailsService do
     context 'when deletion succeeds' do
       before do
         allow(storage_client).to receive(:delete_object).with(key)
+        allow(storage_client).to receive(:recursive_delete).with(image_uploads_path)
       end
 
-      it 'removes details from CDN' do
+      it 'removes files from the CDN (incident first)' do
+        expect(storage_client).to receive(:delete_object).ordered
+        expect(storage_client).to receive(:recursive_delete).with(image_uploads_path).ordered
+
+        result
+      end
+
+      it 'returns service success' do
         expect(result).to be_success
         expect(result.payload).to eq(object_key: key)
       end
     end
 
-    context 'when upload fails due to exception' do
+    context 'when delete fails due to exception' do
       let(:bucket) { 'bucket_name' }
       let(:error) { StandardError.new }
 
@@ -47,13 +56,28 @@ describe StatusPage::UnpublishDetailsService do
         StatusPage::Storage::Error.new(bucket: bucket, error: error)
       end
 
-      before do
-        allow(storage_client).to receive(:delete_object).with(key)
-          .and_raise(exception)
+      context 'when json delete fails' do
+        before do
+          allow(storage_client).to receive(:delete_object).with(key)
+            .and_raise(exception)
+          allow(storage_client).to receive(:recursive_delete)
+        end
+
+        it 'propagates the exception' do
+          expect { result }.to raise_error(exception)
+        end
       end
 
-      it 'propagates the exception' do
-        expect { result }.to raise_error(exception)
+      context 'when image delete fails' do
+        before do
+          allow(storage_client).to receive(:delete_object)
+          allow(storage_client).to receive(:recursive_delete).with(image_uploads_path)
+            .and_raise(exception)
+        end
+
+        it 'propagates the exception' do
+          expect { result }.to raise_error(exception)
+        end
       end
     end
 

ee/spec/support/shared_contexts/status_page/status_page_list_objects.rb

@@ -36,3 +36,14 @@ RSpec.shared_context 'oversized list_objects_v2 result' do
     (0...desired_size).to_a.map { |_| SecureRandom.hex }
   end
 end
+
+RSpec.shared_context 'no objects list_objects_v2 result' do
+  let(:key_list_no_objects) { [] }
+
+  before do
+    stub_responses(
+      :list_objects_v2,
+      list_objects_data(key_list: key_list_no_objects, next_continuation_token: nil, is_truncated: false)
+    )
+  end
+end