Add handling for variables with conflicting definitions

internal/controllers/clusterclass/clusterclass_controller.go

@@ -19,6 +19,7 @@ package clusterclass
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -213,12 +214,10 @@ func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterCla
 
 func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clusterv1.ClusterClass) error {
 	errs := []error{}
-	uniqueVars := map[string]bool{}
-	ccStatusVars := []clusterv1.ClusterClassStatusVariable{}
+	ccStatusVars := map[string]clusterv1.ClusterClassStatusVariable{}
 	// Add inline variable definitions to the ClusterClass status.
 	for _, variable := range clusterClass.Spec.Variables {
-		uniqueVars[variable.Name] = true
-		ccStatusVars = append(ccStatusVars, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline))
+		ccStatusVars[variable.Name] = addNewStatusVariable(variable, clusterv1.VariableDefinitionFromInline)
 	}
 
 	// If RuntimeSDK is enabled call the DiscoverVariables hook for all associated Runtime Extensions and add the variables
@@ -243,13 +242,12 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
 			}
 			if resp.Variables != nil {
 				for _, variable := range resp.Variables {
-					// TODO: Variables should be validated and deduplicated based on their provided definition.
-					if _, ok := uniqueVars[variable.Name]; ok {
-						errs = append(errs, errors.Errorf("duplicate variable name %s discovered in patch: %s", variable.Name, patch.Name))
+					// If a variable of the same name already exists add it to the existing definition list.
+					if _, ok := ccStatusVars[variable.Name]; ok {
+						ccStatusVars[variable.Name] = addDefinitiontoExistingStatusVariable(variable, patch.Name, ccStatusVars[variable.Name])
 						continue
 					}
-					uniqueVars[variable.Name] = true
-					ccStatusVars = append(ccStatusVars, statusVariableFromClusterClassVariable(variable, patch.Name))
+					ccStatusVars[variable.Name] = addNewStatusVariable(variable, patch.Name)
 				}
 			}
 		}
@@ -260,10 +258,15 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
 			"VariableDiscovery failed: %s", kerrors.NewAggregate(errs))
 		return errors.Wrapf(kerrors.NewAggregate(errs), "failed to discover variables for ClusterClass %s", clusterClass.Name)
 	}
-	clusterClass.Status.Variables = ccStatusVars
+	statusVarList := []clusterv1.ClusterClassStatusVariable{}
+	for _, variable := range ccStatusVars {
+		statusVarList = append(statusVarList, variable)
+	}
+	clusterClass.Status.Variables = statusVarList
 	conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition)
 	return nil
 }
+
 func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[*corev1.ObjectReference]*corev1.ObjectReference) {
 	if len(outdatedRefs) > 0 {
 		var msg []string
@@ -288,10 +291,9 @@ func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[
 	)
 }
 
-func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVariable, from string) clusterv1.ClusterClassStatusVariable {
+func addNewStatusVariable(variable clusterv1.ClusterClassVariable, from string) clusterv1.ClusterClassStatusVariable {
 	return clusterv1.ClusterClassStatusVariable{
-		Name: variable.Name,
-		// TODO: In a future iteration this should be true where definitions are equal.
+		Name:                variable.Name,
 		DefinitionsConflict: false,
 		Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
 			{
@@ -302,6 +304,22 @@ func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVaria
 		}}
 }
 
+func addDefinitiontoExistingStatusVariable(variable clusterv1.ClusterClassVariable, from string, existingVariable clusterv1.ClusterClassStatusVariable) clusterv1.ClusterClassStatusVariable {
+	combinedVariable := existingVariable.DeepCopy()
+	newVariableDefinition := clusterv1.ClusterClassStatusVariableDefinition{
+		From:     from,
+		Required: variable.Required,
+		Schema:   variable.Schema,
+	}
+	combinedVariable.Definitions = append(existingVariable.Definitions, newVariableDefinition)
+
+	// If the new definition is different from any existing definition, set DefinitionsConflict to true.
+	if !reflect.DeepEqual(combinedVariable.Definitions[0], newVariableDefinition) {
+		combinedVariable.DefinitionsConflict = true
+	}
+	return *combinedVariable
+}
+
 func refString(ref *corev1.ObjectReference) string {
 	return fmt.Sprintf("%s %s/%s", ref.GroupVersionKind().String(), ref.Namespace, ref.Name)
 }

internal/controllers/topology/cluster/cluster_controller_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -1114,7 +1115,7 @@ func TestReconciler_DefaultCluster(t *testing.T) {
 				Build(),
 			wantCluster: clusterBuilder.DeepCopy().
 				WithTopology(topologyBase.DeepCopy().WithVariables(
-					clusterv1.ClusterVariable{Name: "location", Value: apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}}).
+					clusterv1.ClusterVariable{Name: "location", Value: apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, DefinitionFrom: clusterv1.VariableDefinitionFromInline}).
 					Build()).
 				Build(),
 		},
@@ -1234,7 +1235,7 @@ func TestReconciler_DefaultCluster(t *testing.T) {
 			got := &clusterv1.Cluster{}
 			g.Expect(fakeClient.Get(ctx, client.ObjectKey{Name: tt.initialCluster.Name, Namespace: tt.initialCluster.Namespace}, got)).To(Succeed())
 			// Compare the spec of the two clusters to ensure that variables are defaulted correctly.
-			g.Expect(reflect.DeepEqual(got.Spec, tt.wantCluster.Spec)).To(BeTrue())
+			g.Expect(reflect.DeepEqual(got.Spec, tt.wantCluster.Spec)).To(BeTrue(), cmp.Diff(got.Spec, tt.wantCluster.Spec))
 		})
 	}
 }

internal/topology/variables/cluster_variable_defaulting.go

@@ -30,66 +30,89 @@ import (
 )
 
 // DefaultClusterVariables defaults ClusterVariables.
-func DefaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) {
+func DefaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) {
 	return defaultClusterVariables(clusterVariables, clusterClassVariables, true, fldPath)
 }
 
 // DefaultMachineDeploymentVariables defaults MachineDeploymentVariables.
-func DefaultMachineDeploymentVariables(machineDeploymentVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) {
+func DefaultMachineDeploymentVariables(machineDeploymentVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) {
 	return defaultClusterVariables(machineDeploymentVariables, clusterClassVariables, false, fldPath)
 }
 
 // defaultClusterVariables defaults variables.
 // If they do not exist yet, they are created if createVariables is set.
-func defaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, createVariables bool, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) {
+func defaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassStatusVariable, createVariables bool, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) {
 	var allErrs field.ErrorList
 
-	// Build maps for easier and faster access.
-	clusterVariablesMap := getClusterVariablesMap(clusterVariables)
-	clusterClassVariablesMap := getClusterClassVariablesMap(clusterClassVariables)
-
-	// Validate that all variables in the Cluster are defined in the ClusterClass.
-	// Note: If we don't validate this, we would get a nil pointer dereference below.
-	allErrs = append(allErrs, validateClusterVariablesDefined(clusterVariables, clusterClassVariablesMap, fldPath)...)
-	if len(allErrs) > 0 {
-		return nil, allErrs
+	// Get a map of ClusterVariables and ensure that variables are not defined more than once in Cluster spec.
+	clusterVariablesMap, err := getClusterVariablesMap(clusterVariables)
+	if err != nil {
+		return nil, append(allErrs, field.Invalid(fldPath, clusterVariables,
+			fmt.Sprintf("cluster variables not valid: %s", err)))
 	}
 
+	// Get a map of ClusterClassVariables for each variable name and definition.
+	clusterClassVariablesMap := getClusterClassVariablesMap(clusterClassVariables)
+
 	// allVariables is used to get a full correctly ordered list of variables.
-	allVariables := []string{}
+	allVariables := []clusterv1.ClusterVariable{}
 	// Add any ClusterVariables that already exist.
-	for _, variable := range clusterVariables {
-		allVariables = append(allVariables, variable.Name)
-	}
+	allVariables = append(allVariables, clusterVariables...)
+
 	// Add variables from the ClusterClass, which currently don't exist on the Cluster.
 	for _, variable := range clusterClassVariables {
-		// Continue if the ClusterClass variable already exists.
-		if _, ok := clusterVariablesMap[variable.Name]; ok {
-			continue
+		for _, definition := range variable.Definitions {
+			if _, ok := clusterVariablesMap[variable.Name]; ok {
+				// Continue if the Cluster variable with a definitionFrom this ClusterClass variable exists.
+				if _, ok := clusterVariablesMap[variable.Name][definition.From]; ok {
+					continue
+				}
+				// Continue if the Cluster variable with an empty definitionFrom exists. The user intention here is to
+				// use the default value from a ClusterClass variable with no conflicting variables.
+				if _, ok := clusterVariablesMap[variable.Name][emptyVariableDefinitionFrom]; ok {
+					continue
+				}
+			}
+			allVariables = append(allVariables, clusterv1.ClusterVariable{
+				Name:           variable.Name,
+				DefinitionFrom: definition.From,
+			})
 		}
-
-		allVariables = append(allVariables, variable.Name)
 	}
 
 	// Default all variables.
 	defaultedClusterVariables := []clusterv1.ClusterVariable{}
-	for i, variableName := range allVariables {
-		clusterClassVariable := clusterClassVariablesMap[variableName]
-		clusterVariable := clusterVariablesMap[variableName]
+	for i, variable := range allVariables {
+		clusterClassVariable, found := clusterClassVariablesMap[ccVariableMapKey{name: variable.Name, from: variable.DefinitionFrom}]
+		if !found {
+			allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("name"), variable.Name,
+				fmt.Sprintf("variable with name %q from %q is not defined in ClusterClass `status.Variables`.", variable.Name, variable.DefinitionFrom)))
+			continue
+		}
 
-		defaultedClusterVariable, errs := defaultClusterVariable(clusterVariable, clusterClassVariable, fldPath.Index(i), createVariables)
+		// If the variable is defined in the Cluster spec get the definition.
+		var clusterVariable *clusterv1.ClusterVariable
+		if variablesWithName, ok := clusterVariablesMap[variable.Name]; ok {
+			if variableFromDefinition, ok := variablesWithName[variable.DefinitionFrom]; ok {
+				clusterVariable = &variableFromDefinition
+			}
+		}
+
+		defaultedClusterVariable, errs := defaultClusterVariable(clusterVariable, &clusterv1.ClusterClassVariable{
+			Name:     variable.Name,
+			Required: clusterClassVariable.Required,
+			Schema:   clusterClassVariable.Schema,
+		}, clusterClassVariable.From, fldPath.Index(i), createVariables)
 		if len(errs) > 0 {
 			allErrs = append(allErrs, errs...)
 			continue
 		}
-
 		// Continue if there is no defaulted variable.
 		// NOTE: This happens when the variable doesn't exist on the CLuster before and
 		// there is no top-level default value.
 		if defaultedClusterVariable == nil {
 			continue
 		}
-
 		defaultedClusterVariables = append(defaultedClusterVariables, *defaultedClusterVariable)
 	}
 
@@ -101,7 +124,7 @@ func defaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clust
 }
 
 // defaultClusterVariable defaults a clusterVariable based on the default value in the clusterClassVariable.
-func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterClassVariable *clusterv1.ClusterClassVariable, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) {
+func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterClassVariable *clusterv1.ClusterClassVariable, definitionFrom string, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) {
 	if clusterVariable == nil {
 		// Return if the variable does not exist yet and createVariable is false.
 		if !createVariable {
@@ -159,10 +182,12 @@ func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterC
 		return nil, field.ErrorList{field.Invalid(fldPath, "",
 			fmt.Sprintf("failed to marshal default value of variable %q: %v", clusterClassVariable.Name, err))}
 	}
-	return &clusterv1.ClusterVariable{
+	v := &clusterv1.ClusterVariable{
 		Name: clusterClassVariable.Name,
 		Value: apiextensionsv1.JSON{
 			Raw: defaultedVariableValue,
 		},
-	}, nil
+		DefinitionFrom: definitionFrom,
+	}
+	return v, nil
 }