Revert "Return correct response for creating instances and bindings that already exist"

This reverts commit 5ae429d.

Author: jaymccon

pkg/broker/api.go

@@ -2,7 +2,6 @@ package broker
 
 import (
 	"fmt"
-	"github.com/aws/aws-sdk-go/aws/session"
 	"net/http"
 	"strings"
 
@@ -17,11 +16,6 @@ import (
 	prom "github.com/prometheus/client_golang/prometheus"
 )
 
-const (
-	instanceId = "INSTANCE_ID"
-	bindingId = "BINDING_ID"
-)
-
 // GetCatalog is executed on a /v2/catalog/ osb api call
 // https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#catalog-management
 func (b *AwsBroker) GetCatalog(c *broker.RequestContext) (*broker.CatalogResponse, error) {
@@ -131,31 +125,17 @@ func (b *AwsBroker) Provision(request *osb.ProvisionRequest, c *broker.RequestCo
 		desc := fmt.Sprintf("Failed to get the service instance %s: %v", instance.ID, err)
 		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
 	} else if i != nil {
+		// TODO: This logic could use some love. The docs state that 200 OK MUST be
+		// returned if the service instance already exists, is fully provisioned,
+		// and the requested parameters are identical to the existing service
+		// instance. Right now, this doesn't check whether the instance is fully
+		// provisioned, and the reflect.DeepEqual check in Match will return false
+		// if the parameter order is different.
 		if i.Match(instance) {
 			glog.Infof("Service instance %s already exists.", instance.ID)
-
-			glog.Infof("Checking if service instance %s is fully provisioned.", instance.ID)
-			cfnSvc := getCfn(b, instance.Params)
-
 			response := broker.ProvisionResponse{}
-			status, _, err := getStackStatusAndReason(cfnSvc, i.StackID)
-
-			switch {
-			case err != nil:
-				desc := fmt.Sprintf("Failed to get the stack %s: %v", i.StackID, err)
-				return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
-			case instanceIsFullyProvisioned(status):
-				glog.Infof("Service instance %s is fully provisioned.", instance.ID)
-				response.Exists = true
-				return &response, nil
-			case instanceProvisioningIsInProgress(status):
-				glog.Infof("Service instance %s provisioning is in progress.", instance.ID)
-				response.Async = true
-				return &response, nil
-			default:
-				glog.Infof("Service instance %s provisioning failed.", instance.ID)
-				return &response, newHTTPStatusCodeError(http.StatusBadRequest, "CloudFormationError", *getCfnError(i.StackID, cfnSvc))
-			}
+			response.Exists = true
+			return &response, nil
 		}
 		glog.V(10).Infof("i=%+v instance=%+v", *i, *instance)
 		desc := fmt.Sprintf("Service instance %s already exists but with different attributes.", instance.ID)
@@ -291,33 +271,38 @@ func (b *AwsBroker) LastOperation(request *osb.LastOperationRequest, c *broker.R
 	}
 
 	// Get the CFN stack status
-	cfnSvc := getCfn(b, instance.Params)
-
-	status, reason, err := getStackStatusAndReason(cfnSvc, instance.StackID)
+	cfnSvc := b.Clients.NewCfn(b.GetSession(b.keyid, b.secretkey, b.region, b.accountId, b.profile, instance.Params))
+	resp, err := cfnSvc.Client.DescribeStacks(&cloudformation.DescribeStacksInput{
+		StackName: aws.String(instance.StackID),
+	})
 	if err != nil {
-		return nil, err
+		desc := fmt.Sprintf("Failed to describe the CloudFormation stack %s: %v", instance.StackID, err)
+		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
 	}
+	status := aws.StringValue(resp.Stacks[0].StackStatus)
+	reason := aws.StringValue(resp.Stacks[0].StackStatusReason)
+	glog.V(10).Infof("stack=%s status=%s reason=%s", instance.StackID, status, reason)
 
 	response := broker.LastOperationResponse{}
-	if instanceIsFullyProvisioned(status) {
+	if status == cloudformation.StackStatusCreateComplete ||
+		status == cloudformation.StackStatusDeleteComplete ||
+		status == cloudformation.StackStatusUpdateComplete {
 		response.State = osb.StateSucceeded
 		if status == cloudformation.StackStatusDeleteComplete {
 			// If the resources were successfully deleted, try to delete the instance
 			if err := b.db.DataStorePort.DeleteServiceInstance(instance.ID); err != nil {
 				glog.Errorf("Failed to delete the service instance %s: %v", instance.ID, err)
 			}
 		}
-	} else if instanceProvisioningIsInProgress(status) {
+	} else if strings.HasSuffix(status, "_IN_PROGRESS") && !strings.Contains(status, "ROLLBACK") {
 		response.State = osb.StateInProgress
 	} else {
 		glog.Errorf("CloudFormation stack %s failed with status %s: %s", instance.StackID, status, reason)
-		response := broker.LastOperationResponse{}
 		response.State = osb.StateFailed
 		response.Description = getCfnError(instance.StackID, cfnSvc)
 		if *response.Description == "" {
 			response.Description = &reason
 		}
-
 		// workaround for https://github.com/kubernetes-incubator/service-catalog/issues/2505
 		originatingIdentity := strings.Split(c.Request.Header.Get("X-Broker-Api-Originating-Identity"), " ")[0]
 		if originatingIdentity == "kubernetes" {
@@ -332,35 +317,6 @@ func (b *AwsBroker) LastOperation(request *osb.LastOperationRequest, c *broker.R
 	return &response, nil
 }
 
-func getCfn(b *AwsBroker, instanceParams map[string]string) CfnClient {
-	return b.Clients.NewCfn(b.GetSession(b.keyid, b.secretkey, b.region, b.accountId, b.profile, instanceParams))
-}
-
-func getStackStatusAndReason(cfnSvc CfnClient, stackId string) (status string, reason string, err error) {
-	resp, err := cfnSvc.Client.DescribeStacks(&cloudformation.DescribeStacksInput{
-		StackName: aws.String(stackId),
-	})
-	if err != nil {
-		desc := fmt.Sprintf("Failed to describe the CloudFormation stack %s: %v", stackId, err)
-		return "", "", newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
-	}
-	status = aws.StringValue(resp.Stacks[0].StackStatus)
-	reason = aws.StringValue(resp.Stacks[0].StackStatusReason)
-	glog.V(10).Infof("stack=%s status=%s reason=%s", stackId, status, reason)
-
-	return
-}
-
-func instanceIsFullyProvisioned(status string) bool {
-	return status == cloudformation.StackStatusCreateComplete ||
-		status == cloudformation.StackStatusDeleteComplete ||
-		status == cloudformation.StackStatusUpdateComplete
-}
-
-func instanceProvisioningIsInProgress(status string) bool {
-	return strings.HasSuffix(status, "_IN_PROGRESS") && !strings.Contains(status, "ROLLBACK")
-}
-
 // Bind is executed when the OSB API receives `PUT /v2/service_instances/:instance_id/service_bindings/:binding_id`
 // (https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#request-4).
 func (b *AwsBroker) Bind(request *osb.BindRequest, c *broker.RequestContext) (*broker.BindResponse, error) {
@@ -383,6 +339,22 @@ func (b *AwsBroker) Bind(request *osb.BindRequest, c *broker.RequestContext) (*b
 		}
 	}
 
+	// Verify that the binding doesn't already exist
+	sb, err := b.db.DataStorePort.GetServiceBinding(binding.ID)
+	if err != nil {
+		desc := fmt.Sprintf("Failed to get the service binding %s: %v", binding.ID, err)
+		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
+	} else if sb != nil {
+		if sb.Match(binding) {
+			glog.Infof("Service binding %s already exists.", binding.ID)
+			response := broker.BindResponse{}
+			response.Exists = true
+			return &response, nil
+		}
+		desc := fmt.Sprintf("Service binding %s already exists but with different attributes.", binding.ID)
+		return nil, newHTTPStatusCodeError(http.StatusConflict, "", desc)
+	}
+
 	// Get the service (this is only required because the USER_KEY_ID and
 	// USER_SECRET_KEY credentials need to be prefixed with the service name for
 	// backward compatibility)
@@ -416,18 +388,11 @@ func (b *AwsBroker) Bind(request *osb.BindRequest, c *broker.RequestContext) (*b
 		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
 	}
 
-	// Verify that the binding doesn't already exist
-	sb, err := b.db.DataStorePort.GetServiceBinding(binding.ID)
+	// Get the credentials from the CFN stack outputs
+	credentials, err := getCredentials(service, resp.Stacks[0].Outputs, b.Clients.NewSsm(sess))
 	if err != nil {
-		desc := fmt.Sprintf("Failed to get the service binding %s: %v", binding.ID, err)
+		desc := fmt.Sprintf("Failed to get the credentials from CloudFormation stack %s: %v", instance.StackID, err)
 		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
-	} else if sb != nil {
-		if sb.Match(binding) {
-			glog.Infof("Service binding %s already exists.", binding.ID)
-			return createBindResponse(service, resp, b, sess, instance, binding)
-		}
-		desc := fmt.Sprintf("Service binding %s already exists but with different attributes.", binding.ID)
-		return nil, newHTTPStatusCodeError(http.StatusConflict, "", desc)
 	}
 
 	if binding.RoleName != "" {
@@ -450,68 +415,6 @@ func (b *AwsBroker) Bind(request *osb.BindRequest, c *broker.RequestContext) (*b
 		binding.PolicyArn = policyArn
 	}
 
-	credentials, err := getBindingCredentials(service, resp, b, sess, instance, binding)
-	if err != nil {
-		return nil, err
-	}
-
-        // Store the binding
-        err = b.db.DataStorePort.PutServiceBinding(*binding)
-        if err != nil {
-                desc := fmt.Sprintf("Failed to store the service binding %s: %v", binding.ID, err)
-                return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
-        }
-
-        b.metrics.Actions.With(
-                prom.Labels{
-                        "action":  "bind",
-                        "service": service.Name,
-                        "plan":    "",
-                }).Inc()
-
-        return &broker.BindResponse{
-                BindResponse: osb.BindResponse{
-                        Credentials: credentials,
-                },
-        }, nil
-}
-
-func createBindResponse(
-	service *osb.Service,
-	resp *cloudformation.DescribeStacksOutput,
-	b *AwsBroker,
-	sess *session.Session,
-	instance *serviceinstance.ServiceInstance,
-	binding *serviceinstance.ServiceBinding) (*broker.BindResponse, error) {
-
-	response := broker.BindResponse{}
-	response.Exists = true
-
-	credentials, err := getBindingCredentials(service, resp, b, sess, instance, binding)
-	if err != nil {
-		return nil, err
-	}
-
-	response.Credentials = credentials
-	return &response, nil
-}
-
-func getBindingCredentials(
-	service *osb.Service,
-	resp *cloudformation.DescribeStacksOutput,
-	b *AwsBroker,
-	sess *session.Session,
-	instance *serviceinstance.ServiceInstance,
-	binding *serviceinstance.ServiceBinding) (map[string]interface{}, error) {
-
-	// Get the credentials from the CFN stack outputs
-	credentials, err := getCredentials(service, resp.Stacks[0].Outputs, b.Clients.NewSsm(sess))
-	if err != nil {
-		desc := fmt.Sprintf("Failed to get the credentials from CloudFormation stack %s: %v", instance.StackID, err)
-		glog.Error(desc)
-		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
-	}
-
 	if bindViaLambda(service) {
 		// Copy instance and binding IDs into credentials to
 		// be used as identifiers for resources we create in
@@ -520,18 +423,35 @@ func getBindingCredentials(
 		// IAM User with this information, and avoid the need
 		// to have persist extra identifiers, or have users
 		// provide them.
-		credentials[instanceId] = binding.InstanceID
-		credentials[bindingId] = binding.ID
+		credentials["INSTANCE_ID"] = binding.InstanceID
+		credentials["BINDING_ID"] = binding.ID
 
 		// Replace credentials with a derived set calculated by a lambda function
 		credentials, err = invokeLambdaBindFunc(sess, b.Clients.NewLambda, credentials, "bind")
 		if err != nil {
-			glog.Error(err)
 			return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", err.Error())
 		}
 	}
 
-	return credentials, nil
+	// Store the binding
+	err = b.db.DataStorePort.PutServiceBinding(*binding)
+	if err != nil {
+		desc := fmt.Sprintf("Failed to store the service binding %s: %v", binding.ID, err)
+		return nil, newHTTPStatusCodeError(http.StatusInternalServerError, "", desc)
+	}
+
+	b.metrics.Actions.With(
+		prom.Labels{
+			"action":  "bind",
+			"service": service.Name,
+			"plan":    "",
+		}).Inc()
+
+	return &broker.BindResponse{
+		BindResponse: osb.BindResponse{
+			Credentials: credentials,
+		},
+	}, nil
 }
 
 // Unbind is executed when the OSB API receives `DELETE /v2/service_instances/:instance_id/service_bindings/:binding_id`

pkg/serviceinstance/serviceinstance.go

@@ -11,13 +11,8 @@ type ServiceInstance struct {
 	StackID   string
 }
 
-// Match returns true if the other service instance has the same attributes.
-// StackID is ignored for correct comparing ServiceInstance got from database and ServiceInstance got from API request
 func (i *ServiceInstance) Match(other *ServiceInstance) bool {
-	return i.ID == other.ID &&
-		i.ServiceID == other.ServiceID &&
-		i.PlanID == other.PlanID &&
-		reflect.DeepEqual(i.Params, other.Params)
+	return reflect.DeepEqual(i, other)
 }
 
 // ServiceBinding represents a service binding.