mobile: simplify ConnectivityManager interface (#40329)

danzh2010 · danzh1989 · web-flow · commit f2006890d395 · 2025-07-21T22:12:29.000-04:00
Commit Message: decouple `DnsCache::UpdateCallbacks` and
`ConnectivityManager` interface. Extract `DnsCache::UpdateCallbacks`
implementation from `ConnectivityManagerImpl` into a stand-alone class
`RefreshDnsWithPostDrainHandler` which is created and owned by
`ConnectivityManagerImpl`.

Also remove `clusterManager()` and `getUpstreamSocketOptions()` from
`ConnectivityManager` interface. The caller of `clusterManager()` has
other way to access the global cluster manager.
`getUpstreamSocketOptions()` is only called inside of
`ConnectivityManagerImpl`.

Additional Description: a few refactories:
Refactor `InternalEngine::handleNetworkChange()`;
Rename `NetworkState` to `DefaultNetworkState` and move its member
`mutex_` from the struct to its parent `ConnectivityManagerImpl` to
guard the entire network_state_ and possibly more states of non-default
networks in the future.

Risk Level: low
Testing: existing tests passed
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

---------

Signed-off-by: Dan Zhang &lt;danzh@google.com&gt;
Co-authored-by: Dan Zhang &lt;danzh@google.com&gt;
diff --git a/mobile/library/common/internal_engine.cc b/mobile/library/common/internal_engine.cc
@@ -415,26 +415,29 @@ void InternalEngine::handleNetworkChange(const int network_type, const bool has_
   }
   Http::HttpServerPropertiesCacheManager& cache_manager =
       server_->httpServerPropertiesCacheManager();
-
-  Http::HttpServerPropertiesCacheManager::CacheFn clear_brokenness =
-      [](Http::HttpServerPropertiesCache& cache) { cache.resetBrokenness(); };
-  cache_manager.forEachThreadLocalCache(clear_brokenness);
   if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.quic_no_tcp_delay")) {
-    Http::HttpServerPropertiesCacheManager& cache_manager =
-        server_->httpServerPropertiesCacheManager();
-
+    // Reset HTTP/3 status for all origins.
     Http::HttpServerPropertiesCacheManager::CacheFn reset_status =
         [](Http::HttpServerPropertiesCache& cache) { cache.resetStatus(); };
     cache_manager.forEachThreadLocalCache(reset_status);
+  } else {
+    // Reset HTTP/3 status only for origins marked as broken.
+    Http::HttpServerPropertiesCacheManager::CacheFn clear_brokenness =
+        [](Http::HttpServerPropertiesCache& cache) { cache.resetBrokenness(); };
+    cache_manager.forEachThreadLocalCache(clear_brokenness);
   }
-  if (!disable_dns_refresh_on_network_change_) {
-    connectivity_manager_->refreshDns(configuration, /*drain_connections=*/true);
-  } else if (Runtime::runtimeFeatureEnabled(
-                 "envoy.reloadable_features.drain_pools_on_network_change")) {
-    ENVOY_LOG_EVENT(debug, "netconf_immediate_drain", "DrainAllHosts");
-    connectivity_manager_->clusterManager().drainConnections(
-        [](const Upstream::Host&) { return true; });
+
+  if (disable_dns_refresh_on_network_change_) {
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.drain_pools_on_network_change")) {
+      // Since DNS refreshing is disabled, explicitly drain all connections.
+      ENVOY_LOG_EVENT(debug, "netconf_immediate_drain", "DrainAllHosts");
+      getClusterManager().drainConnections([](const Upstream::Host&) { return true; });
+    }
+    return;
   }
+  // Refresh DNS upon network changes.
+  // This call will possibly drain all connections asynchronously.
+  connectivity_manager_->refreshDns(configuration, /*drain_connections=*/true);
 }
 
 envoy_status_t InternalEngine::recordCounterInc(absl::string_view elements, envoy_stats_tags tags,
diff --git a/mobile/library/common/network/connectivity_manager.cc b/mobile/library/common/network/connectivity_manager.cc
@@ -2,6 +2,8 @@
 
 #include <net/if.h>
 
+#include <memory>
+
 #include "envoy/common/platform.h"
 
 #include "source/common/api/os_sys_calls_impl.h"
@@ -79,11 +81,13 @@ constexpr unsigned int InitialFaultThreshold = 1;
 // L7 bytes) before switching socket mode.
 constexpr unsigned int MaxFaultThreshold = 3;
 
-ConnectivityManagerImpl::NetworkState ConnectivityManagerImpl::network_state_{
-    1, 0, MaxFaultThreshold, SocketMode::DefaultPreferredNetworkMode, Thread::MutexBasicLockable{}};
+ConnectivityManagerImpl::DefaultNetworkState ConnectivityManagerImpl::network_state_{
+    1, 0, MaxFaultThreshold, SocketMode::DefaultPreferredNetworkMode};
+
+Thread::MutexBasicLockable ConnectivityManagerImpl::network_mutex_{};
 
 envoy_netconf_t ConnectivityManagerImpl::setPreferredNetwork(int network) {
-  Thread::LockGuard lock{network_state_.mutex_};
+  Thread::LockGuard lock{network_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
@@ -120,17 +124,17 @@ void ConnectivityManagerImpl::setProxySettings(ProxySettingsConstSharedPtr new_p
 ProxySettingsConstSharedPtr ConnectivityManagerImpl::getProxySettings() { return proxy_settings_; }
 
 int ConnectivityManagerImpl::getPreferredNetwork() {
-  Thread::LockGuard lock{network_state_.mutex_};
+  Thread::LockGuard lock{network_mutex_};
   return network_state_.network_;
 }
 
 SocketMode ConnectivityManagerImpl::getSocketMode() {
-  Thread::LockGuard lock{network_state_.mutex_};
+  Thread::LockGuard lock{network_mutex_};
   return network_state_.socket_mode_;
 }
 
 envoy_netconf_t ConnectivityManagerImpl::getConfigurationKey() {
-  Thread::LockGuard lock{network_state_.mutex_};
+  Thread::LockGuard lock{network_mutex_};
   return network_state_.configuration_key_;
 }
 
@@ -152,7 +156,7 @@ void ConnectivityManagerImpl::reportNetworkUsage(envoy_netconf_t configuration_k
 
   bool configuration_updated = false;
   {
-    Thread::LockGuard lock{network_state_.mutex_};
+    Thread::LockGuard lock{network_mutex_};
 
     // If the configuration_key isn't current, don't do anything.
     if (configuration_key != network_state_.configuration_key_) {
@@ -200,40 +204,55 @@ void ConnectivityManagerImpl::reportNetworkUsage(envoy_netconf_t configuration_k
   }
 }
 
-void ConnectivityManagerImpl::onDnsResolutionComplete(
+void RefreshDnsWithPostDrainHandler::refreshDnsAndDrainHosts() {
+  Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dns_cache =
+      dns_cache_manager_->lookUpCacheByName(BaseDnsCache);
+  if (!dns_cache) {
+    // There may not be a DNS cache during initialization, but if one is available, it should always
+    // exist by the time this handler is instantiated from the NetworkConfigurationFilter.
+    ENVOY_LOG_EVENT(warn, "netconf_dns_cache_missing", "{}", std::string(BaseDnsCache));
+    return;
+  }
+  if (dns_callbacks_handle_ == nullptr) {
+    // Register callbacks once, on demand, using the handler as a sentinel.
+    dns_callbacks_handle_ = dns_cache->addUpdateCallbacks(*this);
+  }
+  dns_cache->iterateHostMap(
+      [&](absl::string_view host,
+          const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) {
+        hosts_to_drain_.emplace(host);
+      });
+
+  dns_cache->forceRefreshHosts();
+}
+
+void RefreshDnsWithPostDrainHandler::onDnsResolutionComplete(
     const std::string& resolved_host,
     const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&,
     Network::DnsResolver::ResolutionStatus) {
-  if (enable_drain_post_dns_refresh_) {
-    // Check if the set of hosts pending drain contains the current resolved host.
-    if (hosts_to_drain_.erase(resolved_host) == 0) {
-      return;
-    }
+  // Check if the set of hosts pending drain contains the current resolved host.
+  if (hosts_to_drain_.erase(resolved_host) == 0) {
+    return;
+  }
 
-    // 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", "{}", resolved_host);
+  // 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", "{}", resolved_host);
 
-    // Pass predicate to only drain connections to the resolved host (for any cluster).
-    cluster_manager_.drainConnections(
-        [resolved_host](const Upstream::Host& host) { return host.hostname() == resolved_host; });
-  }
+  // Pass predicate to only drain connections to the resolved host (for any cluster).
+  cluster_manager_.drainConnections(
+      [resolved_host](const Upstream::Host& host) { return host.hostname() == resolved_host; });
 }
 
 void ConnectivityManagerImpl::setDrainPostDnsRefreshEnabled(bool enabled) {
-  enable_drain_post_dns_refresh_ = enabled;
   if (!enabled) {
-    hosts_to_drain_.clear();
-  } 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);
-    }
+    dns_refresh_handler_ = nullptr;
+  } else if (!dns_refresh_handler_) {
+    dns_refresh_handler_ =
+        std::make_unique<RefreshDnsWithPostDrainHandler>(dns_cache_manager_, cluster_manager_);
   }
 }
 
@@ -244,7 +263,7 @@ void ConnectivityManagerImpl::setInterfaceBindingEnabled(bool enabled) {
 void ConnectivityManagerImpl::refreshDns(envoy_netconf_t configuration_key,
                                          bool drain_connections) {
   {
-    Thread::LockGuard lock{network_state_.mutex_};
+    Thread::LockGuard lock{network_mutex_};
 
     // refreshDns must be queued on Envoy's event loop, whereas network_state_ is updated
     // synchronously. In the event that multiple refreshes become queued on the event loop,
@@ -260,15 +279,11 @@ void ConnectivityManagerImpl::refreshDns(envoy_netconf_t configuration_key,
   if (auto dns_cache = dnsCache()) {
     ENVOY_LOG_EVENT(debug, "netconf_refresh_dns", "{}", std::to_string(configuration_key));
 
-    if (drain_connections && enable_drain_post_dns_refresh_) {
-      dns_cache->iterateHostMap(
-          [&](absl::string_view host,
-              const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) {
-            hosts_to_drain_.emplace(host);
-          });
+    if (drain_connections && (dns_refresh_handler_ != nullptr)) {
+      dns_refresh_handler_->refreshDnsAndDrainHosts();
+    } else {
+      dns_cache->forceRefreshHosts();
     }
-
-    dns_cache->forceRefreshHosts();
   }
 }
 
@@ -283,7 +298,7 @@ Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr ConnectivityManagerIm
 void ConnectivityManagerImpl::resetConnectivityState() {
   envoy_netconf_t configuration_key;
   {
-    Thread::LockGuard lock{network_state_.mutex_};
+    Thread::LockGuard lock{network_mutex_};
     network_state_.network_ = 0;
     network_state_.remaining_faults_ = 1;
     network_state_.socket_mode_ = SocketMode::DefaultPreferredNetworkMode;
@@ -359,7 +374,7 @@ ConnectivityManagerImpl::addUpstreamSocketOptions(Socket::OptionsSharedPtr optio
   SocketMode socket_mode;
 
   {
-    Thread::LockGuard lock{network_state_.mutex_};
+    Thread::LockGuard lock{network_mutex_};
     configuration_key = network_state_.configuration_key_;
     network = network_state_.network_;
     socket_mode = network_state_.socket_mode_;
diff --git a/mobile/library/common/network/connectivity_manager.h b/mobile/library/common/network/connectivity_manager.h
@@ -78,8 +78,7 @@ using InterfacePair = std::pair<const std::string, Address::InstanceConstSharedP
  * if that cache is missing either due to alternate configurations, or lifecycle-related timing.
  *
  */
-class ConnectivityManager
-    : public Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks {
+class ConnectivityManager {
 public:
   virtual ~ConnectivityManager() = default;
 
@@ -168,13 +167,10 @@ class ConnectivityManager
   virtual void resetConnectivityState() PURE;
 
   /**
-   * @returns the current socket options that should be used for connections.
-   */
-  virtual Socket::OptionsSharedPtr getUpstreamSocketOptions(int network,
-                                                            SocketMode socket_mode) PURE;
-
-  /**
-   * @param options, upstream connection options to which additional options should be appended.
+   * Add socket options to be applied to the upstream connection which could
+   * potentially affect which network interface the requests will be sent on.
+   * @param options, upstream connection options to which additional options relate to the current
+   * network states should be appended.
    * @returns configuration key to associate with any related calls.
    */
   virtual envoy_netconf_t addUpstreamSocketOptions(Socket::OptionsSharedPtr options) PURE;
@@ -187,11 +183,38 @@ class ConnectivityManager
    * @returns the default DNS cache set up in base configuration or nullptr.
    */
   virtual Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dnsCache() PURE;
+};
 
-  /**
-   * Returns the cluster manager for this Envoy Mobile instance
-   */
-  virtual Upstream::ClusterManager& clusterManager() PURE;
+// Used when draining hosts upon DNS refreshing is desired.
+class RefreshDnsWithPostDrainHandler
+    : public Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks,
+      public Logger::Loggable<Logger::Id::upstream> {
+public:
+  RefreshDnsWithPostDrainHandler(DnsCacheManagerSharedPtr dns_cache_manager,
+                                 Upstream::ClusterManager& cluster_manager)
+      : dns_cache_manager_(std::move(dns_cache_manager)), cluster_manager_(cluster_manager) {}
+
+  // Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks
+  absl::Status onDnsHostAddOrUpdate(
+      const std::string& /*host*/,
+      const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override {
+    return absl::OkStatus();
+  }
+  void onDnsHostRemove(const std::string& /*host*/) override {}
+  void onDnsResolutionComplete(const std::string& /*host*/,
+                               const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&,
+                               Network::DnsResolver::ResolutionStatus) override;
+
+  // Refresh DNS and drain all hosts upon completion.
+  // No-op if the default DNS cache in base configuration is not available.
+  void refreshDnsAndDrainHosts();
+
+private:
+  DnsCacheManagerSharedPtr dns_cache_manager_;
+  Upstream::ClusterManager& cluster_manager_;
+  absl::flat_hash_set<std::string> hosts_to_drain_;
+  Extensions::Common::DynamicForwardProxy::DnsCache::AddUpdateCallbacksHandlePtr
+      dns_callbacks_handle_;
 };
 
 class ConnectivityManagerImpl : public ConnectivityManager,
@@ -210,17 +233,6 @@ class ConnectivityManagerImpl : public ConnectivityManager,
                           DnsCacheManagerSharedPtr dns_cache_manager)
       : cluster_manager_(cluster_manager), dns_cache_manager_(dns_cache_manager) {}
 
-  // Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks
-  absl::Status onDnsHostAddOrUpdate(
-      const std::string& /*host*/,
-      const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override {
-    return absl::OkStatus();
-  }
-  void onDnsHostRemove(const std::string& /*host*/) override {}
-  void onDnsResolutionComplete(const std::string& /*host*/,
-                               const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&,
-                               Network::DnsResolver::ResolutionStatus) override;
-
   // ConnectivityManager
   std::vector<InterfacePair> enumerateV4Interfaces() override;
   std::vector<InterfacePair> enumerateV6Interfaces() override;
@@ -236,33 +248,31 @@ class ConnectivityManagerImpl : public ConnectivityManager,
   void setInterfaceBindingEnabled(bool enabled) override;
   void refreshDns(envoy_netconf_t configuration_key, bool drain_connections) override;
   void resetConnectivityState() override;
-  Socket::OptionsSharedPtr getUpstreamSocketOptions(int network, SocketMode socket_mode) override;
   envoy_netconf_t addUpstreamSocketOptions(Socket::OptionsSharedPtr options) override;
   Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dnsCache() override;
-  Upstream::ClusterManager& clusterManager() override { return cluster_manager_; }
 
 private:
-  struct NetworkState {
+  // The states of the current default network picked by the platform.
+  struct DefaultNetworkState {
     // The configuration key is passed through calls dispatched on the run loop to determine if
     // they're still valid/relevant at time of execution.
-    envoy_netconf_t configuration_key_ ABSL_GUARDED_BY(mutex_);
-    int network_ ABSL_GUARDED_BY(mutex_);
-    uint8_t remaining_faults_ ABSL_GUARDED_BY(mutex_);
-    SocketMode socket_mode_ ABSL_GUARDED_BY(mutex_);
-    Thread::MutexBasicLockable mutex_;
+    envoy_netconf_t configuration_key_;
+    int network_;
+    uint8_t remaining_faults_;
+    SocketMode socket_mode_;
   };
   Socket::OptionsSharedPtr getAlternateInterfaceSocketOptions(int network);
   InterfacePair getActiveAlternateInterface(int network, unsigned short family);
+  Socket::OptionsSharedPtr getUpstreamSocketOptions(int network, SocketMode socket_mode);
 
-  bool enable_drain_post_dns_refresh_{false};
   bool enable_interface_binding_{false};
-  absl::flat_hash_set<std::string> hosts_to_drain_;
-  Extensions::Common::DynamicForwardProxy::DnsCache::AddUpdateCallbacksHandlePtr
-      dns_callbacks_handle_{nullptr};
   Upstream::ClusterManager& cluster_manager_;
+  // nullptr if draining hosts after refreshing DNS is disabled via setDrainPostDnsRefreshEnabled().
+  std::unique_ptr<RefreshDnsWithPostDrainHandler> dns_refresh_handler_;
   DnsCacheManagerSharedPtr dns_cache_manager_;
   ProxySettingsConstSharedPtr proxy_settings_;
-  static NetworkState network_state_;
+  static DefaultNetworkState network_state_ ABSL_GUARDED_BY(network_mutex_);
+  static Thread::MutexBasicLockable network_mutex_;
 };
 
 using ConnectivityManagerSharedPtr = std::shared_ptr<ConnectivityManager>;
diff --git a/mobile/test/common/network/connectivity_manager_test.cc b/mobile/test/common/network/connectivity_manager_test.cc
