Handle updates removing remaining finalizers on deleted objects

pkg/controller/namespace/deletion/namespaced_resources_deleter.go

@@ -129,6 +129,7 @@ func (d *namespacedResourcesDeleter) Delete(nsName string) error {
 
 	// Delete the namespace if it is already finalized.
 	if d.deleteNamespaceWhenDone && finalized(namespace) {
+		// TODO(liggitt): just return in 1.16, once n-1 apiservers automatically delete when finalizers are all removed
 		return d.deleteNamespace(namespace)
 	}
 
@@ -155,6 +156,7 @@ func (d *namespacedResourcesDeleter) Delete(nsName string) error {
 
 	// Check if we can delete now.
 	if d.deleteNamespaceWhenDone && finalized(namespace) {
+		// TODO(liggitt): just return in 1.16, once n-1 apiservers automatically delete when finalizers are all removed
 		return d.deleteNamespace(namespace)
 	}
 	return nil

pkg/registry/core/namespace/storage/BUILD

@@ -13,6 +13,7 @@ go_test(
     deps = [
         "//pkg/apis/core:go_default_library",
         "//pkg/registry/registrytest:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
@@ -40,6 +41,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/registry/generic:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/registry/generic/registry:go_default_library",

pkg/registry/core/namespace/storage/storage.go

@@ -33,6 +33,7 @@ import (
 	storageerr "k8s.io/apiserver/pkg/storage/errors"
 	"k8s.io/apiserver/pkg/util/dryrun"
 
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/printers"
 	printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
@@ -69,6 +70,8 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, *Finaliz
 		DeleteStrategy:      namespace.Strategy,
 		ReturnDeletedObject: true,
 
+		ShouldDeleteDuringUpdate: ShouldDeleteNamespaceDuringUpdate,
+
 		TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
 	}
 	options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: namespace.GetAttrs}
@@ -238,6 +241,16 @@ func (r *REST) Delete(ctx context.Context, name string, options *metav1.DeleteOp
 	return r.store.Delete(ctx, name, options)
 }
 
+// ShouldDeleteNamespaceDuringUpdate adds namespace-specific spec.finalizer checks on top of the default generic ShouldDeleteDuringUpdate behavior
+func ShouldDeleteNamespaceDuringUpdate(ctx context.Context, key string, obj, existing runtime.Object) bool {
+	ns, ok := obj.(*api.Namespace)
+	if !ok {
+		utilruntime.HandleError(fmt.Errorf("unexpected type %T", obj))
+		return false
+	}
+	return len(ns.Spec.Finalizers) == 0 && genericregistry.ShouldDeleteDuringUpdate(ctx, key, obj, existing)
+}
+
 func shouldHaveOrphanFinalizer(options *metav1.DeleteOptions, haveOrphanFinalizer bool) bool {
 	if options.OrphanDependents != nil {
 		return *options.OrphanDependents

pkg/registry/core/namespace/storage/storage_test.go

@@ -19,6 +19,7 @@ package storage
 import (
 	"testing"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
@@ -164,6 +165,194 @@ func TestDeleteNamespaceWithIncompleteFinalizers(t *testing.T) {
 	if _, _, err := storage.Delete(ctx, "foo", nil); err == nil {
 		t.Errorf("unexpected error: %v", err)
 	}
+	// should still exist
+	_, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+func TestUpdateDeletingNamespaceWithIncompleteMetadataFinalizers(t *testing.T) {
+	storage, server := newStorage(t)
+	defer server.Terminate(t)
+	defer storage.store.DestroyFunc()
+	key := "namespaces/foo"
+	ctx := genericapirequest.NewContext()
+	now := metav1.Now()
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "foo",
+			DeletionTimestamp: &now,
+			Finalizers:        []string{"example.com/foo"},
+		},
+		Spec: api.NamespaceSpec{
+			Finalizers: []api.FinalizerName{},
+		},
+		Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	if err := storage.store.Storage.Create(ctx, key, namespace, nil, 0, false); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if _, _, err = storage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(ns), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// should still exist
+	_, err = storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestUpdateDeletingNamespaceWithIncompleteSpecFinalizers(t *testing.T) {
+	storage, server := newStorage(t)
+	defer server.Terminate(t)
+	defer storage.store.DestroyFunc()
+	key := "namespaces/foo"
+	ctx := genericapirequest.NewContext()
+	now := metav1.Now()
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "foo",
+			DeletionTimestamp: &now,
+		},
+		Spec: api.NamespaceSpec{
+			Finalizers: []api.FinalizerName{api.FinalizerKubernetes},
+		},
+		Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	if err := storage.store.Storage.Create(ctx, key, namespace, nil, 0, false); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if _, _, err = storage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(ns), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// should still exist
+	_, err = storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestUpdateDeletingNamespaceWithCompleteFinalizers(t *testing.T) {
+	storage, server := newStorage(t)
+	defer server.Terminate(t)
+	defer storage.store.DestroyFunc()
+	key := "namespaces/foo"
+	ctx := genericapirequest.NewContext()
+	now := metav1.Now()
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "foo",
+			DeletionTimestamp: &now,
+			Finalizers:        []string{"example.com/foo"},
+		},
+		Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	if err := storage.store.Storage.Create(ctx, key, namespace, nil, 0, false); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns.(*api.Namespace).Finalizers = nil
+	if _, _, err = storage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(ns), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// should not exist
+	_, err = storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if !apierrors.IsNotFound(err) {
+		t.Errorf("expected NotFound, got %v", err)
+	}
+}
+
+func TestFinalizeDeletingNamespaceWithCompleteFinalizers(t *testing.T) {
+	// get finalize storage
+	etcdStorage, server := registrytest.NewEtcdStorage(t, "")
+	restOptions := generic.RESTOptions{StorageConfig: etcdStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: 1, ResourcePrefix: "namespaces"}
+	storage, _, finalizeStorage := NewREST(restOptions)
+
+	defer server.Terminate(t)
+	defer storage.store.DestroyFunc()
+	defer finalizeStorage.store.DestroyFunc()
+	key := "namespaces/foo"
+	ctx := genericapirequest.NewContext()
+	now := metav1.Now()
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "foo",
+			DeletionTimestamp: &now,
+		},
+		Spec: api.NamespaceSpec{
+			Finalizers: []api.FinalizerName{api.FinalizerKubernetes},
+		},
+		Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	if err := storage.store.Storage.Create(ctx, key, namespace, nil, 0, false); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns.(*api.Namespace).Spec.Finalizers = nil
+	if _, _, err = finalizeStorage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(ns), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// should not exist
+	_, err = storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if !apierrors.IsNotFound(err) {
+		t.Errorf("expected NotFound, got %v", err)
+	}
+}
+
+func TestFinalizeDeletingNamespaceWithIncompleteMetadataFinalizers(t *testing.T) {
+	// get finalize storage
+	etcdStorage, server := registrytest.NewEtcdStorage(t, "")
+	restOptions := generic.RESTOptions{StorageConfig: etcdStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: 1, ResourcePrefix: "namespaces"}
+	storage, _, finalizeStorage := NewREST(restOptions)
+
+	defer server.Terminate(t)
+	defer storage.store.DestroyFunc()
+	defer finalizeStorage.store.DestroyFunc()
+	key := "namespaces/foo"
+	ctx := genericapirequest.NewContext()
+	now := metav1.Now()
+	namespace := &api.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "foo",
+			DeletionTimestamp: &now,
+			Finalizers:        []string{"example.com/foo"},
+		},
+		Spec: api.NamespaceSpec{
+			Finalizers: []api.FinalizerName{api.FinalizerKubernetes},
+		},
+		Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	if err := storage.store.Storage.Create(ctx, key, namespace, nil, 0, false); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	ns.(*api.Namespace).Spec.Finalizers = nil
+	if _, _, err = finalizeStorage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(ns), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// should still exist
+	_, err = storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 }
 
 func TestDeleteNamespaceWithCompleteFinalizers(t *testing.T) {
@@ -189,6 +378,11 @@ func TestDeleteNamespaceWithCompleteFinalizers(t *testing.T) {
 	if _, _, err := storage.Delete(ctx, "foo", nil); err != nil {
 		t.Errorf("unexpected error: %v", err)
 	}
+	// should not exist
+	_, err := storage.Get(ctx, "foo", &metav1.GetOptions{})
+	if !apierrors.IsNotFound(err) {
+		t.Errorf("expected NotFound, got %v", err)
+	}
 }
 
 func TestDeleteWithGCFinalizers(t *testing.T) {

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go

@@ -156,7 +156,12 @@ func (c *CRDFinalizer) sync(key string) error {
 	}
 
 	// and now issue another delete, which should clean it all up if no finalizers remain or no-op if they do
-	return c.crdClient.CustomResourceDefinitions().Delete(crd.Name, nil)
+	// TODO(liggitt): just return in 1.16, once n-1 apiservers automatically delete when finalizers are all removed
+	err = c.crdClient.CustomResourceDefinitions().Delete(crd.Name, nil)
+	if apierrors.IsNotFound(err) {
+		return nil
+	}
+	return err
 }
 
 func (c *CRDFinalizer) deleteInstances(crd *apiextensions.CustomResourceDefinition) (apiextensions.CustomResourceDefinitionCondition, error) {

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

@@ -166,6 +166,11 @@ type Store struct {
 	// ReturnDeletedObject determines whether the Store returns the object
 	// that was deleted. Otherwise, return a generic success status response.
 	ReturnDeletedObject bool
+	// ShouldDeleteDuringUpdate is an optional function to determine whether
+	// an update from existing to obj should result in a delete.
+	// If specified, this is checked in addition to standard finalizer,
+	// deletionTimestamp, and deletionGracePeriodSeconds checks.
+	ShouldDeleteDuringUpdate func(ctx context.Context, key string, obj, existing runtime.Object) bool
 	// ExportStrategy implements resource-specific behavior during export,
 	// optional. Exported objects are not decorated.
 	ExportStrategy rest.RESTExportStrategy
@@ -388,10 +393,12 @@ func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation
 	return out, nil
 }
 
-// shouldDeleteDuringUpdate checks if a Update is removing all the object's
-// finalizers. If so, it further checks if the object's
-// DeletionGracePeriodSeconds is 0.
-func (e *Store) shouldDeleteDuringUpdate(ctx context.Context, key string, obj, existing runtime.Object) bool {
+// ShouldDeleteDuringUpdate is the default function for
+// checking if an object should be deleted during an update.
+// It checks if the new object has no finalizers,
+// the existing object's deletionTimestamp is set, and
+// the existing object's deletionGracePeriodSeconds is 0 or nil
+func ShouldDeleteDuringUpdate(ctx context.Context, key string, obj, existing runtime.Object) bool {
 	newMeta, err := meta.Accessor(obj)
 	if err != nil {
 		utilruntime.HandleError(err)
@@ -402,7 +409,16 @@ func (e *Store) shouldDeleteDuringUpdate(ctx context.Context, key string, obj, e
 		utilruntime.HandleError(err)
 		return false
 	}
-	return len(newMeta.GetFinalizers()) == 0 && oldMeta.GetDeletionGracePeriodSeconds() != nil && *oldMeta.GetDeletionGracePeriodSeconds() == 0
+	if len(newMeta.GetFinalizers()) > 0 {
+		// don't delete with finalizers remaining in the new object
+		return false
+	}
+	if oldMeta.GetDeletionTimestamp() == nil {
+		// don't delete if the existing object hasn't had a delete request made
+		return false
+	}
+	// delete if the existing object has no grace period or a grace period of 0
+	return oldMeta.GetDeletionGracePeriodSeconds() == nil || *oldMeta.GetDeletionGracePeriodSeconds() == 0
 }
 
 // deleteWithoutFinalizers handles deleting an object ignoring its finalizer list.
@@ -533,7 +549,9 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
 				return nil, nil, err
 			}
 		}
-		if e.shouldDeleteDuringUpdate(ctx, key, obj, existing) {
+		// Check the default delete-during-update conditions, and store-specific conditions if provided
+		if ShouldDeleteDuringUpdate(ctx, key, obj, existing) &&
+			(e.ShouldDeleteDuringUpdate == nil || e.ShouldDeleteDuringUpdate(ctx, key, obj, existing)) {
 			deleteObj = obj
 			return nil, nil, errEmptiedFinalizers
 		}

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -729,16 +729,6 @@ func testNamespaceDelete(c *testContext) {
 		c.t.Error(err)
 		return
 	}
-
-	// then run the final delete and make sure admission is called again
-	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, false, true)
-	err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background})
-	if err != nil {
-		c.t.Error(err)
-		return
-	}
-	c.admissionHolder.verify(c.t)
-
 	// verify namespace is gone
 	obj, err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Get(obj.GetName(), metav1.GetOptions{})
 	if err == nil || !errors.IsNotFound(err) {