diff --git a/hack/.golint_failures b/hack/.golint_failures index 1b17896a95c..ddac7e574d1 100644 --- a/hack/.golint_failures +++ b/hack/.golint_failures @@ -244,7 +244,6 @@ pkg/util/rlimit pkg/util/selinux pkg/util/sysctl/testing pkg/util/taints -pkg/util/tolerations pkg/version/verflag pkg/volume pkg/volume/azure_dd diff --git a/pkg/util/tolerations/BUILD b/pkg/util/tolerations/BUILD index fb96f4a85f8..0224bc0ba63 100644 --- a/pkg/util/tolerations/BUILD +++ b/pkg/util/tolerations/BUILD @@ -13,7 +13,11 @@ go_library( "tolerations.go", ], importpath = "k8s.io/kubernetes/pkg/util/tolerations", - deps = ["//pkg/apis/core:go_default_library"], + deps = [ + "//pkg/apis/core:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", + "//vendor/k8s.io/klog:go_default_library", + ], ) filegroup( @@ -33,5 +37,12 @@ go_test( name = "go_default_test", srcs = ["tolerations_test.go"], embed = [":go_default_library"], - deps = ["//pkg/apis/core:go_default_library"], + deps = [ + "//pkg/apis/core:go_default_library", + "//pkg/apis/core/validation:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", + "//vendor/github.com/stretchr/testify/require:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", + ], ) diff --git a/pkg/util/tolerations/tolerations.go b/pkg/util/tolerations/tolerations.go index 62b4ac6b58f..ad28b5e1e70 100644 --- a/pkg/util/tolerations/tolerations.go +++ b/pkg/util/tolerations/tolerations.go @@ -17,114 +17,91 @@ limitations under the License. package tolerations import ( + apiequality "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/klog" api "k8s.io/kubernetes/pkg/apis/core" ) -type key struct { - tolerationKey string - effect api.TaintEffect -} - -func convertTolerationToKey(in api.Toleration) key { - return key{in.Key, in.Effect} -} - // VerifyAgainstWhitelist checks if the provided tolerations // satisfy the provided whitelist and returns true, otherwise returns false -func VerifyAgainstWhitelist(tolerations []api.Toleration, whitelist []api.Toleration) bool { - if len(whitelist) == 0 { +func VerifyAgainstWhitelist(tolerations, whitelist []api.Toleration) bool { + if len(whitelist) == 0 || len(tolerations) == 0 { return true } - t := ConvertTolerationToAMap(tolerations) - w := ConvertTolerationToAMap(whitelist) - - for k1, v1 := range t { - if v2, ok := w[k1]; !ok || !AreEqual(v1, v2) { - return false +next: + for _, t := range tolerations { + for _, w := range whitelist { + if isSuperset(w, t) { + continue next + } } - } - return true -} - -// IsConflict returns true if the key of two tolerations match -// but one or more other fields differ, otherwise returns false -func IsConflict(first []api.Toleration, second []api.Toleration) bool { - firstMap := ConvertTolerationToAMap(first) - secondMap := ConvertTolerationToAMap(second) - - for k1, v1 := range firstMap { - if v2, ok := secondMap[k1]; ok && !AreEqual(v1, v2) { - return true - } - } - return false -} - -// MergeTolerations merges two sets of tolerations into one -// it does not check for conflicts -func MergeTolerations(first []api.Toleration, second []api.Toleration) []api.Toleration { - var mergedTolerations []api.Toleration - mergedTolerations = append(mergedTolerations, second...) - firstMap := ConvertTolerationToAMap(first) - secondMap := ConvertTolerationToAMap(second) - // preserve order of first when merging - for _, v := range first { - k := convertTolerationToKey(v) - // if first contains key conflicts, the last one takes precedence - if _, ok := secondMap[k]; !ok && firstMap[k] == v { - mergedTolerations = append(mergedTolerations, v) - } - } - return mergedTolerations -} - -// EqualTolerations returns true if two sets of tolerations are equal, otherwise false -// it assumes no duplicates in individual set of tolerations -func EqualTolerations(first []api.Toleration, second []api.Toleration) bool { - if len(first) != len(second) { return false } - firstMap := ConvertTolerationToAMap(first) - secondMap := ConvertTolerationToAMap(second) - - for k1, v1 := range firstMap { - if v2, ok := secondMap[k1]; !ok || !AreEqual(v1, v2) { - return false - } - } return true } -// ConvertTolerationToAMap converts toleration list into a map[string]api.Toleration -func ConvertTolerationToAMap(in []api.Toleration) map[key]api.Toleration { - out := map[key]api.Toleration{} - for _, v := range in { - out[convertTolerationToKey(v)] = v +// MergeTolerations merges two sets of tolerations into one. If one toleration is a superset of +// another, only the superset is kept. +func MergeTolerations(first, second []api.Toleration) []api.Toleration { + all := append(first, second...) + merged := []api.Toleration{} + +next: + for i, t := range all { + for _, t2 := range merged { + if isSuperset(t2, t) { + continue next // t is redundant; ignore it + } + } + if i+1 < len(all) { + for _, t2 := range all[i+1:] { + // If the tolerations are equal, prefer the first. + if isSuperset(t2, t) && !apiequality.Semantic.DeepEqual(&t, &t2) { + continue next // t is redundant; ignore it + } + } + } + merged = append(merged, t) } - return out + + return merged } -// AreEqual checks if two provided tolerations are equal or not. -func AreEqual(first, second api.Toleration) bool { - if first.Key == second.Key && - first.Operator == second.Operator && - first.Value == second.Value && - first.Effect == second.Effect && - AreTolerationSecondsEqual(first.TolerationSeconds, second.TolerationSeconds) { +// isSuperset checks whether ss tolerates a superset of t. +func isSuperset(ss, t api.Toleration) bool { + if apiequality.Semantic.DeepEqual(&t, &ss) { return true } - return false -} -// AreTolerationSecondsEqual checks if two provided TolerationSeconds are equal or not. -func AreTolerationSecondsEqual(ts1, ts2 *int64) bool { - if ts1 == ts2 { - return true + if t.Key != ss.Key && + // An empty key with Exists operator means match all keys & values. + (ss.Key != "" || ss.Operator != api.TolerationOpExists) { + return false } - if ts1 != nil && ts2 != nil && *ts1 == *ts2 { - return true + + // An empty effect means match all effects. + if t.Effect != ss.Effect && ss.Effect != "" { + return false + } + + if ss.Effect == api.TaintEffectNoExecute { + if ss.TolerationSeconds != nil { + if t.TolerationSeconds == nil || + *t.TolerationSeconds > *ss.TolerationSeconds { + return false + } + } + } + + switch ss.Operator { + case api.TolerationOpEqual, "": // empty operator means Equal + return t.Operator == api.TolerationOpEqual && t.Value == ss.Value + case api.TolerationOpExists: + return true + default: + klog.Errorf("Unknown toleration operator: %s", ss.Operator) + return false } - return false } diff --git a/pkg/util/tolerations/tolerations_test.go b/pkg/util/tolerations/tolerations_test.go index 3f88e30d3bc..ab2f4f23981 100644 --- a/pkg/util/tolerations/tolerations_test.go +++ b/pkg/util/tolerations/tolerations_test.go @@ -17,65 +17,397 @@ limitations under the License. package tolerations import ( - api "k8s.io/kubernetes/pkg/apis/core" + "encoding/json" + "fmt" + "math/rand" + "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/validation/field" + api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/apis/core/validation" + utilpointer "k8s.io/utils/pointer" ) +var ( + tolerations = map[string]api.Toleration{ + "all": {Operator: api.TolerationOpExists}, + "all-nosched": { + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoSchedule, + }, + "all-noexec": { + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, + }, + "foo": { + Key: "foo", + Operator: api.TolerationOpExists, + }, + "foo-bar": { + Key: "foo", + Operator: api.TolerationOpEqual, + Value: "bar", + }, + "foo-nosched": { + Key: "foo", + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoSchedule, + }, + "foo-bar-nosched": { + Key: "foo", + Operator: api.TolerationOpEqual, + Value: "bar", + Effect: api.TaintEffectNoSchedule, + }, + "foo-baz-nosched": { + Key: "foo", + Operator: api.TolerationOpEqual, + Value: "baz", + Effect: api.TaintEffectNoSchedule, + }, + "faz-nosched": { + Key: "faz", + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoSchedule, + }, + "faz-baz-nosched": { + Key: "faz", + Operator: api.TolerationOpEqual, + Value: "baz", + Effect: api.TaintEffectNoSchedule, + }, + "foo-prefnosched": { + Key: "foo", + Operator: api.TolerationOpExists, + Effect: api.TaintEffectPreferNoSchedule, + }, + "foo-noexec": { + Key: "foo", + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, + }, + "foo-bar-noexec": { + Key: "foo", + Operator: api.TolerationOpEqual, + Value: "bar", + Effect: api.TaintEffectNoExecute, + }, + "foo-noexec-10": { + Key: "foo", + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, + TolerationSeconds: utilpointer.Int64Ptr(10), + }, + "foo-noexec-0": { + Key: "foo", + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, + TolerationSeconds: utilpointer.Int64Ptr(0), + }, + "foo-bar-noexec-10": { + Key: "foo", + Operator: api.TolerationOpEqual, + Value: "bar", + Effect: api.TaintEffectNoExecute, + TolerationSeconds: utilpointer.Int64Ptr(10), + }, + } +) + +func TestIsSuperset(t *testing.T) { + tests := []struct { + toleration string + ss []string // t should be a superset of these + }{{ + "all", + []string{"all-nosched", "all-noexec", "foo", "foo-bar", "foo-nosched", "foo-bar-nosched", "foo-baz-nosched", "faz-nosched", "faz-baz-nosched", "foo-prefnosched", "foo-noexec", "foo-bar-noexec", "foo-noexec-10", "foo-noexec-0", "foo-bar-noexec-10"}, + }, { + "all-nosched", + []string{"foo-nosched", "foo-bar-nosched", "foo-baz-nosched", "faz-nosched", "faz-baz-nosched"}, + }, { + "all-noexec", + []string{"foo-noexec", "foo-bar-noexec", "foo-noexec-10", "foo-noexec-0", "foo-bar-noexec-10"}, + }, { + "foo", + []string{"foo-bar", "foo-nosched", "foo-bar-nosched", "foo-baz-nosched", "foo-prefnosched", "foo-noexec", "foo-bar-noexec", "foo-noexec-10", "foo-noexec-0", "foo-bar-noexec-10"}, + }, { + "foo-bar", + []string{"foo-bar-nosched", "foo-bar-noexec", "foo-bar-noexec-10"}, + }, { + "foo-nosched", + []string{"foo-bar-nosched", "foo-baz-nosched"}, + }, { + "foo-bar-nosched", + []string{}, + }, { + "faz-nosched", + []string{"faz-baz-nosched"}, + }, { + "faz-baz-nosched", + []string{}, + }, { + "foo-prenosched", + []string{}, + }, { + "foo-noexec", + []string{"foo-noexec", "foo-bar-noexec", "foo-noexec-10", "foo-noexec-0", "foo-bar-noexec-10"}, + }, { + "foo-bar-noexec", + []string{"foo-bar-noexec-10"}, + }, { + "foo-noexec-10", + []string{"foo-noexec-0", "foo-bar-noexec-10"}, + }, { + "foo-noexec-0", + []string{}, + }, { + "foo-bar-noexec-10", + []string{}, + }} + + assertSuperset := func(t *testing.T, super, sub string) { + assert.True(t, isSuperset(tolerations[super], tolerations[sub]), + "%s should be a superset of %s", super, sub) + } + assertNotSuperset := func(t *testing.T, super, sub string) { + assert.False(t, isSuperset(tolerations[super], tolerations[sub]), + "%s should NOT be a superset of %s", super, sub) + } + contains := func(ss []string, s string) bool { + for _, str := range ss { + if str == s { + return true + } + } + return false + } + + for _, test := range tests { + t.Run(test.toleration, func(t *testing.T) { + for name := range tolerations { + if name == test.toleration || contains(test.ss, name) { + assertSuperset(t, test.toleration, name) + } else { + assertNotSuperset(t, test.toleration, name) + } + } + }) + } +} + func TestVerifyAgainstWhitelist(t *testing.T) { tests := []struct { - input []api.Toleration - whitelist []api.Toleration - testName string - testStatus bool + testName string + input []string + whitelist []string + expected bool }{ { - input: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}}, - whitelist: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}}, - testName: "equal input and whitelist", - testStatus: true, + testName: "equal input and whitelist", + input: []string{"foo-bar-nosched", "foo-baz-nosched"}, + whitelist: []string{"foo-bar-nosched", "foo-baz-nosched"}, + expected: true, }, { - input: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}}, - whitelist: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoExecute"}}, - testName: "input does not exist in whitelist", - testStatus: false, + testName: "duplicate input allowed", + input: []string{"foo-bar-nosched", "foo-bar-nosched"}, + whitelist: []string{"foo-bar-nosched", "foo-baz-nosched"}, + expected: true, + }, + { + testName: "allow all", + input: []string{"foo-bar-nosched", "foo-bar-nosched"}, + whitelist: []string{"all"}, + expected: true, + }, + { + testName: "duplicate input forbidden", + input: []string{"foo-bar-nosched", "foo-bar-nosched"}, + whitelist: []string{"foo-baz-nosched"}, + expected: false, + }, + { + testName: "value mismatch", + input: []string{"foo-bar-nosched", "foo-baz-nosched"}, + whitelist: []string{"foo-baz-nosched"}, + expected: false, + }, + { + testName: "input does not exist in whitelist", + input: []string{"foo-bar-nosched"}, + whitelist: []string{"foo-baz-nosched"}, + expected: false, + }, + { + testName: "disjoint sets", + input: []string{"foo-bar"}, + whitelist: []string{"foo-nosched"}, + expected: false, + }, + { + testName: "empty whitelist", + input: []string{"foo-bar"}, + whitelist: []string{}, + expected: true, + }, + { + testName: "empty input", + input: []string{}, + whitelist: []string{"foo-bar"}, + expected: true, }, } for _, c := range tests { - status := VerifyAgainstWhitelist(c.input, c.whitelist) - if status != c.testStatus { - t.Errorf("Test: %s, expected %v", c.testName, status) - } + t.Run(c.testName, func(t *testing.T) { + actual := VerifyAgainstWhitelist(getTolerations(c.input), getTolerations(c.whitelist)) + assert.Equal(t, c.expected, actual) + }) } - } -func TestIsConflict(t *testing.T) { +func TestMergeTolerations(t *testing.T) { tests := []struct { - input1 []api.Toleration - input2 []api.Toleration - testName string - testStatus bool - }{ - { - input1: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}}, - input2: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}}, - testName: "equal inputs", - testStatus: true, - }, - { - input1: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "foo", Effect: "NoExecute"}}, - input2: []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoExecute"}}, - testName: "mismatch values in inputs", - testStatus: false, - }, - } + name string + a, b []string + expected []string + }{{ + name: "disjoint", + a: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"}, + b: []string{"foo-prefnosched", "foo-baz-nosched"}, + expected: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10", "foo-prefnosched", "foo-baz-nosched"}, + }, { + name: "duplicate", + a: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"}, + b: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"}, + expected: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"}, + }, { + name: "merge redundant", + a: []string{"foo-bar-nosched", "foo-baz-nosched"}, + b: []string{"foo-nosched", "faz-baz-nosched"}, + expected: []string{"foo-nosched", "faz-baz-nosched"}, + }, { + name: "merge all", + a: []string{"foo-bar-nosched", "foo-baz-nosched", "foo-noexec-10"}, + b: []string{"all"}, + expected: []string{"all"}, + }, { + name: "merge into all", + a: []string{"all"}, + b: []string{"foo-bar-nosched", "foo-baz-nosched", "foo-noexec-10"}, + expected: []string{"all"}, + }} - for _, c := range tests { - status := IsConflict(c.input1, c.input2) - if status == c.testStatus { - t.Errorf("Test: %s, expected %v", c.testName, status) - } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := MergeTolerations(getTolerations(test.a), getTolerations(test.b)) + require.Len(t, actual, len(test.expected)) + for i, expect := range getTolerations(test.expected) { + assert.Equal(t, expect, actual[i], "expected[%d] = %s", i, test.expected[i]) + } + }) } } + +func TestFuzzed(t *testing.T) { + r := rand.New(rand.NewSource(1234)) // Fixed source to prevent flakes. + + const ( + allProbability = 0.01 // Chance of getting a tolerate all + existsProbability = 0.3 + tolerationSecondsProbability = 0.5 + ) + effects := []api.TaintEffect{"", api.TaintEffectNoExecute, api.TaintEffectNoSchedule, api.TaintEffectPreferNoSchedule} + genToleration := func() api.Toleration { + gen := api.Toleration{ + Effect: effects[r.Intn(len(effects))], + } + if r.Float32() < allProbability { + gen = tolerations["all"] + return gen + } + // Small key/value space to encourage collisions + gen.Key = strings.Repeat("a", r.Intn(6)+1) + if r.Float32() < existsProbability { + gen.Operator = api.TolerationOpExists + } else { + gen.Operator = api.TolerationOpEqual + gen.Value = strings.Repeat("b", r.Intn(6)+1) + } + if gen.Effect == api.TaintEffectNoExecute && r.Float32() < tolerationSecondsProbability { + gen.TolerationSeconds = utilpointer.Int64Ptr(r.Int63n(10)) + } + // Ensure only valid tolerations are generated. + require.NoError(t, validation.ValidateTolerations([]api.Toleration{gen}, field.NewPath("")).ToAggregate(), "%#v", gen) + return gen + } + genTolerations := func() []api.Toleration { + result := []api.Toleration{} + for i := 0; i < r.Intn(10); i++ { + result = append(result, genToleration()) + } + return result + } + + // Check whether the toleration is a subset of a toleration in the set. + isContained := func(toleration api.Toleration, set []api.Toleration) bool { + for _, ss := range set { + if isSuperset(ss, toleration) { + return true + } + } + return false + } + + const iterations = 1000 + + debugMsg := func(tolerations ...[]api.Toleration) string { + str, err := json.Marshal(tolerations) + if err != nil { + return fmt.Sprintf("[ERR: %v] %v", err, tolerations) + } + return string(str) + } + t.Run("VerifyAgainstWhitelist", func(t *testing.T) { + for i := 0; i < iterations; i++ { + input := genTolerations() + whitelist := append(genTolerations(), genToleration()) // Non-empty + if VerifyAgainstWhitelist(input, whitelist) { + for _, tol := range input { + require.True(t, isContained(tol, whitelist), debugMsg(input, whitelist)) + } + } else { + uncontained := false + for _, tol := range input { + if !isContained(tol, whitelist) { + uncontained = true + break + } + } + require.True(t, uncontained, debugMsg(input, whitelist)) + } + } + }) + + t.Run("MergeTolerations", func(t *testing.T) { + for i := 0; i < iterations; i++ { + a := genTolerations() + b := genTolerations() + result := MergeTolerations(a, b) + for _, tol := range append(a, b...) { + require.True(t, isContained(tol, result), debugMsg(a, b, result)) + } + } + }) +} + +func getTolerations(names []string) []api.Toleration { + result := []api.Toleration{} + for _, name := range names { + result = append(result, tolerations[name]) + } + return result +}