Use /-/health instead of breaking /-/liveness

8 files changed, 101 insertions, 21 deletions

diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb
index 72f59693adc..3fedd5bfb29 100644
--- a/app/controllers/health_controller.rb
+++ b/app/controllers/health_controller.rb
@@ -18,8 +18,9 @@ class HealthController < ActionController::Base
   end
 
   def liveness
-    # This should never be called; it should be intercepted by LivenessHealthCheck middleware
-    head :not_found
+    results = CHECKS.map { |check| [check.name, check.liveness] }
+
+    render_check_results(results)
   end
 
   def storage_check
diff --git a/changelogs/unreleased/sh-simplify-liveness-check.yml b/changelogs/unreleased/sh-simplify-liveness-check.yml
index 7eb108a1300..225e3dc1378 100644
--- a/changelogs/unreleased/sh-simplify-liveness-check.yml
+++ b/changelogs/unreleased/sh-simplify-liveness-check.yml
@@ -1,5 +1,5 @@
 ---
-title: Simplify /-/liveness check to avoid connecting to the database
-merge_request:
+title: Add /-/health basic health check endpoint
+merge_request: 20456
 author:
-type: changed
+type: added
diff --git a/config/application.rb b/config/application.rb
index ae4ff5dd7c9..a086e860e16 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -156,7 +156,7 @@ module Gitlab
 
     # 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 "Rails::Rack::Logger", "Gitlab::Middleware::BasicHealthCheck"
 
     config.middleware.insert_after Warden::Manager, Rack::Attack
 
diff --git a/config/routes.rb b/config/routes.rb
index 6bf0335a923..aac3a77c4f1 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -46,7 +46,8 @@ Rails.application.routes.draw do
   get 'health_check(/:checks)' => 'health_check#index', as: :health_check
 
   scope path: '-' do
-    get 'liveness' => 'health#liveness' # Intercepted via Gitlab::Middleware::LivenessHealthCheck
+    # '/-/health' implemented by BasicHealthMiddleware
+    get 'liveness' => 'health#liveness'
     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 958962fbfbc..6cbca6bd1db 100644
--- a/doc/user/admin_area/monitoring/health_check.md
+++ b/doc/user/admin_area/monitoring/health_check.md
@@ -20,14 +20,24 @@ To access monitoring resources, the client IP needs to be included in a whitelis
 
 [Read how to add IPs to a whitelist for the monitoring endpoints][admin].
 
-## Using the endpoint
+## Using the endpoints
 
 With default whitelist settings, the probes can be accessed from localhost:
 
+- `http://localhost/-/health`
 - `http://localhost/-/readiness`
 - `http://localhost/-/liveness`
 
-The readiness endpoint will provide a report of system health in JSON format.
+
+The first endpoint, `/-/health/`, 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:
+
+```
+GitLab OK
+```
+
+The readiness and liveness probes will provide a report of system health in JSON format.
 
 Readiness example output:
 
@@ -57,12 +67,29 @@ Readiness 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:
+Liveness example output:
 
 ```
-GitLab is alive
+{
+   "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"
+   }
+}
 ```
 
 ## Status
diff --git a/lib/gitlab/middleware/liveness_health_check.rb b/lib/gitlab/middleware/basic_health_check.rb
index 404c506a08e..f2a03217098 100644
--- a/lib/gitlab/middleware/liveness_health_check.rb
+++ b/lib/gitlab/middleware/basic_health_check.rb
@@ -9,20 +9,20 @@
 
 module Gitlab
   module Middleware
-    class LivenessHealthCheck
+    class BasicHealthCheck
       # 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"]]
+      OK_RESPONSE = [200, { 'Content-Type' => 'text/plain' }, ["GitLab OK"]]
       EMPTY_RESPONSE = [404, { 'Content-Type' => 'text/plain' }, [""]]
       # rubocop:enable Style/MutableConstant
-      LIVENESS_PATH = '/-/liveness'
+      HEALTH_PATH = '/-/health'
 
       def initialize(app)
         @app = app
       end
 
       def call(env)
-        return @app.call(env) unless env['PATH_INFO'] == LIVENESS_PATH
+        return @app.call(env) unless env['PATH_INFO'] == HEALTH_PATH
 
         request = Rack::Request.new(env)
 
diff --git a/spec/controllers/health_controller_spec.rb b/spec/controllers/health_controller_spec.rb
index 789b35dc3de..d800ad7c187 100644
--- a/spec/controllers/health_controller_spec.rb
+++ b/spec/controllers/health_controller_spec.rb
@@ -109,4 +109,55 @@ 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/basic_health_check_spec.rb
index 3dee13b7770..187d903a5e1 100644
--- a/spec/lib/gitlab/middleware/liveness_health_check_spec.rb
+++ b/spec/lib/gitlab/middleware/basic_health_check_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Middleware::LivenessHealthCheck do
+describe Gitlab::Middleware::BasicHealthCheck do
   let(:app) { double(:app) }
   let(:middleware) { described_class.new(app) }
   let(:env) { {} }
@@ -12,7 +12,7 @@ describe Gitlab::Middleware::LivenessHealthCheck do
       end
 
       it 'returns a 404' do
-        env['PATH_INFO'] = described_class::LIVENESS_PATH
+        env['PATH_INFO'] = described_class::HEALTH_PATH
 
         response = middleware.call(env)
 
@@ -34,7 +34,7 @@ describe Gitlab::Middleware::LivenessHealthCheck do
       end
 
       it 'returns 200 response when endpoint is hit' do
-        env['PATH_INFO'] = described_class::LIVENESS_PATH
+        env['PATH_INFO'] = described_class::HEALTH_PATH
 
         expect(app).not_to receive(:call)
 
@@ -42,7 +42,7 @@ describe Gitlab::Middleware::LivenessHealthCheck do
 
         expect(response[0]).to eq(200)
         expect(response[1]).to eq({ 'Content-Type' => 'text/plain' })
-        expect(response[2]).to eq(['GitLab is alive'])
+        expect(response[2]).to eq(['GitLab OK'])
       end
 
       it 'forwards the call for other paths' do