fixup! feat: Add preflight checks framework

Remove the use of a checker factory.

Author: dlipovetsky

cmd/main.go

@@ -222,7 +222,7 @@ func main() {
 
 	mgr.GetWebhookServer().Register("/preflight-v1beta1-cluster", &webhook.Admission{
 		Handler: preflight.New(mgr.GetClient(), admission.NewDecoder(mgr.GetScheme()),
-			[]preflight.CheckerFactory{
+			[]preflight.Checker{
 				// Add your preflight checkers here.
 			}...,
 		),

pkg/webhook/preflight/preflight.go

@@ -18,14 +18,11 @@ import (
 )
 
 type (
-	// CheckerFactory returns a Checker for a given cluster.
-	CheckerFactory func(client ctrlclient.Client, cluster *clusterv1.Cluster) Checker
-
 	// 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
+		Init(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster) []Check
 	}
 
 	// Check represents a single preflight check that can be run against a cluster.
@@ -61,16 +58,16 @@ type (
 )
 
 type WebhookHandler struct {
-	client    ctrlclient.Client
-	decoder   admission.Decoder
-	factories []CheckerFactory
+	client   ctrlclient.Client
+	decoder  admission.Decoder
+	checkers []Checker
 }
 
-func New(client ctrlclient.Client, decoder admission.Decoder, factories ...CheckerFactory) *WebhookHandler {
+func New(client ctrlclient.Client, decoder admission.Decoder, checkers ...Checker) *WebhookHandler {
 	h := &WebhookHandler{
-		client:    client,
-		decoder:   decoder,
-		factories: factories,
+		client:   client,
+		decoder:  decoder,
+		checkers: checkers,
 	}
 	return h
 }
@@ -96,7 +93,7 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
 		return admission.Allowed("")
 	}
 
-	resultsOrderedByCheckerAndCheck := run(ctx, h.client, cluster, h.factories)
+	resultsOrderedByCheckerAndCheck := run(ctx, h.client, cluster, h.checkers)
 
 	// Summarize the results.
 	resp := admission.Response{
@@ -148,18 +145,17 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
 func run(ctx context.Context,
 	client ctrlclient.Client,
 	cluster *clusterv1.Cluster,
-	factories []CheckerFactory,
+	checkers []Checker,
 ) [][]namedResult {
-	resultsOrderedByCheckerAndCheck := make([][]namedResult, len(factories))
+	resultsOrderedByCheckerAndCheck := make([][]namedResult, len(checkers))
 
 	checkersWG := sync.WaitGroup{}
-	for i, factory := range factories {
+	for i, checker := range checkers {
 		checkersWG.Add(1)
-		go func(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster, factory CheckerFactory, i int) {
+		go func(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster, checker Checker, i int) {
 			defer checkersWG.Done()
-			checker := factory(client, cluster)
 
-			checks := checker.Init(ctx)
+			checks := checker.Init(ctx, client, cluster)
 			resultsOrderedByCheck := make([]namedResult, len(checks))
 
 			checksWG := sync.WaitGroup{}
@@ -204,7 +200,7 @@ func run(ctx context.Context,
 			ctx,
 			client,
 			cluster,
-			factory,
+			checker,
 			i,
 		)
 	}

pkg/webhook/preflight/preflight_test.go

@@ -24,17 +24,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
-func mockCheckerFactory(checker Checker) CheckerFactory {
-	return func(_ ctrlclient.Client, _ *clusterv1.Cluster) Checker {
-		return checker
-	}
-}
-
 type mockChecker struct {
 	checks []Check
 }
 
-func (m *mockChecker) Init(_ context.Context) []Check {
+func (m *mockChecker) Init(_ context.Context, _ ctrlclient.Client, _ *clusterv1.Cluster) []Check {
 	return m.checks
 }
 
@@ -375,11 +369,7 @@ func TestHandle(t *testing.T) {
 				decoder = tt.decoder
 			}
 
-			factories := make([]CheckerFactory, len(tt.checkers))
-			for i, checker := range tt.checkers {
-				factories[i] = mockCheckerFactory(checker)
-			}
-			handler := New(fake.NewClientBuilder().Build(), decoder, factories...)
+			handler := New(fake.NewClientBuilder().Build(), decoder, tt.checkers...)
 
 			ctx := context.TODO()
 
@@ -511,7 +501,7 @@ func TestHandleCancelledContext(t *testing.T) {
 		},
 	}
 
-	handler := New(fake.NewClientBuilder().Build(), decoder, mockCheckerFactory(checker))
+	handler := New(fake.NewClientBuilder().Build(), decoder, checker)
 
 	ctx := context.Background()
 	ctx, cancel := context.WithCancel(ctx)
@@ -572,8 +562,7 @@ func TestRun_SingleCheckerSingleCheck(t *testing.T) {
 			},
 		},
 	}
-	factory := mockCheckerFactory(checker)
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []CheckerFactory{factory})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
 	if len(resultsOrderedByCheckerAndCheck) != 1 {
 		t.Fatalf("expected results for 1 checker, got %d", len(resultsOrderedByCheckerAndCheck))
 	}
@@ -611,11 +600,8 @@ func TestRun_MultipleCheckersMultipleChecks(t *testing.T) {
 			},
 		},
 	}
-	factories := []CheckerFactory{
-		mockCheckerFactory(checker1),
-		mockCheckerFactory(checker2),
-	}
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, factories)
+
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker1, checker2})
 	if len(resultsOrderedByCheckerAndCheck) != 2 {
 		t.Fatalf("expected results for 2 checkers, got %d", len(resultsOrderedByCheckerAndCheck))
 	}
@@ -677,8 +663,7 @@ func TestRun_ChecksRunInParallel(t *testing.T) {
 			},
 		},
 	}
-	factory := mockCheckerFactory(checker)
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []CheckerFactory{factory})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
 
 	results := resultsOrderedByCheckerAndCheck[0]
 	if len(results) != 2 {
@@ -714,11 +699,8 @@ func TestRun_CheckersRunInParallel(t *testing.T) {
 			},
 		},
 	}
-	factories := []CheckerFactory{
-		mockCheckerFactory(checker1),
-		mockCheckerFactory(checker2),
-	}
-	results := run(ctx, nil, nil, factories)
+
+	results := run(ctx, nil, nil, []Checker{checker1, checker2})
 	if len(results) != 2 {
 		t.Fatalf("expected 2 results, got %d", len(results))
 	}
@@ -774,8 +756,7 @@ func TestRun_ContextCancellation(t *testing.T) {
 		cancel()
 	}()
 
-	factory := mockCheckerFactory(checker)
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []CheckerFactory{factory})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
 
 	select {
 	case <-completed:
@@ -822,11 +803,7 @@ func TestRun_OrderOfResults(t *testing.T) {
 		},
 	}
 
-	factories := []CheckerFactory{
-		mockCheckerFactory(checker1),
-		mockCheckerFactory(checker2),
-	}
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, factories)
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker1, checker2})
 	if len(resultsOrderedByCheckerAndCheck) != 2 {
 		t.Fatalf("expected results for 2 checkers, got %d", len(resultsOrderedByCheckerAndCheck))
 	}
@@ -869,13 +846,8 @@ func TestRun_LargeNumberOfCheckersAndChecks(t *testing.T) {
 		}
 	}
 
-	factories := make([]CheckerFactory, checkerCount)
-	for i, checker := range checkers {
-		factories[i] = mockCheckerFactory(checker)
-	}
-
 	start := time.Now()
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, factories)
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, checkers)
 	duration := time.Since(start)
 
 	resultTotal := 0
@@ -907,10 +879,9 @@ func TestRun_ErrorHandlingInChecks(t *testing.T) {
 	checker := &mockChecker{
 		checks: []Check{errorCheck},
 	}
-	factory := mockCheckerFactory(checker)
 
 	// Run the checks
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []CheckerFactory{factory})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
 	assert.Len(t, resultsOrderedByCheckerAndCheck, 1, "expected results for 1 checker")
 	assert.Len(t, resultsOrderedByCheckerAndCheck[0], 1, "expected 1 result from the checker")
 
@@ -971,10 +942,9 @@ func TestRun_PanicHandlingInChecks(t *testing.T) {
 			panicCheck,
 		},
 	}
-	factory := mockCheckerFactory(checker)
 
 	// Run the checks
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, cluster, []CheckerFactory{factory})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, cluster, []Checker{checker})
 	assert.Len(t, resultsOrderedByCheckerAndCheck, 1, "expected results for 1 checker")
 	assert.Len(t, resultsOrderedByCheckerAndCheck[0], 2, "expected 2 results from the checker")
 
@@ -1020,11 +990,7 @@ func TestRun_ZeroChecksFromChecker(t *testing.T) {
 		},
 	}
 
-	factories := []CheckerFactory{
-		mockCheckerFactory(emptyChecker),
-		mockCheckerFactory(normalChecker),
-	}
-	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, factories)
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{emptyChecker, normalChecker})
 
 	if len(resultsOrderedByCheckerAndCheck) != 2 {
 		t.Fatalf("expected results for 2 checkers, got %d", len(resultsOrderedByCheckerAndCheck))