From 317752b88379085abf08fd89b2737166345ae75d Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 24 Dec 2020 01:15:57 +0000 Subject: [PATCH 01/48] grpc-web: Fix 503 handling for gRPC Web text This fixes the 503 handling for gRPC Web requests with text content-type. Previously, when the upstream cannot be contacted (local reset or connection reset), gRPC Web filter always expect gRPC response from local reply handler. Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 81 ++++++++++++++----- .../filters/http/grpc_web/grpc_web_filter.h | 10 +-- .../grpc_web_filter_integration_test.cc | 71 +++++++++++----- 3 files changed, 115 insertions(+), 47 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 62d7eda354f9e..8fcae0e7439ef 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -7,7 +7,9 @@ #include "common/common/assert.h" #include "common/common/base64.h" #include "common/common/empty_string.h" +#include "common/common/enum_to_int.h" #include "common/common/utility.h" +#include "common/grpc/common.h" #include "common/grpc/context_impl.h" #include "common/http/headers.h" #include "common/http/utility.h" @@ -17,6 +19,37 @@ namespace Extensions { namespace HttpFilters { namespace GrpcWeb { +namespace { + +// Bit mask denotes a trailers frame of gRPC-Web. +constexpr const uint8_t GRPC_WEB_TRAILER = 0b10000000; + +const absl::flat_hash_set& gRpcWebContentTypes() { + // Supported gRPC-Web content-types. + CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set, + Http::Headers::get().ContentTypeValues.GrpcWeb, + Http::Headers::get().ContentTypeValues.GrpcWebProto, + Http::Headers::get().ContentTypeValues.GrpcWebText, + Http::Headers::get().ContentTypeValues.GrpcWebTextProto); +} + +bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) { + const Http::HeaderEntry* content_type = headers.ContentType(); + if (content_type != nullptr) { + return gRpcWebContentTypes().count(content_type->value().getStringView()) > 0; + } + return false; +} + +bool isGrpcWebRequest(const Http::RequestHeaderMap& headers) { + if (!headers.Path()) { + return false; + } + return hasGrpcWebContentType(headers); +} + +} // namespace + Http::RegisterCustomInlineHeader accept_handle(Http::CustomHeaders::get().Accept); Http::RegisterCustomInlineHeader @@ -30,27 +63,17 @@ struct RcDetailsValues { }; using RcDetails = ConstSingleton; -// Bit mask denotes a trailers frame of gRPC-Web. -const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; - -// Supported gRPC-Web content-types. -const absl::flat_hash_set& GrpcWebFilter::gRpcWebContentTypes() const { - static const absl::flat_hash_set* types = new absl::flat_hash_set( - {Http::Headers::get().ContentTypeValues.GrpcWeb, - Http::Headers::get().ContentTypeValues.GrpcWebProto, - Http::Headers::get().ContentTypeValues.GrpcWebText, - Http::Headers::get().ContentTypeValues.GrpcWebTextProto}); - return *types; -} - -bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) { - if (!headers.Path()) { - return false; - } - const Http::HeaderEntry* content_type = headers.ContentType(); - if (content_type != nullptr) { - return gRpcWebContentTypes().count(content_type->value().getStringView()) > 0; +bool GrpcWebFilter::checkGrpcResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { + const bool ok = Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK); + if (ok && (Grpc::Common::isGrpcResponseHeaders(headers, end_stream) || + hasGrpcWebContentType(headers))) { + return true; } + + ENVOY_LOG(info, "upstream response is not gRPC response"); + headers.setGrpcStatus( + Grpc::Utility::httpToGrpcStatus(enumToInt(Http::Utility::getResponseStatus(headers)))); + response_headers_ = &headers; return false; } @@ -143,7 +166,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en } // Implements StreamEncoderFilter. -Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { +Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers, + bool end_stream) { if (!is_grpc_web_request_) { return Http::FilterHeadersStatus::Continue; } @@ -156,6 +180,12 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& } else { headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto); } + + if (!checkGrpcResponseHeaders(headers, end_stream)) { + ENVOY_LOG(info, "stop iteration"); + return Http::FilterHeadersStatus::StopIteration; + } + return Http::FilterHeadersStatus::Continue; } @@ -164,6 +194,15 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { return Http::FilterDataStatus::Continue; } + // When the upstream response is not a gRPC response, we set the "grpc-message" header with the + // upstream body content. + if (response_headers_ != nullptr) { + data.drain(data.length()); + response_headers_->setGrpcMessage(Http::Utility::PercentEncoding::encode(data.toString())); + response_headers_->setContentLength(0); + return Http::FilterDataStatus::Continue; + } + if (!is_text_response_) { // No additional transcoding required if gRPC-Web client asked for binary response. return Http::FilterDataStatus::Continue; diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 2ae3d2381fbf2..acc0eb00aa381 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -16,7 +16,9 @@ namespace GrpcWeb { /** * See docs/configuration/http_filters/grpc_web_filter.rst */ -class GrpcWebFilter : public Http::StreamFilter, NonCopyable { +class GrpcWebFilter : public Http::StreamFilter, + NonCopyable, + public Logger::Loggable { public: explicit GrpcWebFilter(Grpc::Context& context) : context_(context) {} ~GrpcWebFilter() override = default; @@ -55,10 +57,7 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers); void setupStatTracking(const Http::RequestHeaderMap& headers); - bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); - - static const uint8_t GRPC_WEB_TRAILER; - const absl::flat_hash_set& gRpcWebContentTypes() const; + bool checkGrpcResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream); Upstream::ClusterInfoConstSharedPtr cluster_; Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; @@ -70,6 +69,7 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { absl::optional request_stat_names_; bool is_grpc_web_request_{}; Grpc::Context& context_; + Http::ResponseHeaderMap* response_headers_{nullptr}; }; } // namespace GrpcWeb diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 5a8831476b532..1387729dd95c4 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -1,5 +1,7 @@ #include +#include "common/common/base64.h" + #include "extensions/filters/http/well_known_names.h" #include "test/integration/http_integration.h" @@ -9,9 +11,14 @@ namespace Envoy { namespace { +constexpr absl::string_view text{"application/grpc-web-text"}; +constexpr absl::string_view binary{"application/grpc-web"}; + using SkipEncodingEmptyTrailers = bool; -using TestParams = - std::tuple; +using ContentType = std::string; +using Accept = std::string; +using TestParams = std::tuple; class GrpcWebFilterIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { @@ -32,11 +39,13 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, static std::string testParamsToString(const testing::TestParamInfo params) { return fmt::format( - "{}_{}_{}", + "{}_{}_{}_{}_{}", TestUtility::ipTestParamsToString(testing::TestParamInfo( std::get<0>(params.param), params.index)), std::get<1>(params.param) == Http::CodecClient::Type::HTTP2 ? "Http2" : "Http", - std::get<2>(params.param) ? "SkipEncodingEmptyTrailers" : "SubmitEncodingEmptyTrailers"); + std::get<2>(params.param) ? "SkipEncodingEmptyTrailers" : "SubmitEncodingEmptyTrailers", + std::get<3>(params.param) == text ? "SendText" : "SendBinary", + std::get<4>(params.param) == text ? "AcceptText" : "AcceptBinary"); } }; @@ -45,12 +54,16 @@ INSTANTIATE_TEST_SUITE_P( testing::Combine( testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Values(Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2), - testing::Values(SkipEncodingEmptyTrailers{true}, SkipEncodingEmptyTrailers{false})), + testing::Values(SkipEncodingEmptyTrailers{true}, SkipEncodingEmptyTrailers{false}), + testing::Values(ContentType{text}, ContentType{binary}), + testing::Values(Accept{text}, Accept{binary})), GrpcWebFilterIntegrationTest::testParamsToString); TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { const auto downstream_protocol = std::get<1>(GetParam()); const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); + const auto content_type = std::get<3>(GetParam()); + const auto accept = std::get<4>(GetParam()); if (downstream_protocol == Http::CodecClient::Type::HTTP1) { config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); @@ -67,30 +80,37 @@ TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = codec_client_->startRequest( - Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {"content-type", "application/grpc-web"}, - {":authority", "host"}}); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type}, + {"accept", accept}, + {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); - codec_client_->sendData(*request_encoder_, 1, false); + + const std::string body = "hello"; + const std::string encoded_body = + content_type == text ? Base64::encode(body.data(), body.length()) : body; + codec_client_->sendData(*request_encoder_, encoded_body, false); codec_client_->sendTrailers(*request_encoder_, request_trailers); waitForNextUpstreamRequest(); + default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); upstream_request_->encodeHeaders(default_response_headers_, false); upstream_request_->encodeData(1, false); upstream_request_->encodeTrailers(response_trailers); response->waitForEndStream(); EXPECT_TRUE(upstream_request_->complete()); - EXPECT_EQ(1, upstream_request_->bodyLength()); + EXPECT_EQ(body.length(), upstream_request_->bodyLength()); EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - EXPECT_TRUE(absl::StrContains(response->body(), "response1:trailer1")); - EXPECT_TRUE(absl::StrContains(response->body(), "response2:trailer2")); + const auto response_body = accept == text ? Base64::decode(response->body()) : response->body(); + EXPECT_TRUE(absl::StrContains(response_body, "response1:trailer1")); + EXPECT_TRUE(absl::StrContains(response_body, "response2:trailer2")); if (downstream_protocol == Http::CodecClient::Type::HTTP1) { // When the downstream protocol is HTTP/1.1 we expect the trailers to be in the response-body. @@ -113,6 +133,8 @@ TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { const auto downstream_protocol = std::get<1>(GetParam()); const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); + const auto content_type = std::get<3>(GetParam()); + const auto accept = std::get<4>(GetParam()); if (downstream_protocol == Http::CodecClient::Type::HTTP1) { config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); @@ -127,12 +149,13 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = codec_client_->startRequest( - Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {"content-type", "application/grpc-web"}, - {":authority", "host"}}); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type}, + {"accept", accept}, + {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); codec_client_->sendData(*request_encoder_, 1, false); @@ -141,7 +164,13 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { ASSERT_TRUE(fake_upstream_connection_->close()); response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + + // TODO(dio): Need to check each response body, should reflect the requirement mandated by the + // request "Accept" header. EXPECT_EQ("503", response->headers().getStatusValue()); + EXPECT_NE(nullptr, response->headers().GrpcMessage()); + EXPECT_EQ(0U, response->body().length()); codec_client_->close(); } From 4145f68fe386891106dbe5365207bc7f439e50da Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 24 Dec 2020 03:56:54 +0000 Subject: [PATCH 02/48] Tidy Signed-off-by: Dhi Aurrahman --- .../http/grpc_web/grpc_web_filter_integration_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 1387729dd95c4..37d8bd76e4e83 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -62,8 +62,8 @@ INSTANTIATE_TEST_SUITE_P( TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { const auto downstream_protocol = std::get<1>(GetParam()); const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const auto content_type = std::get<3>(GetParam()); - const auto accept = std::get<4>(GetParam()); + const ContentType& content_type = std::get<3>(GetParam()); + const Accept& accept = std::get<4>(GetParam()); if (downstream_protocol == Http::CodecClient::Type::HTTP1) { config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); @@ -133,8 +133,8 @@ TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { const auto downstream_protocol = std::get<1>(GetParam()); const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const auto content_type = std::get<3>(GetParam()); - const auto accept = std::get<4>(GetParam()); + const ContentType& content_type = std::get<3>(GetParam()); + const Accept& accept = std::get<4>(GetParam()); if (downstream_protocol == Http::CodecClient::Type::HTTP1) { config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); From e1107729f6e719eca2126520b81524d867496941 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 29 Dec 2020 22:29:41 +0000 Subject: [PATCH 03/48] WIP Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 28 ++++++++----------- .../filters/http/grpc_web/grpc_web_filter.h | 5 +--- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 8fcae0e7439ef..cae886d8dd233 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -48,6 +48,13 @@ bool isGrpcWebRequest(const Http::RequestHeaderMap& headers) { return hasGrpcWebContentType(headers); } +// Valid response headers contain gRPC or gRPC-Web response headers. +bool isValidResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { + return Grpc::Common::isGrpcResponseHeaders(headers, end_stream) || + (Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && + hasGrpcWebContentType(headers)); +} + } // namespace Http::RegisterCustomInlineHeader @@ -63,20 +70,6 @@ struct RcDetailsValues { }; using RcDetails = ConstSingleton; -bool GrpcWebFilter::checkGrpcResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { - const bool ok = Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK); - if (ok && (Grpc::Common::isGrpcResponseHeaders(headers, end_stream) || - hasGrpcWebContentType(headers))) { - return true; - } - - ENVOY_LOG(info, "upstream response is not gRPC response"); - headers.setGrpcStatus( - Grpc::Utility::httpToGrpcStatus(enumToInt(Http::Utility::getResponseStatus(headers)))); - response_headers_ = &headers; - return false; -} - // Implements StreamDecoderFilter. // TODO(fengli): Implements the subtypes of gRPC-Web content-type other than proto, like +json, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { @@ -181,8 +174,8 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto); } - if (!checkGrpcResponseHeaders(headers, end_stream)) { - ENVOY_LOG(info, "stop iteration"); + if (!end_stream && !isValidResponseHeaders(headers, end_stream)) { + response_headers_ = &headers; return Http::FilterHeadersStatus::StopIteration; } @@ -198,6 +191,9 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { // upstream body content. if (response_headers_ != nullptr) { data.drain(data.length()); + + response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( + enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); response_headers_->setGrpcMessage(Http::Utility::PercentEncoding::encode(data.toString())); response_headers_->setContentLength(0); return Http::FilterDataStatus::Continue; diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index acc0eb00aa381..b20036596f679 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -16,9 +16,7 @@ namespace GrpcWeb { /** * See docs/configuration/http_filters/grpc_web_filter.rst */ -class GrpcWebFilter : public Http::StreamFilter, - NonCopyable, - public Logger::Loggable { +class GrpcWebFilter : public Http::StreamFilter, NonCopyable { public: explicit GrpcWebFilter(Grpc::Context& context) : context_(context) {} ~GrpcWebFilter() override = default; @@ -57,7 +55,6 @@ class GrpcWebFilter : public Http::StreamFilter, void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers); void setupStatTracking(const Http::RequestHeaderMap& headers); - bool checkGrpcResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream); Upstream::ClusterInfoConstSharedPtr cluster_; Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; From ec65cf2dff2ff4b9765248a4c58a1a7979d0476f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 30 Dec 2020 00:38:42 +0000 Subject: [PATCH 04/48] Add MAX_GRPC_MESSAGE_LENGTH Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 18 ++++++++++++++---- .../http/grpc_web/grpc_web_filter_test.cc | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index cae886d8dd233..678a8ed81da86 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -23,6 +23,7 @@ namespace { // Bit mask denotes a trailers frame of gRPC-Web. constexpr const uint8_t GRPC_WEB_TRAILER = 0b10000000; +constexpr const uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; const absl::flat_hash_set& gRpcWebContentTypes() { // Supported gRPC-Web content-types. @@ -182,19 +183,28 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& return Http::FilterHeadersStatus::Continue; } -Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { +Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool end_stream) { if (!is_grpc_web_request_) { return Http::FilterDataStatus::Continue; } - // When the upstream response is not a gRPC response, we set the "grpc-message" header with the - // upstream body content. + // When the upstream response (this is also relevant for local reply, since gRPC-Web request is + // not a gRPC request which makes the local reply's is_grpc_request set to false) is not a gRPC + // response, we set the "grpc-message" header with the upstream body content. if (response_headers_ != nullptr) { + if (!end_stream) { + return Http::FilterDataStatus::StopIterationNoBuffer; + } + + // Take the last frame as the grpc-message value, but the size of it is limited by + // MAX_GRPC_MESSAGE_LENGTH. + const auto message = + Http::Utility::PercentEncoding::encode(data.toString().substr(0, MAX_GRPC_MESSAGE_LENGTH)); data.drain(data.length()); response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); - response_headers_->setGrpcMessage(Http::Utility::PercentEncoding::encode(data.toString())); + response_headers_->setGrpcMessage(message); response_headers_->setContentLength(0); return Http::FilterDataStatus::Continue; } diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 6221286157781..589e19deb3521 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -195,6 +195,25 @@ TEST_F(GrpcWebFilterTest, Base64NoPadding) { EXPECT_EQ(decoder_callbacks_.details(), "grpc_base_64_decode_failed_bad_size"); } +TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { + Http::TestRequestHeaderMapImpl request_headers{ + {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebText}, {":path", "/"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + Buffer::OwnedImpl data1("start"); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data1, false)); + Buffer::OwnedImpl data2("hello"); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data2, false)); + Buffer::OwnedImpl data3("end"); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data3, true)); + + Http::TestResponseTrailerMapImpl response_trailers{{"grpc-status", "0"}}; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); +} + TEST_P(GrpcWebFilterTest, StatsNoCluster) { Http::TestRequestHeaderMapImpl request_headers{ {"content-type", request_content_type()}, From dd02ff5a97ce2c9ef946b3657ad5252560834238 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 30 Dec 2020 00:50:36 +0000 Subject: [PATCH 05/48] Add log Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 10 ++++++---- .../extensions/filters/http/grpc_web/grpc_web_filter.h | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 678a8ed81da86..0f67bf3e6aa9b 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -175,12 +175,14 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto); } - if (!end_stream && !isValidResponseHeaders(headers, end_stream)) { - response_headers_ = &headers; - return Http::FilterHeadersStatus::StopIteration; + if (end_stream || isValidResponseHeaders(headers, end_stream)) { + return Http::FilterHeadersStatus::Continue; } - return Http::FilterHeadersStatus::Continue; + ENVOY_LOG(debug, "received invalid response headers since the upstream response or local reply " + "is not a gRPC or gRPC-Web response"); + response_headers_ = &headers; + return Http::FilterHeadersStatus::StopIteration; } Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool end_stream) { diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index b20036596f679..928d25fda8a5b 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -16,7 +16,9 @@ namespace GrpcWeb { /** * See docs/configuration/http_filters/grpc_web_filter.rst */ -class GrpcWebFilter : public Http::StreamFilter, NonCopyable { +class GrpcWebFilter : public Http::StreamFilter, + NonCopyable, + public Logger::Loggable { public: explicit GrpcWebFilter(Grpc::Context& context) : context_(context) {} ~GrpcWebFilter() override = default; From 363ba3221a0afb50d080aadce5d3379d03d724d5 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 30 Dec 2020 23:30:40 +0000 Subject: [PATCH 06/48] Check for message Signed-off-by: Dhi Aurrahman --- .../http/grpc_web/grpc_web_filter_integration_test.cc | 7 ++++--- .../filters/http/grpc_web/grpc_web_filter_test.cc | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 37d8bd76e4e83..379fa5fd17cd4 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -166,10 +166,11 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); - // TODO(dio): Need to check each response body, should reflect the requirement mandated by the - // request "Accept" header. EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_NE(nullptr, response->headers().GrpcMessage()); + EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); + EXPECT_EQ("upstream connect error or disconnect/reset before headers. reset reason: connection " + "termination", + response->headers().getGrpcMessageValue()); EXPECT_EQ(0U, response->body().length()); codec_client_->close(); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 589e19deb3521..924af3790c090 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -210,6 +210,9 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { Buffer::OwnedImpl data3("end"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data3, true)); + // We only get the last frame here. + EXPECT_EQ("end", response_headers.get_(Http::Headers::get().GrpcMessage)); + Http::TestResponseTrailerMapImpl response_trailers{{"grpc-status", "0"}}; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); } From 17117004edd2ad473cd7b2073d0ccbb30dcc7969 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 6 Jan 2021 22:24:47 +0000 Subject: [PATCH 07/48] Revert Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 72 ++++++++++--------- .../filters/http/grpc_web/grpc_web_filter.h | 6 ++ .../http/grpc_web/grpc_web_filter_test.cc | 4 +- 3 files changed, 45 insertions(+), 37 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 0f67bf3e6aa9b..ddb682367cb15 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -19,22 +19,36 @@ namespace Extensions { namespace HttpFilters { namespace GrpcWeb { -namespace { +Http::RegisterCustomInlineHeader + accept_handle(Http::CustomHeaders::get().Accept); +Http::RegisterCustomInlineHeader + grpc_accept_encoding_handle(Http::CustomHeaders::get().GrpcAcceptEncoding); + +struct RcDetailsValues { + // The grpc web filter couldn't decode the data as the size wasn't a multiple of 4. + const std::string GrpcDecodeFailedDueToSize = "grpc_base_64_decode_failed_bad_size"; + // The grpc web filter couldn't decode the data provided. + const std::string GrpcDecodeFailedDueToData = "grpc_base_64_decode_failed"; +}; +using RcDetails = ConstSingleton; // Bit mask denotes a trailers frame of gRPC-Web. -constexpr const uint8_t GRPC_WEB_TRAILER = 0b10000000; -constexpr const uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; - -const absl::flat_hash_set& gRpcWebContentTypes() { - // Supported gRPC-Web content-types. - CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set, - Http::Headers::get().ContentTypeValues.GrpcWeb, - Http::Headers::get().ContentTypeValues.GrpcWebProto, - Http::Headers::get().ContentTypeValues.GrpcWebText, - Http::Headers::get().ContentTypeValues.GrpcWebTextProto); +const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; + +// Supported gRPC-Web content-types. +const absl::flat_hash_set& GrpcWebFilter::gRpcWebContentTypes() const { + static const absl::flat_hash_set* types = new absl::flat_hash_set( + {Http::Headers::get().ContentTypeValues.GrpcWeb, + Http::Headers::get().ContentTypeValues.GrpcWebProto, + Http::Headers::get().ContentTypeValues.GrpcWebText, + Http::Headers::get().ContentTypeValues.GrpcWebTextProto}); + return *types; } -bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) { +bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) { + if (!headers.Path()) { + return false; + } const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr) { return gRpcWebContentTypes().count(content_type->value().getStringView()) > 0; @@ -42,35 +56,22 @@ bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) { return false; } -bool isGrpcWebRequest(const Http::RequestHeaderMap& headers) { - if (!headers.Path()) { - return false; +bool GrpcWebFilter::hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const { + const Http::HeaderEntry* content_type = headers.ContentType(); + if (content_type != nullptr) { + return gRpcWebContentTypes().count(content_type->value().getStringView()) > 0; } - return hasGrpcWebContentType(headers); + return false; } // Valid response headers contain gRPC or gRPC-Web response headers. -bool isValidResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { +bool GrpcWebFilter::isValidResponseHeaders(Http::ResponseHeaderMap& headers, + bool end_stream) const { return Grpc::Common::isGrpcResponseHeaders(headers, end_stream) || (Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && hasGrpcWebContentType(headers)); } -} // namespace - -Http::RegisterCustomInlineHeader - accept_handle(Http::CustomHeaders::get().Accept); -Http::RegisterCustomInlineHeader - grpc_accept_encoding_handle(Http::CustomHeaders::get().GrpcAcceptEncoding); - -struct RcDetailsValues { - // The grpc web filter couldn't decode the data as the size wasn't a multiple of 4. - const std::string GrpcDecodeFailedDueToSize = "grpc_base_64_decode_failed_bad_size"; - // The grpc web filter couldn't decode the data provided. - const std::string GrpcDecodeFailedDueToData = "grpc_base_64_decode_failed"; -}; -using RcDetails = ConstSingleton; - // Implements StreamDecoderFilter. // TODO(fengli): Implements the subtypes of gRPC-Web content-type other than proto, like +json, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { @@ -195,13 +196,14 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en // response, we set the "grpc-message" header with the upstream body content. if (response_headers_ != nullptr) { if (!end_stream) { - return Http::FilterDataStatus::StopIterationNoBuffer; + return Http::FilterDataStatus::StopIterationAndBuffer; } + constexpr uint64_t max_grpc_message_length = 1024; // Take the last frame as the grpc-message value, but the size of it is limited by - // MAX_GRPC_MESSAGE_LENGTH. + // max_grpc_message_length. const auto message = - Http::Utility::PercentEncoding::encode(data.toString().substr(0, MAX_GRPC_MESSAGE_LENGTH)); + Http::Utility::PercentEncoding::encode(data.toString().substr(0, max_grpc_message_length)); data.drain(data.length()); response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 928d25fda8a5b..4b0a7218562bc 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -57,6 +57,12 @@ class GrpcWebFilter : public Http::StreamFilter, void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers); void setupStatTracking(const Http::RequestHeaderMap& headers); + bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); + bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; + bool isValidResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream) const; + + static const uint8_t GRPC_WEB_TRAILER; + const absl::flat_hash_set& gRpcWebContentTypes() const; Upstream::ClusterInfoConstSharedPtr cluster_; Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 924af3790c090..fc34716f2f5bd 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -204,9 +204,9 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data1("start"); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data1, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data1, false)); Buffer::OwnedImpl data2("hello"); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data2, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data2, false)); Buffer::OwnedImpl data3("end"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data3, true)); From ce1f186356601a1145f09d37fea23d801fff0e69 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 6 Jan 2021 23:51:43 +0000 Subject: [PATCH 08/48] Set no buffering and copyOut Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 12 +++++++----- .../filters/http/grpc_web/grpc_web_filter_test.cc | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index ddb682367cb15..c8094a761f255 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -1,4 +1,5 @@ #include "extensions/filters/http/grpc_web/grpc_web_filter.h" +#include #ifndef WIN32 #include @@ -196,19 +197,20 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en // response, we set the "grpc-message" header with the upstream body content. if (response_headers_ != nullptr) { if (!end_stream) { - return Http::FilterDataStatus::StopIterationAndBuffer; + return Http::FilterDataStatus::StopIterationNoBuffer; } - constexpr uint64_t max_grpc_message_length = 1024; // Take the last frame as the grpc-message value, but the size of it is limited by // max_grpc_message_length. - const auto message = - Http::Utility::PercentEncoding::encode(data.toString().substr(0, max_grpc_message_length)); + constexpr uint64_t max_grpc_message_length = 1024; + const uint64_t message_size = std::min(max_grpc_message_length, data.length()); + std::string message(message_size, 0); + data.copyOut(0, message_size, message.data()); data.drain(data.length()); response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); - response_headers_->setGrpcMessage(message); + response_headers_->setGrpcMessage(Http::Utility::PercentEncoding::encode(message)); response_headers_->setContentLength(0); return Http::FilterDataStatus::Continue; } diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index fc34716f2f5bd..924af3790c090 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -204,9 +204,9 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data1("start"); - EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data1, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data1, false)); Buffer::OwnedImpl data2("hello"); - EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data2, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data2, false)); Buffer::OwnedImpl data3("end"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data3, true)); From d61e3979f3dc3bfd7cc7ab7de7c493920d56a332 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 6 Jan 2021 23:52:34 +0000 Subject: [PATCH 09/48] copyOut and no buffering Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index c8094a761f255..c94a40a46e8c9 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -1,4 +1,5 @@ #include "extensions/filters/http/grpc_web/grpc_web_filter.h" + #include #ifndef WIN32 From 1aa30bb60774afb2fd40949b79217154349f96ab Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 7 Jan 2021 09:24:51 +0000 Subject: [PATCH 10/48] Remove Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index c94a40a46e8c9..27650e8e544c1 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -1,7 +1,5 @@ #include "extensions/filters/http/grpc_web/grpc_web_filter.h" -#include - #ifndef WIN32 #include #endif From 531f8eba662e14496189a062193c0adb2268c3f6 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 7 Jan 2021 13:13:17 +0000 Subject: [PATCH 11/48] Test bad upstream Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 5 +- .../grpc_web_filter_integration_test.cc | 48 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 27650e8e544c1..2d38612228fc3 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -170,13 +170,16 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& if (doStatTracking()) { chargeStat(headers); } + + const bool valid_response = isValidResponseHeaders(headers, end_stream); + if (is_text_response_) { headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebTextProto); } else { headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto); } - if (end_stream || isValidResponseHeaders(headers, end_stream)) { + if (end_stream || valid_response) { return Http::FilterHeadersStatus::Continue; } diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 379fa5fd17cd4..ce61c1d36deba 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -176,5 +176,53 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { codec_client_->close(); } +TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { + const auto downstream_protocol = std::get<1>(GetParam()); + const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); + const ContentType& content_type = std::get<3>(GetParam()); + const Accept& accept = std::get<4>(GetParam()); + + if (downstream_protocol == Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + } else { + skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); + } + + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + + Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, + {"request2", "trailer2"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type}, + {"accept", accept}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 1, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + // Sending back non gRPC-Web response. + default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); + upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); + upstream_request_->encodeData(1, /*end_stream=*/false); + const std::string message = "{\"percentage\": \"100%\"}"; + upstream_request_->encodeData(message, /*end_stream=*/true); + response->waitForEndStream(); + + ASSERT_TRUE(fake_upstream_connection_->close()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("{\"percentage\": \"100%25\"}", response->headers().getGrpcMessageValue()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); + EXPECT_EQ(0U, response->body().length()); + codec_client_->close(); +} + } // namespace } // namespace Envoy From 277ddac8f09e232506dbaae12ef3742d1f72549b Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 7 Jan 2021 23:37:52 +0000 Subject: [PATCH 12/48] Fix test Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter_test.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 924af3790c090..67d1682f7fb8c 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -240,7 +240,8 @@ TEST_P(GrpcWebFilterTest, StatsNormalResponse) { Http::MetadataMap metadata_map{{"metadata", "metadata"}}; EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_.encodeMetadata(metadata_map)); - Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", request_accept()}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, false)); @@ -262,7 +263,8 @@ TEST_P(GrpcWebFilterTest, StatsErrorResponse) { {"content-type", request_content_type()}, {":path", "/lyft.users.BadCompanions/GetBadCompanions"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", request_accept()}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, false)); @@ -280,7 +282,8 @@ TEST_P(GrpcWebFilterTest, StatsErrorResponse) { } TEST_P(GrpcWebFilterTest, ExternallyProvidedEncodingHeader) { - Http::TestRequestHeaderMapImpl request_headers{{"grpc-accept-encoding", "foo"}, {":path", "/"}}; + Http::TestRequestHeaderMapImpl request_headers{ + {"grpc-accept-encoding", "foo"}, {":path", "/"}, {"content-type", request_accept()}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers_, false)); EXPECT_EQ("foo", request_headers.get_(Http::CustomHeaders::get().GrpcAcceptEncoding)); @@ -339,6 +342,7 @@ TEST_P(GrpcWebFilterTest, Unary) { // Tests response headers. Http::TestResponseHeaderMapImpl response_headers; response_headers.addCopy(Http::Headers::get().Status, "200"); + response_headers.addCopy(Http::Headers::get().ContentType, request_accept()); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); EXPECT_EQ("200", response_headers.get_(Http::Headers::get().Status.get())); if (accept_binary_response()) { From 151e3d57f33910b6152cdd38ba71cf7470f4c227 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 8 Jan 2021 08:36:33 +0000 Subject: [PATCH 13/48] Feature flag, release notes Signed-off-by: Dhi Aurrahman --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 1 + .../filters/http/grpc_web/grpc_web_filter.cc | 81 ++++-- .../filters/http/grpc_web/grpc_web_filter.h | 3 +- test/extensions/filters/http/grpc_web/BUILD | 1 + .../grpc_web_filter_integration_test.cc | 236 +++++++++--------- .../http/grpc_web/grpc_web_filter_test.cc | 59 ++++- 7 files changed, 238 insertions(+), 144 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 53537d8eed950..f8a820f4c7526 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -42,6 +42,7 @@ Bug Fixes * dns: fix a bug where custom resolvers provided in configuration were not preserved after network issues. * dns_filter: correctly associate DNS response IDs when multiple queries are received. * grpc mux: fix sending node again after stream is reset when ::ref:`set_node_on_first_message_only ` is set. +* grpc-web: fix local reply and non-gRPC response handling. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. * http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. * http: reject requests with missing required headers after filter chain processing. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 34ba8d3a013e8..30af8187ebbd7 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -69,6 +69,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.enable_dns_cache_circuit_breakers", "envoy.reloadable_features.fix_upgrade_response", "envoy.reloadable_features.fix_wildcard_matching", + "envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling", "envoy.reloadable_features.fixed_connection_close", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.http_default_alpn", diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 2d38612228fc3..84db7954ebc02 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -13,12 +13,36 @@ #include "common/grpc/context_impl.h" #include "common/http/headers.h" #include "common/http/utility.h" +#include "common/runtime/runtime_features.h" namespace Envoy { namespace Extensions { namespace HttpFilters { namespace GrpcWeb { +namespace { + +constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; + +std::string buildGrpcMessage(const Buffer::Instance* buffered, const Buffer::Instance* last) { + Buffer::OwnedImpl buffer; + if (buffered) { + buffer.add(*buffered); + } + + if (last) { + buffer.add(*last); + } + + uint64_t length = std::min(buffer.length(), MAX_GRPC_MESSAGE_LENGTH); + std::string message(length, 0); + buffer.copyOut(0, length, message.data()); + + return Http::Utility::PercentEncoding::encode(message); +} + +} // namespace + Http::RegisterCustomInlineHeader accept_handle(Http::CustomHeaders::get().Accept); Http::RegisterCustomInlineHeader @@ -64,12 +88,14 @@ bool GrpcWebFilter::hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap return false; } -// Valid response headers contain gRPC or gRPC-Web response headers. -bool GrpcWebFilter::isValidResponseHeaders(Http::ResponseHeaderMap& headers, - bool end_stream) const { - return Grpc::Common::isGrpcResponseHeaders(headers, end_stream) || - (Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && - hasGrpcWebContentType(headers)); +// If response headers do not contain gRPC or gRPC-Web response headers, it needs transformation. +bool GrpcWebFilter::needsResponseTransformation(Http::ResponseHeaderMap& headers, + bool end_stream) const { + return Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling") && + !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && + !(Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && + hasGrpcWebContentType(headers)); } // Implements StreamDecoderFilter. @@ -171,7 +197,7 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& chargeStat(headers); } - const bool valid_response = isValidResponseHeaders(headers, end_stream); + needs_response_transformation_ = needsResponseTransformation(headers, end_stream); if (is_text_response_) { headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebTextProto); @@ -179,12 +205,10 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto); } - if (end_stream || valid_response) { + if (end_stream || !needs_response_transformation_) { return Http::FilterHeadersStatus::Continue; } - ENVOY_LOG(debug, "received invalid response headers since the upstream response or local reply " - "is not a gRPC or gRPC-Web response"); response_headers_ = &headers; return Http::FilterHeadersStatus::StopIteration; } @@ -197,22 +221,29 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en // When the upstream response (this is also relevant for local reply, since gRPC-Web request is // not a gRPC request which makes the local reply's is_grpc_request set to false) is not a gRPC // response, we set the "grpc-message" header with the upstream body content. - if (response_headers_ != nullptr) { + if (needs_response_transformation_) { + auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (!end_stream) { + if (encoding_buffer == nullptr || encoding_buffer->length() < MAX_GRPC_MESSAGE_LENGTH) { + return Http::FilterDataStatus::StopIterationAndBuffer; + } return Http::FilterDataStatus::StopIterationNoBuffer; } - // Take the last frame as the grpc-message value, but the size of it is limited by - // max_grpc_message_length. - constexpr uint64_t max_grpc_message_length = 1024; - const uint64_t message_size = std::min(max_grpc_message_length, data.length()); - std::string message(message_size, 0); - data.copyOut(0, message_size, message.data()); + ASSERT(response_headers_ != nullptr); + needs_response_transformation_ = false; + + const auto message = buildGrpcMessage(encoding_buffer, &data); data.drain(data.length()); + if (encoding_buffer != nullptr) { + encoder_callbacks_->modifyEncodingBuffer( + [](Buffer::Instance& buffer) { buffer.drain(buffer.length()); }); + } + response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); - response_headers_->setGrpcMessage(Http::Utility::PercentEncoding::encode(message)); + response_headers_->setGrpcMessage(message); response_headers_->setContentLength(0); return Http::FilterDataStatus::Continue; } @@ -255,6 +286,20 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::ResponseTrailerMa chargeStat(trailers); } + if (needs_response_transformation_) { + const auto message = buildGrpcMessage(encoder_callbacks_->encodingBuffer(), nullptr); + if (encoder_callbacks_->encodingBuffer() != nullptr) { + encoder_callbacks_->modifyEncodingBuffer( + [](Buffer::Instance& buffer) { buffer.drain(buffer.length()); }); + } + + response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( + enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); + response_headers_->setGrpcMessage(message); + response_headers_->setContentLength(0); + return Http::FilterTrailersStatus::Continue; + } + // Trailers are expected to come all in once, and will be encoded into one single trailers frame. // Trailers in the trailers frame are separated by `CRLFs`. Buffer::OwnedImpl temp; diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 4b0a7218562bc..a33faee445342 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -59,7 +59,7 @@ class GrpcWebFilter : public Http::StreamFilter, void setupStatTracking(const Http::RequestHeaderMap& headers); bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; - bool isValidResponseHeaders(Http::ResponseHeaderMap& headers, bool end_stream) const; + bool needsResponseTransformation(Http::ResponseHeaderMap& headers, bool end_stream) const; static const uint8_t GRPC_WEB_TRAILER; const absl::flat_hash_set& gRpcWebContentTypes() const; @@ -69,6 +69,7 @@ class GrpcWebFilter : public Http::StreamFilter, Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; bool is_text_request_{}; bool is_text_response_{}; + bool needs_response_transformation_{}; Buffer::OwnedImpl decoding_buffer_; Grpc::Decoder decoder_; absl::optional request_stat_names_; diff --git a/test/extensions/filters/http/grpc_web/BUILD b/test/extensions/filters/http/grpc_web/BUILD index 831e596ffb149..118bb57ab794c 100644 --- a/test/extensions/filters/http/grpc_web/BUILD +++ b/test/extensions/filters/http/grpc_web/BUILD @@ -19,6 +19,7 @@ envoy_extension_cc_test( "//source/extensions/filters/http/grpc_web:grpc_web_filter_lib", "//test/mocks/http:http_mocks", "//test/test_common:global_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index ce61c1d36deba..1c12aad2e0a6f 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -59,76 +59,77 @@ INSTANTIATE_TEST_SUITE_P( testing::Values(Accept{text}, Accept{binary})), GrpcWebFilterIntegrationTest::testParamsToString); -TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { - const auto downstream_protocol = std::get<1>(GetParam()); - const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const ContentType& content_type = std::get<3>(GetParam()); - const Accept& accept = std::get<4>(GetParam()); - - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); - } - - setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); - - Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, - {"request2", "trailer2"}}; - Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, - {"response2", "trailer2"}}; - - initialize(); - codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {"content-type", content_type}, - {"accept", accept}, - {":authority", "host"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - - const std::string body = "hello"; - const std::string encoded_body = - content_type == text ? Base64::encode(body.data(), body.length()) : body; - codec_client_->sendData(*request_encoder_, encoded_body, false); - codec_client_->sendTrailers(*request_encoder_, request_trailers); - waitForNextUpstreamRequest(); - default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); - upstream_request_->encodeHeaders(default_response_headers_, false); - upstream_request_->encodeData(1, false); - upstream_request_->encodeTrailers(response_trailers); - response->waitForEndStream(); - - EXPECT_TRUE(upstream_request_->complete()); - EXPECT_EQ(body.length(), upstream_request_->bodyLength()); - EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); - - EXPECT_TRUE(response->complete()); - EXPECT_EQ("200", response->headers().getStatusValue()); - const auto response_body = accept == text ? Base64::decode(response->body()) : response->body(); - EXPECT_TRUE(absl::StrContains(response_body, "response1:trailer1")); - EXPECT_TRUE(absl::StrContains(response_body, "response2:trailer2")); - - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - // When the downstream protocol is HTTP/1.1 we expect the trailers to be in the response-body. - EXPECT_EQ(nullptr, response->trailers()); - } - - if (downstream_protocol == Http::CodecClient::Type::HTTP2) { - if (http2_skip_encoding_empty_trailers) { - // When the downstream protocol is HTTP/2 and the feature-flag to skip encoding empty trailers - // is turned on, expect that the trailers are included in the response-body. - EXPECT_EQ(nullptr, response->trailers()); - } else { - // Otherwise, we send empty trailers. - ASSERT_NE(nullptr, response->trailers()); - EXPECT_TRUE(response->trailers()->empty()); - } - } -} +// TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { +// const auto downstream_protocol = std::get<1>(GetParam()); +// const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); +// const ContentType& content_type = std::get<3>(GetParam()); +// const Accept& accept = std::get<4>(GetParam()); + +// if (downstream_protocol == Http::CodecClient::Type::HTTP1) { +// config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); +// } else { +// skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); +// } + +// setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + +// Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, +// {"request2", "trailer2"}}; +// Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, +// {"response2", "trailer2"}}; + +// initialize(); +// codec_client_ = makeHttpConnection(lookupPort("http")); +// auto encoder_decoder = +// codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, +// {":path", "/test/long/url"}, +// {":scheme", "http"}, +// {"content-type", content_type}, +// {"accept", accept}, +// {":authority", "host"}}); +// request_encoder_ = &encoder_decoder.first; +// auto response = std::move(encoder_decoder.second); + +// const std::string body = "hello"; +// const std::string encoded_body = +// content_type == text ? Base64::encode(body.data(), body.length()) : body; +// codec_client_->sendData(*request_encoder_, encoded_body, false); +// codec_client_->sendTrailers(*request_encoder_, request_trailers); +// waitForNextUpstreamRequest(); +// default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); +// upstream_request_->encodeHeaders(default_response_headers_, false); +// upstream_request_->encodeData(1, false); +// upstream_request_->encodeTrailers(response_trailers); +// response->waitForEndStream(); + +// EXPECT_TRUE(upstream_request_->complete()); +// EXPECT_EQ(body.length(), upstream_request_->bodyLength()); +// EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); + +// EXPECT_TRUE(response->complete()); +// EXPECT_EQ("200", response->headers().getStatusValue()); +// const auto response_body = accept == text ? Base64::decode(response->body()) : +// response->body(); EXPECT_TRUE(absl::StrContains(response_body, "response1:trailer1")); +// EXPECT_TRUE(absl::StrContains(response_body, "response2:trailer2")); + +// if (downstream_protocol == Http::CodecClient::Type::HTTP1) { +// // When the downstream protocol is HTTP/1.1 we expect the trailers to be in the +// response-body. EXPECT_EQ(nullptr, response->trailers()); +// } + +// if (downstream_protocol == Http::CodecClient::Type::HTTP2) { +// if (http2_skip_encoding_empty_trailers) { +// // When the downstream protocol is HTTP/2 and the feature-flag to skip encoding empty +// trailers +// // is turned on, expect that the trailers are included in the response-body. +// EXPECT_EQ(nullptr, response->trailers()); +// } else { +// // Otherwise, we send empty trailers. +// ASSERT_NE(nullptr, response->trailers()); +// EXPECT_TRUE(response->trailers()->empty()); +// } +// } +// } TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { const auto downstream_protocol = std::get<1>(GetParam()); @@ -176,53 +177,54 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { codec_client_->close(); } -TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { - const auto downstream_protocol = std::get<1>(GetParam()); - const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const ContentType& content_type = std::get<3>(GetParam()); - const Accept& accept = std::get<4>(GetParam()); - - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); - } - - setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); - - Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, - {"request2", "trailer2"}}; - - initialize(); - codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {"content-type", content_type}, - {"accept", accept}, - {":authority", "host"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - codec_client_->sendData(*request_encoder_, 1, false); - codec_client_->sendTrailers(*request_encoder_, request_trailers); - waitForNextUpstreamRequest(); - // Sending back non gRPC-Web response. - default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); - upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); - upstream_request_->encodeData(1, /*end_stream=*/false); - const std::string message = "{\"percentage\": \"100%\"}"; - upstream_request_->encodeData(message, /*end_stream=*/true); - response->waitForEndStream(); - - ASSERT_TRUE(fake_upstream_connection_->close()); - EXPECT_TRUE(response->complete()); - EXPECT_EQ("{\"percentage\": \"100%25\"}", response->headers().getGrpcMessageValue()); - EXPECT_EQ("200", response->headers().getStatusValue()); - EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); - EXPECT_EQ(0U, response->body().length()); - codec_client_->close(); -} +// TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { +// const auto downstream_protocol = std::get<1>(GetParam()); +// const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); +// const ContentType& content_type = std::get<3>(GetParam()); +// const Accept& accept = std::get<4>(GetParam()); + +// if (downstream_protocol == Http::CodecClient::Type::HTTP1) { +// config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); +// } else { +// skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); +// } + +// setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + +// Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, +// {"request2", "trailer2"}}; +// Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, +// {"response2", "trailer2"}}; + +// initialize(); +// codec_client_ = makeHttpConnection(lookupPort("http")); +// auto encoder_decoder = +// codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, +// {":path", "/test/long/url"}, +// {":scheme", "http"}, +// {"content-type", content_type}, +// {"accept", accept}, +// {":authority", "host"}}); +// request_encoder_ = &encoder_decoder.first; +// auto response = std::move(encoder_decoder.second); +// codec_client_->sendData(*request_encoder_, 1, false); +// codec_client_->sendTrailers(*request_encoder_, request_trailers); +// waitForNextUpstreamRequest(); +// // Sending back non gRPC-Web response. +// default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); +// upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); +// upstream_request_->encodeData("{", /*end_stream=*/false); +// const std::string message = "\"percentage\": \"100%\"}"; +// upstream_request_->encodeData(message, /*end_stream=*/true); +// response->waitForEndStream(); + +// EXPECT_TRUE(response->complete()); +// EXPECT_EQ("{\"percentage\": \"100%25\"}", response->headers().getGrpcMessageValue()); +// EXPECT_EQ("200", response->headers().getStatusValue()); +// EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); +// EXPECT_EQ(0U, response->body().length()); +// codec_client_->close(); +// } } // namespace } // namespace Envoy diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 67d1682f7fb8c..92e811ae5a1d6 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -14,6 +14,7 @@ #include "test/mocks/http/mocks.h" #include "test/test_common/global.h" #include "test/test_common/printers.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -203,18 +204,60 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); - Buffer::OwnedImpl data1("start"); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data1, false)); - Buffer::OwnedImpl data2("hello"); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data2, false)); - Buffer::OwnedImpl data3("end"); - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data3, true)); + Buffer::OwnedImpl data("hello"); + Buffer::InstancePtr buffer(new Buffer::OwnedImpl("hello")); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); + + buffer->add(data); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); + + TestUtility::feedBufferWithRandomCharacters(data, 1024); + const std::string expected_grpc_message = + absl::StrCat("hellohello", data.toString()).substr(0, 1024); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); + EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); +} + +TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithTrailers) { + Http::TestRequestHeaderMapImpl request_headers{ + {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebText}, {":path", "/"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + Buffer::OwnedImpl data("hello"); + Buffer::InstancePtr buffer(new Buffer::OwnedImpl("hello")); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); - // We only get the last frame here. - EXPECT_EQ("end", response_headers.get_(Http::Headers::get().GrpcMessage)); + buffer->add(data); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); Http::TestResponseTrailerMapImpl response_trailers{{"grpc-status", "0"}}; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); + + EXPECT_EQ("hellohello", response_headers.get_(Http::Headers::get().GrpcMessage)); +} + +TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextSkipTransformation) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling", "false"}}); + + Http::TestRequestHeaderMapImpl request_headers{ + {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebText}, + {"accept", Http::Headers::get().ContentTypeValues.GrpcWebText}, + {":path", "/"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); + Buffer::OwnedImpl data("hello"); + // Since the client expects grpc-web-text and the upstream response does not contain gRPC frames, + // the iteration is paused. + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data, false)); } TEST_P(GrpcWebFilterTest, StatsNoCluster) { From 8e4f6a397d7647193aaafad8212cc3cc48197df7 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 8 Jan 2021 08:40:34 +0000 Subject: [PATCH 14/48] Enable tests Signed-off-by: Dhi Aurrahman --- .../grpc_web_filter_integration_test.cc | 237 +++++++++--------- 1 file changed, 118 insertions(+), 119 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 1c12aad2e0a6f..e043dc0566712 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -59,77 +59,76 @@ INSTANTIATE_TEST_SUITE_P( testing::Values(Accept{text}, Accept{binary})), GrpcWebFilterIntegrationTest::testParamsToString); -// TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { -// const auto downstream_protocol = std::get<1>(GetParam()); -// const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); -// const ContentType& content_type = std::get<3>(GetParam()); -// const Accept& accept = std::get<4>(GetParam()); - -// if (downstream_protocol == Http::CodecClient::Type::HTTP1) { -// config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); -// } else { -// skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); -// } - -// setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); - -// Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, -// {"request2", "trailer2"}}; -// Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, -// {"response2", "trailer2"}}; - -// initialize(); -// codec_client_ = makeHttpConnection(lookupPort("http")); -// auto encoder_decoder = -// codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, -// {":path", "/test/long/url"}, -// {":scheme", "http"}, -// {"content-type", content_type}, -// {"accept", accept}, -// {":authority", "host"}}); -// request_encoder_ = &encoder_decoder.first; -// auto response = std::move(encoder_decoder.second); - -// const std::string body = "hello"; -// const std::string encoded_body = -// content_type == text ? Base64::encode(body.data(), body.length()) : body; -// codec_client_->sendData(*request_encoder_, encoded_body, false); -// codec_client_->sendTrailers(*request_encoder_, request_trailers); -// waitForNextUpstreamRequest(); -// default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); -// upstream_request_->encodeHeaders(default_response_headers_, false); -// upstream_request_->encodeData(1, false); -// upstream_request_->encodeTrailers(response_trailers); -// response->waitForEndStream(); - -// EXPECT_TRUE(upstream_request_->complete()); -// EXPECT_EQ(body.length(), upstream_request_->bodyLength()); -// EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); - -// EXPECT_TRUE(response->complete()); -// EXPECT_EQ("200", response->headers().getStatusValue()); -// const auto response_body = accept == text ? Base64::decode(response->body()) : -// response->body(); EXPECT_TRUE(absl::StrContains(response_body, "response1:trailer1")); -// EXPECT_TRUE(absl::StrContains(response_body, "response2:trailer2")); - -// if (downstream_protocol == Http::CodecClient::Type::HTTP1) { -// // When the downstream protocol is HTTP/1.1 we expect the trailers to be in the -// response-body. EXPECT_EQ(nullptr, response->trailers()); -// } - -// if (downstream_protocol == Http::CodecClient::Type::HTTP2) { -// if (http2_skip_encoding_empty_trailers) { -// // When the downstream protocol is HTTP/2 and the feature-flag to skip encoding empty -// trailers -// // is turned on, expect that the trailers are included in the response-body. -// EXPECT_EQ(nullptr, response->trailers()); -// } else { -// // Otherwise, we send empty trailers. -// ASSERT_NE(nullptr, response->trailers()); -// EXPECT_TRUE(response->trailers()->empty()); -// } -// } -// } +TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { + const auto downstream_protocol = std::get<1>(GetParam()); + const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); + const ContentType& content_type = std::get<3>(GetParam()); + const Accept& accept = std::get<4>(GetParam()); + + if (downstream_protocol == Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + } else { + skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); + } + + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + + Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, + {"request2", "trailer2"}}; + Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, + {"response2", "trailer2"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type}, + {"accept", accept}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + const std::string body = "hello"; + const std::string encoded_body = + content_type == text ? Base64::encode(body.data(), body.length()) : body; + codec_client_->sendData(*request_encoder_, encoded_body, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); + upstream_request_->encodeHeaders(default_response_headers_, false); + upstream_request_->encodeData(1, false); + upstream_request_->encodeTrailers(response_trailers); + response->waitForEndStream(); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(body.length(), upstream_request_->bodyLength()); + EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + const auto response_body = accept == text ? Base64::decode(response->body()) : response->body(); + EXPECT_TRUE(absl::StrContains(response_body, "response1:trailer1")); + EXPECT_TRUE(absl::StrContains(response_body, "response2:trailer2")); + + if (downstream_protocol == Http::CodecClient::Type::HTTP1) { + // When the downstream protocol is HTTP/1.1 we expect the trailers to be in the response-body. + EXPECT_EQ(nullptr, response->trailers()); + } + + if (downstream_protocol == Http::CodecClient::Type::HTTP2) { + if (http2_skip_encoding_empty_trailers) { + // When the downstream protocol is HTTP/2 and the feature-flag to skip encoding empty trailers + // is turned on, expect that the trailers are included in the response-body. + EXPECT_EQ(nullptr, response->trailers()); + } else { + // Otherwise, we send empty trailers. + ASSERT_NE(nullptr, response->trailers()); + EXPECT_TRUE(response->trailers()->empty()); + } + } +} TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { const auto downstream_protocol = std::get<1>(GetParam()); @@ -177,54 +176,54 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { codec_client_->close(); } -// TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { -// const auto downstream_protocol = std::get<1>(GetParam()); -// const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); -// const ContentType& content_type = std::get<3>(GetParam()); -// const Accept& accept = std::get<4>(GetParam()); - -// if (downstream_protocol == Http::CodecClient::Type::HTTP1) { -// config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); -// } else { -// skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); -// } - -// setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); - -// Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, -// {"request2", "trailer2"}}; -// Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, -// {"response2", "trailer2"}}; - -// initialize(); -// codec_client_ = makeHttpConnection(lookupPort("http")); -// auto encoder_decoder = -// codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, -// {":path", "/test/long/url"}, -// {":scheme", "http"}, -// {"content-type", content_type}, -// {"accept", accept}, -// {":authority", "host"}}); -// request_encoder_ = &encoder_decoder.first; -// auto response = std::move(encoder_decoder.second); -// codec_client_->sendData(*request_encoder_, 1, false); -// codec_client_->sendTrailers(*request_encoder_, request_trailers); -// waitForNextUpstreamRequest(); -// // Sending back non gRPC-Web response. -// default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); -// upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); -// upstream_request_->encodeData("{", /*end_stream=*/false); -// const std::string message = "\"percentage\": \"100%\"}"; -// upstream_request_->encodeData(message, /*end_stream=*/true); -// response->waitForEndStream(); - -// EXPECT_TRUE(response->complete()); -// EXPECT_EQ("{\"percentage\": \"100%25\"}", response->headers().getGrpcMessageValue()); -// EXPECT_EQ("200", response->headers().getStatusValue()); -// EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); -// EXPECT_EQ(0U, response->body().length()); -// codec_client_->close(); -// } +TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { + const auto downstream_protocol = std::get<1>(GetParam()); + const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); + const ContentType& content_type = std::get<3>(GetParam()); + const Accept& accept = std::get<4>(GetParam()); + + if (downstream_protocol == Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + } else { + skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); + } + + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + + Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, + {"request2", "trailer2"}}; + Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, + {"response2", "trailer2"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type}, + {"accept", accept}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 1, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + // Sending back non gRPC-Web response. + default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); + upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); + upstream_request_->encodeData("{", /*end_stream=*/false); + const std::string message = "\"percentage\": \"100%\"}"; + upstream_request_->encodeData(message, /*end_stream=*/true); + response->waitForEndStream(); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("{\"percentage\": \"100%25\"}", response->headers().getGrpcMessageValue()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); + EXPECT_EQ(0U, response->body().length()); + codec_client_->close(); +} } // namespace } // namespace Envoy From e0c3a009df52a2d7b4b3fd515cbe62585eba6dc5 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 8 Jan 2021 09:00:57 +0000 Subject: [PATCH 15/48] Introduce setTransformedResponseHeaders Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 42 +++++++++---------- .../filters/http/grpc_web/grpc_web_filter.h | 1 + 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 84db7954ebc02..0a6ac2222b7ec 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -24,7 +24,7 @@ namespace { constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; -std::string buildGrpcMessage(const Buffer::Instance* buffered, const Buffer::Instance* last) { +std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { Buffer::OwnedImpl buffer; if (buffered) { buffer.add(*buffered); @@ -32,6 +32,7 @@ std::string buildGrpcMessage(const Buffer::Instance* buffered, const Buffer::Ins if (last) { buffer.add(*last); + last->drain(last->length()); } uint64_t length = std::min(buffer.length(), MAX_GRPC_MESSAGE_LENGTH); @@ -98,6 +99,21 @@ bool GrpcWebFilter::needsResponseTransformation(Http::ResponseHeaderMap& headers hasGrpcWebContentType(headers)); } +void GrpcWebFilter::setTransformedResponseHeaders(Buffer::Instance* data) { + const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); + const auto message = buildGrpcMessage(encoding_buffer, data); + + if (encoding_buffer != nullptr) { + encoder_callbacks_->modifyEncodingBuffer( + [](Buffer::Instance& buffer) { buffer.drain(buffer.length()); }); + } + + response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( + enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); + response_headers_->setGrpcMessage(message); + response_headers_->setContentLength(0); +} + // Implements StreamDecoderFilter. // TODO(fengli): Implements the subtypes of gRPC-Web content-type other than proto, like +json, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { @@ -233,18 +249,7 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en ASSERT(response_headers_ != nullptr); needs_response_transformation_ = false; - const auto message = buildGrpcMessage(encoding_buffer, &data); - data.drain(data.length()); - - if (encoding_buffer != nullptr) { - encoder_callbacks_->modifyEncodingBuffer( - [](Buffer::Instance& buffer) { buffer.drain(buffer.length()); }); - } - - response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( - enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); - response_headers_->setGrpcMessage(message); - response_headers_->setContentLength(0); + setTransformedResponseHeaders(&data); return Http::FilterDataStatus::Continue; } @@ -287,16 +292,7 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::ResponseTrailerMa } if (needs_response_transformation_) { - const auto message = buildGrpcMessage(encoder_callbacks_->encodingBuffer(), nullptr); - if (encoder_callbacks_->encodingBuffer() != nullptr) { - encoder_callbacks_->modifyEncodingBuffer( - [](Buffer::Instance& buffer) { buffer.drain(buffer.length()); }); - } - - response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( - enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); - response_headers_->setGrpcMessage(message); - response_headers_->setContentLength(0); + setTransformedResponseHeaders(nullptr); return Http::FilterTrailersStatus::Continue; } diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index a33faee445342..7f65c11aa21b6 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -60,6 +60,7 @@ class GrpcWebFilter : public Http::StreamFilter, bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; bool needsResponseTransformation(Http::ResponseHeaderMap& headers, bool end_stream) const; + void setTransformedResponseHeaders(Buffer::Instance* data); static const uint8_t GRPC_WEB_TRAILER; const absl::flat_hash_set& gRpcWebContentTypes() const; From fcc89d15a596a6e46179b1c2d11a41cd19b16f49 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 8 Jan 2021 09:04:08 +0000 Subject: [PATCH 16/48] Add check Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 0a6ac2222b7ec..6c0b90af56cff 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -31,7 +31,9 @@ std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* } if (last) { - buffer.add(*last); + if (buffer.length() < MAX_GRPC_MESSAGE_LENGTH) { + buffer.add(*last); + } last->drain(last->length()); } From d1ee48ff46b5a6206f38b596c72f8f7653112b34 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sun, 10 Jan 2021 02:52:44 +0000 Subject: [PATCH 17/48] Fix Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 40 ++++--- .../grpc_web_filter_integration_test.cc | 101 ++++++++++-------- .../http/grpc_web/grpc_web_filter_test.cc | 41 +++++++ 3 files changed, 124 insertions(+), 58 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 6c0b90af56cff..1f0369974de20 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -26,20 +26,25 @@ constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { Buffer::OwnedImpl buffer; - if (buffered) { + if (buffered != nullptr) { + ASSERT(buffered->length() <= MAX_GRPC_MESSAGE_LENGTH); buffer.add(*buffered); } - if (last) { - if (buffer.length() < MAX_GRPC_MESSAGE_LENGTH) { - buffer.add(*last); + if (last != nullptr) { + uint64_t needed = last->length(); + if ((buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { + needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - buffer.length()); } + buffer.move(*last, needed); last->drain(last->length()); } - uint64_t length = std::min(buffer.length(), MAX_GRPC_MESSAGE_LENGTH); - std::string message(length, 0); - buffer.copyOut(0, length, message.data()); + ASSERT(buffer.length() <= MAX_GRPC_MESSAGE_LENGTH); + + const uint64_t message_length = buffer.length(); + std::string message(message_length, 0); + buffer.copyOut(0, message_length, message.data()); return Http::Utility::PercentEncoding::encode(message); } @@ -103,11 +108,21 @@ bool GrpcWebFilter::needsResponseTransformation(Http::ResponseHeaderMap& headers void GrpcWebFilter::setTransformedResponseHeaders(Buffer::Instance* data) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); + + if (encoding_buffer != nullptr && encoding_buffer->length() > MAX_GRPC_MESSAGE_LENGTH) { + encoder_callbacks_->modifyEncodingBuffer([](Buffer::Instance& buffered) { + Buffer::OwnedImpl needed_buffer; + needed_buffer.move(buffered, MAX_GRPC_MESSAGE_LENGTH); + buffered.drain(buffered.length()); + buffered.move(needed_buffer); + }); + } + const auto message = buildGrpcMessage(encoding_buffer, data); if (encoding_buffer != nullptr) { encoder_callbacks_->modifyEncodingBuffer( - [](Buffer::Instance& buffer) { buffer.drain(buffer.length()); }); + [](Buffer::Instance& buffered) { buffered.drain(buffered.length()); }); } response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( @@ -240,17 +255,16 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en // not a gRPC request which makes the local reply's is_grpc_request set to false) is not a gRPC // response, we set the "grpc-message" header with the upstream body content. if (needs_response_transformation_) { - auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); + const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (!end_stream) { - if (encoding_buffer == nullptr || encoding_buffer->length() < MAX_GRPC_MESSAGE_LENGTH) { - return Http::FilterDataStatus::StopIterationAndBuffer; + if (encoding_buffer != nullptr && encoding_buffer->length() >= MAX_GRPC_MESSAGE_LENGTH) { + return Http::FilterDataStatus::StopIterationNoBuffer; } - return Http::FilterDataStatus::StopIterationNoBuffer; + return Http::FilterDataStatus::StopIterationAndBuffer; } ASSERT(response_headers_ != nullptr); needs_response_transformation_ = false; - setTransformedResponseHeaders(&data); return Http::FilterDataStatus::Continue; } diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index e043dc0566712..728e66eccf424 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -37,6 +37,55 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, http2_skip_encoding_empty_trailers ? "true" : "false"); } + void testBadUpstreamResponse(const std::string& start, const std::string& end, + const std::string& expected) { + const auto downstream_protocol = std::get<1>(GetParam()); + const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); + const ContentType& content_type = std::get<3>(GetParam()); + const Accept& accept = std::get<4>(GetParam()); + + if (downstream_protocol == Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + } else { + skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); + } + + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + + Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, + {"request2", "trailer2"}}; + Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, + {"response2", "trailer2"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type}, + {"accept", accept}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 1, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + // Sending back non gRPC-Web response. + default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); + upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); + upstream_request_->encodeData(start, /*end_stream=*/false); + upstream_request_->encodeData(end, /*end_stream=*/true); + response->waitForEndStream(); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ(expected, response->headers().getGrpcMessageValue()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); + EXPECT_EQ(0U, response->body().length()); + codec_client_->close(); + } + static std::string testParamsToString(const testing::TestParamInfo params) { return fmt::format( "{}_{}_{}_{}_{}", @@ -177,52 +226,14 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { - const auto downstream_protocol = std::get<1>(GetParam()); - const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const ContentType& content_type = std::get<3>(GetParam()); - const Accept& accept = std::get<4>(GetParam()); - - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); - } - - setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); - - Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, - {"request2", "trailer2"}}; - Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, - {"response2", "trailer2"}}; - - initialize(); - codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {"content-type", content_type}, - {"accept", accept}, - {":authority", "host"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - codec_client_->sendData(*request_encoder_, 1, false); - codec_client_->sendTrailers(*request_encoder_, request_trailers); - waitForNextUpstreamRequest(); - // Sending back non gRPC-Web response. - default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); - upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); - upstream_request_->encodeData("{", /*end_stream=*/false); - const std::string message = "\"percentage\": \"100%\"}"; - upstream_request_->encodeData(message, /*end_stream=*/true); - response->waitForEndStream(); + testBadUpstreamResponse("{", "\"percentage\": \"100%\"}", + /*expected=*/"{\"percentage\": \"100%25\"}"); +} - EXPECT_TRUE(response->complete()); - EXPECT_EQ("{\"percentage\": \"100%25\"}", response->headers().getGrpcMessageValue()); - EXPECT_EQ("200", response->headers().getStatusValue()); - EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); - EXPECT_EQ(0U, response->body().length()); - codec_client_->close(); +TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLarge) { + const std::string start(1024, 'a'); + const std::string end(1024, 'b'); + testBadUpstreamResponse(start, end, /*expected=*/start); } } // namespace diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 92e811ae5a1d6..851d1a641a420 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -216,6 +216,47 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { const std::string expected_grpc_message = absl::StrCat("hellohello", data.toString()).substr(0, 1024); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); + EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); + EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); +} + +TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeEncodingBuffer) { + Http::TestRequestHeaderMapImpl request_headers{ + {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebText}, {":path", "/"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + Buffer::OwnedImpl data; + TestUtility::feedBufferWithRandomCharacters(data, 2048); + // The buffered length is limited to 1024. + const std::string expected_grpc_message = data.toString().substr(1024); + Buffer::InstancePtr buffer(new Buffer::OwnedImpl(expected_grpc_message)); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); + EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); + EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); +} + +TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeLastData) { + Http::TestRequestHeaderMapImpl request_headers{ + {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebText}, {":path", "/"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + Buffer::OwnedImpl data; + TestUtility::feedBufferWithRandomCharacters(data, 2048); + // The buffered length is limited to 1024. + const std::string expected_grpc_message = data.toString().substr(1024); + Buffer::InstancePtr buffer(new Buffer::OwnedImpl(expected_grpc_message)); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); + EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); } From cf4f00ee27db151cf1965944be4bd5a0ee3d941a Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sun, 10 Jan 2021 07:22:41 +0000 Subject: [PATCH 18/48] Add coverage Signed-off-by: Dhi Aurrahman --- .../grpc_web_filter_integration_test.cc | 23 +++++++++++++-- .../http/grpc_web/grpc_web_filter_test.cc | 29 ++++++++++--------- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 728e66eccf424..346c7dec195db 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -38,7 +38,7 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, } void testBadUpstreamResponse(const std::string& start, const std::string& end, - const std::string& expected) { + const std::string& expected, bool remove_content_type = false) { const auto downstream_protocol = std::get<1>(GetParam()); const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); const ContentType& content_type = std::get<3>(GetParam()); @@ -72,7 +72,12 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, codec_client_->sendTrailers(*request_encoder_, request_trailers); waitForNextUpstreamRequest(); // Sending back non gRPC-Web response. - default_response_headers_.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); + if (remove_content_type) { + default_response_headers_.removeContentType(); + } else { + default_response_headers_.setReferenceContentType( + Http::Headers::get().ContentTypeValues.Json); + } upstream_request_->encodeHeaders(default_response_headers_, /*end_stream=*/false); upstream_request_->encodeData(start, /*end_stream=*/false); upstream_request_->encodeData(end, /*end_stream=*/true); @@ -230,11 +235,23 @@ TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { /*expected=*/"{\"percentage\": \"100%25\"}"); } -TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLarge) { +TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseWithoutContentType) { + testBadUpstreamResponse("{", "\"percentage\": \"100%\"}", + /*expected=*/"{\"percentage\": \"100%25\"}", + /*remove_content_type=*/true); +} + +TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeEnd) { const std::string start(1024, 'a'); const std::string end(1024, 'b'); testBadUpstreamResponse(start, end, /*expected=*/start); } +TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeFirst) { + const std::string start(2048, 'a'); + const std::string end(1024, 'b'); + testBadUpstreamResponse(start, end, /*expected=*/start.substr(0, 1024)); +} + } // namespace } // namespace Envoy diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 851d1a641a420..a6cd86f49c30c 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -228,17 +228,22 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeEncodingBuffer) Http::TestResponseHeaderMapImpl response_headers{{":status", "400"}}; EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); - Buffer::OwnedImpl data; - TestUtility::feedBufferWithRandomCharacters(data, 2048); - // The buffered length is limited to 1024. - const std::string expected_grpc_message = data.toString().substr(1024); - Buffer::InstancePtr buffer(new Buffer::OwnedImpl(expected_grpc_message)); - EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data, false)); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(data, false)); - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); + Buffer::OwnedImpl encoded_buffer; + encoded_buffer.add(std::string(2048, 'a')); + + auto on_modify_encoding_buffer = [&encoded_buffer](std::function cb) { + cb(encoded_buffer); + }; + EXPECT_CALL(encoder_callbacks_, encodingBuffer).WillRepeatedly(Return(&encoded_buffer)); + EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) + .WillRepeatedly(Invoke(on_modify_encoding_buffer)); + + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, + filter_.encodeData(encoded_buffer, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, + filter_.encodeData(encoded_buffer, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(encoded_buffer, true)); EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); - EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); } TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeLastData) { @@ -250,11 +255,9 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeLastData) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data; - TestUtility::feedBufferWithRandomCharacters(data, 2048); + data.add(std::string(2048, 'a')); // The buffered length is limited to 1024. const std::string expected_grpc_message = data.toString().substr(1024); - Buffer::InstancePtr buffer(new Buffer::OwnedImpl(expected_grpc_message)); - EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); From df5257390cf7642e7a0ae9e2f7ece10a7e47c1f0 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 11 Jan 2021 21:17:04 +0000 Subject: [PATCH 19/48] Some of review comments Signed-off-by: Dhi Aurrahman --- docs/root/version_history/current.rst | 2 +- .../filters/http/grpc_web/grpc_web_filter.cc | 13 +++++++++- .../http/grpc_web/grpc_web_filter_test.cc | 24 +++++++++++++++---- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f8a820f4c7526..b77c865a65d7c 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -42,7 +42,7 @@ Bug Fixes * dns: fix a bug where custom resolvers provided in configuration were not preserved after network issues. * dns_filter: correctly associate DNS response IDs when multiple queries are received. * grpc mux: fix sending node again after stream is reset when ::ref:`set_node_on_first_message_only ` is set. -* grpc-web: fix local reply and non-gRPC response handling. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. +* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. * http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. * http: reject requests with missing required headers after filter chain processing. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 1f0369974de20..619e79d628bed 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -24,6 +24,8 @@ namespace { constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; +// This builds grpc-message header value from encoding buffer and the last frame (when +// end_stream=true) and the total length of it is limited by MAX_GRPC_MESSAGE_LENGTH. std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { Buffer::OwnedImpl buffer; if (buffered != nullptr) { @@ -91,7 +93,16 @@ bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) { bool GrpcWebFilter::hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const { const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr) { - return gRpcWebContentTypes().count(content_type->value().getStringView()) > 0; + // The value of media-type is case-sensitive https://tools.ietf.org/html/rfc2616#section-3.7. + const std::string content_type_value = + absl::AsciiStrToLower(content_type->value().getStringView()); + // We ignore "parameter" value. + const std::string current_content_type = + content_type_value.substr(0, content_type_value.find_last_of(';')); + // We expect only proto encoding response + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md. + return current_content_type == Http::Headers::get().ContentTypeValues.GrpcWebProto || + current_content_type == Http::Headers::get().ContentTypeValues.GrpcWeb; } return false; } diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index a6cd86f49c30c..49059af6ec56a 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -327,8 +327,8 @@ TEST_P(GrpcWebFilterTest, StatsNormalResponse) { Http::MetadataMap metadata_map{{"metadata", "metadata"}}; EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_.encodeMetadata(metadata_map)); - Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, - {"content-type", request_accept()}}; + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "200"}, {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebProto}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, false)); @@ -350,8 +350,8 @@ TEST_P(GrpcWebFilterTest, StatsErrorResponse) { {"content-type", request_content_type()}, {":path", "/lyft.users.BadCompanions/GetBadCompanions"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, - {"content-type", request_accept()}}; + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "200"}, {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebProto}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, false)); @@ -376,6 +376,19 @@ TEST_P(GrpcWebFilterTest, ExternallyProvidedEncodingHeader) { EXPECT_EQ("foo", request_headers.get_(Http::CustomHeaders::get().GrpcAcceptEncoding)); } +TEST_P(GrpcWebFilterTest, MediaTypeWithParameter) { + Http::TestRequestHeaderMapImpl request_headers{{"content-type", request_content_type()}, + {":path", "/test.MediaTypes/GetParameter"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "200"}, + // Set a valid media-type with a specified parameter value. + {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebProto + "; version=1"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, false)); +} + TEST_P(GrpcWebFilterTest, Unary) { // Tests request headers. request_headers_.addCopy(Http::Headers::get().ContentType, request_content_type()); @@ -429,7 +442,8 @@ TEST_P(GrpcWebFilterTest, Unary) { // Tests response headers. Http::TestResponseHeaderMapImpl response_headers; response_headers.addCopy(Http::Headers::get().Status, "200"); - response_headers.addCopy(Http::Headers::get().ContentType, request_accept()); + response_headers.addCopy(Http::Headers::get().ContentType, + Http::Headers::get().ContentTypeValues.GrpcWebProto); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); EXPECT_EQ("200", response_headers.get_(Http::Headers::get().Status.get())); if (accept_binary_response()) { From 8becf151aa47cc77486c26e86589c6d953a43974 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 13 Jan 2021 03:25:51 +0000 Subject: [PATCH 20/48] Release notes Signed-off-by: Dhi Aurrahman --- docs/root/version_history/current.rst | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3c0cf5ee68fed..379a5dc60f2f5 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,23 +20,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* config: validate that upgrade configs have a non-empty :ref:`upgrade_type `, fixing a bug where an errant "-" could result in unexpected behavior. -* dns: fixed a bug where custom resolvers provided in configuration were not preserved after network issues. -* dns_filter: correctly associate DNS response IDs when multiple queries are received. -* grpc mux: fix sending node again after stream is reset when ::ref:`set_node_on_first_message_only ` is set. -* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. -* grpc mux: fixed sending node again after stream is reset when :ref:`set_node_on_first_message_only ` is set. -* http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. -* http: reject requests with missing required headers after filter chain processing. -* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. -* proxy_proto: fixed a bug where the wrong downstream address got sent to upstream connections. -* proxy_proto: fixed a bug where network filters would not have the correct downstreamRemoteAddress() when accessed from the StreamInfo. This could result in incorrect enforcement of RBAC rules in the RBAC network filter (but not in the RBAC HTTP filter), or incorrect access log addresses from tcp_proxy. -* sds: fixed a bug that clusters sharing same sds target are marked active immediately. -* tls: fixed detection of the upstream connection close event. -* tls: fixed read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. -* udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. -* watchdog: touch the watchdog before most event loop operations to avoid misses when handling bursts of callbacks. * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. +* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. Removed Config or Runtime ------------------------- From ae9aab94f02633b70eb7d6b0dc7226d293cfee22 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 13 Jan 2021 13:31:45 +0000 Subject: [PATCH 21/48] Set bigger limit Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 12 +++++---- .../grpc_web_filter_integration_test.cc | 11 ++++---- .../http/grpc_web/grpc_web_filter_test.cc | 25 +++++++++++-------- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 619e79d628bed..f0f34494fe3e1 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -22,10 +22,14 @@ namespace GrpcWeb { namespace { -constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 1024; +// This is arbitrarily chosen. +// TODO(dio): Make it configurable when it is needed. +constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; // This builds grpc-message header value from encoding buffer and the last frame (when -// end_stream=true) and the total length of it is limited by MAX_GRPC_MESSAGE_LENGTH. +// end_stream=true). When we have buffered data in encoding buffer, we limit the length of +// grpc-message to be smaller than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" data, +// we send it all. std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { Buffer::OwnedImpl buffer; if (buffered != nullptr) { @@ -35,15 +39,13 @@ std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* if (last != nullptr) { uint64_t needed = last->length(); - if ((buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { + if (buffered != nullptr && (buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - buffer.length()); } buffer.move(*last, needed); last->drain(last->length()); } - ASSERT(buffer.length() <= MAX_GRPC_MESSAGE_LENGTH); - const uint64_t message_length = buffer.length(); std::string message(message_length, 0); buffer.copyOut(0, message_length, message.data()); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 346c7dec195db..2ba4854cb9e32 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -13,6 +13,7 @@ namespace { constexpr absl::string_view text{"application/grpc-web-text"}; constexpr absl::string_view binary{"application/grpc-web"}; +constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; using SkipEncodingEmptyTrailers = bool; using ContentType = std::string; @@ -242,15 +243,15 @@ TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseWithoutContentType) { } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeEnd) { - const std::string start(1024, 'a'); - const std::string end(1024, 'b'); + const std::string start(MAX_GRPC_MESSAGE_LENGTH, 'a'); + const std::string end(MAX_GRPC_MESSAGE_LENGTH, 'b'); testBadUpstreamResponse(start, end, /*expected=*/start); } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeFirst) { - const std::string start(2048, 'a'); - const std::string end(1024, 'b'); - testBadUpstreamResponse(start, end, /*expected=*/start.substr(0, 1024)); + const std::string start(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); + const std::string end(MAX_GRPC_MESSAGE_LENGTH, 'b'); + testBadUpstreamResponse(start, end, /*expected=*/start.substr(0, MAX_GRPC_MESSAGE_LENGTH)); } } // namespace diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 49059af6ec56a..81eaec3c21c98 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -45,6 +45,7 @@ const char INVALID_B64_MESSAGE[] = "****"; const size_t INVALID_B64_MESSAGE_SIZE = sizeof(INVALID_B64_MESSAGE) - 1; const char TRAILERS[] = "\x80\x00\x00\x00\x20grpc-status:0\r\ngrpc-message:ok\r\n"; const size_t TRAILERS_SIZE = sizeof(TRAILERS) - 1; +constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; } // namespace @@ -212,11 +213,12 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { buffer->add(data); EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); - TestUtility::feedBufferWithRandomCharacters(data, 1024); + TestUtility::feedBufferWithRandomCharacters(data, MAX_GRPC_MESSAGE_LENGTH); const std::string expected_grpc_message = - absl::StrCat("hellohello", data.toString()).substr(0, 1024); + absl::StrCat("hellohello", data.toString()).substr(0, MAX_GRPC_MESSAGE_LENGTH); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); - EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); + EXPECT_EQ(MAX_GRPC_MESSAGE_LENGTH, + response_headers.get_(Http::Headers::get().GrpcMessage).length()); EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); } @@ -229,8 +231,8 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeEncodingBuffer) EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl encoded_buffer; - encoded_buffer.add(std::string(2048, 'a')); - + encoded_buffer.add(std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a')); + // The encoding buffer is filled with data more than MAX_GRPC_MESSAGE_LENGTH. auto on_modify_encoding_buffer = [&encoded_buffer](std::function cb) { cb(encoded_buffer); }; @@ -243,7 +245,8 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeEncodingBuffer) EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(encoded_buffer, false)); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(encoded_buffer, true)); - EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); + EXPECT_EQ(MAX_GRPC_MESSAGE_LENGTH, + response_headers.get_(Http::Headers::get().GrpcMessage).length()); } TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeLastData) { @@ -255,11 +258,13 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeLastData) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data; - data.add(std::string(2048, 'a')); - // The buffered length is limited to 1024. - const std::string expected_grpc_message = data.toString().substr(1024); + // The last data length is set to be bigger than "MAX_GRPC_MESSAGE_LENGTH". + const std::string expected_grpc_message = std::string(MAX_GRPC_MESSAGE_LENGTH + 1, 'a'); + data.add(expected_grpc_message); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); - EXPECT_EQ(1024, response_headers.get_(Http::Headers::get().GrpcMessage).length()); + // The grpc-message value length is the same as the last sent buffer. + EXPECT_EQ(MAX_GRPC_MESSAGE_LENGTH + 1, + response_headers.get_(Http::Headers::get().GrpcMessage).length()); EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); } From 3cbab78653f4485d66306946d1737b6fd753f591 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 15 Jan 2021 01:13:44 +0000 Subject: [PATCH 22/48] Add local reply integration test Signed-off-by: Dhi Aurrahman --- .../grpc_web_filter_integration_test.cc | 168 ++++++++++-------- 1 file changed, 89 insertions(+), 79 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 2ba4854cb9e32..280ed3909e243 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -25,53 +25,98 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: GrpcWebFilterIntegrationTest() - : HttpIntegrationTest(std::get<1>(GetParam()), std::get<0>(GetParam())) {} + : HttpIntegrationTest(std::get<1>(GetParam()), std::get<0>(GetParam())), + downstream_protocol_{std::get<1>(GetParam())}, + http2_skip_encoding_empty_trailers_{std::get<2>(GetParam())}, + content_type_{std::get<3>(GetParam())}, accept_{std::get<4>(GetParam())} {} void SetUp() override { setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); config_helper_.addFilter("name: envoy.filters.http.grpc_web"); } + void initialize() override { + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + } else { + skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers_); + } + + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + + HttpIntegrationTest::initialize(); + } + void skipEncodingEmptyTrailers(SkipEncodingEmptyTrailers http2_skip_encoding_empty_trailers) { config_helper_.addRuntimeOverride( "envoy.reloadable_features.http2_skip_encoding_empty_trailers", http2_skip_encoding_empty_trailers ? "true" : "false"); } - void testBadUpstreamResponse(const std::string& start, const std::string& end, - const std::string& expected, bool remove_content_type = false) { - const auto downstream_protocol = std::get<1>(GetParam()); - const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const ContentType& content_type = std::get<3>(GetParam()); - const Accept& accept = std::get<4>(GetParam()); + void setLocalReplyConfig(const std::string& yaml) { + envoy::extensions::filters::network::http_connection_manager::v3::LocalReplyConfig + local_reply_config; + TestUtility::loadFromYaml(yaml, local_reply_config); + config_helper_.setLocalReply(local_reply_config); + } - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); + void testUpstreamDisconnect(const std::string& expected_grpc_message_value, + const std::string& local_reply_config_yaml = EMPTY_STRING) { + if (!local_reply_config_yaml.empty()) { + setLocalReplyConfig(local_reply_config_yaml); } - setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + initialize(); Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, {"request2", "trailer2"}}; - Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, - {"response2", "trailer2"}}; + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", content_type_}, + {"accept", accept_}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 1, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + + ASSERT_TRUE(fake_upstream_connection_->close()); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + + EXPECT_EQ("503", response->headers().getStatusValue()); + EXPECT_EQ(absl::StrCat(accept_, "+proto"), response->headers().getContentTypeValue()); + EXPECT_EQ(expected_grpc_message_value, response->headers().getGrpcMessageValue()); + EXPECT_EQ(0U, response->body().length()); + + codec_client_->close(); + } + + void testBadUpstreamResponse(const std::string& start, const std::string& end, + const std::string& expected, bool remove_content_type = false) { initialize(); + Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, + {"request2", "trailer2"}}; + codec_client_ = makeHttpConnection(lookupPort("http")); auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, {":path", "/test/long/url"}, {":scheme", "http"}, - {"content-type", content_type}, - {"accept", accept}, + {"content-type", content_type_}, + {"accept", accept_}, {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); codec_client_->sendData(*request_encoder_, 1, false); codec_client_->sendTrailers(*request_encoder_, request_trailers); waitForNextUpstreamRequest(); + // Sending back non gRPC-Web response. if (remove_content_type) { default_response_headers_.removeContentType(); @@ -87,7 +132,7 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, EXPECT_TRUE(response->complete()); EXPECT_EQ(expected, response->headers().getGrpcMessageValue()); EXPECT_EQ("200", response->headers().getStatusValue()); - EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); + EXPECT_EQ(absl::StrCat(accept_, "+proto"), response->headers().getContentTypeValue()); EXPECT_EQ(0U, response->body().length()); codec_client_->close(); } @@ -102,6 +147,11 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, std::get<3>(params.param) == text ? "SendText" : "SendBinary", std::get<4>(params.param) == text ? "AcceptText" : "AcceptBinary"); } + + const Envoy::Http::CodecClient::Type downstream_protocol_; + const bool http2_skip_encoding_empty_trailers_; + const ContentType content_type_; + const Accept accept_; }; INSTANTIATE_TEST_SUITE_P( @@ -115,39 +165,27 @@ INSTANTIATE_TEST_SUITE_P( GrpcWebFilterIntegrationTest::testParamsToString); TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { - const auto downstream_protocol = std::get<1>(GetParam()); - const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const ContentType& content_type = std::get<3>(GetParam()); - const Accept& accept = std::get<4>(GetParam()); - - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); - } - - setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + initialize(); Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, {"request2", "trailer2"}}; Http::TestResponseTrailerMapImpl response_trailers{{"response1", "trailer1"}, {"response2", "trailer2"}}; - initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, {":path", "/test/long/url"}, {":scheme", "http"}, - {"content-type", content_type}, - {"accept", accept}, + {"content-type", content_type_}, + {"accept", accept_}, {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); const std::string body = "hello"; const std::string encoded_body = - content_type == text ? Base64::encode(body.data(), body.length()) : body; + content_type_ == text ? Base64::encode(body.data(), body.length()) : body; codec_client_->sendData(*request_encoder_, encoded_body, false); codec_client_->sendTrailers(*request_encoder_, request_trailers); waitForNextUpstreamRequest(); @@ -163,17 +201,17 @@ TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - const auto response_body = accept == text ? Base64::decode(response->body()) : response->body(); + const auto response_body = accept_ == text ? Base64::decode(response->body()) : response->body(); EXPECT_TRUE(absl::StrContains(response_body, "response1:trailer1")); EXPECT_TRUE(absl::StrContains(response_body, "response2:trailer2")); - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { // When the downstream protocol is HTTP/1.1 we expect the trailers to be in the response-body. EXPECT_EQ(nullptr, response->trailers()); } - if (downstream_protocol == Http::CodecClient::Type::HTTP2) { - if (http2_skip_encoding_empty_trailers) { + if (downstream_protocol_ == Http::CodecClient::Type::HTTP2) { + if (http2_skip_encoding_empty_trailers_) { // When the downstream protocol is HTTP/2 and the feature-flag to skip encoding empty trailers // is turned on, expect that the trailers are included in the response-body. EXPECT_EQ(nullptr, response->trailers()); @@ -186,49 +224,20 @@ TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { } TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { - const auto downstream_protocol = std::get<1>(GetParam()); - const bool http2_skip_encoding_empty_trailers = std::get<2>(GetParam()); - const ContentType& content_type = std::get<3>(GetParam()); - const Accept& accept = std::get<4>(GetParam()); - - if (downstream_protocol == Http::CodecClient::Type::HTTP1) { - config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers); - } - - setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); - - Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, - {"request2", "trailer2"}}; - - initialize(); - codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {"content-type", content_type}, - {"accept", accept}, - {":authority", "host"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - codec_client_->sendData(*request_encoder_, 1, false); - codec_client_->sendTrailers(*request_encoder_, request_trailers); - waitForNextUpstreamRequest(); - - ASSERT_TRUE(fake_upstream_connection_->close()); - response->waitForEndStream(); - EXPECT_TRUE(response->complete()); - - EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_EQ(absl::StrCat(accept, "+proto"), response->headers().getContentTypeValue()); - EXPECT_EQ("upstream connect error or disconnect/reset before headers. reset reason: connection " - "termination", - response->headers().getGrpcMessageValue()); - EXPECT_EQ(0U, response->body().length()); + testUpstreamDisconnect("upstream connect error or disconnect/reset before headers. reset reason: " + "connection termination"); +} - codec_client_->close(); +TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnectWithLargeBody) { + // The local reply is configured to send a large (its length is greater than + // MAX_GRPC_MESSAGE_LENGTH) body. + const std::string local_reply_body = std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); + const std::string local_reply_config_yaml = fmt::format(R"EOF( +body_format: + text_format: "{}" +)EOF", + local_reply_body); + testUpstreamDisconnect(local_reply_body, local_reply_config_yaml); } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponse) { @@ -243,6 +252,7 @@ TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseWithoutContentType) { } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeEnd) { + // When we have buffered data in encoding buffer, we limit the length to MAX_GRPC_MESSAGE_LENGTH. const std::string start(MAX_GRPC_MESSAGE_LENGTH, 'a'); const std::string end(MAX_GRPC_MESSAGE_LENGTH, 'b'); testBadUpstreamResponse(start, end, /*expected=*/start); From 041096cd830dec6101a58dc61e09c4b69cdc39ee Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 15 Jan 2021 01:15:30 +0000 Subject: [PATCH 23/48] Fix comment Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index f0f34494fe3e1..9d853fdabc1fc 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -22,8 +22,7 @@ namespace GrpcWeb { namespace { -// This is arbitrarily chosen. -// TODO(dio): Make it configurable when it is needed. +// This is arbitrarily chosen. This can be made configurable when it is required. constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; // This builds grpc-message header value from encoding buffer and the last frame (when From 27070e0c0cc67ff13b874970c0f1eebec8f74724 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 15 Jan 2021 01:47:20 +0000 Subject: [PATCH 24/48] Add unit test Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 27 ++++++++------ .../filters/http/grpc_web/grpc_web_filter.h | 2 +- .../http/grpc_web/grpc_web_filter_test.cc | 37 +++++++++++++++++++ 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 9d853fdabc1fc..c481a10491950 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -1,5 +1,7 @@ #include "extensions/filters/http/grpc_web/grpc_web_filter.h" +#include + #ifndef WIN32 #include #endif @@ -91,19 +93,22 @@ bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) { return false; } -bool GrpcWebFilter::hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const { +bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType( + const Http::RequestOrResponseHeaderMap& headers) const { const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr) { - // The value of media-type is case-sensitive https://tools.ietf.org/html/rfc2616#section-3.7. - const std::string content_type_value = - absl::AsciiStrToLower(content_type->value().getStringView()); - // We ignore "parameter" value. - const std::string current_content_type = - content_type_value.substr(0, content_type_value.find_last_of(';')); + absl::string_view content_type_value = content_type->value().getStringView(); + // We ignore "parameter" value. Note that "*( ";" parameter )" indicates that there can be + // multiple parameters. + absl::string_view current_content_type = + StringUtil::rtrim(content_type_value.substr(0, content_type_value.find_first_of(';'))); // We expect only proto encoding response - // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md. - return current_content_type == Http::Headers::get().ContentTypeValues.GrpcWebProto || - current_content_type == Http::Headers::get().ContentTypeValues.GrpcWeb; + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md. And the value of media-type is + // case-sensitive https://tools.ietf.org/html/rfc2616#section-3.7. + return StringUtil::CaseInsensitiveCompare()( + current_content_type, Http::Headers::get().ContentTypeValues.GrpcWebProto) || + StringUtil::CaseInsensitiveCompare()(current_content_type, + Http::Headers::get().ContentTypeValues.GrpcWeb); } return false; } @@ -115,7 +120,7 @@ bool GrpcWebFilter::needsResponseTransformation(Http::ResponseHeaderMap& headers "envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling") && !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && !(Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && - hasGrpcWebContentType(headers)); + hasProtoEncodedGrpcWebContentType(headers)); } void GrpcWebFilter::setTransformedResponseHeaders(Buffer::Instance* data) { diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 7f65c11aa21b6..817d2db24fe5d 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -58,7 +58,7 @@ class GrpcWebFilter : public Http::StreamFilter, void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers); void setupStatTracking(const Http::RequestHeaderMap& headers); bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); - bool hasGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; + bool hasProtoEncodedGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; bool needsResponseTransformation(Http::ResponseHeaderMap& headers, bool end_stream) const; void setTransformedResponseHeaders(Buffer::Instance* data); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 81eaec3c21c98..2071b53a4bb31 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -105,6 +105,14 @@ class GrpcWebFilterTest : public testing::TestWithParam Date: Fri, 15 Jan 2021 01:51:40 +0000 Subject: [PATCH 25/48] Re-arrange Signed-off-by: Dhi Aurrahman --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3768bbc6867f5..a8a3d84f489ff 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -22,8 +22,8 @@ Bug Fixes *Changes expected to improve the state of the world and are unlikely to have negative effects* * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. -* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. +* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort. Removed Config or Runtime From 8c19be20fd97292cc46a598a0fe1c67b60b9f549 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 15 Jan 2021 05:30:08 +0000 Subject: [PATCH 26/48] Don;t use deprecated config Signed-off-by: Dhi Aurrahman --- .../grpc_web/grpc_web_filter_integration_test.cc | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 280ed3909e243..e2ead3dac5c71 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -25,10 +25,7 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: GrpcWebFilterIntegrationTest() - : HttpIntegrationTest(std::get<1>(GetParam()), std::get<0>(GetParam())), - downstream_protocol_{std::get<1>(GetParam())}, - http2_skip_encoding_empty_trailers_{std::get<2>(GetParam())}, - content_type_{std::get<3>(GetParam())}, accept_{std::get<4>(GetParam())} {} + : HttpIntegrationTest(std::get<1>(GetParam()), std::get<0>(GetParam())) {} void SetUp() override { setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); @@ -148,10 +145,10 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, std::get<4>(params.param) == text ? "AcceptText" : "AcceptBinary"); } - const Envoy::Http::CodecClient::Type downstream_protocol_; - const bool http2_skip_encoding_empty_trailers_; - const ContentType content_type_; - const Accept accept_; + const Envoy::Http::CodecClient::Type downstream_protocol_{std::get<1>(GetParam())}; + const bool http2_skip_encoding_empty_trailers_{std::get<2>(GetParam())}; + const ContentType content_type_{std::get<3>(GetParam())}; + const Accept accept_{std::get<4>(GetParam())}; }; INSTANTIATE_TEST_SUITE_P( @@ -234,7 +231,8 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnectWithLargeBody) { const std::string local_reply_body = std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); const std::string local_reply_config_yaml = fmt::format(R"EOF( body_format: - text_format: "{}" + text_format_source: + inline_string: "{}" )EOF", local_reply_body); testUpstreamDisconnect(local_reply_body, local_reply_config_yaml); From 14293cedbcc5976c0221dac8ec39b1c8de3b2d80 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 19 Jan 2021 20:53:09 +0000 Subject: [PATCH 27/48] Add more comments Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index c481a10491950..7904cf255bece 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -33,6 +33,7 @@ constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; // we send it all. std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { Buffer::OwnedImpl buffer; + // In the case of local reply, "buffered" is nullptr and we only have "last" data filled. if (buffered != nullptr) { ASSERT(buffered->length() <= MAX_GRPC_MESSAGE_LENGTH); buffer.add(*buffered); @@ -40,6 +41,7 @@ std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* if (last != nullptr) { uint64_t needed = last->length(); + // When we have buffered data (from encoding buffer, we limit the length of the final buffer). if (buffered != nullptr && (buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - buffer.length()); } From 291590ae04b9d9c51b8503d51b72d652d019db91 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 19 Jan 2021 20:55:05 +0000 Subject: [PATCH 28/48] Re-phrase Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 7904cf255bece..760bf2a475dfd 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -33,7 +33,7 @@ constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; // we send it all. std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { Buffer::OwnedImpl buffer; - // In the case of local reply, "buffered" is nullptr and we only have "last" data filled. + // In the case of local reply, "buffered" is nullptr and we only have filled "last" data. if (buffered != nullptr) { ASSERT(buffered->length() <= MAX_GRPC_MESSAGE_LENGTH); buffer.add(*buffered); @@ -41,7 +41,7 @@ std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* if (last != nullptr) { uint64_t needed = last->length(); - // When we have buffered data (from encoding buffer, we limit the length of the final buffer). + // When we have buffered data (from encoding buffer), we limit the length of the final buffer. if (buffered != nullptr && (buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - buffer.length()); } From 1f63bcd91a58426403a8d8cb1761f0301a558386 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 19 Jan 2021 20:55:42 +0000 Subject: [PATCH 29/48] Typo Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 760bf2a475dfd..370785f41f131 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -41,7 +41,7 @@ std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* if (last != nullptr) { uint64_t needed = last->length(); - // When we have buffered data (from encoding buffer), we limit the length of the final buffer. + // When we have buffered data (from encoding buffer), we limit the final buffer length. if (buffered != nullptr && (buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - buffer.length()); } From a50f51dd33e998734bd45f44111575d8dcf7c830 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 19 Jan 2021 22:43:47 +0000 Subject: [PATCH 30/48] Try to address review comments Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 64 ++++++++++--------- .../filters/http/grpc_web/grpc_web_filter.h | 7 +- 2 files changed, 39 insertions(+), 32 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 370785f41f131..785049fa33762 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -27,31 +27,34 @@ namespace { // This is arbitrarily chosen. This can be made configurable when it is required. constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; -// This builds grpc-message header value from encoding buffer and the last frame (when -// end_stream=true). When we have buffered data in encoding buffer, we limit the length of -// grpc-message to be smaller than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" data, -// we send it all. -std::string buildGrpcMessage(const Buffer::Instance* buffered, Buffer::Instance* last) { - Buffer::OwnedImpl buffer; - // In the case of local reply, "buffered" is nullptr and we only have filled "last" data. - if (buffered != nullptr) { - ASSERT(buffered->length() <= MAX_GRPC_MESSAGE_LENGTH); - buffer.add(*buffered); +void mergeEncodingBufferAndLastData(Buffer::OwnedImpl& output, + const Buffer::Instance* encoding_buffer, + Buffer::Instance* last_data) { + // In the case of local reply, "encoding_buffer" is nullptr and we only have filled "last_data". + if (encoding_buffer != nullptr) { + ASSERT(encoding_buffer->length() <= MAX_GRPC_MESSAGE_LENGTH); + output.add(*encoding_buffer); } - if (last != nullptr) { - uint64_t needed = last->length(); + if (last_data != nullptr) { + uint64_t needed = last_data->length(); // When we have buffered data (from encoding buffer), we limit the final buffer length. - if (buffered != nullptr && (buffer.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { - needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - buffer.length()); + if (encoding_buffer != nullptr && (output.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { + needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - output.length()); } - buffer.move(*last, needed); - last->drain(last->length()); + output.move(*last_data, needed); + last_data->drain(last_data->length()); } +} - const uint64_t message_length = buffer.length(); +// This builds grpc-message header value from a merged data (built from encoding buffer and the last +// frame, when end_stream=true). When we have buffered data in encoding buffer, we limit the length +// of grpc-message to be smaller than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" +// data, we send it all. +std::string buildGrpcMessage(Buffer::Instance& merged_data) { + const uint64_t message_length = merged_data.length(); std::string message(message_length, 0); - buffer.copyOut(0, message_length, message.data()); + merged_data.copyOut(0, message_length, message.data()); return Http::Utility::PercentEncoding::encode(message); } @@ -116,8 +119,8 @@ bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType( } // If response headers do not contain gRPC or gRPC-Web response headers, it needs transformation. -bool GrpcWebFilter::needsResponseTransformation(Http::ResponseHeaderMap& headers, - bool end_stream) const { +bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers, + bool end_stream) const { return Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling") && !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && @@ -125,7 +128,7 @@ bool GrpcWebFilter::needsResponseTransformation(Http::ResponseHeaderMap& headers hasProtoEncodedGrpcWebContentType(headers)); } -void GrpcWebFilter::setTransformedResponseHeaders(Buffer::Instance* data) { +void GrpcWebFilter::setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (encoding_buffer != nullptr && encoding_buffer->length() > MAX_GRPC_MESSAGE_LENGTH) { @@ -137,7 +140,9 @@ void GrpcWebFilter::setTransformedResponseHeaders(Buffer::Instance* data) { }); } - const auto message = buildGrpcMessage(encoding_buffer, data); + Buffer::OwnedImpl merged_data; + mergeEncodingBufferAndLastData(merged_data, encoding_buffer, data); + const std::string message = buildGrpcMessage(merged_data); if (encoding_buffer != nullptr) { encoder_callbacks_->modifyEncodingBuffer( @@ -249,7 +254,8 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& chargeStat(headers); } - needs_response_transformation_ = needsResponseTransformation(headers, end_stream); + needs_transformation_for_non_proto_encoded_response_ = + needsTransformationForNonProtoEncodedResponse(headers, end_stream); if (is_text_response_) { headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebTextProto); @@ -257,7 +263,7 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto); } - if (end_stream || !needs_response_transformation_) { + if (end_stream || !needs_transformation_for_non_proto_encoded_response_) { return Http::FilterHeadersStatus::Continue; } @@ -273,7 +279,7 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en // When the upstream response (this is also relevant for local reply, since gRPC-Web request is // not a gRPC request which makes the local reply's is_grpc_request set to false) is not a gRPC // response, we set the "grpc-message" header with the upstream body content. - if (needs_response_transformation_) { + if (needs_transformation_for_non_proto_encoded_response_) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (!end_stream) { if (encoding_buffer != nullptr && encoding_buffer->length() >= MAX_GRPC_MESSAGE_LENGTH) { @@ -283,8 +289,8 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en } ASSERT(response_headers_ != nullptr); - needs_response_transformation_ = false; - setTransformedResponseHeaders(&data); + needs_transformation_for_non_proto_encoded_response_ = false; + setTransformedNonProtoEncodedResponseHeaders(&data); return Http::FilterDataStatus::Continue; } @@ -326,8 +332,8 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::ResponseTrailerMa chargeStat(trailers); } - if (needs_response_transformation_) { - setTransformedResponseHeaders(nullptr); + if (needs_transformation_for_non_proto_encoded_response_) { + setTransformedNonProtoEncodedResponseHeaders(nullptr); return Http::FilterTrailersStatus::Continue; } diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 817d2db24fe5d..f75a746f2a310 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -59,8 +59,9 @@ class GrpcWebFilter : public Http::StreamFilter, void setupStatTracking(const Http::RequestHeaderMap& headers); bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); bool hasProtoEncodedGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; - bool needsResponseTransformation(Http::ResponseHeaderMap& headers, bool end_stream) const; - void setTransformedResponseHeaders(Buffer::Instance* data); + bool needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers, + bool end_stream) const; + void setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data); static const uint8_t GRPC_WEB_TRAILER; const absl::flat_hash_set& gRpcWebContentTypes() const; @@ -70,7 +71,7 @@ class GrpcWebFilter : public Http::StreamFilter, Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; bool is_text_request_{}; bool is_text_response_{}; - bool needs_response_transformation_{}; + bool needs_transformation_for_non_proto_encoded_response_{}; Buffer::OwnedImpl decoding_buffer_; Grpc::Decoder decoder_; absl::optional request_stat_names_; From 9beda45c3e00a9b7771091859e421d4b7b3c2670 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 19 Jan 2021 22:44:55 +0000 Subject: [PATCH 31/48] Remove Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 785049fa33762..fcf6869795da8 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -1,7 +1,5 @@ #include "extensions/filters/http/grpc_web/grpc_web_filter.h" -#include - #ifndef WIN32 #include #endif From 28564b2e68f13cfd211743adf7fd0be0f50e8875 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 19 Jan 2021 23:03:22 +0000 Subject: [PATCH 32/48] Fix comment Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index fcf6869795da8..d05a974c898af 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -25,6 +25,8 @@ namespace { // This is arbitrarily chosen. This can be made configurable when it is required. constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; +// When we have buffered data in encoding buffer, we limit the length of the output to be smaller +// than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" data, we send it all. void mergeEncodingBufferAndLastData(Buffer::OwnedImpl& output, const Buffer::Instance* encoding_buffer, Buffer::Instance* last_data) { @@ -46,9 +48,7 @@ void mergeEncodingBufferAndLastData(Buffer::OwnedImpl& output, } // This builds grpc-message header value from a merged data (built from encoding buffer and the last -// frame, when end_stream=true). When we have buffered data in encoding buffer, we limit the length -// of grpc-message to be smaller than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" -// data, we send it all. +// frame, when end_stream=true). std::string buildGrpcMessage(Buffer::Instance& merged_data) { const uint64_t message_length = merged_data.length(); std::string message(message_length, 0); From b3642bea2a84651ef096d031f99fcb7e2a980725 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 20 Jan 2021 06:19:58 +0000 Subject: [PATCH 33/48] Tidy Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 72 +++++++++---------- .../filters/http/grpc_web/grpc_web_filter.h | 2 + .../http/grpc_web/grpc_web_filter_test.cc | 61 ++++++++++++++++ 3 files changed, 97 insertions(+), 38 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index d05a974c898af..30cec783a8793 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -25,30 +25,8 @@ namespace { // This is arbitrarily chosen. This can be made configurable when it is required. constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; -// When we have buffered data in encoding buffer, we limit the length of the output to be smaller -// than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" data, we send it all. -void mergeEncodingBufferAndLastData(Buffer::OwnedImpl& output, - const Buffer::Instance* encoding_buffer, - Buffer::Instance* last_data) { - // In the case of local reply, "encoding_buffer" is nullptr and we only have filled "last_data". - if (encoding_buffer != nullptr) { - ASSERT(encoding_buffer->length() <= MAX_GRPC_MESSAGE_LENGTH); - output.add(*encoding_buffer); - } - - if (last_data != nullptr) { - uint64_t needed = last_data->length(); - // When we have buffered data (from encoding buffer), we limit the final buffer length. - if (encoding_buffer != nullptr && (output.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { - needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - output.length()); - } - output.move(*last_data, needed); - last_data->drain(last_data->length()); - } -} - // This builds grpc-message header value from a merged data (built from encoding buffer and the last -// frame, when end_stream=true). +// frame when end_stream=true). std::string buildGrpcMessage(Buffer::Instance& merged_data) { const uint64_t message_length = merged_data.length(); std::string message(message_length, 0); @@ -126,30 +104,48 @@ bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::Response hasProtoEncodedGrpcWebContentType(headers)); } -void GrpcWebFilter::setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data) { +void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& output, + Buffer::Instance* last_data) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); + if (encoding_buffer != nullptr) { + if (encoding_buffer->length() > MAX_GRPC_MESSAGE_LENGTH) { + encoder_callbacks_->modifyEncodingBuffer([](Buffer::Instance& buffered) { + Buffer::OwnedImpl needed_buffer; + needed_buffer.move(buffered, MAX_GRPC_MESSAGE_LENGTH); + buffered.drain(buffered.length()); + buffered.move(needed_buffer); + }); + } - if (encoding_buffer != nullptr && encoding_buffer->length() > MAX_GRPC_MESSAGE_LENGTH) { - encoder_callbacks_->modifyEncodingBuffer([](Buffer::Instance& buffered) { - Buffer::OwnedImpl needed_buffer; - needed_buffer.move(buffered, MAX_GRPC_MESSAGE_LENGTH); - buffered.drain(buffered.length()); - buffered.move(needed_buffer); - }); - } - - Buffer::OwnedImpl merged_data; - mergeEncodingBufferAndLastData(merged_data, encoding_buffer, data); - const std::string message = buildGrpcMessage(merged_data); + ASSERT(encoding_buffer->length() <= MAX_GRPC_MESSAGE_LENGTH); + output.add(*encoding_buffer); - if (encoding_buffer != nullptr) { encoder_callbacks_->modifyEncodingBuffer( [](Buffer::Instance& buffered) { buffered.drain(buffered.length()); }); } + // In the case of local reply, "encoding_buffer" is nullptr and we only have filled "last_data". + if (last_data != nullptr) { + uint64_t needed = last_data->length(); + // When we have buffered data (from encoding buffer), we limit the final buffer length. + if (encoding_buffer != nullptr && (output.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { + needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - output.length()); + } + output.move(*last_data, needed); + last_data->drain(last_data->length()); + } +} + +void GrpcWebFilter::setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data) { + Buffer::OwnedImpl merged_data; + // When we have buffered data in encoding buffer, we limit the length of the output to be smaller + // than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" data, we send it all. + mergeAndLimitNonProtoEncodedResponseData(merged_data, data); + + const std::string grpc_message = buildGrpcMessage(merged_data); + response_headers_->setGrpcMessage(grpc_message); response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus( enumToInt(Http::Utility::getResponseStatus(*response_headers_)))); - response_headers_->setGrpcMessage(message); response_headers_->setContentLength(0); } diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index f75a746f2a310..d610abdc7cc75 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -61,6 +61,8 @@ class GrpcWebFilter : public Http::StreamFilter, bool hasProtoEncodedGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; bool needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers, bool end_stream) const; + void mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& output, + Buffer::Instance* last_data); void setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data); static const uint8_t GRPC_WEB_TRAILER; diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 2071b53a4bb31..acc711c2f9cb3 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -1,3 +1,5 @@ +#include + #include "envoy/http/filter.h" #include "common/buffer/buffer_impl.h" @@ -113,6 +115,31 @@ class GrpcWebFilterTest : public testing::TestWithParam cb) { cb(encoded_buffer); }; + EXPECT_CALL(encoder_callbacks_, encodingBuffer).WillRepeatedly(Return(&encoded_buffer)); + EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) + .WillRepeatedly(Invoke(on_modify_encoding_buffer)); + } + Buffer::OwnedImpl output; + filter_.mergeAndLimitNonProtoEncodedResponseData(output, last_data); + EXPECT_EQ(expected_merged_length, output.length()); + if (has_filled_encoding_buffer) { + EXPECT_EQ(0U, encoded_buffer.length()); + } + if (last_data != nullptr) { + EXPECT_EQ(0U, last_data->length()); + } + } + Stats::TestUtil::TestSymbolTable symbol_table_; Grpc::ContextImpl grpc_context_; GrpcWebFilter filter_; @@ -169,6 +196,40 @@ TEST_F(GrpcWebFilterTest, UnexpectedGrpcWebProtoContentType) { isProtoEncodedGrpcWebContentType(Http::Headers::get().ContentTypeValues.GrpcWebTextProto)); } +TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseData) { + const std::string encoded_buffer(100, 'a'); + Buffer::OwnedImpl last_data; + last_data.add(std::string(100, 'a')); + expectMergedAndLimitedResponseData(encoded_buffer, &last_data, + /*expected_merged_length=*/encoded_buffer.length() + + last_data.length()); +} + +TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithLargeEncodingBuffer) { + const std::string encoded_buffer(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); + Buffer::OwnedImpl last_data; + last_data.add(std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a')); + // Since the buffered data in encoding buffer is larger than MAX_GRPC_MESSAGE_LENGTH, the output + // length is limited to MAX_GRPC_MESSAGE_LENGTH. + expectMergedAndLimitedResponseData(encoded_buffer, &last_data, + /*expected_merged_length=*/MAX_GRPC_MESSAGE_LENGTH); +} + +TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithNullEncodingBuffer) { + Buffer::OwnedImpl last_data; + last_data.add(std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a')); + // If we don't have buffered data in encoding buffer, the merged data will be the same as last + // data. + expectMergedAndLimitedResponseData(EMPTY_STRING, &last_data, + /*expected_merged_length=*/last_data.length()); +} + +TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithNoEncodingBufferAndLastData) { + // If we don't have buffered data in encoding buffer and no last data either, the output length is + // zero. + expectMergedAndLimitedResponseData(EMPTY_STRING, nullptr, /*expected_merged_length=*/0U); +} + TEST_F(GrpcWebFilterTest, UnsupportedContentType) { Buffer::OwnedImpl data; request_headers_.addCopy(Http::Headers::get().ContentType, "unsupported"); From 6e1efc7f4a324e6ca3bbf00530b62705e9776f64 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 20 Jan 2021 06:23:20 +0000 Subject: [PATCH 34/48] Fix Signed-off-by: Dhi Aurrahman --- test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index acc711c2f9cb3..034449c018b2f 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -1,5 +1,3 @@ -#include - #include "envoy/http/filter.h" #include "common/buffer/buffer_impl.h" From 8fc318a86a5c649b9399ad7f8b854facc582c735 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 20 Jan 2021 22:31:31 +0000 Subject: [PATCH 35/48] Rename Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 33 +++++++++---------- .../grpc_web_filter_integration_test.cc | 18 +++++----- .../http/grpc_web/grpc_web_filter_test.cc | 32 +++++++++--------- 3 files changed, 41 insertions(+), 42 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 30cec783a8793..34f6fd36cf731 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -23,14 +23,13 @@ namespace GrpcWeb { namespace { // This is arbitrarily chosen. This can be made configurable when it is required. -constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; +constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; -// This builds grpc-message header value from a merged data (built from encoding buffer and the last -// frame when end_stream=true). -std::string buildGrpcMessage(Buffer::Instance& merged_data) { - const uint64_t message_length = merged_data.length(); +// This builds grpc-message header value from body data. +std::string buildGrpcMessage(Buffer::Instance& body_data) { + const uint64_t message_length = body_data.length(); std::string message(message_length, 0); - merged_data.copyOut(0, message_length, message.data()); + body_data.copyOut(0, message_length, message.data()); return Http::Utility::PercentEncoding::encode(message); } @@ -108,18 +107,17 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& Buffer::Instance* last_data) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (encoding_buffer != nullptr) { - if (encoding_buffer->length() > MAX_GRPC_MESSAGE_LENGTH) { - encoder_callbacks_->modifyEncodingBuffer([](Buffer::Instance& buffered) { + if (encoding_buffer->length() > MAX_BUFFERED_PLAINTEXT_LENGTH) { + encoder_callbacks_->modifyEncodingBuffer([&output](Buffer::Instance& buffered) { Buffer::OwnedImpl needed_buffer; - needed_buffer.move(buffered, MAX_GRPC_MESSAGE_LENGTH); + needed_buffer.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH); buffered.drain(buffered.length()); - buffered.move(needed_buffer); + output.move(needed_buffer); }); + } else { + output.add(*encoding_buffer); } - ASSERT(encoding_buffer->length() <= MAX_GRPC_MESSAGE_LENGTH); - output.add(*encoding_buffer); - encoder_callbacks_->modifyEncodingBuffer( [](Buffer::Instance& buffered) { buffered.drain(buffered.length()); }); } @@ -128,8 +126,8 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& if (last_data != nullptr) { uint64_t needed = last_data->length(); // When we have buffered data (from encoding buffer), we limit the final buffer length. - if (encoding_buffer != nullptr && (output.length() + needed) >= MAX_GRPC_MESSAGE_LENGTH) { - needed = std::min(needed, MAX_GRPC_MESSAGE_LENGTH - output.length()); + if (encoding_buffer != nullptr && (output.length() + needed) >= MAX_BUFFERED_PLAINTEXT_LENGTH) { + needed = std::min(needed, MAX_BUFFERED_PLAINTEXT_LENGTH - output.length()); } output.move(*last_data, needed); last_data->drain(last_data->length()); @@ -139,7 +137,7 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& void GrpcWebFilter::setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data) { Buffer::OwnedImpl merged_data; // When we have buffered data in encoding buffer, we limit the length of the output to be smaller - // than MAX_GRPC_MESSAGE_LENGTH. However, when we only have "last" data, we send it all. + // than MAX_BUFFERED_PLAINTEXT_LENGTH. However, when we only have "last" data, we send it all. mergeAndLimitNonProtoEncodedResponseData(merged_data, data); const std::string grpc_message = buildGrpcMessage(merged_data); @@ -276,7 +274,8 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en if (needs_transformation_for_non_proto_encoded_response_) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (!end_stream) { - if (encoding_buffer != nullptr && encoding_buffer->length() >= MAX_GRPC_MESSAGE_LENGTH) { + if (encoding_buffer != nullptr && + encoding_buffer->length() >= MAX_BUFFERED_PLAINTEXT_LENGTH) { return Http::FilterDataStatus::StopIterationNoBuffer; } return Http::FilterDataStatus::StopIterationAndBuffer; diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index e2ead3dac5c71..ab7a397d19d5e 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -13,7 +13,7 @@ namespace { constexpr absl::string_view text{"application/grpc-web-text"}; constexpr absl::string_view binary{"application/grpc-web"}; -constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; +constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; using SkipEncodingEmptyTrailers = bool; using ContentType = std::string; @@ -227,8 +227,8 @@ TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnect) { TEST_P(GrpcWebFilterIntegrationTest, UpstreamDisconnectWithLargeBody) { // The local reply is configured to send a large (its length is greater than - // MAX_GRPC_MESSAGE_LENGTH) body. - const std::string local_reply_body = std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); + // MAX_BUFFERED_PLAINTEXT_LENGTH) body. + const std::string local_reply_body = std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); const std::string local_reply_config_yaml = fmt::format(R"EOF( body_format: text_format_source: @@ -250,16 +250,16 @@ TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseWithoutContentType) { } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeEnd) { - // When we have buffered data in encoding buffer, we limit the length to MAX_GRPC_MESSAGE_LENGTH. - const std::string start(MAX_GRPC_MESSAGE_LENGTH, 'a'); - const std::string end(MAX_GRPC_MESSAGE_LENGTH, 'b'); + // When we have buffered data in encoding buffer, we limit the length to MAX_BUFFERED_PLAINTEXT_LENGTH. + const std::string start(MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); + const std::string end(MAX_BUFFERED_PLAINTEXT_LENGTH, 'b'); testBadUpstreamResponse(start, end, /*expected=*/start); } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeFirst) { - const std::string start(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); - const std::string end(MAX_GRPC_MESSAGE_LENGTH, 'b'); - testBadUpstreamResponse(start, end, /*expected=*/start.substr(0, MAX_GRPC_MESSAGE_LENGTH)); + const std::string start(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); + const std::string end(MAX_BUFFERED_PLAINTEXT_LENGTH, 'b'); + testBadUpstreamResponse(start, end, /*expected=*/start.substr(0, MAX_BUFFERED_PLAINTEXT_LENGTH)); } } // namespace diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 034449c018b2f..bea7733f1a2d7 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -45,7 +45,7 @@ const char INVALID_B64_MESSAGE[] = "****"; const size_t INVALID_B64_MESSAGE_SIZE = sizeof(INVALID_B64_MESSAGE) - 1; const char TRAILERS[] = "\x80\x00\x00\x00\x20grpc-status:0\r\ngrpc-message:ok\r\n"; const size_t TRAILERS_SIZE = sizeof(TRAILERS) - 1; -constexpr uint64_t MAX_GRPC_MESSAGE_LENGTH = 16384; +constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; } // namespace @@ -204,18 +204,18 @@ TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseData) { } TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithLargeEncodingBuffer) { - const std::string encoded_buffer(2 * MAX_GRPC_MESSAGE_LENGTH, 'a'); + const std::string encoded_buffer(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); Buffer::OwnedImpl last_data; - last_data.add(std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a')); - // Since the buffered data in encoding buffer is larger than MAX_GRPC_MESSAGE_LENGTH, the output - // length is limited to MAX_GRPC_MESSAGE_LENGTH. + last_data.add(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); + // Since the buffered data in encoding buffer is larger than MAX_BUFFERED_PLAINTEXT_LENGTH, the output + // length is limited to MAX_BUFFERED_PLAINTEXT_LENGTH. expectMergedAndLimitedResponseData(encoded_buffer, &last_data, - /*expected_merged_length=*/MAX_GRPC_MESSAGE_LENGTH); + /*expected_merged_length=*/MAX_BUFFERED_PLAINTEXT_LENGTH); } TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithNullEncodingBuffer) { Buffer::OwnedImpl last_data; - last_data.add(std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a')); + last_data.add(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); // If we don't have buffered data in encoding buffer, the merged data will be the same as last // data. expectMergedAndLimitedResponseData(EMPTY_STRING, &last_data, @@ -309,11 +309,11 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { buffer->add(data); EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); - TestUtility::feedBufferWithRandomCharacters(data, MAX_GRPC_MESSAGE_LENGTH); + TestUtility::feedBufferWithRandomCharacters(data, MAX_BUFFERED_PLAINTEXT_LENGTH); const std::string expected_grpc_message = - absl::StrCat("hellohello", data.toString()).substr(0, MAX_GRPC_MESSAGE_LENGTH); + absl::StrCat("hellohello", data.toString()).substr(0, MAX_BUFFERED_PLAINTEXT_LENGTH); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); - EXPECT_EQ(MAX_GRPC_MESSAGE_LENGTH, + EXPECT_EQ(MAX_BUFFERED_PLAINTEXT_LENGTH, response_headers.get_(Http::Headers::get().GrpcMessage).length()); EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); } @@ -327,8 +327,8 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeEncodingBuffer) EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl encoded_buffer; - encoded_buffer.add(std::string(2 * MAX_GRPC_MESSAGE_LENGTH, 'a')); - // The encoding buffer is filled with data more than MAX_GRPC_MESSAGE_LENGTH. + encoded_buffer.add(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); + // The encoding buffer is filled with data more than MAX_BUFFERED_PLAINTEXT_LENGTH. auto on_modify_encoding_buffer = [&encoded_buffer](std::function cb) { cb(encoded_buffer); }; @@ -341,7 +341,7 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeEncodingBuffer) EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.encodeData(encoded_buffer, false)); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(encoded_buffer, true)); - EXPECT_EQ(MAX_GRPC_MESSAGE_LENGTH, + EXPECT_EQ(MAX_BUFFERED_PLAINTEXT_LENGTH, response_headers.get_(Http::Headers::get().GrpcMessage).length()); } @@ -354,12 +354,12 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithLargeLastData) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data; - // The last data length is set to be bigger than "MAX_GRPC_MESSAGE_LENGTH". - const std::string expected_grpc_message = std::string(MAX_GRPC_MESSAGE_LENGTH + 1, 'a'); + // The last data length is set to be bigger than "MAX_BUFFERED_PLAINTEXT_LENGTH". + const std::string expected_grpc_message = std::string(MAX_BUFFERED_PLAINTEXT_LENGTH + 1, 'a'); data.add(expected_grpc_message); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data, true)); // The grpc-message value length is the same as the last sent buffer. - EXPECT_EQ(MAX_GRPC_MESSAGE_LENGTH + 1, + EXPECT_EQ(MAX_BUFFERED_PLAINTEXT_LENGTH + 1, response_headers.get_(Http::Headers::get().GrpcMessage).length()); EXPECT_EQ(expected_grpc_message, response_headers.get_(Http::Headers::get().GrpcMessage)); } From ba3441c5462f843951e50e451853ae8763e9ae47 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 20 Jan 2021 23:43:03 +0000 Subject: [PATCH 36/48] Review comments Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 5 +---- .../extensions/filters/http/grpc_web/grpc_web_filter_test.cc | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 34f6fd36cf731..04af73c26417a 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -109,10 +109,7 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& if (encoding_buffer != nullptr) { if (encoding_buffer->length() > MAX_BUFFERED_PLAINTEXT_LENGTH) { encoder_callbacks_->modifyEncodingBuffer([&output](Buffer::Instance& buffered) { - Buffer::OwnedImpl needed_buffer; - needed_buffer.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH); - buffered.drain(buffered.length()); - output.move(needed_buffer); + output.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH); }); } else { output.add(*encoding_buffer); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index bea7733f1a2d7..95d5e556b1880 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -207,8 +207,8 @@ TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithLargeEncod const std::string encoded_buffer(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); Buffer::OwnedImpl last_data; last_data.add(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); - // Since the buffered data in encoding buffer is larger than MAX_BUFFERED_PLAINTEXT_LENGTH, the output - // length is limited to MAX_BUFFERED_PLAINTEXT_LENGTH. + // Since the buffered data in encoding buffer is larger than MAX_BUFFERED_PLAINTEXT_LENGTH, the + // output length is limited to MAX_BUFFERED_PLAINTEXT_LENGTH. expectMergedAndLimitedResponseData(encoded_buffer, &last_data, /*expected_merged_length=*/MAX_BUFFERED_PLAINTEXT_LENGTH); } From 4889c28faa9e8300f7c00717f11fea1b26eb8f3f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 20 Jan 2021 23:49:08 +0000 Subject: [PATCH 37/48] Fix Signed-off-by: Dhi Aurrahman --- docs/root/version_history/current.rst | 2 +- source/common/runtime/runtime_features.cc | 2 +- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 5 +++-- .../extensions/filters/http/grpc_web/grpc_web_filter_test.cc | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index e96ea3dd72926..324b60e30302a 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,7 +23,7 @@ Bug Fixes * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. -* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling` to false. +* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9da464b6141f3..05a05787ae5b9 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -67,7 +67,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.fix_wildcard_matching", "envoy.reloadable_features.fixed_connection_close", - "envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling", + "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.health_check.graceful_goaway_handling", "envoy.reloadable_features.http_default_alpn", diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 04af73c26417a..0d3af5a5572a9 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -22,7 +22,8 @@ namespace GrpcWeb { namespace { -// This is arbitrarily chosen. This can be made configurable when it is required. +// This is the maximum buffered plaintext data length to be converted into grpc-message header. This +// is arbitrarily chosen. This can be made configurable when it is required. constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; // This builds grpc-message header value from body data. @@ -97,7 +98,7 @@ bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType( bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers, bool end_stream) const { return Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling") && + "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling") && !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && !(Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && hasProtoEncodedGrpcWebContentType(headers)); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 95d5e556b1880..04a028007f3ec 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -389,7 +389,7 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithTrailers) { TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextSkipTransformation) { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.grpc_web_fix_non_grpc_response_handling", "false"}}); + {{"envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling", "false"}}); Http::TestRequestHeaderMapImpl request_headers{ {"content-type", Http::Headers::get().ContentTypeValues.GrpcWebText}, From d7aad6f79c3d4eda43f3e39e3878cf00074e6dfd Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 00:46:40 +0000 Subject: [PATCH 38/48] Fix tests Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 14 ++++---------- .../filters/http/grpc_web/grpc_web_filter_test.cc | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 0d3af5a5572a9..a84dc204ed66b 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -108,16 +108,10 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& Buffer::Instance* last_data) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (encoding_buffer != nullptr) { - if (encoding_buffer->length() > MAX_BUFFERED_PLAINTEXT_LENGTH) { - encoder_callbacks_->modifyEncodingBuffer([&output](Buffer::Instance& buffered) { - output.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH); - }); - } else { - output.add(*encoding_buffer); - } - - encoder_callbacks_->modifyEncodingBuffer( - [](Buffer::Instance& buffered) { buffered.drain(buffered.length()); }); + encoder_callbacks_->modifyEncodingBuffer([&output](Buffer::Instance& buffered) { + output.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH); + buffered.drain(buffered.length()); + }); } // In the case of local reply, "encoding_buffer" is nullptr and we only have filled "last_data". diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 04a028007f3ec..9a6a76f4350d3 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -304,6 +304,13 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { Buffer::OwnedImpl data("hello"); Buffer::InstancePtr buffer(new Buffer::OwnedImpl("hello")); EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); + auto on_modify_encoding_buffer = [encoded_buffer = + buffer.get()](std::function cb) { + cb(*encoded_buffer); + }; + EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) + .WillRepeatedly(Invoke(on_modify_encoding_buffer)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); buffer->add(data); @@ -375,6 +382,13 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForTextWithTrailers) { Buffer::OwnedImpl data("hello"); Buffer::InstancePtr buffer(new Buffer::OwnedImpl("hello")); EXPECT_CALL(encoder_callbacks_, encodingBuffer()).WillRepeatedly(Return(buffer.get())); + auto on_modify_encoding_buffer = [encoded_buffer = + buffer.get()](std::function cb) { + cb(*encoded_buffer); + }; + EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) + .WillRepeatedly(Invoke(on_modify_encoding_buffer)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); buffer->add(data); From fff111c83bbba512020754e0cf80b7332de5a56a Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 01:55:57 +0000 Subject: [PATCH 39/48] Tidy Signed-off-by: Dhi Aurrahman --- .../http/grpc_web/grpc_web_filter_test.cc | 42 ++++++++----------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 9a6a76f4350d3..0d457997a4db0 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -113,25 +113,22 @@ class GrpcWebFilterTest : public testing::TestWithParam cb) { cb(encoded_buffer); }; - EXPECT_CALL(encoder_callbacks_, encodingBuffer).WillRepeatedly(Return(&encoded_buffer)); + if (encoded_buffer != nullptr) { + auto on_modify_encoding_buffer = [encoded_buffer](std::function cb) { + cb(*encoded_buffer); + }; + EXPECT_CALL(encoder_callbacks_, encodingBuffer).WillRepeatedly(Return(encoded_buffer)); EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) .WillRepeatedly(Invoke(on_modify_encoding_buffer)); } Buffer::OwnedImpl output; filter_.mergeAndLimitNonProtoEncodedResponseData(output, last_data); EXPECT_EQ(expected_merged_length, output.length()); - if (has_filled_encoding_buffer) { - EXPECT_EQ(0U, encoded_buffer.length()); + if (encoded_buffer != nullptr) { + EXPECT_EQ(0U, encoded_buffer->length()); } if (last_data != nullptr) { EXPECT_EQ(0U, last_data->length()); @@ -195,37 +192,34 @@ TEST_F(GrpcWebFilterTest, UnexpectedGrpcWebProtoContentType) { } TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseData) { - const std::string encoded_buffer(100, 'a'); - Buffer::OwnedImpl last_data; - last_data.add(std::string(100, 'a')); - expectMergedAndLimitedResponseData(encoded_buffer, &last_data, + Buffer::OwnedImpl encoded_buffer(std::string(100, 'a')); + Buffer::OwnedImpl last_data(std::string(100, 'a')); + expectMergedAndLimitedResponseData(&encoded_buffer, &last_data, /*expected_merged_length=*/encoded_buffer.length() + last_data.length()); } TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithLargeEncodingBuffer) { - const std::string encoded_buffer(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); - Buffer::OwnedImpl last_data; - last_data.add(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); + Buffer::OwnedImpl encoded_buffer(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); + Buffer::OwnedImpl last_data(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); // Since the buffered data in encoding buffer is larger than MAX_BUFFERED_PLAINTEXT_LENGTH, the // output length is limited to MAX_BUFFERED_PLAINTEXT_LENGTH. - expectMergedAndLimitedResponseData(encoded_buffer, &last_data, + expectMergedAndLimitedResponseData(&encoded_buffer, &last_data, /*expected_merged_length=*/MAX_BUFFERED_PLAINTEXT_LENGTH); } TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithNullEncodingBuffer) { - Buffer::OwnedImpl last_data; - last_data.add(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); + Buffer::OwnedImpl last_data(std::string(2 * MAX_BUFFERED_PLAINTEXT_LENGTH, 'a')); // If we don't have buffered data in encoding buffer, the merged data will be the same as last // data. - expectMergedAndLimitedResponseData(EMPTY_STRING, &last_data, + expectMergedAndLimitedResponseData(nullptr, &last_data, /*expected_merged_length=*/last_data.length()); } TEST_F(GrpcWebFilterTest, MergeAndLimitNonProtoEncodedResponseDataWithNoEncodingBufferAndLastData) { - // If we don't have buffered data in encoding buffer and no last data either, the output length is + // If we don't have both buffered data in encoding buffer and last data, the output length is // zero. - expectMergedAndLimitedResponseData(EMPTY_STRING, nullptr, /*expected_merged_length=*/0U); + expectMergedAndLimitedResponseData(nullptr, nullptr, /*expected_merged_length=*/0U); } TEST_F(GrpcWebFilterTest, UnsupportedContentType) { From 239f0717a61e32712b12879386241da822460360 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 01:59:22 +0000 Subject: [PATCH 40/48] Empty Signed-off-by: Dhi Aurrahman --- test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 0d457997a4db0..1045090e9267e 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -2,6 +2,7 @@ #include "common/buffer/buffer_impl.h" #include "common/common/base64.h" +#include "common/common/empty_string.h" #include "common/common/utility.h" #include "common/grpc/common.h" #include "common/http/codes.h" @@ -178,7 +179,7 @@ TEST_F(GrpcWebFilterTest, ExpectedGrpcWebProtoContentType) { } TEST_F(GrpcWebFilterTest, UnexpectedGrpcWebProtoContentType) { - EXPECT_FALSE(isProtoEncodedGrpcWebContentType("")); + EXPECT_FALSE(isProtoEncodedGrpcWebContentType(EMPTY_STRING)); EXPECT_FALSE(isProtoEncodedGrpcWebContentType("Invalid; ok=1")); EXPECT_FALSE(isProtoEncodedGrpcWebContentType("Invalid; ok=1; nok=2")); EXPECT_FALSE( From 9cc0f619c42a4a5fc4dbef59c9737826f0f8d676 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 02:06:13 +0000 Subject: [PATCH 41/48] Remove Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index d610abdc7cc75..fba48b1265cbf 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -16,9 +16,7 @@ namespace GrpcWeb { /** * See docs/configuration/http_filters/grpc_web_filter.rst */ -class GrpcWebFilter : public Http::StreamFilter, - NonCopyable, - public Logger::Loggable { +class GrpcWebFilter : public Http::StreamFilter, NonCopyable { public: explicit GrpcWebFilter(Grpc::Context& context) : context_(context) {} ~GrpcWebFilter() override = default; From 9d1e41f76775688ad62a2168235297c559cbfc82 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 03:11:46 +0000 Subject: [PATCH 42/48] Fix format Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter_integration_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index ab7a397d19d5e..6f0925196dfd6 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -250,7 +250,8 @@ TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseWithoutContentType) { } TEST_P(GrpcWebFilterIntegrationTest, BadUpstreamResponseLargeEnd) { - // When we have buffered data in encoding buffer, we limit the length to MAX_BUFFERED_PLAINTEXT_LENGTH. + // When we have buffered data in encoding buffer, we limit the length to + // MAX_BUFFERED_PLAINTEXT_LENGTH. const std::string start(MAX_BUFFERED_PLAINTEXT_LENGTH, 'a'); const std::string end(MAX_BUFFERED_PLAINTEXT_LENGTH, 'b'); testBadUpstreamResponse(start, end, /*expected=*/start); From 8bec999db9bd9c351d09b1d7b8d687202489868d Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 21:27:17 +0000 Subject: [PATCH 43/48] Fix Signed-off-by: Dhi Aurrahman --- source/common/runtime/runtime_features.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 6c0d15a670bbb..3d72e229c3d08 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -65,7 +65,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.check_ocsp_policy", "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.fix_wildcard_matching", - "envoy.reloadable_features.fixed_connection_close", "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.health_check.graceful_goaway_handling", From 3329e0fff2216252d4fe472f573b3c6ff4fac3c3 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 21:41:58 +0000 Subject: [PATCH 44/48] Comment Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index a84dc204ed66b..83e0370690b51 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -100,6 +100,10 @@ bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::Response return Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling") && !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && + // We also do transformation when the response status is not OK (since a gRPC (also + // gRPC-Web) response should have 200 OK as its response status + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) and it has no + // valid proto-encoded response. !(Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && hasProtoEncodedGrpcWebContentType(headers)); } From 38b78827eef5250395660113ded5f6f6afe53dd6 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 21 Jan 2021 22:43:52 +0000 Subject: [PATCH 45/48] Add more tests Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 21 ++++++++++++------- .../filters/http/grpc_web/grpc_web_filter.h | 1 + .../http/grpc_web/grpc_web_filter_test.cc | 18 ++++++++++++++++ 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 83e0370690b51..1dc17a2b7af4c 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -74,6 +74,16 @@ bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) { return false; } +bool GrpcWebFilter::isProtoEncodedGrpcWebResponseHeaders( + const Http::ResponseHeaderMap& headers) const { + // We expect the response headers to have 200 OK status (a valid gRPC, also gRPC-Web, response + // needs to have 200 OK status + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) and contain + // proto-encoded gRPC-Web content-type. + return Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && + hasProtoEncodedGrpcWebContentType(headers); +} + bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType( const Http::RequestOrResponseHeaderMap& headers) const { const Http::HeaderEntry* content_type = headers.ContentType(); @@ -94,18 +104,15 @@ bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType( return false; } -// If response headers do not contain gRPC or gRPC-Web response headers, it needs transformation. +// If response headers do not contain valid response headers, it needs transformation. bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers, bool end_stream) const { return Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling") && + // We do transformation when the response headers is not gRPC response headers and it is + // not proto-encoded gRPC-Web response headers. !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && - // We also do transformation when the response status is not OK (since a gRPC (also - // gRPC-Web) response should have 200 OK as its response status - // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) and it has no - // valid proto-encoded response. - !(Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && - hasProtoEncodedGrpcWebContentType(headers)); + !isProtoEncodedGrpcWebResponseHeaders(headers); } void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& output, diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index fba48b1265cbf..01f4d59699b26 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -56,6 +56,7 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers); void setupStatTracking(const Http::RequestHeaderMap& headers); bool isGrpcWebRequest(const Http::RequestHeaderMap& headers); + bool isProtoEncodedGrpcWebResponseHeaders(const Http::ResponseHeaderMap& headers) const; bool hasProtoEncodedGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const; bool needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers, bool end_stream) const; diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index 1045090e9267e..d99404b9dec11 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -114,6 +114,10 @@ class GrpcWebFilterTest : public testing::TestWithParam Date: Tue, 26 Jan 2021 21:10:01 +0000 Subject: [PATCH 46/48] Add some comments Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 1dc17a2b7af4c..b238c17c0a226 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -29,7 +29,8 @@ constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; // This builds grpc-message header value from body data. std::string buildGrpcMessage(Buffer::Instance& body_data) { const uint64_t message_length = body_data.length(); - std::string message(message_length, 0); + std::string message; + message.reserve(message_length); body_data.copyOut(0, message_length, message.data()); return Http::Utility::PercentEncoding::encode(message); @@ -109,8 +110,8 @@ bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::Response bool end_stream) const { return Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling") && - // We do transformation when the response headers is not gRPC response headers and it is - // not proto-encoded gRPC-Web response headers. + // We transform the response unless it is already a gRPC or proto-encoded gRPC-Web + // response. !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && !isProtoEncodedGrpcWebResponseHeaders(headers); } @@ -125,7 +126,8 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& }); } - // In the case of local reply, "encoding_buffer" is nullptr and we only have filled "last_data". + // In the case of local reply and when the response only contains single data chunk, + // "encoding_buffer" is nullptr and we only have filled "last_data". if (last_data != nullptr) { uint64_t needed = last_data->length(); // When we have buffered data (from encoding buffer), we limit the final buffer length. @@ -277,6 +279,9 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en if (needs_transformation_for_non_proto_encoded_response_) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (!end_stream) { + // We limit the buffered data in encoding buffer here to eliminate the possibility of + // buffering too large data from upstream. Note that the buffered data here will be + // transformed as grpc-message later. if (encoding_buffer != nullptr && encoding_buffer->length() >= MAX_BUFFERED_PLAINTEXT_LENGTH) { return Http::FilterDataStatus::StopIterationNoBuffer; From 190879bcbaaf12660f461a9f0c3e6dc78af99830 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 26 Jan 2021 23:15:26 +0000 Subject: [PATCH 47/48] Review comments Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_web/grpc_web_filter.cc | 21 +++++++++---------- .../http/grpc_web/grpc_web_filter_test.cc | 7 +++++++ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index b238c17c0a226..e178d9d40c328 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -31,6 +31,7 @@ std::string buildGrpcMessage(Buffer::Instance& body_data) { const uint64_t message_length = body_data.length(); std::string message; message.reserve(message_length); + message.resize(message_length); body_data.copyOut(0, message_length, message.data()); return Http::Utility::PercentEncoding::encode(message); @@ -85,6 +86,7 @@ bool GrpcWebFilter::isProtoEncodedGrpcWebResponseHeaders( hasProtoEncodedGrpcWebContentType(headers); } +// TODO(dio): Move this as a shared utility function. bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType( const Http::RequestOrResponseHeaderMap& headers) const { const Http::HeaderEntry* content_type = headers.ContentType(); @@ -120,21 +122,18 @@ void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& Buffer::Instance* last_data) { const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); if (encoding_buffer != nullptr) { + if (last_data != nullptr) { + encoder_callbacks_->addEncodedData(*last_data, false); + } encoder_callbacks_->modifyEncodingBuffer([&output](Buffer::Instance& buffered) { + // When we have buffered data (encoding buffer is filled), we limit the final buffer length. output.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH); buffered.drain(buffered.length()); }); - } - - // In the case of local reply and when the response only contains single data chunk, - // "encoding_buffer" is nullptr and we only have filled "last_data". - if (last_data != nullptr) { - uint64_t needed = last_data->length(); - // When we have buffered data (from encoding buffer), we limit the final buffer length. - if (encoding_buffer != nullptr && (output.length() + needed) >= MAX_BUFFERED_PLAINTEXT_LENGTH) { - needed = std::min(needed, MAX_BUFFERED_PLAINTEXT_LENGTH - output.length()); - } - output.move(*last_data, needed); + } else if (last_data != nullptr) { + // In the case of local reply and when the response only contains a single data chunk, + // "encoding_buffer" is nullptr and we only have filled "last_data". + output.move(*last_data); last_data->drain(last_data->length()); } } diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index d99404b9dec11..bbc401405cf97 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -125,6 +125,10 @@ class GrpcWebFilterTest : public testing::TestWithParam cb) { cb(*encoded_buffer); }; + if (last_data != nullptr) { + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) { encoded_buffer->move(data); })); + } EXPECT_CALL(encoder_callbacks_, encodingBuffer).WillRepeatedly(Return(encoded_buffer)); EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) .WillRepeatedly(Invoke(on_modify_encoding_buffer)); @@ -324,6 +328,9 @@ TEST_F(GrpcWebFilterTest, InvalidUpstreamResponseForText) { EXPECT_CALL(encoder_callbacks_, modifyEncodingBuffer) .WillRepeatedly(Invoke(on_modify_encoding_buffer)); + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) { buffer->move(data); })); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(data, false)); buffer->add(data); From c394503c6639ceb6df6bfa7bb9aac91c7d61e160 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 26 Jan 2021 23:18:07 +0000 Subject: [PATCH 48/48] Update MAX_BUFFERED_PLAINTEXT_LENGTH comment Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_web/grpc_web_filter.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index e178d9d40c328..93953fe44efd7 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -22,8 +22,10 @@ namespace GrpcWeb { namespace { -// This is the maximum buffered plaintext data length to be converted into grpc-message header. This -// is arbitrarily chosen. This can be made configurable when it is required. +// This is the maximum buffered plaintext data length when we have buffered data in the encoding +// buffer. This is effectively used (to limit the length of grpc-message) only when we have encoding +// buffer filled with data. The value is arbitrarily chosen. This can be made configurable when it +// is required. constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; // This builds grpc-message header value from body data.