healthcheck: simplify Checker construction with a builder (linkerd#5475)

Currently, Each new instance of `Checker` type have to manually set all the fields with the `NewChecker()`, even though most use-cases are fine with the defaults. This branch makes this simpler by using the Builder pattern, so that the users of `Checker` can override the defaults by using specific field methods when needed. Thus simplifying the code. This also removes some of the methods that were specific to tests, and replaces them with the currently used ones. Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
mackjmr · Jan 6, 2021 · 68c02d8 · 68c02d8
1 parent bce3547
commit 68c02d8
Show file tree

Hide file tree

Showing 5 changed files with 138 additions and 82 deletions.
diff --git a/cli/cmd/check_test.go b/cli/cmd/check_test.go
@@ -16,12 +16,19 @@ func TestCheckStatus(t *testing.T) {
 			[]healthcheck.CategoryID{},
 			&healthcheck.Options{},
 		)
-		hc.Add("category", "check1", "", func(context.Context) error {
-			return nil
-		})
-		hc.Add("category", "check2", "hint-anchor", func(context.Context) error {
-			return fmt.Errorf("This should contain instructions for fail")
-		})
+		hc.AppendCategories(*healthcheck.NewCategory("category", []healthcheck.Checker{
+			*healthcheck.NewChecker("check1").
+				WithCheck(func(context.Context) error {
+					return nil
+				}),
+			*healthcheck.NewChecker("check2").
+				WithHintAnchor("hint-anchor").
+				WithCheck(func(context.Context) error {
+					return fmt.Errorf("This should contain instructions for fail")
+				}),
+		},
+			true,
+		))
 
 		output := bytes.NewBufferString("")
 		healthcheck.RunChecks(output, stderr, hc, tableOutput)
@@ -43,12 +50,19 @@ func TestCheckStatus(t *testing.T) {
 			[]healthcheck.CategoryID{},
 			&healthcheck.Options{},
 		)
-		hc.Add("category", "check1", "", func(context.Context) error {
-			return nil
-		})
-		hc.Add("category", "check2", "hint-anchor", func(context.Context) error {
-			return fmt.Errorf("This should contain instructions for fail")
-		})
+		hc.AppendCategories(*healthcheck.NewCategory("category", []healthcheck.Checker{
+			*healthcheck.NewChecker("check1").
+				WithCheck(func(context.Context) error {
+					return nil
+				}),
+			*healthcheck.NewChecker("check2").
+				WithHintAnchor("hint-anchor").
+				WithCheck(func(context.Context) error {
+					return fmt.Errorf("This should contain instructions for fail")
+				}),
+		},
+			true,
+		))
 
 		output := bytes.NewBufferString("")
 		healthcheck.RunChecks(output, stderr, hc, jsonOutput)

diff --git a/jaeger/cmd/check.go b/jaeger/cmd/check.go
@@ -41,14 +41,19 @@ func jaegerCategory(hc *healthcheck.HealthChecker) (*healthcheck.Category, error
 
 	checkers := []healthcheck.Checker{}
 	checkers = append(checkers,
-		*healthcheck.NewChecker("collector and jaeger service account exists", "l5d-jaeger-sc-exists", true, true, time.Time{}, false).
+		*healthcheck.NewChecker("collector and jaeger service account exists").
+			WithHintAnchor("l5d-jaeger-sc-exists").
+			Fatal().
+			Warning().
 			WithCheck(func(ctx context.Context) error {
 				// Check for Collector Service Account
 				return healthcheck.CheckServiceAccounts(ctx, kubeAPI, []string{"collector", "jaeger"}, jaegerNamespace, "")
 			}))
 
 	checkers = append(checkers,
-		*healthcheck.NewChecker("collector config map exists", "l5d-jaeger-oc-cm-exists", false, true, time.Time{}, false).
+		*healthcheck.NewChecker("collector config map exists").
+			WithHintAnchor("l5d-jaeger-oc-cm-exists").
+			Warning().
 			WithCheck(func(ctx context.Context) error {
 				// Check for Jaeger Service Account
 				_, err = kubeAPI.CoreV1().ConfigMaps(jaegerNamespace).Get(ctx, "collector-config", metav1.GetOptions{})
@@ -59,7 +64,11 @@ func jaegerCategory(hc *healthcheck.HealthChecker) (*healthcheck.Category, error
 			}))
 
 	checkers = append(checkers,
-		*healthcheck.NewChecker("collector pod is running", "l5d-jaeger-collector-running", false, true, hc.RetryDeadline, true).
+		*healthcheck.NewChecker("collector pod is running").
+			WithHintAnchor("l5d-jaeger-collector-running").
+			Warning().
+			WithRetryDeadline(hc.RetryDeadline).
+			SurfaceErrorOnRetry().
 			WithCheck(func(ctx context.Context) error {
 				// Check for Collector pod
 				podList, err := kubeAPI.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: "component=collector"})
@@ -70,7 +79,11 @@ func jaegerCategory(hc *healthcheck.HealthChecker) (*healthcheck.Category, error
 			}))
 
 	checkers = append(checkers,
-		*healthcheck.NewChecker("jaeger pod is running", "l5d-jaeger-jaeger-running", false, true, hc.RetryDeadline, true).
+		*healthcheck.NewChecker("jaeger pod is running").
+			WithHintAnchor("l5d-jaeger-jaeger-running").
+			Warning().
+			WithRetryDeadline(hc.RetryDeadline).
+			SurfaceErrorOnRetry().
 			WithCheck(func(ctx context.Context) error {
 				// Check for Jaeger pod
 				podList, err := kubeAPI.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: "component=jaeger"})
@@ -81,7 +94,9 @@ func jaegerCategory(hc *healthcheck.HealthChecker) (*healthcheck.Category, error
 			}))
 
 	checkers = append(checkers,
-		*healthcheck.NewChecker("jaeger extension pods are injected", "l5d-jaeger-pods-injection", false, true, time.Time{}, false).
+		*healthcheck.NewChecker("jaeger extension pods are injected").
+			WithHintAnchor("l5d-jaeger-pods-injection").
+			Warning().
 			WithCheck(func(ctx context.Context) error {
 				// Check for Jaeger pod
 				pods, err := kubeAPI.GetPodsByNamespace(ctx, jaegerNamespace)

diff --git a/multicluster/cmd/check.go b/multicluster/cmd/check.go
@@ -131,16 +131,22 @@ func configureAndRunChecks(wout io.Writer, werr io.Writer, options *checkOptions
 func multiclusterCategory(hc *healthChecker) *healthcheck.Category {
 	checkers := []healthcheck.Checker{}
 	checkers = append(checkers,
-		*healthcheck.NewChecker("Link CRD exists", "l5d-multicluster-link-crd-exists", true, false, time.Time{}, false).
+		*healthcheck.NewChecker("Link CRD exists").
+			WithHintAnchor("l5d-multicluster-link-crd-exists").
+			Fatal().
 			WithCheck(func(ctx context.Context) error { return hc.checkLinkCRD(ctx) }))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("Link resources are valid", "l5d-multicluster-links-are-valid", true, false, time.Time{}, false).
+		*healthcheck.NewChecker("Link resources are valid").
+			WithHintAnchor("l5d-multicluster-links-are-valid").
+			Fatal().
 			WithCheck(func(ctx context.Context) error { return hc.checkLinks(ctx) }))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("remote cluster access credentials are valid", "l5d-smc-target-clusters-access", false, false, time.Time{}, false).
+		*healthcheck.NewChecker("remote cluster access credentials are valid").
+			WithHintAnchor("l5d-smc-target-clusters-access").
 			WithCheck(func(ctx context.Context) error { return hc.checkRemoteClusterConnectivity(ctx) }))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("clusters share trust anchors", "l5d-multicluster-clusters-share-anchors", false, false, time.Time{}, false).
+		*healthcheck.NewChecker("clusters share trust anchors").
+			WithHintAnchor("l5d-multicluster-clusters-share-anchors").
 			WithCheck(func(ctx context.Context) error {
 				localAnchors, err := tls.DecodePEMCertificates(hc.linkerdHC.LinkerdConfigGlobal().IdentityTrustAnchorsPEM)
 				if err != nil {
@@ -149,27 +155,35 @@ func multiclusterCategory(hc *healthChecker) *healthcheck.Category {
 				return hc.checkRemoteClusterAnchors(ctx, localAnchors)
 			}))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("service mirror controller has required permissions", "l5d-multicluster-source-rbac-correct", false, false, time.Time{}, false).
+		*healthcheck.NewChecker("service mirror controller has required permissions").
+			WithHintAnchor("l5d-multicluster-source-rbac-correct").
 			WithCheck(func(ctx context.Context) error {
 				return hc.checkServiceMirrorLocalRBAC(ctx)
 			}))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("service mirror controllers are running", "l5d-multicluster-service-mirror-running", false, false, hc.linkerdHC.RetryDeadline, true).
+		*healthcheck.NewChecker("service mirror controllers are running").
+			WithHintAnchor("l5d-multicluster-service-mirror-running").
+			WithRetryDeadline(hc.linkerdHC.RetryDeadline).
+			SurfaceErrorOnRetry().
 			WithCheck(func(ctx context.Context) error {
 				return hc.checkServiceMirrorController(ctx)
 			}))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("all gateway mirrors are healthy", "l5d-multicluster-gateways-endpoints", false, false, time.Time{}, false).
+		*healthcheck.NewChecker("all gateway mirrors are healthy").
+			WithHintAnchor("l5d-multicluster-gateways-endpoints").
 			WithCheck(func(ctx context.Context) error {
 				return hc.checkIfGatewayMirrorsHaveEndpoints(ctx)
 			}))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("all mirror services have endpoints", "l5d-multicluster-services-endpoints", false, false, time.Time{}, false).
+		*healthcheck.NewChecker("all mirror services have endpoints").
+			WithHintAnchor("l5d-multicluster-services-endpoints").
 			WithCheck(func(ctx context.Context) error {
 				return hc.checkIfMirrorServicesHaveEndpoints(ctx)
 			}))
 	checkers = append(checkers,
-		*healthcheck.NewChecker("all mirror services are part of a Link", "l5d-multicluster-orphaned-services", false, true, time.Time{}, false).
+		*healthcheck.NewChecker("all mirror services are part of a Link").
+			WithHintAnchor("l5d-multicluster-orphaned-services").
+			Warning().
 			WithCheck(func(ctx context.Context) error {
 				return hc.checkForOrphanedServices(ctx)
 			}))

diff --git a/pkg/healthcheck/healthcheck.go b/pkg/healthcheck/healthcheck.go
@@ -312,24 +312,50 @@ type Checker struct {
 }
 
 // NewChecker returns a new instance of checker type
-func NewChecker(description, hintAnchor string, fatal, warning bool, retryDeadline time.Time, surfaceErrorOnRetry bool) *Checker {
+func NewChecker(description string) *Checker {
 	return &Checker{
-		description:         description,
-		hintAnchor:          hintAnchor,
-		fatal:               fatal,
-		warning:             warning,
-		retryDeadline:       retryDeadline,
-		surfaceErrorOnRetry: surfaceErrorOnRetry,
+		description:   description,
+		retryDeadline: time.Time{},
 	}
 }
 
-// WithCheck Returns a checker with the provided check func
+// WithHintAnchor returns a checker with the given hint anchor
+func (c *Checker) WithHintAnchor(hint string) *Checker {
+	c.hintAnchor = hint
+	return c
+}
+
+// Fatal returns a checker with the fatal field set
+func (c *Checker) Fatal() *Checker {
+	c.fatal = true
+	return c
+}
+
+// Warning returns a checker with the warning field set
+func (c *Checker) Warning() *Checker {
+	c.warning = true
+	return c
+}
+
+// WithRetryDeadline returns a checker with the provided retry timeout
+func (c *Checker) WithRetryDeadline(retryDeadLine time.Time) *Checker {
+	c.retryDeadline = retryDeadLine
+	return c
+}
+
+// SurfaceErrorOnRetry returns a checker with the surfaceErrorOnRetry set
+func (c *Checker) SurfaceErrorOnRetry() *Checker {
+	c.surfaceErrorOnRetry = true
+	return c
+}
+
+// WithCheck returns a checker with the provided check func
 func (c *Checker) WithCheck(check func(context.Context) error) *Checker {
 	c.check = check
 	return c
 }
 
-// WithCheckRPC Returns a checker with the provided checkRPC func
+// WithCheckRPC returns a checker with the provided checkRPC func
 func (c *Checker) WithCheckRPC(checkRPC func(context.Context) (*healthcheckPb.SelfCheckResponse, error)) *Checker {
 	c.checkRPC = checkRPC
 	return c
@@ -1404,30 +1430,6 @@ func (hc *HealthChecker) checkMinReplicasAvailable(ctx context.Context) error {
 	return nil
 }
 
-// Add adds an arbitrary checker. This should only be used for testing. For
-// production code, pass in the desired set of checks when calling
-// NewHealthChecker.
-func (hc *HealthChecker) Add(categoryID CategoryID, description string, hintAnchor string, check func(context.Context) error) {
-	hc.addCategory(
-		Category{
-			id: categoryID,
-			checkers: []Checker{
-				{
-					description: description,
-					check:       check,
-					hintAnchor:  hintAnchor,
-				},
-			},
-		},
-	)
-}
-
-// addCategory is also for testing
-func (hc *HealthChecker) addCategory(c Category) {
-	c.enabled = true
-	hc.categories = append(hc.categories, c)
-}
-
 // RunChecks runs all configured checkers, and passes the results of each
 // check to the observer. If a check fails and is marked as fatal, then all
 // remaining checks are skipped. If at least one check fails, RunChecks returns