17 files changed, 96 insertions, 79 deletions

diff --git a/changelogs/unreleased/jc-allow-disk-access.yml b/changelogs/unreleased/jc-allow-disk-access.yml
new file mode 100644
index 00000000000..aac53c93423
--- /dev/null
+++ b/changelogs/unreleased/jc-allow-disk-access.yml
@@ -0,0 +1,5 @@
+---
+title: Allow rugged auto detect code to bypass disk access check
+merge_request: 30530
+author:
+type: other
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 0b8a6607250..7198a0a9202 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -531,7 +531,7 @@ unless Settings.repositories.storages['default']
 end
 
 Settings.repositories.storages.each do |key, storage|
-  Settings.repositories.storages[key] = Gitlab::GitalyClient::StorageSettings.new(storage)
+  Settings.repositories.storages[key] = Gitlab::GitalyClient::StorageSettings.new(key, storage)
 end
 
 #
diff --git a/lib/gitlab/git/rugged_impl/blob.rb b/lib/gitlab/git/rugged_impl/blob.rb
index 86c9f33d82a..11d2b8001b8 100644
--- a/lib/gitlab/git/rugged_impl/blob.rb
+++ b/lib/gitlab/git/rugged_impl/blob.rb
@@ -15,7 +15,7 @@ module Gitlab
 
           override :tree_entry
           def tree_entry(repository, sha, path, limit)
-            if use_rugged?(repository, :rugged_tree_entry)
+            if use_rugged?(repository.storage, :rugged_tree_entry)
               rugged_tree_entry(repository, sha, path, limit)
             else
               super
diff --git a/lib/gitlab/git/rugged_impl/commit.rb b/lib/gitlab/git/rugged_impl/commit.rb
index 971a33b2e99..7aa883dd1ac 100644
--- a/lib/gitlab/git/rugged_impl/commit.rb
+++ b/lib/gitlab/git/rugged_impl/commit.rb
@@ -35,7 +35,7 @@ module Gitlab
 
           override :find_commit
           def find_commit(repo, commit_id)
-            if use_rugged?(repo, :rugged_find_commit)
+            if use_rugged?(repo.storage, :rugged_find_commit)
               rugged_find(repo, commit_id)
             else
               super
@@ -44,7 +44,7 @@ module Gitlab
 
           override :batch_by_oid
           def batch_by_oid(repo, oids)
-            if use_rugged?(repo, :rugged_list_commits_by_oid)
+            if use_rugged?(repo.storage, :rugged_list_commits_by_oid)
               rugged_batch_by_oid(repo, oids)
             else
               super
@@ -67,7 +67,7 @@ module Gitlab
 
         override :commit_tree_entry
         def commit_tree_entry(path)
-          if use_rugged?(@repository, :rugged_commit_tree_entry)
+          if use_rugged?(@repository.storage, :rugged_commit_tree_entry)
             rugged_tree_entry(path)
           else
             super
diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb
index 9268abdfed9..367f2a24cd2 100644
--- a/lib/gitlab/git/rugged_impl/repository.rb
+++ b/lib/gitlab/git/rugged_impl/repository.rb
@@ -47,7 +47,7 @@ module Gitlab
 
         override :ancestor?
         def ancestor?(from, to)
-          if use_rugged?(self, :rugged_commit_is_ancestor)
+          if use_rugged?(self.storage, :rugged_commit_is_ancestor)
             rugged_is_ancestor?(from, to)
           else
             super
diff --git a/lib/gitlab/git/rugged_impl/tree.rb b/lib/gitlab/git/rugged_impl/tree.rb
index f3721a3f1b7..baa6866546c 100644
--- a/lib/gitlab/git/rugged_impl/tree.rb
+++ b/lib/gitlab/git/rugged_impl/tree.rb
@@ -15,7 +15,7 @@ module Gitlab
 
           override :tree_entries
           def tree_entries(repository, sha, path, recursive)
-            if use_rugged?(repository, :rugged_tree_entries)
+            if use_rugged?(repository.storage, :rugged_tree_entries)
               tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive)
             else
               super
diff --git a/lib/gitlab/git/rugged_impl/use_rugged.rb b/lib/gitlab/git/rugged_impl/use_rugged.rb
index 99091b03cd1..3a70cf32691 100644
--- a/lib/gitlab/git/rugged_impl/use_rugged.rb
+++ b/lib/gitlab/git/rugged_impl/use_rugged.rb
@@ -4,11 +4,11 @@ module Gitlab
   module Git
     module RuggedImpl
       module UseRugged
-        def use_rugged?(repo, feature_key)
+        def use_rugged?(storage_name, feature_key)
           feature = Feature.get(feature_key)
           return feature.enabled? if Feature.persisted?(feature)
 
-          Gitlab::GitalyClient.can_use_disk?(repo.storage)
+          Gitlab.config.repositories.storages[storage_name].can_use_disk?
         end
       end
     end
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index cc9503fb6de..9e3de910e3c 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -30,7 +30,6 @@ module Gitlab
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'
     MAXIMUM_GITALY_CALLS = 30
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
-    GITALY_METADATA_FILENAME = '.gitaly-metadata'
 
     MUTEX = Mutex.new
 
@@ -379,46 +378,6 @@ module Gitlab
       0
     end
 
-    def self.storage_metadata_file_path(storage)
-      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-        File.join(
-          Gitlab.config.repositories.storages[storage].legacy_disk_path, GITALY_METADATA_FILENAME
-        )
-      end
-    end
-
-    def self.can_use_disk?(storage)
-      false
-      # cached_value = MUTEX.synchronize do
-      #   @can_use_disk ||= {}
-      #   @can_use_disk[storage]
-      # end
-
-      # return cached_value unless cached_value.nil?
-
-      # gitaly_filesystem_id = filesystem_id(storage)
-      # direct_filesystem_id = filesystem_id_from_disk(storage)
-
-      # MUTEX.synchronize do
-      #   @can_use_disk[storage] = gitaly_filesystem_id.present? &&
-      #     gitaly_filesystem_id == direct_filesystem_id
-      # end
-    end
-
-    def self.filesystem_id(storage)
-      response = Gitlab::GitalyClient::ServerService.new(storage).info
-      storage_status = response.storage_statuses.find { |status| status.storage_name == storage }
-      storage_status.filesystem_id
-    end
-
-    def self.filesystem_id_from_disk(storage)
-      metadata_file = File.read(storage_metadata_file_path(storage))
-      metadata_hash = JSON.parse(metadata_file)
-      metadata_hash['gitaly_filesystem_id']
-    rescue Errno::ENOENT, JSON::ParserError
-      nil
-    end
-
     def self.timeout(timeout_name)
       Gitlab::CurrentSettings.current_application_settings[timeout_name]
     end
diff --git a/lib/gitlab/gitaly_client/storage_settings.rb b/lib/gitlab/gitaly_client/storage_settings.rb
index 7d1206e551b..0aae77787b6 100644
--- a/lib/gitlab/gitaly_client/storage_settings.rb
+++ b/lib/gitlab/gitaly_client/storage_settings.rb
@@ -25,16 +25,18 @@ module Gitlab
 
       DISK_ACCESS_DENIED_FLAG = :deny_disk_access
       ALLOW_KEY = :allow_disk_access
+      GITALY_METADATA_FILENAME = '.gitaly-metadata'
 
       # If your code needs this method then your code needs to be fixed.
       def self.allow_disk_access
         temporarily_allow(ALLOW_KEY) { yield }
       end
 
-      def self.disk_access_denied?
-        return false if rugged_enabled?
+      def disk_access_denied?
+        return false if self.class.rugged_enabled?
+        return false if can_use_disk?
 
-        !temporarily_allowed?(ALLOW_KEY) && Feature::Gitaly.enabled?(DISK_ACCESS_DENIED_FLAG)
+        !self.class.temporarily_allowed?(ALLOW_KEY) && Feature::Gitaly.enabled?(DISK_ACCESS_DENIED_FLAG)
       rescue
         false # Err on the side of caution, don't break gitlab for people
       end
@@ -45,7 +47,7 @@ module Gitlab
         end
       end
 
-      def initialize(storage)
+      def initialize(name, storage)
         raise InvalidConfigurationError, "expected a Hash, got a #{storage.class.name}" unless storage.is_a?(Hash)
         raise InvalidConfigurationError, INVALID_STORAGE_MESSAGE unless storage.has_key?('path')
 
@@ -54,6 +56,7 @@ module Gitlab
 
         storage['path'] = Deprecated
         @hash = storage
+        @name = name
       end
 
       def gitaly_address
@@ -61,18 +64,44 @@ module Gitlab
       end
 
       def legacy_disk_path
-        if self.class.disk_access_denied?
+        if disk_access_denied?
           raise DirectPathAccessError, "git disk access denied via the gitaly_#{DISK_ACCESS_DENIED_FLAG} feature"
         end
 
         @legacy_disk_path
       end
 
+      def can_use_disk?
+        return @can_use_disk unless @can_use_disk.nil?
+
+        gitaly_filesystem_id = filesystem_id
+
+        @can_use_disk = gitaly_filesystem_id.present? && filesystem_id == filesystem_id_from_disk
+      end
+
       private
 
       def method_missing(msg, *args, &block)
         @hash.public_send(msg, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
       end
+
+      def filesystem_id
+        response = Gitlab::GitalyClient::ServerService.new(@name).info
+        storage_status = response.storage_statuses.find { |status| status.storage_name == @name }
+        storage_status.filesystem_id
+      end
+
+      def filesystem_id_from_disk
+        metadata_file = File.read(storage_metadata_file_path)
+        metadata_hash = JSON.parse(metadata_file)
+        metadata_hash['gitaly_filesystem_id']
+      rescue Errno::ENOENT, JSON::ParserError
+        nil
+      end
+
+      def storage_metadata_file_path
+        File.join(@legacy_disk_path, GITALY_METADATA_FILENAME)
+      end
     end
   end
 end
diff --git a/spec/initializers/6_validations_spec.rb b/spec/initializers/6_validations_spec.rb
index 73fbd4c7a44..ec17e9ce1c9 100644
--- a/spec/initializers/6_validations_spec.rb
+++ b/spec/initializers/6_validations_spec.rb
@@ -5,7 +5,7 @@ describe '6_validations' do
   describe 'validate_storages_config' do
     context 'with correct settings' do
       before do
-        mock_storages('foo' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/paths/a/b/c'), 'bar' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/paths/a/b/d'))
+        mock_storages('foo' => Gitlab::GitalyClient::StorageSettings.new('foo', 'path' => 'tmp/tests/paths/a/b/c'), 'bar' => Gitlab::GitalyClient::StorageSettings.new('foo', 'path' => 'tmp/tests/paths/a/b/d'))
       end
 
       it 'passes through' do
@@ -15,7 +15,7 @@ describe '6_validations' do
 
     context 'with invalid storage names' do
       before do
-        mock_storages('name with spaces' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/paths/a/b/c'))
+        mock_storages('name with spaces' => Gitlab::GitalyClient::StorageSettings.new('foo', 'path' => 'tmp/tests/paths/a/b/c'))
       end
 
       it 'throws an error' do
diff --git a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
index e7ef9d08f80..8d18eb7073a 100644
--- a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
+++ b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
@@ -7,6 +7,8 @@ require 'tempfile'
 describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
+  let(:storage_name) { repository.storage }
+  let(:storage) { Gitlab.config.repositories.storages[storage_name] }
   let(:feature_flag_name) { 'feature-flag-name' }
   let(:feature_flag) { Feature.get(feature_flag_name) }
   let(:temp_gitaly_metadata_file) { create_temporary_gitaly_metadata_file }
@@ -21,7 +23,8 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
   end
 
   before do
-    Gitlab::GitalyClient.instance_variable_set(:@can_use_disk, {})
+    # reset the cached value before each spec
+    storage.instance_variable_set(:@can_use_disk, nil)
   end
 
   context 'when feature flag is not persisted' do
@@ -30,31 +33,29 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
     end
 
     it 'returns true when gitaly matches disk' do
-      pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338')
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be true
     end
 
     it 'returns false when disk access fails' do
-      allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist")
+      allow(storage).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist")
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be false
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be false
     end
 
     it "returns false when gitaly doesn't match disk" do
-      allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file)
+      allow(storage).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file)
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be_falsey
 
       File.delete(temp_gitaly_metadata_file)
     end
 
     it "doesn't lead to a second rpc call because gitaly client should use the cached value" do
-      pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338')
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be true
 
       expect(Gitlab::GitalyClient).not_to receive(:filesystem_id)
 
-      subject.use_rugged?(repository, feature_flag_name)
+      subject.use_rugged?(storage_name, feature_flag_name)
     end
   end
 
@@ -66,14 +67,14 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
     it 'returns false when the feature flag is off' do
       allow(feature_flag).to receive(:enabled?).and_return(false)
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be_falsey
     end
 
     it "returns true when feature flag is on" do
       allow(feature_flag).to receive(:enabled?).and_return(true)
       allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(false)
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be true
     end
   end
 
diff --git a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb
index f2f53982b09..bc774532c09 100644
--- a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::GitalyClient::StorageSettings do
     context 'when the storage contains no path' do
       it 'raises an error' do
         expect do
-          described_class.new("foo" => {})
+          described_class.new("default", "foo" => {})
         end.to raise_error(described_class::InvalidConfigurationError)
       end
     end
@@ -13,7 +13,7 @@ describe Gitlab::GitalyClient::StorageSettings do
     context "when the argument isn't a hash" do
       it 'raises an error' do
         expect do
-          described_class.new("test")
+          described_class.new("default", "test")
         end.to raise_error("expected a Hash, got a String")
       end
     end
@@ -21,22 +21,39 @@ describe Gitlab::GitalyClient::StorageSettings do
     context 'when the storage is valid' do
       it 'raises no error' do
         expect do
-          described_class.new("path" => Rails.root)
+          described_class.new("default", "path" => Rails.root)
         end.not_to raise_error
       end
     end
   end
 
   describe '.disk_access_denied?' do
+    let(:storage_name) { "default" }
+    subject { described_class.new(storage_name, "path" => Rails.root) }
+
+    before do
+      allow(subject).to receive(:can_use_disk?).and_return(false)
+    end
+
     context 'when Rugged is enabled', :enable_rugged do
       it 'returns false' do
-        expect(described_class.disk_access_denied?).to be_falsey
+        expect(subject.disk_access_denied?).to be_falsey
       end
     end
 
     context 'when Rugged is disabled' do
       it 'returns true' do
-        expect(described_class.disk_access_denied?).to be_truthy
+        expect(subject.disk_access_denied?).to be_truthy
+      end
+    end
+
+    context 'when disk is accessible' do
+      before do
+        allow(subject).to receive(:can_use_disk?).and_return(true)
+      end
+
+      it 'returns false' do
+        expect(subject.disk_access_denied?).to be_falsey
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 1bc092fa41a..1e7c625ab73 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1321,8 +1321,8 @@ describe Project do
 
     before do
       storages = {
-        'default' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/repositories'),
-        'picked'  => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/repositories')
+        'default' => Gitlab::GitalyClient::StorageSettings.new('default', 'path' => 'tmp/tests/repositories'),
+        'picked'  => Gitlab::GitalyClient::StorageSettings.new('default', 'path' => 'tmp/tests/repositories')
       }
       allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
     end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 62fdc039b5e..2a83bfe133a 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -118,6 +118,12 @@ RSpec.configure do |config|
     TestEnv.init
   end
 
+  config.before(:all) do
+    Gitlab.config.repositories.storages.each do |key, storage|
+      storage.instance_variable_set(:@can_use_disk, false)
+    end
+  end
+
   config.after(:all) do
     TestEnv.clean_test_path
   end
diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb
index c372a3f0e49..93fea87110f 100644
--- a/spec/support/helpers/stub_configuration.rb
+++ b/spec/support/helpers/stub_configuration.rb
@@ -75,7 +75,7 @@ module StubConfiguration
         storage_hash['path'] = TestEnv.repos_path
       end
 
-      messages[storage_name] = Gitlab::GitalyClient::StorageSettings.new(storage_hash.to_h)
+      messages[storage_name] = Gitlab::GitalyClient::StorageSettings.new(storage_name, storage_hash.to_h)
     end
 
     allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb
index bdbd39475b9..033b899394f 100644
--- a/spec/tasks/gitlab/backup_rake_spec.rb
+++ b/spec/tasks/gitlab/backup_rake_spec.rb
@@ -239,7 +239,7 @@ describe 'gitlab:app namespace rake task' do
 
     context 'multiple repository storages' do
       let(:test_second_storage) do
-        Gitlab::GitalyClient::StorageSettings.new(@default_storage_hash.merge('path' => 'tmp/tests/custom_storage'))
+        Gitlab::GitalyClient::StorageSettings.new('default', @default_storage_hash.merge('path' => 'tmp/tests/custom_storage'))
       end
       let(:storages) do
         {
diff --git a/spec/tasks/gitlab/cleanup_rake_spec.rb b/spec/tasks/gitlab/cleanup_rake_spec.rb
index 92c094f08a4..f00e0d17f34 100644
--- a/spec/tasks/gitlab/cleanup_rake_spec.rb
+++ b/spec/tasks/gitlab/cleanup_rake_spec.rb
@@ -10,7 +10,7 @@ describe 'gitlab:cleanup rake tasks' do
     let(:storage) { storages.keys.first }
     let(:storages) do
       {
-        'default' => Gitlab::GitalyClient::StorageSettings.new(@default_storage_hash.merge('path' => 'tmp/tests/default_storage'))
+        'default' => Gitlab::GitalyClient::StorageSettings.new('default', @default_storage_hash.merge('path' => 'tmp/tests/default_storage'))
       }
     end