http filters: pass configs by reference instead of shared_ptr

Similar as envoyproxy#14711, but for HTTP filters.

This change has two goals:

1) consistently deal with filter configs across all HTTP filters,
   thus making it clear that it's _fine_ to refence objects bound
   to the FilterFactoryCb
2) avoid confusing future filter writers who might think they need
   shared_ptrs when they really don't. I get confused every 6 months,
   so this includes me

As a very small bonus, we create fewer shared_ptrs on every request
though this won't show up in a profiler. It still makes me happy :-)

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

@@ -25,29 +25,29 @@ AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig(
     : stats_prefix_(std::move(stats_prefix)), time_source_(time_source),
       adaptive_concurrency_feature_(proto_config.enabled(), runtime) {}
 
-AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter(
-    AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller)
-    : config_(std::move(config)), controller_(std::move(controller)) {}
+AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfig& config,
+                                                     Controller::ConcurrencyController& controller)
+    : config_(config), controller_(controller) {}
 
 Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::RequestHeaderMap&, bool) {
   // In addition to not sampling if the filter is disabled, health checks should also not be sampled
   // by the concurrency controller since they may potentially bias the sample aggregate to lower
   // latency measurements.
-  if (!config_->filterEnabled() || decoder_callbacks_->streamInfo().healthCheck()) {
+  if (!config_.filterEnabled() || decoder_callbacks_->streamInfo().healthCheck()) {
     return Http::FilterHeadersStatus::Continue;
   }
 
-  if (controller_->forwardingDecision() == Controller::RequestForwardingAction::Block) {
+  if (controller_.forwardingDecision() == Controller::RequestForwardingAction::Block) {
     decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "reached concurrency limit",
                                        nullptr, absl::nullopt, "reached_concurrency_limit");
     return Http::FilterHeadersStatus::StopIteration;
   }
 
   // When the deferred_sample_task_ object is destroyed, the request start time is sampled. This
   // occurs either when encoding is complete or during destruction of this filter object.
-  const auto now = config_->timeSource().monotonicTime();
+  const auto now = config_.timeSource().monotonicTime();
   deferred_sample_task_ =
-      std::make_unique<Cleanup>([this, now]() { controller_->recordLatencySample(now); });
+      std::make_unique<Cleanup>([this, now]() { controller_.recordLatencySample(now); });
 
   return Http::FilterHeadersStatus::Continue;
 }
@@ -62,7 +62,7 @@ void AdaptiveConcurrencyFilter::onDestroy() {
     // TODO (tonya11en): Return some RAII handle from the concurrency controller that performs this
     // logic as part of its lifecycle.
     deferred_sample_task_->cancel();
-    controller_->cancelLatencySample();
+    controller_.cancelLatencySample();
   }
 }
 

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h

@@ -53,8 +53,8 @@ using ConcurrencyControllerSharedPtr = std::shared_ptr<Controller::ConcurrencyCo
 class AdaptiveConcurrencyFilter : public Http::PassThroughFilter,
                                   Logger::Loggable<Logger::Id::filter> {
 public:
-  AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfigSharedPtr config,
-                            ConcurrencyControllerSharedPtr controller);
+  AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfig& config,
+                            Controller::ConcurrencyController& controller);
 
   // Http::StreamDecoderFilter
   Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override;
@@ -64,8 +64,8 @@ class AdaptiveConcurrencyFilter : public Http::PassThroughFilter,
   void onDestroy() override;
 
 private:
-  AdaptiveConcurrencyFilterConfigSharedPtr config_;
-  const ConcurrencyControllerSharedPtr controller_;
+  AdaptiveConcurrencyFilterConfig& config_;
+  Controller::ConcurrencyController& controller_;
   std::unique_ptr<Cleanup> deferred_sample_task_;
 };
 

source/extensions/filters/http/adaptive_concurrency/config.cc

@@ -18,24 +18,23 @@ Http::FilterFactoryCb AdaptiveConcurrencyFilterFactory::createFilterFactoryFromP
 
   auto acc_stats_prefix = stats_prefix + "adaptive_concurrency.";
 
-  std::shared_ptr<Controller::ConcurrencyController> controller;
   using Proto = envoy::extensions::filters::http::adaptive_concurrency::v3::AdaptiveConcurrency;
   ASSERT(config.concurrency_controller_config_case() ==
          Proto::ConcurrencyControllerConfigCase::kGradientControllerConfig);
   auto gradient_controller_config =
       Controller::GradientControllerConfig(config.gradient_controller_config(), context.runtime());
-  controller = std::make_shared<Controller::GradientController>(
+  auto controller = std::make_shared<Controller::GradientController>(
       std::move(gradient_controller_config), context.dispatcher(), context.runtime(),
       acc_stats_prefix + "gradient_controller.", context.scope(), context.api().randomGenerator(),
       context.timeSource());
 
-  AdaptiveConcurrencyFilterConfigSharedPtr filter_config(
-      new AdaptiveConcurrencyFilterConfig(config, context.runtime(), std::move(acc_stats_prefix),
-                                          context.scope(), context.timeSource()));
+  auto filter_config = std::make_shared<AdaptiveConcurrencyFilterConfig>(
+      config, context.runtime(), std::move(acc_stats_prefix), context.scope(),
+      context.timeSource());
 
   return [filter_config, controller](Http::FilterChainFactoryCallbacks& callbacks) -> void {
     callbacks.addStreamFilter(
-        std::make_shared<AdaptiveConcurrencyFilter>(filter_config, controller));
+        std::make_shared<AdaptiveConcurrencyFilter>(*filter_config, *controller));
   };
 }
 

source/extensions/filters/http/admission_control/admission_control.cc

@@ -56,13 +56,13 @@ double AdmissionControlFilterConfig::successRateThreshold() const {
   return std::min<double>(pct, 100.0) / 100.0;
 }
 
-AdmissionControlFilter::AdmissionControlFilter(AdmissionControlFilterConfigSharedPtr config,
+AdmissionControlFilter::AdmissionControlFilter(AdmissionControlFilterConfig& config,
                                                const std::string& stats_prefix)
-    : config_(std::move(config)), stats_(generateStats(config_->scope(), stats_prefix)),
-      record_request_(true) {}
+    : config_(config), stats_(generateStats(config_.scope(), stats_prefix)), record_request_(true) {
+}
 
 Http::FilterHeadersStatus AdmissionControlFilter::decodeHeaders(Http::RequestHeaderMap&, bool) {
-  if (!config_->filterEnabled() || decoder_callbacks_->streamInfo().healthCheck()) {
+  if (!config_.filterEnabled() || decoder_callbacks_->streamInfo().healthCheck()) {
     // We must forego recording the success/failure of this request during encoding.
     record_request_ = false;
     return Http::FilterHeadersStatus::Continue;
@@ -104,11 +104,11 @@ Http::FilterHeadersStatus AdmissionControlFilter::encodeHeaders(Http::ResponseHe
     }
 
     const uint32_t status = enumToInt(grpc_status.value());
-    successful_response = config_->responseEvaluator().isGrpcSuccess(status);
+    successful_response = config_.responseEvaluator().isGrpcSuccess(status);
   } else {
     // HTTP response.
     const uint64_t http_status = Http::Utility::getResponseStatus(headers);
-    successful_response = config_->responseEvaluator().isHttpSuccess(http_status);
+    successful_response = config_.responseEvaluator().isHttpSuccess(http_status);
   }
 
   if (successful_response) {
@@ -125,8 +125,7 @@ AdmissionControlFilter::encodeTrailers(Http::ResponseTrailerMap& trailers) {
   if (expect_grpc_status_in_trailer_) {
     absl::optional<GrpcStatus> grpc_status = Grpc::Common::getGrpcStatus(trailers, false);
 
-    if (grpc_status.has_value() &&
-        config_->responseEvaluator().isGrpcSuccess(grpc_status.value())) {
+    if (grpc_status.has_value() && config_.responseEvaluator().isGrpcSuccess(grpc_status.value())) {
       recordSuccess();
     } else {
       recordFailure();
@@ -139,19 +138,19 @@ AdmissionControlFilter::encodeTrailers(Http::ResponseTrailerMap& trailers) {
 bool AdmissionControlFilter::shouldRejectRequest() const {
   // This formula is documented in the admission control filter documentation:
   // https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/admission_control_filter.html
-  const auto request_counts = config_->getController().requestCounts();
+  const auto request_counts = config_.getController().requestCounts();
   const double total_requests = request_counts.requests;
   const double successful_requests = request_counts.successes;
-  double probability = total_requests - successful_requests / config_->successRateThreshold();
+  double probability = total_requests - successful_requests / config_.successRateThreshold();
   probability = probability / (total_requests + 1);
-  const auto aggression = config_->aggression();
+  const auto aggression = config_.aggression();
   if (aggression != 1.0) {
     probability = std::pow(probability, 1.0 / aggression);
   }
 
   // Choosing an accuracy of 4 significant figures for the probability.
   static constexpr uint64_t accuracy = 1e4;
-  auto r = config_->random().random();
+  auto r = config_.random().random();
   return (accuracy * std::max(probability, 0.0)) > (r % accuracy);
 }
 

source/extensions/filters/http/admission_control/admission_control.h

@@ -86,8 +86,7 @@ using AdmissionControlFilterConfigSharedPtr = std::shared_ptr<const AdmissionCon
 class AdmissionControlFilter : public Http::PassThroughFilter,
                                protected Logger::Loggable<Logger::Id::filter> {
 public:
-  AdmissionControlFilter(AdmissionControlFilterConfigSharedPtr config,
-                         const std::string& stats_prefix);
+  AdmissionControlFilter(AdmissionControlFilterConfig& config, const std::string& stats_prefix);
 
   // Http::StreamDecoderFilter
   Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override;
@@ -106,15 +105,15 @@ class AdmissionControlFilter : public Http::PassThroughFilter,
 
   void recordSuccess() {
     stats_.rq_success_.inc();
-    config_->getController().recordSuccess();
+    config_.getController().recordSuccess();
   }
 
   void recordFailure() {
     stats_.rq_failure_.inc();
-    config_->getController().recordFailure();
+    config_.getController().recordFailure();
   }
 
-  const AdmissionControlFilterConfigSharedPtr config_;
+  AdmissionControlFilterConfig& config_;
   AdmissionControlStats stats_;
   bool expect_grpc_status_in_trailer_{false};
 

source/extensions/filters/http/admission_control/config.cc

@@ -46,13 +46,12 @@ Http::FilterFactoryCb AdmissionControlFilterFactory::createFilterFactoryFromProt
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
 
-  AdmissionControlFilterConfigSharedPtr filter_config =
-      std::make_shared<AdmissionControlFilterConfig>(
-          config, context.runtime(), context.api().randomGenerator(), context.scope(),
-          std::move(tls), std::move(response_evaluator));
+  auto filter_config = std::make_shared<AdmissionControlFilterConfig>(
+      config, context.runtime(), context.api().randomGenerator(), context.scope(), std::move(tls),
+      std::move(response_evaluator));
 
   return [filter_config, prefix](Http::FilterChainFactoryCallbacks& callbacks) -> void {
-    callbacks.addStreamFilter(std::make_shared<AdmissionControlFilter>(filter_config, prefix));
+    callbacks.addStreamFilter(std::make_shared<AdmissionControlFilter>(*filter_config, prefix));
   };
 }
 

source/extensions/filters/http/aws_lambda/aws_lambda_filter.h

@@ -125,7 +125,7 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable<Logger::Id::filt
   // Convert the JSON response to a standard HTTP response.
   void dejsonizeResponse(Http::ResponseHeaderMap& headers, const Buffer::Instance& body,
                          Buffer::Instance& out);
-  const FilterSettings settings_;
+  const FilterSettings& settings_;
   FilterStats stats_;
   Http::RequestHeaderMap* request_headers_ = nullptr;
   Http::ResponseHeaderMap* response_headers_ = nullptr;

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

@@ -13,7 +13,7 @@ FilterConfigImpl::FilterConfigImpl(Extensions::Common::Aws::SignerPtr&& signer,
     : signer_(std::move(signer)), stats_(Filter::generateStats(stats_prefix, scope)),
       host_rewrite_(host_rewrite) {}
 
-Filter::Filter(const std::shared_ptr<FilterConfig>& config) : config_(config) {}
+Filter::Filter(FilterConfig& config) : config_(config) {}
 
 Extensions::Common::Aws::Signer& FilterConfigImpl::signer() { return *signer_; }
 
@@ -27,17 +27,17 @@ FilterStats Filter::generateStats(const std::string& prefix, Stats::Scope& scope
 }
 
 Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
-  const auto& host_rewrite = config_->hostRewrite();
+  const auto& host_rewrite = config_.hostRewrite();
   if (!host_rewrite.empty()) {
     headers.setHost(host_rewrite);
   }
 
   try {
-    config_->signer().sign(headers);
-    config_->stats().signing_added_.inc();
+    config_.signer().sign(headers);
+    config_.stats().signing_added_.inc();
   } catch (const EnvoyException& e) {
     ENVOY_LOG(debug, "signing failed: {}", e.what());
-    config_->stats().signing_failed_.inc();
+    config_.stats().signing_failed_.inc();
   }
 
   return Http::FilterHeadersStatus::Continue;

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.h

@@ -77,15 +77,15 @@ class FilterConfigImpl : public FilterConfig {
  */
 class Filter : public Http::PassThroughDecoderFilter, Logger::Loggable<Logger::Id::filter> {
 public:
-  Filter(const std::shared_ptr<FilterConfig>& config);
+  Filter(FilterConfig& config);
 
   static FilterStats generateStats(const std::string& prefix, Stats::Scope& scope);
 
   Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers,
                                           bool end_stream) override;
 
 private:
-  std::shared_ptr<FilterConfig> config_;
+  FilterConfig& config_;
 };
 
 } // namespace AwsRequestSigningFilter

source/extensions/filters/http/aws_request_signing/config.cc

@@ -28,8 +28,7 @@ Http::FilterFactoryCb AwsRequestSigningFilterFactory::createFilterFactoryFromPro
   auto filter_config = std::make_shared<FilterConfigImpl>(std::move(signer), stats_prefix,
                                                           context.scope(), config.host_rewrite());
   return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
-    auto filter = std::make_shared<Filter>(filter_config);
-    callbacks.addStreamDecoderFilter(filter);
+    callbacks.addStreamDecoderFilter(std::make_shared<Filter>(*filter_config));
   };
 }
 

source/extensions/filters/http/buffer/buffer_filter.cc

@@ -36,14 +36,14 @@ BufferFilterConfig::BufferFilterConfig(
     const envoy::extensions::filters::http::buffer::v3::Buffer& proto_config)
     : settings_(proto_config) {}
 
-BufferFilter::BufferFilter(BufferFilterConfigSharedPtr config)
-    : config_(config), settings_(config->settings()) {}
+BufferFilter::BufferFilter(BufferFilterConfig& config)
+    : config_(config), settings_(config.settings()) {}
 
 void BufferFilter::initConfig() {
   ASSERT(!config_initialized_);
   config_initialized_ = true;
 
-  settings_ = config_->settings();
+  settings_ = config_.settings();
 
   if (!callbacks_->route() || !callbacks_->route()->routeEntry()) {
     return;

source/extensions/filters/http/buffer/buffer_filter.h

@@ -48,7 +48,7 @@ using BufferFilterConfigSharedPtr = std::shared_ptr<BufferFilterConfig>;
  */
 class BufferFilter : public Http::StreamDecoderFilter {
 public:
-  BufferFilter(BufferFilterConfigSharedPtr config);
+  BufferFilter(BufferFilterConfig& config);
 
   // Http::StreamFilterBase
   void onDestroy() override {}
@@ -64,7 +64,7 @@ class BufferFilter : public Http::StreamDecoderFilter {
   void initConfig();
   void maybeAddContentLength();
 
-  BufferFilterConfigSharedPtr config_;
+  BufferFilterConfig& config_;
   const BufferFilterSettings* settings_;
   Http::StreamDecoderFilterCallbacks* callbacks_{};
   Http::RequestHeaderMap* request_headers_{};

source/extensions/filters/http/buffer/config.cc

@@ -22,7 +22,7 @@ Http::FilterFactoryCb BufferFilterFactory::createFilterFactoryFromProtoTyped(
 
   BufferFilterConfigSharedPtr filter_config(new BufferFilterConfig(proto_config));
   return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
-    callbacks.addStreamDecoderFilter(std::make_shared<BufferFilter>(filter_config));
+    callbacks.addStreamDecoderFilter(std::make_shared<BufferFilter>(*filter_config));
   };
 }