Merge pull request #66480 from Huang-Wei/stateless-MatchNodeSelectorTerms

Automatic merge from submit-queue (batch tested with PRs 67042, 66480, 67053). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

ensure MatchNodeSelectorTerms() runs statelessly

**What this PR does**:

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)

**Why we need it**:
- Without this fix, scheduling and daemonset controller doesn't work well in some (corner) cases

**Which issue(s) this PR fixes**:
Fixes #66298

**Special notes for your reviewer**:
Parameter `nodeSelectorTerms` in method MatchNodeSelectorTerms() is a slice, which is fundamentally a {*elements, len, cap} tuple - i.e. it's passing in a pointer. In that method, NodeSelectorRequirementsAsSelector() -> NewRequirement() is invoked, and the `matchExpressions[*].values` is passed into and **modified** via `sort.Strings(vals)`.

This will cause following daemonset pod fall into an infinite create/delete loop:

```yaml
apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: problem
spec:
  selector:
    matchLabels:
      app: sleeper
  template:
    metadata:
      labels:
        app: sleeper
    spec:
      affinity:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
              - key: kubernetes.io/hostname
                operator: In
                values:
                - 127.0.0.2
                - 127.0.0.1
      containers:
      - name: busybox
        image: busybox
        command: ["/bin/sleep", "7200"]
```

(the problem can be stably reproduced on a local cluster started by `hack/local-up-cluster.sh`)

The first time daemonset yaml is handled by apiserver and persisted in etcd with original format (original order of values was kept - 127.0.0.2, 127.0.0.1). After that, daemonset controller tries to schedule pod, and it reuses the predicates logic in scheduler component - where the values are **sorted** deeply. This not only causes the pod to be created in sorted order (127.0.0.1, 127.0.0.2), but also introduced a bug when updating daemonset - internally ds controller use a "rawMessage" (bytes of an object) to calculate hash acting as a "controller-revision-hash" to control revision rollingUpdate/rollBack, so it keeps killing "old" pod and spawning "new" pod back and forth, and fall into an infinite loop.

The issue exists in `master`, `release-1.11` and `release-1.10`.

**Release note**:
```release-note
NONE
```

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

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) {
+		// 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

@@ -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

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)
+			}
+		})
+	}
+}