From d4d564c8e7d2cbc3e6742475a793ba0f630167e3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 1 Feb 2018 18:48:32 +0800 Subject: [PATCH 1/6] Try not to hold env and release the controller after the request. This way, we could release the project referred from the controller, which potentially referred a repository which potentially allocated a lot of memories. Before this change, we could hold the last request data and cannot release the memory. After this change, the largest request data should be able to be collected from GC. This might not impact the instances having heavy load, as the last request should be changing all the time, and GC won't kick in for each request anyway. However it could still potentially allow us to free more memories for each GC runs, because now we could free one more request anyway. --- config.ru | 1 + lib/gitlab/middleware/read_only.rb | 2 +- lib/gitlab/middleware/release_controller.rb | 9 +++++++++ .../middleware/release_controller_spec.rb | 20 +++++++++++++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 lib/gitlab/middleware/release_controller.rb create mode 100644 spec/lib/gitlab/middleware/release_controller_spec.rb diff --git a/config.ru b/config.ru index de0400f4f67..c4bef72308e 100644 --- a/config.ru +++ b/config.ru @@ -23,5 +23,6 @@ warmup do |app| end map ENV['RAILS_RELATIVE_URL_ROOT'] || "/" do + use Gitlab::ReleaseController run Gitlab::Application end diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index c26656704d7..a68c6c3d15c 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -28,7 +28,7 @@ module Gitlab end end - @app.call(env) + @app.call(env).tap { @env = nil } end private diff --git a/lib/gitlab/middleware/release_controller.rb b/lib/gitlab/middleware/release_controller.rb new file mode 100644 index 00000000000..a21d718d51c --- /dev/null +++ b/lib/gitlab/middleware/release_controller.rb @@ -0,0 +1,9 @@ +module Gitlab + module Middleware + ReleaseController = Struct.new(:app) do + def call(env) + app.call(env).tap { env.delete('action_controller.instance') } + end + end + end +end diff --git a/spec/lib/gitlab/middleware/release_controller_spec.rb b/spec/lib/gitlab/middleware/release_controller_spec.rb new file mode 100644 index 00000000000..854bac6e751 --- /dev/null +++ b/spec/lib/gitlab/middleware/release_controller_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Gitlab::Middleware::ReleaseController do + let(:inner_app) { double(:app) } + let(:app) { described_class.new(inner_app) } + let(:env) { { 'action_controller.instance' => 'something' } } + + before do + expect(inner_app).to receive(:call).with(env).and_return('yay') + end + + describe '#call' do + it 'calls the app and delete the controller' do + result = app.call(env) + + expect(result).to eq('yay') + expect(env).to be_empty + end + end +end From bbfce29ba8d75df5344dae34dc472dfb3b3acf4b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 2 Feb 2018 22:12:28 +0800 Subject: [PATCH 2/6] Use a controller to hold request values So that we don't need to hold env after the request. This makes it much harder to test, especially Rails session is acting weirdly, so we need `dig('flash', 'flashes', 'alert')` to dig the actual flash value. --- lib/gitlab/middleware/read_only.rb | 159 ++++++++++--------- spec/lib/gitlab/middleware/read_only_spec.rb | 23 ++- 2 files changed, 104 insertions(+), 78 deletions(-) diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index a68c6c3d15c..b7649ea01db 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -5,86 +5,95 @@ module Gitlab APPLICATION_JSON = 'application/json'.freeze API_VERSIONS = (3..4) + class Controller + def initialize(app, env) + @app = app + @env = env + end + + def call + 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' + + if json_request? + return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] + else + rack_flash.alert = error_message + rack_session['flash'] = rack_flash.to_session_value + + return [301, { 'Location' => last_visited_url }, []] + end + end + + @app.call(@env) + end + + private + + def disallowed_request? + DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && + !whitelisted_routes + end + + def json_request? + request.media_type == APPLICATION_JSON + end + + def rack_flash + @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) + end + + def rack_session + @env['rack.session'] + end + + def request + @env['rack.request'] ||= Rack::Request.new(@env) + end + + def last_visited_url + @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url + end + + def route_hash + @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} + end + + def whitelisted_routes + grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route + end + + def sidekiq_route + request.path.start_with?('/admin/sidekiq') + 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 + + def self.internal_routes + @internal_routes ||= + API_VERSIONS.map { |version| "api/v#{version}/internal" } + end + def initialize(app) @app = app - @whitelisted = internal_routes end def call(env) - @env = env - @route_hash = nil - - 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' - - if json_request? - return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] - else - rack_flash.alert = error_message - rack_session['flash'] = rack_flash.to_session_value - - return [301, { 'Location' => last_visited_url }, []] - end - end - - @app.call(env).tap { @env = nil } - end - - private - - def internal_routes - API_VERSIONS.flat_map { |version| "api/v#{version}/internal" } - end - - def disallowed_request? - DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && !whitelisted_routes - end - - def json_request? - request.media_type == APPLICATION_JSON - end - - def rack_flash - @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) - end - - def rack_session - @env['rack.session'] - end - - def request - @env['rack.request'] ||= Rack::Request.new(@env) - end - - def last_visited_url - @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url - end - - def route_hash - @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} - end - - def whitelisted_routes - grack_route || @whitelisted.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route - end - - def sidekiq_route - request.path.start_with?('/admin/sidekiq') - 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' + Controller.new(@app, env).call end end end diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb index 07ba11b93a3..b3c85142b82 100644 --- a/spec/lib/gitlab/middleware/read_only_spec.rb +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -11,8 +11,10 @@ describe Gitlab::Middleware::ReadOnly do RSpec::Matchers.define :disallow_request do match do |middleware| - flash = middleware.send(:rack_flash) - flash['alert'] && flash['alert'].include?('You cannot do writing operations') + alert = middleware.env['rack.session'].to_hash + .dig('flash', 'flashes', 'alert') + + alert&.include?('You cannot do writing operations') end end @@ -34,7 +36,22 @@ describe Gitlab::Middleware::ReadOnly do rack.to_app end - subject { described_class.new(fake_app) } + let(:observe_env) do + Module.new do + attr_reader :env + + def call(env) + @env = env + super + end + end + end + + subject do + app = described_class.new(fake_app) + app.extend(observe_env) + app + end let(:request) { Rack::MockRequest.new(rack_stack) } From 31f1ec59a7cf7517cd5935ef3af540aceba07bb3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 7 Feb 2018 22:44:05 +0800 Subject: [PATCH 3/6] Release the entire env --- config.ru | 2 +- lib/gitlab/middleware/release_controller.rb | 9 --------- lib/gitlab/middleware/release_env.rb | 14 ++++++++++++++ ...ease_controller_spec.rb => release_env_spec.rb} | 2 +- 4 files changed, 16 insertions(+), 11 deletions(-) delete mode 100644 lib/gitlab/middleware/release_controller.rb create mode 100644 lib/gitlab/middleware/release_env.rb rename spec/lib/gitlab/middleware/{release_controller_spec.rb => release_env_spec.rb} (89%) diff --git a/config.ru b/config.ru index c4bef72308e..7b15939c6ff 100644 --- a/config.ru +++ b/config.ru @@ -23,6 +23,6 @@ warmup do |app| end map ENV['RAILS_RELATIVE_URL_ROOT'] || "/" do - use Gitlab::ReleaseController + use Gitlab::Middleware::ReleaseEnv run Gitlab::Application end diff --git a/lib/gitlab/middleware/release_controller.rb b/lib/gitlab/middleware/release_controller.rb deleted file mode 100644 index a21d718d51c..00000000000 --- a/lib/gitlab/middleware/release_controller.rb +++ /dev/null @@ -1,9 +0,0 @@ -module Gitlab - module Middleware - ReleaseController = Struct.new(:app) do - def call(env) - app.call(env).tap { env.delete('action_controller.instance') } - end - end - end -end diff --git a/lib/gitlab/middleware/release_env.rb b/lib/gitlab/middleware/release_env.rb new file mode 100644 index 00000000000..f8d0a135965 --- /dev/null +++ b/lib/gitlab/middleware/release_env.rb @@ -0,0 +1,14 @@ +module Gitlab + module Middleware + # Some of middleware would hold env for no good reason even after the + # request had already been processed, and we could not garbage collect + # them due to this. Put this middleware as the first middleware so that + # it would clear the env after the request is done, allowing GC gets a + # chance to release memory for the last request. + ReleaseEnv = Struct.new(:app) do + def call(env) + app.call(env).tap { env.clear } + end + end + end +end diff --git a/spec/lib/gitlab/middleware/release_controller_spec.rb b/spec/lib/gitlab/middleware/release_env_spec.rb similarity index 89% rename from spec/lib/gitlab/middleware/release_controller_spec.rb rename to spec/lib/gitlab/middleware/release_env_spec.rb index 854bac6e751..657b705502a 100644 --- a/spec/lib/gitlab/middleware/release_controller_spec.rb +++ b/spec/lib/gitlab/middleware/release_env_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Middleware::ReleaseController do +describe Gitlab::Middleware::ReleaseEnv do let(:inner_app) { double(:app) } let(:app) { described_class.new(inner_app) } let(:env) { { 'action_controller.instance' => 'something' } } From 5309d4457aea74729a8c6be9ec76d535f922bf8a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 7 Feb 2018 22:44:18 +0800 Subject: [PATCH 4/6] Put controller in its separate file --- lib/gitlab/middleware/read_only.rb | 80 +----------------- lib/gitlab/middleware/read_only/controller.rb | 83 +++++++++++++++++++ 2 files changed, 84 insertions(+), 79 deletions(-) create mode 100644 lib/gitlab/middleware/read_only/controller.rb diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index b7649ea01db..19b74c0c122 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -5,84 +5,6 @@ module Gitlab APPLICATION_JSON = 'application/json'.freeze API_VERSIONS = (3..4) - class Controller - def initialize(app, env) - @app = app - @env = env - end - - def call - 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' - - if json_request? - return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] - else - rack_flash.alert = error_message - rack_session['flash'] = rack_flash.to_session_value - - return [301, { 'Location' => last_visited_url }, []] - end - end - - @app.call(@env) - end - - private - - def disallowed_request? - DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && - !whitelisted_routes - end - - def json_request? - request.media_type == APPLICATION_JSON - end - - def rack_flash - @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) - end - - def rack_session - @env['rack.session'] - end - - def request - @env['rack.request'] ||= Rack::Request.new(@env) - end - - def last_visited_url - @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url - end - - def route_hash - @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} - end - - def whitelisted_routes - grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route - end - - def sidekiq_route - request.path.start_with?('/admin/sidekiq') - 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 - def self.internal_routes @internal_routes ||= API_VERSIONS.map { |version| "api/v#{version}/internal" } @@ -93,7 +15,7 @@ module Gitlab end def call(env) - Controller.new(@app, env).call + ReadOnly::Controller.new(@app, env).call end end end diff --git a/lib/gitlab/middleware/read_only/controller.rb b/lib/gitlab/middleware/read_only/controller.rb new file mode 100644 index 00000000000..053cb6f0a9f --- /dev/null +++ b/lib/gitlab/middleware/read_only/controller.rb @@ -0,0 +1,83 @@ +module Gitlab + module Middleware + class ReadOnly + class Controller + def initialize(app, env) + @app = app + @env = env + end + + def call + 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' + + if json_request? + return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] + else + rack_flash.alert = error_message + rack_session['flash'] = rack_flash.to_session_value + + return [301, { 'Location' => last_visited_url }, []] + end + end + + @app.call(@env) + end + + private + + def disallowed_request? + DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && + !whitelisted_routes + end + + def json_request? + request.media_type == APPLICATION_JSON + end + + def rack_flash + @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) + end + + def rack_session + @env['rack.session'] + end + + def request + @env['rack.request'] ||= Rack::Request.new(@env) + end + + def last_visited_url + @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url + end + + def route_hash + @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} + end + + def whitelisted_routes + grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route + end + + def sidekiq_route + request.path.start_with?('/admin/sidekiq') + 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 + end + end +end From 461ecbcf07f0785b5ea50c62b114bf8217ac5199 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 7 Feb 2018 22:55:50 +0800 Subject: [PATCH 5/6] Update peek-performance_bar which doesn't hold env --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index e78c3c5f794..546ba2f2a18 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -601,7 +601,7 @@ GEM atomic (>= 1.0.0) mysql2 peek - peek-performance_bar (1.3.0) + peek-performance_bar (1.3.1) peek (>= 0.1.0) peek-pg (1.3.0) concurrent-ruby From bb4fcb7809aa9d14a60e5c90f11f07fac8f584a8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 3 Mar 2018 00:39:42 +0800 Subject: [PATCH 6/6] Move constants and update for feedback --- lib/gitlab/middleware/read_only.rb | 2 -- lib/gitlab/middleware/read_only/controller.rb | 9 ++++++--- spec/lib/gitlab/middleware/read_only_spec.rb | 16 ++++++++-------- spec/lib/gitlab/middleware/release_env_spec.rb | 8 ++------ 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb index 19b74c0c122..d9d5f90596f 100644 --- a/lib/gitlab/middleware/read_only.rb +++ b/lib/gitlab/middleware/read_only.rb @@ -1,8 +1,6 @@ module Gitlab module Middleware class ReadOnly - DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze - APPLICATION_JSON = 'application/json'.freeze API_VERSIONS = (3..4) def self.internal_routes diff --git a/lib/gitlab/middleware/read_only/controller.rb b/lib/gitlab/middleware/read_only/controller.rb index 053cb6f0a9f..45b644e6510 100644 --- a/lib/gitlab/middleware/read_only/controller.rb +++ b/lib/gitlab/middleware/read_only/controller.rb @@ -2,6 +2,10 @@ module Gitlab module Middleware class ReadOnly class Controller + DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze + APPLICATION_JSON = 'application/json'.freeze + ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance'.freeze + def initialize(app, env) @app = app @env = env @@ -10,12 +14,11 @@ module Gitlab def call 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' if json_request? - return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] + return [403, { 'Content-Type' => APPLICATION_JSON }, [{ 'message' => ERROR_MESSAGE }.to_json]] else - rack_flash.alert = error_message + rack_flash.alert = ERROR_MESSAGE rack_session['flash'] = rack_flash.to_session_value return [301, { 'Location' => last_visited_url }, []] diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb index b3c85142b82..39ec2f37a83 100644 --- a/spec/lib/gitlab/middleware/read_only_spec.rb +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -14,14 +14,14 @@ describe Gitlab::Middleware::ReadOnly do alert = middleware.env['rack.session'].to_hash .dig('flash', 'flashes', 'alert') - alert&.include?('You cannot do writing operations') + alert&.include?('You cannot perform write operations') end end RSpec::Matchers.define :disallow_request_in_json do match do |response| json_response = JSON.parse(response.body) - response.body.include?('You cannot do writing operations') && json_response.key?('message') + response.body.include?('You cannot perform write operations') && json_response.key?('message') end end @@ -47,14 +47,14 @@ describe Gitlab::Middleware::ReadOnly do end end - subject do - app = described_class.new(fake_app) - app.extend(observe_env) - app - end - let(:request) { Rack::MockRequest.new(rack_stack) } + subject do + described_class.new(fake_app).tap do |app| + app.extend(observe_env) + end + end + context 'normal requests to a read-only Gitlab instance' do let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } } diff --git a/spec/lib/gitlab/middleware/release_env_spec.rb b/spec/lib/gitlab/middleware/release_env_spec.rb index 657b705502a..5e3aa877409 100644 --- a/spec/lib/gitlab/middleware/release_env_spec.rb +++ b/spec/lib/gitlab/middleware/release_env_spec.rb @@ -1,16 +1,12 @@ require 'spec_helper' describe Gitlab::Middleware::ReleaseEnv do - let(:inner_app) { double(:app) } + let(:inner_app) { double(:app, call: 'yay') } let(:app) { described_class.new(inner_app) } let(:env) { { 'action_controller.instance' => 'something' } } - before do - expect(inner_app).to receive(:call).with(env).and_return('yay') - end - describe '#call' do - it 'calls the app and delete the controller' do + it 'calls the app and clears the env' do result = app.call(env) expect(result).to eq('yay')