Merge pull request #8332 from killianmuldoon/pr-validate-variables-in…

…-default-webhook

🐛  Validate variables in defaulting webhook

internal/controllers/topology/cluster/cluster_controller.go

@@ -25,7 +25,6 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -221,15 +220,13 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope.Scope) (ctrl.Result
 		return ctrl.Result{}, nil
 	}
 
-	// Default and Validate the Cluster based on information from the ClusterClass.
+	// Default and Validate the Cluster variables based on information from the ClusterClass.
 	// This step is needed as if the ClusterClass does not exist at Cluster creation some fields may not be defaulted or
 	// validated in the webhook.
-	if errs := webhooks.DefaultVariables(s.Current.Cluster, clusterClass); len(errs) > 0 {
-		return ctrl.Result{}, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), s.Current.Cluster.Name, errs)
-	}
-	if errs := webhooks.ValidateClusterForClusterClass(s.Current.Cluster, clusterClass, field.NewPath("spec", "topology")); len(errs) > 0 {
+	if errs := webhooks.DefaultAndValidateVariables(s.Current.Cluster, clusterClass); len(errs) > 0 {
 		return ctrl.Result{}, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), s.Current.Cluster.Name, errs)
 	}
+
 	// Gets the blueprint with the ClusterClass and the referenced templates
 	// and store it in the request scope.
 	s.Blueprint, err = r.getBlueprint(ctx, s.Current.Cluster, s.Blueprint.ClusterClass)

internal/webhooks/cluster.go

@@ -97,7 +97,10 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error {
 			return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be defaulted. ClusterClass %s can not be retrieved", cluster.Name, cluster.Spec.Topology.Class))
 		}
 
-		allErrs = append(allErrs, DefaultVariables(cluster, clusterClass)...)
+		// Doing both defaulting and validating here prevents a race condition where the ClusterClass could be
+		// different in the defaulting and validating webhook.
+		allErrs = append(allErrs, DefaultAndValidateVariables(cluster, clusterClass)...)
+
 		if len(allErrs) > 0 {
 			return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), cluster.Name, allErrs)
 		}
@@ -256,7 +259,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 	}
 	if clusterClassPollErr == nil {
 		// If there's no error validate the Cluster based on the ClusterClass.
-		allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass, fldPath)...)
+		allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass)...)
 	}
 	if oldCluster != nil { // On update
 		// The ClusterClass must exist to proceed with update validation. Return an error if the ClusterClass was
@@ -453,6 +456,29 @@ func validateCIDRBlocks(fldPath *field.Path, cidrs []string) field.ErrorList {
 	return allErrs
 }
 
+// DefaultAndValidateVariables defaults and validates variables in the Cluster and MachineDeployment topologies based
+// on the definitions in the ClusterClass.
+func DefaultAndValidateVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList {
+	var allErrs field.ErrorList
+	allErrs = append(allErrs, DefaultVariables(cluster, clusterClass)...)
+
+	// Variables must be validated in the defaulting webhook. Variable definitions are stored in the ClusterClass status
+	// and are patched in the ClusterClass reconcile.
+	allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables,
+		field.NewPath("spec", "topology", "variables"))...)
+	if cluster.Spec.Topology.Workers != nil {
+		for i, md := range cluster.Spec.Topology.Workers.MachineDeployments {
+			// Continue if there are no variable overrides.
+			if md.Variables == nil || len(md.Variables.Overrides) == 0 {
+				continue
+			}
+			allErrs = append(allErrs, variables.ValidateMachineDeploymentVariables(md.Variables.Overrides, clusterClass.Status.Variables,
+				field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("variables", "overrides"))...)
+		}
+	}
+	return allErrs
+}
+
 // DefaultVariables defaults variables in the Cluster based on information in the ClusterClass.
 func DefaultVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList {
 	var allErrs field.ErrorList
@@ -489,7 +515,7 @@ func DefaultVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.Cluste
 }
 
 // ValidateClusterForClusterClass uses information in the ClusterClass to validate the Cluster.
-func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass, fldPath *field.Path) field.ErrorList {
+func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList {
 	var allErrs field.ErrorList
 	if cluster == nil {
 		return field.ErrorList{field.InternalError(field.NewPath(""), errors.New("Cluster can not be nil"))}
@@ -499,24 +525,8 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl
 	}
 	allErrs = append(allErrs, check.MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(cluster, clusterClass)...)
 
-	// validate the MachineHealthChecks defined in the cluster topology
+	// Validate the MachineHealthChecks defined in the cluster topology.
 	allErrs = append(allErrs, validateMachineHealthChecks(cluster, clusterClass)...)
-
-	// Check if the variables defined in the ClusterClass are valid.
-	allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables,
-		fldPath.Child("variables"))...)
-
-	if cluster.Spec.Topology.Workers != nil {
-		for i, md := range cluster.Spec.Topology.Workers.MachineDeployments {
-			// Continue if there are no variable overrides.
-			if md.Variables == nil || len(md.Variables.Overrides) == 0 {
-				continue
-			}
-			allErrs = append(allErrs, variables.ValidateMachineDeploymentVariables(md.Variables.Overrides, clusterClass.Status.Variables,
-				fldPath.Child("workers", "machineDeployments").Index(i).Child("variables", "overrides"))...)
-		}
-	}
-
 	return allErrs
 }