hcm: fix crash due to config lifetime when loading hcm via ECDS (envo…

…yproxy#33805)

Fixes envoyproxy#33801

Signed-off-by: Greg Greenway <ggreenway@apple.com>

changelogs/current.yaml

@@ -30,6 +30,9 @@ bug_fixes:
   change: |
     Fix a RELEASE_ASSERT when using :ref:`auto_sni <envoy_v3_api_field_config.core.v3.UpstreamHttpProtocolOptions.auto_sni>`
     if the downstream request ``:authority`` was longer than 255 characters.
+- area: http
+  change: |
+    Fix a crash when reloading the HTTP Connection Manager via ECDS.
 - area: cares
   change: |
     Upgraded c-ares library to 1.20.1 and added fix to c-ares DNS implementation to additionally check for ``ARES_EREFUSED``,

source/common/http/conn_manager_config.h

@@ -545,5 +545,7 @@ class ConnectionManagerConfig {
    */
   virtual bool addProxyProtocolConnectionState() const PURE;
 };
+
+using ConnectionManagerConfigSharedPtr = std::shared_ptr<ConnectionManagerConfig>;
 } // namespace Http
 } // namespace Envoy

source/common/http/conn_manager_impl.h

@@ -64,7 +64,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
                               public Network::ConnectionCallbacks,
                               public Http::ApiListener {
 public:
-  ConnectionManagerImpl(ConnectionManagerConfig& config, const Network::DrainDecision& drain_close,
+  ConnectionManagerImpl(ConnectionManagerConfigSharedPtr config,
+                        const Network::DrainDecision& drain_close,
                         Random::RandomGenerator& random_generator, Http::Context& http_context,
                         Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info,
                         Upstream::ClusterManager& cluster_manager,
@@ -566,7 +567,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   void onConnectionDurationTimeout();
   void onDrainTimeout();
   void startDrainSequence();
-  Tracing::Tracer& tracer() { return *config_.tracer(); }
+  Tracing::Tracer& tracer() { return *config_->tracer(); }
   void handleCodecErrorImpl(absl::string_view error, absl::string_view details,
                             StreamInfo::CoreResponseFlag response_flag);
   void handleCodecError(absl::string_view error);
@@ -589,7 +590,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
 
   enum class DrainState { NotDraining, Draining, Closing };
 
-  ConnectionManagerConfig& config_;
+  ConnectionManagerConfigSharedPtr config_;
   ConnectionManagerStats& stats_; // We store a reference here to avoid an extra stats() call on
                                   // the config in the hot path.
   ServerConnectionPtr codec_;

source/extensions/filters/network/http_connection_manager/config.cc

@@ -295,7 +295,7 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoAndHopByHo
     auto& server_context = context.serverFactoryContext();
 
     auto hcm = std::make_shared<Http::ConnectionManagerImpl>(
-        *filter_config, context.drainDecision(), server_context.api().randomGenerator(),
+        filter_config, context.drainDecision(), server_context.api().randomGenerator(),
         server_context.httpContext(), server_context.runtime(), server_context.localInfo(),
         server_context.clusterManager(), server_context.overloadManager(),
         server_context.mainThreadDispatcher().timeSource());
@@ -852,7 +852,7 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto(
     auto& server_context = context.serverFactoryContext();
 
     auto conn_manager = std::make_unique<Http::ConnectionManagerImpl>(
-        *filter_config, context.drainDecision(), server_context.api().randomGenerator(),
+        filter_config, context.drainDecision(), server_context.api().randomGenerator(),
         server_context.httpContext(), server_context.runtime(), server_context.localInfo(),
         server_context.clusterManager(), server_context.overloadManager(),
         server_context.mainThreadDispatcher().timeSource());

source/server/admin/admin.cc

@@ -288,9 +288,9 @@ bool AdminImpl::createNetworkFilterChain(Network::Connection& connection,
   // 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_.api().randomGenerator(), server_.httpContext(),
-      server_.runtime(), server_.localInfo(), server_.clusterManager(), null_overload_manager_,
-      server_.timeSource())});
+      shared_from_this(), server_.drainManager(), server_.api().randomGenerator(),
+      server_.httpContext(), server_.runtime(), server_.localInfo(), server_.clusterManager(),
+      null_overload_manager_, server_.timeSource())});
   return true;
 }
 

source/server/admin/admin.h

@@ -67,6 +67,7 @@ class AdminImpl : public Admin,
                   public Network::FilterChainFactory,
                   public Http::FilterChainFactory,
                   public Http::ConnectionManagerConfig,
+                  public std::enable_shared_from_this<AdminImpl>,
                   Logger::Loggable<Logger::Id::admin> {
 public:
   AdminImpl(const std::string& profile_path, Server::Instance& server,

source/server/server.cc

@@ -606,7 +606,7 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar
 
   OptRef<Server::ConfigTracker> config_tracker;
 #ifdef ENVOY_ADMIN_FUNCTIONALITY
-  admin_ = std::make_unique<AdminImpl>(initial_config.admin().profilePath(), *this,
+  admin_ = std::make_shared<AdminImpl>(initial_config.admin().profilePath(), *this,
                                        initial_config.admin().ignoreGlobalConnLimit());
 
   config_tracker = admin_->getConfigTracker();

source/server/server.h

@@ -379,7 +379,7 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
   std::unique_ptr<Ssl::ContextManager> ssl_context_manager_;
   Event::DispatcherPtr dispatcher_;
   AccessLog::AccessLogManagerImpl access_log_manager_;
-  std::unique_ptr<Admin> admin_;
+  std::shared_ptr<Admin> admin_;
   Singleton::ManagerPtr singleton_manager_;
   Network::ConnectionHandlerPtr handler_;
   std::unique_ptr<Runtime::Loader> runtime_;

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -605,7 +605,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
     return;
   }
 
-  FuzzConfig config(input.forward_client_cert());
+  std::shared_ptr<FuzzConfig> config = std::make_shared<FuzzConfig>(input.forward_client_cert());
   NiceMock<Network::MockDrainDecision> drain_close;
   NiceMock<Random::MockRandomGenerator> random;
   Stats::SymbolTableImpl symbol_table;
@@ -630,7 +630,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, overload_manager, config.time_system_);
+                                     cluster_manager, overload_manager, config->time_system_);
   conn_manager.initializeReadFilterCallbacks(filter_callbacks);
 
   std::vector<FuzzStreamPtr> streams;
@@ -645,7 +645,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
     switch (action.action_selector_case()) {
     case test::common::http::Action::kNewStream: {
       streams.emplace_back(new FuzzStream(
-          conn_manager, config,
+          conn_manager, *config,
           Fuzz::fromHeaders<TestRequestHeaderMapImpl>(action.new_stream().request_headers(),
                                                       /* ignore_headers =*/{}, {":authority"}),
           action.new_stream().status(), action.new_stream().end_stream()));

test/common/http/conn_manager_impl_test_base.cc

@@ -12,6 +12,137 @@ using testing::Return;
 namespace Envoy {
 namespace Http {
 
+// This is a hack that allows getting a shared_ptr to the config, without the config
+// having a lifetime controlled by a shared_ptr.
+//
+// This is here to avoid an enormous change in the HCM tests to make
+// `HttpConnectionManagerImplMixin` be a separate object owned by a shared_ptr instead of a mixin.
+class ConnectionManagerConfigProxyObject : public ConnectionManagerConfig {
+public:
+  ConnectionManagerConfigProxyObject(ConnectionManagerConfig& parent) : parent_(parent) {}
+  const RequestIDExtensionSharedPtr& requestIDExtension() override {
+    return parent_.requestIDExtension();
+  }
+  const std::list<AccessLog::InstanceSharedPtr>& accessLogs() override {
+    return parent_.accessLogs();
+  }
+  const absl::optional<std::chrono::milliseconds>& accessLogFlushInterval() override {
+    return parent_.accessLogFlushInterval();
+  }
+  bool flushAccessLogOnNewRequest() override { return parent_.flushAccessLogOnNewRequest(); }
+  bool flushAccessLogOnTunnelSuccessfullyEstablished() const override {
+    return parent_.flushAccessLogOnTunnelSuccessfullyEstablished();
+  }
+  ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& data,
+                                  ServerConnectionCallbacks& callbacks,
+                                  Server::OverloadManager& overload_manager) override {
+    return parent_.createCodec(connection, data, callbacks, overload_manager);
+  }
+  DateProvider& dateProvider() override { return parent_.dateProvider(); }
+  std::chrono::milliseconds drainTimeout() const override { return parent_.drainTimeout(); }
+  FilterChainFactory& filterFactory() override { return parent_.filterFactory(); }
+  bool generateRequestId() const override { return parent_.generateRequestId(); }
+  bool preserveExternalRequestId() const override { return parent_.preserveExternalRequestId(); }
+  bool alwaysSetRequestIdInResponse() const override {
+    return parent_.alwaysSetRequestIdInResponse();
+  }
+  absl::optional<std::chrono::milliseconds> idleTimeout() const override {
+    return parent_.idleTimeout();
+  }
+  bool isRoutable() const override { return parent_.isRoutable(); }
+  absl::optional<std::chrono::milliseconds> maxConnectionDuration() const override {
+    return parent_.maxConnectionDuration();
+  }
+  uint32_t maxRequestHeadersKb() const override { return parent_.maxRequestHeadersKb(); }
+  uint32_t maxRequestHeadersCount() const override { return parent_.maxRequestHeadersCount(); }
+  std::chrono::milliseconds streamIdleTimeout() const override {
+    return parent_.streamIdleTimeout();
+  }
+  std::chrono::milliseconds requestTimeout() const override { return parent_.requestTimeout(); }
+  std::chrono::milliseconds requestHeadersTimeout() const override {
+    return parent_.requestHeadersTimeout();
+  }
+  std::chrono::milliseconds delayedCloseTimeout() const override {
+    return parent_.delayedCloseTimeout();
+  }
+  absl::optional<std::chrono::milliseconds> maxStreamDuration() const override {
+    return parent_.maxStreamDuration();
+  }
+  Router::RouteConfigProvider* routeConfigProvider() override {
+    return parent_.routeConfigProvider();
+  }
+  Config::ConfigProvider* scopedRouteConfigProvider() override {
+    return parent_.scopedRouteConfigProvider();
+  }
+  OptRef<const Router::ScopeKeyBuilder> scopeKeyBuilder() override {
+    return parent_.scopeKeyBuilder();
+  }
+  const std::string& serverName() const override { return parent_.serverName(); }
+  HttpConnectionManagerProto::ServerHeaderTransformation
+  serverHeaderTransformation() const override {
+    return parent_.serverHeaderTransformation();
+  }
+  const absl::optional<std::string>& schemeToSet() const override { return parent_.schemeToSet(); }
+  ConnectionManagerStats& stats() override { return parent_.stats(); }
+  ConnectionManagerTracingStats& tracingStats() override { return parent_.tracingStats(); }
+  bool useRemoteAddress() const override { return parent_.useRemoteAddress(); }
+  const InternalAddressConfig& internalAddressConfig() const override {
+    return parent_.internalAddressConfig();
+  }
+  uint32_t xffNumTrustedHops() const override { return parent_.xffNumTrustedHops(); }
+  bool skipXffAppend() const override { return parent_.skipXffAppend(); }
+  const std::string& via() const override { return parent_.via(); }
+  ForwardClientCertType forwardClientCert() const override { return parent_.forwardClientCert(); }
+  const std::vector<ClientCertDetailsType>& setCurrentClientCertDetails() const override {
+    return parent_.setCurrentClientCertDetails();
+  }
+  const Network::Address::Instance& localAddress() override { return parent_.localAddress(); }
+  const absl::optional<std::string>& userAgent() override { return parent_.userAgent(); }
+  Tracing::TracerSharedPtr tracer() override { return parent_.tracer(); }
+  const TracingConnectionManagerConfig* tracingConfig() override { return parent_.tracingConfig(); }
+  ConnectionManagerListenerStats& listenerStats() override { return parent_.listenerStats(); }
+  bool proxy100Continue() const override { return parent_.proxy100Continue(); }
+  bool streamErrorOnInvalidHttpMessaging() const override {
+    return parent_.streamErrorOnInvalidHttpMessaging();
+  }
+  const Http::Http1Settings& http1Settings() const override { return parent_.http1Settings(); }
+  bool shouldNormalizePath() const override { return parent_.shouldNormalizePath(); }
+  bool shouldMergeSlashes() const override { return parent_.shouldMergeSlashes(); }
+  StripPortType stripPortType() const override { return parent_.stripPortType(); }
+  envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
+  headersWithUnderscoresAction() const override {
+    return parent_.headersWithUnderscoresAction();
+  }
+  const LocalReply::LocalReply& localReply() const override { return parent_.localReply(); }
+  envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager::
+      PathWithEscapedSlashesAction
+      pathWithEscapedSlashesAction() const override {
+    return parent_.pathWithEscapedSlashesAction();
+  }
+  const std::vector<OriginalIPDetectionSharedPtr>& originalIpDetectionExtensions() const override {
+    return parent_.originalIpDetectionExtensions();
+  }
+  const std::vector<EarlyHeaderMutationPtr>& earlyHeaderMutationExtensions() const override {
+    return parent_.earlyHeaderMutationExtensions();
+  }
+  bool shouldStripTrailingHostDot() const override { return parent_.shouldStripTrailingHostDot(); }
+  uint64_t maxRequestsPerConnection() const override { return parent_.maxRequestsPerConnection(); }
+  const HttpConnectionManagerProto::ProxyStatusConfig* proxyStatusConfig() const override {
+    return parent_.proxyStatusConfig();
+  }
+  ServerHeaderValidatorPtr makeHeaderValidator(Protocol protocol) override {
+    return parent_.makeHeaderValidator(protocol);
+  }
+  bool appendXForwardedPort() const override { return parent_.appendXForwardedPort(); }
+  bool appendLocalOverload() const override { return parent_.appendLocalOverload(); }
+  bool addProxyProtocolConnectionState() const override {
+    return parent_.addProxyProtocolConnectionState();
+  }
+
+private:
+  ConnectionManagerConfig& parent_;
+};
+
 HttpConnectionManagerImplMixin::HttpConnectionManagerImplMixin()
     : fake_stats_(*symbol_table_), http_context_(fake_stats_.symbolTable()),
       access_log_path_("dummy_path"),
@@ -76,8 +207,9 @@ void HttpConnectionManagerImplMixin::setup(bool ssl, const std::string& server_n
   filter_callbacks_.connection_.stream_info_.downstream_connection_info_provider_->setSslConnection(
       ssl_connection_);
   conn_manager_ = std::make_unique<ConnectionManagerImpl>(
-      *this, drain_close_, random_, http_context_, runtime_, local_info_, cluster_manager_,
-      overload_manager_, test_time_.timeSystem());
+      std::make_shared<ConnectionManagerConfigProxyObject>(*this), drain_close_, random_,
+      http_context_, runtime_, local_info_, cluster_manager_, overload_manager_,
+      test_time_.timeSystem());
 
   conn_manager_->initializeReadFilterCallbacks(filter_callbacks_);
 

test/common/http/hcm_router_fuzz_test.cc

@@ -498,20 +498,20 @@ class FuzzConfig : public HttpConnectionManagerImplMixin {
 
 class Harness {
 public:
-  Harness() : hcm_config_(Protobuf::RepeatedPtrField<std::string>{}) {
+  Harness() : hcm_config_(std::make_shared<FuzzConfig>(Protobuf::RepeatedPtrField<std::string>{})) {
     ON_CALL(filter_callbacks_.connection_, close(_, _)).WillByDefault(InvokeWithoutArgs([this]() {
       closed_ = true;
     }));
   }
 
   void fuzz(const FuzzCase& input) {
     hcm_ = std::make_unique<ConnectionManagerImpl>(
-        hcm_config_, drain_close_, random_, hcm_config_.http_context_, runtime_, local_info_,
-        hcm_config_.cm_, overload_manager_, hcm_config_.time_system_);
+        hcm_config_, drain_close_, random_, hcm_config_->http_context_, runtime_, local_info_,
+        hcm_config_->cm_, overload_manager_, hcm_config_->time_system_);
     hcm_->initializeReadFilterCallbacks(filter_callbacks_);
     Buffer::OwnedImpl data;
     hcm_->onData(data, false);
-    FuzzClusterManager& cluster_manager = hcm_config_.getFuzzClusterManager();
+    FuzzClusterManager& cluster_manager = hcm_config_->getFuzzClusterManager();
 
     for (const auto& action : input.actions()) {
       if (closed_) {
@@ -520,7 +520,7 @@ class Harness {
       switch (action.action_case()) {
       case ActionCase::kAdvanceTime: {
         const auto& a = action.advance_time();
-        hcm_config_.time_system_.timeSystem().advanceTimeWait(
+        hcm_config_->time_system_.timeSystem().advanceTimeWait(
             std::chrono::milliseconds(a.milliseconds()));
         break;
       }
@@ -582,7 +582,7 @@ class Harness {
   }
 
 private:
-  FuzzConfig hcm_config_;
+  std::shared_ptr<FuzzConfig> hcm_config_;
   NiceMock<Network::MockDrainDecision> drain_close_;
   NiceMock<Random::MockRandomGenerator> random_;
   NiceMock<Runtime::MockLoader> runtime_;