Fix response error handling and slightly improve the digest auth code #1102

na-- · 2019-08-05T19:31:21Z

Previously, there was an uncaught error (sometimes leading to panics) in the automatic response body decompression. This patch handles these decompression errors and even assigns a custom error code for them. As a bonus, k6 now also properly handles errors due to improper redirects, or too many redirects, and tags the resulting metric samples accordingly.

By necessity, I had to also move the digest authentication handling to its own http.RoundTripper layer. This by no means solves the issues of #800, but it hopefully slightly improves the situation. A proper authentication cache and likely a different library still have to be used there eventually...

Previously, there was an uncaught error (sometimes leading to panics) in the automatic response body decompression. This patch handles these decompression errors and even assigns a custom error code for them. As a bonus, k6 now also properly handles errors due to improper redirects, or too many redirects, and tags the resulting metric samples accordingly. By necessity, I had to also move the digest authentication handling to its own http.RoundTripper layer. This by no means solves the issues of #800, but it hopefully slightly improves the situation. A proper authentication cache and likely a different library still have to be used there eventually...

codecov-io · 2019-08-05T19:43:52Z

Codecov Report

Merging #1102 into master will decrease coverage by <.01%.
The diff coverage is 63.21%.

@@            Coverage Diff             @@
##           master    #1102      +/-   ##
==========================================
- Coverage   73.12%   73.12%   -0.01%     
==========================================
  Files         139      141       +2     
  Lines       10230    10251      +21     
==========================================
+ Hits         7481     7496      +15     
- Misses       2308     2316       +8     
+ Partials      441      439       -2

Impacted Files	Coverage Δ
lib/netext/httpext/response.go	`87.87% <ø> (+10.95%)`	⬆️
lib/netext/httpext/httpdebug_transport.go	`0% <0%> (ø)`
lib/netext/httpext/transport.go	`96.15% <100%> (+0.1%)`	⬆️
lib/netext/httpext/request.go	`91.79% <74.35%> (+2.59%)`	⬆️
lib/netext/httpext/error_codes.go	`97.05% <75%> (-2.95%)`	⬇️
lib/netext/httpext/digest_transport.go	`80% <80%> (ø)`
core/engine.go	`93.92% <0%> (+0.93%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9f19f3...411be0c. Read the comment docs.

mstoykov

LGTM, apart from a few typos that I found ;).

I don't really like that we touch and touch this code constantly ... but it is what it is at least until we completely refactor it.

My only request is that you add a test that uses digest and has a body in order to test the GetBody part of the code. Also any more tests are always welcome although maybe we should start doing the http testing in httpext instead of js/http

lib/netext/httpext/digest_transport.go

lib/netext/httpext/error_codes.go

lib/netext/httpext/request.go

This makes the code slighly saner and fixes the bug where the first NTLM and digest authentication requests weren't displayed. But by no means does this fix all of the bugs in the http-debug code... For those, see: - #986 - #1042 - #774 A discussion how some or all of them can be fixed can be found here: #1102 (comment)

lib/netext/httpext/digest_transport.go

lib/netext/httpext/httpdebug_transport.go

mstoykov · 2019-08-06T11:40:15Z

lib/netext/httpext/httpdebug_transport.go

+)
+
+type httpDebugTransport struct {
+	//TODO: get the state and log to its Logger


I think this should be done in the debug* methods and I would really like a test for this, as it will be very bad if we break .. the http when you are debugging it for example ;)

Not sure I understand... Can you elaborate?

I don't want to waste any time in writing of tests that would be obsolete by the time we properly fix the http-debug functionality, since it's currently a pile of 💩 . Much like the digest authentication, I haven't changed anything significant from before, I've only slightly rearranged and moved the pile of 💩 to a separate place so it handles all cases and is easier to properly fix in the future. But I don't want to test something whose mode of working we plan to significantly change in the near future...

I am/was left with the impression you intend on setting the state in the struct .. while I think it is okay to always get it from the request's context.
And pointing out that a bug in this code will result in the k6/http code breaking when you try to debug it which makes it important to be tested with at least a couple of tests ;) For which being able to change the log will be useful ;)

Being a pile of shit is the exact reason why tests are good idea ... if it wasn't it would've had less reason for it.

Also I don't think the change will make the tests obsolete if anything the tests will show and document the change when it happens and will mostly do with (probably) small change in what we output and some caching ... both of which will not be significant problems , unlike if this code breaks or is broken in a way that nobody tested for.

Lastly "near" and "soon" are relative terms. If you say that this will be fixed in vX.Y.Z in two weeks .. I might be fine .. but given how it usually goes I much prefer if we have tests for things that we intend to change "soon" as "soon" is usually in few months ..

... I respectfully disagree with both points. I see no point if getting the state from the requests' context every time, since we already have it and we can just inject in the struct - much simpler, no contexts and no type asserts required.

But that point is completely moot, since I have no intentions whatsoever of dealing with any of that in this pull request, given the fact that we agreed to leave the proper fixes of the http-debug mess for another time... I won't dump whole requetss the logger, because I have no idea what issues that will bring - we'll deal with that properly when we fix the http-debug properly. This is just a preliminary step so it tracks every request, so that we don't make things even worse...

FFS, now codecov complains 🤦‍♂️

This is as far as I'm willing to invest in testing HttpDebug for now: 411be0c

mstoykov

LGTM but I think a test for the httpdebug is required ;)

This should hopefully ensure we're not accidentally breaking something with it...

mstoykov

LGTM

na-- requested a review from mstoykov August 5, 2019 19:31

mstoykov requested changes Aug 6, 2019

View reviewed changes

lib/netext/httpext/digest_transport.go Outdated Show resolved Hide resolved

lib/netext/httpext/error_codes.go Outdated Show resolved Hide resolved

lib/netext/httpext/request.go Outdated Show resolved Hide resolved

na-- added 2 commits August 6, 2019 12:52

Fix some comment typos

ec493b9

Add a test for digest auth with POST body

911c60c

na-- mentioned this pull request Aug 6, 2019

Fix error_code tag values #1104

Open

na-- added 2 commits August 6, 2019 14:01

Improve the code comments in the new helper roundtrippers

6c85afd

na-- requested a review from mstoykov August 6, 2019 11:10

golangcibot reviewed Aug 6, 2019

View reviewed changes

lib/netext/httpext/digest_transport.go Outdated Show resolved Hide resolved

lib/netext/httpext/digest_transport.go Outdated Show resolved Hide resolved

Fix some typos in comments

c668d7a

mstoykov reviewed Aug 6, 2019

View reviewed changes

lib/netext/httpext/httpdebug_transport.go Outdated Show resolved Hide resolved

mstoykov reviewed Aug 6, 2019

View reviewed changes

mstoykov requested changes Aug 6, 2019

View reviewed changes

Enable HttpDebug in a single test

411be0c

This should hopefully ensure we're not accidentally breaking something with it...

mstoykov approved these changes Aug 6, 2019

View reviewed changes

na-- merged commit 46cdbe2 into master Aug 6, 2019

na-- deleted the fix-response-error-handling branch August 6, 2019 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix response error handling and slightly improve the digest auth code #1102

Fix response error handling and slightly improve the digest auth code #1102

na-- commented Aug 5, 2019

codecov-io commented Aug 5, 2019 •

edited

Loading

mstoykov left a comment

mstoykov Aug 6, 2019 •

edited

Loading

na-- Aug 6, 2019

na-- Aug 6, 2019

mstoykov Aug 6, 2019

mstoykov Aug 6, 2019

na-- Aug 6, 2019

na-- Aug 6, 2019

na-- Aug 6, 2019

mstoykov left a comment

mstoykov left a comment

Fix response error handling and slightly improve the digest auth code #1102

Fix response error handling and slightly improve the digest auth code #1102

Conversation

na-- commented Aug 5, 2019

codecov-io commented Aug 5, 2019 • edited Loading

Codecov Report

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

na-- Aug 6, 2019

Choose a reason for hiding this comment

na-- Aug 6, 2019

Choose a reason for hiding this comment

mstoykov Aug 6, 2019

Choose a reason for hiding this comment

mstoykov Aug 6, 2019

Choose a reason for hiding this comment

na-- Aug 6, 2019

Choose a reason for hiding this comment

na-- Aug 6, 2019

Choose a reason for hiding this comment

na-- Aug 6, 2019

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 5, 2019 •

edited

Loading

mstoykov Aug 6, 2019 •

edited

Loading