Merge pull request #8153 from killianmuldoon/pr-exvars-weaker-variable-validation-clusterclass-update

🌱 Weaken ClusterClass webhook variable validation on update

Author: k8s-ci-robot

internal/webhooks/clusterclass.go

@@ -19,7 +19,6 @@ package webhooks
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -176,10 +175,6 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC
 		// Ensure no MachineHealthCheck currently in use has been removed from the ClusterClass.
 		allErrs = append(allErrs,
 			validateUpdatesToMachineHealthCheckClasses(clusters, oldClusterClass, newClusterClass)...)
-
-		// Ensure no Variable would be invalidated by the update in spec
-		allErrs = append(allErrs,
-			validateVariableUpdates(clusters, oldClusterClass, newClusterClass, field.NewPath("spec", "variables"))...)
 	}
 
 	if len(allErrs) > 0 {
@@ -188,118 +183,6 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC
 	return nil
 }
 
-// validateVariableUpdates checks if the updates made to ClusterClassVariables are valid.
-// It retrieves a list of variables of interest and validates:
-// 1) Altered ClusterClassVariables defined on any exiting Cluster are still valid with the updated Schema.
-// 2) Removed ClusterClassVariables are not in use on any Cluster using the ClusterClass.
-// 3) Added ClusterClassVariables defined on any exiting Cluster are still valid with the updated Schema.
-// 4) Required ClusterClassVariables are defined on each Cluster using the ClusterClass.
-func validateVariableUpdates(clusters []clusterv1.Cluster, oldClusterClass, newClusterClass *clusterv1.ClusterClass, path *field.Path) field.ErrorList {
-	tracker := map[string][]string{}
-
-	// Get the old ClusterClassVariables as a map
-	oldVars, _ := getClusterClassVariablesMapWithReverseIndex(oldClusterClass.Spec.Variables)
-
-	// Get the new ClusterClassVariables as a map with an index linking them to their place in the ClusterClass Variable array.
-	// Note: The index is used to improve the error recording below.
-	newVars, clusterClassVariablesIndex := getClusterClassVariablesMapWithReverseIndex(newClusterClass.Spec.Variables)
-
-	// Compute the diff between old and new ClusterClassVariables.
-	varsDiff := getClusterClassVariablesForValidation(oldVars, newVars)
-
-	errorInfo := errorAggregator{}
-	allClusters := []string{}
-
-	// Validate the variable values on each Cluster ensuring they are still compatible with the new ClusterClass.
-	for _, cluster := range clusters {
-		allClusters = append(allClusters, cluster.Name)
-		for _, c := range cluster.Spec.Topology.Variables {
-			// copy variable to avoid memory aliasing.
-			clusterVar := c
-
-			// Add Cluster Variable entry in clusterVariableReferences to track where it is in use.
-			tracker[clusterVar.Name] = append(tracker[clusterVar.Name], cluster.Name)
-
-			// 1) Error if a variable with a schema altered in the update is no longer valid on the Cluster.
-			if alteredVar, ok := varsDiff[variableValidationKey{clusterVar.Name, altered}]; ok {
-				if errs := variables.ValidateClusterVariable(&clusterVar, alteredVar, field.NewPath("")); len(errs) > 0 {
-					errorInfo.add(alteredVar.Name, altered, cluster.Name)
-				}
-				continue
-			}
-
-			// 2) Error if a variable removed in the update is still in use in some Clusters.
-			if _, ok := varsDiff[variableValidationKey{clusterVar.Name, removed}]; ok {
-				errorInfo.add(clusterVar.Name, removed, cluster.Name)
-				continue
-			}
-
-			// 3) Error if a variable has been added in the update check is no longer valid on the Cluster.
-			// NOTE: This can't occur in normal circumstances as a variable must be defined in a ClusterClass in order to be introduced in
-			// a Cluster. This check may catch errors in cases involving broken Clusters.
-			if addedVar, ok := varsDiff[variableValidationKey{clusterVar.Name, added}]; ok {
-				if errs := variables.ValidateClusterVariable(&clusterVar, addedVar, field.NewPath("")); len(errs) > 0 {
-					errorInfo.add(addedVar.Name, added, cluster.Name)
-				}
-				continue
-			}
-		}
-
-		if cluster.Spec.Topology.Workers == nil {
-			continue
-		}
-
-		for _, md := range cluster.Spec.Topology.Workers.MachineDeployments {
-			// Continue if there are no variable overrides.
-			if md.Variables == nil || len(md.Variables.Overrides) == 0 {
-				continue
-			}
-
-			for _, c := range md.Variables.Overrides {
-				// copy variable to avoid memory aliasing.
-				clusterVar := c
-
-				// 1) Error if a variable with a schema altered in the update is no longer valid on the Cluster.
-				if alteredVar, ok := varsDiff[variableValidationKey{clusterVar.Name, altered}]; ok {
-					if errs := variables.ValidateClusterVariable(&clusterVar, alteredVar, field.NewPath("")); len(errs) > 0 {
-						errorInfo.add(fmt.Sprintf("%s/%s", md.Name, alteredVar.Name), altered, cluster.Name)
-					}
-					continue
-				}
-
-				// 2) Error if a variable removed in the update is still in use in some Clusters.
-				if _, ok := varsDiff[variableValidationKey{clusterVar.Name, removed}]; ok {
-					errorInfo.add(fmt.Sprintf("%s/%s", md.Name, clusterVar.Name), removed, cluster.Name)
-					continue
-				}
-
-				// 3) Error if a variable has been added in the update check is no longer valid on the Cluster.
-				// NOTE: This can't occur in normal circumstances as a variable must be defined in a ClusterClass in order to be introduced in
-				// a Cluster. This check may catch errors in cases involving broken Clusters.
-				if addedVar, ok := varsDiff[variableValidationKey{clusterVar.Name, added}]; ok {
-					if errs := variables.ValidateClusterVariable(&clusterVar, addedVar, field.NewPath("")); len(errs) > 0 {
-						errorInfo.add(fmt.Sprintf("%s/%s", md.Name, addedVar.Name), added, cluster.Name)
-					}
-					continue
-				}
-			}
-		}
-	}
-
-	// 4) Error if a required variable is not defined top-level in every cluster using the ClusterClass.
-	for v := range varsDiff {
-		if v.action == required {
-			clustersMissingVariable := clustersWithoutVar(allClusters, tracker[v.name])
-			if len(clustersMissingVariable) > 0 {
-				errorInfo.add(v.name, v.action, clustersMissingVariable...)
-			}
-		}
-	}
-
-	// Aggregate the errors from the validation checks and return one error message of each type for each variable with a list of violating clusters.
-	return aggregateErrors(errorInfo, clusterClassVariablesIndex, path)
-}
-
 // validateUpdatesToMachineHealthCheckClasses checks if the updates made to MachineHealthChecks are valid.
 // It makes sure that if a MachineHealthCheck definition is dropped from the ClusterClass then none of the
 // clusters using the ClusterClass rely on it to create a MachineHealthCheck.
@@ -366,106 +249,6 @@ func validateUpdatesToMachineHealthCheckClasses(clusters []clusterv1.Cluster, ol
 	return allErrs
 }
 
-type errorAggregator map[variableValidationKey][]string
-
-func (e errorAggregator) add(name string, action variableValidationType, references ...string) {
-	e[variableValidationKey{name, action}] = append(e[variableValidationKey{name, action}], references...)
-}
-
-func aggregateErrors(errs map[variableValidationKey][]string, clusterClassVariablesIndex map[string]int, path *field.Path) field.ErrorList {
-	var allErrs, addedErrs, alteredErrs, removedErrs, requiredErrs field.ErrorList
-	// Look through the errors and append aggregated error messages naming the Clusters blocking changes to existing variables.
-	for variable, clusters := range errs {
-		switch variable.action {
-		case altered:
-			alteredErrs = append(alteredErrs,
-				field.Forbidden(path.Index(clusterClassVariablesIndex[variable.name]),
-					fmt.Sprintf("variable schema change for %s is invalid. It failed validation in Clusters %v", variable.name, clusters),
-				))
-		case removed:
-			removedErrs = append(removedErrs,
-				field.Forbidden(path.Index(clusterClassVariablesIndex[variable.name]),
-					fmt.Sprintf("variable %s can not be removed. It is used in Clusters %v", variable.name, clusters),
-				))
-		case added:
-			addedErrs = append(addedErrs,
-				field.Forbidden(path.Index(clusterClassVariablesIndex[variable.name]),
-					fmt.Sprintf("variable %s can not be added. It failed validation in Clusters %v", variable.name, clusters),
-				))
-		case required:
-			requiredErrs = append(requiredErrs,
-				field.Forbidden(path.Index(clusterClassVariablesIndex[variable.name]),
-					fmt.Sprintf("variable %v is required but is not defined in Clusters %v", variable.name, clusters),
-				))
-		}
-	}
-
-	// Add the slices of errors one by one to maintain a defined order.
-	allErrs = append(allErrs, alteredErrs...)
-	allErrs = append(allErrs, removedErrs...)
-	allErrs = append(allErrs, addedErrs...)
-	allErrs = append(allErrs, requiredErrs...)
-
-	return allErrs
-}
-
-func clustersWithoutVar(allClusters, clustersWithVar []string) []string {
-	clustersWithVarMap := make(map[string]interface{})
-	for _, cluster := range clustersWithVar {
-		clustersWithVarMap[cluster] = nil
-	}
-	var clusterDiff []string
-	for _, cluster := range allClusters {
-		if _, found := clustersWithVarMap[cluster]; !found {
-			clusterDiff = append(clusterDiff, cluster)
-		}
-	}
-	return clusterDiff
-}
-
-type variableValidationType int
-
-const (
-	added variableValidationType = iota
-	altered
-	removed
-	required
-)
-
-// variableValidationKey holds the name of the variable and a validation action to perform on it.
-type variableValidationKey struct {
-	name   string
-	action variableValidationType
-}
-
-// getClusterClassVariablesForValidation returns a struct with the four classes of ClusterClass Variables that require validation:
-// - added which have been added to the ClusterClass on update.
-// - altered which have had their schemas changed on update.
-// - removed which have been removed from the ClusterClass on update.
-// - required (with 'required' : true) from the new ClusterClass.
-func getClusterClassVariablesForValidation(oldVars, newVars map[string]*clusterv1.ClusterClassVariable) map[variableValidationKey]*clusterv1.ClusterClassVariable {
-	out := map[variableValidationKey]*clusterv1.ClusterClassVariable{}
-
-	// Compare the old Variable map to the new one to discover which variables have been removed and which have been altered.
-	for k, v := range oldVars {
-		if _, ok := newVars[k]; !ok {
-			out[variableValidationKey{action: removed, name: k}] = v
-		} else if !reflect.DeepEqual(v.Schema, newVars[k].Schema) {
-			out[variableValidationKey{action: altered, name: k}] = newVars[k]
-		}
-	}
-	// Compare the new ClusterClassVariables to the new ClusterClassVariables to find out what variables have been added and which are now required.
-	for k, v := range newVars {
-		if _, ok := oldVars[k]; !ok {
-			out[variableValidationKey{action: added, name: k}] = v
-		}
-		if v.Required {
-			out[variableValidationKey{action: required, name: v.Name}] = v
-		}
-	}
-	return out
-}
-
 func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(clusters []clusterv1.Cluster, oldClusterClass, newClusterClass *clusterv1.ClusterClass) field.ErrorList {
 	var allErrs field.ErrorList