Merge pull request #44084 from nikhiljindal/removeFinalizers

Automatic merge from submit-queue (batch tested with PRs 44084, 42964)

Removing both finalizers in federation controllers in a single update

Fixes https://github.com/kubernetes/kubernetes/issues/43828

There is a bug right now where the controller fails to delete the object if one finalizer is removed and the second isnt.
This updates the code so that both the finalizers are removed in a single API call. Kept the code changes minimum to enable cherrypick in 1.6.x

cc @csbell @kubernetes/sig-federation-bugs

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

@@ -202,13 +202,13 @@ func (configmapcontroller *ConfigMapController) hasFinalizerFunc(obj pkgruntime.
 	return false
 }
 
-// removeFinalizerFunc removes the finalizer from the given objects ObjectMeta. Assumes that the given object is a configmap.
+// 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, finalizer string) (pkgruntime.Object, error) {
+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 string(configmap.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, configmap.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, configmap.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -221,7 +221,7 @@ func (configmapcontroller *ConfigMapController) removeFinalizerFunc(obj pkgrunti
 	configmap.ObjectMeta.Finalizers = newFinalizers
 	configmap, err := configmapcontroller.federatedApiClient.Core().ConfigMaps(configmap.Namespace).Update(configmap)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from configmap %s: %v", finalizer, configmap.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from configmap %s: %v", finalizers, configmap.Name, err)
 	}
 	return configmap, nil
 }

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

@@ -220,14 +220,14 @@ func (daemonsetcontroller *DaemonSetController) hasFinalizerFunc(obj pkgruntime.
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a daemonset.
-func (daemonsetcontroller *DaemonSetController) removeFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (daemonsetcontroller *DaemonSetController) removeFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	daemonset := obj.(*extensionsv1.DaemonSet)
 	newFinalizers := []string{}
 	hasFinalizer := false
 	for i := range daemonset.ObjectMeta.Finalizers {
-		if string(daemonset.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, daemonset.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, daemonset.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -240,7 +240,7 @@ func (daemonsetcontroller *DaemonSetController) removeFinalizerFunc(obj pkgrunti
 	daemonset.ObjectMeta.Finalizers = newFinalizers
 	daemonset, err := daemonsetcontroller.federatedApiClient.Extensions().DaemonSets(daemonset.Namespace).Update(daemonset)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from daemonset %s: %v", finalizer, daemonset.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from daemonset %s: %v", finalizers, daemonset.Name, err)
 	}
 	return daemonset, nil
 }

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

@@ -234,14 +234,14 @@ func (fdc *DeploymentController) hasFinalizerFunc(obj runtime.Object, finalizer
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a deployment.
-func (fdc *DeploymentController) removeFinalizerFunc(obj runtime.Object, finalizer string) (runtime.Object, error) {
+func (fdc *DeploymentController) removeFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
 	deployment := obj.(*extensionsv1.Deployment)
 	newFinalizers := []string{}
 	hasFinalizer := false
 	for i := range deployment.ObjectMeta.Finalizers {
-		if string(deployment.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, deployment.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, deployment.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -254,7 +254,7 @@ func (fdc *DeploymentController) removeFinalizerFunc(obj runtime.Object, finaliz
 	deployment.ObjectMeta.Finalizers = newFinalizers
 	deployment, err := fdc.fedClient.Extensions().Deployments(deployment.Namespace).Update(deployment)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from deployment %s: %v", finalizer, deployment.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from deployment %s: %v", finalizers, deployment.Name, err)
 	}
 	return deployment, nil
 }

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

@@ -316,14 +316,14 @@ func (ic *IngressController) hasFinalizerFunc(obj pkgruntime.Object, finalizer s
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a ingress.
-func (ic *IngressController) removeFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+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 string(ingress.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, ingress.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, ingress.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -336,7 +336,7 @@ func (ic *IngressController) removeFinalizerFunc(obj pkgruntime.Object, finalize
 	ingress.ObjectMeta.Finalizers = newFinalizers
 	ingress, err := ic.federatedApiClient.Extensions().Ingresses(ingress.Namespace).Update(ingress)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from ingress %s: %v", finalizer, ingress.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from ingress %s: %v", finalizers, ingress.Name, err)
 	}
 	return ingress, nil
 }

federation/pkg/federation-controller/namespace/namespace_controller.go

@@ -210,14 +210,14 @@ func (nc *NamespaceController) hasFinalizerFunc(obj runtime.Object, finalizer st
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a namespace.
-func (nc *NamespaceController) removeFinalizerFunc(obj runtime.Object, finalizer string) (runtime.Object, error) {
+func (nc *NamespaceController) removeFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
 	namespace := obj.(*apiv1.Namespace)
 	newFinalizers := []string{}
 	hasFinalizer := false
 	for i := range namespace.ObjectMeta.Finalizers {
-		if string(namespace.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, namespace.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, namespace.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -230,7 +230,7 @@ func (nc *NamespaceController) removeFinalizerFunc(obj runtime.Object, finalizer
 	namespace.ObjectMeta.Finalizers = newFinalizers
 	namespace, err := nc.federatedApiClient.Core().Namespaces().Update(namespace)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from namespace %s: %v", finalizer, namespace.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from namespace %s: %v", finalizers, namespace.Name, err)
 	}
 	return namespace, nil
 }

federation/pkg/federation-controller/replicaset/replicasetcontroller.go

@@ -242,14 +242,14 @@ func (frsc *ReplicaSetController) hasFinalizerFunc(obj runtime.Object, finalizer
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a replicaset.
-func (frsc *ReplicaSetController) removeFinalizerFunc(obj runtime.Object, finalizer string) (runtime.Object, error) {
+func (frsc *ReplicaSetController) removeFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
 	replicaset := obj.(*extensionsv1.ReplicaSet)
 	newFinalizers := []string{}
 	hasFinalizer := false
 	for i := range replicaset.ObjectMeta.Finalizers {
-		if string(replicaset.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, replicaset.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, replicaset.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -262,7 +262,7 @@ func (frsc *ReplicaSetController) removeFinalizerFunc(obj runtime.Object, finali
 	replicaset.ObjectMeta.Finalizers = newFinalizers
 	replicaset, err := frsc.fedClient.Extensions().ReplicaSets(replicaset.Namespace).Update(replicaset)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from replicaset %s: %v", finalizer, replicaset.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from replicaset %s: %v", finalizers, replicaset.Name, err)
 	}
 	return replicaset, nil
 }

federation/pkg/federation-controller/secret/secret_controller.go

@@ -201,14 +201,14 @@ func (secretcontroller *SecretController) hasFinalizerFunc(obj pkgruntime.Object
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a secret.
-func (secretcontroller *SecretController) removeFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (secretcontroller *SecretController) removeFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	secret := obj.(*apiv1.Secret)
 	newFinalizers := []string{}
 	hasFinalizer := false
 	for i := range secret.ObjectMeta.Finalizers {
-		if string(secret.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, secret.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, secret.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -221,7 +221,7 @@ func (secretcontroller *SecretController) removeFinalizerFunc(obj pkgruntime.Obj
 	secret.ObjectMeta.Finalizers = newFinalizers
 	secret, err := secretcontroller.federatedApiClient.Core().Secrets(secret.Namespace).Update(secret)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from secret %s: %v", finalizer, secret.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from secret %s: %v", finalizers, secret.Name, err)
 	}
 	return secret, nil
 }

federation/pkg/federation-controller/service/servicecontroller.go

@@ -328,14 +328,14 @@ func (s *ServiceController) hasFinalizerFunc(obj pkgruntime.Object, finalizer st
 	return false
 }
 
-// Removes the finalizer from the given objects ObjectMeta.
+// Removes the finalizers from the given objects ObjectMeta.
 // Assumes that the given object is a service.
-func (s *ServiceController) removeFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (s *ServiceController) removeFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	service := obj.(*v1.Service)
 	newFinalizers := []string{}
 	hasFinalizer := false
 	for i := range service.ObjectMeta.Finalizers {
-		if string(service.ObjectMeta.Finalizers[i]) != finalizer {
+		if !deletionhelper.ContainsString(finalizers, service.ObjectMeta.Finalizers[i]) {
 			newFinalizers = append(newFinalizers, service.ObjectMeta.Finalizers[i])
 		} else {
 			hasFinalizer = true
@@ -348,7 +348,7 @@ func (s *ServiceController) removeFinalizerFunc(obj pkgruntime.Object, finalizer
 	service.ObjectMeta.Finalizers = newFinalizers
 	service, err := s.federationClient.Core().Services(service.Namespace).Update(service)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove finalizer %s from service %s: %v", finalizer, service.Name, err)
+		return nil, fmt.Errorf("failed to remove finalizers %v from service %s: %v", finalizers, service.Name, err)
 	}
 	return service, nil
 }

federation/pkg/federation-controller/util/deletionhelper/BUILD

@@ -9,7 +9,10 @@ load(
 
 go_library(
     name = "go_default_library",
-    srcs = ["deletion_helper.go"],
+    srcs = [
+        "deletion_helper.go",
+        "util.go",
+    ],
     tags = ["automanaged"],
     deps = [
         "//federation/pkg/federation-controller/util:go_default_library",

federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go

@@ -45,7 +45,7 @@ const (
 )
 
 type HasFinalizerFunc func(runtime.Object, string) bool
-type RemoveFinalizerFunc func(runtime.Object, string) (runtime.Object, error)
+type RemoveFinalizerFunc func(runtime.Object, []string) (runtime.Object, error)
 type AddFinalizerFunc func(runtime.Object, []string) (runtime.Object, error)
 type ObjNameFunc func(runtime.Object) string
 
@@ -123,11 +123,8 @@ func (dh *DeletionHelper) HandleObjectInUnderlyingClusters(obj runtime.Object) (
 		// If the obj has FinalizerOrphan finalizer, then we need to orphan the
 		// corresponding objects in underlying clusters.
 		// Just remove both the finalizers in that case.
-		obj, err := dh.removeFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters)
+		finalizers := []string{FinalizerDeleteFromUnderlyingClusters, metav1.FinalizerOrphanDependents}
-		if err != nil {
+		return dh.removeFinalizerFunc(obj, finalizers)
-			return obj, err
-		}
-		return dh.removeFinalizerFunc(obj, metav1.FinalizerOrphanDependents)
 	}
 
 	glog.V(2).Infof("Deleting obj %s from underlying clusters", objName)
@@ -183,5 +180,5 @@ func (dh *DeletionHelper) HandleObjectInUnderlyingClusters(obj runtime.Object) (
 	}
 
 	// All done. Just remove the finalizer.
-	return dh.removeFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters)
+	return dh.removeFinalizerFunc(obj, []string{FinalizerDeleteFromUnderlyingClusters})
 }

federation/pkg/federation-controller/util/deletionhelper/util.go

@@ -0,0 +1,28 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package deletionhelper
+
+// ContainsString returns true if the given string slice contains the given string.
+// Returns false otherwise.
+func ContainsString(arr []string, s string) bool {
+	for i := range arr {
+		if arr[i] == s {
+			return true
+		}
+	}
+	return false
+}