Support asynchronous bindings (requires #333) #334

mattmcneeney · 2017-10-10T13:58:05Z

See issue #137

This depends on #333

cfdreddbot · 2017-10-10T13:58:07Z

Hey mattmcneeney!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

vaikas · 2017-10-10T15:48:53Z

spec.md

+##### Body
+
+For success responses, the following fields are supported. Others will be
+ignored. For error responses, see [Broker Errors](#service-broker-errors).


Do we need this 'Others will be ignored'? I assume the intent here is that some brokers might return some other fields due to the platforms/systems they are built on and we want to allow for that? Question is, will there be some brokers that return additional fields that might not be in the spec yet but the platforms will use. I'm just thinking, who ignores these fields and is the intent that they must be ignored, or something. Just curious :)

I just brought this across for consistency. 'Others will be ignored' is always kinda weird for a JSON object. If we remove this across the spec I'll happily remove here!

+1 to a follow-on PR to remove it.

vaikas · 2017-10-10T15:51:23Z

spec.md

@@ -1126,6 +1209,7 @@ $ curl http://username:password@service-broker-url/v2/service_instances/:instanc
 | --- | --- |
 | 200 OK | MUST be returned if the binding already exists and the requested parameters are identical to the existing binding. The expected response body is below. |
 | 201 Created | MUST be returned if the binding was created as a result of this request. The expected response body is below. |
+| 202 Accepted | MUST be returned if the binding is in progress. This triggers the platform marketplace to poll the [Polling Last Operation for Service Bindings](#polling-last-operation-for-service-bindings) endpoint for operation status. Information regarding the service binding (i.e. credentials) MUST NOT be returned in this asynchronous request. Note that a re-sent `PUT` request MUST return a `202 Accepted`, not a `200 OK`, if the binding is not yet fully created. |


re-sent PUT, does it have to be identical PUT?

duglin · 2017-10-24T20:18:27Z

Still discussing this internally but some immediate feedback I received is that requiring GET might be a bit of a security issue or hardship. This will force the broker to not just persist the credentials during the bind() and last_operation() phase, but then to persist it forever.

nilebox · 2017-10-24T23:21:27Z

This will force the broker to not just persist the credentials during the bind() and last_operation() phase, but then to persist it forever.

@duglin Isn't that already true?

According to the spec, if the broker receives a duplicate bind request (with the same parameters), it should return 200 OK with existing credentials:

200 OK	MUST be returned if the binding already exists and the requested parameters are identical to the existing binding. The expected response body is below.

This duplicate binding request can occur at any time, not necessarily within a short amount of time after the initial binding request.

duglin · 2017-10-24T23:56:45Z

hmmm that's a good point. I'm curious, for the CF guys who support stateless brokers, how can this check be performed if the broker is stateless?

nilebox · 2017-10-25T00:11:10Z

spec.md

+| Status Code | Description |
+| --- | --- |
+| 200 OK | The expected response body is below. |
+| 404 Not Found | MUST be returned if the service instance does not exist or if a provisioning operation is still in progress. The expected response body is `{}`. |


What should happen if the instance update request is in progress (i.e. there is an existing resource, but the update processing is not finished yet)?

Seems to me like this should be added here as a 422 - Unprocessable Entity situation.

I wonder if 409 is more appropriate:
409 Conflict
Indicates that the request could not be processed because of conflict in the request, such as an edit conflict between multiple simultaneous updates.

nilebox · 2017-10-25T00:15:08Z

spec.md

+`"state": "failed"` will cause the platform to cease polling and, in the case of
+a [Binding](#binding) request, information on the service binding can then
+immediately be fetched using the
+[Fetching a Service Binding](#fetching-a-service-binding) endpoint. The value


But what if the catalog endpoint returns "binding_retrievable": false? Do we still support async bindings (without being able to fetch any result)?

I think that we should explicitly add that if a binding supports async operations, it must have "binding_retrievable": true.

The wording with binding_retrievable is a bit ambiguous. It says:

If `"binding_retrievable" :true` is declared for a service in the Catalog endpoint, brokers MUST support this endpoint for all plans of the service.

It doesn't say that having "binding_retrievable": false means the endpoint is unavailable for all plans, so you could potentially have some plans able to support it and some not, and thus some plans able to be asynchronously bound to and some not.

I think we should tighten the language on the binding_retrievable definition to say that a platform should not attempt a GET if a service has "binding_retrievable": false.

I think this is sensible, thanks!

vaikas · 2017-11-06T18:18:13Z

Is there anything else that needs to be done before we can move this forward? Service Catalog is working on implementing this and we would like to ensure this is agreed upon asap. Can we make the decisions during tomorrows call for the remaining items?
As far as I understand there's only one remaining item that's been lingering for awhile now?
GET of the Bindings vs some other mechanism?

mattmcneeney · 2017-11-07T15:01:30Z

@vaikas-google I totally agree. Let's get this discussed on today's call and get this marked as being validated so we can work on getting this supported in platforms.

mattmcneeney · 2017-11-07T18:14:10Z

Please all review this PR as we are planning on moving this to validation through implementation on next week's call.

jmrodri · 2017-11-09T00:23:33Z

spec.md

+
+| Response Field | Type | Description |
+| --- | --- | --- |
+| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. |


In the Types of Binding section of the spec it reads:

Credentials are a set of information... If the Service Broker supports generation of credentials it MUST return credentials in the response for a request to create a Service Binding.

I think we should incorporate the MUST in the table. Most of the time I refer to the table, and example responses, to see what I need to return.

Something like this:

A free-form hash of credentials that can be used by applications or users to access the service. If the Service Broker supports generate of credentials it MUST return this in the response.

Where is the MUST that was asked for? I don't see it in the above table.

Ah, missed that bit. I personally think it's clear enough in the types of binding section, but can add this in here too.

jmrodri · 2017-11-09T00:26:55Z

spec.md

+| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. |
+| syslog_drain_url | string | A URL to which logs MUST be streamed. `"requires":["syslog_drain"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the platform MUST consider the response invalid. |
+| route_service_url | string | A URL to which the platform MUST proxy requests for the address sent with `bind_resource.route` in the request body. `"requires":["route_forwarding"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the platform can consider the response invalid. |
+| volume_mounts | array-of-objects | An array of configuration for mounting volumes. `"requires":["volume_mount"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the platform can consider the response invalid. |


Same comment about volume_mounts. The Types of Bindings section reads:

A create binding response from one of these services MUST include volume_mounts.

but the table doesn't mention that this field MUST be returned if the broker supports volume mounts.

is the MUST someplace else in the doc now?

Copied here too

jmrodri · 2017-11-09T00:28:30Z

Other than the above clarification points, I approve of this PR!

jmrodri · 2017-11-09T15:00:00Z

LGTM

duglin · 2017-11-14T13:04:24Z

spec.md

@@ -590,9 +597,8 @@ code `422 Unprocessable Entity` and the following body
 ```

 If the query parameter described above is present, and the Service Broker
-executes the request asynchronously, the Service Broker MUST return the
-asynchronous response `202 Accepted`. The response body SHOULD be the same as
-if the Service Broker were serving the request synchronously.


we've dropped the statement about the response looking the same between async and sync - is that really ok?

If I were to read the spec w/o any history of what was there in the past, I would probably return a 202 with just {} in the Body for async ops. Is that now ok even for provisioning? And, as a platform author I would probably expect that too.

I hope so, it was only a SHOULD. Otherwise we can add text stating that async instance provision SHOULD return the same as sync, but leave this open for async bindings? But I'd prefer to just remove this...

I'm just worried about the dashboard_url. This change pushes us towards not returning it on the provision response during an async provision. While I would prefer this model, I wasn't sure how existing brokers would feel about this shift in direction within the v2 time frame.

Yeah. We could explicity change the dashboard_url in the response table for provision to state that it MUST be returned immediately even in the async case. Do we want to lock ourselves into that though? Or maybe we feel that we already are, no matter what the wording says?

If CF wanted to get dashboard_url at a later date, it would have to use the GET endpoints, which are going to require a change to CF anyway to call those. But I guess for provision the only data is this URL so maybe we can just enforce that to be returned immediately. We haven't had much feedback from service authors on that front as far as I remember.

@duglin we actually already have that on line 922:

Note: a Service Broker that wishes to return `dashboard_url` for a service instance MUST return it with the initial response to the provision request, even if the service is provisioned asynchronously. If present, MUST be a non-empty string.

feels a little inconsistent to have a SHOULD in one spot (or implied SHOULD (or less) now), and a MUST in another. Seems we need to first decide is the MUST should live on - which I suspect it should since that would be a breaking change. If so, then I think we may need to be more explicit that an async provision is not the same as async binding because async provision requires a non-empty response.

duglin · 2018-07-11T23:03:06Z

spec.md

+| --- | --- |
+| 200 OK | MUST be returned upon successful processing of this request. The expected response body is below. |
+| 400 Bad Request | MUST be returned if the request is malformed or missing mandatory data. |
+| 410 Gone | Appropriate only for asynchronous delete operations. The Platform MUST consider this response a success and remove the resource from its database. The expected response body is `{}`. Returning this while the Platform is polling for create operations SHOULD be interpreted as an invalid response and the Platform SHOULD continue polling. |


In other spots you removed "The expected response body is {}.". Should we do that here too?

zrob · 2018-07-11T23:21:57Z

spec.md

@@ -1247,7 +1328,7 @@ binding requests.

 #### cURL
 ```
-$ curl http://username:password@service-broker-url/v2/service_instances/:instance_id/service_bindings/:binding_id -d '{
+$ curl http://username:password@service-broker-url/v2/service_instances/:instance_id/service_bindings/:binding_id?accepts_incomplete=true -d '{


it's interesting that the example curl here now includes an optional query param

the provision curl request does not include it.

do we care either way?

nevermind, curl examples are the same, was looking at the top level request example

duglin · 2018-07-11T23:22:32Z

spec.md

@@ -1270,9 +1351,10 @@ $ curl http://username:password@service-broker-url/v2/service_instances/:instanc
 | --- | --- |
 | 200 OK | MUST be returned if the binding already exists and the requested parameters are identical to the existing binding. The expected response body is below. |
 | 201 Created | MUST be returned if the binding was created as a result of this request. The expected response body is below. |
+| 202 Accepted | MUST be returned if the binding is in progress. The `operation` string MUST match that returned for the original request. This triggers the Platform to poll the [Polling Last Operation for Service Bindings](#polling-last-operation-for-service-bindings) endpoint for operation status. Information regarding the Service Binding (i.e. credentials) MUST NOT be returned in this asynchronous request. Note that a re-sent `PUT` request MUST return a `202 Accepted`, not a `200 OK`, if the binding is not yet fully created. |


s/asynchronous request/asynchronous response/ - maybe?

zrob · 2018-07-11T23:23:46Z

spec.md

@@ -1450,23 +1543,37 @@ The following HTTP Headers are defined for this operation:

 ```
 $ curl 'http://username:password@service-broker-url/v2/service_instances/:instance_id/
-  service_bindings/:binding_id?service_id=service-id-here&plan_id=plan-id-here' -X DELETE -H "X-Broker-API-Version: 2.13"
+  service_bindings/:binding_id?service_id=service-id-here&plan_id=plan-id-here&accepts_incomplete=true' -X DELETE -H "X-Broker-API-Version: 2.13"


similar as previous note about including optional query params in curl example

nevermind, curl examples are the same, was looking at the top level request example

duglin · 2018-07-11T23:30:59Z

spec.md

-| syslog_drain_url | string | A URL to which logs MUST be streamed. `"requires":["syslog_drain"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform MUST consider the response invalid. |
-| route_service_url | string | A URL to which the Platform MUST proxy requests for the address sent with `bind_resource.route` in the request body. `"requires":["route_forwarding"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform can consider the response invalid. |
-| volume_mounts | array of [VolumeMount](#volume-mount-object) objects | An array of configuration for remote storage devices to be mounted into an application container filesystem. `"requires":["volume_mount"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform can consider the response invalid. |
+| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. MUST be returned for a synchronous request if the Service Broker supports generation of credentials. |


s/sync request/sync bind/
I think

yes, as a followup?

duglin · 2018-07-11T23:45:55Z

spec.md

-| route_service_url | string | A URL to which the Platform MUST proxy requests for the address sent with `bind_resource.route` in the request body. `"requires":["route_forwarding"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform can consider the response invalid. |
-| volume_mounts | array of [VolumeMount](#volume-mount-object) objects | An array of configuration for remote storage devices to be mounted into an application container filesystem. `"requires":["volume_mount"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform can consider the response invalid. |
+| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. MUST be returned for a synchronous request if the Service Broker supports generation of credentials. |
+| syslog_drain_url | string | A URL to which logs MUST be streamed. If the service declares `"requires":["syslog_drain"]` in the [Catalog](#catalog-management) endpoint, this MUST be returned for a synchronous request or the Platform can consider the response invalid. |


I don't think the "if" sentence is right since what you wrote now implies that if the "requires" is there then this field MUST be there which isn't true since it might not want to return one for this.

duglin · 2018-07-11T23:50:16Z

spec.md

+| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. MUST be returned for a synchronous request if the Service Broker supports generation of credentials. |
+| syslog_drain_url | string | A URL to which logs MUST be streamed. If the service declares `"requires":["syslog_drain"]` in the [Catalog](#catalog-management) endpoint, this MUST be returned for a synchronous request or the Platform can consider the response invalid. |
+| route_service_url | string | A URL to which the Platform MUST proxy requests for the address sent with `bind_resource.route` in the request body. If the service declares `"requires":["route_forwarding"]` in the [Catalog](#catalog-management) endpoint and `"bind_resource":{"route":"some-address.com"}` is included in the request, this MUST be returned for a synchronous request or the Platform can consider the response invalid. |
+| volume_mounts | array of [VolumeMount](#volume-mount-object) objects | An array of configuration for remote storage devices to be mounted into an application container filesystem. If the service declares `"requires":["volume_mount"]` in the [Catalog](#catalog-management) endpoint, this MUST be returned for a synchronous request or the Platform can consider the response invalid. |


same comment as above, I don't think the "if" is worded correctly.

duglin · 2018-07-11T23:52:24Z

spec.md

+and this endpoint MUST be available immediately after the
+[Polling Last Operation for Service Bindings](#polling-last-operation-for-service-bindings)
+endpoint returns `"state": "succeeded"` for a [Binding](#binding) operation.
+Otherwise, Platforms SHOULD NOT attempt to call this endpoint under any


why not? Wouldn't they just get a 404 if its not ready/there?

zrob · 2018-07-12T00:05:50Z

lgtm

duglin · 2018-07-12T17:13:38Z

LGTM

zrob · 2018-07-12T17:14:05Z

lgtm

fmui · 2018-07-12T17:15:43Z

LGTM

mattmcneeney added the 1 - reviewing proposal label Oct 10, 2017

mattmcneeney self-assigned this Oct 10, 2017

mattmcneeney added this to the 2.14 milestone Oct 10, 2017

mattmcneeney force-pushed the async-bindings branch 3 times, most recently from 2c53363 to e843013 Compare October 10, 2017 14:37

vaikas reviewed Oct 10, 2017

View reviewed changes

nilebox mentioned this pull request Oct 25, 2017

Returning information about the provisioned instance in the response #295

Closed

nilebox reviewed Oct 25, 2017

View reviewed changes

kibbles-n-bytes mentioned this pull request Nov 1, 2017

add alpha asynchronous binding operation support kubernetes-retired/service-catalog#1512

Merged

mattmcneeney force-pushed the async-bindings branch 2 times, most recently from c37a2bf to 20979fd Compare November 3, 2017 14:51

duglin mentioned this pull request Nov 7, 2017

Support async binding operations #137

Closed

mattmcneeney force-pushed the async-bindings branch from 20979fd to 8473e14 Compare November 7, 2017 14:59

mattmcneeney force-pushed the async-bindings branch from 8473e14 to 4187321 Compare November 7, 2017 18:13

jmrodri reviewed Nov 9, 2017

View reviewed changes

mattmcneeney force-pushed the async-bindings branch from 4187321 to ed63ec8 Compare November 9, 2017 12:33

duglin reviewed Nov 14, 2017

View reviewed changes

duglin reviewed Jul 11, 2018

View reviewed changes

n3wscott approved these changes Jul 11, 2018

View reviewed changes

zrob reviewed Jul 11, 2018

View reviewed changes

duglin reviewed Jul 11, 2018

View reviewed changes

zrob reviewed Jul 11, 2018

View reviewed changes

duglin reviewed Jul 11, 2018

View reviewed changes

addressed comments

2e463c3

n3wscott approved these changes Jul 12, 2018

View reviewed changes

Matt McNeeney added 2 commits July 11, 2018 17:12

attempt to fix binding response properties text

11cbe9b

address comments

d86744b

mattmcneeney force-pushed the async-bindings branch from 6428a5c to d86744b Compare July 12, 2018 16:49

seperate async and sync binding response bodies

983ccfd

mattmcneeney force-pushed the async-bindings branch from 0e2a751 to 983ccfd Compare July 12, 2018 16:54

Matt McNeeney added 3 commits July 12, 2018 09:55

reorder bodies

e4814fc

remove duplications

7f8f143

fix table splitting

8adfc38

n3wscott approved these changes Jul 12, 2018

View reviewed changes

mattmcneeney merged commit 03b6839 into openservicebrokerapi:master Jul 12, 2018

mattmcneeney deleted the async-bindings branch July 12, 2018 21:10

georgi-lozev mentioned this pull request Jul 25, 2018

Users can delete service bindings asynchronously cloudfoundry/cloud_controller_ng#1185

Merged

5 tasks

teddyking mentioned this pull request Mar 13, 2020

Add support for OSBAPI 2.14 kubernetes-retired/go-open-service-broker-client#144

Closed

anoopjb mentioned this pull request Apr 11, 2021

Support for asynchronous Service Bindings cloudfoundry/service-fabrik-broker#1302

Merged

Support asynchronous bindings (requires #333) #334

Support asynchronous bindings (requires #333) #334

Conversation

mattmcneeney commented Oct 10, 2017 • edited Loading

cfdreddbot commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Oct 24, 2017

nilebox commented Oct 24, 2017 • edited Loading

duglin commented Oct 24, 2017

Choose a reason for hiding this comment

kibbles-n-bytes Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibbles-n-bytes Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas commented Nov 6, 2017

mattmcneeney commented Nov 7, 2017

mattmcneeney commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri commented Nov 9, 2017

jmrodri commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zrob commented Jul 12, 2018 • edited by avade Loading

duglin commented Jul 12, 2018 • edited by avade Loading

zrob commented Jul 12, 2018 • edited by avade Loading

fmui commented Jul 12, 2018 • edited by avade Loading

mattmcneeney commented Oct 10, 2017 •

edited

Loading

nilebox commented Oct 24, 2017 •

edited

Loading

kibbles-n-bytes Oct 30, 2017 •

edited

Loading

kibbles-n-bytes Nov 1, 2017 •

edited

Loading

duglin Jul 11, 2018 •

edited

Loading

duglin Jul 11, 2018 •

edited

Loading

zrob commented Jul 12, 2018 •

edited by avade

Loading

duglin commented Jul 12, 2018 •

edited by avade

Loading

zrob commented Jul 12, 2018 •

edited by avade

Loading

fmui commented Jul 12, 2018 •

edited by avade

Loading