Remove conflict logic from PodTolerationRestriction

Author: tallclair

pkg/util/tolerations/tolerations.go

@@ -46,7 +46,7 @@ next:
 // another, only the superset is kept.
 func MergeTolerations(first, second []api.Toleration) []api.Toleration {
 	all := append(first, second...)
-	merged := []api.Toleration{}
+	var merged []api.Toleration
 
 next:
 	for i, t := range all {
@@ -58,7 +58,7 @@ next:
 		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) {
+				if !apiequality.Semantic.DeepEqual(&t, &t2) && isSuperset(t2, t) {
 					continue next // t is redundant; ignore it
 				}
 			}

plugin/pkg/admission/podtolerationrestriction/BUILD

@@ -14,7 +14,6 @@ go_test(
         "//pkg/apis/core:go_default_library",
         "//pkg/features:go_default_library",
         "//pkg/scheduler/api:go_default_library",
-        "//pkg/util/tolerations:go_default_library",
         "//plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
@@ -27,6 +26,7 @@ go_test(
         "//staging/src/k8s.io/client-go/kubernetes:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
         "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
     ],
 )
 

plugin/pkg/admission/podtolerationrestriction/admission.go

@@ -84,7 +84,7 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission.
 	}
 
 	pod := a.GetObject().(*api.Pod)
-	var finalTolerations []api.Toleration
+	var extraTolerations []api.Toleration
 	if a.GetOperation() == admission.Create {
 		ts, err := p.getNamespaceDefaultTolerations(a.GetNamespace())
 		if err != nil {
@@ -97,37 +97,20 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission.
 			ts = p.pluginConfig.Default
 		}
 
-		if len(ts) > 0 {
-			if len(pod.Spec.Tolerations) > 0 {
-				if tolerations.IsConflict(ts, pod.Spec.Tolerations) {
-					return fmt.Errorf("namespace tolerations and pod tolerations conflict")
-				}
-
-				// modified pod tolerations = namespace tolerations + current pod tolerations
-				finalTolerations = tolerations.MergeTolerations(ts, pod.Spec.Tolerations)
-			} else {
-				finalTolerations = ts
-
-			}
-		} else {
-			finalTolerations = pod.Spec.Tolerations
-		}
-	} else {
-		finalTolerations = pod.Spec.Tolerations
+		extraTolerations = ts
 	}
 
 	if qoshelper.GetPodQOS(pod) != api.PodQOSBestEffort {
-		finalTolerations = tolerations.MergeTolerations(finalTolerations, []api.Toleration{
-			{
-				Key:      schedulerapi.TaintNodeMemoryPressure,
-				Operator: api.TolerationOpExists,
-				Effect:   api.TaintEffectNoSchedule,
-			},
+		extraTolerations = append(extraTolerations, api.Toleration{
+			Key:      schedulerapi.TaintNodeMemoryPressure,
+			Operator: api.TolerationOpExists,
+			Effect:   api.TaintEffectNoSchedule,
 		})
 	}
-	// Final merge of tolerations irrespective of pod type, if the user while creating pods gives
-	// conflicting tolerations(with same key+effect), the existing ones should be overwritten by latest one
-	pod.Spec.Tolerations = tolerations.MergeTolerations(finalTolerations, []api.Toleration{})
+	// Final merge of tolerations irrespective of pod type.
+	if len(extraTolerations) > 0 {
+		pod.Spec.Tolerations = tolerations.MergeTolerations(pod.Spec.Tolerations, extraTolerations)
+	}
 	return p.Validate(ctx, a, o)
 }
 

plugin/pkg/admission/podtolerationrestriction/admission_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -36,7 +37,6 @@ import (
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/features"
 	schedulerapi "k8s.io/kubernetes/pkg/scheduler/api"
-	"k8s.io/kubernetes/pkg/util/tolerations"
 	pluginapi "k8s.io/kubernetes/plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction"
 )
 
@@ -139,10 +139,11 @@ func TestPodAdmission(t *testing.T) {
 		{
 			pod:                       bestEffortPod,
 			defaultClusterTolerations: []api.Toleration{},
-			namespaceTolerations:      []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}},
-			podTolerations:            []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}},
-			admit:                     false,
-			testName:                  "conflicting pod and namespace tolerations",
+			namespaceTolerations:      []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule"}},
+			podTolerations:            []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule"}},
+			mergedTolerations:         []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule"}, {Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule"}},
+			admit:                     true,
+			testName:                  "duplicate key pod and namespace tolerations",
 		},
 		{
 			pod:                       bestEffortPod,
@@ -210,10 +211,11 @@ func TestPodAdmission(t *testing.T) {
 			whitelist:                 []api.Toleration{},
 			podTolerations:            []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}, {Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}},
 			mergedTolerations: []api.Toleration{
+				{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil},
 				{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil},
 			},
 			admit:    true,
-			testName: "Besteffort pod should overwrite for conflicting tolerations",
+			testName: "Pod with duplicate key tolerations should not be modified",
 		},
 		{
 			pod:                       guaranteedPod,
@@ -275,8 +277,8 @@ func TestPodAdmission(t *testing.T) {
 			}
 
 			updatedPodTolerations := pod.Spec.Tolerations
-			if test.admit && !tolerations.EqualTolerations(updatedPodTolerations, test.mergedTolerations) {
-				t.Errorf("Test: %s, expected: %#v but got: %#v", test.testName, test.mergedTolerations, updatedPodTolerations)
+			if test.admit {
+				assert.ElementsMatch(t, updatedPodTolerations, test.mergedTolerations)
 			}
 		})
 	}