Merge branch 'gitaly-tripswitch' into 'master'

Add "deny disk access" Gitaly feature (tripswitch)

Closes gitaly#1210

See merge request gitlab-org/gitlab-ce!19149

8 files changed, 100 insertions, 22 deletions

diff --git a/lib/gitlab/cycle_analytics/summary/commit.rb b/lib/gitlab/cycle_analytics/summary/commit.rb
index bea78862757..0a88e052f60 100644
--- a/lib/gitlab/cycle_analytics/summary/commit.rb
+++ b/lib/gitlab/cycle_analytics/summary/commit.rb
@@ -7,7 +7,9 @@ module Gitlab
         end
 
         def value
-          @value ||= count_commits
+          Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+            @value ||= count_commits
+          end
         end
 
         private
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 1a21625a322..4cbf20bfe76 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1185,15 +1185,17 @@ module Gitlab
       end
 
       def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
-        with_repo_branch_commit(source_repository, source_branch_name) do |commit|
-          break unless commit
-
-          Gitlab::Git::Compare.new(
-            self,
-            target_branch_name,
-            commit.sha,
-            straight: straight
-          )
+        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+          with_repo_branch_commit(source_repository, source_branch_name) do |commit|
+            break unless commit
+
+            Gitlab::Git::Compare.new(
+              self,
+              target_branch_name,
+              commit.sha,
+              straight: straight
+            )
+          end
         end
       end
 
@@ -1455,7 +1457,7 @@ module Gitlab
           gitaly_repository_client.cleanup if is_enabled && exists?
         end
       rescue Gitlab::Git::CommandError => e # Don't fail if we can't cleanup
-        Rails.logger.error("Unable to clean repository on storage #{storage} with path #{path}: #{e.message}")
+        Rails.logger.error("Unable to clean repository on storage #{storage} with relative path #{relative_path}: #{e.message}")
         Gitlab::Metrics.counter(
           :failed_repository_cleanup_total,
           'Number of failed repository cleanup events'
diff --git a/lib/gitlab/git/storage/checker.rb b/lib/gitlab/git/storage/checker.rb
index 2f611cef37b..391f0d70583 100644
--- a/lib/gitlab/git/storage/checker.rb
+++ b/lib/gitlab/git/storage/checker.rb
@@ -35,7 +35,7 @@ module Gitlab
         def initialize(storage, logger = Rails.logger)
           @storage = storage
           config = Gitlab.config.repositories.storages[@storage]
-          @storage_path = config.legacy_disk_path
+          @storage_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { config.legacy_disk_path }
           @logger = logger
 
           @hostname = Gitlab::Environment.hostname
diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb
index e35054466ff..62427ac9cc4 100644
--- a/lib/gitlab/git/storage/circuit_breaker.rb
+++ b/lib/gitlab/git/storage/circuit_breaker.rb
@@ -22,13 +22,14 @@ module Gitlab
 
         def self.build(storage, hostname = Gitlab::Environment.hostname)
           config = Gitlab.config.repositories.storages[storage]
-
-          if !config.present?
-            NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Storage '#{storage}' is not configured"))
-          elsif !config.legacy_disk_path.present?
-            NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Path for storage '#{storage}' is not configured"))
-          else
-            new(storage, hostname)
+          Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+            if !config.present?
+              NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Storage '#{storage}' is not configured"))
+            elsif !config.legacy_disk_path.present?
+              NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Path for storage '#{storage}' is not configured"))
+            else
+              new(storage, hostname)
+            end
           end
         end
 
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 0abae70c443..550294916a4 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -33,6 +33,11 @@ module Gitlab
     MAXIMUM_GITALY_CALLS = 35
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
 
+    # We have a mechanism to let GitLab automatically opt in to all Gitaly
+    # features. We want to be able to exclude some features from automatic
+    # opt-in. That is what EXPLICIT_OPT_IN_REQUIRED is for.
+    EXPLICIT_OPT_IN_REQUIRED = [Gitlab::GitalyClient::StorageSettings::DISK_ACCESS_DENIED_FLAG].freeze
+
     MUTEX = Mutex.new
 
     class << self
@@ -234,7 +239,7 @@ module Gitlab
       when MigrationStatus::OPT_OUT
         true
       when MigrationStatus::OPT_IN
-        opt_into_all_features?
+        opt_into_all_features? && !EXPLICIT_OPT_IN_REQUIRED.include?(feature_name)
       else
         false
       end
diff --git a/lib/gitlab/gitaly_client/storage_settings.rb b/lib/gitlab/gitaly_client/storage_settings.rb
index 9a576e463e3..02fcb413abd 100644
--- a/lib/gitlab/gitaly_client/storage_settings.rb
+++ b/lib/gitlab/gitaly_client/storage_settings.rb
@@ -4,6 +4,8 @@ module Gitlab
     # where production code (app, config, db, lib) touches Git repositories
     # directly.
     class StorageSettings
+      extend Gitlab::TemporarilyAllow
+
       DirectPathAccessError = Class.new(StandardError)
       InvalidConfigurationError = Class.new(StandardError)
 
@@ -17,7 +19,21 @@ module Gitlab
       # This class will give easily recognizable NoMethodErrors
       Deprecated = Class.new
 
-      attr_reader :legacy_disk_path
+      MUTEX = Mutex.new
+
+      DISK_ACCESS_DENIED_FLAG = :deny_disk_access
+      ALLOW_KEY = :allow_disk_access
+
+      # 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?
+        !temporarily_allowed?(ALLOW_KEY) && GitalyClient.feature_enabled?(DISK_ACCESS_DENIED_FLAG)
+      rescue
+        false # Err on the side of caution, don't break gitlab for people
+      end
 
       def initialize(storage)
         raise InvalidConfigurationError, "expected a Hash, got a #{storage.class.name}" unless storage.is_a?(Hash)
@@ -34,6 +50,14 @@ module Gitlab
         @hash.fetch(:gitaly_address)
       end
 
+      def legacy_disk_path
+        if self.class.disk_access_denied?
+          raise DirectPathAccessError, "git disk access denied via the gitaly_#{DISK_ACCESS_DENIED_FLAG} feature"
+        end
+
+        @legacy_disk_path
+      end
+
       private
 
       def method_missing(m, *args, &block)
diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb
index 6e554383270..fcbf266b80b 100644
--- a/lib/gitlab/health_checks/fs_shards_check.rb
+++ b/lib/gitlab/health_checks/fs_shards_check.rb
@@ -77,7 +77,9 @@ module Gitlab
         end
 
         def storage_path(storage_name)
-          storages_paths[storage_name]&.legacy_disk_path
+          Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+            storages_paths[storage_name]&.legacy_disk_path
+          end
         end
 
         # All below test methods use shell commands to perform actions on storage volumes.
diff --git a/lib/gitlab/temporarily_allow.rb b/lib/gitlab/temporarily_allow.rb
new file mode 100644
index 00000000000..880e55f71df
--- /dev/null
+++ b/lib/gitlab/temporarily_allow.rb
@@ -0,0 +1,42 @@
+module Gitlab
+  module TemporarilyAllow
+    TEMPORARILY_ALLOW_MUTEX = Mutex.new
+
+    def temporarily_allow(key)
+      temporarily_allow_add(key, 1)
+      yield
+    ensure
+      temporarily_allow_add(key, -1)
+    end
+
+    def temporarily_allowed?(key)
+      if RequestStore.active?
+        temporarily_allow_request_store[key] > 0
+      else
+        TEMPORARILY_ALLOW_MUTEX.synchronize do
+          temporarily_allow_ivar[key] > 0
+        end
+      end
+    end
+
+    private
+
+    def temporarily_allow_ivar
+      @temporarily_allow ||= Hash.new(0)
+    end
+
+    def temporarily_allow_request_store
+      RequestStore[:temporarily_allow] ||= Hash.new(0)
+    end
+
+    def temporarily_allow_add(key, value)
+      if RequestStore.active?
+        temporarily_allow_request_store[key] += value
+      else
+        TEMPORARILY_ALLOW_MUTEX.synchronize do
+          temporarily_allow_ivar[key] += value
+        end
+      end
+    end
+  end
+end