don't use panics as flow-control (#282)

* don't use panics as flow-control

brokerapi/brokers/account_managers/service_account_manager.go

@@ -144,8 +144,16 @@ func (sam *ServiceAccountManager) DeleteCredentials(binding models.ServiceBindin
 }
 
 func (b *ServiceAccountManager) BuildInstanceCredentials(bindRecord models.ServiceBindingCredentials, instanceRecord models.ServiceInstanceDetails) (map[string]string, error) {
-	bindDetails := bindRecord.GetOtherDetails()
-	instanceDetails := instanceRecord.GetOtherDetails()
+	bindDetails, err := bindRecord.GetOtherDetails()
+	if err != nil {
+		return nil, err
+	}
+
+	instanceDetails, err := instanceRecord.GetOtherDetails()
+	if err != nil {
+		return nil, err
+	}
+
 	return utils.MergeStringMaps(bindDetails, instanceDetails), nil
 }
 

brokerapi/brokers/account_managers/sql_account_manager.go

@@ -180,8 +180,15 @@ func (sam *SqlAccountManager) pollOperationUntilDone(op *googlecloudsql.Operatio
 }
 
 func (b *SqlAccountManager) BuildInstanceCredentials(bindRecord models.ServiceBindingCredentials, instanceRecord models.ServiceInstanceDetails) (map[string]string, error) {
-	instanceDetails := instanceRecord.GetOtherDetails()
-	bindDetails := bindRecord.GetOtherDetails()
+	instanceDetails, err := instanceRecord.GetOtherDetails()
+	if err != nil {
+		return nil, err
+	}
+
+	bindDetails, err := bindRecord.GetOtherDetails()
+	if err != nil {
+		return nil, err
+	}
 
 	service, err := broker.GetServiceById(instanceRecord.ServiceId)
 	if err != nil {

brokerapi/brokers/broker_base/broker_base.go

@@ -15,6 +15,8 @@
 package broker_base
 
 import (
+	"errors"
+
 	"code.cloudfoundry.org/lager"
 	"github.com/GoogleCloudPlatform/gcp-service-broker/brokerapi/brokers/models"
 
@@ -68,5 +70,5 @@ func (b *BrokerBase) DeprovisionsAsync() bool {
 // determine if the workflow is a provision or deprovision flow based off the
 // type of the most recent operation.
 func (b *BrokerBase) LastOperationWasDelete(instanceId string) (bool, error) {
-	panic("Can't check last operation on a synchronous service")
+	return false, errors.New("can't check last operation on a synchronous service")
 }

brokerapi/brokers/gcp_service_broker.go

@@ -396,7 +396,7 @@ func (gcpBroker *GCPServiceBroker) lastOperationAsync(instanceId string, service
 	// no error and we're done! Delete from the SB database if this was a delete flow and return success
 	deleteFlow, err := service.LastOperationWasDelete(instanceId)
 	if err != nil {
-		return brokerapi.LastOperation{State: brokerapi.Succeeded}, fmt.Errorf("Couldn't determine if provision or deprovision flow, this may leave orphaned resources, contact your operator for cleanup")
+		return brokerapi.LastOperation{State: brokerapi.Succeeded}, fmt.Errorf("Couldn't determine if provision or deprovision flow, this may leave orphaned resources, contact your operator for cleanup: %s", err)
 	}
 	if deleteFlow {
 		err = db_service.DeleteServiceInstanceDetailsById(instanceId)

brokerapi/brokers/models/db.go

@@ -23,31 +23,26 @@ import (
 type ServiceBindingCredentials ServiceBindingCredentialsV1
 
 // GetOtherDetails returns an unmarshaled version of the OtherDetails field
-// or panics.
-func (sbc ServiceBindingCredentials) GetOtherDetails() map[string]string {
+// or errors.
+func (sbc ServiceBindingCredentials) GetOtherDetails() (map[string]string, error) {
 	var creds map[string]string
-	if err := json.Unmarshal([]byte(sbc.OtherDetails), &creds); err != nil {
-		panic(err)
-	}
-	return creds
+	err := json.Unmarshal([]byte(sbc.OtherDetails), &creds)
+	return creds, err
 }
 
-// ServiceInstanceDetails returns an unmarshaled version of the OtherDetails field
-// or panics.
+// ServiceInstanceDetails holds information about provisioned services 
 type ServiceInstanceDetails ServiceInstanceDetailsV1
 
 // GetOtherDetails returns an unmarshaled version of the OtherDetails field
-// or panics.
-func (si ServiceInstanceDetails) GetOtherDetails() map[string]string {
+// or errors.
+func (si ServiceInstanceDetails) GetOtherDetails() (map[string]string, error) {
 	var instanceDetails map[string]string
-	// if the instance has access details saved
-	if si.OtherDetails != "" {
-		if err := json.Unmarshal([]byte(si.OtherDetails), &instanceDetails); err != nil {
-			panic(err)
-		}
+	if si.OtherDetails == "" {
+		return instanceDetails, nil
 	}
-	return instanceDetails
 
+	err := json.Unmarshal([]byte(si.OtherDetails), &instanceDetails)
+	return instanceDetails, err
 }
 
 // ProvisionRequestDetails holds user-defined properties passed to a call

integration_tests/async_integration_test.go

@@ -87,11 +87,7 @@ var _ = Describe("AsyncIntegrationTests", func() {
 		fakes.SetUpTestServices()
 
 		brokerConfig, err = config.NewBrokerConfigFromEnv()
-
-		if err != nil {
-			logger.Error("error", err)
-			panic(err.Error())
-		}
+		Expect(err).NotTo(HaveOccurred())
 
 		instance_name = generateInstanceName(brokerConfig.ProjectId, "-")
 

integration_tests/integration_test.go

@@ -60,7 +60,7 @@ type genericService struct {
 	instanceId             string
 	serviceExistsFn        func(bool) bool
 	cleanupFn              func()
-	serviceMetadataSavedFn func(string) bool
+	serviceMetadataSavedFn func(string) (bool, error)
 }
 
 type iamService struct {
@@ -69,7 +69,7 @@ type iamService struct {
 	planId    string
 }
 
-func getAndUnmarshalInstanceDetails(instanceId string) map[string]string {
+func getAndUnmarshalInstanceDetails(instanceId string) (map[string]string, error) {
 	instanceRecord, _ := db_service.GetServiceInstanceDetailsById(instanceId)
 	return instanceRecord.GetOtherDetails()
 }
@@ -98,7 +98,9 @@ func testGenericService(brokerConfig *config.BrokerConfig, gcpBroker *GCPService
 	if params.serviceExistsFn != nil {
 		Expect(params.serviceExistsFn(true)).To(BeTrue())
 	}
-	Expect(params.serviceMetadataSavedFn(params.instanceId)).To(BeTrue())
+	metadataSaved, err := params.serviceMetadataSavedFn(params.instanceId)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(metadataSaved).To(BeTrue())
 
 	//
 	// Bind
@@ -168,10 +170,10 @@ func testIamBasedService(brokerConfig *config.BrokerConfig, gcpBroker *GCPServic
 		instanceId:       "iam-instance",
 		bindingId:        "iam-instance",
 		rawBindingParams: json.RawMessage{},
-		serviceMetadataSavedFn: func(instanceId string) bool {
+		serviceMetadataSavedFn: func(instanceId string) (bool, error) {
 			// Metadata should be empty, there is no additional information required
-			instanceDetails := getAndUnmarshalInstanceDetails(instanceId)
-			return len(instanceDetails) == 0
+			instanceDetails, err := getAndUnmarshalInstanceDetails(instanceId)
+			return len(instanceDetails) == 0, err
 		},
 	}
 
@@ -288,9 +290,9 @@ var _ = Describe("LiveIntegrationTests", func() {
 
 					return err == nil
 				},
-				serviceMetadataSavedFn: func(instanceId string) bool {
-					instanceDetails := getAndUnmarshalInstanceDetails(instanceId)
-					return instanceDetails["dataset_id"] != ""
+				serviceMetadataSavedFn: func(instanceId string) (bool, error) {
+					instanceDetails, err := getAndUnmarshalInstanceDetails(instanceId)
+					return instanceDetails["dataset_id"] != "", err
 				},
 				cleanupFn: func() {
 					err := service.Datasets.Delete(brokerConfig.ProjectId, instance_name).Do()
@@ -331,9 +333,9 @@ var _ = Describe("LiveIntegrationTests", func() {
 
 					return err == nil && len(instances) == 1 && instances[0].Name == bigtableInstanceName
 				},
-				serviceMetadataSavedFn: func(instanceId string) bool {
-					instanceDetails := getAndUnmarshalInstanceDetails(instanceId)
-					return instanceDetails["instance_id"] != ""
+				serviceMetadataSavedFn: func(instanceId string) (bool, error) {
+					instanceDetails, err := getAndUnmarshalInstanceDetails(instanceId)
+					return instanceDetails["instance_id"] != "", err
 				},
 				cleanupFn: func() {
 					err := service.DeleteInstance(ctx, bigtableInstanceName)
@@ -363,9 +365,9 @@ var _ = Describe("LiveIntegrationTests", func() {
 
 					return err == nil
 				},
-				serviceMetadataSavedFn: func(instanceId string) bool {
-					instanceDetails := getAndUnmarshalInstanceDetails(instanceId)
-					return instanceDetails["bucket_name"] != ""
+				serviceMetadataSavedFn: func(instanceId string) (bool, error) {
+					instanceDetails, err := getAndUnmarshalInstanceDetails(instanceId)
+					return instanceDetails["bucket_name"] != "", err
 				},
 				cleanupFn: func() {
 					bucket := service.Bucket(instance_name)
@@ -396,9 +398,9 @@ var _ = Describe("LiveIntegrationTests", func() {
 					exists, err := topic.Exists(context.Background())
 					return exists && err == nil
 				},
-				serviceMetadataSavedFn: func(instanceId string) bool {
-					instanceDetails := getAndUnmarshalInstanceDetails(instanceId)
-					return instanceDetails["topic_name"] != ""
+				serviceMetadataSavedFn: func(instanceId string) (bool, error) {
+					instanceDetails, err := getAndUnmarshalInstanceDetails(instanceId)
+					return instanceDetails["topic_name"] != "", err
 				},
 				cleanupFn: func() {
 					err := topic.Delete(context.Background())