Merge pull request #76051 from zhouhaibing089/rm-orphan-by-default

namespace: remove gc finalizers based on delete options

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",

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.
+				// the current finalizers which are on namespace
-				if options.OrphanDependents != nil && *options.OrphanDependents == false {
+				currentFinalizers := map[string]bool{}
-					// remove Orphan finalizer.
+				for _, f := range existingNamespace.Finalizers {
-					newFinalizers := []string{}
+					currentFinalizers[f] = true
-					for i := range existingNamespace.ObjectMeta.Finalizers {
+				}
-						finalizer := existingNamespace.ObjectMeta.Finalizers[i]
+				// the finalizers we should ensure on namespace
-						if string(finalizer) != metav1.FinalizerOrphanDependents {
+				shouldHaveFinalizers := map[string]bool{
-							newFinalizers = append(newFinalizers, finalizer)
+					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)
 }

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)