fixup! feat: Add preflight checks framework

Remove unnecessary copying of slice

Author: dlipovetsky

pkg/webhook/preflight/preflight.go

@@ -72,6 +72,9 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
 		return admission.Allowed("")
 	}
 
+	resultsOrderedByCheckerAndCheck := run(ctx, h.client, cluster, h.checkers)
+
+	// Summarize the results.
 	resp := admission.Response{
 		AdmissionResponse: admissionv1.AdmissionResponse{
 			Allowed: true,
@@ -80,26 +83,24 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
 			},
 		},
 	}
-
-	results := run(ctx, h.client, cluster, h.checkers)
-
-	// Summarize the results.
 	internalError := false
-	for _, result := range results {
-		if result.Error {
-			internalError = true
-		}
-		if !result.Allowed {
-			resp.Allowed = false
-		}
-		for _, cause := range result.Causes {
-			resp.Result.Details.Causes = append(resp.Result.Details.Causes, metav1.StatusCause{
-				Type:    metav1.CauseType(fmt.Sprintf("FailedPreflight%s", result.Name)),
-				Message: cause.Message,
-				Field:   cause.Field,
-			})
+	for _, results := range resultsOrderedByCheckerAndCheck {
+		for _, result := range results {
+			if result.Error {
+				internalError = true
+			}
+			if !result.Allowed {
+				resp.Allowed = false
+			}
+			for _, cause := range result.Causes {
+				resp.Result.Details.Causes = append(resp.Result.Details.Causes, metav1.StatusCause{
+					Type:    metav1.CauseType(fmt.Sprintf("FailedPreflight%s", result.Name)),
+					Message: cause.Message,
+					Field:   cause.Field,
+				})
+			}
+			resp.Warnings = append(resp.Warnings, result.Warnings...)
 		}
-		resp.Warnings = append(resp.Warnings, result.Warnings...)
 	}
 
 	switch {
@@ -118,7 +119,13 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
 	return resp
 }
 
-func run(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster, checkers []Checker) []CheckResult {
+// run runs all checks for the cluster, concurrently, and returns the results ordered by checker and check.
+// Checker are initialized concurrently, and checks runs concurrently as well.
+func run(ctx context.Context,
+	client ctrlclient.Client,
+	cluster *clusterv1.Cluster,
+	checkers []Checker,
+) [][]CheckResult {
 	resultsOrderedByCheckerAndCheck := make([][]CheckResult, len(checkers))
 
 	checkersWG := sync.WaitGroup{}
@@ -139,14 +146,11 @@ func run(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Clust
 			}
 			checksWG.Wait()
 			resultsOrderedByCheckerAndCheck[i] = resultsOrderedByCheck
+
 			checkersWG.Done()
 		}(ctx, client, cluster, checker, i)
 	}
 	checkersWG.Wait()
 
-	results := []CheckResult{}
-	for _, resultsByCheck := range resultsOrderedByCheckerAndCheck {
-		results = append(results, resultsByCheck...)
-	}
-	return results
+	return resultsOrderedByCheckerAndCheck
 }

pkg/webhook/preflight/preflight_test.go

@@ -530,7 +530,11 @@ func TestRun_SingleCheckerSingleCheck(t *testing.T) {
 			},
 		},
 	}
-	results := run(ctx, nil, nil, []Checker{checker})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
+	if len(resultsOrderedByCheckerAndCheck) != 1 {
+		t.Fatalf("expected results for 1 checker, got %d", len(resultsOrderedByCheckerAndCheck))
+	}
+	results := resultsOrderedByCheckerAndCheck[0]
 	if len(results) != 1 {
 		t.Fatalf("expected 1 result, got %d", len(results))
 	}
@@ -560,15 +564,19 @@ func TestRun_MultipleCheckersMultipleChecks(t *testing.T) {
 			},
 		},
 	}
-	results := run(ctx, nil, nil, []Checker{checker1, checker2})
-	if len(results) != 3 {
-		t.Fatalf("expected 3 results, got %d", len(results))
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker1, checker2})
+	if len(resultsOrderedByCheckerAndCheck) != 2 {
+		t.Fatalf("expected results for 2 checkers, got %d", len(resultsOrderedByCheckerAndCheck))
 	}
-	names := []string{results[0].Name, results[1].Name, results[2].Name}
+
 	expected := []string{"c1-check1", "c1-check2", "c2-check1"}
-	for i, name := range expected {
-		if names[i] != name {
-			t.Errorf("expected result %d to have name %q, got %q", i, name, names[i])
+	expectedIdx := 0
+	for _, results := range resultsOrderedByCheckerAndCheck {
+		for _, result := range results {
+			if result.Name != expected[expectedIdx] {
+				t.Errorf("expected result %d to be %q, got %q", expectedIdx, expected[expectedIdx], result.Name)
+			}
+			expectedIdx++
 		}
 	}
 }
@@ -595,7 +603,9 @@ func TestRun_ChecksRunInParallel(t *testing.T) {
 			},
 		},
 	}
-	results := run(ctx, nil, nil, []Checker{checker})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
+
+	results := resultsOrderedByCheckerAndCheck[0]
 	if len(results) != 2 {
 		t.Fatalf("expected 2 results, got %d", len(results))
 	}
@@ -670,7 +680,10 @@ func TestRun_ContextCancellation(t *testing.T) {
 		cancel()
 	}()
 
-	results := run(ctx, nil, nil, []Checker{checker})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
+	if len(resultsOrderedByCheckerAndCheck) != 1 {
+		t.Fatalf("expected results for 1 checker, got %d", len(resultsOrderedByCheckerAndCheck))
+	}
 
 	select {
 	case <-completed:
@@ -679,6 +692,7 @@ func TestRun_ContextCancellation(t *testing.T) {
 		// This is expected - the check was cancelled
 	}
 
+	results := resultsOrderedByCheckerAndCheck[0]
 	if len(results) != 1 {
 		t.Fatalf("expected 1 result, got %d", len(results))
 	}
@@ -717,18 +731,21 @@ func TestRun_OrderOfResults(t *testing.T) {
 		},
 	}
 
-	results := run(ctx, nil, nil, []Checker{checker1, checker2})
-
-	if len(results) != 4 {
-		t.Fatalf("expected 4 results, got %d", len(results))
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker1, checker2})
+	if len(resultsOrderedByCheckerAndCheck) != 2 {
+		t.Fatalf("expected results for 2 checkers, got %d", len(resultsOrderedByCheckerAndCheck))
 	}
 
 	// The order should be all checks from checker1 followed by all checks from checker2,
 	// regardless of their completion time
 	expected := []string{"c1-check1", "c1-check2", "c2-check1", "c2-check2"}
-	for i, name := range expected {
-		if results[i].Name != name {
-			t.Errorf("result[%d]: expected %q, got %q", i, name, results[i].Name)
+	expectedIdx := 0
+	for _, results := range resultsOrderedByCheckerAndCheck {
+		for _, result := range results {
+			if result.Name != expected[expectedIdx] {
+				t.Errorf("expected result %d to be %q, got %q", expectedIdx, expected[expectedIdx], result.Name)
+			}
+			expectedIdx++
 		}
 	}
 }
@@ -761,11 +778,16 @@ func TestRun_LargeNumberOfCheckersAndChecks(t *testing.T) {
 	}
 
 	start := time.Now()
-	results := run(ctx, nil, nil, checkers)
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, checkers)
 	duration := time.Since(start)
 
-	if len(results) != expectedTotal {
-		t.Errorf("expected %d results, got %d", expectedTotal, len(results))
+	resultTotal := 0
+	for _, results := range resultsOrderedByCheckerAndCheck {
+		resultTotal += len(results)
+	}
+
+	if resultTotal != expectedTotal {
+		t.Errorf("expected %d results, got %d", expectedTotal, resultTotal)
 	}
 
 	t.Logf("Completed %d checks in %v", expectedTotal, duration)
@@ -799,8 +821,13 @@ func TestRun_ErrorHandlingInChecks(t *testing.T) {
 		},
 	}
 
-	results := run(ctx, nil, nil, []Checker{checker})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{checker})
+
+	if len(resultsOrderedByCheckerAndCheck) != 1 {
+		t.Fatalf("expected results for 1 checker, got %d", len(resultsOrderedByCheckerAndCheck))
+	}
 
+	results := resultsOrderedByCheckerAndCheck[0]
 	if len(results) != 2 {
 		t.Fatalf("expected 2 results, got %d", len(results))
 	}
@@ -840,13 +867,23 @@ func TestRun_ZeroChecksFromChecker(t *testing.T) {
 		},
 	}
 
-	results := run(ctx, nil, nil, []Checker{emptyChecker, normalChecker})
+	resultsOrderedByCheckerAndCheck := run(ctx, nil, nil, []Checker{emptyChecker, normalChecker})
 
-	if len(results) != 1 {
-		t.Fatalf("expected 1 result, got %d", len(results))
+	if len(resultsOrderedByCheckerAndCheck) != 2 {
+		t.Fatalf("expected results for 2 checkers, got %d", len(resultsOrderedByCheckerAndCheck))
+	}
+
+	emptyResults := resultsOrderedByCheckerAndCheck[0] // We expect no results from the empty checker
+	if len(emptyResults) != 0 {
+		t.Fatalf("expected 0 results from empty checker, got %d", len(emptyResults))
+	}
+
+	normalResults := resultsOrderedByCheckerAndCheck[1] // We expect results from the normal checker
+	if len(normalResults) != 1 {
+		t.Fatalf("expected 1 result, got %d", len(normalResults))
 	}
 
-	if results[0].Name != "check1" {
-		t.Errorf("expected result from normal checker, got %s", results[0].Name)
+	if normalResults[0].Name != "check1" {
+		t.Errorf("expected result from normal checker, got %s", normalResults[0].Name)
 	}
 }