-
Notifications
You must be signed in to change notification settings - Fork 460
Generic Extensions (actions) #431
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
Changes from 22 commits
c590bd5
cf0ed7e
5a0820c
394d76e
7242bc5
f59f077
914dc83
f66a060
a400b53
12a1a22
99b2414
451a78f
2e15360
bcae8c0
73663f5
f601296
0ce6075
586a5cc
d236bf4
8360d16
5150a30
643e47c
1a64f74
4adddfe
a38ac79
ba5cf6c
eae12ed
750bdcf
ce8cab7
480c4ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -841,11 +841,46 @@ For success responses, the following fields are defined: | |
| | --- | --- | --- | | ||
| | dashboard_url | string | The URL of a web-based management user interface for the Service Instance; we refer to this as a service dashboard. The URL MUST contain enough information for the dashboard to identify the resource being accessed (`9189kdfsk0vfnku` in the example below). 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. | | ||
| | operation | string | For asynchronous responses, Service Brokers MAY return an identifier representing the operation. The value of this field MUST be provided by the Platform with requests to the [Last Operation](#polling-last-operation) endpoint in a percent-encoded query parameter. If present, MUST be a non-empty string. | | ||
| | extension_apis | array-of-objects | For extensions to the Service Broker API, Service Brokers MAY return one or more `extension_api` objects that describe additional API endpoints via an OpenAPI document. See [Extension API Object](#extension-api-object) for more information. | | ||
|
|
||
| ##### Extension API Object | ||
|
|
||
| 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: Would the mentioned day 2 operations seem be fitting some of the cases above?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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'.
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.
I suspect this is out of scope for the current PR, at least until we see some real world emergent behavior for generic actions. |
||
| Restore, Stop, Start, Restart and Pause. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of these example operations seem like they would be long-running. Is there any way for a broker to report status of a long running action back to the platform? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to this, is there a way brokers can provide either success or failure messages for the platform to display when an operation is run? Ideally if the request fails, the UI can give a specific error about what went wrong instead of a generic failed error.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Async ops, if enabled, would be defined via the swagger file. The platform could detect and subscribe to ops based on the returned swagger. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure how a platform can detect it only from the swagger. A human looking at it might notice an endpoint called As an end user, I'd be worried if I couldn't check the status of an operation like "Restore" and know that it completed successfully. I could see it if the platform recognized the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the short answer is it's not going to be possible across the board for all APIs. My thought was that the platform could recognize a callback object in the openapi doc, (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#callbackObject) and other openapi patterns to figure out what it needs, but we won't know until someone tries. It's the main issue with going the generic route on extensions. |
||
|
|
||
| The `extension_api` MUST include a URI to an OpenAPI 3.0+ document that the | ||
| Platform can use to determine the new endpoint(s), parameter(s) and | ||
| authentication mechanism. The new APIs are extensions to the Open Service Broker | ||
| API. As such they are indended to be invokved by the Platform on behalf of its | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indended -> intended
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. invokved -> invoked
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. thanks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: intended
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. thanks. |
||
| clients. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing that occurred to me reading back through this is that it's not clear how the extensions are expected be invoked. Does the platform invoke them? Do users invoke them? This is probably entangled with @spadgett's question about long-running operations.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The answer to this from the googs perspective is yes. If the platform would like to invoke them then that should be ok, and if the platform provides all the information to a third-party that should work out as well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the PR clarify the concurrency aspects that the platform/users should comply with when invoking extensions ? For instance, should a broker exposing a This relates to #467 |
||
|
|
||
| See [OpenAPI Server Object](https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#serverObject) for more information. | ||
|
|
||
| | Response Field | Type | Description | | ||
| | --- | --- | --- | | ||
| | discovery_url* | string | A URI pointing to a valid OpenAPI 3.0+ document describing the API extension(s) to the Open Service Broker API 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. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the dashboardURL we say this "The URL MUST contain enough information for the dashboard to identify the resource being accessed". I think we need something similar here, and possible about the endpoints returned within the swagger doc, because it doesn't really make sense for two instances to be given the same set of URLs - otherwise how will the server know which instance is being operated on.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For
I think we need something in this PR that allows tells people that something needs to be unique in all of the URLs/data being passed around, otherwise how does the generic action endpoint know which instance is being operated on.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't agree that we MUST do this, but I will bow to the group decision. An example I can think of is you want to return the same swagger for every service instance and pass the service instance details via a body parameter, (as defined by the swagger).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do that, IF we also define how someone can programmatically know which parameter in the swagger represents the instance ID. W/o that I wouldn't know how to write code to do it. So, note I said "URL/data" - meaning I'm ok with a statement about instance-unique URLs, or other text talking about how to know which param to set - either is fine - we just need to be clear about how to do it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the latest addresses your concerns.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rhodie27 thanks - need to think more about it, at at first glace yes I think it does. if we have an aggregation service broker in the path between the platform and the real service broker, I'm assuming the aggregation SB might need to twiddle relative URLs provided so they're not relative to the aggregation SB but point to the real one. Is this something we should say in here or is it obvious and we can leave that as an exercise for the dev?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd lean towards "up to the dev". I could see the aggregation SB actually hosting the openapi doc itself instead of passing the request through. And it could have any combination of local and remote endpoints.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case of a local URI, I assume this is a path. Is this path relative to the broker host oder relative to the URL the broker was registered with?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, I hadn't thought of that. I would assume the former b/c of /broker1 is the path you are executing the provisioning from. I could add something like, "If discovery_url is a path, the Platform can assume it is to be called from the basepath of the Service Broker."
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I guess we need something similar also for the |
||
| | server_url | string | A URI pointing to a remote server where API extensions will run. This URI will be used as the basepath for the endpoint(s) described by the `discovery_url` OpenAPI document. If no `server_url` is present, the Platform MUST assume the extension API endpoint(s) are to be invoked using the Service Broker host and port. If present, MUST be a valid URI. | | ||
| | 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, the same authentication mechanism used for the normal Open Service Broker APIs MUST work for the new endpoint(s). If the Service Broker wants to use alternate methods of authentication, (e.g. on remote servers) it MUST provide details to that mechanism in the OpenAPI document, (e.g. an OAuth Flow Object), and the appropriate credential(s), (e.g. bearer token), as part of the `extension_api` object within the `credentials` field. If credentials are present in an `extension_api` object, the Platform will need to verify the authentication method from the OpenAPI document. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to document the sub-fields of this to make sure the same field names are used by all implementations?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is a blocking comment for moving to validate-thru-impl - but if people agree, then we'll need it before merging.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doug, are you talking about sub-fields of the credentials object?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't restrict them. The field names could be anything based on the openapi spec. The platform needs to read the spec; determine the auth type + variables, and then match those using the credentials object.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I didn't realize this field had to conform to a definition in the OAPI spec - should we say that and add a ref to where in the spec they should look? This wasn't obvious to me since I didn't assume that something outside of an OAPI doc would be controlled by the OAPI spec.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that's what it says. :) Let's talk about how to reword this to make that clear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from call: try to clear up that the workflow is this
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we get an example of an auth spec and a matching credentials object?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored the credentials and added some examples. Hopefully it doesn't make it worse. :) |
||
| | alt_instance_id | string | Refers to a parameter in the `discovery_url` OpenAPI document that maps directly to the Service Broker API `instance_id` parameter. If the extension API endpoint(s) use a different value to represent a Service Broker instance, then `alt_instance_id` MUST be present. If not present, the Platform can assume `instance_id` means Service Broker Instance in the OpenAPI document. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you investigating an alternative key name @rhodie27?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but none of them are great. Open to other suggestions.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More brainstorming:
I don't like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @mattmcneeney on this, something other than id would fit better. Any of the above *_key variants seems acceptable. I think I like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. openapi_instance_key is confirmed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to push back on this field entirely and I apologize for not being in the original discussions for it. I think alt_instance_id is an attempt at serving pre-processed open api information and the correct way to solve this issue is with vendor extensions in the open api doc. You could add a key, say
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apology accepted. I agree, this could be done via vendor extentions, however, the overall theme I have been trying to push is to not make the service provider change their OpenAPI document to fit the service broker's needs. Perhaps the service broker author doesn't have that ability/power or the service provider doesn't want a million different vendor customizations in their pretty swagger. I guess we could always say that the Service Broker author needs to maintain their own OpenAPI document... Sounds messy.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. vote is to remove this for now. Mike to add language about the openapi doc must do this somehow. |
||
| | adheres_to | string | A URI refering to a specification detailing the implementation guidelines for the OpenAPI document hosted at the `discovery_url`. 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. If present, MUST be a valid URI. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The semantics of this column could be clarified a little bit - what is the advantage of specifying a URI that points to a location where no server is running?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't follow. Which part are you referencing?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Am I write in thinking that this field is going to be used to allow clients to spot "common" operations that they could do, say, in bulk across multiple service instances?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Often people push for unique IDs to be URIs so that they avoid strings that too simplistic - like "backup". By making it a URI its trying to force people to namespace things by including some domain name in the URI. While I've heard bulk ops as one reason for this, to me the bigger reason is that a platform (or its UI) can know when it comes across some actions that it knows about and may want to do some special processing for. E.g. perhaps it has a dedicated UI for them. W/o some well defined URI to say "yes, these are the set of ops that's been defined by spec xxx" the platform can't really be sure what those ops do. Checking (strcmp) the names of the ops alone doesn't really guarantee anything.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per Paul's suggestion I will add some text here about what the Platform can do with this information. Should clear this up, (mildly). |
||
|
|
||
| \* Fields with an asterisk are REQUIRED. | ||
|
|
||
| ``` | ||
| { | ||
| "dashboard_url": "http://example-dashboard.example.com/9189kdfsk0vfnku", | ||
| "operation": "task_10" | ||
| "operation": "task_10", | ||
| "extension_apis":[{ | ||
| "discovery_url": "http://example-openapi-doc.example.com/extensions", | ||
| "server_url": "http://myremoteserver.example.com", | ||
| "credentials":[{ | ||
| "tokenURL": "https://example.com/api/oauth/token" | ||
| }], | ||
| "alt_instance_id": "serviceInstance", | ||
| "adheres_to": "http://example-specification.example.com" | ||
| }] | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
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 https://github.com/openservicebrokerapi/servicebroker/pull/459/files is adopted then the 2nd column will need to change.