Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed exception in getResponseStatus() #13314

Merged
merged 13 commits into from
Oct 28, 2020
Merged

Removed exception in getResponseStatus() #13314

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6c1b18e
Removed exception in getResponseStatus()
zasweq Sep 29, 2020
5d54f1e
Responded to Alex's comments
zasweq Sep 30, 2020
b4564b2
Added crashing integration test and responded to Asra's comments
zasweq Sep 30, 2020
1d723cf
Changed back to exception. Added integration test for overflowing int…
zasweq Oct 1, 2020
0900801
Save progress
zasweq Oct 6, 2020
582a966
Dump of work
zasweq Oct 19, 2020
dcfddd3
Removed protocol integration test
zasweq Oct 19, 2020
5f345b3
Added integration test for overflowing status in h2
zasweq Oct 20, 2020
f34ac08
Save progress
zasweq Oct 23, 2020
5d49ae6
Merge branch 'master' into http-util-remove-exceptions
zasweq Oct 23, 2020
0e260cd
Overflowing and missing status added to protocol integration
zasweq Oct 23, 2020
ebd0f1a
Merge branch 'master' into http-util-remove-exceptions
zasweq Oct 26, 2020
8f2e640
Switched from exception to ASSERT
zasweq Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
"//source/common/common:enum_to_int",
"//source/common/common:statusor_lib",
"//source/common/common:utility_lib",
"//source/common/grpc:status_lib",
"//source/common/json:json_loader_lib",
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/http1/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,8 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e

// The contract is that client codecs must ensure that :status is present.
ASSERT(headers.Status() != nullptr);
// The contract is that client codecs will ensure that the :status value is valid. This will crash
// otherwise.
uint64_t numeric_status = Utility::getResponseStatus(headers);

if (connection_.protocol() == Protocol::Http10 && connection_.supportsHttp10()) {
Expand Down
17 changes: 12 additions & 5 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,14 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() {
readDisable(false);
}

void ConnectionImpl::ClientStreamImpl::decodeHeaders() {
Status ConnectionImpl::ClientStreamImpl::decodeHeaders() {
auto& headers = absl::get<ResponseHeaderMapPtr>(headers_or_trailers_);
const uint64_t status = Http::Utility::getResponseStatus(*headers);

auto response_status_or_absl_status = Http::Utility::getResponseStatusOr(*headers);
if (!response_status_or_absl_status.ok()) {
return codecClientError(response_status_or_absl_status.status().message());
}
const uint64_t status = response_status_or_absl_status.value();

if (!upgrade_type_.empty() && headers->Status()) {
Http::Utility::transformUpgradeResponseFromH2toH1(*headers, upgrade_type_);
Expand All @@ -315,19 +320,21 @@ void ConnectionImpl::ClientStreamImpl::decodeHeaders() {
} else {
response_decoder_.decodeHeaders(std::move(headers), remote_end_stream_);
}
return okStatus();
}

void ConnectionImpl::ClientStreamImpl::decodeTrailers() {
response_decoder_.decodeTrailers(
std::move(absl::get<ResponseTrailerMapPtr>(headers_or_trailers_)));
}

void ConnectionImpl::ServerStreamImpl::decodeHeaders() {
Status ConnectionImpl::ServerStreamImpl::decodeHeaders() {
auto& headers = absl::get<RequestHeaderMapPtr>(headers_or_trailers_);
if (Http::Utility::isH2UpgradeRequest(*headers)) {
Http::Utility::transformUpgradeRequestFromH2toH1(*headers);
}
request_decoder_->decodeHeaders(std::move(headers), remote_end_stream_);
return okStatus();
}

void ConnectionImpl::ServerStreamImpl::decodeTrailers() {
Expand Down Expand Up @@ -783,7 +790,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
switch (frame->headers.cat) {
case NGHTTP2_HCAT_RESPONSE:
case NGHTTP2_HCAT_REQUEST: {
stream->decodeHeaders();
RETURN_IF_ERROR(stream->decodeHeaders());
break;
}

Expand All @@ -797,7 +804,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
stream->decodeTrailers();
} else {
// We're a client session and still waiting for non-informational headers.
stream->decodeHeaders();
RETURN_IF_ERROR(stream->decodeHeaders());
}
}
break;
Expand Down
6 changes: 3 additions & 3 deletions source/common/http/http2/codec_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log

// Does any necessary WebSocket/Upgrade conversion, then passes the headers
// to the decoder_.
virtual void decodeHeaders() PURE;
virtual Status decodeHeaders() PURE;
virtual void decodeTrailers() PURE;

// Get MetadataEncoder for this stream.
Expand Down Expand Up @@ -318,7 +318,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
void submitHeaders(const std::vector<nghttp2_nv>& final_headers,
nghttp2_data_provider* provider) override;
StreamDecoder& decoder() override { return response_decoder_; }
void decodeHeaders() override;
Status decodeHeaders() override;
void decodeTrailers() override;
HeaderMap& headers() override {
if (absl::holds_alternative<ResponseHeaderMapPtr>(headers_or_trailers_)) {
Expand Down Expand Up @@ -368,7 +368,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
void submitHeaders(const std::vector<nghttp2_nv>& final_headers,
nghttp2_data_provider* provider) override;
StreamDecoder& decoder() override { return *request_decoder_; }
void decodeHeaders() override;
Status decodeHeaders() override;
void decodeTrailers() override;
HeaderMap& headers() override {
if (absl::holds_alternative<RequestHeaderMapPtr>(headers_or_trailers_)) {
Expand Down
11 changes: 10 additions & 1 deletion source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "absl/strings/str_cat.h"
#include "absl/strings/str_split.h"
#include "absl/strings/string_view.h"
#include "absl/types/optional.h"
#include "nghttp2/nghttp2.h"

namespace Envoy {
Expand Down Expand Up @@ -369,10 +370,18 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
}

uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) {
absl::StatusOr<uint64_t> response_status_or_absl_status = getResponseStatusOr(headers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case where this function can fail parsing the Status header now that there are tests that verify that invalid Status header is rejected by both H/1 and H/2 codecs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but you don't think it's worth removing exceptions (removing meaning converting to error earlier instead of in try catch block) for any future iterations, considering if we keep the exception then it'll still be there in the long run, even though currently not triggerable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is totally worth removing this exception. We have proven with the test that the code does the right thing and does not accept responses with invalid Status header. Given this I think we should just assert that the parsing of the status number was successful. I think we could use ENVOY_BUG or event RELEASE_ASSERT here, given that we have solid test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Yan, I've been thinking about this some, and I think that the problem is that getResponseStatus() is called all around the codebase, and the problem is that if we make it an assert rather than an exception for other parts of the codebase, it would introduce a point of failure out of nothing? Let me know what you're thinking :)

if (!response_status_or_absl_status.ok()) {
throw CodecClientException(response_status_or_absl_status.status().message().data());
}
return response_status_or_absl_status.value();
}

absl::StatusOr<uint64_t> Utility::getResponseStatusOr(const ResponseHeaderMap& headers) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this method is only called from the ConnectionImpl::ClientStreamImpl::decodeHeaders(). Given that the H/2 codec verifies validity of the Status header, the only case where this can fail is if the Status header is missing altogether.
I think we should just move the check for Status header presence into the ConnectionImpl::ClientStreamImpl::decodeHeaders() and here just ASSERT success of the absl::SimpleAtoi with the comment that invalid Status values should not get past response parsing in codec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I liked how this got rid of exceptions, but your suggestion is a much cleaner way to remove the exceptions. Take the two things in the if and move one thing (simpleAtoi() call) to an assert and another (status presence) to codebase. However, I do believe the codecs already account for missing status headers similarly to overflowing status, so why even add a check there? @asraa what are your thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quickly glanced around codebase, couldn't seem to find a missing status integration test? If not there (perhaps ctrl f in GitHub missed it :)) I can add one!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of my own interest I wrote an http2 frame method for header frames without statuses breaking the HTTP/2 requirement and wrote an integration test for it. I got "[2020-10-22 05:28:34.375][31][trace][http2] [source/common/http/http2/codec_impl.cc:715] [C2] about to recv frame type=1, flags=5
[2020-10-22 05:28:34.375][31][debug][http2] [source/common/http/http2/codec_impl.cc:890] [C2] invalid frame: Violation in HTTP messaging rule on stream 1". Thus, this is similar to overflowing statuses. Let me know if you want me to check that test in too, because it would potentially hit this line and if the codec didn't validate status presence which is what the test checked for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Yan, I discussed this with Asra in our standup. Asra wants to keep it consistent, and is indifferent to whether we ASSERT or have a check, however wants to keep it consistent, and not an assert on atoi and a check for headers, because that would be unneeded complexity by splitting them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand. I think given that we also verify that missing status from the response headers is rejected, I think we could either ENVOY_BUG or RELEASE_ASSERT that status header is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So release assert on both conditions in the utility function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Both" meaning missing status and overflowed status

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - so just to make sure - what you want is to keep the function as is, but instead of throwing up an exception with the if clause, wrap those two booleans (headers present, atoi call okay) in a RELASE_ASSERT. Basically, since we have validated that across both codecs the codecs handle it, we can move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Keep the function as is" meaning what is currently checked into master for getResponseStatus? Both utility function and not having my error handling in codecs.

const HeaderEntry* header = headers.Status();
uint64_t response_code;
if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) {
throw CodecClientException(":status must be specified and a valid unsigned long");
return absl::InvalidArgumentError(":status must be specified and a valid unsigned long");
}
return response_code;
}
Expand Down
6 changes: 5 additions & 1 deletion source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "envoy/http/metadata_interface.h"
#include "envoy/http/query_params.h"

#include "common/common/statusor.h"
#include "common/http/exception.h"
#include "common/http/status.h"
#include "common/json/json_loader.h"
Expand Down Expand Up @@ -238,10 +239,13 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value,
/**
* Get the response status from the response headers.
* @param headers supplies the headers to get the status from.
* @return uint64_t the response code or throws an exception if the headers are invalid.
* @return uint64_t the response code and release asserts if the
* headers are invalid.
*/
uint64_t getResponseStatus(const ResponseHeaderMap& headers);

absl::StatusOr<uint64_t> getResponseStatusOr(const ResponseHeaderMap& headers);

/**
* Determine whether these headers are a valid Upgrade request or response.
* This function returns true if the following HTTP headers and values are present:
Expand Down
27 changes: 27 additions & 0 deletions test/common/http/http2/http2_frame.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,33 @@ Http2Frame Http2Frame::makeEmptyHeadersFrame(uint32_t stream_index, HeadersFlags
return frame;
}

Http2Frame Http2Frame::makeHeadersFrameWithStatus(std::string status, uint32_t stream_index) {
Http2Frame frame;
frame.buildHeader(
Type::Headers, 0,
orFlags(HeadersFlags::EndStream,
HeadersFlags::EndHeaders), // TODO: Support not hardcoding these two flags
makeRequestStreamId(stream_index));
if (status == "200") {
frame.appendStaticHeader(StaticHeaderIndex::Status200);
} else if (status == "204") {
frame.appendStaticHeader(StaticHeaderIndex::Status204);
} else if (status == "206") {
frame.appendStaticHeader(StaticHeaderIndex::Status206);
} else if (status == "304") {
frame.appendStaticHeader(StaticHeaderIndex::Status304);
} else if (status == "400") {
frame.appendStaticHeader(StaticHeaderIndex::Status400);
} else if (status == "500") {
frame.appendStaticHeader(StaticHeaderIndex::Status500);
} else { // Not a static header
Header statusHeader = Header(":status", status);
frame.appendHeaderWithoutIndexing(statusHeader);
}
frame.adjustPayloadSize();
return frame;
}

Http2Frame Http2Frame::makeEmptyContinuationFrame(uint32_t stream_index, HeadersFlags flags) {
Http2Frame frame;
frame.buildHeader(Type::Continuation, 0, static_cast<uint8_t>(flags),
Expand Down
9 changes: 9 additions & 0 deletions test/common/http/http2/http2_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ class Http2Frame {
MethodPost = 3,
Path = 4,
Status200 = 8,
Status204 = 9,
Status206 = 10,
Status304 = 11,
Status400 = 12,
Status404 = 13,
Status500 = 14,
SchemeHttps = 7,
Host = 38,
};
Expand Down Expand Up @@ -104,6 +109,10 @@ class Http2Frame {
static Http2Frame makeEmptySettingsFrame(SettingsFlags flags = SettingsFlags::None);
static Http2Frame makeEmptyHeadersFrame(uint32_t stream_index,
HeadersFlags flags = HeadersFlags::None);
static Http2Frame makeHeadersFrameWithStatus(
std::string status,
uint32_t stream_index); // Want to test overridden int here, so make it string
// TODO: MakeHeadersFrameWithStatusAndNonStaticHeaders
static Http2Frame makeEmptyContinuationFrame(uint32_t stream_index,
HeadersFlags flags = HeadersFlags::None);
static Http2Frame makeEmptyDataFrame(uint32_t stream_index, DataFlags flags = DataFlags::None);
Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ envoy_cc_test(
"//source/common/http:header_map_lib",
"//source/extensions/filters/http/buffer:config",
"//source/extensions/filters/http/health_check:config",
"//test/common/http/http2:http2_frame",
"//test/integration/filters:continue_headers_only_inject_body",
"//test/integration/filters:encoder_decoder_buffer_filter_lib",
"//test/integration/filters:random_pause_filter_lib",
Expand Down
45 changes: 45 additions & 0 deletions test/integration/http2_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1611,6 +1611,51 @@ TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) {
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers"));
}

TEST_P(Http2FrameIntegrationTest, OverflowingResponseCode) {
setDownstreamProtocol(Http::CodecClient::Type::HTTP2);
setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
// set lower outbound frame limits to make tests run faster
config_helper_.setOutboundFramesLimits(1000, 100);
initialize();
auto options = std::make_shared<Network::Socket::Options>();
tcp_client_ = makeTcpConnection(lookupPort("http"), options);
FakeRawConnectionPtr fake_upstream_connection;

// setup preamble and setting
ASSERT_TRUE(tcp_client_->write(Http2Frame::Preamble, false, false));
Http2Frame::SettingsFlags settings_flags = static_cast<Http2Frame::SettingsFlags>(0);
Http2Frame setting_frame = Http2Frame::makeEmptySettingsFrame(settings_flags);
ASSERT_TRUE(tcp_client_->write(std::string(setting_frame), false, false)); // empty settings
// Ack settings
settings_flags = static_cast<Http2Frame::SettingsFlags>(1); // ack
Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags);
ASSERT_TRUE(tcp_client_->write(std::string(ack_frame), false, false)); // ack setting
Http2Frame request_frame = Http2Frame::makeRequest(1, "host", "/");
ASSERT_TRUE(tcp_client_->write(std::string(request_frame), false, false));
// setup upstream and settings etc.
ASSERT(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection,
std::chrono::milliseconds(10)));
// Read Preamble
EXPECT_TRUE(fake_upstream_connection->waitForData(
FakeRawConnection::waitForInexactMatch(Http2Frame::Preamble)));
ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings
ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting
// Read settings
EXPECT_TRUE(fake_upstream_connection->waitForData(
FakeRawConnection::waitForInexactMatch(std::string(setting_frame).c_str())));
// Read Ack
EXPECT_TRUE(fake_upstream_connection->waitForData(
FakeRawConnection::waitForInexactMatch(std::string(ack_frame).c_str())));
// Read request frame
EXPECT_TRUE(fake_upstream_connection->waitForData(
FakeRawConnection::waitForInexactMatch(std::string(request_frame).c_str())));
Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus(
"11111111111111111111111111111111111111111111111111111111111111111", 0);
ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status)));

tcp_client_->close();
}

INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);
Expand Down
15 changes: 15 additions & 0 deletions test/integration/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,21 @@ TEST_P(IntegrationTest, TestClientAllowChunkedLength) {
tcp_client->close();
}

TEST_P(IntegrationTest, OverflowingResponseCode) {
initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);

FakeRawConnectionPtr fake_upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
ASSERT(fake_upstream_connection != nullptr);
ASSERT_TRUE(fake_upstream_connection->write(
"HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n", false));
ASSERT_TRUE(fake_upstream_connection->close());
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
}

TEST_P(IntegrationTest, BadFirstline) {
initialize();
std::string response;
Expand Down
Loading