Merge pull request #115606 from wzshiming/fix/termination_grace_period_seconds

`pod.spec.terminationGracePeriodSeconds` is a negative then convert to 1

pkg/api/pod/warnings.go

@@ -250,5 +250,9 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta
 		return true
 	})
 
+	// warn if the terminationGracePeriodSeconds is negative.
+	if podSpec.TerminationGracePeriodSeconds != nil && *podSpec.TerminationGracePeriodSeconds < 0 {
+		warnings = append(warnings, fmt.Sprintf("%s: must be >= 0; negative values are invalid and will be treated as 1", fieldPath.Child("spec", "terminationGracePeriodSeconds")))
+	}
 	return warnings
 }

pkg/api/pod/warnings_test.go

@@ -24,6 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func BenchmarkNoWarnings(b *testing.B) {
@@ -489,6 +490,18 @@ func TestWarnings(t *testing.T) {
 				`spec.volumes[0].ephemeral.volumeClaimTemplate.spec.resources.requests[storage]: fractional byte value "200m" is invalid, must be an integer`,
 			},
 		},
+		{
+			name: "terminationGracePeriodSeconds is negative",
+			template: &api.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{},
+				Spec: api.PodSpec{
+					TerminationGracePeriodSeconds: utilpointer.Int64Ptr(-1),
+				},
+			},
+			expected: []string{
+				`spec.terminationGracePeriodSeconds: must be >= 0; negative values are invalid and will be treated as 1`,
+			},
+		},
 	}
 
 	for _, tc := range testcases {

pkg/apis/core/v1/conversion.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/apis/apps"
 	"k8s.io/kubernetes/pkg/apis/core"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func addConversionFuncs(scheme *runtime.Scheme) error {
@@ -372,6 +373,11 @@ func Convert_v1_Pod_To_core_Pod(in *v1.Pod, out *core.Pod, s conversion.Scope) e
 	// drop init container annotations so they don't show up as differences when receiving requests from old clients
 	out.Annotations = dropInitContainerAnnotations(out.Annotations)
 
+	// Forcing the value of TerminationGracePeriodSeconds to 1 if it is negative.
+	// Just for Pod, not for PodSpec, because we don't want to change the behavior of the PodTemplate.
+	if in.Spec.TerminationGracePeriodSeconds != nil && *in.Spec.TerminationGracePeriodSeconds < 0 {
+		out.Spec.TerminationGracePeriodSeconds = utilpointer.Int64(1)
+	}
 	return nil
 }
 
@@ -384,6 +390,11 @@ func Convert_core_Pod_To_v1_Pod(in *core.Pod, out *v1.Pod, s conversion.Scope) e
 	// remove this once the oldest supported kubelet no longer honors the annotations over the field.
 	out.Annotations = dropInitContainerAnnotations(out.Annotations)
 
+	// Forcing the value of TerminationGracePeriodSeconds to 1 if it is negative.
+	// Just for Pod, not for PodSpec, because we don't want to change the behavior of the PodTemplate.
+	if in.Spec.TerminationGracePeriodSeconds != nil && *in.Spec.TerminationGracePeriodSeconds < 0 {
+		out.Spec.TerminationGracePeriodSeconds = utilpointer.Int64(1)
+	}
 	return nil
 }
 

pkg/apis/core/v1/conversion_test.go

@@ -24,6 +24,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/apitesting/fuzzer"
@@ -704,3 +705,82 @@ func Test_v1_NodeSpec_to_core_NodeSpec(t *testing.T) {
 		}
 	}
 }
+
+func TestConvert_v1_Pod_To_core_Pod(t *testing.T) {
+	type args struct {
+		in  *v1.Pod
+		out *core.Pod
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+		wantOut *core.Pod
+	}{
+		{
+			args: args{
+				in: &v1.Pod{
+					Spec: v1.PodSpec{
+						TerminationGracePeriodSeconds: utilpointer.Int64(-1),
+					},
+				},
+				out: &core.Pod{},
+			},
+			wantOut: &core.Pod{
+				Spec: core.PodSpec{
+					TerminationGracePeriodSeconds: utilpointer.Int64(1),
+					SecurityContext:               &core.PodSecurityContext{},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := corev1.Convert_v1_Pod_To_core_Pod(tt.args.in, tt.args.out, nil); (err != nil) != tt.wantErr {
+				t.Errorf("Convert_v1_Pod_To_core_Pod() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if diff := cmp.Diff(tt.args.out, tt.wantOut); diff != "" {
+				t.Errorf("Convert_v1_Pod_To_core_Pod() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
+
+func TestConvert_core_Pod_To_v1_Pod(t *testing.T) {
+	type args struct {
+		in  *core.Pod
+		out *v1.Pod
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+		wantOut *v1.Pod
+	}{
+		{
+			args: args{
+				in: &core.Pod{
+					Spec: core.PodSpec{
+						TerminationGracePeriodSeconds: utilpointer.Int64(-1),
+					},
+				},
+				out: &v1.Pod{},
+			},
+			wantOut: &v1.Pod{
+				Spec: v1.PodSpec{
+					TerminationGracePeriodSeconds: utilpointer.Int64(1),
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := corev1.Convert_core_Pod_To_v1_Pod(tt.args.in, tt.args.out, nil); (err != nil) != tt.wantErr {
+				t.Errorf("Convert_core_Pod_To_v1_Pod() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if diff := cmp.Diff(tt.args.out, tt.wantOut); diff != "" {
+				t.Errorf("Convert_core_Pod_To_v1_Pod() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

test/e2e/node/pods.go

@@ -44,6 +44,7 @@ import (
 	e2epod "k8s.io/kubernetes/test/e2e/framework/pod"
 	imageutils "k8s.io/kubernetes/test/utils/image"
 	admissionapi "k8s.io/pod-security-admission/api"
+	utilpointer "k8s.io/utils/pointer"
 
 	"github.com/onsi/ginkgo/v2"
 	"github.com/prometheus/client_golang/prometheus"
@@ -341,6 +342,76 @@ var _ = SIGDescribe("Pods Extended", func() {
 		})
 	})
 
+	ginkgo.Describe("Pod TerminationGracePeriodSeconds is negative", func() {
+		var podClient *e2epod.PodClient
+		ginkgo.BeforeEach(func() {
+			podClient = e2epod.NewPodClient(f)
+		})
+
+		ginkgo.It("pod with negative grace period", func(ctx context.Context) {
+			name := "pod-negative-grace-period" + string(uuid.NewUUID())
+			image := imageutils.GetE2EImage(imageutils.BusyBox)
+			pod := &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: v1.PodSpec{
+					RestartPolicy: v1.RestartPolicyOnFailure,
+					Containers: []v1.Container{
+						{
+							Name:  "foo",
+							Image: image,
+							Command: []string{
+								"/bin/sh", "-c", "sleep 10000",
+							},
+						},
+					},
+					TerminationGracePeriodSeconds: utilpointer.Int64(-1),
+				},
+			}
+
+			ginkgo.By("submitting the pod to kubernetes")
+			podClient.Create(ctx, pod)
+
+			pod, err := podClient.Get(ctx, pod.Name, metav1.GetOptions{})
+			framework.ExpectNoError(err, "failed to query for pod")
+
+			if pod.Spec.TerminationGracePeriodSeconds == nil {
+				framework.Failf("pod spec TerminationGracePeriodSeconds is nil")
+			}
+
+			if *pod.Spec.TerminationGracePeriodSeconds != 1 {
+				framework.Failf("pod spec TerminationGracePeriodSeconds is not 1: %d", *pod.Spec.TerminationGracePeriodSeconds)
+			}
+
+			time.Sleep(5 * time.Second)
+
+			pod, err = podClient.Get(ctx, pod.Name, metav1.GetOptions{})
+			framework.ExpectNoError(err, "failed to query for pod")
+
+			ginkgo.By("updating the pod to have a negative TerminationGracePeriodSeconds")
+			pod.Spec.TerminationGracePeriodSeconds = utilpointer.Int64(-1)
+			_, err = podClient.PodInterface.Update(ctx, pod, metav1.UpdateOptions{})
+			framework.ExpectNoError(err, "failed to update pod")
+
+			pod, err = podClient.Get(ctx, pod.Name, metav1.GetOptions{})
+			framework.ExpectNoError(err, "failed to query for pod")
+
+			if pod.Spec.TerminationGracePeriodSeconds == nil {
+				framework.Failf("pod spec TerminationGracePeriodSeconds is nil")
+			}
+
+			if *pod.Spec.TerminationGracePeriodSeconds != 1 {
+				framework.Failf("pod spec TerminationGracePeriodSeconds is not 1: %d", *pod.Spec.TerminationGracePeriodSeconds)
+			}
+
+			ginkgo.DeferCleanup(func(ctx context.Context) error {
+				ginkgo.By("deleting the pod")
+				return podClient.Delete(ctx, pod.Name, metav1.DeleteOptions{})
+			})
+		})
+	})
+
 })
 
 func createAndTestPodRepeatedly(ctx context.Context, workers, iterations int, scenario podScenario, podClient v1core.PodInterface) {