Handle deletion prevention correctly

If an object with the deletion prevention annotation is removed from the
inventory, the config.k8s.io/owning-inventory annotation should be
removed from the object, and the object should be removed from the
inventory.

pkg/apply/prune/prune.go

@@ -14,6 +14,7 @@ package prune
 import (
 	"context"
 	"sort"
+	"strings"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -103,19 +104,50 @@ func (po *PruneOptions) Prune(pruneObjs []*unstructured.Unstructured,
 		var filtered bool
 		var reason string
 		var err error
-		for _, filter := range pruneFilters {
-			klog.V(6).Infof("prune filter %s: %s", filter.Name(), pruneID)
-			filtered, reason, err = filter.Filter(pruneObj)
+		for _, pruneFilter := range pruneFilters {
+			klog.V(6).Infof("prune filter %s: %s", pruneFilter.Name(), pruneID)
+			filtered, reason, err = pruneFilter.Filter(pruneObj)
 			if err != nil {
 				if klog.V(5).Enabled() {
-					klog.Errorf("error during %s, (%s): %s", filter.Name(), pruneID, err)
+					klog.Errorf("error during %s, (%s): %s", pruneFilter.Name(), pruneID, err)
 				}
 				taskContext.EventChannel() <- eventFactory.CreateFailedEvent(pruneID, err)
 				taskContext.CapturePruneFailure(pruneID)
 				break
 			}
 			if filtered {
-				klog.V(4).Infof("prune filtered (filter: %q, resource: %q, reason: %q)", filter.Name(), pruneID, reason)
+				klog.V(4).Infof("prune filtered (filter: %q, resource: %q, reason: %q)", pruneFilter.Name(), pruneID, reason)
+				if !o.DryRunStrategy.ClientOrServerDryRun() && strings.HasPrefix(reason, "annotation prevents deletion (annotation:") {
+					klog.V(4).Infof("Remove the %q annotation from the object %s", inventory.OwningInventoryKey, pruneID)
+					// Remove the `config.k8s.io/owning-inventory` annotation from the object
+					annotations := pruneObj.GetAnnotations()
+					if annotations != nil {
+						if _, ok := annotations[inventory.OwningInventoryKey]; ok {
+							delete(annotations, inventory.OwningInventoryKey)
+							pruneObj.SetAnnotations(annotations)
+							namespacedClient, err := po.namespacedClient(pruneID)
+							if err != nil {
+								if klog.V(4).Enabled() {
+									klog.Errorf("Failed to remove the %q annotation from %s: %v", inventory.OwningInventoryKey, pruneID, err)
+								}
+								taskContext.EventChannel() <- eventFactory.CreateFailedEvent(pruneID, err)
+								taskContext.CapturePruneFailure(pruneID)
+								break
+							}
+							_, err = namespacedClient.Update(context.TODO(), pruneObj, metav1.UpdateOptions{})
+							if err != nil {
+								if klog.V(4).Enabled() {
+									klog.Errorf("Failed to remove the %q annotation from %s: %v", inventory.OwningInventoryKey, pruneID, err)
+								}
+								taskContext.EventChannel() <- eventFactory.CreateFailedEvent(pruneID, err)
+								taskContext.CapturePruneFailure(pruneID)
+								break
+							}
+							taskContext.EventChannel() <- eventFactory.CreateSkippedEvent(pruneObj, reason)
+							break
+						}
+					}
+				}
 				taskContext.EventChannel() <- eventFactory.CreateSkippedEvent(pruneObj, reason)
 				taskContext.CapturePruneFailure(pruneID)
 				break

pkg/apply/prune/prune_test.go

@@ -136,22 +136,35 @@ func createInventoryInfo(children ...*unstructured.Unstructured) inventory.Inven
 	return inventory.WrapInventoryInfoObj(obj)
 }
 
-// preventDelete object contains the "on-remove:keep" lifecycle directive.
-var preventDelete = &unstructured.Unstructured{
+// podDeletionPrevention object contains the "on-remove:keep" lifecycle directive.
+var podDeletionPrevention = &unstructured.Unstructured{
 	Object: map[string]interface{}{
 		"apiVersion": "v1",
 		"kind":       "Pod",
 		"metadata": map[string]interface{}{
 			"name":      "test-prevent-delete",
 			"namespace": testNamespace,
 			"annotations": map[string]interface{}{
-				common.OnRemoveAnnotation: common.OnRemoveKeep,
+				common.OnRemoveAnnotation:    common.OnRemoveKeep,
+				inventory.OwningInventoryKey: testInventoryLabel,
 			},
 			"uid": "prevent-delete",
 		},
 	},
 }
 
+var pdbDeletePreventionManifest = `
+apiVersion: "policy/v1beta1"
+kind: PodDisruptionBudget
+metadata:
+  name: pdb-delete-prevention
+  namespace: test-namespace
+  uid: uid2
+  annotations:
+    client.lifecycle.config.k8s.io/deletion: detach
+    config.k8s.io/owning-inventory: test-app-label
+`
+
 // Options with different dry-run values.
 var (
 	defaultOptions = Options{
@@ -324,7 +337,7 @@ func TestPrune(t *testing.T) {
 			},
 		},
 		"Prevent delete annotation equals prune skipped": {
-			pruneObjs:    []*unstructured.Unstructured{preventDelete},
+			pruneObjs:    []*unstructured.Unstructured{podDeletionPrevention, testutil.Unstructured(t, pdbDeletePreventionManifest)},
 			pruneFilters: []filter.ValidationFilter{filter.PreventRemoveFilter{}},
 			options:      defaultOptions,
 			expectedEvents: []testutil.ExpEvent{
@@ -334,10 +347,16 @@ func TestPrune(t *testing.T) {
 						Operation: event.PruneSkipped,
 					},
 				},
+				{
+					EventType: event.PruneType,
+					PruneEvent: &testutil.ExpPruneEvent{
+						Operation: event.PruneSkipped,
+					},
+				},
 			},
 		},
 		"Prevent delete annotation equals delete skipped": {
-			pruneObjs:    []*unstructured.Unstructured{preventDelete},
+			pruneObjs:    []*unstructured.Unstructured{podDeletionPrevention, testutil.Unstructured(t, pdbDeletePreventionManifest)},
 			pruneFilters: []filter.ValidationFilter{filter.PreventRemoveFilter{}},
 			options:      defaultOptionsDestroy,
 			expectedEvents: []testutil.ExpEvent{
@@ -347,10 +366,16 @@ func TestPrune(t *testing.T) {
 						Operation: event.DeleteSkipped,
 					},
 				},
+				{
+					EventType: event.DeleteType,
+					DeleteEvent: &testutil.ExpDeleteEvent{
+						Operation: event.DeleteSkipped,
+					},
+				},
 			},
 		},
 		"Prevent delete annotation, one skipped, one pruned": {
-			pruneObjs:    []*unstructured.Unstructured{preventDelete, pod},
+			pruneObjs:    []*unstructured.Unstructured{podDeletionPrevention, pod},
 			pruneFilters: []filter.ValidationFilter{filter.PreventRemoveFilter{}},
 			options:      defaultOptions,
 			expectedEvents: []testutil.ExpEvent{
@@ -428,6 +453,72 @@ func TestPrune(t *testing.T) {
 	}
 }
 
+func TestPruneDeletionPrevention(t *testing.T) {
+	tests := map[string]struct {
+		pruneObj *unstructured.Unstructured
+		options  Options
+	}{
+		"an object with the cli-utils.sigs.k8s.io/on-remove annotation (prune)": {
+			pruneObj: podDeletionPrevention,
+			options:  defaultOptions,
+		},
+		"an object with the cli-utils.sigs.k8s.io/on-remove annotation (destroy)": {
+			pruneObj: podDeletionPrevention,
+			options:  defaultOptionsDestroy,
+		},
+		"an object with the client.lifecycle.config.k8s.io/deletion annotation (prune)": {
+			pruneObj: testutil.Unstructured(t, pdbDeletePreventionManifest),
+			options:  defaultOptions,
+		},
+		"an object with the client.lifecycle.config.k8s.io/deletion annotation (destroy)": {
+			pruneObj: testutil.Unstructured(t, pdbDeletePreventionManifest),
+			options:  defaultOptionsDestroy,
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			pruneID, err := object.UnstructuredToObjMeta(tc.pruneObj)
+			require.NoError(t, err)
+
+			po := PruneOptions{
+				InvClient: inventory.NewFakeInventoryClient(object.ObjMetadataSet{pruneID}),
+				Client:    fake.NewSimpleDynamicClient(scheme.Scheme, tc.pruneObj),
+				Mapper: testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme,
+					scheme.Scheme.PrioritizedVersionsAllGroups()...),
+			}
+			// The event channel can not block; make sure its bigger than all
+			// the events that can be put on it.
+			eventChannel := make(chan event.Event, 2)
+			resourceCache := cache.NewResourceCacheMap()
+			taskContext := taskrunner.NewTaskContext(eventChannel, resourceCache)
+			err = func() error {
+				defer close(eventChannel)
+				// Run the prune and validate.
+				return po.Prune([]*unstructured.Unstructured{tc.pruneObj}, []filter.ValidationFilter{filter.PreventRemoveFilter{}}, taskContext, "test-0", tc.options)
+			}()
+
+			if err != nil {
+				t.Fatalf("Unexpected error during Prune(): %#v", err)
+			}
+			// verify that the object no longer has the annotation
+			obj, err := po.GetObject(pruneID)
+			if err != nil {
+				t.Fatalf("Unexpected error: %#v", err)
+			}
+
+			hasOwningInventoryAnnotation := false
+			for annotation := range obj.GetAnnotations() {
+				if annotation == inventory.OwningInventoryKey {
+					hasOwningInventoryAnnotation = true
+				}
+			}
+			if hasOwningInventoryAnnotation {
+				t.Fatalf("Prune() should remove the %s annotation", inventory.OwningInventoryKey)
+			}
+		})
+	}
+}
+
 // failureNamespaceClient wrappers around a namespaceClient with the overwriting to Get and Delete functions.
 type failureNamespaceClient struct {
 	dynamic.ResourceInterface

pkg/inventory/policy.go

@@ -66,7 +66,8 @@ const (
 	AdoptAll
 )
 
-const owningInventoryKey = "config.k8s.io/owning-inventory"
+// OwningInventoryKey is the annotation key indicating the inventory owning an object.
+const OwningInventoryKey = "config.k8s.io/owning-inventory"
 
 // inventoryIDMatchStatus represents the result of comparing the
 // id from current inventory info and the inventory-id from a live object.
@@ -81,7 +82,7 @@ const (
 
 func InventoryIDMatch(inv InventoryInfo, obj *unstructured.Unstructured) inventoryIDMatchStatus {
 	annotations := obj.GetAnnotations()
-	value, found := annotations[owningInventoryKey]
+	value, found := annotations[OwningInventoryKey]
 	if !found {
 		return Empty
 	}
@@ -101,15 +102,15 @@ func CanApply(inv InventoryInfo, obj *unstructured.Unstructured, policy Inventor
 		if policy != InventoryPolicyMustMatch {
 			return true, nil
 		}
-		err := fmt.Errorf("can't adopt an object without the annotation %s", owningInventoryKey)
+		err := fmt.Errorf("can't adopt an object without the annotation %s", OwningInventoryKey)
 		return false, NewNeedAdoptionError(err)
 	case Match:
 		return true, nil
 	case NoMatch:
 		if policy == AdoptAll {
 			return true, nil
 		}
-		err := fmt.Errorf("can't apply the resource since its annotation %s is a different inventory object", owningInventoryKey)
+		err := fmt.Errorf("can't apply the resource since its annotation %s is a different inventory object", OwningInventoryKey)
 		return false, NewInventoryOverlapError(err)
 	}
 	// shouldn't reach here
@@ -137,7 +138,7 @@ func AddInventoryIDAnnotation(obj *unstructured.Unstructured, inv InventoryInfo)
 	if annotations == nil {
 		annotations = make(map[string]string)
 	}
-	annotations[owningInventoryKey] = inv.ID()
+	annotations[OwningInventoryKey] = inv.ID()
 	obj.SetAnnotations(annotations)
 }