From 6c1b18eb9f6f35b7a11b043ea3c3a7d8d3922e8a Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 29 Sep 2020 16:31:02 +0000 Subject: [PATCH 01/11] Removed exception in getResponseStatus() Signed-off-by: Zach --- source/common/http/BUILD | 1 + source/common/http/http1/codec_impl.cc | 2 ++ source/common/http/http2/codec_impl.cc | 17 ++++++++++++----- source/common/http/http2/codec_impl.h | 6 +++--- source/common/http/utility.cc | 16 ++++++++++++++-- source/common/http/utility.h | 3 +++ 6 files changed, 35 insertions(+), 10 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 580a30ac64bc..8307fe9e3a5b 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -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", diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index e43138c95d6c..9722ec2c4c5b 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -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); + // encodeHeaders() is not supposed to throw any exceptions. This will crash on a release assert if + // response status is invalid. uint64_t numeric_status = Utility::getResponseStatus(headers); if (connection_.protocol() == Protocol::Http10 && connection_.supportsHttp10()) { diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 7de223982beb..8fb90dd22f81 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -296,9 +296,14 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { readDisable(false); } -void ConnectionImpl::ClientStreamImpl::decodeHeaders() { +Status ConnectionImpl::ClientStreamImpl::decodeHeaders() { auto& headers = absl::get(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_); @@ -315,6 +320,7 @@ void ConnectionImpl::ClientStreamImpl::decodeHeaders() { } else { response_decoder_.decodeHeaders(std::move(headers), remote_end_stream_); } + return okStatus(); } void ConnectionImpl::ClientStreamImpl::decodeTrailers() { @@ -322,12 +328,13 @@ void ConnectionImpl::ClientStreamImpl::decodeTrailers() { std::move(absl::get(headers_or_trailers_))); } -void ConnectionImpl::ServerStreamImpl::decodeHeaders() { +Status ConnectionImpl::ServerStreamImpl::decodeHeaders() { auto& headers = absl::get(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() { @@ -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; } @@ -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; diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 489c26c8603e..a940124766a2 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -258,7 +258,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& 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(headers_or_trailers_)) { @@ -368,7 +368,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& 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(headers_or_trailers_)) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index b084726453fb..86bbfea3e8c7 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -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 { @@ -369,12 +370,23 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin } uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) { + absl::StatusOr response_status_or_absl_status = getResponseStatusOr(headers); + RELEASE_ASSERT(response_status_or_absl_status.ok(), + ":status must be specified and a valid unsigned long"); + return response_status_or_absl_status.value(); +} + +absl::StatusOr Utility::getResponseStatusOr(const ResponseHeaderMap& headers) { const HeaderEntry* header = headers.Status(); + absl::StatusOr response_status_or_absl_status; uint64_t response_code; if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) { - throw CodecClientException(":status must be specified and a valid unsigned long"); + response_status_or_absl_status = + absl::InvalidArgumentError(":status must be specified and a valid unsigned long"); + return response_status_or_absl_status; } - return response_code; + response_status_or_absl_status = response_code; + return response_status_or_absl_status; } bool Utility::isUpgrade(const RequestOrResponseHeaderMap& headers) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 936ae2f2974a..0bc6c0226fa8 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -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" @@ -242,6 +243,8 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value, */ uint64_t getResponseStatus(const ResponseHeaderMap& headers); +absl::StatusOr 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: From 5d54f1efc77a2d2b67f542e67a66cfacaf8bee3b Mon Sep 17 00:00:00 2001 From: Zach Date: Wed, 30 Sep 2020 13:31:11 +0000 Subject: [PATCH 02/11] Responded to Alex's comments Signed-off-by: Zach --- source/common/http/utility.cc | 8 ++------ source/common/http/utility.h | 3 ++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 86bbfea3e8c7..d268831432e4 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -378,15 +378,11 @@ uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) { absl::StatusOr Utility::getResponseStatusOr(const ResponseHeaderMap& headers) { const HeaderEntry* header = headers.Status(); - absl::StatusOr response_status_or_absl_status; uint64_t response_code; if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) { - response_status_or_absl_status = - absl::InvalidArgumentError(":status must be specified and a valid unsigned long"); - return response_status_or_absl_status; + return absl::InvalidArgumentError(":status must be specified and a valid unsigned long"); } - response_status_or_absl_status = response_code; - return response_status_or_absl_status; + return response_code; } bool Utility::isUpgrade(const RequestOrResponseHeaderMap& headers) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 0bc6c0226fa8..ae673433fa3f 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -239,7 +239,8 @@ 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); From b4564b2e5861f0d6998f57f65832509ff009d1d7 Mon Sep 17 00:00:00 2001 From: Zach Date: Wed, 30 Sep 2020 19:00:59 +0000 Subject: [PATCH 03/11] Added crashing integration test and responded to Asra's comments Signed-off-by: Zach --- source/common/http/http1/codec_impl.cc | 4 ++-- source/common/http/utility.cc | 2 +- test/integration/protocol_integration_test.cc | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 9722ec2c4c5b..60243f884566 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -346,8 +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); - // encodeHeaders() is not supposed to throw any exceptions. This will crash on a release assert if - // response status is invalid. + // 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()) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index d268831432e4..f59c980c4187 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -372,7 +372,7 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) { absl::StatusOr response_status_or_absl_status = getResponseStatusOr(headers); RELEASE_ASSERT(response_status_or_absl_status.ok(), - ":status must be specified and a valid unsigned long"); + response_status_or_absl_status.status().message().data()); return response_status_or_absl_status.value(); } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index da326a437e8c..4adfa7837299 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -169,6 +169,20 @@ TEST_P(ProtocolIntegrationTest, UnknownResponsecode) { EXPECT_EQ("600", response->headers().getStatusValue()); } +TEST_P(ProtocolIntegrationTest, OverflowingResponsecode) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "11111111111111111111111111111111111111111111111111111111111"}}; + auto response = sendRequestAndWaitForResponse(default_request_headers_, 0, response_headers, 0); + + ASSERT_TRUE(response->complete()); + EXPECT_EQ("11111111111111111111111111111111111111111111111111111111111", + response->headers().getStatusValue()); +} + // Add a health check filter and verify correct computation of health based on upstream status. TEST_P(ProtocolIntegrationTest, ComputedHealthCheck) { config_helper_.addFilter(R"EOF( From 1d723cf7fdfcb5f52e56c833ac9ea2cf1b5efddb Mon Sep 17 00:00:00 2001 From: Zach Date: Thu, 1 Oct 2020 16:33:05 +0000 Subject: [PATCH 04/11] Changed back to exception. Added integration test for overflowing integer response status. Signed-off-by: Zach --- source/common/http/utility.cc | 5 +++-- test/integration/integration_test.cc | 15 +++++++++++++++ test/integration/protocol_integration_test.cc | 14 -------------- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index f59c980c4187..c2ed5cbdc645 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -371,8 +371,9 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) { absl::StatusOr response_status_or_absl_status = getResponseStatusOr(headers); - RELEASE_ASSERT(response_status_or_absl_status.ok(), - response_status_or_absl_status.status().message().data()); + if (!response_status_or_absl_status.ok()) { + throw CodecClientException(response_status_or_absl_status.status().message().data()); + } return response_status_or_absl_status.value(); } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 594f5ac656c5..de72491a1ef0 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -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; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 4adfa7837299..da326a437e8c 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -169,20 +169,6 @@ TEST_P(ProtocolIntegrationTest, UnknownResponsecode) { EXPECT_EQ("600", response->headers().getStatusValue()); } -TEST_P(ProtocolIntegrationTest, OverflowingResponsecode) { - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - - Http::TestResponseHeaderMapImpl response_headers{ - {":status", "11111111111111111111111111111111111111111111111111111111111"}}; - auto response = sendRequestAndWaitForResponse(default_request_headers_, 0, response_headers, 0); - - ASSERT_TRUE(response->complete()); - EXPECT_EQ("11111111111111111111111111111111111111111111111111111111111", - response->headers().getStatusValue()); -} - // Add a health check filter and verify correct computation of health based on upstream status. TEST_P(ProtocolIntegrationTest, ComputedHealthCheck) { config_helper_.addFilter(R"EOF( From 0900801ab74dfc6e3392f672d17da317592f48d4 Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 6 Oct 2020 00:57:40 +0000 Subject: [PATCH 05/11] Save progress Signed-off-by: Zach --- test/common/http/http2/http2_frame.cc | 28 +++++++++++ test/common/http/http2/http2_frame.h | 7 +++ test/integration/BUILD | 1 + test/integration/protocol_integration_test.cc | 47 +++++++++++++++++++ 4 files changed, 83 insertions(+) diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index 142e353f28f4..7b69ac20cfe7 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -151,6 +151,34 @@ Http2Frame Http2Frame::makeEmptyHeadersFrame(uint32_t stream_index, HeadersFlags return frame; } +Http2Frame Http2Frame::makeHeadersFrameWithStatus(std::string status, uint32_t stream_index, HeadersFlags flags) { + Http2Frame frame; + frame.buildHeader(Type::Headers, 0, static_cast(flags), + makeRequestStreamId(stream_index)); + if (status == "200") { + frame.appendStaticHeader(StaticHeaderIndex::Status200); + return frame; + } else if (status == "204") { + frame.appendStaticHeader(StaticHeaderIndex::Status204); + return frame; + } else if (status == "206") { + frame.appendStaticHeader(StaticHeaderIndex::Status206); + return frame; + } else if (status == "304") { + frame.appendStaticHeader(StaticHeaderIndex::Status304); + return frame; + } else if (status == "400") { + frame.appendStaticHeader(StaticHeaderIndex::Status400); + return frame; + } else if (status == "500") { + frame.appendStaticHeader(StaticHeaderIndex::Status500); + return frame; + } + Header statusHeader = Header(":status", status); + frame.appendHeaderWithoutIndexing(statusHeader); + return frame; +} + Http2Frame Http2Frame::makeEmptyContinuationFrame(uint32_t stream_index, HeadersFlags flags) { Http2Frame frame; frame.buildHeader(Type::Continuation, 0, static_cast(flags), diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index 43225793a542..7585df66365f 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -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, }; @@ -104,6 +109,8 @@ 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, HeadersFlags flags = HeadersFlags::None); //Want to test overriden 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); diff --git a/test/integration/BUILD b/test/integration/BUILD index 1331932d2c17..d6d5444d94b4 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -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", diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index da326a437e8c..cbb6b27d0f51 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -24,6 +24,7 @@ #include "common/runtime/runtime_impl.h" #include "common/upstream/upstream_impl.h" +#include "test/common/http/http2/http2_frame.h" #include "test/common/upstream/utility.h" #include "test/integration/autonomous_upstream.h" #include "test/integration/http_integration.h" @@ -169,6 +170,52 @@ TEST_P(ProtocolIntegrationTest, UnknownResponsecode) { EXPECT_EQ("600", response->headers().getStatusValue()); } +TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { + initialize(); + + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + FakeRawConnectionPtr fake_upstream_connection; + //HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + 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()); + } /*else { + FakeRawConnectionPtr fake_upstream_connection; + //Respond with response frames on the wire here + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + ASSERT_TRUE(tcp_client->write(Http::Http2::Http2Frame::Preamble, false, false)); + + //Set up responses with required frames - Settings NONE, Settings ACK, Settings NONE, Settings ACK + Http::Http2::Http2Frame settings_none = Http::Http2::Http2Frame::makeEmptySettingsFrame(Http::Http2::Http2Frame::SettingsFlags::None); + Http::Http2::Http2Frame settings_ack = Http::Http2::Http2Frame::makeEmptySettingsFrame(Http::Http2::Http2Frame::SettingsFlags::Ack); + + ASSERT_TRUE(tcp_client->write(std::string(settings_none))); + ASSERT_TRUE(tcp_client->write(std::string(settings_ack))); + + //TODO: Request here? + Http::Http2::Http2Frame request = Http::Http2::Http2Frame::makeRequest(1, "host", "path/to/long/url"); + ASSERT_TRUE(tcp_client->write(std::string(request))); + + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); + + ASSERT_TRUE(fake_upstream_connection->write(std::string(settings_none))); + ASSERT_TRUE(fake_upstream_connection->write(std::string(settings_ack))); + + Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus("11111111111111111111111111111111111111111111111111111111111111111", 1); + + ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); + ASSERT_TRUE(fake_upstream_connection->close()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + }*/ +} + // Add a health check filter and verify correct computation of health based on upstream status. TEST_P(ProtocolIntegrationTest, ComputedHealthCheck) { config_helper_.addFilter(R"EOF( From 582a9663083ed1753880b197b19d327b50bf70f6 Mon Sep 17 00:00:00 2001 From: Zach Date: Mon, 19 Oct 2020 04:23:19 +0000 Subject: [PATCH 06/11] Dump of work Signed-off-by: Zach --- test/common/http/http2/http2_frame.cc | 21 ++++++------ test/common/http/http2/http2_frame.h | 6 ++-- test/integration/http2_integration_test.cc | 40 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index 7b69ac20cfe7..bff503db0ce4 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -151,31 +151,30 @@ Http2Frame Http2Frame::makeEmptyHeadersFrame(uint32_t stream_index, HeadersFlags return frame; } -Http2Frame Http2Frame::makeHeadersFrameWithStatus(std::string status, uint32_t stream_index, HeadersFlags flags) { +Http2Frame Http2Frame::makeHeadersFrameWithStatus(std::string status, uint32_t stream_index) { Http2Frame frame; - frame.buildHeader(Type::Headers, 0, static_cast(flags), - makeRequestStreamId(stream_index)); + 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); - return frame; } else if (status == "204") { frame.appendStaticHeader(StaticHeaderIndex::Status204); - return frame; } else if (status == "206") { frame.appendStaticHeader(StaticHeaderIndex::Status206); - return frame; } else if (status == "304") { frame.appendStaticHeader(StaticHeaderIndex::Status304); - return frame; } else if (status == "400") { frame.appendStaticHeader(StaticHeaderIndex::Status400); - return frame; } else if (status == "500") { frame.appendStaticHeader(StaticHeaderIndex::Status500); - return frame; + } else { // Not a static header + Header statusHeader = Header(":status", status); + frame.appendHeaderWithoutIndexing(statusHeader); } - Header statusHeader = Header(":status", status); - frame.appendHeaderWithoutIndexing(statusHeader); + frame.adjustPayloadSize(); return frame; } diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index 7585df66365f..a4627cc95377 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -109,8 +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, HeadersFlags flags = HeadersFlags::None); //Want to test overriden int here, so make it string - //TODO: MakeHeadersFrameWithStatusAndNonStaticHeaders + 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); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 94248743acf1..d83eefb85188 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1611,6 +1611,46 @@ 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(); + 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(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(1); // ack + Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); + ASSERT_TRUE(tcp_client_->write(std::string(ack_frame), false, false)); // ack setting + Http2Frame h2_frame = Http2Frame::makeRequest(1, "host", "/"); + ASSERT_TRUE(tcp_client_->write(std::string(h2_frame), false, false)); + // setup upstream and settings etc. + ASSERT(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection, + std::chrono::milliseconds(10))); + ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings + ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting + + // Overflowing response code + Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus( + "11111111111111111111111111111111111111111111111111111111111111111", + 1 /*, static_cast(4)*/); + + // Http::Http2::Http2Frame overflowed_status = + // Http::Http2::Http2Frame::makeHeadersFrameWithStatus("400", 1); + + 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); From dcfddd35f216bfb66b6fa7673d6b38b63126d503 Mon Sep 17 00:00:00 2001 From: Zach Date: Mon, 19 Oct 2020 04:26:02 +0000 Subject: [PATCH 07/11] Removed protocol integration test Signed-off-by: Zach --- test/integration/protocol_integration_test.cc | 226 ++---------------- 1 file changed, 26 insertions(+), 200 deletions(-) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index cbb6b27d0f51..39e5b2738e87 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -24,7 +24,6 @@ #include "common/runtime/runtime_impl.h" #include "common/upstream/upstream_impl.h" -#include "test/common/http/http2/http2_frame.h" #include "test/common/upstream/utility.h" #include "test/integration/autonomous_upstream.h" #include "test/integration/http_integration.h" @@ -155,7 +154,7 @@ TEST_P(ProtocolIntegrationTest, RouterRedirect) { ASSERT_TRUE(response->complete()); EXPECT_EQ("301", response->headers().getStatusValue()); EXPECT_EQ("https://www.redirect.com/foo", - response->headers().get(Http::Headers::get().Location)->value().getStringView()); + response->headers().get(Http::Headers::get().Location)[0]->value().getStringView()); } TEST_P(ProtocolIntegrationTest, UnknownResponsecode) { @@ -170,52 +169,6 @@ TEST_P(ProtocolIntegrationTest, UnknownResponsecode) { EXPECT_EQ("600", response->headers().getStatusValue()); } -TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { - initialize(); - - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { - FakeRawConnectionPtr fake_upstream_connection; - //HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - - 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()); - } /*else { - FakeRawConnectionPtr fake_upstream_connection; - //Respond with response frames on the wire here - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); - ASSERT_TRUE(tcp_client->write(Http::Http2::Http2Frame::Preamble, false, false)); - - //Set up responses with required frames - Settings NONE, Settings ACK, Settings NONE, Settings ACK - Http::Http2::Http2Frame settings_none = Http::Http2::Http2Frame::makeEmptySettingsFrame(Http::Http2::Http2Frame::SettingsFlags::None); - Http::Http2::Http2Frame settings_ack = Http::Http2::Http2Frame::makeEmptySettingsFrame(Http::Http2::Http2Frame::SettingsFlags::Ack); - - ASSERT_TRUE(tcp_client->write(std::string(settings_none))); - ASSERT_TRUE(tcp_client->write(std::string(settings_ack))); - - //TODO: Request here? - Http::Http2::Http2Frame request = Http::Http2::Http2Frame::makeRequest(1, "host", "path/to/long/url"); - ASSERT_TRUE(tcp_client->write(std::string(request))); - - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); - ASSERT(fake_upstream_connection != nullptr); - - ASSERT_TRUE(fake_upstream_connection->write(std::string(settings_none))); - ASSERT_TRUE(fake_upstream_connection->write(std::string(settings_ack))); - - Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus("11111111111111111111111111111111111111111111111111111111111111111", 1); - - ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); - ASSERT_TRUE(fake_upstream_connection->close()); - ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); - }*/ -} - // Add a health check filter and verify correct computation of health based on upstream status. TEST_P(ProtocolIntegrationTest, ComputedHealthCheck) { config_helper_.addFilter(R"EOF( @@ -350,7 +303,7 @@ name: add-trailers-filter if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) { EXPECT_EQ("decode", upstream_request_->trailers() - ->get(Http::LowerCaseString("grpc-message")) + ->get(Http::LowerCaseString("grpc-message"))[0] ->value() .getStringView()); } @@ -377,7 +330,7 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); EXPECT_EQ("host", - response->headers().get(Http::LowerCaseString("host"))->value().getStringView()); + response->headers().get(Http::LowerCaseString("host"))[0]->value().getStringView()); } // Regression test for https://github.com/envoyproxy/envoy/issues/10270 @@ -398,7 +351,7 @@ TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { {"longrequestvalue", long_header_value_with_inner_lws}}); waitForNextUpstreamRequest(); EXPECT_EQ(long_header_value_with_inner_lws, upstream_request_->headers() - .get(Http::LowerCaseString("longrequestvalue")) + .get(Http::LowerCaseString("longrequestvalue"))[0] ->value() .getStringView()); upstream_request_->encodeHeaders( @@ -410,10 +363,12 @@ TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); EXPECT_EQ("host", - response->headers().get(Http::LowerCaseString("host"))->value().getStringView()); - EXPECT_EQ( - long_header_value_with_inner_lws, - response->headers().get(Http::LowerCaseString("longresponsevalue"))->value().getStringView()); + response->headers().get(Http::LowerCaseString("host"))[0]->value().getStringView()); + EXPECT_EQ(long_header_value_with_inner_lws, + response->headers() + .get(Http::LowerCaseString("longresponsevalue"))[0] + ->value() + .getStringView()); } TEST_P(ProtocolIntegrationTest, Retry) { @@ -945,8 +900,8 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) { EXPECT_EQ("500", response->headers().getStatusValue()); // Regression test all sendLocalReply paths add route-requested headers. auto foo = Http::LowerCaseString("foo"); - ASSERT_TRUE(response->headers().get(foo) != nullptr); - EXPECT_EQ("bar", response->headers().get(foo)->value().getStringView()); + ASSERT_FALSE(response->headers().get(foo).empty()); + EXPECT_EQ("bar", response->headers().get(foo)[0]->value().getStringView()); // Regression test https://github.com/envoyproxy/envoy/issues/9881 by making // sure this path does standard HCM header transformations. @@ -1308,157 +1263,27 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { } } -TEST_P(DownstreamProtocolIntegrationTest, HeadersOnlyFilterEncoding) { - config_helper_.addFilter(R"EOF( -name: encode-headers-only -)EOF"); - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = - codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}}, - 128); - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, false); - response->waitForEndStream(); - EXPECT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } else { - ASSERT_TRUE(upstream_request_->waitForReset()); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } - - EXPECT_TRUE(response->complete()); - EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_EQ(0, response->body().size()); -} - -TEST_P(DownstreamProtocolIntegrationTest, HeadersOnlyFilterDecoding) { - config_helper_.addFilter(R"EOF( -name: decode-headers-only -)EOF"); - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = - codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}}, - 128); - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, false); - upstream_request_->encodeData(128, true); - response->waitForEndStream(); - - EXPECT_TRUE(response->complete()); - EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_EQ(128, response->body().size()); -} - -TEST_P(DownstreamProtocolIntegrationTest, HeadersOnlyFilterEncodingIntermediateFilters) { - config_helper_.addFilter(R"EOF( -name: passthrough-filter -)EOF"); - config_helper_.addFilter(R"EOF( -name: encode-headers-only -)EOF"); - config_helper_.addFilter(R"EOF( -name: passthrough-filter -)EOF"); - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = - codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}}, - 128); - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, false); - response->waitForEndStream(); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } else { - ASSERT_TRUE(upstream_request_->waitForReset()); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } - - EXPECT_TRUE(response->complete()); - EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_EQ(0, response->body().size()); -} - -TEST_P(DownstreamProtocolIntegrationTest, HeadersOnlyFilterDecodingIntermediateFilters) { - config_helper_.addFilter(R"EOF( -name: passthrough-filter -)EOF"); - config_helper_.addFilter(R"EOF( -name: decode-headers-only -)EOF"); - config_helper_.addFilter(R"EOF( -name: passthrough-filter -)EOF"); - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = - codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}}, - 128); - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, false); - upstream_request_->encodeData(128, true); - response->waitForEndStream(); - - EXPECT_TRUE(response->complete()); - EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_EQ(128, response->body().size()); -} - -// Verifies behavior when request data is encoded after the request has been -// turned into a headers-only request and the response has already begun. -TEST_P(DownstreamProtocolIntegrationTest, HeadersOnlyFilterInterleaved) { +TEST_P(DownstreamProtocolIntegrationTest, LocalReplyDuringEncoding) { config_helper_.addFilter(R"EOF( -name: decode-headers-only +name: local-reply-during-encode )EOF"); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - // First send the request headers. The filter should turn this into a header-only - // request. - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}); // Wait for the upstream request and begin sending a response with end_stream = false. waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, false); - - // Simulate additional data after the request has been turned into a headers only request. - Buffer::OwnedImpl data(std::string(128, 'a')); - request_encoder_->encodeData(data, false); - - // End the response. - upstream_request_->encodeData(128, true); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, true); response->waitForEndStream(); EXPECT_TRUE(response->complete()); - EXPECT_EQ("503", response->headers().getStatusValue()); + EXPECT_EQ("500", response->headers().getStatusValue()); EXPECT_EQ(0, upstream_request_->body().length()); } @@ -1899,15 +1724,15 @@ TEST_P(ProtocolIntegrationTest, MultipleSetCookies) { ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - std::vector out; - Http::HeaderUtility::getAllOfHeader(response->headers(), "set-cookie", out); + const auto out = response->headers().get(Http::LowerCaseString("set-cookie")); ASSERT_EQ(out.size(), 2); - ASSERT_EQ(out[0], "foo"); - ASSERT_EQ(out[1], "bar"); + ASSERT_EQ(out[0]->value().getStringView(), "foo"); + ASSERT_EQ(out[1]->value().getStringView(), "bar"); } // Resets the downstream stream immediately and verifies that we clean up everything. TEST_P(ProtocolIntegrationTest, TestDownstreamResetIdleTimeout) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE_DETAILS%"); config_helper_.setDownstreamHttpIdleTimeout(std::chrono::milliseconds(100)); initialize(); @@ -1936,6 +1761,7 @@ TEST_P(ProtocolIntegrationTest, TestDownstreamResetIdleTimeout) { } ASSERT_TRUE(codec_client_->waitForDisconnect()); + EXPECT_THAT(waitForAccessLog(access_log_name_), Not(HasSubstr("DPE"))); } // Test connection is closed after single request processed. From 5f345b30baaca90fde4f8a4335a9b30018caf92e Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 20 Oct 2020 20:06:29 +0000 Subject: [PATCH 08/11] Added integration test for overflowing status in h2 Signed-off-by: Zach --- test/integration/http2_integration_test.cc | 25 +++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index d83eefb85188..29c849ef9563 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1630,22 +1630,27 @@ TEST_P(Http2FrameIntegrationTest, OverflowingResponseCode) { settings_flags = static_cast(1); // ack Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); ASSERT_TRUE(tcp_client_->write(std::string(ack_frame), false, false)); // ack setting - Http2Frame h2_frame = Http2Frame::makeRequest(1, "host", "/"); - ASSERT_TRUE(tcp_client_->write(std::string(h2_frame), false, false)); + 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 - - // Overflowing response code + // 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", - 1 /*, static_cast(4)*/); - - // Http::Http2::Http2Frame overflowed_status = - // Http::Http2::Http2Frame::makeHeadersFrameWithStatus("400", 1); - + "11111111111111111111111111111111111111111111111111111111111111111", 0); ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); tcp_client_->close(); From f34ac084a35922ba2c5c197b01710ff38657d25d Mon Sep 17 00:00:00 2001 From: Zach Date: Fri, 23 Oct 2020 17:55:17 +0000 Subject: [PATCH 09/11] Save progress Signed-off-by: Zach --- test/common/http/http2/http2_frame.cc | 8 +++ test/common/http/http2/http2_frame.h | 1 + test/integration/http2_integration_test.cc | 59 ++++++++++++++++++- test/integration/integration_test.cc | 3 +- test/integration/protocol_integration_test.cc | 34 +++++++++++ 5 files changed, 103 insertions(+), 2 deletions(-) diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index bff503db0ce4..6d62e9b1608e 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -151,6 +151,14 @@ Http2Frame Http2Frame::makeEmptyHeadersFrame(uint32_t stream_index, HeadersFlags return frame; } +Http2Frame Http2Frame::makeHeadersFrameNoStatus(uint32_t stream_index) { + Http2Frame frame; + frame.buildHeader(Type::Headers, 0, static_cast(orFlags(HeadersFlags::EndStream, + HeadersFlags::EndHeaders)), + makeRequestStreamId(stream_index)); + return frame; +} + Http2Frame Http2Frame::makeHeadersFrameWithStatus(std::string status, uint32_t stream_index) { Http2Frame frame; frame.buildHeader( diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index a4627cc95377..5505a1c2b694 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -109,6 +109,7 @@ class Http2Frame { static Http2Frame makeEmptySettingsFrame(SettingsFlags flags = SettingsFlags::None); static Http2Frame makeEmptyHeadersFrame(uint32_t stream_index, HeadersFlags flags = HeadersFlags::None); + static Http2Frame makeHeadersFrameNoStatus(uint32_t stream_index); static Http2Frame makeHeadersFrameWithStatus( std::string status, uint32_t stream_index); // Want to test overridden int here, so make it string diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 29c849ef9563..33b487a07f75 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1610,7 +1610,64 @@ TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) { // Expect that the details for the first frame are kept. EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers")); } +// Yan version +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(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + FakeRawConnectionPtr fake_upstream_connection; + Http2Frame::SettingsFlags settings_flags = static_cast(0); + Http2Frame setting_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); + // Ack settings + settings_flags = static_cast(1); // ack + Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); + // setup upstream and settings etc. + ASSERT(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection, + std::chrono::milliseconds(10))); + ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings + ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting + Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus( + "11111111111111111111111111111111111111111111111111111111111111111", 0); + ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); + response->waitForEndStream(); + EXPECT_EQ("503", response->headers().getStatusValue()); +} + +// Yan version +TEST_P(Http2FrameIntegrationTest, MissingStatus) { + 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(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + FakeRawConnectionPtr fake_upstream_connection; + + Http2Frame::SettingsFlags settings_flags = static_cast(0); + Http2Frame setting_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); + // Ack settings + settings_flags = static_cast(1); // ack + Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); + // setup upstream and settings etc. + ASSERT(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection, + std::chrono::milliseconds(10))); + ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings + ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting + Http::Http2::Http2Frame missing_status = Http::Http2::Http2Frame::makeHeadersFrameNoStatus( + 0); + ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status))); + response->waitForEndStream(); + EXPECT_EQ("503", response->headers().getStatusValue()); +} +/* TEST_P(Http2FrameIntegrationTest, OverflowingResponseCode) { setDownstreamProtocol(Http::CodecClient::Type::HTTP2); setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); @@ -1654,7 +1711,7 @@ TEST_P(Http2FrameIntegrationTest, OverflowingResponseCode) { ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); tcp_client_->close(); -} +}*/ INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index de72491a1ef0..51a438225b53 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -560,8 +560,9 @@ TEST_P(IntegrationTest, OverflowingResponseCode) { 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()); + response->waitForEndStream(); + EXPECT_EQ("503", response->headers().getStatusValue()); } TEST_P(IntegrationTest, BadFirstline) { diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 39e5b2738e87..ca363ef55144 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -24,6 +24,7 @@ #include "common/runtime/runtime_impl.h" #include "common/upstream/upstream_impl.h" +#include "test/common/http/http2/http2_frame.h" #include "test/common/upstream/utility.h" #include "test/integration/autonomous_upstream.h" #include "test/integration/http_integration.h" @@ -1093,6 +1094,39 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) { } } +TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { + initialize(); + + FakeRawConnectionPtr fake_upstream_connection; + //HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); + + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + ASSERT_TRUE(fake_upstream_connection->write( + "HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n", false)); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + } else { + Http::Http2::Http2Frame::SettingsFlags settings_flags = static_cast(0); + Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); + // Ack settings + settings_flags = static_cast(1); // ack + Http2Frame ack_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); + ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings + ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting + Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus( + "11111111111111111111111111111111111111111111111111111111111111111", 0); + ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); + } + + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + response->waitForEndStream(); + EXPECT_EQ("503", response->headers().getStatusValue()); +} + // Validate that lots of tiny cookies doesn't cause a DoS (single cookie header). TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) { initialize(); From 0e260cda271d0acebd812bfde2e03268e95ec799 Mon Sep 17 00:00:00 2001 From: Zach Date: Fri, 23 Oct 2020 18:29:28 +0000 Subject: [PATCH 10/11] Overflowing and missing status added to protocol integration Signed-off-by: Zach --- test/common/http/http2/http2_frame.cc | 9 +- test/integration/http2_integration_test.cc | 102 ------------------ test/integration/integration_test.cc | 16 --- test/integration/protocol_integration_test.cc | 42 +++++++- 4 files changed, 43 insertions(+), 126 deletions(-) diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index 57b6426adc0d..34decb609a46 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -153,9 +153,10 @@ Http2Frame Http2Frame::makeEmptyHeadersFrame(uint32_t stream_index, HeadersFlags Http2Frame Http2Frame::makeHeadersFrameNoStatus(uint32_t stream_index) { Http2Frame frame; - frame.buildHeader(Type::Headers, 0, static_cast(orFlags(HeadersFlags::EndStream, - HeadersFlags::EndHeaders)), - makeRequestStreamId(stream_index)); + frame.buildHeader( + Type::Headers, 0, + static_cast(orFlags(HeadersFlags::EndStream, HeadersFlags::EndHeaders)), + makeNetworkOrderStreamId(stream_index)); return frame; } @@ -165,7 +166,7 @@ Http2Frame Http2Frame::makeHeadersFrameWithStatus(std::string status, uint32_t s Type::Headers, 0, orFlags(HeadersFlags::EndStream, HeadersFlags::EndHeaders), // TODO: Support not hardcoding these two flags - makeRequestStreamId(stream_index)); + makeNetworkOrderStreamId(stream_index)); if (status == "200") { frame.appendStaticHeader(StaticHeaderIndex::Status200); } else if (status == "204") { diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index b1c0b9e1f701..1ad689a4849e 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1560,108 +1560,6 @@ TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) { // Expect that the details for the first frame are kept. EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers")); } -// Yan version -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(); - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - FakeRawConnectionPtr fake_upstream_connection; - - Http2Frame::SettingsFlags settings_flags = static_cast(0); - Http2Frame setting_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); - // Ack settings - settings_flags = static_cast(1); // ack - Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); - // setup upstream and settings etc. - ASSERT(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection, - std::chrono::milliseconds(10))); - ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings - ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting - Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus( - "11111111111111111111111111111111111111111111111111111111111111111", 0); - ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); - response->waitForEndStream(); - EXPECT_EQ("503", response->headers().getStatusValue()); -} - -// Yan version -TEST_P(Http2FrameIntegrationTest, MissingStatus) { - 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(); - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - FakeRawConnectionPtr fake_upstream_connection; - - Http2Frame::SettingsFlags settings_flags = static_cast(0); - Http2Frame setting_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); - // Ack settings - settings_flags = static_cast(1); // ack - Http2Frame ack_frame = Http2Frame::makeEmptySettingsFrame(settings_flags); - // setup upstream and settings etc. - ASSERT(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection, - std::chrono::milliseconds(10))); - ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings - ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting - Http::Http2::Http2Frame missing_status = Http::Http2::Http2Frame::makeHeadersFrameNoStatus( - 0); - ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status))); - response->waitForEndStream(); - EXPECT_EQ("503", response->headers().getStatusValue()); -} -/* -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(); - 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(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(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()), diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 7295202684bc..3f91991cee94 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -549,22 +549,6 @@ 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->waitForDisconnect()); - response->waitForEndStream(); - EXPECT_EQ("503", response->headers().getStatusValue()); -} - TEST_P(IntegrationTest, BadFirstline) { initialize(); std::string response; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index ca363ef55144..cb72353e1ad2 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1098,7 +1098,7 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { initialize(); FakeRawConnectionPtr fake_upstream_connection; - //HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames + // HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames codec_client_ = makeHttpConnection(lookupPort("http")); auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); @@ -1107,10 +1107,12 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { ASSERT_TRUE(fake_upstream_connection->write( - "HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n", false)); + "HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n", + false)); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); } else { - Http::Http2::Http2Frame::SettingsFlags settings_flags = static_cast(0); + Http::Http2::Http2Frame::SettingsFlags settings_flags = + static_cast(0); Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); // Ack settings settings_flags = static_cast(1); // ack @@ -1118,7 +1120,7 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus( - "11111111111111111111111111111111111111111111111111111111111111111", 0); + "11111111111111111111111111111111111111111111111111111111111111111", 0); ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); } @@ -1127,6 +1129,38 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { EXPECT_EQ("503", response->headers().getStatusValue()); } +TEST_P(ProtocolIntegrationTest, MissingStatus) { + initialize(); + + FakeRawConnectionPtr fake_upstream_connection; + // HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); + + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 OK\r\n", false)); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + } else { + Http::Http2::Http2Frame::SettingsFlags settings_flags = + static_cast(0); + Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); + // Ack settings + settings_flags = static_cast(1); // ack + Http2Frame ack_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); + ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings + ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting + Http::Http2::Http2Frame missing_status = Http::Http2::Http2Frame::makeHeadersFrameNoStatus(0); + ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status))); + } + + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + response->waitForEndStream(); + EXPECT_EQ("503", response->headers().getStatusValue()); +} + // Validate that lots of tiny cookies doesn't cause a DoS (single cookie header). TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) { initialize(); From 8f2e64003a51abc9f8a77a54d104bf466dd48fe0 Mon Sep 17 00:00:00 2001 From: Zach Date: Mon, 26 Oct 2020 20:14:33 +0000 Subject: [PATCH 11/11] Switched from exception to ASSERT Signed-off-by: Zach --- source/common/http/BUILD | 1 - source/common/http/http1/codec_impl.cc | 2 -- source/common/http/http2/codec_impl.cc | 17 +++++------------ source/common/http/http2/codec_impl.h | 6 +++--- source/common/http/utility.cc | 11 +---------- source/common/http/utility.h | 6 +----- 6 files changed, 10 insertions(+), 33 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index c1915d0eba0b..726c33322614 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -387,7 +387,6 @@ 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/network:utility_lib", diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 82a8b09d8e69..42b7f4dfce6a 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -339,8 +339,6 @@ 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()) { diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 5e2269661f6c..4af93895de4a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -304,14 +304,9 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { readDisable(false); } -Status ConnectionImpl::ClientStreamImpl::decodeHeaders() { +void ConnectionImpl::ClientStreamImpl::decodeHeaders() { auto& headers = absl::get(headers_or_trailers_); - - 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(); + const uint64_t status = Http::Utility::getResponseStatus(*headers); if (!upgrade_type_.empty() && headers->Status()) { Http::Utility::transformUpgradeResponseFromH2toH1(*headers, upgrade_type_); @@ -328,7 +323,6 @@ Status ConnectionImpl::ClientStreamImpl::decodeHeaders() { } else { response_decoder_.decodeHeaders(std::move(headers), remote_end_stream_); } - return okStatus(); } void ConnectionImpl::ClientStreamImpl::decodeTrailers() { @@ -336,13 +330,12 @@ void ConnectionImpl::ClientStreamImpl::decodeTrailers() { std::move(absl::get(headers_or_trailers_))); } -Status ConnectionImpl::ServerStreamImpl::decodeHeaders() { +void ConnectionImpl::ServerStreamImpl::decodeHeaders() { auto& headers = absl::get(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() { @@ -799,7 +792,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { switch (frame->headers.cat) { case NGHTTP2_HCAT_RESPONSE: case NGHTTP2_HCAT_REQUEST: { - RETURN_IF_ERROR(stream->decodeHeaders()); + stream->decodeHeaders(); break; } @@ -813,7 +806,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { stream->decodeTrailers(); } else { // We're a client session and still waiting for non-informational headers. - RETURN_IF_ERROR(stream->decodeHeaders()); + stream->decodeHeaders(); } } break; diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index f703857800c6..c3c883f7dfd6 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -258,7 +258,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, nghttp2_data_provider* provider) override; StreamDecoder& decoder() override { return response_decoder_; } - Status decodeHeaders() override; + void decodeHeaders() override; void decodeTrailers() override; HeaderMap& headers() override { if (absl::holds_alternative(headers_or_trailers_)) { @@ -374,7 +374,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, nghttp2_data_provider* provider) override; StreamDecoder& decoder() override { return *request_decoder_; } - Status decodeHeaders() override; + void decodeHeaders() override; void decodeTrailers() override; HeaderMap& headers() override { if (absl::holds_alternative(headers_or_trailers_)) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e05b05c2b4d8..cb018c0c18f2 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -31,7 +31,6 @@ #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 { @@ -409,18 +408,10 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin } uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) { - absl::StatusOr response_status_or_absl_status = getResponseStatusOr(headers); - 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 Utility::getResponseStatusOr(const ResponseHeaderMap& headers) { const HeaderEntry* header = headers.Status(); uint64_t response_code; if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) { - return absl::InvalidArgumentError(":status must be specified and a valid unsigned long"); + throw CodecClientException(":status must be specified and a valid unsigned long"); } return response_code; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 35cd5cbae401..d677b097d86c 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -13,7 +13,6 @@ #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" @@ -255,13 +254,10 @@ 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 and release asserts if the - * headers are invalid. + * @return uint64_t the response code or throws an exception if the headers are invalid. */ uint64_t getResponseStatus(const ResponseHeaderMap& headers); -absl::StatusOr 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: