Allow disk access if we can access storage directlyjc-allow-disk-access

Move the methods to detect if we can access disk directly into
StorageSettings. This reduces complexity by no longer needing
synchronization, as well as being able to bypass the disk access check.

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