fix(exceptionhandler): Remove exception handler from the controller s…

[srekapalli]

• Remove exception handler from the controller since we have a generic handler in Kork.
• Since we have dup. exception handlers, anything that gets wired up last will be used in processing that exception and in this particular scenario it was logging messages related to this controller though they were not related to keel.

[marchello2000]

thank you!

[cfieber]

does the generic ExceptionHandler from kork have the same behaviour of propagating the retrofit exception (status/headers/content)?

if not and if we want to preserve the existing API behaviour then we could wrap the keel API calls with something that catches RetrofitException and wraps it (KeelRetrofitException?) and re-throws, and update this ExceptionHandler to that specific exception

[cfieber]

it does look like this will change the behaviour:

in https://github.com/spinnaker/kork/blob/master/kork-web/src/main/java/com/netflix/spinnaker/kork/web/exceptions/GenericExceptionHandlers.java#L82

it looks like it is just doing a sendError with the HTTP status code

[cfieber]

Via HasAdditionalAttributes in GenericErrorController the new behaviour would be to end up encoding the responseBody as a body attribute under the generic error response

[emjburns]

It is good to know that I broke something when adding this functionality for keel. @srekapalli could you maybe update the normal gate error handling to actually pass the error information through to the user?

I added this functionality so that when we threw good error messages in keel they would actually show up to the user.

[srekapalli]

I added this functionality so that when we threw good error messages in keel they would actually show up to the user.

Sounds good. Will address the response/exception handling to match what we had.

[srekapalli]

@emjburns : If keel sends the errors with JSON content type, GenericExceptionHandler will preserve the response received and sends it back to the caller in the body attribute. Does keel send the error responses in JSON format with content-type as JSON..?

If we need to send the error response as body to the caller we need to raise some special exceptions and handle it that way. But IMO, it's better to stick to our generic error handling scheme and have callers look at body attribute of the error response payload.

@cfieber / @emjburns : thoughts ?

[robfletcher]

I believe Keel's error responses will use either application/json or application/x-yaml depending on the Accept header sent with the request.

[srekapalli]

@emjburns / @robfletcher : If we fallback to our generic exception handler to serialize the Retrofit error, we send the below response to the caller and caller need to look at the body attribute to parse out the actual body returned. If this is acceptable, I would like to merge this PR.

Just a sample response from my unit test:

{
"timestamp": 1580147682079,
"status": 409,
"error": "Conflict",
"message": "409 Keel error",
"body": "{"messageFromKeel":"Error from the cloud provider"}",
"url": "http://something.com"
}

[emjburns]

That sounds fine. When I added this the errors weren't getting propagated successfully. I'm ok with you merging this in and maybe you can help if things aren't working still :)