Federation: Removing duplicate finalizer manipulation logic in federation controllers

federation/pkg/federation-controller/configmap/configmap_controller.go

@@ -175,9 +175,7 @@ func NewConfigMapController(client federationclientset.Interface) *ConfigMapCont
 		})
 
 	configmapcontroller.deletionHelper = deletionhelper.NewDeletionHelper(
-		configmapcontroller.hasFinalizerFunc,
-		configmapcontroller.removeFinalizerFunc,
-		configmapcontroller.addFinalizerFunc,
+		configmapcontroller.updateConfigMap,
 		// objNameFunc
 		func(obj pkgruntime.Object) string {
 			configmap := obj.(*apiv1.ConfigMap)
@@ -192,50 +190,11 @@ func NewConfigMapController(client federationclientset.Interface) *ConfigMapCont
 	return configmapcontroller
 }
 
-// hasFinalizerFunc returns true if the given object has the given finalizer in its ObjectMeta.
-func (configmapcontroller *ConfigMapController) hasFinalizerFunc(obj pkgruntime.Object, finalizer string) bool {
+// Sends the given updated object to apiserver.
+// Assumes that the given object is a configmap.
+func (configmapcontroller *ConfigMapController) updateConfigMap(obj pkgruntime.Object) (pkgruntime.Object, error) {
 	configmap := obj.(*apiv1.ConfigMap)
-	for i := range configmap.ObjectMeta.Finalizers {
-		if string(configmap.ObjectMeta.Finalizers[i]) == finalizer {
-			return true
-		}
-	}
-	return false
-}
-
-// removeFinalizerFunc removes the given finalizers from the given objects ObjectMeta. Assumes that the given object is a configmap.
-func (configmapcontroller *ConfigMapController) removeFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
-	configmap := obj.(*apiv1.ConfigMap)
-	newFinalizers := []string{}
-	hasFinalizer := false
-	for i := range configmap.ObjectMeta.Finalizers {
-		if !deletionhelper.ContainsString(finalizers, configmap.ObjectMeta.Finalizers[i]) {
-			newFinalizers = append(newFinalizers, configmap.ObjectMeta.Finalizers[i])
-		} else {
-			hasFinalizer = true
-		}
-	}
-	if !hasFinalizer {
-		// Nothing to do.
-		return obj, nil
-	}
-	configmap.ObjectMeta.Finalizers = newFinalizers
-	configmap, err := configmapcontroller.federatedApiClient.Core().ConfigMaps(configmap.Namespace).Update(configmap)
-	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizers %v from configmap %s: %v", finalizers, configmap.Name, err)
-	}
-	return configmap, nil
-}
-
-// addFinalizerFunc adds the given finalizer to the given objects ObjectMeta. Assumes that the given object is a configmap.
-func (configmapcontroller *ConfigMapController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
-	configmap := obj.(*apiv1.ConfigMap)
-	configmap.ObjectMeta.Finalizers = append(configmap.ObjectMeta.Finalizers, finalizers...)
-	configmap, err := configmapcontroller.federatedApiClient.Core().ConfigMaps(configmap.Namespace).Update(configmap)
-	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizers %v to configmap %s: %v", finalizers, configmap.Name, err)
-	}
-	return configmap, nil
+	return configmapcontroller.federatedApiClient.Core().ConfigMaps(configmap.Namespace).Update(configmap)
 }
 
 func (configmapcontroller *ConfigMapController) Run(stopChan <-chan struct{}) {

federation/pkg/federation-controller/configmap/configmap_controller_test.go

@@ -103,8 +103,8 @@ func TestConfigMapController(t *testing.T) {
 	configmapWatch.Add(configmap1)
 	// There should be 2 updates to add both the finalizers.
 	updatedConfigMap := GetConfigMapFromChan(configmapUpdateChan)
-	assert.True(t, configmapController.hasFinalizerFunc(updatedConfigMap, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
-	assert.True(t, configmapController.hasFinalizerFunc(updatedConfigMap, metav1.FinalizerOrphanDependents))
+	AssertHasFinalizer(t, updatedConfigMap, deletionhelper.FinalizerDeleteFromUnderlyingClusters)
+	AssertHasFinalizer(t, updatedConfigMap, metav1.FinalizerOrphanDependents)
 
 	// Verify that the configmap is created in underlying cluster1.
 	createdConfigMap := GetConfigMapFromChan(cluster1CreateChan)

federation/pkg/federation-controller/daemonset/daemonset_controller.go

@@ -193,9 +193,7 @@ func NewDaemonSetController(client federationclientset.Interface) *DaemonSetCont
 		})
 
 	daemonsetcontroller.deletionHelper = deletionhelper.NewDeletionHelper(
-		daemonsetcontroller.hasFinalizerFunc,
-		daemonsetcontroller.removeFinalizerFunc,
-		daemonsetcontroller.addFinalizerFunc,
+		daemonsetcontroller.updateDaemonSet,
 		// objNameFunc
 		func(obj pkgruntime.Object) string {
 			daemonset := obj.(*extensionsv1.DaemonSet)
@@ -210,52 +208,11 @@ func NewDaemonSetController(client federationclientset.Interface) *DaemonSetCont
 	return daemonsetcontroller
 }
 
-// Returns true if the given object has the given finalizer in its ObjectMeta.
-func (daemonsetcontroller *DaemonSetController) hasFinalizerFunc(obj pkgruntime.Object, finalizer string) bool {
-	daemonset := obj.(*extensionsv1.DaemonSet)
-	for i := range daemonset.ObjectMeta.Finalizers {
-		if string(daemonset.ObjectMeta.Finalizers[i]) == finalizer {
-			return true
-		}
-	}
-	return false
-}
-
-// Removes the finalizers from the given objects ObjectMeta.
+// Sends the given updated object to apiserver.
 // Assumes that the given object is a daemonset.
-func (daemonsetcontroller *DaemonSetController) removeFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
+func (daemonsetcontroller *DaemonSetController) updateDaemonSet(obj pkgruntime.Object) (pkgruntime.Object, error) {
 	daemonset := obj.(*extensionsv1.DaemonSet)
-	newFinalizers := []string{}
-	hasFinalizer := false
-	for i := range daemonset.ObjectMeta.Finalizers {
-		if !deletionhelper.ContainsString(finalizers, daemonset.ObjectMeta.Finalizers[i]) {
-			newFinalizers = append(newFinalizers, daemonset.ObjectMeta.Finalizers[i])
-		} else {
-			hasFinalizer = true
-		}
-	}
-	if !hasFinalizer {
-		// Nothing to do.
-		return obj, nil
-	}
-	daemonset.ObjectMeta.Finalizers = newFinalizers
-	daemonset, err := daemonsetcontroller.federatedApiClient.Extensions().DaemonSets(daemonset.Namespace).Update(daemonset)
-	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizers %v from daemonset %s: %v", finalizers, daemonset.Name, err)
-	}
-	return daemonset, nil
-}
-
-// Adds the given finalizers to the given objects ObjectMeta.
-// Assumes that the given object is a daemonset.
-func (daemonsetcontroller *DaemonSetController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
-	daemonset := obj.(*extensionsv1.DaemonSet)
-	daemonset.ObjectMeta.Finalizers = append(daemonset.ObjectMeta.Finalizers, finalizers...)
-	daemonset, err := daemonsetcontroller.federatedApiClient.Extensions().DaemonSets(daemonset.Namespace).Update(daemonset)
-	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizers %v to daemonset %s: %v", finalizers, daemonset.Name, err)
-	}
-	return daemonset, nil
+	return daemonsetcontroller.federatedApiClient.Extensions().DaemonSets(daemonset.Namespace).Update(daemonset)
 }
 
 func (daemonsetcontroller *DaemonSetController) Run(stopChan <-chan struct{}) {

federation/pkg/federation-controller/daemonset/daemonset_controller_test.go

@@ -104,8 +104,8 @@ func TestDaemonSetController(t *testing.T) {
 
 	// There should be an update to add both the finalizers.
 	updatedDaemonSet := GetDaemonSetFromChan(daemonsetUpdateChan)
-	assert.True(t, daemonsetController.hasFinalizerFunc(updatedDaemonSet, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
-	assert.True(t, daemonsetController.hasFinalizerFunc(updatedDaemonSet, metav1.FinalizerOrphanDependents))
+	AssertHasFinalizer(t, updatedDaemonSet, deletionhelper.FinalizerDeleteFromUnderlyingClusters)
+	AssertHasFinalizer(t, updatedDaemonSet, metav1.FinalizerOrphanDependents)
 	daemonset1 = *updatedDaemonSet
 
 	createdDaemonSet := GetDaemonSetFromChan(cluster1CreateChan)

federation/pkg/federation-controller/deployment/deploymentcontroller.go

@@ -207,9 +207,7 @@ func NewDeploymentController(federationClient fedclientset.Interface) *Deploymen
 		})
 
 	fdc.deletionHelper = deletionhelper.NewDeletionHelper(
-		fdc.hasFinalizerFunc,
-		fdc.removeFinalizerFunc,
-		fdc.addFinalizerFunc,
+		fdc.updateDeployment,
 		// objNameFunc
 		func(obj runtime.Object) string {
 			deployment := obj.(*extensionsv1.Deployment)
@@ -224,52 +222,11 @@ func NewDeploymentController(federationClient fedclientset.Interface) *Deploymen
 	return fdc
 }
 
-// Returns true if the given object has the given finalizer in its ObjectMeta.
-func (fdc *DeploymentController) hasFinalizerFunc(obj runtime.Object, finalizer string) bool {
-	deployment := obj.(*extensionsv1.Deployment)
-	for i := range deployment.ObjectMeta.Finalizers {
-		if string(deployment.ObjectMeta.Finalizers[i]) == finalizer {
-			return true
-		}
-	}
-	return false
-}
-
-// Removes the finalizers from the given objects ObjectMeta.
+// Sends the given updated object to apiserver.
 // Assumes that the given object is a deployment.
-func (fdc *DeploymentController) removeFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
+func (fdc *DeploymentController) updateDeployment(obj runtime.Object) (runtime.Object, error) {
 	deployment := obj.(*extensionsv1.Deployment)
-	newFinalizers := []string{}
-	hasFinalizer := false
-	for i := range deployment.ObjectMeta.Finalizers {
-		if !deletionhelper.ContainsString(finalizers, deployment.ObjectMeta.Finalizers[i]) {
-			newFinalizers = append(newFinalizers, deployment.ObjectMeta.Finalizers[i])
-		} else {
-			hasFinalizer = true
-		}
-	}
-	if !hasFinalizer {
-		// Nothing to do.
-		return obj, nil
-	}
-	deployment.ObjectMeta.Finalizers = newFinalizers
-	deployment, err := fdc.fedClient.Extensions().Deployments(deployment.Namespace).Update(deployment)
-	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizers %v from deployment %s: %v", finalizers, deployment.Name, err)
-	}
-	return deployment, nil
-}
-
-// Adds the given finalizers to the given objects ObjectMeta.
-// Assumes that the given object is a deployment.
-func (fdc *DeploymentController) addFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
-	deployment := obj.(*extensionsv1.Deployment)
-	deployment.ObjectMeta.Finalizers = append(deployment.ObjectMeta.Finalizers, finalizers...)
-	deployment, err := fdc.fedClient.Extensions().Deployments(deployment.Namespace).Update(deployment)
-	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizers %v to deployment %s: %v", finalizers, deployment.Name, err)
-	}
-	return deployment, nil
+	return fdc.fedClient.Extensions().Deployments(deployment.Namespace).Update(deployment)
 }
 
 func (fdc *DeploymentController) Run(workers int, stopCh <-chan struct{}) {

federation/pkg/federation-controller/ingress/BUILD

@@ -47,6 +47,7 @@ go_test(
         "//federation/client/clientset_generated/federation_clientset/fake:go_default_library",
         "//federation/pkg/federation-controller/util:go_default_library",
         "//federation/pkg/federation-controller/util/deletionhelper:go_default_library",
+        "//federation/pkg/federation-controller/util/finalizers:go_default_library",
         "//federation/pkg/federation-controller/util/test:go_default_library",
         "//pkg/api/v1:go_default_library",
         "//pkg/apis/extensions/v1beta1:go_default_library",

federation/pkg/federation-controller/ingress/ingress_controller.go

@@ -290,9 +290,7 @@ func NewIngressController(client federationclientset.Interface) *IngressControll
 		})
 
 	ic.deletionHelper = deletionhelper.NewDeletionHelper(
-		ic.hasFinalizerFunc,
-		ic.removeFinalizerFunc,
-		ic.addFinalizerFunc,
+		ic.updateIngress,
 		// objNameFunc
 		func(obj pkgruntime.Object) string {
 			ingress := obj.(*extensionsv1beta1.Ingress)
@@ -306,52 +304,11 @@ func NewIngressController(client federationclientset.Interface) *IngressControll
 	return ic
 }
 
-// Returns true if the given object has the given finalizer in its ObjectMeta.
-func (ic *IngressController) hasFinalizerFunc(obj pkgruntime.Object, finalizer string) bool {
+// Sends the given updated object to apiserver.
+// Assumes that the given object is an ingress.
+func (ic *IngressController) updateIngress(obj pkgruntime.Object) (pkgruntime.Object, error) {
 	ingress := obj.(*extensionsv1beta1.Ingress)
-	for i := range ingress.ObjectMeta.Finalizers {
-		if string(ingress.ObjectMeta.Finalizers[i]) == finalizer {
-			return true
-		}
-	}
-	return false
-}
-
-// Removes the finalizers from the given objects ObjectMeta.
-// Assumes that the given object is a ingress.
-func (ic *IngressController) removeFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
-	ingress := obj.(*extensionsv1beta1.Ingress)
-	newFinalizers := []string{}
-	hasFinalizer := false
-	for i := range ingress.ObjectMeta.Finalizers {
-		if !deletionhelper.ContainsString(finalizers, ingress.ObjectMeta.Finalizers[i]) {
-			newFinalizers = append(newFinalizers, ingress.ObjectMeta.Finalizers[i])
-		} else {
-			hasFinalizer = true
-		}
-	}
-	if !hasFinalizer {
-		// Nothing to do.
-		return obj, nil
-	}
-	ingress.ObjectMeta.Finalizers = newFinalizers
-	ingress, err := ic.federatedApiClient.Extensions().Ingresses(ingress.Namespace).Update(ingress)
-	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizers %v from ingress %s: %v", finalizers, ingress.Name, err)
-	}
-	return ingress, nil
-}
-
-// Adds the given finalizers to the given objects ObjectMeta.
-// Assumes that the given object is a ingress.
-func (ic *IngressController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
-	ingress := obj.(*extensionsv1beta1.Ingress)
-	ingress.ObjectMeta.Finalizers = append(ingress.ObjectMeta.Finalizers, finalizers...)
-	ingress, err := ic.federatedApiClient.Extensions().Ingresses(ingress.Namespace).Update(ingress)
-	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizers %v to ingress %s: %v", finalizers, ingress.Name, err)
-	}
-	return ingress, nil
+	return ic.federatedApiClient.Extensions().Ingresses(ingress.Namespace).Update(ingress)
 }
 
 func (ic *IngressController) Run(stopChan <-chan struct{}) {

federation/pkg/federation-controller/ingress/ingress_controller_test.go

@@ -32,6 +32,7 @@ import (
 	fakefedclientset "k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/fake"
 	"k8s.io/kubernetes/federation/pkg/federation-controller/util"
 	"k8s.io/kubernetes/federation/pkg/federation-controller/util/deletionhelper"
+	finalizersutil "k8s.io/kubernetes/federation/pkg/federation-controller/util/finalizers"
 	. "k8s.io/kubernetes/federation/pkg/federation-controller/util/test"
 	apiv1 "k8s.io/kubernetes/pkg/api/v1"
 	extensionsv1beta1 "k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
@@ -160,8 +161,8 @@ func TestIngressController(t *testing.T) {
 	t.Log("Checking that appropriate finalizers are added")
 	// There should be an update to add both the finalizers.
 	updatedIngress := GetIngressFromChan(t, fedIngressUpdateChan)
-	assert.True(t, ingressController.hasFinalizerFunc(updatedIngress, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
-	assert.True(t, ingressController.hasFinalizerFunc(updatedIngress, metav1.FinalizerOrphanDependents), fmt.Sprintf("ingress does not have the orphan finalizer: %v", updatedIngress))
+	AssertHasFinalizer(t, updatedIngress, deletionhelper.FinalizerDeleteFromUnderlyingClusters)
+	AssertHasFinalizer(t, updatedIngress, metav1.FinalizerOrphanDependents)
 	fedIngress = *updatedIngress
 
 	t.Log("Checking that Ingress was correctly created in cluster 1")
@@ -333,8 +334,15 @@ func WaitForFinalizersInFederationStore(ingressController *IngressController, st
 			return false, err
 		}
 		ingress := obj.(*extensionsv1beta1.Ingress)
-		if ingressController.hasFinalizerFunc(ingress, metav1.FinalizerOrphanDependents) &&
-			ingressController.hasFinalizerFunc(ingress, deletionhelper.FinalizerDeleteFromUnderlyingClusters) {
+		hasOrphanFinalizer, err := finalizersutil.HasFinalizer(ingress, metav1.FinalizerOrphanDependents)
+		if err != nil {
+			return false, err
+		}
+		hasDeleteFinalizer, err := finalizersutil.HasFinalizer(ingress, deletionhelper.FinalizerDeleteFromUnderlyingClusters)
+		if err != nil {
+			return false, err
+		}
+		if hasOrphanFinalizer && hasDeleteFinalizer {
 			return true, nil
 		}
 		return false, nil

federation/pkg/federation-controller/namespace/BUILD

@@ -52,6 +52,7 @@ go_test(
         "//pkg/client/clientset_generated/clientset:go_default_library",
         "//pkg/client/clientset_generated/clientset/fake:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",