From 94ee52fd5d64f28d03417d857d2209a95f90dd0f Mon Sep 17 00:00:00 2001 From: z1cheng Date: Fri, 17 Nov 2023 14:51:11 +0800 Subject: [PATCH] refactor: replace runtime.Object with metav1.Object in pkg/util/finalizers Signed-off-by: z1cheng --- pkg/controllers/federate/controller.go | 14 ++---- .../federatedcluster/controller.go | 5 +- pkg/controllers/sync/controller.go | 31 +++++------- pkg/controllers/sync/dispatch/unmanaged.go | 29 +++--------- pkg/controllers/sync/resource.go | 15 ++---- pkg/util/finalizers/finalizers.go | 41 ++++++---------- pkg/util/finalizers/finalizers_test.go | 47 +++++++++---------- 7 files changed, 64 insertions(+), 118 deletions(-) diff --git a/pkg/controllers/federate/controller.go b/pkg/controllers/federate/controller.go index afaf30b8..ea296ce3 100644 --- a/pkg/controllers/federate/controller.go +++ b/pkg/controllers/federate/controller.go @@ -417,17 +417,14 @@ func (c *FederateController) ensureFinalizer( ) (*unstructured.Unstructured, error) { logger := klog.FromContext(ctx).WithValues("finalizer", FinalizerFederateController) - isUpdated, err := finalizersutil.AddFinalizers(sourceObj, sets.NewString(FinalizerFederateController)) - if err != nil { - return nil, fmt.Errorf("failed to add finalizer to source object: %w", err) - } + isUpdated := finalizersutil.AddFinalizers(sourceObj, sets.New(FinalizerFederateController)) if !isUpdated { return sourceObj, nil } logger.V(1).Info("Adding finalizer to source object") - sourceObj, err = c.dynamicClient.Resource(sourceGVR).Namespace(sourceObj.GetNamespace()).Update( + sourceObj, err := c.dynamicClient.Resource(sourceGVR).Namespace(sourceObj.GetNamespace()).Update( ctx, sourceObj, metav1.UpdateOptions{}, @@ -446,17 +443,14 @@ func (c *FederateController) removeFinalizer( ) (*unstructured.Unstructured, error) { logger := klog.FromContext(ctx).WithValues("finalizer", FinalizerFederateController) - isUpdated, err := finalizersutil.RemoveFinalizers(sourceObj, sets.NewString(FinalizerFederateController)) - if err != nil { - return nil, fmt.Errorf("failed to remove finalizer from source object: %w", err) - } + isUpdated := finalizersutil.RemoveFinalizers(sourceObj, sets.New(FinalizerFederateController)) if !isUpdated { return sourceObj, nil } logger.V(1).Info("Removing finalizer from source object") - sourceObj, err = c.dynamicClient.Resource(sourceGVR).Namespace(sourceObj.GetNamespace()).Update( + sourceObj, err := c.dynamicClient.Resource(sourceGVR).Namespace(sourceObj.GetNamespace()).Update( ctx, sourceObj, metav1.UpdateOptions{}, diff --git a/pkg/controllers/federatedcluster/controller.go b/pkg/controllers/federatedcluster/controller.go index b2e1e51b..013014a9 100644 --- a/pkg/controllers/federatedcluster/controller.go +++ b/pkg/controllers/federatedcluster/controller.go @@ -353,10 +353,7 @@ func (c *FederatedClusterController) ensureFinalizer( ctx context.Context, cluster *fedcorev1a1.FederatedCluster, ) (*fedcorev1a1.FederatedCluster, error) { - updated, err := finalizers.AddFinalizers(cluster, sets.NewString(FinalizerFederatedClusterController)) - if err != nil { - return nil, err - } + updated := finalizers.AddFinalizers(cluster, sets.New(FinalizerFederatedClusterController)) if updated { return c.fedClient.CoreV1alpha1().FederatedClusters().Update(ctx, cluster, metav1.UpdateOptions{}) diff --git a/pkg/controllers/sync/controller.go b/pkg/controllers/sync/controller.go index ab90598a..92c4354d 100644 --- a/pkg/controllers/sync/controller.go +++ b/pkg/controllers/sync/controller.go @@ -624,12 +624,7 @@ func (s *SyncController) syncToClusters( ) continue } - hasFinalizer, err := finalizersutil.HasFinalizer(cluster, FinalizerCascadingDelete) - if err != nil { - shouldRecheckAfterDispatch = true - dispatcher.RecordClusterError(fedcorev1a1.FinalizerCheckFailed, clusterName, err) - continue - } + hasFinalizer := finalizersutil.HasFinalizer(cluster, FinalizerCascadingDelete) if !hasFinalizer { // we should not sync before finalizer is added shouldRecheckAfterDispatch = true @@ -971,9 +966,9 @@ func (s *SyncController) ensureFinalizer(ctx context.Context, fedResource Federa ctx, keyedLogger := logging.InjectLoggerValues(ctx, "finalizer-name", FinalizerSyncController) obj := fedResource.Object() - isUpdated, err := finalizersutil.AddFinalizers(obj, sets.NewString(FinalizerSyncController)) - if err != nil || !isUpdated { - return err + isUpdated := finalizersutil.AddFinalizers(obj, sets.New(FinalizerSyncController)) + if !isUpdated { + return nil } keyedLogger.V(1).Info("Adding finalizer to federated object") @@ -995,9 +990,9 @@ func (s *SyncController) removeFinalizer(ctx context.Context, fedResource Federa ctx, keyedLogger := logging.InjectLoggerValues(ctx, "finalizer-name", FinalizerSyncController) obj := fedResource.Object() - isUpdated, err := finalizersutil.RemoveFinalizers(obj, sets.NewString(FinalizerSyncController)) - if err != nil || !isUpdated { - return err + isUpdated := finalizersutil.RemoveFinalizers(obj, sets.New(FinalizerSyncController)) + if !isUpdated { + return nil } keyedLogger.V(1).Info("Removing finalizer from federated object") @@ -1024,9 +1019,9 @@ func (s *SyncController) ensureClusterFinalizer(ctx context.Context, cluster *fe if err != nil { return err } - isUpdated, err := finalizersutil.AddFinalizers(cluster, sets.NewString(FinalizerCascadingDelete)) - if err != nil || !isUpdated { - return err + isUpdated := finalizersutil.AddFinalizers(cluster, sets.New(FinalizerCascadingDelete)) + if !isUpdated { + return nil } cluster, err = s.fedClient.CoreV1alpha1().FederatedClusters().Update(ctx, cluster, metav1.UpdateOptions{}) return err @@ -1046,9 +1041,9 @@ func (s *SyncController) removeClusterFinalizer(ctx context.Context, cluster *fe if err != nil { return err } - isUpdated, err := finalizersutil.RemoveFinalizers(cluster, sets.NewString(FinalizerCascadingDelete)) - if err != nil || !isUpdated { - return err + isUpdated := finalizersutil.RemoveFinalizers(cluster, sets.New(FinalizerCascadingDelete)) + if !isUpdated { + return nil } cluster, err = s.fedClient.CoreV1alpha1().FederatedClusters().Update(ctx, cluster, metav1.UpdateOptions{}) return err diff --git a/pkg/controllers/sync/dispatch/unmanaged.go b/pkg/controllers/sync/dispatch/unmanaged.go index c16aa8ff..be0bec62 100644 --- a/pkg/controllers/sync/dispatch/unmanaged.go +++ b/pkg/controllers/sync/dispatch/unmanaged.go @@ -132,18 +132,9 @@ func (d *unmanagedDispatcherImpl) Delete( // Avoid mutating the resource in the informer cache clusterObj := clusterObj.DeepCopy() - needUpdate, err := removeRetainObjectFinalizer(clusterObj) - if err != nil { - if d.recorder == nil { - wrappedErr := d.wrapOperationError(err, clusterName, op) - keyedLogger.Error(wrappedErr, "Failed to delete target object in cluster") - } else { - d.recorder.recordOperationError(ctx, fedcorev1a1.DeletionFailed, clusterName, op, err) - } - result = false - return result - } + needUpdate := removeRetainObjectFinalizer(clusterObj) if needUpdate { + var err error clusterObj, err = client.Resource(d.targetGVR).Namespace(targetName.Namespace).Update( ctx, clusterObj, @@ -177,7 +168,7 @@ func (d *unmanagedDispatcherImpl) Delete( // To avoid this, we explicitly set the PropagationPolicy to Background like `kubectl delete` does by default. // Ref: https://github.com/kubernetes/kubernetes/pull/65908 deletionPropagation := metav1.DeletePropagationBackground - err = client.Resource(d.targetGVR).Namespace(targetName.Namespace).Delete( + err := client.Resource(d.targetGVR).Namespace(targetName.Namespace).Delete( ctx, targetName.Name, metav1.DeleteOptions{PropagationPolicy: &deletionPropagation}, @@ -218,15 +209,7 @@ func (d *unmanagedDispatcherImpl) RemoveManagedLabel( updateObj := clusterObj.DeepCopy() managedlabel.RemoveManagedLabel(updateObj) - if _, err := removeRetainObjectFinalizer(updateObj); err != nil { - if d.recorder == nil { - wrappedErr := d.wrapOperationError(err, clusterName, op) - keyedLogger.Error(wrappedErr, "Failed to remove managed label from target object in cluster") - } else { - d.recorder.recordOperationError(ctx, fedcorev1a1.LabelRemovalFailed, clusterName, op, err) - } - return false - } + removeRetainObjectFinalizer(updateObj) var err error if _, err = client.Resource(d.targetGVR).Namespace(clusterObj.GetNamespace()).Update( @@ -258,6 +241,6 @@ func wrapOperationError(err error, operation, targetGVR, targetName, clusterName return errors.Wrapf(err, "Failed to "+eventTemplate, operation, targetGVR, targetName, clusterName) } -func removeRetainObjectFinalizer(obj *unstructured.Unstructured) (bool, error) { - return finalizers.RemoveFinalizers(obj, sets.NewString(RetainTerminatingObjectFinalizer)) +func removeRetainObjectFinalizer(obj *unstructured.Unstructured) bool { + return finalizers.RemoveFinalizers(obj, sets.New(RetainTerminatingObjectFinalizer)) } diff --git a/pkg/controllers/sync/resource.go b/pkg/controllers/sync/resource.go index 9d14ead3..07c1f0d1 100644 --- a/pkg/controllers/sync/resource.go +++ b/pkg/controllers/sync/resource.go @@ -177,9 +177,7 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured. return nil, err } - if err := addRetainObjectFinalizer(obj); err != nil { - return nil, err - } + addRetainObjectFinalizer(obj) case common.ServiceGVK: if err := dropServiceFields(obj); err != nil { return nil, err @@ -189,19 +187,14 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured. return nil, err } - if err := addRetainObjectFinalizer(obj); err != nil { - return nil, err - } + addRetainObjectFinalizer(obj) } return obj, nil } -func addRetainObjectFinalizer(obj *unstructured.Unstructured) error { - if _, err := finalizers.AddFinalizers(obj, sets.NewString(dispatch.RetainTerminatingObjectFinalizer)); err != nil { - return err - } - return nil +func addRetainObjectFinalizer(obj *unstructured.Unstructured) { + finalizers.AddFinalizers(obj, sets.New(dispatch.RetainTerminatingObjectFinalizer)) } func dropJobFields(obj *unstructured.Unstructured) error { diff --git a/pkg/util/finalizers/finalizers.go b/pkg/util/finalizers/finalizers.go index 2a711187..86c20efd 100644 --- a/pkg/util/finalizers/finalizers.go +++ b/pkg/util/finalizers/finalizers.go @@ -22,49 +22,36 @@ are Copyright 2023 The KubeAdmiral Authors. package finalizers import ( - meta "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" ) // HasFinalizer returns true if the given object has the given finalizer in its ObjectMeta. -func HasFinalizer(obj runtime.Object, finalizer string) (bool, error) { - accessor, err := meta.Accessor(obj) - if err != nil { - return false, err - } - finalizers := sets.NewString(accessor.GetFinalizers()...) - return finalizers.Has(finalizer), nil +func HasFinalizer(obj metav1.Object, finalizer string) bool { + finalizers := sets.New(obj.GetFinalizers()...) + return finalizers.Has(finalizer) } // AddFinalizers adds the given finalizers to the given objects ObjectMeta. // Returns true if the object was updated. -func AddFinalizers(obj runtime.Object, newFinalizers sets.String) (bool, error) { - accessor, err := meta.Accessor(obj) - if err != nil { - return false, err - } - oldFinalizers := sets.NewString(accessor.GetFinalizers()...) +func AddFinalizers(obj metav1.Object, newFinalizers sets.Set[string]) bool { + oldFinalizers := sets.New(obj.GetFinalizers()...) if oldFinalizers.IsSuperset(newFinalizers) { - return false, nil + return false } allFinalizers := oldFinalizers.Union(newFinalizers) - accessor.SetFinalizers(allFinalizers.List()) - return true, nil + obj.SetFinalizers(sets.List(allFinalizers)) + return true } // RemoveFinalizers removes the given finalizers from the given objects ObjectMeta. // Returns true if the object was updated. -func RemoveFinalizers(obj runtime.Object, finalizers sets.String) (bool, error) { - accessor, err := meta.Accessor(obj) - if err != nil { - return false, err - } - oldFinalizers := sets.NewString(accessor.GetFinalizers()...) +func RemoveFinalizers(obj metav1.Object, finalizers sets.Set[string]) bool { + oldFinalizers := sets.New(obj.GetFinalizers()...) if oldFinalizers.Intersection(finalizers).Len() == 0 { - return false, nil + return false } newFinalizers := oldFinalizers.Difference(finalizers) - accessor.SetFinalizers(newFinalizers.List()) - return true, nil + obj.SetFinalizers(sets.List(newFinalizers)) + return true } diff --git a/pkg/util/finalizers/finalizers_test.go b/pkg/util/finalizers/finalizers_test.go index fcaf0990..bbfa6578 100644 --- a/pkg/util/finalizers/finalizers_test.go +++ b/pkg/util/finalizers/finalizers_test.go @@ -26,12 +26,11 @@ import ( "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" - meta "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" ) -func newObj(finalizers []string) runtime.Object { +func newObj(finalizers []string) metav1.Object { pod := corev1.Pod{} pod.ObjectMeta.Finalizers = finalizers return &pod @@ -39,7 +38,7 @@ func newObj(finalizers []string) runtime.Object { func TestHasFinalizer(t *testing.T) { testCases := []struct { - obj runtime.Object + obj metav1.Object finalizer string result bool }{ @@ -75,7 +74,7 @@ func TestHasFinalizer(t *testing.T) { }, } for index, test := range testCases { - hasFinalizer, _ := HasFinalizer(test.obj, test.finalizer) + hasFinalizer := HasFinalizer(test.obj, test.finalizer) assert.Equal( t, hasFinalizer, @@ -87,52 +86,51 @@ func TestHasFinalizer(t *testing.T) { func TestAddFinalizers(t *testing.T) { testCases := []struct { - obj runtime.Object - finalizers sets.String + obj metav1.Object + finalizers sets.Set[string] isUpdated bool newFinalizers []string }{ { newObj([]string{}), - sets.NewString(), + sets.New[string](), false, []string{}, }, { newObj([]string{}), - sets.NewString("someFinalizer"), + sets.New("someFinalizer"), true, []string{"someFinalizer"}, }, { newObj([]string{"someFinalizer"}), - sets.NewString(), + sets.New[string](), false, []string{"someFinalizer"}, }, { newObj([]string{"someFinalizer"}), - sets.NewString("anotherFinalizer"), + sets.New("anotherFinalizer"), true, []string{"anotherFinalizer", "someFinalizer"}, }, { newObj([]string{"someFinalizer"}), - sets.NewString("someFinalizer"), + sets.New("someFinalizer"), false, []string{"someFinalizer"}, }, } for index, test := range testCases { - isUpdated, _ := AddFinalizers(test.obj, test.finalizers) + isUpdated := AddFinalizers(test.obj, test.finalizers) assert.Equal( t, isUpdated, test.isUpdated, fmt.Sprintf("Test case %d failed. Expected isUpdated: %v, actual: %v", index, test.isUpdated, isUpdated), ) - accessor, _ := meta.Accessor(test.obj) - newFinalizers := accessor.GetFinalizers() + newFinalizers := test.obj.GetFinalizers() assert.Equal( t, test.newFinalizers, @@ -149,52 +147,51 @@ func TestAddFinalizers(t *testing.T) { func TestRemoveFinalizers(t *testing.T) { testCases := []struct { - obj runtime.Object - finalizers sets.String + obj metav1.Object + finalizers sets.Set[string] isUpdated bool newFinalizers []string }{ { newObj([]string{}), - sets.NewString(), + sets.New[string](), false, []string{}, }, { newObj([]string{}), - sets.NewString("someFinalizer"), + sets.New("someFinalizer"), false, []string{}, }, { newObj([]string{"someFinalizer"}), - sets.NewString(), + sets.New[string](), false, []string{"someFinalizer"}, }, { newObj([]string{"someFinalizer"}), - sets.NewString("anotherFinalizer"), + sets.New("anotherFinalizer"), false, []string{"someFinalizer"}, }, { newObj([]string{"someFinalizer", "anotherFinalizer"}), - sets.NewString("someFinalizer"), + sets.New("someFinalizer"), true, []string{"anotherFinalizer"}, }, } for index, test := range testCases { - isUpdated, _ := RemoveFinalizers(test.obj, test.finalizers) + isUpdated := RemoveFinalizers(test.obj, test.finalizers) assert.Equal( t, isUpdated, test.isUpdated, fmt.Sprintf("Test case %d failed. Expected isUpdated: %v, actual: %v", index, test.isUpdated, isUpdated), ) - accessor, _ := meta.Accessor(test.obj) - newFinalizers := accessor.GetFinalizers() + newFinalizers := test.obj.GetFinalizers() assert.Equal( t, test.newFinalizers,