fix: machineDetails fields "cluster" and "subnets" should be optional (#1217)

https://jira.nutanix.com/browse/NCN-108428:
When failureDomain is configured, machineDetails "cluster" and "subnets"
should be optional.

Added webhook validation to Cluster to make sure either cluster/subnets
fields are configured, or failureDomain is configured, for both
controlPlane and worker nodes.

Added unit tests. Did local integration test.

Below is the sample errors in my local test.
```
$ kubectl create -f cl-fd2.yml --dry-run=server
Error from server (Forbidden): error when creating "cl-fd2.yml": admission webhook "cluster-validator.caren.nutanix.com" denied the request: [spec.topology.variables.clusterConfig.value.controlPlane.nutanix.machineDetails.cluster: Required value: "cluster" must set when failureDomains is not configured., spec.topology.variables.clusterConfig.value.controlPlane.nutanix.machineDetails.subnets: Required value: "subnets" must set when failureDomains is not configured., spec.topology.workers.machineDeployments.variables.workerConfig.nutanix.machineDetails.cluster: Required value: "cluster" must set when failureDomain is not configured., spec.topology.workers.machineDeployments.variables.workerConfig.nutanix.machineDetails.subnets: Required value: "subnets" must set when failureDomain is not configured.]
```

Author: yanhua121

api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml

@@ -511,9 +511,7 @@ spec:
                               format: int32
                               type: integer
                           required:
-                            - cluster
                             - memorySize
-                            - subnets
                             - systemDiskSize
                             - vcpuSockets
                             - vcpusPerSocket

api/v1alpha1/crds/caren.nutanix.com_nutanixworkernodeconfigs.yaml

@@ -234,9 +234,7 @@ spec:
                           format: int32
                           type: integer
                       required:
-                        - cluster
                         - memorySize
-                        - subnets
                         - systemDiskSize
                         - vcpuSockets
                         - vcpusPerSocket

api/v1alpha1/nutanix_node_types.go

@@ -50,13 +50,15 @@ type NutanixMachineDetails struct {
 
 	// cluster identifies the Prism Element in which the machine will be created.
 	// The identifier (uuid or name) can be obtained from the console or API.
-	// +kubebuilder:validation:Required
-	Cluster capxv1.NutanixResourceIdentifier `json:"cluster"`
+	// +kubebuilder:validation:Optional
+	// +optional
+	Cluster *capxv1.NutanixResourceIdentifier `json:"cluster,omitempty"`
 
 	// subnet identifies the network subnet to use for the machine.
 	// The identifier (uuid or name) can be obtained from the console or API.
-	// +kubebuilder:validation:Required
-	Subnets []capxv1.NutanixResourceIdentifier `json:"subnets"`
+	// +kubebuilder:validation:Optional
+	// +optional
+	Subnets []capxv1.NutanixResourceIdentifier `json:"subnets,omitempty"`
 
 	// List of categories that need to be added to the machines. Categories must already
 	// exist in Prism Central. One category key can have more than one value.

api/v1alpha1/zz_generated.deepcopy.go

@@ -162,13 +162,18 @@ func minimalNutanixClusterConfigSpec() v1alpha1.NutanixClusterConfigSpec {
 						Type: capxv1.NutanixIdentifierName,
 						Name: ptr.To("fake-image"),
 					},
-					Cluster: capxv1.NutanixResourceIdentifier{
+					Cluster: &capxv1.NutanixResourceIdentifier{
 						Type: capxv1.NutanixIdentifierName,
 						Name: ptr.To("fake-pe-cluster"),
 					},
 					MemorySize:     resource.MustParse("8Gi"),
 					SystemDiskSize: resource.MustParse("40Gi"),
-					Subnets:        []capxv1.NutanixResourceIdentifier{},
+					Subnets: []capxv1.NutanixResourceIdentifier{
+						{
+							Type: capxv1.NutanixIdentifierName,
+							Name: ptr.To("fake-subnet"),
+						},
+					},
 				},
 			},
 		},

pkg/handlers/generic/mutation/kubeproxymode/variables_test.go

@@ -50,13 +50,18 @@ func minimumClusterConfigSpec() v1alpha1.NutanixClusterConfigSpec {
 						Type: capxv1.NutanixIdentifierName,
 						Name: ptr.To("fake-image"),
 					},
-					Cluster: capxv1.NutanixResourceIdentifier{
+					Cluster: &capxv1.NutanixResourceIdentifier{
 						Type: capxv1.NutanixIdentifierName,
 						Name: ptr.To("fake-pe-cluster"),
 					},
 					MemorySize:     resource.MustParse("8Gi"),
 					SystemDiskSize: resource.MustParse("40Gi"),
-					Subnets:        []capxv1.NutanixResourceIdentifier{},
+					Subnets: []capxv1.NutanixResourceIdentifier{
+						{
+							Type: capxv1.NutanixIdentifierName,
+							Name: ptr.To("fake-subnet"),
+						},
+					},
 				},
 			},
 		},

pkg/handlers/nutanix/mutation/controlplanefailuredomains/variables_test.go

@@ -97,7 +97,14 @@ func (h *nutanixMachineDetailsPatchHandler) Mutate(
 			spec := obj.Spec.Template.Spec
 
 			spec.BootType = nutanixMachineDetailsVar.BootType
-			spec.Cluster = nutanixMachineDetailsVar.Cluster
+			if nutanixMachineDetailsVar.Cluster != nil {
+				spec.Cluster = *nutanixMachineDetailsVar.Cluster
+			} else {
+				// Have to set the required Type field.
+				spec.Cluster = capxv1.NutanixResourceIdentifier{
+					Type: capxv1.NutanixIdentifierName,
+				}
+			}
 
 			switch {
 			case nutanixMachineDetailsVar.Image != nil:
@@ -118,7 +125,6 @@ func (h *nutanixMachineDetailsPatchHandler) Mutate(
 			spec.Subnets = slices.Clone(nutanixMachineDetailsVar.Subnets)
 			spec.AdditionalCategories = slices.Clone(nutanixMachineDetailsVar.AdditionalCategories)
 			spec.GPUs = slices.Clone(nutanixMachineDetailsVar.GPUs)
-
 			spec.Project = nutanixMachineDetailsVar.Project.DeepCopy()
 
 			obj.Spec.Template.Spec = spec

pkg/handlers/nutanix/mutation/machinedetails/inject.go

@@ -27,7 +27,7 @@ var (
 			Type: capxv1.NutanixIdentifierName,
 			Name: ptr.To("fake-image"),
 		},
-		Cluster: capxv1.NutanixResourceIdentifier{
+		Cluster: &capxv1.NutanixResourceIdentifier{
 			Type: capxv1.NutanixIdentifierName,
 			Name: ptr.To("fake-pe-cluster"),
 		},
@@ -72,7 +72,7 @@ var (
 		ImageLookup: &capxv1.NutanixImageLookup{
 			BaseOS: "rockylinux-9",
 		},
-		Cluster: capxv1.NutanixResourceIdentifier{
+		Cluster: &capxv1.NutanixResourceIdentifier{
 			Type: capxv1.NutanixIdentifierName,
 			Name: ptr.To("fake-pe-cluster"),
 		},

pkg/handlers/nutanix/mutation/machinedetails/inject_control_plane_test.go

@@ -109,13 +109,18 @@ func minimumClusterConfigSpec() v1alpha1.NutanixClusterConfigSpec {
 						Type: capxv1.NutanixIdentifierName,
 						Name: ptr.To("fake-image"),
 					},
-					Cluster: capxv1.NutanixResourceIdentifier{
+					Cluster: &capxv1.NutanixResourceIdentifier{
 						Type: capxv1.NutanixIdentifierName,
 						Name: ptr.To("fake-pe-cluster"),
 					},
 					MemorySize:     resource.MustParse("8Gi"),
 					SystemDiskSize: resource.MustParse("40Gi"),
-					Subnets:        []capxv1.NutanixResourceIdentifier{},
+					Subnets: []capxv1.NutanixResourceIdentifier{
+						{
+							Type: capxv1.NutanixIdentifierName,
+							Name: ptr.To("fake-subnet"),
+						},
+					},
 				},
 			},
 		},

pkg/handlers/nutanix/mutation/machinedetails/variables_test.go

@@ -11,6 +11,7 @@ import (
 	"net/netip"
 
 	v1 "k8s.io/api/admission/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -89,9 +90,146 @@ func (a *nutanixValidator) validate(
 		}
 	}
 
+	if err := validateTopologyFailureDomainConfig(clusterConfig, cluster); err != nil {
+		return admission.Denied(err.Error())
+	}
+
 	return admission.Allowed("")
 }
 
+// validateTopologyFailureDomainConfig validates the failure domain related configuration in cluster topology.
+func validateTopologyFailureDomainConfig(
+	clusterConfig *variables.ClusterConfigSpec,
+	cluster *clusterv1.Cluster,
+) error {
+	fldErrs := field.ErrorList{}
+
+	// Validate control plane failure domain configuration
+	if controlPlaneErrs := validateControlPlaneFailureDomainConfig(clusterConfig); controlPlaneErrs != nil {
+		fldErrs = append(fldErrs, controlPlaneErrs...)
+	}
+
+	// Validate worker failure domain configuration
+	if workerErrs := validateWorkerFailureDomainConfig(cluster); workerErrs != nil {
+		fldErrs = append(fldErrs, workerErrs...)
+	}
+
+	return fldErrs.ToAggregate()
+}
+
+// validateControlPlaneFailureDomainConfig validates that either failureDomains are set,
+// or cluster and subnets are set with machineDetails, for control plane.
+func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterConfigSpec) field.ErrorList {
+	fldErrs := field.ErrorList{}
+	fldPath := field.NewPath(
+		"spec",
+		"topology",
+		"variables",
+		"clusterConfig",
+		"value",
+		"controlPlane",
+		"nutanix",
+		"machineDetails",
+	)
+
+	if clusterConfig.ControlPlane != nil &&
+		clusterConfig.ControlPlane.Nutanix != nil &&
+		len(clusterConfig.ControlPlane.Nutanix.FailureDomains) == 0 {
+		machineDetails := clusterConfig.ControlPlane.Nutanix.MachineDetails
+		if machineDetails.Cluster == nil || (!machineDetails.Cluster.IsName() && !machineDetails.Cluster.IsUUID()) {
+			fldErrs = append(fldErrs, field.Required(
+				fldPath.Child("cluster"),
+				"\"cluster\" must be set when failureDomains are not configured.",
+			))
+		}
+
+		if len(machineDetails.Subnets) == 0 {
+			fldErrs = append(fldErrs, field.Required(
+				fldPath.Child("subnets"),
+				"\"subnets\" must be set when failureDomains are not configured.",
+			))
+		}
+	}
+
+	return fldErrs
+}
+
+// validateWorkerFailureDomainConfig validates either failureDomains is set,
+// or cluster and subnets are set with machineDetails, for workers.
+func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorList {
+	fldErrs := field.ErrorList{}
+	workerConfigVarPath := field.NewPath("spec", "topology", "variables", "workerConfig")
+	workerConfigMDVarOverridePath := field.NewPath(
+		"spec",
+		"topology",
+		"workers",
+		"machineDeployments",
+		"variables",
+		"overrides",
+		"workerConfig",
+	)
+
+	// Get the machineDetails from cluster variable "workerConfig" if it is configured
+	defaultWorkerConfig, err := variables.UnmarshalWorkerConfigVariable(cluster.Spec.Topology.Variables)
+	if err != nil {
+		fldErrs = append(fldErrs, field.InternalError(workerConfigVarPath,
+			fmt.Errorf("failed to unmarshal cluster topology variable %q: %w", v1alpha1.WorkerConfigVariableName, err)))
+	}
+
+	if cluster.Spec.Topology.Workers != nil {
+		for i := range cluster.Spec.Topology.Workers.MachineDeployments {
+			md := cluster.Spec.Topology.Workers.MachineDeployments[i]
+			if md.FailureDomain != nil && *md.FailureDomain != "" {
+				// failureDomain is configured so we can skip checking the cluster and subnet in machineDetails
+				continue
+			}
+
+			// Get the machineDetails from the overrides variable "workerConfig" if it is configured,
+			// otherwise use the defaultWorkerConfig if it is configured.
+			var workerConfig *variables.WorkerNodeConfigSpec
+			if md.Variables != nil && len(md.Variables.Overrides) > 0 {
+				workerConfig, err = variables.UnmarshalWorkerConfigVariable(md.Variables.Overrides)
+				if err != nil {
+					fldErrs = append(fldErrs, field.InternalError(
+						workerConfigMDVarOverridePath,
+						fmt.Errorf(
+							"failed to unmarshal worker overrides variable %q: %w",
+							v1alpha1.WorkerConfigVariableName,
+							err,
+						),
+					))
+				}
+			}
+
+			wcfgPath := workerConfigMDVarOverridePath
+			if workerConfig == nil {
+				workerConfig = defaultWorkerConfig
+				wcfgPath = workerConfigVarPath
+			}
+			if workerConfig == nil || workerConfig.Nutanix == nil {
+				continue
+			}
+
+			machineDetails := workerConfig.Nutanix.MachineDetails
+			if machineDetails.Cluster == nil ||
+				(!machineDetails.Cluster.IsName() && !machineDetails.Cluster.IsUUID()) {
+				fldErrs = append(fldErrs, field.Required(
+					wcfgPath.Child("value", "nutanix", "machineDetails", "cluster"),
+					"\"cluster\" must be set when failureDomain is not configured.",
+				))
+			}
+			if len(machineDetails.Subnets) == 0 {
+				fldErrs = append(fldErrs, field.Required(
+					wcfgPath.Child("value", "nutanix", "machineDetails", "subnets"),
+					"\"subnets\" must be set when failureDomain is not configured.",
+				))
+			}
+		}
+	}
+
+	return fldErrs
+}
+
 // validatePrismCentralIPNotInLoadBalancerIPRange checks if the Prism Central IP is not
 // in the MetalLB Load Balancer IP range and error out if it is.
 func validatePrismCentralIPNotInLoadBalancerIPRange(