Support async binding operations about servicebroker HOT 72 CLOSED

I missed last week's call in which you may have discussed this, but I agree with many of the points above. Summarising my thoughts:

• We voted at the face to face to favour consistency of the last_operation endpoints over the 'best' workflow
• However, since async bindings is a new feature that no brokers currently support, I believe we can design this with a bit of flexibility over how 'consistent' we are with async service instance provisioning
• If a broker decides to support this new feature, I would like to say that the broker therefore MUST support the GET service bindings endpoint
• It is unrealistic to state that an async binding request should return the same information immediately as for a sync binding request. This defeats the point of offering async bindings and the immediate return of dashboard_url is a bit of a red herring here (as that URL doesn't actually have to be immediately available)

I'm going to make a change to the spec regarding my last point here, and it would be great to discuss this again on next week's call?

from servicebroker.

I've created a branch that adds async binding and unbinding to the spec: View proposed spec (or changes).

Please provide any feedback you have on this change. The spec changes are relatively straightforward, making use of the workflows already in place for provisioning, updating and deprovisioning. Thanks!

from servicebroker.

An alternative solution would be to add an additional GET endpoint that would return the binding information. The platform talking to the broker could handle permissions on who could actually see any information returned in that call (such as credentials).

Additional logic around how long service brokers have to keep credentials for (ie. until the last_operation endpoint has been polled and a 'succeeded' response was returned) seems complex, but you're right that it would work more similarly to the synchronous behaviour.

Edit: There may be overlap with issue Fetching current input parameters values (for a service or a binding) regarding the alternative solution mentioned above.

from servicebroker.

+1. I have already run into a scenario where I needed async binding (Google CloudSQL).

from servicebroker.

We also had the situation where bindings need to return information, potentially asynchronously.

This raises the question of whether it would be better to implement the last_operation function, or a fully fledged GET method on bindings.

Would a GET operation cover your use cases?

from servicebroker.

Would that be adding additional information in the response to the GET /last_operation?

from servicebroker.

from servicebroker.

Unless there is any further discussion I would suggest that @bmelville creates the proposed spec changes on a branch. This can then be shared with the cf-dev mailing list.

I would support this feature.

Any concerns @shalako?

from servicebroker.

@vaikas-google will you be picking up drafting the spec changes in a branch?

from servicebroker.

from servicebroker.

It is also interesting the spec doesn't include timeout information.

Currently, Cloud Foundry times out binding requests after 60 seconds by default. This is configurable by the operator on a global basis, meaning it applies to all brokers.

https://github.com/cloudfoundry/cloud_controller_ng/blob/master/bosh/jobs/cloud_controller_ng/spec#L570

@pmorie does service catalog have any request timeout logic that you are enforcing?

from servicebroker.

Following yesterday's call with the working group, I've updated the branch with an updated spec containing a last operation endpoint for service bindings.

Let us know your thoughts, thanks!

from servicebroker.

@mattmcneeney I understand that the synchronous binding endpoint PUT /v2/service_instances/:instance_id/service_bindings/:binding_id returns the credentials and there is no GET endpoint to retrieve credentials afterwards.

However, I don't see the credentials being returned by the last operation GET /v2/service_instances/:instance_id/service_bindings/:binding_id/last_operation endpoint

c1f610a#commitcomment-21437274

How would platform clients fetch the binding credentials ?

from servicebroker.

Good spot @gberche-orange, it should be in the asynchronous response too.

I've updated the spec so that credentials (and other information) can be included in the last service binding operation endpoint response.

from servicebroker.

@mattmcneeney as we discussed, this seems like the simplest path forward for brokers.

It does mean brokers that support async responses would need to hold on to the credentials / be able to retrieve them until "last_operation" is called.

from servicebroker.

Thanks for the feedback on this so far everyone. Does anyone else have any further thoughts on the proposed spec?

from servicebroker.

thanks @mattmcneeney for the update to the GET /v2/service_instances/:instance_id/service_bindings/:binding_id/last_operation endpoint response.

I wonder whether the specification should mandate that the last_operation endpoint only returns once the credentials, i.e. within the response where "state": "succeeded" was returned.

This would make it consistent with the current synchronous behavior for which there is currently no GET endpoint to retrieve credentials, possibly strengthening security by reducing credentials undesired retransmits over the wire, and reducing the duration into which broker should manage credentials state.

This would be a more defensive wording than the current

Returning "state": "succeeded" or "state": "failed" will cause the platform to cease polling.

from servicebroker.

I don't have strong a opinion on either approach.

Potential drawbacks I can think of for new GET /v2/service_instances/:instance_id/service_bindings/:binding_id new endpoint:

• It seems that adding this new GET endpoint for credentials further pollutes the API specs for brokers that would only need to support support synchronous bindings.
• client callers might get confused in expecting that the GET /v2/service_instances/:instance_id/service_bindings/:binding_id new endpoint would also be returning synchronously created binding credentials

from servicebroker.

I agree with those drawbacks @gberche-orange.

Since this overlaps with issue #159 regarding Fetching current input parameters values (for a service or a binding), we can wait to see what everyone's thoughts are on that issue before making a decision here.

from servicebroker.

For those who want to see a diff, I think this is the link:
master...avade:async-binding

from servicebroker.

Couple of comments:

• master...avade:async-binding#diff-958e7270f96f5407d7d980f500805b1bR328 talks about bindings instead of provisioning.
• why are service_id and plan_id needed on: master...avade:async-binding#diff-958e7270f96f5407d7d980f500805b1bR350 ? The text says they should be there but I'm not clear on why and what happens if they don't match the previous binding request? If they're optional it seems like we should just remove them since the broker is expected to work w/o them.
• same question for "operation". Are brokers expected to support returning the status of a bind() and unbind() at the same time? Or just the last one invoked? It seems like "operation" only makes sense if the platform can query for a previous command, but then of course that makes the phase "last_operation" kind of misleading.
• for the previous 2 comments, if the answer is "they're there just for consistency and not for technical reasons" then ok - I can live with that, but then we may want to consider deprecating them.
• on master...avade:async-binding#diff-958e7270f96f5407d7d980f500805b1bR367 the text implies they should keep polling until they get a 200, but it seems to me that they should stop on other errors too, like a 404.
• Along those lines, do we need to add text to say that brokers should be able to return a 200 on this new API immediately once the binding() response is sent. Meaning, waiting until the async op completes before this URL is valid would not be appropriate.
• master...avade:async-binding#diff-958e7270f96f5407d7d980f500805b1bR685 add a period before "Additionally".
• there are spots where we talk about "broker clients" ( master...avade:async-binding#diff-958e7270f96f5407d7d980f500805b1bR701 ) and spots where we say "platform" - we may want to be consistent. This might be an issue for the spec itself and not this change though. Just noticed it.

from servicebroker.

Thanks for the feedback @duglin. In response to the above:

• The reference to binding was a mistake. Fixed.
• The service_id, plan_id and operation were left in for consistency. I'm not sure how they are used currently for the existing last_operation endpoint. Anyone?
• Agreed that the existing and new endpoints should clarify to stop polling on an error also. Fixed.
• I'm not sure I understand do we need to add text to say that brokers should be able to return a 200.... Brokers supporting async bindings should return a 202.
• Period added before "Additionally"
• I agree with the inconsistent broker client vs platform. I've fixed it in this branch so that we only refer to platform, but happy to move to another branch if preferred (or use broker client instead; platform made more sense to me from a CF perspective).

from servicebroker.

Updated spec: link

from servicebroker.

@mattmcneeney re: "I'm not sure I understand do we need to add text to say that brokers should be able to return a 200.... Brokers supporting async bindings should return a 202."... right now a broker should return a 202 in response to the provision() and bind() operations, if they're doing to do it asynchronously. However, what's not clear is when the URL for the "last_operation" because valid/alive for the new resource. I think its implied that it should be alive immediately but the spec doesn't say so. Which means, its possible for someone to implement it in such a way that the URL will return a 404 until the resource is fully ready. For example, if they simply accept the incoming request and stick it in a queue, then return a 202. They may not do the work to make the polling endpoint available until something starts to processes that entry the queue.

The reason I thought about this is because of a previous comment about error responses to the async polling causing the platform to stop polling. Well, if they stop on a 404 and the broker is kind of goofy and returns a 404 until the resource is ready then the platform will stop prematurely.

I think simply adding text along the lines of:

Once a broker returns a 202 in response to a provision or binding request, the corresponding
`last_operation` endpoint should be ready to respond to polling requests immediately.

should cover it.

from servicebroker.

Aha, I understand. Good spot, the spec was definitely a bit loose. I'll add the above note to the spec for all of the 202 responses.

from servicebroker.

@duglin How's this?
(Whole spec)

from servicebroker.

@mattmcneeney looks good - thanks!

from servicebroker.

Branch updated: https://github.com/mattmcneeney/servicebroker/tree/async-bindings
(Depends on #159)

from servicebroker.

@mattmcneeney LGTM

from servicebroker.

mattmcneeney/servicebroker@get-endpoints...mattmcneeney:async-bindings

from servicebroker.

I've started to draft the proposal doc for this: https://docs.google.com/document/d/16yobzCdZCOcKgHMf8gICJnDeoV8Jp3eUM0Y_t4FoEAA/edit#

@jmrodri thanks for offering to help!
@avade @shalako info on any CF-specific use cases would be great
@duglin @arschles same for your respective platforms

from servicebroker.

Added comments to the proposal doc but will mirror them here.

We use identifiers which are not strictly GUID, but have some sanity (e.g., conform to [a-zA-Z0-9-]+), and it would be difficult in some cases to have to start generating, storing, and using additional GUIDs just to conform to the requirement that operation IDs be GUIDs.

This also is contradictory to the other parts of the API spec which recommend but do not require GUIDs.

I'd love to see that aspect relaxed to some degree, to a requirement that maintains sanity of IDs but is not overly restrictive as to put a burden on producers who do not necessarily use GUIDs as their unique identifier.

from servicebroker.

One common naming requirement for IDs I've seen is to limit to unreserved URI characters, and perhaps also impose a length limitation (e.g., 64 characters).

https://tools.ietf.org/html/rfc3986#section-2.3

from servicebroker.

We should consider #267 when designing these new last operation endpoints. Are folks happy for me to roll @bmelville's suggested changes from that issue into the proposed branch for this issue?

from servicebroker.

I've updated the async bindings proposed spec changes following our discussions yesterday.

@jmrodri could you take a look here and let me know what you think? Hopefully I haven't missed anything!

from servicebroker.

@mattmcneeney LGTM I see you clarified the original last operation to include 'Service Instance' which is good. I confirm that the async binding looks like the service instance last_operation which is precisely what we agreed to at the F2F. Nothing stood out to me.

@pmorie LGTM.

from servicebroker.

Thanks @jmrodri @pmorie

I I get two more LGTMs on this issue, I'm happy to move this into the validation through implementation phase. @bmelville @shalako @duglin

from servicebroker.

was the gdoc updated with the proposed changes from the f2f? In particular, I didn't think we were going to add a new "operations" API:

Proposal:
Add /si/:id/sb/:id/last_operation[?operation=...] endpoint
Only one async anonymous (no “operation” string) at a time
Do not touch /si/:id/last_operation[?operation=...]
Add text to explain why we can get the last op on a resource when a GET on that resource would fail.

from servicebroker.

gdoc isn't but the branch is!

https://github.com/mattmcneeney/servicebroker/blob/async-bindings/spec.md#binding

from servicebroker.

ok - updated the first comment with the link so its easier to find. thanks

from servicebroker.

My only feedback at this point is that I think it would be best if we located the 'async' operations part of the spec as pertains to bindings after describing the operations on bindings instead of before.

from servicebroker.

There seems to be ambiguity with the response body of a 202 Accepted for bindings. Right now, it seems like you could have the broker respond with a 202 but also give credentials at that point in time, in effect saying "here's the credentials, but don't use them yet". You could even have this flow for "binding_retrievable": false instances.

Is this a flow we want to support? I'm thinking no, but in either case I think it would be good to clarify.

from servicebroker.

I would rather have the resource (credentials, etc.) be available through GET and not through the initial body. Otherwise, you'd have to worry about things like can you support partial ones on the initial request and then reconcile, etc. etc. I think semantically saying you have to poll until the resource either successfully gets created or fails, and if it's successfully created, you can then fetch that resource and the credentials, etc. that it contains.

from servicebroker.

I agree; the added complexity is not worth supporting a fringe use case. I think we should clarify that if you receive a 202 Accepted, the platform will ignore any credentials payload that happens to be returned.

from servicebroker.

While I agree with the proposed direction for async/202's - this sentence, for the body of the 202, in the spec is worrisome:

The response body SHOULD be the same as if the broker were serving the request synchronously.

I'm interpreting the current state of the spec as the exact opposite of what @vaikas-google was proposing. Meaning, the async-response/202 has the final results body, and the last response from the poll to last_operation is almost meaningless, other than to say the op is done. I'm curious to know what the intent of the SHOULD is. Meaning, if the broker chooses to not include the creds then how does the platform finally get them? @shalako ?

Yes, it might be nice to follow the message exchange with a GET, but GET will be optional.

It seems to me that we may have to add something to the spec that says the "real response" can be in one but not both response flows. Meaning, you can return the creds in the async/202 OR in the final last_operation response, but not both. Then the platform won't need to worry about conflicting data. Yes, this means adding it to the last_operation response too.

from servicebroker.

See: #307 for some proposed wording changes around lines line:

:instance_id MUST be a globally unique non-empty string. :binding_id MUST be a globally unique non-empty string.

from servicebroker.

Reading the proposal text, I think we should include the binding response (and provision response) in the final response from last_operation - assuming its not part of the original async response/202. This will save people to trouble of doing a GET immediately and will always work even when GET isn't supported.

from servicebroker.

why is in the case of a Binding request snippet in the async binding ( https://github.com/mattmcneeney/servicebroker/blob/async-bindings/spec.md#polling-last-operation-for-service-bindings ) section? Isn't the entire section about "bindings" ?

from servicebroker.

In this

If brokers do not return this operation field, only one asynchronous operation MAY be supported at a time.

I think this needs to use a MUST instead of MAY which implies OPTIONAL, e.g.:

If brokers do not return this operation field then brokers MUST NOT return any other asynchronous response without an operation field while the first request is still being processed. In other words, without the data provided by the operation field, it would not be possible for the broker to distinguish two polling requests to the last_operation endpoint for two different bindings, therefore, only one such request can be active at a time.

from servicebroker.

@duglin Regarding handing the credentials (or any other binding related return values) right up front kind of defeats the purpose of the async provisioning. If they take any time to create, then why even bother with the async request?

from servicebroker.

@duglin I also think that the binding should be gettable resource and in that case last operation polling should only be used to tell whether the requested resource (binding) was created successfully or if it failed, polluting that response with the resulting resource information seems wonky to me. It's a single GET so the overhead for doing that is minimal and I thought we wanted to make Bindings gettable so why wouldn't we return it there for consistency? By consistency I mean if bindings are gettable resource and hence useful for reconciling differences then wouldn't it make more sense to just have one place to fetch the credentials?

from servicebroker.

@vaikas-google I agree that returning it on the 202 makes little sense, but having the spec say it SHOULD be there is going to be problematic - because SHOULD is a soft-MUST. I'd like to get @shalako's take on why it isn't a MUST to better understand the original intent - that might provide some insight into whether we can change it to a MAY instead - which I think is the best we can do on that.

My reasons for allowing the creds to flow back on the final last_operation response is that it just feels like a waste of effort to force all platforms to issue a second request every time. Also not everyone will support GET so we'll need to get the creds for an async request w/o it. Which means there will be some scenarios where the last_op response will be the only way to get the creds - so for consistency it seems like it should be in there regardless of whether GET is supported or not.

from servicebroker.

Seems like since there are no brokers that support async ops for bindings (this is for bindings only, right?), it seems that we have a bit of an opportunity to create a good flow that makes sense. So, one could say that in order to support async bindings, you have to do couple of things:

1. 202 that points to polling that tells the caller when to stop polling [either success or failure]
2. once 202 returns success, call GET

So, broker writer must support 202 + last_operation + GET to get async bindings support.

from servicebroker.

That is certainly an option, but then we're inconsistent with async provisioning. I'm also not sure we can require GET since that mandates stateful brokers. I suspect that might be why the 202 is supposed to return the creds, but that's just a guess.

We should discuss this tomorrow... although, @mattmcneeney may not be on the call.

from servicebroker.

from servicebroker.

I'm not following the "how long" question. If we're going to mandate GET then there's no real data to hold on to for last_op, at least in the successful case since last_op will know the operation was successful since the broker should have all of the info needed.

Yes, returning no other info on last_op is consistent with async provision, but I would like to suggest that we allow creds on the async provision last_op response to be consistent with our new async binding. I say this because I can imagine a case where a stateless broker can't support GET in general but is willing to hold on to the state long enough to return the last_op response- like we expect them to do today for async provision. And in that case they can probably hold on to the creds too for that short amount of time.

At the f2f we opted to not introduce the more correct set of URLs because we didn't want to be inconsistent or offer more than one way to do something. By having async bindings look so different from async provision it feels like we're back-tracking on that consistency decision and leaving the spec in kind of a weird state.

from servicebroker.

from servicebroker.

I would think GET endpoint should return the binding data for as long as the resource (binding) is around.

from servicebroker.

I'm not too concerned with making the async binding flow exactly match the async provision flow. If we're talking ideal design, I could even see the argument that the async provision response should NOT return the dashboard URL, as that is also something that might take a while to get set up correctly, and its presence is more an artifact of not having GETs in the spec at that time.

Ultimately, in my opinion the GET is the most consistent and clear way to get the credentials back. I view credentials as out of scope of what an operation resource is meant to do, which is a generic resource that just reports on the status of an ongoing/completed process. And supporting credentials being passed back with the 202 but not being accessible is just a headache for the platform and I suspect not a common use case. If you're going to support binding asynchronously then I think it's perfectly fine to say you have to be stateful at that point.

from servicebroker.

Doug, are you suggesting that last_op return credentials only once? That seems scary from the perspective of idempotency and could easily lead to lost data.

I will also +1 Kibbe's suggestion that in a true async provisioning world, even dashboard URL may not be available until the instance has been provisioned. Any async operation must allow for the results of that operation to be made available only after it completes.

from servicebroker.

@bmelville no, I'm suggesting that async bindings should have a similar UX as async provisioning to provide consistency between the two. Whether last_op can be called again, even after its said the op is done, is not clear in the spec and I would be fine with it returning the same data over and over until the broker decides to "not remember" that operation any more.

Let me try to be a bit more clear since I probably wasn't in the past....

• I think consistency between the two async ops is important.
• while async bindings are new, and therefore technically we have the freedom to do what is architecturally right, from an UX perspective if we can get the same semantic results via a less clean method that's consistent with async provisioning I think we should. I'm more concerned about presenting an inconsistent API in the v2 time frame. We can "do it right" in v3.
• while the spec says the async response (the 202) SHOULD return the same result as if it was a sync op, we have some wiggle room here to add something like "... SHOULD be the same as if the broker were serving the request synchronously - but if the broker does not have this information available immediately, then it MAY choose to return it on the "successful" last_operation response".
• I think returning it on the final last_op is critical because that's the only required endpoint a broker MUST support if it supports async provisioning. So that's the same guaranteed method I think we should offer for async binding.
• While I agree that returning the creds on the GET is ideal, we have to assume that GET may not be supported by everyone - even if they do support async bindings. I view these two features as independent things and even a stateless broker (that can't support GET) might still support returning creds in the final last_op response. Therefore, if we can't mandate GET on async provisioing, we shouldn't mandate it on async bindings - this leaves us with only the last_op response as the message flow to send this data - when GET is not supported.
• GET (for both) should be optional and should return the full resource in question.

That's at least my current thinking on it.

from servicebroker.

My issue is with the first two points. Why is it important?
I don't understand from the UX perspective why this has any bearing. The platform should be able to provide the UX that basically goes like this for bindings:

• I started something (initial post -> 202)
• provide status updates (by polling the last operation)
• provide results (once the polling completes, platform does a get on the resource).

and for service instances:

• I started something (initial post -> 202 -> stash the URL away but not show it)
• provide status updates (by polling the last operation)
• provide results (once the polling completes, show the URL).

I think we're getting distracted by the dashboard URL because apparently it can be precomputed ahead of time, but has no bearing on the actual resource (Service Instance) being created and mixing that with the result of the operation is making things more complicated.

I'd argue again that baking returning information from the last_operation is a bad idea because then you:

• lose all the benefits of schemas that a binding response can have [or make it more complicated]
• last_response response is not some sort of union
• it's unclean from semantics point

So, your second to last point is what I'd like to focus on. We're designing a new experience for async bindings. If you are saying that the broker can return the credentials (multiple times, mind you since we can't assume that things don't fall on the floor) in the last_operation, it clearly has to hang onto them somehow (or be able to get a mapping from last operation to them and if that's the case, then they could more than likely return or compute the mapping from the serviceinstance/binding as well in order to compute the value for last_operation), and I have a hard time seeing how you could return them in the last_operation but not in the get response. I'm sure such a system could be built, but I'm not sure how realistic it is.
I feel like there's a lot of assumptions baked in there about what can't be done that's leading us down a path of complicated and unclean API :)

from servicebroker.

Branch updated: https://github.com/mattmcneeney/servicebroker/blob/async-bindings/spec.md
Shout if anyone else wants write access.

from servicebroker.

I agree with your points, except for dashboard_url being a red herring. The URL may be specific to a dynamic IP or port, which may not be known until provisioned in the async process. I think this can be captured as a separate issue.

Edit: #170 already exists.

from servicebroker.

from servicebroker.

A couple of nits on the status codes for binding (here):

• We should include MUST be returned if the broker only supports asynchronous binding and the request did not include ?accepts_incomplete=true. for a 422 Unprocessable Entity response code.
• For 200 OK, we may want to have something analogous to "is fully provisioned" from the instance case.

from servicebroker.

Good points @kibbles-n-bytes - branch updated!

How do the interested folks feel about this branch now?

Spec: https://github.com/mattmcneeney/servicebroker/blob/async-bindings/spec.md

Diff against get-endpoints: mattmcneeney/servicebroker@get-endpoints...mattmcneeney:async-bindings

from servicebroker.

Thanks @mattmcneeney!

We should also document 422 Unprocessable Entity in the unbind response status codes (here)

from servicebroker.

Good point. That made me re-check the 422 description for Bindings. I only had one error described regarding the required app_guid field, so I've updated that too. Thanks @kibbles-n-bytes !

from servicebroker.

Can we close this in favor of #334 ?

from servicebroker.

@duglin fine by me as long as folks are happy to continue discussions on the PR

from servicebroker.

👍

from servicebroker.