HTTP2: add error details on some NGHTTP2 errors.#10006
HTTP2: add error details on some NGHTTP2 errors.#10006mattklein123 merged 1 commit intoenvoyproxy:masterfrom
Conversation
alyssawilk
left a comment
There was a problem hiding this comment.
Thanks for tackling this! I'm doing an early drive by because Yan asked me to look at the reset logic - will do a full pass later on.
2f134ef to
cd14b5a
Compare
|
@matteof-google please avoid force pushing as it hides previous review comments. |
alyssawilk
left a comment
There was a problem hiding this comment.
Looking good! Just a few more minor comments on my end
There was a problem hiding this comment.
s/make/Make/
s/DETAILS/details
But given at the end of the day these can basically be static strings, can we switch over to the ConstSingleton and change, comment lifetime requirements (or lock down access to where this is set to be non-public) and avoid the string creation?
There was a problem hiding this comment.
The strings are already static strings created via ConstSingleton. However, in comments above, @yanavlasov remarked that this pattern is risky and recommended that I switch to std::string with the implied copy operation. (I pointed out that the http1 codec has the same issue.) Are you suggesting that I revert 39a3fa75e ?
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, LGTM other than some small comments. Also you will need to fix DCO so feel free to force push (if you setup the support scripts in the repo it will help you avoid this with commit hooks).
/wait
There was a problem hiding this comment.
Do you have coverage for this case?
There was a problem hiding this comment.
The two explicit cases are hit in various places in the unit and integration tests.
I don't have a test for the default case, and I am not sure whether it can be triggered. The theory we discussed with Alyssa was to return unknown, and whoever sees unknown in future logs will dig into it and add another explicit case.
There was a problem hiding this comment.
I think that's fine, but generally there should be some kind of coverage around this, perhaps by making the translation functions static and making sure they do the right thing during targeted unit testing.
There was a problem hiding this comment.
@mattklein123 I can add the test that are you asking for, but I see little value in it, since it will look like a duplication of the switch statement, something like:
check(NGTTP2_ERR_..., "foo");
check(NGTTP2_ERR_..., "bar");
check(-1, "quux");
My inclination is to close this as WONTFIX. If you have other concerns (e.g., you are striving for 100% coverage and this code gets in the way), please let me know and I'll be happy to oblige.
There was a problem hiding this comment.
It doesn't thrill me that we have code that we are not sure whether we can ever hit, and we have no test coverage for it either. I agree though that this type of test is not super useful. Up to you.
There was a problem hiding this comment.
For perf reasons it seems like you could just stipulate/assume that the passed strings are constant, but I don't feel strongly about it. Up to you.
There was a problem hiding this comment.
Yeah, my original code agreed with you, but yanavlasov requested this change, and now both you and Alyssa seem to agree with my initial code. I'll talk to Alyssa and decide what to do.
FWIW, the http1 codec assumes that the details_ string is static, so if we decide to be paranoid, I should fix the http1 codec as well.
There was a problem hiding this comment.
I'll talk to Alyssa and decide what to do.
Alyssa is out this week. My preference us to assume they are static strings, but I agree that if you and @yanavlasov decide otherwise we should be consistent across both codecs here.
There was a problem hiding this comment.
I have pushed a patch tentatively rolling back to static strings, while I sort the issue out with @yanavlasov
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, thanks. I will defer to @yanavlasov for final approval/merge.
|
Needs DCO fix for some commits. |
Add wiring to propagate error details from the http2 codec to the access log. Such wiring inserts a DownstreamProtocolError response flag (appearing as "DPE" in the log) and a string with details of the error. Currently, this mechanism is only triggered by the onInvalidFrame callback. Before this change, DownstreamProtocolError was handled inconsistently. If stream_error_on_invalid_http_messaging, we closed the stream but did not log a DPE. Otherwise, we closed all streams and logged a DPE. The current code reports a DPE in all cases. Signed-off-by: Matteo Frigo <matteof@google.com>
23d3808
3ec17a2 to
23d3808
Compare
|
Thanks @yanavlasov . I have fixed the DCO issue and squashed the somewhat messy history into one commit. The patch should be ready for merging now. |
Add wiring to propagate error details from the http2 codec to the
access log. Such wiring inserts a DownstreamProtocolError response
flag (appearing as "DPE" in the log) and a string with details of the
error. Currently, this mechanism is only triggered by the
onInvalidFrame callback.
Before this change, DownstreamProtocolError was handled
inconsistently. If stream_error_on_invalid_http_messaging, we closed
the stream but did not log a DPE. Otherwise, we closed all streams
and logged a DPE. The current code reports a DPE in all cases.
Signed-off-by: Matteo Frigo matteof@google.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description: add error details on some NGHTTP2 errors
Risk Level: low
Testing: unit tests for specific expected errors, integration tests to verify that the access log has the expected output
Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]