Simplify /-/liveness check to avoid connecting to the databasesh-simplify-liveness-check

The previous implementation would hit the database each time
and provide a dummy response. If the database goes down, this
means all application workers would be taken out of service.
Simplify this check by using a Rails middleware that intercepts
this endpoint and returns a 200 response.

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 97bc86b3e3a..5dd75ca8400 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -151,6 +151,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