Check for existing bindings only for instances with DeprovisionStatus == ServiceInstanceDeprovisionStatusRequired. (openshift#1640)

Closes openshift#1588

Author: dmitri-d

pkg/controller/controller_instance.go

@@ -469,11 +469,6 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns
 
 	instance = instance.DeepCopy()
 
-	// We don't want to delete the instance if there are any bindings associated.
-	if err := c.checkServiceInstanceHasExistingBindings(instance); err != nil {
-		return c.handleServiceInstanceReconciliationError(instance, err)
-	}
-
 	// If the deprovisioning succeeded or is not needed, then no need to
 	// make a request to the broker.
 	if instance.Status.DeprovisionStatus == v1beta1.ServiceInstanceDeprovisionStatusNotRequired ||
@@ -492,6 +487,11 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns
 		return c.processDeprovisionFailure(instance, readyCond, failedCond)
 	}
 
+	// We don't want to delete the instance if there are any bindings associated.
+	if err := c.checkServiceInstanceHasExistingBindings(instance); err != nil {
+		return c.handleServiceInstanceReconciliationError(instance, err)
+	}
+
 	serviceClass, servicePlan, brokerName, brokerClient, err := c.getClusterServiceClassPlanAndClusterServiceBroker(instance)
 	if err != nil {
 		return c.handleServiceInstanceReconciliationError(instance, err)
@@ -1179,7 +1179,11 @@ func (c *controller) checkServiceInstanceHasExistingBindings(instance *v1beta1.S
 	if err != nil {
 		return err
 	}
+
 	for _, binding := range bindingList {
+		// Note that as we are potentially looking at a stale binding resource
+		// and cannot rely on UnbindStatus == ServiceBindingUnbindStatusNotRequired
+		// to filter out binding requests that have yet to be sent to the broker.
 		if instance.Name == binding.Spec.ServiceInstanceRef.Name {
 			return &operationError{
 				reason:  errorDeprovisionBlockedByCredentialsReason,

pkg/controller/controller_instance_test.go

@@ -1725,51 +1725,6 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) {
 	}
 }
 
-// TestReconcileServiceInstanceDeleteFailedProvisionWithoutRequest tests that
-// an instance that failed to provision without making a provision request
-// will be finalized, but no deprovision request will be sent to the broker.
-func TestReconcileServiceInstanceDeleteFailedProvisionWithoutRequest(t *testing.T) {
-	fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, noFakeActions())
-
-	sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker())
-	sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(getTestClusterServiceClass())
-	sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan())
-
-	instance := getTestServiceInstanceWithFailedStatus()
-	instance.ObjectMeta.DeletionTimestamp = &metav1.Time{}
-	instance.ObjectMeta.Finalizers = []string{v1beta1.FinalizerServiceCatalog}
-	instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{}
-	instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired
-
-	instance.Generation = 1
-	instance.Status.ReconciledGeneration = 0
-
-	fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
-		return true, instance, nil
-	})
-
-	err := testController.reconcileServiceInstance(instance)
-	if err != nil {
-		t.Fatalf("Unexpected error from reconcileServiceInstance: %v", err)
-	}
-
-	brokerActions := fakeClusterServiceBrokerClient.Actions()
-	assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0)
-
-	// Verify no core kube actions occurred
-	kubeActions := fakeKubeClient.Actions()
-	assertNumberOfActions(t, kubeActions, 0)
-
-	actions := fakeCatalogClient.Actions()
-	assertNumberOfActions(t, actions, 1)
-
-	updatedServiceInstance := assertUpdateStatus(t, actions[0], instance)
-	assertEmptyFinalizers(t, updatedServiceInstance)
-
-	events := getRecordedEvents(testController)
-	assertNumEvents(t, events, 0)
-}
-
 // TestReconcileServiceInstanceDeleteFailedProvisionWithRequest tests that an
 // instance that failed to provision but for which a provision request was
 // made will have a deprovision request sent to the broker.
@@ -1832,49 +1787,97 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T)
 	}
 }
 
-// TestReconcileServiceInstanceDeleteWhenAlreadyDeprovisionedSuccessfully
-// tests that an instance that has already been deprovisioned will be
-// finalized, but no deprovision request will be sent to the broker.
-func TestReconcileServiceInstanceDeleteWhenAlreadyDeprovisionedSuccessfully(t *testing.T) {
-	fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, noFakeActions())
+// TestReconsileServiceInstanceDeleteWithParameters tests that
+// an instance will be finalized under various conditions defined by test parameters
+func TestReconsileServiceInstanceDeleteWithParameters(t *testing.T) {
+	cases := []struct {
+		name                  string
+		externalProperties    *v1beta1.ServiceInstancePropertiesState
+		deprovisionStatus     v1beta1.ServiceInstanceDeprovisionStatus
+		serviceBinding        *v1beta1.ServiceBinding
+		generation            int64
+		reconceiledGeneration int64
+	}{
+		{
+			name:                  "With a failed to provision instance and without making a provision request",
+			externalProperties:    &v1beta1.ServiceInstancePropertiesState{},
+			deprovisionStatus:     v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+			serviceBinding:        nil,
+			generation:            1,
+			reconceiledGeneration: 0,
+		},
+		{
+			name:                  "With a failed to provision instance, with inactive binding, and without making a provision request",
+			externalProperties:    &v1beta1.ServiceInstancePropertiesState{},
+			deprovisionStatus:     v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+			serviceBinding:        getTestServiceInactiveBinding(),
+			generation:            1,
+			reconceiledGeneration: 0,
+		},
+		{
+			name:                  "With a deprovisioned instance and without making a deprovision request",
+			externalProperties:    nil,
+			deprovisionStatus:     v1beta1.ServiceInstanceDeprovisionStatusSucceeded,
+			serviceBinding:        nil,
+			generation:            2,
+			reconceiledGeneration: 1,
+		},
+		{
+			name:                  "With a deprovisioned instance, with inactive binding, and without making a deprovision request",
+			externalProperties:    nil,
+			deprovisionStatus:     v1beta1.ServiceInstanceDeprovisionStatusSucceeded,
+			serviceBinding:        getTestServiceInactiveBinding(),
+			generation:            2,
+			reconceiledGeneration: 1,
+		},
+	}
 
-	sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker())
-	sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(getTestClusterServiceClass())
-	sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan())
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, noFakeActions())
 
-	instance := getTestServiceInstanceWithFailedStatus()
-	instance.ObjectMeta.DeletionTimestamp = &metav1.Time{}
-	instance.ObjectMeta.Finalizers = []string{v1beta1.FinalizerServiceCatalog}
-	instance.Status.ExternalProperties = nil
-	instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded
+			sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker())
+			sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(getTestClusterServiceClass())
+			sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan())
+			if tc.serviceBinding != nil {
+				sharedInformers.ClusterServicePlans().Informer().GetStore().Add(tc.serviceBinding)
+			}
 
-	instance.Generation = 2
-	instance.Status.ReconciledGeneration = 1
+			instance := getTestServiceInstanceWithFailedStatus()
+			instance.ObjectMeta.DeletionTimestamp = &metav1.Time{}
+			instance.ObjectMeta.Finalizers = []string{v1beta1.FinalizerServiceCatalog}
+			instance.Status.ExternalProperties = tc.externalProperties
+			instance.Status.DeprovisionStatus = tc.deprovisionStatus
 
-	fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
-		return true, instance, nil
-	})
+			instance.Generation = tc.generation
+			instance.Status.ReconciledGeneration = tc.reconceiledGeneration
 
-	err := testController.reconcileServiceInstance(instance)
-	if err != nil {
-		t.Fatalf("Unexpected error from reconcileServiceInstance: %v", err)
-	}
+			fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+				return true, instance, nil
+			})
 
-	brokerActions := fakeClusterServiceBrokerClient.Actions()
-	assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0)
+			err := testController.reconcileServiceInstance(instance)
+			if err != nil {
+				t.Fatalf("Unexpected error from reconcileServiceInstance: %v", err)
+			}
 
-	// Verify no core kube actions occurred
-	kubeActions := fakeKubeClient.Actions()
-	assertNumberOfActions(t, kubeActions, 0)
+			brokerActions := fakeClusterServiceBrokerClient.Actions()
+			assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0)
 
-	actions := fakeCatalogClient.Actions()
-	assertNumberOfActions(t, actions, 1)
+			// Verify no core kube actions occurred
+			kubeActions := fakeKubeClient.Actions()
+			assertNumberOfActions(t, kubeActions, 0)
 
-	updatedServiceInstance := assertUpdateStatus(t, actions[0], instance)
-	assertEmptyFinalizers(t, updatedServiceInstance)
+			actions := fakeCatalogClient.Actions()
+			assertNumberOfActions(t, actions, 1)
 
-	events := getRecordedEvents(testController)
-	assertNumEvents(t, events, 0)
+			updatedServiceInstance := assertUpdateStatus(t, actions[0], instance)
+			assertEmptyFinalizers(t, updatedServiceInstance)
+
+			events := getRecordedEvents(testController)
+			assertNumEvents(t, events, 0)
+		})
+	}
 }
 
 // TestReconcileServiceInstanceDeleteWhenAlreadyDeprovisionedUnsuccessfully

pkg/controller/controller_test.go

@@ -853,6 +853,26 @@ func getTestServiceBinding() *v1beta1.ServiceBinding {
 	}
 }
 
+// instantiates a yet-to-be-created binding
+func getTestServiceInactiveBinding() *v1beta1.ServiceBinding {
+	return &v1beta1.ServiceBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       testServiceBindingName,
+			Namespace:  testNamespace,
+			Finalizers: []string{v1beta1.FinalizerServiceCatalog},
+			Generation: 1,
+		},
+		Spec: v1beta1.ServiceBindingSpec{
+			ServiceInstanceRef: v1beta1.LocalObjectReference{Name: testServiceInstanceName},
+			ExternalID:         testServiceBindingGUID,
+		},
+		Status: v1beta1.ServiceBindingStatus{
+			Conditions:   []v1beta1.ServiceBindingCondition{},
+			UnbindStatus: v1beta1.ServiceBindingUnbindStatusNotRequired,
+		},
+	}
+}
+
 func getTestServiceBindingUnbinding() *v1beta1.ServiceBinding {
 	binding := &v1beta1.ServiceBinding{
 		ObjectMeta: metav1.ObjectMeta{