Copy changes from `pkg/util/taints` to `pkg/kubectl/cmd/taint`

pkg/kubectl/cmd/taint/utils.go

@@ -35,16 +35,27 @@ const (
 )
 
 // parseTaints takes a spec which is an array and creates slices for new taints to be added, taints to be deleted.
+// It also validates the spec. For example, the form `<key>` may be used to remove a taint, but not to add one.
 func parseTaints(spec []string) ([]corev1.Taint, []corev1.Taint, error) {
 	var taints, taintsToRemove []corev1.Taint
 	uniqueTaints := map[corev1.TaintEffect]sets.String{}
 
 	for _, taintSpec := range spec {
-		if strings.Index(taintSpec, "=") != -1 && strings.Index(taintSpec, ":") != -1 {
+		if strings.HasSuffix(taintSpec, "-") {
+			taintToRemove, err := parseTaint(strings.TrimSuffix(taintSpec, "-"))
+			if err != nil {
+				return nil, nil, err
+			}
+			taintsToRemove = append(taintsToRemove, corev1.Taint{Key: taintToRemove.Key, Effect: taintToRemove.Effect})
+		} else {
 			newTaint, err := parseTaint(taintSpec)
 			if err != nil {
 				return nil, nil, err
 			}
+			// validate that the taint has an effect, which is required to add the taint
+			if len(newTaint.Effect) == 0 {
+				return nil, nil, fmt.Errorf("invalid taint spec: %v", taintSpec)
+			}
 			// validate if taint is unique by <key, effect>
 			if len(uniqueTaints[newTaint.Effect]) > 0 && uniqueTaints[newTaint.Effect].Has(newTaint.Key) {
 				return nil, nil, fmt.Errorf("duplicated taints with the same key and effect: %v", newTaint)
@@ -56,52 +67,51 @@ func parseTaints(spec []string) ([]corev1.Taint, []corev1.Taint, error) {
 			uniqueTaints[newTaint.Effect].Insert(newTaint.Key)
 
 			taints = append(taints, newTaint)
-		} else if strings.HasSuffix(taintSpec, "-") {
-			taintKey := taintSpec[:len(taintSpec)-1]
-			var effect corev1.TaintEffect
-			if strings.Index(taintKey, ":") != -1 {
-				parts := strings.Split(taintKey, ":")
-				taintKey = parts[0]
-				effect = corev1.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, corev1.Taint{Key: taintKey, Effect: effect})
-		} else {
-			return nil, nil, fmt.Errorf("unknown taint spec: %v", taintSpec)
 		}
 	}
 	return taints, taintsToRemove, nil
 }
 
-// parseTaint parses a taint from a string. Taint must be of the format '<key>=<value>:<effect>'.
+// parseTaint parses a taint from a string, whose form must be either
+// '<key>=<value>:<effect>', '<key>:<effect>', or '<key>'.
 func parseTaint(st string) (corev1.Taint, error) {
 	var taint corev1.Taint
-	parts := strings.Split(st, "=")
-	if len(parts) != 2 || len(parts[1]) == 0 || len(validation.IsQualifiedName(parts[0])) > 0 {
+
+	var key string
+	var value string
+	var effect corev1.TaintEffect
+
+	parts := strings.Split(st, ":")
+	switch len(parts) {
+	case 1:
+		key = parts[0]
+	case 2:
+		effect = corev1.TaintEffect(parts[1])
+		if err := validateTaintEffect(effect); err != nil {
+			return taint, err
+		}
+
+		partsKV := strings.Split(parts[0], "=")
+		if len(partsKV) > 2 {
+			return taint, fmt.Errorf("invalid taint spec: %v", st)
+		}
+		key = partsKV[0]
+		if len(partsKV) == 2 {
+			value = partsKV[1]
+			if errs := validation.IsValidLabelValue(value); len(errs) > 0 {
+				return taint, fmt.Errorf("invalid taint spec: %v, %s", st, strings.Join(errs, "; "))
+			}
+		}
+	default:
 		return taint, fmt.Errorf("invalid taint spec: %v", st)
 	}
 
-	parts2 := strings.Split(parts[1], ":")
-
-	errs := validation.IsValidLabelValue(parts2[0])
-	if len(parts2) != 2 || len(errs) != 0 {
+	if errs := validation.IsQualifiedName(key); len(errs) > 0 {
 		return taint, fmt.Errorf("invalid taint spec: %v, %s", st, strings.Join(errs, "; "))
 	}
 
-	effect := corev1.TaintEffect(parts2[1])
-	if err := validateTaintEffect(effect); err != nil {
-		return taint, err
-	}
-
-	taint.Key = parts[0]
-	taint.Value = parts2[0]
+	taint.Key = key
+	taint.Value = value
 	taint.Effect = effect
 
 	return taint, nil

pkg/kubectl/cmd/taint/utils_test.go

@@ -377,11 +377,31 @@ func TestParseTaints(t *testing.T) {
 		expectedTaintsToRemove []corev1.Taint
 		expectedErr            bool
 	}{
+		{
+			name:        "invalid spec format",
+			spec:        []string{""},
+			expectedErr: true,
+		},
 		{
 			name:        "invalid spec format",
 			spec:        []string{"foo=abc"},
 			expectedErr: true,
 		},
+		{
+			name:        "invalid spec format",
+			spec:        []string{"foo=abc=xyz:NoSchedule"},
+			expectedErr: true,
+		},
+		{
+			name:        "invalid spec format",
+			spec:        []string{"foo=abc:xyz:NoSchedule"},
+			expectedErr: true,
+		},
+		{
+			name:        "invalid spec format for adding taint",
+			spec:        []string{"foo"},
+			expectedErr: true,
+		},
 		{
 			name:        "invalid spec effect for adding taint",
 			spec:        []string{"foo=abc:invalid_effect"},
@@ -394,7 +414,7 @@ func TestParseTaints(t *testing.T) {
 		},
 		{
 			name: "add new taints",
-			spec: []string{"foo=abc:NoSchedule", "bar=abc:NoSchedule"},
+			spec: []string{"foo=abc:NoSchedule", "bar=abc:NoSchedule", "baz:NoSchedule", "qux:NoSchedule", "foobar=:NoSchedule"},
 			expectedTaints: []corev1.Taint{
 				{
 					Key:    "foo",
@@ -406,12 +426,27 @@ func TestParseTaints(t *testing.T) {
 					Value:  "abc",
 					Effect: corev1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "baz",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "qux",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "foobar",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
 			},
 			expectedErr: false,
 		},
 		{
 			name: "delete taints",
-			spec: []string{"foo:NoSchedule-", "bar:NoSchedule-"},
+			spec: []string{"foo:NoSchedule-", "bar:NoSchedule-", "qux=:NoSchedule-", "dedicated-"},
 			expectedTaintsToRemove: []corev1.Taint{
 				{
 					Key:    "foo",
@@ -421,12 +456,19 @@ func TestParseTaints(t *testing.T) {
 					Key:    "bar",
 					Effect: corev1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "qux",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key: "dedicated",
+				},
 			},
 			expectedErr: false,
 		},
 		{
 			name: "add taints and delete taints",
-			spec: []string{"foo=abc:NoSchedule", "bar=abc:NoSchedule", "foo:NoSchedule-", "bar:NoSchedule-"},
+			spec: []string{"foo=abc:NoSchedule", "bar=abc:NoSchedule", "baz:NoSchedule", "qux:NoSchedule", "foobar=:NoSchedule", "foo:NoSchedule-", "bar:NoSchedule-", "baz=:NoSchedule-"},
 			expectedTaints: []corev1.Taint{
 				{
 					Key:    "foo",
@@ -438,6 +480,21 @@ func TestParseTaints(t *testing.T) {
 					Value:  "abc",
 					Effect: corev1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "baz",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "qux",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "foobar",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
 			},
 			expectedTaintsToRemove: []corev1.Taint{
 				{
@@ -448,6 +505,11 @@ func TestParseTaints(t *testing.T) {
 					Key:    "bar",
 					Effect: corev1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "baz",
+					Value:  "",
+					Effect: corev1.TaintEffectNoSchedule,
+				},
 			},
 			expectedErr: false,
 		},
@@ -456,10 +518,10 @@ func TestParseTaints(t *testing.T) {
 	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)
+			t.Errorf("[%s] expected error for spec %s, but got nothing", c.name, c.spec)
 		}
 		if !c.expectedErr && err != nil {
-			t.Errorf("[%s] expected no error, but got: %v", c.name, err)
+			t.Errorf("[%s] expected no error for spec %s, but got: %v", c.name, c.spec, err)
 		}
 		if !reflect.DeepEqual(c.expectedTaints, taints) {
 			t.Errorf("[%s] expected returen taints as %v, but got: %v", c.name, c.expectedTaints, taints)