From fba885a0d2cb723a50c95ebc4562696d9f931f04 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 15 May 2019 15:04:38 -0400 Subject: [PATCH] Handle updates removing remaining finalizers on deleted objects --- .../deletion/namespaced_resources_deleter.go | 2 + pkg/registry/core/namespace/storage/BUILD | 2 + .../core/namespace/storage/storage.go | 13 ++ .../core/namespace/storage/storage_test.go | 194 ++++++++++++++++++ .../pkg/controller/finalizer/crd_finalizer.go | 7 +- .../pkg/registry/generic/registry/store.go | 30 ++- .../admissionwebhook/admission_test.go | 10 - 7 files changed, 241 insertions(+), 17 deletions(-) diff --git a/pkg/controller/namespace/deletion/namespaced_resources_deleter.go b/pkg/controller/namespace/deletion/namespaced_resources_deleter.go index 379a8219cb..21e3f6f814 100644 --- a/pkg/controller/namespace/deletion/namespaced_resources_deleter.go +++ b/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 diff --git a/pkg/registry/core/namespace/storage/BUILD b/pkg/registry/core/namespace/storage/BUILD index 881e518aa6..37e5ce3d10 100644 --- a/pkg/registry/core/namespace/storage/BUILD +++ b/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", diff --git a/pkg/registry/core/namespace/storage/storage.go b/pkg/registry/core/namespace/storage/storage.go index 0ad2ae4604..fecac7428c 100644 --- a/pkg/registry/core/namespace/storage/storage.go +++ b/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 diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index 340d752194..68191b936b 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/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) { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index 19f8fd6e13..04caac8290 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/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) { diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 1db4845def..7b8aad1449 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/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 } diff --git a/test/integration/apiserver/admissionwebhook/admission_test.go b/test/integration/apiserver/admissionwebhook/admission_test.go index 3672eb3874..4146053ea6 100644 --- a/test/integration/apiserver/admissionwebhook/admission_test.go +++ b/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) {