Fix usage of finalizers (openshift#894)

pkg/api/meta/finalizers.go

@@ -19,6 +19,7 @@ package meta
 import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // GetFinalizers gets the list of finalizers on obj
@@ -36,32 +37,22 @@ func AddFinalizer(obj runtime.Object, value string) error {
 	if err != nil {
 		return err
 	}
-	finalizers := append(accessor.GetFinalizers(), value)
-	accessor.SetFinalizers(finalizers)
+	finalizers := sets.NewString(accessor.GetFinalizers()...)
+	finalizers.Insert(value)
+	accessor.SetFinalizers(finalizers.List())
 	return nil
 }
 
-// RemoveFinalizer removes the given value from the list of finalizers in obj, then returns the list
-// of finalizers after value has been removed. The returned slice will have the same ordering as
-// the list of finalizers that was in obj.
-//
-// If value doesn't exist in the finalizers in obj, the returned slice is the same as the finalizers
-// that were in obj.
-//
-// All of the finalizers that match value will be removed from the list in obj.
+// RemoveFinalizer removes the given value from the list of finalizers in obj, then returns a new list
+// of finalizers after value has been removed.
 func RemoveFinalizer(obj runtime.Object, value string) ([]string, error) {
 	accessor, err := meta.Accessor(obj)
 	if err != nil {
 		return nil, err
 	}
-	finalizers := accessor.GetFinalizers()
-	newFinalizers := []string{}
-	for _, finalizer := range finalizers {
-		if finalizer == value {
-			continue
-		}
-		newFinalizers = append(newFinalizers, finalizer)
-	}
+	finalizers := sets.NewString(accessor.GetFinalizers()...)
+	finalizers.Delete(value)
+	newFinalizers := finalizers.List()
 	accessor.SetFinalizers(newFinalizers)
 	return newFinalizers, nil
 }

pkg/apis/servicecatalog/types.go

@@ -396,3 +396,8 @@ const (
 	// BindingConditionReady represents a BindingCondition is in ready state.
 	BindingConditionReady BindingConditionType = "Ready"
 )
+
+// These are internal finalizer values to service catalog, must be qualified name.
+const (
+	FinalizerServiceCatalog string = "kubernetes-incubator/service-catalog"
+)

pkg/apis/servicecatalog/v1alpha1/types.go

@@ -397,3 +397,8 @@ const (
 	// BindingConditionReady represents a binding condition is in ready state.
 	BindingConditionReady BindingConditionType = "Ready"
 )
+
+// These are external finalizer values to service catalog, must be qualified name.
+const (
+	FinalizerServiceCatalog string = "kubernetes-incubator/service-catalog"
+)

pkg/controller/controller_binding.go

@@ -26,6 +26,7 @@ import (
 	"github.com/kubernetes-incubator/service-catalog/pkg/brokerapi"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/pkg/api"
 	"k8s.io/client-go/pkg/api/v1"
 	"k8s.io/client-go/tools/cache"
@@ -258,8 +259,7 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error {
 	// since those most be cleared in order, we proceed with the soft delete
 	// only if it's "our turn--" i.e. only if the finalizer we care about is at
 	// the head of the finalizers list.
-	// TODO: Should we use a more specific string here?
-	if len(binding.Finalizers) > 0 && binding.Finalizers[0] == "kubernetes" {
+	if finalizers := sets.NewString(binding.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) {
 		glog.V(4).Infof("Finalizing Binding %v/%v", binding.Namespace, binding.Name)
 		err = c.ejectBinding(binding)
 		if err != nil {
@@ -306,7 +306,8 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error {
 			"The binding was deleted successfully",
 		)
 		// Clear the finalizer
-		c.updateBindingFinalizers(binding, binding.Finalizers[1:])
+		finalizers.Delete(v1alpha1.FinalizerServiceCatalog)
+		c.updateBindingFinalizers(binding, finalizers.List())
 		c.recorder.Event(binding, api.EventTypeNormal, successUnboundReason, "This binding was deleted successfully")
 
 		glog.V(5).Infof("Successfully deleted Binding %v/%v of Instance %v/%v of ServiceClass %v at Broker %v", binding.Namespace, binding.Name, instance.Namespace, instance.Name, serviceClass.Name, brokerName)

pkg/controller/controller_binding_test.go

@@ -474,7 +474,7 @@ func TestReconcileBindingDelete(t *testing.T) {
 			Name:              testBindingName,
 			Namespace:         testNamespace,
 			DeletionTimestamp: &metav1.Time{},
-			Finalizers:        []string{"kubernetes"},
+			Finalizers:        []string{v1alpha1.FinalizerServiceCatalog},
 		},
 		Spec: v1alpha1.BindingSpec{
 			InstanceRef: v1.LocalObjectReference{Name: testInstanceName},

pkg/controller/controller_broker.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/pkg/api"
 	"k8s.io/client-go/tools/cache"
 )
@@ -235,8 +236,7 @@ func (c *controller) reconcileBroker(broker *v1alpha1.Broker) error {
 	// since those most be cleared in order, we proceed with the soft delete
 	// only if it's "our turn--" i.e. only if the finalizer we care about is at
 	// the head of the finalizers list.
-	// TODO: Should we use a more specific string here?
-	if len(broker.Finalizers) > 0 && broker.Finalizers[0] == "kubernetes" {
+	if finalizers := sets.NewString(broker.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) {
 		glog.V(4).Infof("Finalizing Broker %v", broker.Name)
 
 		// Get ALL ServiceClasses. Remove those that reference this Broker.
@@ -286,7 +286,8 @@ func (c *controller) reconcileBroker(broker *v1alpha1.Broker) error {
 			"The broker was deleted successfully",
 		)
 		// Clear the finalizer
-		c.updateBrokerFinalizers(broker, broker.Finalizers[1:])
+		finalizers.Delete(v1alpha1.FinalizerServiceCatalog)
+		c.updateBrokerFinalizers(broker, finalizers.List())
 
 		c.recorder.Eventf(broker, api.EventTypeNormal, successBrokerDeletedReason, successBrokerDeletedMessage, broker.Name)
 		glog.V(5).Infof("Successfully deleted Broker %v", broker.Name)

pkg/controller/controller_broker_test.go

@@ -232,7 +232,7 @@ func TestReconcileBrokerDelete(t *testing.T) {
 
 	broker := getTestBroker()
 	broker.DeletionTimestamp = &metav1.Time{}
-	broker.Finalizers = []string{"kubernetes"}
+	broker.Finalizers = []string{v1alpha1.FinalizerServiceCatalog}
 
 	testController.reconcileBroker(broker)
 

pkg/controller/controller_instance.go

@@ -27,6 +27,7 @@ import (
 	"github.com/kubernetes-incubator/service-catalog/pkg/brokerapi"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/pkg/api"
 	"k8s.io/client-go/tools/cache"
 )
@@ -230,8 +231,7 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 	// since those most be cleared in order, we proceed with the soft delete
 	// only if it's "our turn--" i.e. only if the finalizer we care about is at
 	// the head of the finalizers list.
-	// TODO: Should we use a more specific string here?
-	if len(instance.Finalizers) > 0 && instance.Finalizers[0] == "kubernetes" {
+	if finalizers := sets.NewString(instance.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) {
 		glog.V(4).Infof("Finalizing Instance %v/%v", instance.Namespace, instance.Name)
 
 		request := &brokerapi.DeleteServiceInstanceRequest{
@@ -290,7 +290,8 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 				successDeprovisionMessage,
 			)
 			// Clear the finalizer
-			c.updateInstanceFinalizers(instance, instance.Finalizers[1:])
+			finalizers.Delete(v1alpha1.FinalizerServiceCatalog)
+			c.updateInstanceFinalizers(instance, finalizers.List())
 			c.recorder.Event(instance, api.EventTypeNormal, successDeprovisionReason, successDeprovisionMessage)
 			glog.V(5).Infof("Successfully deprovisioned Instance %v/%v of ServiceClass %v at Broker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName)
 		}
@@ -339,8 +340,9 @@ func (c *controller) pollInstance(serviceClass *v1alpha1.ServiceClass, servicePl
 	if rc == http.StatusGone && deleting {
 		instance.Status.AsyncOpInProgress = false
 		// Clear the finalizer
-		if len(instance.Finalizers) > 0 && instance.Finalizers[0] == "kubernetes" {
-			c.updateInstanceFinalizers(instance, instance.Finalizers[1:])
+		if finalizers := sets.NewString(instance.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) {
+			finalizers.Delete(v1alpha1.FinalizerServiceCatalog)
+			c.updateInstanceFinalizers(instance, finalizers.List())
 		}
 		c.updateInstanceCondition(
 			instance,
@@ -375,8 +377,9 @@ func (c *controller) pollInstance(serviceClass *v1alpha1.ServiceClass, servicePl
 				successDeprovisionMessage,
 			)
 			// Clear the finalizer
-			if len(instance.Finalizers) > 0 && instance.Finalizers[0] == "kubernetes" {
-				c.updateInstanceFinalizers(instance, instance.Finalizers[1:])
+			if finalizers := sets.NewString(instance.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) {
+				finalizers.Delete(v1alpha1.FinalizerServiceCatalog)
+				c.updateInstanceFinalizers(instance, finalizers.List())
 			}
 			c.recorder.Event(instance, api.EventTypeNormal, successDeprovisionReason, successDeprovisionMessage)
 			glog.V(5).Infof("Successfully deprovisioned Instance %v/%v of ServiceClass %v at Broker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName)

pkg/controller/controller_instance_test.go

@@ -602,7 +602,7 @@ func TestReconcileInstanceDelete(t *testing.T) {
 
 	instance := getTestInstance()
 	instance.ObjectMeta.DeletionTimestamp = &metav1.Time{}
-	instance.ObjectMeta.Finalizers = []string{"kubernetes"}
+	instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog}
 
 	fakeCatalogClient.AddReactor("get", "instances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
 		return true, instance, nil

pkg/controller/controller_test.go

@@ -376,7 +376,7 @@ func getTestInstanceAsyncDeprovisioning(operation string) *v1alpha1.Instance {
 
 func getTestInstanceAsyncDeprovisioningWithFinalizer(operation string) *v1alpha1.Instance {
 	instance := getTestInstanceAsyncDeprovisioning(operation)
-	instance.ObjectMeta.Finalizers = []string{"kubernetes"}
+	instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog}
 	return instance
 }
 

pkg/registry/servicecatalog/binding/strategy.go

@@ -96,9 +96,7 @@ func (bindingRESTStrategy) PrepareForCreate(ctx genericapirequest.Context, obj r
 	binding.Status = sc.BindingStatus{}
 	// Fill in the first entry set to "creating"?
 	binding.Status.Conditions = []sc.BindingCondition{}
-
-	// TODO: Should we use a more specific string here?
-	binding.Finalizers = []string{"kubernetes"}
+	binding.Finalizers = []string{sc.FinalizerServiceCatalog}
 }
 
 func (bindingRESTStrategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList {

pkg/registry/servicecatalog/broker/strategy.go

@@ -96,9 +96,7 @@ func (brokerRESTStrategy) PrepareForCreate(ctx genericapirequest.Context, obj ru
 	broker.Status = sc.BrokerStatus{}
 	// Fill in the first entry set to "creating"?
 	broker.Status.Conditions = []sc.BrokerCondition{}
-
-	// TODO: Should we use a more specific string here?
-	broker.Finalizers = []string{"kubernetes"}
+	broker.Finalizers = []string{sc.FinalizerServiceCatalog}
 }
 
 func (brokerRESTStrategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList {

pkg/registry/servicecatalog/instance/strategy.go

@@ -97,9 +97,7 @@ func (instanceRESTStrategy) PrepareForCreate(ctx genericapirequest.Context, obj
 	instance.Status = sc.InstanceStatus{}
 	// Fill in the first entry set to "creating"?
 	instance.Status.Conditions = []sc.InstanceCondition{}
-
-	// TODO: Should we use a more specific string here?
-	instance.Finalizers = []string{"kubernetes"}
+	instance.Finalizers = []string{sc.FinalizerServiceCatalog}
 }
 
 func (instanceRESTStrategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList {

pkg/storage/tpr/storage_interface.go

@@ -24,7 +24,7 @@ import (
 
 	"github.com/golang/glog"
 	scmeta "github.com/kubernetes-incubator/service-catalog/pkg/api/meta"
-	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1"
 	"golang.org/x/net/context"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -39,7 +39,6 @@ import (
 
 var (
 	errNotImplemented = errors.New("not implemented for third party resources")
-	tprFinalizer      = fmt.Sprintf("%s/%s", servicecatalog.GroupName, tprVersion)
 )
 
 type store struct {
@@ -101,7 +100,7 @@ func (t *store) Create(
 		return err
 	}
 
-	if err := scmeta.AddFinalizer(obj, tprFinalizer); err != nil {
+	if err := scmeta.AddFinalizer(obj, v1alpha1.FinalizerServiceCatalog); err != nil {
 		glog.Errorf("adding finalizer to %s (%s)", key, err)
 		return err
 	}
@@ -194,7 +193,7 @@ func (t *store) Delete(
 		return err
 	}
 
-	if _, err := scmeta.RemoveFinalizer(out, tprFinalizer); err != nil {
+	if _, err := scmeta.RemoveFinalizer(out, v1alpha1.FinalizerServiceCatalog); err != nil {
 		glog.Errorf("removing finalizer from %#v (%s)", out, err)
 		return err
 	}

pkg/storage/tpr/storage_interface_test.go

@@ -28,6 +28,7 @@ import (
 	sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
 	_ "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/install"
 	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/testapi"
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1"
 	"github.com/kubernetes-incubator/service-catalog/pkg/rest/core/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -873,7 +874,7 @@ func TestDeleteWithNoNamespace(t *testing.T) {
 		},
 	}
 	brokerWithFinalizers := *brokerNoFinalizers
-	brokerWithFinalizers.Finalizers = append(brokerWithFinalizers.Finalizers, tprFinalizer)
+	brokerWithFinalizers.Finalizers = append(brokerWithFinalizers.Finalizers, v1alpha1.FinalizerServiceCatalog)
 	fakeCl.Storage.Set(globalNamespace, ServiceBrokerKind.URLName(), name, &brokerWithFinalizers)
 	key, err := keyer.Key(request.NewContext(), name)
 	if err != nil {
@@ -923,7 +924,7 @@ func TestDeleteWithNamespace(t *testing.T) {
 		},
 	}
 	instanceWithFinalizers := *instanceNoFinalizers
-	instanceWithFinalizers.Finalizers = append(instanceWithFinalizers.Finalizers, tprFinalizer)
+	instanceWithFinalizers.Finalizers = append(instanceWithFinalizers.Finalizers, v1alpha1.FinalizerServiceCatalog)
 	fakeCl.Storage.Set(namespace, ServiceInstanceKind.URLName(), name, &instanceWithFinalizers)
 	ctx := request.NewContext()
 	ctx = request.WithNamespace(ctx, namespace)