Skip to content

Commit 7b06600

Browse files
dlipovetskydlipovetsky
committed
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.
1 parent 40b981e commit 7b06600

File tree

2 files changed

+125
-80
lines changed

2 files changed

+125
-80
lines changed

‎pkg/webhook/preflight/nutanix/storagecontainer.go‎

Lines changed: 68 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,9 @@ package nutanix
66
import (
77
"context"
88
"fmt"
9+
"sync"
910

1011
clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config"
11-
clustermgmtv4errors "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/error"
12-
"k8s.io/utils/ptr"
1312

1413
"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/external/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
1514
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 {
6362
return result
6463
}
6564

65+
clusterIdentifier := &c.nodeSpec.MachineDetails.Cluster
66+
67+
// We wait to get the cluster until we know we need to.
68+
// We should only get the cluster once.
69+
getClusterOnce := sync.OnceValues(func() (*clustermgmtv4.Cluster, error) {
70+
return getCluster(c.nclient, clusterIdentifier)
71+
})
72+
6673
for _, storageClassConfig := range c.csiSpec.StorageClassConfigs {
6774
if storageClassConfig.Parameters == nil {
6875
continue
@@ -73,19 +80,62 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
7380
continue
7481
}
7582

76-
if _, err := getStorageContainer(c.nclient, c.nodeSpec, storageContainer); err != nil {
83+
cluster, err := getClusterOnce()
84+
if err != nil {
85+
result.Allowed = false
86+
result.Error = true
87+
result.Causes = append(result.Causes, preflight.Cause{
88+
Message: fmt.Sprintf(
89+
"failed to check if storage container %q exists: failed to get cluster %q: %s",
90+
storageContainer,
91+
clusterIdentifier,
92+
err,
93+
),
94+
Field: c.field,
95+
})
96+
continue
97+
}
98+
99+
containers, err := getStorageContainers(c.nclient, *cluster.ExtId, storageContainer)
100+
if err != nil {
77101
result.Allowed = false
78102
result.Error = true
79103
result.Causes = append(result.Causes, preflight.Cause{
80104
Message: fmt.Sprintf(
81-
"failed to check if storage container named %q exists: %s",
105+
"failed to check if storage container %q exists in cluster %q: %s",
82106
storageContainer,
107+
clusterIdentifier,
83108
err,
84109
),
85110
Field: c.field,
86111
})
112+
continue
113+
}
87114

88-
return result
115+
if len(containers) == 0 {
116+
result.Allowed = false
117+
result.Causes = append(result.Causes, preflight.Cause{
118+
Message: fmt.Sprintf(
119+
"storage container %q not found on cluster %q",
120+
storageContainer,
121+
clusterIdentifier,
122+
),
123+
Field: c.field,
124+
})
125+
continue
126+
}
127+
128+
if len(containers) > 1 {
129+
result.Allowed = false
130+
result.Causes = append(result.Causes, preflight.Cause{
131+
Message: fmt.Sprintf(
132+
"multiple storage containers named %q found on cluster %q",
133+
storageContainer,
134+
clusterIdentifier,
135+
),
136+
Field: c.field,
137+
})
138+
continue
89139
}
90140
}
91141

@@ -138,55 +188,25 @@ func newStorageContainerChecks(cd *checkDependencies) []preflight.Check {
138188
return checks
139189
}
140190

141-
func getStorageContainer(
191+
func getStorageContainers(
142192
client client,
143-
nodeSpec *carenv1.NutanixNodeSpec,
193+
clusterUUID string,
144194
storageContainerName string,
145-
) (*clustermgmtv4.StorageContainer, error) {
146-
cluster, err := getCluster(client, &nodeSpec.MachineDetails.Cluster)
147-
if err != nil {
148-
return nil, fmt.Errorf("failed to get cluster: %w", err)
149-
}
150-
151-
fltr := fmt.Sprintf("name eq '%s' and clusterExtId eq '%s'", storageContainerName, *cluster.ExtId)
195+
) ([]clustermgmtv4.StorageContainer, error) {
196+
fltr := fmt.Sprintf("name eq '%s' and clusterExtId eq '%s'", storageContainerName, clusterUUID)
152197
resp, err := client.ListStorageContainers(nil, nil, &fltr, nil, nil)
153198
if err != nil {
154-
return nil, fmt.Errorf("failed to list storage containers: %w", err)
199+
return nil, err
155200
}
156-
157-
switch data := resp.GetData().(type) {
158-
case nil:
159-
return nil, fmt.Errorf("failed to find a matching storage container")
160-
161-
case clustermgmtv4errors.ErrorResponse:
162-
return nil, fmt.Errorf("failed to list storage containers: %v", data.GetError())
163-
164-
case []clustermgmtv4.StorageContainer:
165-
if len(data) == 0 {
166-
return nil, fmt.Errorf(
167-
"no storage container named %q found on cluster named %q",
168-
storageContainerName,
169-
*cluster.Name,
170-
)
171-
}
172-
173-
if len(data) > 1 {
174-
return nil, fmt.Errorf(
175-
"multiple storage containers found with name %q on cluster %q",
176-
storageContainerName,
177-
*cluster.Name,
178-
)
179-
}
180-
181-
return ptr.To(data[0]), nil
182-
default:
183-
return nil,
184-
fmt.Errorf(
185-
"unexpected response type from ListStorageContainers(filter=%q): %T",
186-
fltr,
187-
resp.GetData(),
188-
)
201+
if resp == nil || resp.GetData() == nil {
202+
// No images were returned.
203+
return []clustermgmtv4.StorageContainer{}, nil
204+
}
205+
containers, ok := resp.GetData().([]clustermgmtv4.StorageContainer)
206+
if !ok {
207+
return nil, fmt.Errorf("failed to get data returned by ListStorageContainers(filter=%q)", fltr)
189208
}
209+
return containers, nil
190210
}
191211

192212
func getCluster(

‎pkg/webhook/preflight/nutanix/storagecontainer_test.go‎

Lines changed: 57 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"testing"
1010

1111
clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config"
12+
clustermgmtv4errors "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/error"
1213
"github.com/stretchr/testify/assert"
1314
"github.com/stretchr/testify/require"
1415
"k8s.io/utils/ptr"
@@ -405,10 +406,9 @@ func TestStorageContainerCheck(t *testing.T) {
405406
return resp, nil
406407
},
407408
},
408-
expectedAllowed: false,
409-
expectedError: true,
410-
expectedCauseMessage: "failed to check if storage container named \"missing-container\" exists:" +
411-
" no storage container named \"missing-container\" found on cluster named",
409+
expectedAllowed: false,
410+
expectedError: false,
411+
expectedCauseMessage: "storage container \"missing-container\" not found on cluster \"test-cluster\"",
412412
},
413413
{
414414
name: "multiple storage containers found",
@@ -483,10 +483,9 @@ func TestStorageContainerCheck(t *testing.T) {
483483
return resp, nil
484484
},
485485
},
486-
expectedAllowed: false,
487-
expectedError: true,
488-
expectedCauseMessage: "failed to check if storage container named \"duplicate-container\" exists:" +
489-
" multiple storage containers found with name",
486+
expectedAllowed: false,
487+
expectedError: false,
488+
expectedCauseMessage: "multiple storage containers named \"duplicate-container\" found on cluster \"test-cluster\"",
490489
},
491490
{
492491
name: "successful storage container check",
@@ -601,8 +600,8 @@ func TestStorageContainerCheck(t *testing.T) {
601600
},
602601
expectedAllowed: false,
603602
expectedError: true,
604-
expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" +
605-
" failed to get cluster: API error",
603+
expectedCauseMessage: "failed to check if storage container \"valid-container\" exists: " +
604+
"failed to get cluster \"test-cluster\": API error",
606605
},
607606
{
608607
name: "error listing storage containers",
@@ -667,11 +666,12 @@ func TestStorageContainerCheck(t *testing.T) {
667666
},
668667
expectedAllowed: false,
669668
expectedError: true,
670-
expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" +
671-
" failed to list storage containers: API error listing containers",
669+
// preflight.Cause {Message: "failed to check if storage container \"valid-container\" exists in cluster \"test-cluster\": API error listing containers", Field: "test.field.path"}
670+
expectedCauseMessage: "failed to check if storage container \"valid-container\" exists in cluster " +
671+
"\"test-cluster\": API error listing containers",
672672
},
673673
{
674-
name: "invalid response data type",
674+
name: "error response from ListStorageContainers",
675675
nodeSpec: &carenv1.NutanixNodeSpec{
676676
MachineDetails: carenv1.NutanixMachineDetails{
677677
Cluster: capxv1.NutanixResourceIdentifier{
@@ -728,18 +728,20 @@ func TestStorageContainerCheck(t *testing.T) {
728728
*clustermgmtv4.ListStorageContainersApiResponse,
729729
error,
730730
) {
731-
// Return a non-nil response but with nil Data or wrong type to simulate data conversion error
732-
return &clustermgmtv4.ListStorageContainersApiResponse{
733-
ObjectType_: ptr.To("wrong-data-type"),
734-
}, nil
731+
resp := &clustermgmtv4.ListStorageContainersApiResponse{}
732+
err := resp.SetData(*clustermgmtv4errors.NewErrorResponse())
733+
require.NoError(t, err)
734+
return resp, nil
735735
},
736736
},
737-
expectedAllowed: false,
738-
expectedError: true,
739-
expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists: failed to find a matching storage container", //nolint:lll // long error message
737+
expectedAllowed: false,
738+
expectedError: true,
739+
expectedCauseMessage: "failed to check if storage container \"valid-container\" exists in cluster " +
740+
"\"test-cluster\": failed to get data returned by ListStorageContainers" +
741+
"(filter=\"name eq 'valid-container' and clusterExtId eq 'cluster-uuid-123'\")",
740742
},
741743
{
742-
name: "nil response data type",
744+
name: "nil data from ListStorageContainers",
743745
nodeSpec: &carenv1.NutanixNodeSpec{
744746
MachineDetails: carenv1.NutanixMachineDetails{
745747
Cluster: capxv1.NutanixResourceIdentifier{
@@ -796,15 +798,12 @@ func TestStorageContainerCheck(t *testing.T) {
796798
*clustermgmtv4.ListStorageContainersApiResponse,
797799
error,
798800
) {
799-
// Return a non-nil response but with nil Data or wrong type to simulate data conversion error
800-
return &clustermgmtv4.ListStorageContainersApiResponse{
801-
Data: nil,
802-
}, nil
801+
return &clustermgmtv4.ListStorageContainersApiResponse{}, nil
803802
},
804803
},
805804
expectedAllowed: false,
806-
expectedError: true,
807-
expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists: failed to find a matching storage container", //nolint:lll // long error message
805+
expectedError: false,
806+
expectedCauseMessage: "storage container \"valid-container\" not found on cluster \"test-cluster\"",
808807
},
809808
{
810809
name: "multiple storage class configs with success",
@@ -982,17 +981,16 @@ func TestGetCluster(t *testing.T) {
982981
errorContains: "API error",
983982
},
984983
{
985-
name: "get cluster by UUID - invalid response data",
984+
name: "get cluster by UUID - error response",
986985
clusterIdentifier: &capxv1.NutanixResourceIdentifier{
987986
Type: capxv1.NutanixIdentifierUUID,
988987
UUID: ptr.To("test-uuid-invalid"),
989988
},
990989
client: &mocknclient{
991990
getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) {
992-
// Return an invalid data type
993-
resp := &clustermgmtv4.GetClusterApiResponse{
994-
ObjectType_: ptr.To("wrong-data-type"),
995-
}
991+
resp := &clustermgmtv4.GetClusterApiResponse{}
992+
err := resp.SetData(*clustermgmtv4errors.NewErrorResponse())
993+
require.NoError(t, err)
996994
return resp, nil
997995
},
998996
},
@@ -1083,6 +1081,33 @@ func TestGetCluster(t *testing.T) {
10831081
expectError: true,
10841082
errorContains: "no clusters were returned",
10851083
},
1084+
{
1085+
name: "get cluster by name - error response",
1086+
clusterIdentifier: &capxv1.NutanixResourceIdentifier{
1087+
Type: capxv1.NutanixIdentifierName,
1088+
Name: ptr.To("test-cluster-nil"),
1089+
},
1090+
client: &mocknclient{
1091+
listClustersFunc: func(page,
1092+
limit *int,
1093+
filter,
1094+
orderby,
1095+
apply,
1096+
select_ *string,
1097+
args ...map[string]interface{},
1098+
) (
1099+
*clustermgmtv4.ListClustersApiResponse,
1100+
error,
1101+
) {
1102+
resp := &clustermgmtv4.ListClustersApiResponse{}
1103+
err := resp.SetData(*clustermgmtv4errors.NewErrorResponse())
1104+
require.NoError(t, err)
1105+
return resp, nil
1106+
},
1107+
},
1108+
expectError: true,
1109+
errorContains: "failed to get data returned by ListClusters",
1110+
},
10861111
{
10871112
name: "get cluster by name - nil data",
10881113
clusterIdentifier: &capxv1.NutanixResourceIdentifier{

0 commit comments

Comments
 (0)