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

Revert "delay health checks until transport socket secrets are ready.… #13639

Merged
merged 1 commit into from
Oct 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 0 additions & 2 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ Bug Fixes
---------
*Changes expected to improve the state of the world and are unlikely to have negative effects*

* active health checks: health checks using a TLS transport socket and secrets delivered via :ref:`SDS <config_secret_discovery_service>` will now wait until secrets are loaded before the first health check attempt. This should improve startup times by not having to wait for the :ref:`no_traffic_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_interval>` until the next attempt.

* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.

Removed Config or Runtime
Expand Down
7 changes: 0 additions & 7 deletions include/envoy/network/transport_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,6 @@ class TransportSocketFactory {
*/
virtual TransportSocketPtr
createTransportSocket(TransportSocketOptionsSharedPtr options) const PURE;

/**
* @param a callback to be invoked when the secrets required by the created transport
* sockets are ready. Will be invoked immediately if no secrets are required or if they
* are already loaded.
*/
virtual void addReadyCb(std::function<void()> callback) PURE;
};

using TransportSocketFactoryPtr = std::unique_ptr<TransportSocketFactory>;
Expand Down
9 changes: 0 additions & 9 deletions include/envoy/upstream/upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,6 @@ class Host : virtual public HostDescription {
Network::TransportSocketOptionsSharedPtr transport_socket_options,
const envoy::config::core::v3::Metadata* metadata) const PURE;

/**
* Register a callback to be invoked when secrets are ready for the transport socket that
* corresponds to the provided metadata.
* @param callback supplies the callback to be invoked.
* @param metadata supplies the metadata to be used for resolving transport socket matches.
*/
virtual void addReadyCb(std::function<void()> callback,
const envoy::config::core::v3::Metadata* metadata) const PURE;

/**
* @return host specific gauges.
*/
Expand Down
1 change: 0 additions & 1 deletion source/common/network/raw_buffer_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class RawBufferSocketFactory : public TransportSocketFactory {
// Network::TransportSocketFactory
TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override;
bool implementsSecureTransport() const override;
void addReadyCb(std::function<void()> callback) override { callback(); }
};

} // namespace Network
Expand Down
8 changes: 0 additions & 8 deletions source/common/upstream/health_checker_base_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -384,14 +384,6 @@ void HealthCheckerImplBase::ActiveHealthCheckSession::onTimeoutBase() {
handleFailure(envoy::data::core::v3::NETWORK);
}

void HealthCheckerImplBase::ActiveHealthCheckSession::start() {
// Start health checks only after secrets are ready for the transport socket
// that health checks will be performed on. If health checks start
// immediately, they may fail with "network" errors due to TLS credentials
// not yet being loaded, which can result in long startup times.
host_->addReadyCb([this] { onInitialInterval(); }, parent_.transportSocketMatchMetadata().get());
}

void HealthCheckerImplBase::ActiveHealthCheckSession::onInitialInterval() {
if (parent_.initial_jitter_.count() == 0) {
onIntervalBase();
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/health_checker_base_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class HealthCheckerImplBase : public HealthChecker,
~ActiveHealthCheckSession() override;
HealthTransition setUnhealthy(envoy::data::core::v3::HealthCheckFailureType type);
void onDeferredDeleteBase();
void start();
void start() { onInitialInterval(); }

protected:
ActiveHealthCheckSession(HealthCheckerImplBase& parent, HostSharedPtr host);
Expand Down
8 changes: 0 additions & 8 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -356,14 +356,6 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu
return connection;
}

void HostImpl::addReadyCb(std::function<void()> callback,
const envoy::config::core::v3::Metadata* metadata) const {
Network::TransportSocketFactory& factory =
(metadata != nullptr) ? cluster_->transportSocketMatcher().resolve(metadata).factory_
: socket_factory_;
factory.addReadyCb(callback);
}

void HostImpl::weight(uint32_t new_weight) { weight_ = std::max(1U, new_weight); }

std::vector<HostsPerLocalityConstSharedPtr> HostsPerLocalityImpl::filter(
Expand Down
2 changes: 0 additions & 2 deletions source/common/upstream/upstream_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,6 @@ class HostImpl : public HostDescriptionImpl,
createHealthCheckConnection(Event::Dispatcher& dispatcher,
Network::TransportSocketOptionsSharedPtr transport_socket_options,
const envoy::config::core::v3::Metadata* metadata) const override;
void addReadyCb(std::function<void()> callback,
const envoy::config::core::v3::Metadata* metadata) const override;

std::vector<std::pair<absl::string_view, Stats::PrimitiveGaugeReference>>
gauges() const override {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory {
NOT_REACHED_GCOVR_EXCL_LINE;
}
bool implementsSecureTransport() const override { return true; }

// TODO(mpuncel) only invoke callback() once secrets are ready.
void addReadyCb(std::function<void()> callback) override { callback(); };
};

// TODO(danzh): when implement ProofSource, examine of it's necessary to
Expand Down
3 changes: 0 additions & 3 deletions source/extensions/transport_sockets/alts/tsi_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,6 @@ class TsiSocketFactory : public Network::TransportSocketFactory {
Network::TransportSocketPtr
createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;

// TODO(mpuncel) only invoke callback() once secrets are ready.
void addReadyCb(std::function<void()> callback) override { callback(); };

private:
HandshakerFactory handshaker_factory_;
HandshakeValidator handshake_validator_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor
Network::TransportSocketPtr
createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
bool implementsSecureTransport() const override;
void addReadyCb(std::function<void()> callback) override { callback(); };

private:
Network::TransportSocketFactoryPtr transport_socket_factory_;
Expand Down
2 changes: 0 additions & 2 deletions source/extensions/transport_sockets/tap/tap.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ class TapSocketFactory : public Network::TransportSocketFactory,
Network::TransportSocketPtr
createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
bool implementsSecureTransport() const override;
// TODO(mpuncel) only invoke callback() once secrets are ready.
void addReadyCb(std::function<void()> callback) override { callback(); };

private:
Network::TransportSocketFactoryPtr transport_socket_factory_;
Expand Down
52 changes: 0 additions & 52 deletions source/extensions/transport_sockets/tls/ssl_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -355,39 +355,13 @@ bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }

void ClientSslSocketFactory::onAddOrUpdateSecret() {
ENVOY_LOG(debug, "Secret is updated.");
bool should_run_callbacks = false;
{
absl::WriterMutexLock l(&ssl_ctx_mu_);
ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
if (ssl_ctx_) {
should_run_callbacks = true;
}
}
if (should_run_callbacks) {
for (const auto& cb : secrets_ready_callbacks_) {
cb();
}
secrets_ready_callbacks_.clear();
}
stats_.ssl_context_update_by_sds_.inc();
}

void ClientSslSocketFactory::addReadyCb(std::function<void()> callback) {
bool immediately_run_callback = false;
{
absl::ReaderMutexLock l(&ssl_ctx_mu_);
if (ssl_ctx_) {
immediately_run_callback = true;
}
}

if (immediately_run_callback) {
callback();
} else {
secrets_ready_callbacks_.push_back(callback);
}
}

ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config,
Envoy::Ssl::ContextManager& manager,
Stats::Scope& stats_scope,
Expand Down Expand Up @@ -422,39 +396,13 @@ bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }

void ServerSslSocketFactory::onAddOrUpdateSecret() {
ENVOY_LOG(debug, "Secret is updated.");
bool should_run_callbacks = false;
{
absl::WriterMutexLock l(&ssl_ctx_mu_);
ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);

if (ssl_ctx_) {
should_run_callbacks = true;
}
}
if (should_run_callbacks) {
for (const auto& cb : secrets_ready_callbacks_) {
cb();
}
secrets_ready_callbacks_.clear();
}
stats_.ssl_context_update_by_sds_.inc();
}

void ServerSslSocketFactory::addReadyCb(std::function<void()> callback) {
bool immediately_run_callback = false;
{
absl::ReaderMutexLock l(&ssl_ctx_mu_);
if (ssl_ctx_) {
immediately_run_callback = true;
}
}
if (immediately_run_callback) {
callback();
} else {
secrets_ready_callbacks_.push_back(callback);
}
}

} // namespace Tls
} // namespace TransportSockets
} // namespace Extensions
Expand Down
6 changes: 0 additions & 6 deletions source/extensions/transport_sockets/tls/ssl_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
bool implementsSecureTransport() const override;

void addReadyCb(std::function<void()> callback) override;

// Secret::SecretCallbacks
void onAddOrUpdateSecret() override;

Expand All @@ -121,7 +119,6 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
Envoy::Ssl::ClientContextConfigPtr config_;
mutable absl::Mutex ssl_ctx_mu_;
Envoy::Ssl::ClientContextSharedPtr ssl_ctx_ ABSL_GUARDED_BY(ssl_ctx_mu_);
std::list<std::function<void()>> secrets_ready_callbacks_;
};

class ServerSslSocketFactory : public Network::TransportSocketFactory,
Expand All @@ -136,8 +133,6 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
bool implementsSecureTransport() const override;

void addReadyCb(std::function<void()> callback) override;

// Secret::SecretCallbacks
void onAddOrUpdateSecret() override;

Expand All @@ -149,7 +144,6 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
const std::vector<std::string> server_names_;
mutable absl::Mutex ssl_ctx_mu_;
Envoy::Ssl::ServerContextSharedPtr ssl_ctx_ ABSL_GUARDED_BY(ssl_ctx_mu_);
std::list<std::function<void()>> secrets_ready_callbacks_;
};

} // namespace Tls
Expand Down
21 changes: 5 additions & 16 deletions test/common/upstream/health_checker_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -942,8 +942,6 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) {
Network::TransportSocketFactoryPtr(socket_factory));
cluster_->info_->transport_socket_matcher_.reset(transport_socket_match);

EXPECT_CALL(*socket_factory, addReadyCb(_))
.WillOnce(Invoke([&](std::function<void()> callback) -> void { callback(); }));
EXPECT_CALL(*socket_factory, createTransportSocket(ApplicationProtocolListEq("http1")));

allocHealthChecker(yaml);
Expand Down Expand Up @@ -2431,19 +2429,13 @@ TEST_F(HttpHealthCheckerImplTest, TransportSocketMatchCriteria) {
ALL_TRANSPORT_SOCKET_MATCH_STATS(POOL_COUNTER_PREFIX(stats_store, "test"))};
auto health_check_only_socket_factory = std::make_unique<Network::MockTransportSocketFactory>();

// We expect resolve() to be called 3 times, once for endpoint socket matching (with no metadata
// in this test) and twice for health check socket matching (once for checking if secrets are
// ready on the transport socket, and again for actually getting the health check transport socket
// to create a connection). In the latter 2 calls, we expect metadata that matches the above
// object.
// We expect resolve() to be called twice, once for endpoint socket matching (with no metadata in
// this test) and once for health check socket matching. In the latter we expect metadata that
// matches the above object.
EXPECT_CALL(*transport_socket_match, resolve(nullptr));
EXPECT_CALL(*transport_socket_match, resolve(MetadataEq(metadata)))
.Times(2)
.WillRepeatedly(Return(TransportSocketMatcher::MatchData(
*health_check_only_socket_factory, health_transport_socket_stats, "health_check_only")))
.RetiresOnSaturation();
EXPECT_CALL(*health_check_only_socket_factory, addReadyCb(_))
.WillOnce(Invoke([&](std::function<void()> callback) -> void { callback(); }));
.WillOnce(Return(TransportSocketMatcher::MatchData(
*health_check_only_socket_factory, health_transport_socket_stats, "health_check_only")));
// The health_check_only_socket_factory should be used to create a transport socket for the health
// check connection.
EXPECT_CALL(*health_check_only_socket_factory, createTransportSocket(_));
Expand Down Expand Up @@ -2479,9 +2471,6 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) {
)EOF";

auto default_socket_factory = std::make_unique<Network::MockTransportSocketFactory>();

EXPECT_CALL(*default_socket_factory, addReadyCb(_))
.WillOnce(Invoke([&](std::function<void()> callback) -> void { callback(); }));
// The default_socket_factory should be used to create a transport socket for the health check
// connection.
EXPECT_CALL(*default_socket_factory, createTransportSocket(_));
Expand Down
2 changes: 0 additions & 2 deletions test/common/upstream/transport_socket_matcher_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory {
MOCK_METHOD(bool, implementsSecureTransport, (), (const));
MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket,
(Network::TransportSocketOptionsSharedPtr), (const));
MOCK_METHOD(void, addReadyCb, (std::function<void()>));
FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {}
std::string id() const { return id_; }

Expand All @@ -49,7 +48,6 @@ class FooTransportSocketFactory
MOCK_METHOD(bool, implementsSecureTransport, (), (const));
MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket,
(Network::TransportSocketOptionsSharedPtr), (const));
MOCK_METHOD(void, addReadyCb, (std::function<void()>));

Network::TransportSocketFactoryPtr
createTransportSocketFactory(const Protobuf::Message& proto,
Expand Down
Loading