gorouter icon indicating copy to clipboard operation
gorouter copied to clipboard

fix: provide meaningful error message for rs error

Open maxmoehl opened this issue 3 years ago • 4 comments

  • [X] I have viewed signed and have submitted the Contributor License Agreement
  • [X] I have made this pull request to the main branch
  • [X] I have run all the unit tests using scripts/run-unit-tests-in-docker from 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:

  1. If the route-service validation failed because of a route-service timeout, respond with the proper error message and 504 GW timeout
  2. 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

maxmoehl avatar Jul 13 '22 14:07 maxmoehl

The new response for route-service timeouts would be:

504 Gateway Timeout: Failed to validate Route Service Signature: route service request expired

maxmoehl avatar Jul 13 '22 15:07 maxmoehl

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.

maxmoehl avatar Jul 14 '22 07:07 maxmoehl

Thanks for the review!

  1. Will check that, but if I remember correctly it returns a 502 in that case
  2. Didn't catch that, will update this section as well
  3. Yes, that should be possible, will do
  4. It doesn't, there are no cases for sanitizeError

I will try to prepare a commit for the open points today.

maxmoehl avatar Jul 27 '22 06:07 maxmoehl

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?

geofffranks avatar Sep 02 '22 15:09 geofffranks

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.

maxmoehl avatar Nov 10 '22 10:11 maxmoehl