Add name field to provision and update (corresponding to the name being used to represent the Service Instance in the Platform)

[mattmcneeney]

See #514

Needs further discussion so adding the DO NOT MERGE label for now, but please review.

[cfdreddbot]

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.

[mattmcneeney]

cc @kieron-pivotal I wanted to flag this as supporting this would require ODB changes

[duglin]

I went back to talk to our guys on this and their preferred approach would be to define an API for the Broker to querythe paltform for this info, however since that's a huge change (since we have no broker->platform APIs at all right now), they consider this the next best option.

[mattmcneeney]

Jatin asked why we are sending mutable platform metadata on a provision and update call, which opens up an avenue where the platform has to try to keep the broker in sync. This also seems unaligned with the text describing what update should be used for:

By implementing this endpoint, Service Broker authors can enable users to modify two attributes of an existing Service Instance: the Service Plan and parameters. By changing the Service Plan, users can upgrade or downgrade their Service Instance to other plans. By modifying parameters, users can change configuration options that are specific to a service or plan.

We started revisiting moving this information to the context object, understanding that this means we may send update requests with no actual changes.

Discuss...

[duglin]

rebase needed

[mattmcneeney]

I'll take the action to update this to put the name field back in context, and we can go from there..

[mattmcneeney]

Updated! name is now a Cloud Foundry context object field. If the svc-cat folks want to use this too then shout and I can have a go at adding it to the Kubernetes context object too.

Thoughts @duglin @tinygrasshopper?

[jagadish-kb]

Context having name certainly helps (we need name in some cases additionally apart from Dashboard). Since context now carries name, can we request for API spec to mandate name changes as well to invoke broker update api ?

Currently renaming a service instance does not result in a call back to service broker.

One could argue that this is out scope for the API spec and is part of the Platform to either invoke or not invoke it based on their CLI/API set. However I feel making this explicit in the API spec would at-least provide the right guidance to the platform.

[tinygrasshopper]

My scepticism around putting mutable identifiers in the api and having to keep them in sync using update service endpoint persists. Currently all other entities in the context map are immutable.

I think there are also considerations about what to do when the broker update call fails when a user tries to rename a service on the platform, should the platform reject the rename request? In the case of an async update, should the platform update the name on the platform during the time it is polling the broker to check if the update request is successful. Does the spec need to provide any guidance for such scenarios?

[tinygrasshopper]

Thinking out loud here: but does it makes sense in the usecases like the dashboard(where it is required to display the service name, as its known on the platform), it is more suitable for that dashboard application to call back into the platform to find out the current name for the service instance, rather than the name being given to the broker over the API.

[zhongyi-zhang]

@krancour @jeremyrickard what do you think of the adding? Looks it overlaps "alias" in OSBA. In other words, OSBA ever wanted such a field. That's why OSBA brought in "alias" as a substitution. Compared to "alias", the instance name can be gave by Cloud Controller / Service Catalog -- users don't need to specify an extra "alias". It wouldn't impact OSBA if OSBA just ignores the adding. But it would be a breaking change if OSBA tries to refactor to take advantage of this.

[mattmcneeney]

I have removed the do not merge label from this PR, as I think it is a fairly simple one for Platform's to implement (ignoring rename...) and provides some value.

cc @williammartin for adding support to CF

Let's try to get this merged!

[tinygrasshopper]

@mattmcneeney does this imply that platforms will start making update calls when the name field is changed on the platform? How would that invocation look like, would it be an update without any parameters and plan changes? How would existing brokers 'know' how to react to the additional update invocations in the lifecycle.

I think we should also introduce a new field in the catalog something like name_updatetable, which signifies the service instances name needs to be updated on the broker everytime there is a name change on the platform.

[mattmcneeney]

@mattmcneeney does this imply that platforms will start making update calls when the name field is changed on the platform? How would that invocation look like, would it be an update without any parameters and plan changes? How would existing brokers 'know' how to react to the additional update invocations in the lifecycle.

Yes - this does imply that Platforms would make an update request to the broker if a service instance is renamed (probably with no parameters and no plan change). Existing brokers MAY reject that request, so we'd have to implement Platform-side logic to handle the fact that this is new behaviour which may not be supported. Service brokers may just return a 200 OK as per the spec if the instance/binding already exists and no changes are being made.

I think we should also introduce a new field in the catalog something like name_updatetable, which signifies the service instances name needs to be updated on the broker everytime there is a name change on the platform.

How important do you think it is for brokers to opt-in to this behaviour? I'm not against it, but it does raise the complexity of this feature, and it seems weird to have a flag in the catalog controlling some contextual information that gets sent through.

[mattmcneeney]

@duglin I didn't want to change this PR (again) unless we can get agreement on where this info should go.

I know @tinygrasshopper had thoughts too. If I move this to be a top-level field in the create and update requests, do you think we would have agreement?

[krancour]

@zhongyi-zhang sorry... just saw your comment from way back on Aug 28. This serves a different purpose than "alias" in OSBA. Email for details if you need some clarification.

[zhongyi-zhang]

@krancour yeah, I think so. I just feel like it can be generally used for covering the purpose of alias. Anyway, OSBA can keep using alias.

[duglin]

Spoke with the IBM Cloud team and they would prefer that this be at the top level (sibling to service-id). We view "context" as platform specific contextual information about the environment in which this service is being used. Data about the service itself (like service, plan, params and now "name") are all siblings to us. We also discussed the idea of a new bucket to hold this (and potentially future) information, but we didn't see any value in that. It just adds another wrapper w/o any semantic purpose since we already have an extension point at the top-level that can be used for this purpose. Once you add a bucket we then need to explain when it should be used vs the top-level -and what if people get it wrong? Then we have interop issues. Its a problem we can avoid by not introducing syntactical sugar.

[mattmcneeney]

@duglin I can see that argument! It also makes the update call a little easier, as again, name can be sent at the top-level (though I do still worry some brokers may not like an UPDATE call with just a name field, when a user renames a service instance in the platform, but the platform can just be careful to handle errors in these cases).

Update inbound...

[mattmcneeney]

@duglin what do you think now?

[mattmcneeney]

Updated @duglin

[duglin]

From a technical perspective I like it. Thanks.

I'm wondering if we should add some text, someplace, to help explain this field bit more though. I'm worried that people won't realize that it's really just an "FYI" field and not really one that's meant to be used for anything serious - like a key into a DB - unlike our IDs. For example, a person coming from Kube (where "name" is critical) might not realize how temporal (or optional) this field is despite the spec not saying its required.

Perhaps around line 901 we could add something along the lines of:

---

Platforms MAY choose to include a name field as part of the metadata about a Service Instance. When present, this field is meant to be for informational purposes only. Meaning, Service Brokers are not expected to use it as any kind of "primary key" or identifier for the Service Instance. The instance_id is meant to be used for that purpose. The name field is made available simply to allow Service Brokers to use it in situations such as a user interface where they would like to show the name of the Service Instance rather than (a less UX friendly) instance ID. Because this field is OPTIONAL, Service Brokers will need to be prepared for cases where this field is not provided. Additionally, Platforms SHOULD notify Service Brokers of updates to this field (via the PATCH operation).

---

[mattmcneeney]

I took your words and changed them a little @duglin! Feedback please!

[duglin]

I like it

[mattmcneeney]

Discussion point from today's call:

• Should this be an opt-in feature for service brokers? Is this too heavyweight?
• For brokers who don't need the name field, they are going to receive an additional call that they don't need to receive
• Should we make this extensible for other platform metadata? Or is the name the only important field?

[mattmcneeney]

I've thought a little more about the above discussion points, and personally I think this approach now outlined in this PR seems reasonable. Broker authors can ignore the name field if they don't want to use it, and the worst case scenario seems to be that they'd need to handle the case where an update request arrives with no plan or parameters. And any failure that occurs here could be hidden by the platform (we'll probably do this for cf rename-service).

What do you think @openservicebrokerapi/osbapi-pmc ?

[mattmcneeney]

Ping again @openservicebrokerapi/osbapi-pmc I'd like to get this reviewed and merged if possible!

[duglin]

LGTM

One question, what are we doing with https://github.com/openservicebrokerapi/servicebroker/pull/620 w.r.t. this one? I thought #620 was meant to be the more generic version of this. If #620 goes in, is this one necessary?

[mattmcneeney]

I'm still not sure what would belong in #620 that isn't covered by either name or context.*. @tinygrasshopper may disagree with me, but he's on vacation now. We can leave this until January, but personally I think name is such a commonly used thing that it could belong at the top level.

[mattmcneeney]

Closing this PR as I'm going to create a new one which covers both this and #620