Merge pull request #52471 from xingzhou/taint-delete

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Need to validate taint effect when removing taints.

Instead of reporting taint not found, it's better to report user
that the effect is invalid. This will help user to check errors.
So when user tries to remove a taint, two conditions will be checked:
1. Whether or not the effect is an empty string.
2. Whether or not the non-empty effect is a valid taint effect.

**Release note**:
```release-note
None
```

pkg/util/taints/taints.go

@@ -45,15 +45,14 @@ func parseTaint(st string) (v1.Taint, error) {
 
 	parts2 := strings.Split(parts[1], ":")
 
-	effect := v1.TaintEffect(parts2[1])
-
 	errs := validation.IsValidLabelValue(parts2[0])
 	if len(parts2) != 2 || len(errs) != 0 {
 		return taint, fmt.Errorf("invalid taint spec: %v, %s", st, strings.Join(errs, "; "))
 	}
 
-	if effect != v1.TaintEffectNoSchedule && effect != v1.TaintEffectPreferNoSchedule && effect != v1.TaintEffectNoExecute {
-		return taint, fmt.Errorf("invalid taint spec: %v, unsupported taint effect", st)
+	effect := v1.TaintEffect(parts2[1])
+	if err := validateTaintEffect(effect); err != nil {
+		return taint, err
 	}
 
 	taint.Key = parts[0]
@@ -63,6 +62,14 @@ func parseTaint(st string) (v1.Taint, error) {
 	return taint, nil
 }
 
+func validateTaintEffect(effect v1.TaintEffect) error {
+	if effect != v1.TaintEffectNoSchedule && effect != v1.TaintEffectPreferNoSchedule && effect != v1.TaintEffectNoExecute {
+		return fmt.Errorf("invalid taint effect: %v, unsupported taint effect", effect)
+	}
+
+	return nil
+}
+
 // NewTaintsVar wraps []api.Taint in a struct that implements flag.Value to allow taints to be
 // bound to command line flags.
 func NewTaintsVar(ptr *[]api.Taint) taintsVar {
@@ -138,6 +145,14 @@ func ParseTaints(spec []string) ([]v1.Taint, []v1.Taint, error) {
 				taintKey = parts[0]
 				effect = v1.TaintEffect(parts[1])
 			}
+
+			// If effect is specified, need to validate it.
+			if len(effect) > 0 {
+				err := validateTaintEffect(effect)
+				if err != nil {
+					return nil, nil, err
+				}
+			}
 			taintsToRemove = append(taintsToRemove, v1.Taint{Key: taintKey, Effect: effect})
 		} else {
 			return nil, nil, fmt.Errorf("unknown taint spec: %v", taintSpec)

pkg/util/taints/taints_test.go

@@ -111,3 +111,104 @@ func TestAddOrUpdateTaint(t *testing.T) {
 	newNode, result, err = AddOrUpdateTaint(node, taint)
 	checkResult("Add Duplicate Taint", newNode, taint, result, false, err)
 }
+
+func TestParseTaints(t *testing.T) {
+	cases := []struct {
+		name                   string
+		spec                   []string
+		expectedTaints         []v1.Taint
+		expectedTaintsToRemove []v1.Taint
+		expectedErr            bool
+	}{
+		{
+			name:        "invalid spec format",
+			spec:        []string{"foo=abc"},
+			expectedErr: true,
+		},
+		{
+			name:        "invalid spec effect for adding taint",
+			spec:        []string{"foo=abc:invalid_effect"},
+			expectedErr: true,
+		},
+		{
+			name:        "invalid spec effect for deleting taint",
+			spec:        []string{"foo:invalid_effect-"},
+			expectedErr: true,
+		},
+		{
+			name: "add new taints",
+			spec: []string{"foo=abc:NoSchedule", "bar=abc:NoSchedule"},
+			expectedTaints: []v1.Taint{
+				{
+					Key:    "foo",
+					Value:  "abc",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "bar",
+					Value:  "abc",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+			},
+			expectedErr: false,
+		},
+		{
+			name: "delete taints",
+			spec: []string{"foo:NoSchedule-", "bar:NoSchedule-"},
+			expectedTaintsToRemove: []v1.Taint{
+				{
+					Key:    "foo",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "bar",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+			},
+			expectedErr: false,
+		},
+		{
+			name: "add taints and delete taints",
+			spec: []string{"foo=abc:NoSchedule", "bar=abc:NoSchedule", "foo:NoSchedule-", "bar:NoSchedule-"},
+			expectedTaints: []v1.Taint{
+				{
+					Key:    "foo",
+					Value:  "abc",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "bar",
+					Value:  "abc",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+			},
+			expectedTaintsToRemove: []v1.Taint{
+				{
+					Key:    "foo",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "bar",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+			},
+			expectedErr: false,
+		},
+	}
+
+	for _, c := range cases {
+		taints, taintsToRemove, err := ParseTaints(c.spec)
+		if c.expectedErr && err == nil {
+			t.Errorf("[%s] expected error, but got nothing", c.name)
+		}
+		if !c.expectedErr && err != nil {
+			t.Errorf("[%s] expected no error, but got: %v", c.name, err)
+		}
+		if !reflect.DeepEqual(c.expectedTaints, taints) {
+			t.Errorf("[%s] expected returen taints as %v, but got: %v", c.name, c.expectedTaints, taints)
+		}
+		if !reflect.DeepEqual(c.expectedTaintsToRemove, taintsToRemove) {
+			t.Errorf("[%s] expected return taints to be removed as %v, but got: %v", c.name, c.expectedTaintsToRemove, taintsToRemove)
+		}
+	}
+}