Content-Length: 19524 | pFad | http://github.com/kubernetes/kubernetes/pull/98866.diff
thub.com diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index c2e05ed61f0a7..f29e04e58dad2 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3973,6 +3973,7 @@ func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel // 1. spec.containers[*].image // 2. spec.initContainers[*].image // 3. spec.activeDeadlineSeconds + // 4. spec.terminationGracePeriodSeconds containerErrs, stop := ValidateContainerUpdates(newPod.Spec.Containers, oldPod.Spec.Containers, specPath.Child("containers")) allErrs = append(allErrs, containerErrs...) @@ -4039,11 +4040,17 @@ func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel // tolerations are checked before the deep copy, so munge those too mungedPodSpec.Tolerations = oldPod.Spec.Tolerations // +k8s:verify-mutation:reason=clone + // Relax validation of immutable fields to allow it to be set to 1 if it was previously negative. + if oldPod.Spec.TerminationGracePeriodSeconds != nil && *oldPod.Spec.TerminationGracePeriodSeconds < 0 && + mungedPodSpec.TerminationGracePeriodSeconds != nil && *mungedPodSpec.TerminationGracePeriodSeconds == 1 { + mungedPodSpec.TerminationGracePeriodSeconds = oldPod.Spec.TerminationGracePeriodSeconds // +k8s:verify-mutation:reason=clone + } + if !apiequality.Semantic.DeepEqual(mungedPodSpec, oldPod.Spec) { // This diff isn't perfect, but it's a helluva lot better an "I'm not going to tell you what the difference is". //TODO: Pinpoint the specific field that causes the invalid error after we have strategic merge diff specDiff := diff.ObjectDiff(mungedPodSpec, oldPod.Spec) - allErrs = append(allErrs, field.Forbidden(specPath, fmt.Sprintf("pod updates may not change fields other than `spec.containers[*].image`, `spec.initContainers[*].image`, `spec.activeDeadlineSeconds` or `spec.tolerations` (only additions to existing tolerations)\n%v", specDiff))) + allErrs = append(allErrs, field.Forbidden(specPath, fmt.Sprintf("pod updates may not change fields other than `spec.containers[*].image`, `spec.initContainers[*].image`, `spec.activeDeadlineSeconds`, `spec.tolerations` (only additions to existing tolerations) or `spec.terminationGracePeriodSeconds` (allow it to be set to 1 if it was previously negative)\n%v", specDiff))) } return allErrs diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d444b68b56463..6f1658873914a 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -9803,6 +9803,46 @@ func TestValidatePodUpdate(t *testing.T) { "spec: Forbidden: pod updates", "removed priority class name", }, + { + core.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: core.PodSpec{ + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(1), + }, + }, + core.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: core.PodSpec{ + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1), + }, + }, + "", + "update termination grace period seconds", + }, + { + core.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: core.PodSpec{ + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(0), + }, + }, + core.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: core.PodSpec{ + TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1), + }, + }, + "spec: Forbidden: pod updates", + "update termination grace period seconds not 1", + }, } for _, test := range tests { diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 89f0771239491..4f2904c840d20 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -167,6 +167,11 @@ func (podStrategy) CheckGracefulDelete(ctx context.Context, obj runtime.Object, if pod.Status.Phase == api.PodFailed || pod.Status.Phase == api.PodSucceeded { period = 0 } + + if period < 0 { + period = 1 + } + // ensure the options and the pod are in sync options.GracePeriodSeconds = &period return true diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index cd3b1bbe55e72..841d574a53c5f 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -42,6 +42,7 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/client" + utilpointer "k8s.io/utils/pointer" // ensure types are installed _ "k8s.io/kubernetes/pkg/apis/core/install" @@ -266,55 +267,82 @@ func TestGetPodQOS(t *testing.T) { func TestCheckGracefulDelete(t *testing.T) { defaultGracePeriod := int64(30) tcs := []struct { - in *api.Pod - gracePeriod int64 + name string + pod *api.Pod + deleteGracePeriod *int64 + gracePeriod int64 }{ { - in: &api.Pod{ + name: "in pending phase with has node name", + pod: &api.Pod{ Spec: api.PodSpec{NodeName: "something"}, Status: api.PodStatus{Phase: api.PodPending}, }, - - gracePeriod: defaultGracePeriod, + deleteGracePeriod: &defaultGracePeriod, + gracePeriod: defaultGracePeriod, }, { - in: &api.Pod{ + name: "in failed phase with has node name", + pod: &api.Pod{ Spec: api.PodSpec{NodeName: "something"}, Status: api.PodStatus{Phase: api.PodFailed}, }, - gracePeriod: 0, + deleteGracePeriod: &defaultGracePeriod, + gracePeriod: 0, }, { - in: &api.Pod{ + name: "in failed phase", + pod: &api.Pod{ Spec: api.PodSpec{}, Status: api.PodStatus{Phase: api.PodPending}, }, - gracePeriod: 0, + deleteGracePeriod: &defaultGracePeriod, + gracePeriod: 0, }, { - in: &api.Pod{ + name: "in succeeded phase", + pod: &api.Pod{ Spec: api.PodSpec{}, Status: api.PodStatus{Phase: api.PodSucceeded}, }, - gracePeriod: 0, + deleteGracePeriod: &defaultGracePeriod, + gracePeriod: 0, }, { - in: &api.Pod{ + name: "no phase", + pod: &api.Pod{ Spec: api.PodSpec{}, Status: api.PodStatus{}, }, - gracePeriod: 0, + deleteGracePeriod: &defaultGracePeriod, + gracePeriod: 0, + }, + { + name: "has negative grace period", + pod: &api.Pod{ + Spec: api.PodSpec{ + NodeName: "something", + TerminationGracePeriodSeconds: utilpointer.Int64(-1), + }, + Status: api.PodStatus{}, + }, + gracePeriod: 1, }, } for _, tc := range tcs { - out := &metav1.DeleteOptions{GracePeriodSeconds: &defaultGracePeriod} - Strategy.CheckGracefulDelete(genericapirequest.NewContext(), tc.in, out) - if out.GracePeriodSeconds == nil { - t.Errorf("out grace period was nil but supposed to be %v", tc.gracePeriod) - } - if *(out.GracePeriodSeconds) != tc.gracePeriod { - t.Errorf("out grace period was %v but was expected to be %v", *out, tc.gracePeriod) - } + t.Run(tc.name, func(t *testing.T) { + out := &metav1.DeleteOptions{} + if tc.deleteGracePeriod != nil { + out.GracePeriodSeconds = utilpointer.Int64(*tc.deleteGracePeriod) + } + Strategy.CheckGracefulDelete(genericapirequest.NewContext(), tc.pod, out) + if out.GracePeriodSeconds == nil { + t.Errorf("out grace period was nil but supposed to be %v", tc.gracePeriod) + } + if *(out.GracePeriodSeconds) != tc.gracePeriod { + t.Errorf("out grace period was %v but was expected to be %v", *out, tc.gracePeriod) + } + }) } } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go index 3e7ca85b761ab..dcb78e4e22070 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" + utilpointer "k8s.io/utils/pointer" ) // RESTDeleteStrategy defines deletion behavior on an object that follows Kubernetes @@ -86,6 +87,15 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime. return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the ResourceVersion in the precondition (%s) does not match the ResourceVersion in record (%s). The object might have been modified", *options.Preconditions.ResourceVersion, objectMeta.GetResourceVersion())) } } + + // Negative values will be treated as the value `1s` on the delete path. + if gracePeriodSeconds := options.GracePeriodSeconds; gracePeriodSeconds != nil && *gracePeriodSeconds < 0 { + options.GracePeriodSeconds = utilpointer.Int64(1) + } + if deletionGracePeriodSeconds := objectMeta.GetDeletionGracePeriodSeconds(); deletionGracePeriodSeconds != nil && *deletionGracePeriodSeconds < 0 { + objectMeta.SetDeletionGracePeriodSeconds(utilpointer.Int64(1)) + } + gracefulStrategy, ok := strategy.(RESTGracefulDeleteStrategy) if !ok { // If we're not deleting gracefully there's no point in updating Generation, as we won't update diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete_test.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete_test.go new file mode 100644 index 0000000000000..622a1765f5414 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete_test.go @@ -0,0 +1,305 @@ +/* +Copyright 2021 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 rest + +import ( + "context" + "testing" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + utilpointer "k8s.io/utils/pointer" +) + +type mockStrategy struct { + RESTDeleteStrategy + RESTGracefulDeleteStrategy +} + +func (m mockStrategy) ObjectKinds(obj runtime.Object) ([]schema.GroupVersionKind, bool, error) { + gvk := obj.GetObjectKind().GroupVersionKind() + if len(gvk.Kind) == 0 { + return nil, false, runtime.NewMissingKindErr("object has no kind field ") + } + if len(gvk.Version) == 0 { + return nil, false, runtime.NewMissingVersionErr("object has no apiVersion field") + } + return []schema.GroupVersionKind{obj.GetObjectKind().GroupVersionKind()}, false, nil +} + +func TestBeforeDelete(t *testing.T) { + type args struct { + strategy RESTDeleteStrategy + ctx context.Context + pod *v1.Pod + options *metav1.DeleteOptions + } + + makePod := func(deletionGracePeriodSeconds int64) *v1.Pod { + return &v1.Pod{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Pod"}, + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &metav1.Time{}, + DeletionGracePeriodSeconds: &deletionGracePeriodSeconds, + }, + } + } + makeOption := func(gracePeriodSeconds int64) *metav1.DeleteOptions { + return &metav1.DeleteOptions{ + GracePeriodSeconds: &gracePeriodSeconds, + } + } + + tests := []struct { + name string + args args + wantGraceful bool + wantGracefulPending bool + wantGracePeriodSeconds *int64 + wantDeletionGracePeriodSeconds *int64 + wantErr bool + }{ + { + name: "when DeletionGracePeriodSeconds=-1, GracePeriodSeconds=-1", + args: args{ + pod: makePod(-1), + options: makeOption(-1), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: false, + wantGracefulPending: true, + }, + { + name: "when DeletionGracePeriodSeconds=-1, GracePeriodSeconds=0", + args: args{ + pod: makePod(-1), + options: makeOption(0), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(0), + wantGraceful: true, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=-1, GracePeriodSeconds=1", + args: args{ + pod: makePod(-1), + options: makeOption(1), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: false, + wantGracefulPending: true, + }, + { + name: "when DeletionGracePeriodSeconds=-1, GracePeriodSeconds=2", + args: args{ + pod: makePod(-1), + options: makeOption(2), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(2), + wantGraceful: false, + wantGracefulPending: true, + }, + + { + name: "when DeletionGracePeriodSeconds=0, GracePeriodSeconds=-1", + args: args{ + pod: makePod(0), + options: makeOption(-1), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: false, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=0, GracePeriodSeconds=0", + args: args{ + pod: makePod(0), + options: makeOption(0), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(0), + wantGraceful: false, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=0, GracePeriodSeconds=1", + args: args{ + pod: makePod(0), + options: makeOption(1), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: false, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=0, GracePeriodSeconds=2", + args: args{ + pod: makePod(0), + options: makeOption(2), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(2), + wantGraceful: false, + wantGracefulPending: false, + }, + + { + name: "when DeletionGracePeriodSeconds=1, GracePeriodSeconds=-1", + args: args{ + pod: makePod(1), + options: makeOption(-1), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: false, + wantGracefulPending: true, + }, + { + name: "when DeletionGracePeriodSeconds=1, GracePeriodSeconds=0", + args: args{ + pod: makePod(1), + options: makeOption(0), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(0), + wantGraceful: true, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=1, GracePeriodSeconds=1", + args: args{ + pod: makePod(1), + options: makeOption(1), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: false, + wantGracefulPending: true, + }, + { + name: "when DeletionGracePeriodSeconds=1, GracePeriodSeconds=2", + args: args{ + pod: makePod(1), + options: makeOption(2), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(2), + wantGraceful: false, + wantGracefulPending: true, + }, + + { + name: "when DeletionGracePeriodSeconds=2, GracePeriodSeconds=-1", + args: args{ + pod: makePod(2), + options: makeOption(-1), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: true, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=2, GracePeriodSeconds=0", + args: args{ + pod: makePod(2), + options: makeOption(0), + }, + // want 0 + wantDeletionGracePeriodSeconds: utilpointer.Int64(0), + wantGracePeriodSeconds: utilpointer.Int64(0), + wantGraceful: true, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=2, GracePeriodSeconds=1", + args: args{ + pod: makePod(2), + options: makeOption(1), + }, + // want 1 + wantDeletionGracePeriodSeconds: utilpointer.Int64(1), + wantGracePeriodSeconds: utilpointer.Int64(1), + wantGraceful: true, + wantGracefulPending: false, + }, + { + name: "when DeletionGracePeriodSeconds=2, GracePeriodSeconds=2", + args: args{ + pod: makePod(2), + options: makeOption(2), + }, + // want 2 + wantDeletionGracePeriodSeconds: utilpointer.Int64(2), + wantGracePeriodSeconds: utilpointer.Int64(2), + wantGraceful: false, + wantGracefulPending: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.args.strategy == nil { + tt.args.strategy = mockStrategy{} + } + if tt.args.ctx == nil { + tt.args.ctx = context.Background() + } + + gotGraceful, gotGracefulPending, err := BeforeDelete(tt.args.strategy, tt.args.ctx, tt.args.pod, tt.args.options) + if (err != nil) != tt.wantErr { + t.Errorf("BeforeDelete() error = %v, wantErr %v", err, tt.wantErr) + return + } + if gotGraceful != tt.wantGraceful { + t.Errorf("BeforeDelete() gotGraceful = %v, want %v", gotGraceful, tt.wantGraceful) + } + if gotGracefulPending != tt.wantGracefulPending { + t.Errorf("BeforeDelete() gotGracefulPending = %v, want %v", gotGracefulPending, tt.wantGracefulPending) + } + if gotGracefulPending != tt.wantGracefulPending { + t.Errorf("BeforeDelete() gotGracefulPending = %v, want %v", gotGracefulPending, tt.wantGracefulPending) + } + if !utilpointer.Int64Equal(tt.args.pod.DeletionGracePeriodSeconds, tt.wantDeletionGracePeriodSeconds) { + t.Errorf("metadata.DeletionGracePeriodSeconds = %v, want %v", utilpointer.Int64Deref(tt.args.pod.DeletionGracePeriodSeconds, 0), utilpointer.Int64Deref(tt.wantDeletionGracePeriodSeconds, 0)) + } + if !utilpointer.Int64Equal(tt.args.options.GracePeriodSeconds, tt.wantGracePeriodSeconds) { + t.Errorf("options.GracePeriodSeconds = %v, want %v", utilpointer.Int64Deref(tt.args.options.GracePeriodSeconds, 0), utilpointer.Int64Deref(tt.wantGracePeriodSeconds, 0)) + } + }) + } +}Fetched URL: http://github.com/kubernetes/kubernetes/pull/98866.diff
Alternative Proxies: