From 8735d981ba41c78c02c7b06750d5c1efec5e1e89 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 2 Feb 2021 15:20:25 +0000 Subject: [PATCH 01/10] filter_manager: extend local reply rewrites to upstream responses Signed-off-by: John Esmet --- source/common/http/filter_manager.cc | 91 ++++++++++++++++++- source/common/http/filter_manager.h | 7 ++ source/common/local_reply/local_reply.cc | 54 ++++++++++- source/common/local_reply/local_reply.h | 5 + .../local_reply_integration_test.cc | 72 +++++++++++++++ test/mocks/local_reply/mocks.h | 7 ++ 6 files changed, 232 insertions(+), 4 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 6caf33f7536fb..00d2c52d8b2f0 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -879,8 +879,15 @@ void FilterManager::sendLocalReplyViaFilterChain( absl::string_view& content_type) -> void { // TODO(snowp): This &get() business isn't nice, rework LocalReply and others to accept // opt refs. + ENVOY_STREAM_LOG( + trace, "sendLocalReply local_reply_.rewrite code={}, body={}, content_type={}", + *this, code, body, content_type); local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers, stream_info_, code, body, content_type); + // Note that we did a local reply rewrite so that we don't try to do it again in encodeHeaders. + // This isn't very clean but we're trying to support local reply rewrites as well as upstream + // rewrites, where the match + rewrite logic makes the most sense in encodeHeaders/Data. + did_rewrite_ = true; }, [this, modify_headers](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { filter_manager_callbacks_.setResponseHeaders(std::move(headers)); @@ -1044,9 +1051,44 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea } } - const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); + // See if the response would be written by local_reply_. + if (!did_rewrite_) { + do_rewrite_ = + local_reply_.match(filter_manager_callbacks_.requestHeaders().ptr(), *filter_manager_callbacks_.responseHeaders(), + filter_manager_callbacks_.responseTrailers().ptr(), + stream_info_); + } + + bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); + const bool original_modified_end_stream = modified_end_stream; + ENVOY_STREAM_LOG(debug, "FilterManager::encodeHeaders: end_stream={}, modified_end_stream={}, do_rewrite_={}", + *this, end_stream, modified_end_stream, do_rewrite_); + if (do_rewrite_) { + // _This_ actually sets buffered_response_data_ internally. + rewriteResponse(); + + if (buffered_response_data_->length() > 0) { + // If we're going to rewrite the response here, then modified_end_stream can no longer be true + // because we have a body now. + modified_end_stream = false; + } + } + state_.non_100_response_headers_encoded_ = true; filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); + + // Encode the rewritten response right away if the original `modified_end_stream` was true. + // If it wasn't, then we know a body will be encoded later, and we'll let that function + // take care of encoding the rewritten response. + if (do_rewrite_ && original_modified_end_stream && buffered_response_data_->length() > 0) { + ASSERT(!did_rewrite_); + ENVOY_STREAM_LOG(trace, + "FilterManager::encodeData calling filter_manager_callbacks_ with {} bytes " + "from buffered_response_data and modified_end_stream={}", + *this, buffered_response_data_->length(), modified_end_stream); + filter_manager_callbacks_.encodeData(*buffered_response_data_, modified_end_stream); + } + maybeEndEncode(modified_end_stream); if (!modified_end_stream) { @@ -1180,7 +1222,24 @@ void FilterManager::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instan } const bool modified_end_stream = end_stream && trailers_added_entry == encoder_filters_.end(); - filter_manager_callbacks_.encodeData(data, modified_end_stream); + if (do_rewrite_ && buffered_response_data_->length() > 0) { + // If we're doing a rewrite and modified_end_stream=true, encode the buffered_response_data_ + // that was set by rewriteResponse() earlier in encodeHeaders(). + if (modified_end_stream) { + ENVOY_STREAM_LOG(trace, + "FilterManager::encodeData calling filter_manager_callbacks_ with {} bytes " + "from buffered_response_data and modified_end_stream={}", + *this, buffered_response_data_->length(), modified_end_stream); + filter_manager_callbacks_.encodeData(*buffered_response_data_, modified_end_stream); + } + } else { + // We're not rewriting the response, so encode the data as is. + ENVOY_STREAM_LOG(trace, + "FilterManager::encodeData calling filter_manager_callbacks_.encodeData with " + "{} bytes and modified_end_stream={}", + *this, data.length(), modified_end_stream); + filter_manager_callbacks_.encodeData(data, modified_end_stream); + } maybeEndEncode(modified_end_stream); // If trailers were adding during encodeData we need to trigger decodeTrailers in order @@ -1225,6 +1284,34 @@ void FilterManager::maybeEndEncode(bool end_stream) { } } +void FilterManager::rewriteResponse() { + ASSERT(do_rewrite_); + ASSERT(!did_rewrite_); + + auto response_headers = filter_manager_callbacks_.responseHeaders(); + ASSERT(response_headers.ptr() != nullptr); + + std::string rewritten_body{}; + absl::string_view rewritten_content_type{}; + Http::Code rewritten_code{static_cast(Utility::getResponseStatus(*response_headers))}; + + ENVOY_STREAM_LOG(trace, "rewriteResponse: calling local_reply_.rewrite with code={}", *this, + rewritten_code); + local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), *response_headers, + stream_info_, rewritten_code, rewritten_body, rewritten_content_type); + ENVOY_STREAM_LOG(trace, "rewriteResponse: local_reply_.rewrite returned body=\"{}\", content_type={}, code={}", *this, + rewritten_body, rewritten_content_type, rewritten_code); + + buffered_response_data_ = std::make_unique(rewritten_body); + if (!rewritten_body.empty()) { + // Since we overwrote the response body, we need to set the content-length too. + response_headers->setContentLength(buffered_response_data_->length()); + } + if (!rewritten_content_type.empty()) { + response_headers->setContentType(rewritten_content_type); + } +} + bool FilterManager::processNewlyAddedMetadata() { if (request_metadata_map_vector_ == nullptr) { return false; diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 80845e0f9375a..4e301141418da 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -909,6 +909,11 @@ class FilterManager : public ScopeTrackedObject, */ void maybeEndEncode(bool end_stream); + /** + * Rewrite the response headers and body using local_reply_. + */ + void rewriteResponse(); + void sendLocalReply(bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, const absl::optional grpc_status, @@ -1076,6 +1081,8 @@ class FilterManager : public ScopeTrackedObject, FilterChainFactory& filter_chain_factory_; const LocalReply::LocalReply& local_reply_; + bool do_rewrite_{}; + bool did_rewrite_{}; OverridableRemoteSocketAddressSetterStreamInfo stream_info_; // TODO(snowp): Once FM has been moved to its own file we'll make these private classes of FM, // at which point they no longer need to be friends. diff --git a/source/common/local_reply/local_reply.cc b/source/common/local_reply/local_reply.cc index 42f8d32b0d383..72a3650eb7df4 100644 --- a/source/common/local_reply/local_reply.cc +++ b/source/common/local_reply/local_reply.cc @@ -11,6 +11,7 @@ #include "common/formatter/substitution_format_string.h" #include "common/formatter/substitution_formatter.h" #include "common/http/header_map_impl.h" +#include "common/http/utility.h" #include "common/router/header_parser.h" namespace Envoy { @@ -80,10 +81,48 @@ class ResponseMapper { StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, BodyFormatter*& final_formatter) const { // If not matched, just bail out. - if (!filter_->evaluate(stream_info, request_headers, response_headers, response_trailers)) { + if (!match(&request_headers, response_headers, &response_trailers, stream_info)) { return false; } + rewrite(request_headers, response_headers, stream_info, code, body, final_formatter); + return true; + } + + // Decide if a request/response pair matches this mapper. + bool match(const Http::RequestHeaderMap* request_headers, + const Http::ResponseHeaderMap& response_headers, + const Http::ResponseTrailerMap* response_trailers, + StreamInfo::StreamInfo& stream_info) const { + // Set response code on the stream_info because it's used by the StatusCode filter. + // Further, we know that the status header present on the upstream response headers + // is the status we want to match on. It may not be the status we send downstream + // to the client, though, because we may call `rewrite` later. + // + // Under normal circumstances we should have a response status by this point, because + // either the upstream set it or the router filter set it. If for whatever reason we + // don't, skip setting the stream info's response code and just let our evaluation + // logic do without it. We can't do much better, and we certainly don't want to throw + // an exception and crash here. + if (response_headers.Status() != nullptr) { + stream_info.setResponseCode( + static_cast(Http::Utility::getResponseStatus(response_headers))); + } + + if (request_headers == nullptr) { + request_headers = Http::StaticEmptyHeaders::get().request_headers.get(); + } + + if (response_trailers == nullptr) { + response_trailers = Http::StaticEmptyHeaders::get().response_trailers.get(); + } + + return filter_->evaluate(stream_info, *request_headers, response_headers, *response_trailers); + } + + void rewrite(const Http::RequestHeaderMap&, Http::ResponseHeaderMap& response_headers, + StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, + BodyFormatter*& final_formatter) const { if (body_.has_value()) { body = body_.value(); } @@ -99,7 +138,6 @@ class ResponseMapper { if (body_formatter_) { final_formatter = body_formatter_.get(); } - return true; } private: @@ -128,6 +166,18 @@ class LocalReplyImpl : public LocalReply { } } + bool match(const Http::RequestHeaderMap* request_headers, + const Http::ResponseHeaderMap& response_headers, + const Http::ResponseTrailerMap* response_trailers, + StreamInfo::StreamInfo& stream_info) const override { + for (const auto& mapper : mappers_) { + if (mapper->match(request_headers, response_headers, response_trailers, stream_info)) { + return true; + } + } + return false; + } + void rewrite(const Http::RequestHeaderMap* request_headers, Http::ResponseHeaderMap& response_headers, StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, diff --git a/source/common/local_reply/local_reply.h b/source/common/local_reply/local_reply.h index 5db93caa07fda..14e2004cf379d 100644 --- a/source/common/local_reply/local_reply.h +++ b/source/common/local_reply/local_reply.h @@ -26,6 +26,11 @@ class LocalReply { Http::ResponseHeaderMap& response_headers, StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, absl::string_view& content_type) const PURE; + + virtual bool match(const Http::RequestHeaderMap* request_headers, + const Http::ResponseHeaderMap& response_headers, + const Http::ResponseTrailerMap* response_trailers, + StreamInfo::StreamInfo& stream_info) const PURE; }; using LocalReplyPtr = std::unique_ptr; diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc index 8bf22764eb101..6257bade08245 100644 --- a/test/integration/local_reply_integration_test.cc +++ b/test/integration/local_reply_integration_test.cc @@ -466,4 +466,76 @@ TEST_P(LocalReplyIntegrationTest, ShouldFormatResponseToEmptyBody) { EXPECT_EQ(response->body(), ""); } +// Should match and rewrite an upstream response that does not contain a body. +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusHeadersOnly) { + const std::string yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 429 + runtime_key: key_b + status_code: 450 +body_format: + text_format_source: + inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" +)EOF"; + setLocalReplyConfig(yaml); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}}); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "429"}}, true); + response->waitForHeaders(); + + EXPECT_EQ("450", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), "450: via_upstream"); + + cleanupUpstreamAndDownstream(); +} + +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithBody) { + const std::string yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 429 + runtime_key: key_b + status_code: 451 +body_format: + text_format_source: + inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" +)EOF"; + setLocalReplyConfig(yaml); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}}); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "429"}}, false); + upstream_request_->encodeData(512, true); + response->waitForHeaders(); + + EXPECT_EQ("451", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), "451: via_upstream"); + + cleanupUpstreamAndDownstream(); +} + } // namespace Envoy diff --git a/test/mocks/local_reply/mocks.h b/test/mocks/local_reply/mocks.h index 913f815d50691..c13d9e9f9318b 100644 --- a/test/mocks/local_reply/mocks.h +++ b/test/mocks/local_reply/mocks.h @@ -14,6 +14,13 @@ class MockLocalReply : public LocalReply { Http::ResponseHeaderMap& response_headers, StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, absl::string_view& content_type), (const)); + + MOCK_METHOD(bool, match, + (const Http::RequestHeaderMap* request_headers, + const Http::ResponseHeaderMap& response_headers, + const Http::ResponseTrailerMap* response_trailers, + StreamInfo::StreamInfo& stream_info), + (const)); }; } // namespace LocalReply } // namespace Envoy From e95f42ad8792b322a5cb40feb9510f55231947a0 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 3 Feb 2021 17:17:47 +0000 Subject: [PATCH 02/10] Fix format Signed-off-by: John Esmet --- source/common/http/filter_manager.cc | 23 +++++++++++-------- .../local_reply_integration_test.cc | 14 ++++------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 00d2c52d8b2f0..c23df8d5a18a2 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -884,9 +884,10 @@ void FilterManager::sendLocalReplyViaFilterChain( *this, code, body, content_type); local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers, stream_info_, code, body, content_type); - // Note that we did a local reply rewrite so that we don't try to do it again in encodeHeaders. - // This isn't very clean but we're trying to support local reply rewrites as well as upstream - // rewrites, where the match + rewrite logic makes the most sense in encodeHeaders/Data. + // Note that we did a local reply rewrite so that we don't try to do it again in + // encodeHeaders. This isn't very clean but we're trying to support local reply rewrites + // as well as upstream rewrites, where the match + rewrite logic makes the most sense in + // encodeHeaders/Data. did_rewrite_ = true; }, [this, modify_headers](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { @@ -1054,15 +1055,16 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea // See if the response would be written by local_reply_. if (!did_rewrite_) { do_rewrite_ = - local_reply_.match(filter_manager_callbacks_.requestHeaders().ptr(), *filter_manager_callbacks_.responseHeaders(), - filter_manager_callbacks_.responseTrailers().ptr(), - stream_info_); + local_reply_.match(filter_manager_callbacks_.requestHeaders().ptr(), + *filter_manager_callbacks_.responseHeaders(), + filter_manager_callbacks_.responseTrailers().ptr(), stream_info_); } bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); const bool original_modified_end_stream = modified_end_stream; - ENVOY_STREAM_LOG(debug, "FilterManager::encodeHeaders: end_stream={}, modified_end_stream={}, do_rewrite_={}", - *this, end_stream, modified_end_stream, do_rewrite_); + ENVOY_STREAM_LOG( + debug, "FilterManager::encodeHeaders: end_stream={}, modified_end_stream={}, do_rewrite_={}", + *this, end_stream, modified_end_stream, do_rewrite_); if (do_rewrite_) { // _This_ actually sets buffered_response_data_ internally. rewriteResponse(); @@ -1299,8 +1301,9 @@ void FilterManager::rewriteResponse() { rewritten_code); local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), *response_headers, stream_info_, rewritten_code, rewritten_body, rewritten_content_type); - ENVOY_STREAM_LOG(trace, "rewriteResponse: local_reply_.rewrite returned body=\"{}\", content_type={}, code={}", *this, - rewritten_body, rewritten_content_type, rewritten_code); + ENVOY_STREAM_LOG( + trace, "rewriteResponse: local_reply_.rewrite returned body=\"{}\", content_type={}, code={}", + *this, rewritten_body, rewritten_content_type, rewritten_code); buffered_response_data_ = std::make_unique(rewritten_body); if (!rewritten_body.empty()) { diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc index 6257bade08245..2710a86d320ed 100644 --- a/test/integration/local_reply_integration_test.cc +++ b/test/integration/local_reply_integration_test.cc @@ -487,11 +487,8 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusHeadersOnly) { codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest( - Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/"}, - {":scheme", "http"}, - {":authority", "host"}}); + auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}); waitForNextUpstreamRequest(); upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "429"}}, true); response->waitForHeaders(); @@ -522,11 +519,8 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithBody) { codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest( - Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/"}, - {":scheme", "http"}, - {":authority", "host"}}); + auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}); waitForNextUpstreamRequest(); upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "429"}}, false); upstream_request_->encodeData(512, true); From e794d028f5ea919e56520b0bc4484e268ae04be0 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 3 Feb 2021 18:25:54 +0000 Subject: [PATCH 03/10] Add tests, add LocalReplyDataPtr, add encode_grpc_ to EncodeFunctions Signed-off-by: John Esmet --- source/common/http/async_client_impl.h | 2 +- source/common/http/filter_manager.cc | 120 ++++++---- source/common/http/filter_manager.h | 12 +- source/common/http/utility.cc | 48 ++-- source/common/http/utility.h | 18 ++ source/common/local_reply/local_reply.cc | 23 +- source/common/local_reply/local_reply.h | 19 +- test/integration/fake_upstream.h | 2 +- .../local_reply_integration_test.cc | 225 ++++++++++++++++-- test/mocks/http/mocks.cc | 2 +- test/tools/router_check/router.cc | 7 +- 11 files changed, 366 insertions(+), 112 deletions(-) diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 9e248c5d57182..e0a1e3fb3c23d 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -384,7 +384,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, } Utility::sendLocalReply( remote_closed_, - Utility::EncodeFunctions{nullptr, nullptr, + Utility::EncodeFunctions{nullptr, nullptr, nullptr, [this, modify_headers, &details](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index c23df8d5a18a2..b22cedffa49b2 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -862,6 +862,12 @@ void FilterManager::sendLocalReplyViaFilterChain( // a no-op. createFilterChain(); + // A bit awkward, but save the local reply data for later. This will come in handy. + // When this pointer is non-null, it means a local reply has been initiated, which + // is important later on. + local_reply_data_ = std::make_unique( + Utility::LocalReplyData{is_grpc_request, code, body, grpc_status, is_head_request}); + Utility::sendLocalReply( state_.destroyed_, Utility::EncodeFunctions{ @@ -875,21 +881,14 @@ void FilterManager::sendLocalReplyViaFilterChain( modify_headers(headers); } }, - [this](ResponseHeaderMap& response_headers, Code& code, std::string& body, - absl::string_view& content_type) -> void { - // TODO(snowp): This &get() business isn't nice, rework LocalReply and others to accept - // opt refs. - ENVOY_STREAM_LOG( - trace, "sendLocalReply local_reply_.rewrite code={}, body={}, content_type={}", - *this, code, body, content_type); - local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers, - stream_info_, code, body, content_type); - // Note that we did a local reply rewrite so that we don't try to do it again in - // encodeHeaders. This isn't very clean but we're trying to support local reply rewrites - // as well as upstream rewrites, where the match + rewrite logic makes the most sense in - // encodeHeaders/Data. - did_rewrite_ = true; - }, + // Local reply rewrites (and upstream rewrites, for that matter) happen on the filter + // chain now, so we do not need to do any additional rewriting here. + /*rewrite_=*/nullptr, + // Local gRPC replies are encoded as trailers-only responses on the filter chain, + // so we don't need to any additional encoding of the gRPC response here. This + // is different from the direct local reply case, where we do encode the gRPC + // reply as a headers-only response using an encoder callback. + /*encode_grpc_=*/nullptr, [this, modify_headers](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { filter_manager_callbacks_.setResponseHeaders(std::move(headers)); // TODO: Start encoding from the last decoder filter that saw the @@ -926,9 +925,18 @@ void FilterManager::sendDirectLocalReply( }, [&](ResponseHeaderMap& response_headers, Code& code, std::string& body, absl::string_view& content_type) -> void { + /* Direct local reply rewrites happen here since we are not going through the filter + * chain. */ local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers, stream_info_, code, body, content_type); }, + [&](ResponseHeaderMap& headers, Code& code, std::string& body, + const absl::optional grpc_status, + bool is_head_request) -> void { + /* Direct local reply gRPC encoding happens here since we are not going through the + * filter chain. */ + Utility::toGrpcTrailersOnlyResponse(headers, code, body, grpc_status, is_head_request); + }, [&](ResponseHeaderMapPtr&& response_headers, bool end_stream) -> void { // Move the response headers into the FilterManager to make sure they're visible to // access logs. @@ -1052,24 +1060,35 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea } } - // See if the response would be written by local_reply_. - if (!did_rewrite_) { - do_rewrite_ = - local_reply_.match(filter_manager_callbacks_.requestHeaders().ptr(), - *filter_manager_callbacks_.responseHeaders(), - filter_manager_callbacks_.responseTrailers().ptr(), stream_info_); - } - bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); + + // Capture the original modified_end_stream value. We may modify it again below if we add + // a response body. const bool original_modified_end_stream = modified_end_stream; - ENVOY_STREAM_LOG( - debug, "FilterManager::encodeHeaders: end_stream={}, modified_end_stream={}, do_rewrite_={}", - *this, end_stream, modified_end_stream, do_rewrite_); - if (do_rewrite_) { - // _This_ actually sets buffered_response_data_ internally. + + // See if we should do an upstream rewrite. For local replies, always try to do a rewrite. + // For upstream replies, only try to do a rewrite if some filter rule actually applies. + // The rewrite config supports unconditional rewrites with no filter rules. This makes sense + // for local reply rewrites (e.g. one consistent format for all responses generated at Envoy) + // but makes a lot less sense for all responses upstream. + state_.do_response_rewrite_ = + local_reply_data_ != nullptr || + local_reply_.matchesAnyMapper(filter_manager_callbacks_.requestHeaders().ptr(), + *filter_manager_callbacks_.responseHeaders(), + filter_manager_callbacks_.responseTrailers().ptr(), + stream_info_); + ENVOY_STREAM_LOG(debug, + "FilterManager::encodeHeaders: end_stream={}, modified_end_stream={}, " + "state_.do_response_rewrite_={}", + *this, end_stream, modified_end_stream, state_.do_response_rewrite_); + + if (state_.do_response_rewrite_) { + // Try to rewrite the response. This may end up not doing any actual rewrite if this is + // a local reply and there is no matching rule. If it's not a local reply, we know there's + // a match at this point since that's a condition for state_.do_response_rewrite_ above. rewriteResponse(); - if (buffered_response_data_->length() > 0) { + if (buffered_response_data_) { // If we're going to rewrite the response here, then modified_end_stream can no longer be true // because we have a body now. modified_end_stream = false; @@ -1082,8 +1101,7 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea // Encode the rewritten response right away if the original `modified_end_stream` was true. // If it wasn't, then we know a body will be encoded later, and we'll let that function // take care of encoding the rewritten response. - if (do_rewrite_ && original_modified_end_stream && buffered_response_data_->length() > 0) { - ASSERT(!did_rewrite_); + if (state_.do_response_rewrite_ && original_modified_end_stream && buffered_response_data_) { ENVOY_STREAM_LOG(trace, "FilterManager::encodeData calling filter_manager_callbacks_ with {} bytes " "from buffered_response_data and modified_end_stream={}", @@ -1224,7 +1242,7 @@ void FilterManager::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instan } const bool modified_end_stream = end_stream && trailers_added_entry == encoder_filters_.end(); - if (do_rewrite_ && buffered_response_data_->length() > 0) { + if (state_.do_response_rewrite_ && buffered_response_data_) { // If we're doing a rewrite and modified_end_stream=true, encode the buffered_response_data_ // that was set by rewriteResponse() earlier in encodeHeaders(). if (modified_end_stream) { @@ -1235,11 +1253,6 @@ void FilterManager::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instan filter_manager_callbacks_.encodeData(*buffered_response_data_, modified_end_stream); } } else { - // We're not rewriting the response, so encode the data as is. - ENVOY_STREAM_LOG(trace, - "FilterManager::encodeData calling filter_manager_callbacks_.encodeData with " - "{} bytes and modified_end_stream={}", - *this, data.length(), modified_end_stream); filter_manager_callbacks_.encodeData(data, modified_end_stream); } maybeEndEncode(modified_end_stream); @@ -1287,9 +1300,6 @@ void FilterManager::maybeEndEncode(bool end_stream) { } void FilterManager::rewriteResponse() { - ASSERT(do_rewrite_); - ASSERT(!did_rewrite_); - auto response_headers = filter_manager_callbacks_.responseHeaders(); ASSERT(response_headers.ptr() != nullptr); @@ -1297,16 +1307,34 @@ void FilterManager::rewriteResponse() { absl::string_view rewritten_content_type{}; Http::Code rewritten_code{static_cast(Utility::getResponseStatus(*response_headers))}; - ENVOY_STREAM_LOG(trace, "rewriteResponse: calling local_reply_.rewrite with code={}", *this, - rewritten_code); - local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), *response_headers, - stream_info_, rewritten_code, rewritten_body, rewritten_content_type); + // Start with the local reply body, if we have it. + if (local_reply_data_) { + rewritten_body = local_reply_data_->body_text_; + } + + ENVOY_STREAM_LOG(trace, "rewriteResponse: calling local_reply_.rewrite with body=\"{}\", code={}", + *this, rewritten_body, rewritten_code); + const bool did_rewrite = + local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), *response_headers, + stream_info_, rewritten_code, rewritten_body, rewritten_content_type); ENVOY_STREAM_LOG( trace, "rewriteResponse: local_reply_.rewrite returned body=\"{}\", content_type={}, code={}", *this, rewritten_body, rewritten_content_type, rewritten_code); - buffered_response_data_ = std::make_unique(rewritten_body); - if (!rewritten_body.empty()) { + if (local_reply_data_ && local_reply_data_->is_grpc_) { + // Send a trailers-only grpc response + Utility::toGrpcTrailersOnlyResponse(*response_headers, rewritten_code, rewritten_body, + local_reply_data_->grpc_status_, + local_reply_data_->is_head_request_); + // We're sending a trailers-only response with no body. Make sure there's no buffered + // response data nor a content length header. + buffered_response_data_ = nullptr; + return; + } + + buffered_response_data_ = + did_rewrite ? std::make_unique(rewritten_body) : nullptr; + if (buffered_response_data_) { // Since we overwrote the response body, we need to set the content-length too. response_headers->setContentLength(buffered_response_data_->length()); } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 4e301141418da..56204da77f0d4 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -20,6 +20,7 @@ #include "common/grpc/common.h" #include "common/http/header_utility.h" #include "common/http/headers.h" +#include "common/http/utility.h" #include "common/local_reply/local_reply.h" #include "common/matcher/matcher.h" #include "common/protobuf/utility.h" @@ -1080,9 +1081,12 @@ class FilterManager : public ScopeTrackedObject, std::make_shared(); FilterChainFactory& filter_chain_factory_; + // Used to track local reply state + // TODO(esmet): We could combine the local reply reference and data into one pointer + // member that stays nullptr when no local reply config is present to save space in + // the 'off' case. But, for now, we spend two pointers of space in all cases. const LocalReply::LocalReply& local_reply_; - bool do_rewrite_{}; - bool did_rewrite_{}; + Utility::LocalReplyDataPtr local_reply_data_; OverridableRemoteSocketAddressSetterStreamInfo stream_info_; // TODO(snowp): Once FM has been moved to its own file we'll make these private classes of FM, // at which point they no longer need to be friends. @@ -1116,7 +1120,7 @@ class FilterManager : public ScopeTrackedObject, State() : remote_complete_(false), local_complete_(false), has_continue_headers_(false), created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), - non_100_response_headers_encoded_(false) {} + non_100_response_headers_encoded_(false), do_response_rewrite_(false) {} uint32_t filter_call_state_{0}; @@ -1134,6 +1138,8 @@ class FilterManager : public ScopeTrackedObject, bool is_grpc_request_ : 1; // Tracks if headers other than 100-Continue have been encoded to the codec. bool non_100_response_headers_encoded_ : 1; + // True if we should rewrite the upstream response using local_reply_ + bool do_response_rewrite_ : 1; // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 86660479c683a..60c45136138c7 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -512,7 +512,7 @@ void Utility::sendLocalReply(const bool& is_reset, StreamDecoderFilterCallbacks& sendLocalReply( is_reset, - Utility::EncodeFunctions{nullptr, nullptr, + Utility::EncodeFunctions{nullptr, nullptr, nullptr, [&](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { callbacks.encodeHeaders(std::move(headers), end_stream, details); }, @@ -522,6 +522,27 @@ void Utility::sendLocalReply(const bool& is_reset, StreamDecoderFilterCallbacks& local_reply_data); } +void Utility::toGrpcTrailersOnlyResponse(Http::ResponseHeaderMap& response_headers, + const Http::Code& code, std::string& response_body, + const absl::optional grpc_status, + bool is_head_request) { + response_headers.setStatus(std::to_string(enumToInt(Http::Code::OK))); + response_headers.setReferenceContentType(Headers::get().ContentTypeValues.Grpc); + response_headers.setGrpcStatus(std::to_string(enumToInt( + grpc_status ? grpc_status.value() : Grpc::Utility::httpToGrpcStatus(enumToInt(code))))); + if (!response_body.empty() && !is_head_request) { + // TODO(dio): Probably it is worth to consider caching the encoded message based on gRPC + // status. + // JsonFormatter adds a '\n' at the end. For header value, it should be removed. + // https://github.com/envoyproxy/envoy/blob/main/source/common/formatter/substitution_formatter.cc#L129 + if (response_body[response_body.length() - 1] == '\n') { + response_body = response_body.substr(0, response_body.length() - 1); + } + response_headers.setGrpcMessage(Http::Utility::PercentEncoding::encode(response_body)); + } + response_headers.removeContentLength(); +} + void Utility::sendLocalReply(const bool& is_reset, const EncodeFunctions& encode_functions, const LocalReplyData& local_reply_data) { // encode_headers() may reset the stream, so the stream must not be reset before calling it. @@ -542,26 +563,15 @@ void Utility::sendLocalReply(const bool& is_reset, const EncodeFunctions& encode encode_functions.rewrite_(*response_headers, response_code, body_text, content_type); } - // Respond with a gRPC trailers-only response if the request is gRPC + // Respond with a gRPC trailers-only response if the request is gRPC. The actual encoding of the + // trailers-only response happens in encode_grpc_, if set. Otherwise it is assumed that the + // response is already encoded and ready to be finalized by encodeHeaders with end_stream=true. if (local_reply_data.is_grpc_) { - response_headers->setStatus(std::to_string(enumToInt(Code::OK))); - response_headers->setReferenceContentType(Headers::get().ContentTypeValues.Grpc); - response_headers->setGrpcStatus( - std::to_string(enumToInt(local_reply_data.grpc_status_ - ? local_reply_data.grpc_status_.value() - : Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))); - if (!body_text.empty() && !local_reply_data.is_head_request_) { - // TODO(dio): Probably it is worth to consider caching the encoded message based on gRPC - // status. - // JsonFormatter adds a '\n' at the end. For header value, it should be removed. - // https://github.com/envoyproxy/envoy/blob/main/source/common/formatter/substitution_formatter.cc#L129 - if (body_text[body_text.length() - 1] == '\n') { - body_text = body_text.substr(0, body_text.length() - 1); - } - response_headers->setGrpcMessage(PercentEncoding::encode(body_text)); + if (encode_functions.encode_grpc_) { + encode_functions.encode_grpc_(*response_headers, response_code, body_text, + local_reply_data.grpc_status_, + local_reply_data.is_head_request_); } - // The `modify_headers` function may have added content-length, remove it. - response_headers->removeContentLength(); encode_functions.encode_headers_(std::move(response_headers), true); // Trailers only response return; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d677b097d86cb..62001803b9be7 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -295,6 +295,11 @@ struct EncodeFunctions { std::function rewrite_; + // Function to encode a gRPC response. + std::function grpc_status, + bool is_head_request)> + encode_grpc_; // Function to encode response headers. std::function encode_headers_; // Function to encode the response body. @@ -313,6 +318,7 @@ struct LocalReplyData { // Tells if this is a response to a HEAD request. bool is_head_request_ = false; }; +using LocalReplyDataPtr = std::unique_ptr; /** * Create a locally generated response using filter callbacks. @@ -337,6 +343,18 @@ void sendLocalReply(const bool& is_reset, StreamDecoderFilterCallbacks& callback void sendLocalReply(const bool& is_reset, const EncodeFunctions& encode_functions, const LocalReplyData& local_reply_data); +/** + * Convert a response into a gRPC trailers-only response. + * @param response_headers the response headers. will be modified. + * @param code the upstream response code. will be converted to grpc-status + * @param grpc_status the original grpc status + * @param is_head_request whether this is a HEAD request + */ +void toGrpcTrailersOnlyResponse(Http::ResponseHeaderMap& response_headers, const Http::Code& code, + std::string& response_body, + const absl::optional grpc_status, + bool is_head_request); + struct GetLastAddressFromXffInfo { // Last valid address pulled from the XFF header. Network::Address::InstanceConstSharedPtr address_; diff --git a/source/common/local_reply/local_reply.cc b/source/common/local_reply/local_reply.cc index 72a3650eb7df4..0e12fa80446fb 100644 --- a/source/common/local_reply/local_reply.cc +++ b/source/common/local_reply/local_reply.cc @@ -160,16 +160,17 @@ class LocalReplyImpl : public LocalReply { Server::Configuration::FactoryContext& context) : body_formatter_(config.has_body_format() ? std::make_unique(config.body_format(), context.api()) - : std::make_unique()) { + : std::make_unique()), + has_configured_body_formatter_(config.has_body_format()) { for (const auto& mapper : config.mappers()) { mappers_.emplace_back(std::make_unique(mapper, context)); } } - bool match(const Http::RequestHeaderMap* request_headers, - const Http::ResponseHeaderMap& response_headers, - const Http::ResponseTrailerMap* response_trailers, - StreamInfo::StreamInfo& stream_info) const override { + bool matchesAnyMapper(const Http::RequestHeaderMap* request_headers, + const Http::ResponseHeaderMap& response_headers, + const Http::ResponseTrailerMap* response_trailers, + StreamInfo::StreamInfo& stream_info) const override { for (const auto& mapper : mappers_) { if (mapper->match(request_headers, response_headers, response_trailers, stream_info)) { return true; @@ -178,7 +179,7 @@ class LocalReplyImpl : public LocalReply { return false; } - void rewrite(const Http::RequestHeaderMap* request_headers, + bool rewrite(const Http::RequestHeaderMap* request_headers, Http::ResponseHeaderMap& response_headers, StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, absl::string_view& content_type) const override { @@ -204,14 +205,18 @@ class LocalReplyImpl : public LocalReply { if (!final_formatter) { final_formatter = body_formatter_.get(); } - return final_formatter->format(*request_headers, response_headers, - *Http::StaticEmptyHeaders::get().response_trailers, stream_info, - body, content_type); + final_formatter->format(*request_headers, response_headers, + *Http::StaticEmptyHeaders::get().response_trailers, stream_info, body, + content_type); + // If this local reply has a configured body formatter or the final formatter is not the + // default formatter, then we know `body` was modified by an explicitly configured formatter. + return has_configured_body_formatter_ || final_formatter != body_formatter_.get(); } private: std::list mappers_; const BodyFormatterPtr body_formatter_; + bool has_configured_body_formatter_; }; LocalReplyPtr Factory::createDefault() { return std::make_unique(); } diff --git a/source/common/local_reply/local_reply.h b/source/common/local_reply/local_reply.h index 14e2004cf379d..d0eb3edf4a53c 100644 --- a/source/common/local_reply/local_reply.h +++ b/source/common/local_reply/local_reply.h @@ -21,16 +21,25 @@ class LocalReply { * @param code status code. * @param body response body. * @param content_type response content_type. + * @return whether the local reply used a non-default body formatter to populate `body` */ - virtual void rewrite(const Http::RequestHeaderMap* request_headers, + virtual bool rewrite(const Http::RequestHeaderMap* request_headers, Http::ResponseHeaderMap& response_headers, StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, absl::string_view& content_type) const PURE; - virtual bool match(const Http::RequestHeaderMap* request_headers, - const Http::ResponseHeaderMap& response_headers, - const Http::ResponseTrailerMap* response_trailers, - StreamInfo::StreamInfo& stream_info) const PURE; + /** + * decide if any mapper matches the request/response. + * @param request_headers the request headers. + * @param response_headers the response headers. + * @param response_trailers the response trailers, if any. + * @param stream_info the stream info. + * @return whether any mapper is a match + */ + virtual bool matchesAnyMapper(const Http::RequestHeaderMap* request_headers, + const Http::ResponseHeaderMap& response_headers, + const Http::ResponseTrailerMap* response_trailers, + StreamInfo::StreamInfo& stream_info) const PURE; }; using LocalReplyPtr = std::unique_ptr; diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 1eb98c10c7f16..548d67163307d 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -105,7 +105,7 @@ class FakeStream : public Http::RequestDecoder, Http::Utility::sendLocalReply( false, Http::Utility::EncodeFunctions( - {nullptr, nullptr, + {nullptr, nullptr, nullptr, [&](Http::ResponseHeaderMapPtr&& headers, bool end_stream) -> void { encoder_.encodeHeaders(*headers, end_stream); }, diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc index 2710a86d320ed..5c5de4085f8c0 100644 --- a/test/integration/local_reply_integration_test.cc +++ b/test/integration/local_reply_integration_test.cc @@ -13,6 +13,35 @@ class LocalReplyIntegrationTest : public HttpProtocolIntegrationTest { TestUtility::loadFromYaml(yaml, local_reply_config); config_helper_.setLocalReply(local_reply_config); } + + IntegrationStreamDecoderPtr getUpstreamResponse(const std::string& yaml, + const std::string& response_code, + uint64_t upstream_body_size, + const std::string& method = "GET") { + setLocalReplyConfig(yaml); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", method}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"test-header", "exact-match-value"}}); + waitForNextUpstreamRequest(); + + if (upstream_body_size > 0) { + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", response_code}}, + false); + upstream_request_->encodeData(upstream_body_size, true); + } else { + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", response_code}}, + true); + } + response->waitForHeaders(); + return response; + } }; INSTANTIATE_TEST_SUITE_P(Protocols, LocalReplyIntegrationTest, @@ -466,9 +495,49 @@ TEST_P(LocalReplyIntegrationTest, ShouldFormatResponseToEmptyBody) { EXPECT_EQ(response->body(), ""); } -// Should match and rewrite an upstream response that does not contain a body. -TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusHeadersOnly) { - const std::string yaml = R"EOF( +const std::string match_501_yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 501 + runtime_key: key_b + headers_to_add: + - header: + key: x-upstream-5xx + value: "1" + append: true + status_code: 500 +body_format: + text_format_source: + inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" +)EOF"; + +// Should not match the http response code and not add a header nor modify (add) the body. +TEST_P(LocalReplyIntegrationTest, LocalyReplyNoMatchNoUpstreamBody) { + auto response = getUpstreamResponse(match_501_yaml, "200", 0, "GET"); + + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ(0, response->headers().get(Http::LowerCaseString("x-upstream-5xx")).size()); + EXPECT_EQ(response->body(), ""); + + cleanupUpstreamAndDownstream(); +} + +// Should not match the http response code and not add a header nor modify (replace) the body. +TEST_P(LocalReplyIntegrationTest, LocalyReplyNoMatchWithUpstreamBody) { + auto response = getUpstreamResponse(match_501_yaml, "200", 8, "GET"); + + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ(0, response->headers().get(Http::LowerCaseString("x-upstream-5xx")).size()); + EXPECT_EQ(response->body(), std::string(8, 'a')); + + cleanupUpstreamAndDownstream(); +} + +const std::string match_429_rewrite_450_yaml = R"EOF( mappers: - filter: status_code_filter: @@ -482,16 +551,10 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusHeadersOnly) { text_format_source: inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" )EOF"; - setLocalReplyConfig(yaml); - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{ - {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}); - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "429"}}, true); - response->waitForHeaders(); +// Should match an http response code and modify (add) the upstream response. +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusNoBody) { + auto response = getUpstreamResponse(match_429_rewrite_450_yaml, "429", 0); EXPECT_EQ("450", response->headers().Status()->value().getStringView()); EXPECT_EQ(response->body(), "450: via_upstream"); @@ -499,8 +562,17 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusHeadersOnly) { cleanupUpstreamAndDownstream(); } +// Should match an http response code and modify (replace) the upstream response. TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithBody) { - const std::string yaml = R"EOF( + auto response = getUpstreamResponse(match_429_rewrite_450_yaml, "429", 512); + + EXPECT_EQ("450", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), "450: via_upstream"); + + cleanupUpstreamAndDownstream(); +} + +const std::string match_429_rewrite_451_remove_body_yaml = R"EOF( mappers: - filter: status_code_filter: @@ -512,22 +584,127 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithBody) { status_code: 451 body_format: text_format_source: - inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" + inline_string: "" )EOF"; - setLocalReplyConfig(yaml); - initialize(); - codec_client_ = makeHttpConnection(lookupPort("http")); +// Should match an http response code and remove the response body (no body from upstream). +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusRemoveEmptyBody) { + auto response = getUpstreamResponse(match_429_rewrite_451_remove_body_yaml, "429", 0); - auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{ - {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}); - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "429"}}, false); - upstream_request_->encodeData(512, true); - response->waitForHeaders(); + EXPECT_EQ("451", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), ""); + + cleanupUpstreamAndDownstream(); +} + +// Should match an http response code and remove the response body (non-empty body from upstream). +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusRemoveNonEmptyBody) { + auto response = getUpstreamResponse(match_429_rewrite_451_remove_body_yaml, "429", 512); EXPECT_EQ("451", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), "451: via_upstream"); + EXPECT_EQ(response->body(), ""); + + cleanupUpstreamAndDownstream(); +} + +const std::string match_429_rewrite_475_unmodified_body_yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 429 + runtime_key: key_b + status_code: 475 +)EOF"; + +// Should match an http response code and rewrite status without modifying upstream body (no body +// from upstream). +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithUnmodifiedEmptyBody) { + auto response = getUpstreamResponse(match_429_rewrite_475_unmodified_body_yaml, "429", 0); + + EXPECT_EQ("475", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), ""); + + cleanupUpstreamAndDownstream(); +} + +// Should match an http response code and rewrite status without modifying upstream body (non-empty +// body from upstream). +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithUnmodifiedNonEmptyBody) { + auto response = getUpstreamResponse(match_429_rewrite_475_unmodified_body_yaml, "429", 8); + + EXPECT_EQ("475", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), std::string(8, 'a')); + + cleanupUpstreamAndDownstream(); +} + +const std::string match_test_header_rewrite_435_yaml = R"EOF( +mappers: +- filter: + header_filter: + header: + name: test-header + exact_match: exact-match-value + status_code: 435 +body_format: + text_format_source: + inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" +)EOF"; + +// Should match an http header, rewrite the response code, and modify the upstream response body (no +// upstream response body) +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeaderNoBody) { + auto response = getUpstreamResponse(match_test_header_rewrite_435_yaml, "200", 0); + + EXPECT_EQ("435", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), "435: via_upstream"); + + cleanupUpstreamAndDownstream(); +} + +// Should match an http header, rewrite the response code, and modify the upstream response body +// (non-empty upstream response body) +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeaderWithBody) { + auto response = getUpstreamResponse(match_test_header_rewrite_435_yaml, "200", 512); + + EXPECT_EQ("435", response->headers().Status()->value().getStringView()); + EXPECT_EQ(response->body(), "435: via_upstream"); + + cleanupUpstreamAndDownstream(); +} + +// Should match an http response code, add a response header, but not add any body to +// a HEAD response, even though a body format is configured. +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeadResponseAddHeaderNoBody) { + const std::string yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 500 + runtime_key: key_b + headers_to_add: + - header: + key: x-upstream-500 + value: "1" + append: true +body_format: + text_format_source: + inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" +)EOF"; + auto response = getUpstreamResponse(yaml, "500", 0, "HEAD"); + + EXPECT_EQ("500", response->headers().Status()->value().getStringView()); + EXPECT_EQ(1, response->headers().get(Http::LowerCaseString("x-upstream-500")).size()); + EXPECT_EQ( + "1", + response->headers().get(Http::LowerCaseString("x-upstream-500"))[0]->value().getStringView()); + EXPECT_EQ(response->body(), ""); cleanupUpstreamAndDownstream(); } diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index 81f6bcd093ff6..13033a157c7f6 100644 --- a/test/mocks/http/mocks.cc +++ b/test/mocks/http/mocks.cc @@ -90,7 +90,7 @@ void MockStreamDecoderFilterCallbacks::sendLocalReply_( Utility::sendLocalReply( stream_destroyed_, Utility::EncodeFunctions{ - nullptr, nullptr, + nullptr, nullptr, nullptr, [this, modify_headers, details](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { modify_headers(*headers); diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index 73a5d1a0d6334..571d9e22f303f 100644 --- a/test/tools/router_check/router.cc +++ b/test/tools/router_check/router.cc @@ -170,7 +170,7 @@ void RouterCheckTool::finalizeHeaders(ToolConfig& tool_config, void RouterCheckTool::sendLocalReply(ToolConfig& tool_config, const Router::DirectResponseEntry& entry) { auto encode_functions = Envoy::Http::Utility::EncodeFunctions{ - nullptr, nullptr, + nullptr, nullptr, nullptr, [&](Envoy::Http::ResponseHeaderMapPtr&& headers, bool end_stream) -> void { UNREFERENCED_PARAMETER(end_stream); Http::HeaderMapImpl::copyFrom(*tool_config.response_headers_->header_map_, *headers); @@ -182,8 +182,9 @@ void RouterCheckTool::sendLocalReply(ToolConfig& tool_config, bool is_grpc = false; bool is_head_request = false; - Envoy::Http::Utility::LocalReplyData local_reply_data{ - is_grpc, entry.responseCode(), entry.responseBody(), absl::nullopt, is_head_request}; + Envoy::Http::Utility::LocalReplyData local_reply_data{is_direct_local_reply, is_grpc, + entry.responseCode(), entry.responseBody(), + absl::nullopt, is_head_request}; Envoy::Http::Utility::sendLocalReply(false, encode_functions, local_reply_data); } From cc4b6181422d9d2a5f91f62a03c44bf8b94ad880 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 8 Feb 2021 16:59:54 +0000 Subject: [PATCH 04/10] Fix up a test Signed-off-by: John Esmet --- test/common/local_reply/local_reply_test.cc | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/test/common/local_reply/local_reply_test.cc b/test/common/local_reply/local_reply_test.cc index 64afad622bd1a..b3b6975ba983c 100644 --- a/test/common/local_reply/local_reply_test.cc +++ b/test/common/local_reply/local_reply_test.cc @@ -29,6 +29,9 @@ class LocalReplyTest : public testing::Test { code_ = code; body_ = TestInitBody; content_type_ = TestInitContentType; + // LocalReply::matchesAnyMapper() is used in a context where response_headers are assumed to + // exist // so we have to set the response code in the headers here. + response_headers_.setStatus(std::to_string(enumToInt(code))); } void resetData(uint32_t code) { resetData(static_cast(code)); } @@ -49,6 +52,7 @@ TEST_F(LocalReplyTest, TestEmptyConfig) { // Empty LocalReply config. auto local = Factory::create(config_, context_); + EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -62,6 +66,7 @@ TEST_F(LocalReplyTest, TestDefaultLocalReply) { // Default LocalReply should be the same as empty config. auto local = Factory::createDefault(); + EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -112,6 +117,7 @@ TEST_F(LocalReplyTest, TestDefaultTextFormatter) { TestUtility::loadFromYaml(yaml, config_); auto local = Factory::create(config_, context_); + EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -134,6 +140,7 @@ TEST_F(LocalReplyTest, TestDefaultJsonFormatter) { TestUtility::loadFromYaml(yaml, config_); auto local = Factory::create(config_, context_); + EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -203,6 +210,7 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=400 matches the first filter; rewrite code and body resetData(400); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(401)); EXPECT_EQ(stream_info_.response_code_, 401U); @@ -213,6 +221,7 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=403 matches the second filter; does not rewrite code, sets an empty body and content_type. resetData(403); body_ = "original body text"; + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(403)); EXPECT_EQ(stream_info_.response_code_, 403U); @@ -222,6 +231,7 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=410 matches the third filter; rewrite body only resetData(410); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(410)); EXPECT_EQ(stream_info_.response_code_, 410U); @@ -231,6 +241,7 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=420 matches the fourth filter; rewrite code only resetData(420); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(421)); EXPECT_EQ(stream_info_.response_code_, 421U); @@ -240,6 +251,7 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=430 matches the fifth filter; rewrite nothing resetData(430); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(430)); EXPECT_EQ(stream_info_.response_code_, 430U); @@ -271,6 +283,7 @@ TEST_F(LocalReplyTest, DEPRECATED_FEATURE_TEST(TestMapperRewriteDeprecatedTextFo // code=404 matches the only filter; does not rewrite code, sets an empty body and content_type. resetData(404); body_ = "original body text"; + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(404)); EXPECT_EQ(stream_info_.response_code_, 404U); @@ -319,6 +332,7 @@ TEST_F(LocalReplyTest, TestMapperFormat) { // code=400 matches the first filter; rewrite code and body // has its own formatter resetData(400); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(401)); EXPECT_EQ(stream_info_.response_code_, 401U); @@ -336,6 +350,7 @@ TEST_F(LocalReplyTest, TestMapperFormat) { // code=410 matches the second filter; rewrite code and body // but using default formatter resetData(410); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(411)); EXPECT_EQ(stream_info_.response_code_, 411U); @@ -374,6 +389,7 @@ TEST_F(LocalReplyTest, TestHeaderAddition) { response_headers_.addCopy("foo-2", "bar2"); response_headers_.addCopy("foo-3", "bar3"); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -440,6 +456,7 @@ TEST_F(LocalReplyTest, TestMapperWithContentType) { // has its own formatter. // content-type is explicitly set to text/html; charset=UTF-8. resetData(400); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(401)); EXPECT_EQ(stream_info_.response_code_, 401U); @@ -451,6 +468,7 @@ TEST_F(LocalReplyTest, TestMapperWithContentType) { // but using default formatter. // content-type is explicitly set to text/html; charset=UTF-8. resetData(410); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(411)); EXPECT_EQ(stream_info_.response_code_, 411U); @@ -462,7 +480,8 @@ TEST_F(LocalReplyTest, TestMapperWithContentType) { // has its own formatter. // default content-type is set based on reply format type. resetData(420); - local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); + EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_)); EXPECT_EQ(code_, static_cast(421)); EXPECT_EQ(stream_info_.response_code_, 421U); EXPECT_EQ(response_headers_.Status()->value().getStringView(), "421"); From 7a2f5de78a773bde88dd75b6f117f74c065d3c79 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 8 Feb 2021 17:08:16 +0000 Subject: [PATCH 05/10] Fix test formatting Signed-off-by: John Esmet --- test/common/local_reply/local_reply_test.cc | 51 ++++++++++++++------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/test/common/local_reply/local_reply_test.cc b/test/common/local_reply/local_reply_test.cc index b3b6975ba983c..73384b373af88 100644 --- a/test/common/local_reply/local_reply_test.cc +++ b/test/common/local_reply/local_reply_test.cc @@ -52,7 +52,8 @@ TEST_F(LocalReplyTest, TestEmptyConfig) { // Empty LocalReply config. auto local = Factory::create(config_, context_); - EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(false, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -66,7 +67,8 @@ TEST_F(LocalReplyTest, TestDefaultLocalReply) { // Default LocalReply should be the same as empty config. auto local = Factory::createDefault(); - EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(false, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -117,7 +119,8 @@ TEST_F(LocalReplyTest, TestDefaultTextFormatter) { TestUtility::loadFromYaml(yaml, config_); auto local = Factory::create(config_, context_); - EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(false, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -140,7 +143,8 @@ TEST_F(LocalReplyTest, TestDefaultJsonFormatter) { TestUtility::loadFromYaml(yaml, config_); auto local = Factory::create(config_, context_); - EXPECT_EQ(false, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(false, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -210,7 +214,8 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=400 matches the first filter; rewrite code and body resetData(400); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(401)); EXPECT_EQ(stream_info_.response_code_, 401U); @@ -221,7 +226,8 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=403 matches the second filter; does not rewrite code, sets an empty body and content_type. resetData(403); body_ = "original body text"; - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(403)); EXPECT_EQ(stream_info_.response_code_, 403U); @@ -231,7 +237,8 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=410 matches the third filter; rewrite body only resetData(410); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(410)); EXPECT_EQ(stream_info_.response_code_, 410U); @@ -241,7 +248,8 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=420 matches the fourth filter; rewrite code only resetData(420); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(421)); EXPECT_EQ(stream_info_.response_code_, 421U); @@ -251,7 +259,8 @@ TEST_F(LocalReplyTest, TestMapperRewrite) { // code=430 matches the fifth filter; rewrite nothing resetData(430); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(430)); EXPECT_EQ(stream_info_.response_code_, 430U); @@ -283,7 +292,8 @@ TEST_F(LocalReplyTest, DEPRECATED_FEATURE_TEST(TestMapperRewriteDeprecatedTextFo // code=404 matches the only filter; does not rewrite code, sets an empty body and content_type. resetData(404); body_ = "original body text"; - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(404)); EXPECT_EQ(stream_info_.response_code_, 404U); @@ -332,7 +342,8 @@ TEST_F(LocalReplyTest, TestMapperFormat) { // code=400 matches the first filter; rewrite code and body // has its own formatter resetData(400); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(401)); EXPECT_EQ(stream_info_.response_code_, 401U); @@ -350,7 +361,8 @@ TEST_F(LocalReplyTest, TestMapperFormat) { // code=410 matches the second filter; rewrite code and body // but using default formatter resetData(410); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(411)); EXPECT_EQ(stream_info_.response_code_, 411U); @@ -389,7 +401,8 @@ TEST_F(LocalReplyTest, TestHeaderAddition) { response_headers_.addCopy("foo-2", "bar2"); response_headers_.addCopy("foo-3", "bar3"); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, TestInitCode); EXPECT_EQ(stream_info_.response_code_, static_cast(TestInitCode)); @@ -456,7 +469,8 @@ TEST_F(LocalReplyTest, TestMapperWithContentType) { // has its own formatter. // content-type is explicitly set to text/html; charset=UTF-8. resetData(400); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(401)); EXPECT_EQ(stream_info_.response_code_, 401U); @@ -468,7 +482,8 @@ TEST_F(LocalReplyTest, TestMapperWithContentType) { // but using default formatter. // content-type is explicitly set to text/html; charset=UTF-8. resetData(410); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); EXPECT_EQ(code_, static_cast(411)); EXPECT_EQ(stream_info_.response_code_, 411U); @@ -480,8 +495,10 @@ TEST_F(LocalReplyTest, TestMapperWithContentType) { // has its own formatter. // default content-type is set based on reply format type. resetData(420); - EXPECT_EQ(true, local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); - EXPECT_EQ(true, local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_)); + EXPECT_EQ(true, + local->matchesAnyMapper(&request_headers_, response_headers_, nullptr, stream_info_)); + EXPECT_EQ(true, local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, + content_type_)); EXPECT_EQ(code_, static_cast(421)); EXPECT_EQ(stream_info_.response_code_, 421U); EXPECT_EQ(response_headers_.Status()->value().getStringView(), "421"); From a4e01f4404765abe09c75347a9e15d61953171e7 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 10 Feb 2021 01:30:02 +0000 Subject: [PATCH 06/10] Fix test Signed-off-by: John Esmet --- test/tools/router_check/router.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index 571d9e22f303f..83f7a39ea9146 100644 --- a/test/tools/router_check/router.cc +++ b/test/tools/router_check/router.cc @@ -182,9 +182,8 @@ void RouterCheckTool::sendLocalReply(ToolConfig& tool_config, bool is_grpc = false; bool is_head_request = false; - Envoy::Http::Utility::LocalReplyData local_reply_data{is_direct_local_reply, is_grpc, - entry.responseCode(), entry.responseBody(), - absl::nullopt, is_head_request}; + Envoy::Http::Utility::LocalReplyData local_reply_data{ + is_grpc, entry.responseCode(), entry.responseBody(), absl::nullopt, is_head_request}; Envoy::Http::Utility::sendLocalReply(false, encode_functions, local_reply_data); } From 12320a125f03df64b8add5e3bce0b2eb6140dabc Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 10 Feb 2021 04:38:39 +0000 Subject: [PATCH 07/10] Add more tests around content type and content length Signed-off-by: John Esmet --- source/common/http/filter_manager.cc | 40 +++--- .../local_reply_integration_test.cc | 136 ++++++++++++++++-- test/mocks/http/mocks.cc | 7 +- 3 files changed, 152 insertions(+), 31 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index b22cedffa49b2..965e4e20bba56 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1089,7 +1089,7 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea rewriteResponse(); if (buffered_response_data_) { - // If we're going to rewrite the response here, then modified_end_stream can no longer be true + // If we have a rewritten response body, then modified_end_stream can no longer be true // because we have a body now. modified_end_stream = false; } @@ -1307,39 +1307,47 @@ void FilterManager::rewriteResponse() { absl::string_view rewritten_content_type{}; Http::Code rewritten_code{static_cast(Utility::getResponseStatus(*response_headers))}; - // Start with the local reply body, if we have it. + // Start with the local reply body and text/plain content type, if we have it. if (local_reply_data_) { rewritten_body = local_reply_data_->body_text_; + rewritten_content_type = Headers::get().ContentTypeValues.Text; } + // Get rid of any buffered response data we may have at this point. We're going to use this + // as the output parameter for a rewritten body, if any. + buffered_response_data_ = nullptr; + ENVOY_STREAM_LOG(trace, "rewriteResponse: calling local_reply_.rewrite with body=\"{}\", code={}", *this, rewritten_body, rewritten_code); - const bool did_rewrite = + const bool did_body_rewrite = local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), *response_headers, stream_info_, rewritten_code, rewritten_body, rewritten_content_type); ENVOY_STREAM_LOG( - trace, "rewriteResponse: local_reply_.rewrite returned body=\"{}\", content_type={}, code={}", - *this, rewritten_body, rewritten_content_type, rewritten_code); + trace, + "rewriteResponse: local_reply_.rewrite returned did_body_rewrite={}, body=\"{}\", " + "content_type={}, code={}", + *this, did_body_rewrite, rewritten_body, rewritten_content_type, rewritten_code); if (local_reply_data_ && local_reply_data_->is_grpc_) { // Send a trailers-only grpc response Utility::toGrpcTrailersOnlyResponse(*response_headers, rewritten_code, rewritten_body, local_reply_data_->grpc_status_, local_reply_data_->is_head_request_); - // We're sending a trailers-only response with no body. Make sure there's no buffered - // response data nor a content length header. - buffered_response_data_ = nullptr; return; } - buffered_response_data_ = - did_rewrite ? std::make_unique(rewritten_body) : nullptr; - if (buffered_response_data_) { - // Since we overwrote the response body, we need to set the content-length too. - response_headers->setContentLength(buffered_response_data_->length()); - } - if (!rewritten_content_type.empty()) { - response_headers->setContentType(rewritten_content_type); + if (did_body_rewrite) { + buffered_response_data_ = std::make_unique(rewritten_body); + + // If we rewrote with a non-empty body, set the content length and type. + // Otherwise, remove them. + if (buffered_response_data_->length() > 0) { + response_headers->setContentLength(buffered_response_data_->length()); + response_headers->setContentType(rewritten_content_type); + } else { + response_headers->removeContentLength(); + response_headers->removeContentType(); + } } } diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc index 5c5de4085f8c0..ce155f5f6d81d 100644 --- a/test/integration/local_reply_integration_test.cc +++ b/test/integration/local_reply_integration_test.cc @@ -32,12 +32,16 @@ class LocalReplyIntegrationTest : public HttpProtocolIntegrationTest { waitForNextUpstreamRequest(); if (upstream_body_size > 0) { - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", response_code}}, - false); + upstream_request_->encodeHeaders( + Http::TestResponseHeaderMapImpl{{":status", response_code}, + {"content-type", "application/original"}, + {"content-length", std::to_string(upstream_body_size)}}, + false); upstream_request_->encodeData(upstream_body_size, true); } else { - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", response_code}}, - true); + upstream_request_->encodeHeaders( + Http::TestResponseHeaderMapImpl{{":status", response_code}, {"content-length", "0"}}, + true); } response->waitForHeaders(); return response; @@ -521,7 +525,8 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyNoMatchNoUpstreamBody) { EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ(0, response->headers().get(Http::LowerCaseString("x-upstream-5xx")).size()); - EXPECT_EQ(response->body(), ""); + EXPECT_EQ("", response->body()); + EXPECT_EQ("", response->headers().getContentTypeValue()); cleanupUpstreamAndDownstream(); } @@ -532,7 +537,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyNoMatchWithUpstreamBody) { EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ(0, response->headers().get(Http::LowerCaseString("x-upstream-5xx")).size()); - EXPECT_EQ(response->body(), std::string(8, 'a')); + EXPECT_EQ(std::string(8, 'a'), response->body()); + EXPECT_EQ("application/original", response->headers().getContentTypeValue()); + EXPECT_EQ("8", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -558,6 +565,8 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusNoBody) { EXPECT_EQ("450", response->headers().Status()->value().getStringView()); EXPECT_EQ(response->body(), "450: via_upstream"); + EXPECT_EQ("text/plain", response->headers().getContentTypeValue()); + EXPECT_EQ("17", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -567,7 +576,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithBody) { auto response = getUpstreamResponse(match_429_rewrite_450_yaml, "429", 512); EXPECT_EQ("450", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), "450: via_upstream"); + EXPECT_EQ("450: via_upstream", response->body()); + EXPECT_EQ("text/plain", response->headers().getContentTypeValue()); + EXPECT_EQ("17", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -592,7 +603,8 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusRemoveEmptyBody) auto response = getUpstreamResponse(match_429_rewrite_451_remove_body_yaml, "429", 0); EXPECT_EQ("451", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), ""); + EXPECT_EQ("", response->body()); + EXPECT_EQ("", response->headers().getContentTypeValue()); cleanupUpstreamAndDownstream(); } @@ -602,7 +614,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusRemoveNonEmptyBo auto response = getUpstreamResponse(match_429_rewrite_451_remove_body_yaml, "429", 512); EXPECT_EQ("451", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), ""); + EXPECT_EQ("", response->body()); + EXPECT_EQ("", response->headers().getContentTypeValue()); + EXPECT_EQ("", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -625,7 +639,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithUnmodifiedEm auto response = getUpstreamResponse(match_429_rewrite_475_unmodified_body_yaml, "429", 0); EXPECT_EQ("475", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), ""); + // The unmodified upstream body is empty and has no content type. + EXPECT_EQ("", response->body()); + EXPECT_EQ("", response->headers().getContentTypeValue()); cleanupUpstreamAndDownstream(); } @@ -636,7 +652,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamStatusWithUnmodifiedNo auto response = getUpstreamResponse(match_429_rewrite_475_unmodified_body_yaml, "429", 8); EXPECT_EQ("475", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), std::string(8, 'a')); + EXPECT_EQ(std::string(8, 'a'), response->body()); + EXPECT_EQ("application/original", response->headers().getContentTypeValue()); + EXPECT_EQ("8", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -660,7 +678,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeaderNoBody) { auto response = getUpstreamResponse(match_test_header_rewrite_435_yaml, "200", 0); EXPECT_EQ("435", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), "435: via_upstream"); + EXPECT_EQ("435: via_upstream", response->body()); + EXPECT_EQ("text/plain", response->headers().getContentTypeValue()); + EXPECT_EQ("17", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -671,7 +691,93 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeaderWithBody) { auto response = getUpstreamResponse(match_test_header_rewrite_435_yaml, "200", 512); EXPECT_EQ("435", response->headers().Status()->value().getStringView()); - EXPECT_EQ(response->body(), "435: via_upstream"); + EXPECT_EQ("435: via_upstream", response->body()); + EXPECT_EQ("text/plain", response->headers().getContentTypeValue()); + EXPECT_EQ("17", response->headers().getContentLengthValue()); + + cleanupUpstreamAndDownstream(); +} + +const std::string match_500_rewrite_content_type_yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 500 + runtime_key: key_b +body_format: + content_type: application/custom + text_format_source: + inline_string: "%RESPONSE_CODE%: %RESPONSE_CODE_DETAILS%" +)EOF"; + +// Should match an http header, rewrite the content type, and modify the upstream response body (no +// upstream response body) +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeaderRewriteContentTypeNoBody) { + auto response = getUpstreamResponse(match_500_rewrite_content_type_yaml, "500", 0); + + EXPECT_EQ("500", response->headers().Status()->value().getStringView()); + EXPECT_EQ("500: via_upstream", response->body()); + EXPECT_EQ("application/custom", response->headers().getContentTypeValue()); + EXPECT_EQ("34", response->headers().getContentLengthValue()); + + cleanupUpstreamAndDownstream(); +} + +// Should match an http header, rewrite the content type, and modify the upstream response body +// (non-empty upstream response body) +TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeaderRewriteContentTypeWithBody) { + auto response = getUpstreamResponse(match_500_rewrite_content_type_yaml, "500", 512); + + EXPECT_EQ("500", response->headers().Status()->value().getStringView()); + EXPECT_EQ("500: via_upstream", response->body()); + EXPECT_EQ("application/custom", response->headers().getContentTypeValue()); + EXPECT_EQ("34", response->headers().getContentLengthValue()); + + cleanupUpstreamAndDownstream(); +} + +const std::string match_400_rewrite_json_yaml = R"EOF( +mappers: +- filter: + status_code_filter: + comparison: + op: EQ + value: + default_value: 400 + runtime_key: key_b +body_format: + json_format: + response_code: "%RESPONSE_CODE%" + details: "%RESPONSE_CODE_DETAILS%" +)EOF"; + +// Should match an http header, rewrite the content type, and modify the upstream response body +// (non-empty upstream response body) +TEST_P(LocalReplyIntegrationTest, LocalyReplyRewriteToJsonNoBody) { + auto response = getUpstreamResponse(match_400_rewrite_json_yaml, "400", 0); + + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + EXPECT_TRUE(TestUtility::jsonStringEqual(response->body(), + "{\"response_code\":400,\"details\":\"via_upstream\"}")); + EXPECT_EQ("application/json", response->headers().getContentTypeValue()); + EXPECT_EQ("47", response->headers().getContentLengthValue()); + + cleanupUpstreamAndDownstream(); +} + +// Should match an http header, rewrite the content type, and modify the upstream response body +// (non-empty upstream response body) +TEST_P(LocalReplyIntegrationTest, LocalyReplyRewriteToJsonWithBody) { + auto response = getUpstreamResponse(match_400_rewrite_json_yaml, "400", 512); + + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + EXPECT_EQ("application/json", response->headers().getContentTypeValue()); + EXPECT_TRUE(TestUtility::jsonStringEqual(response->body(), + "{\"response_code\":400,\"details\":\"via_upstream\"}")); + EXPECT_EQ("47", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } @@ -704,7 +810,9 @@ TEST_P(LocalReplyIntegrationTest, LocalyReplyMatchUpstreamHeadResponseAddHeaderN EXPECT_EQ( "1", response->headers().get(Http::LowerCaseString("x-upstream-500"))[0]->value().getStringView()); - EXPECT_EQ(response->body(), ""); + EXPECT_EQ("", response->body()); + EXPECT_EQ("text/plain", response->headers().getContentTypeValue()); + EXPECT_EQ("17", response->headers().getContentLengthValue()); cleanupUpstreamAndDownstream(); } diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index 13033a157c7f6..3a6dbc6619a74 100644 --- a/test/mocks/http/mocks.cc +++ b/test/mocks/http/mocks.cc @@ -90,7 +90,12 @@ void MockStreamDecoderFilterCallbacks::sendLocalReply_( Utility::sendLocalReply( stream_destroyed_, Utility::EncodeFunctions{ - nullptr, nullptr, nullptr, + nullptr, nullptr, + [this](ResponseHeaderMap& headers, Code& code, std::string& body, + const absl::optional grpc_status, + bool is_head_request) -> void { + Utility::toGrpcTrailersOnlyResponse(headers, code, body, grpc_status, is_head_request); + }, [this, modify_headers, details](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { modify_headers(*headers); From cf285b65a46a8f048a0e6b0c26cb9b418645d767 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 11 Feb 2021 15:56:20 +0000 Subject: [PATCH 08/10] Fix some issues found with other tests Signed-off-by: John Esmet --- source/common/local_reply/local_reply.cc | 3 ++- test/mocks/http/mocks.cc | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/local_reply/local_reply.cc b/source/common/local_reply/local_reply.cc index 0e12fa80446fb..e0d9b8806263d 100644 --- a/source/common/local_reply/local_reply.cc +++ b/source/common/local_reply/local_reply.cc @@ -152,7 +152,8 @@ using ResponseMapperPtr = std::unique_ptr; class LocalReplyImpl : public LocalReply { public: - LocalReplyImpl() : body_formatter_(std::make_unique()) {} + LocalReplyImpl() + : body_formatter_(std::make_unique()), has_configured_body_formatter_(false) {} LocalReplyImpl( const envoy::extensions::filters::network::http_connection_manager::v3::LocalReplyConfig& diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index 3a6dbc6619a74..143734394a904 100644 --- a/test/mocks/http/mocks.cc +++ b/test/mocks/http/mocks.cc @@ -91,7 +91,7 @@ void MockStreamDecoderFilterCallbacks::sendLocalReply_( stream_destroyed_, Utility::EncodeFunctions{ nullptr, nullptr, - [this](ResponseHeaderMap& headers, Code& code, std::string& body, + [](ResponseHeaderMap& headers, Code& code, std::string& body, const absl::optional grpc_status, bool is_head_request) -> void { Utility::toGrpcTrailersOnlyResponse(headers, code, body, grpc_status, is_head_request); From 6fbfb7797828754f4c667862d89a79de63231201 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 11 Feb 2021 16:01:29 +0000 Subject: [PATCH 09/10] Formatting Signed-off-by: John Esmet --- test/mocks/http/mocks.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index 143734394a904..3e53e79dd35a9 100644 --- a/test/mocks/http/mocks.cc +++ b/test/mocks/http/mocks.cc @@ -92,8 +92,8 @@ void MockStreamDecoderFilterCallbacks::sendLocalReply_( Utility::EncodeFunctions{ nullptr, nullptr, [](ResponseHeaderMap& headers, Code& code, std::string& body, - const absl::optional grpc_status, - bool is_head_request) -> void { + const absl::optional grpc_status, + bool is_head_request) -> void { Utility::toGrpcTrailersOnlyResponse(headers, code, body, grpc_status, is_head_request); }, [this, modify_headers, details](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { From 0e7e0fe4bfcd0cae7e3fe3c5e5229b5ddea828f7 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 12 Feb 2021 02:50:54 +0000 Subject: [PATCH 10/10] Fix local reply mock Signed-off-by: John Esmet --- test/mocks/local_reply/mocks.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mocks/local_reply/mocks.h b/test/mocks/local_reply/mocks.h index c13d9e9f9318b..52b03d1536a98 100644 --- a/test/mocks/local_reply/mocks.h +++ b/test/mocks/local_reply/mocks.h @@ -9,7 +9,7 @@ class MockLocalReply : public LocalReply { MockLocalReply(); ~MockLocalReply() override; - MOCK_METHOD(void, rewrite, + MOCK_METHOD(bool, rewrite, (const Http::RequestHeaderMap* request_headers, Http::ResponseHeaderMap& response_headers, StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body, absl::string_view& content_type),