Skip to content

Commit 9d39b53

Browse files
jacobstrjacobstr
authored
Merge pull request #108 from DataDog/david.benque/issue-107
fail to start if node-label is provided twice with the same key - issue #107
2 parents e0d5277 + f5f0f45 commit 9d39b53

File tree

3 files changed

+75
-25
lines changed

3 files changed

+75
-25
lines changed

‎cmd/draino/draino.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func main() {
5858
maxGracePeriod = app.Flag("max-grace-period", "Maximum time evicted pods will be given to terminate gracefully.").Default(kubernetes.DefaultMaxGracePeriod.String()).Duration()
5959
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()
6060
drainBuffer = app.Flag("drain-buffer", "Minimum time between starting each drain. Nodes are always cordoned immediately.").Default(kubernetes.DefaultDrainBuffer.String()).Duration()
61-
nodeLabels = app.Flag("node-label", "(Deprecated) Nodes with this label will be eligible for cordoning and draining. May be specified multiple times").StringMap()
61+
nodeLabels = app.Flag("node-label", "(Deprecated) Nodes with this label will be eligible for cordoning and draining. May be specified multiple times").Strings()
6262
nodeLabelsExpr = app.Flag("node-label-expr", "Nodes that match this expression will be eligible for cordoning and draining.").String()
6363
namespace = app.Flag("namespace", "Namespace used to create leader election lock object.").Default("kube-system").String()
6464

@@ -188,8 +188,9 @@ func main() {
188188
if *nodeLabelsExpr != "" {
189189
kingpin.Fatalf("nodeLabels and NodeLabelsExpr cannot both be set")
190190
}
191-
192-
nodeLabelsExpr = kubernetes.ConvertLabelsToFilterExpr(*nodeLabels)
191+
if nodeLabelsExpr, err = kubernetes.ConvertLabelsToFilterExpr(*nodeLabels); err != nil {
192+
kingpin.Fatalf(err.Error())
193+
}
193194
}
194195

195196
var nodeLabelFilter cache.ResourceEventHandler

‎internal/kubernetes/nodefilters.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,18 @@ func (processed NodeProcessed) Filter(o interface{}) bool {
108108
}
109109

110110
// ConvertLabelsToFilterExpr Convert old list labels into new expression syntax
111-
func ConvertLabelsToFilterExpr(labels map[string]string) *string {
111+
func ConvertLabelsToFilterExpr(labelsSlice []string) (*string, error) {
112+
labels := map[string]string{}
113+
for _, label := range labelsSlice {
114+
tokens := strings.SplitN(label, "=", 2)
115+
key := tokens[0]
116+
value := tokens[1]
117+
if v, found := labels[key]; found && v != value {
118+
return nil, fmt.Errorf("node-label parameter is used twice with the same key and different values: '%s' , '%s", v, value)
119+
}
120+
labels[key] = value
121+
}
112122
res := []string{}
113-
114123
//sort the maps so that the unit tests actually work
115124
keys := make([]string, 0, len(labels))
116125
for k := range labels {
@@ -126,5 +135,5 @@ func ConvertLabelsToFilterExpr(labels map[string]string) *string {
126135
}
127136
}
128137
temp := strings.Join(res, " && ")
129-
return &temp
138+
return &temp, nil
130139
}

‎internal/kubernetes/nodefilters_test.go

Lines changed: 59 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
220220
cases := []struct {
221221
name string
222222
obj interface{}
223-
labels map[string]string
223+
labels []string
224224
passesFilter bool
225225
}{
226226
{
@@ -231,7 +231,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
231231
Labels: map[string]string{"cool": "very"},
232232
},
233233
},
234-
labels: map[string]string{"cool": "very"},
234+
labels: []string{"cool=very"},
235235
passesFilter: true,
236236
},
237237
{
@@ -242,7 +242,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
242242
Labels: map[string]string{"planetlabs.com/cool": "very"},
243243
},
244244
},
245-
labels: map[string]string{"planetlabs.com/cool": "very"},
245+
labels: []string{"planetlabs.com/cool=very"},
246246
passesFilter: true,
247247
},
248248
{
@@ -253,7 +253,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
253253
Labels: map[string]string{"cool": "very", "lame": "nope"},
254254
},
255255
},
256-
labels: map[string]string{"cool": "very", "lame": "nope"},
256+
labels: []string{"cool=very", "lame=nope"},
257257
passesFilter: true,
258258
},
259259
{
@@ -264,7 +264,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
264264
Labels: map[string]string{"cool": "notsocool"},
265265
},
266266
},
267-
labels: map[string]string{"cool": "very"},
267+
labels: []string{"cool=very"},
268268
passesFilter: false,
269269
},
270270
{
@@ -275,7 +275,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
275275
Labels: map[string]string{"cool": "very", "lame": "somehowyes"},
276276
},
277277
},
278-
labels: map[string]string{"cool": "very", "lame": "nope"},
278+
labels: []string{"cool=very", "lame=nope"},
279279
passesFilter: false,
280280
}, {
281281
name: "PartiallyAbsentLabels",
@@ -285,7 +285,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
285285
Labels: map[string]string{"cool": "very"},
286286
},
287287
},
288-
labels: map[string]string{"cool": "very", "lame": "nope"},
288+
labels: []string{"cool=very", "lame=nope"},
289289
passesFilter: false,
290290
},
291291
{
@@ -296,7 +296,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
296296
Labels: map[string]string{},
297297
},
298298
},
299-
labels: map[string]string{"cool": "very"},
299+
labels: []string{"cool=very"},
300300
passesFilter: false,
301301
},
302302
{
@@ -317,7 +317,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
317317
Labels: map[string]string{"cool": "very"},
318318
},
319319
},
320-
labels: map[string]string{"keyWithNoValue": ""},
320+
labels: []string{"keyWithNoValue="},
321321
passesFilter: false,
322322
},
323323
{
@@ -328,7 +328,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
328328
Labels: map[string]string{"cool": "very", "keyWithNoValue": ""},
329329
},
330330
},
331-
labels: map[string]string{"keyWithNoValue": ""},
331+
labels: []string{"keyWithNoValue="},
332332
passesFilter: true,
333333
},
334334
{
@@ -339,7 +339,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
339339
Labels: map[string]string{"cool": "very"},
340340
},
341341
},
342-
labels: map[string]string{"cool": "very"},
342+
labels: []string{"cool=very"},
343343
passesFilter: false,
344344
},
345345
}
@@ -348,7 +348,7 @@ func TestOldNodeLabelFilter(t *testing.T) {
348348

349349
for _, tc := range cases {
350350
t.Run(tc.name, func(t *testing.T) {
351-
labelExpr := ConvertLabelsToFilterExpr(tc.labels)
351+
labelExpr, err := ConvertLabelsToFilterExpr(tc.labels)
352352

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

439439
func TestConvertLabelsToFilterExpr(t *testing.T) {
440-
input := map[string]string{
441-
"foo": "bar",
442-
"sup": "cool",
440+
cases := []struct {
441+
name string
442+
input []string
443+
expected string
444+
wantErr bool
445+
}{
446+
{
447+
name: "2 labels",
448+
input: []string{"foo=bar", "sup=cool"},
449+
expected: "metadata.labels['foo'] == 'bar' && metadata.labels['sup'] == 'cool'",
450+
},
451+
{
452+
name: "2 labels same key",
453+
input: []string{"foo=bar", "foo=cool"},
454+
expected: "",
455+
wantErr: true,
456+
},
457+
{
458+
name: "no filter",
459+
input: nil,
460+
expected: "",
461+
wantErr: false,
462+
},
443463
}
444464

445-
desired := "metadata.labels['foo'] == 'bar' && metadata.labels['sup'] == 'cool'"
446-
actual := ConvertLabelsToFilterExpr(input)
447-
448-
assert.Equal(t, desired, *actual)
465+
for _, tc := range cases {
466+
t.Run(tc.name, func(t *testing.T) {
467+
actual, err := ConvertLabelsToFilterExpr(tc.input)
468+
if tc.wantErr && err == nil {
469+
t.Errorf("error was expected for that case")
470+
return
471+
}
472+
if !tc.wantErr && err != nil {
473+
t.Errorf("no error was expected for that case")
474+
return
475+
}
476+
if tc.wantErr && err != nil {
477+
return
478+
}
479+
if actual == nil {
480+
t.Errorf("string value was expected")
481+
return
482+
}
483+
got := *actual
484+
if !reflect.DeepEqual(tc.expected, got) {
485+
t.Errorf("expect %v, got: %v", tc.expected, got)
486+
}
487+
})
488+
}
449489
}

0 commit comments

Comments
 (0)