Add handling for variables with conflicting definitions

Author: killianmuldoon

internal/controllers/clusterclass/clusterclass_controller.go

@@ -19,6 +19,8 @@ package clusterclass
 import (
 	"context"
 	"fmt"
+	"reflect"
+	"sort"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -213,12 +215,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{}
+	allVariableDefinitions := 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))
+		allVariableDefinitions[variable.Name] = addNewStatusVariable(variable, clusterv1.VariableDefinitionFromInline)
 	}
 
 	// If RuntimeSDK is enabled call the DiscoverVariables hook for all associated Runtime Extensions and add the variables
@@ -242,14 +242,23 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
 				continue
 			}
 			if resp.Variables != nil {
+				uniqueNamesForPatch := sets.Set[string]{}
 				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))
+					// Ensure a patch doesn't define multiple variables with the same name.
+					if uniqueNamesForPatch.Has(variable.Name) {
+						errs = append(errs, errors.Errorf("variable %q is defined multiple times in variable discovery response from patch %q", variable.Name, patch.Name))
 						continue
 					}
-					uniqueVars[variable.Name] = true
-					allVars = append(allVars, statusVariableFromClusterClassVariable(variable, patch.Name))
+					uniqueNamesForPatch.Insert(variable.Name)
+
+					// If a variable of the same name already exists in allVariableDefinitions add the new definition to the existing list.
+					if _, ok := allVariableDefinitions[variable.Name]; ok {
+						allVariableDefinitions[variable.Name] = addDefinitionToExistingStatusVariable(variable, patch.Name, allVariableDefinitions[variable.Name])
+						continue
+					}
+
+					// Add the new variable to the list.
+					allVariableDefinitions[variable.Name] = addNewStatusVariable(variable, patch.Name)
 				}
 			}
 		}
@@ -260,10 +269,22 @@ 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 allVariableDefinitions {
+		statusVarList = append(statusVarList, *variable)
+	}
+	// Alphabetically sort the variables by name. This ensures no unnecessary updates to the ClusterClass status.
+	// Note: Definitions in `statusVarList[i].Definitions` have a stable order as they are added in a deterministic order
+	// and are always held in an array.
+	sort.SliceStable(statusVarList, func(i, j int) bool {
+		return statusVarList[i].Name < statusVarList[j].Name
+	})
+	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 +309,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 +322,26 @@ 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 {
+		currentDefinition := combinedVariable.Definitions[0]
+		if !(currentDefinition.Required == newVariableDefinition.Required && reflect.DeepEqual(currentDefinition.Schema, newVariableDefinition.Schema)) {
+			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/clusterclass/clusterclass_controller_test.go

@@ -23,14 +23,21 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/gomega"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/component-base/featuregate/testing"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
+	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
+	"sigs.k8s.io/cluster-api/feature"
 	tlog "sigs.k8s.io/cluster-api/internal/log"
+	fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
 )
 
@@ -308,3 +315,233 @@ func isOwnerReferenceEqual(a, b metav1.OwnerReference) bool {
 	}
 	return true
 }
+
+func TestReconciler_reconcileVariables(t *testing.T) {
+	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()
+
+	g := NewWithT(t)
+	catalog := runtimecatalog.New()
+	_ = runtimehooksv1.AddToCatalog(catalog)
+
+	clusterClassWithInlineVariables := builder.ClusterClass(metav1.NamespaceDefault, "class1").
+		WithVariables(
+			[]clusterv1.ClusterClassVariable{
+				{
+					Name: "cpu",
+					Schema: clusterv1.VariableSchema{
+						OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+							Type: "integer",
+						},
+					},
+				},
+				{
+					Name: "memory",
+					Schema: clusterv1.VariableSchema{
+						OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+							Type: "string",
+						},
+					},
+				},
+			}...,
+		)
+	tests := []struct {
+		name          string
+		clusterClass  *clusterv1.ClusterClass
+		want          []clusterv1.ClusterClassStatusVariable
+		patchResponse *runtimehooksv1.DiscoverVariablesResponse
+		wantErr       bool
+	}{
+		{
+			name:         "Reconcile inline variables to ClusterClass status",
+			clusterClass: clusterClassWithInlineVariables.DeepCopy().Build(),
+			want: []clusterv1.ClusterClassStatusVariable{
+				{
+					Name: "cpu",
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: clusterv1.VariableDefinitionFromInline,
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "integer",
+								},
+							},
+						},
+					},
+				},
+				{
+					Name: "memory",
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: clusterv1.VariableDefinitionFromInline,
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "string",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "Reconcile variables from inline and external variables to ClusterClass status",
+			clusterClass: clusterClassWithInlineVariables.DeepCopy().WithPatches(
+				[]clusterv1.ClusterClassPatch{
+					{
+						Name: "patch1",
+						External: &clusterv1.ExternalPatchDefinition{
+							DiscoverVariablesExtension: pointer.String("variables-one"),
+						}}}).
+				Build(),
+			patchResponse: &runtimehooksv1.DiscoverVariablesResponse{
+				CommonResponse: runtimehooksv1.CommonResponse{
+					Status: runtimehooksv1.ResponseStatusSuccess,
+				},
+				Variables: []clusterv1.ClusterClassVariable{
+					{
+						Name: "cpu",
+						Schema: clusterv1.VariableSchema{
+							OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+								Type: "string",
+							},
+						},
+					},
+					{
+						Name: "memory",
+						Schema: clusterv1.VariableSchema{
+							OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+								Type: "string",
+							},
+						},
+					},
+					{
+						Name: "location",
+						Schema: clusterv1.VariableSchema{
+							OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+								Type: "string",
+							},
+						},
+					},
+				},
+			},
+			want: []clusterv1.ClusterClassStatusVariable{
+				{
+					Name:                "cpu",
+					DefinitionsConflict: true,
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: clusterv1.VariableDefinitionFromInline,
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "integer",
+								},
+							},
+						},
+						{
+							From: "patch1",
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "string",
+								},
+							},
+						},
+					},
+				},
+				{
+					Name:                "location",
+					DefinitionsConflict: false,
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: "patch1",
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "string",
+								},
+							},
+						},
+					},
+				},
+				{
+					Name:                "memory",
+					DefinitionsConflict: false,
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: clusterv1.VariableDefinitionFromInline,
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "string",
+								},
+							},
+						},
+						{
+							From: "patch1",
+							Schema: clusterv1.VariableSchema{
+								OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+									Type: "string",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name:    "Error if external patch defines a variable with same name multiple times",
+			wantErr: true,
+			clusterClass: clusterClassWithInlineVariables.DeepCopy().WithPatches(
+				[]clusterv1.ClusterClassPatch{
+					{
+						Name: "patch1",
+						External: &clusterv1.ExternalPatchDefinition{
+							DiscoverVariablesExtension: pointer.String("variables-one"),
+						}}}).
+				Build(),
+			patchResponse: &runtimehooksv1.DiscoverVariablesResponse{
+				CommonResponse: runtimehooksv1.CommonResponse{
+					Status: runtimehooksv1.ResponseStatusSuccess,
+				},
+				Variables: []clusterv1.ClusterClassVariable{
+					{
+						Name: "cpu",
+						Schema: clusterv1.VariableSchema{
+							OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+								Type: "string",
+							},
+						},
+					},
+					{
+						Name: "cpu",
+						Schema: clusterv1.VariableSchema{
+							OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+								Type: "integer",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fakeRuntimeClient := fakeruntimeclient.NewRuntimeClientBuilder().
+				WithCallExtensionResponses(
+					map[string]runtimehooksv1.ResponseObject{
+						"variables-one": tt.patchResponse,
+					}).
+				WithCatalog(catalog).
+				Build()
+
+			r := &Reconciler{
+				RuntimeClient: fakeRuntimeClient,
+			}
+
+			err := r.reconcileVariables(ctx, tt.clusterClass)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				return
+			}
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(tt.clusterClass.Status.Variables).To(Equal(tt.want), cmp.Diff(tt.clusterClass.Status.Variables, tt.want))
+		})
+	}
+}

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))
 		})
 	}
 }