fix(webhook): Update cluster webhook to validate failure domains

Use XOR instead of OR for cluster and subnet presence check against
failure domains on control plane and worker overrides.

Author: thunderboltsid

pkg/webhook/cluster/nutanix_validator.go

@@ -117,8 +117,8 @@ func validateTopologyFailureDomainConfig(
 	return fldErrs.ToAggregate()
 }
 
-// validateControlPlaneFailureDomainConfig validates that either failureDomains are set,
-// or cluster and subnets are set with machineDetails, for control plane.
+// validateControlPlaneFailureDomainConfig validates XOR behavior: either failureDomains are set
+// OR cluster and subnets are set with machineDetails, but not both, for control plane.
 func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterConfigSpec) field.ErrorList {
 	fldErrs := field.ErrorList{}
 	fldPath := field.NewPath(
@@ -132,31 +132,54 @@ func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterCon
 		"machineDetails",
 	)
 
-	if clusterConfig.ControlPlane != nil &&
-		clusterConfig.ControlPlane.Nutanix != nil &&
-		len(clusterConfig.ControlPlane.Nutanix.FailureDomains) == 0 {
+	if clusterConfig.ControlPlane != nil && clusterConfig.ControlPlane.Nutanix != nil {
 		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.",
-			))
-		}
+		hasFailureDomains := len(clusterConfig.ControlPlane.Nutanix.FailureDomains) > 0
+		hasCluster := machineDetails.Cluster != nil &&
+			(machineDetails.Cluster.IsName() || machineDetails.Cluster.IsUUID())
+		hasSubnets := len(machineDetails.Subnets) > 0
+
+		if !hasFailureDomains {
+			// No failure domains -> cluster/subnets MUST be set
+			if !hasCluster {
+				fldErrs = append(fldErrs, field.Required(
+					fldPath.Child("cluster"),
+					"\"cluster\" must be set when failureDomains are not configured.",
+				))
+			}
+
+			if !hasSubnets {
+				fldErrs = append(fldErrs, field.Required(
+					fldPath.Child("subnets"),
+					"\"subnets\" must be set when failureDomains are not configured.",
+				))
+			}
+		} else {
+			// Failure domains present -> cluster/subnets MUST NOT be set
+			if hasCluster {
+				fldErrs = append(fldErrs, field.Forbidden(
+					fldPath.Child("cluster"),
+					"\"cluster\" must not be set when failureDomains are configured.",
+				))
+			}
 
-		if len(machineDetails.Subnets) == 0 {
-			fldErrs = append(fldErrs, field.Required(
-				fldPath.Child("subnets"),
-				"\"subnets\" must be set when failureDomains are not configured.",
-			))
+			if hasSubnets {
+				fldErrs = append(fldErrs, field.Forbidden(
+					fldPath.Child("subnets"),
+					"\"subnets\" must not be set when failureDomains are 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 {
+// validateWorkerFailureDomainConfig validates XOR behavior: either failureDomain is set
+// OR cluster and subnets are set with machineDetails, but not both, for workers.
+func validateWorkerFailureDomainConfig(
+	cluster *clusterv1.Cluster,
+) field.ErrorList {
 	fldErrs := field.ErrorList{}
 	workerConfigVarPath := field.NewPath("spec", "topology", "variables", "workerConfig")
 	workerConfigMDVarOverridePath := field.NewPath(
@@ -179,10 +202,7 @@ func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorLi
 	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
-			}
+			hasFailureDomain := md.FailureDomain != nil && *md.FailureDomain != ""
 
 			// Get the machineDetails from the overrides variable "workerConfig" if it is configured,
 			// otherwise use the defaultWorkerConfig if it is configured.
@@ -211,18 +231,38 @@ func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorLi
 			}
 
 			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.",
-				))
+			hasCluster := machineDetails.Cluster != nil &&
+				(machineDetails.Cluster.IsName() || machineDetails.Cluster.IsUUID())
+			hasSubnets := len(machineDetails.Subnets) > 0
+
+			if !hasFailureDomain {
+				// No failure domain -> cluster/subnets MUST be set
+				if !hasCluster {
+					fldErrs = append(fldErrs, field.Required(
+						wcfgPath.Child("value", "nutanix", "machineDetails", "cluster"),
+						"\"cluster\" must be set when failureDomain is not configured.",
+					))
+				}
+				if !hasSubnets {
+					fldErrs = append(fldErrs, field.Required(
+						wcfgPath.Child("value", "nutanix", "machineDetails", "subnets"),
+						"\"subnets\" must be set when failureDomain is not configured.",
+					))
+				}
+			} else {
+				// Failure domain present -> cluster/subnets MUST NOT be set
+				if hasCluster {
+					fldErrs = append(fldErrs, field.Forbidden(
+						wcfgPath.Child("value", "nutanix", "machineDetails", "cluster"),
+						"\"cluster\" must not be set when failureDomain is configured.",
+					))
+				}
+				if hasSubnets {
+					fldErrs = append(fldErrs, field.Forbidden(
+						wcfgPath.Child("value", "nutanix", "machineDetails", "subnets"),
+						"\"subnets\" must not be set when failureDomain is configured.",
+					))
+				}
 			}
 		}
 	}

pkg/webhook/cluster/nutanix_validator_test.go

@@ -270,13 +270,7 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
 			expectedErr:         false,
 			expectedErrMessages: nil,
 		},
-		{
-			name:                "controlPlane failureDomains configured and machineDetail cluster and subnets set",
-			clusterConfig:       fakeClusterConfigSpec(true, true, true),
-			cluster:             fakeCluster(t, true, false, false, workerConfigNone),
-			expectedErr:         false,
-			expectedErrMessages: nil,
-		},
+
 		{
 			name:          "worker failureDomain not configured and missing 'cluster' in workerConfig overrides only",
 			clusterConfig: fakeClusterConfigSpec(true, false, false),
@@ -312,13 +306,7 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
 			expectedErr:         false,
 			expectedErrMessages: nil,
 		},
-		{
-			name:                "worker failureDomain configured and machineDetail cluster and subnets set in workerConfig overrides only", //nolint:lll // name is long.
-			clusterConfig:       fakeClusterConfigSpec(true, false, false),
-			cluster:             fakeCluster(t, true, true, true, workerConfigOverridesOnly),
-			expectedErr:         false,
-			expectedErrMessages: nil,
-		},
+
 		{
 			name:          "worker failureDomain not configured and missing machineDetail 'cluster' and 'subnets' in workerConfig default only", //nolint:lll // name is long.
 			clusterConfig: fakeClusterConfigSpec(true, false, false),
@@ -353,6 +341,33 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
 			expectedErr:         false,
 			expectedErrMessages: nil,
 		},
+		{
+			name:          "controlPlane failureDomains configured with cluster/subnets set violates XOR",
+			clusterConfig: fakeClusterConfigSpec(true, true, true),
+			cluster:       fakeCluster(t, true, false, false, workerConfigNone),
+			expectedErr:   true,
+			expectedErrMessages: []string{
+				"spec.topology.variables.clusterConfig.value.controlPlane.nutanix.machineDetails.cluster: Forbidden: \"cluster\" must not be set when failureDomains are configured.", //nolint:lll // Message is long.
+				"spec.topology.variables.clusterConfig.value.controlPlane.nutanix.machineDetails.subnets: Forbidden: \"subnets\" must not be set when failureDomains are configured.", //nolint:lll // Message is long.
+			},
+		},
+		{
+			name:          "worker failureDomain configured with cluster/subnets set in variables overrides violates XOR",
+			clusterConfig: fakeClusterConfigSpec(true, false, false),
+			cluster:       fakeCluster(t, true, true, true, workerConfigOverridesOnly),
+			expectedErr:   true,
+			expectedErrMessages: []string{
+				"spec.topology.workers.machineDeployments.variables.overrides.workerConfig.value.nutanix.machineDetails.cluster: Forbidden: \"cluster\" must not be set when failureDomain is configured.", //nolint:lll // Message is long.
+				"spec.topology.workers.machineDeployments.variables.overrides.workerConfig.value.nutanix.machineDetails.subnets: Forbidden: \"subnets\" must not be set when failureDomain is configured.", //nolint:lll // Message is long.
+			},
+		},
+		{
+			name:                "default workerConfig variable with cluster/subnets set when control plane has failureDomains should be allowed", //nolint:lll // name is long.
+			clusterConfig:       fakeClusterConfigSpec(true, false, false),
+			cluster:             fakeCluster(t, false, true, true, workerConfigDefaultOnly),
+			expectedErr:         false,
+			expectedErrMessages: nil,
+		},
 	}
 
 	for _, tc := range testcases {