peripli / specification Goto Github PK

Operation that returns all registered service brokers is missing in SM Control API.
Add this operation to specification with all required details:

• Request syntax
  • Parameters
  • Payload (if any)
• Response
  • Payload
  • Error codes

Here is a list of feedback I collected while re-reading the spec. I am willing to create PRs but I want to make sure people think this feedback is valid.

https://github.com/bkrannich/specification/blob/sm-ops/api.md#overview

• "defines an HTTP interface" - isn't that a REST interface, actually? If not, what prevents it from being one?
• SM abbreviation is used without being introduced (even if it is obvious)
• Same for CLI
• is all lowercase, with no spaces - dashes are allowed, right? Wondering if this is the right section to discuss this requirement.

https://github.com/bkrannich/specification/blob/sm-ops/api.md#creating-a-resource-entity

• Would it be helpful to have a resource returning all the resource entities known to the system and their LIST endpoint? E.g. for dynamic discovery?
• "Some APIs may allow passing in the resource entity id (that is the ID to be used to uniquely identify the resource entity) for backward compatibility reasons. If an id is not passed as part of the request body, the Service Manager takes care of generating one." - what happens if the ID is already assigned to some other entity?

https://github.com/bkrannich/specification/blob/sm-ops/api.md#body-3

• "* Fields with an asterisk are REQUIRED." - why does this appear at this stage in the document (and not earlier)?

https://github.com/bkrannich/specification/blob/sm-ops/api.md#resource-types

• Meta-comment: This section is rather lengthy for the fact that it "only" provides "forward pointers" to later sections of the document.

https://github.com/bkrannich/specification/blob/sm-ops/api.md#osb-management

• "and be registered as one in the platforms that are associated with it" -> "and be registered as service broker in the platforms that are associated with it"
• "When a request is send" -> "When a request is sent"

Define domain model and corresponding data structures that shall be used for persisting the model in database.

Write details about CLI commands. For each command specify:

• input parameters
• expected result
• error cases

The structure need to be like this:

• problem definition and possible solutions and negative and positive values of the current approach
• overall architecture and concept for current approach (high level architecture diagram)
• scenarios
• sequence diagrams on scenarios
• APIs with details

Keep each section in separate document, linked from index (README.md).

Create sequence diagrams that visualise the communication and participants on each of the calls in all of the covered scenarios.

All committers have to sign the CLA (SAP_CLA) prior PR to be merged.
Link the repository to cla-assistant.io to ensure this.

PATCH is used to update a resource and missing fields of the request are treated as "non-changed" fields - we should then provide a mechanism to delete (or null out) a field. I would suggest that passing in "null" to mean null out a field - and I would make a "null" value for a field the same as deleting the field. I'm not sure there's value in an optional field being there with a value of "null".

Once #6 is Done, proceed and create detailed content for section "problem definition"

Operation that returns all registered platforms is missing in SM Control API.
Add this operation to specification with all required details:

• Request syntax
  • Parameters
  • Payload (if any)
• Response
  • Payload
  • Error codes

Rather than supporting OSBAPI for instance and binding management, we should consider using a more "normal" REST HTTP approach - one that continues the pattern defined by the APIs in the spec for managing platforms and brokers.

I think more is needed w.r.t. describing how async works. I would prefer a model like:

• request is sent to the server
• 202 Accepted is returned + Location header for the client to check status
  • even if the Location is the same URL as the resource itself
• in the case of a failed create(), the resource at the Location URL still exists but with the failed status and error message. How long it lives for is an impl detail. But this is also a good reason to NOT let the client pass in the ID and use it as part of the URL of resource. Let each create result in a new resource.
• in the case of an async delete(), a GET on the resource will always return at least the status section until the resource is gone - then a 404 would be returned.

To register (or create) a new platform resource we do: POST /v1/platforms
and the ID of the platform is in the HTTP Body.

We can then later get info about that platform (or update it) via /v1/platforms/:ID.

1. it seems to me that if we're going to have the client pass in the ID then it should be a PUT to the v1/platforms/:ID URL not a POST since POST is typically for when the server is going to decide things like the resource's ID and final URL.

2. should the client pick the ID at all? What is the benefit of forcing the client to pick a globally unique value? Is it just for easy reference later on? If so, what is the benefit of using that ID as part of the URL? When we allow some other system to pick our unique IDs then we run the risk of them needing to change them later on - or (forever) maintain a mapping. E.g. what if they need to update the ID? I'm wondering if ID should just be a field of the resource but not part of the URL of the resource. Then clients can update the ID if they need to w/o the URL of the resource changing. Since we're not bound by the "brokers are stateless" constraints, we don't need to force the client to provide info on behalf of the server.

Service Manager could expose an unsecured endpoint that provides information that can be used when accessing its APIs.

This endpoint would be of use to the CLI. It can obtain the endpoints for issuing and refreshing tokens when calling the Service Manager's APIs.

Example:
GET /v1/info
Reponse:

{
    "token_issuer": "https://example.com"
}

Where example.com has .well-known/openid-configuration endpoint as per https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest

This configuration used in the Service Manager authentication should be similar to https://kubernetes.io/docs/admin/authentication/#openid-connect-tokens whereas this info endpoint could be similar to https://apidocs.cloudfoundry.org/268/info/get_info.html

State Object

All resources that support mutation operations (creation, deletion and update) MUST contain a state object. After performing a mutation request, the response MUST include a Location header from where information about the resource's state can be obtained. This MAY or MAY NOT be the Retrieve (GET) API for the resource entity that is being mutated.

The state reflects the real state in which the resource currently is. It includes information regarding whether the resource is currently usable or not (ready or not) and it contains conditions that provide further details if the resource is not ready (is not usable).

The state is of particular interest when the mutation operations are asynchronous. But even synchronously mutatable resources should provide state for consistency reasons. This state of synchronously mutated resources would often include no conditions.

Conditions Object

The conditions describe the current condition in which the resource is.
Each resource MAY add resource specific conditions and should also handle those conditions accordingly in case they sum up to an undesired state (in most cases this would be a ready:false state).
Each condition MAY include additional fields apart from those specified below in order to provide meaningful information.

The state MUST be maintained up-to-date and reflect the real state of the resource. When a process or a component takes actions due to the state not matching what is actually desired, the process or component MUST also take care of updating the respective conditions that it has performed actions upon. When any conditions are updated the ready field MUST be recalculated and updated by the Service Manager.

Each Service Manager resource MUST define the conditions that are relevant for it. It MUST also define an aggregation policy that sums up the conditions and decides whether the resource is ready or not. Based on this policy every time a resource's condition is changed, the ready field should be recalculated and updated. The fact that each resource defines it's own conditions and an aggregation policy based on those conditions is an implementation detail but it is worth mentioning it here to better describe the idea behind the state.

Example Resource with a State Object:

{
  "id": "0941-12c4b6f2-335a-44a3-b971-424ec78c7353",
  ...
  "state": {  
    "ready": "False",
    "message": "Service Binding is currently being created",
    "conditions": [  
      {  
        "type": "last_operation",
        "status": "in_progress",
        "message": "Creating deployment pg-0941-12c4b6f2-335a-44a3-b971-424ec78c7353",
        "name": "Create"
      }
    ]
  }
}  

The example state above represents the state of a resource called service_binding with an id equal to 0941-12c4b6f2-335a-44a3-b971-424ec78c7353. The state implies that the service binding is not ready (it is not usable) due to the fact that the last operation (namely Create, hence the name field) performed on this entity is currently still running (the status of the last_operation condition is in_progress).

The word "Landscape", to me, brings up thoughts of something abstract - e.g. "What is the Cloud Landscape with respect to Serverless?" But in this case we're talking about a concrete set of Platform Instances. I think it would require less learning for people to use "Environment" or even "Instance".

Create a document with clear statements about contribution to this project.
Similar document have on other open source projects. Examples are:
https://github.com/openservicebrokerapi/servicebroker/blob/master/CONTRIBUTING.md
https://github.com/cloudfoundry/bosh/blob/master/CONTRIBUTING.md
...
Mandatory sections are:

• License
• CLA (https://cla-assistant.io/Peripli/service-manager)
• Review process explanation
• Definition of Done for documentation and for implementation related contributions

Once #6 is Done, proceed and create detailed content for section "high level architecture"

architecture diagram + sequence diagram + scenarios

As part of phase 1 some limitation on specification and corresponding reference implementation will be applied. Such limitations are:

• no async calls and operations
• no retries
• initial setup steps with restart of the proxy
  and probably some more.
  Ensure all limitations are included as part of the specification, so we can enhance it in the next phases by providing support for them.

The document structure can be validated with serve of static checks.
Automate those checks with travis build prior PR merge.

I am registering a service broker via SMCTL tool (smctl curl curl /v1/service_brokers -X POST ...). The name of the service broker is always original_name_<subaccount_id>. Can a broker be registered with a name independent from the subaccount?

We could use offset and limit query parameters as suggested in https://pages.apigee.com/eBook-WebAPIDesignTheMissingLink_ConfirmationPage.html
How is this done in the service-catalog or K8S?

Is there any reason why "state" isn't just part of each object instead of under a different url (e.g. /resource/state) ? It would be good if each object had "state" (just like each has "labels") to provide some consistency. Then a user can GET a resource and see it all in one response object instead of having to GET /resource and GET /resource/state.

This would then also allow for a more consistent pattern between sync and async - meaning doing a GET on a resource URL will always tell us what's going on with it.

Create repository for the specification.
The repository should include:

• README.MD that shall be used for index page
• LICENSE file with Apache 2.0 license inside
• NOTICE file
• release_notes.md

The release notes shall be filled manually on each change that modifies the specification.
The changes and the corresponding release notes shall be part of single pull request.

CI for Documentation repository that executes the following tasks on each PR and merge:

• Execute static checks for:
  ** Empty files
  ** Misspelled words
  ** Common formatting issues in MD files
• Generate sequence diagrams (issue 10) and push them in the repository.

Pullapprove-like feature is available in Github by using Code Owners[1]. I think it would be nice to have this for the specification. Have a list of "maintainers" from which a subset must approve a specific pull request before it can be merged. Granted, it lacks the "disable merge" functionality of pullapprove but I think that's sufficient.

[1]https://blog.github.com/2017-07-06-introducing-code-owners/