Merge pull request #237 from a-hilaly/static-finalizer

remove UUID-based finalizers in favor of static string

Author: a-hilaly

api/v1alpha1/zz_generated.deepcopy.go

@@ -394,7 +394,7 @@ func (igr *instanceGraphReconciler) finalizeDeletion(ctx context.Context) error
 
 	// Remove finalizer from instance
 	instance := igr.runtime.GetInstance()
-	patched, err := igr.setUnmanaged(ctx, instance, instance.GetUID())
+	patched, err := igr.setUnmanaged(ctx, instance)
 	if err != nil {
 		return fmt.Errorf("failed to remove instance finalizer: %w", err)
 	}
@@ -405,14 +405,14 @@ func (igr *instanceGraphReconciler) finalizeDeletion(ctx context.Context) error
 
 // setManaged ensures the instance has the necessary finalizer and labels.
 func (igr *instanceGraphReconciler) setManaged(ctx context.Context, obj *unstructured.Unstructured, uid types.UID) (*unstructured.Unstructured, error) {
-	if exist, _ := metadata.HasInstanceFinalizerUnstructured(obj, uid); exist {
+	if exist, _ := metadata.HasInstanceFinalizerUnstructured(obj); exist {
 		return obj, nil
 	}
 
 	igr.log.V(1).Info("Setting managed state", "name", obj.GetName(), "namespace", obj.GetNamespace())
 
 	copy := obj.DeepCopy()
-	if err := metadata.SetInstanceFinalizerUnstructured(copy, uid); err != nil {
+	if err := metadata.SetInstanceFinalizerUnstructured(copy); err != nil {
 		return nil, fmt.Errorf("failed to set finalizer: %w", err)
 	}
 
@@ -429,15 +429,15 @@ func (igr *instanceGraphReconciler) setManaged(ctx context.Context, obj *unstruc
 }
 
 // setUnmanaged removes the finalizer from the instance.
-func (igr *instanceGraphReconciler) setUnmanaged(ctx context.Context, obj *unstructured.Unstructured, uid types.UID) (*unstructured.Unstructured, error) {
-	if exist, _ := metadata.HasInstanceFinalizerUnstructured(obj, uid); !exist {
+func (igr *instanceGraphReconciler) setUnmanaged(ctx context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	if exist, _ := metadata.HasInstanceFinalizerUnstructured(obj); !exist {
 		return obj, nil
 	}
 
 	igr.log.V(1).Info("Removing managed state", "name", obj.GetName(), "namespace", obj.GetNamespace())
 
 	copy := obj.DeepCopy()
-	if err := metadata.RemoveInstanceFinalizerUnstructured(copy, uid); err != nil {
+	if err := metadata.RemoveInstanceFinalizerUnstructured(copy); err != nil {
 		return nil, fmt.Errorf("failed to remove finalizer: %w", err)
 	}
 

pkg/controller/instance/controller_reconcile.go

@@ -18,7 +18,6 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/kro-run/kro/api/v1alpha1"
 )
@@ -42,36 +41,15 @@ func HasResourceGraphDefinitionFinalizer(obj metav1.Object) bool {
 	return containsString(obj.GetFinalizers(), kroFinalizer)
 }
 
-// SetInstanceFinalizer adds an instance-specific finalizer to the object.
-func SetInstanceFinalizer(obj metav1.Object, uid types.UID) {
-	finalizerName := getInstanceFinalizerName(uid)
-	if !HasInstanceFinalizer(obj, uid) {
-		obj.SetFinalizers(append(obj.GetFinalizers(), finalizerName))
-	}
-}
-
-// RemoveInstanceFinalizer removes an instance-specific finalizer from the object.
-func RemoveInstanceFinalizer(obj metav1.Object, uid types.UID) {
-	finalizerName := getInstanceFinalizerName(uid)
-	obj.SetFinalizers(removeString(obj.GetFinalizers(), finalizerName))
-}
-
-// HasInstanceFinalizer checks if the object has an instance-specific finalizer.
-func HasInstanceFinalizer(obj metav1.Object, uid types.UID) bool {
-	finalizerName := getInstanceFinalizerName(uid)
-	return containsString(obj.GetFinalizers(), finalizerName)
-}
-
 // SetInstanceFinalizerUnstructured adds an instance-specific finalizer to an unstructured object.
-func SetInstanceFinalizerUnstructured(obj *unstructured.Unstructured, uid types.UID) error {
-	finalizerName := getInstanceFinalizerName(uid)
+func SetInstanceFinalizerUnstructured(obj *unstructured.Unstructured) error {
 	finalizers, found, err := unstructured.NestedStringSlice(obj.Object, "metadata", "finalizers")
 	if err != nil {
 		return fmt.Errorf("error getting finalizers: %w", err)
 	}
 
-	if !found || !containsString(finalizers, finalizerName) {
-		finalizers = append(finalizers, finalizerName)
+	if !found || !containsString(finalizers, kroFinalizer) {
+		finalizers = append(finalizers, kroFinalizer)
 		if err := unstructured.SetNestedStringSlice(obj.Object, finalizers, "metadata", "finalizers"); err != nil {
 			return fmt.Errorf("error setting finalizers: %w", err)
 		}
@@ -80,15 +58,14 @@ func SetInstanceFinalizerUnstructured(obj *unstructured.Unstructured, uid types.
 }
 
 // RemoveInstanceFinalizerUnstructured removes an instance-specific finalizer from an unstructured object.
-func RemoveInstanceFinalizerUnstructured(obj *unstructured.Unstructured, uid types.UID) error {
-	finalizerName := getInstanceFinalizerName(uid)
+func RemoveInstanceFinalizerUnstructured(obj *unstructured.Unstructured) error {
 	finalizers, found, err := unstructured.NestedStringSlice(obj.Object, "metadata", "finalizers")
 	if err != nil {
 		return fmt.Errorf("error getting finalizers: %w", err)
 	}
 
 	if found {
-		finalizers = removeString(finalizers, finalizerName)
+		finalizers = removeString(finalizers, kroFinalizer)
 		if err := unstructured.SetNestedStringSlice(obj.Object, finalizers, "metadata", "finalizers"); err != nil {
 			return fmt.Errorf("error setting finalizers: %w", err)
 		}
@@ -97,8 +74,7 @@ func RemoveInstanceFinalizerUnstructured(obj *unstructured.Unstructured, uid typ
 }
 
 // HasInstanceFinalizerUnstructured checks if an unstructured object has an instance-specific finalizer.
-func HasInstanceFinalizerUnstructured(obj *unstructured.Unstructured, uid types.UID) (bool, error) {
-	finalizerName := getInstanceFinalizerName(uid)
+func HasInstanceFinalizerUnstructured(obj *unstructured.Unstructured) (bool, error) {
 	finalizers, found, err := unstructured.NestedStringSlice(obj.Object, "metadata", "finalizers")
 	if err != nil {
 		return false, fmt.Errorf("error getting finalizers: %w", err)
@@ -108,15 +84,11 @@ func HasInstanceFinalizerUnstructured(obj *unstructured.Unstructured, uid types.
 		return false, nil
 	}
 
-	return containsString(finalizers, finalizerName), nil
+	return containsString(finalizers, kroFinalizer), nil
 }
 
 // Helper functions
 
-func getInstanceFinalizerName(uid types.UID) string {
-	return fmt.Sprintf("%s.%s", string(uid), kroFinalizer)
-}
-
 func containsString(slice []string, s string) bool {
 	for _, item := range slice {
 		if item == s {

pkg/metadata/finalizers.go

@@ -19,7 +19,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/types"
 )
 
 func TestResourceGraphDefinitionFinalizer(t *testing.T) {
@@ -68,60 +67,12 @@ func TestResourceGraphDefinitionFinalizer(t *testing.T) {
 	}
 }
 
-func TestInstanceFinalizer(t *testing.T) {
-	uid := types.UID("test-uid")
-	cases := []struct {
-		name          string
-		initialObject metav1.Object
-		operation     func(metav1.Object, types.UID)
-		check         func(metav1.Object, types.UID) bool
-		expected      bool
-	}{
-		{
-			name:          "Set instance finalizer on objet w/o finalizers",
-			initialObject: &metav1.ObjectMeta{},
-			operation:     SetInstanceFinalizer,
-			check:         HasInstanceFinalizer,
-			expected:      true,
-		},
-		{
-			name:          "Set instanc finalizer on object w/ existing finalizers",
-			initialObject: &metav1.ObjectMeta{Finalizers: []string{"some-other-finalizer"}},
-			operation:     SetInstanceFinalizer,
-			check:         HasInstanceFinalizer,
-			expected:      true,
-		},
-		{
-			name:          "Remove instance finalizer from object that has it",
-			initialObject: &metav1.ObjectMeta{Finalizers: []string{getInstanceFinalizerName(uid)}},
-			operation:     RemoveInstanceFinalizer,
-			check:         HasInstanceFinalizer,
-			expected:      false,
-		},
-		{
-			name:          "Try to remove instance finalizer when its not present",
-			initialObject: &metav1.ObjectMeta{Finalizers: []string{"some-other-finalizer"}},
-			operation:     RemoveInstanceFinalizer,
-			check:         HasInstanceFinalizer,
-			expected:      false,
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			tc.operation(tc.initialObject, uid)
-			assert.Equal(t, tc.expected, tc.check(tc.initialObject, uid))
-		})
-	}
-}
-
 func TestInstanceFinalizerUnstructured(t *testing.T) {
-	uid := types.UID("test-uid")
 	cases := []struct {
 		name          string
 		initialObject *unstructured.Unstructured
-		operation     func(*unstructured.Unstructured, types.UID) error
-		check         func(*unstructured.Unstructured, types.UID) (bool, error)
+		operation     func(*unstructured.Unstructured) error
+		check         func(*unstructured.Unstructured) (bool, error)
 		expected      bool
 		expectError   bool
 	}{
@@ -154,7 +105,7 @@ func TestInstanceFinalizerUnstructured(t *testing.T) {
 			initialObject: &unstructured.Unstructured{
 				Object: map[string]interface{}{
 					"metadata": map[string]interface{}{
-						"finalizers": []interface{}{getInstanceFinalizerName(uid)},
+						"finalizers": []interface{}{kroFinalizer},
 					},
 				},
 			},
@@ -179,12 +130,12 @@ func TestInstanceFinalizerUnstructured(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := tc.operation(tc.initialObject, uid)
+			err := tc.operation(tc.initialObject)
 			if tc.expectError {
 				assert.Error(t, err)
 			} else {
 				assert.NoError(t, err)
-				hasF, err := tc.check(tc.initialObject, uid)
+				hasF, err := tc.check(tc.initialObject)
 				assert.NoError(t, err)
 				assert.Equal(t, tc.expected, hasF)
 			}