github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-08-03 17:30:00 +00:00
Code Issues Projects Releases Wiki Activity

Merge pull request #28474 from hongchaodeng/lb

Browse Source 
Automatic merge from submit-queue

selector: make sure value of GT and LT is integer

GT and LT in selector has been introduced in Node Affinity feature: https://github.com/kubernetes/kubernetes/pull/19758, https://github.com/kubernetes/kubernetes/pull/18261

According to the API:

>  If the operator is Gt or Lt, the values array must have a single element, which will be interpreted as an integer.

But the implementation has parsed it as float64:
ef0c9f0c5b/pkg/labels/selector.go (L183)

Modeling integer as float is dangerous. We don't even have fixed precision guarantee when doing comparison.

This PR is to get rid of this pre-optimization and convert **integer** to int64.
This commit is contained in:
k8s-merge-robot 2016-07-06 06:22:11 -07:00 committed by GitHub
commit 1ff33c576d
4 changed files with 41 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
pkg/api/helpers_test.go
View File

@ -212,17 +212,17 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) {
in: []NodeSelectorRequirement{{ in: []NodeSelectorRequirement{{
Key: "foo", Key: "foo",
Operator: NodeSelectorOpGt, Operator: NodeSelectorOpGt,
Values: []string{"1.1"}, Values: []string{"1"},
}}, }},
out: mustParse("foo>1.1"), out: mustParse("foo>1"),
}, },
{ {
in: []NodeSelectorRequirement{{ in: []NodeSelectorRequirement{{
Key: "bar", Key: "bar",
Operator: NodeSelectorOpLt, Operator: NodeSelectorOpLt,
Values: []string{"7.1"}, Values: []string{"7"},
}}, }},
out: mustParse("bar<7.1"), out: mustParse("bar<7"),
}, },
} }

20
pkg/labels/selector.go
View File

@ -108,7 +108,7 @@ type Requirement struct {
// (2) If the operator is In or NotIn, the values set must be non-empty. // (2) If the operator is In or NotIn, the values set must be non-empty.
// (3) If the operator is Equals, DoubleEquals, or NotEquals, the values set must contain one value. // (3) If the operator is Equals, DoubleEquals, or NotEquals, the values set must contain one value.
// (4) If the operator is Exists or DoesNotExist, the value set must be empty. // (4) If the operator is Exists or DoesNotExist, the value set must be empty.
// (5) If the operator is Gt or Lt, the values set must contain only one value. // (5) If the operator is Gt or Lt, the values set must contain only one value, which will be interpreted as an integer.
// (6) The key is invalid due to its length, or sequence // (6) The key is invalid due to its length, or sequence
// of characters. See validateLabelKey for more details. // of characters. See validateLabelKey for more details.
// //
@ -135,8 +135,8 @@ func NewRequirement(key string, op Operator, vals sets.String) (*Requirement, er
return nil, fmt.Errorf("for 'Gt', 'Lt' operators, exactly one value is required") return nil, fmt.Errorf("for 'Gt', 'Lt' operators, exactly one value is required")
} }
for val := range vals { for val := range vals {
if _, err := strconv.ParseFloat(val, 64); err != nil { if _, err := strconv.ParseInt(val, 10, 64); err != nil {
return nil, fmt.Errorf("for 'Gt', 'Lt' operators, the value must be a number") return nil, fmt.Errorf("for 'Gt', 'Lt' operators, the value must be an integer")
} }
} }
default: default:
@ -160,6 +160,8 @@ func NewRequirement(key string, op Operator, vals sets.String) (*Requirement, er
// Labels' value for that key is not in Requirement's value set. // Labels' value for that key is not in Requirement's value set.
// (4) The operator is DoesNotExist or NotIn and Labels does not have the // (4) The operator is DoesNotExist or NotIn and Labels does not have the
// Requirement's key. // Requirement's key.
// (5) The operator is GreaterThanOperator or LessThanOperator, and Labels has
// the Requirement's key and the corresponding value satisfies mathematical inequality.
func (r *Requirement) Matches(ls Labels) bool { func (r *Requirement) Matches(ls Labels) bool {
switch r.operator { switch r.operator {
case InOperator, EqualsOperator, DoubleEqualsOperator: case InOperator, EqualsOperator, DoubleEqualsOperator:
@ -180,23 +182,23 @@ func (r *Requirement) Matches(ls Labels) bool {
if !ls.Has(r.key) { if !ls.Has(r.key) {
return false return false
} }
lsValue, err := strconv.ParseFloat(ls.Get(r.key), 64) lsValue, err := strconv.ParseInt(ls.Get(r.key), 10, 64)
if err != nil { if err != nil {
glog.V(10).Infof("Parse float failed for value %+v in label %+v, %+v", ls.Get(r.key), ls, err) glog.V(10).Infof("ParseInt failed for value %+v in label %+v, %+v", ls.Get(r.key), ls, err)
return false return false
} }
// There should be only one strValue in r.strValues, and can be converted to a float number. // There should be only one strValue in r.strValues, and can be converted to a integer.
if len(r.strValues) != 1 { if len(r.strValues) != 1 {
glog.V(10).Infof("Invalid values count %+v of requirement %+v, for 'Gt', 'Lt' operators, exactly one value is required", len(r.strValues), r) glog.V(10).Infof("Invalid values count %+v of requirement %+v, for 'Gt', 'Lt' operators, exactly one value is required", len(r.strValues), r)
return false return false
} }
var rValue float64 var rValue int64
for strValue := range r.strValues { for strValue := range r.strValues {
rValue, err = strconv.ParseFloat(strValue, 64) rValue, err = strconv.ParseInt(strValue, 10, 64)
if err != nil { if err != nil {
glog.V(10).Infof("Parse float failed for value %+v in requirement %+v, for 'Gt', 'Lt' operators, the value must be a number", strValue, r) glog.V(10).Infof("ParseInt failed for value %+v in requirement %+v, for 'Gt', 'Lt' operators, the value must be an integer", strValue, r)
return false return false
} }
} }

46
pkg/labels/selector_test.go
View File

@ -34,8 +34,8 @@ func TestSelectorParse(t *testing.T) {
"x=,z= ", "x=,z= ",
"x= ,z= ", "x= ,z= ",
"!x", "!x",
"x>1.1", "x>1",
"x>1.1,z<5.3", "x>1,z<5",
} }
testBadStrings := []string{ testBadStrings := []string{
"x=a||y=b", "x=a||y=b",
@ -110,16 +110,16 @@ func TestSelectorMatches(t *testing.T) {
expectMatch(t, "notin=in", Set{"notin": "in"}) // in and notin in exactMatch expectMatch(t, "notin=in", Set{"notin": "in"}) // in and notin in exactMatch
expectMatch(t, "x", Set{"x": "z"}) expectMatch(t, "x", Set{"x": "z"})
expectMatch(t, "!x", Set{"y": "z"}) expectMatch(t, "!x", Set{"y": "z"})
expectMatch(t, "x>1.1", Set{"x": "1.2"}) expectMatch(t, "x>1", Set{"x": "2"})
expectMatch(t, "x<1.1", Set{"x": "0.8"}) expectMatch(t, "x<1", Set{"x": "0"})
expectNoMatch(t, "x=z", Set{}) expectNoMatch(t, "x=z", Set{})
expectNoMatch(t, "x=y", Set{"x": "z"}) expectNoMatch(t, "x=y", Set{"x": "z"})
expectNoMatch(t, "x=y,z=w", Set{"x": "w", "z": "w"}) expectNoMatch(t, "x=y,z=w", Set{"x": "w", "z": "w"})
expectNoMatch(t, "x!=y,z!=w", Set{"x": "z", "z": "w"}) expectNoMatch(t, "x!=y,z!=w", Set{"x": "z", "z": "w"})
expectNoMatch(t, "x", Set{"y": "z"}) expectNoMatch(t, "x", Set{"y": "z"})
expectNoMatch(t, "!x", Set{"x": "z"}) expectNoMatch(t, "!x", Set{"x": "z"})
expectNoMatch(t, "x>1.1", Set{"x": "0.8"}) expectNoMatch(t, "x>1", Set{"x": "0"})
expectNoMatch(t, "x<1.1", Set{"x": "1.1"}) expectNoMatch(t, "x<1", Set{"x": "2"})
labelset := Set{ labelset := Set{
"foo": "bar", "foo": "bar",
@ -235,8 +235,8 @@ func TestLexerSequence(t *testing.T) {
{"()", []Token{OpenParToken, ClosedParToken}}, {"()", []Token{OpenParToken, ClosedParToken}},
{"x in (),y", []Token{IdentifierToken, InToken, OpenParToken, ClosedParToken, CommaToken, IdentifierToken}}, {"x in (),y", []Token{IdentifierToken, InToken, OpenParToken, ClosedParToken, CommaToken, IdentifierToken}},
{"== != (), = notin", []Token{DoubleEqualsToken, NotEqualsToken, OpenParToken, ClosedParToken, CommaToken, EqualsToken, NotInToken}}, {"== != (), = notin", []Token{DoubleEqualsToken, NotEqualsToken, OpenParToken, ClosedParToken, CommaToken, EqualsToken, NotInToken}},
{"key>1.1", []Token{IdentifierToken, GreaterThanToken, IdentifierToken}}, {"key>2", []Token{IdentifierToken, GreaterThanToken, IdentifierToken}},
{"key<0.8", []Token{IdentifierToken, LessThanToken, IdentifierToken}}, {"key<1", []Token{IdentifierToken, LessThanToken, IdentifierToken}},
} }
for _, v := range testcases { for _, v := range testcases {
var literals []string var literals []string
@ -274,8 +274,8 @@ func TestParserLookahead(t *testing.T) {
{"", []Token{EndOfStringToken}}, {"", []Token{EndOfStringToken}},
{"x in (),y", []Token{IdentifierToken, InToken, OpenParToken, ClosedParToken, CommaToken, IdentifierToken, EndOfStringToken}}, {"x in (),y", []Token{IdentifierToken, InToken, OpenParToken, ClosedParToken, CommaToken, IdentifierToken, EndOfStringToken}},
{"== != (), = notin", []Token{DoubleEqualsToken, NotEqualsToken, OpenParToken, ClosedParToken, CommaToken, EqualsToken, NotInToken, EndOfStringToken}}, {"== != (), = notin", []Token{DoubleEqualsToken, NotEqualsToken, OpenParToken, ClosedParToken, CommaToken, EqualsToken, NotInToken, EndOfStringToken}},
{"key>1.1", []Token{IdentifierToken, GreaterThanToken, IdentifierToken, EndOfStringToken}}, {"key>2", []Token{IdentifierToken, GreaterThanToken, IdentifierToken, EndOfStringToken}},
{"key<0.8", []Token{IdentifierToken, LessThanToken, IdentifierToken, EndOfStringToken}}, {"key<1", []Token{IdentifierToken, LessThanToken, IdentifierToken, EndOfStringToken}},
} }
for _, v := range testcases { for _, v := range testcases {
p := &Parser{l: &Lexer{s: v.s, pos: 0}, position: 0} p := &Parser{l: &Lexer{s: v.s, pos: 0}, position: 0}
@ -312,8 +312,8 @@ func TestRequirementConstructor(t *testing.T) {
{"x", DoesNotExistOperator, nil, true}, {"x", DoesNotExistOperator, nil, true},
{"1foo", InOperator, sets.NewString("bar"), true}, {"1foo", InOperator, sets.NewString("bar"), true},
{"1234", InOperator, sets.NewString("bar"), true}, {"1234", InOperator, sets.NewString("bar"), true},
{"y", GreaterThanOperator, sets.NewString("1.1"), true}, {"y", GreaterThanOperator, sets.NewString("1"), true},
{"z", LessThanOperator, sets.NewString("5.3"), true}, {"z", LessThanOperator, sets.NewString("6"), true},
{"foo", GreaterThanOperator, sets.NewString("bar"), false}, {"foo", GreaterThanOperator, sets.NewString("bar"), false},
{"barz", LessThanOperator, sets.NewString("blah"), false}, {"barz", LessThanOperator, sets.NewString("blah"), false},
{strings.Repeat("a", 254), ExistsOperator, nil, false}, //breaks DNS rule that len(key) <= 253 {strings.Repeat("a", 254), ExistsOperator, nil, false}, //breaks DNS rule that len(key) <= 253
@ -361,10 +361,10 @@ func TestToString(t *testing.T) {
getRequirement("z", ExistsOperator, nil, t)}, getRequirement("z", ExistsOperator, nil, t)},
"x=abc,y==jkl,z!=a,z", true}, "x=abc,y==jkl,z!=a,z", true},
{&internalSelector{ {&internalSelector{
getRequirement("x", GreaterThanOperator, sets.NewString("2.4"), t), getRequirement("x", GreaterThanOperator, sets.NewString("2"), t),
getRequirement("y", LessThanOperator, sets.NewString("7.1"), t), getRequirement("y", LessThanOperator, sets.NewString("8"), t),
getRequirement("z", ExistsOperator, nil, t)}, getRequirement("z", ExistsOperator, nil, t)},
"x>2.4,y<7.1,z", true}, "x>2,y<8,z", true},
} }
for _, ts := range toStringTests { for _, ts := range toStringTests {
if out := ts.In.String(); out == "" && ts.Valid { if out := ts.In.String(); out == "" && ts.Valid {
@ -408,11 +408,11 @@ func TestRequirementSelectorMatching(t *testing.T) {
{Set{"y": "baz"}, &internalSelector{ {Set{"y": "baz"}, &internalSelector{
getRequirement("x", InOperator, sets.NewString(""), t), getRequirement("x", InOperator, sets.NewString(""), t),
}, false}, }, false},
{Set{"z": "1.2"}, &internalSelector{ {Set{"z": "2"}, &internalSelector{
getRequirement("z", GreaterThanOperator, sets.NewString("1.0"), t), getRequirement("z", GreaterThanOperator, sets.NewString("1"), t),
}, true}, }, true},
{Set{"z": "v1.2"}, &internalSelector{ {Set{"z": "v2"}, &internalSelector{
getRequirement("z", GreaterThanOperator, sets.NewString("1.0"), t), getRequirement("z", GreaterThanOperator, sets.NewString("1"), t),
}, false}, }, false},
} }
for _, lsm := range labelSelectorMatchingTests { for _, lsm := range labelSelectorMatchingTests {
@ -473,11 +473,11 @@ func TestSetSelectorParser(t *testing.T) {
{"x=a", internalSelector{ {"x=a", internalSelector{
getRequirement("x", EqualsOperator, sets.NewString("a"), t), getRequirement("x", EqualsOperator, sets.NewString("a"), t),
}, true, true}, }, true, true},
{"x>1.1", internalSelector{ {"x>1", internalSelector{
getRequirement("x", GreaterThanOperator, sets.NewString("1.1"), t), getRequirement("x", GreaterThanOperator, sets.NewString("1"), t),
}, true, true}, }, true, true},
{"x<7.1", internalSelector{ {"x<7", internalSelector{
getRequirement("x", LessThanOperator, sets.NewString("7.1"), t), getRequirement("x", LessThanOperator, sets.NewString("7"), t),
}, true, true}, }, true, true},
{"x=a,y!=b", internalSelector{ {"x=a,y!=b", internalSelector{
getRequirement("x", EqualsOperator, sets.NewString("a"), t), getRequirement("x", EqualsOperator, sets.NewString("a"), t),

5
plugin/pkg/scheduler/algorithm/predicates/predicates_test.go
View File

@ -719,7 +719,7 @@ func TestPodFitsSelector(t *testing.T) {
"matchExpressions": [{ "matchExpressions": [{
"key": "kernel-version", "key": "kernel-version",
"operator": "Gt", "operator": "Gt",
"values": ["2.4"] "values": ["0204"]
}] }]
}] }]
}}}`, }}}`,
@ -727,7 +727,8 @@ func TestPodFitsSelector(t *testing.T) {
}, },
}, },
labels: map[string]string{ labels: map[string]string{
"kernel-version": "2.6", // We use two digit to denote major version and two digit for minior version.
"kernel-version": "0206",
}, },
fits: true, fits: true,
test: "Pod with matchExpressions using Gt operator that matches the existing node", test: "Pod with matchExpressions using Gt operator that matches the existing node",