Merge branch 'gitaly-tripswitch' into 'master'

Add "deny disk access" Gitaly feature (tripswitch)

Closes gitaly#1210

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

config/initializers/1_settings.rb

@@ -391,8 +391,10 @@ repositories_storages          = Settings.repositories.storages.values
 repository_downloads_path      = Settings.gitlab['repository_downloads_path'].to_s.gsub(%r{/$}, '')
 repository_downloads_full_path = File.expand_path(repository_downloads_path, Settings.gitlab['user_home'])
 
-if repository_downloads_path.blank? || repositories_storages.any? { |rs| [repository_downloads_path, repository_downloads_full_path].include?(rs.legacy_disk_path.gsub(%r{/$}, '')) }
-  Settings.gitlab['repository_downloads_path'] = File.join(Settings.shared['path'], 'cache/archive')
+Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+  if repository_downloads_path.blank? || repositories_storages.any? { |rs| [repository_downloads_path, repository_downloads_full_path].include?(rs.legacy_disk_path.gsub(%r{/$}, '')) }
+    Settings.gitlab['repository_downloads_path'] = File.join(Settings.shared['path'], 'cache/archive')
+  end
 end
 
 #

config/initializers/6_validations.rb

@@ -38,10 +38,12 @@ def validate_storages_config
 end
 
 def validate_storages_paths
-  Gitlab.config.repositories.storages.each do |name, repository_storage|
-    parent_name, _parent_path = find_parent_path(name, repository_storage.legacy_disk_path)
-    if parent_name
-      storage_validation_error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages")
+  Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+    Gitlab.config.repositories.storages.each do |name, repository_storage|
+      parent_name, _parent_path = find_parent_path(name, repository_storage.legacy_disk_path)
+      if parent_name
+        storage_validation_error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages")
+      end
     end
   end
 end

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

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'

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

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
 

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

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)

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.

lib/gitlab/temporarily_allow.rb

+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

spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb

@@ -299,7 +299,11 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits, :m
       let(:commits) { merge_request_diff.commits.map(&:to_hash) }
       let(:first_commit) { project.repository.commit(merge_request_diff.head_commit_sha) }
       let(:expected_commits) { commits }
-      let(:diffs) { first_commit.rugged_diff_from_parent.patches }
+      let(:diffs) do
+        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+          first_commit.rugged_diff_from_parent.patches
+        end
+      end
       let(:expected_diffs) { [] }
 
       include_examples 'updated MR diff'
@@ -309,7 +313,11 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits, :m
       let(:commits) { merge_request_diff.commits.map(&:to_hash) }
       let(:first_commit) { project.repository.commit(merge_request_diff.head_commit_sha) }
       let(:expected_commits) { commits }
-      let(:diffs) { first_commit.rugged_diff_from_parent.deltas }
+      let(:diffs) do
+        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+          first_commit.rugged_diff_from_parent.deltas
+        end
+      end
       let(:expected_diffs) { [] }
 
       include_examples 'updated MR diff'

spec/lib/gitlab/checks/lfs_integrity_spec.rb

@@ -6,7 +6,9 @@ describe Gitlab::Checks::LfsIntegrity do
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
   let(:newrev) do
-    operations = BareRepoOperations.new(repository.path)
+    operations = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+      BareRepoOperations.new(repository.path)
+    end
 
     # Create a commit not pointed at by any ref to emulate being in the
     # pre-receive hook so that `--not --all` returns some objects

spec/lib/gitlab/conflict/file_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Gitlab::Conflict::File do
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
-  let(:rugged) { repository.rugged }
+  let(:rugged) { Gitlab::GitalyClient::StorageSettings.allow_disk_access { repository.rugged } }
   let(:their_commit) { rugged.branches['conflict-start'].target }
   let(:our_commit) { rugged.branches['conflict-resolvable'].target }
   let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) }

spec/support/gitaly.rb

@@ -7,7 +7,10 @@ RSpec.configure do |config|
       next if example.metadata[:skip_gitaly_mock]
 
       # Use 'and_wrap_original' to make sure the arguments are valid
-      allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_wrap_original { |m, *args| m.call(*args) || true }
+      allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_wrap_original do |m, *args|
+        m.call(*args)
+        !Gitlab::GitalyClient::EXPLICIT_OPT_IN_REQUIRED.include?(args.first)
+      end
     end
   end
 end