From 462b70c74ab8a1213783e2f6ca371740c314878d Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Tue, 12 Mar 2019 10:45:07 +0800 Subject: [PATCH 01/15] Refactor sendLocalReply Signed-off-by: tianqian.zyf --- source/common/http/async_client_impl.cc | 4 + source/common/http/async_client_impl.h | 7 +- source/common/http/conn_manager_impl.cc | 98 ++++++++----------- source/common/http/conn_manager_impl.h | 11 +-- source/common/http/utility.cc | 51 +++++++++- source/common/http/utility.h | 24 +++-- test/common/http/utility_test.cc | 31 +++--- .../reverse_bridge_test.cc | 14 +-- test/mocks/http/mocks.h | 3 +- 9 files changed, 142 insertions(+), 101 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index a54b9eea5a568..05f8a91659dbe 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -110,11 +110,15 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { +<<<<<<< HEAD if (Http::Headers::get().MethodValues.Head == headers.Method()->value().getStringView()) { is_head_request_ = true; } is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); +======= + local_reply_info_ = Utility::generateLocalReplyInfo(headers); +>>>>>>> Refactor sendLocalReply headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); if (send_xff_) { diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index b5374c455f1aa..95bcd6e76b41d 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -318,7 +318,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, absl::string_view details) override { stream_info_.setResponseCodeDetails(details); Utility::sendLocalReply( - is_grpc_request_, + local_reply_info_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { modify_headers(*headers); @@ -326,7 +326,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - remote_closed_, code, body, grpc_status, is_head_request_); + remote_closed_, code, body, grpc_status); } // The async client won't pause if sending an Expect: 100-Continue so simply // swallows any incoming encode100Continue. @@ -363,8 +363,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, bool local_closed_{}; bool remote_closed_{}; Buffer::InstancePtr buffered_body_; - bool is_grpc_request_{}; - bool is_head_request_{false}; + Utility::LocalReplyInfo local_reply_info_; bool send_xff_{true}; friend class AsyncClientImpl; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 998642cd2cd6f..961988bb4bb49 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -513,18 +513,18 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { connection_manager_.doEndStream(*this); } else { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout); - sendLocalReply(request_headers_ != nullptr && - Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout); + + sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_) + : Utility::LocalReplyInfo{}, + Http::Code::RequestTimeout, "stream timeout", nullptr, absl::nullopt); } } void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); - sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::RequestTimeout, "request timeout", nullptr, is_head_request_, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); + sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_) + : Utility::LocalReplyInfo{}, + Http::Code::RequestTimeout, "request timeout", nullptr, absl::nullopt); } void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker( @@ -595,10 +595,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, ScopeTrackerScopeState scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); request_headers_ = std::move(headers); - if (Http::Headers::get().MethodValues.Head == - request_headers_->Method()->value().getStringView()) { - is_head_request_ = true; - } ENVOY_STREAM_LOG(debug, "request headers complete (end_stream={}):\n{}", *this, end_stream, *request_headers_); @@ -614,9 +610,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // overload it is more important to avoid unnecessary allocation than to create the filters. state_.created_filter_chain_ = true; connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, is_head_request_, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().Overload); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), + Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, absl::nullopt); return; } @@ -644,8 +639,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, stream_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. - sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().LowVersion); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::UpgradeRequired, "", + nullptr, absl::nullopt); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -667,18 +662,16 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.config_.http1Settings().default_host_for_http_10_); } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().MissingHost); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", + nullptr, absl::nullopt); return; } } ASSERT(connection_manager_.config_.maxRequestHeadersKb() > 0); if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), - Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), + Code::RequestHeaderFieldsTooLarge, "", nullptr, absl::nullopt); return; } @@ -692,9 +685,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, const bool has_path = request_headers_->Path() && !request_headers_->Path()->value().getStringView().empty(); connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr, - is_head_request_, absl::nullopt, - has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::NotFound, "", nullptr, + absl::nullopt, has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath : StreamInfo::ResponseCodeDetails::get().MissingPath); return; } @@ -702,9 +694,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Path sanitization should happen before any path access other than the above sanity check. if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_, connection_manager_.config_)) { - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", nullptr, + absl::nullopt, StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); return; } @@ -749,9 +740,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (upgrade_rejected) { // Do not allow upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", - nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().UpgradeFailed); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::Forbidden, "", + nullptr, absl::nullopt); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -1220,34 +1210,31 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } void ConnectionManagerImpl::ActiveStream::sendLocalReply( - bool is_grpc_request, Code code, absl::string_view body, - const std::function& modify_headers, bool is_head_request, - const absl::optional grpc_status, absl::string_view details) { - ENVOY_STREAM_LOG(debug, "Sending local reply with details {}", *this, details); + const Utility::LocalReplyInfo& local_reply_info, Code code, absl::string_view body, + const std::function& modify_headers, + const absl::optional grpc_status) { ASSERT(response_headers_ == nullptr); // For early error handling, do a best-effort attempt to create a filter chain // to ensure access logging. if (!state_.created_filter_chain_) { createFilterChain(); } - stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply( - is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply(local_reply_info, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream); + }, + state_.destroyed_, code, body, grpc_status); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( @@ -2135,7 +2122,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.stream_info_.setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); Http::Utility::sendLocalReply( - Grpc::Common::hasGrpcContentType(*parent_.request_headers_), + Utility::generateLocalReplyInfo(*parent_.request_headers_), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { parent_.chargeStats(*response_headers); parent_.response_headers_ = std::move(response_headers); @@ -2147,8 +2134,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.local_complete_ = end_stream; }, parent_.state_.destroyed_, Http::Code::InternalServerError, - CodeUtility::toString(Http::Code::InternalServerError), absl::nullopt, - parent_.is_head_request_); + CodeUtility::toString(Http::Code::InternalServerError), absl::nullopt); parent_.maybeEndEncode(parent_.state_.local_complete_); } else { resetStream(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 7b3a650d26dde..a76523b0988ce 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -267,8 +267,7 @@ class ConnectionManagerImpl : Logger::Loggable, const absl::optional grpc_status, absl::string_view details) override { parent_.stream_info_.setResponseCodeDetails(details); - parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_, - grpc_status, details); + parent_.sendLocalReply(local_reply_info_, code, body, modify_headers, grpc_status, details); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -297,7 +296,7 @@ class ConnectionManagerImpl : Logger::Loggable, // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() // called here may change the content type, so we must check it before the call. FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { - is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + local_reply_info_ = Utility::generateLocalReplyInfo(headers); FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); if (end_stream) { handle_->decodeComplete(); @@ -309,7 +308,7 @@ class ConnectionManagerImpl : Logger::Loggable, void requestDataDrained(); StreamDecoderFilterSharedPtr handle_; - bool is_grpc_request_{}; + Utility::LocalReplyInfo local_reply_info_; }; using ActiveStreamDecoderFilterPtr = std::unique_ptr; @@ -423,9 +422,8 @@ class ConnectionManagerImpl : Logger::Loggable, void maybeEndDecode(bool end_stream); void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); HeaderMap& addEncodedTrailers(); - void sendLocalReply(bool is_grpc_request, Code code, absl::string_view body, + void sendLocalReply(const Utility::LocalReplyInfo& info, Code code, absl::string_view body, const std::function& modify_headers, - bool is_head_request, const absl::optional grpc_status, absl::string_view details); void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); @@ -617,7 +615,6 @@ class ConnectionManagerImpl : Logger::Loggable, // By default, we will assume there are no 100-Continue headers. If encode100ContinueHeaders // is ever called, this is set to true so commonContinue resumes processing the 100-Continue. bool has_continue_headers_{}; - bool is_head_request_{}; // Whether a filter has indicated that the request should be treated as a headers only request. bool decoding_headers_only_{}; // Whether a filter has indicated that the response should be treated as a headers only diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 52171de11e6c6..69710b1724c48 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -264,6 +264,7 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co return ret; } +<<<<<<< HEAD void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, absl::string_view body_text, const absl::optional grpc_status, @@ -277,17 +278,57 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac callbacks.encodeData(data, end_stream); }, is_reset, response_code, body_text, grpc_status, is_head_request); +======= +Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { + bool is_grpc = false, is_head_request = false, is_json_content_type = false; + const Http::HeaderEntry* content_type = request_headers.ContentType(); + // Content type is gRPC if it is exactly "application/grpc" or starts with + // "application/grpc+". Specifically, something like application/grpc-web is not gRPC. + is_grpc = + content_type != nullptr && + absl::StartsWith(content_type->value().getStringView(), + Http::Headers::get().ContentTypeValues.Grpc) && + (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() || + content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); + + if (Http::Headers::get().MethodValues.Head == request_headers.Method()->value().c_str()) { + is_head_request = true; + } + + if (!is_grpc) { + is_json_content_type = + content_type != nullptr && + (Http::Headers::get().ContentTypeValues.Json == content_type->value().c_str()); + } + + return LocalReplyInfo{is_grpc, is_head_request, is_json_content_type}; +} + +void Utility::sendLocalReply(const Utility::LocalReplyInfo& info, + StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, + Code response_code, absl::string_view body_text, + const absl::optional grpc_status) { + sendLocalReply(info, + [&](HeaderMapPtr&& headers, bool end_stream) -> void { + callbacks.encodeHeaders(std::move(headers), end_stream); + }, + [&](Buffer::Instance& data, bool end_stream) -> void { + callbacks.encodeData(data, end_stream); + }, + is_reset, response_code, body_text, grpc_status); +>>>>>>> Refactor sendLocalReply } void Utility::sendLocalReply( - bool is_grpc, std::function encode_headers, + const Utility::LocalReplyInfo& info, + std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, absl::string_view body_text, - const absl::optional grpc_status, bool is_head_request) { + const absl::optional grpc_status) { // encode_headers() may reset the stream, so the stream must not be reset before calling it. ASSERT(!is_reset); // Respond with a gRPC trailers-only response if the request is gRPC - if (is_grpc) { + if (info.is_grpc) { HeaderMapPtr response_headers{new HeaderMapImpl{ {Headers::get().Status, std::to_string(enumToInt(Code::OK))}, {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc}, @@ -295,7 +336,7 @@ void Utility::sendLocalReply( std::to_string( enumToInt(grpc_status ? grpc_status.value() : Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))}}}; - if (!body_text.empty() && !is_head_request) { + if (!body_text.empty() && !info.is_head_request) { // TODO(dio): Probably it is worth to consider caching the encoded message based on gRPC // status. response_headers->insertGrpcMessage().value(PercentEncoding::encode(body_text)); @@ -311,7 +352,7 @@ void Utility::sendLocalReply( response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } - if (is_head_request) { + if (info.is_head_request) { encode_headers(std::move(response_headers), true); return; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index fef7b08e11b6d..d1ef2fdc1dfd8 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -175,6 +175,18 @@ Http2Settings parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOption */ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& config); +struct LocalReplyInfo { + bool is_grpc{false}; + bool is_head_request{false}; + bool is_json_content_type{false}; +}; + +/** + * @return LocalReplyInfo An LocalReplyInfo populated from the + * Http::HeaderMap& request_headers. + */ +LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers); + /** * Create a locally generated response using filter callbacks. * @param is_grpc tells if this is a response to a gRPC request. @@ -188,10 +200,9 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. * @param is_head_request tells if this is a response to a HEAD request */ -void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, absl::string_view body_text, - const absl::optional grpc_status, - bool is_head_request); +void sendLocalReply(const LocalReplyInfo& info, StreamDecoderFilterCallbacks& callbacks, + const bool& is_reset, Code response_code, absl::string_view body_text, + const absl::optional grpc_status); /** * Create a locally generated response using the provided lambdas. @@ -206,12 +217,11 @@ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const * type. * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. */ -void sendLocalReply(bool is_grpc, +void sendLocalReply(const LocalReplyInfo& info, std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, absl::string_view body_text, - const absl::optional grpc_status, - bool is_head_request = false); + const absl::optional grpc_status); struct GetLastAddressFromXffInfo { // Last valid address pulled from the XFF header. diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index daab4c044d181..0e6553c1d7e29 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -436,8 +436,8 @@ TEST(HttpUtility, SendLocalReply) { EXPECT_CALL(callbacks, encodeHeaders_(_, false)); EXPECT_CALL(callbacks, encodeData(_, true)); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", - absl::nullopt, false); + Utility::sendLocalReply(Utility::LocalReplyInfo{}, callbacks, is_reset, + Http::Code::PayloadTooLarge, "large", absl::nullopt); } TEST(HttpUtility, SendLocalGrpcReply) { @@ -453,8 +453,10 @@ TEST(HttpUtility, SendLocalGrpcReply) { EXPECT_NE(headers.GrpcMessage(), nullptr); EXPECT_EQ(headers.GrpcMessage()->value().getStringView(), "large"); })); - Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", - absl::nullopt, false); + Utility::LocalReplyInfo info; + info.is_grpc = true; + Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt); } TEST(HttpUtility, SendLocalGrpcReplyWithUpstreamJsonPayload) { @@ -493,8 +495,9 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { EXPECT_EQ(headers.GrpcStatus()->value().getStringView(), std::to_string(enumToInt(Grpc::Status::GrpcStatus::Unavailable))); })); - Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", absl::nullopt, - false); + Utility::LocalReplyInfo info; + info.is_grpc = true; + Utility::sendLocalReply(info, callbacks, false, Http::Code::TooManyRequests, "", absl::nullopt); EXPECT_CALL(callbacks, encodeHeaders_(_, true)) .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { @@ -503,9 +506,8 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { std::to_string(enumToInt(Grpc::Status::GrpcStatus::ResourceExhausted))); })); Utility::sendLocalReply( - true, callbacks, false, Http::Code::TooManyRequests, "", - absl::make_optional(Grpc::Status::GrpcStatus::ResourceExhausted), - false); + info, callbacks, false, Http::Code::TooManyRequests, "", + absl::make_optional(Grpc::Status::GrpcStatus::ResourceExhausted)); } TEST(HttpUtility, SendLocalReplyDestroyedEarly) { @@ -516,8 +518,9 @@ TEST(HttpUtility, SendLocalReplyDestroyedEarly) { is_reset = true; })); EXPECT_CALL(callbacks, encodeData(_, true)).Times(0); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", - absl::nullopt, false); + + Utility::sendLocalReply(Utility::LocalReplyInfo{}, callbacks, is_reset, + Http::Code::PayloadTooLarge, "large", absl::nullopt); } TEST(HttpUtility, SendLocalReplyHeadRequest) { @@ -528,8 +531,10 @@ TEST(HttpUtility, SendLocalReplyHeadRequest) { EXPECT_EQ(headers.ContentLength()->value().getStringView(), fmt::format("{}", strlen("large"))); })); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", - absl::nullopt, true); + Utility::LocalReplyInfo info; + info.is_head_request = true; + Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt); } TEST(HttpUtility, TestExtractHostPathFromUri) { diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc index 137970bb6bb67..eee4d577026ba 100644 --- a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc +++ b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc @@ -47,7 +47,7 @@ class ReverseBridgeTest : public testing::Test { // Verifies that an incoming request with too small a request body will immediately fail. TEST_F(ReverseBridgeTest, InvalidGrpcRequest) { initialize(); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -81,7 +81,7 @@ TEST_F(ReverseBridgeTest, InvalidGrpcRequest) { // Verifies that we do nothing to a header only request even if it looks like a gRPC request. TEST_F(ReverseBridgeTest, HeaderOnlyGrpcRequest) { initialize(); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { Http::TestHeaderMapImpl headers({{"content-type", "application/grpc"}, @@ -157,7 +157,7 @@ TEST_F(ReverseBridgeTest, NoGrpcRequest) { // frames, then the data should not be modified. TEST_F(ReverseBridgeTest, GrpcRequestNoManageFrameHeader) { initialize(false); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -216,7 +216,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestNoManageFrameHeader) { // to gRPC. TEST_F(ReverseBridgeTest, GrpcRequest) { initialize(); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -296,7 +296,7 @@ TEST_F(ReverseBridgeTest, GrpcRequest) { // Same as ReverseBridgeTest.GrpcRequest except no content-length header is passed. TEST_F(ReverseBridgeTest, GrpcRequestNoContentLength) { initialize(); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -375,7 +375,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestNoContentLength) { // grpc-status. TEST_F(ReverseBridgeTest, GrpcRequestInternalError) { initialize(); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -448,7 +448,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestInternalError) { // has an invalid content type we respond with a useful error message. TEST_F(ReverseBridgeTest, GrpcRequestBadResponse) { initialize(); - decoder_callbacks_.is_grpc_request_ = true; + decoder_callbacks_.local_reply_info_.is_grpc = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 13f5d6717a031..6d68849629700 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -192,8 +192,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, testing::NiceMock tracing_config_; testing::NiceMock scope_; std::string details_; - bool is_grpc_request_{}; - bool is_head_request_{false}; + Utility::LocalReplyInfo local_reply_info_; bool stream_destroyed_{}; }; From 7e258acc9dea4ecdb2460dbd46263a4fca10201a Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Wed, 13 Mar 2019 19:03:36 +0800 Subject: [PATCH 02/15] Support return response of json type Signed-off-by: tianqian.zyf --- api/envoy/data/core/v2alpha/BUILD | 7 +++++- .../data/core/v2alpha/local_reply_body.proto | 14 +++++++++++ source/common/http/BUILD | 1 + source/common/http/utility.cc | 24 +++++++++++++++---- 4 files changed, 40 insertions(+), 6 deletions(-) create mode 100644 api/envoy/data/core/v2alpha/local_reply_body.proto diff --git a/api/envoy/data/core/v2alpha/BUILD b/api/envoy/data/core/v2alpha/BUILD index 8320031d8466f..3db232ffd4b95 100644 --- a/api/envoy/data/core/v2alpha/BUILD +++ b/api/envoy/data/core/v2alpha/BUILD @@ -1,4 +1,4 @@ -load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library") +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library", "api_proto_library_internal") licenses(["notice"]) # Apache 2 @@ -13,3 +13,8 @@ api_proto_library( "//envoy/api/v2/core:base", ], ) + +api_proto_library_internal( + name = "local_reply_body", + srcs = ["local_reply_body.proto"], +) diff --git a/api/envoy/data/core/v2alpha/local_reply_body.proto b/api/envoy/data/core/v2alpha/local_reply_body.proto new file mode 100644 index 0000000000000..43cdef77bcb64 --- /dev/null +++ b/api/envoy/data/core/v2alpha/local_reply_body.proto @@ -0,0 +1,14 @@ +syntax = "proto3"; + +package envoy.data.core.v2alpha; + +option java_outer_classname = "LocalReplyBodyProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; + +// [#protodoc-title: core data] + +// Wrapper for local reply body data. +message LocalReplyBody { + string body = 3; +} diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 1dcf5045fa261..49bf573b2fc47 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -307,6 +307,7 @@ envoy_cc_library( "//source/common/protobuf:utility_lib", "@envoy_api//envoy/api/v2/core:http_uri_cc", "@envoy_api//envoy/api/v2/core:protocol_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_body_cc", ], ) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 69710b1724c48..0f89a3f2d6d5c 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -6,6 +6,7 @@ #include #include +#include "envoy/data/core/v2alpha/local_reply_body.pb.h" #include "envoy/http/header_map.h" #include "common/buffer/buffer_impl.h" @@ -347,8 +348,21 @@ void Utility::sendLocalReply( HeaderMapPtr response_headers{ new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))}}}; - if (!body_text.empty()) { - response_headers->insertContentLength().value(body_text.size()); + + envoy::data::core::v2alpha::LocalReplyBody local_reply_body; + std::string json_body; + absl::string_view finally_body_text = body_text; + + if (info.is_json_content_type) { + local_reply_body.set_body(finally_body_text.begin(), finally_body_text.length()); + json_body = MessageUtil::getJsonStringFromMessage(local_reply_body, true, true); + response_headers->insertContentLength().value(json_body.size()); + finally_body_text = absl::string_view(json_body); + response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); + } + + if (!info.is_json_content_type && !finally_body_text.empty()) { + response_headers->insertContentLength().value(finally_body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } @@ -357,10 +371,10 @@ void Utility::sendLocalReply( return; } - encode_headers(std::move(response_headers), body_text.empty()); + encode_headers(std::move(response_headers), finally_body_text.empty()); // encode_headers()) may have changed the referenced is_reset so we need to test it - if (!body_text.empty() && !is_reset) { - Buffer::OwnedImpl buffer(body_text); + if (!finally_body_text.empty() && !is_reset) { + Buffer::OwnedImpl buffer(finally_body_text); encode_data(buffer, true); } } From f533efd9a7571c34c96f5f74aa48175e7a17d722 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Wed, 13 Mar 2019 20:38:21 +0800 Subject: [PATCH 03/15] Add unittests and integration test Signed-off-by: tianqian.zyf --- source/common/http/utility.cc | 2 ++ test/common/http/BUILD | 1 + test/common/http/utility_test.cc | 22 ++++++++++++++++ test/integration/BUILD | 1 + .../idle_timeout_integration_test.cc | 26 +++++++++++++++++-- 5 files changed, 50 insertions(+), 2 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 0f89a3f2d6d5c..b2f8409c15dd4 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -282,6 +282,8 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac ======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { bool is_grpc = false, is_head_request = false, is_json_content_type = false; + // TODO(zyfjeff): This code is copied from Envoy::Grpc::Common::hasGrpcContentType in order to + // rely directly on it to avoid causing circular dependencies. const Http::HeaderEntry* content_type = request_headers.ContentType(); // Content type is gRPC if it is exactly "application/grpc" or starts with // "application/grpc+". Specifically, something like application/grpc-web is not gRPC. diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 5cfeaff671fa3..0491c010bc5d2 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -334,6 +334,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", + "@envoy_api//envoy/data/core/v2alpha:local_reply_body_cc", "@envoy_api//envoy/api/v2/core:protocol_cc", ], ) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 0e6553c1d7e29..27976ec047b5a 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1,6 +1,7 @@ #include #include +#include "envoy/data/core/v2alpha/local_reply_body.pb.h" #include "envoy/api/v2/core/protocol.pb.h" #include "envoy/api/v2/core/protocol.pb.validate.h" @@ -537,6 +538,27 @@ TEST(HttpUtility, SendLocalReplyHeadRequest) { absl::nullopt); } +TEST(HttpUtility, SendLocalReplyJsonConntentTypeRequest) { + MockStreamDecoderFilterCallbacks callbacks; + bool is_reset = false; + EXPECT_CALL(callbacks, encodeHeaders_(_, false)) + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.ContentType()->value().c_str(), + Http::Headers::get().ContentTypeValues.Json); + })); + EXPECT_CALL(callbacks, encodeData(_, true)) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { + envoy::data::core::v2alpha::LocalReplyBody local_reply_body; + local_reply_body.set_body("large"); + EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply_body, true, true), + data.toString()); + })); + Utility::LocalReplyInfo info; + info.is_json_content_type = true; + Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt); +} + TEST(HttpUtility, TestExtractHostPathFromUri) { absl::string_view host, path; diff --git a/test/integration/BUILD b/test/integration/BUILD index 64949483c84ea..c2945190e80f6 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -363,6 +363,7 @@ envoy_cc_test( deps = [ ":http_protocol_integration_lib", "//test/test_common:test_time_lib", + "@envoy_api//envoy/data/core/v2alpha:local_reply_body_cc", ], ) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index b0ec3fb192c40..6018cb356f1e2 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -1,3 +1,5 @@ +#include "envoy/data/core/v2alpha/local_reply_body.pb.h" + #include "test/integration/http_protocol_integration.h" #include "test/test_common/test_time.h" @@ -32,7 +34,9 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { HttpProtocolIntegrationTest::initialize(); } - IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest(const char* method = "GET") { + IntegrationStreamDecoderPtr + setupPerStreamIdleTimeoutTest(const char* method = "GET", + const char* content_type = "text/plain") { initialize(); fake_upstreams_[0]->set_allow_unexpected_disconnects(true); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); @@ -40,7 +44,8 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", method}, {":path", "/test/long/url"}, {":scheme", "http"}, - {":authority", "host"}}); + {":authority", "host"}, + {"content-type", content_type}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); AssertionResult result = @@ -189,6 +194,23 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstrea EXPECT_EQ("", response->body()); } +TEST_P(IdleTimeoutIntegrationTest, + PerStreamIdleTimeoutHeadRequestAfterDownstreamJsonContentTypeRequest) { + enable_per_stream_idle_timeout_ = true; + auto response = setupPerStreamIdleTimeoutTest("GET", "application/json"); + + waitForTimeout(*response, "downstream_rq_idle_timeout"); + EXPECT_FALSE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + EXPECT_TRUE(response->complete()); + EXPECT_STREQ("408", response->headers().Status()->value().c_str()); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.Json, + response->headers().ContentType()->value().c_str()); + envoy::data::core::v2alpha::LocalReplyBody local_reply_body; + local_reply_body.set_body("stream timeout"); + EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply_body, true, true), response->body()); +} + // Global per-stream idle timeout applies if there is no per-stream idle timeout. TEST_P(IdleTimeoutIntegrationTest, GlobalPerStreamIdleTimeoutAfterDownstreamHeaders) { enable_per_stream_idle_timeout_ = true; From dd9636adc082dd68658eea13860ecd6e4265c920 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Mon, 18 Mar 2019 10:23:37 +0800 Subject: [PATCH 04/15] Fix code review comments Signed-off-by: tianqian.zyf --- api/envoy/data/core/v2alpha/local_reply_body.proto | 7 +++---- source/common/http/utility.cc | 10 +++++----- source/common/http/utility.h | 4 ++-- test/common/http/utility_test.cc | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/api/envoy/data/core/v2alpha/local_reply_body.proto b/api/envoy/data/core/v2alpha/local_reply_body.proto index 43cdef77bcb64..e235a45a86009 100644 --- a/api/envoy/data/core/v2alpha/local_reply_body.proto +++ b/api/envoy/data/core/v2alpha/local_reply_body.proto @@ -6,9 +6,8 @@ option java_outer_classname = "LocalReplyBodyProto"; option java_multiple_files = true; option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; -// [#protodoc-title: core data] - -// Wrapper for local reply body data. +// [#protodoc-title: local reply body] +// Used to wrap a local response, and then converted to json format to return. message LocalReplyBody { - string body = 3; + string body = 1; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index b2f8409c15dd4..14084d7096f6a 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -281,7 +281,7 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac is_reset, response_code, body_text, grpc_status, is_head_request); ======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { - bool is_grpc = false, is_head_request = false, is_json_content_type = false; + bool is_grpc = false, is_head_request = false, has_json_content_type = false; // TODO(zyfjeff): This code is copied from Envoy::Grpc::Common::hasGrpcContentType in order to // rely directly on it to avoid causing circular dependencies. const Http::HeaderEntry* content_type = request_headers.ContentType(); @@ -299,12 +299,12 @@ Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& r } if (!is_grpc) { - is_json_content_type = + has_json_content_type = content_type != nullptr && (Http::Headers::get().ContentTypeValues.Json == content_type->value().c_str()); } - return LocalReplyInfo{is_grpc, is_head_request, is_json_content_type}; + return LocalReplyInfo{is_grpc, is_head_request, has_json_content_type}; } void Utility::sendLocalReply(const Utility::LocalReplyInfo& info, @@ -355,7 +355,7 @@ void Utility::sendLocalReply( std::string json_body; absl::string_view finally_body_text = body_text; - if (info.is_json_content_type) { + if (info.has_json_content_type) { local_reply_body.set_body(finally_body_text.begin(), finally_body_text.length()); json_body = MessageUtil::getJsonStringFromMessage(local_reply_body, true, true); response_headers->insertContentLength().value(json_body.size()); @@ -363,7 +363,7 @@ void Utility::sendLocalReply( response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); } - if (!info.is_json_content_type && !finally_body_text.empty()) { + if (!info.has_json_content_type && !finally_body_text.empty()) { response_headers->insertContentLength().value(finally_body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d1ef2fdc1dfd8..5866ae2ac88ab 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -178,11 +178,11 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption struct LocalReplyInfo { bool is_grpc{false}; bool is_head_request{false}; - bool is_json_content_type{false}; + bool has_json_content_type{false}; }; /** - * @return LocalReplyInfo An LocalReplyInfo populated from the + * @return LocalReplyInfo A populated LocalReplyInfo object from the * Http::HeaderMap& request_headers. */ LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 27976ec047b5a..dcbf7cd89d560 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -554,7 +554,7 @@ TEST(HttpUtility, SendLocalReplyJsonConntentTypeRequest) { data.toString()); })); Utility::LocalReplyInfo info; - info.is_json_content_type = true; + info.has_json_content_type = true; Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", absl::nullopt); } From 73b29978155f6371879a5ae15f41a0c801f80344 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Mon, 25 Mar 2019 12:08:34 +0800 Subject: [PATCH 05/15] Fix code review comments Signed-off-by: tianqian.zyf --- api/envoy/data/core/v2alpha/BUILD | 4 ++-- api/envoy/data/core/v2alpha/local_reply.proto | 13 +++++++++++ .../data/core/v2alpha/local_reply_body.proto | 13 ----------- docs/root/intro/version_history.rst | 1 + source/common/http/BUILD | 2 +- source/common/http/utility.cc | 23 ++++++++++--------- source/common/http/utility.h | 9 ++++---- test/common/http/BUILD | 2 +- test/common/http/utility_test.cc | 11 ++++----- test/integration/BUILD | 2 +- .../idle_timeout_integration_test.cc | 12 +++++----- 11 files changed, 47 insertions(+), 45 deletions(-) create mode 100644 api/envoy/data/core/v2alpha/local_reply.proto delete mode 100644 api/envoy/data/core/v2alpha/local_reply_body.proto diff --git a/api/envoy/data/core/v2alpha/BUILD b/api/envoy/data/core/v2alpha/BUILD index 3db232ffd4b95..11b75fb3f020a 100644 --- a/api/envoy/data/core/v2alpha/BUILD +++ b/api/envoy/data/core/v2alpha/BUILD @@ -15,6 +15,6 @@ api_proto_library( ) api_proto_library_internal( - name = "local_reply_body", - srcs = ["local_reply_body.proto"], + name = "local_reply", + srcs = ["local_reply.proto"], ) diff --git a/api/envoy/data/core/v2alpha/local_reply.proto b/api/envoy/data/core/v2alpha/local_reply.proto new file mode 100644 index 0000000000000..cd4aec5b2ce3f --- /dev/null +++ b/api/envoy/data/core/v2alpha/local_reply.proto @@ -0,0 +1,13 @@ +syntax = "proto3"; + +package envoy.data.core.v2alpha; + +option java_outer_classname = "LocalReplyProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; + +// [#protodoc-title: Local reply] +// Used to wrap a local response, and then convert to JSON. +message LocalReply { + string body = 1; +} diff --git a/api/envoy/data/core/v2alpha/local_reply_body.proto b/api/envoy/data/core/v2alpha/local_reply_body.proto deleted file mode 100644 index e235a45a86009..0000000000000 --- a/api/envoy/data/core/v2alpha/local_reply_body.proto +++ /dev/null @@ -1,13 +0,0 @@ -syntax = "proto3"; - -package envoy.data.core.v2alpha; - -option java_outer_classname = "LocalReplyBodyProto"; -option java_multiple_files = true; -option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; - -// [#protodoc-title: local reply body] -// Used to wrap a local response, and then converted to json format to return. -message LocalReplyBody { - string body = 1; -} diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index db59eeb09e7d0..39fe041003dc1 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -161,6 +161,7 @@ Version history ` to the HTTP fault filter. * governance: extending Envoy deprecation policy from 1 release (0-3 months) to 2 releases (3-6 months). * health check: expected response codes in http health checks are now :ref:`configurable `. +* http: local reply support return json type. * http: added new grpc_http1_reverse_bridge filter for converting gRPC requests into HTTP/1.1 requests. * http: fixed a bug where Content-Length:0 was added to HTTP/1 204 responses. * http: added :ref:`max request headers size `. The default behaviour is unchanged. diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 49bf573b2fc47..6e2aaffc7c8d7 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -307,7 +307,7 @@ envoy_cc_library( "//source/common/protobuf:utility_lib", "@envoy_api//envoy/api/v2/core:http_uri_cc", "@envoy_api//envoy/api/v2/core:protocol_cc", - "@envoy_api//envoy/data/core/v2alpha:local_reply_body_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", ], ) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 14084d7096f6a..bb6413ad0762e 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -6,7 +6,7 @@ #include #include -#include "envoy/data/core/v2alpha/local_reply_body.pb.h" +#include "envoy/data/core/v2alpha/local_reply.pb.h" #include "envoy/http/header_map.h" #include "common/buffer/buffer_impl.h" @@ -281,7 +281,7 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac is_reset, response_code, body_text, grpc_status, is_head_request); ======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { - bool is_grpc = false, is_head_request = false, has_json_content_type = false; + bool is_grpc = false, is_head_request = false, accept_json_type = false; // TODO(zyfjeff): This code is copied from Envoy::Grpc::Common::hasGrpcContentType in order to // rely directly on it to avoid causing circular dependencies. const Http::HeaderEntry* content_type = request_headers.ContentType(); @@ -299,12 +299,13 @@ Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& r } if (!is_grpc) { - has_json_content_type = - content_type != nullptr && - (Http::Headers::get().ContentTypeValues.Json == content_type->value().c_str()); + const Http::HeaderEntry* accept = request_headers.Accept(); + // As long as can accept the json type response priority return json type. + accept_json_type = accept != nullptr && + (accept->value().find(Http::Headers::get().ContentTypeValues.Json.c_str())); } - return LocalReplyInfo{is_grpc, is_head_request, has_json_content_type}; + return LocalReplyInfo{is_grpc, is_head_request, accept_json_type}; } void Utility::sendLocalReply(const Utility::LocalReplyInfo& info, @@ -351,19 +352,19 @@ void Utility::sendLocalReply( HeaderMapPtr response_headers{ new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))}}}; - envoy::data::core::v2alpha::LocalReplyBody local_reply_body; + envoy::data::core::v2alpha::LocalReply local_reply; std::string json_body; absl::string_view finally_body_text = body_text; - if (info.has_json_content_type) { - local_reply_body.set_body(finally_body_text.begin(), finally_body_text.length()); - json_body = MessageUtil::getJsonStringFromMessage(local_reply_body, true, true); + if (info.accept_json_type) { + local_reply.set_body(finally_body_text.begin(), finally_body_text.length()); + json_body = MessageUtil::getJsonStringFromMessage(local_reply, true, true); response_headers->insertContentLength().value(json_body.size()); finally_body_text = absl::string_view(json_body); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); } - if (!info.has_json_content_type && !finally_body_text.empty()) { + if (!info.accept_json_type && !finally_body_text.empty()) { response_headers->insertContentLength().value(finally_body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 5866ae2ac88ab..bd60b15756060 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -178,7 +178,7 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption struct LocalReplyInfo { bool is_grpc{false}; bool is_head_request{false}; - bool has_json_content_type{false}; + bool accept_json_type{false}; }; /** @@ -189,7 +189,8 @@ LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers); /** * Create a locally generated response using filter callbacks. - * @param is_grpc tells if this is a response to a gRPC request. + * @param info used to indicate what type of response is, such as grpc, head response, or json + * content type. * @param callbacks supplies the filter callbacks to use. * @param is_reset boolean reference that indicates whether a stream has been reset. It is the * responsibility of the caller to ensure that this is set to false if onDestroy() @@ -198,7 +199,6 @@ LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers); * @param body_text supplies the optional body text which is sent using the text/plain content * type. * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. - * @param is_head_request tells if this is a response to a HEAD request */ void sendLocalReply(const LocalReplyInfo& info, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, absl::string_view body_text, @@ -206,7 +206,8 @@ void sendLocalReply(const LocalReplyInfo& info, StreamDecoderFilterCallbacks& ca /** * Create a locally generated response using the provided lambdas. - * @param is_grpc tells if this is a response to a gRPC request. + * @param info used to indicate what type of response is, such as grpc, head response, or json + * content type. * @param encode_headers supplies the function to encode response headers. * @param encode_data supplies the function to encode the response body. * @param is_reset boolean reference that indicates whether a stream has been reset. It is the diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 0491c010bc5d2..b92c49a841aca 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -334,7 +334,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", - "@envoy_api//envoy/data/core/v2alpha:local_reply_body_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", "@envoy_api//envoy/api/v2/core:protocol_cc", ], ) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index dcbf7cd89d560..1616fb07677bc 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1,7 +1,7 @@ #include #include -#include "envoy/data/core/v2alpha/local_reply_body.pb.h" +#include "envoy/data/core/v2alpha/local_reply.pb.h" #include "envoy/api/v2/core/protocol.pb.h" #include "envoy/api/v2/core/protocol.pb.validate.h" @@ -548,13 +548,12 @@ TEST(HttpUtility, SendLocalReplyJsonConntentTypeRequest) { })); EXPECT_CALL(callbacks, encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { - envoy::data::core::v2alpha::LocalReplyBody local_reply_body; - local_reply_body.set_body("large"); - EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply_body, true, true), - data.toString()); + envoy::data::core::v2alpha::LocalReply local_reply; + local_reply.set_body("large"); + EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply, true, true), data.toString()); })); Utility::LocalReplyInfo info; - info.has_json_content_type = true; + info.accept_json_type = true; Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", absl::nullopt); } diff --git a/test/integration/BUILD b/test/integration/BUILD index c2945190e80f6..56955e17bb370 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -363,7 +363,7 @@ envoy_cc_test( deps = [ ":http_protocol_integration_lib", "//test/test_common:test_time_lib", - "@envoy_api//envoy/data/core/v2alpha:local_reply_body_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", ], ) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 6018cb356f1e2..65d575d6560e5 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -1,4 +1,4 @@ -#include "envoy/data/core/v2alpha/local_reply_body.pb.h" +#include "envoy/data/core/v2alpha/local_reply.pb.h" #include "test/integration/http_protocol_integration.h" #include "test/test_common/test_time.h" @@ -36,7 +36,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest(const char* method = "GET", - const char* content_type = "text/plain") { + const char* accept_type = "text/plain") { initialize(); fake_upstreams_[0]->set_allow_unexpected_disconnects(true); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); @@ -45,7 +45,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}, - {"content-type", content_type}}); + {"accept", accept_type}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); AssertionResult result = @@ -206,9 +206,9 @@ TEST_P(IdleTimeoutIntegrationTest, EXPECT_STREQ("408", response->headers().Status()->value().c_str()); EXPECT_EQ(Http::Headers::get().ContentTypeValues.Json, response->headers().ContentType()->value().c_str()); - envoy::data::core::v2alpha::LocalReplyBody local_reply_body; - local_reply_body.set_body("stream timeout"); - EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply_body, true, true), response->body()); + envoy::data::core::v2alpha::LocalReply local_reply; + local_reply.set_body("stream timeout"); + EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply, true, true), response->body()); } // Global per-stream idle timeout applies if there is no per-stream idle timeout. From 6b19f4ae948377c71874e3842dee31ecc0958cc8 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Mon, 1 Apr 2019 12:05:08 +0800 Subject: [PATCH 06/15] Addeess code review comments Signed-off-by: tianqian.zyf --- source/common/access_log/access_log_impl.h | 2 +- source/common/grpc/BUILD | 10 ++++++---- source/common/grpc/common.cc | 17 ++--------------- source/common/grpc/common.h | 10 +++------- source/common/grpc/{status.cc => utility.cc} | 17 ++++++++++++++++- source/common/grpc/{status.h => utility.h} | 9 ++++++++- source/common/http/BUILD | 2 +- source/common/http/utility.cc | 15 +++------------ source/common/router/config_impl.cc | 2 +- source/common/router/router.cc | 2 +- source/common/upstream/health_checker_impl.cc | 2 +- .../grpc_http1_bridge/http1_bridge_filter.cc | 2 +- .../http/grpc_http1_reverse_bridge/BUILD | 2 +- .../http/grpc_http1_reverse_bridge/filter.cc | 3 +-- .../http/grpc_http1_reverse_bridge/filter.h | 2 +- .../json_transcoder_filter.cc | 2 +- test/common/grpc/common_test.cc | 4 ++-- 17 files changed, 50 insertions(+), 53 deletions(-) rename source/common/grpc/{status.cc => utility.cc} (74%) rename source/common/grpc/{status.h => utility.h} (79%) diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index 0941602a30b09..6436829340a04 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -10,7 +10,7 @@ #include "envoy/runtime/runtime.h" #include "envoy/server/access_log_config.h" -#include "common/grpc/status.h" +#include "common/grpc/utility.h" #include "common/http/header_utility.h" #include "common/protobuf/protobuf.h" diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 603474289767f..166ed994c9a01 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -63,12 +63,14 @@ envoy_cc_library( ) envoy_cc_library( - name = "status_lib", - srcs = ["status.cc"], - hdrs = ["status.h"], + name = "utility_lib", + srcs = ["utility.cc"], + hdrs = ["utility.h"], external_deps = ["abseil_optional"], deps = [ "//include/envoy/grpc:status", + "//include/envoy/http:header_map_interface", + "//source/common/http:headers_lib", ], ) @@ -91,7 +93,7 @@ envoy_cc_library( "//source/common/common:hash_lib", "//source/common/common:macros", "//source/common/common:utility_lib", - "//source/common/grpc:status_lib", + "//source/common/grpc:utility_lib", "//source/common/http:headers_lib", "//source/common/http:message_lib", "//source/common/http:utility_lib", diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 2683044b715cf..34e7ec0728e41 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -16,28 +16,15 @@ #include "common/common/macros.h" #include "common/common/stack_array.h" #include "common/common/utility.h" +#include "common/grpc/utility.h" #include "common/http/headers.h" #include "common/http/message_impl.h" #include "common/http/utility.h" #include "common/protobuf/protobuf.h" -#include "absl/strings/match.h" - namespace Envoy { namespace Grpc { -bool Common::hasGrpcContentType(const Http::HeaderMap& headers) { - const Http::HeaderEntry* content_type = headers.ContentType(); - // Content type is gRPC if it is exactly "application/grpc" or starts with - // "application/grpc+". Specifically, something like application/grpc-web is not gRPC. - return content_type != nullptr && - absl::StartsWith(content_type->value().getStringView(), - Http::Headers::get().ContentTypeValues.Grpc) && - (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() || - content_type->value() - .getStringView()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); -} - bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream) { if (end_stream) { // Trailers-only response, only grpc-status is required. @@ -46,7 +33,7 @@ bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_strea if (Http::Utility::getResponseStatus(headers) != enumToInt(Http::Code::OK)) { return false; } - return hasGrpcContentType(headers); + return Utility::hasGrpcContentType(headers); } absl::optional Common::getGrpcStatus(const Http::HeaderMap& trailers) { diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index fa5fe72f7bc7f..51fba77a599c2 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -9,8 +9,10 @@ #include "envoy/http/header_map.h" #include "envoy/http/message.h" -#include "common/common/hash.h" #include "common/grpc/status.h" +#include "common/grpc/utility.h" +#include "common/common/hash.h" + #include "common/protobuf/protobuf.h" #include "absl/types/optional.h" @@ -28,12 +30,6 @@ class Exception : public EnvoyException { class Common { public: - /** - * @param headers the headers to parse. - * @return bool indicating whether content-type is gRPC. - */ - static bool hasGrpcContentType(const Http::HeaderMap& headers); - /** * @param headers the headers to parse. * @param bool indicating whether the header is at end_stream. diff --git a/source/common/grpc/status.cc b/source/common/grpc/utility.cc similarity index 74% rename from source/common/grpc/status.cc rename to source/common/grpc/utility.cc index 70fdcce027113..841e79c48f9c2 100644 --- a/source/common/grpc/status.cc +++ b/source/common/grpc/utility.cc @@ -1,4 +1,8 @@ -#include "common/grpc/status.h" +#include "common/grpc/utility.h" + +#include "common/http/headers.h" + +#include "absl/strings/match.h" namespace Envoy { namespace Grpc { @@ -85,5 +89,16 @@ uint64_t Utility::grpcToHttpStatus(Status::GrpcStatus grpc_status) { } } +bool Utility::hasGrpcContentType(const Http::HeaderMap& headers) { + const Http::HeaderEntry* content_type = headers.ContentType(); + // Content type is gRPC if it is exactly "application/grpc" or starts with + // "application/grpc+". Specifically, something like application/grpc-web is not gRPC. + return content_type != nullptr && + absl::StartsWith(content_type->value().getStringView(), + Http::Headers::get().ContentTypeValues.Grpc) && + (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() || + content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); +} + } // namespace Grpc } // namespace Envoy diff --git a/source/common/grpc/status.h b/source/common/grpc/utility.h similarity index 79% rename from source/common/grpc/status.h rename to source/common/grpc/utility.h index 6355e002c2a93..c2434aa358dad 100644 --- a/source/common/grpc/status.h +++ b/source/common/grpc/utility.h @@ -3,12 +3,13 @@ #include #include "envoy/grpc/status.h" +#include "envoy/http/header_map.h" namespace Envoy { namespace Grpc { /** - * Grpc::Status utilities. + * Grpc utilities. */ class Utility { public: @@ -26,6 +27,12 @@ class Utility { * @return uint64_t the canonical HTTP status code corresponding to a gRPC status code. */ static uint64_t grpcToHttpStatus(Status::GrpcStatus grpc_status); + + /** + * @param headers the headers to parse. + * @return bool indicating whether content-type is gRPC. + */ + static bool hasGrpcContentType(const Http::HeaderMap& headers); }; } // namespace Grpc diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 6e2aaffc7c8d7..94951385f6a1f 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -301,7 +301,7 @@ envoy_cc_library( "//source/common/common:empty_string", "//source/common/common:enum_to_int", "//source/common/common:utility_lib", - "//source/common/grpc:status_lib", + "//source/common/grpc:utility_lib", "//source/common/json:json_loader_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index bb6413ad0762e..a3eb5ad19b77b 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -15,7 +15,7 @@ #include "common/common/enum_to_int.h" #include "common/common/fmt.h" #include "common/common/utility.h" -#include "common/grpc/status.h" +#include "common/grpc/utility.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" @@ -282,17 +282,8 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac ======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { bool is_grpc = false, is_head_request = false, accept_json_type = false; - // TODO(zyfjeff): This code is copied from Envoy::Grpc::Common::hasGrpcContentType in order to - // rely directly on it to avoid causing circular dependencies. - const Http::HeaderEntry* content_type = request_headers.ContentType(); - // Content type is gRPC if it is exactly "application/grpc" or starts with - // "application/grpc+". Specifically, something like application/grpc-web is not gRPC. - is_grpc = - content_type != nullptr && - absl::StartsWith(content_type->value().getStringView(), - Http::Headers::get().ContentTypeValues.Grpc) && - (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() || - content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); + + is_grpc = Grpc::Utility::hasGrpcContentType(request_headers); if (Http::Headers::get().MethodValues.Head == request_headers.Method()->value().c_str()) { is_head_request = true; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index dc11c7bde57b7..17cce3029cbde 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -492,7 +492,7 @@ bool RouteEntryImplBase::matchRoute(const Http::HeaderMap& headers, uint64_t ran } if (match_grpc_) { - matches &= Grpc::Common::hasGrpcContentType(headers); + matches &= Grpc::Utility::hasGrpcContentType(headers); } matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 5ad636479afc1..e75bfa214011b 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -329,7 +329,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e : nullptr; // TODO: Maybe add a filter API for this. - grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + grpc_request_ = Grpc::Utility::hasGrpcContentType(headers); // Only increment rq total stat if we actually decode headers here. This does not count requests // that get handled by earlier filters. diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 9e7718b3e35f7..955fee8b3d5e4 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -523,7 +523,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( end_stream); return; } - if (!Grpc::Common::hasGrpcContentType(*headers)) { + if (!Grpc::Utility::hasGrpcContentType(*headers)) { onRpcComplete(Grpc::Status::GrpcStatus::Internal, "invalid gRPC content-type", false); return; } diff --git a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc index 5a9ffc9869d00..3d7c4d107d58f 100644 --- a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc +++ b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc @@ -24,7 +24,7 @@ void Http1BridgeFilter::chargeStat(const Http::HeaderMap& headers) { } Http::FilterHeadersStatus Http1BridgeFilter::decodeHeaders(Http::HeaderMap& headers, bool) { - const bool grpc_request = Grpc::Common::hasGrpcContentType(headers); + const bool grpc_request = Grpc::Utility::hasGrpcContentType(headers); if (grpc_request) { setupStatTracking(headers); } diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/BUILD b/source/extensions/filters/http/grpc_http1_reverse_bridge/BUILD index 4dc282fc3129e..95cf6fa5096dc 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/BUILD +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/BUILD @@ -17,7 +17,7 @@ envoy_cc_library( "//source/common/common:enum_to_int", "//source/common/grpc:codec_lib", "//source/common/grpc:common_lib", - "//source/common/grpc:status_lib", + "//source/common/grpc:utility_lib", "//source/common/http:header_map_lib", "//source/extensions/filters/http:well_known_names", "//source/extensions/filters/http/common:pass_through_filter_lib", diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc index 865c2cb08b6ac..f0b51fa966aa3 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc @@ -3,7 +3,6 @@ #include "common/common/enum_to_int.h" #include "common/grpc/codec.h" #include "common/grpc/common.h" -#include "common/grpc/status.h" #include "common/http/headers.h" #include "common/http/utility.h" @@ -60,7 +59,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // If this is a gRPC request we: // - mark this request as being gRPC // - change the content-type to application/x-protobuf - if (Envoy::Grpc::Common::hasGrpcContentType(headers)) { + if (Envoy::Grpc::Utility::hasGrpcContentType(headers)) { enabled_ = true; // We keep track of the original content-type to ensure that we handle diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h index 169a9ce783e08..20d77c20dc066 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h @@ -5,7 +5,7 @@ #include "envoy/http/filter.h" #include "common/buffer/buffer_impl.h" -#include "common/grpc/status.h" +#include "common/grpc/utility.h" #include "extensions/filters/http/common/pass_through_filter.h" diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index fb26f1f39c0d4..6dcbb0f8b20da 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -172,7 +172,7 @@ ProtobufUtil::Status JsonTranscoderConfig::createTranscoder( const Http::HeaderMap& headers, ZeroCopyInputStream& request_input, google::grpc::transcoding::TranscoderInputStream& response_input, std::unique_ptr& transcoder, const Protobuf::MethodDescriptor*& method_descriptor) { - if (Grpc::Common::hasGrpcContentType(headers)) { + if (Grpc::Utility::hasGrpcContentType(headers)) { return ProtobufUtil::Status(Code::INVALID_ARGUMENT, "Request headers has application/grpc content-type"); } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index b2ab23f919ad4..20f7645fc6d19 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -218,11 +218,11 @@ TEST(GrpcContextTest, HttpToGrpcStatus) { TEST(GrpcContextTest, HasGrpcContentType) { { Http::TestHeaderMapImpl headers{}; - EXPECT_FALSE(Common::hasGrpcContentType(headers)); + EXPECT_FALSE(Grpc::Utility::hasGrpcContentType(headers)); } auto isGrpcContentType = [](const std::string& s) { Http::TestHeaderMapImpl headers{{"content-type", s}}; - return Common::hasGrpcContentType(headers); + return Grpc::Utility::hasGrpcContentType(headers); }; EXPECT_FALSE(isGrpcContentType("")); EXPECT_FALSE(isGrpcContentType("application/text")); From 4a67ec838e37bacb5c28c91376d51f5ffa7e6b60 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Tue, 2 Apr 2019 20:36:21 +0800 Subject: [PATCH 07/15] Address code review comments Signed-off-by: tianqian.zyf --- api/envoy/data/core/v2alpha/local_reply.proto | 2 +- source/common/http/utility.cc | 20 +++++++++---------- test/integration/BUILD | 1 + .../idle_timeout_integration_test.cc | 6 +++++- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/api/envoy/data/core/v2alpha/local_reply.proto b/api/envoy/data/core/v2alpha/local_reply.proto index cd4aec5b2ce3f..356378aa091f0 100644 --- a/api/envoy/data/core/v2alpha/local_reply.proto +++ b/api/envoy/data/core/v2alpha/local_reply.proto @@ -6,7 +6,7 @@ option java_outer_classname = "LocalReplyProto"; option java_multiple_files = true; option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; -// [#protodoc-title: Local reply] +// [#protodoc-title: Local reply] // Used to wrap a local response, and then convert to JSON. message LocalReply { string body = 1; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index a3eb5ad19b77b..b5c4a730d4dab 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -281,9 +281,9 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac is_reset, response_code, body_text, grpc_status, is_head_request); ======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { - bool is_grpc = false, is_head_request = false, accept_json_type = false; + bool is_head_request = false, accept_json_type = false; - is_grpc = Grpc::Utility::hasGrpcContentType(request_headers); + const bool is_grpc = Grpc::Utility::hasGrpcContentType(request_headers); if (Http::Headers::get().MethodValues.Head == request_headers.Method()->value().c_str()) { is_head_request = true; @@ -345,18 +345,18 @@ void Utility::sendLocalReply( envoy::data::core::v2alpha::LocalReply local_reply; std::string json_body; - absl::string_view finally_body_text = body_text; + absl::string_view final_body_text = body_text; if (info.accept_json_type) { - local_reply.set_body(finally_body_text.begin(), finally_body_text.length()); + local_reply.set_body(final_body_text.begin(), final_body_text.length()); json_body = MessageUtil::getJsonStringFromMessage(local_reply, true, true); response_headers->insertContentLength().value(json_body.size()); - finally_body_text = absl::string_view(json_body); + final_body_text = absl::string_view(json_body); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); } - if (!info.accept_json_type && !finally_body_text.empty()) { - response_headers->insertContentLength().value(finally_body_text.size()); + if (!info.accept_json_type && !final_body_text.empty()) { + response_headers->insertContentLength().value(final_body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } @@ -365,10 +365,10 @@ void Utility::sendLocalReply( return; } - encode_headers(std::move(response_headers), finally_body_text.empty()); + encode_headers(std::move(response_headers), final_body_text.empty()); // encode_headers()) may have changed the referenced is_reset so we need to test it - if (!finally_body_text.empty() && !is_reset) { - Buffer::OwnedImpl buffer(finally_body_text); + if (!final_body_text.empty() && !is_reset) { + Buffer::OwnedImpl buffer(final_body_text); encode_data(buffer, true); } } diff --git a/test/integration/BUILD b/test/integration/BUILD index 56955e17bb370..daf0ead7666f9 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -363,6 +363,7 @@ envoy_cc_test( deps = [ ":http_protocol_integration_lib", "//test/test_common:test_time_lib", + "//test/test_common:utility_lib", "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", ], ) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 65d575d6560e5..58940ceeb03b3 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -2,6 +2,7 @@ #include "test/integration/http_protocol_integration.h" #include "test/test_common/test_time.h" +#include "test/test_common/utility.h" namespace Envoy { namespace { @@ -208,7 +209,10 @@ TEST_P(IdleTimeoutIntegrationTest, response->headers().ContentType()->value().c_str()); envoy::data::core::v2alpha::LocalReply local_reply; local_reply.set_body("stream timeout"); - EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply, true, true), response->body()); + envoy::data::core::v2alpha::LocalReply receive_local_reply; + MessageUtil::loadFromJsonEx(response->body(), receive_local_reply, + MessageUtil::proto_unknown_fields); + EXPECT_TRUE(TestUtility::protoEqual(local_reply, receive_local_reply)); } // Global per-stream idle timeout applies if there is no per-stream idle timeout. From e4d6585c01cd13cdb7a9f2cd15bd217aeb8dc3a6 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 May 2019 10:40:22 +0800 Subject: [PATCH 08/15] Fix merge conflict Signed-off-by: tianqian.zyf --- source/common/http/async_client_impl.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 05f8a91659dbe..af0e8dea6e89a 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -110,15 +110,7 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { -<<<<<<< HEAD - if (Http::Headers::get().MethodValues.Head == headers.Method()->value().getStringView()) { - is_head_request_ = true; - } - - is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); -======= local_reply_info_ = Utility::generateLocalReplyInfo(headers); ->>>>>>> Refactor sendLocalReply headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); if (send_xff_) { From 80f4e092e89e4f2e727439e37dd48aa5185ab788 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 May 2019 10:44:46 +0800 Subject: [PATCH 09/15] Fix code format Signed-off-by: tianqian.zyf --- source/common/http/conn_manager_impl.cc | 8 +++++--- source/common/http/utility.cc | 17 ++++++++--------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 961988bb4bb49..575b5258cf2a9 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -686,7 +686,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, request_headers_->Path() && !request_headers_->Path()->value().getStringView().empty(); connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::NotFound, "", nullptr, - absl::nullopt, has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath + absl::nullopt, + has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath : StreamInfo::ResponseCodeDetails::get().MissingPath); return; } @@ -694,8 +695,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Path sanitization should happen before any path access other than the above sanity check. if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_, connection_manager_.config_)) { - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", nullptr, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", + nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); return; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index b5c4a730d4dab..39c8ba9693d32 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -270,15 +270,14 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac const bool& is_reset, Code response_code, absl::string_view body_text, const absl::optional grpc_status, bool is_head_request) { - sendLocalReply( - is_grpc, - [&](HeaderMapPtr&& headers, bool end_stream) -> void { - callbacks.encodeHeaders(std::move(headers), end_stream); - }, - [&](Buffer::Instance& data, bool end_stream) -> void { - callbacks.encodeData(data, end_stream); - }, - is_reset, response_code, body_text, grpc_status, is_head_request); + sendLocalReply(is_grpc, + [&](HeaderMapPtr&& headers, bool end_stream) -> void { + callbacks.encodeHeaders(std::move(headers), end_stream); + }, + [&](Buffer::Instance& data, bool end_stream) -> void { + callbacks.encodeData(data, end_stream); + }, + is_reset, response_code, body_text, grpc_status, is_head_request); ======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { bool is_head_request = false, accept_json_type = false; From 162c95f926752dfc0197b00e6e1630f992f440ad Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 May 2019 14:30:30 +0800 Subject: [PATCH 10/15] Fix merge conflict Signed-off-by: tianqian.zyf --- source/common/grpc/utility.cc | 3 +- source/common/http/conn_manager_impl.cc | 54 ++++++++++++++----------- source/common/http/utility.cc | 39 ++++++------------ 3 files changed, 45 insertions(+), 51 deletions(-) diff --git a/source/common/grpc/utility.cc b/source/common/grpc/utility.cc index 841e79c48f9c2..6d8215e38776e 100644 --- a/source/common/grpc/utility.cc +++ b/source/common/grpc/utility.cc @@ -97,7 +97,8 @@ bool Utility::hasGrpcContentType(const Http::HeaderMap& headers) { absl::StartsWith(content_type->value().getStringView(), Http::Headers::get().ContentTypeValues.Grpc) && (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() || - content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); + content_type->value() + .getStringView()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); } } // namespace Grpc diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 575b5258cf2a9..799cd05e05da3 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -516,7 +516,8 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_) : Utility::LocalReplyInfo{}, - Http::Code::RequestTimeout, "stream timeout", nullptr, absl::nullopt); + Http::Code::RequestTimeout, "stream timeout", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout); } } @@ -524,7 +525,8 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_) : Utility::LocalReplyInfo{}, - Http::Code::RequestTimeout, "request timeout", nullptr, absl::nullopt); + Http::Code::RequestTimeout, "request timeout", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); } void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker( @@ -611,7 +613,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, state_.created_filter_chain_ = true; connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), - Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, absl::nullopt); + Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().Overload); return; } @@ -640,7 +643,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::UpgradeRequired, "", - nullptr, absl::nullopt); + nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().LowVersion); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -663,7 +666,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", - nullptr, absl::nullopt); + nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().MissingHost); return; } } @@ -671,7 +674,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, ASSERT(connection_manager_.config_.maxRequestHeadersKb() > 0); if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), - Code::RequestHeaderFieldsTooLarge, "", nullptr, absl::nullopt); + Code::RequestHeaderFieldsTooLarge, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); return; } @@ -743,7 +747,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Do not allow upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::Forbidden, "", - nullptr, absl::nullopt); + nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().UpgradeFailed); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -1214,29 +1218,31 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { void ConnectionManagerImpl::ActiveStream::sendLocalReply( const Utility::LocalReplyInfo& local_reply_info, Code code, absl::string_view body, const std::function& modify_headers, - const absl::optional grpc_status) { + const absl::optional grpc_status, absl::string_view details) { + ENVOY_STREAM_LOG(debug, "Sending local reply with details {}", *this, details); ASSERT(response_headers_ == nullptr); // For early error handling, do a best-effort attempt to create a filter chain // to ensure access logging. if (!state_.created_filter_chain_) { createFilterChain(); } - Utility::sendLocalReply(local_reply_info, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream); - }, - state_.destroyed_, code, body, grpc_status); + Utility::sendLocalReply( + local_reply_info, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 39c8ba9693d32..5e7fe062e6919 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -265,34 +265,21 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co return ret; } -<<<<<<< HEAD -void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, - const bool& is_reset, Code response_code, absl::string_view body_text, - const absl::optional grpc_status, - bool is_head_request) { - sendLocalReply(is_grpc, - [&](HeaderMapPtr&& headers, bool end_stream) -> void { - callbacks.encodeHeaders(std::move(headers), end_stream); - }, - [&](Buffer::Instance& data, bool end_stream) -> void { - callbacks.encodeData(data, end_stream); - }, - is_reset, response_code, body_text, grpc_status, is_head_request); -======= Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { bool is_head_request = false, accept_json_type = false; const bool is_grpc = Grpc::Utility::hasGrpcContentType(request_headers); - if (Http::Headers::get().MethodValues.Head == request_headers.Method()->value().c_str()) { + if (Http::Headers::get().MethodValues.Head == request_headers.Method()->value().getStringView()) { is_head_request = true; } if (!is_grpc) { const Http::HeaderEntry* accept = request_headers.Accept(); // As long as can accept the json type response priority return json type. - accept_json_type = accept != nullptr && - (accept->value().find(Http::Headers::get().ContentTypeValues.Json.c_str())); + accept_json_type = + accept != nullptr && + (accept->value().getStringView().find(Http::Headers::get().ContentTypeValues.Json.c_str())); } return LocalReplyInfo{is_grpc, is_head_request, accept_json_type}; @@ -302,15 +289,15 @@ void Utility::sendLocalReply(const Utility::LocalReplyInfo& info, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, absl::string_view body_text, const absl::optional grpc_status) { - sendLocalReply(info, - [&](HeaderMapPtr&& headers, bool end_stream) -> void { - callbacks.encodeHeaders(std::move(headers), end_stream); - }, - [&](Buffer::Instance& data, bool end_stream) -> void { - callbacks.encodeData(data, end_stream); - }, - is_reset, response_code, body_text, grpc_status); ->>>>>>> Refactor sendLocalReply + sendLocalReply( + info, + [&](HeaderMapPtr&& headers, bool end_stream) -> void { + callbacks.encodeHeaders(std::move(headers), end_stream); + }, + [&](Buffer::Instance& data, bool end_stream) -> void { + callbacks.encodeData(data, end_stream); + }, + is_reset, response_code, body_text, grpc_status); } void Utility::sendLocalReply( From f305fa739cf74067e8552e19a0afafc216dcd01f Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 May 2019 16:35:00 +0800 Subject: [PATCH 11/15] Refactor local reply Signed-off-by: tianqian.zyf --- .../network/http_connection_manager/v2/BUILD | 2 + .../v2/http_connection_manager.proto | 4 ++ api/envoy/data/core/v2alpha/BUILD | 16 ++++-- .../v2alpha/local_reply_configuration.proto | 23 +++++++++ source/common/http/BUILD | 3 +- source/common/http/async_client_impl.cc | 3 +- source/common/http/conn_manager_config.h | 7 +++ source/common/http/conn_manager_impl.cc | 49 ++++++++++++------- source/common/http/conn_manager_impl.h | 3 +- source/common/http/utility.cc | 40 ++++++++------- source/common/http/utility.h | 6 ++- .../network/http_connection_manager/config.cc | 16 ++++++ .../network/http_connection_manager/config.h | 3 ++ source/server/http/admin.h | 1 + test/common/http/BUILD | 2 +- test/common/http/conn_manager_utility_test.cc | 1 + test/common/http/utility_test.cc | 8 +-- test/integration/BUILD | 2 +- .../idle_timeout_integration_test.cc | 18 ++++--- 19 files changed, 150 insertions(+), 57 deletions(-) create mode 100644 api/envoy/data/core/v2alpha/local_reply_configuration.proto diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/BUILD b/api/envoy/config/filter/network/http_connection_manager/v2/BUILD index 95d3811f426af..98b2514aa12ed 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/BUILD +++ b/api/envoy/config/filter/network/http_connection_manager/v2/BUILD @@ -12,6 +12,7 @@ api_proto_library_internal( "//envoy/api/v2/core:config_source", "//envoy/api/v2/core:protocol", "//envoy/config/filter/accesslog/v2:accesslog", + "//envoy/data/core/v2alpha:local_reply_configuration", "//envoy/type:percent", ], ) @@ -26,6 +27,7 @@ api_go_proto_library( "//envoy/api/v2/core:config_source_go_proto", "//envoy/api/v2/core:protocol_go_proto", "//envoy/config/filter/accesslog/v2:accesslog_go_proto", + "//envoy/data/core/v2alpha:local_reply_configuration_go_proto", "//envoy/type:percent_go_proto", ], ) diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index f0c7b0f59e1ef..69c9d0309248f 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -12,6 +12,7 @@ import "envoy/api/v2/core/protocol.proto"; import "envoy/api/v2/rds.proto"; import "envoy/api/v2/srds.proto"; import "envoy/config/filter/accesslog/v2/accesslog.proto"; +import "envoy/data/core/v2alpha/local_reply_configuration.proto"; import "envoy/type/percent.proto"; import "google/protobuf/any.proto"; @@ -424,6 +425,9 @@ message HttpConnectionManager { // Note that Envoy does not perform // `case normalization ` google.protobuf.BoolValue normalize_path = 30; + + // Used to configure the Local reply the type of the body + envoy.data.core.v2alpha.LocalReplyConfiguration local_reply_config = 32; } message Rds { diff --git a/api/envoy/data/core/v2alpha/BUILD b/api/envoy/data/core/v2alpha/BUILD index 11b75fb3f020a..8435c024bf93d 100644 --- a/api/envoy/data/core/v2alpha/BUILD +++ b/api/envoy/data/core/v2alpha/BUILD @@ -1,4 +1,4 @@ -load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library", "api_proto_library_internal") +load("@envoy_api//bazel:api_build_system.bzl", "api_go_proto_library", "api_proto_library") licenses(["notice"]) # Apache 2 @@ -14,7 +14,15 @@ api_proto_library( ], ) -api_proto_library_internal( - name = "local_reply", - srcs = ["local_reply.proto"], +api_proto_library( + name = "local_reply_configuration", + srcs = ["local_reply_configuration.proto"], + visibility = [ + "//visibility:public", + ], +) + +api_go_proto_library( + name = "local_reply_configuration", + proto = ":local_reply_configuration", ) diff --git a/api/envoy/data/core/v2alpha/local_reply_configuration.proto b/api/envoy/data/core/v2alpha/local_reply_configuration.proto new file mode 100644 index 0000000000000..0150647ebcff6 --- /dev/null +++ b/api/envoy/data/core/v2alpha/local_reply_configuration.proto @@ -0,0 +1,23 @@ +syntax = "proto3"; + +package envoy.data.core.v2alpha; + +option java_outer_classname = "LocalReplyConfigurationProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; + +// [#protodoc-title: Local reply configuration] +// Behavior for configuring local replies +message LocalReplyConfiguration { + message JsonReply { + string body = 1; + } + + // If not set, the default is always_text + oneof reply_type { + // [#not-implemented-hide:] + bool determine_via_accept_header = 2; + bool always_json = 3; + bool always_text = 4; + } +} diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 94951385f6a1f..43727028a660a 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -288,6 +288,7 @@ envoy_cc_library( "http_parser", ], deps = [ + ":conn_manager_config_interface", ":exception_lib", ":header_map_lib", ":headers_lib", @@ -307,7 +308,7 @@ envoy_cc_library( "//source/common/protobuf:utility_lib", "@envoy_api//envoy/api/v2/core:http_uri_cc", "@envoy_api//envoy/api/v2/core:protocol_cc", - "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_configuration_cc", ], ) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index af0e8dea6e89a..118cdf0cf8384 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -110,7 +110,8 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { - local_reply_info_ = Utility::generateLocalReplyInfo(headers); + // TODO(zyfjeff): Make the local reply body type can be configured + local_reply_info_ = Utility::generateLocalReplyInfo(headers, Http::LocalReplyType::AlwaysText); headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); if (send_xff_) { diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 78ebc7a19161c..c59f5369fce86 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -165,6 +165,8 @@ class DefaultInternalAddressConfig : public Http::InternalAddressConfig { } }; +enum class LocalReplyType { DetermineViaAcceptHeader, AlwaysJson, AlwaysText }; + /** * Abstract configuration for the connection manager. */ @@ -357,6 +359,11 @@ class ConnectionManagerConfig { * @return if the HttpConnectionManager should normalize url following RFC3986 */ virtual bool shouldNormalizePath() const PURE; + + /* + * @return LocalReplyType the Local reply body type + */ + virtual LocalReplyType localReplyType() const PURE; }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 799cd05e05da3..d2b65c369d95d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -514,8 +514,10 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { } else { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout); - sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_) - : Utility::LocalReplyInfo{}, + sendLocalReply(request_headers_ != nullptr + ? Utility::generateLocalReplyInfo( + *request_headers_, connection_manager_.config_.localReplyType()) + : Utility::LocalReplyInfo{}, Http::Code::RequestTimeout, "stream timeout", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout); } @@ -523,8 +525,10 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); - sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_) - : Utility::LocalReplyInfo{}, + sendLocalReply(request_headers_ != nullptr + ? Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()) + : Utility::LocalReplyInfo{}, Http::Code::RequestTimeout, "request timeout", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); } @@ -612,7 +616,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // overload it is more important to avoid unnecessary allocation than to create the filters. state_.created_filter_chain_ = true; connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().Overload); return; @@ -642,8 +647,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, stream_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::UpgradeRequired, "", - nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().LowVersion); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), + Code::UpgradeRequired, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().LowVersion); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -665,15 +672,18 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.config_.http1Settings().default_host_for_http_10_); } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", - nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().MissingHost); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), + Code::BadRequest, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().MissingHost); return; } } ASSERT(connection_manager_.config_.maxRequestHeadersKb() > 0); if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), Code::RequestHeaderFieldsTooLarge, "", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); return; @@ -689,8 +699,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, const bool has_path = request_headers_->Path() && !request_headers_->Path()->value().getStringView().empty(); connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::NotFound, "", nullptr, - absl::nullopt, + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), + Code::NotFound, "", nullptr, absl::nullopt, has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath : StreamInfo::ResponseCodeDetails::get().MissingPath); return; @@ -699,8 +710,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Path sanitization should happen before any path access other than the above sanity check. if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_, connection_manager_.config_)) { - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::BadRequest, "", - nullptr, absl::nullopt, + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), + Code::BadRequest, "", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); return; } @@ -746,8 +758,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (upgrade_rejected) { // Do not allow upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_), Code::Forbidden, "", - nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().UpgradeFailed); + sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.localReplyType()), + Code::Forbidden, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().UpgradeFailed); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -2130,7 +2144,8 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.stream_info_.setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); Http::Utility::sendLocalReply( - Utility::generateLocalReplyInfo(*parent_.request_headers_), + Utility::generateLocalReplyInfo(*parent_.request_headers_, + parent_.connection_manager_.config_.localReplyType()), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { parent_.chargeStats(*response_headers); parent_.response_headers_ = std::move(response_headers); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index a76523b0988ce..f72b745891da0 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -296,7 +296,8 @@ class ConnectionManagerImpl : Logger::Loggable, // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() // called here may change the content type, so we must check it before the call. FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { - local_reply_info_ = Utility::generateLocalReplyInfo(headers); + local_reply_info_ = Utility::generateLocalReplyInfo( + headers, parent_.connection_manager_.config_.localReplyType()); FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); if (end_stream) { handle_->decodeComplete(); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5e7fe062e6919..6228d400474d1 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -6,7 +6,7 @@ #include #include -#include "envoy/data/core/v2alpha/local_reply.pb.h" +#include "envoy/data/core/v2alpha/local_reply_configuration.pb.h" #include "envoy/http/header_map.h" #include "common/buffer/buffer_impl.h" @@ -265,8 +265,9 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co return ret; } -Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers) { - bool is_head_request = false, accept_json_type = false; +Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers, + const Http::LocalReplyType& reply_type) { + bool is_head_request = false; const bool is_grpc = Grpc::Utility::hasGrpcContentType(request_headers); @@ -274,15 +275,7 @@ Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& r is_head_request = true; } - if (!is_grpc) { - const Http::HeaderEntry* accept = request_headers.Accept(); - // As long as can accept the json type response priority return json type. - accept_json_type = - accept != nullptr && - (accept->value().getStringView().find(Http::Headers::get().ContentTypeValues.Json.c_str())); - } - - return LocalReplyInfo{is_grpc, is_head_request, accept_json_type}; + return LocalReplyInfo{is_grpc, is_head_request, reply_type}; } void Utility::sendLocalReply(const Utility::LocalReplyInfo& info, @@ -329,21 +322,30 @@ void Utility::sendLocalReply( HeaderMapPtr response_headers{ new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))}}}; - envoy::data::core::v2alpha::LocalReply local_reply; std::string json_body; absl::string_view final_body_text = body_text; - - if (info.accept_json_type) { + switch (info.reply_type) { + case Http::LocalReplyType::AlwaysJson: { + std::string json_body; + envoy::data::core::v2alpha::LocalReplyConfiguration::JsonReply local_reply; local_reply.set_body(final_body_text.begin(), final_body_text.length()); json_body = MessageUtil::getJsonStringFromMessage(local_reply, true, true); response_headers->insertContentLength().value(json_body.size()); final_body_text = absl::string_view(json_body); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); + break; } - - if (!info.accept_json_type && !final_body_text.empty()) { - response_headers->insertContentLength().value(final_body_text.size()); - response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); + case Http::LocalReplyType::AlwaysText: { + if (!final_body_text.empty()) { + response_headers->insertContentLength().value(final_body_text.size()); + response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); + } + break; + } + case Http::LocalReplyType::DetermineViaAcceptHeader: + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + default: + NOT_REACHED_GCOVR_EXCL_LINE; } if (info.is_head_request) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index bd60b15756060..8a0b5dc1a355b 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -13,6 +13,7 @@ #include "envoy/http/metadata_interface.h" #include "envoy/http/query_params.h" +#include "common/http/conn_manager_config.h" #include "common/json/json_loader.h" #include "absl/strings/string_view.h" @@ -178,14 +179,15 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption struct LocalReplyInfo { bool is_grpc{false}; bool is_head_request{false}; - bool accept_json_type{false}; + Http::LocalReplyType reply_type{Http::LocalReplyType::AlwaysText}; }; /** * @return LocalReplyInfo A populated LocalReplyInfo object from the * Http::HeaderMap& request_headers. */ -LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers); +LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers, + const Http::LocalReplyType& reply_type); /** * Create a locally generated response using filter callbacks. diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index f5596e83bfe0a..86338ea4cf202 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -330,6 +330,22 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( std::make_pair(name, FilterConfig{std::move(factories), enabled})); } } + + if (config.has_local_reply_config()) { + switch (config.local_reply_config().reply_type_case()) { + case envoy::data::core::v2alpha::LocalReplyConfiguration::kAlwaysJson: + local_reply_type_ = Http::LocalReplyType::AlwaysJson; + break; + case envoy::data::core::v2alpha::LocalReplyConfiguration::kAlwaysText: + local_reply_type_ = Http::LocalReplyType::AlwaysJson; + break; + case envoy::data::core::v2alpha::LocalReplyConfiguration::kDetermineViaAcceptHeader: + local_reply_type_ = Http::LocalReplyType::DetermineViaAcceptHeader; + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } + } } void HttpConnectionManagerConfig::processFilter( diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index eb97a9bf21dbe..8f752c4a9fc5c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -141,6 +141,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, bool shouldNormalizePath() const override { return normalize_path_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } + Http::LocalReplyType localReplyType() const override { return local_reply_type_; } + private: enum class CodecType { HTTP1, HTTP2, AUTO }; void processFilter( @@ -183,6 +185,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const bool proxy_100_continue_; std::chrono::milliseconds delayed_close_timeout_; const bool normalize_path_; + Http::LocalReplyType local_reply_type_{Http::LocalReplyType::AlwaysText}; // Default idle timeout is 5 minutes if nothing is specified in the HCM config. static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 9d1da1e8700da..f03a4296b4004 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -148,6 +148,7 @@ class AdminImpl : public Admin, Http::HeaderMap& response_headers, std::string& body) override; void closeSocket(); void addListenerToHandler(Network::ConnectionHandler* handler) override; + Http::LocalReplyType localReplyType() const override { return Http::LocalReplyType::AlwaysText; } private: /** diff --git a/test/common/http/BUILD b/test/common/http/BUILD index b92c49a841aca..632719b1dbb1d 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -334,7 +334,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", - "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_configuration_cc", "@envoy_api//envoy/api/v2/core:protocol_cc", ], ) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 48fa93d141743..190b95556ac42 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -85,6 +85,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_CONST_METHOD0(proxy100Continue, bool()); MOCK_CONST_METHOD0(http1Settings, const Http::Http1Settings&()); MOCK_CONST_METHOD0(shouldNormalizePath, bool()); + MOCK_CONST_METHOD0(localReplyType, Http::LocalReplyType()); std::unique_ptr internal_address_config_ = std::make_unique(); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 1616fb07677bc..99f0dca35aa5d 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1,7 +1,7 @@ #include #include -#include "envoy/data/core/v2alpha/local_reply.pb.h" +#include "envoy/data/core/v2alpha/local_reply_configuration.pb.h" #include "envoy/api/v2/core/protocol.pb.h" #include "envoy/api/v2/core/protocol.pb.validate.h" @@ -543,17 +543,17 @@ TEST(HttpUtility, SendLocalReplyJsonConntentTypeRequest) { bool is_reset = false; EXPECT_CALL(callbacks, encodeHeaders_(_, false)) .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { - EXPECT_EQ(headers.ContentType()->value().c_str(), + EXPECT_EQ(headers.ContentType()->value().getStringView(), Http::Headers::get().ContentTypeValues.Json); })); EXPECT_CALL(callbacks, encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { - envoy::data::core::v2alpha::LocalReply local_reply; + envoy::data::core::v2alpha::LocalReplyConfiguration::JsonReply local_reply; local_reply.set_body("large"); EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply, true, true), data.toString()); })); Utility::LocalReplyInfo info; - info.accept_json_type = true; + info.reply_type = Http::LocalReplyType::AlwaysJson; Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", absl::nullopt); } diff --git a/test/integration/BUILD b/test/integration/BUILD index daf0ead7666f9..3777b20430628 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -364,7 +364,7 @@ envoy_cc_test( ":http_protocol_integration_lib", "//test/test_common:test_time_lib", "//test/test_common:utility_lib", - "@envoy_api//envoy/data/core/v2alpha:local_reply_cc", + "@envoy_api//envoy/data/core/v2alpha:local_reply_configuration_cc", ], ) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 58940ceeb03b3..b08695e38476d 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -1,4 +1,4 @@ -#include "envoy/data/core/v2alpha/local_reply.pb.h" +#include "envoy/data/core/v2alpha/local_reply_configuration.pb.h" #include "test/integration/http_protocol_integration.h" #include "test/test_common/test_time.h" @@ -197,19 +197,25 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstrea TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstreamJsonContentTypeRequest) { + config_helper_.addConfigModifier( + [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { + hcm.mutable_local_reply_config()->set_always_json(true); + }; + enable_per_stream_idle_timeout_ = true; - auto response = setupPerStreamIdleTimeoutTest("GET", "application/json"); + auto response = setupPerStreamIdleTimeoutTest("GET"); waitForTimeout(*response, "downstream_rq_idle_timeout"); EXPECT_FALSE(upstream_request_->complete()); EXPECT_EQ(0U, upstream_request_->bodyLength()); EXPECT_TRUE(response->complete()); - EXPECT_STREQ("408", response->headers().Status()->value().c_str()); + EXPECT_EQ("408", response->headers().Status()->value().getStringView()); EXPECT_EQ(Http::Headers::get().ContentTypeValues.Json, - response->headers().ContentType()->value().c_str()); - envoy::data::core::v2alpha::LocalReply local_reply; + response->headers().ContentType()->value().getStringView()); + envoy::data::core::v2alpha::LocalReplyConfiguration::JsonReply local_reply; local_reply.set_body("stream timeout"); - envoy::data::core::v2alpha::LocalReply receive_local_reply; + envoy::data::core::v2alpha::LocalReplyConfiguration::JsonReply receive_local_reply; MessageUtil::loadFromJsonEx(response->body(), receive_local_reply, MessageUtil::proto_unknown_fields); EXPECT_TRUE(TestUtility::protoEqual(local_reply, receive_local_reply)); From 67b56c9d7a922d5235d95db1a1e348883cb5790b Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 May 2019 17:08:59 +0800 Subject: [PATCH 12/15] Fix doc failed Signed-off-by: tianqian.zyf --- api/envoy/data/core/v2alpha/local_reply.proto | 13 ------------- .../core/v2alpha/local_reply_configuration.proto | 2 +- docs/root/api-v2/data/core/core.rst | 1 + test/integration/idle_timeout_integration_test.cc | 4 +--- 4 files changed, 3 insertions(+), 17 deletions(-) delete mode 100644 api/envoy/data/core/v2alpha/local_reply.proto diff --git a/api/envoy/data/core/v2alpha/local_reply.proto b/api/envoy/data/core/v2alpha/local_reply.proto deleted file mode 100644 index 356378aa091f0..0000000000000 --- a/api/envoy/data/core/v2alpha/local_reply.proto +++ /dev/null @@ -1,13 +0,0 @@ -syntax = "proto3"; - -package envoy.data.core.v2alpha; - -option java_outer_classname = "LocalReplyProto"; -option java_multiple_files = true; -option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; - -// [#protodoc-title: Local reply] -// Used to wrap a local response, and then convert to JSON. -message LocalReply { - string body = 1; -} diff --git a/api/envoy/data/core/v2alpha/local_reply_configuration.proto b/api/envoy/data/core/v2alpha/local_reply_configuration.proto index 0150647ebcff6..e0a8683dc3338 100644 --- a/api/envoy/data/core/v2alpha/local_reply_configuration.proto +++ b/api/envoy/data/core/v2alpha/local_reply_configuration.proto @@ -7,7 +7,7 @@ option java_multiple_files = true; option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; // [#protodoc-title: Local reply configuration] -// Behavior for configuring local replies + message LocalReplyConfiguration { message JsonReply { string body = 1; diff --git a/docs/root/api-v2/data/core/core.rst b/docs/root/api-v2/data/core/core.rst index f9d7e77bf4d71..09f7166155bb5 100644 --- a/docs/root/api-v2/data/core/core.rst +++ b/docs/root/api-v2/data/core/core.rst @@ -6,3 +6,4 @@ Core data :maxdepth: 2 v2alpha/health_check_event.proto + v2alpha/local_reply_configuration.proto diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index b08695e38476d..6868298056276 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -199,9 +199,7 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstreamJsonContentTypeRequest) { config_helper_.addConfigModifier( [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) - -> void { - hcm.mutable_local_reply_config()->set_always_json(true); - }; + -> void { hcm.mutable_local_reply_config()->set_always_json(true); }); enable_per_stream_idle_timeout_ = true; auto response = setupPerStreamIdleTimeoutTest("GET"); From 20542e11428a4a54707c00358aaa2d42f3f3b31a Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 May 2019 19:30:13 +0800 Subject: [PATCH 13/15] Fix compile error Signed-off-by: tianqian.zyf --- .../v2alpha/local_reply_configuration.proto | 1 + source/common/http/conn_manager_impl.cc | 1 + source/common/http/utility.cc | 18 +++++++++--------- .../network/http_connection_manager/config.cc | 3 +++ .../common/http/conn_manager_impl_fuzz_test.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 1 + .../idle_timeout_integration_test.cc | 7 ++----- 7 files changed, 18 insertions(+), 15 deletions(-) diff --git a/api/envoy/data/core/v2alpha/local_reply_configuration.proto b/api/envoy/data/core/v2alpha/local_reply_configuration.proto index e0a8683dc3338..df1f82e6dcdde 100644 --- a/api/envoy/data/core/v2alpha/local_reply_configuration.proto +++ b/api/envoy/data/core/v2alpha/local_reply_configuration.proto @@ -8,6 +8,7 @@ option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; // [#protodoc-title: Local reply configuration] +// Configure the local reply of the body type message LocalReplyConfiguration { message JsonReply { string body = 1; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d2b65c369d95d..a551e4b2171bb 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1240,6 +1240,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( if (!state_.created_filter_chain_) { createFilterChain(); } + stream_info_.setResponseCodeDetails(details); Utility::sendLocalReply( local_reply_info, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 6228d400474d1..9f4698d2e1ad9 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -322,22 +322,22 @@ void Utility::sendLocalReply( HeaderMapPtr response_headers{ new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))}}}; - std::string json_body; - absl::string_view final_body_text = body_text; + std::string body(body_text); + switch (info.reply_type) { case Http::LocalReplyType::AlwaysJson: { std::string json_body; envoy::data::core::v2alpha::LocalReplyConfiguration::JsonReply local_reply; - local_reply.set_body(final_body_text.begin(), final_body_text.length()); + local_reply.set_body(body.c_str(), body.size()); json_body = MessageUtil::getJsonStringFromMessage(local_reply, true, true); response_headers->insertContentLength().value(json_body.size()); - final_body_text = absl::string_view(json_body); + body = std::move(json_body); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); break; } case Http::LocalReplyType::AlwaysText: { - if (!final_body_text.empty()) { - response_headers->insertContentLength().value(final_body_text.size()); + if (!body_text.empty()) { + response_headers->insertContentLength().value(body.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } break; @@ -353,10 +353,10 @@ void Utility::sendLocalReply( return; } - encode_headers(std::move(response_headers), final_body_text.empty()); + encode_headers(std::move(response_headers), body.empty()); // encode_headers()) may have changed the referenced is_reset so we need to test it - if (!final_body_text.empty() && !is_reset) { - Buffer::OwnedImpl buffer(final_body_text); + if (!body.empty() && !is_reset) { + Buffer::OwnedImpl buffer(body); encode_data(buffer, true); } } diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 86338ea4cf202..64dc45b8203f0 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -342,6 +342,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( case envoy::data::core::v2alpha::LocalReplyConfiguration::kDetermineViaAcceptHeader: local_reply_type_ = Http::LocalReplyType::DetermineViaAcceptHeader; break; + case envoy::data::core::v2alpha::LocalReplyConfiguration::REPLY_TYPE_NOT_SET: + local_reply_type_ = Http::LocalReplyType::AlwaysJson; + break; default: NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index e15e88e97473b..64ae7564eaad2 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -110,7 +110,7 @@ class FuzzConfig : public ConnectionManagerConfig { bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return false; } - + LocalReplyType localReplyType() const override { return LocalReplyType::AlwaysText; } const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager config_; std::list access_logs_; MockServerConnection* codec_{}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 7f2abde9ba580..8bc20a7d5b8aa 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -279,6 +279,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } + LocalReplyType localReplyType() const override { return LocalReplyType::AlwaysText; } DangerousDeprecatedTestTime test_time_; ConnectionManagerImplHelper::RouteConfigProvider route_config_provider_; diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 6868298056276..77248150ec869 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -35,9 +35,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { HttpProtocolIntegrationTest::initialize(); } - IntegrationStreamDecoderPtr - setupPerStreamIdleTimeoutTest(const char* method = "GET", - const char* accept_type = "text/plain") { + IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest(const char* method = "GET") { initialize(); fake_upstreams_[0]->set_allow_unexpected_disconnects(true); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); @@ -45,8 +43,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", method}, {":path", "/test/long/url"}, {":scheme", "http"}, - {":authority", "host"}, - {"accept", accept_type}}); + {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); AssertionResult result = From 395a0f8008cc9fca7b109d286c489e5c73ef7803 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Fri, 10 May 2019 15:45:38 +0800 Subject: [PATCH 14/15] Fix doc error Signed-off-by: tianqian.zyf --- docs/build.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/build.sh b/docs/build.sh index 8a4c7850908f7..f29663203fb56 100755 --- a/docs/build.sh +++ b/docs/build.sh @@ -137,6 +137,7 @@ PROTO_RST=" /envoy/config/transport_socket/tap/v2alpha/tap/envoy/config/transport_socket/tap/v2alpha/tap.proto.rst /envoy/data/accesslog/v2/accesslog/envoy/data/accesslog/v2/accesslog.proto.rst /envoy/data/core/v2alpha/health_check_event/envoy/data/core/v2alpha/health_check_event.proto.rst + /envoy/data/core/v2alpha/local_reply_configuration/envoy/data/core/v2alpha/local_reply_configuration.proto.rst /envoy/data/tap/v2alpha/common/envoy/data/tap/v2alpha/common.proto.rst /envoy/data/tap/v2alpha/transport/envoy/data/tap/v2alpha/transport.proto.rst /envoy/data/tap/v2alpha/http/envoy/data/tap/v2alpha/http.proto.rst From c5c159807c7e6848f102ea8f06afb42734099fed Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Mon, 27 May 2019 00:39:11 +0800 Subject: [PATCH 15/15] Fix code review comments Signed-off-by: tianqian.zyf --- .../v2alpha/local_reply_configuration.proto | 12 ++--- source/common/http/async_client_impl.cc | 4 +- source/common/http/conn_manager_config.h | 6 +-- source/common/http/conn_manager_impl.cc | 48 +++++++++---------- source/common/http/conn_manager_impl.h | 4 +- source/common/http/utility.cc | 18 +++---- source/common/http/utility.h | 8 ++-- .../network/http_connection_manager/config.cc | 17 +++---- .../network/http_connection_manager/config.h | 4 +- source/server/http/admin.h | 2 +- .../http/conn_manager_impl_fuzz_test.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 2 +- test/common/http/conn_manager_utility_test.cc | 2 +- test/common/http/utility_test.cc | 8 ++-- .../reverse_bridge_test.cc | 14 +++--- .../idle_timeout_integration_test.cc | 5 +- 16 files changed, 78 insertions(+), 78 deletions(-) diff --git a/api/envoy/data/core/v2alpha/local_reply_configuration.proto b/api/envoy/data/core/v2alpha/local_reply_configuration.proto index df1f82e6dcdde..402b9b9e276e9 100644 --- a/api/envoy/data/core/v2alpha/local_reply_configuration.proto +++ b/api/envoy/data/core/v2alpha/local_reply_configuration.proto @@ -14,11 +14,11 @@ message LocalReplyConfiguration { string body = 1; } - // If not set, the default is always_text - oneof reply_type { - // [#not-implemented-hide:] - bool determine_via_accept_header = 2; - bool always_json = 3; - bool always_text = 4; + enum MediaType { + TEXT_PLAIN = 0; + APPLICATION_JSON = 1; + NEGOTIATE_VIA_ACCEPT_HEADER = 2; } + + MediaType media_type = 1; } diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 118cdf0cf8384..4fd352755f47c 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -110,8 +110,8 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { - // TODO(zyfjeff): Make the local reply body type can be configured - local_reply_info_ = Utility::generateLocalReplyInfo(headers, Http::LocalReplyType::AlwaysText); + // TODO(zyfjeff): Make the local reply body type configurable + local_reply_info_ = Utility::generateLocalReplyInfo(headers, Http::MediaType::TextPlain); headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); if (send_xff_) { diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index c59f5369fce86..33f417f784e74 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -165,7 +165,7 @@ class DefaultInternalAddressConfig : public Http::InternalAddressConfig { } }; -enum class LocalReplyType { DetermineViaAcceptHeader, AlwaysJson, AlwaysText }; +enum class MediaType { TextPlain, ApplicationJson, NegotiateViaAcceptHeader }; /** * Abstract configuration for the connection manager. @@ -361,9 +361,9 @@ class ConnectionManagerConfig { virtual bool shouldNormalizePath() const PURE; /* - * @return LocalReplyType the Local reply body type + * @return MediaType the Local reply body media type */ - virtual LocalReplyType localReplyType() const PURE; + virtual Http::MediaType mediaType() const PURE; }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index a551e4b2171bb..f1927d2d31d77 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -515,8 +515,8 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout); sendLocalReply(request_headers_ != nullptr - ? Utility::generateLocalReplyInfo( - *request_headers_, connection_manager_.config_.localReplyType()) + ? Utility::generateLocalReplyInfo(*request_headers_, + connection_manager_.config_.mediaType()) : Utility::LocalReplyInfo{}, Http::Code::RequestTimeout, "stream timeout", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout); @@ -527,7 +527,7 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); sendLocalReply(request_headers_ != nullptr ? Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()) + connection_manager_.config_.mediaType()) : Utility::LocalReplyInfo{}, Http::Code::RequestTimeout, "request timeout", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); @@ -616,10 +616,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // overload it is more important to avoid unnecessary allocation than to create the filters. state_.created_filter_chain_ = true; connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), - Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().Overload); + sendLocalReply( + Utility::generateLocalReplyInfo(*request_headers_, connection_manager_.config_.mediaType()), + Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().Overload); return; } @@ -648,7 +648,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), + connection_manager_.config_.mediaType()), Code::UpgradeRequired, "", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().LowVersion); return; @@ -673,7 +673,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), + connection_manager_.config_.mediaType()), Code::BadRequest, "", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().MissingHost); return; @@ -682,10 +682,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, ASSERT(connection_manager_.config_.maxRequestHeadersKb() > 0); if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), - Code::RequestHeaderFieldsTooLarge, "", nullptr, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); + sendLocalReply( + Utility::generateLocalReplyInfo(*request_headers_, connection_manager_.config_.mediaType()), + Code::RequestHeaderFieldsTooLarge, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); return; } @@ -699,21 +699,21 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, const bool has_path = request_headers_->Path() && !request_headers_->Path()->value().getStringView().empty(); connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), - Code::NotFound, "", nullptr, absl::nullopt, - has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath - : StreamInfo::ResponseCodeDetails::get().MissingPath); + sendLocalReply( + Utility::generateLocalReplyInfo(*request_headers_, connection_manager_.config_.mediaType()), + Code::NotFound, "", nullptr, absl::nullopt, + has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath + : StreamInfo::ResponseCodeDetails::get().MissingPath); return; } // Path sanitization should happen before any path access other than the above sanity check. if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_, connection_manager_.config_)) { - sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), - Code::BadRequest, "", nullptr, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); + sendLocalReply( + Utility::generateLocalReplyInfo(*request_headers_, connection_manager_.config_.mediaType()), + Code::BadRequest, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); return; } @@ -759,7 +759,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Do not allow upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Utility::generateLocalReplyInfo(*request_headers_, - connection_manager_.config_.localReplyType()), + connection_manager_.config_.mediaType()), Code::Forbidden, "", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().UpgradeFailed); return; @@ -2146,7 +2146,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); Http::Utility::sendLocalReply( Utility::generateLocalReplyInfo(*parent_.request_headers_, - parent_.connection_manager_.config_.localReplyType()), + parent_.connection_manager_.config_.mediaType()), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { parent_.chargeStats(*response_headers); parent_.response_headers_ = std::move(response_headers); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index f72b745891da0..20d337f777130 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -296,8 +296,8 @@ class ConnectionManagerImpl : Logger::Loggable, // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() // called here may change the content type, so we must check it before the call. FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { - local_reply_info_ = Utility::generateLocalReplyInfo( - headers, parent_.connection_manager_.config_.localReplyType()); + local_reply_info_ = + Utility::generateLocalReplyInfo(headers, parent_.connection_manager_.config_.mediaType()); FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); if (end_stream) { handle_->decodeComplete(); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 9f4698d2e1ad9..cd4457ec86f85 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -266,7 +266,7 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co } Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& request_headers, - const Http::LocalReplyType& reply_type) { + const Http::MediaType& media_type) { bool is_head_request = false; const bool is_grpc = Grpc::Utility::hasGrpcContentType(request_headers); @@ -275,7 +275,7 @@ Utility::LocalReplyInfo Utility::generateLocalReplyInfo(const Http::HeaderMap& r is_head_request = true; } - return LocalReplyInfo{is_grpc, is_head_request, reply_type}; + return LocalReplyInfo{is_grpc, is_head_request, media_type}; } void Utility::sendLocalReply(const Utility::LocalReplyInfo& info, @@ -302,7 +302,7 @@ void Utility::sendLocalReply( // encode_headers() may reset the stream, so the stream must not be reset before calling it. ASSERT(!is_reset); // Respond with a gRPC trailers-only response if the request is gRPC - if (info.is_grpc) { + if (info.is_grpc_) { HeaderMapPtr response_headers{new HeaderMapImpl{ {Headers::get().Status, std::to_string(enumToInt(Code::OK))}, {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc}, @@ -310,7 +310,7 @@ void Utility::sendLocalReply( std::to_string( enumToInt(grpc_status ? grpc_status.value() : Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))}}}; - if (!body_text.empty() && !info.is_head_request) { + if (!body_text.empty() && !info.is_head_request_) { // TODO(dio): Probably it is worth to consider caching the encoded message based on gRPC // status. response_headers->insertGrpcMessage().value(PercentEncoding::encode(body_text)); @@ -324,8 +324,8 @@ void Utility::sendLocalReply( std::string body(body_text); - switch (info.reply_type) { - case Http::LocalReplyType::AlwaysJson: { + switch (info.media_type_) { + case Http::MediaType::ApplicationJson: { std::string json_body; envoy::data::core::v2alpha::LocalReplyConfiguration::JsonReply local_reply; local_reply.set_body(body.c_str(), body.size()); @@ -335,20 +335,20 @@ void Utility::sendLocalReply( response_headers->insertContentType().value(Headers::get().ContentTypeValues.Json); break; } - case Http::LocalReplyType::AlwaysText: { + case Http::MediaType::TextPlain: { if (!body_text.empty()) { response_headers->insertContentLength().value(body.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } break; } - case Http::LocalReplyType::DetermineViaAcceptHeader: + case Http::MediaType::NegotiateViaAcceptHeader: NOT_IMPLEMENTED_GCOVR_EXCL_LINE; default: NOT_REACHED_GCOVR_EXCL_LINE; } - if (info.is_head_request) { + if (info.is_head_request_) { encode_headers(std::move(response_headers), true); return; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 8a0b5dc1a355b..f3d5bceb66e5b 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -177,9 +177,9 @@ Http2Settings parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOption Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& config); struct LocalReplyInfo { - bool is_grpc{false}; - bool is_head_request{false}; - Http::LocalReplyType reply_type{Http::LocalReplyType::AlwaysText}; + bool is_grpc_{false}; + bool is_head_request_{false}; + Http::MediaType media_type_{Http::MediaType::TextPlain}; }; /** @@ -187,7 +187,7 @@ struct LocalReplyInfo { * Http::HeaderMap& request_headers. */ LocalReplyInfo generateLocalReplyInfo(const Http::HeaderMap& request_headers, - const Http::LocalReplyType& reply_type); + const Http::MediaType& media_type); /** * Create a locally generated response using filter callbacks. diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 64dc45b8203f0..ac5fcbbfdac38 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -332,18 +332,15 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( } if (config.has_local_reply_config()) { - switch (config.local_reply_config().reply_type_case()) { - case envoy::data::core::v2alpha::LocalReplyConfiguration::kAlwaysJson: - local_reply_type_ = Http::LocalReplyType::AlwaysJson; + switch (config.local_reply_config().media_type()) { + case envoy::data::core::v2alpha::LocalReplyConfiguration::TEXT_PLAIN: + local_reply_media_type_ = Http::MediaType::TextPlain; break; - case envoy::data::core::v2alpha::LocalReplyConfiguration::kAlwaysText: - local_reply_type_ = Http::LocalReplyType::AlwaysJson; + case envoy::data::core::v2alpha::LocalReplyConfiguration::APPLICATION_JSON: + local_reply_media_type_ = Http::MediaType::ApplicationJson; break; - case envoy::data::core::v2alpha::LocalReplyConfiguration::kDetermineViaAcceptHeader: - local_reply_type_ = Http::LocalReplyType::DetermineViaAcceptHeader; - break; - case envoy::data::core::v2alpha::LocalReplyConfiguration::REPLY_TYPE_NOT_SET: - local_reply_type_ = Http::LocalReplyType::AlwaysJson; + case envoy::data::core::v2alpha::LocalReplyConfiguration::NEGOTIATE_VIA_ACCEPT_HEADER: + local_reply_media_type_ = Http::MediaType::NegotiateViaAcceptHeader; break; default: NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 8f752c4a9fc5c..4fdec95b2a660 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -141,7 +141,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, bool shouldNormalizePath() const override { return normalize_path_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } - Http::LocalReplyType localReplyType() const override { return local_reply_type_; } + Http::MediaType mediaType() const override { return local_reply_media_type_; } private: enum class CodecType { HTTP1, HTTP2, AUTO }; @@ -185,7 +185,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const bool proxy_100_continue_; std::chrono::milliseconds delayed_close_timeout_; const bool normalize_path_; - Http::LocalReplyType local_reply_type_{Http::LocalReplyType::AlwaysText}; + Http::MediaType local_reply_media_type_{Http::MediaType::TextPlain}; // Default idle timeout is 5 minutes if nothing is specified in the HCM config. static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index f03a4296b4004..9de0aac3fb085 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -148,7 +148,7 @@ class AdminImpl : public Admin, Http::HeaderMap& response_headers, std::string& body) override; void closeSocket(); void addListenerToHandler(Network::ConnectionHandler* handler) override; - Http::LocalReplyType localReplyType() const override { return Http::LocalReplyType::AlwaysText; } + Http::MediaType mediaType() const override { return Http::MediaType::TextPlain; } private: /** diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 64ae7564eaad2..254cfafb04891 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -110,7 +110,7 @@ class FuzzConfig : public ConnectionManagerConfig { bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return false; } - LocalReplyType localReplyType() const override { return LocalReplyType::AlwaysText; } + Http::MediaType mediaType() const override { return MediaType::TextPlain; } const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager config_; std::list access_logs_; MockServerConnection* codec_{}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 8bc20a7d5b8aa..98f1d088f0d14 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -279,7 +279,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } - LocalReplyType localReplyType() const override { return LocalReplyType::AlwaysText; } + Http::MediaType mediaType() const override { return Http::MediaType::TextPlain; } DangerousDeprecatedTestTime test_time_; ConnectionManagerImplHelper::RouteConfigProvider route_config_provider_; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 190b95556ac42..ce97e19d8174f 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -85,7 +85,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_CONST_METHOD0(proxy100Continue, bool()); MOCK_CONST_METHOD0(http1Settings, const Http::Http1Settings&()); MOCK_CONST_METHOD0(shouldNormalizePath, bool()); - MOCK_CONST_METHOD0(localReplyType, Http::LocalReplyType()); + MOCK_CONST_METHOD0(mediaType, Http::MediaType()); std::unique_ptr internal_address_config_ = std::make_unique(); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 99f0dca35aa5d..6e5e98d207775 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -455,7 +455,7 @@ TEST(HttpUtility, SendLocalGrpcReply) { EXPECT_EQ(headers.GrpcMessage()->value().getStringView(), "large"); })); Utility::LocalReplyInfo info; - info.is_grpc = true; + info.is_grpc_ = true; Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", absl::nullopt); } @@ -497,7 +497,7 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { std::to_string(enumToInt(Grpc::Status::GrpcStatus::Unavailable))); })); Utility::LocalReplyInfo info; - info.is_grpc = true; + info.is_grpc_ = true; Utility::sendLocalReply(info, callbacks, false, Http::Code::TooManyRequests, "", absl::nullopt); EXPECT_CALL(callbacks, encodeHeaders_(_, true)) @@ -538,7 +538,7 @@ TEST(HttpUtility, SendLocalReplyHeadRequest) { absl::nullopt); } -TEST(HttpUtility, SendLocalReplyJsonConntentTypeRequest) { +TEST(HttpUtility, SendLocalReplyJsonContentTypeRequest) { MockStreamDecoderFilterCallbacks callbacks; bool is_reset = false; EXPECT_CALL(callbacks, encodeHeaders_(_, false)) @@ -553,7 +553,7 @@ TEST(HttpUtility, SendLocalReplyJsonConntentTypeRequest) { EXPECT_EQ(MessageUtil::getJsonStringFromMessage(local_reply, true, true), data.toString()); })); Utility::LocalReplyInfo info; - info.reply_type = Http::LocalReplyType::AlwaysJson; + info.media_type_ = Http::MediaType::TextPlain; Utility::sendLocalReply(info, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", absl::nullopt); } diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc index eee4d577026ba..dfecd3bb6ddb1 100644 --- a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc +++ b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc @@ -47,7 +47,7 @@ class ReverseBridgeTest : public testing::Test { // Verifies that an incoming request with too small a request body will immediately fail. TEST_F(ReverseBridgeTest, InvalidGrpcRequest) { initialize(); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -81,7 +81,7 @@ TEST_F(ReverseBridgeTest, InvalidGrpcRequest) { // Verifies that we do nothing to a header only request even if it looks like a gRPC request. TEST_F(ReverseBridgeTest, HeaderOnlyGrpcRequest) { initialize(); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { Http::TestHeaderMapImpl headers({{"content-type", "application/grpc"}, @@ -157,7 +157,7 @@ TEST_F(ReverseBridgeTest, NoGrpcRequest) { // frames, then the data should not be modified. TEST_F(ReverseBridgeTest, GrpcRequestNoManageFrameHeader) { initialize(false); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -216,7 +216,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestNoManageFrameHeader) { // to gRPC. TEST_F(ReverseBridgeTest, GrpcRequest) { initialize(); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -296,7 +296,7 @@ TEST_F(ReverseBridgeTest, GrpcRequest) { // Same as ReverseBridgeTest.GrpcRequest except no content-length header is passed. TEST_F(ReverseBridgeTest, GrpcRequestNoContentLength) { initialize(); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -375,7 +375,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestNoContentLength) { // grpc-status. TEST_F(ReverseBridgeTest, GrpcRequestInternalError) { initialize(); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -448,7 +448,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestInternalError) { // has an invalid content type we respond with a useful error message. TEST_F(ReverseBridgeTest, GrpcRequestBadResponse) { initialize(); - decoder_callbacks_.local_reply_info_.is_grpc = true; + decoder_callbacks_.local_reply_info_.is_grpc_ = true; { EXPECT_CALL(decoder_callbacks_, clearRouteCache()); diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 77248150ec869..26240f8229634 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -196,7 +196,10 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstreamJsonContentTypeRequest) { config_helper_.addConfigModifier( [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) - -> void { hcm.mutable_local_reply_config()->set_always_json(true); }); + -> void { + hcm.mutable_local_reply_config()->set_media_type( + envoy::data::core::v2alpha::LocalReplyConfiguration_MediaType_APPLICATION_JSON); + }); enable_per_stream_idle_timeout_ = true; auto response = setupPerStreamIdleTimeoutTest("GET");