From f1b50613426f2c2a0bd85544efc35b96c2d126b1 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Fri, 4 Nov 2022 16:58:52 +0000 Subject: [PATCH] first steps. Need to create a common function for MHC validation Signed-off-by: killianmuldoon --- api/v1beta1/clusterclass_types.go | 1 + api/v1beta1/machinehealthcheck_webhook.go | 24 ++++- internal/webhooks/cluster.go | 111 ++++++++++++---------- internal/webhooks/clusterclass.go | 35 +++++-- internal/webhooks/clusterclass_test.go | 8 +- 5 files changed, 111 insertions(+), 68 deletions(-) diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go index 0ea529a1348a..3a37c8e7e505 100644 --- 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 index ce27eba4ab6f..84276e0ef7f0 100644 --- 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 index b50f99beb632..d2ea97a4032d 100644 --- 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 index 470a3b4238ca..ae3018cfad5f 100644 --- 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 index 555e3fe900ed..f893762cc31e 100644 --- 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(), },