Making the controller handle unexpected provision results

Fixes kubernetes-retired#914

pkg/controller/controller_instance.go

@@ -163,12 +163,16 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 		if err != nil {
 			s := fmt.Sprintf("Error provisioning Instance \"%s/%s\" of ServiceClass %q at Broker %q: %s", instance.Namespace, instance.Name, serviceClass.Name, brokerName, err)
 			glog.Warning(s)
-			c.updateInstanceCondition(
+			if err := c.updateInstanceCondition(
 				instance,
 				v1alpha1.InstanceConditionReady,
 				v1alpha1.ConditionFalse,
 				errorProvisionCalledReason,
-				"Provision call failed. "+s)
+				"Provision call failed. "+s,
+			); err != nil {
+				glog.Errorf("error updating the instance condition on error creating instance (%s)", err)
+				return err
+			}
 			c.recorder.Event(instance, api.EventTypeWarning, errorProvisionCalledReason, s)
 			return err
 		}
@@ -192,13 +196,16 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 			// no other operations against it can start.
 			instance.Status.AsyncOpInProgress = true
 
-			c.updateInstanceCondition(
+			if err := c.updateInstanceCondition(
 				instance,
 				v1alpha1.InstanceConditionReady,
 				v1alpha1.ConditionFalse,
 				asyncProvisioningReason,
 				asyncProvisioningMessage,
-			)
+			); err != nil {
+				glog.Errorf("updating instance condition for asynchronous provision (%s)", err)
+				return err
+			}
 			c.recorder.Eventf(instance, api.EventTypeNormal, asyncProvisioningReason, asyncProvisioningMessage)
 
 			// Actually, start polling this Service Instance by adding it into the polling queue
@@ -209,16 +216,20 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 			}
 			c.pollingQueue.Add(key)
 		} else {
+			// the broker returned a successful, non-asynchronous response
 			glog.V(5).Infof("Successfully provisioned Instance %v/%v of ServiceClass %v at Broker %v: response: %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName, response)
 
 			// TODO: process response
-			c.updateInstanceCondition(
+			if err := c.updateInstanceCondition(
 				instance,
 				v1alpha1.InstanceConditionReady,
 				v1alpha1.ConditionTrue,
 				successProvisionReason,
 				successProvisionMessage,
-			)
+			); err != nil {
+				glog.Errorf("updating instance condition to success (%s)", err)
+				return err
+			}
 			c.recorder.Eventf(instance, api.EventTypeNormal, successProvisionReason, successProvisionMessage)
 		}
 		return nil

pkg/controller/controller_instance_test.go

@@ -557,6 +557,74 @@ func TestReconcileInstanceAsynchronousNoOperation(t *testing.T) {
 	assertInstanceLastOperation(t, updatedInstance, "")
 }
 
+// TestReconcileInstanceAsynchronousUnsupportedBrokerError tests to ensure that, on an asynchronous
+// provision, an Instance's conditions get set with a Broker failure that is not OSB API spec
+// compliant
+func TestReconcileInstanceAsynchronousUnsupportedBrokerError(t *testing.T) {
+	fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t)
+
+	fakeBrokerClient.CatalogClient.RetCatalog = getTestCatalog()
+	fakeBrokerClient.InstanceClient.DashboardURL = testDashboardURL
+
+	fakeKubeClient.AddReactor("get", "namespaces", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+		return true, &v1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				UID: types.UID("test_uid_foo"),
+			},
+		}, nil
+	})
+
+	sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker())
+	sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass())
+
+	// Specify an error from the instance client
+	fakeBrokerClient.InstanceClient.ResponseCode = http.StatusInternalServerError
+	// And specify that we want broker to return an operation
+	fakeBrokerClient.InstanceClient.Operation = testOperation
+	instance := getTestInstance()
+
+	if testController.pollingQueue.Len() != 0 {
+		t.Fatalf("Expected the polling queue to be empty")
+	}
+
+	testController.reconcileInstance(instance)
+
+	actions := fakeCatalogClient.Actions()
+	assertNumberOfActions(t, actions, 1)
+
+	// verify no kube resources created.
+	// One single action comes from getting namespace uid
+	kubeActions := fakeKubeClient.Actions()
+	if e, a := 1, len(kubeActions); e != a {
+		t.Fatalf("Unexpected number of actions: expected %v, got %v", e, a)
+	}
+
+	updatedInstance := assertUpdateStatus(t, actions[0], instance)
+	assertInstanceReadyFalse(t, updatedInstance)
+
+	if si, ok := fakeBrokerClient.InstanceClient.Instances[instanceGUID]; !ok {
+		t.Fatalf("Did not find the created Instance in fakeInstanceClient after creation")
+	} else {
+		if len(si.Parameters) > 0 {
+			t.Fatalf("Unexpected parameters, expected none, got %+v", si.Parameters)
+		}
+
+		ns, _ := fakeKubeClient.Core().Namespaces().Get(instance.Namespace, metav1.GetOptions{})
+		if string(ns.UID) != si.OrganizationGUID {
+			t.Fatalf("Unexpected OrganizationGUID: expected %q, got %q", string(ns.UID), si.OrganizationGUID)
+		}
+		if string(ns.UID) != si.SpaceGUID {
+			t.Fatalf("Unexpected SpaceGUID: expected %q, got %q", string(ns.UID), si.SpaceGUID)
+		}
+	}
+
+	// The item should not have been added to the polling queue for later processing
+	if testController.pollingQueue.Len() != 0 {
+		t.Fatalf("Expected the asynchronous instance to end up in the polling queue")
+	}
+	assertAsyncOpInProgressFalse(t, updatedInstance)
+}
+
 func TestReconcileInstanceNamespaceError(t *testing.T) {
 	fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t)