fixup! feat: Add preflight checks framework

Recover from and log panic in check

Author: dlipovetsky

pkg/webhook/preflight/preflight.go

@@ -6,24 +6,43 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"runtime/debug"
 	"sync"
 
 	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 type (
 	// CheckerFactory returns a Checker for a given cluster.
 	CheckerFactory func(client ctrlclient.Client, cluster *clusterv1.Cluster) Checker
-	Checker        interface {
+
+	// Checker returns a set of checks that have been initialized with common dependencies,
+	// such as an infrastructure API client.
+	Checker interface {
 		// Init returns the checks that should run for the cluster.
 		Init(ctx context.Context) []Check
 	}
 
-	Check       = func(ctx context.Context) CheckResult
+	// Check represents a single preflight check that can be run against a cluster.
+	// It has a Name method that returns the name of the check, and a Run method executes
+	// the check, and returns a CheckResult.
+	// The Name method is used to identify the check if Run fails to return a result, for
+	// example if it panics.
+	Check interface {
+		Name() string
+		Run(ctx context.Context) CheckResult
+	}
+
+	// CheckResult represents the result of a check.
+	// It contains the name of the check, a boolean indicating whether the check passed, an
+	// error boolean indicating whether there was an internal error running the check, and a
+	// list of causes for the failure. It also contains a list of warnings that were
+	// generated during the check.
 	CheckResult struct {
 		Name string
 
@@ -33,6 +52,10 @@ type (
 		Causes   []Cause
 		Warnings []string
 	}
+
+	// Cause represents a cause of a check failure. It contains a message and an optional
+	// field that the cause relates to. The field is used to indicate which part of the
+	// cluster configuration the cause relates to.
 	Cause struct {
 		Message string
 		Field   string
@@ -130,6 +153,7 @@ func run(ctx context.Context,
 	for i, factory := range factories {
 		checkersWG.Add(1)
 		go func(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster, factory CheckerFactory, i int) {
+			defer checkersWG.Done()
 			checker := factory(client, cluster)
 
 			checks := checker.Init(ctx)
@@ -139,15 +163,35 @@ func run(ctx context.Context,
 			for j, check := range checks {
 				checksWG.Add(1)
 				go func(ctx context.Context, check Check, j int) {
-					result := check(ctx)
+					defer checksWG.Done()
+					defer func() {
+						if r := recover(); r != nil {
+							resultsOrderedByCheck[j] = CheckResult{
+								Name:  check.Name(),
+								Error: true,
+								Causes: []Cause{
+									{
+										Message: fmt.Sprintf("internal error (panic): %s", r),
+										Field:   "",
+									},
+								},
+							}
+							ctrl.LoggerFrom(ctx).Error(
+								fmt.Errorf("preflight check panic"),
+								fmt.Sprintf("%v", r),
+								"checkName", check.Name(),
+								"clusterName", cluster.Name,
+								"clusterNamespace", cluster.Namespace,
+								"stackTrace", string(debug.Stack()),
+							)
+						}
+					}()
+					result := check.Run(ctx)
 					resultsOrderedByCheck[j] = result
-					checksWG.Done()
 				}(ctx, check, j)
 			}
 			checksWG.Wait()
 			resultsOrderedByCheckerAndCheck[i] = resultsOrderedByCheck
-
-			checkersWG.Done()
 		}(
 			ctx,
 			client,