Merge pull request #122646 from liggitt/deletionTimestamp

prevent deletionTimestamp from moving into the past

staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go

@@ -126,9 +126,22 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime.
 			if period >= *objectMeta.GetDeletionGracePeriodSeconds() {
 				return false, true, nil
 			}
+			// Move the existing deletionTimestamp back by existing object.DeletionGracePeriod, then forward by options.DeletionGracePeriod.
+			// This moves the deletionTimestamp back, since the grace period can only be shortened in this code path.
 			newDeletionTimestamp := metav1.NewTime(
 				objectMeta.GetDeletionTimestamp().Add(-time.Second * time.Duration(*objectMeta.GetDeletionGracePeriodSeconds())).
 					Add(time.Second * time.Duration(*options.GracePeriodSeconds)))
+			// Prevent shortening the grace period moving deletionTimestamp into the past
+			if now := metav1Now(); newDeletionTimestamp.Before(&now) {
+				newDeletionTimestamp = now
+				if period != 0 {
+					// Since a graceful deletion was requested (options.GracePeriodSeconds != 0), but the entire grace period has already expired,
+					// shorten to the minimum period possible while still treating this as a graceful delete.
+					// This means the API server updates the object, another actor observes the update
+					// and is still responsible for the final delete with options.GracePeriodSeconds == 0.
+					period = int64(1)
+				}
+			}
 			objectMeta.SetDeletionTimestamp(&newDeletionTimestamp)
 			objectMeta.SetDeletionGracePeriodSeconds(&period)
 			return true, false, nil
@@ -147,8 +160,8 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime.
 		return false, false, errors.NewInternalError(fmt.Errorf("options.GracePeriodSeconds should not be nil"))
 	}
 
-	now := metav1.NewTime(metav1.Now().Add(time.Second * time.Duration(*options.GracePeriodSeconds)))
-	objectMeta.SetDeletionTimestamp(&now)
+	requestedDeletionTimestamp := metav1.NewTime(metav1Now().Add(time.Second * time.Duration(*options.GracePeriodSeconds)))
+	objectMeta.SetDeletionTimestamp(&requestedDeletionTimestamp)
 	objectMeta.SetDeletionGracePeriodSeconds(options.GracePeriodSeconds)
 	// If it's the first graceful deletion we are going to set the DeletionTimestamp to non-nil.
 	// Controllers of the object that's being deleted shouldn't take any nontrivial actions, hence its behavior changes.

staging/src/k8s.io/apiserver/pkg/registry/rest/delete_test.go

@@ -18,7 +18,9 @@ package rest
 
 import (
 	"context"
+	"reflect"
 	"testing"
+	"time"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -52,15 +54,32 @@ func TestBeforeDelete(t *testing.T) {
 		options  *metav1.DeleteOptions
 	}
 
-	makePod := func(deletionGracePeriodSeconds int64) *v1.Pod {
+	// snapshot and restore real metav1Now function
+	originalMetav1Now := metav1Now
+	t.Cleanup(func() {
+		metav1Now = originalMetav1Now
+	})
+
+	// make now refer to a fixed point in time
+	now := metav1.Time{Time: time.Now().Truncate(time.Second)}
+	metav1Now = func() metav1.Time {
+		return now
+	}
+
+	makePodWithDeletionTimestamp := func(deletionTimestamp *metav1.Time, deletionGracePeriodSeconds int64) *v1.Pod {
 		return &v1.Pod{
 			TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Pod"},
 			ObjectMeta: metav1.ObjectMeta{
-				DeletionTimestamp:          &metav1.Time{},
+				DeletionTimestamp:          deletionTimestamp,
 				DeletionGracePeriodSeconds: &deletionGracePeriodSeconds,
 			},
 		}
 	}
+	makePod := func(deletionGracePeriodSeconds int64) *v1.Pod {
+		deletionTimestamp := now
+		deletionTimestamp.Time = deletionTimestamp.Time.Add(time.Duration(deletionGracePeriodSeconds) * time.Second)
+		return makePodWithDeletionTimestamp(&deletionTimestamp, deletionGracePeriodSeconds)
+	}
 	makeOption := func(gracePeriodSeconds int64) *metav1.DeleteOptions {
 		return &metav1.DeleteOptions{
 			GracePeriodSeconds: &gracePeriodSeconds,
@@ -74,6 +93,7 @@ func TestBeforeDelete(t *testing.T) {
 		wantGracefulPending            bool
 		wantGracePeriodSeconds         *int64
 		wantDeletionGracePeriodSeconds *int64
+		wantDeletionTimestamp          *metav1.Time
 		wantErr                        bool
 	}{
 		{
@@ -271,6 +291,28 @@ func TestBeforeDelete(t *testing.T) {
 			wantGraceful:                   false,
 			wantGracefulPending:            true,
 		},
+		{
+			name: "when a shorter non-zero grace period would move into the past",
+			args: args{
+				pod:     makePodWithDeletionTimestamp(&metav1.Time{Time: now.Time.Add(-time.Minute)}, 60),
+				options: makeOption(50),
+			},
+			wantDeletionTimestamp:          &now,
+			wantDeletionGracePeriodSeconds: utilpointer.Int64(1),
+			wantGracePeriodSeconds:         utilpointer.Int64(50),
+			wantGraceful:                   true,
+		},
+		{
+			name: "when a zero grace period would move into the past",
+			args: args{
+				pod:     makePodWithDeletionTimestamp(&metav1.Time{Time: now.Time.Add(-time.Minute)}, 60),
+				options: makeOption(0),
+			},
+			wantDeletionTimestamp:          &now,
+			wantDeletionGracePeriodSeconds: utilpointer.Int64(0),
+			wantGracePeriodSeconds:         utilpointer.Int64(0),
+			wantGraceful:                   true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -301,6 +343,11 @@ func TestBeforeDelete(t *testing.T) {
 			if !utilpointer.Int64Equal(tt.args.options.GracePeriodSeconds, tt.wantGracePeriodSeconds) {
 				t.Errorf("options.GracePeriodSeconds = %v, want %v", ptr.Deref(tt.args.options.GracePeriodSeconds, 0), ptr.Deref(tt.wantGracePeriodSeconds, 0))
 			}
+			if tt.wantDeletionTimestamp != nil {
+				if !reflect.DeepEqual(tt.args.pod.DeletionTimestamp, tt.wantDeletionTimestamp) {
+					t.Errorf("pod.deletionTimestamp = %v, want %v", tt.args.pod.DeletionTimestamp, tt.wantDeletionTimestamp)
+				}
+			}
 		})
 	}
 }

staging/src/k8s.io/apiserver/pkg/registry/rest/meta.go

@@ -23,6 +23,9 @@ import (
 	"k8s.io/apimachinery/pkg/util/uuid"
 )
 
+// metav1Now returns metav1.Now(), but allows override for unit testing
+var metav1Now = func() metav1.Time { return metav1.Now() }
+
 // WipeObjectMetaSystemFields erases fields that are managed by the system on ObjectMeta.
 func WipeObjectMetaSystemFields(meta metav1.Object) {
 	meta.SetCreationTimestamp(metav1.Time{})
@@ -34,7 +37,7 @@ func WipeObjectMetaSystemFields(meta metav1.Object) {
 
 // FillObjectMetaSystemFields populates fields that are managed by the system on ObjectMeta.
 func FillObjectMetaSystemFields(meta metav1.Object) {
-	meta.SetCreationTimestamp(metav1.Now())
+	meta.SetCreationTimestamp(metav1Now())
 	meta.SetUID(uuid.NewUUID())
 }