From 909ba8527a2eb76bbd8c7a122b34ca5c625fb278 Mon Sep 17 00:00:00 2001
From: whitewindmills <jayfantasyhjh@gmail.com>
Date: Wed, 2 Aug 2023 11:40:38 +0800
Subject: [PATCH] avoid updating directly cached resource template for clean up
 policy

Signed-off-by: whitewindmills <jayfantasyhjh@gmail.com>
---
 pkg/detector/detector.go   | 12 ++++++------
 pkg/detector/policy.go     |  6 +++---
 pkg/util/helper/binding.go |  3 +++
 pkg/util/label.go          | 16 +++++++++++-----
 pkg/util/label_test.go     |  2 +-
 5 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go
index 9730579fa920..f615307e942d 100644
--- a/pkg/detector/detector.go
+++ b/pkg/detector/detector.go
@@ -565,6 +565,9 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
 }
 
 // GetUnstructuredObject retrieves object by key and returned its unstructured.
+// Any updates to this resource template are not recommended as it may come from the informer cache.
+// We should abide by the principle of making a deep copy first and then modifying it.
+// See issue: https://github.com/karmada-io/karmada/issues/3878.
 func (d *ResourceDetector) GetUnstructuredObject(objectKey keys.ClusterWideKey) (*unstructured.Unstructured, error) {
 	objectGVR, err := restmapper.GetGroupVersionResource(d.RESTMapper, objectKey.GroupVersionKind())
 	if err != nil {
@@ -1113,7 +1116,7 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyCreationOrUpdate(policy
 }
 
 // CleanupLabels removes labels from object referencing by objRef.
-func (d *ResourceDetector) CleanupLabels(objRef workv1alpha2.ObjectReference, labels ...string) error {
+func (d *ResourceDetector) CleanupLabels(objRef workv1alpha2.ObjectReference, labelKeys ...string) error {
 	workload, err := helper.FetchResourceTemplate(d.DynamicClient, d.InformerManager, d.RESTMapper, objRef)
 	if err != nil {
 		// do nothing if resource template not exist, it might has been removed.
@@ -1124,11 +1127,8 @@ func (d *ResourceDetector) CleanupLabels(objRef workv1alpha2.ObjectReference, la
 		return err
 	}
 
-	workloadLabels := workload.GetLabels()
-	for _, l := range labels {
-		delete(workloadLabels, l)
-	}
-	workload.SetLabels(workloadLabels)
+	workload = workload.DeepCopy()
+	util.RemoveLabels(workload, labelKeys...)
 
 	gvr, err := restmapper.GetGroupVersionResource(d.RESTMapper, workload.GroupVersionKind())
 	if err != nil {
diff --git a/pkg/detector/policy.go b/pkg/detector/policy.go
index e25b7f77743a..801520236e03 100644
--- a/pkg/detector/policy.go
+++ b/pkg/detector/policy.go
@@ -272,9 +272,9 @@ func (d *ResourceDetector) removeResourceLabelsIfNotMatch(objectReference workv1
 		return false, nil
 	}
 
-	for _, labelKey := range labelKeys {
-		util.RemoveLabel(object, labelKey)
-	}
+	object = object.DeepCopy()
+	util.RemoveLabels(object, labelKeys...)
+
 	err = d.Client.Update(context.TODO(), object)
 	if err != nil {
 		return false, err
diff --git a/pkg/util/helper/binding.go b/pkg/util/helper/binding.go
index c3f72090b12a..7b896b34ff55 100644
--- a/pkg/util/helper/binding.go
+++ b/pkg/util/helper/binding.go
@@ -207,6 +207,9 @@ func RemoveOrphanWorks(c client.Client, works []workv1alpha1.Work) error {
 }
 
 // FetchResourceTemplate fetches the resource template to be propagated.
+// Any updates to this resource template are not recommended as it may come from the informer cache.
+// We should abide by the principle of making a deep copy first and then modifying it.
+// See issue: https://github.com/karmada-io/karmada/issues/3878.
 func FetchResourceTemplate(
 	dynamicClient dynamic.Interface,
 	informerManager genericmanager.SingleClusterInformerManager,
diff --git a/pkg/util/label.go b/pkg/util/label.go
index d9bee47eeee2..8e9c025ae644 100644
--- a/pkg/util/label.go
+++ b/pkg/util/label.go
@@ -57,11 +57,17 @@ func MergeLabel(obj *unstructured.Unstructured, labelKey string, labelValue stri
 	obj.SetLabels(labels)
 }
 
-// RemoveLabel removes the label from the given object.
-func RemoveLabel(obj *unstructured.Unstructured, labelKey string) {
-	labels := obj.GetLabels()
-	delete(labels, labelKey)
-	obj.SetLabels(labels)
+// RemoveLabels removes the labels from the given object.
+func RemoveLabels(obj *unstructured.Unstructured, labelKeys ...string) {
+	if len(labelKeys) == 0 {
+		return
+	}
+
+	objLabels := obj.GetLabels()
+	for _, labelKey := range labelKeys {
+		delete(objLabels, labelKey)
+	}
+	obj.SetLabels(objLabels)
 }
 
 // DedupeAndMergeLabels merges the new labels into exist labels.
diff --git a/pkg/util/label_test.go b/pkg/util/label_test.go
index 9740e834cf0e..e3ad3351763d 100644
--- a/pkg/util/label_test.go
+++ b/pkg/util/label_test.go
@@ -306,7 +306,7 @@ func TestRemoveLabel(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			RemoveLabel(tt.args.obj, tt.args.labelKey)
+			RemoveLabels(tt.args.obj, tt.args.labelKey)
 			if !reflect.DeepEqual(tt.args.obj, tt.expected) {
 				t.Errorf("RemoveLabel() = %v, want %v", tt.args.obj, tt.expected)
 			}