Fix error codes for http2, x509 and tls errors #2025

mstoykov · 2021-05-15T11:22:51Z

It seems http2, x509 and tls errors are not pointer errors, after all.

I mostly found this through the fact that even after the last patches it
still didn't match them, but now there are finally tests for all of
them, so hopefully this will be a permanent fix.

codecov-commenter · 2021-05-15T11:31:45Z

Codecov Report

Merging #2025 (8de4709) into master (a527715) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 8de4709 differs from pull request most recent head e9f2385. Consider uploading reports for the commit e9f2385 to get more accurate results

@@            Coverage Diff             @@
##           master    #2025      +/-   ##
==========================================
- Coverage   71.73%   71.72%   -0.02%     
==========================================
  Files         179      179              
  Lines       14086    14095       +9     
==========================================
+ Hits        10105    10109       +4     
- Misses       3345     3347       +2     
- Partials      636      639       +3

Flag	Coverage Δ
ubuntu	`71.67% <100.00%> (+0.01%)`	⬆️
windows	`71.34% <88.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/ui.go	`20.61% <ø> (ø)`
lib/options.go	`83.78% <ø> (ø)`
js/runner.go	`81.86% <100.00%> (+0.10%)`	⬆️
lib/netext/httpext/error_codes.go	`97.26% <100.00%> (-1.37%)`	⬇️
lib/netext/httpext/request.go	`94.41% <100.00%> (+0.09%)`	⬆️
lib/netext/httpext/transport.go	`97.32% <100.00%> (+0.09%)`	⬆️
lib/testutils/httpmultibin/httpmultibin.go	`90.00% <100.00%> (ø)`
lib/netext/httpext/compression.go	`84.37% <0.00%> (-2.09%)`	⬇️
output/influxdb/config.go	`41.81% <0.00%> (-1.82%)`	⬇️

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 a527715...e9f2385. Read the comment docs.

imiric

I'm concerned that we're replicating a scenario in tests that matches our expectations but doesn't reproduce what happens in production. Could we add integration tests for this to ensure it's fixed?

imiric · 2021-05-17T09:33:46Z

lib/netext/httpext/error_codes_test.go

+	require.Error(t, err)
+
+	code, msg := errorCodeForError(err)
+	assert.Equal(t, unknownHTTP2StreamErrorCode+errCode(http2.ErrCodeInternal)+1, code)


This summing of error codes seems very brittle, and I just saw we do the same in http2ErrorCodeOffset(). :-/ Why is this done instead of having a specific error code we can assert on?

there are a bunch of error codes defined in the http2 specification and some of them can be returned for stream, some for connection errors so it was easier to just have 20 codes between those and make the codes on the spot instead of defining like 14 error codes per http2*Error

OK, I guess it's fine since they're constants, but it's very confusing to read. Maybe some comments would help.

lib/testutils/httpmultibin/httpmultibin.go

mstoykov · 2021-05-17T10:05:24Z

Could we add integration tests for this to ensure it's fixed?

By integration tests do you mean using the js http module? We could, but IMO it makes very little sense. What I've added here are 3(1 http2+ 2 x509) tests for the things I could reproduce and moving it to the http module will not help with that. I would argue these are integration tests even if not between the http module and the error codes code, and the problem with adding more is that I will need to find a way to reproduce this error, which has always been the problem.

We can move them (or add additional) tests in http module but we will probably need to make the error_codes exported and in another package which is already kind of planned for #2024, but I would argue the more important part is having ways to reproduce the errors

mstoykov · 2021-05-18T08:55:32Z

lib/netext/httpext/error_codes.go

 		return x509HostnameErrorCode, x509HostnameErrorCodeMsg
-	case *tls.RecordHeaderError:
+	case tls.RecordHeaderError:
 		return defaultTLSErrorCode, err.Error()


I do think we should probably add TLSHeaderErrorCode ...

Given that defaultTLSErrorCode doesn't seem to be used for anything else, I am not sure it's necessary 🤷‍♂️

well this is also the only tls error I am matching ... as it is the only one I get, but arguably it isn't the only one that can be so it taking the "default" one seems like a bad idea ... ;)

imiric

LGTM, nice work on the tests! 👏

lib/testutils/httpmultibin/httpmultibin.go

imiric · 2021-05-19T10:30:58Z

lib/testutils/httpmultibin/httpmultibin.go

+	for _, opt := range opts {
+		switch opt.Key {
+		case NullOpt:
+			t.Fatalf("unsupported multibin option %d, the NullOpt should not be used", opt.Key)


What's the purpose of defining/checking for it then?

Literally a linter told me I don't check all of them and it is nice to have a more specific message for why it failed. On the other hand, maybe I should add that this will happen also if Key wasn't set as it will default to 0 🤔

OK, but I mean why even define NullOpt at all then? It's not used anywhere, and you could start ConnContextFuncOpt at iota + 1 if you don't want to use 0. Though it's a minor thing, I don't mind it much, just wondering.

hm .... I guess given that this is not user-facing code it might be easier if I just drop it 🤔 in general I prefer to have it for user-facing enums so we can see when it isn't set and provide an error.

lib/netext/httpext/request_test.go

na--

I don't think the whole MultiBinOptWrap is necessary. If you need custom low-level options in a certain test, simply spin up a custom httptest.NewUnstartedServer(), it's only a few lines of code. If you need it in multiple tests, write another small helper around it, instead of making HTTPMultiBin into more of a monster than it already is... We risk it becoming sentient and eating us soon 😅

mstoykov · 2021-05-20T08:49:26Z

I don't think the whole MultiBinOptWrap is necessary. If you need custom low-level options in a certain test, simply spin up a custom httptest.NewUnstartedServer(), it's only a few lines of code. If you need it in multiple tests, write another small helper around it, instead of making HTTPMultiBin into more of a monster than it already is... We risk it becoming sentient and eating us soon sweat_smile

I am fine by this ... but in that case I don't think we have more than a few tests that actually use more then what httptest.NewUnstartedServer can provide in "few lines"(in my case the lines were close to 20 before the lines in the actual tests as I need tls configuration).

na-- · 2021-05-20T09:44:10Z

I'd still very much prefer a new 20-line helper that allows for the modification of the low-level options than cramming that in HTTPMultiBin.

We said we want to split HTTPMultiBin apart. That starts by not cramming functionality that will only be used for very few tests into it...

mstoykov · 2021-05-20T13:03:15Z

        	Error Trace:	error_codes_test.go:306
        	Error:      	Not equal: 
        	            	expected: "read tcp 127.0.0.1:51368->127.0.0.1:51367: wsarecv: An existing connection was forcibly closed by the remote host."
        	            	actual  : "read: connection reset by peer"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-read tcp 127.0.0.1:51368->127.0.0.1:51367: wsarecv: An existing connection was forcibly closed by the remote host.
        	            	+read: connection reset by peer
        	Test:       	TestDefaultTLSError

this happens ever 1 out of 3-4 runs :( ... but I don't know with what I could've broken it

imiric

LGTM, nice job fixing that test 🎉

It seems http2, x509 and tls errors are not pointer errors, after all. I mostly found this through the fact that even after the last patches it still didn't match them, but now there are finally tests for all of them, so hopefully this will be a permanent fix.

This was dropped as part of #2008 but it will be better if we differentiate between different timeouts Also add test for the `request timeout` error code in `httpext`

mstoykov added this to the v0.33.0 milestone May 15, 2021

mstoykov requested review from na-- and imiric May 15, 2021 11:23

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch 2 times, most recently from e802dff to bcf6fb9 Compare May 15, 2021 11:26

mstoykov changed the title ~~Hopefully fix http2 error codes~~ It seems http2 errors are not pointer errors May 15, 2021

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch 2 times, most recently from 25c0d3a to 9972481 Compare May 15, 2021 16:54

na-- previously approved these changes May 17, 2021

View reviewed changes

mstoykov dismissed na--’s stale review via a3866d4 May 17, 2021 09:27

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch from 9972481 to a3866d4 Compare May 17, 2021 09:27

imiric reviewed May 17, 2021

View reviewed changes

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch from a3866d4 to 75a839d Compare May 17, 2021 09:41

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch 2 times, most recently from a7d0837 to 22023f4 Compare May 18, 2021 08:51

mstoykov changed the title ~~It seems http2 errors are not pointer errors~~ Fix error codes for http2, x509 and tls errors May 18, 2021

mstoykov commented May 18, 2021

View reviewed changes

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch 4 times, most recently from 68ff9c7 to 398b6f7 Compare May 18, 2021 14:59

mstoykov requested review from imiric and na-- May 19, 2021 08:41

imiric previously approved these changes May 19, 2021

View reviewed changes

mstoykov dismissed imiric’s stale review via 3d34f68 May 19, 2021 12:39

imiric previously approved these changes May 19, 2021

View reviewed changes

mstoykov dismissed imiric’s stale review via f1881ec May 19, 2021 16:06

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch from 3d34f68 to f1881ec Compare May 19, 2021 16:06

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch 2 times, most recently from 2a51fc7 to 202f0e7 Compare May 20, 2021 08:01

na-- reviewed May 20, 2021

View reviewed changes

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

na-- requested changes May 20, 2021

View reviewed changes

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch from 202f0e7 to df1929f Compare May 20, 2021 08:43

Drop httpmultibin dependency on httpext

5e40c92

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch from 1e93136 to d1654fe Compare May 20, 2021 12:31

mstoykov requested review from na-- and imiric May 20, 2021 12:44

na-- previously approved these changes May 20, 2021

View reviewed changes

mstoykov dismissed na--’s stale review via 3a28142 May 20, 2021 15:47

imiric previously approved these changes May 21, 2021

View reviewed changes

na-- previously approved these changes May 21, 2021

View reviewed changes

mstoykov added 5 commits May 21, 2021 13:55

Readd dial timeout error code and a test

01a5239

This was dropped as part of #2008 but it will be better if we differentiate between different timeouts Also add test for the `request timeout` error code in `httpext`

Use t.Parallel across tests in httpext

17e73a9

Add tlsHeaderErrorCode

ee7680d

Return request timeout error code when the reading of the body timeouts

09862f2

mstoykov dismissed stale reviews from na-- and imiric via 09862f2 May 21, 2021 10:55

mstoykov force-pushed the hopefullyFixHttp2ErrorCodes branch from e9f2385 to 09862f2 Compare May 21, 2021 10:55

na-- approved these changes May 21, 2021

View reviewed changes

imiric approved these changes May 21, 2021

View reviewed changes

mstoykov merged commit f01c2d2 into master May 21, 2021

mstoykov deleted the hopefullyFixHttp2ErrorCodes branch May 21, 2021 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error codes for http2, x509 and tls errors #2025

Fix error codes for http2, x509 and tls errors #2025

mstoykov commented May 15, 2021 •

edited

Loading

codecov-commenter commented May 15, 2021 •

edited

Loading

imiric left a comment

imiric May 17, 2021

mstoykov May 17, 2021

imiric May 17, 2021

mstoykov commented May 17, 2021

mstoykov May 18, 2021

na-- May 20, 2021

mstoykov May 20, 2021

imiric left a comment

imiric May 19, 2021

mstoykov May 19, 2021

imiric May 19, 2021

mstoykov May 19, 2021

na-- left a comment

mstoykov commented May 20, 2021

na-- commented May 20, 2021

mstoykov commented May 20, 2021

imiric left a comment

Fix error codes for http2, x509 and tls errors #2025

Fix error codes for http2, x509 and tls errors #2025

Conversation

mstoykov commented May 15, 2021 • edited Loading

codecov-commenter commented May 15, 2021 • edited Loading

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

mstoykov commented May 20, 2021

na-- commented May 20, 2021

mstoykov commented May 20, 2021

imiric left a comment

Choose a reason for hiding this comment

mstoykov commented May 15, 2021 •

edited

Loading

codecov-commenter commented May 15, 2021 •

edited

Loading