Add support for OSB API client

gberche-orange commented 6 years ago

The spring-cloud-open-service-broker repo is currently supporting brokers that implement the OSB API.

There are also some use cases that require to act as an OSB API client. This includes brokers (e.g. sec-group-broker-filter or Open Service Broker Catalog), or OSB tooling (e.g. OSB API validators mentionned in https://github.com/openservicebrokerapi/servicebroker/issues/363 such as osb-checker) or UIs/CLI such as eden sb-cli. Having support for OSB client in the spring-cloud-open-service-broker repo would enable such uses cases to be using spring cloud.

The OSB client at https://github.com/orange-cloudfoundry/cf-ops-automation-broker/tree/cassandra4/cf-ops-automation-broker-core/src/main/java/com/orange/oss/cloudfoundry/broker/opsautomation/ondemandbroker/osbclient leverages the spring-cloud-open-service-broker 1.0.2 model classes and adapts the interfaces to use them with Feign. I have plans to update to spring-cloud-open-service-broker 2.0.0M1 release.

This approach is limited by the fact that support for OSB API polymorphic responses requires additional annotations for the model classes (either CreateServiceInstanceAppBindingResponse or CreateServiceInstanceRouteBindingResponse), and likely a custom jackson deserializer see inspiration from https://stackoverflow.com/a/32777371/1484823

I wonder whether there is a way for the spring-cloud-open-service-broker repo to accept contributed support for OSB client, or alternatively accept non breaking changes on the model classes so that a distinct repo can import the model classes.

Thanks in advance,

Guillaume.

/CC @JCL38-ORANGE

royclarkson commented 6 years ago

I believe we have already addressed one concern about importing the model classes. In the 2.0.x line, the models are available in the spring-cloud-open-service-broker-core artifact. Including that won't pull in any Spring Boot dependencies or enable Boot in any other way. Otherwise, we're happy to discuss specific ideas you have to make this project more flexible.

scottfrederick commented 6 years ago

As part of the 2.0.0 changes of the project, attention has been given to the usability of the request and response classes based on their typical usage by a service broker project. Request objects are unmarshaled from JSON by the framework, and typically only constructed by client code in tests (e.g. unit tests for an implementation of the ServiceInstanceService or ServiceInstanceBindingService interface). Response objects are constructed by client code and marshaled to JSON by the framework.

It sounds like your use case of re-using these Request and Response objects in an OSB client would flip that typical usage - Request objects would be marshaled to JSON and Response unmarshaled from from JSON. I don't see anything wrong with the objects supporting both types of usage, but we'd likely need more tests to ensure the JSON marshaling and unmarshaling works as expected in the "reverse" OSB client case. Your point that the CreateServiceInstanceBindingResponse subclasses might not unmarshal cleanly is a good example of something that's not tested.

I wonder whether there is a way for the spring-cloud-open-service-broker repo to accept contributed support for OSB client, or alternatively accept non breaking changes on the model classes so that a distinct repo can import the model classes.

We'd be interested in discussing either approach. The latter would be less intrusive to our RC and GA release plans for the project, but those plans aren't chiseled in stone.

gberche-orange commented 6 years ago

thanks @royclarkson and @scottfrederick !

Once I would have completed the bump of my osbclient to 2.0.0 version, I'll get back to you with either contributing the full OSB client (along with their tests ) or merely the models unmarshalling support

royclarkson commented 6 years ago

@gberche-orange unless it's minimal effort, let's please discuss further before you invest too much time in a PR, so we can insure we're all in agreement. Thanks!

gberche-orange commented 5 years ago

@royclarkson @scottfrederick

I'm working to upgrade my osbclient to 2.0.1 version (and later one contribute it back) and am facing the following issue with Jackson deserialization of the spring-cloud-open-service-broker model classes (e.g. org.springframework.cloud.servicebroker.model.catalog.Catalog) that now require usage of a builder.

Would you accept a PR that would add jackson annotations to the model to ease the deserialization of the spring-cloud-open-service-broker model classes ? If so, on which branch should I make this PR against in order to be able to use it in a stable version ?

More details on faced issue:

I managed to configure jackson to use the associated builder class: See https://gist.github.com/gberche-orange/58ef24889e96080a50dc325721bb7fa0. However I did not find a way to configure jackson to use the builder methods without modifying the builder itself in the spring-cloud-open-service-broker: seems builders behaviors can only currently be configured using the @JsonPOJOBuilder. See related https://github.com/FasterXML/jackson-databind/issues/242 and associated source code at https://github.com/FasterXML/jackson-databind/blob/ac82499e5ac9ecbf39e497462aa9542ca0195158/src/main/java/com/fasterxml/jackson/databind/introspect/JacksonAnnotationIntrospector.java#L1221-L1225

I did not find another way to configure jackson without creating a new builder than delegates all mutation calls to the existing builder, and is likely to would require constant maintenance.

gberche-orange commented 5 years ago

I realized that some related work was done in 2.1.0M1 as a result of https://github.com/spring-cloud/spring-cloud-open-service-broker/issues/110 for all model response classes in 77eb38d8d8ba87ebaf781ce4dfaa2f08568f90ec. I only observe that collection fields with null defaults, deserialize as empty collections instead of their original null value (see Plan gist example), which I guess is fine (it only breaks serialization/deserialization tests that make would use of Object.equals() to compare original and deserialized objects).

Response model classes [de]serialization is therefore not an issue w.r.t. builder changes.

Request model classes however seems to serialize some extra fields, submitted related PR #140

gberche-orange commented 5 years ago

@royclarkson @scottfrederick I see in this issue that support for osb client had once been planned for 2.0.1M1 and then de-prioritized. Was there any work/throughts put into this support ? Is there still interest into bringing this feature into spring-cloud-open-service-broker ?

Here would be my design proposal for a synchronous client support (I did not study a reactive one)

new interfaces CatalogServiceClient, ServiceInstanceServiceClient, ServiceInstanceBindingServiceClient that have the same java method signature as corresponding controllers (e.g. CatalogController.getCatalog()).
- It would have been great to share the spring-web annotations on parameters and path between client and controllers. I however did not find a solution to have spring-cloud-netflix support @RequestMapping annotation with multiple paths. It fails with message similar to java.lang.IllegalStateException: Method createServiceInstance can only contain at most 1 value field. Found: [/{cfInstanceId}/v2/service_instances/{instanceId}, /v2/service_instances/{instanceId}]
an integration test with some wiremock recorded responses (e.g. for asserting older versions of OSB clients can be contacted by the client)
an integration test with a OSB broker started in memory, and the client making call to it (to avoid maintaining wiremock recording for each latest OSB spec version)

royclarkson commented 5 years ago

@gberche-orange we didn't make it any further than having a discussion about a client being a useful addition. It's not going to make it into the 2.1.0 release at this point, but we'll certainly reconsider it for the future. The general consensus between us is that we should also build out spring cloud contracts for everything. That would greatly benefit the addition of a client, as well as anyone else implementing a service broker. Your thoughts are greatly appreciated. I can't commit to a timeline now, but we'll continue to consider this for a future release. And we're always open to PRs too of course.

royclarkson commented 5 years ago

@gberche-orange also, if you find anything off with the serialization in your testing, please do create a new issue for that. Thanks again for your support of this project and your feedback.

gberche-orange commented 5 years ago

thanks @royclarkson

also, if you find anything off with the serialization in your testing, please do create a new issue for that.

I submitted #140 and will complete it with more serialization related commits today.

The general consensus between us is that we should also build out spring cloud contracts for everything. That would greatly benefit the addition of a client, as well as anyone else implementing a service broker.

The OSB spec now includes an openapi spec https://github.com/openservicebrokerapi/servicebroker/blob/master/openapi.yaml It would be great to leverage this for authoritative API definition for both client and server implementation. I see related work discussed into https://github.com/spring-cloud/spring-cloud-contract/issues/136

This API definition could then be complemented with additional contracts precisions, either in openapi extensions leveraging spring cloud contract (see https://github.com/springframeworkguru/spring-cloud-contract-oa3), or through externally managed contracts that complement a base contract generated from the openapi spec (e.g. with community contrib described at https://svenbayer.blog/2018/07/17/cdc-with-swagger/)

royclarkson commented 3 years ago

Closing this issue as developing a client within this project is out of scope.

spring-cloud / spring-cloud-open-service-broker

Add support for OSB API client #77