server: Update health check to use plugin status

The health check now supports a `?plugin` option which will make the
response depend on whether or not all configured plugins are in an OK
state.

The `bundle` parameter will now use the bundle *and* discovery plugin
statuses to determine if the bundles are ready. This corrects an issue
where discovery bundles, and bundles defined by the discovery dynamic
config, were not included with `/health?bundle=true` checks.

The URL parameter parsing has also changed to allow for omitting the
value for the `bundle` option. It will default to `true` so that
`/health?bundle=true` can be shortened to `/health?bundle`.

Fixes: #2010
Fixes: #2015
Signed-off-by: Patrick East <east.patrick@gmail.com>

docs/content/kubernetes-tutorial.md

@@ -187,7 +187,7 @@ spec:
               name: opa-server
           readinessProbe:
             httpGet:
-              path: /health
+              path: /health?plugins&bundle
               scheme: HTTPS
               port: 443
             initialDelaySeconds: 3

docs/content/rest-api.md

@@ -2046,12 +2046,15 @@ that the server is operational. Optionally it can account for bundle activation
 
 #### Query Parameters
 `bundle` - Boolean parameter to account for bundle activation status in response.
+`plugins` - Boolean parameter to account for plugin status in response.
 
 #### Status Codes
 - **200** - OPA service is healthy. If `bundle=true` then all configured bundles have
-            been activated.
+            been activated. If the `plugins` option is specified then all plugins are in an OK state.
 - **500** - OPA service is not healthy. If `bundle=true` this can mean any of the configured
-            bundles have not yet been activated.
+            bundles have not yet been activated. If the `plugins` option is specified then at least one
+            plugin is in a non-OK state.
+
 
 > *Note*: The bundle activation check is only for initial startup. Subsequent downloads
   will not affect the health check. The [Status](../management/#status)
@@ -2067,6 +2070,11 @@ GET /health HTTP/1.1
 GET /health?bundle=true HTTP/1.1
 ```
 
+#### Example Request (plugin status)
+```http
+GET /health?plugins HTTP/1.1
+```
+
 #### Healthy Response
 ```http
 HTTP/1.1 200 OK

server/server.go

@@ -110,8 +110,6 @@ type Server struct {
 	pprofEnabled        bool
 	runtime             *ast.Term
 	httpListeners       []httpListener
-	bundleStatuses      map[string]*bundlePlugin.Status
-	bundleStatusMtx     sync.RWMutex
 	metrics             Metrics
 	defaultDecisionPath string
 }
@@ -180,18 +178,6 @@ func (s *Server) Init(ctx context.Context) (*Server, error) {
 	s.partials = map[string]rego.PartialResult{}
 	s.preparedEvalQueries = newCache(pqMaxCacheSize)
 
-	bp := bundlePlugin.Lookup(s.manager)
-	if bp != nil {
-
-		// initialize statuses to empty defaults for server /health check
-		s.bundleStatuses = map[string]*bundlePlugin.Status{}
-		for bundleName := range bp.Config().Bundles {
-			s.bundleStatuses[bundleName] = &bundlePlugin.Status{Name: bundleName}
-		}
-
-		bp.RegisterBulkListener("REST API Server", s.updateBundleStatus)
-	}
-
 	// Check if there is a bundle revision available at the legacy storage path
 	rev, err := bundle.LegacyReadRevisionFromStore(ctx, s.store, txn)
 	if err == nil && rev != "" {
@@ -871,12 +857,6 @@ func (s *Server) getCachedPreparedEvalQuery(key string, m metrics.Metrics) (*reg
 	return nil, false
 }
 
-func (s *Server) updateBundleStatus(status map[string]*bundlePlugin.Status) {
-	s.bundleStatusMtx.Lock()
-	defer s.bundleStatusMtx.Unlock()
-	s.bundleStatuses = status
-}
-
 func (s *Server) canEval(ctx context.Context) bool {
 	// Create very simple query that binds a single variable.
 	eval := rego.New(rego.Compiler(s.getCompiler()),
@@ -886,7 +866,7 @@ func (s *Server) canEval(ctx context.Context) bool {
 	if err != nil {
 		return false
 	}
-	type emptyObject struct{}
+
 	v, ok := rs[0].Bindings["x"]
 	if ok {
 		jsonNumber, ok := v.(json.Number)
@@ -897,39 +877,73 @@ func (s *Server) canEval(ctx context.Context) bool {
 	return false
 }
 
-func (s *Server) bundlesActivated() bool {
-	s.bundleStatusMtx.RLock()
-	defer s.bundleStatusMtx.RUnlock()
+func (s *Server) bundlesReady(pluginStatuses map[string]*plugins.Status) bool {
 
-	for _, status := range s.bundleStatuses {
-		// Ensure that all of the bundle statuses have an activation time set on them
-		if (status.LastSuccessfulActivation == time.Time{}) {
-			return false
-		}
+	// Look for a discovery plugin first, if it exists and isn't ready
+	// then don't bother with the others.
+	// Note: use "discovery" instead of `discovery.Name` to avoid import
+	// cycle problems..
+	dpStatus, ok := pluginStatuses["discovery"]
+	if ok && dpStatus != nil && (dpStatus.State != plugins.StateOK) {
+		return false
+	}
+
+	// The bundle plugin won't return "OK" until the first activation
+	// of each configured bundle.
+	bpStatus, ok := pluginStatuses[bundlePlugin.Name]
+	if ok && bpStatus != nil && (bpStatus.State != plugins.StateOK) {
+		return false
 	}
 
 	return true
 }
 
 func (s *Server) unversionedGetHealth(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
-	includeBundleStatus := getBoolParam(r.URL, types.ParamBundleActivationV1, false)
+	includeBundleStatus := getBoolParam(r.URL, types.ParamBundleActivationV1, true)
+	includePluginStatus := getBoolParam(r.URL, types.ParamPluginsV1, true)
 
 	// Ensure the server can evaluate a simple query
-	type emptyObject struct{}
 	if !s.canEval(ctx) {
-		writer.JSON(w, http.StatusInternalServerError, emptyObject{}, false)
+		writeHealthResponse(w, errors.New("unable to perform evaluation"), struct{}{})
 		return
 	}
 
+	pluginStatuses := s.manager.PluginStatus()
+
 	// Ensure that bundles (if configured, and requested to be included in the result)
-	// have been activated successfully.
-	if includeBundleStatus && s.hasBundle() && !s.bundlesActivated() {
-		writer.JSON(w, http.StatusInternalServerError, emptyObject{}, false)
+	// have been activated successfully. This will include discovery bundles as well as
+	// normal bundles that are configured.
+	if includeBundleStatus && !s.bundlesReady(pluginStatuses) {
+		// For backwards compatibility we don't return a payload with statuses for the bundle endpoint
+		writeHealthResponse(w, errors.New("not all configured bundles have been activated"), struct{}{})
 		return
 	}
 
-	writer.JSON(w, http.StatusOK, emptyObject{}, false)
+	if includePluginStatus {
+		// Ensure that all plugins (if requested to be included in the result) have an OK status.
+		hasErr := false
+		for _, status := range pluginStatuses {
+			if status.State != plugins.StateOK {
+				hasErr = true
+				break
+			}
+		}
+		if hasErr {
+			writeHealthResponse(w, errors.New("not all plugins in OK state"), struct{}{})
+			return
+		}
+	}
+	writeHealthResponse(w, nil, struct{}{})
+}
+
+func writeHealthResponse(w http.ResponseWriter, err error, payload interface{}) {
+	status := http.StatusOK
+	if err != nil {
+		status = http.StatusInternalServerError
+	}
+
+	writer.JSON(w, status, payload, false)
 }
 
 func (s *Server) v1CompilePost(w http.ResponseWriter, r *http.Request) {
@@ -2128,10 +2142,6 @@ func (s *Server) getProvenance() *types.ProvenanceV1 {
 	return p
 }
 
-func (s *Server) hasBundle() bool {
-	return bundlePlugin.Lookup(s.manager) != nil || s.legacyRevision != ""
-}
-
 func (s *Server) hasLegacyBundle() bool {
 	bp := bundlePlugin.Lookup(s.manager)
 	return s.legacyRevision != "" || (bp != nil && !bp.Config().IsMultiBundle())