diff options
-rw-r--r-- | app/controllers/health_controller.rb | 5 | ||||
-rw-r--r-- | changelogs/unreleased/sh-simplify-liveness-check.yml | 5 | ||||
-rw-r--r-- | config/application.rb | 4 | ||||
-rw-r--r-- | config/routes.rb | 2 | ||||
-rw-r--r-- | doc/user/admin_area/monitoring/health_check.md | 27 | ||||
-rw-r--r-- | lib/gitlab/middleware/liveness_health_check.rb | 43 | ||||
-rw-r--r-- | spec/controllers/health_controller_spec.rb | 51 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/liveness_health_check_spec.rb | 57 |
8 files changed, 117 insertions, 77 deletions
diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb index 3fedd5bfb29..72f59693adc 100644 --- a/app/controllers/health_controller.rb +++ b/app/controllers/health_controller.rb @@ -18,9 +18,8 @@ class HealthController < ActionController::Base end def liveness - results = CHECKS.map { |check| [check.name, check.liveness] } - - render_check_results(results) + # This should never be called; it should be intercepted by LivenessHealthCheck middleware + head :not_found end def storage_check diff --git a/changelogs/unreleased/sh-simplify-liveness-check.yml b/changelogs/unreleased/sh-simplify-liveness-check.yml new file mode 100644 index 00000000000..7eb108a1300 --- /dev/null +++ b/changelogs/unreleased/sh-simplify-liveness-check.yml @@ -0,0 +1,5 @@ +--- +title: Simplify /-/liveness check to avoid connecting to the database +merge_request: +author: +type: changed diff --git a/config/application.rb b/config/application.rb index b9d4f6765e3..ae4ff5dd7c9 100644 --- a/config/application.rb +++ b/config/application.rb @@ -154,6 +154,10 @@ module Gitlab config.action_view.sanitized_allowed_protocols = %w(smb) + # This middleware needs to precede ActiveRecord::QueryCache and other middlewares that + # connect to the database. + config.middleware.insert_after "Rails::Rack::Logger", "Gitlab::Middleware::LivenessHealthCheck" + config.middleware.insert_after Warden::Manager, Rack::Attack # Allow access to GitLab API from other domains diff --git a/config/routes.rb b/config/routes.rb index e0a9139b1b4..6bf0335a923 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -46,7 +46,7 @@ Rails.application.routes.draw do get 'health_check(/:checks)' => 'health_check#index', as: :health_check scope path: '-' do - get 'liveness' => 'health#liveness' + get 'liveness' => 'health#liveness' # Intercepted via Gitlab::Middleware::LivenessHealthCheck get 'readiness' => 'health#readiness' post 'storage_check' => 'health#storage_check' resources :metrics, only: [:index] diff --git a/doc/user/admin_area/monitoring/health_check.md b/doc/user/admin_area/monitoring/health_check.md index 843fb4ce26b..958962fbfbc 100644 --- a/doc/user/admin_area/monitoring/health_check.md +++ b/doc/user/admin_area/monitoring/health_check.md @@ -27,7 +27,7 @@ With default whitelist settings, the probes can be accessed from localhost: - `http://localhost/-/readiness` - `http://localhost/-/liveness` -which will then provide a report of system health in JSON format. +The readiness endpoint will provide a report of system health in JSON format. Readiness example output: @@ -57,29 +57,12 @@ Readiness example output: } ``` -Liveness example output: +The liveness endpoint only checks whether the application server is running. It does +not verify the database or other services are running. A successful response with return +a 200 status code with the following message: ``` -{ - "fs_shards_check" : { - "status" : "ok" - }, - "cache_check" : { - "status" : "ok" - }, - "db_check" : { - "status" : "ok" - }, - "redis_check" : { - "status" : "ok" - }, - "queues_check" : { - "status" : "ok" - }, - "shared_state_check" : { - "status" : "ok" - } -} +GitLab is alive ``` ## Status diff --git a/lib/gitlab/middleware/liveness_health_check.rb b/lib/gitlab/middleware/liveness_health_check.rb new file mode 100644 index 00000000000..404c506a08e --- /dev/null +++ b/lib/gitlab/middleware/liveness_health_check.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# This middleware provides a health check that does not hit the database. Its purpose +# is to notify the prober that the application server is handling requests, but a 200 +# response does not signify that the database or other services are ready. +# +# See https://thisdata.com/blog/making-a-rails-health-check-that-doesnt-hit-the-database/ for +# more details. + +module Gitlab + module Middleware + class LivenessHealthCheck + # This can't be frozen because Rails::Rack::Logger wraps the body + # rubocop:disable Style/MutableConstant + OK_RESPONSE = [200, { 'Content-Type' => 'text/plain' }, ["GitLab is alive"]] + EMPTY_RESPONSE = [404, { 'Content-Type' => 'text/plain' }, [""]] + # rubocop:enable Style/MutableConstant + LIVENESS_PATH = '/-/liveness' + + def initialize(app) + @app = app + end + + def call(env) + return @app.call(env) unless env['PATH_INFO'] == LIVENESS_PATH + + request = Rack::Request.new(env) + + return OK_RESPONSE if client_ip_whitelisted?(request) + + EMPTY_RESPONSE + end + + def client_ip_whitelisted?(request) + ip_whitelist.any? { |e| e.include?(request.ip) } + end + + def ip_whitelist + @ip_whitelist ||= Settings.monitoring.ip_whitelist.map(&IPAddr.method(:new)) + end + end + end +end diff --git a/spec/controllers/health_controller_spec.rb b/spec/controllers/health_controller_spec.rb index d800ad7c187..789b35dc3de 100644 --- a/spec/controllers/health_controller_spec.rb +++ b/spec/controllers/health_controller_spec.rb @@ -109,55 +109,4 @@ describe HealthController do end end end - - describe '#liveness' do - shared_context 'endpoint responding with liveness data' do - subject { get :liveness } - - it 'responds with liveness checks data' do - subject - - expect(json_response['db_check']['status']).to eq('ok') - expect(json_response['cache_check']['status']).to eq('ok') - expect(json_response['queues_check']['status']).to eq('ok') - expect(json_response['shared_state_check']['status']).to eq('ok') - end - end - - context 'accessed from whitelisted ip' do - before do - allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) - end - - it_behaves_like 'endpoint responding with liveness data' - end - - context 'accessed from not whitelisted ip' do - before do - allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip) - end - - it 'responds with resource not found' do - get :liveness - - expect(response.status).to eq(404) - end - - context 'accessed with valid token' do - context 'token passed in request header' do - before do - request.headers['TOKEN'] = token - end - - it_behaves_like 'endpoint responding with liveness data' - end - - context 'token passed as URL param' do - it_behaves_like 'endpoint responding with liveness data' do - subject { get :liveness, token: token } - end - end - end - end - end end diff --git a/spec/lib/gitlab/middleware/liveness_health_check_spec.rb b/spec/lib/gitlab/middleware/liveness_health_check_spec.rb new file mode 100644 index 00000000000..3dee13b7770 --- /dev/null +++ b/spec/lib/gitlab/middleware/liveness_health_check_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +describe Gitlab::Middleware::LivenessHealthCheck do + let(:app) { double(:app) } + let(:middleware) { described_class.new(app) } + let(:env) { {} } + + describe '#call' do + context 'outside IP' do + before do + env['REMOTE_ADDR'] = '8.8.8.8' + end + + it 'returns a 404' do + env['PATH_INFO'] = described_class::LIVENESS_PATH + + response = middleware.call(env) + + expect(response[0]).to eq(404) + end + + it 'forwards the call for other paths' do + env['PATH_INFO'] = '/' + + expect(app).to receive(:call) + + middleware.call(env) + end + end + + context 'whitelisted IP' do + before do + env['REMOTE_ADDR'] = '127.0.0.1' + end + + it 'returns 200 response when endpoint is hit' do + env['PATH_INFO'] = described_class::LIVENESS_PATH + + expect(app).not_to receive(:call) + + response = middleware.call(env) + + expect(response[0]).to eq(200) + expect(response[1]).to eq({ 'Content-Type' => 'text/plain' }) + expect(response[2]).to eq(['GitLab is alive']) + end + + it 'forwards the call for other paths' do + env['PATH_INFO'] = '/-/readiness' + + expect(app).to receive(:call) + + middleware.call(env) + end + end + end +end |