From 653c710b0db8f092c2d46201e178f866c82d0b58 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Fri, 15 Feb 2019 19:54:22 -0800 Subject: [PATCH 1/3] Update v1.Taint parser to accept `key:effect`, `key=:effect-`, `key`, and `key-` forms Also add missing tests for `key=:value` form. --- pkg/util/taints/taints.go | 78 +++++++++++++++++------------- pkg/util/taints/taints_test.go | 88 ++++++++++++++++++++++++++++++---- 2 files changed, 124 insertions(+), 42 deletions(-) diff --git a/pkg/util/taints/taints.go b/pkg/util/taints/taints.go index 0777f6d6922..a3eaca37036 100644 --- a/pkg/util/taints/taints.go +++ b/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 '=:'. +// parseTaint parses a taint from a string, whose form must be either +// '=:', ':', or ''. 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 { + + 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 + } + + 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 := v1.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 @@ -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 `` 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 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 diff --git a/pkg/util/taints/taints_test.go b/pkg/util/taints/taints_test.go index 110d78f787c..a2dfca67c85 100644 --- a/pkg/util/taints/taints_test.go +++ b/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"}, + }, }, } @@ -593,11 +603,31 @@ func TestParseTaints(t *testing.T) { expectedTaintsToRemove []v1.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"}, @@ -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) From f5ac2392890ada134ed4e10bee41251894351c68 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Mon, 25 Feb 2019 16:15:18 -0800 Subject: [PATCH 2/3] Copy changes from `pkg/util/taints` to `pkg/kubectl/cmd/taint` --- pkg/kubectl/cmd/taint/utils.go | 78 ++++++++++++++++------------- pkg/kubectl/cmd/taint/utils_test.go | 72 ++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 39 deletions(-) diff --git a/pkg/kubectl/cmd/taint/utils.go b/pkg/kubectl/cmd/taint/utils.go index 46245fb24ec..d3ec76d4086 100644 --- a/pkg/kubectl/cmd/taint/utils.go +++ b/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 `` 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 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 '=:'. +// parseTaint parses a taint from a string, whose form must be either +// '=:', ':', or ''. 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 diff --git a/pkg/kubectl/cmd/taint/utils_test.go b/pkg/kubectl/cmd/taint/utils_test.go index 1004193dc04..e3dc1ac0a8f 100644 --- a/pkg/kubectl/cmd/taint/utils_test.go +++ b/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) From ab6d901497692e6cbe1f8eed9bc2fc70b5d92804 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Mon, 25 Feb 2019 16:21:09 -0800 Subject: [PATCH 3/3] Update kubectl taint usage - Explain that the value is optional. - Add example of adding a taint with no value to kubectl taint usage. --- pkg/kubectl/cmd/taint/taint.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/kubectl/cmd/taint/taint.go b/pkg/kubectl/cmd/taint/taint.go index 02f56513079..8f7a7d0c542 100644 --- a/pkg/kubectl/cmd/taint/taint.go +++ b/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 {