Merge pull request #3879 from whitewindmills/detector-resource

fix bug: avoid updating directly cached resource template

pkg/detector/detector.go

@@ -566,6 +566,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 {
@@ -1114,7 +1117,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.
@@ -1125,11 +1128,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

@@ -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

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,

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