From 3e1edfb55fa50ab0c3e17f5053c5f6cd892dfe47 Mon Sep 17 00:00:00 2001 From: Sid Shukla Date: Wed, 4 Jun 2025 18:35:15 +0200 Subject: [PATCH 1/5] feat(preflight): Add a check for storage containers Add pre-flight checks to ensure the storage container mentioned in CSI Provider storage class config parameters is present in all relevant AOS clusters i.e. control plane and workers. Co-authored-by: Daniel Lipovetsky --- go.mod | 3 +- go.sum | 6 +- pkg/webhook/preflight/nutanix/checker_test.go | 80 +- pkg/webhook/preflight/nutanix/clients.go | 75 ++ pkg/webhook/preflight/nutanix/clients_test.go | 42 + .../preflight/nutanix/storagecontainer.go | 222 +++ .../nutanix/storagecontainer_test.go | 1198 +++++++++++++++++ 7 files changed, 1593 insertions(+), 33 deletions(-) create mode 100644 pkg/webhook/preflight/nutanix/storagecontainer.go create mode 100644 pkg/webhook/preflight/nutanix/storagecontainer_test.go diff --git a/go.mod b/go.mod index 3bb343975..5e8a41caf 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/google/uuid v1.6.0 github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api v0.0.0-00010101000000-000000000000 github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common v0.7.0 - github.com/nutanix-cloud-native/prism-go-client v0.5.1 + github.com/nutanix-cloud-native/prism-go-client v0.5.2-0.20250602134145-662333927e6d github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4 v4.0.1-beta.2 github.com/nutanix/ntnx-api-golang-clients/networking-go-client/v4 v4.0.2-beta.1 github.com/nutanix/ntnx-api-golang-clients/prism-go-client/v4 v4.0.1-beta.1 @@ -122,7 +122,6 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect - github.com/nutanix/ntnx-api-golang-clients/storage-go-client/v4 v4.0.2-alpha.3 // indirect github.com/nutanix/ntnx-api-golang-clients/volumes-go-client/v4 v4.0.1-beta.1 // indirect github.com/oklog/ulid v1.3.1 // indirect github.com/olekukonko/tablewriter v0.0.5 // indirect diff --git a/go.sum b/go.sum index 5bdafe191..dd19186f1 100644 --- a/go.sum +++ b/go.sum @@ -227,16 +227,14 @@ github.com/morikuni/aec v1.0.0 h1:nP9CBfwrvYnBRgY6qfDQkygYDmYwOilePFkwzv4dU8A= github.com/morikuni/aec v1.0.0/go.mod h1:BbKIizmSmc5MMPqRYbxO4ZU0S0+P200+tUnFx7PXmsc= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= -github.com/nutanix-cloud-native/prism-go-client v0.5.1 h1:ykiXPCILzEMORHz7XvI8KXNomChsdLIpOAlT/YqBCmo= -github.com/nutanix-cloud-native/prism-go-client v0.5.1/go.mod h1:QhLX+sEep0cStzHVYU6mPgIlnA8U3DySskagrbDprRk= +github.com/nutanix-cloud-native/prism-go-client v0.5.2-0.20250602134145-662333927e6d h1:ZjrHbyZLeaWMhtBNCe8uIfvAHs0ebqx8WxJRW59hnMg= +github.com/nutanix-cloud-native/prism-go-client v0.5.2-0.20250602134145-662333927e6d/go.mod h1:N/O9fz5fimjb30RxlPbKbGs/Z2lqMgDqrb6CrsZvQrA= github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4 v4.0.1-beta.2 h1:s1u5/GEw3mTZakepJoTD1OvPVU1YuioRxmKZin+W99s= github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4 v4.0.1-beta.2/go.mod h1:sd4Fnk6MVfEDVY+8WyRoQTmLhi2SgZ3riySWErVHf8E= github.com/nutanix/ntnx-api-golang-clients/networking-go-client/v4 v4.0.2-beta.1 h1:PvZQwYhhJtxmzLpnzEhHTpp2fV6woc6W65PHGsHzVfs= github.com/nutanix/ntnx-api-golang-clients/networking-go-client/v4 v4.0.2-beta.1/go.mod h1:+eZgV1+xL/r84qmuFSVt5R8OFRO70rEz92jOnVgJNco= github.com/nutanix/ntnx-api-golang-clients/prism-go-client/v4 v4.0.1-beta.1 h1:hvy3QCc2SgVidYxTq0rRPOazJOt1PP8A86kW7j6sywU= github.com/nutanix/ntnx-api-golang-clients/prism-go-client/v4 v4.0.1-beta.1/go.mod h1:Yhk+xD4mN90OKEHnk5ARf97CX5p4+MEC/B/YIVoZeZ0= -github.com/nutanix/ntnx-api-golang-clients/storage-go-client/v4 v4.0.2-alpha.3 h1:K3I9YtqKcKKxSL4+tcxnFeLOoaptiVlpsOJ9Xzq3shM= -github.com/nutanix/ntnx-api-golang-clients/storage-go-client/v4 v4.0.2-alpha.3/go.mod h1:kz3gO87xtWnPOCP2kN7yw5LvCDVRnvg8BOWL7CarqXA= github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4 v4.0.1-beta.1 h1:XuTRvYu1kiNjdXOYVwyjhKlFWyo9nMit6GsOYV8+5Cg= github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4 v4.0.1-beta.1/go.mod h1:CaWm4GFpAjQQDc6YXl/dUDrHpuW54h8j6Cj7EslE4Qk= github.com/nutanix/ntnx-api-golang-clients/volumes-go-client/v4 v4.0.1-beta.1 h1:VJSaQDnnYeNEk1mkQqEbt573OdM62+5s/B0e9kszdas= diff --git a/pkg/webhook/preflight/nutanix/checker_test.go b/pkg/webhook/preflight/nutanix/checker_test.go index 0e50a6d3a..7191cf17c 100644 --- a/pkg/webhook/preflight/nutanix/checker_test.go +++ b/pkg/webhook/preflight/nutanix/checker_test.go @@ -33,22 +33,24 @@ func (m *mockCheck) Run(ctx context.Context) preflight.CheckResult { func TestNutanixChecker_Init(t *testing.T) { tests := []struct { - name string - nutanixConfig *carenv1.NutanixClusterConfigSpec - workerNodeConfigs map[string]*carenv1.NutanixWorkerNodeConfigSpec - expectedCheckCount int - expectedFirstCheckName string - expectedSecondCheckName string - vmImageCheckCount int + name string + nutanixConfig *carenv1.NutanixClusterConfigSpec + workerNodeConfigs map[string]*carenv1.NutanixWorkerNodeConfigSpec + expectedCheckCount int + expectedFirstCheckName string + expectedSecondCheckName string + vmImageCheckCount int + storageContainerCheckCount int }{ { - name: "basic initialization with no configs", - nutanixConfig: nil, - workerNodeConfigs: nil, - expectedCheckCount: 2, // config check and credentials check - expectedFirstCheckName: "NutanixConfiguration", - expectedSecondCheckName: "NutanixCredentials", - vmImageCheckCount: 0, + name: "basic initialization with no configs", + nutanixConfig: nil, + workerNodeConfigs: nil, + expectedCheckCount: 2, // config check and credentials check + expectedFirstCheckName: "NutanixConfiguration", + expectedSecondCheckName: "NutanixCredentials", + vmImageCheckCount: 0, + storageContainerCheckCount: 0, }, { name: "initialization with control plane config", @@ -57,11 +59,12 @@ func TestNutanixChecker_Init(t *testing.T) { Nutanix: &carenv1.NutanixNodeSpec{}, }, }, - workerNodeConfigs: nil, - expectedCheckCount: 3, // config check, credentials check, 1 VM image check - expectedFirstCheckName: "NutanixConfiguration", - expectedSecondCheckName: "NutanixCredentials", - vmImageCheckCount: 1, + workerNodeConfigs: nil, + expectedCheckCount: 4, // config check, credentials check, 1 VM image check, 1 storage container check + expectedFirstCheckName: "NutanixConfiguration", + expectedSecondCheckName: "NutanixCredentials", + vmImageCheckCount: 1, + storageContainerCheckCount: 1, }, { name: "initialization with worker node configs", @@ -74,10 +77,11 @@ func TestNutanixChecker_Init(t *testing.T) { Nutanix: &carenv1.NutanixNodeSpec{}, }, }, - expectedCheckCount: 4, // config check, credentials check, 2 VM image checks - expectedFirstCheckName: "NutanixConfiguration", - expectedSecondCheckName: "NutanixCredentials", - vmImageCheckCount: 2, + expectedCheckCount: 6, // config check, credentials check, 2 VM image checks, 2 storage container checks + expectedFirstCheckName: "NutanixConfiguration", + expectedSecondCheckName: "NutanixCredentials", + vmImageCheckCount: 2, + storageContainerCheckCount: 2, }, { name: "initialization with both control plane and worker node configs", @@ -91,10 +95,12 @@ func TestNutanixChecker_Init(t *testing.T) { Nutanix: &carenv1.NutanixNodeSpec{}, }, }, - expectedCheckCount: 4, // config check, credentials check, 2 VM image checks (1 CP + 1 worker) - expectedFirstCheckName: "NutanixConfiguration", - expectedSecondCheckName: "NutanixCredentials", - vmImageCheckCount: 2, + // config check, credentials check, 2 VM image checks (1 CP + 1 worker), 2 storage container checks (1 CP + 1 worker) + expectedCheckCount: 6, + expectedFirstCheckName: "NutanixConfiguration", + expectedSecondCheckName: "NutanixCredentials", + vmImageCheckCount: 2, + storageContainerCheckCount: 2, }, } @@ -107,6 +113,7 @@ func TestNutanixChecker_Init(t *testing.T) { configCheckCalled := false credsCheckCalled := false vmImageCheckCount := 0 + storageContainerCheckCount := 0 checker.configurationCheckFactory = func(cd *checkDependencies) preflight.Check { configCheckCalled = true @@ -144,6 +151,19 @@ func TestNutanixChecker_Init(t *testing.T) { return checks } + checker.initStorageContainerChecksFunc = func(n *nutanixChecker) []preflight.Check { + checks := []preflight.Check{} + for i := 0; i < tt.storageContainerCheckCount; i++ { + storageContainerCheckCount++ + checks = append(checks, func(ctx context.Context) preflight.CheckResult { + return preflight.CheckResult{ + Name: fmt.Sprintf("StorageContainerCheck-%d", i), + } + }) + } + return checks + } + // Call Init ctx := context.Background() checks := checker.Init(ctx, nil, &clusterv1.Cluster{ @@ -160,6 +180,12 @@ func TestNutanixChecker_Init(t *testing.T) { assert.True(t, configCheckCalled, "initNutanixConfiguration should have been called") assert.True(t, credsCheckCalled, "initCredentialsCheck should have been called") assert.Equal(t, tt.vmImageCheckCount, vmImageCheckCount, "Wrong number of VM image checks") + assert.Equal( + t, + tt.storageContainerCheckCount, + storageContainerCheckCount, + "Wrong number of storage container checks", + ) // Verify the first two checks when we have results if len(checks) >= 2 { diff --git a/pkg/webhook/preflight/nutanix/clients.go b/pkg/webhook/preflight/nutanix/clients.go index c80bc111e..167356259 100644 --- a/pkg/webhook/preflight/nutanix/clients.go +++ b/pkg/webhook/preflight/nutanix/clients.go @@ -7,6 +7,7 @@ import ( "context" "fmt" + clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" vmmv4 "github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4/models/vmm/v4/content" prismgoclient "github.com/nutanix-cloud-native/prism-go-client" @@ -29,6 +30,24 @@ type client interface { *vmmv4.ListImagesApiResponse, error, ) + GetClusterById(id *string) (*clustermgmtv4.GetClusterApiResponse, error) + ListClusters( + page_ *int, + limit_ *int, + filter_ *string, + orderby_ *string, + apply_ *string, + select_ *string, + args ...map[string]interface{}, + ) (*clustermgmtv4.ListClustersApiResponse, error) + ListStorageContainers( + page_ *int, + limit_ *int, + filter_ *string, + orderby_ *string, + select_ *string, + args ...map[string]interface{}, + ) (*clustermgmtv4.ListStorageContainersApiResponse, error) } // clientWrapper implements the client interface and wraps both v3 and v4 clients. @@ -90,3 +109,59 @@ func (c *clientWrapper) ListImages(page_ *int, } return resp, nil } + +func (c *clientWrapper) GetClusterById(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + resp, err := c.v4client.ClustersApiInstance.GetClusterById(id) + if err != nil { + return nil, err + } + return resp, nil +} + +func (c *clientWrapper) ListClusters( + page_ *int, + limit_ *int, + filter_ *string, + orderby_ *string, + apply_ *string, + select_ *string, + args ...map[string]interface{}, +) (*clustermgmtv4.ListClustersApiResponse, error) { + resp, err := c.v4client.ClustersApiInstance.ListClusters( + page_, + limit_, + filter_, + orderby_, + apply_, + select_, + args..., + ) + if err != nil { + return nil, err + } + return resp, nil +} + +func (c *clientWrapper) ListStorageContainers( + page_ *int, + limit_ *int, + filter_ *string, + orderby_ *string, + select_ *string, + args ...map[string]interface{}, +) (*clustermgmtv4.ListStorageContainersApiResponse, error) { + resp, err := c.v4client.StorageContainerAPI.ListStorageContainers( + page_, + limit_, + filter_, + orderby_, + select_, + args..., + ) + if err != nil { + return nil, err + } + return resp, nil +} + + diff --git a/pkg/webhook/preflight/nutanix/clients_test.go b/pkg/webhook/preflight/nutanix/clients_test.go index acf66f5f6..52a78e4af 100644 --- a/pkg/webhook/preflight/nutanix/clients_test.go +++ b/pkg/webhook/preflight/nutanix/clients_test.go @@ -6,6 +6,7 @@ package nutanix import ( "context" + clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" vmmv4 "github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4/models/vmm/v4/content" prismv3 "github.com/nutanix-cloud-native/prism-go-client/v3" @@ -35,6 +36,27 @@ type mocknclient struct { *vmmv4.ListImagesApiResponse, error, ) + + getClusterByIdFunc func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) + + listClustersFunc func( + page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) (*clustermgmtv4.ListClustersApiResponse, error) + + listStorageContainersFunc func( + page, + limit *int, + filter, + orderby, + select_ *string, + args ...map[string]interface{}, + ) (*clustermgmtv4.ListStorageContainersApiResponse, error) } func (m *mocknclient) GetCurrentLoggedInUser(ctx context.Context) (*prismv3.UserIntentResponse, error) { @@ -52,3 +74,23 @@ func (m *mocknclient) ListImages( ) (*vmmv4.ListImagesApiResponse, error) { return m.listImagesFunc(page, limit, filter, orderby, select_) } + +func (m *mocknclient) GetClusterById(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + return m.getClusterByIdFunc(id) +} + +func (m *mocknclient) ListClusters( + page, limit *int, + filter, orderby, apply, select_ *string, + args ...map[string]interface{}, +) (*clustermgmtv4.ListClustersApiResponse, error) { + return m.listClustersFunc(page, limit, filter, orderby, apply, select_, args...) +} + +func (m *mocknclient) ListStorageContainers( + page, limit *int, + filter, orderby, select_ *string, + args ...map[string]interface{}, +) (*clustermgmtv4.ListStorageContainersApiResponse, error) { + return m.listStorageContainersFunc(page, limit, filter, orderby, select_, args...) +} diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go new file mode 100644 index 000000000..e5effd5ce --- /dev/null +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -0,0 +1,222 @@ +// Copyright 2025 Nutanix. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nutanix + +import ( + "context" + "fmt" + + 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" + "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/webhook/preflight" +) + +func initStorageContainerChecks(n *nutanixChecker) []preflight.Check { + checks := []preflight.Check{} + + // If there is no CSI configuration, there is no need to check for storage containers. + if n.nutanixClusterConfigSpec == nil || + n.nutanixClusterConfigSpec.Addons == nil || + n.nutanixClusterConfigSpec.Addons.CSI == nil { + return checks + } + + if n.nutanixClusterConfigSpec != nil && n.nutanixClusterConfigSpec.ControlPlane != nil && + n.nutanixClusterConfigSpec.ControlPlane.Nutanix != nil { + checks = append(checks, + n.storageContainerCheckFunc( + n, + n.nutanixClusterConfigSpec.ControlPlane.Nutanix, + "cluster.spec.topology[.name=clusterConfig].value.controlPlane.nutanix", + &n.nutanixClusterConfigSpec.Addons.CSI.Providers.NutanixCSI, + ), + ) + } + + for mdName, nutanixWorkerNodeConfigSpec := range n.nutanixWorkerNodeConfigSpecByMachineDeploymentName { + if nutanixWorkerNodeConfigSpec.Nutanix != nil { + checks = append(checks, + n.storageContainerCheckFunc( + n, + nutanixWorkerNodeConfigSpec.Nutanix, + fmt.Sprintf( + "cluster.spec.topology.workers.machineDeployments[.name=%s]"+ + ".variables[.name=workerConfig].value.nutanix", + mdName, + ), + &n.nutanixClusterConfigSpec.Addons.CSI.Providers.NutanixCSI, + ), + ) + } + } + + return checks +} + +// storageContainerCheck checks if the storage container specified in the CSIProvider's StorageClassConfigs exists. +// It admits the NodeSpec instead of the MachineDetails because the failure domains will be specified in the NodeSpec +// and the MachineDetails.Cluster will be nil in that case. +func storageContainerCheck( + n *nutanixChecker, + nodeSpec *carenv1.NutanixNodeSpec, + field string, + csiSpec *carenv1.CSIProvider, +) preflight.Check { + const ( + csiParameterKeyStorageContainer = "storageContainer" + ) + + return func(ctx context.Context) preflight.CheckResult { + result := preflight.CheckResult{ + Name: "NutanixStorageContainer", + Allowed: true, + } + if csiSpec == nil { + result.Allowed = false + result.Error = true + result.Causes = append(result.Causes, preflight.Cause{ + Message: fmt.Sprintf( + "no storage container found for cluster %q", + *nodeSpec.MachineDetails.Cluster.Name, + ), + Field: field, + }) + + return result + } + + if csiSpec.StorageClassConfigs == nil { + result.Allowed = false + result.Causes = append(result.Causes, preflight.Cause{ + Message: fmt.Sprintf( + "no storage class configs found for cluster %q", + *nodeSpec.MachineDetails.Cluster.Name, + ), + Field: field, + }) + + return result + } + + for _, storageClassConfig := range csiSpec.StorageClassConfigs { + if storageClassConfig.Parameters == nil { + continue + } + + storageContainer, ok := storageClassConfig.Parameters[csiParameterKeyStorageContainer] + if !ok { + continue + } + + // TODO: check if cluster name is set, if not use uuid. + // If neither is set, use the cluster name from the NodeSpec failure domain. + if _, err := getStorageContainer(n.v4client, nodeSpec, storageContainer); 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", + storageContainer, + err, + ), + Field: field, + }) + + return result + } + } + + return result + } +} + +func getStorageContainer( + client v4client, + nodeSpec *carenv1.NutanixNodeSpec, + 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) + resp, err := client.ListStorageContainers(nil, nil, &fltr, nil, nil) + if err != nil { + 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) + } + + 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 +} + +func getCluster( + client v4client, + clusterIdentifier *v1beta1.NutanixResourceIdentifier, +) (*clustermgmtv4.Cluster, error) { + switch { + case clusterIdentifier.IsUUID(): + resp, err := client.GetClusterById(clusterIdentifier.UUID) + if err != nil { + return nil, err + } + + cluster, ok := resp.GetData().(clustermgmtv4.Cluster) + if !ok { + return nil, fmt.Errorf("failed to get data returned by GetClusterById") + } + + return &cluster, nil + case clusterIdentifier.IsName(): + filter := fmt.Sprintf("name eq '%s'", *clusterIdentifier.Name) + resp, err := client.ListClusters(nil, nil, &filter, nil, nil, nil) + if err != nil { + return nil, err + } + + if resp == nil || resp.GetData() == nil { + return nil, fmt.Errorf("no clusters were returned") + } + + clusters, ok := resp.GetData().([]clustermgmtv4.Cluster) + if !ok { + return nil, fmt.Errorf("failed to get data returned by ListClusters") + } + + if len(clusters) == 0 { + return nil, fmt.Errorf("no clusters found with name %q", *clusterIdentifier.Name) + } + + if len(clusters) > 1 { + return nil, fmt.Errorf("multiple clusters found with name %q", *clusterIdentifier.Name) + } + + return &clusters[0], nil + default: + return nil, fmt.Errorf("cluster identifier is missing both name and uuid") + } +} diff --git a/pkg/webhook/preflight/nutanix/storagecontainer_test.go b/pkg/webhook/preflight/nutanix/storagecontainer_test.go new file mode 100644 index 000000000..efce79480 --- /dev/null +++ b/pkg/webhook/preflight/nutanix/storagecontainer_test.go @@ -0,0 +1,1198 @@ +// Copyright 2025 Nutanix. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nutanix + +import ( + "context" + "fmt" + "testing" + + "github.com/go-logr/logr/testr" + clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/utils/ptr" + + capxv1 "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" + "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/webhook/preflight" +) + +func TestInitStorageContainerChecks(t *testing.T) { + testCases := []struct { + name string + nutanixClusterConfigSpec *carenv1.NutanixClusterConfigSpec + workerNodeConfigSpecByMDName map[string]*carenv1.NutanixWorkerNodeConfigSpec + expectedChecksCount int + }{ + { + name: "nil cluster config", + nutanixClusterConfigSpec: nil, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, + expectedChecksCount: 0, + }, + { + name: "cluster config without addons", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + ControlPlane: &carenv1.NutanixControlPlaneSpec{ + Nutanix: &carenv1.NutanixNodeSpec{}, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, + expectedChecksCount: 0, + }, + { + name: "cluster config with addons but no CSI", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + ControlPlane: &carenv1.NutanixControlPlaneSpec{ + Nutanix: &carenv1.NutanixNodeSpec{}, + }, + Addons: &carenv1.NutanixAddons{}, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, + expectedChecksCount: 0, + }, + { + name: "cluster config with CSI but no control plane or worker nodes", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + Addons: &carenv1.NutanixAddons{ + CSI: &carenv1.NutanixCSI{ + Providers: carenv1.NutanixCSIProviders{ + NutanixCSI: carenv1.CSIProvider{}, + }, + }, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, + expectedChecksCount: 0, + }, + { + name: "cluster config with CSI and control plane", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + ControlPlane: &carenv1.NutanixControlPlaneSpec{ + Nutanix: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("my-cluster"), + }, + }, + }, + }, + Addons: &carenv1.NutanixAddons{ + CSI: &carenv1.NutanixCSI{ + Providers: carenv1.NutanixCSIProviders{ + NutanixCSI: carenv1.CSIProvider{ + StorageClassConfigs: map[string]carenv1.StorageClassConfig{ + "test-sc": { + Parameters: map[string]string{ + "storageContainer": "test-container", + }, + }, + }, + }, + }, + }, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, + expectedChecksCount: 1, + }, + { + name: "cluster config with CSI and worker nodes", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + Addons: &carenv1.NutanixAddons{ + CSI: &carenv1.NutanixCSI{ + Providers: carenv1.NutanixCSIProviders{ + NutanixCSI: carenv1.CSIProvider{}, + }, + }, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{ + "worker-1": { + Nutanix: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("worker-cluster"), + }, + }, + }, + }, + }, + expectedChecksCount: 1, + }, + { + name: "cluster config with CSI, control plane and worker nodes", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + ControlPlane: &carenv1.NutanixControlPlaneSpec{ + Nutanix: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("cp-cluster"), + }, + }, + }, + }, + Addons: &carenv1.NutanixAddons{ + CSI: &carenv1.NutanixCSI{ + Providers: carenv1.NutanixCSIProviders{ + NutanixCSI: carenv1.CSIProvider{}, + }, + }, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{ + "worker-1": { + Nutanix: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("worker1-cluster"), + }, + }, + }, + }, + "worker-2": { + Nutanix: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("worker2-cluster"), + }, + }, + }, + }, + }, + expectedChecksCount: 3, // 1 for control plane, 2 for workers + }, + { + name: "cluster config with CSI and null control plane nutanix", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + ControlPlane: &carenv1.NutanixControlPlaneSpec{ + Nutanix: nil, // explicitly null + }, + Addons: &carenv1.NutanixAddons{ + CSI: &carenv1.NutanixCSI{ + Providers: carenv1.NutanixCSIProviders{ + NutanixCSI: carenv1.CSIProvider{}, + }, + }, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, + expectedChecksCount: 0, + }, + { + name: "cluster config with CSI and some nutanix nil workers", + nutanixClusterConfigSpec: &carenv1.NutanixClusterConfigSpec{ + Addons: &carenv1.NutanixAddons{ + CSI: &carenv1.NutanixCSI{ + Providers: carenv1.NutanixCSIProviders{ + NutanixCSI: carenv1.CSIProvider{}, + }, + }, + }, + }, + workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{ + "worker-1": { + Nutanix: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("worker1-cluster"), + }, + }, + }, + }, + "worker-2": { + Nutanix: nil, + }, + }, + expectedChecksCount: 1, // only for the defined worker-1 + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + logger := testr.New(t) + + // Create checker + checker := &nutanixChecker{ + log: logger, + nutanixClusterConfigSpec: tc.nutanixClusterConfigSpec, + nutanixWorkerNodeConfigSpecByMachineDeploymentName: tc.workerNodeConfigSpecByMDName, + initStorageContainerChecksFunc: initStorageContainerChecks, + } + + // Set up a tracing function to replace storageContainerCheck + var capturedFields []string + checker.storageContainerCheckFunc = func( + n *nutanixChecker, + nodeSpec *carenv1.NutanixNodeSpec, + field string, + csiSpec *carenv1.CSIProvider, + ) preflight.Check { + capturedFields = append(capturedFields, field) + return func(ctx context.Context) preflight.CheckResult { + return preflight.CheckResult{Name: "StorageContainerCheck:" + field} + } + } + + // Call the function under test + checks := checker.initStorageContainerChecksFunc(checker) + + // Verify number of checks + assert.Len(t, checks, tc.expectedChecksCount, "Wrong number of checks created") + + // Verify number of captured fields matches number of checks + assert.Len(t, capturedFields, tc.expectedChecksCount, "Wrong number of fields captured") + + // Additional verification that each field is correctly formatted + for _, field := range capturedFields { + if tc.nutanixClusterConfigSpec != nil && tc.nutanixClusterConfigSpec.ControlPlane != nil && + tc.nutanixClusterConfigSpec.ControlPlane.Nutanix != nil { + expectedCPField := "cluster.spec.topology[.name=clusterConfig].value.controlPlane.nutanix" + if field == expectedCPField { + continue + } + } + // If not CP field, should be worker field with MD name + assert.Contains(t, field, "cluster.spec.topology.workers.machineDeployments[.name=") + assert.Contains(t, field, "].variables[.name=workerConfig].value.nutanix") + } + }) + } +} + +func TestStorageContainerCheck(t *testing.T) { + clusterName := "test-cluster" + fieldPath := "test.field.path" + + testCases := []struct { + name string + nodeSpec *carenv1.NutanixNodeSpec + csiSpec *carenv1.CSIProvider + mockv4client *mockv4client + expectedResult preflight.CheckResult + expectedAllowed bool + expectedError bool + expectedCauseMessage string + }{ + { + name: "nil CSI spec", + nodeSpec: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To(clusterName), + }, + }, + }, + csiSpec: nil, + mockv4client: nil, + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: fmt.Sprintf("no storage container found for cluster %q", clusterName), + }, + { + name: "nil storage class configs", + nodeSpec: &carenv1.NutanixNodeSpec{ + MachineDetails: carenv1.NutanixMachineDetails{ + Cluster: capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To(clusterName), + }, + }, + }, + csiSpec: &carenv1.CSIProvider{StorageClassConfigs: nil}, + mockv4client: nil, + expectedAllowed: false, + expectedError: false, + expectedCauseMessage: fmt.Sprintf("no storage class configs found for cluster %q", clusterName), + }, + { + name: "storage class config without parameters", + 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: nil, + }, + }, + }, + mockv4client: nil, + expectedAllowed: true, + expectedError: false, + }, + { + name: "storage class config without storage container parameter", + 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{ + "otherParam": "value", + }, + }, + }, + }, + mockv4client: nil, + expectedAllowed: true, + expectedError: false, + }, + { + name: "storage container not found", + 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": "missing-container", + }, + }, + }, + }, + mockv4client: &mockv4client{ + 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, + ) { + resp := &clustermgmtv4.ListStorageContainersApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.ListStorageContainersApiResponse"), + } + err := resp.SetData([]clustermgmtv4.StorageContainer{}) // Empty list - container not found + require.NoError(t, err) + 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", + }, + { + name: "multiple storage containers found", + 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": "duplicate-container", + }, + }, + }, + }, + mockv4client: &mockv4client{ + 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, + ) { + resp := &clustermgmtv4.ListStorageContainersApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.ListStorageContainersApiResponse"), + } + err := resp.SetData([]clustermgmtv4.StorageContainer{ + { + Name: ptr.To("duplicate-container"), + }, + { + Name: ptr.To("duplicate-container"), + }, + }) + require.NoError(t, err) + return resp, nil + }, + }, + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container named \"duplicate-container\" exists:" + + " multiple storage containers found with name", + }, + { + name: "successful storage container check", + 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", + }, + }, + }, + }, + mockv4client: &mockv4client{ + 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, + ) { + resp := &clustermgmtv4.ListStorageContainersApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.ListStorageContainersApiResponse"), + } + err := resp.SetData([]clustermgmtv4.StorageContainer{ + { + Name: ptr.To("valid-container"), + }, + }) + require.NoError(t, err) + return resp, nil + }, + }, + expectedAllowed: true, + expectedError: false, + }, + { + name: "error getting cluster", + 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", + }, + }, + }, + }, + mockv4client: &mockv4client{ + getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + return nil, fmt.Errorf("API error") + }, + listClustersFunc: func( + page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + return nil, fmt.Errorf("API error") + }, + }, + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" + + " failed to get cluster: API error", + }, + { + name: "error listing storage containers", + 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", + }, + }, + }, + }, + mockv4client: &mockv4client{ + 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 nil, fmt.Errorf("API error listing containers") + }, + }, + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" + + " failed to list storage containers: API error listing containers", + }, + { + name: "invalid 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", + }, + }, + }, + }, + mockv4client: &mockv4client{ + 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{ + ObjectType_: ptr.To("wrong-data-type"), + }, nil + }, + }, + expectedAllowed: false, + expectedError: true, + expectedCauseMessage: "failed to check if storage container named \"valid-container\" exists:" + + " failed to get data returned by ListStorageContainers", + }, + { + name: "multiple storage class configs with success", + 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-1": { + Parameters: map[string]string{ + "otherParam": "value", + }, + }, + "test-sc-2": { + Parameters: map[string]string{ + "storageContainer": "valid-container", + }, + }, + "test-sc-3": { + Parameters: map[string]string{ + "storageContainer": "another-valid-container", + }, + }, + }, + }, + mockv4client: &mockv4client{ + 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, + ) { + require.NotNil(t, filter) + // Extract name from filter + containerName := "" + switch *filter { + case "name eq 'valid-container' and clusterExtId eq 'cluster-uuid-123'": + containerName = "valid-container" + case "name eq 'another-valid-container' and clusterExtId eq 'cluster-uuid-123'": + containerName = "another-valid-container" + default: + return nil, fmt.Errorf("filter %q does not match any storage container", *filter) + } + + resp := &clustermgmtv4.ListStorageContainersApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.ListStorageContainersApiResponse"), + } + err := resp.SetData([]clustermgmtv4.StorageContainer{ + { + Name: ptr.To(containerName), + }, + }) + require.NoError(t, err) + return resp, nil + }, + }, + expectedAllowed: true, + expectedError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + logger := testr.New(t) + + // Create checker with mock v4 client + checker := &nutanixChecker{ + log: logger, + v4client: tc.mockv4client, + } + + // Create the check function + checkFunc := storageContainerCheck(checker, tc.nodeSpec, fieldPath, tc.csiSpec) + + // Run the check + ctx := context.Background() + result := checkFunc(ctx) + + // Verify the result + assert.Equal(t, tc.expectedAllowed, result.Allowed) + assert.Equal(t, tc.expectedError, result.Error) + + if tc.expectedCauseMessage != "" { + require.NotEmpty(t, result.Causes) + assert.Contains(t, result.Causes[0].Message, tc.expectedCauseMessage) + assert.Equal(t, fieldPath, result.Causes[0].Field) + } else { + assert.Empty(t, result.Causes) + } + }) + } +} + +func TestGetCluster(t *testing.T) { + testCases := []struct { + name string + clusterIdentifier *capxv1.NutanixResourceIdentifier + mockv4client *mockv4client + expectError bool + errorContains string + expectedClusterID string + }{ + { + name: "get cluster by UUID - success", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierUUID, + UUID: ptr.To("test-uuid-123"), + }, + mockv4client: &mockv4client{ + getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + assert.Equal(t, "test-uuid-123", *id) + resp := &clustermgmtv4.GetClusterApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.GetClusterApiResponse"), + } + err := resp.SetData( + clustermgmtv4.Cluster{ + ObjectType_: ptr.To("clustermgmt.v4.config.Cluster"), + ExtId: ptr.To("test-uuid-123"), + Name: ptr.To("test-cluster"), + }, + ) + require.NoError(t, err) + return resp, nil + }, + }, + expectError: false, + expectedClusterID: "test-uuid-123", + }, + { + name: "get cluster by UUID - API error", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierUUID, + UUID: ptr.To("test-uuid-error"), + }, + mockv4client: &mockv4client{ + getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + return nil, fmt.Errorf("API error") + }, + }, + expectError: true, + errorContains: "API error", + }, + { + name: "get cluster by UUID - invalid response data", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierUUID, + UUID: ptr.To("test-uuid-invalid"), + }, + mockv4client: &mockv4client{ + getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + // Return an invalid data type + resp := &clustermgmtv4.GetClusterApiResponse{ + ObjectType_: ptr.To("wrong-data-type"), + } + return resp, nil + }, + }, + expectError: true, + errorContains: "failed to get data returned by GetClusterById", + }, + { + name: "get cluster by name - success", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster"), + }, + mockv4client: &mockv4client{ + listClustersFunc: func(page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + assert.NotNil(t, filter) + assert.Equal(t, "name eq 'test-cluster'", *filter) + resp := &clustermgmtv4.ListClustersApiResponse{ + ObjectType_: ptr.To("clustermgmt.v4.config.ListClustersApiResponse"), + } + err := resp.SetData([]clustermgmtv4.Cluster{ + { + ExtId: ptr.To("test-uuid-123"), + Name: ptr.To("test-cluster"), + }, + }) + require.NoError(t, err) + return resp, nil + }, + }, + expectError: false, + expectedClusterID: "test-uuid-123", + }, + { + name: "get cluster by name - API error", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster-error"), + }, + mockv4client: &mockv4client{ + listClustersFunc: func(page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + return nil, fmt.Errorf("API error") + }, + }, + expectError: true, + errorContains: "API error", + }, + { + name: "get cluster by name - nil response", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster-nil"), + }, + mockv4client: &mockv4client{ + listClustersFunc: func(page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + return nil, nil + }, + }, + expectError: true, + errorContains: "no clusters were returned", + }, + { + name: "get cluster by name - nil data", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster-nil-data"), + }, + mockv4client: &mockv4client{ + listClustersFunc: func(page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + return &clustermgmtv4.ListClustersApiResponse{ + Data: nil, + }, nil + }, + }, + expectError: true, + errorContains: "no clusters were returned", + }, + { + name: "get cluster by name - no clusters found", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster-not-found"), + }, + mockv4client: &mockv4client{ + 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{}) + require.NoError(t, err) + return resp, nil + }, + }, + expectError: true, + errorContains: "no clusters found with name", + }, + { + name: "get cluster by name - multiple clusters found", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: ptr.To("test-cluster-duplicate"), + }, + mockv4client: &mockv4client{ + 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{ + { + ExtId: ptr.To("test-uuid-1"), + Name: ptr.To("test-cluster-duplicate"), + }, + { + ExtId: ptr.To("test-uuid-2"), + Name: ptr.To("test-cluster-duplicate"), + }, + }) + require.NoError(t, err) + return resp, nil + }, + }, + expectError: true, + errorContains: "multiple clusters found with name", + }, + { + name: "invalid identifier type", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: "invalid", + }, + mockv4client: &mockv4client{}, + expectError: true, + errorContains: "cluster identifier is missing both name and uuid", + }, + { + name: "nil UUID for UUID type", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierUUID, + UUID: nil, + }, + mockv4client: &mockv4client{ + getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { + return nil, fmt.Errorf("should not be called") + }, + }, + expectError: true, + errorContains: "cluster identifier is missing both name and uuid", + }, + { + name: "nil name for Name type", + clusterIdentifier: &capxv1.NutanixResourceIdentifier{ + Type: capxv1.NutanixIdentifierName, + Name: nil, + }, + mockv4client: &mockv4client{ + listClustersFunc: func(page, + limit *int, + filter, + orderby, + apply, + select_ *string, + args ...map[string]interface{}, + ) ( + *clustermgmtv4.ListClustersApiResponse, + error, + ) { + return nil, fmt.Errorf("should not be called") + }, + }, + expectError: true, + errorContains: "cluster identifier is missing both name and uuid", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cluster, err := getCluster(tc.mockv4client, tc.clusterIdentifier) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorContains) + assert.Nil(t, cluster) + } else { + require.NoError(t, err) + require.NotNil(t, cluster) + assert.Equal(t, tc.expectedClusterID, *cluster.ExtId) + } + }) + } +} From 20ac8e98a377514629f23449c2ac674ff5368923 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Fri, 13 Jun 2025 15:24:54 -0700 Subject: [PATCH 2/5] fixup! feat(preflight): Add a check for storage containers Update for latest changes in base branch --- pkg/webhook/preflight/nutanix/checker.go | 12 +- pkg/webhook/preflight/nutanix/checker_test.go | 15 +- pkg/webhook/preflight/nutanix/clients.go | 2 - .../preflight/nutanix/storagecontainer.go | 189 +++++++++--------- .../nutanix/storagecontainer_test.go | 116 ++++------- 5 files changed, 151 insertions(+), 183 deletions(-) diff --git a/pkg/webhook/preflight/nutanix/checker.go b/pkg/webhook/preflight/nutanix/checker.go index dc61ec445..4eb39f963 100644 --- a/pkg/webhook/preflight/nutanix/checker.go +++ b/pkg/webhook/preflight/nutanix/checker.go @@ -18,9 +18,10 @@ import ( ) var Checker = &nutanixChecker{ - configurationCheckFactory: newConfigurationCheck, - credentialsCheckFactory: newCredentialsCheck, - vmImageChecksFactory: newVMImageChecks, + configurationCheckFactory: newConfigurationCheck, + credentialsCheckFactory: newCredentialsCheck, + vmImageChecksFactory: newVMImageChecks, + storageContainerChecksFactory: newStorageContainerChecks, } type nutanixChecker struct { @@ -37,6 +38,10 @@ type nutanixChecker struct { vmImageChecksFactory func( cd *checkDependencies, ) []preflight.Check + + storageContainerChecksFactory func( + cd *checkDependencies, + ) []preflight.Check } type checkDependencies struct { @@ -69,6 +74,7 @@ func (n *nutanixChecker) Init( } checks = append(checks, n.vmImageChecksFactory(cd)...) + checks = append(checks, n.storageContainerChecksFactory(cd)...) // Add more checks here as needed. diff --git a/pkg/webhook/preflight/nutanix/checker_test.go b/pkg/webhook/preflight/nutanix/checker_test.go index 7191cf17c..5c75af66c 100644 --- a/pkg/webhook/preflight/nutanix/checker_test.go +++ b/pkg/webhook/preflight/nutanix/checker_test.go @@ -151,15 +151,18 @@ func TestNutanixChecker_Init(t *testing.T) { return checks } - checker.initStorageContainerChecksFunc = func(n *nutanixChecker) []preflight.Check { + checker.storageContainerChecksFactory = func(cd *checkDependencies) []preflight.Check { checks := []preflight.Check{} for i := 0; i < tt.storageContainerCheckCount; i++ { storageContainerCheckCount++ - checks = append(checks, func(ctx context.Context) preflight.CheckResult { - return preflight.CheckResult{ - Name: fmt.Sprintf("StorageContainerCheck-%d", i), - } - }) + checks = append(checks, + &mockCheck{ + name: fmt.Sprintf("NutanixStorageContainer-%d", i), + result: preflight.CheckResult{ + Allowed: true, + }, + }, + ) } return checks } diff --git a/pkg/webhook/preflight/nutanix/clients.go b/pkg/webhook/preflight/nutanix/clients.go index 167356259..231e83440 100644 --- a/pkg/webhook/preflight/nutanix/clients.go +++ b/pkg/webhook/preflight/nutanix/clients.go @@ -163,5 +163,3 @@ func (c *clientWrapper) ListStorageContainers( } return resp, nil } - - diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go index e5effd5ce..b333dbde0 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -15,127 +15,128 @@ import ( "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/webhook/preflight" ) -func initStorageContainerChecks(n *nutanixChecker) []preflight.Check { - checks := []preflight.Check{} +const ( + csiParameterKeyStorageContainer = "storageContainer" +) - // If there is no CSI configuration, there is no need to check for storage containers. - if n.nutanixClusterConfigSpec == nil || - n.nutanixClusterConfigSpec.Addons == nil || - n.nutanixClusterConfigSpec.Addons.CSI == nil { - return checks +type storageContainerCheck struct { + nodeSpec *carenv1.NutanixNodeSpec + field string + csiSpec *carenv1.CSIProvider + nclient client +} + +func (c *storageContainerCheck) Name() string { + return "NutanixStorageContainer" +} + +func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult { + result := preflight.CheckResult{ + Allowed: true, } - if n.nutanixClusterConfigSpec != nil && n.nutanixClusterConfigSpec.ControlPlane != nil && - n.nutanixClusterConfigSpec.ControlPlane.Nutanix != nil { - checks = append(checks, - n.storageContainerCheckFunc( - n, - n.nutanixClusterConfigSpec.ControlPlane.Nutanix, - "cluster.spec.topology[.name=clusterConfig].value.controlPlane.nutanix", - &n.nutanixClusterConfigSpec.Addons.CSI.Providers.NutanixCSI, + if c.csiSpec == nil { + result.Allowed = false + result.Error = true + result.Causes = append(result.Causes, preflight.Cause{ + Message: fmt.Sprintf( + "no storage container found for cluster %q", + *c.nodeSpec.MachineDetails.Cluster.Name, ), - ) + Field: c.field, + }) + + return result } - for mdName, nutanixWorkerNodeConfigSpec := range n.nutanixWorkerNodeConfigSpecByMachineDeploymentName { - if nutanixWorkerNodeConfigSpec.Nutanix != nil { - checks = append(checks, - n.storageContainerCheckFunc( - n, - nutanixWorkerNodeConfigSpec.Nutanix, - fmt.Sprintf( - "cluster.spec.topology.workers.machineDeployments[.name=%s]"+ - ".variables[.name=workerConfig].value.nutanix", - mdName, - ), - &n.nutanixClusterConfigSpec.Addons.CSI.Providers.NutanixCSI, - ), - ) - } + if c.csiSpec.StorageClassConfigs == nil { + result.Allowed = false + result.Causes = append(result.Causes, preflight.Cause{ + Message: fmt.Sprintf( + "no storage class configs found for cluster %q", + *c.nodeSpec.MachineDetails.Cluster.Name, + ), + Field: c.field, + }) + + return result } - return checks -} + for _, storageClassConfig := range c.csiSpec.StorageClassConfigs { + if storageClassConfig.Parameters == nil { + continue + } -// storageContainerCheck checks if the storage container specified in the CSIProvider's StorageClassConfigs exists. -// It admits the NodeSpec instead of the MachineDetails because the failure domains will be specified in the NodeSpec -// and the MachineDetails.Cluster will be nil in that case. -func storageContainerCheck( - n *nutanixChecker, - nodeSpec *carenv1.NutanixNodeSpec, - field string, - csiSpec *carenv1.CSIProvider, -) preflight.Check { - const ( - csiParameterKeyStorageContainer = "storageContainer" - ) - - return func(ctx context.Context) preflight.CheckResult { - result := preflight.CheckResult{ - Name: "NutanixStorageContainer", - Allowed: true, + storageContainer, ok := storageClassConfig.Parameters[csiParameterKeyStorageContainer] + if !ok { + continue } - if csiSpec == nil { + + // TODO: check if cluster name is set, if not use uuid. + // If neither is set, use the cluster name from the NodeSpec failure domain. + if _, err := getStorageContainer(c.nclient, c.nodeSpec, storageContainer); err != nil { result.Allowed = false result.Error = true result.Causes = append(result.Causes, preflight.Cause{ Message: fmt.Sprintf( - "no storage container found for cluster %q", - *nodeSpec.MachineDetails.Cluster.Name, + "failed to check if storage container named %q exists: %s", + storageContainer, + err, ), - Field: field, + Field: c.field, }) return result } + } - if csiSpec.StorageClassConfigs == nil { - result.Allowed = false - result.Causes = append(result.Causes, preflight.Cause{ - Message: fmt.Sprintf( - "no storage class configs found for cluster %q", - *nodeSpec.MachineDetails.Cluster.Name, - ), - Field: field, - }) + return result +} - return result - } +func newStorageContainerChecks(cd *checkDependencies) []preflight.Check { + checks := []preflight.Check{} - for _, storageClassConfig := range csiSpec.StorageClassConfigs { - if storageClassConfig.Parameters == nil { - continue - } - - storageContainer, ok := storageClassConfig.Parameters[csiParameterKeyStorageContainer] - if !ok { - continue - } - - // TODO: check if cluster name is set, if not use uuid. - // If neither is set, use the cluster name from the NodeSpec failure domain. - if _, err := getStorageContainer(n.v4client, nodeSpec, storageContainer); 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", - storageContainer, - err, - ), - Field: field, - }) + // If there is no CSI configuration, there is no need to check for storage containers. + if cd.nutanixClusterConfigSpec == nil || + cd.nutanixClusterConfigSpec.Addons == nil || + cd.nutanixClusterConfigSpec.Addons.CSI == nil { + return checks + } - return result - } - } + if cd.nutanixClusterConfigSpec != nil && cd.nutanixClusterConfigSpec.ControlPlane != nil && + cd.nutanixClusterConfigSpec.ControlPlane.Nutanix != nil { + checks = append(checks, + &storageContainerCheck{ + nodeSpec: cd.nutanixClusterConfigSpec.ControlPlane.Nutanix, + field: "cluster.spec.topology[.name=clusterConfig].value.controlPlane.nutanix", + csiSpec: &cd.nutanixClusterConfigSpec.Addons.CSI.Providers.NutanixCSI, + nclient: cd.nclient, + }, + ) + } - return result + for mdName, nutanixWorkerNodeConfigSpec := range cd.nutanixWorkerNodeConfigSpecByMachineDeploymentName { + if nutanixWorkerNodeConfigSpec.Nutanix != nil { + checks = append(checks, + &storageContainerCheck{ + nodeSpec: nutanixWorkerNodeConfigSpec.Nutanix, + field: fmt.Sprintf( + "cluster.spec.topology.workers.machineDeployments[.name=%s]"+ + ".variables[.name=workerConfig].value.nutanix", + mdName, + ), + csiSpec: &cd.nutanixClusterConfigSpec.Addons.CSI.Providers.NutanixCSI, + nclient: cd.nclient, + }, + ) + } } + + return checks } func getStorageContainer( - client v4client, + client client, nodeSpec *carenv1.NutanixNodeSpec, storageContainerName string, ) (*clustermgmtv4.StorageContainer, error) { @@ -175,7 +176,7 @@ func getStorageContainer( } func getCluster( - client v4client, + client client, clusterIdentifier *v1beta1.NutanixResourceIdentifier, ) (*clustermgmtv4.Cluster, error) { switch { diff --git a/pkg/webhook/preflight/nutanix/storagecontainer_test.go b/pkg/webhook/preflight/nutanix/storagecontainer_test.go index efce79480..e2284e3ba 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer_test.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer_test.go @@ -8,7 +8,6 @@ import ( "fmt" "testing" - "github.com/go-logr/logr/testr" clustermgmtv4 "github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4/models/clustermgmt/v4/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -218,65 +217,29 @@ func TestInitStorageContainerChecks(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - logger := testr.New(t) - - // Create checker - checker := &nutanixChecker{ - log: logger, - nutanixClusterConfigSpec: tc.nutanixClusterConfigSpec, + cd := &checkDependencies{ + nutanixClusterConfigSpec: tc.nutanixClusterConfigSpec, nutanixWorkerNodeConfigSpecByMachineDeploymentName: tc.workerNodeConfigSpecByMDName, - initStorageContainerChecksFunc: initStorageContainerChecks, - } - - // Set up a tracing function to replace storageContainerCheck - var capturedFields []string - checker.storageContainerCheckFunc = func( - n *nutanixChecker, - nodeSpec *carenv1.NutanixNodeSpec, - field string, - csiSpec *carenv1.CSIProvider, - ) preflight.Check { - capturedFields = append(capturedFields, field) - return func(ctx context.Context) preflight.CheckResult { - return preflight.CheckResult{Name: "StorageContainerCheck:" + field} - } } // Call the function under test - checks := checker.initStorageContainerChecksFunc(checker) + checks := newStorageContainerChecks(cd) // Verify number of checks assert.Len(t, checks, tc.expectedChecksCount, "Wrong number of checks created") - - // Verify number of captured fields matches number of checks - assert.Len(t, capturedFields, tc.expectedChecksCount, "Wrong number of fields captured") - - // Additional verification that each field is correctly formatted - for _, field := range capturedFields { - if tc.nutanixClusterConfigSpec != nil && tc.nutanixClusterConfigSpec.ControlPlane != nil && - tc.nutanixClusterConfigSpec.ControlPlane.Nutanix != nil { - expectedCPField := "cluster.spec.topology[.name=clusterConfig].value.controlPlane.nutanix" - if field == expectedCPField { - continue - } - } - // If not CP field, should be worker field with MD name - assert.Contains(t, field, "cluster.spec.topology.workers.machineDeployments[.name=") - assert.Contains(t, field, "].variables[.name=workerConfig].value.nutanix") - } }) } } func TestStorageContainerCheck(t *testing.T) { clusterName := "test-cluster" - fieldPath := "test.field.path" + field := "test.field.path" testCases := []struct { name string nodeSpec *carenv1.NutanixNodeSpec csiSpec *carenv1.CSIProvider - mockv4client *mockv4client + nclient client expectedResult preflight.CheckResult expectedAllowed bool expectedError bool @@ -293,7 +256,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, csiSpec: nil, - mockv4client: nil, + nclient: nil, expectedAllowed: false, expectedError: true, expectedCauseMessage: fmt.Sprintf("no storage container found for cluster %q", clusterName), @@ -309,7 +272,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, csiSpec: &carenv1.CSIProvider{StorageClassConfigs: nil}, - mockv4client: nil, + nclient: nil, expectedAllowed: false, expectedError: false, expectedCauseMessage: fmt.Sprintf("no storage class configs found for cluster %q", clusterName), @@ -331,7 +294,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: nil, + nclient: nil, expectedAllowed: true, expectedError: false, }, @@ -354,7 +317,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: nil, + nclient: nil, expectedAllowed: true, expectedError: false, }, @@ -377,7 +340,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, nil }, @@ -448,7 +411,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, nil }, @@ -526,7 +489,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, nil }, @@ -599,7 +562,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, fmt.Errorf("API error") }, @@ -642,7 +605,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, nil }, @@ -708,7 +671,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, nil }, @@ -787,7 +750,7 @@ func TestStorageContainerCheck(t *testing.T) { }, }, }, - mockv4client: &mockv4client{ + nclient: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, nil }, @@ -857,20 +820,17 @@ func TestStorageContainerCheck(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - logger := testr.New(t) - - // Create checker with mock v4 client - checker := &nutanixChecker{ - log: logger, - v4client: tc.mockv4client, - } - // Create the check function - checkFunc := storageContainerCheck(checker, tc.nodeSpec, fieldPath, tc.csiSpec) + check := storageContainerCheck{ + nodeSpec: tc.nodeSpec, + csiSpec: tc.csiSpec, + nclient: tc.nclient, + field: field, + } // Run the check ctx := context.Background() - result := checkFunc(ctx) + result := check.Run(ctx) // Verify the result assert.Equal(t, tc.expectedAllowed, result.Allowed) @@ -879,7 +839,7 @@ func TestStorageContainerCheck(t *testing.T) { if tc.expectedCauseMessage != "" { require.NotEmpty(t, result.Causes) assert.Contains(t, result.Causes[0].Message, tc.expectedCauseMessage) - assert.Equal(t, fieldPath, result.Causes[0].Field) + assert.Equal(t, field, result.Causes[0].Field) } else { assert.Empty(t, result.Causes) } @@ -891,7 +851,7 @@ func TestGetCluster(t *testing.T) { testCases := []struct { name string clusterIdentifier *capxv1.NutanixResourceIdentifier - mockv4client *mockv4client + client client expectError bool errorContains string expectedClusterID string @@ -902,7 +862,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierUUID, UUID: ptr.To("test-uuid-123"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { assert.Equal(t, "test-uuid-123", *id) resp := &clustermgmtv4.GetClusterApiResponse{ @@ -928,7 +888,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierUUID, UUID: ptr.To("test-uuid-error"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, fmt.Errorf("API error") }, @@ -942,7 +902,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierUUID, UUID: ptr.To("test-uuid-invalid"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { // Return an invalid data type resp := &clustermgmtv4.GetClusterApiResponse{ @@ -960,7 +920,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: ptr.To("test-cluster"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -996,7 +956,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: ptr.To("test-cluster-error"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -1020,7 +980,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: ptr.To("test-cluster-nil"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -1044,7 +1004,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: ptr.To("test-cluster-nil-data"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -1070,7 +1030,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: ptr.To("test-cluster-not-found"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -1099,7 +1059,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: ptr.To("test-cluster-duplicate"), }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -1136,7 +1096,7 @@ func TestGetCluster(t *testing.T) { clusterIdentifier: &capxv1.NutanixResourceIdentifier{ Type: "invalid", }, - mockv4client: &mockv4client{}, + client: &mocknclient{}, expectError: true, errorContains: "cluster identifier is missing both name and uuid", }, @@ -1146,7 +1106,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierUUID, UUID: nil, }, - mockv4client: &mockv4client{ + client: &mocknclient{ getClusterByIdFunc: func(id *string) (*clustermgmtv4.GetClusterApiResponse, error) { return nil, fmt.Errorf("should not be called") }, @@ -1160,7 +1120,7 @@ func TestGetCluster(t *testing.T) { Type: capxv1.NutanixIdentifierName, Name: nil, }, - mockv4client: &mockv4client{ + client: &mocknclient{ listClustersFunc: func(page, limit *int, filter, @@ -1182,7 +1142,7 @@ func TestGetCluster(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - cluster, err := getCluster(tc.mockv4client, tc.clusterIdentifier) + cluster, err := getCluster(tc.client, tc.clusterIdentifier) if tc.expectError { require.Error(t, err) From 85226e458238de1fc7005e64e54b13d138edfae5 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Wed, 18 Jun 2025 13:52:43 -0700 Subject: [PATCH 3/5] fixup! feat(preflight): Add a check for storage containers Only create checks if client is initialized --- .../preflight/nutanix/storagecontainer.go | 4 ++++ .../preflight/nutanix/storagecontainer_test.go | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go index b333dbde0..2a712ff37 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -96,6 +96,10 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult { func newStorageContainerChecks(cd *checkDependencies) []preflight.Check { checks := []preflight.Check{} + if cd.nclient == nil { + return checks + } + // If there is no CSI configuration, there is no need to check for storage containers. if cd.nutanixClusterConfigSpec == nil || cd.nutanixClusterConfigSpec.Addons == nil || diff --git a/pkg/webhook/preflight/nutanix/storagecontainer_test.go b/pkg/webhook/preflight/nutanix/storagecontainer_test.go index e2284e3ba..e76207b8d 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer_test.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer_test.go @@ -24,12 +24,21 @@ func TestInitStorageContainerChecks(t *testing.T) { nutanixClusterConfigSpec *carenv1.NutanixClusterConfigSpec workerNodeConfigSpecByMDName map[string]*carenv1.NutanixWorkerNodeConfigSpec expectedChecksCount int + nclient client }{ + { + name: "client not initialized", + nutanixClusterConfigSpec: nil, + workerNodeConfigSpecByMDName: nil, + expectedChecksCount: 0, + nclient: nil, + }, { name: "nil cluster config", nutanixClusterConfigSpec: nil, workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, expectedChecksCount: 0, + nclient: &mocknclient{}, }, { name: "cluster config without addons", @@ -40,6 +49,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, expectedChecksCount: 0, + nclient: &mocknclient{}, }, { name: "cluster config with addons but no CSI", @@ -51,6 +61,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, expectedChecksCount: 0, + nclient: &mocknclient{}, }, { name: "cluster config with CSI but no control plane or worker nodes", @@ -65,6 +76,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, expectedChecksCount: 0, + nclient: &mocknclient{}, }, { name: "cluster config with CSI and control plane", @@ -97,6 +109,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, expectedChecksCount: 1, + nclient: &mocknclient{}, }, { name: "cluster config with CSI and worker nodes", @@ -122,6 +135,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, }, expectedChecksCount: 1, + nclient: &mocknclient{}, }, { name: "cluster config with CSI, control plane and worker nodes", @@ -167,6 +181,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, }, expectedChecksCount: 3, // 1 for control plane, 2 for workers + nclient: &mocknclient{}, }, { name: "cluster config with CSI and null control plane nutanix", @@ -184,6 +199,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, workerNodeConfigSpecByMDName: map[string]*carenv1.NutanixWorkerNodeConfigSpec{}, expectedChecksCount: 0, + nclient: &mocknclient{}, }, { name: "cluster config with CSI and some nutanix nil workers", @@ -212,6 +228,7 @@ func TestInitStorageContainerChecks(t *testing.T) { }, }, expectedChecksCount: 1, // only for the defined worker-1 + nclient: &mocknclient{}, }, } @@ -220,6 +237,7 @@ func TestInitStorageContainerChecks(t *testing.T) { cd := &checkDependencies{ nutanixClusterConfigSpec: tc.nutanixClusterConfigSpec, nutanixWorkerNodeConfigSpecByMachineDeploymentName: tc.workerNodeConfigSpecByMDName, + nclient: tc.nclient, } // Call the function under test From 85b44224887522db285afdf34816ddfb4f253827 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Wed, 18 Jun 2025 15:04:17 -0700 Subject: [PATCH 4/5] fixup! feat(preflight): Add a check for storage containers Remove stale comment --- pkg/webhook/preflight/nutanix/storagecontainer.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/webhook/preflight/nutanix/storagecontainer.go b/pkg/webhook/preflight/nutanix/storagecontainer.go index 2a712ff37..2a68ad440 100644 --- a/pkg/webhook/preflight/nutanix/storagecontainer.go +++ b/pkg/webhook/preflight/nutanix/storagecontainer.go @@ -72,8 +72,6 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult { continue } - // TODO: check if cluster name is set, if not use uuid. - // If neither is set, use the cluster name from the NodeSpec failure domain. if _, err := getStorageContainer(c.nclient, c.nodeSpec, storageContainer); err != nil { result.Allowed = false result.Error = true From 8f3fffde246a124ee3f1a3fd7d09272245ca6938 Mon Sep 17 00:00:00 2001 From: Sid Shukla Date: Thu, 19 Jun 2025 23:52:28 +0200 Subject: [PATCH 5/5] fix(deps): set prism-go-client to v0.5.2 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5e8a41caf..b520b3137 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/google/uuid v1.6.0 github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api v0.0.0-00010101000000-000000000000 github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common v0.7.0 - github.com/nutanix-cloud-native/prism-go-client v0.5.2-0.20250602134145-662333927e6d + github.com/nutanix-cloud-native/prism-go-client v0.5.2 github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4 v4.0.1-beta.2 github.com/nutanix/ntnx-api-golang-clients/networking-go-client/v4 v4.0.2-beta.1 github.com/nutanix/ntnx-api-golang-clients/prism-go-client/v4 v4.0.1-beta.1 diff --git a/go.sum b/go.sum index dd19186f1..314ed64e1 100644 --- a/go.sum +++ b/go.sum @@ -227,8 +227,8 @@ github.com/morikuni/aec v1.0.0 h1:nP9CBfwrvYnBRgY6qfDQkygYDmYwOilePFkwzv4dU8A= github.com/morikuni/aec v1.0.0/go.mod h1:BbKIizmSmc5MMPqRYbxO4ZU0S0+P200+tUnFx7PXmsc= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= -github.com/nutanix-cloud-native/prism-go-client v0.5.2-0.20250602134145-662333927e6d h1:ZjrHbyZLeaWMhtBNCe8uIfvAHs0ebqx8WxJRW59hnMg= -github.com/nutanix-cloud-native/prism-go-client v0.5.2-0.20250602134145-662333927e6d/go.mod h1:N/O9fz5fimjb30RxlPbKbGs/Z2lqMgDqrb6CrsZvQrA= +github.com/nutanix-cloud-native/prism-go-client v0.5.2 h1:qhFJeC3CRrWM8BTaNvxnjShVNH5E99z5MPU3a2BO14A= +github.com/nutanix-cloud-native/prism-go-client v0.5.2/go.mod h1:N/O9fz5fimjb30RxlPbKbGs/Z2lqMgDqrb6CrsZvQrA= github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4 v4.0.1-beta.2 h1:s1u5/GEw3mTZakepJoTD1OvPVU1YuioRxmKZin+W99s= github.com/nutanix/ntnx-api-golang-clients/clustermgmt-go-client/v4 v4.0.1-beta.2/go.mod h1:sd4Fnk6MVfEDVY+8WyRoQTmLhi2SgZ3riySWErVHf8E= github.com/nutanix/ntnx-api-golang-clients/networking-go-client/v4 v4.0.2-beta.1 h1:PvZQwYhhJtxmzLpnzEhHTpp2fV6woc6W65PHGsHzVfs=