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{}
-	allVars := []clusterv1.ClusterClassStatusVariable{}
+	allVars := map[string]*clusterv1.ClusterClassStatusVariable{}
 	// Add inline variable definitions to the ClusterClass status.
 	for _, variable := range clusterClass.Spec.Variables {
-		uniqueVars[variable.Name] = true
-		allVars = append(allVars, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline))
+		allVars[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 := allVars[variable.Name]; ok {
+						allVars[variable.Name] = addDefinitionToExistingStatusVariable(variable, patch.Name, allVars[variable.Name])
 						continue
 					}
-					uniqueVars[variable.Name] = true
-					allVars = append(allVars, statusVariableFromClusterClassVariable(variable, patch.Name))
+					allVars[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 = allVars
+	statusVarList := []clusterv1.ClusterClassStatusVariable{}
+	for _, variable := range allVars {
+		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 {
-	return clusterv1.ClusterClassStatusVariable{
-		Name: variable.Name,
-		// TODO: In a future iteration this should be true where definitions are equal.
+func addNewStatusVariable(variable clusterv1.ClusterClassVariable, from string) *clusterv1.ClusterClassStatusVariable {
+	return &clusterv1.ClusterClassStatusVariable{
+		Name:                variable.Name,
 		DefinitionsConflict: false,
 		Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
 			{
@@ -302,6 +304,24 @@ 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 definitions already conflict, no need to check.
+	if !combinedVariable.DefinitionsConflict &&
+		!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: ""}).
 					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,54 +30,50 @@ 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(values []clusterv1.ClusterVariable, definitions []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.
+	valuesMap, err := getValuesMap(values)
+	if err != nil {
+		return nil, append(allErrs, field.Invalid(fldPath, values,
+			fmt.Sprintf("cluster variables not valid: %s", err)))
 	}
 
-	// allVariables is used to get a full correctly ordered list of variables.
-	allVariables := []string{}
-	// Add any ClusterVariables that already exist.
-	for _, variable := range clusterVariables {
-		allVariables = append(allVariables, variable.Name)
-	}
-	// 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 {
+	// Get an index for each variable name and definition.
+	defIndex := newDefinitionsIndex(definitions)
+
+	// Get a correctly ordered list of all variables defined in both the Cluster and the ClusterClass.
+	allVariables := getAllVariables(values, valuesMap, definitions)
+
+	// Default all variables.
+	defaultedValues := []clusterv1.ClusterVariable{}
+	uniqueDefaults := map[string]bool{}
+	for i, variable := range allVariables {
+		// Get the variable definition from the ClusterClass. If the variable is not defined add an error.
+		definition, err := defIndex.get(variable.Name, variable.DefinitionFrom)
+		if err != nil {
+			allErrs = append(allErrs, field.Required(fldPath.Index(i).Child("name"), err.Error()))
 			continue
 		}
 
-		allVariables = append(allVariables, variable.Name)
-	}
+		// Get the current value of the variable if it is defined in the Cluster spec.
+		currentValue := getCurrentValue(variable, valuesMap)
 
-	// Default all variables.
-	defaultedClusterVariables := []clusterv1.ClusterVariable{}
-	for i, variableName := range allVariables {
-		clusterClassVariable := clusterClassVariablesMap[variableName]
-		clusterVariable := clusterVariablesMap[variableName]
+		// Default the variable.
+		defaultedValue, errs := defaultValue(currentValue, definition, fldPath.Index(i), createVariables)
 
-		defaultedClusterVariable, errs := defaultClusterVariable(clusterVariable, clusterClassVariable, fldPath.Index(i), createVariables)
 		if len(errs) > 0 {
 			allErrs = append(allErrs, errs...)
 			continue
@@ -86,83 +82,172 @@ func defaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clust
 		// 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 {
+		if defaultedValue == nil {
 			continue
 		}
 
-		defaultedClusterVariables = append(defaultedClusterVariables, *defaultedClusterVariable)
+		// Set the definitionFrom field for the defaulted value.
+		defaultedValue = defaultDefinitionFrom(defaultedValue, definition, valuesMap)
+
+		// Only append the variable if it has not already been defaulted.
+		defaultedValues = appendIfUnique(defaultedValue, defaultedValues, uniqueDefaults)
 	}
 
 	if len(allErrs) > 0 {
 		return nil, allErrs
 	}
+	return defaultedValues, nil
+}
+
+func appendIfUnique(defaultedValue *clusterv1.ClusterVariable, defaultedValues []clusterv1.ClusterVariable, uniqueDefaults map[string]bool) []clusterv1.ClusterVariable {
+	if _, ok := uniqueDefaults[fmt.Sprintf("%s/%s", defaultedValue.DefinitionFrom, defaultedValue.Name)]; !ok {
+		uniqueDefaults[fmt.Sprintf("%s/%s", defaultedValue.DefinitionFrom, defaultedValue.Name)] = true
+		defaultedValues = append(defaultedValues, *defaultedValue)
+	}
+	return defaultedValues
+}
+
+// getCurrentValue returns the value of a variable for its definitionFrom, or for an empty definitionFrom if it exists.
+func getCurrentValue(variable clusterv1.ClusterVariable, valuesMap map[string]map[string]clusterv1.ClusterVariable) *clusterv1.ClusterVariable {
+	var currentValue *clusterv1.ClusterVariable
+	// If the value is set in the Cluster spec get the value.
+	if valuesWithName, ok := valuesMap[variable.Name]; ok {
+		if value, ok := valuesWithName[variable.DefinitionFrom]; ok {
+			currentValue = &value
+		}
+		if value, ok := valuesWithName[emptyVariableDefinitionFrom]; ok {
+			currentValue = &value
+		}
+	}
+	return currentValue
+}
+
+// defaultDefinitionFrom sets the definitionFrom field of a variable based on the variable definition and values.
+func defaultDefinitionFrom(defaultedValue *clusterv1.ClusterVariable, definition *statusVariableDefinition, values map[string]map[string]clusterv1.ClusterVariable) *clusterv1.ClusterVariable {
+	// If definitionFrom is non-empty don't make any changes.
+	if defaultedValue.DefinitionFrom != emptyVariableDefinitionFrom {
+		return defaultedValue
+	}
+
+	// If there are conflicts set the definitionFrom field.
+	if definition.conflicts {
+		defaultedValue.DefinitionFrom = definition.From
+		return defaultedValue
+	}
 
-	return defaultedClusterVariables, nil
+	// If a value with the same name and a different non-empty definitionsFrom is in the Cluster, set the definitionFrom field.
+	if valuesForName, ok := values[definition.name]; ok {
+		for _, value := range valuesForName {
+			if value.DefinitionFrom != definition.From && value.DefinitionFrom != emptyVariableDefinitionFrom {
+				defaultedValue.DefinitionFrom = definition.From
+				return defaultedValue
+			}
+		}
+	}
+	return defaultedValue
 }
 
-// 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) {
-	if clusterVariable == nil {
+// defaultValue defaults a clusterVariable based on the default value in the clusterClassVariable.
+func defaultValue(currentValue *clusterv1.ClusterVariable, definition *statusVariableDefinition, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) {
+	if currentValue == nil {
 		// Return if the variable does not exist yet and createVariable is false.
 		if !createVariable {
 			return nil, nil
 		}
 		// Return if the variable does not exist yet and there is no top-level default value.
-		if clusterClassVariable.Schema.OpenAPIV3Schema.Default == nil {
+		if definition.Schema.OpenAPIV3Schema.Default == nil {
 			return nil, nil
 		}
 	}
 
 	// Convert schema to Kubernetes APIExtensions schema.
-	apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(&clusterClassVariable.Schema.OpenAPIV3Schema, field.NewPath("schema"))
+	apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(&definition.Schema.OpenAPIV3Schema, field.NewPath("schema"))
 	if len(errs) > 0 {
 		return nil, field.ErrorList{field.Invalid(fldPath, "",
-			fmt.Sprintf("invalid schema in ClusterClass for variable %q: error to convert schema %v", clusterClassVariable.Name, errs))}
+			fmt.Sprintf("invalid schema in ClusterClass for variable %q: error to convert schema %v", definition.name, errs))}
 	}
 
 	var value interface{}
 	// If the variable already exists, parse the current value.
-	if clusterVariable != nil && len(clusterVariable.Value.Raw) > 0 {
-		if err := json.Unmarshal(clusterVariable.Value.Raw, &value); err != nil {
+	if currentValue != nil && len(currentValue.Value.Raw) > 0 {
+		if err := json.Unmarshal(currentValue.Value.Raw, &value); err != nil {
 			return nil, field.ErrorList{field.Invalid(fldPath, "",
-				fmt.Sprintf("failed to unmarshal variable value %q: %v", string(clusterVariable.Value.Raw), err))}
+				fmt.Sprintf("failed to unmarshal variable value %q: %v", string(currentValue.Value.Raw), err))}
 		}
 	}
 
 	// Structural schema defaulting does not work with scalar values,
 	// so we wrap the schema and the variable in objects.
 	// <variable-name>: <variable-value>
 	wrappedVariable := map[string]interface{}{
-		clusterClassVariable.Name: value,
+		definition.name: value,
 	}
 	// type: object
 	// properties:
 	//   <variable-name>: <variable-schema>
 	wrappedSchema := &apiextensions.JSONSchemaProps{
 		Type: "object",
 		Properties: map[string]apiextensions.JSONSchemaProps{
-			clusterClassVariable.Name: *apiExtensionsSchema,
+			definition.name: *apiExtensionsSchema,
 		},
 	}
 
 	// Default the variable via the structural schema library.
 	ss, err := structuralschema.NewStructural(wrappedSchema)
 	if err != nil {
 		return nil, field.ErrorList{field.Invalid(fldPath, "",
-			fmt.Sprintf("failed defaulting variable %q: %v", clusterVariable.Name, err))}
+			fmt.Sprintf("failed defaulting variable %q: %v", currentValue.Name, err))}
 	}
 	structuraldefaulting.Default(wrappedVariable, ss)
 
 	// Marshal the defaulted value.
-	defaultedVariableValue, err := json.Marshal(wrappedVariable[clusterClassVariable.Name])
+	defaultedVariableValue, err := json.Marshal(wrappedVariable[definition.name])
 	if err != nil {
 		return nil, field.ErrorList{field.Invalid(fldPath, "",
-			fmt.Sprintf("failed to marshal default value of variable %q: %v", clusterClassVariable.Name, err))}
+			fmt.Sprintf("failed to marshal default value of variable %q: %v", definition.name, err))}
 	}
-	return &clusterv1.ClusterVariable{
-		Name: clusterClassVariable.Name,
+	v := &clusterv1.ClusterVariable{
+		Name: definition.name,
 		Value: apiextensionsv1.JSON{
 			Raw: defaultedVariableValue,
 		},
-	}, nil
+	}
+
+	// If the currentValue exists set definitionFrom on the defaulted value
+	if currentValue != nil {
+		v.DefinitionFrom = currentValue.DefinitionFrom
+	}
+
+	return v, nil
+}
+
+// getAllVariables returns a correctly ordered list of all variables defined in the ClusterClass and the Cluster.
+func getAllVariables(values []clusterv1.ClusterVariable, valuesMap map[string]map[string]clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable) []clusterv1.ClusterVariable {
+	// allVariables is used to get a full correctly ordered list of variables.
+	allVariables := []clusterv1.ClusterVariable{}
+
+	// Add any values that already exist.
+	allVariables = append(allVariables, values...)
+
+	// Add variables from the ClusterClass, which currently don't exist on the Cluster.
+	for _, variable := range definitions {
+		for _, definition := range variable.Definitions {
+			if _, ok := valuesMap[variable.Name]; ok {
+				// Continue if the Cluster variable with a definitionFrom this ClusterClass variable exists.
+				if _, ok := valuesMap[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 := valuesMap[variable.Name][emptyVariableDefinitionFrom]; ok {
+					continue
+				}
+			}
+			allVariables = append(allVariables, clusterv1.ClusterVariable{
+				Name:           variable.Name,
+				DefinitionFrom: definition.From,
+			})
+		}
+	}
+	return allVariables
 }