Does not sync job artifact on Object Storage when 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

@@ -4,12 +4,9 @@ module Gitlab
   module Geo
     module Replication
       class BaseDownloader
-        attr_reader :object_type, :object_db_id
+        include ::Gitlab::Utils::StrongMemoize
 
-        def initialize(object_type, object_db_id)
-          @object_type = object_type
-          @object_db_id = object_db_id
-        end
+        attr_reader :object_type, :object_db_id
 
         class Result
           attr_reader :success, :bytes_downloaded, :primary_missing_file, :failed_before_transfer, :reason
@@ -29,8 +26,64 @@ module Gitlab
           end
         end
 
+        def initialize(object_type, object_db_id)
+          @object_type = object_type
+          @object_db_id = object_db_id
+        end
+
+        def execute
+          check_result = check_preconditions
+          return check_result if check_result
+
+          result = if local_store?
+                     transfer.download_from_primary
+                   else
+                     transfer.stream_from_primary_to_object_storage
+                   end
+
+          Result.from_transfer_result(result)
+        end
+
         private
 
+        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")
+          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")
+            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")
+            end
+          end
+
+          nil
+        end
+
+        def local_store?
+          resource.local_store?
+        end
+
+        def resource
+          raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+        end
+
+        def transfer
+          raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+        end
+
+        def object_store_enabled?
+          raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+        end
+
+        def sync_object_storage_enabled?
+          Gitlab::Geo.current_node.sync_object_storage
+        end
+
         def fail_before_transfer(reason: nil)
           Result.new(success: false, bytes_downloaded: 0, reason: reason, failed_before_transfer: true)
         end

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

@@ -9,25 +9,18 @@ module Gitlab
       #   * Returning a detailed Result
       #
       class JobArtifactDownloader < BaseDownloader
-        def execute
-          job_artifact = find_resource
-          return fail_before_transfer unless job_artifact.present?
-
-          transfer = ::Gitlab::Geo::Replication::JobArtifactTransfer.new(job_artifact)
-
-          result = if job_artifact.local_store?
-                     transfer.download_from_primary
-                   else
-                     transfer.stream_from_primary_to_object_storage
-                   end
+        private
 
-          Result.from_transfer_result(result)
+        def resource
+          strong_memoize(:resource) { ::Ci::JobArtifact.find_by_id(object_db_id) }
         end
 
-        private
+        def transfer
+          strong_memoize(:transfer) { ::Gitlab::Geo::Replication::JobArtifactTransfer.new(resource) }
+        end
 
-        def find_resource
-          ::Ci::JobArtifact.find_by_id(object_db_id)
+        def object_store_enabled?
+          ::JobArtifactUploader.object_store_enabled?
         end
       end
     end

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

@@ -9,59 +9,18 @@ module Gitlab
       #   * Returning a detailed Result
       #
       class LfsDownloader < BaseDownloader
-        include ::Gitlab::Utils::StrongMemoize
-
-        def execute
-          check_result = check_preconditions
-          return check_result if check_result
-
-          result = if local_store?
-                     transfer.download_from_primary
-                   else
-                     transfer.stream_from_primary_to_object_storage
-                   end
-
-          Result.from_transfer_result(result)
-        end
-
         private
 
-        def local_store?
-          resource.local_store?
-        end
-
         def resource
-          strong_memoize(:resource) { LfsObject.find_by_id(object_db_id) }
+          strong_memoize(:resource) { ::LfsObject.find_by_id(object_db_id) }
         end
 
         def transfer
           strong_memoize(:transfer) { ::Gitlab::Geo::Replication::LfsTransfer.new(resource) }
         end
 
-        def check_preconditions
-          unless resource.present?
-            return fail_before_transfer(reason: "Skipping transfer as the #{object_type.to_s.capitalize} (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")
-            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.capitalize} on Object Storage")
-            end
-          end
-
-          nil
-        end
-
         def object_store_enabled?
-          LfsObjectUploader.object_store_enabled?
-        end
-
-        def sync_object_storage_enabled?
-          Gitlab::Geo.current_node.sync_object_storage
+          ::LfsObjectUploader.object_store_enabled?
         end
       end
     end

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

@@ -3,31 +3,98 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Geo::Replication::JobArtifactDownloader, :geo do
-  let(:job_artifact) { create(:ci_job_artifact) }
+  include ::EE::GeoHelpers
 
   describe '#execute' do
+    let_it_be(:secondary, reload: true) { create(:geo_node) }
+
+    before do
+      stub_current_geo_node(secondary)
+    end
+
     context 'with job artifact' do
-      it 'returns a FileDownloader::Result object' do
-        downloader = described_class.new(:job_artifact, job_artifact.id)
-        result = Gitlab::Geo::Replication::BaseTransfer::Result.new(success: true, bytes_downloaded: 1)
+      context 'on local storage' do
+        let(:job_artifact) { create(:ci_job_artifact) }
+
+        subject(:downloader) { described_class.new(:job_artifact, job_artifact.id) }
+
+        it 'downloads the job artifact from the primary' do
+          result = Gitlab::Geo::Replication::BaseTransfer::Result.new(success: true, bytes_downloaded: 1)
+
+          expect_next_instance_of(Gitlab::Geo::Replication::JobArtifactTransfer) do |instance|
+            expect(instance).to receive(:download_from_primary).and_return(result)
+          end
+
+          expect(downloader.execute).to have_attributes(success: true, bytes_downloaded: 1)
+        end
+      end
+
+      context 'on object storage' do
+        before do
+          stub_artifacts_object_storage
+        end
+
+        let!(:job_artifact) { create(:ci_job_artifact, :remote_store) }
 
-        allow_next_instance_of(Gitlab::Geo::Replication::JobArtifactTransfer) do |instance|
-          allow(instance).to receive(:download_from_primary).and_return(result)
+        subject(:downloader) { described_class.new(:job_artifact, job_artifact.id) }
+
+        it 'streams the job artifact file from the primary to object storage' do
+          result = Gitlab::Geo::Replication::BaseTransfer::Result.new(success: true, bytes_downloaded: 1)
+
+          expect_next_instance_of(Gitlab::Geo::Replication::JobArtifactTransfer) do |instance|
+            expect(instance).to receive(:stream_from_primary_to_object_storage).and_return(result)
+          end
+
+          expect(downloader.execute).to have_attributes(success: true, bytes_downloaded: 1)
+        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
 
-        expect(downloader.execute).to be_a(Gitlab::Geo::Replication::FileDownloader::Result)
+        context 'with object storage disabled' do
+          before do
+            stub_artifacts_object_storage(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 job artifact on Object Storage'
+            )
+          end
+        end
       end
     end
 
-    context 'with unknown job artifact' do
-      let(:downloader) { described_class.new(:job_artifact, 10000) }
+    context 'with unknown object ID' do
+      let(:unknown_id) { Ci::JobArtifact.maximum(:id).to_i + 1 }
 
-      it 'returns a FileDownloader::Result object' do
-        expect(downloader.execute).to be_a(Gitlab::Geo::Replication::FileDownloader::Result)
-      end
+      subject(:downloader) { described_class.new(:job_artifact, unknown_id) }
 
       it 'returns a result indicating a failure before a transfer was attempted' do
-        expect(downloader.execute.failed_before_transfer).to be_truthy
+        result = downloader.execute
+
+        expect(result).to have_attributes(
+          success: false,
+          failed_before_transfer: true,
+          reason: "Skipping transfer as the job artifact (ID = #{unknown_id}) could not be found"
+        )
       end
     end
   end

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

@@ -75,7 +75,7 @@ RSpec.describe Gitlab::Geo::Replication::LfsDownloader, :geo do
             expect(result).to have_attributes(
               success: false,
               failed_before_transfer: true,
-              reason: 'Skipping transfer as this secondary node is not configured to store Lfs on Object Storage'
+              reason: 'Skipping transfer as this secondary node is not configured to store lfs on Object Storage'
             )
           end
         end
@@ -93,7 +93,7 @@ RSpec.describe Gitlab::Geo::Replication::LfsDownloader, :geo do
         expect(result).to have_attributes(
           success: false,
           failed_before_transfer: true,
-          reason: "Skipping transfer as the Lfs (ID = #{unknown_id}) could not be found"
+          reason: "Skipping transfer as the lfs (ID = #{unknown_id}) could not be found"
         )
       end
     end