Fix toleration comparison & merging logic

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

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",
+    ],
 )

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 {
+func VerifyAgainstWhitelist(tolerations, whitelist []api.Toleration) bool {
-	if len(whitelist) == 0 {
+	if len(whitelist) == 0 || len(tolerations) == 0 {
 		return true
 	}
 
-	t := ConvertTolerationToAMap(tolerations)
+next:
-	w := ConvertTolerationToAMap(whitelist)
+	for _, t := range tolerations {
-
+		for _, w := range whitelist {
-	for k1, v1 := range t {
+			if isSuperset(w, t) {
-		if v2, ok := w[k1]; !ok || !AreEqual(v1, v2) {
+				continue next
-			return false
-		}
-	}
-	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
+// MergeTolerations merges two sets of tolerations into one. If one toleration is a superset of
-func ConvertTolerationToAMap(in []api.Toleration) map[key]api.Toleration {
+// another, only the superset is kept.
-	out := map[key]api.Toleration{}
+func MergeTolerations(first, second []api.Toleration) []api.Toleration {
-	for _, v := range in {
+	all := append(first, second...)
-		out[convertTolerationToKey(v)] = v
+	merged := []api.Toleration{}
+
+next:
+	for i, t := range all {
+		for _, t2 := range merged {
+			if isSuperset(t2, t) {
+				continue next // t is redundant; ignore it
 			}
-	return out
+		}
+		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)
 	}
 
-// AreEqual checks if two provided tolerations are equal or not.
+	return merged
-func AreEqual(first, second api.Toleration) bool {
+}
-	if first.Key == second.Key &&
+
-		first.Operator == second.Operator &&
+// isSuperset checks whether ss tolerates a superset of t.
-		first.Value == second.Value &&
+func isSuperset(ss, t api.Toleration) bool {
-		first.Effect == second.Effect &&
+	if apiequality.Semantic.DeepEqual(&t, &ss) {
-		AreTolerationSecondsEqual(first.TolerationSeconds, second.TolerationSeconds) {
 		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
 	}
 
-// AreTolerationSecondsEqual checks if two provided TolerationSeconds are equal or not.
+	// An empty effect means match all effects.
-func AreTolerationSecondsEqual(ts1, ts2 *int64) bool {
+	if t.Effect != ss.Effect && ss.Effect != "" {
-	if ts1 == ts2 {
-		return true
-	}
-	if ts1 != nil && ts2 != nil && *ts1 == *ts2 {
-		return true
-	}
 		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
+	}
+}

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
+		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,
+			input:     []string{"foo-bar-nosched", "foo-baz-nosched"},
+			whitelist: []string{"foo-bar-nosched", "foo-baz-nosched"},
+			expected:  true,
+		},
+		{
+			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,
 		},
 		{
-			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,
+			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)
+		t.Run(c.testName, func(t *testing.T) {
-		if status != c.testStatus {
+			actual := VerifyAgainstWhitelist(getTolerations(c.input), getTolerations(c.whitelist))
-			t.Errorf("Test: %s, expected %v", c.testName, status)
+			assert.Equal(t, c.expected, actual)
+		})
 	}
 }
 
-}
+func TestMergeTolerations(t *testing.T) {
-
-func TestIsConflict(t *testing.T) {
 	tests := []struct {
-		input1     []api.Toleration
+		name     string
-		input2     []api.Toleration
+		a, b     []string
-		testName   string
+		expected []string
-		testStatus bool
+	}{{
-	}{
+		name:     "disjoint",
-		{
+		a:        []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"},
-			input1:     []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}},
+		b:        []string{"foo-prefnosched", "foo-baz-nosched"},
-			input2:     []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoSchedule"}},
+		expected: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10", "foo-prefnosched", "foo-baz-nosched"},
-			testName:   "equal inputs",
+	}, {
-			testStatus: true,
+		name:     "duplicate",
-		},
+		a:        []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"},
-		{
+		b:        []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"},
-			input1:     []api.Toleration{{Key: "foo", Operator: "Equal", Value: "foo", Effect: "NoExecute"}},
+		expected: []string{"foo-bar-nosched", "faz-baz-nosched", "foo-noexec-10"},
-			input2:     []api.Toleration{{Key: "foo", Operator: "Equal", Value: "bar", Effect: "NoExecute"}},
+	}, {
-			testName:   "mismatch values in inputs",
+		name:     "merge redundant",
-			testStatus: false,
+		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 _, 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])
+			}
+		})
+	}
 }
 
-	for _, c := range tests {
+func TestFuzzed(t *testing.T) {
-		status := IsConflict(c.input1, c.input2)
+	r := rand.New(rand.NewSource(1234)) // Fixed source to prevent flakes.
-		if status == c.testStatus {
+
-			t.Errorf("Test: %s, expected %v", c.testName, status)
+	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
 }