Merge pull request #74159 from dlipovetsky/issue-73249-revise-parsetaint

pkg/kubectl/cmd/taint/taint.go

@@ -64,7 +64,7 @@ var (
 		* A taint consists of a key, value, and effect. As an argument here, it is expressed as key=value:effect.
 		* The key must begin with a letter or number, and may contain letters, numbers, hyphens, dots, and underscores, up to %[1]d characters.
 		* Optionally, the key can begin with a DNS subdomain prefix and a single '/', like example.com/my-app
-		* The value must begin with a letter or number, and may contain letters, numbers, hyphens, dots, and underscores, up to %[2]d characters.
+		* The value is optional. If given, it must begin with a letter or number, and may contain letters, numbers, hyphens, dots, and underscores, up to %[2]d characters.
 		* The effect must be NoSchedule, PreferNoSchedule or NoExecute.
 		* Currently taint can only apply to node.`))
 
@@ -80,7 +80,10 @@ var (
 		kubectl taint nodes foo dedicated-
 
 		# Add a taint with key 'dedicated' on nodes having label mylabel=X
-		kubectl taint node -l myLabel=X  dedicated=foo:PreferNoSchedule`))
+		kubectl taint node -l myLabel=X  dedicated=foo:PreferNoSchedule
+
+		# Add to node 'foo' a taint with key 'bar' and no value
+		kubectl taint nodes foo bar:NoSchedule`))
 )
 
 func NewCmdTaint(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.Command {

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 {
-		return taint, fmt.Errorf("invalid taint spec: %v", st)
-	}
 
-	parts2 := strings.Split(parts[1], ":")
+	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
+		}
 
-	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, "; "))
+		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)
 	}
 
-	effect := corev1.TaintEffect(parts2[1])
-	if err := validateTaintEffect(effect); err != nil {
-		return taint, err
+	if errs := validation.IsQualifiedName(key); len(errs) > 0 {
+		return taint, fmt.Errorf("invalid taint spec: %v, %s", st, strings.Join(errs, "; "))
 	}
 
-	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

@@ -379,10 +379,30 @@ func TestParseTaints(t *testing.T) {
 	}{
 		{
 			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"},
 			expectedErr: true,
@@ -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)

pkg/util/taints/taints.go

@@ -35,28 +35,46 @@ const (
 	UNTAINTED = "untainted"
 )
 
-// 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) (v1.Taint, error) {
 	var taint v1.Taint
-	parts := strings.Split(st, "=")
-	if len(parts) != 2 || len(parts[1]) == 0 || len(validation.IsQualifiedName(parts[0])) > 0 {
-		return taint, fmt.Errorf("invalid taint spec: %v", st)
-	}
 
-	parts2 := strings.Split(parts[1], ":")
+	var key string
+	var value string
+	var effect v1.TaintEffect
+
+	parts := strings.Split(st, ":")
+	switch len(parts) {
+	case 1:
+		key = parts[0]
+	case 2:
+		effect = v1.TaintEffect(parts[1])
+		if err := validateTaintEffect(effect); err != nil {
+			return taint, err
+		}
 
-	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, "; "))
+		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)
 	}
 
-	effect := v1.TaintEffect(parts2[1])
-	if err := validateTaintEffect(effect); err != nil {
-		return taint, err
+	if errs := validation.IsQualifiedName(key); len(errs) > 0 {
+		return taint, fmt.Errorf("invalid taint spec: %v, %s", st, strings.Join(errs, "; "))
 	}
 
-	taint.Key = parts[0]
-	taint.Value = parts2[0]
+	taint.Key = key
+	taint.Value = value
 	taint.Effect = effect
 
 	return taint, nil
@@ -116,16 +134,27 @@ func (t taintsVar) Type() string {
 }
 
 // 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) ([]v1.Taint, []v1.Taint, error) {
 	var taints, taintsToRemove []v1.Taint
 	uniqueTaints := map[v1.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, v1.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)
@@ -137,25 +166,6 @@ func ParseTaints(spec []string) ([]v1.Taint, []v1.Taint, error) {
 			uniqueTaints[newTaint.Effect].Insert(newTaint.Key)
 
 			taints = append(taints, newTaint)
-		} else if strings.HasSuffix(taintSpec, "-") {
-			taintKey := taintSpec[:len(taintSpec)-1]
-			var effect v1.TaintEffect
-			if strings.Index(taintKey, ":") != -1 {
-				parts := strings.Split(taintKey, ":")
-				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)
 		}
 	}
 	return taints, taintsToRemove, nil

pkg/util/taints/taints_test.go

@@ -42,15 +42,25 @@ func TestTaintsVar(t *testing.T) {
 			t: []api.Taint{{Key: "foo", Value: "bar", Effect: "NoSchedule"}},
 		},
 		{
-			f: "--t=foo=bar:NoSchedule,bing=bang:PreferNoSchedule",
+			f: "--t=baz:NoSchedule",
+			t: []api.Taint{{Key: "baz", Value: "", Effect: "NoSchedule"}},
+		},
+		{
+			f: "--t=foo=bar:NoSchedule,baz:NoSchedule,bing=bang:PreferNoSchedule,qux=:NoSchedule",
 			t: []api.Taint{
 				{Key: "foo", Value: "bar", Effect: api.TaintEffectNoSchedule},
+				{Key: "baz", Value: "", Effect: "NoSchedule"},
 				{Key: "bing", Value: "bang", Effect: api.TaintEffectPreferNoSchedule},
+				{Key: "qux", Value: "", Effect: "NoSchedule"},
 			},
 		},
 		{
-			f: "--t=dedicated-for=user1:NoExecute",
-			t: []api.Taint{{Key: "dedicated-for", Value: "user1", Effect: "NoExecute"}},
+			f: "--t=dedicated-for=user1:NoExecute,baz:NoSchedule,foo-bar=:NoSchedule",
+			t: []api.Taint{
+				{Key: "dedicated-for", Value: "user1", Effect: "NoExecute"},
+				{Key: "baz", Value: "", Effect: "NoSchedule"},
+				{Key: "foo-bar", Value: "", Effect: "NoSchedule"},
+			},
 		},
 	}
 
@@ -595,10 +605,30 @@ func TestParseTaints(t *testing.T) {
 	}{
 		{
 			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"},
 			expectedErr: true,
@@ -610,7 +640,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: []v1.Taint{
 				{
 					Key:    "foo",
@@ -622,12 +652,27 @@ func TestParseTaints(t *testing.T) {
 					Value:  "abc",
 					Effect: v1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "baz",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "qux",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "foobar",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
 			},
 			expectedErr: false,
 		},
 		{
 			name: "delete taints",
-			spec: []string{"foo:NoSchedule-", "bar:NoSchedule-"},
+			spec: []string{"foo:NoSchedule-", "bar:NoSchedule-", "qux=:NoSchedule-", "dedicated-"},
 			expectedTaintsToRemove: []v1.Taint{
 				{
 					Key:    "foo",
@@ -637,12 +682,19 @@ func TestParseTaints(t *testing.T) {
 					Key:    "bar",
 					Effect: v1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "qux",
+					Effect: v1.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: []v1.Taint{
 				{
 					Key:    "foo",
@@ -654,6 +706,21 @@ func TestParseTaints(t *testing.T) {
 					Value:  "abc",
 					Effect: v1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "baz",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "qux",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
+				{
+					Key:    "foobar",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
 			},
 			expectedTaintsToRemove: []v1.Taint{
 				{
@@ -664,6 +731,11 @@ func TestParseTaints(t *testing.T) {
 					Key:    "bar",
 					Effect: v1.TaintEffectNoSchedule,
 				},
+				{
+					Key:    "baz",
+					Value:  "",
+					Effect: v1.TaintEffectNoSchedule,
+				},
 			},
 			expectedErr: false,
 		},
@@ -672,10 +744,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)