test: adding a multi-envoy test (envoyproxy#20016)

Functionally this handles the multi-envoy signal handler crash skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags envoyproxy#19847 is closed) Multi-envoy does not correctly support runtime flags or deprecation stats due to envoyproxy#19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test. Risk Level: low Testing: yes Docs Changes: n/a Release Notes: n/a Part of envoyproxy/envoy-mobile#2003 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]>
rexnp · Mar 2, 2022 · d63fec0 · d63fec0
1 parent c654d63
commit d63fec0
Show file tree

Hide file tree

Showing 13 changed files with 163 additions and 56 deletions.
diff --git a/source/common/quic/active_quic_listener.cc b/source/common/quic/active_quic_listener.cc
@@ -59,9 +59,7 @@ ActiveQuicListener::ActiveQuicListener(
   ASSERT(GetQuicReloadableFlag(quic_single_ack_in_packet2));
   ASSERT(!GetQuicFlag(FLAGS_quic_header_size_limit_includes_overhead));
 
-  if (Runtime::LoaderSingleton::getExisting()) {
-    enabled_.emplace(Runtime::FeatureFlag(enabled, runtime));
-  }
+  enabled_.emplace(Runtime::FeatureFlag(enabled, runtime));
 
   quic::QuicRandom* const random = quic::QuicRandom::GetInstance();
   random->RandBytes(random_seed_, sizeof(random_seed_));

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -63,6 +63,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_test_feature_false);
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiple_dns_addresses);
 // TODO(adisuissa) reset to true to enable unified mux by default
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_unified_mux);
+// TODO(alyssar) flip false once issue complete.
+FALSE_RUNTIME_GUARD(envoy_restart_features_no_runtime_singleton);
 
 // Block of non-boolean flags. These are deprecated. Do not add more.
 ABSL_FLAG(uint64_t, envoy_buffer_overflow_multiplier, 0, "");                        // NOLINT
@@ -169,6 +171,7 @@ constexpr absl::Flag<bool>* runtime_features[] = {
   &FLAGS_envoy_reloadable_features_validate_connect,
   &FLAGS_envoy_restart_features_explicit_wildcard_resource,
   &FLAGS_envoy_restart_features_use_apple_api_for_dns_lookups,
+  &FLAGS_envoy_restart_features_no_runtime_singleton,
 };
 // clang-format on
 

diff --git a/source/common/signal/fatal_error_handler.cc b/source/common/signal/fatal_error_handler.cc
@@ -153,12 +153,10 @@ void registerFatalActions(FatalAction::FatalActionPtrList safe_actions,
                           FatalAction::FatalActionPtrList unsafe_actions,
                           Thread::ThreadFactory& thread_factory) {
   // Create a FatalActionManager and store it.
-  FatalAction::FatalActionManager* previous_manager =
-      fatal_action_manager.exchange(new FatalAction::FatalActionManager(
-          std::move(safe_actions), std::move(unsafe_actions), thread_factory));
-
-  // Previous manager should be NULL.
-  ASSERT(!previous_manager);
+  if (!fatal_action_manager) {
+    fatal_action_manager.exchange(new FatalAction::FatalActionManager(
+        std::move(safe_actions), std::move(unsafe_actions), thread_factory));
+  }
 }
 
 FatalAction::Status runSafeActions() { return runFatalActions(FatalActionType::Safe); }

diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h
@@ -80,6 +80,8 @@ template <class T> class ScopedInjectableLoader {
   }
   ~ScopedInjectableLoader() { InjectableSingleton<T>::clear(); }
 
+  T& instance() { return *instance_; }
+
 private:
   std::unique_ptr<T> instance_;
 };

diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h
@@ -91,7 +91,7 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
   ServerLifecycleNotifier& lifecycleNotifier() override { return *this; }
   ListenerManager& listenerManager() override { return *listener_manager_; }
   Secret::SecretManager& secretManager() override { return *secret_manager_; }
-  Runtime::Loader& runtime() override { return Runtime::LoaderSingleton::get(); }
+  Runtime::Loader& runtime() override { return runtime_singleton_->instance(); }
   void shutdown() override;
   bool isShutdown() override { return false; }
   void shutdownAdmin() override {}

diff --git a/source/server/server.cc b/source/server/server.cc
@@ -619,8 +619,12 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add
 
   // Runtime gets initialized before the main configuration since during main configuration
   // load things may grab a reference to the loader for later use.
-  runtime_singleton_ = std::make_unique<Runtime::ScopedLoaderSingleton>(
-      component_factory.createRuntime(*this, initial_config));
+  Runtime::LoaderPtr runtime_ptr = component_factory.createRuntime(*this, initial_config);
+  if (runtime_ptr->snapshot().getBoolean("envoy.restart_features.no_runtime_singleton", false)) {
+    runtime_ = std::move(runtime_ptr);
+  } else {
+    runtime_singleton_ = std::make_unique<Runtime::ScopedLoaderSingleton>(std::move(runtime_ptr));
+  }
   initial_config.initAdminAccessLog(bootstrap_, *this);
   validation_context_.setRuntime(runtime());
 
@@ -648,10 +652,10 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add
       dns_resolver_factory.createDnsResolver(dispatcher(), api(), typed_dns_resolver_config);
 
   cluster_manager_factory_ = std::make_unique<Upstream::ProdClusterManagerFactory>(
-      *admin_, Runtime::LoaderSingleton::get(), stats_store_, thread_local_, dns_resolver_,
-      *ssl_context_manager_, *dispatcher_, *local_info_, *secret_manager_,
-      messageValidationContext(), *api_, http_context_, grpc_context_, router_context_,
-      access_log_manager_, *singleton_manager_, options_, quic_stat_names_);
+      *admin_, runtime(), stats_store_, thread_local_, dns_resolver_, *ssl_context_manager_,
+      *dispatcher_, *local_info_, *secret_manager_, messageValidationContext(), *api_,
+      http_context_, grpc_context_, router_context_, access_log_manager_, *singleton_manager_,
+      options_, quic_stat_names_);
 
   // Now the configuration gets parsed. The configuration may start setting
   // thread local data per above. See MainImpl::initialize() for why ConfigImpl
@@ -675,7 +679,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add
 
   // We have to defer RTDS initialization until after the cluster manager is
   // instantiated (which in turn relies on runtime...).
-  Runtime::LoaderSingleton::get().initialize(clusterManager());
+  runtime().initialize(clusterManager());
 
   clusterManager().setPrimaryClustersInitializedCb(
       [this]() { onClusterManagerPrimaryInitializationComplete(); });
@@ -706,7 +710,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add
 
 void InstanceImpl::onClusterManagerPrimaryInitializationComplete() {
   // If RTDS was not configured the `onRuntimeReady` callback is immediately invoked.
-  Runtime::LoaderSingleton::get().startRtdsSubscriptions([this]() { onRuntimeReady(); });
+  runtime().startRtdsSubscriptions([this]() { onRuntimeReady(); });
 }
 
 void InstanceImpl::onRuntimeReady() {
@@ -730,8 +734,8 @@ void InstanceImpl::onRuntimeReady() {
           Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config,
                                                          stats_store_, false)
               ->createUncachedRawAsyncClient(),
-          *dispatcher_, Runtime::LoaderSingleton::get(), stats_store_, *ssl_context_manager_,
-          info_factory_, access_log_manager_, *config_.clusterManager(), *local_info_, *admin_,
+          *dispatcher_, runtime(), stats_store_, *ssl_context_manager_, info_factory_,
+          access_log_manager_, *config_.clusterManager(), *local_info_, *admin_,
           *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(),
           *api_, options_);
     }
@@ -931,7 +935,12 @@ void InstanceImpl::terminate() {
   FatalErrorHandler::clearFatalActionsOnTerminate();
 }
 
-Runtime::Loader& InstanceImpl::runtime() { return Runtime::LoaderSingleton::get(); }
+Runtime::Loader& InstanceImpl::runtime() {
+  if (runtime_singleton_) {
+    return runtime_singleton_->instance();
+  }
+  return *runtime_;
+}
 
 void InstanceImpl::shutdown() {
   ENVOY_LOG(info, "shutting down server instance");

diff --git a/source/server/server.h b/source/server/server.h
@@ -357,6 +357,7 @@ class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
   Singleton::ManagerPtr singleton_manager_;
   Network::ConnectionHandlerPtr handler_;
   std::unique_ptr<Runtime::ScopedLoaderSingleton> runtime_singleton_;
+  std::unique_ptr<Runtime::Loader> runtime_;
   std::unique_ptr<Ssl::ContextManager> ssl_context_manager_;
   ProdListenerComponentFactory listener_component_factory_;
   ProdWorkerFactory worker_factory_;

diff --git a/test/common/signal/fatal_action_test.cc b/test/common/signal/fatal_action_test.cc
@@ -72,13 +72,6 @@ TEST_F(FatalActionTest, ShouldNotBeAbleToRunActionsBeforeRegistration) {
   EXPECT_EQ(FatalErrorHandler::runUnsafeActions(), Status::ActionManagerUnset);
 }
 
-TEST_F(FatalActionTest, ShouldOnlyBeAbleToRegisterFatalActionsOnce) {
-  // Register empty list of actions
-  FatalErrorHandler::registerFatalActions({}, {}, Thread::threadFactoryForTest());
-  EXPECT_DEBUG_DEATH(
-      { FatalErrorHandler::registerFatalActions({}, {}, Thread::threadFactoryForTest()); }, "");
-}
-
 TEST_F(FatalActionTest, CanCallRegisteredActions) {
   // Set up Fatal Actions
   safe_actions_.emplace_back(std::make_unique<TestFatalAction>(true, &counter_));

diff --git a/test/integration/BUILD b/test/integration/BUILD
@@ -554,6 +554,16 @@ envoy_cc_test(
     ],
 )
 
+envoy_cc_test(
+    name = "multi_envoy_test",
+    srcs = [
+        "multi_envoy_test.cc",
+    ],
+    deps = [
+        ":http_protocol_integration_lib",
+    ],
+)
+
 envoy_cc_test(
     name = "multiplexed_upstream_integration_test",
     srcs = [

diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc
@@ -157,19 +157,14 @@ void BaseIntegrationTest::createUpstreams() {
   }
 }
 
-void BaseIntegrationTest::createEnvoy() {
-  std::vector<uint32_t> ports;
-  for (auto& upstream : fake_upstreams_) {
-    if (upstream->localAddress()->ip()) {
-      ports.push_back(upstream->localAddress()->ip()->port());
-    }
-  }
-
-  if (use_lds_) {
+std::string BaseIntegrationTest::finalizeConfigWithPorts(ConfigHelper& config_helper,
+                                                         std::vector<uint32_t>& ports,
+                                                         bool use_lds) {
+  if (use_lds) {
     ENVOY_LOG_MISC(debug, "Setting up file-based LDS");
     // Before finalization, set up a real lds path, replacing the default /dev/null
     std::string lds_path = TestEnvironment::temporaryPath(TestUtility::uniqueFilename());
-    config_helper_.addConfigModifier(
+    config_helper.addConfigModifier(
         [lds_path](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
           bootstrap.mutable_dynamic_resources()->mutable_lds_config()->set_resource_api_version(
               envoy::config::core::v3::V3);
@@ -183,16 +178,16 @@ void BaseIntegrationTest::createEnvoy() {
   // Note that finalize assumes that every fake_upstream_ must correspond to a bootstrap config
   // static entry. So, if you want to manually create a fake upstream without specifying it in the
   // config, you will need to do so *after* initialize() (which calls this function) is done.
-  config_helper_.finalize(ports);
+  config_helper.finalize(ports);
 
-  envoy::config::bootstrap::v3::Bootstrap bootstrap = config_helper_.bootstrap();
-  if (use_lds_) {
+  envoy::config::bootstrap::v3::Bootstrap bootstrap = config_helper.bootstrap();
+  if (use_lds) {
     // After the config has been finalized, write the final listener config to the lds file.
     const std::string lds_path =
-        config_helper_.bootstrap().dynamic_resources().lds_config().path_config_source().path();
+        config_helper.bootstrap().dynamic_resources().lds_config().path_config_source().path();
     envoy::service::discovery::v3::DiscoveryResponse lds;
     lds.set_version_info("0");
-    for (auto& listener : config_helper_.bootstrap().static_resources().listeners()) {
+    for (auto& listener : config_helper.bootstrap().static_resources().listeners()) {
       ProtobufWkt::Any* resource = lds.add_resources();
       resource->PackFrom(listener);
     }
@@ -208,6 +203,18 @@ void BaseIntegrationTest::createEnvoy() {
 
   const std::string bootstrap_path = TestEnvironment::writeStringToFileForTest(
       "bootstrap.pb", TestUtility::getProtobufBinaryStringFromMessage(bootstrap));
+  return bootstrap_path;
+}
+
+void BaseIntegrationTest::createEnvoy() {
+  std::vector<uint32_t> ports;
+  for (auto& upstream : fake_upstreams_) {
+    if (upstream->localAddress()->ip()) {
+      ports.push_back(upstream->localAddress()->ip()->port());
+    }
+  }
+
+  const std::string bootstrap_path = finalizeConfigWithPorts(config_helper_, ports, use_lds_);
 
   std::vector<std::string> named_ports;
   const auto& static_resources = config_helper_.bootstrap().static_resources();
@@ -295,12 +302,13 @@ void BaseIntegrationTest::setUpstreamAddress(
   socket_address->set_port_value(fake_upstreams_[upstream_index]->localAddress()->ip()->port());
 }
 
-void BaseIntegrationTest::registerTestServerPorts(const std::vector<std::string>& port_names) {
+void BaseIntegrationTest::registerTestServerPorts(const std::vector<std::string>& port_names,
+                                                  IntegrationTestServerPtr& test_server) {
   bool listeners_ready = false;
   absl::Mutex l;
   std::vector<std::reference_wrapper<Network::ListenerConfig>> listeners;
-  test_server_->server().dispatcher().post([this, &listeners, &listeners_ready, &l]() {
-    listeners = test_server_->server().listenerManager().listeners();
+  test_server->server().dispatcher().post([&listeners, &listeners_ready, &l, &test_server]() {
+    listeners = test_server->server().listenerManager().listeners();
     l.Lock();
     listeners_ready = true;
     l.Unlock();
@@ -318,7 +326,7 @@ void BaseIntegrationTest::registerTestServerPorts(const std::vector<std::string>
     }
   }
   const auto admin_addr =
-      test_server_->server().admin().socket().connectionInfoProvider().localAddress();
+      test_server->server().admin().socket().connectionInfoProvider().localAddress();
   if (admin_addr->type() == Network::Address::Type::Ip) {
     registerPort("admin", admin_addr->ip()->port());
   }
@@ -334,8 +342,15 @@ std::string getListenerDetails(Envoy::Server::Instance& server) {
 void BaseIntegrationTest::createGeneratedApiTestServer(
     const std::string& bootstrap_path, const std::vector<std::string>& port_names,
     Server::FieldValidationConfig validator_config, bool allow_lds_rejection) {
+  createGeneratedApiTestServer(bootstrap_path, port_names, validator_config, allow_lds_rejection,
+                               test_server_);
+}
 
-  test_server_ = IntegrationTestServer::create(
+void BaseIntegrationTest::createGeneratedApiTestServer(
+    const std::string& bootstrap_path, const std::vector<std::string>& port_names,
+    Server::FieldValidationConfig validator_config, bool allow_lds_rejection,
+    IntegrationTestServerPtr& test_server) {
+  test_server = IntegrationTestServer::create(
       bootstrap_path, version_, on_server_ready_function_, on_server_init_function_,
       deterministic_value_, timeSystem(), *api_, defer_listener_finalization_, process_object_,
       validator_config, concurrency_, drain_time_, drain_strategy_, proxy_buffer_factory_,
@@ -350,27 +365,27 @@ void BaseIntegrationTest::createGeneratedApiTestServer(
     Event::TestTimeSystem::RealTimeBound bound(2 * TestUtility::DefaultTimeout);
     const char* success = "listener_manager.listener_create_success";
     const char* rejected = "listener_manager.lds.update_rejected";
-    for (Stats::CounterSharedPtr success_counter = test_server_->counter(success),
-                                 rejected_counter = test_server_->counter(rejected);
+    for (Stats::CounterSharedPtr success_counter = test_server->counter(success),
+                                 rejected_counter = test_server->counter(rejected);
          (success_counter == nullptr ||
           success_counter->value() <
               concurrency_ * config_helper_.bootstrap().static_resources().listeners_size()) &&
          (!allow_lds_rejection || rejected_counter == nullptr || rejected_counter->value() == 0);
-         success_counter = test_server_->counter(success),
-                                 rejected_counter = test_server_->counter(rejected)) {
+         success_counter = test_server->counter(success),
+                                 rejected_counter = test_server->counter(rejected)) {
       if (!bound.withinBound()) {
         RELEASE_ASSERT(0, "Timed out waiting for listeners.");
       }
       if (!allow_lds_rejection) {
         RELEASE_ASSERT(rejected_counter == nullptr || rejected_counter->value() == 0,
                        absl::StrCat("Lds update failed. Details\n",
-                                    getListenerDetails(test_server_->server())));
+                                    getListenerDetails(test_server->server())));
       }
       // TODO(mattklein123): Switch to events and waitFor().
       time_system_.realSleepDoNotUseWithoutScrutiny(std::chrono::milliseconds(10));
     }
 
-    registerTestServerPorts(port_names);
+    registerTestServerPorts(port_names, test_server);
   }
 }
 

diff --git a/test/integration/base_integration_test.h b/test/integration/base_integration_test.h
@@ -94,7 +94,11 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
   makeClientConnectionWithOptions(uint32_t port,
                                   const Network::ConnectionSocket::OptionsSharedPtr& options);
 
-  void registerTestServerPorts(const std::vector<std::string>& port_names);
+  void registerTestServerPorts(const std::vector<std::string>& port_names) {
+    registerTestServerPorts(port_names, test_server_);
+  }
+  void registerTestServerPorts(const std::vector<std::string>& port_names,
+                               IntegrationTestServerPtr& test_server);
   void createGeneratedApiTestServer(const std::string& bootstrap_path,
                                     const std::vector<std::string>& port_names,
                                     Server::FieldValidationConfig validator_config,
@@ -104,6 +108,12 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
                            Server::FieldValidationConfig validator_config,
                            bool allow_lds_rejection);
 
+  void createGeneratedApiTestServer(const std::string& bootstrap_path,
+                                    const std::vector<std::string>& port_names,
+                                    Server::FieldValidationConfig validator_config,
+                                    bool allow_lds_rejection,
+                                    IntegrationTestServerPtr& test_server);
+
   Event::TestTimeSystem& timeSystem() { return time_system_; }
 
   Stats::IsolatedStoreImpl stats_store_;
@@ -333,6 +343,9 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
   }
 
 protected:
+  static std::string finalizeConfigWithPorts(ConfigHelper& helper, std::vector<uint32_t>& ports,
+                                             bool use_lds);
+
   void setUdpFakeUpstream(absl::optional<FakeUpstreamConfig::UdpConfig> config) {
     upstream_config_.udp_fake_upstream_ = config;
   }

diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc
@@ -357,7 +357,7 @@ void HttpIntegrationTest::initialize() {
   config_helper_.addQuicDownstreamTransportSocketConfig();
 
   BaseIntegrationTest::initialize();
-  registerTestServerPorts({"http"});
+  registerTestServerPorts({"http"}, test_server_);
 
   // Needs to outlive all QUIC connections.
   auto quic_connection_persistent_info =