Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http filters: pass configs by reference instead of shared_ptr #14737

Closed
wants to merge 10 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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_;
};

Expand Down
11 changes: 5 additions & 6 deletions source/extensions/filters/http/adaptive_concurrency/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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();
Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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};

Expand Down
9 changes: 4 additions & 5 deletions source/extensions/filters/http/admission_control/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/http/aws_lambda/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ Http::FilterFactoryCb AwsLambdaFilterFactory::createFilterFactoryFromProtoTyped(
auto signer = std::make_shared<Extensions::Common::Aws::SignerImpl>(
service_name, region, std::move(credentials_provider), context.dispatcher().timeSource());

FilterSettings filter_settings{*arn, getInvocationMode(proto_config),
proto_config.payload_passthrough()};
auto filter_settings = std::make_shared<FilterSettings>(*arn, getInvocationMode(proto_config),
proto_config.payload_passthrough());

FilterStats stats = generateStats(stat_prefix, context.scope());
return [stats, signer, filter_settings](Http::FilterChainFactoryCallbacks& cb) {
auto filter = std::make_shared<Filter>(filter_settings, stats, signer);
auto filter = std::make_shared<Filter>(*filter_settings, stats, signer);
cb.addStreamFilter(filter);
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }

Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
}

Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/http/buffer/buffer_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/http/buffer/buffer_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand All @@ -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_{};
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/buffer/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
}

Expand Down
Loading