apisurface only returns InternalServerErrors.

[jmrodri]

apisurface needs to handle a bunch of error conditions from the Brokers and return appropriate OSB spec compliant error codes.

Here are the responses from handler.go https://gist.github.com/jmrodri/8336a1c7738c916ed3ea39330821dcbf

https://github.com/openshift/ansible-service-broker/blob/master/pkg/handler/handler.go

and the corresponding utility file

https://github.com/openshift/ansible-service-broker/blob/master/pkg/handler/io.go

[jmrodri]

catalog:

• 500
• 200

last_operation (service instances) and last_operation (service bindings):

• 200
• 400 malformed request
• 410 async delete operations

provision:

• 200 instance exists...
• 201 instance provisioned synchronously
• 202 async provision
• 400 malformed or missing mandatory data
• 409 instance already exists but with different attributes
• 422 broker only supports async no accepts_incomplete flag passed

update:

• 200 changes applied
• 202 update in progress
• 400 malformed or missing data
• 422 if change is not supported

binding:

• 200 binding exists...
• 201 binding created synchronously
• 202 async binding
• 400 malformed or missing mandatory data
• 409 binding already exists but with different attributes
• 422 broker only supports async no accepts_incomplete flag passed

unbinding:

• 200 binding exists...
• 202 async unbind
• 400 malformed or missing mandatory data
• 410 binding gone
• 422 broker only supports async no accepts_incomplete flag passed

deprovision:

• 200 instance deleted
• 202 async deprovision
• 400 malformed or missing mandatory data
• 410 gone
• 422 broker only supports async no accepts_incomplete flag passed

getserviceinstance:

• 200 instance
• 404 not found
• 422 instance being updated, can not be fetched

getbinding:

• 200 binding
• 404 not found

[jmrodri]

Best to create a test for all of these cases then fix the error conditions.

[shawn-hurley]

I actually worry about this because for get binding and other async binding logic is not apart of the spec released spec. I think that they would need to be behind some sort of feature gate to turn it on and off. I personally think that whatever we choose to do here, it needs to be generic and easily extensible as it appears the OSB is going to continue with proving spec changes by implementing.

[pmorie]

You control the return code by returning an HTTPStatusCodeError from the callback.

[pmorie]

Hrm i though the thing with the erros was better documented - i’ll fix that.

Your list of error codes should go ito the godoc on the interface

[pmorie]

https://github.com/pmorie/osb-broker-lib/blob/master/pkg/broker/logic.go#L37

[jmrodri]

Are you saying that the Provision method, https://github.com/pmorie/osb-broker-lib/blob/master/pkg/broker/logic.go#L56, should return an HTTPStatusCodeError for the particular scenarios outlined above in , #13 (comment)?

[shawn-hurley]

I personally think that the library should create semantic meanings for the returns. InProgressError or something similar and then the library handles the correct return for that semantic value in the context of the action being performed.

Pushing the logic of conforming to the spec down to the implementors of the library does not, IMO, provide that value that it could.

[pmorie]

@jmrodri @shawn-hurley

Yes, Provision should return an HTTPStatusCode error for the scenarios you defined.

I would be happy to add helper methods to create errors for specific scenarios like InProgressError, but I want to avoid creating a detailed internal model in this repo. The key here is to give you just enough glue to get started implementing a broker rather than having a prescriptive model for handling every different scenario. If you wanted to build a library that provided that type of programming experience, you could do that in a different repo that built on top of this library.

[shawn-hurley]

@pmorie I think a perfect example of where the current model does not work is Provision.

Provision has 3 success response codes: https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#response-2

200 - OK
201 - Created
202 - Accepted

So if I wanted to be spec compliant right now, I would return an HTTPStatusCode Error for 201? But that would return an error response and not the provision response that I would need.

The same thing exists for binding. We could add a field to the provision response to handle these scenarios, or have some other type of return to capture these?. This is not about an "internal model" or "prescriptive model" to me it is about being able to use this library to conform to the spec.

[shawn-hurley]

Let me rephrase that last sentence, I agree that this should be simple glue code, I just don't want to have to switch from this to my own to conform to the spec after I get started.

[pmorie]

So if I wanted to be spec compliant right now, I would return an HTTPStatusCode Error for 201? But that would return an error response and not the provision response that I would need.

Nope, you return a response with Async = true and the library sends the right thing.

See: https://github.com/pmorie/osb-broker-lib/blob/master/pkg/rest/apisurface.go#L95

[pmorie]

@shawn-hurley I see what you mean now - I think we should have our own response types that embed the osb client ones but add fields to indicate, for example, that you should respond to a provision request with 201 without having to return an error, example:

type ProvisionResponse struct {
  osb.ProvisionResponse

  AlreadyReceived bool  // if true, return 201
}

[jmrodri]

Fixed by PR #27