From f18d4f54b7264902abdca8093c56aecc0b1a3dbc Mon Sep 17 00:00:00 2001
From: Wei Huang <wei.huang1@ibm.com>
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/meta/v1/helpers_test.go          | 11 ++++-
 .../apimachinery/pkg/labels/selector.go       | 16 ++++++-
 .../apimachinery/pkg/labels/selector_test.go  | 44 +++++++++++++++++++
 3 files changed, 67 insertions(+), 4 deletions(-)

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 fba6b158ed032..fa42493002db4 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 b301b42840371..374d2ef1377a8 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 995317bd17774..a2702989b5f1f 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)
+			}
+		})
+	}
+}