diff --git a/cmd/draino/draino.go b/cmd/draino/draino.go index bd1ba5ab..c821d193 100644 --- a/cmd/draino/draino.go +++ b/cmd/draino/draino.go @@ -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() @@ -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 diff --git a/internal/kubernetes/nodefilters.go b/internal/kubernetes/nodefilters.go index 5aada7a0..7846e153 100644 --- a/internal/kubernetes/nodefilters.go +++ b/internal/kubernetes/nodefilters.go @@ -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 { @@ -126,5 +135,5 @@ func ConvertLabelsToFilterExpr(labels map[string]string) *string { } } temp := strings.Join(res, " && ") - return &temp + return &temp, nil } diff --git a/internal/kubernetes/nodefilters_test.go b/internal/kubernetes/nodefilters_test.go index c0c7f491..78aee695 100644 --- a/internal/kubernetes/nodefilters_test.go +++ b/internal/kubernetes/nodefilters_test.go @@ -220,7 +220,7 @@ func TestOldNodeLabelFilter(t *testing.T) { cases := []struct { name string obj interface{} - labels map[string]string + labels []string passesFilter bool }{ { @@ -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, }, { @@ -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, }, { @@ -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, }, { @@ -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, }, { @@ -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", @@ -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, }, { @@ -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, }, { @@ -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, }, { @@ -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, }, { @@ -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, }, } @@ -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 { @@ -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) + } + }) + } }