Skip to content

Commit 72db81d

Browse files
author
Raúl Gutiérrez Segalés
authored
network filters: avoid unnecessary std::shared_ptrs (#14711)
While debugging a crash in: #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: #8633 Signed-off-by: Raul Gutierrez Segales <[email protected]>
1 parent 7d48928 commit 72db81d

File tree

51 files changed

+294
-297
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

51 files changed

+294
-297
lines changed

‎source/common/tcp_proxy/tcp_proxy.cc‎

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -219,14 +219,12 @@ UpstreamDrainManager& Config::drainManager() {
219219
return upstream_drain_manager_slot_->getTyped<UpstreamDrainManager>();
220220
}
221221

222-
Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager)
222+
Filter::Filter(Config& config, Upstream::ClusterManager& cluster_manager)
223223
: config_(config), cluster_manager_(cluster_manager), downstream_callbacks_(*this),
224-
upstream_callbacks_(new UpstreamCallbacks(this)) {
225-
ASSERT(config != nullptr);
226-
}
224+
upstream_callbacks_(new UpstreamCallbacks(this)) {}
227225

228226
Filter::~Filter() {
229-
for (const auto& access_log : config_->accessLogs()) {
227+
for (const auto& access_log : config_.accessLogs()) {
230228
access_log->log(nullptr, nullptr, nullptr, getStreamInfo());
231229
}
232230

@@ -254,13 +252,13 @@ void Filter::initialize(Network::ReadFilterCallbacks& callbacks, bool set_connec
254252
// established.
255253
read_callbacks_->connection().readDisable(true);
256254

257-
config_->stats().downstream_cx_total_.inc();
255+
config_.stats().downstream_cx_total_.inc();
258256
if (set_connection_stats) {
259257
read_callbacks_->connection().setConnectionStats(
260-
{config_->stats().downstream_cx_rx_bytes_total_,
261-
config_->stats().downstream_cx_rx_bytes_buffered_,
262-
config_->stats().downstream_cx_tx_bytes_total_,
263-
config_->stats().downstream_cx_tx_bytes_buffered_, nullptr, nullptr});
258+
{config_.stats().downstream_cx_rx_bytes_total_,
259+
config_.stats().downstream_cx_rx_bytes_buffered_,
260+
config_.stats().downstream_cx_tx_bytes_total_,
261+
config_.stats().downstream_cx_tx_bytes_buffered_, nullptr, nullptr});
264262
}
265263
}
266264

@@ -295,9 +293,9 @@ void Filter::readDisableDownstream(bool disable) {
295293
read_callbacks_->connection().readDisable(disable);
296294

297295
if (disable) {
298-
config_->stats().downstream_flow_control_paused_reading_total_.inc();
296+
config_.stats().downstream_flow_control_paused_reading_total_.inc();
299297
} else {
300-
config_->stats().downstream_flow_control_resumed_reading_total_.inc();
298+
config_.stats().downstream_flow_control_resumed_reading_total_.inc();
301299
}
302300
}
303301

@@ -395,7 +393,7 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
395393
cluster_name);
396394
} else {
397395
ENVOY_CONN_LOG(debug, "Cluster not found {}", read_callbacks_->connection(), cluster_name);
398-
config_->stats().downstream_cx_no_route_.inc();
396+
config_.stats().downstream_cx_no_route_.inc();
399397
getStreamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound);
400398
onInitFailure(UpstreamFailureReason::NoRoute);
401399
return Network::FilterStatus::StopIteration;
@@ -413,7 +411,7 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
413411
return Network::FilterStatus::StopIteration;
414412
}
415413

416-
const uint32_t max_connect_attempts = config_->maxConnectAttempts();
414+
const uint32_t max_connect_attempts = config_.maxConnectAttempts();
417415
if (connect_attempts_ >= max_connect_attempts) {
418416
getStreamInfo().setResponseFlag(StreamInfo::ResponseFlag::UpstreamRetryLimitExceeded);
419417
cluster->stats().upstream_cx_connect_attempts_exceeded_.inc();
@@ -461,7 +459,7 @@ bool Filter::maybeTunnel(Upstream::ThreadLocalCluster& cluster) {
461459
return false;
462460
}
463461

464-
generic_conn_pool_ = factory->createGenericConnPool(cluster, config_->tunnelingConfig(), this,
462+
generic_conn_pool_ = factory->createGenericConnPool(cluster, config_.tunnelingConfig(), this,
465463
*upstream_callbacks_);
466464
if (generic_conn_pool_) {
467465
connecting_ = true;
@@ -561,10 +559,10 @@ Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) {
561559
}
562560

563561
Network::FilterStatus Filter::onNewConnection() {
564-
if (config_->maxDownstreamConnectionDuration()) {
562+
if (config_.maxDownstreamConnectionDuration()) {
565563
connection_duration_timer_ = read_callbacks_->connection().dispatcher().createTimer(
566564
[this]() -> void { onMaxDownstreamConnectionDuration(); });
567-
connection_duration_timer_->enableTimer(config_->maxDownstreamConnectionDuration().value());
565+
connection_duration_timer_->enableTimer(config_.maxDownstreamConnectionDuration().value());
568566
}
569567
return initializeUpstreamConnection();
570568
}
@@ -574,9 +572,9 @@ void Filter::onDownstreamEvent(Network::ConnectionEvent event) {
574572
Tcp::ConnectionPool::ConnectionDataPtr conn_data(upstream_->onDownstreamEvent(event));
575573
if (conn_data != nullptr &&
576574
conn_data->connection().state() != Network::Connection::State::Closed) {
577-
config_->drainManager().add(config_->sharedConfig(), std::move(conn_data),
578-
std::move(upstream_callbacks_), std::move(idle_timer_),
579-
read_callbacks_->upstreamHost());
575+
config_.drainManager().add(config_.sharedConfig(), std::move(conn_data),
576+
std::move(upstream_callbacks_), std::move(idle_timer_),
577+
read_callbacks_->upstreamHost());
580578
}
581579
if (event != Network::ConnectionEvent::Connected) {
582580
upstream_.reset();
@@ -640,7 +638,7 @@ void Filter::onUpstreamConnection() {
640638
ENVOY_CONN_LOG(debug, "TCP:onUpstreamEvent(), requestedServerName: {}",
641639
read_callbacks_->connection(), getStreamInfo().requestedServerName());
642640

643-
if (config_->idleTimeout()) {
641+
if (config_.idleTimeout()) {
644642
// The idle_timer_ can be moved to a Drainer, so related callbacks call into
645643
// the UpstreamCallbacks, which has the same lifetime as the timer, and can dispatch
646644
// the call to either TcpProxy or to Drainer, depending on the current state.
@@ -662,7 +660,7 @@ void Filter::onUpstreamConnection() {
662660

663661
void Filter::onIdleTimeout() {
664662
ENVOY_CONN_LOG(debug, "Session timed out", read_callbacks_->connection());
665-
config_->stats().idle_timeout_.inc();
663+
config_.stats().idle_timeout_.inc();
666664

667665
// This results in also closing the upstream connection.
668666
read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
@@ -671,14 +669,14 @@ void Filter::onIdleTimeout() {
671669
void Filter::onMaxDownstreamConnectionDuration() {
672670
ENVOY_CONN_LOG(debug, "max connection duration reached", read_callbacks_->connection());
673671
getStreamInfo().setResponseFlag(StreamInfo::ResponseFlag::DurationTimeout);
674-
config_->stats().max_downstream_connection_duration_.inc();
672+
config_.stats().max_downstream_connection_duration_.inc();
675673
read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
676674
}
677675

678676
void Filter::resetIdleTimer() {
679677
if (idle_timer_ != nullptr) {
680-
ASSERT(config_->idleTimeout());
681-
idle_timer_->enableTimer(config_->idleTimeout().value());
678+
ASSERT(config_.idleTimeout());
679+
idle_timer_->enableTimer(config_.idleTimeout().value());
682680
}
683681
}
684682

‎source/common/tcp_proxy/tcp_proxy.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ class Filter : public Network::ReadFilter,
245245
protected Logger::Loggable<Logger::Id::filter>,
246246
public GenericConnectionPoolCallbacks {
247247
public:
248-
Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager);
248+
Filter(Config& config, Upstream::ClusterManager& cluster_manager);
249249
~Filter() override;
250250

251251
// Network::ReadFilter
@@ -264,7 +264,7 @@ class Filter : public Network::ReadFilter,
264264
// Upstream::LoadBalancerContext
265265
const Router::MetadataMatchCriteria* metadataMatchCriteria() override;
266266
absl::optional<uint64_t> computeHashKey() override {
267-
auto hash_policy = config_->hashPolicy();
267+
auto hash_policy = config_.hashPolicy();
268268
if (hash_policy) {
269269
return hash_policy->generateHash(
270270
downstreamConnection()->addressProvider().remoteAddress().get(),
@@ -337,7 +337,7 @@ class Filter : public Network::ReadFilter,
337337

338338
// Callbacks for different error and success states during connection establishment
339339
virtual RouteConstSharedPtr pickRoute() {
340-
return config_->getRouteFromEntries(read_callbacks_->connection());
340+
return config_.getRouteFromEntries(read_callbacks_->connection());
341341
}
342342

343343
virtual void onInitFailure(UpstreamFailureReason) {
@@ -357,7 +357,7 @@ class Filter : public Network::ReadFilter,
357357
void disableIdleTimer();
358358
void onMaxDownstreamConnectionDuration();
359359

360-
const ConfigSharedPtr config_;
360+
Config& config_;
361361
Upstream::ClusterManager& cluster_manager_;
362362
Network::ReadFilterCallbacks* read_callbacks_{};
363363

‎source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ ClientSslAuthConfigSharedPtr ClientSslAuthConfig::create(
5353
return new_config;
5454
}
5555

56-
const AllowedPrincipals& ClientSslAuthConfig::allowedPrincipals() {
56+
const AllowedPrincipals& ClientSslAuthConfig::allowedPrincipals() const {
5757
return tls_->getTyped<AllowedPrincipals>();
5858
}
5959

@@ -98,7 +98,7 @@ Network::FilterStatus ClientSslAuthFilter::onNewConnection() {
9898
// If this is not an SSL connection, do no further checking. High layers should redirect, etc.
9999
// if SSL is required.
100100
if (!read_callbacks_->connection().ssl()) {
101-
config_->stats().auth_no_ssl_.inc();
101+
config_.stats().auth_no_ssl_.inc();
102102
return Network::FilterStatus::Continue;
103103
} else {
104104
// Otherwise we need to wait for handshake to be complete before proceeding.
@@ -112,21 +112,21 @@ void ClientSslAuthFilter::onEvent(Network::ConnectionEvent event) {
112112
}
113113

114114
ASSERT(read_callbacks_->connection().ssl());
115-
if (config_->ipAllowlist().contains(
115+
if (config_.ipAllowlist().contains(
116116
*read_callbacks_->connection().addressProvider().remoteAddress())) {
117-
config_->stats().auth_ip_allowlist_.inc();
117+
config_.stats().auth_ip_allowlist_.inc();
118118
read_callbacks_->continueReading();
119119
return;
120120
}
121121

122-
if (!config_->allowedPrincipals().allowed(
122+
if (!config_.allowedPrincipals().allowed(
123123
read_callbacks_->connection().ssl()->sha256PeerCertificateDigest())) {
124-
config_->stats().auth_digest_no_match_.inc();
124+
config_.stats().auth_digest_no_match_.inc();
125125
read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
126126
return;
127127
}
128128

129-
config_->stats().auth_digest_match_.inc();
129+
config_.stats().auth_digest_match_.inc();
130130
read_callbacks_->continueReading();
131131
}
132132

‎source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ class ClientSslAuthConfig : public Http::RestApiFetcher {
8080
ThreadLocal::SlotAllocator& tls, Upstream::ClusterManager& cm,
8181
Event::Dispatcher& dispatcher, Stats::Scope& scope, Random::RandomGenerator& random);
8282

83-
const AllowedPrincipals& allowedPrincipals();
84-
const Network::Address::IpList& ipAllowlist() { return ip_allowlist_; }
83+
const AllowedPrincipals& allowedPrincipals() const;
84+
const Network::Address::IpList& ipAllowlist() const { return ip_allowlist_; }
8585
GlobalStats& stats() { return stats_; }
8686

8787
private:
@@ -108,7 +108,7 @@ class ClientSslAuthConfig : public Http::RestApiFetcher {
108108
*/
109109
class ClientSslAuthFilter : public Network::ReadFilter, public Network::ConnectionCallbacks {
110110
public:
111-
ClientSslAuthFilter(ClientSslAuthConfigSharedPtr config) : config_(config) {}
111+
ClientSslAuthFilter(ClientSslAuthConfig& config) : config_(config) {}
112112

113113
// Network::ReadFilter
114114
Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override;
@@ -124,7 +124,7 @@ class ClientSslAuthFilter : public Network::ReadFilter, public Network::Connecti
124124
void onBelowWriteBufferLowWatermark() override {}
125125

126126
private:
127-
ClientSslAuthConfigSharedPtr config_;
127+
ClientSslAuthConfig& config_;
128128
Network::ReadFilterCallbacks* read_callbacks_{};
129129
};
130130

‎source/extensions/filters/network/client_ssl_auth/config.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ Network::FilterFactoryCb ClientSslAuthConfigFactory::createFilterFactoryFromProt
2222
proto_config, context.threadLocal(), context.clusterManager(), context.dispatcher(),
2323
context.scope(), context.api().randomGenerator()));
2424
return [filter_config](Network::FilterManager& filter_manager) -> void {
25-
filter_manager.addReadFilter(std::make_shared<ClientSslAuthFilter>(filter_config));
25+
filter_manager.addReadFilter(std::make_shared<ClientSslAuthFilter>(*filter_config));
2626
};
2727
}
2828

‎source/extensions/filters/network/ext_authz/config.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ Network::FilterFactoryCb ExtAuthzConfigFactory::createFilterFactoryFromProtoType
3838
async_client_factory->create(), std::chrono::milliseconds(timeout_ms),
3939
transport_api_version);
4040
filter_manager.addReadFilter(Network::ReadFilterSharedPtr{
41-
std::make_shared<Filter>(ext_authz_config, std::move(client))});
41+
std::make_shared<Filter>(*ext_authz_config, std::move(client))});
4242
};
4343
}
4444

‎source/extensions/filters/network/ext_authz/ext_authz.cc‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@ InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope
2323

2424
void Filter::callCheck() {
2525
Filters::Common::ExtAuthz::CheckRequestUtils::createTcpCheck(filter_callbacks_, check_request_,
26-
config_->includePeerCertificate());
26+
config_.includePeerCertificate());
2727

2828
status_ = Status::Calling;
29-
config_->stats().active_.inc();
30-
config_->stats().total_.inc();
29+
config_.stats().active_.inc();
30+
config_.stats().total_.inc();
3131

3232
calling_check_ = true;
3333
client_->check(*this, check_request_, Tracing::NullSpan::instance(),
@@ -37,7 +37,7 @@ void Filter::callCheck() {
3737

3838
Network::FilterStatus Filter::onData(Buffer::Instance&, bool /* end_stream */) {
3939
if (!filterEnabled(filter_callbacks_->connection().streamInfo().dynamicMetadata())) {
40-
config_->stats().disabled_.inc();
40+
config_.stats().disabled_.inc();
4141
return Network::FilterStatus::Continue;
4242
}
4343

@@ -62,40 +62,40 @@ void Filter::onEvent(Network::ConnectionEvent event) {
6262
// Make sure that any pending request in the client is cancelled. This will be NOP if the
6363
// request already completed.
6464
client_->cancel();
65-
config_->stats().active_.dec();
65+
config_.stats().active_.dec();
6666
}
6767
}
6868
}
6969

7070
void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
7171
status_ = Status::Complete;
72-
config_->stats().active_.dec();
72+
config_.stats().active_.dec();
7373

7474
switch (response->status) {
7575
case Filters::Common::ExtAuthz::CheckStatus::OK:
76-
config_->stats().ok_.inc();
76+
config_.stats().ok_.inc();
7777
break;
7878
case Filters::Common::ExtAuthz::CheckStatus::Error:
79-
config_->stats().error_.inc();
79+
config_.stats().error_.inc();
8080
break;
8181
case Filters::Common::ExtAuthz::CheckStatus::Denied:
82-
config_->stats().denied_.inc();
82+
config_.stats().denied_.inc();
8383
break;
8484
}
8585

8686
// Fail open only if configured to do so and if the check status was a error.
8787
if (response->status == Filters::Common::ExtAuthz::CheckStatus::Denied ||
8888
(response->status == Filters::Common::ExtAuthz::CheckStatus::Error &&
89-
!config_->failureModeAllow())) {
90-
config_->stats().cx_closed_.inc();
89+
!config_.failureModeAllow())) {
90+
config_.stats().cx_closed_.inc();
9191
filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
9292
} else {
9393
// Let the filter chain continue.
9494
filter_return_ = FilterReturn::Continue;
95-
if (config_->failureModeAllow() &&
95+
if (config_.failureModeAllow() &&
9696
response->status == Filters::Common::ExtAuthz::CheckStatus::Error) {
9797
// Status is Error and yet we are configured to allow traffic. Click a counter.
98-
config_->stats().failure_mode_allowed_.inc();
98+
config_.stats().failure_mode_allowed_.inc();
9999
}
100100

101101
if (!response->dynamic_metadata.fields().empty()) {

‎source/extensions/filters/network/ext_authz/ext_authz.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ class Filter : public Network::ReadFilter,
8787
public Network::ConnectionCallbacks,
8888
public Filters::Common::ExtAuthz::RequestCallbacks {
8989
public:
90-
Filter(ConfigSharedPtr config, Filters::Common::ExtAuthz::ClientPtr&& client)
90+
Filter(Config& config, Filters::Common::ExtAuthz::ClientPtr&& client)
9191
: config_(config), client_(std::move(client)) {}
9292
~Filter() override = default;
9393

@@ -119,10 +119,10 @@ class Filter : public Network::ReadFilter,
119119
void callCheck();
120120

121121
bool filterEnabled(const envoy::config::core::v3::Metadata& metadata) {
122-
return config_->filterEnabledMetadata(metadata);
122+
return config_.filterEnabledMetadata(metadata);
123123
}
124124

125-
ConfigSharedPtr config_;
125+
Config& config_;
126126
Filters::Common::ExtAuthz::ClientPtr client_;
127127
Network::ReadFilterCallbacks* filter_callbacks_{};
128128
Status status_{Status::NotStarted};

‎source/extensions/filters/network/local_ratelimit/config.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ Network::FilterFactoryCb LocalRateLimitConfigFactory::createFilterFactoryFromPro
1616
ConfigSharedPtr filter_config(
1717
new Config(proto_config, context.dispatcher(), context.scope(), context.runtime()));
1818
return [filter_config](Network::FilterManager& filter_manager) -> void {
19-
filter_manager.addReadFilter(std::make_shared<Filter>(filter_config));
19+
filter_manager.addReadFilter(std::make_shared<Filter>(*filter_config));
2020
};
2121
}
2222

‎source/extensions/filters/network/local_ratelimit/local_ratelimit.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,13 @@ LocalRateLimitStats Config::generateStats(const std::string& prefix, Stats::Scop
3232
bool Config::canCreateConnection() { return rate_limiter_.requestAllowed(descriptors_); }
3333

3434
Network::FilterStatus Filter::onNewConnection() {
35-
if (!config_->enabled()) {
35+
if (!config_.enabled()) {
3636
ENVOY_CONN_LOG(trace, "local_rate_limit: runtime disabled", read_callbacks_->connection());
3737
return Network::FilterStatus::Continue;
3838
}
3939

40-
if (!config_->canCreateConnection()) {
41-
config_->stats().rate_limited_.inc();
40+
if (!config_.canCreateConnection()) {
41+
config_.stats().rate_limited_.inc();
4242
ENVOY_CONN_LOG(trace, "local_rate_limit: rate limiting connection",
4343
read_callbacks_->connection());
4444
read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);

0 commit comments

Comments
 (0)