Merge branch 'mk/add-secondary-lag-message-on-http-push-geo' into 'master'

Add secondary lag message on Git push over HTTP

See merge request gitlab-org/gitlab!15901

config/routes/project.rb

 resources :projects, only: [:index, :new, :create]
 
+Gitlab.ee do
+  scope "/-/push_from_secondary/:geo_node_id" do
+    draw :git_http
+  end
+end
+
 draw :git_http
 
 get '/projects/:id' => 'projects#resolve'

ee/app/controllers/ee/projects/git_http_client_controller.rb

@@ -155,7 +155,13 @@ module EE
       end
 
       def primary_full_url
-        ::Gitlab::Utils.append_path(::Gitlab::Geo.primary_node.internal_url, request_fullpath_for_primary)
+        path = File.join(secondary_referrer_path_prefix, request_fullpath_for_primary)
+
+        ::Gitlab::Utils.append_path(::Gitlab::Geo.primary_node.internal_url, path)
+      end
+
+      def secondary_referrer_path_prefix
+        File.join(::Gitlab::Geo::GitPushHttp::PATH_PREFIX, ::Gitlab::Geo.current_node.id.to_s)
       end
 
       def redirect?

ee/app/controllers/ee/projects/git_http_controller.rb

@@ -13,6 +13,21 @@ module EE
         render json: ::Gitlab::Workhorse.git_http_ok(repository, repo_type, user, action_name, show_all_refs: geo_request?)
       end
 
+      override :git_receive_pack
+      def git_receive_pack
+        # Authentication/authorization already happened in `before_action`s
+
+        if ::Gitlab::Geo.primary?
+          # This ID is used by the /internal/post_receive API call
+          gl_id = ::Gitlab::GlId.gl_id(user)
+          gl_repository = repo_type.identifier_for_subject(project)
+          node_id = params["geo_node_id"]
+          ::Gitlab::Geo::GitPushHttp.new(gl_id, gl_repository).cache_referrer_node(node_id)
+        end
+
+        super
+      end
+
       private
 
       def user

ee/changelogs/unreleased/mk-add-secondary-lag-message-on-http-push-geo.yml

+---
+title: Add secondary lag message on Git push over HTTP
+merge_request: 15901
+author:
+type: added

ee/lib/ee/api/internal/base.rb

@@ -14,6 +14,26 @@ module EE
             def lfs_authentication_url(project)
               project.lfs_http_url_to_repo(params[:operation])
             end
+
+            override :ee_post_receive_response_hook
+            def ee_post_receive_response_hook(response)
+              response.add_basic_message(geo_secondary_lag_message) if ::Gitlab::Geo.primary?
+            end
+
+            def geo_secondary_lag_message
+              lag = current_replication_lag
+              return if lag.to_i <= 0
+
+              "Current replication lag: #{lag} seconds"
+            end
+
+            def current_replication_lag
+              fetch_geo_node_referrer&.status&.db_replication_lag_seconds
+            end
+
+            def fetch_geo_node_referrer
+              ::Gitlab::Geo::GitPushHttp.new(params[:identifier], params[:gl_repository]).fetch_referrer_node
+            end
           end
         end
       end

ee/lib/gitlab/geo/git_push_http.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Geo
+    class GitPushHttp
+      PATH_PREFIX = '/-/push_from_secondary'
+      CACHE_KEY_PREFIX = 'git_receive_pack:geo_node_id'
+      EXPIRES_IN = 5.minutes
+
+      def initialize(gl_id, gl_repository)
+        @gl_id = gl_id
+        @gl_repository = gl_repository
+      end
+
+      def cache_referrer_node(geo_node_id)
+        geo_node_id = geo_node_id.to_i
+        return unless geo_node_id > 0
+
+        Rails.cache.write(cache_key, geo_node_id, expires_in: EXPIRES_IN)
+      end
+
+      def fetch_referrer_node
+        id = Rails.cache.read(cache_key)
+
+        if id
+          # There is a race condition but since this is only used to display a
+          # notice, it's ok. If we didn't delete it, then a subsequent push
+          # directly to the primary would inappropriately show the secondary lag
+          # notice again.
+          Rails.cache.delete(cache_key)
+
+          GeoNode.find_by_id(id)
+        end
+      end
+
+      private
+
+      def cache_key
+        [
+          CACHE_KEY_PREFIX,
+          @gl_id,
+          @gl_repository
+        ].join(':')
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo/git_push_http_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Geo::GitPushHttp, :geo, :use_clean_rails_memory_store_caching do
+  include EE::GeoHelpers
+
+  let(:gl_id) { 'user-1234' }
+  let(:gl_repository) { 'project-77777' }
+  let(:cache_key) { "#{described_class::CACHE_KEY_PREFIX}:#{gl_id}:#{gl_repository}" }
+  set(:secondary) { create(:geo_node) }
+  subject { described_class.new(gl_id, gl_repository) }
+
+  describe '#cache_referrer_node' do
+    context 'when geo_node_id is present' do
+      context 'when geo_node_id is an integer' do
+        it 'stores the ID in cache' do
+          subject.cache_referrer_node(secondary.id)
+
+          value = Rails.cache.read(cache_key)
+          expect(value).to eq(secondary.id)
+        end
+
+        it 'stores the ID with an expiration' do
+          Timecop.freeze do
+            subject.cache_referrer_node(secondary.id)
+
+            Timecop.travel(described_class::EXPIRES_IN + 20.seconds) do
+              value = Rails.cache.read(cache_key)
+              expect(value).to be_nil
+            end
+          end
+        end
+      end
+
+      context 'when geo_node_id is not an integer' do
+        it 'does not cache anything' do
+          subject.cache_referrer_node('bad input')
+
+          value = Rails.cache.read(cache_key)
+          expect(value).to be_nil
+        end
+      end
+    end
+
+    context 'when geo_node_id is blank' do
+      it 'does not cache anything' do
+        subject.cache_referrer_node(' ')
+
+        value = Rails.cache.read(cache_key)
+        expect(value).to be_nil
+      end
+    end
+  end
+
+  describe '#fetch_referrer_node' do
+    context 'when there is a cached ID' do
+      it 'deletes the key' do
+        Rails.cache.write(cache_key, secondary.id, expires_in: described_class::EXPIRES_IN)
+
+        subject.fetch_referrer_node
+
+        expect(subject.fetch_referrer_node).to be_nil
+      end
+
+      context 'when the GeoNode exists' do
+        it 'returns the GeoNode with the cached ID' do
+          Rails.cache.write(cache_key, secondary.id, expires_in: described_class::EXPIRES_IN)
+
+          expect(subject.fetch_referrer_node).to eq(secondary)
+        end
+      end
+
+      context 'when the GeoNode does not exist' do
+        it 'returns nil' do
+          Rails.cache.write(cache_key, 9999998, expires_in: described_class::EXPIRES_IN)
+
+          expect(subject.fetch_referrer_node).to be_nil
+        end
+      end
+    end
+
+    context 'when there is no cached ID' do
+      it 'returns nil' do
+        expect(subject.fetch_referrer_node).to be_nil
+      end
+    end
+  end
+end

ee/spec/requests/api/internal/base_spec.rb

@@ -4,6 +4,113 @@ require 'spec_helper'
 describe API::Internal::Base do
   include EE::GeoHelpers
 
+  set(:primary_node) { create(:geo_node, :primary) }
+  set(:secondary_node) { create(:geo_node) }
+
+  describe 'POST /internal/post_receive', :geo do
+    set(:user) { create(:user) }
+    let(:key) { create(:key, user: user) }
+    set(:project) { create(:project, :repository, :wiki_repo) }
+    let(:secret_token) { Gitlab::Shell.secret_token }
+    let(:gl_repository) { "project-#{project.id}" }
+    let(:reference_counter) { double('ReferenceCounter') }
+
+    let(:identifier) { 'key-123' }
+
+    let(:valid_params) do
+      {
+        gl_repository: gl_repository,
+        secret_token: secret_token,
+        identifier: identifier,
+        changes: changes,
+        push_options: {}
+      }
+    end
+
+    let(:branch_name) { 'feature' }
+
+    let(:changes) do
+      "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{branch_name}"
+    end
+
+    let(:git_push_http) { double('GitPushHttp') }
+
+    before do
+      project.add_developer(user)
+      allow(described_class).to receive(:identify).and_return(user)
+      allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user)
+      stub_current_geo_node(primary_node)
+    end
+
+    context 'when the push was redirected from a Geo secondary to the primary' do
+      before do
+        expect(Gitlab::Geo::GitPushHttp).to receive(:new).with(identifier, gl_repository).and_return(git_push_http)
+        expect(git_push_http).to receive(:fetch_referrer_node).and_return(secondary_node)
+      end
+
+      context 'when the secondary has a GeoNodeStatus' do
+        context 'when the GeoNodeStatus db_replication_lag_seconds is greater than 0' do
+          let!(:status) { create(:geo_node_status, geo_node: secondary_node, db_replication_lag_seconds: 17) }
+
+          it 'includes current Geo secondary lag in the output' do
+            post api('/internal/post_receive'), params: valid_params
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['messages']).to include({
+              'type' => 'basic',
+              'message' => "Current replication lag: 17 seconds"
+            })
+          end
+        end
+
+        context 'when the GeoNodeStatus db_replication_lag_seconds is 0' do
+          let!(:status) { create(:geo_node_status, geo_node: secondary_node, db_replication_lag_seconds: 0) }
+
+          it 'does not include current Geo secondary lag in the output' do
+            post api('/internal/post_receive'), params: valid_params
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+          end
+        end
+
+        context 'when the GeoNodeStatus db_replication_lag_seconds is nil' do
+          let!(:status) { create(:geo_node_status, geo_node: secondary_node, db_replication_lag_seconds: nil) }
+
+          it 'does not include current Geo secondary lag in the output' do
+            post api('/internal/post_receive'), params: valid_params
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+          end
+        end
+      end
+
+      context 'when the secondary does not have a GeoNodeStatus' do
+        it 'does not include current Geo secondary lag in the output' do
+          post api('/internal/post_receive'), params: valid_params
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+        end
+      end
+    end
+
+    context 'when the push was not redirected from a Geo secondary to the primary' do
+      before do
+        expect(Gitlab::Geo::GitPushHttp).to receive(:new).with(identifier, gl_repository).and_return(git_push_http)
+        expect(git_push_http).to receive(:fetch_referrer_node).and_return(nil)
+      end
+
+      it 'does not include current Geo secondary lag in the output' do
+        post api('/internal/post_receive'), params: valid_params
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+      end
+    end
+  end
+
   describe "POST /internal/allowed" do
     set(:user) { create(:user) }
     set(:key) { create(:key, user: user) }
@@ -147,17 +254,14 @@ describe API::Internal::Base do
     end
   end
 
-  describe "POST /internal/lfs_authenticate" do
+  describe "POST /internal/lfs_authenticate", :geo do
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }
     let(:secret_token) { Gitlab::Shell.secret_token }
 
     context 'for a secondary node' do
-      let!(:primary) { create(:geo_node, :primary) }
-      let!(:secondary) { create(:geo_node) }
-
       before do
-        stub_current_geo_node(secondary)
+        stub_current_geo_node(secondary_node)
         project.add_developer(user)
       end
 

ee/spec/requests/git_http_geo_spec.rb

@@ -113,7 +113,7 @@ describe "Git HTTP requests (Geo)", :geo do
 
         it 'redirects to the primary' do
           is_expected.to have_gitlab_http_status(:redirect)
-          redirect_location = "#{primary.url.chomp('/')}#{url}?service=git-receive-pack"
+          redirect_location = "#{redirected_primary_url}?service=git-receive-pack"
           expect(subject.header['Location']).to eq(redirect_location)
         end
       end
@@ -166,7 +166,7 @@ describe "Git HTTP requests (Geo)", :geo do
 
               it 'redirects to the primary' do
                 is_expected.to have_gitlab_http_status(:redirect)
-                redirect_location = "#{primary.url.chomp('/')}#{url}"
+                redirect_location = "#{redirected_primary_url}"
                 expect(subject.header['Location']).to eq(redirect_location)
               end
             end
@@ -254,97 +254,136 @@ describe "Git HTTP requests (Geo)", :geo do
 
             it 'redirects to the primary' do
               is_expected.to have_gitlab_http_status(:redirect)
-              redirect_location = "#{primary.url.chomp('/')}#{url}"
+              redirect_location = "#{redirected_primary_url}"
               expect(subject.header['Location']).to eq(redirect_location)
             end
           end
         end
       end
     end
+
+    def redirected_primary_url
+      "#{primary.url.chomp('/')}#{::Gitlab::Geo::GitPushHttp::PATH_PREFIX}/#{secondary.id}#{url}"
+    end
   end
 
-  context 'when current node is the primary' do
+  context 'when current node is the primary', :use_clean_rails_memory_store_caching do
     let(:current_node) { primary }
 
     describe 'POST git_receive_pack' do
-      def make_request
-        post url, params: {}, headers: env
-      end
-
-      let(:url) { "/#{project.full_path}.git/git-receive-pack" }
-
-      before do
-        env['Geo-GL-Id'] = geo_gl_id
-      end
-
       subject do
         make_request
         response
       end
 
-      context 'when gl_id is incorrectly provided via HTTP headers' do
-        where(:geo_gl_id) do
-          [
-            nil,
-            ''
-          ]
+      context 'when HTTP redirected from a secondary node' do
+        def make_request
+          post url, headers: auth_env(user.username, user.password, nil)
         end
 
-        with_them do
-          it 'returns a 403' do
-            is_expected.to have_gitlab_http_status(:forbidden)
-            expect(response.body).to eql('You are not allowed to upload code for this project.')
-          end
+        let(:identifier) { "user-#{user.id}" }
+        let(:gl_repository) { "project-#{project.id}" }
+        let(:url) { "#{::Gitlab::Geo::GitPushHttp::PATH_PREFIX}/#{secondary.id}/#{project.full_path}.git/git-receive-pack" }
+
+        # The bigger picture request flow relevant to this feature is:
+        #
+        #   * The HTTP request hits NGINX
+        #   * Then Workhorse
+        #   * Then Rails (the scope of request tests is limited to this line item)
+        #   * Rails responds OK to Workhorse
+        #   * Workhorse connects to Gitaly: SmartHTTP Service, ReceivePack RPC
+        #   * In a pre-receive hook, Gitaly makes a request to Rails' POST /api/v4/internal/allowed
+        #   * Rails says OK
+        #   * In a post-receive hook, Gitaly makes a request to Rails' POST /api/v4/internal/post_receive
+        #   * Rails responds to Gitaly, including a collection of messages, which includes the replication lag message
+        #   * Gitaly outputs the messages in the stream of Proto messages
+        #   * Pipe the output through Workhorse and NGINX
+        #
+        # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9195
+        #
+        it 'stores the secondary node ID so the internal API post_receive request can generate the replication lag message' do
+          is_expected.to have_gitlab_http_status(:ok)
+
+          stored_node = ::Gitlab::Geo::GitPushHttp.new(identifier, gl_repository).fetch_referrer_node
+          expect(stored_node).to eq(secondary)
         end
       end
 
-      context 'when gl_id is provided via HTTP headers' do
-        context 'but is invalid' do
+      context 'when proxying an SSH request from a secondary node' do
+        def make_request
+          post url, params: {}, headers: env
+        end
+
+        let(:url) { "/#{project.full_path}.git/git-receive-pack" }
+
+        before do
+          env['Geo-GL-Id'] = geo_gl_id
+        end
+
+        context 'when gl_id is incorrectly provided via HTTP headers' do
           where(:geo_gl_id) do
             [
-              'key-999',
-              'key-1',
-              'key-999',
-              'junk',
-              'junk-1',
-              'kkey-1'
+              nil,
+              ''
             ]
           end
 
           with_them do
             it 'returns a 403' do
               is_expected.to have_gitlab_http_status(:forbidden)
-              expect(response.body).to eql('Geo push user is invalid.')
+              expect(response.body).to eql('You are not allowed to upload code for this project.')
             end
           end
         end
 
-        context 'and is valid' do
-          context 'but the user has no access' do
-            let(:geo_gl_id) { "key-#{key_for_user_without_any_access.id}" }
+        context 'when gl_id is provided via HTTP headers' do
+          context 'but is invalid' do
+            where(:geo_gl_id) do
+              [
+                'key-999',
+                'key-1',
+                'key-999',
+                'junk',
+                'junk-1',
+                'kkey-1'
+              ]
+            end
 
-            it 'returns a 404' do
-              is_expected.to have_gitlab_http_status(:not_found)
-              expect(response.body).to eql('The project you were looking for could not be found.')
+            with_them do
+              it 'returns a 403' do
+                is_expected.to have_gitlab_http_status(:forbidden)
+                expect(response.body).to eql('Geo push user is invalid.')
+              end
             end
           end
 
-          context 'but the user does not have push access' do
-            let(:geo_gl_id) { "key-#{key_for_user_without_push_access.id}" }
+          context 'and is valid' do
+            context 'but the user has no access' do
+              let(:geo_gl_id) { "key-#{key_for_user_without_any_access.id}" }
 
-            it 'returns a 403' do
-              is_expected.to have_gitlab_http_status(:forbidden)
-              expect(response.body).to eql('You are not allowed to push code to this project.')
+              it 'returns a 404' do
+                is_expected.to have_gitlab_http_status(:not_found)
+                expect(response.body).to eql('The project you were looking for could not be found.')
+              end
             end
-          end
 
-          context 'and the user has push access' do
-            let(:geo_gl_id) { "key-#{key.id}" }
+            context 'but the user does not have push access' do
+              let(:geo_gl_id) { "key-#{key_for_user_without_push_access.id}" }
 
-            it 'returns a 200' do
-              is_expected.to have_gitlab_http_status(:ok)
-              expect(json_response['GL_ID']).to match("user-#{user.id}")
-              expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository::PROJECT.identifier_for_subject(project))
+              it 'returns a 403' do
+                is_expected.to have_gitlab_http_status(:forbidden)
+                expect(response.body).to eql('You are not allowed to push code to this project.')
+              end
+            end
+
+            context 'and the user has push access' do
+              let(:geo_gl_id) { "key-#{key.id}" }
+
+              it 'returns a 200' do
+                is_expected.to have_gitlab_http_status(:ok)
+                expect(json_response['GL_ID']).to match("user-#{user.id}")
+                expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository::PROJECT.identifier_for_subject(project))
+              end
             end
           end
         end

lib/api/internal/base.rb

@@ -22,6 +22,10 @@ module API
           # easily.
           project.http_url_to_repo
         end
+
+        def ee_post_receive_response_hook(response)
+          # Hook for EE to add messages
+        end
       end
 
       namespace 'internal' do
@@ -265,6 +269,8 @@ module API
             response.add_basic_message(project_created_message)
           end
 
+          ee_post_receive_response_hook(response)
+
           present response, with: Entities::InternalPostReceive::Response
         end
       end