diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 010ac8ee9468..e10812add8fd 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -33,25 +33,8 @@ using OverloadActionCb = std::function; */ class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { public: - const OverloadActionState& getState(const std::string& action) { - auto it = actions_.find(action); - if (it == actions_.end()) { - it = actions_.insert(std::make_pair(action, OverloadActionState::Inactive)).first; - } - return it->second; - } - - void setState(const std::string& action, OverloadActionState state) { - auto it = actions_.find(action); - if (it == actions_.end()) { - actions_[action] = state; - } else { - it->second = state; - } - } - -private: - std::unordered_map actions_; + // Get a thread-local reference to the value for the given action key. + virtual const OverloadActionState& getState(const std::string& action) PURE; }; /** @@ -106,17 +89,6 @@ class OverloadManager { * an alternative to registering a callback for overload action state changes. */ virtual ThreadLocalOverloadState& getThreadLocalOverloadState() PURE; - - /** - * Convenience method to get a statically allocated reference to the inactive overload - * action state. Useful for code that needs to initialize a reference either to an - * entry in the ThreadLocalOverloadState map (if overload behavior is enabled) or to - * some other static memory location set to the inactive state (if overload behavior - * is disabled). - */ - static const OverloadActionState& getInactiveState() { - CONSTRUCT_ON_FIRST_USE(OverloadActionState, OverloadActionState::Inactive); - } }; } // namespace Server diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fb9c36d5d730..4a76e84be282 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -104,7 +104,7 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, Http::Context& http_context, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info, Upstream::ClusterManager& cluster_manager, - Server::OverloadManager* overload_manager, + Server::OverloadManager& overload_manager, TimeSource& time_source) : config_(config), stats_(config_.stats()), conn_length_(new Stats::HistogramCompletableTimespanImpl( @@ -113,14 +113,10 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, random_generator_(random_generator), http_context_(http_context), runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager), listener_stats_(config_.listenerStats()), - overload_stop_accepting_requests_ref_( - overload_manager ? overload_manager->getThreadLocalOverloadState().getState( - Server::OverloadActionNames::get().StopAcceptingRequests) - : Server::OverloadManager::getInactiveState()), - overload_disable_keepalive_ref_( - overload_manager ? overload_manager->getThreadLocalOverloadState().getState( - Server::OverloadActionNames::get().DisableHttpKeepAlive) - : Server::OverloadManager::getInactiveState()), + overload_stop_accepting_requests_ref_(overload_manager.getThreadLocalOverloadState().getState( + Server::OverloadActionNames::get().StopAcceptingRequests)), + overload_disable_keepalive_ref_(overload_manager.getThreadLocalOverloadState().getState( + Server::OverloadActionNames::get().DisableHttpKeepAlive)), time_source_(time_source) {} const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 4e8f18814b09..04d3896a3ff8 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -58,7 +58,7 @@ class ConnectionManagerImpl : Logger::Loggable, Random::RandomGenerator& random_generator, Http::Context& http_context, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info, Upstream::ClusterManager& cluster_manager, - Server::OverloadManager* overload_manager, TimeSource& time_system); + Server::OverloadManager& overload_manager, TimeSource& time_system); ~ConnectionManagerImpl() override; static ConnectionManagerStats generateStats(const std::string& prefix, Stats::Scope& scope); diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 33047d76d643..3f8abde5ae24 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -144,8 +144,8 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( return [singletons, filter_config, &context](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *filter_config, context.drainDecision(), context.random(), context.httpContext(), - context.runtime(), context.localInfo(), context.clusterManager(), - &context.overloadManager(), context.dispatcher().timeSource())}); + context.runtime(), context.localInfo(), context.clusterManager(), context.overloadManager(), + context.dispatcher().timeSource())}); }; } @@ -588,8 +588,8 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( return [singletons, filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { auto conn_manager = std::make_unique( *filter_config, context.drainDecision(), context.random(), context.httpContext(), - context.runtime(), context.localInfo(), context.clusterManager(), - &context.overloadManager(), context.dispatcher().timeSource()); + context.runtime(), context.localInfo(), context.clusterManager(), context.overloadManager(), + context.dispatcher().timeSource()); // This factory creates a new ConnectionManagerImpl in the absence of its usual environment as // an L4 filter, so this factory needs to take a few actions. diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 5b3d3b3c4255..227f0a92277e 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -660,6 +660,7 @@ void AdminImpl::startHttpListener(const std::string& access_log_path, access_logs_.emplace_back(new Extensions::AccessLoggers::File::FileAccessLog( access_log_path, {}, Formatter::SubstitutionFormatUtils::defaultSubstitutionFormatter(), server_.accessLogManager())); + null_overload_manager_.start(); socket_ = std::make_shared(address, socket_options, true); socket_factory_ = std::make_shared(socket_); listener_ = std::make_unique(*this, std::move(listener_scope)); @@ -679,6 +680,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) request_id_extension_(Http::RequestIDExtensionFactory::defaultInstance(server_.random())), profile_path_(profile_path), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), + null_overload_manager_(server_.threadLocal()), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats("http.admin.", no_op_store_)), route_config_provider_(server.timeSource()), @@ -760,11 +762,12 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, const std::vector&) { - // Don't pass in the overload manager so that the admin interface is accessible even when - // the envoy is overloaded. + // Pass in the null overload manager so that the admin interface is accessible even when Envoy is + // overloaded. connection.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *this, server_.drainManager(), server_.random(), server_.httpContext(), server_.runtime(), - server_.localInfo(), server_.clusterManager(), nullptr, server_.timeSource())}); + server_.localInfo(), server_.clusterManager(), null_overload_manager_, + server_.timeSource())}); return true; } diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 9c035d123e80..278aa30c342b 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -20,9 +20,11 @@ #include "envoy/server/admin.h" #include "envoy/server/instance.h" #include "envoy/server/listener_manager.h" +#include "envoy/server/overload_manager.h" #include "envoy/upstream/outlier_detection.h" #include "envoy/upstream/resource_manager.h" +#include "common/common/assert.h" #include "common/common/basic_resource_impl.h" #include "common/common/empty_string.h" #include "common/common/logger.h" @@ -245,6 +247,39 @@ class AdminImpl : public Admin, TimeSource& time_source_; }; + /** + * Implementation of OverloadManager that is never overloaded. Using this instead of the real + * OverloadManager keeps the admin interface accessible even when the proxy is overloaded. + */ + struct NullOverloadManager : public OverloadManager { + struct NullThreadLocalOverloadState : public ThreadLocalOverloadState { + const OverloadActionState& getState(const std::string&) override { return inactive_; } + + const OverloadActionState inactive_ = OverloadActionState::Inactive; + }; + + NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator) + : tls_(slot_allocator.allocateSlot()) {} + + void start() override { + tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { + return std::make_shared(); + }); + } + + ThreadLocalOverloadState& getThreadLocalOverloadState() override { + return tls_->getTyped(); + } + + bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override { + // This method shouldn't be called by the admin listener + NOT_REACHED_GCOVR_EXCL_LINE; + return false; + } + + ThreadLocal::SlotPtr tls_; + }; + /** * Helper methods for the /clusters url handler. */ @@ -389,6 +424,7 @@ class AdminImpl : public Admin, std::list access_logs_; const std::string profile_path_; Http::ConnectionManagerStats stats_; + NullOverloadManager null_overload_manager_; // Note: this is here to essentially blackhole the tracing stats since they aren't used in the // Admin case. Stats::IsolatedStoreImpl no_op_store_; diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index ef56fc8d7fae..40156ed9b179 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -35,6 +35,25 @@ class ThresholdTriggerImpl : public OverloadAction::Trigger { absl::optional value_; }; +/** + * Thread-local copy of the state of each configured overload action. + */ +class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { +public: + const OverloadActionState& getState(const std::string& action) override { + auto it = actions_.find(action); + if (it == actions_.end()) { + it = actions_.insert(std::make_pair(action, OverloadActionState::Inactive)).first; + } + return it->second; + } + + void setState(const std::string& action, OverloadActionState state) { actions_[action] = state; } + +private: + std::unordered_map actions_; +}; + Stats::Counter& makeCounter(Stats::Scope& scope, absl::string_view a, absl::string_view b) { Stats::StatNameManagedStorage stat_name(absl::StrCat("overload.", a, ".", b), scope.symbolTable()); @@ -148,7 +167,7 @@ void OverloadManagerImpl::start() { started_ = true; tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { - return std::make_shared(); + return std::make_shared(); }); if (resources_.empty()) { @@ -191,7 +210,7 @@ bool OverloadManagerImpl::registerForAction(const std::string& action, } ThreadLocalOverloadState& OverloadManagerImpl::getThreadLocalOverloadState() { - return tls_->getTyped(); + return tls_->getTyped(); } void OverloadManagerImpl::updateResourcePressure(const std::string& resource, double pressure) { @@ -208,7 +227,7 @@ void OverloadManagerImpl::updateResourcePressure(const std::string& resource, do ENVOY_LOG(info, "Overload action {} became {}", action, is_active ? "active" : "inactive"); tls_->runOnAllThreads([this, action, state] { - tls_->getTyped().setState(action, state); + tls_->getTyped().setState(action, state); }); auto callback_range = action_to_callbacks_.equal_range(action); std::for_each(callback_range.first, callback_range.second, diff --git a/source/server/server.cc b/source/server/server.cc index e0f7157c54c6..80c48c279c06 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -382,23 +382,6 @@ void InstanceImpl::initialize(const Options& options, // Learn original_start_time_ if our parent is still around to inform us of it. restarter_.sendParentAdminShutdownRequest(original_start_time_); admin_ = std::make_unique(initial_config.admin().profilePath(), *this); - if (initial_config.admin().address()) { - if (initial_config.admin().accessLogPath().empty()) { - throw EnvoyException("An admin access log path is required for a listening server."); - } - ENVOY_LOG(info, "admin address: {}", initial_config.admin().address()->asString()); - admin_->startHttpListener(initial_config.admin().accessLogPath(), options.adminAddressPath(), - initial_config.admin().address(), - initial_config.admin().socketOptions(), - stats_store_.createScope("listener.admin.")); - } else { - ENVOY_LOG(warn, "No admin address given, so no admin HTTP server started."); - } - config_tracker_entry_ = - admin_->getConfigTracker().add("bootstrap", [this] { return dumpBootstrapConfig(); }); - if (initial_config.admin().address()) { - admin_->addListenerToHandler(handler_.get()); - } loadServerFlags(initial_config.flagsPath()); @@ -428,6 +411,24 @@ void InstanceImpl::initialize(const Options& options, dispatcher_->initializeStats(stats_store_, "server."); } + if (initial_config.admin().address()) { + if (initial_config.admin().accessLogPath().empty()) { + throw EnvoyException("An admin access log path is required for a listening server."); + } + ENVOY_LOG(info, "admin address: {}", initial_config.admin().address()->asString()); + admin_->startHttpListener(initial_config.admin().accessLogPath(), options.adminAddressPath(), + initial_config.admin().address(), + initial_config.admin().socketOptions(), + stats_store_.createScope("listener.admin.")); + } else { + ENVOY_LOG(warn, "No admin address given, so no admin HTTP server started."); + } + config_tracker_entry_ = + admin_->getConfigTracker().add("bootstrap", [this] { return dumpBootstrapConfig(); }); + if (initial_config.admin().address()) { + admin_->addListenerToHandler(handler_.get()); + } + // The broad order of initialization from this point on is the following: // 1. Statically provisioned configuration (bootstrap) are loaded. // 2. Cluster manager is created and all primary clusters (i.e. with endpoint assignments diff --git a/test/common/http/BUILD b/test/common/http/BUILD index eeee6f0d6a09..ec2987c48403 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -182,6 +182,7 @@ envoy_cc_fuzz_test( "//test/mocks/network:network_mocks", "//test/mocks/router:router_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/ssl:ssl_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:upstream_mocks", diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index c563f72ec381..7c85051528ab 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -34,6 +34,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/router/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/ssl/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -541,6 +542,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { NiceMock local_info; NiceMock cluster_manager; NiceMock filter_callbacks; + NiceMock overload_manager; auto ssl_connection = std::make_shared(); bool connection_alive = true; @@ -554,7 +556,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { std::make_shared("0.0.0.0"); ConnectionManagerImpl conn_manager(config, drain_close, random, http_context, runtime, local_info, - cluster_manager, nullptr, config.time_system_); + cluster_manager, overload_manager, config.time_system_); conn_manager.initializeReadFilterCallbacks(filter_callbacks); std::vector streams; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a22aa5432296..efd7222b9123 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -133,7 +133,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::make_shared("0.0.0.0"); conn_manager_ = std::make_unique( *this, drain_close_, random_, http_context_, runtime_, local_info_, cluster_manager_, - &overload_manager_, test_time_.timeSystem()); + overload_manager_, test_time_.timeSystem()); conn_manager_->initializeReadFilterCallbacks(filter_callbacks_); if (tracing) { @@ -5601,11 +5601,12 @@ TEST(HttpConnectionManagerTracingStatsTest, verifyTracingStats) { } TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { - setup(false, ""); + Server::OverloadActionState stop_accepting_requests = Server::OverloadActionState::Active; + ON_CALL(overload_manager_.overload_state_, + getState(Server::OverloadActionNames::get().StopAcceptingRequests)) + .WillByDefault(ReturnRef(stop_accepting_requests)); - overload_manager_.overload_state_.setState( - Server::OverloadActionNames::get().StopAcceptingRequests, - Server::OverloadActionState::Active); + setup(false, ""); EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance&) -> Http::Status { RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_); @@ -5631,10 +5632,12 @@ TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { } TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenOverloaded) { - setup(false, ""); + Server::OverloadActionState disable_http_keep_alive = Server::OverloadActionState::Active; + ON_CALL(overload_manager_.overload_state_, + getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)) + .WillByDefault(ReturnRef(disable_http_keep_alive)); - overload_manager_.overload_state_.setState( - Server::OverloadActionNames::get().DisableHttpKeepAlive, Server::OverloadActionState::Active); + setup(false, ""); std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) diff --git a/test/mocks/server/overload_manager.cc b/test/mocks/server/overload_manager.cc index d105df80e690..d0fd9b545ec6 100644 --- a/test/mocks/server/overload_manager.cc +++ b/test/mocks/server/overload_manager.cc @@ -9,6 +9,12 @@ namespace Envoy { namespace Server { using ::testing::ReturnRef; + +MockThreadLocalOverloadState::MockThreadLocalOverloadState() + : disabled_state_(OverloadActionState::Inactive) { + ON_CALL(*this, getState).WillByDefault(ReturnRef(disabled_state_)); +} + MockOverloadManager::MockOverloadManager() { ON_CALL(*this, getThreadLocalOverloadState()).WillByDefault(ReturnRef(overload_state_)); } diff --git a/test/mocks/server/overload_manager.h b/test/mocks/server/overload_manager.h index 8ce63cef1b12..86c194d5586d 100644 --- a/test/mocks/server/overload_manager.h +++ b/test/mocks/server/overload_manager.h @@ -8,6 +8,16 @@ namespace Envoy { namespace Server { + +class MockThreadLocalOverloadState : public ThreadLocalOverloadState { +public: + MockThreadLocalOverloadState(); + MOCK_METHOD(const OverloadActionState&, getState, (const std::string&), (override)); + +private: + const OverloadActionState disabled_state_; +}; + class MockOverloadManager : public OverloadManager { public: MockOverloadManager(); @@ -20,7 +30,8 @@ class MockOverloadManager : public OverloadManager { OverloadActionCb callback)); MOCK_METHOD(ThreadLocalOverloadState&, getThreadLocalOverloadState, ()); - ThreadLocalOverloadState overload_state_; + testing::NiceMock overload_state_; }; + } // namespace Server } // namespace Envoy