Making the controller handle unexpected provision results

Fixes kubernetes-retired#914

pkg/brokerapi/fake/server/convert_catalog.go

@@ -17,41 +17,46 @@ limitations under the License.
 package server
 
 import (
-	pkgbrokerapi "github.com/kubernetes-incubator/service-catalog/pkg/brokerapi"
 	pivbrokerapi "github.com/pivotal-cf/brokerapi"
+	osb "github.com/pmorie/go-open-service-broker-client/v2"
 )
 
 // ConvertCatalog converts a (github.com/kubernetes-incubator/service-catalog/pkg/brokerapi).Catalog
 // to an array of brokerapi.Services
-func ConvertCatalog(cat *pkgbrokerapi.Catalog) []pivbrokerapi.Service {
+func ConvertCatalog(cat *osb.CatalogResponse) []pivbrokerapi.Service {
 	ret := make([]pivbrokerapi.Service, len(cat.Services))
 	for i, svc := range cat.Services {
 		ret[i] = convertService(svc)
 	}
 	return ret
 }
 
-func convertService(svc *pkgbrokerapi.Service) pivbrokerapi.Service {
+func convertService(svc osb.Service) pivbrokerapi.Service {
+	updateable := false
+	if svc.PlanUpdatable != nil {
+		updateable = *svc.PlanUpdatable
+	}
 	return pivbrokerapi.Service{
 		ID:            svc.ID,
 		Name:          svc.Name,
 		Description:   svc.Description,
 		Bindable:      svc.Bindable,
 		Tags:          svc.Tags,
-		PlanUpdatable: svc.PlanUpdateable,
+		PlanUpdatable: updateable,
 		Plans:         convertPlans(svc.Plans),
 		// TODO: convert Requires, Metadata, DashboardClient
 	}
 }
 
-func convertPlans(plans []pkgbrokerapi.ServicePlan) []pivbrokerapi.ServicePlan {
+func convertPlans(plans []osb.Plan) []pivbrokerapi.ServicePlan {
 	ret := make([]pivbrokerapi.ServicePlan, len(plans))
 	for i, plan := range plans {
+
 		ret[i] = pivbrokerapi.ServicePlan{
 			ID:          plan.ID,
 			Name:        plan.Name,
 			Description: plan.Description,
-			Free:        &plan.Free,
+			Free:        plan.Free,
 			Bindable:    plan.Bindable,
 			// TODO: convert Metadata
 		}

pkg/controller/controller_instance_test.go

@@ -31,6 +31,8 @@ import (
 
 	checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1"
 	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1"
+	// "github.com/kubernetes-incubator/service-catalog/pkg/brokerapi"
+	fakebrokerserver "github.com/kubernetes-incubator/service-catalog/pkg/brokerapi/fake/server"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -581,6 +583,81 @@ 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 one of the
+// "expected" response codes in the OSB API spec for provision.
+// See https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-2 for
+// the list of expected codes and a description of what we should do if another code is returned
+func TestReconcileInstanceAsynchronousUnsupportedBrokerError(t *testing.T) {
+	const (
+		brokerUsername = "testbrokeruser"
+		brokerPassword = "testbrokerpass"
+	)
+	controllerItems, err := newTestControllerWithBrokerServer(brokerUsername, brokerPassword)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer controllerItems.Close()
+
+	fakeKubeClient := controllerItems.FakeKubeClient
+	fakeCatalogClient := controllerItems.FakeCatalogClient
+	fakeBrokerServerHandler := controllerItems.BrokerServerHandler
+	testController := controllerItems.Controller
+	sharedInformers := controllerItems.Informers
+
+	fakeBrokerServerHandler.Catalog = fakebrokerserver.ConvertCatalog(getTestCatalog())
+
+	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())
+
+	// Make the provision return an error that is "unexpected"
+	fakeBrokerServerHandler.ProvisionRespError = errors.New("test provision error")
+	instance := getTestInstance()
+
+	// there should be nothing that is polling since no instances have been reconciled yet
+	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)
+
+	// there should be 1 request to the provision endpoint
+	numProvReqs := len(fakeBrokerServerHandler.ProvisionRequests)
+	if numProvReqs != 1 {
+		t.Fatalf("%d provision requests were made, expected 1", numProvReqs)
+	}
+
+	// The item should not have been added to the polling queue for later processing
+	if testController.pollingQueue.Len() != 0 {
+		t.Fatalf("Expected polling queue to be empty")
+	}
+	assertAsyncOpInProgressFalse(t, updatedInstance)
+	assertInstanceReadyFalse(t, updatedInstance)
+	assertInstanceReadyCondition(t, updatedInstance, v1alpha1.ConditionFalse)
+}
+
 func TestReconcileInstanceNamespaceError(t *testing.T) {
 	fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, noFakeActions())