Merge pull request #6157 from CharlesQQ/detector-update-fix

fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated.

pkg/detector/detector.go

@@ -466,6 +466,7 @@ func (d *ResourceDetector) ApplyPolicy(object *unstructured.Unstructured, object
 			// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
 			bindingCopy.Annotations = util.DedupeAndMergeAnnotations(bindingCopy.Annotations, binding.Annotations)
 			bindingCopy.Labels = util.DedupeAndMergeLabels(bindingCopy.Labels, binding.Labels)
+			bindingCopy.Finalizers = util.DedupeAndMergeFinalizers(bindingCopy.Finalizers, binding.Finalizers)
 			bindingCopy.OwnerReferences = binding.OwnerReferences
 			bindingCopy.Finalizers = binding.Finalizers
 			bindingCopy.Spec.Resource = binding.Spec.Resource
@@ -560,6 +561,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
 				// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
 				bindingCopy.Annotations = util.DedupeAndMergeAnnotations(bindingCopy.Annotations, binding.Annotations)
 				bindingCopy.Labels = util.DedupeAndMergeLabels(bindingCopy.Labels, binding.Labels)
+				bindingCopy.Finalizers = util.DedupeAndMergeFinalizers(bindingCopy.Finalizers, binding.Finalizers)
 				bindingCopy.OwnerReferences = binding.OwnerReferences
 				bindingCopy.Finalizers = binding.Finalizers
 				bindingCopy.Spec.Resource = binding.Spec.Resource
@@ -613,6 +615,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
 				// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
 				bindingCopy.Annotations = util.DedupeAndMergeAnnotations(bindingCopy.Annotations, binding.Annotations)
 				bindingCopy.Labels = util.DedupeAndMergeLabels(bindingCopy.Labels, binding.Labels)
+				bindingCopy.Finalizers = util.DedupeAndMergeFinalizers(bindingCopy.Finalizers, binding.Finalizers)
 				bindingCopy.OwnerReferences = binding.OwnerReferences
 				bindingCopy.Finalizers = binding.Finalizers
 				bindingCopy.Spec.Resource = binding.Spec.Resource

pkg/util/label.go

@@ -124,3 +124,21 @@ func RecordManagedLabels(object *unstructured.Unstructured) {
 	annotations[workv1alpha2.ManagedLabels] = strings.Join(managedKeys, ",")
 	object.SetAnnotations(annotations)
 }
+
+// DedupeAndMergeFinalizers merges the new finalizers into exist finalizers.
+func DedupeAndMergeFinalizers(existFinalizers, newFinalizers []string) []string {
+	if len(existFinalizers) == 0 {
+		return newFinalizers
+	}
+	existFinalizerSets := sets.Set[string]{}
+	existFinalizerSets.Insert(existFinalizers...)
+
+	var mergedFinalizers []string
+	mergedFinalizers = append(mergedFinalizers, existFinalizers...)
+	for _, item := range newFinalizers {
+		if !existFinalizerSets.Has(item) {
+			mergedFinalizers = append(mergedFinalizers, item)
+		}
+	}
+	return mergedFinalizers
+}

pkg/util/label_test.go

@@ -20,6 +20,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
@@ -713,3 +714,69 @@ func TestRecordManagedLabels(t *testing.T) {
 		})
 	}
 }
+
+func TestDedupeAndMergeFinalizers(t *testing.T) {
+	type args struct {
+		existFinalizers []string
+		newFinalizers   []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want []string
+	}{
+		{
+			name: "existFinalizers is nil",
+			args: args{
+				existFinalizers: nil,
+				newFinalizers:   []string{"karmada.io/binding-controller"},
+			},
+			want: []string{"karmada.io/binding-controller"},
+		},
+		{
+			name: "newFinalizers is nil",
+			args: args{
+				existFinalizers: []string{"karmada.io/binding-controller"},
+				newFinalizers:   nil,
+			},
+			want: []string{"karmada.io/binding-controller"},
+		},
+		{
+			name: "binding-controller in front of binding-dependencies-distributor",
+			args: args{
+				existFinalizers: []string{"karmada.io/binding-controller", "karmada.io/binding-dependencies-distributor"},
+				newFinalizers:   []string{"karmada.io/binding-controller"},
+			},
+			want: []string{"karmada.io/binding-controller", "karmada.io/binding-dependencies-distributor"},
+		},
+		{
+			name: "binding-dependencies-distributor in front of binding-controller",
+			args: args{
+				existFinalizers: []string{"karmada.io/binding-dependencies-distributor", "karmada.io/binding-controller"},
+				newFinalizers:   []string{"karmada.io/binding-controller"},
+			},
+			want: []string{"karmada.io/binding-dependencies-distributor", "karmada.io/binding-controller"},
+		},
+		{
+			name: "new finalizers have all Finalizers",
+			args: args{
+				existFinalizers: []string{"karmada.io/binding-dependencies-distributor", "karmada.io/binding-controller"},
+				newFinalizers:   []string{"karmada.io/binding-controller", "karmada.io/binding-dependencies-distributor"},
+			},
+			want: []string{"karmada.io/binding-dependencies-distributor", "karmada.io/binding-controller"},
+		},
+		{
+			name: "existFinalizers have only one item",
+			args: args{
+				existFinalizers: []string{"karmada.io/binding-dependencies-distributor"},
+				newFinalizers:   []string{"karmada.io/binding-controller", "karmada.io/binding-dependencies-distributor"},
+			},
+			want: []string{"karmada.io/binding-dependencies-distributor", "karmada.io/binding-controller"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, DedupeAndMergeFinalizers(tt.args.existFinalizers, tt.args.newFinalizers), "DedupeAndMergeFinalizers(%v, %v)", tt.args.existFinalizers, tt.args.newFinalizers)
+		})
+	}
+}