Merge pull request #614 from manfredlift/mliiv-patch-1

fix: objects flagged by CurrentUIDFilter should be removed from the inventory

Author: k8s-ci-robot

pkg/apply/prune/prune.go

@@ -153,6 +153,18 @@ func (p *Pruner) Prune(
 					}
 				}
 
+				// Remove the object from inventory if it was determined that the object should not be pruned,
+				// because it had recently been applied. This probably means that the object is in the inventory
+				// more than one time with a different group (e.g. kind Ingress and apiGroups networking.k8s.io & extensions)
+				// due to being cohabitated: https://github.com/kubernetes/kubernetes/blob/v1.25.0/pkg/kubeapiserver/default_storage_factory_builder.go#L124-L131
+				var deleteAfterApplyErr *filter.ApplyPreventedDeletionError
+				if errors.As(filterErr, &deleteAfterApplyErr) {
+					if !opts.DryRunStrategy.ClientOrServerDryRun() {
+						// Register for removal from the inventory.
+						taskContext.AddAbandonedObject(id)
+					}
+				}
+
 				taskContext.SendEvent(eventFactory.CreateSkippedEvent(obj, filterErr))
 				taskContext.InventoryManager().AddSkippedDelete(id)
 				break

pkg/apply/prune/prune_test.go

@@ -325,7 +325,7 @@ func TestPrune(t *testing.T) {
 				},
 			},
 		},
-		"UID match means prune skipped": {
+		"UID match means prune skipped and object abandoned": {
 			clusterObjs: []*unstructured.Unstructured{pod},
 			pruneObjs:   []*unstructured.Unstructured{pod},
 			pruneFilters: []filter.ValidationFilter{
@@ -351,8 +351,11 @@ func TestPrune(t *testing.T) {
 			expectedSkipped: object.ObjMetadataSet{
 				object.UnstructuredToObjMetadata(pod),
 			},
+			expectedAbandoned: object.ObjMetadataSet{
+				object.UnstructuredToObjMetadata(pod),
+			},
 		},
-		"UID match for only one object one pruned, one skipped": {
+		"UID match for only one object one pruned, one skipped and abandoned": {
 			clusterObjs: []*unstructured.Unstructured{pod, pdb},
 			pruneObjs:   []*unstructured.Unstructured{pod, pdb},
 			pruneFilters: []filter.ValidationFilter{
@@ -386,6 +389,9 @@ func TestPrune(t *testing.T) {
 			expectedSkipped: object.ObjMetadataSet{
 				object.UnstructuredToObjMetadata(pod),
 			},
+			expectedAbandoned: object.ObjMetadataSet{
+				object.UnstructuredToObjMetadata(pod),
+			},
 		},
 		"Prevent delete annotation equals prune skipped": {
 			clusterObjs: []*unstructured.Unstructured{

test/e2e/current_uid_filter_test.go

@@ -0,0 +1,109 @@
+// Copyright 2021 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package e2e
+
+import (
+	"context"
+	"fmt"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/cli-utils/pkg/apply"
+	"sigs.k8s.io/cli-utils/test/e2e/e2eutil"
+	"sigs.k8s.io/cli-utils/test/e2e/invconfig"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+const v1EventTemplate = `
+apiVersion: v1
+involvedObject:
+  apiVersion: v1
+  kind: Pod
+  name: pod
+  namespace: {{.Namespace}}
+kind: Event
+message: Back-off restarting failed container
+metadata:
+  name: test
+  namespace: {{.Namespace}}
+reason: BackOff
+type: Warning
+`
+
+const v1EventsEventTemplate = `
+apiVersion: events.k8s.io/v1
+eventTime: null
+kind: Event
+metadata:
+  name: test
+  namespace: {{.Namespace}}
+note: Back-off restarting failed container
+reason: BackOff
+regarding:
+  apiVersion: v1
+  kind: Pod
+  name: pod
+  namespace: {{.Namespace}}
+type: Warning
+`
+
+// Note this tests the scenario of "cohabitating" k8s objects (an object available via multiple apiGroups), but having the same UID.
+// As of k8s 1.25 an example of such "cohabitating" kinds is Event which is available via both "v1" and "events.k8s.io/v1".
+// See the full list of cohabitating resources on the storage level here:
+// - https://github.com/kubernetes/kubernetes/blob/v1.25.0/pkg/kubeapiserver/default_storage_factory_builder.go#L124-L131
+// We test that when the user upgrades their manifest from one cohabitated apiGroup to the other, then:
+// - it should not result in object being pruned
+// - object pruning should be skipped due to CurrentUIDFilter (even though a diff is found)
+// - inventory should not double-track the object i.e. we should hold reference only to the object with the groupKind that was most recently applied
+func currentUIDFilterTest(ctx context.Context, c client.Client, invConfig invconfig.InventoryConfig, inventoryName, namespaceName string) {
+	applier := invConfig.ApplierFactoryFunc()
+	inventoryID := fmt.Sprintf("%s-%s", inventoryName, namespaceName)
+	inventoryInfo := invconfig.CreateInventoryInfo(invConfig, inventoryName, namespaceName, inventoryID)
+
+	templateFields := struct{ Namespace string }{Namespace: namespaceName}
+	v1Event := e2eutil.TemplateToUnstructured(v1EventTemplate, templateFields)
+	v1EventsEvent := e2eutil.TemplateToUnstructured(v1EventsEventTemplate, templateFields)
+
+	By("Apply resource with deprecated groupKind")
+	resources := []*unstructured.Unstructured{
+		v1Event,
+	}
+	err := e2eutil.Run(applier.Run(ctx, inventoryInfo, resources, apply.ApplierOptions{}))
+	Expect(err).ToNot(HaveOccurred())
+
+	By("Verify resource available in both apiGroups")
+	objDeprecated := e2eutil.AssertUnstructuredExists(ctx, c, v1Event)
+	objNew := e2eutil.AssertUnstructuredExists(ctx, c, v1EventsEvent)
+
+	By("Verify UID matches for cohabitating resources")
+	uid := objDeprecated.GetUID()
+	Expect(uid).ToNot(BeEmpty())
+	Expect(objDeprecated.GetUID()).To(Equal(objNew.GetUID()))
+
+	By("Verify only 1 item in inventory")
+	invConfig.InvSizeVerifyFunc(ctx, c, inventoryName, namespaceName, inventoryID, 1, 1)
+
+	By("Apply resource with new groupKind")
+	resources = []*unstructured.Unstructured{
+		v1EventsEvent,
+	}
+	err = e2eutil.Run(applier.Run(ctx, inventoryInfo, resources, apply.ApplierOptions{}))
+	Expect(err).ToNot(HaveOccurred())
+
+	By("Verify resource still available in both apiGroups")
+	objDeprecated = e2eutil.AssertUnstructuredExists(ctx, c, v1Event)
+	objNew = e2eutil.AssertUnstructuredExists(ctx, c, v1EventsEvent)
+
+	By("Verify UID matches for cohabitating resources")
+	Expect(objDeprecated.GetUID()).To(Equal(objNew.GetUID()))
+
+	By("Verify UID matches the UID from previous apply")
+	Expect(objDeprecated.GetUID()).To(Equal(uid))
+
+	By("Verify still only 1 item in inventory")
+	// Expecting statusCount=2:
+	//   one object applied and one prune skipped
+	invConfig.InvSizeVerifyFunc(ctx, c, inventoryName, namespaceName, inventoryID, 1, 2)
+}

test/e2e/e2e_test.go

@@ -183,6 +183,10 @@ var _ = Describe("E2E", func() {
 					namespaceFilterTest(ctx, c, invConfig, inventoryName, namespace.GetName())
 				})
 
+				It("CurrentUIDFilter", func() {
+					currentUIDFilterTest(ctx, c, invConfig, inventoryName, namespace.GetName())
+				})
+
 				It("PruneRetrievalError", func() {
 					pruneRetrieveErrorTest(ctx, c, invConfig, inventoryName, namespace.GetName())
 				})