server: make overload manager non-optional

include/envoy/server/overload_manager.h

@@ -33,25 +33,10 @@ using OverloadActionCb = std::function<void(OverloadActionState)>;
  */
 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<std::string, OverloadActionState> actions_;
+  virtual ~ThreadLocalOverloadState() = default;
+
+  // Get a thread-local reference to the value for the given action key.
+  virtual const OverloadActionState& getState(const std::string& action) PURE;
 };
 
 /**
@@ -106,17 +91,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

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() {

source/common/http/conn_manager_impl.h

@@ -57,7 +57,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
                         Runtime::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);

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())});
   };
 }
 
@@ -591,8 +591,8 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto(
   return [singletons, filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr {
     auto conn_manager = std::make_unique<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());
 
     // 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.

source/server/admin/admin.cc

@@ -679,6 +679,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 +761,9 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection
 
 bool AdminImpl::createNetworkFilterChain(Network::Connection& connection,
                                          const std::vector<Network::FilterFactoryCb>&) {
-  // Don't pass in the overload manager so that the admin interface is accessible even when
-  // the 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;
 }
 

source/server/admin/admin.h

@@ -12,6 +12,7 @@
 #include "envoy/admin/v3/server_info.pb.h"
 #include "envoy/config/core/v3/base.pb.h"
 #include "envoy/config/route/v3/route.pb.h"
+#include "envoy/event/timer.h"
 #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h"
 #include "envoy/http/filter.h"
 #include "envoy/http/request_id_extension.h"
@@ -20,6 +21,7 @@
 #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"
 
@@ -244,6 +246,37 @@ 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<NullThreadLocalOverloadState>();
+      });
+    }
+
+    ThreadLocalOverloadState& getThreadLocalOverloadState() override {
+      return tls_->getTyped<NullThreadLocalOverloadState>();
+    }
+
+    bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override {
+      return false;
+    }
+
+    ThreadLocal::SlotPtr tls_;
+  };
+
   /**
    * Helper methods for the /clusters url handler.
    */
@@ -386,6 +419,7 @@ class AdminImpl : public Admin,
   std::list<AccessLog::InstanceSharedPtr> 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_;

source/server/overload_manager_impl.cc

@@ -35,6 +35,32 @@ class ThresholdTriggerImpl : public OverloadAction::Trigger {
   absl::optional<double> 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) {
+    auto it = actions_.find(action);
+    if (it == actions_.end()) {
+      actions_[action] = state;
+    } else {
+      it->second = state;
+    }
+  }
+
+private:
+  std::unordered_map<std::string, OverloadActionState> 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 +174,7 @@ void OverloadManagerImpl::start() {
   started_ = true;
 
   tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr {
-    return std::make_shared<ThreadLocalOverloadState>();
+    return std::make_shared<ThreadLocalOverloadStateImpl>();
   });
 
   if (resources_.empty()) {
@@ -191,7 +217,7 @@ bool OverloadManagerImpl::registerForAction(const std::string& action,
 }
 
 ThreadLocalOverloadState& OverloadManagerImpl::getThreadLocalOverloadState() {
-  return tls_->getTyped<ThreadLocalOverloadState>();
+  return tls_->getTyped<ThreadLocalOverloadStateImpl>();
 }
 
 void OverloadManagerImpl::updateResourcePressure(const std::string& resource, double pressure) {
@@ -208,7 +234,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<ThreadLocalOverloadState>().setState(action, state);
+                      tls_->getTyped<ThreadLocalOverloadStateImpl>().setState(action, state);
                     });
                     auto callback_range = action_to_callbacks_.equal_range(action);
                     std::for_each(callback_range.first, callback_range.second,

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",

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<LocalInfo::MockLocalInfo> local_info;
   NiceMock<Upstream::MockClusterManager> cluster_manager;
   NiceMock<Network::MockReadFilterCallbacks> filter_callbacks;
+  NiceMock<Server::MockOverloadManager> overload_manager;
   auto ssl_connection = std::make_shared<Ssl::MockConnectionInfo>();
   bool connection_alive = true;
 
@@ -554,7 +556,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
       std::make_shared<Network::Address::Ipv4Instance>("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<FuzzStreamPtr> streams;

test/common/http/conn_manager_impl_test.cc

@@ -133,7 +133,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
         std::make_shared<Network::Address::Ipv4Instance>("0.0.0.0");
     conn_manager_ = std::make_unique<ConnectionManagerImpl>(
         *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) {
@@ -5600,11 +5600,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_);
@@ -5630,10 +5631,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<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
   EXPECT_CALL(filter_factory_, createFilterChain(_))

test/mocks/server/mocks.cc

@@ -76,6 +76,11 @@ MockHotRestart::MockHotRestart() : stats_allocator_(*symbol_table_) {
 }
 MockHotRestart::~MockHotRestart() = default;
 
+MockThreadLocalOverloadState::MockThreadLocalOverloadState()
+    : disabled_state_(OverloadActionState::Inactive) {
+  ON_CALL(*this, getState).WillByDefault(ReturnRef(disabled_state_));
+}
+
 MockOverloadManager::MockOverloadManager() {
   ON_CALL(*this, getThreadLocalOverloadState()).WillByDefault(ReturnRef(overload_state_));
 }

test/mocks/server/mocks.h

@@ -331,6 +331,15 @@ class MockWorker : public Worker {
   std::function<void()> remove_filter_chains_completion_;
 };
 
+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();
@@ -343,7 +352,7 @@ class MockOverloadManager : public OverloadManager {
                OverloadActionCb callback));
   MOCK_METHOD(ThreadLocalOverloadState&, getThreadLocalOverloadState, ());
 
-  ThreadLocalOverloadState overload_state_;
+  NiceMock<MockThreadLocalOverloadState> overload_state_;
 };
 
 class MockInstance : public Instance {