plumb observer registry

Signed-off-by: Dan Zhang <danzh@google.com>

changelogs/current.yaml

@@ -51,10 +51,6 @@ minor_behavior_changes:
   change: |
     HTTP/3 alt-svc headers will now be respected from IP-address-based hostnames. This change is
     guarded by runtime guard ``envoy.reloadable_features.allow_alt_svc_for_ips``.
-- area: http3
-  change: |
-    Make upstream connections respond to default network change on mobile devices. This change is guarded
-    by runtime guard ``envoy.reloadable_features.quic_upstream_connection_handle_network_change``.
 - area: lua
   change: |
     When Lua script executes httpCall, backpressure is exercised when receiving body from downstream client. This behavior can be reverted

envoy/upstream/cluster_manager.h

@@ -41,14 +41,8 @@ namespace Envoy {
 
 namespace Quic {
 
-#ifdef ENVOY_ENABLE_QUIC
 class EnvoyQuicNetworkObserverRegistryFactory;
 class EnvoyQuicNetworkObserverRegistry;
-#else
-// Dumb definitions of QUIC classes if QUICHE is compiled out.
-class EnvoyQuicNetworkObserverRegistryFactory {};
-class EnvoyQuicNetworkObserverRegistry {};
-#endif
 
 } // namespace Quic
 

mobile/library/common/internal_engine.cc

@@ -283,7 +283,8 @@ envoy_status_t InternalEngine::resetConnectivityState() {
 
 envoy_status_t InternalEngine::setPreferredNetwork(NetworkType network) {
   return dispatcher_->post([&, network]() -> void {
-    envoy_netconf_t configuration_key = connectivity_manager_->onNetworkMadeDefault(network);
+    envoy_netconf_t configuration_key =
+        Network::ConnectivityManagerImpl::setPreferredNetwork(network);
     if (Runtime::runtimeFeatureEnabled(
             "envoy.reloadable_features.dns_cache_set_ip_version_to_remove")) {
       // The IP version to remove flag must be set first before refreshing the DNS cache so that

mobile/library/common/network/BUILD

@@ -1,19 +1,9 @@
-load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_mobile_package", "envoy_select_enable_http3")
+load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_mobile_package")
 
 licenses(["notice"])  # Apache 2
 
 envoy_mobile_package()
 
-envoy_cc_library(
-    name = "envoy_mobile_quic_network_observer_registry_factory_lib",
-    srcs = ["envoy_mobile_quic_network_observer_registry_factory.cc"],
-    hdrs = ["envoy_mobile_quic_network_observer_registry_factory.h"],
-    repository = "@envoy",
-    deps = [
-        "@envoy//source/common/quic:envoy_quic_client_session_lib",
-    ],
-)
-
 envoy_cc_library(
     name = "connectivity_manager_lib",
     srcs = [
@@ -35,12 +25,7 @@ envoy_cc_library(
         "@envoy//source/common/network:addr_family_aware_socket_option_lib",
         "@envoy//source/common/network:socket_option_lib",
         "@envoy//source/extensions/common/dynamic_forward_proxy:dns_cache_manager_impl",
-    ] + envoy_select_enable_http3(
-        [
-            ":envoy_mobile_quic_network_observer_registry_factory_lib",
-        ],
-        "@envoy",
-    ),
+    ],
 )
 
 envoy_cc_library(

mobile/library/common/network/connectivity_manager.cc

@@ -77,20 +77,6 @@ constexpr unsigned int InitialFaultThreshold = 1;
 // L7 bytes) before switching socket mode.
 constexpr unsigned int MaxFaultThreshold = 3;
 
-ConnectivityManagerImpl::ConnectivityManagerImpl(Upstream::ClusterManager& cluster_manager,
-                                                 DnsCacheManagerSharedPtr dns_cache_manager)
-    : cluster_manager_(cluster_manager), dns_cache_manager_(dns_cache_manager),
-      quic_upstream_connection_handle_network_change_(Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.quic_upstream_connection_handle_network_change")) {
-#ifdef ENVOY_ENABLE_QUIC
-  quic_observer_registry_factory_ =
-      std::make_unique<Quic::EnvoyMobileQuicNetworkObserverRegistryFactory>();
-  if (quic_upstream_connection_handle_network_change_) {
-    cluster_manager_.createNetworkObserverRegistries(*quic_observer_registry_factory_);
-  }
-#endif
-}
-
 ConnectivityManagerImpl::NetworkState ConnectivityManagerImpl::network_state_{
     1, NetworkType::Generic, MaxFaultThreshold, SocketMode::DefaultPreferredNetworkMode,
     Thread::MutexBasicLockable{}};
@@ -116,18 +102,6 @@ envoy_netconf_t ConnectivityManagerImpl::setPreferredNetwork(NetworkType network
   return network_state_.configuration_key_;
 }
 
-envoy_netconf_t ConnectivityManagerImpl::onNetworkMadeDefault(NetworkType network) {
-  ENVOY_LOG_MISC(trace, "Default network changed to {}", static_cast<int>(network));
-  envoy_netconf_t configuration_key = setPreferredNetwork(network);
-#ifdef ENVOY_ENABLE_QUIC
-  for (std::reference_wrapper<Quic::EnvoyMobileQuicNetworkObserverRegistry> registry :
-       quic_observer_registry_factory_->getCreatedObserverRegistries()) {
-    registry.get().onNetworkMadeDefault();
-  }
-#endif
-  return configuration_key;
-}
-
 void ConnectivityManagerImpl::setProxySettings(ProxySettingsConstSharedPtr new_proxy_settings) {
   if (proxy_settings_ == nullptr && new_proxy_settings != nullptr) {
     ENVOY_LOG_EVENT(info, "netconf_proxy_change", "{}", new_proxy_settings->asString());
@@ -331,17 +305,16 @@ Socket::OptionsSharedPtr ConnectivityManagerImpl::getUpstreamSocketOptions(Netwo
       network != NetworkType::Generic) {
     return getAlternateInterfaceSocketOptions(network);
   }
+
+  // Envoy uses the hash signature of overridden socket options to choose a connection pool.
+  // Setting a dummy socket option is a hack that allows us to select a different
+  // connection pool without materially changing the socket configuration.
   ASSERT(static_cast<int>(network) >= 0 && static_cast<int>(network) < 3);
+  int ttl_value = DEFAULT_IP_TTL + static_cast<int>(network);
   auto options = std::make_shared<Socket::Options>();
-  if (!quic_upstream_connection_handle_network_change_) {
-    // Envoy uses the hash signature of overridden socket options to choose a connection pool.
-    // Setting a dummy socket option is a hack that allows us to select a different
-    // connection pool without materially changing the socket configuration.
-    int ttl_value = DEFAULT_IP_TTL + static_cast<int>(network);
-    options->push_back(std::make_shared<AddrFamilyAwareSocketOptionImpl>(
-        envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_SOCKET_IP_TTL,
-        ENVOY_SOCKET_IPV6_UNICAST_HOPS, ttl_value));
-  }
+  options->push_back(std::make_shared<AddrFamilyAwareSocketOptionImpl>(
+      envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_SOCKET_IP_TTL,
+      ENVOY_SOCKET_IPV6_UNICAST_HOPS, ttl_value));
   return options;
 }
 

mobile/library/common/network/connectivity_manager.h

@@ -14,10 +14,6 @@
 #include "library/common/network/proxy_settings.h"
 #include "library/common/types/c_types.h"
 
-#ifdef ENVOY_ENABLE_QUIC
-#include "library/common/network/envoy_mobile_quic_network_observer_registry_factory.h"
-#endif
-
 /**
  * envoy_netconf_t identifies a snapshot of network configuration state. It's returned from calls
  * that may alter current state, and passed back as a parameter to this API to determine if calls
@@ -190,13 +186,6 @@ class ConnectivityManager
    * @returns the default DNS cache set up in base configuration or nullptr.
    */
   virtual Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dnsCache() PURE;
-
-  /**
-   * Called when OS changes the preferred network.
-   * @param network, the OS-preferred network.
-   * @returns configuration key of the latest snapshot of network configuration state.
-   */
-  virtual envoy_netconf_t onNetworkMadeDefault(NetworkType network) PURE;
 };
 
 class ConnectivityManagerImpl : public ConnectivityManager,
@@ -212,7 +201,8 @@ class ConnectivityManagerImpl : public ConnectivityManager,
   static envoy_netconf_t setPreferredNetwork(NetworkType network);
 
   ConnectivityManagerImpl(Upstream::ClusterManager& cluster_manager,
-                          DnsCacheManagerSharedPtr dns_cache_manager);
+                          DnsCacheManagerSharedPtr dns_cache_manager)
+      : cluster_manager_(cluster_manager), dns_cache_manager_(dns_cache_manager) {}
 
   // Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks
   void onDnsHostAddOrUpdate(
@@ -242,7 +232,6 @@ class ConnectivityManagerImpl : public ConnectivityManager,
                                                     SocketMode socket_mode) override;
   envoy_netconf_t addUpstreamSocketOptions(Socket::OptionsSharedPtr options) override;
   Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dnsCache() override;
-  envoy_netconf_t onNetworkMadeDefault(NetworkType network) override;
 
 private:
   struct NetworkState {
@@ -263,14 +252,9 @@ class ConnectivityManagerImpl : public ConnectivityManager,
   Extensions::Common::DynamicForwardProxy::DnsCache::AddUpdateCallbacksHandlePtr
       dns_callbacks_handle_{nullptr};
   Upstream::ClusterManager& cluster_manager_;
-#ifdef ENVOY_ENABLE_QUIC
-  std::unique_ptr<Quic::EnvoyMobileQuicNetworkObserverRegistryFactory>
-      quic_observer_registry_factory_;
-#endif
   DnsCacheManagerSharedPtr dns_cache_manager_;
   ProxySettingsConstSharedPtr proxy_settings_;
   static NetworkState network_state_;
-  const bool quic_upstream_connection_handle_network_change_;
 };
 
 using ConnectivityManagerSharedPtr = std::shared_ptr<ConnectivityManager>;

mobile/test/common/extensions/filters/http/network_configuration/network_configuration_filter_test.cc

@@ -57,7 +57,6 @@ class MockConnectivityManager : public Network::ConnectivityManager {
                const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&,
                Network::DnsResolver::ResolutionStatus));
   MOCK_METHOD(Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr, dnsCache, ());
-  MOCK_METHOD(envoy_netconf_t, onNetworkMadeDefault, (NetworkType network));
 };
 
 class NetworkConfigurationFilterTest : public testing::Test {

mobile/test/common/integration/client_integration_test.cc

@@ -1337,16 +1337,12 @@ TEST_P(ClientIntegrationTest, TestProxyResolutionApi) {
 // This test is simply to test the IPv6 connectivity check and DNS refresh and make sure the code
 // doesn't crash. It doesn't really test the actual network change event.
 TEST_P(ClientIntegrationTest, OnNetworkChanged) {
-  builder_.addRuntimeGuard("quic_upstream_connection_handle_network_change", true);
+  builder_.addRuntimeGuard("dns_cache_set_ip_version_to_remove", true);
   initialize();
-  basicTest();
   internalEngine()->setPreferredNetwork(NetworkType::WLAN);
+  basicTest();
   if (upstreamProtocol() == Http::CodecType::HTTP1) {
     ASSERT_EQ(cc_.on_complete_received_byte_count_, 67);
-  } else if (upstreamProtocol() == Http::CodecType::HTTP3) {
-    ASSERT_TRUE(waitForCounterGe("http3.upstream.tx.quic_connection_close_error_code_QUIC_"
-                                 "CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS",
-                                 1));
   }
 }
 

mobile/test/common/network/connectivity_manager_test.cc

@@ -1,7 +1,6 @@
 #include <net/if.h>
 
 #include "test/extensions/common/dynamic_forward_proxy/mocks.h"
-#include "test/mocks/event/mocks.h"
 #include "test/mocks/upstream/cluster_manager.h"
 
 #include "gtest/gtest.h"
@@ -49,26 +48,6 @@ TEST_F(ConnectivityManagerTest,
   EXPECT_EQ(original_key, connectivity_manager_->getConfigurationKey());
 }
 
-#ifdef ENVOY_ENABLE_QUIC
-TEST_F(ConnectivityManagerTest, OnNetworkMadeDefault) {
-  Runtime::maybeSetRuntimeGuard(
-      "envoy.reloadable_features.quic_upstream_connection_handle_network_change", true);
-  Quic::EnvoyMobileQuicNetworkObserverRegistryPtr registry;
-  Event::MockDispatcher dispatcher;
-  EXPECT_CALL(cm_, createNetworkObserverRegistries(_))
-      .WillOnce(Invoke([&](Quic::EnvoyQuicNetworkObserverRegistryFactory& factory) {
-        registry.reset(static_cast<Quic::EnvoyMobileQuicNetworkObserverRegistry*>(
-            factory.createQuicNetworkObserverRegistry(dispatcher).release()));
-      }));
-  connectivity_manager_ = std::make_shared<ConnectivityManagerImpl>(cm_, dns_cache_manager_);
-  EXPECT_NE(nullptr, registry);
-  envoy_netconf_t original_key = connectivity_manager_->getConfigurationKey();
-  EXPECT_CALL(dispatcher, post(_));
-  envoy_netconf_t new_key = connectivity_manager_->onNetworkMadeDefault(NetworkType::WWAN);
-  EXPECT_NE(original_key, new_key);
-}
-#endif
-
 TEST_F(ConnectivityManagerTest, RefreshDnsForCurrentConfigurationTriggersDnsRefresh) {
   EXPECT_CALL(*dns_cache_, forceRefreshHosts());
   envoy_netconf_t configuration_key = connectivity_manager_->getConfigurationKey();

mobile/test/java/org/chromium/net/CronetHttp3Test.java

@@ -3,9 +3,6 @@
 import static org.chromium.net.testing.CronetTestRule.getContext;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.Matchers.anyOf;
 
 import io.envoyproxy.envoymobile.engine.types.EnvoyNetworkType;
 import org.chromium.net.impl.CronvoyUrlRequestContext;
@@ -96,8 +93,6 @@ public void log(int logLevel, String message) {
     NativeCronvoyEngineBuilderImpl nativeCronetEngineBuilder =
         new NativeCronvoyEngineBuilderImpl(ApplicationProvider.getApplicationContext());
     nativeCronetEngineBuilder.addRuntimeGuard("reset_brokenness_on_nework_change", true);
-    nativeCronetEngineBuilder.addRuntimeGuard("quic_upstream_connection_handle_network_change",
-                                              true);
     if (setUpLogging) {
       nativeCronetEngineBuilder.setLogger(logger);
       nativeCronetEngineBuilder.setLogLevel(EnvoyEngine.LogLevel.TRACE);
@@ -292,105 +287,13 @@ public void networkChangeAffectsBrokenness() throws Exception {
     String preStats = cronvoyEngine.getEnvoyEngine().dumpStats();
     assertTrue(preStats.contains("cluster.base.upstream_cx_http3_total: 1"));
 
-    // Even though there is HTTP/3 connection, the follow request should go over HTTP/2 because
-    // HTTP/3 is marked as broken.
-    TestUrlRequestCallback get_callback = doBasicGetRequest();
-    assertEquals(200, get_callback.mResponseInfo.getHttpStatusCode());
-    // Verify the request used HTTP/2.
-    assertEquals("h2", get_callback.mResponseInfo.getNegotiatedProtocol());
-
-    // This should change QUIC brokenness to "failed recently" and close the HTTP/3 connection
-    // asynchronously.
-    cronvoyEngine.getEnvoyEngine().setPreferredNetwork(EnvoyNetworkType.WLAN);
-
-    // Send a new POST request which may re-use the HTTP/3 connection if it hasn't been closed yet.
-    TestUrlRequestCallback post_callback = doBasicPostRequest();
-    String postStats = cronvoyEngine.getEnvoyEngine().dumpStats();
-    if (postStats.contains("cluster.base.upstream_cx_http3_total: 2")) {
-      // The 1st HTTP/3 connection has been closed before processing the next request. The next
-      // request may go out over HTTP/2 or HTTP/3 (depends on who wins the race) but HTTP/3 will be
-      // tried.
-      assertTrue(
-          postStats,
-          postStats.contains(
-              "http3.upstream.tx.quic_connection_close_error_code_QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS: 1"));
-    } else {
-      // The 1st HTTP/3 connection has not been closed. And the next request will go over it.
-      assertTrue(postStats.contains("cluster.base.upstream_cx_http3_total: 1"));
-      assertEquals("h3", post_callback.mResponseInfo.getNegotiatedProtocol());
-
-      // The network change will still drain the connection.
-      assertTrue(postStats,
-                 postStats.contains(
-                     "http3.upstream.tx.quic_connection_close_error_code_QUIC_NO_ERROR: 1"));
-    }
-  }
-
-  @Test
-  @SmallTest
-  @Feature({"Cronet"})
-  public void ConnectionGoAwayUponNetworkChange() throws Exception {
-    setUp(true);
-    cronvoyEngine.getEnvoyEngine().setPreferredNetwork(EnvoyNetworkType.WWAN);
-
-    // Do the initial HTTP/2 request to get the alt-svc response.
-    doInitialHttp2Request();
-    // Set up a second request, which will hopefully go out over HTTP/3 due to alt-svc
-    // advertisement.
-    TestUrlRequestCallback callback = doBasicPostRequest();
-    // Verify the second request used HTTP/3
-    assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
-    assertEquals("h3", callback.mResponseInfo.getNegotiatedProtocol());
-
-    // From prior calls, there was one HTTP/3 connection established. Following requests should be
-    // able to use it. Send a POST request without waiting for response.
-    TestUrlRequestCallback post_callback = new TestUrlRequestCallback();
-    ExperimentalUrlRequest.Builder urlRequestBuilder = cronvoyEngine.newUrlRequestBuilder(
-        testServerUrl, post_callback, post_callback.getExecutor());
-    urlRequestBuilder.addHeader("content-type", "text");
-    urlRequestBuilder.addHeader("no_end_stream", "yes");
-    urlRequestBuilder.setHttpMethod("POST");
-    urlRequestBuilder.setIdempotency(ExperimentalUrlRequest.Builder.IDEMPOTENT);
-    TestUploadDataProvider dataProvider = new TestUploadDataProvider(
-        TestUploadDataProvider.SuccessCallbackMode.SYNC, post_callback.getExecutor());
-    dataProvider.addRead("test".getBytes());
-    urlRequestBuilder.setUploadDataProvider(dataProvider, post_callback.getExecutor());
-    urlRequestBuilder.build().start();
-
-    String preStats = cronvoyEngine.getEnvoyEngine().dumpStats();
-    assertTrue(preStats, preStats.contains("cluster.base.upstream_cx_http3_total: 1"));
-
-    // Trigger a network change which may race with the on-going POST request which should by all
-    // means be able to finish.
+    // This should change QUIC brokenness to "failed recently".
     cronvoyEngine.getEnvoyEngine().setPreferredNetwork(EnvoyNetworkType.WLAN);
 
-    // Finish the 2nd POST request.
-    post_callback.blockForDone();
-    assertEquals(200, post_callback.mResponseInfo.getHttpStatusCode());
-    // Verify the request used HTTP/3
-    assertEquals("h3", post_callback.mResponseInfo.getNegotiatedProtocol());
-
-    // Set up another 2 GET requests. At least the 2nd one should go over a new connection either H2
-    // or H3 as the brokenness status has been reset. The 1st one may race with the network change
-    // event.
-    for (int i = 0; i < 2; ++i) {
-      TestUrlRequestCallback get_callback = doBasicGetRequest();
-      assertEquals(200, get_callback.mResponseInfo.getHttpStatusCode());
-    }
-
+    // The next request may go out over HTTP/2 or HTTP/3 (depends on who wins the race)
+    // but HTTP/3 will be tried.
+    doBasicGetRequest();
     String postStats = cronvoyEngine.getEnvoyEngine().dumpStats();
-    // Another HTTP/3 connection should have been attempted in any case.
     assertTrue(postStats.contains("cluster.base.upstream_cx_http3_total: 2"));
-    // The 1st connection should have been closed with QUIC_NO_ERROR if network change was
-    // propagated to the Envoy thread when the 2nd POST request or the following GET request was
-    // in-flight, with QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS if the network change was
-    // propagated to the Envoy thread after the POST request finished and before the follow GET was
-    // processed when the connection was idle.
-    assertThat(
-        postStats,
-        anyOf(
-            containsString("http3.upstream.tx.quic_connection_close_error_code_QUIC_NO_ERROR: 1"),
-            containsString(
-                "http3.upstream.tx.quic_connection_close_error_code_QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS: 1")));
   }
 }