Skip to content

Remove ambiguity on provisioning status code#717

Merged
rsampaio merged 8 commits intocloudfoundry:masterfrom
rsampaio:rsampaio-provision-status-code
Nov 24, 2020
Merged

Remove ambiguity on provisioning status code#717
rsampaio merged 8 commits intocloudfoundry:masterfrom
rsampaio:rsampaio-provision-status-code

Conversation

@rsampaio
Copy link
Contributor

@rsampaio rsampaio commented Jun 9, 2020

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:

  • The swagger.yaml doc has been updated with any required changes
  • The openapi.yaml doc has been updated with any required changes

Fixes Issue #709

@Samze
Copy link
Contributor

Samze commented Jun 9, 2020

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
Copy link
Contributor

fmui commented Jun 23, 2020

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 rsampaio force-pushed the rsampaio-provision-status-code branch from 6b4e9e2 to fe2650e Compare July 7, 2020 15:04
tinygrasshopper
tinygrasshopper previously approved these changes Jul 7, 2020
@rsampaio
Copy link
Contributor Author

@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.

fmui
fmui previously approved these changes Sep 15, 2020
Copy link
Contributor

@Samze Samze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed response @rsampaio!

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.

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

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.

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.

@rsampaio
Copy link
Contributor Author

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.

fmui
fmui previously approved these changes Oct 27, 2020
The specification for synchronous provisioning says `201 Created` status code
should be returned when for provisioning and bind and `200 OK` only for
update, unbind and deprovisioning requests.

This also reverts cloudfoundry#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.
@rsampaio rsampaio force-pushed the rsampaio-provision-status-code branch from a4f1ca5 to 0dae18c Compare October 27, 2020 15:08
@rsampaio
Copy link
Contributor Author

@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!

@rsampaio rsampaio merged commit 5066559 into cloudfoundry:master Nov 24, 2020
@rsampaio rsampaio deleted the rsampaio-provision-status-code branch November 24, 2020 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants