cloud_controller_ng icon indicating copy to clipboard operation
cloud_controller_ng copied to clipboard

Use V3ErrorHasher in LoggingContextJob when originating in a v3 endpoint

Open philippthun opened this issue 4 years ago • 2 comments

The LoggingContextJob does not use the correct ErrorHasher based on the endpoint (i.e. v2 vs v3) of the originating request. This change makes the api version available to the LoggingContextJob (via Enqueuer) by setting it in the middleware (similar to the VCAP-Request-ID). No dedicated middleware was introduced; instead it has been added to the existing VcapRequestId middleware as it can be removed when v2 is discontinued.

Besides using the api version from the current thread in the LoggingContextJob to decide whether V2ErrorHasher (default) or V3ErrorHasher should be used, the same is done in other places where this differentiation is needed (i.e. SecurityContextSetter, RateLimiter).

When checking for a 'compound_error?' the BaseErrorHasher uses the instance type rather than duck-typing as the V3ErrorHasher can only handle CompoundErrors correctly, i.e. it expects 'underlying_errors' to be ApiErrors with fields message, name and code.

This PR has been 'extracted' from #2537 for an easier review.

  • [x] I have reviewed the contributing guide

  • [x] I have viewed, signed, and 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 bundle exec rake

  • [ ] I have run CF Acceptance Tests

philippthun avatar Sep 20 '21 15:09 philippthun

Thanks for submitting this PR. We are having a hard time understanding the repercussions this PR, so we are currently working to understand #2537. Theres a lot going on there so it may take us a few days before we have a clear read.

MerricdeLauney avatar Dec 09 '21 00:12 MerricdeLauney

Thanks for looking into this PR, @MerricdeLauney. As you already noticed, PR #2537 is the one addressing concrete issues (e.g. #2504). My journey began with adding a test reproducing this issue (here is a Gist containing a Git patch file with only this test that can be applied to main). This test fails with:

expected "---\nerror_code: UnknownError\ndescription: An unknown error occurred.\ncode: 10001\n" to match /title:\s+CF-UnprocessableEntity/
Diff:
@@ -1,4 +1,7 @@
-/title:\s+CF-UnprocessableEntity/
+---
+error_code: UnknownError
+description: An unknown error occurred.
+code: 10001

You can see that enqueuer_spec.rb contains additional tests as I found more logs on our foundations showing the "UnknownError".

So starting with some tests I came to the solution outlined in PR #2537. And as this PR became quite big, I looked at what portions could be extracted and thought that the mechanism to store the api version in the context would be a good candidate. Somehow passing the api version to the LoggingContextJob seems to me like a reasonable approach to ensure that the correct ErrorHasher is used.

philippthun avatar Dec 09 '21 10:12 philippthun

This is a very old PR not worked on anymore, thus I'm going to close it.

philippthun avatar Oct 11 '23 15:10 philippthun