-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generic Extensions (actions) #431
Generic Extensions (actions) #431
Conversation
Hey rhodie27! Thanks for submitting this pull request! All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate). When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization. If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions. Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look. |
Reopen please, my org was private for some reason. I fixed the CI issues. |
Hey rhodie27! 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. |
spec.md
Outdated
|
||
##### Extension APIs Object | ||
|
||
The `extension_apis` object MAY be used to describe any additional endpoint needed related to a Service Instance. An examples of this might include backup and restore endpoints for a database. If present, MUST return a `discovery_url`. See [Extensions](#extensions) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you wrap non-table lines at 80 columns to be consistent with the rest of the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if present, each item in the extension_apis
array MUST include a discovery_url
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, I think this MUST is redundant with the REQUIRED aspect of the discovery_url you mention in the table below. So it might be best to remove here so we're not duplicating requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got these.
spec.md
Outdated
|
||
| Response field | Type | Description | | ||
| --- | --- | --- | | ||
| discovery_url | string | A URI pointing to a valid OpenAPI 3.0+ document describing the API extension(s) to the Service Instance including server location, endpoints, parameters and any other detail required for invocation. The location of the API extension endpoint(s) can be local to the Service Broker or on a remote server. The Platform MUST execute these API extensions on behalf of the client. MUST be a valid URI. See [OpenAPI](#openapi-document) for more information.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/required for invocation/needed for invocation/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably use the '*' pattern to indicate that its a required field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than "The Platform MUST execute...", which sounds like the platform is required to invoke these even if it has no need to, I would suggest that we remove that sentence from here. Instead, since this is really more of a generic statement about the entire concept and not about this one field, let's add a sentence to the intro section at line 849 about this issue. And perhaps there say something like:
These APIs are extensions to the Open Service Broker API. As such they are intended to be invoked by the Platform on behalf of its clients.
I tried to think of a way to make it normative but it felt a bit odd. I don't think we ever say that the normal OSBAPIs MUST be invoked by the Platform, although its implied. So I think we can probably live with the same non-normative-ness in these too. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense. I changed it.
spec.md
Outdated
| Response field | Type | Description | | ||
| --- | --- | --- | | ||
| discovery_url | string | A URI pointing to a valid OpenAPI 3.0+ document describing the API extension(s) to the Service Instance including server location, endpoints, parameters and any other detail required for invocation. The location of the API extension endpoint(s) can be local to the Service Broker or on a remote server. The Platform MUST execute these API extensions on behalf of the client. MUST be a valid URI. See [OpenAPI](#openapi-document) for more information.| | ||
| credentials | object | A Service Broker MAY return authentication details for running any of the extension API calls, especially for those running on remote servers. If not present, platforms can assume that standard broker authentication will work for the new endpoint(s). See [Extension Authentication](#extension-api-authentication) for more information.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not present, the same authentication mechanism used for the normal Open Service Broker APIs MUST work for the new endpoint(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
spec.md
Outdated
| --- | --- | --- | | ||
| discovery_url | string | A URI pointing to a valid OpenAPI 3.0+ document describing the API extension(s) to the Service Instance including server location, endpoints, parameters and any other detail required for invocation. The location of the API extension endpoint(s) can be local to the Service Broker or on a remote server. The Platform MUST execute these API extensions on behalf of the client. MUST be a valid URI. See [OpenAPI](#openapi-document) for more information.| | ||
| credentials | object | A Service Broker MAY return authentication details for running any of the extension API calls, especially for those running on remote servers. If not present, platforms can assume that standard broker authentication will work for the new endpoint(s). See [Extension Authentication](#extension-api-authentication) for more information.| | ||
| adheres_to | string | A URI pointing to a specification detailing the implementation guidelines for the OpenAPI document hosted at the `discovery_url`. If present, must be a valid URI.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if instead of "pointing" (which sort of implies there has to be a server listening at the endpoint), we should say "referring". Almost the same but perhaps wishy-washy enough to then allow us to add: While this property is a URI, there is no requirement for there to be an actual server listening at that endpoint. This value is meant to provide a unique identifier representing the set of extensions APIs supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Wishy-washy it is!
spec.md
Outdated
"discovery_url": "http://example-openapi-doc.example.com/extensions", | ||
"credentials":[{ | ||
"tokenURL": "https://example.com/api/oauth/token" | ||
}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Got it.
spec.md
Outdated
|
||
### Extension API on Remote Servers | ||
|
||
Additional API endpoints MAY be executed on a remote server, however the OpenAPI document MUST include the servers `url` so that the Platform will know how to invoke the endpoint(s). If the server `context` parameter of the OpenAPI document is set to local host the Platform can assume the extension API endpoints are to be invoked using the Service Broker host and port. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/can assume.../MUST use....
I assume that its just the host (localhost) that is swapped out in favor of the original host/port, right? meaning any path stuff in the OpenAPI doc should still be used right? We should probably say that if so. You may want to also say that any port number specified will be replace too if "localhost" is used - if that's your intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking specifically about the server object, https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#serverObject, url. It can or cannot include a port. I'm think we want to say that if the doc just says http://localhost for url that signifies that it should run on the SB host/port. However, there is an issue if the OAPI doc provides more than one url. Perhaps we can say the doc should provide a url with a description "Service Broker host" to clearly identify the right one, whether it's localhost or other...
I need to think about this more, so I didn't modify it. Can get to it tomorrow.
spec.md
Outdated
|
||
### Extension API Authentication | ||
|
||
The Service Broker MAY use the the same broker authentication method for invocation of extension endpoints by not providing credentials as part of a `extensions_api` object. If no credentials are present in an `extensions_api` object, the Platform can assume that the default broker authentication is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dup from what's above - I would suggest that we only say it once - and I think the one from above is good enough. You may be able to just move all of this section up above - not sure its worthy enough for just one para. Actually, would it read better to just move this entire "Extension" section into what we have above since it kind of feels like a repeat of the same material and anything new could just be sprinkled up there. Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't sure. Like I wrote in PR, while writing this I separated it out b/c a lot of it seemed like it would be redundant if we added extensions on other resources down the line. But, obviously, we could always abstract it out at that point in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it your way, for now. If others chime in and want it separated out I can always revert.
@rhodie27 overall I think it looks good! Just some editorial suggestions. |
@rhodie27 any chance of addressing the comments before tomorrow's call? |
spec.md
Outdated
Extension API endpoints MAY be executed on a remote server, however the OpenAPI | ||
document MUST include the servers `url` so that the Platform will know how to | ||
invoke the endpoint(s). If the server `context` parameter of the OpenAPI | ||
document is set to localhost the Platform can assume the extension API endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may want to backtick localhost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we s/can assume/MUST assume/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I think that makes sense. I can't think of a reason they wouldn't want to assume that.
spec.md
Outdated
|
||
| Response field | Type | Description | | ||
| --- | --- | --- | | ||
| discovery_url | string | A URI pointing to a valid OpenAPI 3.0+ document describing the API extension(s) to the Service Instance including server location, endpoints, parameters and any other detail the platform needs for invocation. The location of the API extension endpoint(s) can be local to the Service Broker or on a remote server. MUST be a valid URI. The returned OpenAPI document MUST be in json format. See the [OpenAPI Specification](https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md) for more information. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add a *
and the * Fields with an asterisk are REQUIRED.
sentence after the description of the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I thought I did. Must've reverted. There now.
|
||
The `extension_api` object MAY be used to describe any additional endpoint | ||
to the Open Service Broker API. An example of this could be lifecycle | ||
management of a Service Instance, (e.g. "Day Two Operations"), like Backup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the specs to clarify when an extension_api should be defined and used by broker authors w.r.t. to asking 1st class support in the OSB API.
Are platforms expected to provide UI/CLI tooling for any declared extensions (i.e. dynamically generating UI from the openAPI document) ? Would generated UIs and user workflows be restricted to single API endpoint calls, or would instead spawn across multiple API calls (e.g. list backups, restore backup, delete backup) ?
Are platforms instead expected to additionally support specific curated extensions and bring optimized user experience for them? If so, where/how would such curated extensions be defined and published ?
Can the specs recommend use-cases for extensions such as:
1- operator specific extensions : a platform operators is adding custom features to a broker and brings custom UI on along it.
2- service specific extensions (say stop/resume operation) that only applies to a category of services
3- vendor specific extensions (say K8S/CF vendor X way of offering backup which differ from vendor Y)
4- platform specific extensions (e.g. CF AR service metrics ingestion which differs from K8S metrics ingestion) that can't be supported by platform-specific profiles
Would the mentioned day 2 operations seem be fitting some of the cases above?
Backup, Restore
: vendor specific extensionsStop, Start, Restart and Pause
: service specific extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the specs to clarify when an extension_api should be defined and used by broker authors w.r.t. to asking 1st class support in the OSB API.
The current thinking is it is entirely up to the broker author, and the platform can choose to provide, at minimum, the OpenAPI UI (or even just the OpenAPI url). Then platforms can extend the UI based on 'adheres_to'.
Are platforms expected to provide UI/CLI tooling for any declared extensions (i.e. dynamically generating UI from the openAPI document) ? Would generated UIs and user workflows be restricted to single API endpoint calls, or would instead spawn across multiple API calls (e.g. list backups, restore backup, delete backup) ?
We had talked about how to batch calls but it became clear that this could be designed forever and we decided to support just the single endpoint as a first step, but nothing stops a clever broker from providing batch/aggregation on their own.
Are platforms instead expected to additionally support specific curated extensions and bring optimized user experience for them? If so, where/how would such curated extensions be defined and published ?
adheres_to
is the mechanism we think will do this. If it is proven that this is not enough we can always revisit the spec to add what is lacking.
Can the specs recommend use-cases for extensions such as:
1- operator specific extensions : a platform operators is adding custom features to a broker and brings custom UI on along it.
2- service specific extensions (say stop/resume operation) that only applies to a category of services
3- vendor specific extensions (say K8S/CF vendor X way of offering backup which differ from vendor Y)
4- platform specific extensions (e.g. CF AR service metrics ingestion which differs from K8S metrics ingestion) that can't be supported by platform-specific profiles
I suspect this is out of scope for the current PR, at least until we see some real world emergent behavior for generic actions.
I believe we were supposed to find out how much interest we each had on this one... from our side we are still interested in the idea in general but have no immediate need for it. I guess that means its a "nice to have" but not a "show stopper" or "must have" for us right now. |
After a period of inactivity we are now revisiting generic instance extensions. There is a new proposal doc here. |
Closing this as we are planning on moving forward with #670 |
See issue #114.
First draft of the generic actions (aka extensions) proposal on a per-service-instance basis.
A couple notes: