Merge branch 'pks-gitaly-force-primary-routing-with-hookenv' into 'master'

gitaly: Fix access checks with transactions and quarantine environments

See merge request gitlab-org/gitlab!53449

changelogs/unreleased/pks-gitaly-force-primary-routing-with-hookenv.yml

+---
+title: 'gitaly: Fix access checks with transactions and quarantine environments'
+merge_request: 53449
+author:
+type: fixed

lib/gitlab/gitaly_client.rb

@@ -226,6 +226,7 @@ def self.request_kwargs(storage, timeout:, remote_storage: nil)
       metadata['username'] = context_data['meta.user'] if context_data&.fetch('meta.user', nil)
       metadata['remote_ip'] = context_data['meta.remote_ip'] if context_data&.fetch('meta.remote_ip', nil)
       metadata.merge!(Feature::Gitaly.server_feature_flags)
+      metadata.merge!(route_to_primary)
       deadline_info = request_deadline(timeout)
       metadata.merge!(deadline_info.slice(:deadline_type))
@@ -233,6 +234,24 @@ def self.request_kwargs(storage, timeout:, remote_storage: nil)
       { metadata: metadata, deadline: deadline_info[:deadline] }
     end
+    # Gitlab::Git::HookEnv will set the :gitlab_git_env variable in case we're
+    # running in the context of a Gitaly hook call, which may make use of
+    # quarantined object directories. We thus need to pass along the path of
+    # the quarantined object directory to Gitaly, otherwise it won't be able to
+    # find these quarantined objects. Given that the quarantine directory is
+    # generated with a random name, they'll have different names when multiple
+    # Gitaly nodes take part in a single transaction. As a result, we are
+    # forced to route all requests to the primary node which has injected the
+    # quarantine object directory to us.
+    def self.route_to_primary
+      return {} unless Gitlab::SafeRequestStore.active?
+
+      return {} unless Gitlab::SafeRequestStore[:gitlab_git_env]
+
+      { 'gitaly-route-repository-accessor-policy' => 'primary-only' }
+    end
+    private_class_method :route_to_primary
+
     def self.request_deadline(timeout)
       # timeout being 0 means the request is allowed to run indefinitely.
       # We can't allow that inside a request, but this won't count towards Gitaly

spec/lib/gitlab/gitaly_client_spec.rb

@@ -296,6 +296,32 @@ def stub_repos_storages(address)
       end
     end
+    context 'gitlab_git_env' do
+      let(:policy) { 'gitaly-route-repository-accessor-policy' }
+
+      context 'when RequestStore is disabled' do
+        it 'does not force-route to primary' do
+          expect(described_class.request_kwargs('default', timeout: 1)[:metadata][policy]).to be_nil
+        end
+      end
+
+      context 'when RequestStore is enabled without git_env', :request_store do
+        it 'does not force-orute to primary' do
+          expect(described_class.request_kwargs('default', timeout: 1)[:metadata][policy]).to be_nil
+        end
+      end
+
+      context 'when RequestStore is enabled with git_env', :request_store do
+        before do
+          Gitlab::SafeRequestStore[:gitlab_git_env] = true
+        end
+
+        it 'enables force-routing to primary' do
+          expect(described_class.request_kwargs('default', timeout: 1)[:metadata][policy]).to eq('primary-only')
+        end
+      end
+    end
+
     context 'deadlines', :request_store do
       let(:request_deadline) { real_time + 10.0 }