Skip to content

K
k3s
Commit e5e0de1b authored by Wei Huang's avatar Wei Huang
Browse files
Options

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
parent 2755000b
Hide whitespace changes
Inline Side-by-side
Showing with 364 additions and 4 deletions
pkg/apis/core/v1/helper/helpers_test.go
View file @ e5e0de1b
...@@ -811,6 +811,303 @@ func TestMatchNodeSelectorTerms(t *testing.T) { ...@@ -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) { func TestMatchTopologySelectorTerms(t *testing.T) {
type args struct { type args struct {
topologySelectorTerms []v1.TopologySelectorTerm topologySelectorTerms []v1.TopologySelectorTerm
......
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go
View file @ e5e0de1b
...@@ -17,6 +17,7 @@ limitations under the License. ...@@ -17,6 +17,7 @@ limitations under the License.
package v1 package v1
import ( import (
"fmt"
"reflect" "reflect"
"strings" "strings"
"testing" "testing"
...@@ -70,15 +71,21 @@ func TestLabelSelectorAsSelector(t *testing.T) { ...@@ -70,15 +71,21 @@ func TestLabelSelectorAsSelector(t *testing.T) {
} }
for i, tc := range tc { for i, tc := range tc {
inCopy := tc.in.DeepCopy()
out, err := LabelSelectorAsSelector(tc.in) 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 { if err == nil && tc.expectErr {
t.Errorf("[%v]expected error but got none.", i) t.Errorf("[%v]expected error but got none.", i)
} }
if err != nil && !tc.expectErr { if err != nil && !tc.expectErr {
t.Errorf("[%v]did not expect error but got: %v", i, err) t.Errorf("[%v]did not expect error but got: %v", i, err)
} }
if !reflect.DeepEqual(out, tc.out) { // fmt.Sprint() over String() as nil.String() will panic
t.Errorf("[%v]expected:\n\t%+v\nbut got:\n\t%+v", i, tc.out, out) 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))
} }
} }
} }
......
staging/src/k8s.io/apimachinery/pkg/labels/selector.go
View file @ e5e0de1b
...@@ -166,7 +166,6 @@ func NewRequirement(key string, op selection.Operator, vals []string) (*Requirem ...@@ -166,7 +166,6 @@ func NewRequirement(key string, op selection.Operator, vals []string) (*Requirem
return nil, err return nil, err
} }
} }
sort.Strings(vals)
return &Requirement{key: key, operator: op, strValues: vals}, nil return &Requirement{key: key, operator: op, strValues: vals}, nil
} }
...@@ -299,7 +298,9 @@ func (r *Requirement) String() string { ...@@ -299,7 +298,9 @@ func (r *Requirement) String() string {
if len(r.strValues) == 1 { if len(r.strValues) == 1 {
buffer.WriteString(r.strValues[0]) buffer.WriteString(r.strValues[0])
} else { // only > 1 since == 0 prohibited by NewRequirement } 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 { switch r.operator {
...@@ -309,6 +310,17 @@ func (r *Requirement) String() string { ...@@ -309,6 +310,17 @@ func (r *Requirement) String() string {
return buffer.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 // Add adds requirements to the selector. It copies the current selector returning a new one
func (lsel internalSelector) Add(reqs ...Requirement) Selector { func (lsel internalSelector) Add(reqs ...Requirement) Selector {
var sel internalSelector var sel internalSelector
......
staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go
View file @ e5e0de1b
...@@ -573,3 +573,47 @@ func TestAdd(t *testing.T) { ...@@ -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)
}
})
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment