Skip to content

Commit

Permalink
Merge pull request planetlabs#108 from DataDog/david.benque/issue-107
Browse files Browse the repository at this point in the history 
fail to start if node-label is provided twice with the same key - issue planetlabs#107
  • Loading branch information
@jacobstr
jacobstr authored Dec 14, 2020
2 parents e0d5277 + f5f0f45 commit 9d39b53
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 25 deletions.
7 changes: 4 additions & 3 deletions cmd/draino/draino.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func main() {
maxGracePeriod = app.Flag("max-grace-period", "Maximum time evicted pods will be given to terminate gracefully.").Default(kubernetes.DefaultMaxGracePeriod.String()).Duration()
evictionHeadroom = app.Flag("eviction-headroom", "Additional time to wait after a pod's termination grace period for it to have been deleted.").Default(kubernetes.DefaultEvictionOverhead.String()).Duration()
drainBuffer = app.Flag("drain-buffer", "Minimum time between starting each drain. Nodes are always cordoned immediately.").Default(kubernetes.DefaultDrainBuffer.String()).Duration()
nodeLabels = app.Flag("node-label", "(Deprecated) Nodes with this label will be eligible for cordoning and draining. May be specified multiple times").StringMap()
nodeLabels = app.Flag("node-label", "(Deprecated) Nodes with this label will be eligible for cordoning and draining. May be specified multiple times").Strings()
nodeLabelsExpr = app.Flag("node-label-expr", "Nodes that match this expression will be eligible for cordoning and draining.").String()
namespace = app.Flag("namespace", "Namespace used to create leader election lock object.").Default("kube-system").String()

Expand Down Expand Up @@ -188,8 +188,9 @@ func main() {
if *nodeLabelsExpr != "" {
kingpin.Fatalf("nodeLabels and NodeLabelsExpr cannot both be set")
}

nodeLabelsExpr = kubernetes.ConvertLabelsToFilterExpr(*nodeLabels)
if nodeLabelsExpr, err = kubernetes.ConvertLabelsToFilterExpr(*nodeLabels); err != nil {
kingpin.Fatalf(err.Error())
}
}

var nodeLabelFilter cache.ResourceEventHandler
Expand Down
15 changes: 12 additions & 3 deletions internal/kubernetes/nodefilters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,18 @@ func (processed NodeProcessed) Filter(o interface{}) bool {
}

// ConvertLabelsToFilterExpr Convert old list labels into new expression syntax
func ConvertLabelsToFilterExpr(labels map[string]string) *string {
func ConvertLabelsToFilterExpr(labelsSlice []string) (*string, error) {
labels := map[string]string{}
for _, label := range labelsSlice {
tokens := strings.SplitN(label, "=", 2)
key := tokens[0]
value := tokens[1]
if v, found := labels[key]; found && v != value {
return nil, fmt.Errorf("node-label parameter is used twice with the same key and different values: '%s' , '%s", v, value)
}
labels[key] = value
}
res := []string{}

//sort the maps so that the unit tests actually work
keys := make([]string, 0, len(labels))
for k := range labels {
Expand All @@ -126,5 +135,5 @@ func ConvertLabelsToFilterExpr(labels map[string]string) *string {
}
}
temp := strings.Join(res, " && ")
return &temp
return &temp, nil
}
78 changes: 59 additions & 19 deletions internal/kubernetes/nodefilters_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
cases := []struct {
name string
obj interface{}
labels map[string]string
labels []string
passesFilter bool
}{
{
Expand All @@ -231,7 +231,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very"},
},
},
labels: map[string]string{"cool": "very"},
labels: []string{"cool=very"},
passesFilter: true,
},
{
Expand All @@ -242,7 +242,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"planetlabs.com/cool": "very"},
},
},
labels: map[string]string{"planetlabs.com/cool": "very"},
labels: []string{"planetlabs.com/cool=very"},
passesFilter: true,
},
{
Expand All @@ -253,7 +253,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very", "lame": "nope"},
},
},
labels: map[string]string{"cool": "very", "lame": "nope"},
labels: []string{"cool=very", "lame=nope"},
passesFilter: true,
},
{
Expand All @@ -264,7 +264,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "notsocool"},
},
},
labels: map[string]string{"cool": "very"},
labels: []string{"cool=very"},
passesFilter: false,
},
{
Expand All @@ -275,7 +275,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very", "lame": "somehowyes"},
},
},
labels: map[string]string{"cool": "very", "lame": "nope"},
labels: []string{"cool=very", "lame=nope"},
passesFilter: false,
}, {
name: "PartiallyAbsentLabels",
Expand All @@ -285,7 +285,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very"},
},
},
labels: map[string]string{"cool": "very", "lame": "nope"},
labels: []string{"cool=very", "lame=nope"},
passesFilter: false,
},
{
Expand All @@ -296,7 +296,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{},
},
},
labels: map[string]string{"cool": "very"},
labels: []string{"cool=very"},
passesFilter: false,
},
{
Expand All @@ -317,7 +317,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very"},
},
},
labels: map[string]string{"keyWithNoValue": ""},
labels: []string{"keyWithNoValue="},
passesFilter: false,
},
{
Expand All @@ -328,7 +328,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very", "keyWithNoValue": ""},
},
},
labels: map[string]string{"keyWithNoValue": ""},
labels: []string{"keyWithNoValue="},
passesFilter: true,
},
{
Expand All @@ -339,7 +339,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
Labels: map[string]string{"cool": "very"},
},
},
labels: map[string]string{"cool": "very"},
labels: []string{"cool=very"},
passesFilter: false,
},
}
Expand All @@ -348,7 +348,7 @@ func TestOldNodeLabelFilter(t *testing.T) {

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
labelExpr := ConvertLabelsToFilterExpr(tc.labels)
labelExpr, err := ConvertLabelsToFilterExpr(tc.labels)

filter, err := NewNodeLabelFilter(labelExpr, log)
if err != nil {
Expand Down Expand Up @@ -437,13 +437,53 @@ func TestParseConditions(t *testing.T) {
}

func TestConvertLabelsToFilterExpr(t *testing.T) {
input := map[string]string{
"foo": "bar",
"sup": "cool",
cases := []struct {
name string
input []string
expected string
wantErr bool
}{
{
name: "2 labels",
input: []string{"foo=bar", "sup=cool"},
expected: "metadata.labels['foo'] == 'bar' && metadata.labels['sup'] == 'cool'",
},
{
name: "2 labels same key",
input: []string{"foo=bar", "foo=cool"},
expected: "",
wantErr: true,
},
{
name: "no filter",
input: nil,
expected: "",
wantErr: false,
},
}

desired := "metadata.labels['foo'] == 'bar' && metadata.labels['sup'] == 'cool'"
actual := ConvertLabelsToFilterExpr(input)

assert.Equal(t, desired, *actual)
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
actual, err := ConvertLabelsToFilterExpr(tc.input)
if tc.wantErr && err == nil {
t.Errorf("error was expected for that case")
return
}
if !tc.wantErr && err != nil {
t.Errorf("no error was expected for that case")
return
}
if tc.wantErr && err != nil {
return
}
if actual == nil {
t.Errorf("string value was expected")
return
}
got := *actual
if !reflect.DeepEqual(tc.expected, got) {
t.Errorf("expect %v, got: %v", tc.expected, got)
}
})
}
}

0 comments on commit 9d39b53

Please sign in to comment.