fixup! refactor: Handle overrides in failure domain validation

Author: jimmidyson

pkg/webhook/cluster/nutanix_validator.go

@@ -11,13 +11,15 @@ import (
 	"net/netip"
 
 	v1 "k8s.io/api/admission/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/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"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/variables"
+	commonvariables "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/clustertopology/variables"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/utils"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/helpers"
 )
@@ -220,41 +222,69 @@ func validateWorkerFailureDomainConfig(
 		"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)))
-	}
+	// Merge the global cluster variables with the worker config overrides.
+	mdVariables := commonvariables.ClusterVariablesToVariablesMap(cluster.Spec.Topology.Variables)
 
 	if cluster.Spec.Topology.Workers != nil {
 		for i := range cluster.Spec.Topology.Workers.MachineDeployments {
 			md := cluster.Spec.Topology.Workers.MachineDeployments[i]
 			hasFailureDomain := md.FailureDomain != nil && *md.FailureDomain != ""
+			wcfgPath := workerConfigVarPath
 
-			// Get the machineDetails from the overrides variable "workerConfig" if it is configured,
-			// otherwise use the defaultWorkerConfig if it is configured.
-			var workerConfig *variables.WorkerNodeConfigSpec
+			// Get the md variable overrides.
+			var mdVariableOverrides map[string]apiextensionsv1.JSON
 			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
+				mdVariableOverrides = commonvariables.ClusterVariablesToVariablesMap(md.Variables.Overrides)
+
+				// If mdVariables is nil, initialize it with mdVariableOverrides, otherwise merge global and
+				// variable overrides.
+				if mdVariables == nil {
+					mdVariables = mdVariableOverrides
+				} else {
+					// Merge global and variable overrides if global variables are present.
+					mergedVariables, err := commonvariables.MergeVariableOverridesWithGlobal(
+						mdVariableOverrides,
+						mdVariables,
+					)
+					if err != nil {
+						fldErrs = append(fldErrs, field.InternalError(
+							workerConfigMDVarOverridePath,
+							fmt.Errorf(
+								"failed to merge global and worker variable overrides for MachineDeployment %q: %w",
+								md.Name,
+								err,
+							),
+						))
+					}
+
+					mdVariables = mergedVariables
 				}
 			}
 
-			wcfgPath := workerConfigMDVarOverridePath
-			if workerConfig == nil {
-				workerConfig = defaultWorkerConfig
-				wcfgPath = workerConfigVarPath
+			workerConfigVarJSON, workerConfigPresent := mdVariables[v1alpha1.WorkerConfigVariableName]
+			if !workerConfigPresent {
+				continue
+			}
+
+			workerConfigClusterVar := clusterv1.ClusterVariable{
+				Name:  v1alpha1.WorkerConfigVariableName,
+				Value: *workerConfigVarJSON.DeepCopy(),
+			}
+
+			workerConfig := variables.WorkerNodeConfigSpec{}
+			if err := variables.UnmarshalClusterVariable(&workerConfigClusterVar, &workerConfig); err != nil {
+				fldErrs = append(fldErrs, field.InternalError(
+					workerConfigMDVarOverridePath,
+					fmt.Errorf(
+						"failed to unmarshal worker overrides variable %q: %w",
+						v1alpha1.WorkerConfigVariableName,
+						err,
+					),
+				))
 			}
-			if workerConfig == nil || workerConfig.Nutanix == nil {
+
+			if workerConfig.Nutanix == nil {
 				continue
 			}
 

pkg/webhook/cluster/nutanix_validator_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -366,6 +367,16 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
 			expectedErr:         false,
 			expectedErrMessages: nil,
 		},
+		{
+			name:          "worker cluster and subnet configured in default with failure domain in variables overrides violates XOR", //nolint:lll // name is long.
+			clusterConfig: fakeClusterConfigSpec(false, true, true),
+			cluster:       fakeCluster(t, true, false, false, workerConfigDefaultOverridesConflicting),
+			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.
+			},
+		},
 	}
 
 	for _, tc := range testcases {
@@ -432,10 +443,11 @@ func fakeClusterConfigSpec(fd, pe, subnets bool) *variables.ClusterConfigSpec {
 type workerConfigExistence string
 
 const (
-	workerConfigDefaultOnly          workerConfigExistence = "workerConfigDefaultOnly"
-	workerConfigOverridesOnly        workerConfigExistence = "workerConfigOverridesOnly"
-	workerConfigDefaultOverridesBoth workerConfigExistence = "workerConfigDefaultOverridesBoth"
-	workerConfigNone                 workerConfigExistence = "workerConfigNone"
+	workerConfigDefaultOnly                 workerConfigExistence = "workerConfigDefaultOnly"
+	workerConfigOverridesOnly               workerConfigExistence = "workerConfigOverridesOnly"
+	workerConfigDefaultOverridesBoth        workerConfigExistence = "workerConfigDefaultOverridesBoth"
+	workerConfigDefaultOverridesConflicting workerConfigExistence = "workerConfigDefaultOverridesConflicting"
+	workerConfigNone                        workerConfigExistence = "workerConfigNone"
 )
 
 func fakeCluster(t *testing.T, fd, pe, subnets bool, wcfg workerConfigExistence) *clusterv1.Cluster {
@@ -469,8 +481,8 @@ func fakeCluster(t *testing.T, fd, pe, subnets bool, wcfg workerConfigExistence)
 			MachineDetails: *fakeMachineDetails(pe, subnets),
 		},
 	}
-	workerConfigVar, err := variables.MarshalToClusterVariable("workerConfig", workerConfig)
-	assert.NoError(t, err)
+	workerConfigVar, err := variables.MarshalToClusterVariable(string(v1alpha1.WorkerConfigVariableName), workerConfig)
+	require.NoError(t, err)
 
 	switch wcfg {
 	case workerConfigDefaultOnly:
@@ -486,6 +498,26 @@ func fakeCluster(t *testing.T, fd, pe, subnets bool, wcfg workerConfigExistence)
 			cluster.Spec.Topology.Workers.MachineDeployments[0].Variables.Overrides,
 			*workerConfigVar,
 		)
+	case workerConfigDefaultOverridesConflicting:
+		// If a failure domain is defined, create a default worker config with cluster and subnet to force
+		// conflict and therefore error.
+		defaultWorkerConfig := variables.WorkerNodeConfigSpec{
+			Nutanix: &v1alpha1.NutanixWorkerNodeSpec{
+				MachineDetails: *fakeMachineDetails(fd, fd),
+			},
+		}
+
+		defaultWorkerConfigVar, err := variables.MarshalToClusterVariable(
+			string(v1alpha1.WorkerConfigVariableName),
+			defaultWorkerConfig,
+		)
+		require.NoError(t, err)
+
+		cluster.Spec.Topology.Variables = append(cluster.Spec.Topology.Variables, *defaultWorkerConfigVar)
+		cluster.Spec.Topology.Workers.MachineDeployments[0].Variables.Overrides = append(
+			cluster.Spec.Topology.Workers.MachineDeployments[0].Variables.Overrides,
+			*workerConfigVar,
+		)
 	case workerConfigNone:
 	default:
 		break