first steps. Need to create a common function for MHC validation

Signed-off-by: killianmuldoon <[email protected]>
kubernetes-sigs · Nov 4, 2022 · f1b5061 · f1b5061
1 parent 05246e5
commit f1b5061
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 68 deletions.
diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go
@@ -149,6 +149,7 @@ type MachineHealthCheckClass struct {
 	// UnhealthyConditions contains a list of the conditions that determine
 	// whether a node is considered unhealthy. The conditions are combined in a
 	// logical OR, i.e. if any of the conditions is met, the node is unhealthy.
+	// +kubebuilder:validation:MinItems=1
 	UnhealthyConditions []UnhealthyCondition `json:"unhealthyConditions,omitempty"`
 
 	// Any further remediation is only allowed if at most "MaxUnhealthy" machines selected by

diff --git a/api/v1beta1/machinehealthcheck_webhook.go b/api/v1beta1/machinehealthcheck_webhook.go
@@ -136,6 +136,18 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error {
 		)
 	}
 
+	allErrs = append(allErrs, m.ValidateCommonFields(specPath)...)
+
+	if len(allErrs) == 0 {
+		return nil
+	}
+	return apierrors.NewInvalid(GroupVersion.WithKind("MachineHealthCheck").GroupKind(), m.Name, allErrs)
+}
+
+// ValidateCommonFields validates nodeStartupTimeout, maxUnhealthy, and remediationTemplate of the MHC.
+func (m *MachineHealthCheck) ValidateCommonFields(specPath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+
 	if m.Spec.NodeStartupTimeout != nil &&
 		m.Spec.NodeStartupTimeout.Seconds() != disabledNodeStartupTimeout.Seconds() &&
 		m.Spec.NodeStartupTimeout.Seconds() < minNodeStartupTimeout.Seconds() {
@@ -144,7 +156,6 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error {
 			field.Invalid(specPath.Child("nodeStartupTimeout"), m.Spec.NodeStartupTimeout.Seconds(), "must be at least 30s"),
 		)
 	}
-
 	if m.Spec.MaxUnhealthy != nil {
 		if _, err := intstr.GetScaledValueFromIntOrPercent(m.Spec.MaxUnhealthy, 0, false); err != nil {
 			allErrs = append(
@@ -153,7 +164,6 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error {
 			)
 		}
 	}
-
 	if m.Spec.RemediationTemplate != nil && m.Spec.RemediationTemplate.Namespace != m.Namespace {
 		allErrs = append(
 			allErrs,
@@ -165,8 +175,12 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error {
 		)
 	}
 
-	if len(allErrs) == 0 {
-		return nil
+	if len(m.Spec.UnhealthyConditions) == 0 {
+		allErrs = append(allErrs, field.Forbidden(
+			specPath.Child("unhealthyConditions"),
+			"must have at least one value",
+		))
 	}
-	return apierrors.NewInvalid(GroupVersion.WithKind("MachineHealthCheck").GroupKind(), m.Name, allErrs)
+
+	return allErrs
 }
diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go
@@ -24,6 +24,7 @@ import (
 	"github.com/blang/semver"
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -366,65 +367,56 @@ func (webhook *Cluster) getClusterClassForCluster(ctx context.Context, cluster *
 func validateMachineHealthChecks(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList {
 	var allErrs field.ErrorList
 
-	// Validate ControlPlane MachineHealthCheck if defined.
-	if cluster.Spec.Topology.ControlPlane.MachineHealthCheck != nil && !cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.IsZero() {
-		// Ensure ControlPlane does not define a MachineHealthCheck if the ClusterClass does not define MachineInfrastructure.
-		if clusterClass.Spec.ControlPlane.MachineInfrastructure == nil {
-			allErrs = append(allErrs, field.Forbidden(
-				field.NewPath("spec", "topology", "controlPlane", "machineHealthCheck"),
-				"can be set only if spec.controlPlane.machineInfrastructure is set in ClusterClass",
-			))
+	if cluster.Spec.Topology.ControlPlane.MachineHealthCheck != nil {
+		path := field.NewPath("spec", "topology", "controlPlane", "machineHealthCheck")
+
+		// Validate ControlPlane MachineHealthCheck if defined.
+		if !cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.IsZero() {
+			// Ensure ControlPlane does not define a MachineHealthCheck if the ClusterClass does not define MachineInfrastructure.
+			if clusterClass.Spec.ControlPlane.MachineInfrastructure == nil {
+				allErrs = append(allErrs, field.Forbidden(
+					path,
+					"can be set only if spec.controlPlane.machineInfrastructure is set in ClusterClass",
+				))
+			}
 		}
-		// Ensure ControlPlane MachineHealthCheck defines UnhealthyConditions.
-		if len(cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.UnhealthyConditions) == 0 {
+
+		// If MachineHealthCheck is explicitly enabled then make sure that a MachineHealthCheck definition is
+		// available either in the Cluster topology or in the ClusterClass.
+		// (One of these definitions will be used in the controller to create the MachineHealthCheck)
+		if cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable != nil &&
+			*cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable &&
+			cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.IsZero() &&
+			clusterClass.Spec.ControlPlane.MachineHealthCheck == nil {
 			allErrs = append(allErrs, field.Forbidden(
-				field.NewPath("spec", "topology", "controlPlane", "machineHealthCheck", "unhealthyConditions"),
-				"must have at least one value",
+				path.Child("enable"),
+				fmt.Sprintf("cannot be set to %t as MachineHealthCheck definition is not available in the Cluster topology or the ClusterClass", *cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable),
 			))
 		}
-	}
-
-	// If MachineHealthCheck is explicitly enabled then make sure that a MachineHealthCheck definition is
-	// available either in the Cluster topology or in the ClusterClass.
-	// (One of these definitions will be used in the controller to create the MachineHealthCheck)
-	if cluster.Spec.Topology.ControlPlane.MachineHealthCheck != nil &&
-		cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable != nil &&
-		*cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable &&
-		cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.IsZero() &&
-		clusterClass.Spec.ControlPlane.MachineHealthCheck == nil {
-		allErrs = append(allErrs, field.Forbidden(
-			field.NewPath("spec", "topology", "controlPlane", "machineHealthCheck", "enable"),
-			fmt.Sprintf("cannot be set to %t as MachineHealthCheck definition is not available in the Cluster topology or the ClusterClass", *cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable),
-		))
+		allErrs = append(allErrs, validateMachineHealthCheckTopology(path, cluster.Namespace, cluster.Spec.Topology.ControlPlane.MachineHealthCheck)...)
 	}
 
 	if cluster.Spec.Topology.Workers != nil {
 		for i, md := range cluster.Spec.Topology.Workers.MachineDeployments {
-			// If MachineHealthCheck is defined ensure it defines UnhealthyConditions.
-			if md.MachineHealthCheck != nil && !md.MachineHealthCheck.MachineHealthCheckClass.IsZero() {
-				if len(md.MachineHealthCheck.MachineHealthCheckClass.UnhealthyConditions) == 0 {
-					allErrs = append(allErrs, field.Forbidden(
-						field.NewPath("spec", "topology", "workers", "machineDeployments", "machineHealthCheck").Index(i).Child("unhealthyConditions"),
-						"must have at least one value",
-					))
-				}
-			}
-
-			// If MachineHealthCheck is explicitly enabled then make sure that a MachineHealthCheck definition is
-			// available either in the Cluster topology or in the ClusterClass.
-			// (One of these definitions will be used in the controller to create the MachineHealthCheck)
-			mdClass := machineDeploymentClassOfName(clusterClass, md.Class)
-			if mdClass != nil { // Note: we skip handling the nil case here as it is already handled in previous validations.
-				if md.MachineHealthCheck != nil &&
-					md.MachineHealthCheck.Enable != nil &&
-					*md.MachineHealthCheck.Enable &&
-					md.MachineHealthCheck.MachineHealthCheckClass.IsZero() &&
-					mdClass.MachineHealthCheck == nil {
-					allErrs = append(allErrs, field.Forbidden(
-						field.NewPath("spec", "topology", "workers", "machineDeployments", "machineHealthCheck").Index(i).Child("enable"),
-						fmt.Sprintf("cannot be set to %t as MachineHealthCheck definition is not available in the Cluster topology or the ClusterClass", *md.MachineHealthCheck.Enable),
-					))
+			if md.MachineHealthCheck != nil {
+				path := field.NewPath("topology", "workers", "machineDeployments", "machineHealthCheck").Index(i)
+
+				// If MachineHealthCheck is explicitly enabled then make sure that a MachineHealthCheck definition is
+				// available either in the Cluster topology or in the ClusterClass.
+				// (One of these definitions will be used in the controller to create the MachineHealthCheck)
+				mdClass := machineDeploymentClassOfName(clusterClass, md.Class)
+				if mdClass != nil { // Note: we skip handling the nil case here as it is already handled in previous validations.
+					if md.MachineHealthCheck.Enable != nil &&
+						*md.MachineHealthCheck.Enable &&
+						md.MachineHealthCheck.MachineHealthCheckClass.IsZero() &&
+						mdClass.MachineHealthCheck == nil {
+						allErrs = append(allErrs, field.Forbidden(
+							field.NewPath("spec", "topology", "workers", "machineDeployments", "machineHealthCheck").Index(i).Child("enable"),
+							fmt.Sprintf("cannot be set to %t as MachineHealthCheck definition is not available in the Cluster topology or the ClusterClass", *md.MachineHealthCheck.Enable),
+						))
+					}
 				}
+				allErrs = append(allErrs, validateMachineHealthCheckTopology(path, cluster.Namespace, md.MachineHealthCheck)...)
 			}
 		}
 	}
@@ -433,7 +425,7 @@ func validateMachineHealthChecks(cluster *clusterv1.Cluster, clusterClass *clust
 }
 
 // machineDeploymentClassOfName find a MachineDeploymentClass of the given name in the provided ClusterClass.
-// Returns nill if can not find one.
+// Returns nil if it can not find one.
 // TODO: Check if there is already a helper function that can do this.
 func machineDeploymentClassOfName(clusterClass *clusterv1.ClusterClass, name string) *clusterv1.MachineDeploymentClass {
 	for _, mdClass := range clusterClass.Spec.Workers.MachineDeployments {
@@ -443,3 +435,20 @@ func machineDeploymentClassOfName(clusterClass *clusterv1.ClusterClass, name str
 	}
 	return nil
 }
+
+// validateMachineHealthCheckTopology creates a MachineHealthCheck and runs common validation on the fields.
+func validateMachineHealthCheckTopology(specPath *field.Path, namepace string, m *clusterv1.MachineHealthCheckTopology) field.ErrorList {
+	mhc := clusterv1.MachineHealthCheck{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namepace,
+		},
+		Spec: clusterv1.MachineHealthCheckSpec{
+			NodeStartupTimeout:  m.NodeStartupTimeout,
+			MaxUnhealthy:        m.MaxUnhealthy,
+			UnhealthyConditions: m.UnhealthyConditions,
+			UnhealthyRange:      m.UnhealthyRange,
+			RemediationTemplate: m.RemediationTemplate,
+		}}
+
+	return mhc.ValidateCommonFields(specPath)
+}
diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go
@@ -25,6 +25,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -538,17 +539,21 @@ func validateMachineHealthCheckClasses(clusterClass *clusterv1.ClusterClass) fie
 
 	// Validate ControlPlane MachineHealthCheck if defined.
 	if clusterClass.Spec.ControlPlane.MachineHealthCheck != nil {
+		controlPlaneMHCPath := field.NewPath("spec", "controlPlane", "machineHealthCheck")
+
+		allErrs = append(allErrs, validateMachineHealthCheckClass(controlPlaneMHCPath, clusterClass.Namespace, clusterClass.Spec.ControlPlane.MachineHealthCheck)...)
+
 		// Ensure ControlPlane does not define a MachineHealthCheck if it does not define MachineInfrastructure.
 		if clusterClass.Spec.ControlPlane.MachineInfrastructure == nil {
 			allErrs = append(allErrs, field.Forbidden(
-				field.NewPath("spec", "controlPlane", "machineHealthCheck"),
+				controlPlaneMHCPath.Child("machineInfrastructure"),
 				"can be set only if spec.controlPlane.machineInfrastructure is set",
 			))
 		}
 		// Ensure ControlPlane MachineHealthCheck defines UnhealthyConditions.
 		if len(clusterClass.Spec.ControlPlane.MachineHealthCheck.UnhealthyConditions) == 0 {
 			allErrs = append(allErrs, field.Forbidden(
-				field.NewPath("spec", "controlPlane", "machineHealthCheck", "unhealthyConditions"),
+				controlPlaneMHCPath.Child("unhealthyConditions"),
 				"must be defined and have at least one value",
 			))
 		}
@@ -559,12 +564,26 @@ func validateMachineHealthCheckClasses(clusterClass *clusterv1.ClusterClass) fie
 		if md.MachineHealthCheck == nil {
 			continue
 		}
-		if len(md.MachineHealthCheck.UnhealthyConditions) == 0 {
-			allErrs = append(allErrs, field.Forbidden(
-				field.NewPath("spec", "workers", "machineDeployments", "machineHealthCheck").Index(i).Child("unhealthyConditions"),
-				"must be defined and have at least one value",
-			))
-		}
+		machineDeploymentMHCPath := field.NewPath("spec", "machineDeployments", "machineHealthCheck").Index(i)
+
+		allErrs = append(allErrs, validateMachineHealthCheckClass(machineDeploymentMHCPath, clusterClass.Namespace, md.MachineHealthCheck)...)
 	}
 	return allErrs
 }
+
+// validateMachineHealthCheckClass creates a MachineHealthCheck and runs common validation on the fields.
+func validateMachineHealthCheckClass(specPath *field.Path, namepace string, m *clusterv1.MachineHealthCheckClass) field.ErrorList {
+	mhc := clusterv1.MachineHealthCheck{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namepace,
+		},
+		Spec: clusterv1.MachineHealthCheckSpec{
+			NodeStartupTimeout:  m.NodeStartupTimeout,
+			MaxUnhealthy:        m.MaxUnhealthy,
+			UnhealthyConditions: m.UnhealthyConditions,
+			UnhealthyRange:      m.UnhealthyRange,
+			RemediationTemplate: m.RemediationTemplate,
+		}}
+
+	return mhc.ValidateCommonFields(specPath)
+}
diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go
@@ -643,7 +643,7 @@ func TestClusterClassValidation(t *testing.T) {
 						},
 					},
 					NodeStartupTimeout: &metav1.Duration{
-						Duration: time.Duration(1)}}).
+						Duration: time.Duration(6000000000000)}}).
 				Build(),
 		},
 		{
@@ -657,7 +657,7 @@ func TestClusterClassValidation(t *testing.T) {
 				// No ControlPlaneMachineInfrastructure makes this an invalid creation request.
 				WithControlPlaneMachineHealthCheck(&clusterv1.MachineHealthCheckClass{
 					NodeStartupTimeout: &metav1.Duration{
-						Duration: time.Duration(1)}}).
+						Duration: time.Duration(6000000000000)}}).
 				Build(),
 			expectErr: true,
 		},
@@ -674,7 +674,7 @@ func TestClusterClassValidation(t *testing.T) {
 						Build()).
 				WithControlPlaneMachineHealthCheck(&clusterv1.MachineHealthCheckClass{
 					NodeStartupTimeout: &metav1.Duration{
-						Duration: time.Duration(1)}}).
+						Duration: time.Duration(6000000000000)}}).
 				Build(),
 			expectErr: true,
 		},
@@ -701,7 +701,7 @@ func TestClusterClassValidation(t *testing.T) {
 								},
 							},
 							NodeStartupTimeout: &metav1.Duration{
-								Duration: time.Duration(1)}}).
+								Duration: time.Duration(6000000000000)}}).
 						Build()).
 				Build(),
 		},