diff --git a/library/common/config/config.cc b/library/common/config/config.cc index b246594e1f..f36c7120eb 100644 --- a/library/common/config/config.cc +++ b/library/common/config/config.cc @@ -52,6 +52,7 @@ const std::string config_header = R"( - &dns_refresh_rate 60s - &dns_resolver_name envoy.network.dns_resolver.cares - &dns_resolver_config {"@type":"type.googleapis.com/envoy.extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig"} +- &enable_drain_post_dns_refresh false - &enable_interface_binding false - &h2_connection_keepalive_idle_interval 100000s - &h2_connection_keepalive_timeout 10s @@ -288,6 +289,7 @@ const char* config_template = R"( - name: envoy.filters.http.network_configuration typed_config: "@type": type.googleapis.com/envoymobile.extensions.filters.http.network_configuration.NetworkConfiguration + enable_drain_post_dns_refresh: *enable_drain_post_dns_refresh enable_interface_binding: *enable_interface_binding - name: envoy.filters.http.local_error typed_config: diff --git a/library/common/engine.cc b/library/common/engine.cc index bf8388bbbc..f5280c3d6f 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -301,12 +301,6 @@ void Engine::flushStats() { server_->flushStats(); } -void Engine::drainConnections() { - ASSERT(dispatcher_->isThreadSafe(), - "drainConnections must be called from the dispatcher's context"); - server_->clusterManager().drainConnections(nullptr); -} - Upstream::ClusterManager& Engine::getClusterManager() { ASSERT(dispatcher_->isThreadSafe(), "getClusterManager must be called from the dispatcher's context"); diff --git a/library/common/engine.h b/library/common/engine.h index 6e47e1f194..82c2eff943 100644 --- a/library/common/engine.h +++ b/library/common/engine.h @@ -122,11 +122,6 @@ class Engine : public Logger::Loggable { */ void flushStats(); - /** - * Drain all upstream connections associated with this Engine. - */ - void drainConnections(); - /** * Get cluster manager from the Engine. */ diff --git a/library/common/extensions/filters/http/network_configuration/config.cc b/library/common/extensions/filters/http/network_configuration/config.cc index d0d28f2629..944235b0b3 100644 --- a/library/common/extensions/filters/http/network_configuration/config.cc +++ b/library/common/extensions/filters/http/network_configuration/config.cc @@ -13,12 +13,13 @@ Http::FilterFactoryCb NetworkConfigurationFilterFactory::createFilterFactoryFrom const std::string&, Server::Configuration::FactoryContext& context) { auto network_configurator = Network::ConfiguratorFactory{context}.get(); + bool enable_drain_post_dns_refresh = proto_config.enable_drain_post_dns_refresh(); bool enable_interface_binding = proto_config.enable_interface_binding(); - return [network_configurator, + return [network_configurator, enable_drain_post_dns_refresh, enable_interface_binding](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(std::make_shared( - network_configurator, enable_interface_binding)); + network_configurator, enable_drain_post_dns_refresh, enable_interface_binding)); }; } diff --git a/library/common/extensions/filters/http/network_configuration/filter.cc b/library/common/extensions/filters/http/network_configuration/filter.cc index 8108be8fb7..9eed2c045a 100644 --- a/library/common/extensions/filters/http/network_configuration/filter.cc +++ b/library/common/extensions/filters/http/network_configuration/filter.cc @@ -21,6 +21,7 @@ void NetworkConfigurationFilter::setDecoderFilterCallbacks( auto options = std::make_shared(); network_configurator_->setInterfaceBindingEnabled(enable_interface_binding_); + network_configurator_->setDrainPostDnsRefreshEnabled(enable_drain_post_dns_refresh_); extra_stream_info_->configuration_key_ = network_configurator_->addUpstreamSocketOptions(options); decoder_callbacks_->addUpstreamSocketOptions(options); } diff --git a/library/common/extensions/filters/http/network_configuration/filter.h b/library/common/extensions/filters/http/network_configuration/filter.h index c632032b69..cd628d2bdf 100644 --- a/library/common/extensions/filters/http/network_configuration/filter.h +++ b/library/common/extensions/filters/http/network_configuration/filter.h @@ -22,9 +22,10 @@ class NetworkConfigurationFilter final : public Http::PassThroughFilter, public Logger::Loggable { public: NetworkConfigurationFilter(Network::ConfiguratorSharedPtr network_configurator, - bool enable_interface_binding) + bool enable_drain_post_dns_refresh, bool enable_interface_binding) : network_configurator_(network_configurator), extra_stream_info_(nullptr), // always set in setDecoderFilterCallbacks + enable_drain_post_dns_refresh_(enable_drain_post_dns_refresh), enable_interface_binding_(enable_interface_binding) {} // Http::StreamDecoderFilter @@ -37,6 +38,7 @@ class NetworkConfigurationFilter final : public Http::PassThroughFilter, private: Network::ConfiguratorSharedPtr network_configurator_; StreamInfo::ExtraStreamInfo* extra_stream_info_; + bool enable_drain_post_dns_refresh_; bool enable_interface_binding_; }; diff --git a/library/common/extensions/filters/http/network_configuration/filter.proto b/library/common/extensions/filters/http/network_configuration/filter.proto index 3e4b909158..88ff62ca9e 100644 --- a/library/common/extensions/filters/http/network_configuration/filter.proto +++ b/library/common/extensions/filters/http/network_configuration/filter.proto @@ -6,4 +6,8 @@ message NetworkConfiguration { // If set to true, the filter will permit the NetworkConfigurator to provide upstream // socket option that MAY bind a connection to a specific network interface. bool enable_interface_binding = 1; + + // If set to true, the filter will permit the NetworkConfigurator to drain connections + // when a DNS refresh is externally triggered. + bool enable_drain_post_dns_refresh = 2; } diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc index 9ebe1c0e70..72a2399183 100644 --- a/library/common/jni/jni_interface.cc +++ b/library/common/jni/jni_interface.cc @@ -1016,11 +1016,11 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_registerStringAccessor(JNIEnv* } extern "C" JNIEXPORT void JNICALL -Java_io_envoyproxy_envoymobile_engine_JniLibrary_drainConnections(JNIEnv* env, - jclass, // class - jlong engine) { - jni_log("[Envoy]", "drainConnections"); - drain_connections(engine); +Java_io_envoyproxy_envoymobile_engine_JniLibrary_resetConnectivityState(JNIEnv* env, + jclass, // class + jlong engine) { + jni_log("[Envoy]", "resetConnectivityState"); + reset_connectivity_state(engine); } extern "C" JNIEXPORT jint JNICALL diff --git a/library/common/main_interface.cc b/library/common/main_interface.cc index 40e2a444cb..a8a70d6c7c 100644 --- a/library/common/main_interface.cc +++ b/library/common/main_interface.cc @@ -68,7 +68,7 @@ envoy_status_t reset_stream(envoy_engine_t engine, envoy_stream_t stream) { envoy_status_t set_preferred_network(envoy_engine_t engine, envoy_network_t network) { envoy_netconf_t configuration_key = Envoy::Network::Configurator::setPreferredNetwork(network); Envoy::EngineHandle::runOnEngineDispatcher(engine, [configuration_key](auto& engine) -> void { - engine.networkConfigurator().refreshDns(configuration_key); + engine.networkConfigurator().refreshDns(configuration_key, true); }); // TODO(snowp): Should this return failure ever? return ENVOY_SUCCESS; @@ -188,9 +188,7 @@ envoy_status_t run_engine(envoy_engine_t engine, const char* config, const char* void terminate_engine(envoy_engine_t engine) { Envoy::EngineHandle::terminateEngine(engine); } -envoy_status_t drain_connections(envoy_engine_t e) { - // This will change once multiple engine support is in place. - // https://github.com/envoyproxy/envoy-mobile/issues/332 +envoy_status_t reset_connectivity_state(envoy_engine_t e) { return Envoy::EngineHandle::runOnEngineDispatcher( - e, [](auto& engine) { engine.drainConnections(); }); + e, [](auto& engine) { engine.networkConfigurator().resetConnectivityState(); }); } diff --git a/library/common/main_interface.h b/library/common/main_interface.h index af2c288ea6..45b239f964 100644 --- a/library/common/main_interface.h +++ b/library/common/main_interface.h @@ -203,11 +203,11 @@ envoy_status_t run_engine(envoy_engine_t engine, const char* config, const char* void terminate_engine(envoy_engine_t engine); /** - * Drain all upstream connections associated with an engine + * Refresh DNS, and drain connections associated with an engine. * @param engine, handle to the engine to drain. * @return envoy_status_t, the resulting status of the operation. */ -envoy_status_t drain_connections(envoy_engine_t engine); +envoy_status_t reset_connectivity_state(envoy_engine_t engine); #ifdef __cplusplus } // functions diff --git a/library/common/network/configurator.cc b/library/common/network/configurator.cc index 981c9682b0..8f46ad4fdd 100644 --- a/library/common/network/configurator.cc +++ b/library/common/network/configurator.cc @@ -82,6 +82,15 @@ Configurator::NetworkState Configurator::network_state_{1, ENVOY_NET_GENERIC, Ma envoy_netconf_t Configurator::setPreferredNetwork(envoy_network_t network) { Thread::LockGuard lock{network_state_.mutex_}; + + // TODO(goaway): Re-enable this guard. There's some concern that this will miss network updates + // moving from offline to online states. We should address this then re-enable this guard to + // avoid unnecessary cache refresh and connection drain. + // if (network_state_.network_ == network) { + // // Provide a non-current key preventing further scheduled effects (e.g. DNS refresh). + // return network_state_.configuration_key_ - 1; + //} + ENVOY_LOG_EVENT(debug, "netconf_network_change", std::to_string(network)); network_state_.configuration_key_++; @@ -167,13 +176,43 @@ void Configurator::reportNetworkUsage(envoy_netconf_t configuration_key, bool ne // If configuration state changed, refresh dns. if (configuration_updated) { - refreshDns(configuration_key); + refreshDns(configuration_key, false); + } +} + +void Configurator::onDnsResolutionComplete( + const std::string& host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&, + Network::DnsResolver::ResolutionStatus) { + if (enable_drain_post_dns_refresh_ && pending_drain_) { + pending_drain_ = false; + + // We ignore whether DNS resolution has succeeded here. If it failed, we may be offline and + // should probably drain connections. If it succeeds, we may have new DNS entries and so we + // drain connections. It may be possible to refine this logic in the future. + // TODO(goaway): check the set of cached hosts from the last triggered DNS refresh for this + // host, and if present, remove it and trigger connection drain for this host specifically. + ENVOY_LOG_EVENT(debug, "netconf_post_dns_drain_cx", host); + cluster_manager_.drainConnections(nullptr); + } +} + +void Configurator::setDrainPostDnsRefreshEnabled(bool enabled) { + enable_drain_post_dns_refresh_ = enabled; + if (!enabled) { + pending_drain_ = false; + } else if (!dns_callbacks_handle_) { + // Register callbacks once, on demand, using the handle as a sentinel. There may not be + // a DNS cache during initialization, but if one is available, it should always exist by the + // time this function is called from the NetworkConfigurationFilter. + if (auto dns_cache = dnsCache()) { + dns_callbacks_handle_ = dns_cache->addUpdateCallbacks(*this); + } } } void Configurator::setInterfaceBindingEnabled(bool enabled) { enable_interface_binding_ = enabled; } -void Configurator::refreshDns(envoy_netconf_t configuration_key) { +void Configurator::refreshDns(envoy_netconf_t configuration_key, bool drain_connections) { { Thread::LockGuard lock{network_state_.mutex_}; @@ -188,12 +227,33 @@ void Configurator::refreshDns(envoy_netconf_t configuration_key) { } } - if (auto dns_cache = dns_cache_manager_->lookUpCacheByName(BaseDnsCache)) { + if (auto dns_cache = dnsCache()) { ENVOY_LOG_EVENT(debug, "netconf_refresh_dns", std::to_string(configuration_key)); + pending_drain_ = drain_connections && enable_drain_post_dns_refresh_; + // TODO(goaway): capture the list of currently cached hosts here in a set, to be checked + // for draining when DNS resolutions occur. dns_cache->forceRefreshHosts(); - } else { + } +} + +Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr Configurator::dnsCache() { + auto cache = dns_cache_manager_->lookUpCacheByName(BaseDnsCache); + if (!cache) { ENVOY_LOG_EVENT(warn, "netconf_dns_cache_missing", BaseDnsCache); } + return cache; +} + +void Configurator::resetConnectivityState() { + envoy_netconf_t configuration_key; + { + Thread::LockGuard lock{network_state_.mutex_}; + network_state_.remaining_faults_ = 1; + network_state_.socket_mode_ = DefaultPreferredNetworkMode; + configuration_key = ++network_state_.configuration_key_; + } + + refreshDns(configuration_key, true); } std::vector Configurator::enumerateV4Interfaces() { @@ -347,7 +407,8 @@ ConfiguratorSharedPtr ConfiguratorFactory::get() { SINGLETON_MANAGER_REGISTERED_NAME(network_configurator), [&] { Extensions::Common::DynamicForwardProxy::DnsCacheManagerFactoryImpl cache_manager_factory{ context_}; - return std::make_shared(cache_manager_factory.get()); + return std::make_shared(context_.clusterManager(), + cache_manager_factory.get()); }); } diff --git a/library/common/network/configurator.h b/library/common/network/configurator.h index e0b768c1a1..60c857d801 100644 --- a/library/common/network/configurator.h +++ b/library/common/network/configurator.h @@ -5,7 +5,9 @@ #include "envoy/network/socket.h" #include "envoy/singleton/manager.h" +#include "envoy/upstream/cluster_manager.h" +#include "source/extensions/common/dynamic_forward_proxy/dns_cache.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h" #include "library/common/types/c_types.h" @@ -60,11 +62,34 @@ using InterfacePair = std::pair, public Singleton::Instance { +class Configurator : public Logger::Loggable, + public Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks, + public Singleton::Instance { public: - Configurator(DnsCacheManagerSharedPtr dns_cache_manager) - : enable_interface_binding_{false}, dns_cache_manager_(dns_cache_manager) {} + Configurator(Upstream::ClusterManager& cluster_manager, + DnsCacheManagerSharedPtr dns_cache_manager) + : cluster_manager_(cluster_manager), dns_cache_manager_(dns_cache_manager) {} + + // Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks + void onDnsHostAddOrUpdate( + const std::string& /*host*/, + const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override {} + void onDnsHostRemove(const std::string& /*host*/) override {} + void onDnsResolutionComplete(const std::string& /*host*/, + const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&, + Network::DnsResolver::ResolutionStatus) override; /** * @returns a list of local network interfaces supporting IPv4. @@ -108,12 +133,21 @@ class Configurator : public Logger::Loggable, public Singl void reportNetworkUsage(envoy_netconf_t configuration_key, bool network_fault); /** - * Sets the current OS default/preferred network class. + * Sets the current OS default/preferred network class. Note this function is allowed to be + * called from any thread. * @param network, the OS-preferred network. * @returns configuration key to associate with any related calls. */ static envoy_netconf_t setPreferredNetwork(envoy_network_t network); + /** + * Configure whether connections should be drained after a triggered DNS refresh. Currently this + * may happen either due to an external call to refreshConnectivityState or an update to + * setPreferredNetwork. + * @param enabled, whether to enable connection drain after DNS refresh. + */ + void setDrainPostDnsRefreshEnabled(bool enabled); + /** * Sets whether subsequent calls for upstream socket options may leverage options that bind * to specific network interfaces. @@ -124,8 +158,14 @@ class Configurator : public Logger::Loggable, public Singl /** * Refresh DNS in response to preferred network update. May be no-op. * @param configuration_key, key provided by this class representing the current configuration. + * @param drain_connections, request that connections be drained after next DNS resolution. */ - void refreshDns(envoy_netconf_t configuration_key); + void refreshDns(envoy_netconf_t configuration_key, bool drain_connections); + + /** + * Drain all upstream connections associated with this Engine. + */ + void resetConnectivityState(); /** * @returns the current socket options that should be used for connections. @@ -152,7 +192,21 @@ class Configurator : public Logger::Loggable, public Singl Socket::OptionsSharedPtr getAlternateInterfaceSocketOptions(envoy_network_t network); InterfacePair getActiveAlternateInterface(envoy_network_t network, unsigned short family); - bool enable_interface_binding_; + /** + * Returns the default DNS cache set up in base configuration. This cache may be missing either + * due to engine lifecycle-related timing or alternate configurations. If it is, operations + * that use it should revert to no-ops. + * + * @returns the default DNS cache set up in base configuration or nullptr. + */ + Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dnsCache(); + + bool enable_drain_post_dns_refresh_{false}; + bool enable_interface_binding_{false}; + bool pending_drain_{false}; + Extensions::Common::DynamicForwardProxy::DnsCache::AddUpdateCallbacksHandlePtr + dns_callbacks_handle_{nullptr}; + Upstream::ClusterManager& cluster_manager_; DnsCacheManagerSharedPtr dns_cache_manager_; static NetworkState network_state_; }; @@ -164,7 +218,7 @@ using ConfiguratorSharedPtr = std::shared_ptr; */ class ConfiguratorFactory { public: - ConfiguratorFactory(Server::Configuration::FactoryContextBase& context) : context_(context) {} + ConfiguratorFactory(Server::Configuration::CommonFactoryContext& context) : context_(context) {} /** * @returns singleton Configurator instance. @@ -172,7 +226,7 @@ class ConfiguratorFactory { ConfiguratorSharedPtr get(); private: - Server::Configuration::FactoryContextBase& context_; + Server::Configuration::CommonFactoryContext& context_; }; /** diff --git a/library/java/io/envoyproxy/envoymobile/engine/AndroidEngineImpl.java b/library/java/io/envoyproxy/envoymobile/engine/AndroidEngineImpl.java index 2ba5be1c9e..5434f327e9 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/AndroidEngineImpl.java +++ b/library/java/io/envoyproxy/envoymobile/engine/AndroidEngineImpl.java @@ -90,8 +90,8 @@ public int registerStringAccessor(String accessorName, EnvoyStringAccessor acces } @Override - public void drainConnections() { - envoyEngine.drainConnections(); + public void resetConnectivityState() { + envoyEngine.resetConnectivityState(); } @Override diff --git a/library/java/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java b/library/java/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java index 04e5787409..5d8f973e74 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java +++ b/library/java/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java @@ -35,6 +35,7 @@ public enum TrustChainVerification { public final String dnsPreresolveHostnames; public final List dnsFallbackNameservers; public final Boolean dnsFilterUnroutableFamilies; + public final Boolean enableDrainPostDnsRefresh; public final Boolean enableHttp3; public final Boolean enableHappyEyeballs; public final Boolean enableInterfaceBinding; @@ -71,6 +72,7 @@ public enum TrustChainVerification { * @param dnsPreresolveHostnames hostnames to preresolve on Envoy Client construction. * @param dnsFallbackNameservers addresses to use as DNS name server fallback. * @param dnsFilterUnroutableFamilies whether to filter unroutable IP families or not. + * @param enableDrainPostDnsRefresh whether to drain connections after soft DNS refresh. * @param enableHttp3 whether to enable experimental support for HTTP/3 (QUIC). * @param enableHappyEyeballs whether to enable RFC 6555 handling for IPv4/IPv6. * @param enableInterfaceBinding whether to allow interface binding. @@ -97,13 +99,13 @@ public EnvoyConfiguration( int connectTimeoutSeconds, int dnsRefreshSeconds, int dnsFailureRefreshSecondsBase, int dnsFailureRefreshSecondsMax, int dnsQueryTimeoutSeconds, int dnsMinRefreshSeconds, String dnsPreresolveHostnames, List dnsFallbackNameservers, - Boolean dnsFilterUnroutableFamilies, boolean enableHttp3, boolean enableHappyEyeballs, - boolean enableInterfaceBinding, int h2ConnectionKeepaliveIdleIntervalMilliseconds, - int h2ConnectionKeepaliveTimeoutSeconds, boolean h2ExtendKeepaliveTimeout, - List h2RawDomains, int maxConnectionsPerHost, int statsFlushSeconds, - int streamIdleTimeoutSeconds, int perTryIdleTimeoutSeconds, String appVersion, String appId, - TrustChainVerification trustChainVerification, String virtualClusters, - List nativeFilterChain, + Boolean dnsFilterUnroutableFamilies, boolean enableDrainPostDnsRefresh, boolean enableHttp3, + boolean enableHappyEyeballs, boolean enableInterfaceBinding, + int h2ConnectionKeepaliveIdleIntervalMilliseconds, int h2ConnectionKeepaliveTimeoutSeconds, + boolean h2ExtendKeepaliveTimeout, List h2RawDomains, int maxConnectionsPerHost, + int statsFlushSeconds, int streamIdleTimeoutSeconds, int perTryIdleTimeoutSeconds, + String appVersion, String appId, TrustChainVerification trustChainVerification, + String virtualClusters, List nativeFilterChain, List httpPlatformFilterFactories, Map stringAccessors) { this.adminInterfaceEnabled = adminInterfaceEnabled; @@ -118,6 +120,7 @@ public EnvoyConfiguration( this.dnsPreresolveHostnames = dnsPreresolveHostnames; this.dnsFallbackNameservers = dnsFallbackNameservers; this.dnsFilterUnroutableFamilies = dnsFilterUnroutableFamilies; + this.enableDrainPostDnsRefresh = enableDrainPostDnsRefresh; this.enableHttp3 = enableHttp3; this.enableHappyEyeballs = enableHappyEyeballs; this.enableInterfaceBinding = enableInterfaceBinding; @@ -222,6 +225,8 @@ String resolveTemplate(final String configTemplate, final String platformFilterT .append("- &dns_resolver_name envoy.network.dns_resolver.cares\n") .append(String.format("- &dns_refresh_rate %ss\n", dnsRefreshSeconds)) .append(String.format("- &dns_resolver_config %s\n", dnsResolverConfig)) + .append(String.format("- &enable_drain_post_dns_refresh %s\n", + enableDrainPostDnsRefresh ? "true" : "false")) .append(String.format("- &enable_interface_binding %s\n", enableInterfaceBinding ? "true" : "false")) .append(String.format("- &h2_connection_keepalive_idle_interval %ss\n", diff --git a/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngine.java b/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngine.java index deb338991f..7b9e18036b 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngine.java +++ b/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngine.java @@ -116,9 +116,9 @@ int runWithTemplate(String configurationYAML, EnvoyConfiguration envoyConfigurat String dumpStats(); /** - * Drain all connections owned by this Engine. + * Refresh DNS, and drain connections owned by this Engine. */ - void drainConnections(); + void resetConnectivityState(); /** * Update the network interface to the preferred network for opening new diff --git a/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java b/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java index e62ed3d497..6f1e7e94c2 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java +++ b/library/java/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java @@ -203,8 +203,8 @@ public int registerStringAccessor(String accessor_name, EnvoyStringAccessor acce } @Override - public void drainConnections() { - JniLibrary.drainConnections(engineHandle); + public void resetConnectivityState() { + JniLibrary.resetConnectivityState(engineHandle); } @Override diff --git a/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java b/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java index 0729dd1c07..cac0daae03 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java +++ b/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java @@ -316,11 +316,11 @@ protected static native int registerStringAccessor(String accessorName, JvmStringAccessorContext context); /** - * Drain all connections owned by this Engine. + * Refresh DNS, and drain connections owned by this Engine. * * @param engine Handle to the engine for which to drain connections. */ - protected static native int drainConnections(long engine); + protected static native int resetConnectivityState(long engine); /** * Update the network interface to the preferred network for opening new diff --git a/library/java/org/chromium/net/impl/NativeCronetEngineBuilderImpl.java b/library/java/org/chromium/net/impl/NativeCronetEngineBuilderImpl.java index 820bbec90d..0cbd6ef679 100644 --- a/library/java/org/chromium/net/impl/NativeCronetEngineBuilderImpl.java +++ b/library/java/org/chromium/net/impl/NativeCronetEngineBuilderImpl.java @@ -60,6 +60,7 @@ public class NativeCronetEngineBuilderImpl extends CronetEngineBuilderImpl { private String mDnsPreresolveHostnames = "[]"; private List mDnsFallbackNameservers = Collections.emptyList(); private boolean mEnableDnsFilterUnroutableFamilies = false; + private boolean mEnableDrainPostDnsRefresh = false; private boolean mEnableHttp3 = false; private boolean mEnableHappyEyeballs = false; private boolean mEnableInterfaceBinding = false; @@ -123,8 +124,8 @@ private EnvoyConfiguration createEnvoyConfiguration() { mAdminInterfaceEnabled, mGrpcStatsDomain, mStatsDPort, mConnectTimeoutSeconds, mDnsRefreshSeconds, mDnsFailureRefreshSecondsBase, mDnsFailureRefreshSecondsMax, mDnsQueryTimeoutSeconds, mDnsMinRefreshSeconds, mDnsPreresolveHostnames, - mDnsFallbackNameservers, mEnableDnsFilterUnroutableFamilies, mEnableHttp3, - mEnableHappyEyeballs, mEnableInterfaceBinding, + mDnsFallbackNameservers, mEnableDnsFilterUnroutableFamilies, mEnableDrainPostDnsRefresh, + mEnableHttp3, mEnableHappyEyeballs, mEnableInterfaceBinding, mH2ConnectionKeepaliveIdleIntervalMilliseconds, mH2ConnectionKeepaliveTimeoutSeconds, mH2ExtendKeepaliveTimeout, mH2RawDomains, mMaxConnectionsPerHost, mStatsFlushSeconds, mStreamIdleTimeoutSeconds, mPerTryIdleTimeoutSeconds, mAppVersion, mAppId, diff --git a/library/kotlin/io/envoyproxy/envoymobile/Engine.kt b/library/kotlin/io/envoyproxy/envoymobile/Engine.kt index acdf8cd367..18dcab9d5a 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/Engine.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/Engine.kt @@ -35,7 +35,7 @@ interface Engine { fun dumpStats(): String /** - * Drain all connections owned by this Engine. + * Refresh DNS, and drain connections owned by this Engine. */ - fun drainConnections() + fun resetConnectivityState() } diff --git a/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt b/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt index a11d79cbb0..84b5de64e0 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt @@ -39,6 +39,7 @@ open class EngineBuilder( private var dnsQueryTimeoutSeconds = 25 private var dnsMinRefreshSeconds = 60 private var dnsPreresolveHostnames = "[]" + private var enableDrainPostDnsRefresh = false private var enableHttp3 = false private var enableHappyEyeballs = false private var enableInterfaceBinding = false @@ -199,6 +200,21 @@ open class EngineBuilder( return this } + /** + * Specify whether to drain connections after the resolution of a soft DNS refresh. A refresh may + * be triggered directly via the Engine API, or as a result of a network status update provided by + * the OS. Draining connections does not interrupt existing connections or requests, but will + * establish new connections for any further requests. + * + * @param enableDrainPostDnsRefresh whether to drain connections after soft DNS refresh. + * + * @return This builder. + */ + fun enableDrainPostDnsRefresh(enableDrainPostDnsRefresh: Boolean): EngineBuilder { + this.enableDrainPostDnsRefresh = enableDrainPostDnsRefresh + return this + } + /** * Specify whether to enable experimental HTTP/3 (QUIC) support. Note the actual protocol will * be negotiated with the upstream endpoint and so upstream support is still required for HTTP/3 @@ -503,6 +519,7 @@ open class EngineBuilder( dnsPreresolveHostnames, dnsFallbackNameservers, dnsFilterUnroutableFamilies, + enableDrainPostDnsRefresh, enableHttp3, enableHappyEyeballs, enableInterfaceBinding, diff --git a/library/kotlin/io/envoyproxy/envoymobile/EngineImpl.kt b/library/kotlin/io/envoyproxy/envoymobile/EngineImpl.kt index f741637b33..893338418b 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/EngineImpl.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/EngineImpl.kt @@ -52,7 +52,7 @@ class EngineImpl constructor( return envoyEngine.dumpStats() } - override fun drainConnections() { - envoyEngine.drainConnections() + override fun resetConnectivityState() { + envoyEngine.resetConnectivityState() } } diff --git a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockEnvoyEngine.kt b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockEnvoyEngine.kt index 1145bacbf8..fcbbfe18b4 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockEnvoyEngine.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockEnvoyEngine.kt @@ -46,7 +46,7 @@ internal class MockEnvoyEngine : EnvoyEngine { override fun dumpStats(): String = "" - override fun drainConnections() = Unit + override fun resetConnectivityState() = Unit override fun setPreferredNetwork(network: EnvoyNetworkType) = Unit } diff --git a/library/objective-c/EnvoyEngine.h b/library/objective-c/EnvoyEngine.h index c713ab03b2..dc6d212d45 100644 --- a/library/objective-c/EnvoyEngine.h +++ b/library/objective-c/EnvoyEngine.h @@ -539,7 +539,7 @@ extern const int kEnvoyFailure; - (void)terminate; -- (void)drainConnections; +- (void)resetConnectivityState; @end diff --git a/library/objective-c/EnvoyEngineImpl.m b/library/objective-c/EnvoyEngineImpl.m index 08f4a2b320..b355cdd8db 100644 --- a/library/objective-c/EnvoyEngineImpl.m +++ b/library/objective-c/EnvoyEngineImpl.m @@ -590,8 +590,8 @@ - (void)terminate { terminate_engine(_engineHandle); } -- (void)drainConnections { - drain_connections(_engineHandle); +- (void)resetConnectivityState { + reset_connectivity_state(_engineHandle); } #pragma mark - Private diff --git a/library/swift/Engine.swift b/library/swift/Engine.swift index 877ef1b539..b3a8e7eccc 100644 --- a/library/swift/Engine.swift +++ b/library/swift/Engine.swift @@ -20,6 +20,6 @@ public protocol Engine: AnyObject { /// Terminates the running engine. func terminate() - /// Drain all connections owned by this Engine. - func drainConnections() + /// Refresh DNS, and drain connections owned by this Engine. + func resetConnectivityState() } diff --git a/library/swift/EngineImpl.swift b/library/swift/EngineImpl.swift index e490495401..642b3a02c6 100644 --- a/library/swift/EngineImpl.swift +++ b/library/swift/EngineImpl.swift @@ -70,7 +70,7 @@ extension EngineImpl: Engine { self.engine.terminate() } - func drainConnections() { - self.engine.drainConnections() + func resetConnectivityState() { + self.engine.resetConnectivityState() } } diff --git a/library/swift/mocks/MockEnvoyEngine.swift b/library/swift/mocks/MockEnvoyEngine.swift index c89f08dbd9..592c2db784 100644 --- a/library/swift/mocks/MockEnvoyEngine.swift +++ b/library/swift/mocks/MockEnvoyEngine.swift @@ -96,5 +96,5 @@ extension MockEnvoyEngine: EnvoyEngine { func terminate() {} - func drainConnections() {} + func resetConnectivityState() {} } diff --git a/test/common/main_interface_test.cc b/test/common/main_interface_test.cc index f52194ccf7..c9c22c615d 100644 --- a/test/common/main_interface_test.cc +++ b/test/common/main_interface_test.cc @@ -672,7 +672,7 @@ TEST(EngineTest, EventTrackerRegistersEnvoyBugRecordAction) { ASSERT_TRUE(test_context.on_exit.WaitForNotificationWithTimeout(absl::Seconds(3))); } -TEST(MainInterfaceTest, DrainConnections) { +TEST(MainInterfaceTest, ResetConnectivityState) { engine_test_context test_context{}; envoy_engine_callbacks engine_cbs{[](void* context) -> void { auto* engine_running = @@ -688,7 +688,7 @@ TEST(MainInterfaceTest, DrainConnections) { run_engine(engine_handle, MINIMAL_TEST_CONFIG.c_str(), LEVEL_DEBUG.c_str()); ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(3))); - ASSERT_EQ(ENVOY_SUCCESS, drain_connections(engine_handle)); + ASSERT_EQ(ENVOY_SUCCESS, reset_connectivity_state(engine_handle)); terminate_engine(engine_handle); ASSERT_TRUE(test_context.on_exit.WaitForNotificationWithTimeout(absl::Seconds(3))); diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 8ad30ac170..41db4e965f 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -11,6 +11,7 @@ envoy_cc_test( deps = [ "//library/common/network:configurator_lib", "@envoy//test/extensions/common/dynamic_forward_proxy:mocks", + "@envoy//test/mocks/upstream:cluster_manager_mocks", ], ) diff --git a/test/common/network/configurator_test.cc b/test/common/network/configurator_test.cc index d3bd529c63..31bcf256c9 100644 --- a/test/common/network/configurator_test.cc +++ b/test/common/network/configurator_test.cc @@ -1,11 +1,13 @@ #include #include "test/extensions/common/dynamic_forward_proxy/mocks.h" +#include "test/mocks/upstream/cluster_manager.h" #include "gtest/gtest.h" #include "library/common/network/configurator.h" using testing::_; +using testing::Ref; using testing::Return; namespace Envoy { @@ -17,31 +19,77 @@ class ConfiguratorTest : public testing::Test { : dns_cache_manager_( new NiceMock()), dns_cache_(dns_cache_manager_->dns_cache_), - configurator_(std::make_shared(dns_cache_manager_)) { + configurator_(std::make_shared(cm_, dns_cache_manager_)) { ON_CALL(*dns_cache_manager_, lookUpCacheByName(_)).WillByDefault(Return(dns_cache_)); + // Toggle network to reset network state. + Configurator::setPreferredNetwork(ENVOY_NET_GENERIC); + Configurator::setPreferredNetwork(ENVOY_NET_WLAN); } std::shared_ptr> dns_cache_manager_; std::shared_ptr dns_cache_; + NiceMock cm_{}; ConfiguratorSharedPtr configurator_; }; +TEST_F(ConfiguratorTest, SetPreferredNetworkWithNewNetworkChangesConfigurationKey) { + envoy_netconf_t original_key = configurator_->getConfigurationKey(); + envoy_netconf_t new_key = Configurator::setPreferredNetwork(ENVOY_NET_WWAN); + EXPECT_NE(original_key, new_key); + EXPECT_EQ(new_key, configurator_->getConfigurationKey()); +} + +TEST_F(ConfiguratorTest, + DISABLED_SetPreferredNetworkWithUnchangedNetworkReturnsStaleConfigurationKey) { + envoy_netconf_t original_key = configurator_->getConfigurationKey(); + envoy_netconf_t stale_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); + EXPECT_NE(original_key, stale_key); + EXPECT_EQ(original_key, configurator_->getConfigurationKey()); +} + TEST_F(ConfiguratorTest, RefreshDnsForCurrentConfigurationTriggersDnsRefresh) { EXPECT_CALL(*dns_cache_, forceRefreshHosts()); - envoy_netconf_t configuration_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); - configurator_->refreshDns(configuration_key); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); + configurator_->refreshDns(configuration_key, false); } TEST_F(ConfiguratorTest, RefreshDnsForStaleConfigurationDoesntTriggerDnsRefresh) { EXPECT_CALL(*dns_cache_, forceRefreshHosts()).Times(0); - envoy_netconf_t configuration_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); - configurator_->refreshDns(configuration_key - 1); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); + configurator_->refreshDns(configuration_key - 1, false); +} + +TEST_F(ConfiguratorTest, WhenDrainPostDnsRefreshEnabledDrainsPostDnsRefresh) { + EXPECT_CALL(*dns_cache_, addUpdateCallbacks_(Ref(*configurator_))); + configurator_->setDrainPostDnsRefreshEnabled(true); + + EXPECT_CALL(*dns_cache_, forceRefreshHosts()); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); + configurator_->refreshDns(configuration_key, true); + + EXPECT_CALL(cm_, drainConnections(_)); + configurator_->onDnsResolutionComplete( + "example.com", std::make_shared(), + Network::DnsResolver::ResolutionStatus::Success); +} + +TEST_F(ConfiguratorTest, WhenDrainPostDnsNotEnabledDoesntDrainPostDnsRefresh) { + configurator_->setDrainPostDnsRefreshEnabled(false); + + EXPECT_CALL(*dns_cache_, forceRefreshHosts()); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); + configurator_->refreshDns(configuration_key, true); + + EXPECT_CALL(cm_, drainConnections(_)).Times(0); + configurator_->onDnsResolutionComplete( + "example.com", std::make_shared(), + Network::DnsResolver::ResolutionStatus::Success); } TEST_F(ConfiguratorTest, ReportNetworkUsageDoesntAlterNetworkConfigurationWhenBoundInterfacesAreDisabled) { - envoy_netconf_t configuration_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); configurator_->setInterfaceBindingEnabled(false); EXPECT_EQ(DefaultPreferredNetworkMode, configurator_->getSocketMode()); @@ -54,7 +102,7 @@ TEST_F(ConfiguratorTest, } TEST_F(ConfiguratorTest, ReportNetworkUsageTriggersOverrideAfterFirstFaultAfterNetworkUpdate) { - envoy_netconf_t configuration_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); configurator_->setInterfaceBindingEnabled(true); EXPECT_EQ(DefaultPreferredNetworkMode, configurator_->getSocketMode()); @@ -65,7 +113,7 @@ TEST_F(ConfiguratorTest, ReportNetworkUsageTriggersOverrideAfterFirstFaultAfterN } TEST_F(ConfiguratorTest, ReportNetworkUsageDisablesOverrideAfterFirstFaultAfterOverride) { - envoy_netconf_t configuration_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); configurator_->setInterfaceBindingEnabled(true); EXPECT_EQ(DefaultPreferredNetworkMode, configurator_->getSocketMode()); @@ -82,7 +130,7 @@ TEST_F(ConfiguratorTest, ReportNetworkUsageDisablesOverrideAfterFirstFaultAfterO } TEST_F(ConfiguratorTest, ReportNetworkUsageDisablesOverrideAfterThirdFaultAfterSuccess) { - envoy_netconf_t configuration_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); + envoy_netconf_t configuration_key = configurator_->getConfigurationKey(); configurator_->setInterfaceBindingEnabled(true); EXPECT_EQ(DefaultPreferredNetworkMode, configurator_->getSocketMode()); @@ -100,7 +148,7 @@ TEST_F(ConfiguratorTest, ReportNetworkUsageDisablesOverrideAfterThirdFaultAfterS } TEST_F(ConfiguratorTest, ReportNetworkUsageDisregardsCallsWithStaleConfigurationKey) { - envoy_netconf_t stale_key = Configurator::setPreferredNetwork(ENVOY_NET_WLAN); + envoy_netconf_t stale_key = configurator_->getConfigurationKey(); envoy_netconf_t current_key = Configurator::setPreferredNetwork(ENVOY_NET_WWAN); EXPECT_NE(stale_key, current_key); diff --git a/test/java/io/envoyproxy/envoymobile/engine/EnvoyConfigurationTest.kt b/test/java/io/envoyproxy/envoymobile/engine/EnvoyConfigurationTest.kt index 1c41f433cf..4bdced2db7 100644 --- a/test/java/io/envoyproxy/envoymobile/engine/EnvoyConfigurationTest.kt +++ b/test/java/io/envoyproxy/envoymobile/engine/EnvoyConfigurationTest.kt @@ -44,6 +44,7 @@ class EnvoyConfigurationTest { dnsPreresolveHostnames: String = "[hostname]", dnsFallbackNameservers: List = emptyList(), enableDnsFilterUnroutableFamilies: Boolean = false, + enableDrainPostDnsRefresh: Boolean = false, enableHttp3: Boolean = false, enableHappyEyeballs: Boolean = false, enableInterfaceBinding: Boolean = false, @@ -73,6 +74,7 @@ class EnvoyConfigurationTest { dnsPreresolveHostnames, dnsFallbackNameservers, enableDnsFilterUnroutableFamilies, + enableDrainPostDnsRefresh, enableHttp3, enableHappyEyeballs, enableInterfaceBinding, @@ -116,6 +118,7 @@ class EnvoyConfigurationTest { assertThat(resolvedTemplate).contains("&dns_preresolve_hostnames [hostname]") assertThat(resolvedTemplate).contains("&dns_resolver_name envoy.network.dns_resolver.cares") assertThat(resolvedTemplate).contains("&dns_resolver_config {\"@type\":\"type.googleapis.com/envoy.extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig\",\"resolvers\":[],\"use_resolvers_as_fallback\": false, \"filter_unroutable_families\": false}") + assertThat(resolvedTemplate).contains("&enable_drain_post_dns_refresh false") // Interface Binding assertThat(resolvedTemplate).contains("&enable_interface_binding false") @@ -161,6 +164,7 @@ class EnvoyConfigurationTest { val envoyConfiguration = buildTestEnvoyConfiguration( dnsFallbackNameservers = listOf("8.8.8.8"), enableDnsFilterUnroutableFamilies = true, + enableDrainPostDnsRefresh = true, enableHappyEyeballs = true, enableHttp3 = true, enableInterfaceBinding = true, @@ -175,6 +179,7 @@ class EnvoyConfigurationTest { assertThat(resolvedTemplate).contains("&dns_resolver_config {\"@type\":\"type.googleapis.com/envoy.extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig\",\"resolvers\":[{\"socket_address\":{\"address\":\"8.8.8.8\"}}],\"use_resolvers_as_fallback\": true, \"filter_unroutable_families\": true}") assertThat(resolvedTemplate).contains("&dns_lookup_family ALL") assertThat(resolvedTemplate).contains("&dns_multiple_addresses true") + assertThat(resolvedTemplate).contains("&enable_drain_post_dns_refresh true") // H2 assertThat(resolvedTemplate).contains("&h2_delay_keepalive_timeout true") diff --git a/test/kotlin/integration/BUILD b/test/kotlin/integration/BUILD index 2fe57f6510..01a75e7a9f 100644 --- a/test/kotlin/integration/BUILD +++ b/test/kotlin/integration/BUILD @@ -86,9 +86,9 @@ envoy_mobile_jni_kt_test( ) envoy_mobile_jni_kt_test( - name = "drain_connections_test", + name = "reset_connectivity_state_test", srcs = [ - "DrainConnectionsTest.kt", + "ResetConnectivityStateTest.kt", ], native_deps = [ "//library/common/jni:libjava_jni_lib.so", diff --git a/test/kotlin/integration/DrainConnectionsTest.kt b/test/kotlin/integration/ResetConnectivityStateTest.kt similarity index 98% rename from test/kotlin/integration/DrainConnectionsTest.kt rename to test/kotlin/integration/ResetConnectivityStateTest.kt index 597c659120..b2dab9c894 100644 --- a/test/kotlin/integration/DrainConnectionsTest.kt +++ b/test/kotlin/integration/ResetConnectivityStateTest.kt @@ -55,7 +55,7 @@ static_resources: "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router """ -class DrainConnectionsTest { +class ResetConnectivityStateTest { init { JniLibrary.loadTestLibrary() @@ -91,7 +91,7 @@ class DrainConnectionsTest { headersExpectation.await(10, TimeUnit.SECONDS) - engine.drainConnections() + engine.resetConnectivityState() var resultHeaders2: ResponseHeaders? = null var resultEndStream2: Boolean? = null diff --git a/test/swift/integration/BUILD b/test/swift/integration/BUILD index d8ca9cf690..47f604fe5f 100644 --- a/test/swift/integration/BUILD +++ b/test/swift/integration/BUILD @@ -15,13 +15,13 @@ envoy_mobile_swift_test( "DirectResponsePrefixHeadersMatchIntegrationTest.swift", "DirectResponsePrefixPathMatchIntegrationTest.swift", "DirectResponseSuffixHeadersMatchIntegrationTest.swift", - "DrainConnectionsTest.swift", "EngineApiTest.swift", "FilterResetIdleTest.swift", "GRPCReceiveErrorTest.swift", "IdleTimeoutTest.swift", "ReceiveDataTest.swift", "ReceiveErrorTest.swift", + "ResetConnectivityStateTest.swift", "SendDataTest.swift", "SendHeadersTest.swift", "SendTrailersTest.swift", diff --git a/test/swift/integration/DrainConnectionsTest.swift b/test/swift/integration/ResetConnectivityStateTest.swift similarity index 95% rename from test/swift/integration/DrainConnectionsTest.swift rename to test/swift/integration/ResetConnectivityStateTest.swift index 5de9b22a99..cd285b63dd 100644 --- a/test/swift/integration/DrainConnectionsTest.swift +++ b/test/swift/integration/ResetConnectivityStateTest.swift @@ -3,8 +3,8 @@ import EnvoyEngine import Foundation import XCTest -final class DrainConnectionsTest: XCTestCase { - func testDrainConnections() { +final class ResetConnectivityStateTest: XCTestCase { + func testResetConnectivityState() { // swiftlint:disable:next line_length let emhcmType = "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.EnvoyMobileHttpConnectionManager" // swiftlint:disable:next line_length @@ -77,7 +77,7 @@ static_resources: XCTAssertEqual(XCTWaiter.wait(for: [expectation1], timeout: 1), .completed) - engine.drainConnections() + engine.resetConnectivityState() let expectation2 = self.expectation(description: "Run called with expected http status first request")