From e5e0de1b9d3ebe0ba4ba78547e2558fe030cd28b Mon Sep 17 00:00:00 2001 From: Wei Huang Date: Fri, 20 Jul 2018 17:38:50 -0700 Subject: [PATCH] fix sorting behavior in selector.go - move sorting from NewRequirement() out to String() - add related unit tests - add unit tests in one of outer callers (pkg/apis/core/v1/helper) Closes #66298 --- pkg/apis/core/v1/helper/helpers_test.go | 297 ++++++++++++++++++ .../pkg/apis/meta/v1/helpers_test.go | 11 +- .../apimachinery/pkg/labels/selector.go | 16 +- .../apimachinery/pkg/labels/selector_test.go | 44 +++ 4 files changed, 364 insertions(+), 4 deletions(-) diff --git a/pkg/apis/core/v1/helper/helpers_test.go b/pkg/apis/core/v1/helper/helpers_test.go index e914d273339..02d21e7f2b1 100644 --- a/pkg/apis/core/v1/helper/helpers_test.go +++ b/pkg/apis/core/v1/helper/helpers_test.go @@ -811,6 +811,303 @@ func TestMatchNodeSelectorTerms(t *testing.T) { } } +// TestMatchNodeSelectorTermsStateless ensures MatchNodeSelectorTerms() +// is invoked in a "stateless" manner, i.e. nodeSelectorTerms should NOT +// be deeply modifed after invoking +func TestMatchNodeSelectorTermsStateless(t *testing.T) { + type args struct { + nodeSelectorTerms []v1.NodeSelectorTerm + nodeLabels labels.Set + nodeFields fields.Set + } + + tests := []struct { + name string + args args + want []v1.NodeSelectorTerm + }{ + { + name: "nil terms", + args: args{ + nodeSelectorTerms: nil, + nodeLabels: nil, + nodeFields: nil, + }, + want: nil, + }, + { + name: "nodeLabels: preordered matchExpressions and nil matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_1_val", "label_2_val"}, + }}, + }, + }, + nodeLabels: map[string]string{"label_1": "label_1_val"}, + nodeFields: nil, + }, + want: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_1_val", "label_2_val"}, + }}, + }, + }, + }, + { + name: "nodeLabels: unordered matchExpressions and nil matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_2_val", "label_1_val"}, + }}, + }, + }, + nodeLabels: map[string]string{"label_1": "label_1_val"}, + nodeFields: nil, + }, + want: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_2_val", "label_1_val"}, + }}, + }, + }, + }, + { + name: "nodeFields: nil matchExpressions and preordered matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_1", "host_2"}, + }}, + }, + }, + nodeLabels: nil, + nodeFields: map[string]string{ + "metadata.name": "host_1", + }, + }, + want: []v1.NodeSelectorTerm{ + { + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_1", "host_2"}, + }}, + }, + }, + }, + { + name: "nodeFields: nil matchExpressions and unordered matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_2", "host_1"}, + }}, + }, + }, + nodeLabels: nil, + nodeFields: map[string]string{ + "metadata.name": "host_1", + }, + }, + want: []v1.NodeSelectorTerm{ + { + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_2", "host_1"}, + }}, + }, + }, + }, + { + name: "nodeLabels and nodeFields: ordered matchExpressions and ordered matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_1_val", "label_2_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_1", "host_2"}, + }}, + }, + }, + nodeLabels: map[string]string{ + "label_1": "label_1_val", + }, + nodeFields: map[string]string{ + "metadata.name": "host_1", + }, + }, + want: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_1_val", "label_2_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_1", "host_2"}, + }}, + }, + }, + }, + { + name: "nodeLabels and nodeFields: ordered matchExpressions and unordered matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_1_val", "label_2_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_2", "host_1"}, + }}, + }, + }, + nodeLabels: map[string]string{ + "label_1": "label_1_val", + }, + nodeFields: map[string]string{ + "metadata.name": "host_1", + }, + }, + want: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_1_val", "label_2_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_2", "host_1"}, + }}, + }, + }, + }, + { + name: "nodeLabels and nodeFields: unordered matchExpressions and ordered matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_2_val", "label_1_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_1", "host_2"}, + }}, + }, + }, + nodeLabels: map[string]string{ + "label_1": "label_1_val", + }, + nodeFields: map[string]string{ + "metadata.name": "host_1", + }, + }, + want: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_2_val", "label_1_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_1", "host_2"}, + }}, + }, + }, + }, + { + name: "nodeLabels and nodeFields: unordered matchExpressions and unordered matchFields", + args: args{ + nodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_2_val", "label_1_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_2", "host_1"}, + }}, + }, + }, + nodeLabels: map[string]string{ + "label_1": "label_1_val", + }, + nodeFields: map[string]string{ + "metadata.name": "host_1", + }, + }, + want: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{{ + Key: "label_1", + Operator: v1.NodeSelectorOpIn, + Values: []string{"label_2_val", "label_1_val"}, + }}, + MatchFields: []v1.NodeSelectorRequirement{{ + Key: "metadata.name", + Operator: v1.NodeSelectorOpIn, + Values: []string{"host_2", "host_1"}, + }}, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + MatchNodeSelectorTerms(tt.args.nodeSelectorTerms, tt.args.nodeLabels, tt.args.nodeFields) + if !apiequality.Semantic.DeepEqual(tt.args.nodeSelectorTerms, tt.want) { + // fail when tt.args.nodeSelectorTerms is deeply modified + t.Errorf("MatchNodeSelectorTerms() got = %v, want %v", tt.args.nodeSelectorTerms, tt.want) + } + }) + } +} + func TestMatchTopologySelectorTerms(t *testing.T) { type args struct { topologySelectorTerms []v1.TopologySelectorTerm diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go index fba6b158ed0..fa42493002d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go @@ -17,6 +17,7 @@ limitations under the License. package v1 import ( + "fmt" "reflect" "strings" "testing" @@ -70,15 +71,21 @@ func TestLabelSelectorAsSelector(t *testing.T) { } for i, tc := range tc { + inCopy := tc.in.DeepCopy() out, err := LabelSelectorAsSelector(tc.in) + // after calling LabelSelectorAsSelector, tc.in shouldn't be modified + if !reflect.DeepEqual(inCopy, tc.in) { + t.Errorf("[%v]expected:\n\t%#v\nbut got:\n\t%#v", i, inCopy, tc.in) + } if err == nil && tc.expectErr { t.Errorf("[%v]expected error but got none.", i) } if err != nil && !tc.expectErr { t.Errorf("[%v]did not expect error but got: %v", i, err) } - if !reflect.DeepEqual(out, tc.out) { - t.Errorf("[%v]expected:\n\t%+v\nbut got:\n\t%+v", i, tc.out, out) + // fmt.Sprint() over String() as nil.String() will panic + if fmt.Sprint(out) != fmt.Sprint(tc.out) { + t.Errorf("[%v]expected:\n\t%s\nbut got:\n\t%s", i, fmt.Sprint(tc.out), fmt.Sprint(out)) } } } diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/selector.go b/staging/src/k8s.io/apimachinery/pkg/labels/selector.go index b301b428403..374d2ef1377 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/selector.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/selector.go @@ -166,7 +166,6 @@ func NewRequirement(key string, op selection.Operator, vals []string) (*Requirem return nil, err } } - sort.Strings(vals) return &Requirement{key: key, operator: op, strValues: vals}, nil } @@ -299,7 +298,9 @@ func (r *Requirement) String() string { if len(r.strValues) == 1 { buffer.WriteString(r.strValues[0]) } else { // only > 1 since == 0 prohibited by NewRequirement - buffer.WriteString(strings.Join(r.strValues, ",")) + // normalizes value order on output, without mutating the in-memory selector representation + // also avoids normalization when it is not required, and ensures we do not mutate shared data + buffer.WriteString(strings.Join(safeSort(r.strValues), ",")) } switch r.operator { @@ -309,6 +310,17 @@ func (r *Requirement) String() string { return buffer.String() } +// safeSort sort input strings without modification +func safeSort(in []string) []string { + if sort.StringsAreSorted(in) { + return in + } + out := make([]string, len(in)) + copy(out, in) + sort.Strings(out) + return out +} + // Add adds requirements to the selector. It copies the current selector returning a new one func (lsel internalSelector) Add(reqs ...Requirement) Selector { var sel internalSelector diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go b/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go index 995317bd177..a2702989b5f 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go @@ -573,3 +573,47 @@ func TestAdd(t *testing.T) { } } } + +func TestSafeSort(t *testing.T) { + tests := []struct { + name string + in []string + inCopy []string + want []string + }{ + { + name: "nil strings", + in: nil, + inCopy: nil, + want: nil, + }, + { + name: "ordered strings", + in: []string{"bar", "foo"}, + inCopy: []string{"bar", "foo"}, + want: []string{"bar", "foo"}, + }, + { + name: "unordered strings", + in: []string{"foo", "bar"}, + inCopy: []string{"foo", "bar"}, + want: []string{"bar", "foo"}, + }, + { + name: "duplicated strings", + in: []string{"foo", "bar", "foo", "bar"}, + inCopy: []string{"foo", "bar", "foo", "bar"}, + want: []string{"bar", "bar", "foo", "foo"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := safeSort(tt.in); !reflect.DeepEqual(got, tt.want) { + t.Errorf("safeSort() = %v, want %v", got, tt.want) + } + if !reflect.DeepEqual(tt.in, tt.inCopy) { + t.Errorf("after safeSort(), input = %v, want %v", tt.in, tt.inCopy) + } + }) + } +}