http: adding response code details for downstream HTTP/1.1 codec errors #9286
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
fe9c655 to
f7b3c4f
Compare
| struct Http1ResponseCodeDetailValues { | ||
| const std::string TooManyHeaders = "http1.too_many_headers"; | ||
| const std::string HeadersTooLarge = "http1.headers_too_large"; | ||
| const std::string HttpCodecError = "http1.codec_error"; | ||
| const std::string InvalidCharacters = "http1.invalid_characters"; | ||
| const std::string ConnectionHeaderSanitization = "http1.connection_header_bad"; | ||
| const std::string InvalidUrl = "http1.invalid_url"; | ||
| }; | ||
|
|
||
| using Http1ResponseCodeDetails = ConstSingleton<Http1ResponseCodeDetailValues>; |
There was a problem hiding this comment.
These can all be absl::string_view instead. They're always passed to functions expecting string_view.
Also, honestly since there's no initialization order fiasco here involved.
Why not just declare all those literals as constexpr auto var = "..."?
Then you wouldn't need any allocations in the singleton and no std::string allocations either (even though many of the values here qualify for SSO)
| @@ -367,6 +367,10 @@ void ConnectionManagerImpl::resetAllStreams( | |||
| stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view()); | |||
| if (response_flag.has_value()) { | |||
| stream.stream_info_.setResponseFlag(response_flag.value()); | |||
| if (response_flag.value() == StreamInfo::ResponseFlag::DownstreamProtocolError) { | |||
There was a problem hiding this comment.
micro optimization: you already checked that is has_value() above. You can use the value directly *response_flag and save a branch.
| @@ -1349,6 +1349,7 @@ TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejected) { | |||
| // the 60th 1kb header should induce overflow | |||
| buffer = Buffer::OwnedImpl(fmt::format("big: {}\r\n", long_string)); | |||
| EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit"); | |||
| EXPECT_EQ("http1.headers_too_large", response_encoder->getStream().responseDetails()); | |||
There was a problem hiding this comment.
Can we get tests that cover each of the new details?
| /* | ||
| * @return string_view optionally return the reason behind codec level errors. | ||
| */ | ||
| virtual absl::string_view responseDetails() { return ""; } |
There was a problem hiding this comment.
Instead of returning this error this way, WDYT about just throwing this out with the CodecProtocolException? I think this would lead to clearer logic since in the HCM it can just be passed into the code that resets all of the streams?
There was a problem hiding this comment.
So I started with that, but it didn't work the way I wanted to, because if one H2 stream has bad headers, I only wanted that particular stream to be tagged with bad headers - the others would maybe eventually get flagged with "you shared a connection with a bad stream" but shouldn't be tagged with the error that wasn't theirs. Only the codec knows which stream causes the problem so it has to be on a per-stream basis to be tagged correctly.
There was a problem hiding this comment.
OK that makes sense. Can you leave a comment trail about that thought process?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…rs (envoyproxy#9286) Also adding a fix to Connect header rejection where the codec sent a protocol error without throwing an exception. Risk Level: Medium (minimal tweaks to data plane code) Testing: new unit tests, integration test Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Prakhar <prakhar_au@yahoo.com>
Also adding a fix to Connect header rejection where the codec sent a protocol error without throwing an exception.
Risk Level: Medium (minimal tweaks to data plane code)
Testing: new unit tests, integration test
Docs Changes: n/a
Release Notes: n/a