Merge pull request #54812 from aveshagarwal/master-pod-toleration-restrictions-issues

Automatic merge from submit-queue (batch tested with PRs 54800, 53898, 54812, 54921, 53558). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Allow override of cluster level (default, whitelist) tolerations by namespace level empty (default, whitelist) tolerations.

Currently In PodTolerationRestriction admission plugin, if namespace level default and whitelist of tolerations are nil or empty, they do not override cluster level default and whitelist tolerations. 

This PR fixes the plugin to not override cluster level tolerations only when namespace level toleration are nil. IOW, if namespace level toleration are empty, they override cluster level tolerations. To be more clear, if following annotations are set to empty, they override cluster level tolerations.
 
``` 
scheduler.alpha.kubernetes.io/defaultTolerations : ""
scheduler.alpha.kubernetes.io/tolerationsWhitelist: ""
```

This behavior is inline with PodNodeSelector admission plugin too.

@sjenning @derekwaynecarr 

**Release Note**:

```release-note
In PodTolerationRestriction admisson plugin, if namespace level tolerations are empty, now they override cluster level tolerations. 
```

plugin/pkg/admission/podtolerationrestriction/admission.go

@@ -125,7 +125,7 @@ func (p *podTolerationsPlugin) Admit(a admission.Attributes) error {
 
 		// If the namespace has not specified its default tolerations,
 		// fall back to cluster's default tolerations.
-		if len(ts) == 0 {
+		if ts == nil {
 			ts = p.pluginConfig.Default
 		}
 
@@ -157,7 +157,7 @@ func (p *podTolerationsPlugin) Admit(a admission.Attributes) error {
 
 		// If the namespace has not specified its tolerations whitelist,
 		// fall back to cluster's whitelist of tolerations.
-		if len(whitelist) == 0 {
+		if whitelist == nil {
 			whitelist = p.pluginConfig.Whitelist
 		}
 
@@ -220,13 +220,32 @@ func (p *podTolerationsPlugin) getNamespaceTolerationsWhitelist(ns *api.Namespac
 	return extractNSTolerations(ns, NSWLTolerations)
 }
 
+// extractNSTolerations extracts default or whitelist of tolerations from
+// following namespace annotations keys: "scheduler.alpha.kubernetes.io/defaultTolerations"
+// and "scheduler.alpha.kubernetes.io/tolerationsWhitelist". If these keys are
+// unset (nil), extractNSTolerations returns nil. If the value to these
+// keys are set to empty, an empty toleration is returned, otherwise
+// configured tolerations are returned.
 func extractNSTolerations(ns *api.Namespace, key string) ([]api.Toleration, error) {
+	// if a namespace does not have any annotations
+	if len(ns.Annotations) == 0 {
+		return nil, nil
+	}
+
+	// if NSWLTolerations or NSDefaultTolerations does not exist
+	if _, ok := ns.Annotations[key]; !ok {
+		return nil, nil
+	}
+
+	// if value is set to empty
+	if len(ns.Annotations[key]) == 0 {
+		return []api.Toleration{}, nil
+	}
+
 	var v1Tolerations []v1.Toleration
-	if len(ns.Annotations) > 0 && ns.Annotations[key] != "" {
-		err := json.Unmarshal([]byte(ns.Annotations[key]), &v1Tolerations)
-		if err != nil {
-			return nil, err
-		}
+	err := json.Unmarshal([]byte(ns.Annotations[key]), &v1Tolerations)
+	if err != nil {
+		return nil, err
 	}
 
 	ts := make([]api.Toleration, len(v1Tolerations))

plugin/pkg/admission/podtolerationrestriction/admission_test.go

@@ -115,11 +115,11 @@ func TestPodAdmission(t *testing.T) {
 		{
 			pod: bestEffortPod,
 			defaultClusterTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}},
-			namespaceTolerations:      []api.Toleration{},
+			namespaceTolerations:      nil,
 			podTolerations:            []api.Toleration{},
 			mergedTolerations:         []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}},
 			admit:                     true,
-			testName:                  "default cluster tolerations with empty pod tolerations",
+			testName:                  "default cluster tolerations with empty pod tolerations and nil namespace tolerations",
 		},
 		{
 			pod: bestEffortPod,
@@ -161,8 +161,9 @@ func TestPodAdmission(t *testing.T) {
 			defaultClusterTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue2", Effect: "NoSchedule", TolerationSeconds: nil}},
 			namespaceTolerations:      []api.Toleration{},
 			podTolerations:            []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}},
-			admit:                     false,
-			testName:                  "conflicting pod and default cluster tolerations",
+			mergedTolerations:         []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}},
+			admit:                     true,
+			testName:                  "conflicting pod and default cluster tolerations but overridden by empty namespace tolerations",
 		},
 		{
 			pod: bestEffortPod,
@@ -174,6 +175,24 @@ func TestPodAdmission(t *testing.T) {
 			admit:                     true,
 			testName:                  "merged pod tolerations satisfy whitelist",
 		},
+		{
+			pod: bestEffortPod,
+			defaultClusterTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}},
+			namespaceTolerations:      []api.Toleration{},
+			podTolerations:            []api.Toleration{},
+			mergedTolerations:         []api.Toleration{},
+			admit:                     true,
+			testName:                  "Override default cluster toleration by empty namespace level toleration",
+		},
+		{
+			pod:               bestEffortPod,
+			whitelist:         []api.Toleration{},
+			clusterWhitelist:  []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}},
+			podTolerations:    []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}},
+			mergedTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}},
+			admit:             true,
+			testName:          "pod toleration conflicts with default cluster white list which is overridden by empty namespace whitelist",
+		},
 		{
 			pod: bestEffortPod,
 			defaultClusterTolerations: []api.Toleration{},
@@ -211,7 +230,7 @@ func TestPodAdmission(t *testing.T) {
 		},
 	}
 	for _, test := range tests {
-		if len(test.namespaceTolerations) > 0 {
+		if test.namespaceTolerations != nil {
 			tolerationStr, err := json.Marshal(test.namespaceTolerations)
 			if err != nil {
 				t.Errorf("error in marshalling namespace tolerations %v", test.namespaceTolerations)
@@ -219,7 +238,7 @@ func TestPodAdmission(t *testing.T) {
 			namespace.Annotations = map[string]string{NSDefaultTolerations: string(tolerationStr)}
 		}
 
-		if len(test.whitelist) > 0 {
+		if test.whitelist != nil {
 			tolerationStr, err := json.Marshal(test.whitelist)
 			if err != nil {
 				t.Errorf("error in marshalling namespace whitelist %v", test.whitelist)