Merge pull request #50399 from caesarxuchao/update-admission-plugin

Automatic merge from submit-queue (batch tested with PRs 50536, 50809, 50220, 50399, 50176)

Make admission plugin handle mutating spec of uninitialized pods

Address https://github.com/kubernetes/kubernetes/issues/47837#issuecomment-321323243.

Updated to handle mutating pod spec of uninitialized pods:
* InitialResources
* PodNodeSelector
* PodTolerationRestriction
* ServiceAccount


Doesn't change:
* NodeRestriction: this plugin only cares about the mirror pods created by nodes, and mirror pods are exempted from initializers, so no modification required
* PersistentVolumeLabel, DefaultStorageClass: It only cares about PersistentVolume. We can revisit when we relax its validation.
* InitialResource: deprecated according to https://github.com/kubernetes/kubernetes/issues/47837#issuecomment-321388879

pkg/kubeapiserver/admission/BUILD

@@ -42,6 +42,7 @@ filegroup(
     srcs = [
         ":package-srcs",
         "//pkg/kubeapiserver/admission/configuration:all-srcs",
+        "//pkg/kubeapiserver/admission/util:all-srcs",
     ],
     tags = ["automanaged"],
 )

pkg/kubeapiserver/admission/util/BUILD

@@ -0,0 +1,31 @@
+package(default_visibility = ["//visibility:public"])
+
+licenses(["notice"])
+
+load(
+    "@io_bazel_rules_go//go:def.bzl",
+    "go_library",
+)
+
+go_library(
+    name = "go_default_library",
+    srcs = ["initializer.go"],
+    tags = ["automanaged"],
+    deps = [
+        "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+)

pkg/kubeapiserver/admission/util/initializer.go

@@ -0,0 +1,56 @@
+/*
+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 util
+
+import (
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apiserver/pkg/admission"
+)
+
+// IsUpdatingInitializedObject returns if the operation is trying to update an
+// already initialized object.
+func IsUpdatingInitializedObject(a admission.Attributes) (bool, error) {
+	if a.GetOperation() != admission.Update {
+		return false, nil
+	}
+	oldObj := a.GetOldObject()
+	accessor, err := meta.Accessor(oldObj)
+	if err != nil {
+		return false, err
+	}
+	if accessor.GetInitializers() == nil {
+		return true, nil
+	}
+	return false, nil
+}
+
+// IsUpdatingUninitializedObject returns if the operation is trying to update an
+// object that is not initialized yet.
+func IsUpdatingUninitializedObject(a admission.Attributes) (bool, error) {
+	if a.GetOperation() != admission.Update {
+		return false, nil
+	}
+	oldObj := a.GetOldObject()
+	accessor, err := meta.Accessor(oldObj)
+	if err != nil {
+		return false, err
+	}
+	if accessor.GetInitializers() == nil {
+		return false, nil
+	}
+	return true, nil
+}

plugin/pkg/admission/podnodeselector/BUILD

@@ -15,6 +15,7 @@ go_library(
         "//pkg/client/informers/informers_generated/internalversion:go_default_library",
         "//pkg/client/listers/core/internalversion:go_default_library",
         "//pkg/kubeapiserver/admission:go_default_library",
+        "//pkg/kubeapiserver/admission/util:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",

plugin/pkg/admission/podnodeselector/admission.go

@@ -33,6 +33,7 @@ import (
 	informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
 	corelisters "k8s.io/kubernetes/pkg/client/listers/core/internalversion"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
+	"k8s.io/kubernetes/pkg/kubeapiserver/admission/util"
 )
 
 // The annotation key scheduler.alpha.kubernetes.io/node-selector is for assigning
@@ -112,11 +113,20 @@ func (p *podNodeSelector) Admit(a admission.Attributes) error {
 		return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
 	}
 
+	updateInitialized, err := util.IsUpdatingInitializedObject(a)
+	if err != nil {
+		return err
+	}
+	if updateInitialized {
+		// node selector of an initialized pod is immutable
+		return nil
+	}
+
 	name := pod.Name
 	nsName := a.GetNamespace()
 	var namespace *api.Namespace
 
-	namespace, err := p.namespaceLister.Get(nsName)
+	namespace, err = p.namespaceLister.Get(nsName)
 	if errors.IsNotFound(err) {
 		namespace, err = p.defaultGetNamespace(nsName)
 		if err != nil {
@@ -158,7 +168,7 @@ func (p *podNodeSelector) Admit(a admission.Attributes) error {
 
 func NewPodNodeSelector(clusterNodeSelectors map[string]string) *podNodeSelector {
 	return &podNodeSelector{
-		Handler:              admission.NewHandler(admission.Create),
+		Handler:              admission.NewHandler(admission.Create, admission.Update),
 		clusterNodeSelectors: clusterNodeSelectors,
 	}
 }

plugin/pkg/admission/podnodeselector/admission_test.go

@@ -52,6 +52,12 @@ func TestPodAdmission(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{Name: "testPod", Namespace: "testNamespace"},
 	}
 
+	oldPod := *pod
+	oldPod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "init"}}}
+	oldPod.Spec.NodeSelector = map[string]string{
+		"old": "true",
+	}
+
 	tests := []struct {
 		defaultNodeSelector             string
 		namespaceNodeSelector           string
@@ -166,7 +172,17 @@ func TestPodAdmission(t *testing.T) {
 		} else if !test.admit && err == nil {
 			t.Errorf("Test: %s, expected an error", test.testName)
 		}
+		if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
+			t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector)
+		}
 
+		// handles update of uninitialized pod like it's newly created.
+		err = handler.Admit(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
+		if test.admit && err != nil {
+			t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
+		} else if !test.admit && err == nil {
+			t.Errorf("Test: %s, expected an error", test.testName)
+		}
 		if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
 			t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector)
 		}
@@ -176,7 +192,7 @@ func TestPodAdmission(t *testing.T) {
 func TestHandles(t *testing.T) {
 	for op, shouldHandle := range map[admission.Operation]bool{
 		admission.Create:  true,
-		admission.Update:  false,
+		admission.Update:  true,
 		admission.Connect: false,
 		admission.Delete:  false,
 	} {
@@ -187,6 +203,40 @@ func TestHandles(t *testing.T) {
 	}
 }
 
+func TestIgnoreUpdatingInitializedPod(t *testing.T) {
+	mockClient := &fake.Clientset{}
+	handler, informerFactory, err := newHandlerForTest(mockClient)
+	if err != nil {
+		t.Errorf("unexpected error initializing handler: %v", err)
+	}
+	handler.SetReadyFunc(func() bool { return true })
+
+	podNodeSelector := map[string]string{"infra": "false"}
+	pod := &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{Name: "testPod", Namespace: "testNamespace"},
+		Spec:       api.PodSpec{NodeSelector: podNodeSelector},
+	}
+	// this conflicts with podNodeSelector
+	namespaceNodeSelector := "infra=true"
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "testNamespace",
+			Namespace:   "",
+			Annotations: map[string]string{"scheduler.alpha.kubernetes.io/node-selector": namespaceNodeSelector},
+		},
+	}
+	err = informerFactory.Core().InternalVersion().Namespaces().Informer().GetStore().Update(namespace)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// if the update of initialized pod is not ignored, an error will be returned because the pod's nodeSelector conflicts with namespace's nodeSelector.
+	err = handler.Admit(admission.NewAttributesRecord(pod, pod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
+	if err != nil {
+		t.Errorf("expected no error, got: %v", err)
+	}
+}
+
 // newHandlerForTest returns the admission controller configured for testing.
 func newHandlerForTest(c clientset.Interface) (*podNodeSelector, informers.SharedInformerFactory, error) {
 	f := informers.NewSharedInformerFactory(c, 5*time.Minute)

plugin/pkg/admission/podtolerationrestriction/BUILD

@@ -40,6 +40,7 @@ go_library(
         "//pkg/client/informers/informers_generated/internalversion:go_default_library",
         "//pkg/client/listers/core/internalversion:go_default_library",
         "//pkg/kubeapiserver/admission:go_default_library",
+        "//pkg/kubeapiserver/admission/util:go_default_library",
         "//pkg/util/tolerations:go_default_library",
         "//plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction:go_default_library",
         "//plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction/install:go_default_library",

plugin/pkg/admission/podtolerationrestriction/admission.go

@@ -34,6 +34,7 @@ import (
 	informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
 	corelisters "k8s.io/kubernetes/pkg/client/listers/core/internalversion"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
+	"k8s.io/kubernetes/pkg/kubeapiserver/admission/util"
 	"k8s.io/kubernetes/pkg/util/tolerations"
 	pluginapi "k8s.io/kubernetes/plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction"
 	"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm"
@@ -112,7 +113,11 @@ func (p *podTolerationsPlugin) Admit(a admission.Attributes) error {
 	}
 
 	var finalTolerations []api.Toleration
-	if a.GetOperation() == admission.Create {
+	updateUninitialized, err := util.IsUpdatingUninitializedObject(a)
+	if err != nil {
+		return err
+	}
+	if a.GetOperation() == admission.Create || updateUninitialized {
 		ts, err := p.getNamespaceDefaultTolerations(namespace)
 		if err != nil {
 			return err

plugin/pkg/admission/podtolerationrestriction/admission_test.go

@@ -233,6 +233,11 @@ func TestPodAdmission(t *testing.T) {
 		pod := test.pod
 		pod.Spec.Tolerations = test.podTolerations
 
+		// copy the original pod for tests of uninitialized pod updates.
+		oldPod := *pod
+		oldPod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "init"}}}
+		oldPod.Spec.Tolerations = []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}}
+
 		err := handler.Admit(admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Create, nil))
 		if test.admit && err != nil {
 			t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
@@ -244,6 +249,19 @@ func TestPodAdmission(t *testing.T) {
 		if test.admit && !tolerations.EqualTolerations(updatedPodTolerations, test.mergedTolerations) {
 			t.Errorf("Test: %s, expected: %#v but got: %#v", test.testName, test.mergedTolerations, updatedPodTolerations)
 		}
+
+		// handles update of uninitialized pod like it's newly created.
+		err = handler.Admit(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
+		if test.admit && err != nil {
+			t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
+		} else if !test.admit && err == nil {
+			t.Errorf("Test: %s, expected an error", test.testName)
+		}
+
+		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)
+		}
 	}
 }
 
@@ -267,6 +285,54 @@ func TestHandles(t *testing.T) {
 	}
 }
 
+func TestIgnoreUpdatingInitializedPod(t *testing.T) {
+	mockClient := &fake.Clientset{}
+	handler, informerFactory, err := newHandlerForTest(mockClient)
+	if err != nil {
+		t.Errorf("unexpected error initializing handler: %v", err)
+	}
+	handler.SetReadyFunc(func() bool { return true })
+
+	pod := &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{Name: "testPod", Namespace: "testNamespace"},
+		Spec:       api.PodSpec{},
+	}
+	podToleration := api.Toleration{
+		Key:               "testKey",
+		Operator:          "Equal",
+		Value:             "testValue1",
+		Effect:            "NoSchedule",
+		TolerationSeconds: nil,
+	}
+	pod.Spec.Tolerations = []api.Toleration{podToleration}
+
+	// this conflicts with pod's Tolerations
+	namespaceToleration := podToleration
+	namespaceToleration.Value = "testValue2"
+	namespaceTolerations := []api.Toleration{namespaceToleration}
+	tolerationsStr, err := json.Marshal(namespaceTolerations)
+	if err != nil {
+		t.Errorf("error in marshalling namespace tolerations %v", namespaceTolerations)
+	}
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "testNamespace",
+			Namespace: "",
+		},
+	}
+	namespace.Annotations = map[string]string{NSDefaultTolerations: string(tolerationsStr)}
+	err = informerFactory.Core().InternalVersion().Namespaces().Informer().GetStore().Update(namespace)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// if the update of initialized pod is not ignored, an error will be returned because the pod's Tolerations conflicts with namespace's Tolerations.
+	err = handler.Admit(admission.NewAttributesRecord(pod, pod, api.Kind("Pod").WithVersion("version"), "testNamespace", pod.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
+	if err != nil {
+		t.Errorf("expected no error, got: %v", err)
+	}
+}
+
 // newHandlerForTest returns the admission controller configured for testing.
 func newHandlerForTest(c clientset.Interface) (*podTolerationsPlugin, informers.SharedInformerFactory, error) {
 	f := informers.NewSharedInformerFactory(c, 5*time.Minute)

plugin/pkg/admission/serviceaccount/BUILD

@@ -19,6 +19,7 @@ go_library(
         "//pkg/client/informers/informers_generated/internalversion:go_default_library",
         "//pkg/client/listers/core/internalversion:go_default_library",
         "//pkg/kubeapiserver/admission:go_default_library",
+        "//pkg/kubeapiserver/admission/util:go_default_library",
         "//pkg/serviceaccount:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",

plugin/pkg/admission/serviceaccount/admission.go

@@ -36,6 +36,7 @@ import (
 	informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
 	corelisters "k8s.io/kubernetes/pkg/client/listers/core/internalversion"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
+	"k8s.io/kubernetes/pkg/kubeapiserver/admission/util"
 	"k8s.io/kubernetes/pkg/serviceaccount"
 )
 
@@ -90,7 +91,7 @@ var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&serviceAccount{
 // 5. If MountServiceAccountToken is true, it adds a VolumeMount with the pod's ServiceAccount's api token secret to containers
 func NewServiceAccount() *serviceAccount {
 	return &serviceAccount{
-		Handler: admission.NewHandler(admission.Create),
+		Handler: admission.NewHandler(admission.Create, admission.Update),
 		// TODO: enable this once we've swept secret usage to account for adding secret references to service accounts
 		LimitSecretReferences: false,
 		// Auto mount service account API token secrets
@@ -143,6 +144,15 @@ func (s *serviceAccount) Admit(a admission.Attributes) (err error) {
 		return nil
 	}
 
+	updateInitialized, err := util.IsUpdatingInitializedObject(a)
+	if err != nil {
+		return err
+	}
+	if updateInitialized {
+		// related pod spec fields are immutable after the pod is initialized
+		return nil
+	}
+
 	// Don't modify the spec of mirror pods.
 	// That makes the kubelet very angry and confused, and it immediately deletes the pod (because the spec doesn't match)
 	// That said, don't allow mirror pods to reference ServiceAccounts or SecretVolumeSources either

plugin/pkg/admission/serviceaccount/admission_test.go

@@ -36,7 +36,7 @@ import (
 
 func TestIgnoresNonCreate(t *testing.T) {
 	pod := &api.Pod{}
-	for _, op := range []admission.Operation{admission.Update, admission.Delete, admission.Connect} {
+	for _, op := range []admission.Operation{admission.Delete, admission.Connect} {
 		attrs := admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("pods").WithVersion("version"), "", op, nil)
 		handler := admission.NewChainHandler(NewServiceAccount())
 		err := handler.Admit(attrs)
@@ -46,6 +46,17 @@ func TestIgnoresNonCreate(t *testing.T) {
 	}
 }
 
+func TestIgnoresUpdateOfInitializedPod(t *testing.T) {
+	pod := &api.Pod{}
+	oldPod := &api.Pod{}
+	attrs := admission.NewAttributesRecord(pod, oldPod, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("pods").WithVersion("version"), "", admission.Update, nil)
+	handler := admission.NewChainHandler(NewServiceAccount())
+	err := handler.Admit(attrs)
+	if err != nil {
+		t.Errorf("Expected update of initialized pod allowed, got err: %v", err)
+	}
+}
+
 func TestIgnoresNonPodResource(t *testing.T) {
 	pod := &api.Pod{}
 	attrs := admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("CustomResource").WithVersion("version"), "", admission.Create, nil)
@@ -309,6 +320,55 @@ func TestAutomountsAPIToken(t *testing.T) {
 		t.Fatalf("Expected\n\t%#v\ngot\n\t%#v", expectedVolumeMount, pod.Spec.Containers[0].VolumeMounts[0])
 	}
 
+	// Test ServiceAccount admission plugin applies the same changes if the
+	// operation is an update to an uninitialized pod.
+	oldPod := &api.Pod{
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					// the volumeMount in the oldPod shouldn't affect the result.
+					VolumeMounts: []api.VolumeMount{
+						{
+							Name:      "wrong-" + tokenName,
+							ReadOnly:  true,
+							MountPath: DefaultAPITokenMountPath,
+						},
+					},
+				},
+			},
+		},
+	}
+	// oldPod is not intialized.
+	oldPod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "init"}}}
+	pod = &api.Pod{
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{},
+			},
+		},
+	}
+	attrs = admission.NewAttributesRecord(pod, oldPod, api.Kind("Pod").WithVersion("version"), ns, "myname", api.Resource("pods").WithVersion("version"), "", admission.Update, nil)
+	err = admit.Admit(attrs)
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if pod.Spec.ServiceAccountName != DefaultServiceAccountName {
+		t.Errorf("Expected service account %s assigned, got %s", DefaultServiceAccountName, pod.Spec.ServiceAccountName)
+	}
+	if len(pod.Spec.Volumes) != 1 {
+		t.Fatalf("Expected 1 volume, got %d", len(pod.Spec.Volumes))
+	}
+	if !reflect.DeepEqual(expectedVolume, pod.Spec.Volumes[0]) {
+		t.Fatalf("Expected\n\t%#v\ngot\n\t%#v", expectedVolume, pod.Spec.Volumes[0])
+	}
+	if len(pod.Spec.Containers[0].VolumeMounts) != 1 {
+		t.Fatalf("Expected 1 volume mount, got %d", len(pod.Spec.Containers[0].VolumeMounts))
+	}
+	if !reflect.DeepEqual(expectedVolumeMount, pod.Spec.Containers[0].VolumeMounts[0]) {
+		t.Fatalf("Expected\n\t%#v\ngot\n\t%#v", expectedVolumeMount, pod.Spec.Containers[0].VolumeMounts[0])
+	}
+
+	// testing InitContainers
 	pod = &api.Pod{
 		Spec: api.PodSpec{
 			InitContainers: []api.Container{