From 4ff414b8966ba746184582df36b66bfa3b2065fb Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 6 Dec 2022 16:37:05 -0500 Subject: [PATCH] mobile: changing direct response to not do a loopback pass Signed-off-by: Alyssa Wilk --- mobile/library/common/config/config.cc | 10 +++++++- .../extensions/filters/http/local_error/BUILD | 1 + .../filters/http/local_error/filter.cc | 4 ++- .../filters/http/network_configuration/BUILD | 1 + .../http/network_configuration/filter.cc | 4 ++- .../filters/http/platform_bridge/filter.cc | 2 +- mobile/library/common/http/BUILD | 1 + mobile/library/common/http/header_utility.cc | 11 ++++++++ mobile/library/common/http/header_utility.h | 9 +++++++ .../integration/client_integration_test.cc | 25 +++++++++++++++++++ 10 files changed, 64 insertions(+), 4 deletions(-) diff --git a/mobile/library/common/config/config.cc b/mobile/library/common/config/config.cc index dc28190962d19..e328a57d01a1e 100644 --- a/mobile/library/common/config/config.cc +++ b/mobile/library/common/config/config.cc @@ -267,7 +267,6 @@ const char* config_template = R"( - name: remote_service domains: ["*"] routes: -#{fake_remote_responses} - match: { prefix: "/" } direct_response: { status: 404, body: { inline_string: "not found" } } request_headers_to_remove: @@ -375,6 +374,15 @@ const char* config_template = R"( route_config: name: api_router virtual_hosts: + - name: remote_service + domains: ["127.0.0.1"] + routes: +#{fake_remote_responses} + - match: { prefix: "/" } + direct_response: { status: 404, body: { inline_string: "not found" } } + request_headers_to_remove: + - x-forwarded-proto + - x-envoy-mobile-cluster )" // The list of virtual hosts impacts directly the number of virtual cluster stats. // That's because we create a separate set of virtual clusters stats for every diff --git a/mobile/library/common/extensions/filters/http/local_error/BUILD b/mobile/library/common/extensions/filters/http/local_error/BUILD index f11fc51dbf1f6..3c3b8248a208c 100644 --- a/mobile/library/common/extensions/filters/http/local_error/BUILD +++ b/mobile/library/common/extensions/filters/http/local_error/BUILD @@ -21,6 +21,7 @@ envoy_cc_extension( repository = "@envoy", deps = [ ":filter_cc_proto", + "//library/common/http:header_utility_lib", "//library/common/http:internal_headers_lib", "//library/common/types:c_types_lib", "@envoy//envoy/http:codes_interface", diff --git a/mobile/library/common/extensions/filters/http/local_error/filter.cc b/mobile/library/common/extensions/filters/http/local_error/filter.cc index 0f6bd20bf4c9c..2c6c26265a8a3 100644 --- a/mobile/library/common/extensions/filters/http/local_error/filter.cc +++ b/mobile/library/common/extensions/filters/http/local_error/filter.cc @@ -2,6 +2,8 @@ #include "envoy/server/filter_config.h" +#include "library/common/http/header_utility.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -18,7 +20,7 @@ Http::LocalErrorStatus LocalErrorFilter::onLocalReply(const LocalReplyData& repl // ASSERT(reply.details_ == info.responseCodeDetails().value()); info.setResponseCode(static_cast(reply.code_)); - return Http::LocalErrorStatus::ContinueAndResetStream; + return Http::Utility::statusForOnLocalReply(reply, decoder_callbacks_->streamInfo()); } } // namespace LocalError diff --git a/mobile/library/common/extensions/filters/http/network_configuration/BUILD b/mobile/library/common/extensions/filters/http/network_configuration/BUILD index 6e3788d4e16f5..ad0cce1697a77 100644 --- a/mobile/library/common/extensions/filters/http/network_configuration/BUILD +++ b/mobile/library/common/extensions/filters/http/network_configuration/BUILD @@ -21,6 +21,7 @@ envoy_cc_extension( repository = "@envoy", deps = [ ":filter_cc_proto", + "//library/common/http:header_utility_lib", "//library/common/http:internal_headers_lib", "//library/common/network:connectivity_manager_lib", "//library/common/stream_info:extra_stream_info_lib", diff --git a/mobile/library/common/extensions/filters/http/network_configuration/filter.cc b/mobile/library/common/extensions/filters/http/network_configuration/filter.cc index 428bc66e6710d..7721406031ad8 100644 --- a/mobile/library/common/extensions/filters/http/network_configuration/filter.cc +++ b/mobile/library/common/extensions/filters/http/network_configuration/filter.cc @@ -4,6 +4,8 @@ #include "source/common/network/filter_state_proxy_info.h" +#include "library/common/http/header_utility.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -159,7 +161,7 @@ Http::LocalErrorStatus NetworkConfigurationFilter::onLocalReply(const LocalReply connectivity_manager_->reportNetworkUsage(extra_stream_info_->configuration_key_.value(), network_fault); - return Http::LocalErrorStatus::ContinueAndResetStream; + return Http::Utility::statusForOnLocalReply(reply, decoder_callbacks_->streamInfo()); } void NetworkConfigurationFilter::onDestroy() { dns_cache_handle_.reset(); } diff --git a/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc b/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc index baea5572adc4e..1bc8d4cd90621 100644 --- a/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc +++ b/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc @@ -214,7 +214,7 @@ Http::LocalErrorStatus PlatformBridgeFilter::onLocalReply(const LocalReplyData& finalStreamIntel(), platform_filter_.instance_context); } - return Http::LocalErrorStatus::ContinueAndResetStream; + return Http::Utility::statusForOnLocalReply(reply, decoder_callbacks_->streamInfo()); } envoy_final_stream_intel PlatformBridgeFilter::finalStreamIntel() { diff --git a/mobile/library/common/http/BUILD b/mobile/library/common/http/BUILD index 0ad55b4c06cc7..495422b437815 100644 --- a/mobile/library/common/http/BUILD +++ b/mobile/library/common/http/BUILD @@ -55,6 +55,7 @@ envoy_cc_library( "//library/common/data:utility_lib", "//library/common/types:c_types_lib", "@envoy//envoy/buffer:buffer_interface", + "@envoy//envoy/http:filter_interface", "@envoy//envoy/http:header_map_interface", "@envoy//source/common/http:header_map_lib", "@envoy//source/extensions/http/header_formatters/preserve_case:preserve_case_formatter", diff --git a/mobile/library/common/http/header_utility.cc b/mobile/library/common/http/header_utility.cc index 99d689e51c934..9f41b728dfca4 100644 --- a/mobile/library/common/http/header_utility.cc +++ b/mobile/library/common/http/header_utility.cc @@ -9,6 +9,17 @@ namespace Envoy { namespace Http { namespace Utility { +Http::LocalErrorStatus statusForOnLocalReply(const StreamDecoderFilter::LocalReplyData& reply, + const StreamInfo::StreamInfo& info) { + // This is a horrible hack to work around legacy swift direct response API. + // TODO(https://github.com/envoyproxy/envoy/issues/24428) remove. + if (reply.details_ == "direct_response" && info.getRequestHeaders() && + info.getRequestHeaders()->getHostValue() == "127.0.0.1") { + return Http::LocalErrorStatus::Continue; + } + return Http::LocalErrorStatus::ContinueAndResetStream; +} + void toEnvoyHeaders(HeaderMap& envoy_result_headers, envoy_headers headers) { Envoy::Http::StatefulHeaderKeyFormatter& formatter = envoy_result_headers.formatter().value(); for (envoy_map_size_t i = 0; i < headers.length; i++) { diff --git a/mobile/library/common/http/header_utility.h b/mobile/library/common/http/header_utility.h index b30de35281513..5da377223706e 100644 --- a/mobile/library/common/http/header_utility.h +++ b/mobile/library/common/http/header_utility.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/buffer/buffer.h" +#include "envoy/http/filter.h" #include "envoy/http/header_map.h" #include "library/common/types/c_types.h" @@ -9,6 +10,14 @@ namespace Envoy { namespace Http { namespace Utility { +/* + * Returns the proper status code for onLocalReply + * @param reply the local reply data for the stream. + * @param stream_info the info for the stream. + */ +Http::LocalErrorStatus statusForOnLocalReply(const StreamDecoderFilter::LocalReplyData& reply, + const StreamInfo::StreamInfo& stream_info); + /** * Transform envoy_headers to the supplied HeaderMap * This function copies the content. diff --git a/mobile/test/common/integration/client_integration_test.cc b/mobile/test/common/integration/client_integration_test.cc index 4a25fb7e7897a..f5c25d30295ce 100644 --- a/mobile/test/common/integration/client_integration_test.cc +++ b/mobile/test/common/integration/client_integration_test.cc @@ -315,5 +315,30 @@ TEST_P(ClientIntegrationTest, DISABLED_Proxying) { ASSERT_EQ(cc_.on_complete_calls, 2); } +TEST_P(ClientIntegrationTest, DirectResponse) { + initialize(); + if (version_ == Network::Address::IpVersion::v6) { + // Localhost only resolves to an ipv4 address - alas no kernel happy eyeballs. + return; + } + + // Override to not validate stream intel. + stream_prototype_->setOnComplete( + [this](envoy_stream_intel, envoy_final_stream_intel final_intel) { + cc_.on_complete_received_byte_count = final_intel.received_byte_count; + cc_.on_complete_calls++; + cc_.terminal_callback->setReady(); + }); + + default_request_headers_.setHost("127.0.0.1"); + default_request_headers_.setPath("/"); + + stream_->sendHeaders(envoyToMobileHeaders(default_request_headers_), true); + terminal_callback_.waitReady(); + ASSERT_EQ(cc_.status, "404"); + ASSERT_EQ(cc_.on_headers_calls, 1); + stream_.reset(); +} + } // namespace } // namespace Envoy