From 653c710b0db8f092c2d46201e178f866c82d0b58 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Fri, 15 Feb 2019 19:54:22 -0800 Subject: [PATCH] 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)