Merge pull request kubernetes-sigs#10812 from sbueringer/pr-cel-cc-controller

✨ Add variable validation to ClusterClass controller, block Cluster reconcile if variables not reconciled

Author: k8s-ci-robot

cmd/clusterctl/client/cluster/topology.go

@@ -140,15 +140,18 @@ func (t *topologyClient) Plan(ctx context.Context, in *TopologyPlanInput) (*Topo
 	// This mimics the defaulting and validation webhooks that will run on the objects during a real execution.
 	// Running defaulting and validation on these objects helps to improve the UX of using the plan operation.
 	// This is especially important when working with Clusters and ClusterClasses that use variable and patches.
-	if err := t.runDefaultAndValidationWebhooks(ctx, in, c); err != nil {
+	reconciledClusterClasses, err := t.runDefaultAndValidationWebhooks(ctx, in, c)
+	if err != nil {
 		return nil, errors.Wrap(err, "failed defaulting and validation on input objects")
 	}
 
 	objs := []client.Object{}
 	// Add all the objects from the input to the list used when initializing the dry run client.
-	for _, o := range in.Objs {
+	for _, o := range filterObjects(in.Objs, clusterv1.GroupVersion.WithKind("ClusterClass")) {
 		objs = append(objs, o)
 	}
+	// Note: We have to add the reconciled ClusterClasses, because the Cluster reconciler depends on that.
+	objs = append(objs, reconciledClusterClasses...)
 	// Add mock CRDs of all the provider objects in the input to the list used when initializing the dry run client.
 	// Adding these CRDs makes sure that UpdateReferenceAPIContract calls in the reconciler can work.
 	for _, o := range t.generateCRDs(in.Objs) {
@@ -362,11 +365,11 @@ func (t *topologyClient) prepareClusters(ctx context.Context, clusters []*unstru
 // ValidateCreate is performed.
 // *Important Note*: We cannot perform defaulting and validation on provider objects as we do not have access to
 // that code.
-func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in *TopologyPlanInput, apiReader client.Reader) error {
+func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in *TopologyPlanInput, apiReader client.Reader) ([]client.Object, error) {
 	// Enable the ClusterTopology feature gate so that the defaulter and validators do not complain.
 	// Note: We don't need to disable it later because the CLI is short lived.
 	if err := feature.Gates.(featuregate.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", feature.ClusterTopology, true)); err != nil {
-		return errors.Wrapf(err, "failed to enable %s feature gate", feature.ClusterTopology)
+		return nil, errors.Wrapf(err, "failed to enable %s feature gate", feature.ClusterTopology)
 	}
 
 	// From the inputs gather all the objects that are not Clusters or ClusterClasses.
@@ -395,7 +398,7 @@ func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in
 		ccWebhook,
 		apiReader,
 	); err != nil {
-		return errors.Wrap(err, "failed to run defaulting and validation on ClusterClasses")
+		return nil, errors.Wrap(err, "failed to run defaulting and validation on ClusterClasses")
 	}
 
 	// From the inputs gather all the objects that are not Clusters or ClusterClasses.
@@ -416,7 +419,7 @@ func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in
 	// during ClusterClass reconciliation.
 	reconciledClusterClasses, err := t.reconcileClusterClasses(ctx, in.Objs, apiReader)
 	if err != nil {
-		return errors.Wrapf(err, "failed to reconcile ClusterClasses for defaulting and validating")
+		return nil, errors.Wrapf(err, "failed to reconcile ClusterClasses for defaulting and validating")
 	}
 	objs = append(objs, reconciledClusterClasses...)
 
@@ -432,10 +435,10 @@ func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in
 		clusterWebhook,
 		apiReader,
 	); err != nil {
-		return errors.Wrap(err, "failed to run defaulting and validation on Clusters")
+		return nil, errors.Wrap(err, "failed to run defaulting and validation on Clusters")
 	}
 
-	return nil
+	return reconciledClusterClasses, nil
 }
 
 func (t *topologyClient) reconcileClusterClasses(ctx context.Context, inputObjects []*unstructured.Unstructured, apiReader client.Reader) ([]client.Object, error) {

internal/controllers/clusterclass/clusterclass_controller.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -45,6 +46,7 @@ import (
 	"sigs.k8s.io/cluster-api/feature"
 	tlog "sigs.k8s.io/cluster-api/internal/log"
 	runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client"
+	"sigs.k8s.io/cluster-api/internal/topology/variables"
 	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/conversion"
@@ -245,15 +247,13 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
 				continue
 			}
 			if resp.Variables != nil {
-				uniqueNamesForPatch := sets.Set[string]{}
-				for _, variable := range resp.Variables {
-					// 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
-					}
-					uniqueNamesForPatch.Insert(variable.Name)
+				validationErrors := variables.ValidateClusterClassVariables(ctx, nil, resp.Variables, field.NewPath(patch.Name, "variables")).ToAggregate()
+				if validationErrors != nil {
+					errs = append(errs, validationErrors)
+					continue
+				}
 
+				for _, variable := range resp.Variables {
 					// 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])