From 9f533de80d497c86c776b74feb8ac2afdd42d234 Mon Sep 17 00:00:00 2001 From: Avesh Agarwal Date: Wed, 8 Mar 2017 16:22:39 -0500 Subject: [PATCH] Fix DefaultTolerationSeconds admission plugin. It was using versioned object whereas admission plugins operate on internal objects. --- pkg/api/helpers.go | 38 ++++ .../admission/defaulttolerationseconds/BUILD | 5 +- .../defaulttolerationseconds/admission.go | 33 ++-- .../admission_test.go | 165 +++++++++--------- .../defaulttolerationseconds_test.go | 104 +++++++++++ 5 files changed, 244 insertions(+), 101 deletions(-) create mode 100644 test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go diff --git a/pkg/api/helpers.go b/pkg/api/helpers.go index 4ada0ba3743..4705e11fed5 100644 --- a/pkg/api/helpers.go +++ b/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 , +// if the two tolerations have same 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 { diff --git a/plugin/pkg/admission/defaulttolerationseconds/BUILD b/plugin/pkg/admission/defaulttolerationseconds/BUILD index dbd842fcd2e..76b2a305c57 100644 --- a/plugin/pkg/admission/defaulttolerationseconds/BUILD +++ b/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", - "//vendor:github.com/golang/glog", + "//pkg/api:go_default_library", + "//vendor:k8s.io/apimachinery/pkg/api/errors", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apiserver/pkg/admission", ], diff --git a/plugin/pkg/admission/defaulttolerationseconds/admission.go b/plugin/pkg/admission/defaulttolerationseconds/admission.go index 4ed47197f83..b0f5a8c903a 100644 --- a/plugin/pkg/admission/defaulttolerationseconds/admission.go +++ b/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 !ok { - glog.Errorf("expected pod but got %s", attributes.GetKind().Kind) + if len(attributes.GetSubresource()) > 0 { + // only run the checks below on pods proper and not subresources 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, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: defaultUnreachableTolerationSeconds, }) if err != nil { diff --git a/plugin/pkg/admission/defaulttolerationseconds/admission_test.go b/plugin/pkg/admission/defaulttolerationseconds/admission_test.go index 7e20f702f87..eed602adf18 100644 --- a/plugin/pkg/admission/defaulttolerationseconds/admission_test.go +++ b/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 - expectedPod v1.Pod + requestedPod api.Pod + expectedPod api.Pod }{ { description: "pod has no tolerations, expect add tolerations for `notread:NoExecute` and `unreachable:NoExecute`", - requestedPod: v1.Pod{ - Spec: v1.PodSpec{}, + requestedPod: api.Pod{ + Spec: api.PodSpec{}, }, - expectedPod: v1.Pod{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: &defaultTolerationSeconds, }, { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + requestedPod: api.Pod{ + Spec: api.PodSpec{ + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + 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, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: &defaultTolerationSeconds, }, { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + requestedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, }, }, }, - expectedPod: v1.Pod{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + requestedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, }, }, }, - expectedPod: v1.Pod{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + requestedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, }, }, }, - expectedPod: v1.Pod{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoExecute, TolerationSeconds: genTolerationSeconds(700), }, { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + requestedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, + Operator: api.TolerationOpExists, TolerationSeconds: genTolerationSeconds(700), }, }, }, }, - expectedPod: v1.Pod{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ { Key: metav1.TaintNodeUnreachable, - Operator: v1.TolerationOpExists, + Operator: api.TolerationOpExists, TolerationSeconds: genTolerationSeconds(700), }, { Key: metav1.TaintNodeNotReady, - Operator: v1.TolerationOpExists, - Effect: v1.TaintEffectNoExecute, + Operator: api.TolerationOpExists, + 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{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ - {Operator: v1.TolerationOpExists, TolerationSeconds: genTolerationSeconds(700)}, + requestedPod: api.Pod{ + Spec: api.PodSpec{ + Tolerations: []api.Toleration{ + {Operator: api.TolerationOpExists, TolerationSeconds: genTolerationSeconds(700)}, }, }, }, - expectedPod: v1.Pod{ - Spec: v1.PodSpec{ - Tolerations: []v1.Toleration{ + expectedPod: api.Pod{ + Spec: api.PodSpec{ + 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) } diff --git a/test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go b/test/integration/defaulttolerationseconds/defaulttolerationseconds_test.go new file mode 100644 index 00000000000..948818ba110 --- /dev/null +++ b/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) + } +}