Fix DefaultTolerationSeconds admission plugin. It was using

versioned object whereas admission plugins operate on internal objects.

pkg/api/helpers.go

@@ -465,6 +465,44 @@ const (
 	ObjectTTLAnnotationKey string = "node.alpha.kubernetes.io/ttl"
 )
 
+// AddOrUpdateTolerationInPod tries to add a toleration to the pod's toleration list.
+// Returns true if something was updated, false otherwise.
+func AddOrUpdateTolerationInPod(pod *Pod, toleration *Toleration) (bool, error) {
+	podTolerations := pod.Spec.Tolerations
+
+	var newTolerations []Toleration
+	updated := false
+	for i := range podTolerations {
+		if toleration.MatchToleration(&podTolerations[i]) {
+			if Semantic.DeepEqual(toleration, podTolerations[i]) {
+				return false, nil
+			}
+			newTolerations = append(newTolerations, *toleration)
+			updated = true
+			continue
+		}
+
+		newTolerations = append(newTolerations, podTolerations[i])
+	}
+
+	if !updated {
+		newTolerations = append(newTolerations, *toleration)
+	}
+
+	pod.Spec.Tolerations = newTolerations
+	return true, nil
+}
+
+// MatchToleration checks if the toleration matches tolerationToMatch. Tolerations are unique by <key,effect,operator,value>,
+// if the two tolerations have same <key,effect,operator,value> combination, regard as they match.
+// TODO: uniqueness check for tolerations in api validations.
+func (t *Toleration) MatchToleration(tolerationToMatch *Toleration) bool {
+	return t.Key == tolerationToMatch.Key &&
+		t.Effect == tolerationToMatch.Effect &&
+		t.Operator == tolerationToMatch.Operator &&
+		t.Value == tolerationToMatch.Value
+}
+
 // TolerationToleratesTaint checks if the toleration tolerates the taint.
 func TolerationToleratesTaint(toleration *Toleration, taint *Taint) bool {
 	if len(toleration.Effect) != 0 && toleration.Effect != taint.Effect {

plugin/pkg/admission/defaulttolerationseconds/BUILD

@@ -15,7 +15,6 @@ go_test(
     tags = ["automanaged"],
     deps = [
         "//pkg/api:go_default_library",
-        "//pkg/api/v1:go_default_library",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apiserver/pkg/admission",
     ],
@@ -26,8 +25,8 @@ go_library(
     srcs = ["admission.go"],
     tags = ["automanaged"],
     deps = [
-        "//pkg/api/v1:go_default_library",
+        "//pkg/api:go_default_library",
-        "//vendor:github.com/golang/glog",
+        "//vendor:k8s.io/apimachinery/pkg/api/errors",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apiserver/pkg/admission",
     ],

plugin/pkg/admission/defaulttolerationseconds/admission.go

@@ -21,11 +21,10 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/golang/glog"
+	"k8s.io/apimachinery/pkg/api/errors"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/admission"
-	"k8s.io/kubernetes/pkg/api/v1"
+	"k8s.io/kubernetes/pkg/api"
 )
 
 var (
@@ -62,28 +61,32 @@ func NewDefaultTolerationSeconds() admission.Interface {
 }
 
 func (p *plugin) Admit(attributes admission.Attributes) (err error) {
-	if attributes.GetResource().GroupResource() != v1.Resource("pods") {
+	if attributes.GetResource().GroupResource() != api.Resource("pods") {
 		return nil
 	}
 
-	pod, ok := attributes.GetObject().(*v1.Pod)
+	if len(attributes.GetSubresource()) > 0 {
-	if !ok {
+		// only run the checks below on pods proper and not subresources
-		glog.Errorf("expected pod but got %s", attributes.GetKind().Kind)
 		return nil
 	}
 
+	pod, ok := attributes.GetObject().(*api.Pod)
+	if !ok {
+		return errors.NewBadRequest(fmt.Sprintf("expected *api.Pod but got %T", attributes.GetObject()))
+	}
+
 	tolerations := pod.Spec.Tolerations
 
 	toleratesNodeNotReady := false
 	toleratesNodeUnreachable := false
 	for _, toleration := range tolerations {
 		if (toleration.Key == metav1.TaintNodeNotReady || len(toleration.Key) == 0) &&
-			(toleration.Effect == v1.TaintEffectNoExecute || len(toleration.Effect) == 0) {
+			(toleration.Effect == api.TaintEffectNoExecute || len(toleration.Effect) == 0) {
 			toleratesNodeNotReady = true
 		}
 
 		if (toleration.Key == metav1.TaintNodeUnreachable || len(toleration.Key) == 0) &&
-			(toleration.Effect == v1.TaintEffectNoExecute || len(toleration.Effect) == 0) {
+			(toleration.Effect == api.TaintEffectNoExecute || len(toleration.Effect) == 0) {
 			toleratesNodeUnreachable = true
 		}
 	}
@@ -94,10 +97,10 @@ func (p *plugin) Admit(attributes admission.Attributes) (err error) {
 	}
 
 	if !toleratesNodeNotReady {
-		_, err := v1.AddOrUpdateTolerationInPod(pod, &v1.Toleration{
+		_, err := api.AddOrUpdateTolerationInPod(pod, &api.Toleration{
 			Key:               metav1.TaintNodeNotReady,
-			Operator:          v1.TolerationOpExists,
+			Operator:          api.TolerationOpExists,
-			Effect:            v1.TaintEffectNoExecute,
+			Effect:            api.TaintEffectNoExecute,
 			TolerationSeconds: defaultNotReadyTolerationSeconds,
 		})
 		if err != nil {
@@ -107,10 +110,10 @@ func (p *plugin) Admit(attributes admission.Attributes) (err error) {
 	}
 
 	if !toleratesNodeUnreachable {
-		_, err := v1.AddOrUpdateTolerationInPod(pod, &v1.Toleration{
+		_, err := api.AddOrUpdateTolerationInPod(pod, &api.Toleration{
 			Key:               metav1.TaintNodeUnreachable,
-			Operator:          v1.TolerationOpExists,
+			Operator:          api.TolerationOpExists,
-			Effect:            v1.TaintEffectNoExecute,
+			Effect:            api.TaintEffectNoExecute,
 			TolerationSeconds: defaultUnreachableTolerationSeconds,
 		})
 		if err != nil {

plugin/pkg/admission/defaulttolerationseconds/admission_test.go

@@ -22,7 +22,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/kubernetes/pkg/api"
-	"k8s.io/kubernetes/pkg/api/v1"
 )
 
 func TestForgivenessAdmission(t *testing.T) {
@@ -35,27 +34,27 @@ func TestForgivenessAdmission(t *testing.T) {
 	handler := NewDefaultTolerationSeconds()
 	tests := []struct {
 		description  string
-		requestedPod v1.Pod
+		requestedPod api.Pod
-		expectedPod  v1.Pod
+		expectedPod  api.Pod
 	}{
 		{
 			description: "pod has no tolerations, expect add tolerations for `notread:NoExecute` and `unreachable:NoExecute`",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{},
+				Spec: api.PodSpec{},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: &defaultTolerationSeconds,
 						},
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: &defaultTolerationSeconds,
 						},
 					},
@@ -64,39 +63,39 @@ func TestForgivenessAdmission(t *testing.T) {
 		},
 		{
 			description: "pod has tolerations, but none is for taint `notread:NoExecute` or `unreachable:NoExecute`, expect add tolerations for `notread:NoExecute` and `unreachable:NoExecute`",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               "foo",
-							Operator:          v1.TolerationOpEqual,
+							Operator:          api.TolerationOpEqual,
 							Value:             "bar",
-							Effect:            v1.TaintEffectNoSchedule,
+							Effect:            api.TaintEffectNoSchedule,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
 				},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               "foo",
-							Operator:          v1.TolerationOpEqual,
+							Operator:          api.TolerationOpEqual,
 							Value:             "bar",
-							Effect:            v1.TaintEffectNoSchedule,
+							Effect:            api.TaintEffectNoSchedule,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: &defaultTolerationSeconds,
 						},
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: &defaultTolerationSeconds,
 						},
 					},
@@ -105,31 +104,31 @@ func TestForgivenessAdmission(t *testing.T) {
 		},
 		{
 			description: "pod specified a toleration for taint `notReady:NoExecute`, expect add toleration for `unreachable:NoExecute`",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
 				},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: &defaultTolerationSeconds,
 						},
 					},
@@ -138,31 +137,31 @@ func TestForgivenessAdmission(t *testing.T) {
 		},
 		{
 			description: "pod specified a toleration for taint `unreachable:NoExecute`, expect add toleration for `notReady:NoExecute`",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
 				},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: &defaultTolerationSeconds,
 						},
 					},
@@ -171,37 +170,37 @@ func TestForgivenessAdmission(t *testing.T) {
 		},
 		{
 			description: "pod specified tolerations for both `notread:NoExecute` and `unreachable:NoExecute`, expect no change",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
 				},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
@@ -210,29 +209,29 @@ func TestForgivenessAdmission(t *testing.T) {
 		},
 		{
 			description: "pod specified toleration for taint `unreachable`, expect add toleration for `notReady:NoExecute`",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
 				},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
 							Key:               metav1.TaintNodeUnreachable,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 						{
 							Key:               metav1.TaintNodeNotReady,
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
-							Effect:            v1.TaintEffectNoExecute,
+							Effect:            api.TaintEffectNoExecute,
 							TolerationSeconds: genTolerationSeconds(300),
 						},
 					},
@@ -241,18 +240,18 @@ func TestForgivenessAdmission(t *testing.T) {
 		},
 		{
 			description: "pod has wildcard toleration for all kind of taints, expect no change",
-			requestedPod: v1.Pod{
+			requestedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
-						{Operator: v1.TolerationOpExists, TolerationSeconds: genTolerationSeconds(700)},
+						{Operator: api.TolerationOpExists, TolerationSeconds: genTolerationSeconds(700)},
 					},
 				},
 			},
-			expectedPod: v1.Pod{
+			expectedPod: api.Pod{
-				Spec: v1.PodSpec{
+				Spec: api.PodSpec{
-					Tolerations: []v1.Toleration{
+					Tolerations: []api.Toleration{
 						{
-							Operator:          v1.TolerationOpExists,
+							Operator:          api.TolerationOpExists,
 							TolerationSeconds: genTolerationSeconds(700),
 						},
 					},
@@ -262,7 +261,7 @@ func TestForgivenessAdmission(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		err := handler.Admit(admission.NewAttributesRecord(&test.requestedPod, nil, api.Kind("Pod").WithVersion("version"), "foo", "name", v1.Resource("pods").WithVersion("version"), "", "ignored", nil))
+		err := handler.Admit(admission.NewAttributesRecord(&test.requestedPod, nil, api.Kind("Pod").WithVersion("version"), "foo", "name", api.Resource("pods").WithVersion("version"), "", "ignored", nil))
 		if err != nil {
 			t.Errorf("[%s]: unexpected error %v for pod %+v", test.description, err, test.requestedPod)
 		}

test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go

@@ -0,0 +1,104 @@
+// +build integration,!no-etcd
+
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package defaulttolerationseconds
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	restclient "k8s.io/client-go/rest"
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/v1"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
+	"k8s.io/kubernetes/plugin/pkg/admission/defaulttolerationseconds"
+	"k8s.io/kubernetes/test/integration/framework"
+)
+
+func TestAdmission(t *testing.T) {
+	masterConfig := framework.NewMasterConfig()
+	masterConfig.GenericConfig.EnableProfiling = true
+	masterConfig.GenericConfig.EnableMetrics = true
+	masterConfig.GenericConfig.AdmissionControl = defaulttolerationseconds.NewDefaultTolerationSeconds()
+	_, s := framework.RunAMaster(masterConfig)
+	defer s.Close()
+
+	client := clientset.NewForConfigOrDie(&restclient.Config{Host: s.URL, ContentConfig: restclient.ContentConfig{GroupVersion: &api.Registry.GroupOrDie(v1.GroupName).GroupVersion}})
+
+	ns := framework.CreateTestingNamespace("default-toleration-seconds", s, t)
+	defer framework.DeleteTestingNamespace(ns, s, t)
+
+	pod := v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: ns.Name,
+			Name:      "foo",
+		},
+		Spec: v1.PodSpec{
+			Containers: []v1.Container{
+				{
+					Name:  "test",
+					Image: "an-image",
+				},
+			},
+		},
+	}
+
+	updatedPod, err := client.Core().Pods(pod.Namespace).Create(&pod)
+	if err != nil {
+		t.Fatalf("error creating pod: %v", err)
+	}
+
+	var defaultSeconds int64 = 300
+	nodeNotReady := v1.Toleration{
+		Key:               metav1.TaintNodeNotReady,
+		Operator:          v1.TolerationOpExists,
+		Effect:            v1.TaintEffectNoExecute,
+		TolerationSeconds: &defaultSeconds,
+	}
+
+	nodeUnreachable := v1.Toleration{
+		Key:               metav1.TaintNodeUnreachable,
+		Operator:          v1.TolerationOpExists,
+		Effect:            v1.TaintEffectNoExecute,
+		TolerationSeconds: &defaultSeconds,
+	}
+
+	found := 0
+	tolerations := updatedPod.Spec.Tolerations
+	for i := range tolerations {
+		if found == 2 {
+			break
+		}
+		if tolerations[i].MatchToleration(&nodeNotReady) {
+			if api.Semantic.DeepEqual(tolerations[i], nodeNotReady) {
+				found++
+				continue
+			}
+		}
+		if tolerations[i].MatchToleration(&nodeUnreachable) {
+			if api.Semantic.DeepEqual(tolerations[i], nodeUnreachable) {
+				found++
+				continue
+			}
+		}
+	}
+
+	if found != 2 {
+		t.Fatalf("unexpected tolerations: %v\n", updatedPod.Spec.Tolerations)
+	}
+}