Skip to content

Commit

Permalink
Revert "Return correct response for creating instances and bindings t…
Browse files Browse the repository at this point in the history 
…hat already exist"

This reverts commit 5ae429d.
  • Loading branch information
@jaymccon
jaymccon committed Oct 6, 2021
1 parent 764edfc commit ce13226
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 147 deletions.
202 changes: 61 additions & 141 deletions pkg/broker/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package broker

import (
"fmt"
"github.com/aws/aws-sdk-go/aws/session"
"net/http"
"strings"

Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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" {
Expand All @@ -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) {
Expand All @@ -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)
Expand Down Expand Up @@ -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 != "" {
Expand All @@ -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
Expand All @@ -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`
Expand Down
7 changes: 1 addition & 6 deletions pkg/serviceinstance/serviceinstance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit ce13226

Please sign in to comment.