diff --git a/pkg/registry/core/pod/BUILD b/pkg/registry/core/pod/BUILD index 11059592410..af48252b278 100644 --- a/pkg/registry/core/pod/BUILD +++ b/pkg/registry/core/pod/BUILD @@ -19,7 +19,9 @@ go_library( "//pkg/api/validation:go_default_library", "//pkg/kubelet/client:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library", "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", @@ -27,9 +29,11 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library", + "//vendor/k8s.io/apiserver/pkg/features:go_default_library", "//vendor/k8s.io/apiserver/pkg/registry/generic:go_default_library", "//vendor/k8s.io/apiserver/pkg/storage:go_default_library", "//vendor/k8s.io/apiserver/pkg/storage/names:go_default_library", + "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/registry/core/pod/storage/BUILD b/pkg/registry/core/pod/storage/BUILD index eb4e1897e66..ea9feaea6d3 100644 --- a/pkg/registry/core/pod/storage/BUILD +++ b/pkg/registry/core/pod/storage/BUILD @@ -25,11 +25,14 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library", + "//vendor/k8s.io/apiserver/pkg/features:go_default_library", "//vendor/k8s.io/apiserver/pkg/registry/generic:go_default_library", "//vendor/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//vendor/k8s.io/apiserver/pkg/storage:go_default_library", "//vendor/k8s.io/apiserver/pkg/storage/errors:go_default_library", "//vendor/k8s.io/apiserver/pkg/storage/etcd/testing:go_default_library", + "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library", + "//vendor/k8s.io/apiserver/pkg/util/feature/testing:go_default_library", ], ) diff --git a/pkg/registry/core/pod/storage/storage_test.go b/pkg/registry/core/pod/storage/storage_test.go index 3e82177f24c..9f85678e35c 100644 --- a/pkg/registry/core/pod/storage/storage_test.go +++ b/pkg/registry/core/pod/storage/storage_test.go @@ -32,11 +32,14 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/diff" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" storeerr "k8s.io/apiserver/pkg/storage/errors" etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing" + utilfeature "k8s.io/apiserver/pkg/util/feature" + utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/registry/registrytest" "k8s.io/kubernetes/pkg/securitycontext" @@ -699,6 +702,76 @@ func TestEtcdCreateBinding(t *testing.T) { } } +func TestEtcdUpdateUninitialized(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.Initializers, true)() + storage, _, _, server := newStorage(t) + defer server.Terminate(t) + defer storage.Store.DestroyFunc() + ctx := genericapirequest.NewDefaultContext() + + pod := validNewPod() + // add pending initializers to the pod + pod.ObjectMeta.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "init.k8s.io"}}} + if _, err := storage.Create(ctx, pod, true); err != nil { + t.Fatalf("unexpected error: %v", err) + } + podIn := *pod + // only uninitialized pod is allowed to add containers via update + podIn.Spec.Containers = append(podIn.Spec.Containers, api.Container{ + Name: "foo2", + Image: "test", + ImagePullPolicy: api.PullAlways, + TerminationMessagePath: api.TerminationMessagePathDefault, + TerminationMessagePolicy: api.TerminationMessageReadFile, + SecurityContext: securitycontext.ValidInternalSecurityContextWithContainerDefaults(), + }) + podIn.ObjectMeta.Initializers = nil + + _, _, err := storage.Update(ctx, podIn.Name, rest.DefaultUpdatedObjectInfo(&podIn, api.Scheme)) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + obj, err := storage.Get(ctx, podIn.ObjectMeta.Name, &metav1.GetOptions{}) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + podOut := obj.(*api.Pod) + if podOut.GetInitializers() != nil { + t.Errorf("expect nil initializers, got %v", podOut.ObjectMeta.Initializers) + } + if !apiequality.Semantic.DeepEqual(podIn.Spec.Containers, podOut.Spec.Containers) { + t.Errorf("objects differ: %v", diff.ObjectDiff(podOut, &podIn)) + } +} + +func TestEtcdStatusUpdateUninitialized(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.Initializers, true)() + storage, _, statusStorage, server := newStorage(t) + defer server.Terminate(t) + defer storage.Store.DestroyFunc() + ctx := genericapirequest.NewDefaultContext() + + pod := validNewPod() + // add pending initializers to the pod + pod.ObjectMeta.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "init.k8s.io"}}} + if _, err := storage.Create(ctx, pod, true); err != nil { + t.Fatalf("unexpected error: %v", err) + } + podIn := *pod + // only uninitialized pod is allowed to add containers via update + podIn.Status.Phase = api.PodRunning + podIn.ObjectMeta.Initializers = nil + + _, _, err := statusStorage.Update(ctx, podIn.Name, rest.DefaultUpdatedObjectInfo(&podIn, api.Scheme)) + expected := "Forbidden: must not update status when the object is uninitialized" + if err == nil { + t.Fatalf("Unexpected no err, expected %q", expected) + } + if !strings.Contains(err.Error(), expected) { + t.Errorf("unexpected error: %v, expected %q", err, expected) + } +} + func TestEtcdUpdateNotScheduled(t *testing.T) { storage, _, _, server := newStorage(t) defer server.Terminate(t) diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index a3d19438c76..bb6e0afb10f 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -26,7 +26,9 @@ import ( "time" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apimachineryvalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -34,9 +36,11 @@ import ( utilnet "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/helper/qos" podutil "k8s.io/kubernetes/pkg/api/pod" @@ -95,9 +99,31 @@ func (podStrategy) AllowCreateOnUpdate() bool { return false } +func isUpdatingUninitializedPod(old runtime.Object) (bool, error) { + if !utilfeature.DefaultFeatureGate.Enabled(features.Initializers) { + return false, nil + } + oldMeta, err := meta.Accessor(old) + if err != nil { + return false, err + } + oldInitializers := oldMeta.GetInitializers() + if oldInitializers != nil && len(oldInitializers.Pending) != 0 { + return true, nil + } + return false, nil +} + // ValidateUpdate is the default update validation for an end user. func (podStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList { errorList := validation.ValidatePod(obj.(*api.Pod)) + uninitializedUpdate, err := isUpdatingUninitializedPod(old) + if err != nil { + return append(errorList, field.InternalError(field.NewPath("metadata"), err)) + } + if uninitializedUpdate { + return errorList + } return append(errorList, validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod))...) } @@ -166,6 +192,14 @@ func (podStatusStrategy) PrepareForUpdate(ctx genericapirequest.Context, obj, ol } func (podStatusStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList { + var errorList field.ErrorList + uninitializedUpdate, err := isUpdatingUninitializedPod(old) + if err != nil { + return append(errorList, field.InternalError(field.NewPath("metadata"), err)) + } + if uninitializedUpdate { + return append(errorList, field.Forbidden(field.NewPath("status"), apimachineryvalidation.UninitializedStatusUpdateErrorMsg)) + } // TODO: merge valid fields after update return validation.ValidatePodStatusUpdate(obj.(*api.Pod), old.(*api.Pod)) } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go index f06f075bf77..26c5a0cdca3 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go @@ -88,3 +88,5 @@ func ValidateDeleteOptions(options *metav1.DeleteOptions) field.ErrorList { } return allErrs } + +const UninitializedStatusUpdateErrorMsg string = `must not update status when the object is uninitialized` diff --git a/test/e2e/BUILD b/test/e2e/BUILD index 89363a386d9..426e4957907 100644 --- a/test/e2e/BUILD +++ b/test/e2e/BUILD @@ -75,6 +75,7 @@ go_library( "//vendor/github.com/onsi/gomega:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/api/rbac/v1beta1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library", diff --git a/test/e2e/resource_quota.go b/test/e2e/resource_quota.go index e70f8fb65b9..0e87c16758c 100644 --- a/test/e2e/resource_quota.go +++ b/test/e2e/resource_quota.go @@ -21,6 +21,7 @@ import ( "time" "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -147,72 +148,72 @@ var _ = framework.KubeDescribe("ResourceQuota", func() { Expect(err).NotTo(HaveOccurred()) }) - It("should create a ResourceQuota and capture the life of an uninitialized pod.", func() { - // TODO: uncomment the test when #50344 is merged. - // By("Creating a ResourceQuota") - // quotaName := "test-quota" - // resourceQuota := newTestResourceQuota(quotaName) - // resourceQuota, err := createResourceQuota(f.ClientSet, f.Namespace.Name, resourceQuota) - // Expect(err).NotTo(HaveOccurred()) + It("[Feature:Initializers] should create a ResourceQuota and capture the life of an uninitialized pod.", func() { + By("Creating a ResourceQuota") + quotaName := "test-quota" + resourceQuota := newTestResourceQuota(quotaName) + resourceQuota, err := createResourceQuota(f.ClientSet, f.Namespace.Name, resourceQuota) + Expect(err).NotTo(HaveOccurred()) - // By("Ensuring resource quota status is calculated") - // usedResources := v1.ResourceList{} - // usedResources[v1.ResourceQuotas] = resource.MustParse("1") - // err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) - // Expect(err).NotTo(HaveOccurred()) + By("Ensuring resource quota status is calculated") + usedResources := v1.ResourceList{} + usedResources[v1.ResourceQuotas] = resource.MustParse("1") + err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) + Expect(err).NotTo(HaveOccurred()) - // By("Creating an uninitialized Pod that fits quota") - // podName := "test-pod" - // requests := v1.ResourceList{} - // requests[v1.ResourceCPU] = resource.MustParse("500m") - // requests[v1.ResourceMemory] = resource.MustParse("252Mi") - // pod := newTestPodForQuota(f, podName, requests, v1.ResourceList{}) - // pod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "unhandled"}}} - // _, err = f.ClientSet.Core().Pods(f.Namespace.Name).Create(pod) - // // because no one is handling the initializer, server will return a 504 timeout - // if err != nil && !errors.IsTimeout(err) { - // framework.Failf("expect err to be timeout error, got %v", err) - // } - // podToUpdate, err := f.ClientSet.Core().Pods(f.Namespace.Name).Get(podName, metav1.GetOptions{}) - // Expect(err).NotTo(HaveOccurred()) + By("Creating an uninitialized Pod that fits quota") + podName := "test-pod" + requests := v1.ResourceList{} + requests[v1.ResourceCPU] = resource.MustParse("500m") + requests[v1.ResourceMemory] = resource.MustParse("252Mi") + pod := newTestPodForQuota(f, podName, requests, v1.ResourceList{}) + pod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "unhandled"}}} + _, err = f.ClientSet.Core().Pods(f.Namespace.Name).Create(pod) + // because no one is handling the initializer, server will return a 504 timeout + if err != nil && !errors.IsTimeout(err) { + framework.Failf("expect err to be timeout error, got %v", err) + } + podToUpdate, err := f.ClientSet.Core().Pods(f.Namespace.Name).Get(podName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) - // By("Ensuring ResourceQuota status captures the pod usage") - // usedResources[v1.ResourceQuotas] = resource.MustParse("1") - // usedResources[v1.ResourcePods] = resource.MustParse("1") - // usedResources[v1.ResourceCPU] = requests[v1.ResourceCPU] - // usedResources[v1.ResourceMemory] = requests[v1.ResourceMemory] - // err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) - // Expect(err).NotTo(HaveOccurred()) + By("Ensuring ResourceQuota status captures the pod usage") + usedResources[v1.ResourceQuotas] = resource.MustParse("1") + usedResources[v1.ResourcePods] = resource.MustParse("1") + usedResources[v1.ResourceCPU] = requests[v1.ResourceCPU] + usedResources[v1.ResourceMemory] = requests[v1.ResourceMemory] + err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) + Expect(err).NotTo(HaveOccurred()) - // By("Not allowing an uninitialized pod to be created that exceeds remaining quota") - // requests = v1.ResourceList{} - // requests[v1.ResourceCPU] = resource.MustParse("600m") - // requests[v1.ResourceMemory] = resource.MustParse("100Mi") - // pod = newTestPodForQuota(f, "fail-pod", requests, v1.ResourceList{}) - // pod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "unhandled"}}} - // pod, err = f.ClientSet.Core().Pods(f.Namespace.Name).Create(pod) - // Expect(err).To(HaveOccurred()) - // fmt.Println("CHAO: err=", err) + By("Not allowing an uninitialized pod to be created that exceeds remaining quota") + requests = v1.ResourceList{} + requests[v1.ResourceCPU] = resource.MustParse("600m") + requests[v1.ResourceMemory] = resource.MustParse("100Mi") + pod = newTestPodForQuota(f, "fail-pod", requests, v1.ResourceList{}) + pod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "unhandled"}}} + pod, err = f.ClientSet.Core().Pods(f.Namespace.Name).Create(pod) + Expect(err).To(HaveOccurred()) + fmt.Println("CHAO: err=", err) - // By("Ensuring an uninitialized pod can update its resource requirements") - // // a pod cannot dynamically update its resource requirements. - // requests = v1.ResourceList{} - // requests[v1.ResourceCPU] = resource.MustParse("100m") - // requests[v1.ResourceMemory] = resource.MustParse("100Mi") - // podToUpdate.Spec.Containers[0].Resources.Requests = requests - // _, err = f.ClientSet.Core().Pods(f.Namespace.Name).Update(podToUpdate) - // Expect(err).NotTo(HaveOccurred()) + By("Ensuring an uninitialized pod can update its resource requirements") + // a pod cannot dynamically update its resource requirements. + requests = v1.ResourceList{} + requests[v1.ResourceCPU] = resource.MustParse("100m") + requests[v1.ResourceMemory] = resource.MustParse("100Mi") + podToUpdate.Spec.Containers[0].Resources.Requests = requests + _, err = f.ClientSet.Core().Pods(f.Namespace.Name).Update(podToUpdate) + Expect(err).NotTo(HaveOccurred()) - // By("Ensuring attempts to update pod resource requirements did change quota usage") - // usedResources[v1.ResourceQuotas] = resource.MustParse("1") - // usedResources[v1.ResourcePods] = resource.MustParse("1") - // usedResources[v1.ResourceCPU] = requests[v1.ResourceCPU] - // usedResources[v1.ResourceMemory] = requests[v1.ResourceMemory] - // err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) - // Expect(err).NotTo(HaveOccurred()) + By("Ensuring attempts to update pod resource requirements did change quota usage") + usedResources[v1.ResourceQuotas] = resource.MustParse("1") + usedResources[v1.ResourcePods] = resource.MustParse("1") + usedResources[v1.ResourceCPU] = requests[v1.ResourceCPU] + usedResources[v1.ResourceMemory] = requests[v1.ResourceMemory] + err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) + Expect(err).NotTo(HaveOccurred()) - // TODO: uncomment the test when the replenishment_controller uses the - // sharedInformer that list/watches uninitialized objects. + // TODO: uncomment the test after 51247 is merged, in which the + // replenishment_controller uses the sharedInformer that list/watches + // uninitialized objects. // By("Deleting the pod") // err = f.ClientSet.Core().Pods(f.Namespace.Name).Delete(podName, metav1.NewDeleteOptions(0)) // Expect(err).NotTo(HaveOccurred())