fix(preflight): improved error reporting for storage container and VM image checks (#1180)

- Handle nil responses of get storage container and get cluster API
calls.
- Improve error wording. 
- Correctly construct error responses of list API calls.
- Test error response of list clusters API call.
- Treat a missing storageContainer as a failed check, but not an
internal error.

---------

Co-authored-by: Daniel Lipovetsky <[email protected]>

Author: thunderboltsid

pkg/webhook/preflight/nutanix/image_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/go-logr/logr/testr"
 	vmmv4 "github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4/models/vmm/v4/content"
+	vmmv4error "github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4/models/vmm/v4/error"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/utils/ptr"
@@ -230,6 +231,42 @@ func TestVMImageCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "listing images returns an error response",
+			nclient: &mocknclient{
+				listImagesFunc: func(page,
+					limit *int,
+					filter,
+					orderby,
+					select_ *string,
+					args ...map[string]interface{},
+				) (
+					*vmmv4.ListImagesApiResponse,
+					error,
+				) {
+					resp := &vmmv4.ListImagesApiResponse{}
+					err := resp.SetData(*vmmv4error.NewErrorResponse())
+					require.NoError(t, err)
+					return resp, nil
+				},
+			},
+			machineDetails: &carenv1.NutanixMachineDetails{
+				Image: &capxv1.NutanixResourceIdentifier{
+					Type: capxv1.NutanixIdentifierName,
+					Name: ptr.To("test-image"),
+				},
+			},
+			want: preflight.CheckResult{
+				Allowed: false,
+				Error:   true,
+				Causes: []preflight.Cause{
+					{
+						Message: "failed to get VM Image: failed to get data returned by ListImages",
+						Field:   "test-field",
+					},
+				},
+			},
+		},
 		{
 			name:           "neither image nor imageLookup specified",
 			nclient:        &mocknclient{},

pkg/webhook/preflight/nutanix/storagecontainer.go

@@ -6,9 +6,9 @@ package nutanix
 import (
 	"context"
 	"fmt"
+	"sync"
 
 	clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config"
-	"k8s.io/utils/ptr"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/external/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
 	carenv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
@@ -62,6 +62,14 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 		return result
 	}
 
+	clusterIdentifier := &c.nodeSpec.MachineDetails.Cluster
+
+	// We wait to get the cluster until we know we need to.
+	// We should only get the cluster once.
+	getClusterOnce := sync.OnceValues(func() (*clustermgmtv4.Cluster, error) {
+		return getCluster(c.nclient, clusterIdentifier)
+	})
+
 	for _, storageClassConfig := range c.csiSpec.StorageClassConfigs {
 		if storageClassConfig.Parameters == nil {
 			continue
@@ -72,19 +80,62 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 			continue
 		}
 
-		if _, err := getStorageContainer(c.nclient, c.nodeSpec, storageContainer); err != nil {
+		cluster, err := getClusterOnce()
+		if err != nil {
+			result.Allowed = false
+			result.Error = true
+			result.Causes = append(result.Causes, preflight.Cause{
+				Message: fmt.Sprintf(
+					"failed to check if storage container %q exists: failed to get cluster %q: %s",
+					storageContainer,
+					clusterIdentifier,
+					err,
+				),
+				Field: c.field,
+			})
+			continue
+		}
+
+		containers, err := getStorageContainers(c.nclient, *cluster.ExtId, storageContainer)
+		if err != nil {
 			result.Allowed = false
 			result.Error = true
 			result.Causes = append(result.Causes, preflight.Cause{
 				Message: fmt.Sprintf(
-					"failed to check if storage container named %q exists: %s",
+					"failed to check if storage container %q exists in cluster %q: %s",
 					storageContainer,
+					clusterIdentifier,
 					err,
 				),
 				Field: c.field,
 			})
+			continue
+		}
+
+		if len(containers) == 0 {
+			result.Allowed = false
+			result.Causes = append(result.Causes, preflight.Cause{
+				Message: fmt.Sprintf(
+					"storage container %q not found on cluster %q",
+					storageContainer,
+					clusterIdentifier,
+				),
+				Field: c.field,
+			})
+			continue
+		}
 
-			return result
+		if len(containers) > 1 {
+			result.Allowed = false
+			result.Causes = append(result.Causes, preflight.Cause{
+				Message: fmt.Sprintf(
+					"multiple storage containers named %q found on cluster %q",
+					storageContainer,
+					clusterIdentifier,
+				),
+				Field: c.field,
+			})
+			continue
 		}
 	}
 
@@ -137,44 +188,25 @@ func newStorageContainerChecks(cd *checkDependencies) []preflight.Check {
 	return checks
 }
 
-func getStorageContainer(
+func getStorageContainers(
 	client client,
-	nodeSpec *carenv1.NutanixNodeSpec,
+	clusterUUID string,
 	storageContainerName string,
-) (*clustermgmtv4.StorageContainer, error) {
-	cluster, err := getCluster(client, &nodeSpec.MachineDetails.Cluster)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get cluster: %w", err)
-	}
-
-	fltr := fmt.Sprintf("name eq '%s' and clusterExtId eq '%s'", storageContainerName, *cluster.ExtId)
+) ([]clustermgmtv4.StorageContainer, error) {
+	fltr := fmt.Sprintf("name eq '%s' and clusterExtId eq '%s'", storageContainerName, clusterUUID)
 	resp, err := client.ListStorageContainers(nil, nil, &fltr, nil, nil)
 	if err != nil {
-		return nil, fmt.Errorf("failed to list storage containers: %w", err)
+		return nil, err
+	}
+	if resp == nil || resp.GetData() == nil {
+		// No images were returned.
+		return []clustermgmtv4.StorageContainer{}, nil
 	}
-
 	containers, ok := resp.GetData().([]clustermgmtv4.StorageContainer)
 	if !ok {
 		return nil, fmt.Errorf("failed to get data returned by ListStorageContainers(filter=%q)", fltr)
 	}
-
-	if len(containers) == 0 {
-		return nil, fmt.Errorf(
-			"no storage container named %q found on cluster named %q",
-			storageContainerName,
-			*cluster.Name,
-		)
-	}
-
-	if len(containers) > 1 {
-		return nil, fmt.Errorf(
-			"multiple storage containers found with name %q on cluster %q",
-			storageContainerName,
-			*cluster.Name,
-		)
-	}
-
-	return ptr.To(containers[0]), nil
+	return containers, nil
 }
 
 func getCluster(