From 3c52e2f06ef3234ab5ace532e21e194abab96b59 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 20 Nov 2017 15:27:52 -0800 Subject: Optimize read-only middleware so that it does not consume as much CPU In !15082, we changed the behavior of the middleware to call `Rails.application.routes.recognize_path` whenever a new route arrived. However, this can be a CPU-intensive task because Rails needs to allocate memory and compile 850+ different regular expressions, which are complicated in GitLab. As a short-term fix, we can do a lightweight string match before we do the heavier comparison. Closes #40185, gitlab-com/infrastructure#3240 --- lib/gitlab/middleware/read_only.rb | 6 ++++++ spec/lib/gitlab/middleware/read_only_spec.rb | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index 5e4932e4e57..dc77f737da8 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -74,10 +74,16 @@ module Gitlab end def grack_route + # Calling route_hash may be expensive. Only do it if we think there's a possible match + return false unless request.path.end_with?('.git/git-upload-pack') + route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack' end def lfs_route + # Calling route_hash may be expensive. Only do it if we think there's a possible match + return false unless request.path.end_with?('/info/lfs/objects/batch') + route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch' end end diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb index b14735943a5..044b93f0120 100644 --- a/spec/lib/gitlab/middleware/read_only_spec.rb +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -84,6 +84,7 @@ describe Gitlab::Middleware::ReadOnly do end it 'expects POST of new file that looks like an LFS batch url to be disallowed' do + expect(Rails.application.routes).to receive(:recognize_path).and_call_original response = request.post('/root/gitlab-ce/new/master/app/info/lfs/objects/batch') expect(response).to be_a_redirect @@ -92,6 +93,8 @@ describe Gitlab::Middleware::ReadOnly do context 'whitelisted requests' do it 'expects a POST internal request to be allowed' do + expect(Rails.application.routes).not_to receive(:recognize_path) + response = request.post("/api/#{API::API.version}/internal") expect(response).not_to be_a_redirect @@ -99,6 +102,7 @@ describe Gitlab::Middleware::ReadOnly do end it 'expects a POST LFS request to batch URL to be allowed' do + expect(Rails.application.routes).to receive(:recognize_path).and_call_original response = request.post('/root/rouge.git/info/lfs/objects/batch') expect(response).not_to be_a_redirect @@ -106,6 +110,7 @@ describe Gitlab::Middleware::ReadOnly do end it 'expects a POST request to git-upload-pack URL to be allowed' do + expect(Rails.application.routes).to receive(:recognize_path).and_call_original response = request.post('/root/rouge.git/git-upload-pack') expect(response).not_to be_a_redirect -- cgit v1.2.1 From cba68d338bfac97baab153d38fa63b85a27f6d19 Mon Sep 17 00:00:00 2001 From: digitalMoksha Date: Tue, 21 Nov 2017 13:29:57 +0100 Subject: use `Gitlab::Routing.url_helpers` instead of `Rails.application.routes.url_helpers` since `Rails.application.routes.url_helpers` creates a new anonymous module every time it's called --- lib/gitlab/middleware/read_only.rb | 2 +- spec/lib/gitlab/middleware/read_only_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index dc77f737da8..c26656704d7 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -58,7 +58,7 @@ module Gitlab end def last_visited_url - @env['HTTP_REFERER'] || rack_session['user_return_to'] || Rails.application.routes.url_helpers.root_url + @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url end def route_hash diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb index 044b93f0120..07ba11b93a3 100644 --- a/spec/lib/gitlab/middleware/read_only_spec.rb +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -91,6 +91,12 @@ describe Gitlab::Middleware::ReadOnly do expect(subject).to disallow_request end + it 'returns last_vistited_url for disallowed request' do + response = request.post('/test_request') + + expect(response.location).to eq 'http://localhost/' + end + context 'whitelisted requests' do it 'expects a POST internal request to be allowed' do expect(Rails.application.routes).not_to receive(:recognize_path) -- cgit v1.2.1 From 91075c8237307e09c2be8a88ffb3711fd62417d1 Mon Sep 17 00:00:00 2001 From: digitalMoksha Date: Tue, 21 Nov 2017 13:30:54 +0100 Subject: check for `read_only?` first before seeing if request is disallowed --- lib/gitlab/middleware/read_only.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index c26656704d7..f8b9c5bff4c 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -14,7 +14,7 @@ module Gitlab @env = env @route_hash = nil - if disallowed_request? && Gitlab::Database.read_only? + if Gitlab::Database.read_only? && disallowed_request? Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation') error_message = 'You cannot do writing operations on a read-only GitLab instance' -- cgit v1.2.1 From aeb2f49fd4c65b9f5e3b2d7858a250a747a0f702 Mon Sep 17 00:00:00 2001 From: digitalMoksha Date: Tue, 21 Nov 2017 15:35:30 +0100 Subject: Revert "check for `read_only?` first before seeing if request is disallowed" This reverts commit 91075c8237307e09c2be8a88ffb3711fd62417d1. --- lib/gitlab/middleware/read_only.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index f8b9c5bff4c..c26656704d7 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -14,7 +14,7 @@ module Gitlab @env = env @route_hash = nil - if Gitlab::Database.read_only? && disallowed_request? + if disallowed_request? && Gitlab::Database.read_only? Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation') error_message = 'You cannot do writing operations on a read-only GitLab instance' -- cgit v1.2.1