From 9cd219969fc3a2892a21a5cfa1b373905335b071 Mon Sep 17 00:00:00 2001
From: Xing Zhou <xingzhou@cn.ibm.com>
Date: Thu, 14 Sep 2017 13:37:17 +0800
Subject: [PATCH] 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.
---
 pkg/util/taints/taints.go      |  23 ++++++--
 pkg/util/taints/taints_test.go | 101 +++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+), 4 deletions(-)

diff --git a/pkg/util/taints/taints.go b/pkg/util/taints/taints.go
index 5957adfb278..5b7ff0d81ee 100644
--- a/pkg/util/taints/taints.go
+++ b/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 {
@@ -134,6 +141,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)
diff --git a/pkg/util/taints/taints_test.go b/pkg/util/taints/taints_test.go
index e27e6ad37ab..280cb616d0a 100644
--- a/pkg/util/taints/taints_test.go
+++ b/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)
+		}
+	}
+}