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.

Sign up for GitHub

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

Jump to bottom

Revert "network filters: avoid unnecessary std::shared_ptrs (#14711)" #14755

Merged
merged 1 commit into from
Jan 20, 2021
Merged

Revert "network filters: avoid unnecessary std::shared_ptrs (#14711)" #14755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 25 additions & 23 deletions source/common/tcp_proxy/tcp_proxy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,14 @@ UpstreamDrainManager& Config::drainManager() {
return upstream_drain_manager_slot_->getTyped<UpstreamDrainManager>();
}

Filter::Filter(Config& config, Upstream::ClusterManager& cluster_manager)
Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager)
: config_(config), cluster_manager_(cluster_manager), downstream_callbacks_(*this),
upstream_callbacks_(new UpstreamCallbacks(this)) {}
upstream_callbacks_(new UpstreamCallbacks(this)) {
ASSERT(config != nullptr);
}

Filter::~Filter() {
for (const auto& access_log : config_.accessLogs()) {
for (const auto& access_log : config_->accessLogs()) {
access_log->log(nullptr, nullptr, nullptr, getStreamInfo());
}

Expand Down Expand Up @@ -252,13 +254,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});
}
}

Expand Down Expand Up @@ -293,9 +295,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();
}
}

Expand Down Expand Up @@ -393,7 +395,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;
Expand All @@ -411,7 +413,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();
Expand Down Expand Up @@ -459,7 +461,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;
Expand Down Expand Up @@ -559,10 +561,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();
}
Expand All @@ -572,9 +574,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();
Expand Down Expand Up @@ -638,7 +640,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.
Expand All @@ -660,7 +662,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);
Expand All @@ -669,14 +671,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());
}
}

Expand Down
8 changes: 4 additions & 4 deletions source/common/tcp_proxy/tcp_proxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ class Filter : public Network::ReadFilter,
protected Logger::Loggable<Logger::Id::filter>,
public GenericConnectionPoolCallbacks {
public:
Filter(Config& config, Upstream::ClusterManager& cluster_manager);
Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager);
~Filter() override;

// Network::ReadFilter
Expand All @@ -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(),
Expand Down Expand Up @@ -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) {
Expand All @@ -357,7 +357,7 @@ class Filter : public Network::ReadFilter,
void disableIdleTimer();
void onMaxDownstreamConnectionDuration();

Config& config_;
const ConfigSharedPtr config_;
Upstream::ClusterManager& cluster_manager_;
Network::ReadFilterCallbacks* read_callbacks_{};

Expand Down
14 changes: 7 additions & 7 deletions source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ ClientSslAuthConfigSharedPtr ClientSslAuthConfig::create(
return new_config;
}

const AllowedPrincipals& ClientSslAuthConfig::allowedPrincipals() const {
const AllowedPrincipals& ClientSslAuthConfig::allowedPrincipals() {
return tls_->getTyped<AllowedPrincipals>();
}

Expand Down Expand Up @@ -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.
Expand All @@ -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();
}

Expand Down
8 changes: 4 additions & 4 deletions source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
const Network::Address::IpList& ipAllowlist() const { return ip_allowlist_; }
const AllowedPrincipals& allowedPrincipals();
const Network::Address::IpList& ipAllowlist() { return ip_allowlist_; }
GlobalStats& stats() { return stats_; }

private:
Expand All @@ -108,7 +108,7 @@ class ClientSslAuthConfig : public Http::RestApiFetcher {
*/
class ClientSslAuthFilter : public Network::ReadFilter, public Network::ConnectionCallbacks {
public:
ClientSslAuthFilter(ClientSslAuthConfig& config) : config_(config) {}
ClientSslAuthFilter(ClientSslAuthConfigSharedPtr config) : config_(config) {}

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

private:
ClientSslAuthConfig& config_;
ClientSslAuthConfigSharedPtr config_;
Network::ReadFilterCallbacks* read_callbacks_{};
};

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/client_ssl_auth/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
}

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/ext_authz/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))});
};
}

Expand Down
26 changes: 13 additions & 13 deletions source/extensions/filters/network/ext_authz/ext_authz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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;
}

Expand All @@ -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()) {
Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/network/ext_authz/ext_authz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class Filter : public Network::ReadFilter,
public Network::ConnectionCallbacks,
public Filters::Common::ExtAuthz::RequestCallbacks {
public:
Filter(Config& config, Filters::Common::ExtAuthz::ClientPtr&& client)
Filter(ConfigSharedPtr config, Filters::Common::ExtAuthz::ClientPtr&& client)
: config_(config), client_(std::move(client)) {}
~Filter() override = default;

Expand Down Expand Up @@ -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);
}

Config& config_;
ConfigSharedPtr config_;
Filters::Common::ExtAuthz::ClientPtr client_;
Network::ReadFilterCallbacks* filter_callbacks_{};
Status status_{Status::NotStarted};
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/local_ratelimit/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
}

Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/network/local_ratelimit/local_ratelimit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading