Prevent "Exists" tolerations from having a value

This commit introduces a validation check in the webhook,
to ensure that tolerations with operator: "Exists" do not specify a value,
as this is invalid in k8s.

Author: TomerNewman

Diff for: internal/webhook/module.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"regexp"
 	"strings"
 
@@ -110,7 +111,7 @@ func validateModule(mod *kmmv1beta1.Module) (admission.Warnings, error) {
 		return nil, fmt.Errorf("failed to validate kernel mappings: %v", err)
 	}
 
-	if err := validateModuleTolerations(mod); err != nil {
+	if err := validateTolerations(mod.Spec.Tolerations); err != nil {
 		return nil, fmt.Errorf("failed to validate Module's tolerations: %v", err)
 	}
 
@@ -212,17 +213,51 @@ func validateModprobe(modprobe kmmv1beta1.ModprobeSpec) error {
 
 	return nil
 }
-func validateModuleTolerations(mod *kmmv1beta1.Module) error {
-	for _, toleration := range mod.Spec.Tolerations {
 
-		if toleration.Operator != corev1.TolerationOpExists && toleration.Operator != corev1.TolerationOpEqual {
-			return fmt.Errorf("toleration operator can be only {Exists, Equal} but got %s", toleration.Operator)
+func validateTolerations(tolerations []corev1.Toleration) error {
+
+	for i, toleration := range tolerations {
+		tolIndex := fmt.Sprintf("Toleration[%d]", i)
+
+		if len(toleration.Key) > 0 {
+			if errs := validation.IsQualifiedName(toleration.Key); len(errs) > 0 {
+				return fmt.Errorf("%s invalid key '%s': %s", tolIndex, toleration.Key, strings.Join(errs, "; "))
+			}
+		}
+
+		if len(toleration.Key) == 0 && toleration.Operator != corev1.TolerationOpExists {
+			return fmt.Errorf("%s operator must be 'Exists' when key is empty", tolIndex)
+		}
+
+		if toleration.TolerationSeconds != nil && toleration.Effect != corev1.TaintEffectNoExecute {
+			return fmt.Errorf("%s effect must be 'NoExecute' when tolerationSeconds is set", tolIndex)
 		}
 
-		if toleration.Effect != corev1.TaintEffectNoSchedule &&
-			toleration.Effect != corev1.TaintEffectNoExecute &&
-			toleration.Effect != corev1.TaintEffectPreferNoSchedule {
-			return fmt.Errorf("toleration effect can be only {NoSchedule, NoExecute, PreferNoSchedule} but got %s", toleration.Effect)
+		switch toleration.Operator {
+		case corev1.TolerationOpEqual, "":
+			if errs := validation.IsValidLabelValue(toleration.Value); len(errs) > 0 {
+				return fmt.Errorf("%s invalid value '%s' for Equal operator %s", tolIndex, toleration.Value, strings.Join(errs, "; "))
+			}
+		case corev1.TolerationOpExists:
+			if len(toleration.Value) > 0 {
+				return fmt.Errorf("%s value must be empty when operator is 'Exists'", tolIndex)
+			}
+		default:
+			return fmt.Errorf("%s invalid operator '%s', allowed values are ['Equal', 'Exists']", tolIndex, toleration.Operator)
+		}
+
+		if len(toleration.Effect) > 0 {
+			validEffects := []corev1.TaintEffect{corev1.TaintEffectNoSchedule, corev1.TaintEffectPreferNoSchedule, corev1.TaintEffectNoExecute}
+			valid := false
+			for _, v := range validEffects {
+				if toleration.Effect == v {
+					valid = true
+					break
+				}
+			}
+			if !valid {
+				return fmt.Errorf("%s invalid effect '%s', allowed values are ['NoSchedule', 'PreferNoSchedule', 'NoExecute']", tolIndex, toleration.Effect)
+			}
 		}
 	}
 	return nil

Diff for: internal/webhook/module_test.go

@@ -553,24 +553,23 @@ var _ = Describe("ValidateDelete", func() {
 	})
 })
 
-var _ = Describe("validateModuleTolerarations", func() {
+var _ = Describe("validateTolerations", func() {
 	It("should fail when Module has an invalid toleration effect", func() {
-		mod := validModule
-		mod.Spec.Tolerations = []v1.Toleration{
+		tolerations := []v1.Toleration{
 			{
 				Key: "Test-Key1", Operator: v1.TolerationOpEqual, Value: "Test-Value1", Effect: v1.TaintEffectPreferNoSchedule,
 			},
 			{
-				Key: "Test-Key2", Operator: v1.TolerationOpExists, Value: "Test-Value2", Effect: "Invalid-Effect",
+				Key: "Test-Key2", Operator: v1.TolerationOpExists, Effect: "Invalid-Effect",
 			},
 		}
 
-		err := validateModuleTolerations(&mod)
+		err := validateTolerations(tolerations)
 		Expect(err).To(HaveOccurred())
 	})
+
 	It("should fail when Module has an invalid toleration operator", func() {
-		mod := validModule
-		mod.Spec.Tolerations = []v1.Toleration{
+		tolerations := []v1.Toleration{
 			{
 				Key: "Test-Key1", Operator: v1.TolerationOpEqual, Value: "Test-Value1", Effect: v1.TaintEffectPreferNoSchedule,
 			},
@@ -579,14 +578,28 @@ var _ = Describe("validateModuleTolerarations", func() {
 			},
 		}
 
-		err := validateModuleTolerations(&mod)
+		err := validateTolerations(tolerations)
 		Expect(err).To(HaveOccurred())
 	})
-	It("should work when all tolerations have valid effects ", func() {
-		mod := validModule
-		mod.Spec.Tolerations = []v1.Toleration{
+
+	It("should fail when toleration has non empty value and Exists operator", func() {
+		tolerations := []v1.Toleration{
+			{
+				Key: "Test-Key1", Operator: v1.TolerationOpEqual, Value: "Test-Value1", Effect: v1.TaintEffectPreferNoSchedule,
+			},
+			{
+				Key: "Test-Key2", Operator: v1.TolerationOpExists, Value: "Test-Value2", Effect: v1.TaintEffectNoExecute,
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("should work when all tolerations have valid effects", func() {
+		tolerations := []v1.Toleration{
 			{
-				Key: "Test-Key1", Operator: v1.TolerationOpExists, Value: "Test-Value", Effect: v1.TaintEffectPreferNoSchedule,
+				Key: "Test-Key1", Operator: v1.TolerationOpExists, Effect: v1.TaintEffectPreferNoSchedule,
 			},
 			{
 				Key: "Test-Key2", Operator: v1.TolerationOpEqual, Value: "Test-Value", Effect: v1.TaintEffectNoSchedule,
@@ -596,8 +609,82 @@ var _ = Describe("validateModuleTolerarations", func() {
 			},
 		}
 
-		err := validateModuleTolerations(&mod)
-		Expect(err).ToNot(HaveOccurred())
+		err := validateTolerations(tolerations)
+		Expect(err).To(Not(HaveOccurred()))
+	})
+
+	It("should fail when toleration has an invalid key format", func() {
+		tolerations := []v1.Toleration{
+			{
+				Key: "Invalid Key!", Operator: v1.TolerationOpEqual, Value: "Test-Value",
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("should fail when toleration has empty key with an invalid operator", func() {
+		tolerations := []v1.Toleration{
+			{
+				Key: "", Operator: v1.TolerationOpEqual, Value: "Test-Value",
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(HaveOccurred())
 	})
 
+	It("should fail when TolerationSeconds is set but Effect is not NoExecute", func() {
+		tolSecs := int64(30)
+		tolerations := []v1.Toleration{
+			{
+				Key: "Test-Key", Operator: v1.TolerationOpEqual, Value: "Test-Value", Effect: v1.TaintEffectNoSchedule, TolerationSeconds: &tolSecs,
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("should pass when TolerationSeconds is set with NoExecute effect", func() {
+		tolSecs := int64(60)
+		tolerations := []v1.Toleration{
+			{
+				Key: "Test-Key", Operator: v1.TolerationOpEqual, Value: "Test-Value", Effect: v1.TaintEffectNoExecute, TolerationSeconds: &tolSecs,
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(Not(HaveOccurred()))
+	})
+
+	It("should fail when an invalid effect is provided", func() {
+		tolerations := []v1.Toleration{
+			{
+				Key: "Test-Key", Operator: v1.TolerationOpExists, Effect: "Invalid-Effect",
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("should fail when operator is not Equal or Exists", func() {
+		tolerations := []v1.Toleration{
+			{
+				Key: "Test-Key", Operator: "InvalidOperator", Value: "Test-Value",
+			},
+		}
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("should pass with an empty toleration list", func() {
+		var tolerations []v1.Toleration
+
+		err := validateTolerations(tolerations)
+		Expect(err).To(Not(HaveOccurred()))
+	})
 })