Bug 1536088 - fixes panic when bind can't find ServiceInstance

[mhrivnak]

fixes #652

[rthallisey]

Do we want to keep err.Error() since there are multiple errors we could return back from GetServiceInstance?

[mhrivnak]

Hm. I'm assuming that if GetServiceInstance returns a ErrorNotFound, it means the service instance was not found. Any other reason for returning that error would be surprising, I think. Different failure reasons should result in different error values, right?

I changed the message because from a client perspective, they just saw description "not found", without any hint as to what wasn't found. So in any case, at some point in the stack I think we need to get specific about what wasn't found, so the error in the HTTP response can be more helpful. A different way to do that would be to have GetServiceInstance return an error that already has a specific message.

[rthallisey]

Hm. I'm assuming that if GetServiceInstance returns a ErrorNotFound, it means the service instance was not found.

I agree, I think the the right assumption. Because we're either returning ErrorNotFound or the error from getObject(...), which I think are going to be the same.

A different way to do that would be to have GetServiceInstance return an error that already has a specific message.

I think this would be beneficial because there are multiple places in the handler that call broker.GetServiceInstance. We could pass back the error message from that function so it improves the error message for all callers.

[shawn-hurley]

I think this would be beneficial because there are multiple places in the handler that call broker.GetServiceInstance. We could pass back the error message from that function so it improves the error message for all callers.

I do not prefer this approach, This would mean that the handler is assuming that the broker is handling it's error messages correctly. I would prefer to have the handler to manage what it returns in the response and not pass that responsibility deeper into the call stack.

I do think that would make sense to log the error so that we still get visibility of the error in the logs.

Just my two cents.

[rthallisey]

This would mean that the handler is assuming that the broker is handling it's error messages correctly

uh oh, I think this would be a bug in the broker.

I do not prefer this approach

I could go either way on this, but what I would prefer for now, is that we are uniform with either using the function to return a detailed error or having the handler deal with its own errors. I think the latter is a separate PR and I don't want to block this bug fix on that in case there is more discussion we want to have on that change.

[jmrodri]

I would have just left this as err.Error(). Almost all the other cases of ErrorNotFound we just return err.Error()

$ grep -A2 ErrorNotFound pkg/handler/handler.go
		case broker.ErrorNotFound: // return 404
			writeResponse(w, http.StatusNotFound, broker.ErrorResponse{Description: err.Error()})
		default: // return 422
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()})
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()})
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusGone, broker.DeprovisionResponse{})
			return
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusGone, broker.DeprovisionResponse{})
			return
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()})
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusNotFound, broker.ErrorResponse{Description: err.Error()})
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()})
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()})
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusGone, nil)
		default:
--
		case broker.ErrorNotFound:
			writeResponse(w, http.StatusGone, nil)
		default:
--
		case broker.ErrorNotFound: // return 404
			log.Debugf("Binding not found.")
			writeResponse(w, http.StatusNotFound, broker.ErrorResponse{Description: err.Error()})

[mhrivnak]

It looks like there are a lot of cases already in handler.go where a handler function is setting a specific error message when writing a response, based on inspecting the return values (error included) from some other function. I did a search for Description: " and came up with 29 hits in that file.

Beyond just the Description, the handler functions are always deciding what http response code to use based on details of an error or other return values, and that's arguably more impactful than the human-intended text description.

So my take is that this behavior is already pervasive within handler.go. Inspecting errors from the broker and using the details to compose new and descriptive HTTP responses is the established precedent.

That said, I very much see your point about there being an opportunity to tighten up the error handling flow and better-define that part of the API between the broker and handlers. The errors are being used today both to decide what response code to use, and to sometimes convey a human-readable error message for either devs or http client users (but it's not always clear which). Those two roles are in conflict with each other in some cases, as we're seeing here, so it would be great to better-define that. However that is the direction that I think would require a lot of change (for example it gets more complex to have a generic ErrorNotFound as we do now if we expect it to specify an object type). Maybe it makes sense to tackle that as an enhancement once we're out of bug-fix mode? I'd be happy to help out with it.

For now, I think the options are to keep the PR as-is, or go back to just using the description provided from the error. I don't love the latter, because it's a worse user experience, but I'm ok either way. Improving the error message coming out of broker.go would be a much bigger project.

[coveralls]

Changes Unknown when pulling 0ac92d6 on mhrivnak:fix-panic-on-missing-serviceinstance into ** on openshift:master**.

[jmrodri]

I think returning "ServiceInstance not found" is what hangs me up. I'd rather leave it at just "not found" for now and figure out if we need to address this across the board or not.

[shawn-hurley]

@jmrodri @rthallisey it makes sense to deal with err.Error() stuff in a different PR.

[eriknelson]

I agree with @shawn-hurley. I would leave the err.Error() as-is, and make a holistic change that addresses the issue in a separate PR, since the contents of Description are functionally distinct from the bug that's actually fixed by this PR. There's definitely a discussion to be had on exactly what makes sense for that response to contain, and we should make sure we're consistent.

[mhrivnak]

Ok, I reverted the error message change, so we can save that for another day.

[coveralls]

Changes Unknown when pulling 3c5fce8 on mhrivnak:fix-panic-on-missing-serviceinstance into ** on openshift:master**.