From 1877c1f7f8c82ebc284c49d1676ecab7682e1f44 Mon Sep 17 00:00:00 2001
From: Paul Morie <pmorie@redhat.com>
Date: Fri, 28 Apr 2017 19:35:40 -0400
Subject: [PATCH] Bind only once the instance becomes ready (#747)

---
 pkg/controller/controller.go      | 27 +++++++++
 pkg/controller/controller_test.go | 92 ++++++++++++++++++++++++++++++-
 2 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index d3b58ea5b4a..fc06fe022d7 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -269,6 +269,7 @@ const (
 	errorWithOngoingAsyncOperation        string = "ErrorAsyncOperationInProgress"
 	errorWithOngoingAsyncOperationMessage string = "Another operation for this service instance is in progress. "
 	errorNonbindableServiceClassReason    string = "ErrorNonbindableServiceClass"
+	errorInstanceNotReadyReason           string = "ErrorInstanceNotReady"
 
 	successInjectedBindResultReason  string = "InjectedBindResult"
 	successInjectedBindResultMessage string = "Injected bind result"
@@ -1247,6 +1248,20 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error {
 			return err
 		}
 
+		if !isInstanceReady(instance) {
+			s := fmt.Sprintf(`Binding cannot begin because referenced instance "%v/%v" is not ready`, instance.Namespace, instance.Name)
+			glog.Info(s)
+			c.updateBindingCondition(
+				binding,
+				v1alpha1.BindingConditionReady,
+				v1alpha1.ConditionFalse,
+				errorInstanceNotReadyReason,
+				s,
+			)
+			c.recorder.Eventf(binding, api.EventTypeWarning, errorInstanceNotReadyReason, s)
+			return err
+		}
+
 		request := &brokerapi.BindingRequest{
 			ServiceID:    serviceClass.OSBGUID,
 			PlanID:       servicePlan.OSBGUID,
@@ -1730,3 +1745,15 @@ func unmarshalParameters(in []byte) (map[string]interface{}, error) {
 	}
 	return parameters, nil
 }
+
+// isInstanceReady returns whether the given instance has a ready condition
+// with status true.
+func isInstanceReady(instance *v1alpha1.Instance) bool {
+	for _, cond := range instance.Status.Conditions {
+		if cond.Type == v1alpha1.InstanceConditionReady {
+			return cond.Status == v1alpha1.ConditionTrue
+		}
+	}
+
+	return false
+}
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index d1250dd6f2b..43281b32777 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -284,6 +284,19 @@ func getTestNonbindableInstance() *v1alpha1.Instance {
 	return i
 }
 
+func getTestInstanceWithStatus(status v1alpha1.ConditionStatus) *v1alpha1.Instance {
+	instance := getTestInstance()
+	instance.Status = v1alpha1.InstanceStatus{
+		Conditions: []v1alpha1.InstanceCondition{{
+			Type:               v1alpha1.InstanceConditionReady,
+			Status:             status,
+			LastTransitionTime: metav1.NewTime(time.Now().Add(-5 * time.Minute)),
+		}},
+	}
+
+	return instance
+}
+
 // getTestInstanceAsync returns an instance in async mode
 func getTestInstanceAsyncProvisioning(operation string) *v1alpha1.Instance {
 	instance := getTestInstance()
@@ -1925,7 +1938,7 @@ func TestReconcileBindingWithParameters(t *testing.T) {
 
 	sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker())
 	sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass())
-	sharedInformers.Instances().Informer().GetStore().Add(getTestInstance())
+	sharedInformers.Instances().Informer().GetStore().Add(getTestInstanceWithStatus(v1alpha1.ConditionTrue))
 
 	binding := &v1alpha1.Binding{
 		ObjectMeta: metav1.ObjectMeta{Name: testBindingName, Namespace: testNamespace},
@@ -2098,6 +2111,53 @@ func TestReconcileBindingFailsWithInstanceAsyncOngoing(t *testing.T) {
 	}
 }
 
+func TestReconcileBindingInstanceNotReady(t *testing.T) {
+	fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t)
+
+	fakeBrokerClient.CatalogClient.RetCatalog = getTestCatalog()
+
+	fakeKubeClient.AddReactor("get", "namespaces", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+		return true, &v1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				UID: types.UID("test_ns_uid"),
+			},
+		}, nil
+	})
+
+	sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker())
+	sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass())
+	sharedInformers.Instances().Informer().GetStore().Add(getTestInstance())
+
+	binding := &v1alpha1.Binding{
+		ObjectMeta: metav1.ObjectMeta{Name: testBindingName, Namespace: testNamespace},
+		Spec: v1alpha1.BindingSpec{
+			InstanceRef: v1.LocalObjectReference{Name: testInstanceName},
+			OSBGUID:     bindingGUID,
+		},
+	}
+
+	testController.reconcileBinding(binding)
+
+	if _, ok := fakeBrokerClient.Bindings[fakebrokerapi.BindingsMapKey(instanceGUID, bindingGUID)]; ok {
+		t.Fatalf("Unexpected broker binding call")
+	}
+
+	actions := fakeCatalogClient.Actions()
+	assertNumberOfActions(t, actions, 1)
+
+	// There should only be one action that says binding was created
+	updatedBinding := assertUpdateStatus(t, actions[0], binding)
+	assertBindingReadyFalse(t, updatedBinding)
+
+	events := getRecordedEvents(testController)
+	assertNumEvents(t, events, 1)
+
+	expectedEvent := api.EventTypeWarning + " " + errorInstanceNotReadyReason + " " + `Binding cannot begin because referenced instance "test-ns/test-instance" is not ready`
+	if e, a := expectedEvent, events[0]; e != a {
+		t.Fatalf("Received unexpected event: %v", a)
+	}
+}
+
 func TestReconcileBindingNamespaceError(t *testing.T) {
 	fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t)
 
@@ -2499,6 +2559,36 @@ func TestCatalogConversionMultipleServiceClasses(t *testing.T) {
 
 }
 
+func TestIsBrokerReady(t *testing.T) {
+	cases := []struct {
+		name  string
+		input *v1alpha1.Instance
+		ready bool
+	}{
+		{
+			name:  "ready",
+			input: getTestInstanceWithStatus(v1alpha1.ConditionTrue),
+			ready: true,
+		},
+		{
+			name:  "no status",
+			input: getTestInstance(),
+			ready: false,
+		},
+		{
+			name:  "not ready",
+			input: getTestInstanceWithStatus(v1alpha1.ConditionFalse),
+			ready: false,
+		},
+	}
+
+	for _, tc := range cases {
+		if e, a := tc.ready, isInstanceReady(tc.input); e != a {
+			t.Errorf("%v: expected result %v, got %v", tc.name, e, a)
+		}
+	}
+}
+
 // newTestController creates a new test controller injected with fake clients
 // and returns:
 //