Skip to content

Commit 99b7902

Browse files
XiShanYongYe-ChangXiShanYongYe-Chang
committed
merge EndpointSlice Work finalizers instead of overwriting it
Signed-off-by: changzhen <[email protected]>
1 parent 5380f61 commit 99b7902

File tree

6 files changed

+254
-6
lines changed

6 files changed

+254
-6
lines changed

‎pkg/controllers/ctrlutil/work.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func CreateOrUpdateWork(ctx context.Context, c client.Client, workMeta metav1.Ob
7171

7272
runtimeObject.Labels = util.DedupeAndMergeLabels(runtimeObject.Labels, work.Labels)
7373
runtimeObject.Annotations = util.DedupeAndMergeAnnotations(runtimeObject.Annotations, work.Annotations)
74-
runtimeObject.Finalizers = work.Finalizers
74+
runtimeObject.Finalizers = util.MergeFinalizers(runtimeObject.Finalizers, work.Finalizers)
7575
runtimeObject.Spec = work.Spec
7676

7777
// Do the same thing as the mutating webhook does, add the permanent ID to workload if not exist,

‎pkg/controllers/mcs/service_export_controller.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -541,10 +541,14 @@ func getEndpointSliceWorkMeta(ctx context.Context, c client.Client, ns string, w
541541
return metav1.ObjectMeta{}, err
542542
}
543543

544+
existFinalizers := existWork.GetFinalizers()
545+
finalizersToAdd := []string{util.EndpointSliceControllerFinalizer}
546+
newFinalizers := util.MergeFinalizers(existFinalizers, finalizersToAdd)
547+
544548
workMeta := metav1.ObjectMeta{
545549
Name: workName,
546550
Namespace: ns,
547-
Finalizers: []string{util.EndpointSliceControllerFinalizer},
551+
Finalizers: newFinalizers,
548552
Labels: map[string]string{
549553
util.ServiceNamespaceLabel: endpointSlice.GetNamespace(),
550554
util.ServiceNameLabel: endpointSlice.GetLabels()[discoveryv1.LabelServiceName],

‎pkg/controllers/multiclusterservice/endpointslice_collect_controller.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -415,13 +415,22 @@ func getEndpointSliceWorkMeta(ctx context.Context, c client.Client, ns string, w
415415
return metav1.ObjectMeta{}, err
416416
}
417417

418+
existFinalizers := existWork.GetFinalizers()
419+
finalizersToAdd := []string{util.MCSEndpointSliceDispatchControllerFinalizer}
420+
newFinalizers := util.MergeFinalizers(existFinalizers, finalizersToAdd)
421+
418422
ls := map[string]string{
419423
util.MultiClusterServiceNamespaceLabel: endpointSlice.GetNamespace(),
420424
util.MultiClusterServiceNameLabel: endpointSlice.GetLabels()[discoveryv1.LabelServiceName],
421425
util.EndpointSliceWorkManagedByLabel: util.MultiClusterServiceKind,
422426
}
423427
if existWork.Labels == nil || (err != nil && apierrors.IsNotFound(err)) {
424-
workMeta := metav1.ObjectMeta{Name: workName, Namespace: ns, Labels: ls}
428+
workMeta := metav1.ObjectMeta{
429+
Name: workName,
430+
Namespace: ns,
431+
Labels: ls,
432+
Finalizers: newFinalizers,
433+
}
425434
return workMeta, nil
426435
}
427436

@@ -436,7 +445,7 @@ func getEndpointSliceWorkMeta(ctx context.Context, c client.Client, ns string, w
436445
Name: workName,
437446
Namespace: ns,
438447
Labels: ls,
439-
Finalizers: []string{util.MCSEndpointSliceDispatchControllerFinalizer},
448+
Finalizers: newFinalizers,
440449
}, nil
441450
}
442451

‎pkg/controllers/multiclusterservice/endpointslice_collect_controller_test.go‎

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,11 @@ func TestGetEndpointSliceWorkMeta(t *testing.T) {
146146
util.MultiClusterServiceNameLabel: "test-service",
147147
util.EndpointSliceWorkManagedByLabel: util.MultiClusterServiceKind,
148148
},
149+
Finalizers: []string{util.MCSEndpointSliceDispatchControllerFinalizer},
149150
},
150151
},
151152
{
152-
name: "Existing work for EndpointSlice",
153+
name: "Existing work for EndpointSlice without finalizers",
153154
existingWork: createExistingWork("endpointslice-test-eps-default", "test-cluster", "ExistingController"),
154155
endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
155156
expectedMeta: metav1.ObjectMeta{
@@ -163,6 +164,51 @@ func TestGetEndpointSliceWorkMeta(t *testing.T) {
163164
Finalizers: []string{util.MCSEndpointSliceDispatchControllerFinalizer},
164165
},
165166
},
167+
{
168+
name: "Existing work with existing finalizers",
169+
existingWork: createExistingWorkWithFinalizers("endpointslice-test-eps-default", "test-cluster", "ExistingController", []string{"existing.finalizer", "another.finalizer"}),
170+
endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
171+
expectedMeta: metav1.ObjectMeta{
172+
Name: "endpointslice-test-eps-default",
173+
Namespace: "test-cluster",
174+
Labels: map[string]string{
175+
util.MultiClusterServiceNamespaceLabel: "default",
176+
util.MultiClusterServiceNameLabel: "test-service",
177+
util.EndpointSliceWorkManagedByLabel: "ExistingController.MultiClusterService",
178+
},
179+
Finalizers: []string{"another.finalizer", "existing.finalizer", util.MCSEndpointSliceDispatchControllerFinalizer},
180+
},
181+
},
182+
{
183+
name: "Existing work with duplicate finalizer",
184+
existingWork: createExistingWorkWithFinalizers("endpointslice-test-eps-default", "test-cluster", "ExistingController", []string{util.MCSEndpointSliceDispatchControllerFinalizer, "another.finalizer"}),
185+
endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
186+
expectedMeta: metav1.ObjectMeta{
187+
Name: "endpointslice-test-eps-default",
188+
Namespace: "test-cluster",
189+
Labels: map[string]string{
190+
util.MultiClusterServiceNamespaceLabel: "default",
191+
util.MultiClusterServiceNameLabel: "test-service",
192+
util.EndpointSliceWorkManagedByLabel: "ExistingController.MultiClusterService",
193+
},
194+
Finalizers: []string{"another.finalizer", util.MCSEndpointSliceDispatchControllerFinalizer},
195+
},
196+
},
197+
{
198+
name: "Existing work without labels",
199+
existingWork: createExistingWorkWithoutLabels("endpointslice-test-eps-default", "test-cluster", []string{"existing.finalizer"}),
200+
endpointSlice: createEndpointSliceForTest("test-eps", "default", "test-service", false),
201+
expectedMeta: metav1.ObjectMeta{
202+
Name: "endpointslice-test-eps-default",
203+
Namespace: "test-cluster",
204+
Labels: map[string]string{
205+
util.MultiClusterServiceNamespaceLabel: "default",
206+
util.MultiClusterServiceNameLabel: "test-service",
207+
util.EndpointSliceWorkManagedByLabel: util.MultiClusterServiceKind,
208+
},
209+
Finalizers: []string{"existing.finalizer", util.MCSEndpointSliceDispatchControllerFinalizer},
210+
},
211+
},
166212
{
167213
name: "Nil EndpointSlice",
168214
endpointSlice: nil,
@@ -186,7 +232,10 @@ func TestGetEndpointSliceWorkMeta(t *testing.T) {
186232
require.NoError(t, err)
187233
assert.Equal(t, tc.expectedMeta.Name, meta.Name)
188234
assert.Equal(t, tc.expectedMeta.Namespace, meta.Namespace)
189-
assert.Equal(t, tc.expectedMeta.Finalizers, meta.Finalizers)
235+
236+
assert.Equal(t, tc.expectedMeta.Finalizers, meta.Finalizers,
237+
"Finalizers do not match. Expected: %v, Got: %v", tc.expectedMeta.Finalizers, meta.Finalizers)
238+
190239
assert.True(t, compareLabels(meta.Labels, tc.expectedMeta.Labels),
191240
"Labels do not match. Expected: %v, Got: %v", tc.expectedMeta.Labels, meta.Labels)
192241
}
@@ -325,6 +374,31 @@ func createExistingWork(name, namespace, managedBy string) *workv1alpha1.Work {
325374
}
326375
}
327376

377+
// Helper function to create an existing Work resource for testing with specific finalizers
378+
func createExistingWorkWithFinalizers(name, namespace, managedBy string, finalizers []string) *workv1alpha1.Work {
379+
return &workv1alpha1.Work{
380+
ObjectMeta: metav1.ObjectMeta{
381+
Name: name,
382+
Namespace: namespace,
383+
Labels: map[string]string{
384+
util.EndpointSliceWorkManagedByLabel: managedBy,
385+
},
386+
Finalizers: finalizers,
387+
},
388+
}
389+
}
390+
391+
// Helper function to create an existing Work resource for testing without labels
392+
func createExistingWorkWithoutLabels(name, namespace string, finalizers []string) *workv1alpha1.Work {
393+
return &workv1alpha1.Work{
394+
ObjectMeta: metav1.ObjectMeta{
395+
Name: name,
396+
Namespace: namespace,
397+
Finalizers: finalizers,
398+
},
399+
}
400+
}
401+
328402
// Helper function to create a fake client with an optional existing Work
329403
func createFakeClient(existingWork *workv1alpha1.Work) client.Client {
330404
scheme := setupSchemeEndpointCollect()

‎pkg/util/finalizer.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
Copyright 2025 The Karmada Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package util
18+
19+
import "k8s.io/apimachinery/pkg/util/sets"
20+
21+
// MergeFinalizers merges the new finalizers into exist finalizers, and deduplicates the finalizers.
22+
// The result is sorted.
23+
func MergeFinalizers(existFinalizers, newFinalizers []string) []string {
24+
if existFinalizers == nil && newFinalizers == nil {
25+
return nil
26+
}
27+
28+
finalizers := sets.New[string](existFinalizers...).Insert(newFinalizers...)
29+
return sets.List[string](finalizers)
30+
}

‎pkg/util/finalizer_test.go‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/*
2+
Copyright 2025 The Karmada Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package util
18+
19+
import (
20+
"reflect"
21+
"testing"
22+
)
23+
24+
func TestMergeFinalizers(t *testing.T) {
25+
tests := []struct {
26+
name string
27+
existFinalizers []string
28+
newFinalizers []string
29+
expectedResult []string
30+
}{
31+
{
32+
name: "both nil",
33+
existFinalizers: nil,
34+
newFinalizers: nil,
35+
expectedResult: nil,
36+
},
37+
{
38+
name: "exist finalizers is nil",
39+
existFinalizers: nil,
40+
newFinalizers: []string{"finalizer1", "finalizer2"},
41+
expectedResult: []string{"finalizer1", "finalizer2"},
42+
},
43+
{
44+
name: "new finalizers is nil",
45+
existFinalizers: []string{"finalizer1", "finalizer2"},
46+
newFinalizers: nil,
47+
expectedResult: []string{"finalizer1", "finalizer2"},
48+
},
49+
{
50+
name: "both empty",
51+
existFinalizers: []string{},
52+
newFinalizers: []string{},
53+
expectedResult: []string{},
54+
},
55+
{
56+
name: "exist finalizers is empty",
57+
existFinalizers: []string{},
58+
newFinalizers: []string{"finalizer1", "finalizer2"},
59+
expectedResult: []string{"finalizer1", "finalizer2"},
60+
},
61+
{
62+
name: "new finalizers is empty",
63+
existFinalizers: []string{"finalizer1", "finalizer2"},
64+
newFinalizers: []string{},
65+
expectedResult: []string{"finalizer1", "finalizer2"},
66+
},
67+
{
68+
name: "no duplicates",
69+
existFinalizers: []string{"finalizer1", "finalizer2"},
70+
newFinalizers: []string{"finalizer3", "finalizer4"},
71+
expectedResult: []string{"finalizer1", "finalizer2", "finalizer3", "finalizer4"},
72+
},
73+
{
74+
name: "with duplicates",
75+
existFinalizers: []string{"finalizer1", "finalizer2"},
76+
newFinalizers: []string{"finalizer2", "finalizer3"},
77+
expectedResult: []string{"finalizer1", "finalizer2", "finalizer3"},
78+
},
79+
{
80+
name: "all duplicates",
81+
existFinalizers: []string{"finalizer1", "finalizer2"},
82+
newFinalizers: []string{"finalizer1", "finalizer2"},
83+
expectedResult: []string{"finalizer1", "finalizer2"},
84+
},
85+
{
86+
name: "duplicates in exist finalizers",
87+
existFinalizers: []string{"finalizer1", "finalizer2", "finalizer1"},
88+
newFinalizers: []string{"finalizer3"},
89+
expectedResult: []string{"finalizer1", "finalizer2", "finalizer3"},
90+
},
91+
{
92+
name: "duplicates in new finalizers",
93+
existFinalizers: []string{"finalizer1"},
94+
newFinalizers: []string{"finalizer2", "finalizer3", "finalizer2"},
95+
expectedResult: []string{"finalizer1", "finalizer2", "finalizer3"},
96+
},
97+
{
98+
name: "duplicates in both",
99+
existFinalizers: []string{"finalizer1", "finalizer2", "finalizer1"},
100+
newFinalizers: []string{"finalizer2", "finalizer3", "finalizer2"},
101+
expectedResult: []string{"finalizer1", "finalizer2", "finalizer3"},
102+
},
103+
{
104+
name: "single finalizer in exist",
105+
existFinalizers: []string{"finalizer1"},
106+
newFinalizers: []string{"finalizer2"},
107+
expectedResult: []string{"finalizer1", "finalizer2"},
108+
},
109+
{
110+
name: "single duplicate finalizer",
111+
existFinalizers: []string{"finalizer1"},
112+
newFinalizers: []string{"finalizer1"},
113+
expectedResult: []string{"finalizer1"},
114+
},
115+
{
116+
name: "sort with result",
117+
existFinalizers: []string{"finalizer3", "finalizer1", "finalizer2"},
118+
newFinalizers: []string{"finalizer4", "finalizer5"},
119+
expectedResult: []string{"finalizer1", "finalizer2", "finalizer3", "finalizer4", "finalizer5"},
120+
},
121+
}
122+
123+
for _, tt := range tests {
124+
t.Run(tt.name, func(t *testing.T) {
125+
result := MergeFinalizers(tt.existFinalizers, tt.newFinalizers)
126+
if !reflect.DeepEqual(result, tt.expectedResult) {
127+
t.Errorf("MergeFinalizers() = %v, want %v", result, tt.expectedResult)
128+
}
129+
})
130+
}
131+
}

0 commit comments

Comments
 (0)