Delete removed serviceClasses when they have no instances left (#1450)

pkg/controller/controller_serviceclass.go

@@ -19,6 +19,10 @@ package controller
 import (
 	"github.com/golang/glog"
 	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -33,30 +37,65 @@ func (c *controller) serviceClassAdd(obj interface{}) {
 	c.serviceClassQueue.Add(key)
 }
 
+func (c *controller) serviceClassUpdate(oldObj, newObj interface{}) {
+	c.serviceClassAdd(newObj)
+}
+
+func (c *controller) serviceClassDelete(obj interface{}) {
+	serviceClass, ok := obj.(*v1beta1.ClusterServiceClass)
+	if serviceClass == nil || !ok {
+		return
+	}
+
+	glog.V(4).Infof("Received delete event for ServiceClass %v; no further processing will occur", serviceClass.Name)
+}
+
 // reconcileServiceClassKey reconciles a ServiceClass due to controller resync
 // or an event on the ServiceClass.  Note that this is NOT the main
 // reconciliation loop for ServiceClass. ServiceClasses are primarily
 // reconciled in a separate flow when a ClusterServiceBroker is reconciled.
 func (c *controller) reconcileClusterServiceClassKey(key string) error {
-	// currently, this is a no-op.  In the future, we'll maintain status
-	// information here.
-	return nil
+	plan, err := c.serviceClassLister.Get(key)
+	if errors.IsNotFound(err) {
+		glog.Infof("ClusterServiceClass %q: Not doing work because it has been deleted", key)
+		return nil
+	}
+	if err != nil {
+		glog.Infof("ClusterServiceClass %q: Unable to retrieve object from store: %v", key, err)
+		return err
+	}
+
+	return c.reconcileClusterServiceClass(plan)
 }
 
 func (c *controller) reconcileClusterServiceClass(serviceClass *v1beta1.ClusterServiceClass) error {
-	glog.V(4).Infof("Processing ServiceClass %v", serviceClass.Name)
-	return nil
-}
+	glog.Infof("ClusterServiceClass %q (ExternalName: %q): processing", serviceClass.Name, serviceClass.Spec.ExternalName)
 
-func (c *controller) serviceClassUpdate(oldObj, newObj interface{}) {
-	c.serviceClassAdd(newObj)
+	if !serviceClass.Status.RemovedFromBrokerCatalog {
+		return nil
+	}
+
+	glog.Infof("ClusterServiceClass %q (ExternalName: %q): has been removed from broker catalog; determining whether there are instances remaining", serviceClass.Name, serviceClass.Spec.ExternalName)
+
+	serviceInstances, err := c.findServiceInstancesOnClusterServiceClass(serviceClass)
+	if err != nil {
+		return err
+	}
+
+	if len(serviceInstances.Items) != 0 {
+		return nil
+	}
+
+	glog.Infof("ClusterServiceClass %q (ExternalName: %q): has been removed from broker catalog and has zero instances remaining; deleting", serviceClass.Name, serviceClass.Spec.ExternalName)
+	return c.serviceCatalogClient.ClusterServiceClasses().Delete(serviceClass.Name, &metav1.DeleteOptions{})
 }
 
-func (c *controller) serviceClassDelete(obj interface{}) {
-	serviceClass, ok := obj.(*v1beta1.ClusterServiceClass)
-	if serviceClass == nil || !ok {
-		return
+func (c *controller) findServiceInstancesOnClusterServiceClass(serviceClass *v1beta1.ClusterServiceClass) (*v1beta1.ServiceInstanceList, error) {
+	fieldSet := fields.Set{
+		"spec.clusterServiceClassRef.name": serviceClass.Name,
 	}
+	fieldSelector := fields.SelectorFromSet(fieldSet).String()
+	listOpts := metav1.ListOptions{FieldSelector: fieldSelector}
 
-	glog.V(4).Infof("Received delete event for ServiceClass %v; no further processing will occur", serviceClass.Name)
+	return c.serviceCatalogClient.ServiceInstances(metav1.NamespaceAll).List(listOpts)
 }

pkg/controller/controller_serviceclass_test.go

@@ -0,0 +1,137 @@
+/*
+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 controller
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
+	"github.com/kubernetes-incubator/service-catalog/test/fake"
+	"k8s.io/apimachinery/pkg/runtime"
+
+	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/labels"
+	clientgotesting "k8s.io/client-go/testing"
+)
+
+func TestReconcileClusterServiceClassRemovedFromCatalog(t *testing.T) {
+	getRemovedServiceClass := func() *v1beta1.ClusterServiceClass {
+		p := getTestClusterServiceClass()
+		p.Status.RemovedFromBrokerCatalog = true
+		return p
+	}
+
+	cases := []struct {
+		name                    string
+		serviceClass            *v1beta1.ClusterServiceClass
+		instances               []v1beta1.ServiceInstance
+		catalogClientPrepFunc   func(*fake.Clientset)
+		shouldError             bool
+		errText                 *string
+		catalogActionsCheckFunc func(t *testing.T, name string, actions []clientgotesting.Action)
+	}{
+		{
+			name:         "not removed from catalog",
+			serviceClass: getTestClusterServiceClass(),
+			shouldError:  false,
+		},
+		{
+			name:         "removed from catalog, instances left",
+			serviceClass: getRemovedServiceClass(),
+			instances:    []v1beta1.ServiceInstance{*getTestServiceInstance()},
+			shouldError:  false,
+			catalogActionsCheckFunc: func(t *testing.T, name string, actions []clientgotesting.Action) {
+				listRestrictions := clientgotesting.ListRestrictions{
+					Labels: labels.Everything(),
+					Fields: fields.OneTermEqualSelector("spec.clusterServiceClassRef.name", "SCGUID"),
+				}
+
+				expectNumberOfActions(t, name, actions, 1)
+				assertList(t, actions[0], &v1beta1.ServiceInstance{}, listRestrictions)
+			},
+		},
+		{
+			name:         "removed from catalog, no instances left",
+			serviceClass: getRemovedServiceClass(),
+			instances:    nil,
+			shouldError:  false,
+			catalogActionsCheckFunc: func(t *testing.T, name string, actions []clientgotesting.Action) {
+				listRestrictions := clientgotesting.ListRestrictions{
+					Labels: labels.Everything(),
+					Fields: fields.OneTermEqualSelector("spec.clusterServiceClassRef.name", "SCGUID"),
+				}
+
+				expectNumberOfActions(t, name, actions, 2)
+				assertList(t, actions[0], &v1beta1.ServiceInstance{}, listRestrictions)
+				assertDelete(t, actions[1], getRemovedServiceClass())
+			},
+		},
+		{
+			name: "removed from catalog, no instances left, delete fails", serviceClass: getRemovedServiceClass(),
+			instances:   nil,
+			shouldError: true,
+			catalogClientPrepFunc: func(client *fake.Clientset) {
+				client.AddReactor("delete", "clusterserviceclasses", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+					return true, nil, errors.New("oops")
+				})
+			},
+			errText: strPtr("oops"),
+			catalogActionsCheckFunc: func(t *testing.T, name string, actions []clientgotesting.Action) {
+				listRestrictions := clientgotesting.ListRestrictions{
+					Labels: labels.Everything(),
+					Fields: fields.OneTermEqualSelector("spec.clusterServiceClassRef.name", "SCGUID"),
+				}
+
+				expectNumberOfActions(t, name, actions, 2)
+				assertList(t, actions[0], &v1beta1.ServiceInstance{}, listRestrictions)
+				assertDelete(t, actions[1], getRemovedServiceClass())
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		_, fakeCatalogClient, _, testController, _ := newTestController(t, noFakeActions())
+
+		fakeCatalogClient.AddReactor("list", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+			return true, &v1beta1.ServiceInstanceList{Items: tc.instances}, nil
+		})
+
+		if tc.catalogClientPrepFunc != nil {
+			tc.catalogClientPrepFunc(fakeCatalogClient)
+		}
+
+		err := testController.reconcileClusterServiceClass(tc.serviceClass)
+		if err != nil {
+			if !tc.shouldError {
+				t.Errorf("%v: unexpected error from method under test: %v", tc.name, err)
+				continue
+			} else if tc.errText != nil && *tc.errText != err.Error() {
+				t.Errorf("%v: unexpected error text from method under test; expected %v, got %v", tc.name, tc.errText, err.Error())
+				continue
+			}
+		}
+
+		actions := fakeCatalogClient.Actions()
+
+		if tc.catalogActionsCheckFunc != nil {
+			tc.catalogActionsCheckFunc(t, tc.name, actions)
+		} else {
+			expectNumberOfActions(t, tc.name, actions, 0)
+		}
+	}
+}

test/integration/deleted_services_and_plans_test.go

@@ -93,8 +93,10 @@ func TestClusterServicePlanRemovedFromCatalogWithoutInstances(t *testing.T) {
 }
 
 const (
-	testRemovedClusterServicePlanGUID         = "removed-plan"
-	testRemovedClusterServicePlanExternalName = "removed-plan-name"
+	testRemovedClusterServicePlanGUID          = "removed-plan"
+	testRemovedClusterServicePlanExternalName  = "removed-plan-name"
+	testRemovedClusterServiceClassGUID         = "removed-class"
+	testRemovedClusterServiceClassExternalName = "removed-class-name"
 )
 
 func getTestClusterServicePlanRemoved() *v1beta1.ClusterServicePlan {
@@ -113,3 +115,79 @@ func getTestClusterServicePlanRemoved() *v1beta1.ClusterServicePlan {
 		Status: v1beta1.ClusterServicePlanStatus{},
 	}
 }
+
+func TestClusterServiceClassRemovedFromCatalogWithoutInstances(t *testing.T) {
+	_, catalogClient, _, _, _, _, shutdownServer, shutdownController := newTestController(t, fakeosb.FakeClientConfiguration{
+		CatalogReaction: &fakeosb.CatalogReaction{
+			Response: getTestCatalogResponse(),
+		},
+	})
+	defer shutdownController()
+	defer shutdownServer()
+
+	client := catalogClient.ServicecatalogV1beta1()
+
+	broker := &v1beta1.ClusterServiceBroker{
+		ObjectMeta: metav1.ObjectMeta{Name: testClusterServiceBrokerName},
+		Spec: v1beta1.ClusterServiceBrokerSpec{
+			URL: testBrokerURL,
+		},
+	}
+
+	_, err := client.ClusterServiceBrokers().Create(broker)
+	if nil != err {
+		t.Fatalf("error creating the broker %q (%q)", broker.Name, err)
+	}
+
+	err = util.WaitForBrokerCondition(client,
+		testClusterServiceBrokerName,
+		v1beta1.ServiceBrokerCondition{
+			Type:   v1beta1.ServiceBrokerConditionReady,
+			Status: v1beta1.ConditionTrue,
+		})
+	if err != nil {
+		t.Fatalf("error waiting for broker to become ready: %v", err)
+	}
+
+	err = util.WaitForClusterServiceClassToExist(client, testClusterServiceClassGUID)
+	if nil != err {
+		t.Fatalf("error waiting from ClusterServiceClass to exist: %v", err)
+	}
+
+	removedClass := getTestClusterServiceClassRemoved()
+	removedClass, err = client.ClusterServiceClasses().Create(removedClass)
+	if err != nil {
+		t.Fatalf("error creating ClusterServiceClass: %v", err)
+	}
+
+	err = util.WaitForClusterServiceClassToExist(client, testRemovedClusterServiceClassGUID)
+	if err != nil {
+		t.Fatalf("error waiting for ClusterServiceClass to exist: %v", err)
+	}
+
+	t.Log("updating ClusterServiceClass status")
+	removedClass.Status.RemovedFromBrokerCatalog = true
+	_, err = client.ClusterServiceClasses().UpdateStatus(removedClass)
+	if err != nil {
+		t.Fatalf("error marking ClusterServiceClass as removed from catalog: %v", err)
+	}
+
+	err = util.WaitForClusterServiceClassToNotExist(client, testRemovedClusterServiceClassGUID)
+	if err != nil {
+		t.Fatalf("error waiting for remove ClusterServiceClass to not exist: %v", err)
+	}
+}
+
+func getTestClusterServiceClassRemoved() *v1beta1.ClusterServiceClass {
+	return &v1beta1.ClusterServiceClass{
+		ObjectMeta: metav1.ObjectMeta{Name: testRemovedClusterServiceClassGUID},
+		Spec: v1beta1.ClusterServiceClassSpec{
+			ClusterServiceBrokerName: testClusterServiceBrokerName,
+			ExternalID:               testRemovedClusterServiceClassGUID,
+			ExternalName:             testRemovedClusterServiceClassExternalName,
+			Description:              "a serviceclass that will be removed",
+			Bindable:                 true,
+		},
+		Status: v1beta1.ClusterServiceClassStatus{},
+	}
+}