diff --git a/pkg/registry/core/namespace/storage/BUILD b/pkg/registry/core/namespace/storage/BUILD index 257a5a75982..881e518aa6f 100644 --- a/pkg/registry/core/namespace/storage/BUILD +++ b/pkg/registry/core/namespace/storage/BUILD @@ -16,6 +16,7 @@ go_test( "//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", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/generic:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/generic/testing:go_default_library", diff --git a/pkg/registry/core/namespace/storage/storage.go b/pkg/registry/core/namespace/storage/storage.go index 32ed5d776a0..0ad2ae46048 100644 --- a/pkg/registry/core/namespace/storage/storage.go +++ b/pkg/registry/core/namespace/storage/storage.go @@ -185,17 +185,33 @@ func (r *REST) Delete(ctx context.Context, name string, options *metav1.DeleteOp existingNamespace.Status.Phase = api.NamespaceTerminating } - // Remove orphan finalizer if options.OrphanDependents = false. - if options.OrphanDependents != nil && *options.OrphanDependents == false { - // remove Orphan finalizer. - newFinalizers := []string{} - for i := range existingNamespace.ObjectMeta.Finalizers { - finalizer := existingNamespace.ObjectMeta.Finalizers[i] - if string(finalizer) != metav1.FinalizerOrphanDependents { - newFinalizers = append(newFinalizers, finalizer) - } + // the current finalizers which are on namespace + currentFinalizers := map[string]bool{} + for _, f := range existingNamespace.Finalizers { + currentFinalizers[f] = true + } + // the finalizers we should ensure on namespace + shouldHaveFinalizers := map[string]bool{ + metav1.FinalizerOrphanDependents: shouldHaveOrphanFinalizer(options, currentFinalizers[metav1.FinalizerOrphanDependents]), + metav1.FinalizerDeleteDependents: shouldHaveDeleteDependentsFinalizer(options, currentFinalizers[metav1.FinalizerDeleteDependents]), + } + // determine whether there are changes + changeNeeded := false + for finalizer, shouldHave := range shouldHaveFinalizers { + changeNeeded = currentFinalizers[finalizer] != shouldHave || changeNeeded + if shouldHave { + currentFinalizers[finalizer] = true + } else { + delete(currentFinalizers, finalizer) } - existingNamespace.ObjectMeta.Finalizers = newFinalizers + } + // make the changes if needed + if changeNeeded { + newFinalizers := []string{} + for f := range currentFinalizers { + newFinalizers = append(newFinalizers, f) + } + existingNamespace.Finalizers = newFinalizers } return existingNamespace, nil }), @@ -222,6 +238,26 @@ func (r *REST) Delete(ctx context.Context, name string, options *metav1.DeleteOp return r.store.Delete(ctx, name, options) } +func shouldHaveOrphanFinalizer(options *metav1.DeleteOptions, haveOrphanFinalizer bool) bool { + if options.OrphanDependents != nil { + return *options.OrphanDependents + } + if options.PropagationPolicy != nil { + return *options.PropagationPolicy == metav1.DeletePropagationOrphan + } + return haveOrphanFinalizer +} + +func shouldHaveDeleteDependentsFinalizer(options *metav1.DeleteOptions, haveDeleteDependentsFinalizer bool) bool { + if options.OrphanDependents != nil { + return *options.OrphanDependents == false + } + if options.PropagationPolicy != nil { + return *options.PropagationPolicy == metav1.DeletePropagationForeground + } + return haveDeleteDependentsFinalizer +} + func (e *REST) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1beta1.Table, error) { return e.store.ConvertToTable(ctx, object, tableOptions) } diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index c1e93f2d1ca..340d7521945 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/pkg/registry/core/namespace/storage/storage_test.go @@ -22,11 +22,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" "k8s.io/apiserver/pkg/registry/rest" etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing" + api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/registry/registrytest" ) @@ -189,6 +191,217 @@ func TestDeleteNamespaceWithCompleteFinalizers(t *testing.T) { } } +func TestDeleteWithGCFinalizers(t *testing.T) { + storage, server := newStorage(t) + defer server.Terminate(t) + defer storage.store.DestroyFunc() + + propagationBackground := metav1.DeletePropagationBackground + propagationForeground := metav1.DeletePropagationForeground + propagationOrphan := metav1.DeletePropagationOrphan + trueVar := true + + var tests = []struct { + name string + deleteOptions *metav1.DeleteOptions + + existingFinalizers []string + remainingFinalizers map[string]bool + }{ + { + name: "nil-with-orphan", + deleteOptions: nil, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "nil-with-delete", + deleteOptions: nil, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "nil-without-finalizers", + deleteOptions: nil, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-background-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationBackground, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-background-with-delete", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationBackground, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-background-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationBackground, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-foreground-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationForeground, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "propagation-foreground-with-delete", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationForeground, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "propagation-foreground-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationForeground, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "propagation-orphan-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationOrphan, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "propagation-orphan-with-delete", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationOrphan, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "propagation-orphan-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationOrphan, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "orphan-dependents-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + OrphanDependents: &trueVar, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "orphan-dependents-with-delete", + deleteOptions: &metav1.DeleteOptions{ + OrphanDependents: &trueVar, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "orphan-dependents-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + OrphanDependents: &trueVar, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + } + + for _, test := range tests { + key := "namespaces/" + test.name + ctx := genericapirequest.NewContext() + namespace := &api.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: test.name, + Finalizers: test.existingFinalizers, + }, + 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) + } + var obj runtime.Object + var err error + if obj, _, err = storage.Delete(ctx, test.name, test.deleteOptions); err != nil { + t.Fatalf("unexpected error: %v", err) + } + ns, ok := obj.(*api.Namespace) + if !ok { + t.Errorf("unexpected object kind: %+v", obj) + } + if len(ns.Finalizers) != len(test.remainingFinalizers) { + t.Errorf("%s: unexpected remaining finalizers: %v", test.name, ns.Finalizers) + } + for _, f := range ns.Finalizers { + if test.remainingFinalizers[f] != true { + t.Errorf("%s: unexpected finalizer %s", test.name, f) + } + } + } +} + func TestShortNames(t *testing.T) { storage, server := newStorage(t) defer server.Terminate(t)