enhancement: simplify ready and health check handler usage

ocis-pkg/handlers/checkgrpc.go → ocis-pkg/checks/checkgrpc.go

@@ -1,4 +1,4 @@
-package handlers
+package checks
 
 import (
 	"context"
@@ -10,7 +10,7 @@ import (
 )
 
 // NewGRPCCheck checks the reachability of a grpc server.
-func NewGRPCCheck(address string) func(ctx context.Context) error {
+func NewGRPCCheck(address string) func(context.Context) error {
 	return func(_ context.Context) error {
 		conn, err := grpc.NewClient(address, grpc.WithTransportCredentials(insecure.NewCredentials()))
 		if err != nil {

ocis-pkg/handlers/checkhttp.go → ocis-pkg/checks/checkhttp.go

@@ -1,4 +1,4 @@
-package handlers
+package checks
 
 import (
 	"context"
@@ -7,7 +7,7 @@ import (
 	"time"
 )
 
-// NewHttpCheck checks the reachability of a http server.
+// NewHTTPCheck checks the reachability of a http server.
 func NewHTTPCheck(url string) func(context.Context) error {
 	return func(_ context.Context) error {
 		c := http.Client{

ocis-pkg/handlers/checknats.go → ocis-pkg/checks/checknats.go

@@ -1,4 +1,4 @@
-package handlers
+package checks
 
 import (
 	"context"

ocis-pkg/handlers/checktcp.go → ocis-pkg/checks/checktcp.go

@@ -1,4 +1,4 @@
-package handlers
+package checks
 
 import (
 	"context"
@@ -7,7 +7,7 @@ import (
 )
 
 // NewTCPCheck returns a check that connects to a given tcp endpoint.
-func NewTCPCheck(address string) func(ctx context.Context) error {
+func NewTCPCheck(address string) func(context.Context) error {
 	return func(_ context.Context) error {
 		conn, err := net.DialTimeout("tcp", address, 3*time.Second)
 		if err != nil {

ocis-pkg/handlers/checker.go

@@ -13,12 +13,14 @@ import (
 )
 
 // check is a function that performs a check.
-type check func(ctx context.Context) error
+type checker func(ctx context.Context) error
+
+// checks is a map of check names to check functions.
+type checkers map[string]func(ctx context.Context) error
 
 // CheckHandlerConfiguration defines the configuration for the CheckHandler.
 type CheckHandlerConfiguration struct {
-	Checks map[string]check
-
+	checks        checkers
 	logger        log.Logger
 	limit         int
 	statusFailed  int
@@ -28,7 +30,7 @@ type CheckHandlerConfiguration struct {
 // NewCheckHandlerConfiguration initializes a new CheckHandlerConfiguration.
 func NewCheckHandlerConfiguration() CheckHandlerConfiguration {
 	return CheckHandlerConfiguration{
-		Checks: make(map[string]check),
+		checks: make(checkers),
 
 		limit:         -1,
 		statusFailed:  http.StatusInternalServerError,
@@ -43,18 +45,18 @@ func (c CheckHandlerConfiguration) WithLogger(l log.Logger) CheckHandlerConfigur
 }
 
 // WithCheck sets a check for the CheckHandlerConfiguration.
-func (c CheckHandlerConfiguration) WithCheck(name string, f check) CheckHandlerConfiguration {
-	if _, ok := c.Checks[name]; ok {
+func (c CheckHandlerConfiguration) WithCheck(name string, check checker) CheckHandlerConfiguration {
+	if _, ok := c.checks[name]; ok {
 		c.logger.Panic().Str("check", name).Msg("check already exists")
 	}
 
-	c.Checks[name] = f
+	c.checks[name] = check
 	return c
 }
 
-// WithInheritedChecksFrom appends the checks from another CheckHandlerConfiguration.
-func (c CheckHandlerConfiguration) WithInheritedChecksFrom(other CheckHandlerConfiguration) CheckHandlerConfiguration {
-	for name, check := range other.Checks {
+// WithChecks adds multiple checks to the CheckHandlerConfiguration.
+func (c CheckHandlerConfiguration) WithChecks(checks checkers) CheckHandlerConfiguration {
+	for name, check := range checks {
 		c.WithCheck(name, check)
 	}
 
@@ -81,27 +83,26 @@ func (c CheckHandlerConfiguration) WithStatusSuccess(status int) CheckHandlerCon
 
 // CheckHandler is a http Handler that performs different checks.
 type CheckHandler struct {
-	Conf CheckHandlerConfiguration
+	conf CheckHandlerConfiguration
 }
 
 // NewCheckHandler initializes a new CheckHandler.
 func NewCheckHandler(c CheckHandlerConfiguration) *CheckHandler {
-	c.Checks = maps.Clone(c.Checks) // prevent check duplication after initialization
+	c.checks = maps.Clone(c.checks) // prevent check duplication after initialization
 	return &CheckHandler{
-		Conf: c,
+		conf: c,
 	}
 }
 
-// AddCheck adds a check to the CheckHandler.
-func (h *CheckHandler) AddCheck(name string, c check) {
-	h.Conf.WithCheck(name, c)
+func (h *CheckHandler) Checks() map[string]func(ctx context.Context) error {
+	return maps.Clone(h.conf.checks)
 }
 
 func (h *CheckHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	g, ctx := errgroup.WithContext(r.Context())
-	g.SetLimit(h.Conf.limit)
+	g.SetLimit(h.conf.limit)
 
-	for name, check := range h.Conf.Checks {
+	for name, check := range h.conf.checks {
 		checker := check
 		checkerName := name
 		g.Go(func() error { // https://go.dev/blog/loopvar-preview per iteration scope since go 1.22
@@ -113,16 +114,16 @@ func (h *CheckHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		})
 	}
 
-	status := h.Conf.statusSuccess
+	status := h.conf.statusSuccess
 	if err := g.Wait(); err != nil {
-		status = h.Conf.statusFailed
-		h.Conf.logger.Error().Err(err).Msg("check failed")
+		status = h.conf.statusFailed
+		h.conf.logger.Error().Err(err).Msg("check failed")
 	}
 
 	w.Header().Set("Content-Type", "text/plain")
 	w.WriteHeader(status)
 
 	if _, err := io.WriteString(w, http.StatusText(status)); err != nil { // io.WriteString should not fail, but if it does, we want to know.
-		h.Conf.logger.Panic().Err(err).Msg("failed to write response")
+		h.conf.logger.Panic().Err(err).Msg("failed to write response")
 	}
 }

ocis-pkg/handlers/checker_test.go

@@ -1,29 +1,125 @@
 package handlers_test
 
 import (
+	"bytes"
 	"context"
-	"fmt"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"slices"
 	"testing"
 
+	"github.com/test-go/testify/require"
+	"github.com/tidwall/gjson"
+
 	"github.com/owncloud/ocis/v2/ocis-pkg/handlers"
+	"github.com/owncloud/ocis/v2/ocis-pkg/log"
 )
 
-func TestCheckHandler_AddCheck(t *testing.T) {
-	c := handlers.NewCheckHandlerConfiguration().WithCheck("shared-check", func(ctx context.Context) error { return nil })
+func TestCheckHandlerConfiguration(t *testing.T) {
+	nopCheck := func(_ context.Context) error { return nil }
+	handlerConfiguration := handlers.NewCheckHandlerConfiguration().WithCheck("check-1", nopCheck)
+
+	t.Run("add check", func(t *testing.T) {
+		handlerConfiguration.WithCheck("check-2", nopCheck)
+		require.Equal(t, 2, len(handlers.NewCheckHandler(handlerConfiguration).Checks()))
+	})
+
+	t.Run("add checks", func(t *testing.T) {
+		handlerConfiguration.WithChecks(map[string]func(ctx context.Context) error{"check-3": nopCheck, "check-4": nopCheck})
+		require.Equal(t, 4, len(handlers.NewCheckHandler(handlerConfiguration).Checks()))
+	})
 
-	t.Run("configured checks are unique once added", func(t *testing.T) {
+	t.Run("checks are unique", func(t *testing.T) {
 		defer func() {
-			if r := recover(); r != nil {
-				t.Errorf("checks should be unique, got %v", r)
+			if r := recover(); r == nil {
+				t.Error("checks should be unique")
 			}
 		}()
 
-		h1 := handlers.NewCheckHandler(c)
-		h1.AddCheck("check-with-same-name", func(ctx context.Context) error { return nil })
+		handlerConfiguration.WithCheck("check-1", nopCheck)
+	})
+}
+
+func TestCheckHandler(t *testing.T) {
+	checkFactory := func(err error) func(ctx context.Context) error {
+		return func(ctx context.Context) error {
+			if err != nil {
+				return err
+			}
+
+			<-ctx.Done()
+			return nil
+		}
+	}
+
+	t.Run("passes with custom status", func(t *testing.T) {
+		rec := httptest.NewRecorder()
+		handler := handlers.NewCheckHandler(
+			handlers.
+				NewCheckHandlerConfiguration().
+				WithStatusSuccess(http.StatusCreated),
+		)
+
+		handler.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
+		require.Equal(t, http.StatusCreated, rec.Code)
+		require.Equal(t, http.StatusText(http.StatusCreated), rec.Body.String())
+	})
+
+	t.Run("is not ok if any check fails", func(t *testing.T) {
+		rec := httptest.NewRecorder()
+		handler := handlers.NewCheckHandler(
+			handlers.
+				NewCheckHandlerConfiguration().
+				WithCheck("check-1", checkFactory(errors.New("failed"))),
+		)
+		handler.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
+		require.Equal(t, http.StatusInternalServerError, rec.Code)
+		require.Equal(t, http.StatusText(http.StatusInternalServerError), rec.Body.String())
+	})
+
+	t.Run("fails with custom status", func(t *testing.T) {
+		rec := httptest.NewRecorder()
+		handler := handlers.NewCheckHandler(
+			handlers.
+				NewCheckHandlerConfiguration().
+				WithCheck("check-1", checkFactory(errors.New("failed"))).
+				WithStatusFailed(http.StatusTeapot),
+		)
+		handler.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
+		require.Equal(t, http.StatusTeapot, rec.Code)
+		require.Equal(t, http.StatusText(http.StatusTeapot), rec.Body.String())
+	})
 
-		h2 := handlers.NewCheckHandler(c)
-		h2.AddCheck("check-with-same-name", func(ctx context.Context) error { return nil })
+	t.Run("exits all other running tests on failure", func(t *testing.T) {
+		var errs []error
+		rec := httptest.NewRecorder()
+		buffer := &bytes.Buffer{}
+		logger := log.Logger{Logger: log.NewLogger().Output(buffer)}
+		handler := handlers.NewCheckHandler(
+			handlers.
+				NewCheckHandlerConfiguration().
+				WithLogger(logger).
+				WithCheck("check-1", func(ctx context.Context) error {
+					err := checkFactory(nil)(ctx)
+					errs = append(errs, err)
+					return err
+				}).
+				WithCheck("check-2", func(ctx context.Context) error {
+					err := checkFactory(errors.New("failed"))(ctx)
+					errs = append(errs, err)
+					return err
+				}).
+				WithCheck("check-3", func(ctx context.Context) error {
+					err := checkFactory(nil)(ctx)
+					errs = append(errs, err)
+					return err
+				}),
+		)
+		handler.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
 
-		fmt.Print(1)
+		require.Equal(t, "'check-2': failed", gjson.Get(buffer.String(), "error").String())
+		require.Equal(t, 1, len(slices.DeleteFunc(errs, func(err error) bool { return err == nil })))
+		require.Equal(t, 2, len(slices.DeleteFunc(errs, func(err error) bool { return err != nil })))
 	})
 }

services/activitylog/pkg/server/debug/server.go

@@ -3,6 +3,7 @@ package debug
 import (
 	"net/http"
 
+	"github.com/owncloud/ocis/v2/ocis-pkg/checks"
 	"github.com/owncloud/ocis/v2/ocis-pkg/handlers"
 	"github.com/owncloud/ocis/v2/ocis-pkg/service/debug"
 	"github.com/owncloud/ocis/v2/ocis-pkg/version"
@@ -15,14 +16,14 @@ func Server(opts ...Option) (*http.Server, error) {
 	healthHandler := handlers.NewCheckHandler(
 		handlers.NewCheckHandlerConfiguration().
 			WithLogger(options.Logger).
-			WithCheck("http reachability", handlers.NewHTTPCheck(options.Config.HTTP.Addr)),
+			WithCheck("http reachability", checks.NewHTTPCheck(options.Config.HTTP.Addr)),
 	)
 
 	readyHandler := handlers.NewCheckHandler(
 		handlers.NewCheckHandlerConfiguration().
 			WithLogger(options.Logger).
-			WithCheck("nats reachability", handlers.NewNatsCheck(options.Config.Events.Cluster)).
-			WithInheritedChecksFrom(healthHandler.Conf),
+			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
+			WithChecks(healthHandler.Checks()),
 	)
 
 	return debug.NewService(

services/antivirus/pkg/server/debug/server.go

@@ -6,6 +6,8 @@ import (
 	"net/http"
 
 	"github.com/dutchcoders/go-clamd"
+
+	"github.com/owncloud/ocis/v2/ocis-pkg/checks"
 	"github.com/owncloud/ocis/v2/ocis-pkg/handlers"
 	"github.com/owncloud/ocis/v2/ocis-pkg/service/debug"
 	"github.com/owncloud/ocis/v2/ocis-pkg/version"
@@ -23,7 +25,7 @@ func Server(opts ...Option) (*http.Server, error) {
 	readyHandler := handlers.NewCheckHandler(
 		handlers.NewCheckHandlerConfiguration().
 			WithLogger(options.Logger).
-			WithCheck("nats reachability", handlers.NewNatsCheck(options.Config.Events.Cluster)).
+			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
 			WithCheck("antivirus reachability", func(ctx context.Context) error {
 				cfg := options.Config
 				switch cfg.Scanner.Type {
@@ -32,7 +34,7 @@ func Server(opts ...Option) (*http.Server, error) {
 				case "clamav":
 					return clamd.NewClamd(cfg.Scanner.ClamAV.Socket).Ping()
 				case "icap":
-					return handlers.NewTCPCheck(cfg.Scanner.ICAP.URL)(ctx)
+					return checks.NewTCPCheck(cfg.Scanner.ICAP.URL)(ctx)
 				}
 			}),
 	)

services/audit/pkg/server/debug/server.go

@@ -3,6 +3,7 @@ package debug
 import (
 	"net/http"
 
+	"github.com/owncloud/ocis/v2/ocis-pkg/checks"
 	"github.com/owncloud/ocis/v2/ocis-pkg/handlers"
 	"github.com/owncloud/ocis/v2/ocis-pkg/service/debug"
 	"github.com/owncloud/ocis/v2/ocis-pkg/version"
@@ -20,8 +21,8 @@ func Server(opts ...Option) (*http.Server, error) {
 	readyHandler := handlers.NewCheckHandler(
 		handlers.NewCheckHandlerConfiguration().
 			WithLogger(options.Logger).
-			WithCheck("nats reachability", handlers.NewNatsCheck(options.Config.Events.Cluster)).
-			WithInheritedChecksFrom(checkHandler.Conf),
+			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
+			WithChecks(checkHandler.Checks()),
 	)
 
 	return debug.NewService(

services/clientlog/pkg/server/debug/server.go

@@ -3,6 +3,7 @@ package debug
 import (
 	"net/http"
 
+	"github.com/owncloud/ocis/v2/ocis-pkg/checks"
 	"github.com/owncloud/ocis/v2/ocis-pkg/handlers"
 	"github.com/owncloud/ocis/v2/ocis-pkg/service/debug"
 	"github.com/owncloud/ocis/v2/ocis-pkg/version"
@@ -20,8 +21,8 @@ func Server(opts ...Option) (*http.Server, error) {
 	readyHandler := handlers.NewCheckHandler(
 		handlers.NewCheckHandlerConfiguration().
 			WithLogger(options.Logger).
-			WithCheck("nats reachability", handlers.NewNatsCheck(options.Config.Events.Cluster)).
-			WithInheritedChecksFrom(checkHandler.Conf),
+			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
+			WithChecks(checkHandler.Checks()),
 	)
 
 	return debug.NewService(