Bind request seg-faults when no credentials are available to the broker for return

[eriknelson]

If a bind request is made against an instance, and no credentials have been returned from the apb during the provision or bind, we end up without a credentials object in etcd (as we should), and the broker actually segfaults somewhere around here:

https://github.com/fusor/ansible-service-broker/blob/master/pkg/broker/broker.go#L386

The segfault is absolutely a bug, because my intent as written was to handle this case, so it's not behaving as expected.

Is it okay for the catalog to request a bind against an instance that hasn't given the broker any credentials, either by choice or because of an error in the apb? How should the broker handle this?

[eriknelson]

From a comment on #60, because it's probably more appropriate to discuss here and I don't want it lost:

Because credentials can now come out of a provision or a bind call, we need to chat about the various scenarios that could arise and how to react to each. Ex:

Credentials returned:
1) prov: T, bind: F -> pass back prov creds from bind, we do this today

2) prov: F, bind: T -> pass back bind creds, basically the same as the above

3) prov: T, bind: T -> First, is this okay? If so, how do we handle this? Merge one onto the other? Which 
  takes precendence if there is a key conflict?

4) prov: F, bind F -> Is this okay? I lean no. If a bind is basically defined by the return of credentials, if 
  there are none available, this is bad. Can we think of a case where a bind makes sense that 
  *doesn't* return anything to the consuming application? NOTE: This currently segfaults. Needs to get 
   fixed and tested against our decision here.

@rthallisey @jmrodri @jwmatthews thoughts?

[rthallisey]

Since we don't want to block on a bind, we could create a go routine that monitors the apb for the bind creds. Then, we can return a 201 with the assumption that the bind operation was executed and will be completed some time in the future.

go func() {
   if err != nil {
      monitorOutput(...)
    }()
return 201

https://github.com/fusor/ansible-service-broker/pull/60/files#diff-354ba5b1a51aebd68d32f9772b760a2aR57

[eriknelson]

@rthallisey There's a specific protocol for handling non-blocking comms between the catalog and broker, and the spec doesn't define async for bind in the same way it does for provision and update. There's recent discussion around async bind; we actually had to entirely disable the oc run ... bind operation because the catalog was timing out if it took too long to execute. It's a distinct (albeit related) problem we should capture in its own issue. I think it should be handled with the same work engine pattern employed for provision (if it needs to get implemented; to your point, it absolutely does).

My question assumes both provision and bind have run to completion. We have to consider the correct way the broker should handle each of those states. I'm particularly interested in 3) and 4).

I'd propose: 3) is valid. We merge bind onto provision and return the result. If there's a key collision, we prefer the value from the bind object. Open to dissenting opinion here. 4) is not valid. Does this warrant a 4xx response from the broker because it's considered some kind of a bad request from the catalog? I'm having a hard time coming up with a case where an application is bound to an instance where nothing is delivered to the binding consumer. Semantically, that's a NOOP, and probably means something has either gone wrong, or you're doing something you probably don't intend to be doing.

[jmrodri]

I agree 4) is invalid. We should always return a bind during a bind request. If one is not found, then we should return a 404 NOT FOUND. Any code except for 200, 201, 409, 422, will be considered an error.

If the provision didn't create a binding, and the call to apb binding.yaml doesn't return one, that's an error. The catalog thinks there should be a binding because bindable must have been true.

The spec will then send an unbind just in case:

Responses with any other status code will be interpreted as a failure and an unbind request will be sent to the broker to prevent an orphan being created on the broker. Brokers can include a user-facing message in the description field; for details see Broker Errors.

[jmrodri]

For scenario 3) I would merge the values of provision and bind with the bind values having precedence. If provision returns keys A, B, C and bind returns D, E, F, the resulting response should be A, B, C, D, E, F. If provision returns A, B, C, and bind returns B, C, D. The resulting response should be A, B, C, D with B & C being the values from the bind.

[jmrodri]

Summary

Credentials returned:
1) prov: T, bind: F -> pass back prov creds from bind, we do this today

2) prov: F, bind: T -> pass back bind creds, basically the same as the above

3) prov: T, bind: T -> pass back the merged creds with the bind values taking precendence

4) prov: F, bind F -> ERROR condition, return 404 NOT FOUND

[rthallisey]

I think it's safe to close this