avoid updating directly cached resource template for clean up policy

Signed-off-by: whitewindmills <[email protected]>

pkg/detector/detector.go

@@ -551,6 +551,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 {
@@ -1077,7 +1080,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.FetchWorkload(d.DynamicClient, d.InformerManager, d.RESTMapper, objRef)
 	if err != nil {
 		// do nothing if resource template not exist, it might has been removed.
@@ -1088,11 +1091,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 {

pkg/detector/policy.go

@@ -263,9 +263,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

pkg/util/helper/binding.go

@@ -215,7 +215,10 @@ func RemoveOrphanWorks(c client.Client, works []workv1alpha1.Work) error {
 	return errors.NewAggregate(errs)
 }
 
-// FetchWorkload fetches the kubernetes resource to be propagated.
+// FetchWorkload fetches the workload to be propagated.
+// Any updates to this workload 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 FetchWorkload(dynamicClient dynamic.Interface, informerManager genericmanager.SingleClusterInformerManager,
 	restMapper meta.RESTMapper, resource workv1alpha2.ObjectReference) (*unstructured.Unstructured, error) {
 	dynamicResource, err := restmapper.GetGroupVersionResource(restMapper,

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.

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)
 			}