Merge pull request #98866 from wzshiming/fix/termination_grace_period_seconds_is_negative

Fix TerminationGracePeriodSeconds is negative (part 1)

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

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 {

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

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
+		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},
 			},
-
+			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},
 			},
+			deleteGracePeriod: &defaultGracePeriod,
 			gracePeriod:       0,
 		},
 		{
-			in: &api.Pod{
+			name: "in failed phase",
+			pod: &api.Pod{
 				Spec:   api.PodSpec{},
 				Status: api.PodStatus{Phase: api.PodPending},
 			},
+			deleteGracePeriod: &defaultGracePeriod,
 			gracePeriod:       0,
 		},
 		{
-			in: &api.Pod{
+			name: "in succeeded phase",
+			pod: &api.Pod{
 				Spec:   api.PodSpec{},
 				Status: api.PodStatus{Phase: api.PodSucceeded},
 			},
+			deleteGracePeriod: &defaultGracePeriod,
 			gracePeriod:       0,
 		},
 		{
-			in: &api.Pod{
+			name: "no phase",
+			pod: &api.Pod{
 				Spec:   api.PodSpec{},
 				Status: api.PodStatus{},
 			},
+			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}
+		t.Run(tc.name, func(t *testing.T) {
-		Strategy.CheckGracefulDelete(genericapirequest.NewContext(), tc.in, out)
+			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)
 			}
+		})
 	}
 }
 

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

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))
+			}
+		})
+	}
+}