From be7b2f65d00cc74657442d6e4cab85abe90abf53 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Tue, 8 Jun 2021 20:29:34 -0600 Subject: [PATCH 01/12] listener: match rebalancer to listener IP family type Commit Message: When getting a rebalancer by address and a wild card match is being used, the first match in the list is returned. However, if there are listeners with addresses "0.0.0.0" and "::" then the first active listener found will be used, irrespective of the IP family type. Change the behavior to always return the listener of the same IP family type as the rebalancer. Risk Level: Medium Testing: TBD Docs Changes: TBD Release Notes: inline Fixes #16804 Runtime guard: envoy.reloadable_features.listener_wildcard_match_ip_family Co-authored-by: yingchun.cai@volunteers.acasi.info Signed-off-by: Jacob Delgado --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 1 + source/server/BUILD | 1 + source/server/connection_handler_impl.cc | 36 ++++++++++++++++------- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3e88b63f3c90b..07e0078e0cf31 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -36,6 +36,7 @@ Minor Behavior Changes * listener: respect the :ref:`connection balance config ` defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior. * tcp: switched to the new connection pool by default. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.new_tcp_connection_pool`` to false. +* listener: added an option when balancing across active listeners and wildcard matching is used to return the listener that matches the IP family type associated with the listener's socket address. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.listener_wildcard_match_ip_family`` to false. Bug Fixes --------- diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index b1aa82b623822..8af87c3cfcc05 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -76,6 +76,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.improved_stream_limit_handling", "envoy.reloadable_features.internal_redirects_with_body", "envoy.reloadable_features.new_tcp_connection_pool", + "envoy.reloadable_features.listener_wildcard_match_ip_family", "envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing", "envoy.reloadable_features.preserve_downstream_scheme", "envoy.reloadable_features.remove_forked_chromium_url", diff --git a/source/server/BUILD b/source/server/BUILD index dc5d9ad5a8fb6..fe398a54f2429 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -86,6 +86,7 @@ envoy_cc_library( "//envoy/network:filter_interface", "//envoy/network:listen_socket_interface", "//envoy/network:listener_interface", + "//envoy/runtime:runtime_interface", "//envoy/server:listener_manager_interface", "//envoy/stats:timespan_interface", "//source/common/common:linked_object", diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index e13ca96dbffff..8b2d30097b9e9 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -8,6 +8,7 @@ #include "source/common/event/deferred_task.h" #include "source/common/network/utility.h" #include "source/server/active_tcp_listener.h" +#include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Server { @@ -191,17 +192,32 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta // Otherwise, we need to look for the wild card match, i.e., 0.0.0.0:[address_port]. // We do not return stopped listeners. // TODO(wattli): consolidate with previous search for more efficiency. + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.listener_wildcard_match_ip_family")) { listener_it = - std::find_if(listeners_.begin(), listeners_.end(), - [&address](const std::pair& p) { - return absl::holds_alternative>( - p.second.typed_listener_) && - p.second.listener_->listener() != nullptr && - p.first->type() == Network::Address::Type::Ip && - p.first->ip()->port() == address.ip()->port() && - p.first->ip()->isAnyAddress(); - }); + std::find_if(listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return absl::holds_alternative>( + p.second.typed_listener_) && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && + p.first->ip()->port() == address.ip()->port() && + p.first->ip()->isAnyAddress() && + ((p.first->ip()->ipv4() != nullptr && address.ip()->ipv4() != nullptr) || + (p.first->ip()->ipv6() != nullptr && address.ip()->ipv6() != nullptr)); + }); + } else { + std::find_if(listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return absl::holds_alternative>( + p.second.typed_listener_) && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && + p.first->ip()->port() == address.ip()->port() && + p.first->ip()->isAnyAddress(); + }); + } return (listener_it != listeners_.end()) ? Network::BalancedConnectionHandlerOptRef( ActiveTcpListenerOptRef(absl::get>( From 1d7bf67bcf17e79b82f9f0ef0d5479536601c65d Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Wed, 9 Jun 2021 18:16:13 -0600 Subject: [PATCH 02/12] Fix formatting Signed-off-by: Jacob Delgado --- docs/root/version_history/current.rst | 2 +- source/common/runtime/runtime_features.cc | 2 +- source/server/connection_handler_impl.cc | 50 +++++++++++------------ 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 07e0078e0cf31..b5eb7b818661a 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -33,10 +33,10 @@ Minor Behavior Changes * http: serve HEAD requests from cache. * http: the behavior of the *present_match* in route header matcher changed. The value of *present_match* is ignored in the past. The new behavior is *present_match* performed when value is true. absent match performed when the value is false. Please reference :ref:`present_match `. +* listener: added an option when balancing across active listeners and wildcard matching is used to return the listener that matches the IP family type associated with the listener's socket address. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.listener_wildcard_match_ip_family`` to false. * listener: respect the :ref:`connection balance config ` defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior. * tcp: switched to the new connection pool by default. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.new_tcp_connection_pool`` to false. -* listener: added an option when balancing across active listeners and wildcard matching is used to return the listener that matches the IP family type associated with the listener's socket address. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.listener_wildcard_match_ip_family`` to false. Bug Fixes --------- diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 8af87c3cfcc05..0d170d2da9111 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -75,8 +75,8 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.http2_skip_encoding_empty_trailers", "envoy.reloadable_features.improved_stream_limit_handling", "envoy.reloadable_features.internal_redirects_with_body", - "envoy.reloadable_features.new_tcp_connection_pool", "envoy.reloadable_features.listener_wildcard_match_ip_family", + "envoy.reloadable_features.new_tcp_connection_pool", "envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing", "envoy.reloadable_features.preserve_downstream_scheme", "envoy.reloadable_features.remove_forked_chromium_url", diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 8b2d30097b9e9..0c46922c3f37b 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -7,8 +7,8 @@ #include "source/common/event/deferred_task.h" #include "source/common/network/utility.h" -#include "source/server/active_tcp_listener.h" #include "source/common/runtime/runtime_features.h" +#include "source/server/active_tcp_listener.h" namespace Envoy { namespace Server { @@ -192,31 +192,31 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta // Otherwise, we need to look for the wild card match, i.e., 0.0.0.0:[address_port]. // We do not return stopped listeners. // TODO(wattli): consolidate with previous search for more efficiency. - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.listener_wildcard_match_ip_family")) { - listener_it = - std::find_if(listeners_.begin(), listeners_.end(), - [&address](const std::pair& p) { - return absl::holds_alternative>( - p.second.typed_listener_) && - p.second.listener_->listener() != nullptr && - p.first->type() == Network::Address::Type::Ip && - p.first->ip()->port() == address.ip()->port() && - p.first->ip()->isAnyAddress() && - ((p.first->ip()->ipv4() != nullptr && address.ip()->ipv4() != nullptr) || - (p.first->ip()->ipv6() != nullptr && address.ip()->ipv6() != nullptr)); - }); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.listener_wildcard_match_ip_family")) { + listener_it = std::find_if( + listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return absl::holds_alternative>( + p.second.typed_listener_) && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && + p.first->ip()->port() == address.ip()->port() && p.first->ip()->isAnyAddress() && + ((p.first->ip()->ipv4() != nullptr && address.ip()->ipv4() != nullptr) || + (p.first->ip()->ipv6() != nullptr && address.ip()->ipv6() != nullptr)); + }); } else { - std::find_if(listeners_.begin(), listeners_.end(), - [&address](const std::pair& p) { - return absl::holds_alternative>( - p.second.typed_listener_) && - p.second.listener_->listener() != nullptr && - p.first->type() == Network::Address::Type::Ip && - p.first->ip()->port() == address.ip()->port() && - p.first->ip()->isAnyAddress(); - }); + std::find_if(listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return absl::holds_alternative>( + p.second.typed_listener_) && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && + p.first->ip()->port() == address.ip()->port() && + p.first->ip()->isAnyAddress(); + }); } return (listener_it != listeners_.end()) ? Network::BalancedConnectionHandlerOptRef( From 9e22bbd5d570e168cf3f4c80d8a6c0ed6df8fc9d Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Wed, 9 Jun 2021 20:04:42 -0600 Subject: [PATCH 03/12] capture result of std::find_if Signed-off-by: Jacob Delgado --- source/server/connection_handler_impl.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 0c46922c3f37b..738647213cb53 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -207,16 +207,17 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta (p.first->ip()->ipv6() != nullptr && address.ip()->ipv6() != nullptr)); }); } else { - std::find_if(listeners_.begin(), listeners_.end(), - [&address](const std::pair& p) { - return absl::holds_alternative>( - p.second.typed_listener_) && - p.second.listener_->listener() != nullptr && - p.first->type() == Network::Address::Type::Ip && - p.first->ip()->port() == address.ip()->port() && - p.first->ip()->isAnyAddress(); - }); + listener_it = + std::find_if(listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return absl::holds_alternative>( + p.second.typed_listener_) && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && + p.first->ip()->port() == address.ip()->port() && + p.first->ip()->isAnyAddress(); + }); } return (listener_it != listeners_.end()) ? Network::BalancedConnectionHandlerOptRef( From 6f5436ad06509a460146607e890f0fbff0299dd4 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Thu, 10 Jun 2021 14:58:31 -0600 Subject: [PATCH 04/12] Code review comments Signed-off-by: Jacob Delgado --- source/server/connection_handler_impl.cc | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 738647213cb53..14961a5c18747 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -194,18 +194,18 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta // TODO(wattli): consolidate with previous search for more efficiency. if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.listener_wildcard_match_ip_family")) { - listener_it = std::find_if( - listeners_.begin(), listeners_.end(), - [&address](const std::pair& p) { - return absl::holds_alternative>( - p.second.typed_listener_) && - p.second.listener_->listener() != nullptr && - p.first->type() == Network::Address::Type::Ip && - p.first->ip()->port() == address.ip()->port() && p.first->ip()->isAnyAddress() && - ((p.first->ip()->ipv4() != nullptr && address.ip()->ipv4() != nullptr) || - (p.first->ip()->ipv6() != nullptr && address.ip()->ipv6() != nullptr)); - }); + listener_it = + std::find_if(listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return absl::holds_alternative>( + p.second.typed_listener_) && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && + p.first->ip()->port() == address.ip()->port() && + p.first->ip()->isAnyAddress() && + p.first->ip()->version() == address.ip()->version(); + }); } else { listener_it = std::find_if(listeners_.begin(), listeners_.end(), From 693624f4d33e33cd1f1be40bc666e48e8ad84874 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Mon, 14 Jun 2021 07:11:31 +0000 Subject: [PATCH 05/12] Add a new environment variable ...to support dual stack listeners when running integration tests. When ENVOY_IP_FAMILY_TEST is set to true then listeners are not changed when usting 0.0.0.0 or :: to be of the IP type that is under test. Signed-off-by: Jacob Delgado --- test/config/utility.cc | 11 +++++++---- test/test_common/environment.cc | 11 +++++++++++ test/test_common/environment.h | 8 ++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/test/config/utility.cc b/test/config/utility.cc index 6357327f5b171..d7373084b7fb8 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -601,10 +601,13 @@ ConfigHelper::ConfigHelper(const Network::Address::IpVersion version, Api::Api& for (int i = 0; i < static_resources->listeners_size(); ++i) { auto* listener = static_resources->mutable_listeners(i); auto* listener_socket_addr = listener->mutable_address()->mutable_socket_address(); - if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { - listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); - } else { - listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); + + if (!TestEnvironment::allowListenersOnBothIPFamilyTypes()) { + if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { + listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); + } else { + listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); + } } } diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index d2d33a73c4e71..a2e7bd3fdb9aa 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -230,6 +230,17 @@ bool TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion type return true; } +bool TestEnvironment::allowListenersOnBothIPFamilyTypes() { + const char* value = std::getenv("ENVOY_IP_FAMILY_TEST"); + std::string option(value ? value : ""); + + if (option == "true") { + return true; + } + + return false; +} + std::vector TestEnvironment::getIpVersionsForTest() { std::vector parameters; for (auto version : {Network::Address::IpVersion::v4, Network::Address::IpVersion::v6}) { diff --git a/test/test_common/environment.h b/test/test_common/environment.h index 0aa1cdd632d74..6c5b5aee6f3c8 100644 --- a/test/test_common/environment.h +++ b/test/test_common/environment.h @@ -52,6 +52,14 @@ class TestEnvironment { */ static bool shouldRunTestForIpVersion(Network::Address::IpVersion type); + /** + * Check whether listeners should be altered to conform to the existing + * IP family type for the specific test + * @return bool if listeners are allowed to be specified in different types from the current + * version. + */ + static bool allowListenersOnBothIPFamilyTypes(); + /** * Return a vector of IP address parameters to test. Tests can be run with * only IPv4 addressing or only IPv6 addressing by setting the environment From 3a0f8fde4e6e6bdbfff7bea533e2321d3214b002 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Mon, 14 Jun 2021 07:11:31 +0000 Subject: [PATCH 06/12] Add a new environment variable ...to support dual stack listeners when running integration tests. When ENVOY_IP_FAMILY_TEST is set to true then listeners are not changed when usting 0.0.0.0 or :: to be of the IP type that is under test. Signed-off-by: Jacob Delgado --- test/config/utility.cc | 11 +++++++---- test/test_common/environment.cc | 11 +++++++++++ test/test_common/environment.h | 8 ++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/test/config/utility.cc b/test/config/utility.cc index 6357327f5b171..d7373084b7fb8 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -601,10 +601,13 @@ ConfigHelper::ConfigHelper(const Network::Address::IpVersion version, Api::Api& for (int i = 0; i < static_resources->listeners_size(); ++i) { auto* listener = static_resources->mutable_listeners(i); auto* listener_socket_addr = listener->mutable_address()->mutable_socket_address(); - if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { - listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); - } else { - listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); + + if (!TestEnvironment::allowListenersOnBothIPFamilyTypes()) { + if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { + listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); + } else { + listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); + } } } diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index d2d33a73c4e71..a2e7bd3fdb9aa 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -230,6 +230,17 @@ bool TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion type return true; } +bool TestEnvironment::allowListenersOnBothIPFamilyTypes() { + const char* value = std::getenv("ENVOY_IP_FAMILY_TEST"); + std::string option(value ? value : ""); + + if (option == "true") { + return true; + } + + return false; +} + std::vector TestEnvironment::getIpVersionsForTest() { std::vector parameters; for (auto version : {Network::Address::IpVersion::v4, Network::Address::IpVersion::v6}) { diff --git a/test/test_common/environment.h b/test/test_common/environment.h index 0aa1cdd632d74..6c5b5aee6f3c8 100644 --- a/test/test_common/environment.h +++ b/test/test_common/environment.h @@ -52,6 +52,14 @@ class TestEnvironment { */ static bool shouldRunTestForIpVersion(Network::Address::IpVersion type); + /** + * Check whether listeners should be altered to conform to the existing + * IP family type for the specific test + * @return bool if listeners are allowed to be specified in different types from the current + * version. + */ + static bool allowListenersOnBothIPFamilyTypes(); + /** * Return a vector of IP address parameters to test. Tests can be run with * only IPv4 addressing or only IPv6 addressing by setting the environment From 93d9ce4adcd8e8302807041735c4233c80c7851f Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Wed, 23 Jun 2021 20:06:39 +0000 Subject: [PATCH 07/12] wip: add integration test for rebalancer Signed-off-by: Jacob Delgado --- source/server/connection_handler_impl.cc | 7 + .../address_restore_listener_filter.cc | 11 +- .../listener_lds_integration_test.cc | 157 +++++++++++++++++- 3 files changed, 170 insertions(+), 5 deletions(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 14961a5c18747..8686534753990 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -174,6 +174,7 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta // This is a linear operation, may need to add a map to improve performance. // However, linear performance might be adequate since the number of listeners is small. // We do not return stopped listeners. + ENVOY_LOG(debug, "getBalancedHandlerByAddress"); auto listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](std::pairsecond.tcpListener().value().get()); } + ENVOY_LOG(debug, "getBalancedHandlerByAddress: no match found"); + + + ENVOY_LOG(debug, "wildcard matching"); // Otherwise, we need to look for the wild card match, i.e., 0.0.0.0:[address_port]. // We do not return stopped listeners. // TODO(wattli): consolidate with previous search for more efficiency. if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.listener_wildcard_match_ip_family")) { + ENVOY_LOG(debug, "wildcard matching: RUNTIME enabled"); listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](const std::pairip()->version() == address.ip()->version(); }); } else { + ENVOY_LOG(debug, "wildcard matching: RUNTIME disabled"); listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](const std::pair("127.0.0.2", 80)); + + // if (cb.socket().ipVersion().value() == Network::Address::IpVersion::v6) { + // socket.addressProvider().restoreLocalAddress( + // std::make_shared("::2", 8001)); + + // } else { + socket.addressProvider().restoreLocalAddress( + std::make_shared("127.0.0.2", 80)); + // } FANCY_LOG(debug, "current local socket address is {} restored = {}", socket.addressProvider().localAddress()->asString(), socket.addressProvider().localAddressRestored()); diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index 498f9f2a22bba..98d7077809270 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/test/integration/listener_lds_integration_test.cc @@ -619,11 +619,11 @@ struct PerConnection { // listener. // Currently flaky because the virtual listener create listen socket anyway despite the socket is // never used. Will enable this test once https://github.com/envoyproxy/envoy/pull/16259 is merged. -TEST_P(RebalancerTest, DISABLED_RedirectConnectionIsBalancedOnDestinationListener) { +TEST_P(RebalancerTest, RedirectConnectionIsBalancedOnDestinationListener) { auto ip_address_str = Network::Test::getLoopbackAddressUrlString(TestEnvironment::getIpVersionsForTest().front()); - concurrency_ = 2; - int repeats = 10; + concurrency_ = 1; + int repeats = 1; initialize(); // The balancer is balanced as per active connection instead of total connection. @@ -659,5 +659,156 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, RebalancerTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); + +class RebalancerDualStackTest : public testing::TestWithParam, + public BaseIntegrationTest { +public: + RebalancerDualStackTest() + : BaseIntegrationTest(GetParam(), ConfigHelper::baseConfig() + R"EOF( + filter_chains: + - filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + stat_prefix: tcp_stats + cluster: cluster_0 +)EOF") {} + + void initialize() override { + config_helper_.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + // add an ipv6 cluster + auto& src_cluster_config = *bootstrap.mutable_static_resources()->mutable_clusters(0); + auto& cluster_1 = *bootstrap.mutable_static_resources()->add_clusters(); + cluster_1 = src_cluster_config; + cluster_1.set_name("cluster_1"); + cluster_1.mutable_load_assignment()->set_cluster_name("cluster_1"); + cluster_1.mutable_load_assignment()->mutable_endpoints()->mutable_data()[0]->mutable_lb_endpoints()->mutable_data()[0]->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address("::"); + cluster_1.mutable_load_assignment()->mutable_endpoints()->mutable_data()[0]->mutable_lb_endpoints()->mutable_data()[0]->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(0); + + auto& src_listener_config = *bootstrap.mutable_static_resources()->mutable_listeners(0); + src_listener_config.mutable_use_original_dst()->set_value(true); + src_listener_config.mutable_address()->mutable_socket_address()->set_port_value(8000); + src_listener_config.mutable_address()->mutable_socket_address()->set_address("0.0.0.0"); + + // // Note that the below original_dst is replaced by FakeOriginalDstListenerFilter at the + // // link time. + src_listener_config.add_listener_filters()->set_name( + "envoy.filters.listener.original_dst"); + auto& listener_1 = *bootstrap.mutable_static_resources()->add_listeners(); + listener_1 = src_listener_config; + listener_1.set_name("listener_1"); + listener_1.mutable_address()->mutable_socket_address()->set_address("::"); + listener_1.mutable_address()->mutable_socket_address()->set_port_value(8000); + + auto& virtual_listener_0 = *bootstrap.mutable_static_resources()->add_listeners(); + virtual_listener_0 = src_listener_config; + virtual_listener_0.mutable_use_original_dst()->set_value(false); + virtual_listener_0.clear_listener_filters(); + virtual_listener_0.mutable_bind_to_port()->set_value(false); + virtual_listener_0.set_name("virtual_listener_ipv4"); + virtual_listener_0.mutable_connection_balance_config()->mutable_exact_balance(); + virtual_listener_0.mutable_address()->mutable_socket_address()->set_address("127.0.0.2"); + virtual_listener_0.mutable_address()->mutable_socket_address()->set_port_value(8001); + + auto& virtual_listener_1 = *bootstrap.mutable_static_resources()->add_listeners(); + virtual_listener_1 = src_listener_config; + virtual_listener_1.mutable_use_original_dst()->set_value(false); + virtual_listener_1.clear_listener_filters(); + virtual_listener_1.mutable_bind_to_port()->set_value(false); + virtual_listener_1.set_name("virtual_listener_ipv6"); + virtual_listener_1.mutable_connection_balance_config()->mutable_exact_balance(); + virtual_listener_1.mutable_address()->mutable_socket_address()->set_address("::2"); + virtual_listener_1.mutable_address()->mutable_socket_address()->set_port_value(8001); + + }); + BaseIntegrationTest::initialize(); + } + + void createUpstreams() override { + setUpstreamCount(2); + Network::TransportSocketFactoryPtr factory = Network::Test::createRawBufferSocketFactory(); + auto ipv4_endpoint = Network::Utility::parseInternetAddress( + Network::Test::getLoopbackAddressString(Network::Address::IpVersion::v4), 0); + fake_upstreams_.emplace_back( + new FakeUpstream(std::move(factory), ipv4_endpoint, upstreamConfig())); + + auto ipv6_endpoint = Network::Utility::parseInternetAddress( + Network::Test::getLoopbackAddressString(Network::Address::IpVersion::v6), 0); + fake_upstreams_.emplace_back( + new FakeUpstream(std::move(factory), ipv6_endpoint, upstreamConfig())); + } + + std::unique_ptr createConnectionAndWrite(Network::Address::IpVersion version, + const std::string& request, + std::string& response) { + + ENVOY_LOG(debug, "createConnectionAddr"); + Buffer::OwnedImpl buffer(request); + return std::make_unique( + 8000, buffer, + [&response](Network::ClientConnection&, const Buffer::Instance& data) -> void { + response.append(data.toString()); + }, + version, *dispatcher_); + } +}; + +// Verify the connections are distributed evenly on the 2 worker threads of the redirected +// listener. +// Currently flaky because the virtual listener create listen socket anyway despite the socket is +// never used. Will enable this test once https://github.com/envoyproxy/envoy/pull/16259 is merged. +TEST_P(RebalancerDualStackTest, ChoosesProperListener) { + + FANCY_LOG(debug, "starting"); + // ipv4 and ipv6 are required for this test to run + auto ipVersions = TestEnvironment::getIpVersionsForTest(); + if (ipVersions.size() != 2) { + FANCY_LOG(debug, "wtf"); + return; + } + TestEnvironment::setEnvVar("ENVOY_IP_FAMILY_TEST", "true", 0); + + FANCY_LOG(debug, "continuing"); + + auto ip_address_str = + Network::Test::getLoopbackAddressUrlString(TestEnvironment::getIpVersionsForTest().front()); + initialize(); + + std::vector connections; + connections.emplace_back(); + connections.back().client_conn_ = + createConnectionAndWrite(Network::Address::IpVersion::v4, "dummy", connections.back().response_); + connections.back().client_conn_->waitForConnection(); + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(connections.back().upstream_conn_)); + + // connections.back().client_conn_ = + // createConnectionAndWrite(Network::Address::IpVersion::v6, "dummy", connections.back().response_); + // connections.back().client_conn_->waitForConnection(); + // ASSERT_TRUE(fake_upstreams_[1]->waitForRawConnection(connections.back().upstream_conn_)); + + for (auto& conn : connections) { + conn.client_conn_->close(); + while (!conn.client_conn_->closed()) { + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + } + } + + // ASSERT_EQ(TestUtility::findCounter( + // test_server_->statStore(), + // absl::StrCat("listener.[::1]_80000.worker_0.downstream_cx_total")) + // ->value(), + // 2); + ASSERT_EQ(TestUtility::findCounter( + test_server_->statStore(), + absl::StrCat("listener.127.0.0.1_80.worker_0.downstream_cx_total")) + ->value(), + 3); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, RebalancerDualStackTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + } // namespace } // namespace Envoy From 8f29ecc5ca9149a63b90c4556a2b64172a6178eb Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Wed, 23 Jun 2021 20:07:29 +0000 Subject: [PATCH 08/12] Manually revert to code at 6f5436ad06509a460146607e890f0fbff0299dd4 Signed-off-by: Jacob Delgado --- source/server/connection_handler_impl.cc | 7 - test/config/utility.cc | 11 +- .../address_restore_listener_filter.cc | 11 +- .../listener_lds_integration_test.cc | 157 +----------------- test/test_common/environment.cc | 11 -- test/test_common/environment.h | 8 - 6 files changed, 9 insertions(+), 196 deletions(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 8686534753990..14961a5c18747 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -174,7 +174,6 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta // This is a linear operation, may need to add a map to improve performance. // However, linear performance might be adequate since the number of listeners is small. // We do not return stopped listeners. - ENVOY_LOG(debug, "getBalancedHandlerByAddress"); auto listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](std::pairsecond.tcpListener().value().get()); } - ENVOY_LOG(debug, "getBalancedHandlerByAddress: no match found"); - - - ENVOY_LOG(debug, "wildcard matching"); // Otherwise, we need to look for the wild card match, i.e., 0.0.0.0:[address_port]. // We do not return stopped listeners. // TODO(wattli): consolidate with previous search for more efficiency. if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.listener_wildcard_match_ip_family")) { - ENVOY_LOG(debug, "wildcard matching: RUNTIME enabled"); listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](const std::pairip()->version() == address.ip()->version(); }); } else { - ENVOY_LOG(debug, "wildcard matching: RUNTIME disabled"); listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](const std::pairlisteners_size(); ++i) { auto* listener = static_resources->mutable_listeners(i); auto* listener_socket_addr = listener->mutable_address()->mutable_socket_address(); - - if (!TestEnvironment::allowListenersOnBothIPFamilyTypes()) { - if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { - listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); - } else { - listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); - } + if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { + listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); + } else { + listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); } } diff --git a/test/integration/filters/address_restore_listener_filter.cc b/test/integration/filters/address_restore_listener_filter.cc index 85c4db646a36a..f0d6945019760 100644 --- a/test/integration/filters/address_restore_listener_filter.cc +++ b/test/integration/filters/address_restore_listener_filter.cc @@ -16,15 +16,8 @@ class FakeOriginalDstListenerFilter : public Network::ListenerFilter { Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { FANCY_LOG(debug, "in FakeOriginalDstListenerFilter::onAccept"); Network::ConnectionSocket& socket = cb.socket(); - - // if (cb.socket().ipVersion().value() == Network::Address::IpVersion::v6) { - // socket.addressProvider().restoreLocalAddress( - // std::make_shared("::2", 8001)); - - // } else { - socket.addressProvider().restoreLocalAddress( - std::make_shared("127.0.0.2", 80)); - // } + socket.addressProvider().restoreLocalAddress( + std::make_shared("127.0.0.2", 80)); FANCY_LOG(debug, "current local socket address is {} restored = {}", socket.addressProvider().localAddress()->asString(), socket.addressProvider().localAddressRestored()); diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index 98d7077809270..498f9f2a22bba 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/test/integration/listener_lds_integration_test.cc @@ -619,11 +619,11 @@ struct PerConnection { // listener. // Currently flaky because the virtual listener create listen socket anyway despite the socket is // never used. Will enable this test once https://github.com/envoyproxy/envoy/pull/16259 is merged. -TEST_P(RebalancerTest, RedirectConnectionIsBalancedOnDestinationListener) { +TEST_P(RebalancerTest, DISABLED_RedirectConnectionIsBalancedOnDestinationListener) { auto ip_address_str = Network::Test::getLoopbackAddressUrlString(TestEnvironment::getIpVersionsForTest().front()); - concurrency_ = 1; - int repeats = 1; + concurrency_ = 2; + int repeats = 10; initialize(); // The balancer is balanced as per active connection instead of total connection. @@ -659,156 +659,5 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, RebalancerTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); - -class RebalancerDualStackTest : public testing::TestWithParam, - public BaseIntegrationTest { -public: - RebalancerDualStackTest() - : BaseIntegrationTest(GetParam(), ConfigHelper::baseConfig() + R"EOF( - filter_chains: - - filters: - - name: envoy.filters.network.tcp_proxy - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy - stat_prefix: tcp_stats - cluster: cluster_0 -)EOF") {} - - void initialize() override { - config_helper_.addConfigModifier( - [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { - // add an ipv6 cluster - auto& src_cluster_config = *bootstrap.mutable_static_resources()->mutable_clusters(0); - auto& cluster_1 = *bootstrap.mutable_static_resources()->add_clusters(); - cluster_1 = src_cluster_config; - cluster_1.set_name("cluster_1"); - cluster_1.mutable_load_assignment()->set_cluster_name("cluster_1"); - cluster_1.mutable_load_assignment()->mutable_endpoints()->mutable_data()[0]->mutable_lb_endpoints()->mutable_data()[0]->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address("::"); - cluster_1.mutable_load_assignment()->mutable_endpoints()->mutable_data()[0]->mutable_lb_endpoints()->mutable_data()[0]->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(0); - - auto& src_listener_config = *bootstrap.mutable_static_resources()->mutable_listeners(0); - src_listener_config.mutable_use_original_dst()->set_value(true); - src_listener_config.mutable_address()->mutable_socket_address()->set_port_value(8000); - src_listener_config.mutable_address()->mutable_socket_address()->set_address("0.0.0.0"); - - // // Note that the below original_dst is replaced by FakeOriginalDstListenerFilter at the - // // link time. - src_listener_config.add_listener_filters()->set_name( - "envoy.filters.listener.original_dst"); - auto& listener_1 = *bootstrap.mutable_static_resources()->add_listeners(); - listener_1 = src_listener_config; - listener_1.set_name("listener_1"); - listener_1.mutable_address()->mutable_socket_address()->set_address("::"); - listener_1.mutable_address()->mutable_socket_address()->set_port_value(8000); - - auto& virtual_listener_0 = *bootstrap.mutable_static_resources()->add_listeners(); - virtual_listener_0 = src_listener_config; - virtual_listener_0.mutable_use_original_dst()->set_value(false); - virtual_listener_0.clear_listener_filters(); - virtual_listener_0.mutable_bind_to_port()->set_value(false); - virtual_listener_0.set_name("virtual_listener_ipv4"); - virtual_listener_0.mutable_connection_balance_config()->mutable_exact_balance(); - virtual_listener_0.mutable_address()->mutable_socket_address()->set_address("127.0.0.2"); - virtual_listener_0.mutable_address()->mutable_socket_address()->set_port_value(8001); - - auto& virtual_listener_1 = *bootstrap.mutable_static_resources()->add_listeners(); - virtual_listener_1 = src_listener_config; - virtual_listener_1.mutable_use_original_dst()->set_value(false); - virtual_listener_1.clear_listener_filters(); - virtual_listener_1.mutable_bind_to_port()->set_value(false); - virtual_listener_1.set_name("virtual_listener_ipv6"); - virtual_listener_1.mutable_connection_balance_config()->mutable_exact_balance(); - virtual_listener_1.mutable_address()->mutable_socket_address()->set_address("::2"); - virtual_listener_1.mutable_address()->mutable_socket_address()->set_port_value(8001); - - }); - BaseIntegrationTest::initialize(); - } - - void createUpstreams() override { - setUpstreamCount(2); - Network::TransportSocketFactoryPtr factory = Network::Test::createRawBufferSocketFactory(); - auto ipv4_endpoint = Network::Utility::parseInternetAddress( - Network::Test::getLoopbackAddressString(Network::Address::IpVersion::v4), 0); - fake_upstreams_.emplace_back( - new FakeUpstream(std::move(factory), ipv4_endpoint, upstreamConfig())); - - auto ipv6_endpoint = Network::Utility::parseInternetAddress( - Network::Test::getLoopbackAddressString(Network::Address::IpVersion::v6), 0); - fake_upstreams_.emplace_back( - new FakeUpstream(std::move(factory), ipv6_endpoint, upstreamConfig())); - } - - std::unique_ptr createConnectionAndWrite(Network::Address::IpVersion version, - const std::string& request, - std::string& response) { - - ENVOY_LOG(debug, "createConnectionAddr"); - Buffer::OwnedImpl buffer(request); - return std::make_unique( - 8000, buffer, - [&response](Network::ClientConnection&, const Buffer::Instance& data) -> void { - response.append(data.toString()); - }, - version, *dispatcher_); - } -}; - -// Verify the connections are distributed evenly on the 2 worker threads of the redirected -// listener. -// Currently flaky because the virtual listener create listen socket anyway despite the socket is -// never used. Will enable this test once https://github.com/envoyproxy/envoy/pull/16259 is merged. -TEST_P(RebalancerDualStackTest, ChoosesProperListener) { - - FANCY_LOG(debug, "starting"); - // ipv4 and ipv6 are required for this test to run - auto ipVersions = TestEnvironment::getIpVersionsForTest(); - if (ipVersions.size() != 2) { - FANCY_LOG(debug, "wtf"); - return; - } - TestEnvironment::setEnvVar("ENVOY_IP_FAMILY_TEST", "true", 0); - - FANCY_LOG(debug, "continuing"); - - auto ip_address_str = - Network::Test::getLoopbackAddressUrlString(TestEnvironment::getIpVersionsForTest().front()); - initialize(); - - std::vector connections; - connections.emplace_back(); - connections.back().client_conn_ = - createConnectionAndWrite(Network::Address::IpVersion::v4, "dummy", connections.back().response_); - connections.back().client_conn_->waitForConnection(); - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(connections.back().upstream_conn_)); - - // connections.back().client_conn_ = - // createConnectionAndWrite(Network::Address::IpVersion::v6, "dummy", connections.back().response_); - // connections.back().client_conn_->waitForConnection(); - // ASSERT_TRUE(fake_upstreams_[1]->waitForRawConnection(connections.back().upstream_conn_)); - - for (auto& conn : connections) { - conn.client_conn_->close(); - while (!conn.client_conn_->closed()) { - dispatcher_->run(Event::Dispatcher::RunType::NonBlock); - } - } - - // ASSERT_EQ(TestUtility::findCounter( - // test_server_->statStore(), - // absl::StrCat("listener.[::1]_80000.worker_0.downstream_cx_total")) - // ->value(), - // 2); - ASSERT_EQ(TestUtility::findCounter( - test_server_->statStore(), - absl::StrCat("listener.127.0.0.1_80.worker_0.downstream_cx_total")) - ->value(), - 3); -} - -INSTANTIATE_TEST_SUITE_P(IpVersions, RebalancerDualStackTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); - } // namespace } // namespace Envoy diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index a2e7bd3fdb9aa..d2d33a73c4e71 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -230,17 +230,6 @@ bool TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion type return true; } -bool TestEnvironment::allowListenersOnBothIPFamilyTypes() { - const char* value = std::getenv("ENVOY_IP_FAMILY_TEST"); - std::string option(value ? value : ""); - - if (option == "true") { - return true; - } - - return false; -} - std::vector TestEnvironment::getIpVersionsForTest() { std::vector parameters; for (auto version : {Network::Address::IpVersion::v4, Network::Address::IpVersion::v6}) { diff --git a/test/test_common/environment.h b/test/test_common/environment.h index 6c5b5aee6f3c8..0aa1cdd632d74 100644 --- a/test/test_common/environment.h +++ b/test/test_common/environment.h @@ -52,14 +52,6 @@ class TestEnvironment { */ static bool shouldRunTestForIpVersion(Network::Address::IpVersion type); - /** - * Check whether listeners should be altered to conform to the existing - * IP family type for the specific test - * @return bool if listeners are allowed to be specified in different types from the current - * version. - */ - static bool allowListenersOnBothIPFamilyTypes(); - /** * Return a vector of IP address parameters to test. Tests can be run with * only IPv4 addressing or only IPv6 addressing by setting the environment From 8ec1a833810c67ac1c0446a15e294964cdd797a0 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Wed, 23 Jun 2021 23:17:09 -0600 Subject: [PATCH 09/12] Add unit test Signed-off-by: Jacob Delgado --- test/server/BUILD | 1 + test/server/connection_handler_test.cc | 143 +++++++++++++++++++++++++ 2 files changed, 144 insertions(+) diff --git a/test/server/BUILD b/test/server/BUILD index c35ef66fa63b9..39b7a0c776bb3 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -86,6 +86,7 @@ envoy_cc_test( "//test/mocks/api:api_mocks", "//test/mocks/network:network_mocks", "//test/test_common:network_utility_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:threadsafe_singleton_injector_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index b97a3d140c614..6fed143238442 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -21,6 +21,7 @@ #include "test/mocks/common.h" #include "test/mocks/network/mocks.h" #include "test/test_common/network_utility.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "gmock/gmock.h" @@ -746,6 +747,148 @@ TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { EXPECT_CALL(*access_log_, log(_, _, _, _)); } +TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { + auto scoped_runtime = std::make_unique(); + + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.listener_wildcard_match_ip_family", "false"}}); + + Network::TcpListenerCallbacks* listener_callbacks1; + auto listener1 = new NiceMock(); + TestListener* test_listener1 = + addListener(1, true, true, "test_listener1", listener1, &listener_callbacks1); + Network::Address::InstanceConstSharedPtr normal_address( + new Network::Address::Ipv4Instance("127.0.0.1", 10001)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); + handler_->addListener(absl::nullopt, *test_listener1); + + Network::TcpListenerCallbacks* ipv4_any_listener_callbacks; + auto listener2 = new NiceMock(); + TestListener* ipv4_any_listener = + addListener(1, false, false, "ipv4_any_test_listener", listener2, &ipv4_any_listener_callbacks); + Network::Address::InstanceConstSharedPtr any_address( + new Network::Address::Ipv4Instance("0.0.0.0", 80)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); + handler_->addListener(absl::nullopt, *ipv4_any_listener); + + Network::TcpListenerCallbacks* ipv6_any_listener_callbacks; + auto listener3 = new NiceMock(); + TestListener* ipv6_any_listener = + addListener(1, false, false, "ipv6_any_test_listener", listener3, &ipv6_any_listener_callbacks); + Network::Address::InstanceConstSharedPtr any_address_ipv6( + new Network::Address::Ipv6Instance("::", 80)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address_ipv6)); + handler_->addListener(absl::nullopt, *ipv6_any_listener); + + Network::MockListenerFilter* test_filter = new Network::MockListenerFilter(); + EXPECT_CALL(*test_filter, destroy_()); + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + bool redirected = false; + EXPECT_CALL(factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { + // Insert the Mock filter. + if (!redirected) { + manager.addAcceptFilter(listener_filter_matcher_, + Network::ListenerFilterPtr{test_filter}); + redirected = true; + } + return true; + })); + + Network::Address::InstanceConstSharedPtr alt_address( + new Network::Address::Ipv6Instance("::2", 80, nullptr)); + EXPECT_CALL(*test_filter, onAccept(_)) + .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { + cb.socket().addressProvider().restoreLocalAddress(alt_address); + return Network::FilterStatus::Continue; + })); + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + auto* connection = new NiceMock(); + EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); + EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); + listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + EXPECT_EQ(1UL, handler_->numConnections()); + + // Add negative test case check + + EXPECT_CALL(*listener3, onDestroy()); + EXPECT_CALL(*listener2, onDestroy()); + EXPECT_CALL(*listener1, onDestroy()); + EXPECT_CALL(*access_log_, log(_, _, _, _)); +} + +TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { + auto scoped_runtime = std::make_unique(); + + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.listener_wildcard_match_ip_family", "false"}}); + + Network::TcpListenerCallbacks* listener_callbacks1; + auto listener1 = new NiceMock(); + TestListener* test_listener1 = + addListener(1, true, true, "test_listener1", listener1, &listener_callbacks1); + Network::Address::InstanceConstSharedPtr normal_address( + new Network::Address::Ipv4Instance("127.0.0.1", 10001)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); + handler_->addListener(absl::nullopt, *test_listener1); + + Network::TcpListenerCallbacks* ipv4_any_listener_callbacks; + auto listener2 = new NiceMock(); + TestListener* ipv4_any_listener = + addListener(1, false, false, "ipv4_any_test_listener", listener2, &ipv4_any_listener_callbacks); + Network::Address::InstanceConstSharedPtr any_address( + new Network::Address::Ipv4Instance("0.0.0.0", 80)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); + handler_->addListener(absl::nullopt, *ipv4_any_listener); + + Network::TcpListenerCallbacks* ipv6_any_listener_callbacks; + auto listener3 = new NiceMock(); + TestListener* ipv6_any_listener = + addListener(1, false, false, "ipv6_any_test_listener", listener3, &ipv6_any_listener_callbacks); + Network::Address::InstanceConstSharedPtr any_address_ipv6( + new Network::Address::Ipv6Instance("::", 80)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address_ipv6)); + handler_->addListener(absl::nullopt, *ipv6_any_listener); + + Network::MockListenerFilter* test_filter = new Network::MockListenerFilter(); + EXPECT_CALL(*test_filter, destroy_()); + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + bool redirected = false; + EXPECT_CALL(factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { + // Insert the Mock filter. + if (!redirected) { + manager.addAcceptFilter(listener_filter_matcher_, + Network::ListenerFilterPtr{test_filter}); + redirected = true; + } + return true; + })); + + Network::Address::InstanceConstSharedPtr alt_address( + new Network::Address::Ipv6Instance("::2", 80, nullptr)); + EXPECT_CALL(*test_filter, onAccept(_)) + .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { + cb.socket().addressProvider().restoreLocalAddress(alt_address); + return Network::FilterStatus::Continue; + })); + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + auto* connection = new NiceMock(); + EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); + EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); + listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + EXPECT_EQ(1UL, handler_->numConnections()); + + // metrics aren't sufficient for testing; need a better way to identify the socket + EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); + EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); + + EXPECT_CALL(*listener3, onDestroy()); + EXPECT_CALL(*listener2, onDestroy()); + EXPECT_CALL(*listener1, onDestroy()); + EXPECT_CALL(*access_log_, log(_, _, _, _)); +} + TEST_F(ConnectionHandlerTest, WildcardListenerWithOriginalDstInbound) { Network::TcpListenerCallbacks* listener_callbacks1; auto listener1 = new NiceMock(); From ee81915b50623498c82e88a0df1dd5e6870d1348 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Wed, 23 Jun 2021 23:29:13 -0600 Subject: [PATCH 10/12] Remove integration framework modifications Signed-off-by: Jacob Delgado --- test/config/utility.cc | 11 ++++------- test/server/connection_handler_test.cc | 20 ++++++++++---------- test/test_common/environment.cc | 11 ----------- test/test_common/environment.h | 8 -------- 4 files changed, 14 insertions(+), 36 deletions(-) diff --git a/test/config/utility.cc b/test/config/utility.cc index d7373084b7fb8..6357327f5b171 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -601,13 +601,10 @@ ConfigHelper::ConfigHelper(const Network::Address::IpVersion version, Api::Api& for (int i = 0; i < static_resources->listeners_size(); ++i) { auto* listener = static_resources->mutable_listeners(i); auto* listener_socket_addr = listener->mutable_address()->mutable_socket_address(); - - if (!TestEnvironment::allowListenersOnBothIPFamilyTypes()) { - if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { - listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); - } else { - listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); - } + if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { + listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); + } else { + listener_socket_addr->set_address(Network::Test::getLoopbackAddressString(version)); } } diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 6fed143238442..0ed97ae223fae 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -748,7 +748,7 @@ TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { } TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { - auto scoped_runtime = std::make_unique(); + auto scoped_runtime = std::make_unique(); Runtime::LoaderSingleton::getExisting()->mergeValues( {{"envoy.reloadable_features.listener_wildcard_match_ip_family", "false"}}); @@ -764,8 +764,8 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { Network::TcpListenerCallbacks* ipv4_any_listener_callbacks; auto listener2 = new NiceMock(); - TestListener* ipv4_any_listener = - addListener(1, false, false, "ipv4_any_test_listener", listener2, &ipv4_any_listener_callbacks); + TestListener* ipv4_any_listener = addListener(1, false, false, "ipv4_any_test_listener", + listener2, &ipv4_any_listener_callbacks); Network::Address::InstanceConstSharedPtr any_address( new Network::Address::Ipv4Instance("0.0.0.0", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); @@ -773,8 +773,8 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { Network::TcpListenerCallbacks* ipv6_any_listener_callbacks; auto listener3 = new NiceMock(); - TestListener* ipv6_any_listener = - addListener(1, false, false, "ipv6_any_test_listener", listener3, &ipv6_any_listener_callbacks); + TestListener* ipv6_any_listener = addListener(1, false, false, "ipv6_any_test_listener", + listener3, &ipv6_any_listener_callbacks); Network::Address::InstanceConstSharedPtr any_address_ipv6( new Network::Address::Ipv6Instance("::", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address_ipv6)); @@ -818,7 +818,7 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { } TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { - auto scoped_runtime = std::make_unique(); + auto scoped_runtime = std::make_unique(); Runtime::LoaderSingleton::getExisting()->mergeValues( {{"envoy.reloadable_features.listener_wildcard_match_ip_family", "false"}}); @@ -834,8 +834,8 @@ TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { Network::TcpListenerCallbacks* ipv4_any_listener_callbacks; auto listener2 = new NiceMock(); - TestListener* ipv4_any_listener = - addListener(1, false, false, "ipv4_any_test_listener", listener2, &ipv4_any_listener_callbacks); + TestListener* ipv4_any_listener = addListener(1, false, false, "ipv4_any_test_listener", + listener2, &ipv4_any_listener_callbacks); Network::Address::InstanceConstSharedPtr any_address( new Network::Address::Ipv4Instance("0.0.0.0", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); @@ -843,8 +843,8 @@ TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { Network::TcpListenerCallbacks* ipv6_any_listener_callbacks; auto listener3 = new NiceMock(); - TestListener* ipv6_any_listener = - addListener(1, false, false, "ipv6_any_test_listener", listener3, &ipv6_any_listener_callbacks); + TestListener* ipv6_any_listener = addListener(1, false, false, "ipv6_any_test_listener", + listener3, &ipv6_any_listener_callbacks); Network::Address::InstanceConstSharedPtr any_address_ipv6( new Network::Address::Ipv6Instance("::", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address_ipv6)); diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index a2e7bd3fdb9aa..d2d33a73c4e71 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -230,17 +230,6 @@ bool TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion type return true; } -bool TestEnvironment::allowListenersOnBothIPFamilyTypes() { - const char* value = std::getenv("ENVOY_IP_FAMILY_TEST"); - std::string option(value ? value : ""); - - if (option == "true") { - return true; - } - - return false; -} - std::vector TestEnvironment::getIpVersionsForTest() { std::vector parameters; for (auto version : {Network::Address::IpVersion::v4, Network::Address::IpVersion::v6}) { diff --git a/test/test_common/environment.h b/test/test_common/environment.h index 6c5b5aee6f3c8..0aa1cdd632d74 100644 --- a/test/test_common/environment.h +++ b/test/test_common/environment.h @@ -52,14 +52,6 @@ class TestEnvironment { */ static bool shouldRunTestForIpVersion(Network::Address::IpVersion type); - /** - * Check whether listeners should be altered to conform to the existing - * IP family type for the specific test - * @return bool if listeners are allowed to be specified in different types from the current - * version. - */ - static bool allowListenersOnBothIPFamilyTypes(); - /** * Return a vector of IP address parameters to test. Tests can be run with * only IPv4 addressing or only IPv6 addressing by setting the environment From f834f736aa094e5d85db5358955603886deb11f2 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Tue, 6 Jul 2021 22:05:02 -0600 Subject: [PATCH 11/12] Fix unit test Signed-off-by: Jacob Delgado --- test/server/connection_handler_test.cc | 50 +++++++++++++++++--------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 0ed97ae223fae..61064640d2c62 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -762,19 +762,27 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); handler_->addListener(absl::nullopt, *test_listener1); + auto ipv4_overridden_filter_chain_manager = + std::make_shared>(); Network::TcpListenerCallbacks* ipv4_any_listener_callbacks; auto listener2 = new NiceMock(); - TestListener* ipv4_any_listener = addListener(1, false, false, "ipv4_any_test_listener", - listener2, &ipv4_any_listener_callbacks); + TestListener* ipv4_any_listener = + addListener(1, false, false, "ipv4_any_test_listener", listener2, + &ipv4_any_listener_callbacks, nullptr, nullptr, Network::Socket::Type::Stream, + std::chrono::milliseconds(15000), false, ipv4_overridden_filter_chain_manager); Network::Address::InstanceConstSharedPtr any_address( new Network::Address::Ipv4Instance("0.0.0.0", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *ipv4_any_listener); + auto ipv6_overridden_filter_chain_manager = + std::make_shared>(); Network::TcpListenerCallbacks* ipv6_any_listener_callbacks; auto listener3 = new NiceMock(); - TestListener* ipv6_any_listener = addListener(1, false, false, "ipv6_any_test_listener", - listener3, &ipv6_any_listener_callbacks); + TestListener* ipv6_any_listener = + addListener(1, false, false, "ipv6_any_test_listener", listener3, + &ipv6_any_listener_callbacks, nullptr, nullptr, Network::Socket::Type::Stream, + std::chrono::milliseconds(15000), false, ipv6_overridden_filter_chain_manager); Network::Address::InstanceConstSharedPtr any_address_ipv6( new Network::Address::Ipv6Instance("::", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address_ipv6)); @@ -802,7 +810,10 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { cb.socket().addressProvider().restoreLocalAddress(alt_address); return Network::FilterStatus::Continue; })); - EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(manager_, findFilterChain(_)).Times(0); + EXPECT_CALL(*ipv4_overridden_filter_chain_manager, findFilterChain(_)) + .WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(*ipv6_overridden_filter_chain_manager, findFilterChain(_)).Times(0); auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); @@ -820,9 +831,6 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { auto scoped_runtime = std::make_unique(); - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.listener_wildcard_match_ip_family", "false"}}); - Network::TcpListenerCallbacks* listener_callbacks1; auto listener1 = new NiceMock(); TestListener* test_listener1 = @@ -832,19 +840,28 @@ TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); handler_->addListener(absl::nullopt, *test_listener1); + auto ipv4_overridden_filter_chain_manager = + std::make_shared>(); Network::TcpListenerCallbacks* ipv4_any_listener_callbacks; auto listener2 = new NiceMock(); - TestListener* ipv4_any_listener = addListener(1, false, false, "ipv4_any_test_listener", - listener2, &ipv4_any_listener_callbacks); + TestListener* ipv4_any_listener = + addListener(1, false, false, "ipv4_any_test_listener", listener2, + &ipv4_any_listener_callbacks, nullptr, nullptr, Network::Socket::Type::Stream, + std::chrono::milliseconds(15000), false, ipv4_overridden_filter_chain_manager); + Network::Address::InstanceConstSharedPtr any_address( new Network::Address::Ipv4Instance("0.0.0.0", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *ipv4_any_listener); + auto ipv6_overridden_filter_chain_manager = + std::make_shared>(); Network::TcpListenerCallbacks* ipv6_any_listener_callbacks; auto listener3 = new NiceMock(); - TestListener* ipv6_any_listener = addListener(1, false, false, "ipv6_any_test_listener", - listener3, &ipv6_any_listener_callbacks); + TestListener* ipv6_any_listener = + addListener(1, false, false, "ipv6_any_test_listener", listener3, + &ipv6_any_listener_callbacks, nullptr, nullptr, Network::Socket::Type::Stream, + std::chrono::milliseconds(15000), false, ipv6_overridden_filter_chain_manager); Network::Address::InstanceConstSharedPtr any_address_ipv6( new Network::Address::Ipv6Instance("::", 80)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address_ipv6)); @@ -872,17 +889,16 @@ TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) { cb.socket().addressProvider().restoreLocalAddress(alt_address); return Network::FilterStatus::Continue; })); - EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(manager_, findFilterChain(_)).Times(0); + EXPECT_CALL(*ipv4_overridden_filter_chain_manager, findFilterChain(_)).Times(0); + EXPECT_CALL(*ipv6_overridden_filter_chain_manager, findFilterChain(_)) + .WillOnce(Return(filter_chain_.get())); auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); EXPECT_EQ(1UL, handler_->numConnections()); - // metrics aren't sufficient for testing; need a better way to identify the socket - EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); - EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); - EXPECT_CALL(*listener3, onDestroy()); EXPECT_CALL(*listener2, onDestroy()); EXPECT_CALL(*listener1, onDestroy()); From 7845d2b058806de8858dac713793304c7e2c09f8 Mon Sep 17 00:00:00 2001 From: Jacob Delgado Date: Tue, 6 Jul 2021 22:22:07 -0600 Subject: [PATCH 12/12] Remove unnecessary comment Signed-off-by: Jacob Delgado --- test/server/connection_handler_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 61064640d2c62..914a959d8d225 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -820,8 +820,6 @@ TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) { listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); EXPECT_EQ(1UL, handler_->numConnections()); - // Add negative test case check - EXPECT_CALL(*listener3, onDestroy()); EXPECT_CALL(*listener2, onDestroy()); EXPECT_CALL(*listener1, onDestroy());