network filters: avoid unnecessary std::shared_ptrs (envoyproxy#14711)

While debugging a crash in: envoyproxy#13592 I ended up discussing with @lambdai and @mattklein123 whether network filters can hold references to things owned by their corresponding FactoryFilterCb. The answer is yes and the HCM and some other notable filters already use references instead of std::shared_ptrs. So let's consistently do this everywhere to avoid someone else asking this same question in the future. Plus, it's always nice to create fewer std::shared_ptrs. Follow-up on: envoyproxy#8633 Signed-off-by: Raul Gutierrez Segales <[email protected]>
rgs1 · Jan 19, 2021 · 72db81d · 72db81d
1 parent 7d48928
commit 72db81d
Show file tree

Hide file tree

Showing 51 changed files with 294 additions and 297 deletions.
diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -219,14 +219,12 @@ UpstreamDrainManager& Config::drainManager() {
   return upstream_drain_manager_slot_->getTyped<UpstreamDrainManager>();
 }
 
-Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager)
+Filter::Filter(Config& config, Upstream::ClusterManager& cluster_manager)
     : config_(config), cluster_manager_(cluster_manager), downstream_callbacks_(*this),
-      upstream_callbacks_(new UpstreamCallbacks(this)) {
-  ASSERT(config != nullptr);
-}
+      upstream_callbacks_(new UpstreamCallbacks(this)) {}
 
 Filter::~Filter() {
-  for (const auto& access_log : config_->accessLogs()) {
+  for (const auto& access_log : config_.accessLogs()) {
     access_log->log(nullptr, nullptr, nullptr, getStreamInfo());
   }
 
@@ -254,13 +252,13 @@ void Filter::initialize(Network::ReadFilterCallbacks& callbacks, bool set_connec
   // established.
   read_callbacks_->connection().readDisable(true);
 
-  config_->stats().downstream_cx_total_.inc();
+  config_.stats().downstream_cx_total_.inc();
   if (set_connection_stats) {
     read_callbacks_->connection().setConnectionStats(
-        {config_->stats().downstream_cx_rx_bytes_total_,
-         config_->stats().downstream_cx_rx_bytes_buffered_,
-         config_->stats().downstream_cx_tx_bytes_total_,
-         config_->stats().downstream_cx_tx_bytes_buffered_, nullptr, nullptr});
+        {config_.stats().downstream_cx_rx_bytes_total_,
+         config_.stats().downstream_cx_rx_bytes_buffered_,
+         config_.stats().downstream_cx_tx_bytes_total_,
+         config_.stats().downstream_cx_tx_bytes_buffered_, nullptr, nullptr});
   }
 }
 
@@ -295,9 +293,9 @@ void Filter::readDisableDownstream(bool disable) {
   read_callbacks_->connection().readDisable(disable);
 
   if (disable) {
-    config_->stats().downstream_flow_control_paused_reading_total_.inc();
+    config_.stats().downstream_flow_control_paused_reading_total_.inc();
   } else {
-    config_->stats().downstream_flow_control_resumed_reading_total_.inc();
+    config_.stats().downstream_flow_control_resumed_reading_total_.inc();
   }
 }
 
@@ -395,7 +393,7 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
                    cluster_name);
   } else {
     ENVOY_CONN_LOG(debug, "Cluster not found {}", read_callbacks_->connection(), cluster_name);
-    config_->stats().downstream_cx_no_route_.inc();
+    config_.stats().downstream_cx_no_route_.inc();
     getStreamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound);
     onInitFailure(UpstreamFailureReason::NoRoute);
     return Network::FilterStatus::StopIteration;
@@ -413,7 +411,7 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
     return Network::FilterStatus::StopIteration;
   }
 
-  const uint32_t max_connect_attempts = config_->maxConnectAttempts();
+  const uint32_t max_connect_attempts = config_.maxConnectAttempts();
   if (connect_attempts_ >= max_connect_attempts) {
     getStreamInfo().setResponseFlag(StreamInfo::ResponseFlag::UpstreamRetryLimitExceeded);
     cluster->stats().upstream_cx_connect_attempts_exceeded_.inc();
@@ -461,7 +459,7 @@ bool Filter::maybeTunnel(Upstream::ThreadLocalCluster& cluster) {
     return false;
   }
 
-  generic_conn_pool_ = factory->createGenericConnPool(cluster, config_->tunnelingConfig(), this,
+  generic_conn_pool_ = factory->createGenericConnPool(cluster, config_.tunnelingConfig(), this,
                                                       *upstream_callbacks_);
   if (generic_conn_pool_) {
     connecting_ = true;
@@ -561,10 +559,10 @@ Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) {
 }
 
 Network::FilterStatus Filter::onNewConnection() {
-  if (config_->maxDownstreamConnectionDuration()) {
+  if (config_.maxDownstreamConnectionDuration()) {
     connection_duration_timer_ = read_callbacks_->connection().dispatcher().createTimer(
         [this]() -> void { onMaxDownstreamConnectionDuration(); });
-    connection_duration_timer_->enableTimer(config_->maxDownstreamConnectionDuration().value());
+    connection_duration_timer_->enableTimer(config_.maxDownstreamConnectionDuration().value());
   }
   return initializeUpstreamConnection();
 }
@@ -574,9 +572,9 @@ void Filter::onDownstreamEvent(Network::ConnectionEvent event) {
     Tcp::ConnectionPool::ConnectionDataPtr conn_data(upstream_->onDownstreamEvent(event));
     if (conn_data != nullptr &&
         conn_data->connection().state() != Network::Connection::State::Closed) {
-      config_->drainManager().add(config_->sharedConfig(), std::move(conn_data),
-                                  std::move(upstream_callbacks_), std::move(idle_timer_),
-                                  read_callbacks_->upstreamHost());
+      config_.drainManager().add(config_.sharedConfig(), std::move(conn_data),
+                                 std::move(upstream_callbacks_), std::move(idle_timer_),
+                                 read_callbacks_->upstreamHost());
     }
     if (event != Network::ConnectionEvent::Connected) {
       upstream_.reset();
@@ -640,7 +638,7 @@ void Filter::onUpstreamConnection() {
   ENVOY_CONN_LOG(debug, "TCP:onUpstreamEvent(), requestedServerName: {}",
                  read_callbacks_->connection(), getStreamInfo().requestedServerName());
 
-  if (config_->idleTimeout()) {
+  if (config_.idleTimeout()) {
     // The idle_timer_ can be moved to a Drainer, so related callbacks call into
     // the UpstreamCallbacks, which has the same lifetime as the timer, and can dispatch
     // the call to either TcpProxy or to Drainer, depending on the current state.
@@ -662,7 +660,7 @@ void Filter::onUpstreamConnection() {
 
 void Filter::onIdleTimeout() {
   ENVOY_CONN_LOG(debug, "Session timed out", read_callbacks_->connection());
-  config_->stats().idle_timeout_.inc();
+  config_.stats().idle_timeout_.inc();
 
   // This results in also closing the upstream connection.
   read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
@@ -671,14 +669,14 @@ void Filter::onIdleTimeout() {
 void Filter::onMaxDownstreamConnectionDuration() {
   ENVOY_CONN_LOG(debug, "max connection duration reached", read_callbacks_->connection());
   getStreamInfo().setResponseFlag(StreamInfo::ResponseFlag::DurationTimeout);
-  config_->stats().max_downstream_connection_duration_.inc();
+  config_.stats().max_downstream_connection_duration_.inc();
   read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
 }
 
 void Filter::resetIdleTimer() {
   if (idle_timer_ != nullptr) {
-    ASSERT(config_->idleTimeout());
-    idle_timer_->enableTimer(config_->idleTimeout().value());
+    ASSERT(config_.idleTimeout());
+    idle_timer_->enableTimer(config_.idleTimeout().value());
   }
 }
 

diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h
@@ -245,7 +245,7 @@ class Filter : public Network::ReadFilter,
                protected Logger::Loggable<Logger::Id::filter>,
                public GenericConnectionPoolCallbacks {
 public:
-  Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager);
+  Filter(Config& config, Upstream::ClusterManager& cluster_manager);
   ~Filter() override;
 
   // Network::ReadFilter
@@ -264,7 +264,7 @@ class Filter : public Network::ReadFilter,
   // Upstream::LoadBalancerContext
   const Router::MetadataMatchCriteria* metadataMatchCriteria() override;
   absl::optional<uint64_t> computeHashKey() override {
-    auto hash_policy = config_->hashPolicy();
+    auto hash_policy = config_.hashPolicy();
     if (hash_policy) {
       return hash_policy->generateHash(
           downstreamConnection()->addressProvider().remoteAddress().get(),
@@ -337,7 +337,7 @@ class Filter : public Network::ReadFilter,
 
   // Callbacks for different error and success states during connection establishment
   virtual RouteConstSharedPtr pickRoute() {
-    return config_->getRouteFromEntries(read_callbacks_->connection());
+    return config_.getRouteFromEntries(read_callbacks_->connection());
   }
 
   virtual void onInitFailure(UpstreamFailureReason) {
@@ -357,7 +357,7 @@ class Filter : public Network::ReadFilter,
   void disableIdleTimer();
   void onMaxDownstreamConnectionDuration();
 
-  const ConfigSharedPtr config_;
+  Config& config_;
   Upstream::ClusterManager& cluster_manager_;
   Network::ReadFilterCallbacks* read_callbacks_{};
 

diff --git a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc
@@ -53,7 +53,7 @@ ClientSslAuthConfigSharedPtr ClientSslAuthConfig::create(
   return new_config;
 }
 
-const AllowedPrincipals& ClientSslAuthConfig::allowedPrincipals() {
+const AllowedPrincipals& ClientSslAuthConfig::allowedPrincipals() const {
   return tls_->getTyped<AllowedPrincipals>();
 }
 
@@ -98,7 +98,7 @@ Network::FilterStatus ClientSslAuthFilter::onNewConnection() {
   // If this is not an SSL connection, do no further checking. High layers should redirect, etc.
   // if SSL is required.
   if (!read_callbacks_->connection().ssl()) {
-    config_->stats().auth_no_ssl_.inc();
+    config_.stats().auth_no_ssl_.inc();
     return Network::FilterStatus::Continue;
   } else {
     // Otherwise we need to wait for handshake to be complete before proceeding.
@@ -112,21 +112,21 @@ void ClientSslAuthFilter::onEvent(Network::ConnectionEvent event) {
   }
 
   ASSERT(read_callbacks_->connection().ssl());
-  if (config_->ipAllowlist().contains(
+  if (config_.ipAllowlist().contains(
           *read_callbacks_->connection().addressProvider().remoteAddress())) {
-    config_->stats().auth_ip_allowlist_.inc();
+    config_.stats().auth_ip_allowlist_.inc();
     read_callbacks_->continueReading();
     return;
   }
 
-  if (!config_->allowedPrincipals().allowed(
+  if (!config_.allowedPrincipals().allowed(
           read_callbacks_->connection().ssl()->sha256PeerCertificateDigest())) {
-    config_->stats().auth_digest_no_match_.inc();
+    config_.stats().auth_digest_no_match_.inc();
     read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
     return;
   }
 
-  config_->stats().auth_digest_match_.inc();
+  config_.stats().auth_digest_match_.inc();
   read_callbacks_->continueReading();
 }
 

diff --git a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h
@@ -80,8 +80,8 @@ class ClientSslAuthConfig : public Http::RestApiFetcher {
          ThreadLocal::SlotAllocator& tls, Upstream::ClusterManager& cm,
          Event::Dispatcher& dispatcher, Stats::Scope& scope, Random::RandomGenerator& random);
 
-  const AllowedPrincipals& allowedPrincipals();
-  const Network::Address::IpList& ipAllowlist() { return ip_allowlist_; }
+  const AllowedPrincipals& allowedPrincipals() const;
+  const Network::Address::IpList& ipAllowlist() const { return ip_allowlist_; }
   GlobalStats& stats() { return stats_; }
 
 private:
@@ -108,7 +108,7 @@ class ClientSslAuthConfig : public Http::RestApiFetcher {
  */
 class ClientSslAuthFilter : public Network::ReadFilter, public Network::ConnectionCallbacks {
 public:
-  ClientSslAuthFilter(ClientSslAuthConfigSharedPtr config) : config_(config) {}
+  ClientSslAuthFilter(ClientSslAuthConfig& config) : config_(config) {}
 
   // Network::ReadFilter
   Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override;
@@ -124,7 +124,7 @@ class ClientSslAuthFilter : public Network::ReadFilter, public Network::Connecti
   void onBelowWriteBufferLowWatermark() override {}
 
 private:
-  ClientSslAuthConfigSharedPtr config_;
+  ClientSslAuthConfig& config_;
   Network::ReadFilterCallbacks* read_callbacks_{};
 };
 

diff --git a/source/extensions/filters/network/client_ssl_auth/config.cc b/source/extensions/filters/network/client_ssl_auth/config.cc
@@ -22,7 +22,7 @@ Network::FilterFactoryCb ClientSslAuthConfigFactory::createFilterFactoryFromProt
       proto_config, context.threadLocal(), context.clusterManager(), context.dispatcher(),
       context.scope(), context.api().randomGenerator()));
   return [filter_config](Network::FilterManager& filter_manager) -> void {
-    filter_manager.addReadFilter(std::make_shared<ClientSslAuthFilter>(filter_config));
+    filter_manager.addReadFilter(std::make_shared<ClientSslAuthFilter>(*filter_config));
   };
 }
 

diff --git a/source/extensions/filters/network/ext_authz/config.cc b/source/extensions/filters/network/ext_authz/config.cc
@@ -38,7 +38,7 @@ Network::FilterFactoryCb ExtAuthzConfigFactory::createFilterFactoryFromProtoType
         async_client_factory->create(), std::chrono::milliseconds(timeout_ms),
         transport_api_version);
     filter_manager.addReadFilter(Network::ReadFilterSharedPtr{
-        std::make_shared<Filter>(ext_authz_config, std::move(client))});
+        std::make_shared<Filter>(*ext_authz_config, std::move(client))});
   };
 }
 

diff --git a/source/extensions/filters/network/ext_authz/ext_authz.cc b/source/extensions/filters/network/ext_authz/ext_authz.cc
@@ -23,11 +23,11 @@ InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope
 
 void Filter::callCheck() {
   Filters::Common::ExtAuthz::CheckRequestUtils::createTcpCheck(filter_callbacks_, check_request_,
-                                                               config_->includePeerCertificate());
+                                                               config_.includePeerCertificate());
 
   status_ = Status::Calling;
-  config_->stats().active_.inc();
-  config_->stats().total_.inc();
+  config_.stats().active_.inc();
+  config_.stats().total_.inc();
 
   calling_check_ = true;
   client_->check(*this, check_request_, Tracing::NullSpan::instance(),
@@ -37,7 +37,7 @@ void Filter::callCheck() {
 
 Network::FilterStatus Filter::onData(Buffer::Instance&, bool /* end_stream */) {
   if (!filterEnabled(filter_callbacks_->connection().streamInfo().dynamicMetadata())) {
-    config_->stats().disabled_.inc();
+    config_.stats().disabled_.inc();
     return Network::FilterStatus::Continue;
   }
 
@@ -62,40 +62,40 @@ void Filter::onEvent(Network::ConnectionEvent event) {
       // Make sure that any pending request in the client is cancelled. This will be NOP if the
       // request already completed.
       client_->cancel();
-      config_->stats().active_.dec();
+      config_.stats().active_.dec();
     }
   }
 }
 
 void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
   status_ = Status::Complete;
-  config_->stats().active_.dec();
+  config_.stats().active_.dec();
 
   switch (response->status) {
   case Filters::Common::ExtAuthz::CheckStatus::OK:
-    config_->stats().ok_.inc();
+    config_.stats().ok_.inc();
     break;
   case Filters::Common::ExtAuthz::CheckStatus::Error:
-    config_->stats().error_.inc();
+    config_.stats().error_.inc();
     break;
   case Filters::Common::ExtAuthz::CheckStatus::Denied:
-    config_->stats().denied_.inc();
+    config_.stats().denied_.inc();
     break;
   }
 
   // Fail open only if configured to do so and if the check status was a error.
   if (response->status == Filters::Common::ExtAuthz::CheckStatus::Denied ||
       (response->status == Filters::Common::ExtAuthz::CheckStatus::Error &&
-       !config_->failureModeAllow())) {
-    config_->stats().cx_closed_.inc();
+       !config_.failureModeAllow())) {
+    config_.stats().cx_closed_.inc();
     filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
   } else {
     // Let the filter chain continue.
     filter_return_ = FilterReturn::Continue;
-    if (config_->failureModeAllow() &&
+    if (config_.failureModeAllow() &&
         response->status == Filters::Common::ExtAuthz::CheckStatus::Error) {
       // Status is Error and yet we are configured to allow traffic. Click a counter.
-      config_->stats().failure_mode_allowed_.inc();
+      config_.stats().failure_mode_allowed_.inc();
     }
 
     if (!response->dynamic_metadata.fields().empty()) {

diff --git a/source/extensions/filters/network/ext_authz/ext_authz.h b/source/extensions/filters/network/ext_authz/ext_authz.h
@@ -87,7 +87,7 @@ class Filter : public Network::ReadFilter,
                public Network::ConnectionCallbacks,
                public Filters::Common::ExtAuthz::RequestCallbacks {
 public:
-  Filter(ConfigSharedPtr config, Filters::Common::ExtAuthz::ClientPtr&& client)
+  Filter(Config& config, Filters::Common::ExtAuthz::ClientPtr&& client)
       : config_(config), client_(std::move(client)) {}
   ~Filter() override = default;
 
@@ -119,10 +119,10 @@ class Filter : public Network::ReadFilter,
   void callCheck();
 
   bool filterEnabled(const envoy::config::core::v3::Metadata& metadata) {
-    return config_->filterEnabledMetadata(metadata);
+    return config_.filterEnabledMetadata(metadata);
   }
 
-  ConfigSharedPtr config_;
+  Config& config_;
   Filters::Common::ExtAuthz::ClientPtr client_;
   Network::ReadFilterCallbacks* filter_callbacks_{};
   Status status_{Status::NotStarted};

diff --git a/source/extensions/filters/network/local_ratelimit/config.cc b/source/extensions/filters/network/local_ratelimit/config.cc
@@ -16,7 +16,7 @@ Network::FilterFactoryCb LocalRateLimitConfigFactory::createFilterFactoryFromPro
   ConfigSharedPtr filter_config(
       new Config(proto_config, context.dispatcher(), context.scope(), context.runtime()));
   return [filter_config](Network::FilterManager& filter_manager) -> void {
-    filter_manager.addReadFilter(std::make_shared<Filter>(filter_config));
+    filter_manager.addReadFilter(std::make_shared<Filter>(*filter_config));
   };
 }
 

diff --git a/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc
@@ -32,13 +32,13 @@ LocalRateLimitStats Config::generateStats(const std::string& prefix, Stats::Scop
 bool Config::canCreateConnection() { return rate_limiter_.requestAllowed(descriptors_); }
 
 Network::FilterStatus Filter::onNewConnection() {
-  if (!config_->enabled()) {
+  if (!config_.enabled()) {
     ENVOY_CONN_LOG(trace, "local_rate_limit: runtime disabled", read_callbacks_->connection());
     return Network::FilterStatus::Continue;
   }
 
-  if (!config_->canCreateConnection()) {
-    config_->stats().rate_limited_.inc();
+  if (!config_.canCreateConnection()) {
+    config_.stats().rate_limited_.inc();
     ENVOY_CONN_LOG(trace, "local_rate_limit: rate limiting connection",
                    read_callbacks_->connection());
     read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);