Skip to content

Commit

Permalink
first steps. Need to create a common function for MHC validation
Browse files Browse the repository at this point in the history 
Signed-off-by: killianmuldoon <[email protected]>
  • Loading branch information
@killianmuldoon
killianmuldoon committed Nov 8, 2022
1 parent 6f32637 commit 928b779
Show file tree
Hide file tree
Showing 5 changed files with 246 additions and 79 deletions.
32 changes: 24 additions & 8 deletions api/v1beta1/machinehealthcheck_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,37 +136,53 @@ 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 UnhealthyConditions NodeStartupTimeout, MaxUnhealthy, and RemediationTemplate of the MHC.
// It is also used to help validate other types which define MachineHealthChecks such as MachineHealthCheckClass and MachineHealthCheckTopology.
// Note: this function is used for internal validation and is not intended for external consumption.
func (m *MachineHealthCheck) ValidateCommonFields(fldPath *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() {
allErrs = append(
allErrs,
field.Invalid(specPath.Child("nodeStartupTimeout"), m.Spec.NodeStartupTimeout.Seconds(), "must be at least 30s"),
field.Invalid(fldPath.Child("nodeStartupTimeout"), m.Spec.NodeStartupTimeout.String(), "must be at least 30s"),
)
}

if m.Spec.MaxUnhealthy != nil {
if _, err := intstr.GetScaledValueFromIntOrPercent(m.Spec.MaxUnhealthy, 0, false); err != nil {
allErrs = append(
allErrs,
field.Invalid(specPath.Child("maxUnhealthy"), m.Spec.MaxUnhealthy, fmt.Sprintf("must be either an int or a percentage: %v", err.Error())),
field.Invalid(fldPath.Child("maxUnhealthy"), m.Spec.MaxUnhealthy, fmt.Sprintf("must be either an int or a percentage: %v", err.Error())),
)
}
}

if m.Spec.RemediationTemplate != nil && m.Spec.RemediationTemplate.Namespace != m.Namespace {
allErrs = append(
allErrs,
field.Invalid(
specPath.Child("remediationTemplate", "namespace"),
fldPath.Child("remediationTemplate", "namespace"),
m.Spec.RemediationTemplate.Namespace,
"must match metadata.namespace",
),
)
}

if len(allErrs) == 0 {
return nil
if len(m.Spec.UnhealthyConditions) == 0 {
allErrs = append(allErrs, field.Forbidden(
fldPath.Child("unhealthyConditions"),
"must have at least one entry",
))
}
return apierrors.NewInvalid(GroupVersion.WithKind("MachineHealthCheck").GroupKind(), m.Name, allErrs)

return allErrs
}
111 changes: 110 additions & 1 deletion api/v1beta1/machinehealthcheck_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ func TestMachineHealthCheckDefault(t *testing.T) {
MatchLabels: map[string]string{"foo": "bar"},
},
RemediationTemplate: &corev1.ObjectReference{},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}
t.Run("for MachineHealthCheck", utildefaulting.DefaultValidateTest(mhc))
Expand Down Expand Up @@ -77,6 +83,12 @@ func TestMachineHealthCheckLabelSelectorAsSelectorValidation(t *testing.T) {
Selector: metav1.LabelSelector{
MatchLabels: tt.selectors,
},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}
if tt.expectErr {
Expand Down Expand Up @@ -123,6 +135,12 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) {
"test": "test",
},
},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}
oldMHC := &MachineHealthCheck{
Expand All @@ -133,6 +151,12 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) {
"test": "test",
},
},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}

Expand All @@ -145,6 +169,58 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) {
}
}

func TestMachineHealthCheckUnhealthyConditions(t *testing.T) {
tests := []struct {
name string
unhealthConditions []UnhealthyCondition
expectErr bool
}{
{
name: "pass with correctly defined unhealthyConditions",
unhealthConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
expectErr: false,
},
{
name: "fail if the UnhealthCondition array is nil",
unhealthConditions: nil,
expectErr: true,
},
{
name: "fail if the UnhealthCondition array is empty",
unhealthConditions: []UnhealthyCondition{},
expectErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
mhc := &MachineHealthCheck{
Spec: MachineHealthCheckSpec{
Selector: metav1.LabelSelector{
MatchLabels: map[string]string{
"test": "test",
},
},
UnhealthyConditions: tt.unhealthConditions,
},
}
if tt.expectErr {
g.Expect(mhc.ValidateCreate()).NotTo(Succeed())
g.Expect(mhc.ValidateUpdate(mhc)).NotTo(Succeed())
} else {
g.Expect(mhc.ValidateCreate()).To(Succeed())
g.Expect(mhc.ValidateUpdate(mhc)).To(Succeed())
}
})
}
}

func TestMachineHealthCheckNodeStartupTimeout(t *testing.T) {
zero := metav1.Duration{Duration: 0}
twentyNineSeconds := metav1.Duration{Duration: 29 * time.Second}
Expand Down Expand Up @@ -200,6 +276,12 @@ func TestMachineHealthCheckNodeStartupTimeout(t *testing.T) {
"test": "test",
},
},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}

Expand Down Expand Up @@ -253,6 +335,12 @@ func TestMachineHealthCheckMaxUnhealthy(t *testing.T) {
"test": "test",
},
},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}

Expand All @@ -268,7 +356,16 @@ func TestMachineHealthCheckMaxUnhealthy(t *testing.T) {

func TestMachineHealthCheckSelectorValidation(t *testing.T) {
g := NewWithT(t)
mhc := &MachineHealthCheck{}
mhc := &MachineHealthCheck{
Spec: MachineHealthCheckSpec{
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}
err := mhc.validate(nil)
g.Expect(err).ToNot(BeNil())
g.Expect(err.Error()).To(ContainSubstring("selector must not be empty"))
Expand All @@ -285,6 +382,12 @@ func TestMachineHealthCheckClusterNameSelectorValidation(t *testing.T) {
"baz": "qux",
},
},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}
err := mhc.validate(nil)
Expand All @@ -305,6 +408,12 @@ func TestMachineHealthCheckRemediationTemplateNamespaceValidation(t *testing.T)
Spec: MachineHealthCheckSpec{
Selector: metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
RemediationTemplate: &corev1.ObjectReference{Namespace: "foo"},
UnhealthyConditions: []UnhealthyCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionFalse,
},
},
},
}
invalid := valid.DeepCopy()
Expand Down
102 changes: 51 additions & 51 deletions internal/webhooks/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,64 +366,64 @@ 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",
))
}
// Ensure ControlPlane MachineHealthCheck defines UnhealthyConditions.
if len(cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.UnhealthyConditions) == 0 {
allErrs = append(allErrs, field.Forbidden(
field.NewPath("spec", "topology", "controlPlane", "machineHealthCheck", "unhealthyConditions"),
"must have at least one value",
))
if cluster.Spec.Topology.ControlPlane.MachineHealthCheck != nil {
fldPath := 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(
fldPath,
"can be set only if spec.controlPlane.machineInfrastructure is set in ClusterClass",
))
}
allErrs = append(allErrs, validateMachineHealthCheckClass(fldPath, cluster.Namespace,
&cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass)...)
}
}

// 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),
))
// 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)

// Check if the machineHealthCheck is explicitly enabled in the ControlPlaneTopology.
if cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable != nil && *cluster.Spec.Topology.ControlPlane.MachineHealthCheck.Enable {
// Ensure the MHC is defined in at least one of the ControlPlaneTopology of the Cluster or the ControlPlaneClass of the ClusterClass.
if cluster.Spec.Topology.ControlPlane.MachineHealthCheck.MachineHealthCheckClass.IsZero() && clusterClass.Spec.ControlPlane.MachineHealthCheck == nil {
allErrs = append(allErrs, field.Forbidden(
fldPath.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 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 md.MachineHealthCheck != nil {
fldPath := field.NewPath("spec", "topology", "workers", "machineDeployments", "machineHealthCheck").Index(i)

// Validate the MachineDeployment MachineHealthCheck if defined.
if !md.MachineHealthCheck.MachineHealthCheckClass.IsZero() {
allErrs = append(allErrs, validateMachineHealthCheckClass(fldPath, cluster.Namespace,
&md.MachineHealthCheck.MachineHealthCheckClass)...)
}
}

// 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 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.
// Check if the machineHealthCheck is explicitly enabled in the machineDeploymentTopology.
if md.MachineHealthCheck.Enable != nil && *md.MachineHealthCheck.Enable {
// Ensure the MHC is defined in at least one of the MachineDeploymentTopology of the Cluster or the MachineDeploymentClass of the ClusterClass.
if md.MachineHealthCheck.MachineHealthCheckClass.IsZero() && mdClass.MachineHealthCheck == nil {
allErrs = append(allErrs, field.Forbidden(
fldPath.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),
))
}
}
}
}
}
Expand All @@ -433,7 +433,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 {
Expand Down
Loading

0 comments on commit 928b779

Please sign in to comment.