openservicebrokerapi / servicebroker

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

Remove ambiguity on provisioning status code #717

Closed rsampaio closed 3 years ago

rsampaio commented 4 years ago

What is the problem this PR solves?

The specification for synchronous provisioning says 201 Created status code should be returned for provisioning and bind and 200 OK only for the update, unbind, and deprovision requests. This change should remove ambiguity on the provisioning and binding requests and converge to a single way to verify if a service instance has completed the provisioning step by calling the last_operation endpoint.

This also reverts #561 and assumes duplicated requests will return 409 Conflict for the same unique UUID and it should be advised to platforms to start polling the last_operation endpoint to wait for completion.

Checklist:

Fixes Issue #709

Samze commented 4 years ago

I agree the spec looks inconsistent right now with this statement.

To execute a request synchronously, the Service Broker need only return the usual status codes: 201 Created for provision and bind, and 200 OK for update, unbind, and deprovision.

However I think replacing 200 with 409 for service brokers that receive a request for an instance that already exist would be a breaking change. For example if a newly written broken with this guidance returned a 409 then older Cloud Foundry platforms would interrupt that as an error and not a no-op but fail instead.

It's also worth mentioning that not all brokers or platforms support last_operation as it was an addition to the specification, so there needs to be a synchronous only mechanism for handling the possibility that a platform might call provision/bind multiple times.

I wonder if instead of removing 200, we should change that statement to include 200 as a valid response for a synchronous provision/bind? And then perhaps this is something that can be cleaned up in a future version of the spec?

fmui commented 4 years ago

I agree with @Samze, this is probably a breaking change for v2.x. We cannot remove the 200 and with that a (probably unused) "feature". We can discourage its use, though.

rsampaio commented 4 years ago

@Samze I added some clarifying points around the synchronous requests and the use of 200 code by existent brokers. I was also reflecting on the goal of this PR and I believe the intent is to encourage the use of last_operation for provisioning status and the use of 409 Conflict for duplicated requests (with the same UUIDs and parameters) since it is more aligned with the HTTP semantics and ultimately encourage the adoption of asynchronous patterns instead of synchronous across the board. I made sure to mention that returning 200 OK is still supported but not recommended.

I'm curious to hear your thoughts.

rsampaio commented 4 years ago

No worries @Samze!

Just so I understand, the goal of this is to encourage platforms to use the broker response of 409 conflict mechanism instead of 200? I think maybe I'm not quite getting how this works for the synchronous case - I think This response is deprecated in favor of the last_operation endpoint is throwing me off.

That is correct and it only applies to the asynchronous case.

The asynchronous case:

  1. Platform provision to broker, broker responses 202
  2. Platform provision to again broker with same request, broker returns 409
  3. Platform hits last operation endpoint....continues until success

Exactly!

The synchronous case

  1. Platform provision to broker, broker responses 201
  2. Platform provision to again broker with same request, broker returns 409
  3. Platform hits last operation endpoint.. ? In this case the broker initially returned 201, so may not support the last operation endpoint.

In this case, when the broker returns a 201 the service instance should be already created and there is no reason to call last_operation at all, the change is really intended to clarify the response of requests containing the same uuid for provisioning

Perhaps we should deprecated 200 in favour of 200 response? The only issue is that currently 409 and 200 are distinguished by 409 having different parameters and 200 having the same parameters.

Maybe you mean 200 in favor of 201 and I agree, I believe the broker should return 409 for duplicated uuid and not only for duplicated parameters.

rsampaio commented 3 years ago

@Samze just rebased and fixed the conflict, linter was complaining about a table and I fixed that as well, this is definitely ready to go.

@fmui thanks for the review!