search

openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 434 forks source link

Spec requires statefulness if binding generates a password #291

Closed wryun closed 5 years ago

wryun commented 7 years ago

Not taking a position on whether this is a good or bad thing, but I believe there is some desire for brokers to be able to be stateless.

Unfortunately, because the spec says that one can do multiple PUTs to the same binding as long as they have the same parameters, I believe this forces brokers that generate passwords to be stateful (i.e. they have to keep the password so they can return it for subsequent binding requests). See https://github.com/cloudfoundry-community/cf-mysql-java-broker/issues/4

Alternatively, either the user or the marketplace could generate the password and pass it to the broker as a parameter, which seems somewhat problematic either way. The first, because the user will likely have to put a secret in their application configuration (hard/annoying to properly protect), and the second because it would imply that the marketplace needs to 'know' that a particular provider has to have the password inserted into their parameters block (and stored). We actually have an internal non-OSB system which sometimes uses CloudFormation where we have a magic 'password' variable which we provide if it's a parameter. It works, but it's pretty ugly...

If we desire brokers to be non-stateful, I would suggest modifying the OSB spec to demand that the marketplace provides a consistent per binding_id randomly generated seed on every binding PUT. i.e. have the statefulness in the marketplace, but codify it in the contract. Perhaps this could involve adding seed to the possible values of requires in the catalog, and then providing seed inside the bind_resource on binding PUTs. Or more radically, simply change the PUT into a POST, and don't provide a binding_id... obviously this would be a big compatibility break, but are there other reasons not to do this?

duglin commented 7 years ago

I think the problem is a bit easier to solve. If you look at the results section it says:

409 Conflict | MUST be returned if a service instance with the same id already exists but with different attributes. The expected response body is {}, though the description field MAY be used to return a user-facing error message, as described in Broker Errors.

So, what I would suggest is that we modify the text here and in the 200 section to make it clear that the idea of checking against existing instances/bindings is optional in a stateless world. Which means that if a broker can't check the request against the existing resource then it MUST return a 409.

@shalako would this align with what today's stateless brokers do?

shalako commented 7 years ago

Guidance in the spec as to what a broker should do when receiving a duplicate request is an edge case; this should only happen if there is a bug in the marketplace.

That said, if the broker knows that it has already created a resource with the same id, but can't check whether the params were identical, should return a 409 as Doug suggests.

On Aug 18, 2017, at 5:46 AM, Doug Davis notifications@github.com wrote:

I think the problem is a bit easier to solve. If you look at the results section it says:

409 Conflict | MUST be returned if a service instance with the same id already exists but with different attributes. The expected response body is {}, though the description field MAY be used to return a user-facing error message, as described in Broker Errors.

So, what I would suggest is that we modify the text here and in the 200 section to make it clear that the idea of checking against existing instances/bindings is optional in a stateless world. Which means that if a broker can't check the request against the existing resource then it MUST return a 409.

@shalako would this align with what today's stateless brokers do?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

wryun commented 7 years ago

Why does it mean that it should only happen if there's a bug in the marketplace? Because of this aspect of the spec, our current design is that the marketplace 're-requests' credentials for the same binding id (since the broker is assumed to be the source of truth). Is there a problem with this approach?

@duglin sounds ok. I'm not entirely comfortable with changing the usual PUT semantics (this is why POSTs make sense?), or the marketplace having to persist the password, but I guess the seed approach I was advocating really does the same thing split across two services (just requires the broker secret as well as the marketplace secret).

nilebox commented 7 years ago

what a broker should do when receiving a duplicate request is an edge case; this should only happen if there is a bug in the marketplace

I found this statement confusing. The broker is whether considered to be stateful and idempotent, or not.

So far it was reiterated many times that it's nice to support stateless brokers, which means that it's marketplace's job to keep the broker's state and prevent concurrent and duplicate requests. Justification for duplicate check in broker as a way to mitigate bugs in marketplace sound weird to me then, I would just remove the requirement as previously suggested in https://github.com/openservicebrokerapi/servicebroker/issues/260.

mattmcneeney commented 5 years ago

From reading the spec, it looks like brokers do not have to remember the credentials they gave out. As Doug pointed out, they can simply return a 409. Brokers that do want to return these credentials multiple times can use the new GET /v2/service_instances/:instance_guid/service_bindings/:binding_guid endpoint.