Does not sync uploads on Object Storage when is not enabled

Add some checks to skip file transfers on a Geo secondary
node when file is stored on Object Storage but syncing
object storage is disabled or the object storage is
not configure for the data type on the secondary node.

ee/lib/gitlab/geo/replication/base_downloader.rb

@@ -48,16 +48,16 @@ module Gitlab
 
         def check_preconditions
           unless resource.present?
-            return fail_before_transfer(reason: "Skipping transfer as the #{object_type.to_s.humanize(capitalize: false)} (ID = #{object_db_id}) could not be found")
+            return skip_transfer_error(reason: "Skipping transfer as the #{object_type.to_s.humanize(capitalize: false)} (ID = #{object_db_id}) could not be found")
           end
 
           unless local_store?
             unless sync_object_storage_enabled?
-              return fail_before_transfer(reason: "Skipping transfer as this secondary node is not allowed to replicate content on Object Storage")
+              return skip_transfer_error(reason: "Skipping transfer as this secondary node is not allowed to replicate content on Object Storage")
             end
 
             unless object_store_enabled?
-              return fail_before_transfer(reason: "Skipping transfer as this secondary node is not configured to store #{object_type.to_s.humanize(capitalize: false)} on Object Storage")
+              return skip_transfer_error(reason: "Skipping transfer as this secondary node is not configured to store #{object_type.to_s.humanize(capitalize: false)} on Object Storage")
             end
           end
 
@@ -84,11 +84,11 @@ module Gitlab
           Gitlab::Geo.current_node.sync_object_storage
         end
 
-        def fail_before_transfer(reason: nil)
+        def skip_transfer_error(reason: nil)
           Result.new(success: false, bytes_downloaded: 0, reason: reason, failed_before_transfer: true)
         end
 
-        def missing_on_primary
+        def missing_on_primary_error
           Result.new(success: true, bytes_downloaded: 0, primary_missing_file: true)
         end
       end

ee/lib/gitlab/geo/replication/file_downloader.rb

@@ -9,30 +9,28 @@ module Gitlab
       #   * Returning a detailed Result
       #
       class FileDownloader < BaseDownloader
-        # Executes the actual file download
-        #
-        # Subclasses should return the number of bytes downloaded,
-        # or nil or -1 if a failure occurred.
-        def execute
-          upload = find_resource
-          return fail_before_transfer unless upload.present?
-          return missing_on_primary if upload.model.nil?
+        private
 
-          transfer = ::Gitlab::Geo::Replication::FileTransfer.new(object_type.to_sym, upload)
+        def check_preconditions
+          return missing_on_primary_error if resource && resource.model.nil?
 
-          result = if upload.local?
-                     transfer.download_from_primary
-                   else
-                     transfer.stream_from_primary_to_object_storage
-                   end
+          super
+        end
 
-          Result.from_transfer_result(result)
+        def local_store?
+          resource.local?
         end
 
-        private
+        def resource
+          strong_memoize(:resource) { ::Upload.find_by_id(object_db_id) }
+        end
+
+        def transfer
+          strong_memoize(:transfer) { ::Gitlab::Geo::Replication::FileTransfer.new(object_type.to_sym, resource) }
+        end
 
-        def find_resource
-          Upload.find_by_id(object_db_id)
+        def object_store_enabled?
+          ::FileUploader.object_store_enabled?
         end
       end
     end

ee/spec/lib/gitlab/geo/replication/file_downloader_spec.rb

@@ -5,37 +5,116 @@ require 'spec_helper'
 RSpec.describe Gitlab::Geo::Replication::FileDownloader, :geo do
   include EE::GeoHelpers
 
-  let_it_be(:primary_node) { create(:geo_node, :primary) }
+  describe '#execute' do
+    let_it_be(:primary_node) { create(:geo_node, :primary) }
+    let_it_be(:secondary, reload: true) { create(:geo_node) }
 
-  subject { downloader.execute }
+    before do
+      stub_current_geo_node(secondary)
+    end
 
-  let(:upload) { create(:upload, :issuable_upload, :with_file) }
-  let(:downloader) { described_class.new(:file, upload.id) }
+    context 'with upload' do
+      context 'on local storage' do
+        let(:upload) { create(:upload, :with_file) }
 
-  context 'when in primary geo node' do
-    before do
-      stub_current_geo_node(primary_node)
+        subject(:downloader) { described_class.new(:avatar, upload.id) }
+
+        it 'downloads the file from the primary' do
+          stub_geo_file_transfer(:avatar, upload)
+
+          expect_next_instance_of(Gitlab::Geo::Replication::FileTransfer) do |instance|
+            expect(instance).to receive(:download_from_primary).and_call_original
+          end
+
+          expect(downloader.execute).to have_attributes(success: true)
+        end
+      end
+
+      context 'on object storage' do
+        before do
+          stub_uploads_object_storage(AvatarUploader, direct_upload: true)
+        end
+
+        let!(:upload) { create(:upload, :object_storage) }
+
+        subject(:downloader) { described_class.new(:avatar, upload.id) }
+
+        it 'streams the upload file from the primary to object storage' do
+          stub_geo_file_transfer_object_storage(:avatar, upload)
+
+          expect_next_instance_of(Gitlab::Geo::Replication::FileTransfer) do |instance|
+            expect(instance).to receive(:stream_from_primary_to_object_storage).and_call_original
+          end
+
+          expect(downloader.execute).to have_attributes(success: true)
+        end
+
+        context 'with object storage sync disabled' do
+          before do
+            secondary.update_column(:sync_object_storage, false)
+          end
+
+          it 'returns a result indicating a failure before a transfer was attempted' do
+            result = downloader.execute
+
+            expect(result).to have_attributes(
+              success: false,
+              failed_before_transfer: true,
+              reason: 'Skipping transfer as this secondary node is not allowed to replicate content on Object Storage'
+            )
+          end
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(AvatarUploader, enabled: false)
+          end
+
+          it 'returns a result indicating a failure before a transfer was attempted' do
+            result = downloader.execute
+
+            expect(result).to have_attributes(
+              success: false,
+              failed_before_transfer: true,
+              reason: 'Skipping transfer as this secondary node is not configured to store avatar on Object Storage'
+            )
+          end
+        end
+      end
     end
 
-    it 'fails to download the file' do
-      expect(subject.success).to be_falsey
-      expect(subject.primary_missing_file).to be_falsey
+    context 'with unknown object ID' do
+      let(:unknown_id) { Upload.maximum(:id).to_i + 1 }
+
+      subject(:downloader) { described_class.new(:avatar, unknown_id) }
+
+      it 'returns a result indicating a failure before a transfer was attempted' do
+        result = downloader.execute
+
+        expect(result).to have_attributes(
+          success: false,
+          failed_before_transfer: true,
+          reason: "Skipping transfer as the avatar (ID = #{unknown_id}) could not be found"
+        )
+      end
     end
-  end
 
-  context 'when in a secondary geo node' do
-    context 'with local storage only' do
-      let(:secondary_node) { create(:geo_node, :local_storage_only) }
+    context 'when the upload parent object does not exist' do
+      let(:upload) { create(:upload) }
 
-      before do
-        stub_current_geo_node(secondary_node)
+      subject(:downloader) { described_class.new(:avatar, upload.id) }
 
-        stub_geo_file_transfer(:file, upload)
+      before do
+        upload.update_columns(model_id: nil, model_type: nil)
       end
 
-      it 'downloads the file' do
-        expect(subject.success).to be_truthy
-        expect(subject.primary_missing_file).to be_falsey
+      it 'returns a result indicating a failure before a transfer was attempted' do
+        result = downloader.execute
+
+        expect(result).to have_attributes(
+          success: true,
+          primary_missing_file: true # FIXME: https://gitlab.com/gitlab-org/gitlab/-/issues/220855
+        )
       end
     end
   end
@@ -48,7 +127,10 @@ RSpec.describe Gitlab::Geo::Replication::FileDownloader, :geo do
 
   def stub_geo_file_transfer_object_storage(file_type, upload)
     url = primary_node.geo_transfers_url(file_type, upload.id.to_s)
+    redirection = upload.retrieve_uploader.url
+    file = fixture_file_upload('spec/fixtures/dk.png')
 
-    stub_request(:get, url).to_return(status: 307, body: upload.retrieve_uploader.url, headers: {})
+    stub_request(:get, url).to_return(status: 307, headers: { location: redirection })
+    stub_request(:get, redirection).to_return(status: 200, body: file.read, headers: {})
   end
 end