chore: keep the finalizers added externally on the object (#9403)

(cherry picked from commit c975491)

Author: leon-ape

controllers/workloads/instanceset_controller_test.go

@@ -197,7 +197,9 @@ var _ = Describe("InstanceSet Controller", func() {
 
 			By("delete the ITS object")
 			Expect(k8sClient.Delete(ctx, itsObj)).Should(Succeed())
-			Eventually(testapps.CheckObjExists(&testCtx, itsKey, &workloads.InstanceSet{}, false)).Should(Succeed())
+
+			By("check its object NOT deleted")
+			Consistently(testapps.CheckObjExists(&testCtx, itsKey, &workloads.InstanceSet{}, true)).Should(Succeed())
 
 			By("check pods deleted")
 			podKey := types.NamespacedName{
@@ -206,12 +208,16 @@ var _ = Describe("InstanceSet Controller", func() {
 			}
 			Eventually(testapps.CheckObjExists(&testCtx, podKey, &corev1.Pod{}, false)).Should(Succeed())
 
-			By("check PVCs deleted")
+			By("check PVCs deleted, but the pvc-protection finalizer prevent the pvc to be deleted physically")
 			pvcKey := types.NamespacedName{
 				Namespace: itsObj.Namespace,
 				Name:      fmt.Sprintf("%s-%s-0", pvc.Name, itsObj.Name),
 			}
-			Eventually(testapps.CheckObjExists(&testCtx, pvcKey, &corev1.PersistentVolumeClaim{}, false)).Should(Succeed())
+			Eventually(testapps.CheckObj(&testCtx, pvcKey, func(g Gomega, pvc *corev1.PersistentVolumeClaim) {
+				g.Expect(pvc.DeletionTimestamp).ShouldNot(BeNil())
+				g.Expect(pvc.Finalizers).To(HaveLen(1))
+				g.Expect(pvc.Finalizers[0]).To(Equal("kubernetes.io/pvc-protection"))
+			})).Should(Succeed())
 		})
 
 		It("when deleted - retain", func() {
@@ -233,12 +239,14 @@ var _ = Describe("InstanceSet Controller", func() {
 			}
 			Eventually(testapps.CheckObjExists(&testCtx, podKey, &corev1.Pod{}, false)).Should(Succeed())
 
-			By("check PVCs retained")
+			By("check PVCs retained and not deleted")
 			pvcKey := types.NamespacedName{
 				Namespace: itsObj.Namespace,
 				Name:      fmt.Sprintf("%s-%s-0", pvc.Name, itsObj.Name),
 			}
-			Eventually(testapps.CheckObjExists(&testCtx, pvcKey, &corev1.PersistentVolumeClaim{}, true)).Should(Succeed())
+			Consistently(testapps.CheckObj(&testCtx, pvcKey, func(g Gomega, pvc *corev1.PersistentVolumeClaim) {
+				g.Expect(pvc.DeletionTimestamp).Should(BeNil())
+			})).Should(Succeed())
 		})
 
 		It("when scaled - delete", func() {
@@ -261,12 +269,16 @@ var _ = Describe("InstanceSet Controller", func() {
 			}
 			Eventually(testapps.CheckObjExists(&testCtx, podKey, &corev1.Pod{}, false)).Should(Succeed())
 
-			By("check PVCs deleted")
+			By("check PVCs deleted, but the pvc-protection finalizer prevent the pvc to be deleted physically")
 			pvcKey := types.NamespacedName{
 				Namespace: itsObj.Namespace,
 				Name:      fmt.Sprintf("%s-%s-0", pvc.Name, itsObj.Name),
 			}
-			Eventually(testapps.CheckObjExists(&testCtx, pvcKey, &corev1.PersistentVolumeClaim{}, false)).Should(Succeed())
+			Eventually(testapps.CheckObj(&testCtx, pvcKey, func(g Gomega, pvc *corev1.PersistentVolumeClaim) {
+				g.Expect(pvc.DeletionTimestamp).ShouldNot(BeNil())
+				g.Expect(pvc.Finalizers).To(HaveLen(1))
+				g.Expect(pvc.Finalizers[0]).To(Equal("kubernetes.io/pvc-protection"))
+			})).Should(Succeed())
 		})
 
 		It("when scaled - retain", func() {
@@ -289,12 +301,14 @@ var _ = Describe("InstanceSet Controller", func() {
 			}
 			Eventually(testapps.CheckObjExists(&testCtx, podKey, &corev1.Pod{}, false)).Should(Succeed())
 
-			By("check PVCs retained")
+			By("check PVCs retained and not deleted")
 			pvcKey := types.NamespacedName{
 				Namespace: itsObj.Namespace,
 				Name:      fmt.Sprintf("%s-%s-0", pvc.Name, itsObj.Name),
 			}
-			Eventually(testapps.CheckObjExists(&testCtx, pvcKey, &corev1.PersistentVolumeClaim{}, true)).Should(Succeed())
+			Consistently(testapps.CheckObj(&testCtx, pvcKey, func(g Gomega, pvc *corev1.PersistentVolumeClaim) {
+				g.Expect(pvc.DeletionTimestamp).Should(BeNil())
+			})).Should(Succeed())
 		})
 	})
 

pkg/controller/kubebuilderx/plan_builder.go

@@ -178,11 +178,9 @@ func buildOrderedVertices(ctx context.Context, currentTree *ObjectTree, desiredT
 			}
 		}
 	}
-	finalizer := currentTree.GetFinalizer()
 	deleteOrphanObjects := func() {
 		for name := range deleteSet {
 			object := oldSnapshot[name]
-			keepFinalizer(object, finalizer)
 			v := model.NewObjectVertex(nil, object, model.ActionDeletePtr(), inDataContext4G())
 			findAndAppend(v)
 		}
@@ -203,21 +201,6 @@ func buildOrderedVertices(ctx context.Context, currentTree *ObjectTree, desiredT
 	return vertices
 }
 
-func keepFinalizer(object client.Object, finalizer string) {
-	var finalizers []string
-	if len(finalizer) > 0 {
-		finalizers = append(finalizers, finalizer)
-	}
-	object.SetFinalizers(finalizers)
-}
-
-func getRemainingFinalizer(obj client.Object) string {
-	if len(obj.GetFinalizers()) > 0 {
-		return obj.GetFinalizers()[0]
-	}
-	return ""
-}
-
 // Plan implementation
 
 func (p *Plan) Execute() error {
@@ -285,7 +268,10 @@ func (b *PlanBuilder) patchObject(ctx context.Context, vertex *model.ObjectVerte
 }
 
 func (b *PlanBuilder) deleteObject(ctx context.Context, vertex *model.ObjectVertex) error {
-	finalizer := getRemainingFinalizer(vertex.Obj)
+	var finalizer string
+	if b.currentTree != nil {
+		finalizer = b.currentTree.GetFinalizer()
+	}
 	if len(finalizer) > 0 && controllerutil.RemoveFinalizer(vertex.Obj, finalizer) {
 		err := b.cli.Update(ctx, vertex.Obj, clientOption(vertex))
 		if err != nil && !apierrors.IsNotFound(err) {

pkg/controller/kubebuilderx/plan_builder_test.go

@@ -128,20 +128,21 @@ var _ = Describe("plan builder test", func() {
 				Obj:    its,
 				Action: model.ActionDeletePtr(),
 			}
-			k8sMock.EXPECT().
-				Update(gomock.Any(), gomock.Any(), gomock.Any()).
-				DoAndReturn(func(_ context.Context, obj *workloads.InstanceSet, _ ...client.UpdateOption) error {
-					Expect(obj).ShouldNot(BeNil())
-					Expect(obj.Finalizers).Should(HaveLen(0))
-					return nil
-				}).Times(1)
+			// k8sMock.EXPECT().
+			//	Update(gomock.Any(), gomock.Any(), gomock.Any()).
+			//	DoAndReturn(func(_ context.Context, obj *workloads.InstanceSet, _ ...client.UpdateOption) error {
+			//		Expect(obj).ShouldNot(BeNil())
+			//		Expect(obj.Finalizers).Should(HaveLen(0))
+			//		return nil
+			//	}).Times(1)
 			k8sMock.EXPECT().
 				Delete(gomock.Any(), gomock.Any(), gomock.Any()).
 				DoAndReturn(func(_ context.Context, obj *workloads.InstanceSet, _ ...client.DeleteOption) error {
 					Expect(obj).ShouldNot(BeNil())
 					Expect(obj.Namespace).Should(Equal(its.Namespace))
 					Expect(obj.Name).Should(Equal(its.Name))
-					Expect(obj.Finalizers).Should(HaveLen(0))
+					Expect(obj.Finalizers).Should(HaveLen(1))
+					Expect(obj.Finalizers[0]).Should(Equal(finalizer))
 					return nil
 				}).Times(1)
 			Expect(planBuilder.defaultWalkFunc(v)).Should(Succeed())