Merge pull request #18157 from sdminonne/LabelSelector_internalSelector

pkg/apis/extensions/helpers.go

@@ -18,7 +18,6 @@ package extensions
 
 import (
 	"fmt"
-	"sort"
 
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/labels"
@@ -34,13 +33,13 @@ func LabelSelectorAsSelector(ps *LabelSelector) (labels.Selector, error) {
 	if len(ps.MatchLabels)+len(ps.MatchExpressions) == 0 {
 		return labels.Everything(), nil
 	}
-	selector := labels.LabelSelector{}
+	selector := labels.NewSelector()
 	for k, v := range ps.MatchLabels {
-		req, err := labels.NewRequirement(k, labels.InOperator, sets.NewString(v))
+		r, err := labels.NewRequirement(k, labels.InOperator, sets.NewString(v))
 		if err != nil {
 			return nil, err
 		}
-		selector = append(selector, *req)
+		selector = selector.Add(*r)
 	}
 	for _, expr := range ps.MatchExpressions {
 		var op labels.Operator
@@ -56,13 +55,12 @@ func LabelSelectorAsSelector(ps *LabelSelector) (labels.Selector, error) {
 		default:
 			return nil, fmt.Errorf("%q is not a valid pod selector operator", expr.Operator)
 		}
-		req, err := labels.NewRequirement(expr.Key, op, sets.NewString(expr.Values...))
+		r, err := labels.NewRequirement(expr.Key, op, sets.NewString(expr.Values...))
 		if err != nil {
 			return nil, err
 		}
-		selector = append(selector, *req)
+		selector = selector.Add(*r)
 	}
-	sort.Sort(labels.ByKey(selector))
 	return selector, nil
 }
 

pkg/labels/selector.go

@@ -37,21 +37,21 @@ type Selector interface {
 	// String returns a human readable string that represents this selector.
 	String() string
 
-	// Add add a specific requirement for the selector
-	Add(key string, operator Operator, values []string) Selector
+	// Add adds requirements to the Selector
+	Add(r ...Requirement) Selector
 }
 
 // Everything returns a selector that matches all labels.
 func Everything() Selector {
-	return LabelSelector{}
+	return internalSelector{}
 }
 
 type nothingSelector struct{}
 
-func (n nothingSelector) Matches(_ Labels) bool                         { return false }
-func (n nothingSelector) Empty() bool                                   { return false }
-func (n nothingSelector) String() string                                { return "<null>" }
-func (n nothingSelector) Add(_ string, _ Operator, _ []string) Selector { return n }
+func (n nothingSelector) Matches(_ Labels) bool         { return false }
+func (n nothingSelector) Empty() bool                   { return false }
+func (n nothingSelector) String() string                { return "<null>" }
+func (n nothingSelector) Add(_ ...Requirement) Selector { return n }
 
 // Nothing returns a selector that matches no labels
 func Nothing() Selector {
@@ -72,10 +72,13 @@ const (
 	ExistsOperator       Operator = "exists"
 )
 
-//LabelSelector is a list of Requirements.
-type LabelSelector []Requirement
+func NewSelector() Selector {
+	return internalSelector(nil)
+}
+
+type internalSelector []Requirement
 
-// Sort by  obtain determisitic parser
+// Sort by key to obtain determisitic parser
 type ByKey []Requirement
 
 func (a ByKey) Len() int { return len(a) }
@@ -178,8 +181,8 @@ func (r *Requirement) Values() sets.String {
 	return ret
 }
 
-// Return true if the LabelSelector doesn't restrict selection space
-func (lsel LabelSelector) Empty() bool {
+// Return true if the internalSelector doesn't restrict selection space
+func (lsel internalSelector) Empty() bool {
 	if lsel == nil {
 		return true
 	}
@@ -228,22 +231,23 @@ func (r *Requirement) String() string {
 	return buffer.String()
 }
 
-// Add adds a requirement to the selector. It copies the current selector returning a new one
-func (lsel LabelSelector) Add(key string, operator Operator, values []string) Selector {
-	var reqs []Requirement
+// 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
 	for _, item := range lsel {
-		reqs = append(reqs, item)
+		sel = append(sel, item)
 	}
-	if r, err := NewRequirement(key, operator, sets.NewString(values...)); err == nil {
-		reqs = append(reqs, *r)
+	for _, r := range reqs {
+		sel = append(sel, r)
 	}
-	return LabelSelector(reqs)
+	sort.Sort(ByKey(sel))
+	return sel
 }
 
-// Matches for a LabelSelector returns true if all
+// Matches for a internalSelector returns true if all
 // its Requirements match the input Labels. If any
 // Requirement does not match, false is returned.
-func (lsel LabelSelector) Matches(l Labels) bool {
+func (lsel internalSelector) Matches(l Labels) bool {
 	for _, req := range lsel {
 		if matches := req.Matches(l); !matches {
 			return false
@@ -253,8 +257,8 @@ func (lsel LabelSelector) Matches(l Labels) bool {
 }
 
 // String returns a comma-separated string of all
-// the LabelSelector Requirements' human-readable strings.
-func (lsel LabelSelector) String() string {
+// the internalSelector Requirements' human-readable strings.
+func (lsel internalSelector) String() string {
 	var reqs []string
 	for _, req := range lsel {
 		reqs = append(reqs, req.String())
@@ -473,10 +477,10 @@ func (p *Parser) scan() {
 
 // parse runs the left recursive descending algorithm
 // on input string. It returns a list of Requirement objects.
-func (p *Parser) parse() ([]Requirement, error) {
+func (p *Parser) parse() (internalSelector, error) {
 	p.scan() // init scannedItems
 
-	var requirements []Requirement
+	var requirements internalSelector
 	for {
 		tok, lit := p.lookahead(Values)
 		switch tok {
@@ -692,7 +696,7 @@ func Parse(selector string) (Selector, error) {
 	items, error := p.parse()
 	if error == nil {
 		sort.Sort(ByKey(items)) // sort to grant determistic parsing
-		return LabelSelector(items), error
+		return internalSelector(items), error
 	}
 	return nil, error
 }
@@ -718,18 +722,18 @@ func validateLabelValue(v string) error {
 // nil and empty Sets are considered equivalent to Everything().
 func SelectorFromSet(ls Set) Selector {
 	if ls == nil {
-		return LabelSelector{}
+		return internalSelector{}
 	}
-	var requirements []Requirement
+	var requirements internalSelector
 	for label, value := range ls {
 		if r, err := NewRequirement(label, EqualsOperator, sets.NewString(value)); err != nil {
 			//TODO: double check errors when input comes from serialization?
-			return LabelSelector{}
+			return internalSelector{}
 		} else {
 			requirements = append(requirements, *r)
 		}
 	}
 	// sort to have deterministic string representation
 	sort.Sort(ByKey(requirements))
-	return LabelSelector(requirements)
+	return internalSelector(requirements)
 }

pkg/labels/selector_test.go

@@ -168,8 +168,8 @@ func TestSetIsEmpty(t *testing.T) {
 	if !(Set{}).AsSelector().Empty() {
 		t.Errorf("Empty set should be empty")
 	}
-	if !(LabelSelector(nil)).Empty() {
-		t.Errorf("Nil LabelSelector should be empty")
+	if !(NewSelector()).Empty() {
+		t.Errorf("Nil Selector should be empty")
 	}
 }
 
@@ -313,30 +313,31 @@ func TestRequirementConstructor(t *testing.T) {
 func TestToString(t *testing.T) {
 	var req Requirement
 	toStringTests := []struct {
-		In    *LabelSelector
+		In    *internalSelector
 		Out   string
 		Valid bool
 	}{
-		{&LabelSelector{
+
+		{&internalSelector{
 			getRequirement("x", InOperator, sets.NewString("abc", "def"), t),
 			getRequirement("y", NotInOperator, sets.NewString("jkl"), t),
 			getRequirement("z", ExistsOperator, nil, t)},
 			"x in (abc,def),y notin (jkl),z", true},
-		{&LabelSelector{
+		{&internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString("abc", "def"), t),
 			getRequirement("y", NotEqualsOperator, sets.NewString("jkl"), t),
 			getRequirement("z", DoesNotExistOperator, nil, t)},
 			"x notin (abc,def),y!=jkl,!z", true},
-		{&LabelSelector{
+		{&internalSelector{
 			getRequirement("x", InOperator, sets.NewString("abc", "def"), t),
 			req}, // adding empty req for the trailing ','
 			"x in (abc,def),", false},
-		{&LabelSelector{
+		{&internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString("abc"), t),
 			getRequirement("y", InOperator, sets.NewString("jkl", "mno"), t),
 			getRequirement("z", NotInOperator, sets.NewString(""), t)},
 			"x notin (abc),y in (jkl,mno),z notin ()", true},
-		{&LabelSelector{
+		{&internalSelector{
 			getRequirement("x", EqualsOperator, sets.NewString("abc"), t),
 			getRequirement("y", DoubleEqualsOperator, sets.NewString("jkl"), t),
 			getRequirement("z", NotEqualsOperator, sets.NewString("a"), t),
@@ -352,37 +353,37 @@ func TestToString(t *testing.T) {
 	}
 }
 
-func TestRequirementLabelSelectorMatching(t *testing.T) {
+func TestRequirementSelectorMatching(t *testing.T) {
 	var req Requirement
 	labelSelectorMatchingTests := []struct {
 		Set   Set
-		Sel   *LabelSelector
+		Sel   Selector
 		Match bool
 	}{
-		{Set{"x": "foo", "y": "baz"}, &LabelSelector{
+		{Set{"x": "foo", "y": "baz"}, &internalSelector{
 			req,
 		}, false},
-		{Set{"x": "foo", "y": "baz"}, &LabelSelector{
+		{Set{"x": "foo", "y": "baz"}, &internalSelector{
 			getRequirement("x", InOperator, sets.NewString("foo"), t),
 			getRequirement("y", NotInOperator, sets.NewString("alpha"), t),
 		}, true},
-		{Set{"x": "foo", "y": "baz"}, &LabelSelector{
+		{Set{"x": "foo", "y": "baz"}, &internalSelector{
 			getRequirement("x", InOperator, sets.NewString("foo"), t),
 			getRequirement("y", InOperator, sets.NewString("alpha"), t),
 		}, false},
-		{Set{"y": ""}, &LabelSelector{
+		{Set{"y": ""}, &internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString(""), t),
 			getRequirement("y", ExistsOperator, nil, t),
 		}, true},
-		{Set{"y": ""}, &LabelSelector{
+		{Set{"y": ""}, &internalSelector{
 			getRequirement("x", DoesNotExistOperator, nil, t),
 			getRequirement("y", ExistsOperator, nil, t),
 		}, true},
-		{Set{"y": ""}, &LabelSelector{
+		{Set{"y": ""}, &internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString(""), t),
 			getRequirement("y", DoesNotExistOperator, nil, t),
 		}, false},
-		{Set{"y": "baz"}, &LabelSelector{
+		{Set{"y": "baz"}, &internalSelector{
 			getRequirement("x", InOperator, sets.NewString(""), t),
 		}, false},
 	}
@@ -400,75 +401,75 @@ func TestSetSelectorParser(t *testing.T) {
 		Match bool
 		Valid bool
 	}{
-		{"", LabelSelector(nil), true, true},
-		{"\rx", LabelSelector{
+		{"", NewSelector(), true, true},
+		{"\rx", internalSelector{
 			getRequirement("x", ExistsOperator, nil, t),
 		}, true, true},
-		{"this-is-a-dns.domain.com/key-with-dash", LabelSelector{
+		{"this-is-a-dns.domain.com/key-with-dash", internalSelector{
 			getRequirement("this-is-a-dns.domain.com/key-with-dash", ExistsOperator, nil, t),
 		}, true, true},
-		{"this-is-another-dns.domain.com/key-with-dash in (so,what)", LabelSelector{
+		{"this-is-another-dns.domain.com/key-with-dash in (so,what)", internalSelector{
 			getRequirement("this-is-another-dns.domain.com/key-with-dash", InOperator, sets.NewString("so", "what"), t),
 		}, true, true},
-		{"0.1.2.domain/99 notin (10.10.100.1, tick.tack.clock)", LabelSelector{
+		{"0.1.2.domain/99 notin (10.10.100.1, tick.tack.clock)", internalSelector{
 			getRequirement("0.1.2.domain/99", NotInOperator, sets.NewString("10.10.100.1", "tick.tack.clock"), t),
 		}, true, true},
-		{"foo  in	 (abc)", LabelSelector{
+		{"foo  in	 (abc)", internalSelector{
 			getRequirement("foo", InOperator, sets.NewString("abc"), t),
 		}, true, true},
-		{"x notin\n (abc)", LabelSelector{
+		{"x notin\n (abc)", internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString("abc"), t),
 		}, true, true},
-		{"x  notin	\t	(abc,def)", LabelSelector{
+		{"x  notin	\t	(abc,def)", internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString("abc", "def"), t),
 		}, true, true},
-		{"x in (abc,def)", LabelSelector{
+		{"x in (abc,def)", internalSelector{
 			getRequirement("x", InOperator, sets.NewString("abc", "def"), t),
 		}, true, true},
-		{"x in (abc,)", LabelSelector{
+		{"x in (abc,)", internalSelector{
 			getRequirement("x", InOperator, sets.NewString("abc", ""), t),
 		}, true, true},
-		{"x in ()", LabelSelector{
+		{"x in ()", internalSelector{
 			getRequirement("x", InOperator, sets.NewString(""), t),
 		}, true, true},
-		{"x notin (abc,,def),bar,z in (),w", LabelSelector{
+		{"x notin (abc,,def),bar,z in (),w", internalSelector{
 			getRequirement("bar", ExistsOperator, nil, t),
 			getRequirement("w", ExistsOperator, nil, t),
 			getRequirement("x", NotInOperator, sets.NewString("abc", "", "def"), t),
 			getRequirement("z", InOperator, sets.NewString(""), t),
 		}, true, true},
-		{"x,y in (a)", LabelSelector{
+		{"x,y in (a)", internalSelector{
 			getRequirement("y", InOperator, sets.NewString("a"), t),
 			getRequirement("x", ExistsOperator, nil, t),
 		}, false, true},
-		{"x=a", LabelSelector{
+		{"x=a", internalSelector{
 			getRequirement("x", EqualsOperator, sets.NewString("a"), t),
 		}, true, true},
-		{"x=a,y!=b", LabelSelector{
+		{"x=a,y!=b", internalSelector{
 			getRequirement("x", EqualsOperator, sets.NewString("a"), t),
 			getRequirement("y", NotEqualsOperator, sets.NewString("b"), t),
 		}, true, true},
-		{"x=a,y!=b,z in (h,i,j)", LabelSelector{
+		{"x=a,y!=b,z in (h,i,j)", internalSelector{
 			getRequirement("x", EqualsOperator, sets.NewString("a"), t),
 			getRequirement("y", NotEqualsOperator, sets.NewString("b"), t),
 			getRequirement("z", InOperator, sets.NewString("h", "i", "j"), t),
 		}, true, true},
-		{"x=a||y=b", LabelSelector{}, false, false},
+		{"x=a||y=b", internalSelector{}, false, false},
 		{"x,,y", nil, true, false},
 		{",x,y", nil, true, false},
 		{"x nott in (y)", nil, true, false},
-		{"x notin ( )", LabelSelector{
+		{"x notin ( )", internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString(""), t),
 		}, true, true},
-		{"x notin (, a)", LabelSelector{
+		{"x notin (, a)", internalSelector{
 			getRequirement("x", NotInOperator, sets.NewString("", "a"), t),
 		}, true, true},
 		{"a in (xyz),", nil, true, false},
 		{"a in (xyz)b notin ()", nil, true, false},
-		{"a ", LabelSelector{
+		{"a ", internalSelector{
 			getRequirement("a", ExistsOperator, nil, t),
 		}, true, true},
-		{"a in (x,y,notin, z,in)", LabelSelector{
+		{"a in (x,y,notin, z,in)", internalSelector{
 			getRequirement("a", InOperator, sets.NewString("in", "notin", "x", "y", "z"), t),
 		}, true, true}, // operator 'in' inside list of identifiers
 		{"a in (xyz abc)", nil, false, false}, // no comma
@@ -483,7 +484,7 @@ func TestSetSelectorParser(t *testing.T) {
 		} else if err == nil && !ssp.Valid {
 			t.Errorf("Parse(%s) => %+v expected error", ssp.In, sel)
 		} else if ssp.Match && !reflect.DeepEqual(sel, ssp.Out) {
-			t.Errorf("Parse(%s) => parse output '%v' doesn't match '%v' expected match", ssp.In, sel, ssp.Out)
+			t.Errorf("Parse(%s) => parse output '%#v' doesn't match '%#v' expected match", ssp.In, sel, ssp.Out)
 		}
 	}
 }
@@ -499,6 +500,7 @@ func getRequirement(key string, op Operator, vals sets.String, t *testing.T) Req
 
 func TestAdd(t *testing.T) {
 	testCases := []struct {
+		name        string
 		sel         Selector
 		key         string
 		operator    Operator
@@ -506,27 +508,33 @@ func TestAdd(t *testing.T) {
 		refSelector Selector
 	}{
 		{
-			LabelSelector{},
+			"keyInOperator",
+			internalSelector{},
 			"key",
 			InOperator,
 			[]string{"value"},
-			LabelSelector{Requirement{"key", InOperator, sets.NewString("value")}},
+			internalSelector{Requirement{"key", InOperator, sets.NewString("value")}},
 		},
 		{
-			LabelSelector{Requirement{"key", InOperator, sets.NewString("value")}},
+			"keyEqualsOperator",
+			internalSelector{Requirement{"key", InOperator, sets.NewString("value")}},
 			"key2",
 			EqualsOperator,
 			[]string{"value2"},
-			LabelSelector{
+			internalSelector{
 				Requirement{"key", InOperator, sets.NewString("value")},
 				Requirement{"key2", EqualsOperator, sets.NewString("value2")},
 			},
 		},
 	}
 	for _, ts := range testCases {
-		ts.sel = ts.sel.Add(ts.key, ts.operator, ts.values)
+		req, err := NewRequirement(ts.key, ts.operator, sets.NewString(ts.values...))
+		if err != nil {
+			t.Errorf("%s - Unable to create labels.Requirement", ts.name)
+		}
+		ts.sel = ts.sel.Add(*req)
 		if !reflect.DeepEqual(ts.sel, ts.refSelector) {
-			t.Errorf("Expected %v found %v", ts.refSelector, ts.sel)
+			t.Errorf("%s - Expected %v found %v", ts.name, ts.refSelector, ts.sel)
 		}
 	}
 }