fixup! feat: Nutanix VM image preflight check

Refactor to simplify, and remove need to checker factory.

Author: dlipovetsky

cmd/main.go

@@ -225,7 +225,7 @@ func main() {
 		Handler: preflight.New(mgr.GetClient(), admission.NewDecoder(mgr.GetScheme()),
 			[]preflight.Checker{
 				// Add your preflight checkers here.
-				preflightnutanix.New,
+				preflightnutanix.Checker,
 			}...,
 		),
 	})

pkg/webhook/preflight/nutanix/checker.go

@@ -17,58 +17,58 @@ import (
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/webhook/preflight"
 )
 
-func New(kclient ctrlclient.Client, cluster *clusterv1.Cluster) preflight.Checker {
-	return &nutanixChecker{
-		kclient: kclient,
-		cluster: cluster,
-
-		nclientFactory: newClient,
-
-		initNutanixConfigurationFunc: initNutanixConfiguration,
-		initCredentialsCheckFunc:     initCredentialsCheck,
-		initVMImageChecksFunc:        initVMImageChecks,
-	}
+var Checker = &nutanixChecker{
+	configurationCheckFactory: newConfigurationCheck,
+	credentialsCheckFactory:   newCredentialsCheck,
+	vmImageChecksFactory:      newVMImageChecks,
 }
 
 type nutanixChecker struct {
-	kclient ctrlclient.Client
-	cluster *clusterv1.Cluster
-
-	nutanixClusterConfigSpec                           *carenv1.NutanixClusterConfigSpec
-	nutanixWorkerNodeConfigSpecByMachineDeploymentName map[string]*carenv1.NutanixWorkerNodeConfigSpec
-
-	nclient        client
-	nclientFactory func(prismgoclient.Credentials) (client, error)
-
-	initNutanixConfigurationFunc func(
-		n *nutanixChecker,
+	configurationCheckFactory func(
+		cd *checkDependencies,
 	) preflight.Check
 
-	initCredentialsCheckFunc func(
+	credentialsCheckFactory func(
 		ctx context.Context,
-		n *nutanixChecker,
+		nclientFactory func(prismgoclient.Credentials) (client, error),
+		cd *checkDependencies,
 	) preflight.Check
 
-	initVMImageChecksFunc func(
-		n *nutanixChecker,
+	vmImageChecksFactory func(
+		cd *checkDependencies,
 	) []preflight.Check
+}
+
+type checkDependencies struct {
+	kclient ctrlclient.Client
+	cluster *clusterv1.Cluster
 
-	log logr.Logger
+	nutanixClusterConfigSpec                           *carenv1.NutanixClusterConfigSpec
+	nutanixWorkerNodeConfigSpecByMachineDeploymentName map[string]*carenv1.NutanixWorkerNodeConfigSpec
+
+	nclient client
+	log     logr.Logger
 }
 
 func (n *nutanixChecker) Init(
 	ctx context.Context,
+	kclient ctrlclient.Client,
+	cluster *clusterv1.Cluster,
 ) []preflight.Check {
-	n.log = ctrl.LoggerFrom(ctx).WithName("preflight/nutanix")
+	cd := &checkDependencies{
+		kclient: kclient,
+		cluster: cluster,
+		log:     ctrl.LoggerFrom(ctx).WithName("preflight/nutanix"),
+	}
 
 	checks := []preflight.Check{
 		// The configuration check must run first, because it initializes data used by all other checks,
 		// and the credentials check second, because it initializes the Nutanix clients used by other checks.
-		n.initNutanixConfigurationFunc(n),
-		n.initCredentialsCheckFunc(ctx, n),
+		n.configurationCheckFactory(cd),
+		n.credentialsCheckFactory(ctx, newClient, cd),
 	}
 
-	checks = append(checks, n.initVMImageChecksFunc(n)...)
+	checks = append(checks, n.vmImageChecksFactory(cd)...)
 
 	// Add more checks here as needed.
 

pkg/webhook/preflight/nutanix/checker_test.go

@@ -9,8 +9,11 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 
+	prismgoclient "github.com/nutanix-cloud-native/prism-go-client"
+
 	carenv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/webhook/preflight"
 )
@@ -98,34 +101,34 @@ func TestNutanixChecker_Init(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			// Create the checker
-			checker := &nutanixChecker{
-				cluster:                  &clusterv1.Cluster{},
-				nutanixClusterConfigSpec: tt.nutanixConfig,
-				nutanixWorkerNodeConfigSpecByMachineDeploymentName: tt.workerNodeConfigs,
-			}
+			checker := &nutanixChecker{}
 
 			// Mock the sub-check functions to track their calls
 			configCheckCalled := false
 			credsCheckCalled := false
 			vmImageCheckCount := 0
 
-			checker.initNutanixConfigurationFunc = func(n *nutanixChecker) preflight.Check {
+			checker.configurationCheckFactory = func(cd *checkDependencies) preflight.Check {
 				configCheckCalled = true
 				return &mockCheck{
 					name:   tt.expectedFirstCheckName,
 					result: preflight.CheckResult{Allowed: true},
 				}
 			}
 
-			checker.initCredentialsCheckFunc = func(ctx context.Context, n *nutanixChecker) preflight.Check {
+			checker.credentialsCheckFactory = func(
+				ctx context.Context,
+				nclientFactory func(prismgoclient.Credentials) (client, error),
+				cd *checkDependencies,
+			) preflight.Check {
 				credsCheckCalled = true
 				return &mockCheck{
 					name:   tt.expectedSecondCheckName,
 					result: preflight.CheckResult{Allowed: true},
 				}
 			}
 
-			checker.initVMImageChecksFunc = func(n *nutanixChecker) []preflight.Check {
+			checker.vmImageChecksFactory = func(cd *checkDependencies) []preflight.Check {
 				checks := []preflight.Check{}
 				for i := 0; i < tt.vmImageCheckCount; i++ {
 					vmImageCheckCount++
@@ -143,10 +146,12 @@ func TestNutanixChecker_Init(t *testing.T) {
 
 			// Call Init
 			ctx := context.Background()
-			checks := checker.Init(ctx)
-
-			// Verify the logger was set
-			assert.NotNil(t, checker.log)
+			checks := checker.Init(ctx, nil, &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: "default",
+				},
+			})
 
 			// Verify correct number of checks
 			assert.Len(t, checks, tt.expectedCheckCount)

pkg/webhook/preflight/nutanix/credentials.go

@@ -30,26 +30,27 @@ func (c *credentialsCheck) Run(_ context.Context) preflight.CheckResult {
 	return c.result
 }
 
-func initCredentialsCheck(
+func newCredentialsCheck(
 	ctx context.Context,
-	n *nutanixChecker,
+	nclientFactory func(prismgoclient.Credentials) (client, error),
+	cd *checkDependencies,
 ) preflight.Check {
-	n.log.V(5).Info("Initializing Nutanix credentials check")
+	cd.log.V(5).Info("Initializing Nutanix credentials check")
 
 	credentialsCheck := &credentialsCheck{
 		result: preflight.CheckResult{
 			Allowed: true,
 		},
 	}
 
-	if n.nutanixClusterConfigSpec == nil && len(n.nutanixWorkerNodeConfigSpecByMachineDeploymentName) == 0 {
+	if cd.nutanixClusterConfigSpec == nil && len(cd.nutanixWorkerNodeConfigSpecByMachineDeploymentName) == 0 {
 		// If there is no Nutanix configuration at all, the credentials check is not needed.
 		return credentialsCheck
 	}
 
 	// There is some Nutanix configuration, so the credentials check is needed.
 	// However, the credentials configuration is missing, so we cannot perform the check.
-	if n.nutanixClusterConfigSpec == nil || n.nutanixClusterConfigSpec.Nutanix == nil {
+	if cd.nutanixClusterConfigSpec == nil || cd.nutanixClusterConfigSpec.Nutanix == nil {
 		credentialsCheck.result.Allowed = false
 		credentialsCheck.result.Error = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
@@ -62,7 +63,7 @@ func initCredentialsCheck(
 	}
 
 	// Get the credentials data in order to initialize the credentials and clients.
-	prismCentralEndpointSpec := n.nutanixClusterConfigSpec.Nutanix.PrismCentralEndpoint
+	prismCentralEndpointSpec := cd.nutanixClusterConfigSpec.Nutanix.PrismCentralEndpoint
 
 	host, port, err := prismCentralEndpointSpec.ParseURL()
 	if err != nil {
@@ -79,10 +80,10 @@ func initCredentialsCheck(
 	}
 
 	credentialsSecret := &corev1.Secret{}
-	err = n.kclient.Get(
+	err = cd.kclient.Get(
 		ctx,
 		types.NamespacedName{
-			Namespace: n.cluster.Namespace,
+			Namespace: cd.cluster.Namespace,
 			Name:      prismCentralEndpointSpec.Credentials.SecretRef.Name,
 		},
 		credentialsSecret,
@@ -154,7 +155,7 @@ func initCredentialsCheck(
 	}
 
 	// Initialize the Nutanix client.
-	nclient, err := n.nclientFactory(credentials)
+	nclient, err := nclientFactory(credentials)
 	if err != nil {
 		credentialsCheck.result.Allowed = false
 		credentialsCheck.result.Error = true
@@ -183,7 +184,7 @@ func initCredentialsCheck(
 	}
 
 	// We initialized both clients, and verified the credentials using the v3 client.
-	n.nclient = nclient
+	cd.nclient = nclient
 
 	return credentialsCheck
 }