fix: provide meaningful error message for rs error
- [X] I have viewed signed and have submitted the Contributor License Agreement
- [X] I have made this pull request to the
mainbranch - [X] I have run all the unit tests using
scripts/run-unit-tests-in-dockerfrom routing-release. - [ ] (Optional) I have run Routing Acceptance Tests and Routing Smoke Tests on bosh lite
- [ ] (Optional) I have run CF Acceptance Tests on bosh lite
If a route-service violates the route-service timeout, the previous
error message was 400 Bad Request: Failed to validate Route Service Signature for x-forwarded-client-cert which is not indicative of the
underlying failure. This change does two things:
- If the route-service validation failed because of a route-service timeout, respond with the proper error message and 504 GW timeout
- If the route-service validation failed because of something else, respond with a 502 Bad GW since the error is most likely not on user side, but in the intermediate server. I know that the status codes do not match 100%, but IMHO they are more appropriate then the previous 400 Bad Request, and the route-service is, in some sense, a upstream server to the gorouter, just not to the one giving the error message.
Resolves: cloudfoundry/routing-release#272
The new response for route-service timeouts would be:
504 Gateway Timeout: Failed to validate Route Service Signature: route service request expired
I ran the tests and got the test case related to my change green again, but there are still lots of other tests that were already failing.
Thanks for the review!
- Will check that, but if I remember correctly it returns a 502 in that case
- Didn't catch that, will update this section as well
- Yes, that should be possible, will do
- It doesn't, there are no cases for
sanitizeError
I will try to prepare a commit for the open points today.
FYI the CI job for this was failing irrespective of the PR, and it has been since disabled. Ignore that status check when reviewing.
@maxmoehl any progress on the updates for this?
Hi @geofffranks, sorry i totally forgot about this PR 😓
I changed the second occurrence you pointed out and adapted the unit-tests, they are now passing locally (./scripts/run-unit-tests-in-docker gorouter). I also did a free-text search for more occurrences of such error handling but couldn't find any.