Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update v1.Taint parser to accept the form key:effect and key=:effect- #74159

Merged
merged 3 commits into from
Apr 2, 2019
Merged

Update v1.Taint parser to accept the form key:effect and key=:effect- #74159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
653c710
Update v1.Taint parser to accept `key:effect`, `key=:effect-`, `key`,…
Feb 16, 2019
f5ac239
Copy changes from `pkg/util/taints` to `pkg/kubectl/cmd/taint`
Feb 26, 2019
ab6d901
Update kubectl taint usage
Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions pkg/kubectl/cmd/taint/taint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.`))

Expand All @@ -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 {
Expand Down
78 changes: 44 additions & 34 deletions pkg/kubectl/cmd/taint/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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>'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why <key> is considered a valid taint here? It looks like it gets rejected later in parseTaints when adding a taint, but it is accepted when removing a taint. If this behavior is desired, it should be documented in the comments and the release note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read your comment above. It is fine, but please update comments and the release note to point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment to ParseTaints, since that's the exported method.

I do want to point out that the exported behavior has not changed. Prior to this PR, the ParseTaints function parsed some forms (including <key>) itself, but delegated parsing of other forms to parseTaint. This PR moves all parsing to parseTaint.

Given this, do you think updating the release note is necessary?

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
Expand Down
72 changes: 67 additions & 5 deletions pkg/kubectl/cmd/taint/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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{
{
Expand All @@ -448,6 +505,11 @@ func TestParseTaints(t *testing.T) {
Key: "bar",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "baz",
Value: "",
Effect: corev1.TaintEffectNoSchedule,
},
},
expectedErr: false,
},
Expand All @@ -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)
Expand Down
78 changes: 44 additions & 34 deletions pkg/util/taints/taints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
Loading