From d63fec09e216b7ff16af5f02158812259ee769f5 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 1 Mar 2022 12:44:58 -0500 Subject: [PATCH] test: adding a multi-envoy test (#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 #19847 is closed) Multi-envoy does not correctly support runtime flags or deprecation stats due to #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 Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com> --- source/common/quic/active_quic_listener.cc | 4 +- source/common/runtime/runtime_features.cc | 3 + source/common/signal/fatal_error_handler.cc | 10 ++- .../common/singleton/threadsafe_singleton.h | 2 + source/server/config_validation/server.h | 2 +- source/server/server.cc | 31 ++++++--- source/server/server.h | 1 + test/common/signal/fatal_action_test.cc | 7 -- test/integration/BUILD | 10 +++ test/integration/base_integration_test.cc | 67 ++++++++++++------- test/integration/base_integration_test.h | 15 ++++- test/integration/http_integration.cc | 2 +- test/integration/multi_envoy_test.cc | 65 ++++++++++++++++++ 13 files changed, 163 insertions(+), 56 deletions(-) create mode 100644 test/integration/multi_envoy_test.cc diff --git a/source/common/quic/active_quic_listener.cc b/source/common/quic/active_quic_listener.cc index 92d7c723ce5d..abc2b968dc01 100644 --- 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 index 7ae10128a120..ab63a8c5a79a 100644 --- 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* 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 index baf1ea267b57..ab1669c3b18f 100644 --- 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 index f88ae9ed161a..97afb282edfc 100644 --- a/source/common/singleton/threadsafe_singleton.h +++ b/source/common/singleton/threadsafe_singleton.h @@ -80,6 +80,8 @@ template class ScopedInjectableLoader { } ~ScopedInjectableLoader() { InjectableSingleton::clear(); } + T& instance() { return *instance_; } + private: std::unique_ptr instance_; }; diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 082410352f8f..a8fa7aba5f31 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -91,7 +91,7 @@ class ValidationInstance final : Logger::Loggable, 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 index 10c1ffd4c91c..9871b5662e40 100644 --- 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( - 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(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( - *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 index 6da357374d97..cef5cba3015b 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -357,6 +357,7 @@ class InstanceImpl final : Logger::Loggable, Singleton::ManagerPtr singleton_manager_; Network::ConnectionHandlerPtr handler_; std::unique_ptr runtime_singleton_; + std::unique_ptr runtime_; std::unique_ptr 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 index 5565c592eee5..b6b43470c60f 100644 --- 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(true, &counter_)); diff --git a/test/integration/BUILD b/test/integration/BUILD index d93a55ba7dfd..27ad677a7867 100644 --- 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 index 461bb39acab1..9fc56685856e 100644 --- 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 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& 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 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 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& port_names) { +void BaseIntegrationTest::registerTestServerPorts(const std::vector& port_names, + IntegrationTestServerPtr& test_server) { bool listeners_ready = false; absl::Mutex l; std::vector> 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 } } 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& 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& 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 index ecc5f538b494..2492031f3797 100644 --- a/test/integration/base_integration_test.h +++ b/test/integration/base_integration_test.h @@ -94,7 +94,11 @@ class BaseIntegrationTest : protected Logger::Loggable { makeClientConnectionWithOptions(uint32_t port, const Network::ConnectionSocket::OptionsSharedPtr& options); - void registerTestServerPorts(const std::vector& port_names); + void registerTestServerPorts(const std::vector& port_names) { + registerTestServerPorts(port_names, test_server_); + } + void registerTestServerPorts(const std::vector& port_names, + IntegrationTestServerPtr& test_server); void createGeneratedApiTestServer(const std::string& bootstrap_path, const std::vector& port_names, Server::FieldValidationConfig validator_config, @@ -104,6 +108,12 @@ class BaseIntegrationTest : protected Logger::Loggable { Server::FieldValidationConfig validator_config, bool allow_lds_rejection); + void createGeneratedApiTestServer(const std::string& bootstrap_path, + const std::vector& 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 { } protected: + static std::string finalizeConfigWithPorts(ConfigHelper& helper, std::vector& ports, + bool use_lds); + void setUdpFakeUpstream(absl::optional config) { upstream_config_.udp_fake_upstream_ = config; } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index fa2aa4d9a523..7c3fc675fd7f 100644 --- 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 = diff --git a/test/integration/multi_envoy_test.cc b/test/integration/multi_envoy_test.cc new file mode 100644 index 000000000000..f36c125d1fee --- /dev/null +++ b/test/integration/multi_envoy_test.cc @@ -0,0 +1,65 @@ +#include "test/integration/http_protocol_integration.h" + +namespace Envoy { +namespace { + +class MultiEnvoyTest : public HttpProtocolIntegrationTest { +public: + ~MultiEnvoyTest() override { test_server_.reset(); } + + // Create an envoy in front of the original Envoy. + void createL1Envoy(); + + IntegrationTestServerPtr l1_server_; + Thread::SkipAsserts skip_; +}; + +void MultiEnvoyTest::createL1Envoy() { + std::vector ports; + std::vector zero; + int l2_port = lookupPort("http"); + for (auto& upstream : fake_upstreams_) { + if (upstream->localAddress()->ip()) { + ports.push_back(l2_port); + zero.push_back(0); + } + } + ConfigHelper l1_helper(version_, *api_, + MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + l1_helper.setPorts(zero, true); // Zero out ports set by config_helper_'s finalize(); + const std::string bootstrap_path = finalizeConfigWithPorts(l1_helper, ports, use_lds_); + + std::vector named_ports; + const auto& static_resources = config_helper_.bootstrap().static_resources(); + named_ports.reserve(static_resources.listeners_size()); + for (int i = 0; i < static_resources.listeners_size(); ++i) { + named_ports.push_back(static_resources.listeners(i).name() + "_l1"); + } + createGeneratedApiTestServer(bootstrap_path, named_ports, {false, true, false}, false, + l1_server_); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, MultiEnvoyTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams( + {Http::CodecType::HTTP1}, {Http::CodecType::HTTP1})), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +// A simple test to make sure that traffic can flow between client - L1 - L2 - upstream. +// This test does not currently support mixed protocol hops, or much of the other envoy test +// framework knobs. +TEST_P(MultiEnvoyTest, SimpleRequestAndResponse) { + config_helper_.addRuntimeOverride("envoy.restart_features.no_runtime_singleton", "true"); + initialize(); + createL1Envoy(); + + codec_client_ = makeHttpConnection(lookupPort("http_l1")); + auto response = + sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0); + EXPECT_EQ(1, test_server_->counter("http.config_test.rq_total")); + EXPECT_EQ(1, l1_server_->counter("http.config_test.rq_total")); + + l1_server_.reset(); +} + +} // namespace +} // namespace Envoy