Bugfix: Fix health and ready endpoint checker configurations

owncloud · Oct 17, 2024 · 7ad0afa · 7ad0afa
1 parent 08b3362
commit 7ad0afa
Show file tree

Hide file tree

Showing 13 changed files with 99 additions and 155 deletions.
diff --git a/changelog/unreleased/fix-health-and-ready-endpoints.md b/changelog/unreleased/fix-health-and-ready-endpoints.md
@@ -0,0 +1,13 @@
+Bugfix: Fix health and ready endpoints
+
+We added new checks to the `/readyz` and `/healthz` endpoints to ensure that the services are ready and healthy.
+This change ensures that the endpoints return the correct status codes, which is needed to stabilize the k8s deployments.
+
+https://github.com/owncloud/ocis/pull/10163
+https://github.com/owncloud/ocis/pull/10301
+https://github.com/owncloud/ocis/pull/10302
+https://github.com/owncloud/ocis/pull/10303
+https://github.com/owncloud/ocis/pull/10308
+https://github.com/owncloud/ocis/pull/10323
+https://github.com/owncloud/ocis/pull/10163
+https://github.com/owncloud/ocis/issues/10316
diff --git a/ocis-pkg/handlers/checker.go b/ocis-pkg/handlers/checker.go
@@ -15,12 +15,9 @@ import (
 // check is a function that performs a check.
 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        checkers
+	checks        map[string]checker
 	logger        log.Logger
 	limit         int
 	statusFailed  int
@@ -30,7 +27,7 @@ type CheckHandlerConfiguration struct {
 // NewCheckHandlerConfiguration initializes a new CheckHandlerConfiguration.
 func NewCheckHandlerConfiguration() CheckHandlerConfiguration {
 	return CheckHandlerConfiguration{
-		checks: make(checkers),
+		checks: make(map[string]checker),
 
 		limit:         -1,
 		statusFailed:  http.StatusInternalServerError,
@@ -56,15 +53,6 @@ func (c CheckHandlerConfiguration) WithCheck(name string, check checker) CheckHa
 	return c
 }
 
-// WithChecks adds multiple checks to the CheckHandlerConfiguration.
-func (c CheckHandlerConfiguration) WithChecks(checks checkers) CheckHandlerConfiguration {
-	for name, check := range checks {
-		c.checks = c.WithCheck(name, check).checks
-	}
-
-	return c
-}
-
 // WithLimit limits the number of active goroutines for the checks to at most n
 func (c CheckHandlerConfiguration) WithLimit(n int) CheckHandlerConfiguration {
 	c.limit = n
@@ -96,10 +84,6 @@ func NewCheckHandler(c CheckHandlerConfiguration) *CheckHandler {
 	}
 }
 
-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)

diff --git a/ocis-pkg/handlers/checker_test.go b/ocis-pkg/handlers/checker_test.go
@@ -17,19 +17,18 @@ import (
 )
 
 func TestCheckHandlerConfiguration(t *testing.T) {
-	nopCheck := func(_ context.Context) error { return nil }
+	nopCheckCounter := 0
+	nopCheck := func(_ context.Context) error { nopCheckCounter++; return nil }
 	handlerConfiguration := handlers.NewCheckHandlerConfiguration().WithCheck("check-1", nopCheck)
 
 	t.Run("add check", func(t *testing.T) {
-		inheritedHandlerConfiguration := handlerConfiguration.WithCheck("check-2", nopCheck)
-		require.Equal(t, 1, len(handlers.NewCheckHandler(handlerConfiguration).Checks()))
-		require.Equal(t, 2, len(handlers.NewCheckHandler(inheritedHandlerConfiguration).Checks()))
-	})
+		localCounter := 0
+		handlers.NewCheckHandler(handlerConfiguration).ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil))
+		require.Equal(t, 1, nopCheckCounter)
 
-	t.Run("add checks", func(t *testing.T) {
-		inheritedHandlerConfiguration := handlerConfiguration.WithChecks(map[string]func(ctx context.Context) error{"check-2": nopCheck, "check-3": nopCheck})
-		require.Equal(t, 1, len(handlers.NewCheckHandler(handlerConfiguration).Checks()))
-		require.Equal(t, 3, len(handlers.NewCheckHandler(inheritedHandlerConfiguration).Checks()))
+		handlers.NewCheckHandler(handlerConfiguration.WithCheck("check-2", func(_ context.Context) error { localCounter++; return nil })).ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil))
+		require.Equal(t, 2, nopCheckCounter)
+		require.Equal(t, 1, localCounter)
 	})
 
 	t.Run("checks are unique", func(t *testing.T) {
@@ -40,6 +39,7 @@ func TestCheckHandlerConfiguration(t *testing.T) {
 		}()
 
 		handlerConfiguration.WithCheck("check-1", nopCheck)
+		require.Equal(t, 3, nopCheckCounter)
 	})
 }
 

diff --git a/ocis-pkg/service/debug/service.go b/ocis-pkg/service/debug/service.go
@@ -16,6 +16,8 @@ import (
 	graphMiddleware "github.com/owncloud/ocis/v2/services/graph/pkg/middleware"
 )
 
+var notImplementedEndpoint = func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }
+
 // NewService initializes a new debug service.
 func NewService(opts ...Option) *http.Server {
 	dopts := newOptions(opts...)
@@ -29,12 +31,20 @@ func NewService(opts ...Option) *http.Server {
 		promhttp.Handler(),
 	))
 
-	if dopts.Health != nil {
-		mux.Handle("/healthz", dopts.Health)
+	{ // healthiness check
+		var h http.Handler = http.HandlerFunc(notImplementedEndpoint)
+		if dopts.Health != nil {
+			h = dopts.Health
+		}
+		mux.Handle("/healthz", h)
 	}
 
-	if dopts.Ready != nil {
-		mux.Handle("/readyz", dopts.Ready)
+	{ // readiness check
+		var h http.Handler = http.HandlerFunc(notImplementedEndpoint)
+		if dopts.Ready != nil {
+			h = dopts.Ready
+		}
+		mux.Handle("/readyz", h)
 	}
 
 	if dopts.ConfigDump != nil {

diff --git a/services/activitylog/pkg/server/debug/server.go b/services/activitylog/pkg/server/debug/server.go
@@ -13,18 +13,12 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	healthHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("http reachability", checks.NewHTTPCheck(options.Config.HTTP.Addr)),
-	)
+	healthHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("http reachability", checks.NewHTTPCheck(options.Config.HTTP.Addr))
 
-	readyHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
-			WithChecks(healthHandler.Checks()),
-	)
+	readyHandlerConfiguration := healthHandlerConfiguration.
+		WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster))
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -34,7 +28,7 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(healthHandler),
-		debug.Ready(readyHandler),
+		debug.Health(handlers.NewCheckHandler(healthHandlerConfiguration)),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }
diff --git a/services/audit/pkg/server/debug/server.go b/services/audit/pkg/server/debug/server.go
@@ -13,17 +13,9 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	checkHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger),
-	)
-
-	readyHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
-			WithChecks(checkHandler.Checks()),
-	)
+	readyHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster))
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -33,7 +25,6 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(checkHandler),
-		debug.Ready(readyHandler),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }
diff --git a/services/clientlog/pkg/server/debug/server.go b/services/clientlog/pkg/server/debug/server.go
@@ -13,17 +13,9 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	checkHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger),
-	)
-
-	readyHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
-			WithChecks(checkHandler.Checks()),
-	)
+	readyHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster))
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -33,7 +25,6 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(checkHandler),
-		debug.Ready(readyHandler),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }
diff --git a/services/eventhistory/pkg/server/debug/server.go b/services/eventhistory/pkg/server/debug/server.go
@@ -13,18 +13,12 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	checkHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("grpc reachability", checks.NewGRPCCheck(options.Config.GRPC.Addr)),
-	)
+	healthHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("grpc reachability", checks.NewGRPCCheck(options.Config.GRPC.Addr))
 
-	readyHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
-			WithChecks(checkHandler.Checks()),
-	)
+	readyHandlerConfiguration := healthHandlerConfiguration.
+		WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster))
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -34,7 +28,7 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(checkHandler),
-		debug.Ready(readyHandler),
+		debug.Health(handlers.NewCheckHandler(healthHandlerConfiguration)),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }
diff --git a/services/idp/pkg/server/debug/server.go b/services/idp/pkg/server/debug/server.go
@@ -13,18 +13,12 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	healthHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("http reachability", checks.NewHTTPCheck(options.Config.HTTP.Addr)),
-	)
+	healthHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("http reachability", checks.NewHTTPCheck(options.Config.HTTP.Addr))
 
-	readinessHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("ldap-check", checks.NewTCPCheck(options.Config.Ldap.URI)).
-			WithChecks(healthHandler.Checks()),
-	)
+	readyHandlerConfiguration := healthHandlerConfiguration.
+		WithCheck("ldap-check", checks.NewTCPCheck(options.Config.Ldap.URI))
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -34,7 +28,7 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(healthHandler),
-		debug.Ready(readinessHandler),
+		debug.Health(handlers.NewCheckHandler(healthHandlerConfiguration)),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }
diff --git a/services/notifications/pkg/server/debug/server.go b/services/notifications/pkg/server/debug/server.go
@@ -14,18 +14,10 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	checkHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger),
-	)
-
-	readyHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Notifications.Events.Cluster)).
-			WithCheck("smtp-check", checks.NewTCPCheck(options.Config.Notifications.SMTP.Host+":"+strconv.Itoa(options.Config.Notifications.SMTP.Port))).
-			WithChecks(checkHandler.Checks()),
-	)
+	readyHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Notifications.Events.Cluster)).
+		WithCheck("smtp-check", checks.NewTCPCheck(options.Config.Notifications.SMTP.Host+":"+strconv.Itoa(options.Config.Notifications.SMTP.Port)))
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -35,7 +27,6 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(checkHandler),
-		debug.Ready(readyHandler),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }
diff --git a/services/search/pkg/server/debug/server.go b/services/search/pkg/server/debug/server.go
@@ -14,24 +14,18 @@ import (
 func Server(opts ...Option) (*http.Server, error) {
 	options := newOptions(opts...)
 
-	checkHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("grpc reachability", checks.NewGRPCCheck(options.Config.GRPC.Addr)),
-	)
+	healthHandlerConfiguration := handlers.NewCheckHandlerConfiguration().
+		WithLogger(options.Logger).
+		WithCheck("grpc reachability", checks.NewGRPCCheck(options.Config.GRPC.Addr))
 
-	readyHandler := handlers.NewCheckHandler(
-		handlers.NewCheckHandlerConfiguration().
-			WithLogger(options.Logger).
-			WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
-			WithCheck("tika-check", func(ctx context.Context) error {
-				if options.Config.Extractor.Type == "tika" {
-					return checks.NewTCPCheck(options.Config.Extractor.Tika.TikaURL)(ctx)
-				}
-				return nil
-			}).
-			WithChecks(checkHandler.Checks()),
-	)
+	readyHandlerConfiguration := healthHandlerConfiguration.
+		WithCheck("nats reachability", checks.NewNatsCheck(options.Config.Events.Cluster)).
+		WithCheck("tika-check", func(ctx context.Context) error {
+			if options.Config.Extractor.Type == "tika" {
+				return checks.NewTCPCheck(options.Config.Extractor.Tika.TikaURL)(ctx)
+			}
+			return nil
+		})
 
 	return debug.NewService(
 		debug.Logger(options.Logger),
@@ -41,7 +35,7 @@ func Server(opts ...Option) (*http.Server, error) {
 		debug.Token(options.Config.Debug.Token),
 		debug.Pprof(options.Config.Debug.Pprof),
 		debug.Zpages(options.Config.Debug.Zpages),
-		debug.Health(checkHandler),
-		debug.Ready(readyHandler),
+		debug.Health(handlers.NewCheckHandler(healthHandlerConfiguration)),
+		debug.Ready(handlers.NewCheckHandler(readyHandlerConfiguration)),
 	), nil
 }