Skip to content

Commit bfa9322

Browse files
committed
fixup! Use SSA instead of Update for finalizer operations
Signed-off-by: Todd Short <tshort@redhat.com>
1 parent 1e353c7 commit bfa9322

File tree

5 files changed

+54
-95
lines changed

5 files changed

+54
-95
lines changed

internal/catalogd/controllers/core/clustercatalog_controller.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ import (
4444
)
4545

4646
const (
47-
ClusterCatalogFinalizerOwner = "olm.operatorframework.io/clustercatalog-controller"
48-
fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache"
47+
clusterCatalogFinalizerOwner = finalizerutil.FinalizerPrefix + "clustercatalog-controller"
48+
fbcDeletionFinalizer = finalizerutil.FinalizerPrefix + "delete-server-cache"
4949
// CatalogSources are polled if PollInterval is mentioned, in intervals of wait.Jitter(pollDuration, maxFactor)
5050
// wait.Jitter returns a time.Duration between pollDuration and pollDuration + maxFactor * pollDuration.
5151
requeueJitterMaxFactor = 0.01
@@ -157,7 +157,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
157157

158158
// Remove the fbcDeletionFinalizer as we do not want a finalizer attached to the catalog
159159
// when it is disabled. Because the finalizer serves no purpose now.
160-
if err := finalizerutil.RemoveFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer); err != nil {
160+
if _, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil {
161161
return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err)
162162
}
163163

@@ -173,7 +173,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
173173
if err := r.deleteCatalogCache(ctx, catalog); err != nil {
174174
return ctrl.Result{}, fmt.Errorf("finalizer %q failed: %w", fbcDeletionFinalizer, err)
175175
}
176-
if err := finalizerutil.RemoveFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer); err != nil {
176+
if _, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil {
177177
return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err)
178178
}
179179
// Update status to reflect that catalog is no longer serving
@@ -182,7 +182,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
182182
}
183183

184184
// Add finalizer
185-
finalizerAdded, err := finalizerutil.AddFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer)
185+
finalizerAdded, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer)
186186
if err != nil {
187187
return ctrl.Result{}, fmt.Errorf("error ensuring finalizer: %v", err)
188188
}

internal/operator-controller/controllers/clusterextension_controller.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,9 @@ import (
5959
)
6060

6161
const (
62-
ClusterExtensionCleanupUnpackCacheFinalizer = "olm.operatorframework.io/cleanup-unpack-cache"
63-
ClusterExtensionCleanupContentManagerCacheFinalizer = "olm.operatorframework.io/cleanup-contentmanager-cache"
64-
ClusterExtensionFinalizerOwner = "olm.operatorframework.io/clusterextension-controller"
62+
ClusterExtensionCleanupUnpackCacheFinalizer = finalizerutil.FinalizerPrefix + "cleanup-unpack-cache"
63+
ClusterExtensionCleanupContentManagerCacheFinalizer = finalizerutil.FinalizerPrefix + "cleanup-contentmanager-cache"
64+
clusterExtensionFinalizerOwner = finalizerutil.FinalizerPrefix + "clusterextension-controller"
6565
)
6666

6767
// ClusterExtensionReconciler reconciles a ClusterExtension object
@@ -178,7 +178,7 @@ func checkForUnexpectedClusterExtensionFieldChange(a, b *ocv1.ClusterExtension)
178178

179179
func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *ocv1.ClusterExtension) (ctrl.Result, error) {
180180
// Run cleanup for each registered finalizer and collect finalizers to remove
181-
finalizersToRemove := make([]string, 0, len(r.FinalizerHandlers))
181+
removeFinalizers := false
182182
for finalizerKey, handler := range r.FinalizerHandlers {
183183
if !controllerutil.ContainsFinalizer(ext, finalizerKey) {
184184
continue
@@ -187,11 +187,11 @@ func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *oc
187187
setStatusProgressing(ext, err)
188188
return ctrl.Result{}, err
189189
}
190-
finalizersToRemove = append(finalizersToRemove, finalizerKey)
190+
removeFinalizers = true
191191
}
192192
// Remove all finalizers in a single patch operation
193-
if len(finalizersToRemove) > 0 {
194-
if err := finalizerutil.RemoveFinalizers(ctx, ClusterExtensionFinalizerOwner, r.Client, ext, finalizersToRemove...); err != nil {
193+
if removeFinalizers {
194+
if _, err := finalizerutil.UpdateFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext); err != nil {
195195
setStatusProgressing(ext, err)
196196
return ctrl.Result{}, fmt.Errorf("error removing finalizers: %v", err)
197197
}
@@ -217,7 +217,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
217217
finalizers = append(finalizers, finalizerKey)
218218
}
219219
if len(finalizers) > 0 {
220-
if _, err := finalizerutil.AddFinalizers(ctx, ClusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil {
220+
if _, err := finalizerutil.UpdateFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil {
221221
setStatusProgressing(ext, err)
222222
return ctrl.Result{}, fmt.Errorf("error ensuring finalizers: %v", err)
223223
}

internal/operator-controller/controllers/clusterextension_controller_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
3333
"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
3434
"github.com/operator-framework/operator-controller/internal/operator-controller/resolve"
35+
finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer"
3536
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
3637
)
3738

@@ -767,7 +768,7 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) {
767768
Image: "quay.io/operatorhubio/prometheus@fake1.0.0",
768769
}, &v, nil, nil
769770
})
770-
fakeFinalizer := "fake.testfinalizer.io"
771+
fakeFinalizer := finalizerutil.FinalizerPrefix + "fake.testfinalizer"
771772
finalizersMessage := "still have finalizers"
772773
reconciler.Applier = &MockApplier{
773774
installCompleted: true,

internal/operator-controller/controllers/clusterextensionrevision_controller.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ import (
3737

3838
const (
3939
ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner"
40-
clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown"
41-
CluserExtensionRevisionFinalizerOwner = "olm.operatorframework.io/clusterextensionrevision-controller"
40+
clusterExtensionRevisionTeardownFinalizer = finalizerutil.FinalizerPrefix + "teardown"
41+
cluserExtensionRevisionFinalizerOwner = finalizerutil.FinalizerPrefix + "clusterextensionrevision-controller"
4242
)
4343

4444
// ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions,
@@ -124,7 +124,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
124124
//
125125
// Reconcile
126126
//
127-
if _, err := finalizerutil.AddFinalizers(ctx, CluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
127+
if _, err := finalizerutil.UpdateFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
128128
meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
129129
Type: ocv1.ClusterExtensionRevisionTypeAvailable,
130130
Status: metav1.ConditionFalse,
@@ -353,7 +353,7 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *
353353
return ctrl.Result{}, nil
354354
}
355355

356-
if err := finalizerutil.RemoveFinalizers(ctx, CluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
356+
if _, err := finalizerutil.UpdateFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev); err != nil {
357357
return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err)
358358
}
359359
return ctrl.Result{}, nil

internal/shared/util/finalizer/finalizer.go

Lines changed: 35 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -21,100 +21,58 @@ import (
2121
"fmt"
2222
"slices"
2323
"sort"
24+
"strings"
2425

2526
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
26-
"k8s.io/apimachinery/pkg/util/sets"
2727
"sigs.k8s.io/controller-runtime/pkg/client"
2828
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
29-
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3029
)
3130

32-
// AddFinalizers adds one or more finalizers to the object using server-side apply.
33-
// If all finalizers already exist, this is a no-op and returns (false, nil).
34-
// Returns (true, nil) if any finalizers were added.
31+
const (
32+
FinalizerPrefix = "olm.operatorframework.io/"
33+
)
34+
35+
// UpdateFinalizers sets the finalizers on an object to exactly the provided list using server-side apply.
36+
// If no finalizers are supplied, all finalizers will be removed from the object.
37+
// If one finalizer is supplied, all other finalizers will be removed and only the supplied one will remain.
38+
// Returns (true, nil) if the finalizers were changed, (false, nil) if they were already set to the desired value.
3539
// Note: This function will update the passed object with the server response.
36-
func AddFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) {
37-
if len(finalizers) == 0 {
38-
return false, nil
40+
func UpdateFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) {
41+
// Sort the desired finalizers for consistent ordering
42+
newFinalizers := slices.Clone(finalizers)
43+
if newFinalizers == nil {
44+
newFinalizers = []string{}
3945
}
40-
41-
// Determine which finalizers need to be added
42-
var toAdd []string
43-
for _, finalizer := range finalizers {
44-
if !controllerutil.ContainsFinalizer(obj, finalizer) {
45-
toAdd = append(toAdd, finalizer)
46+
// Possibly overkill, but it will ensure our finalizers use the proper prefix
47+
for _, s := range newFinalizers {
48+
if !strings.HasPrefix(s, FinalizerPrefix) {
49+
panic(fmt.Sprintf("finalizer does not have %q prefix: %q", FinalizerPrefix, s))
4650
}
4751
}
52+
sort.Strings(newFinalizers)
4853

49-
if len(toAdd) == 0 {
50-
return false, nil
51-
}
52-
53-
// Sort the finalizers to add for consistent ordering
54-
sort.Strings(toAdd)
55-
56-
// Create a copy of the finalizers list to avoid mutating the object
54+
// Check if the current finalizers already match the desired state
55+
// Remove any non-"olm.operatorframework.io" finalizers (ones we don't manage) from the list
5756
currentFinalizers := obj.GetFinalizers()
58-
newFinalizers := make([]string, len(currentFinalizers), len(currentFinalizers)+len(toAdd))
59-
copy(newFinalizers, currentFinalizers)
60-
newFinalizers = append(newFinalizers, toAdd...)
61-
62-
// Get the GVK for this object type
63-
gvk, err := apiutil.GVKForObject(obj, c.Scheme())
64-
if err != nil {
65-
return false, fmt.Errorf("getting object kind: %w", err)
66-
}
67-
68-
// Create an unstructured object for server-side apply
69-
u := &unstructured.Unstructured{}
70-
u.SetGroupVersionKind(gvk)
71-
u.SetName(obj.GetName())
72-
u.SetNamespace(obj.GetNamespace())
73-
u.SetFinalizers(newFinalizers)
74-
75-
// Use server-side apply to update finalizers
76-
if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner(owner)); err != nil {
77-
return false, fmt.Errorf("adding finalizer: %w", err)
78-
}
79-
80-
// Update the passed object with the new finalizers
81-
obj.SetFinalizers(u.GetFinalizers())
82-
obj.SetResourceVersion(u.GetResourceVersion())
83-
84-
return true, nil
85-
}
86-
87-
// RemoveFinalizers removes one or more finalizers from the object using server-side apply.
88-
// If none of the finalizers exist, this is a no-op.
89-
func RemoveFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) error {
90-
if len(finalizers) == 0 {
91-
return nil
92-
}
93-
94-
// Create a set of finalizers to remove for efficient lookup
95-
toRemove := sets.New(finalizers...)
96-
hasAny := false
97-
for _, finalizer := range finalizers {
98-
if controllerutil.ContainsFinalizer(obj, finalizer) {
99-
hasAny = true
100-
}
57+
currentSorted := slices.Clone(currentFinalizers)
58+
currentSorted = slices.DeleteFunc(currentSorted, func(f string) bool {
59+
return !strings.HasPrefix(f, FinalizerPrefix)
60+
})
61+
if currentSorted == nil {
62+
currentSorted = []string{}
10163
}
64+
sort.Strings(currentSorted)
10265

103-
if !hasAny {
104-
return nil
66+
// With only "olm.operatorframework.io" finalizers, other controller's finalizers
67+
// won't interfere in this check
68+
if slices.Equal(currentSorted, newFinalizers) {
69+
return false, nil
10570
}
10671

107-
// Create a copy of the finalizers list and remove the specified finalizers
108-
currentFinalizers := obj.GetFinalizers()
109-
newFinalizers := slices.Clone(currentFinalizers)
110-
newFinalizers = slices.DeleteFunc(newFinalizers, func(f string) bool {
111-
return toRemove.Has(f)
112-
})
113-
11472
// Get the GVK for this object type
11573
gvk, err := apiutil.GVKForObject(obj, c.Scheme())
11674
if err != nil {
117-
return fmt.Errorf("getting object kind: %w", err)
75+
return false, fmt.Errorf("getting object kind: %w", err)
11876
}
11977

12078
// Create an unstructured object for server-side apply
@@ -126,12 +84,12 @@ func RemoveFinalizers(ctx context.Context, owner string, c client.Client, obj cl
12684

12785
// Use server-side apply to update finalizers
12886
if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner(owner)); err != nil {
129-
return fmt.Errorf("removing finalizer: %w", err)
87+
return false, fmt.Errorf("updating finalizers: %w", err)
13088
}
13189

13290
// Update the passed object with the new finalizers
13391
obj.SetFinalizers(u.GetFinalizers())
13492
obj.SetResourceVersion(u.GetResourceVersion())
13593

136-
return nil
94+
return true, nil
13795
}

0 commit comments

Comments
 (0)