From 1911e2eebf0db6aec14e723c61938d41db316f73 Mon Sep 17 00:00:00 2001 From: Sid Shukla Date: Tue, 24 Jun 2025 21:46:27 +0200 Subject: [PATCH 1/4] fix(preflight): improved error reporting for storage container checks more user-friendly errors. --- .../preflight/nutanix/storagecontainer.go | 53 ++++++++----- .../nutanix/storagecontainer_test.go | 75 ++++++++++++++++++- 2 files changed, 106 insertions(+), 22 deletions(-) diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go index 2a68ad440..61a713519 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -8,6 +8,7 @@ import ( "fmt" clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" + clustermgmtv4errors "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/error" "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" @@ -153,28 +154,44 @@ func getStorageContainer( return nil, fmt.Errorf("failed to list storage containers: %w", err) } - containers, ok := resp.GetData().([]clustermgmtv4.StorageContainer) - if !ok { - return nil, fmt.Errorf("failed to get data returned by ListStorageContainers(filter=%q)", fltr) - } + switch resp.GetData().(type) { + case nil: + return nil, fmt.Errorf("failed to find a matching storage container") - if len(containers) == 0 { - return nil, fmt.Errorf( - "no storage container named %q found on cluster named %q", - storageContainerName, - *cluster.Name, - ) - } + case clustermgmtv4errors.ErrorResponse: + errResp, ok := resp.GetData().(clustermgmtv4errors.ErrorResponse) + if !ok { + return nil, fmt.Errorf("failed to parse error response from %v", resp.GetData()) + } - if len(containers) > 1 { - return nil, fmt.Errorf( - "multiple storage containers found with name %q on cluster %q", - storageContainerName, - *cluster.Name, - ) + return nil, fmt.Errorf("failed to list storage containers: %v", errResp.GetError()) + + case []clustermgmtv4.StorageContainer: + containers, ok := resp.GetData().([]clustermgmtv4.StorageContainer) + if !ok { + return nil, fmt.Errorf("failed to parse storage containers from %v", resp.GetData()) + } + + 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 ptr.To(containers[0]), nil + return nil, fmt.Errorf("unexpected response type from ListStorageContainers(filter=%q): %T", fltr, resp.GetData()) } func getCluster( diff --git a/pkg/webhook/preflight/nutanix/storagecontainer_test.go b/pkg/webhook/preflight/nutanix/storagecontainer_test.go index e76207b8d..1beb3a70b 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer_test.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer_test.go @@ -734,10 +734,77 @@ func TestStorageContainerCheck(t *testing.T) { }, nil }, }, - expectedAllowed: false, - expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" + - " failed to get data returned by ListStorageContainers", + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists: failed to find a matching storage container", //nolint:lll // long error message + }, + { + name: "nil response data type", + nodeSpec: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To(clusterName), + }, + }, + }, + csiSpec: &carenv1.CSIProvider{ + StorageClassConfigs: map[string]carenv1.StorageClassConfig{ + "test-sc": { + Parameters: map[string]string{ + "storageContainer": "valid-container", + }, + }, + }, + }, + nclient: &mocknclient{ + getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + return nil, nil + }, + listClustersFunc: func( + page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + resp := &clustermgmtv4.ListClustersApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.ListClustersApiResponse"), + } + err := resp.SetData([]clustermgmtv4.Cluster{ + { + Name: ptr.To(clusterName), + ExtId: ptr.To("cluster-uuid-123"), + }, + }) + require.NoError(t, err) + return resp, nil + }, + listStorageContainersFunc: func( + page, + limit *int, + filter, + orderby, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListStorageContainersApiResponse, + error, + ) { + // Return a non-nil response but with nil Data or wrong type to simulate data conversion error + return &clustermgmtv4.ListStorageContainersApiResponse{ + Data: nil, + }, nil + }, + }, + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists: failed to find a matching storage container", //nolint:lll // long error message }, { name: "multiple storage class configs with success", From 40b981e53ee2b7d591b9bc5854a8245f8ca109c3 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Tue, 24 Jun 2025 14:40:52 -0700 Subject: [PATCH 2/4] fixup! fix(preflight): improved error reporting for storage container checks Remove unnecessary type conversions --- .../preflight/nutanix/storagecontainer.go | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go index 61a713519..14ca62842 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -154,25 +154,15 @@ func getStorageContainer( return nil, fmt.Errorf("failed to list storage containers: %w", err) } - switch resp.GetData().(type) { + switch data := resp.GetData().(type) { case nil: return nil, fmt.Errorf("failed to find a matching storage container") case clustermgmtv4errors.ErrorResponse: - errResp, ok := resp.GetData().(clustermgmtv4errors.ErrorResponse) - if !ok { - return nil, fmt.Errorf("failed to parse error response from %v", resp.GetData()) - } - - return nil, fmt.Errorf("failed to list storage containers: %v", errResp.GetError()) + return nil, fmt.Errorf("failed to list storage containers: %v", data.GetError()) case []clustermgmtv4.StorageContainer: - containers, ok := resp.GetData().([]clustermgmtv4.StorageContainer) - if !ok { - return nil, fmt.Errorf("failed to parse storage containers from %v", resp.GetData()) - } - - if len(containers) == 0 { + if len(data) == 0 { return nil, fmt.Errorf( "no storage container named %q found on cluster named %q", storageContainerName, @@ -180,7 +170,7 @@ func getStorageContainer( ) } - if len(containers) > 1 { + if len(data) > 1 { return nil, fmt.Errorf( "multiple storage containers found with name %q on cluster %q", storageContainerName, @@ -188,10 +178,15 @@ func getStorageContainer( ) } - return ptr.To(containers[0]), nil + return ptr.To(data[0]), nil + default: + return nil, + fmt.Errorf( + "unexpected response type from ListStorageContainers(filter=%q): %T", + fltr, + resp.GetData(), + ) } - - return nil, fmt.Errorf("unexpected response type from ListStorageContainers(filter=%q): %T", fltr, resp.GetData()) } func getCluster( From 6a2190f0d070b5cb03f899bfd653bc44558def35 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Tue, 24 Jun 2025 10:13:17 -0700 Subject: [PATCH 3/4] fixup! fix(preflight): improved error reporting for storage container checks - 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. --- .../preflight/nutanix/storagecontainer.go | 116 ++++++++++-------- .../nutanix/storagecontainer_test.go | 88 ++++++++----- 2 files changed, 124 insertions(+), 80 deletions(-) diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go index 14ca62842..a44b7d177 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -6,10 +6,9 @@ package nutanix import ( "context" "fmt" + "sync" clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" - clustermgmtv4errors "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/error" - "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" @@ -63,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 @@ -73,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 + } - return result + 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 + } + + 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 } } @@ -138,55 +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 } - - switch data := resp.GetData().(type) { - case nil: - return nil, fmt.Errorf("failed to find a matching storage container") - - case clustermgmtv4errors.ErrorResponse: - return nil, fmt.Errorf("failed to list storage containers: %v", data.GetError()) - - case []clustermgmtv4.StorageContainer: - if len(data) == 0 { - return nil, fmt.Errorf( - "no storage container named %q found on cluster named %q", - storageContainerName, - *cluster.Name, - ) - } - - if len(data) > 1 { - return nil, fmt.Errorf( - "multiple storage containers found with name %q on cluster %q", - storageContainerName, - *cluster.Name, - ) - } - - return ptr.To(data[0]), nil - default: - return nil, - fmt.Errorf( - "unexpected response type from ListStorageContainers(filter=%q): %T", - fltr, - resp.GetData(), - ) + 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) } + return containers, nil } func getCluster( diff --git a/pkg/webhook/preflight/nutanix/storagecontainer_test.go b/pkg/webhook/preflight/nutanix/storagecontainer_test.go index 1beb3a70b..d561af2aa 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer_test.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer_test.go @@ -9,6 +9,7 @@ import ( "testing" clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" + clustermgmtv4errors "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/error" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/utils/ptr" @@ -405,10 +406,9 @@ func TestStorageContainerCheck(t *testing.T) { return resp, nil }, }, - expectedAllowed: false, - expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"missing-container\" exists:" + - " no storage container named \"missing-container\" found on cluster named", + expectedAllowed: false, + expectedError: false, + expectedCauseMessage: "storage container \"missing-container\" not found on cluster \"test-cluster\"", }, { name: "multiple storage containers found", @@ -483,10 +483,9 @@ func TestStorageContainerCheck(t *testing.T) { return resp, nil }, }, - expectedAllowed: false, - expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"duplicate-container\" exists:" + - " multiple storage containers found with name", + expectedAllowed: false, + expectedError: false, + expectedCauseMessage: "multiple storage containers named \"duplicate-container\" found on cluster \"test-cluster\"", }, { name: "successful storage container check", @@ -601,8 +600,8 @@ func TestStorageContainerCheck(t *testing.T) { }, expectedAllowed: false, expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" + - " failed to get cluster: API error", + expectedCauseMessage: "failed to check if storage container \"valid-container\" exists: " + + "failed to get cluster \"test-cluster\": API error", }, { name: "error listing storage containers", @@ -667,11 +666,11 @@ func TestStorageContainerCheck(t *testing.T) { }, expectedAllowed: false, expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" + - " failed to list storage containers: API error listing containers", + expectedCauseMessage: "failed to check if storage container \"valid-container\" exists in cluster " + + "\"test-cluster\": API error listing containers", }, { - name: "invalid response data type", + name: "error response from ListStorageContainers", nodeSpec: &carenv1.NutanixNodeSpec{ MachineDetails: carenv1.NutanixMachineDetails{ Cluster: capxv1.NutanixResourceIdentifier{ @@ -728,18 +727,20 @@ func TestStorageContainerCheck(t *testing.T) { *clustermgmtv4.ListStorageContainersApiResponse, error, ) { - // Return a non-nil response but with nil Data or wrong type to simulate data conversion error - return &clustermgmtv4.ListStorageContainersApiResponse{ - ObjectType_: ptr.To("wrong-data-type"), - }, nil + resp := &clustermgmtv4.ListStorageContainersApiResponse{} + err := resp.SetData(*clustermgmtv4errors.NewErrorResponse()) + require.NoError(t, err) + return resp, nil }, }, - expectedAllowed: false, - expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists: failed to find a matching storage container", //nolint:lll // long error message + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container \"valid-container\" exists in cluster " + + "\"test-cluster\": failed to get data returned by ListStorageContainers" + + "(filter=\"name eq 'valid-container' and clusterExtId eq 'cluster-uuid-123'\")", }, { - name: "nil response data type", + name: "nil data from ListStorageContainers", nodeSpec: &carenv1.NutanixNodeSpec{ MachineDetails: carenv1.NutanixMachineDetails{ Cluster: capxv1.NutanixResourceIdentifier{ @@ -796,15 +797,12 @@ func TestStorageContainerCheck(t *testing.T) { *clustermgmtv4.ListStorageContainersApiResponse, error, ) { - // Return a non-nil response but with nil Data or wrong type to simulate data conversion error - return &clustermgmtv4.ListStorageContainersApiResponse{ - Data: nil, - }, nil + return &clustermgmtv4.ListStorageContainersApiResponse{}, nil }, }, expectedAllowed: false, - expectedError: true, - expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists: failed to find a matching storage container", //nolint:lll // long error message + expectedError: false, + expectedCauseMessage: "storage container \"valid-container\" not found on cluster \"test-cluster\"", }, { name: "multiple storage class configs with success", @@ -982,17 +980,16 @@ func TestGetCluster(t *testing.T) { errorContains: "API error", }, { - name: "get cluster by UUID - invalid response data", + name: "get cluster by UUID - error response", clusterIdentifier: &capxv1.NutanixResourceIdentifier{ Type: capxv1.NutanixIdentifierUUID, UUID: ptr.To("test-uuid-invalid"), }, client: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { - // Return an invalid data type - resp := &clustermgmtv4.GetClusterApiResponse{ - ObjectType_: ptr.To("wrong-data-type"), - } + resp := &clustermgmtv4.GetClusterApiResponse{} + err := resp.SetData(*clustermgmtv4errors.NewErrorResponse()) + require.NoError(t, err) return resp, nil }, }, @@ -1083,6 +1080,33 @@ func TestGetCluster(t *testing.T) { expectError: true, errorContains: "no clusters were returned", }, + { + name: "get cluster by name - error response", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster-nil"), + }, + client: &mocknclient{ + listClustersFunc: func(page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + resp := &clustermgmtv4.ListClustersApiResponse{} + err := resp.SetData(*clustermgmtv4errors.NewErrorResponse()) + require.NoError(t, err) + return resp, nil + }, + }, + expectError: true, + errorContains: "failed to get data returned by ListClusters", + }, { name: "get cluster by name - nil data", clusterIdentifier: &capxv1.NutanixResourceIdentifier{ From da1e63bbeaf29d4b988d6b8f53c0b425131f413d Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Wed, 25 Jun 2025 12:38:38 -0700 Subject: [PATCH 4/4] fixup! fix(preflight): improved error reporting for storage container checks Handle error response for list images API call --- pkg/webhook/preflight/nutanix/image_test.go | 37 +++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/pkg/webhook/preflight/nutanix/image_test.go b/pkg/webhook/preflight/nutanix/image_test.go index e8083873d..8a095d312 100644 --- a/pkg/webhook/preflight/nutanix/image_test.go +++ b/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{},