Skip to content

Commit 6cb1f1b

Browse files
ramaraochavaliramaraochavali
authored
clear old context from contexts when secret is updated (envoyproxy#13752)
When secret is updated via SDS, the old contexts still linger in the context list making the "/certs" render them and also "DaysUntilFirstCertExpiry" is incorrect - it reflects the replaced stats. Fixes envoyproxy#8614 Signed-off-by: Rama Chavali <[email protected]>
1 parent a4733f6 commit 6cb1f1b

File tree

10 files changed

+132
-57
lines changed

10 files changed

+132
-57
lines changed

‎include/envoy/ssl/context_manager.h

+6-4
Original file line numberDiff line numberDiff line change
@@ -22,23 +22,25 @@ class ContextManager {
2222
/**
2323
* Builds a ClientContext from a ClientContextConfig.
2424
*/
25-
virtual ClientContextSharedPtr createSslClientContext(Stats::Scope& scope,
26-
const ClientContextConfig& config) PURE;
25+
virtual ClientContextSharedPtr
26+
createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config,
27+
Envoy::Ssl::ClientContextSharedPtr old_context) PURE;
2728

2829
/**
2930
* Builds a ServerContext from a ServerContextConfig.
3031
*/
3132
virtual ServerContextSharedPtr
3233
createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config,
33-
const std::vector<std::string>& server_names) PURE;
34+
const std::vector<std::string>& server_names,
35+
Envoy::Ssl::ServerContextSharedPtr old_context) PURE;
3436

3537
/**
3638
* @return the number of days until the next certificate being managed will expire.
3739
*/
3840
virtual size_t daysUntilFirstCertExpires() const PURE;
3941

4042
/**
41-
* Iterate through all currently allocated contexts.
43+
* Iterates through the contexts currently attached to a listener.
4244
*/
4345
virtual void iterateContexts(std::function<void(const Context&)> callback) PURE;
4446

‎source/extensions/transport_sockets/tls/context_manager_impl.cc

+19-5
Original file line numberDiff line numberDiff line change
@@ -24,30 +24,44 @@ void ContextManagerImpl::removeEmptyContexts() {
2424
contexts_.remove_if([](const std::weak_ptr<Envoy::Ssl::Context>& n) { return n.expired(); });
2525
}
2626

27+
void ContextManagerImpl::removeOldContext(std::shared_ptr<Envoy::Ssl::Context> old_context) {
28+
if (old_context) {
29+
contexts_.remove_if([old_context](const std::weak_ptr<Envoy::Ssl::Context>& n) {
30+
std::shared_ptr<Envoy::Ssl::Context> sp = n.lock();
31+
if (sp) {
32+
return old_context == sp;
33+
}
34+
return false;
35+
});
36+
}
37+
}
38+
2739
Envoy::Ssl::ClientContextSharedPtr
2840
ContextManagerImpl::createSslClientContext(Stats::Scope& scope,
29-
const Envoy::Ssl::ClientContextConfig& config) {
41+
const Envoy::Ssl::ClientContextConfig& config,
42+
Envoy::Ssl::ClientContextSharedPtr old_context) {
3043
if (!config.isReady()) {
3144
return nullptr;
3245
}
3346

3447
Envoy::Ssl::ClientContextSharedPtr context =
3548
std::make_shared<ClientContextImpl>(scope, config, time_source_);
49+
removeOldContext(old_context);
3650
removeEmptyContexts();
3751
contexts_.emplace_back(context);
3852
return context;
3953
}
4054

41-
Envoy::Ssl::ServerContextSharedPtr
42-
ContextManagerImpl::createSslServerContext(Stats::Scope& scope,
43-
const Envoy::Ssl::ServerContextConfig& config,
44-
const std::vector<std::string>& server_names) {
55+
Envoy::Ssl::ServerContextSharedPtr ContextManagerImpl::createSslServerContext(
56+
Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config,
57+
const std::vector<std::string>& server_names, Envoy::Ssl::ServerContextSharedPtr old_context) {
4558
if (!config.isReady()) {
4659
return nullptr;
4760
}
4861

4962
Envoy::Ssl::ServerContextSharedPtr context =
5063
std::make_shared<ServerContextImpl>(scope, config, server_names, time_source_);
64+
removeOldContext(old_context);
5165
removeEmptyContexts();
5266
contexts_.emplace_back(context);
5367
return context;

‎source/extensions/transport_sockets/tls/context_manager_impl.h

+5-3
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,12 @@ class ContextManagerImpl final : public Envoy::Ssl::ContextManager {
2929

3030
// Ssl::ContextManager
3131
Ssl::ClientContextSharedPtr
32-
createSslClientContext(Stats::Scope& scope,
33-
const Envoy::Ssl::ClientContextConfig& config) override;
32+
createSslClientContext(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config,
33+
Envoy::Ssl::ClientContextSharedPtr old_context) override;
3434
Ssl::ServerContextSharedPtr
3535
createSslServerContext(Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config,
36-
const std::vector<std::string>& server_names) override;
36+
const std::vector<std::string>& server_names,
37+
Envoy::Ssl::ServerContextSharedPtr old_context) override;
3738
size_t daysUntilFirstCertExpires() const override;
3839
absl::optional<uint64_t> secondsUntilFirstOcspResponseExpires() const override;
3940
void iterateContexts(std::function<void(const Envoy::Ssl::Context&)> callback) override;
@@ -43,6 +44,7 @@ class ContextManagerImpl final : public Envoy::Ssl::ContextManager {
4344

4445
private:
4546
void removeEmptyContexts();
47+
void removeOldContext(std::shared_ptr<Envoy::Ssl::Context> old_context);
4648
TimeSource& time_source_;
4749
std::list<std::weak_ptr<Envoy::Ssl::Context>> contexts_;
4850
PrivateKeyMethodManagerImpl private_key_method_manager_{};

‎source/extensions/transport_sockets/tls/ssl_socket.cc

+4-4
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ ClientSslSocketFactory::ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPt
327327
Stats::Scope& stats_scope)
328328
: manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)),
329329
config_(std::move(config)),
330-
ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {
330+
ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_, nullptr)) {
331331
config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); });
332332
}
333333

@@ -357,7 +357,7 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() {
357357
ENVOY_LOG(debug, "Secret is updated.");
358358
{
359359
absl::WriterMutexLock l(&ssl_ctx_mu_);
360-
ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
360+
ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_, ssl_ctx_);
361361
}
362362
stats_.ssl_context_update_by_sds_.inc();
363363
}
@@ -368,7 +368,7 @@ ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPt
368368
const std::vector<std::string>& server_names)
369369
: manager_(manager), stats_scope_(stats_scope), stats_(generateStats("server", stats_scope)),
370370
config_(std::move(config)), server_names_(server_names),
371-
ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) {
371+
ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_, nullptr)) {
372372
config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); });
373373
}
374374

@@ -398,7 +398,7 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() {
398398
ENVOY_LOG(debug, "Secret is updated.");
399399
{
400400
absl::WriterMutexLock l(&ssl_ctx_mu_);
401-
ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
401+
ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_, ssl_ctx_);
402402
}
403403
stats_.ssl_context_update_by_sds_.inc();
404404
}

‎source/server/ssl_context_manager.cc

+4-2
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,16 @@ namespace Server {
1414
class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager {
1515
Ssl::ClientContextSharedPtr
1616
createSslClientContext(Stats::Scope& /* scope */,
17-
const Envoy::Ssl::ClientContextConfig& /* config */) override {
17+
const Envoy::Ssl::ClientContextConfig& /* config */,
18+
Envoy::Ssl::ClientContextSharedPtr /* old_context */) override {
1819
throwException();
1920
}
2021

2122
Ssl::ServerContextSharedPtr
2223
createSslServerContext(Stats::Scope& /* scope */,
2324
const Envoy::Ssl::ServerContextConfig& /* config */,
24-
const std::vector<std::string>& /* server_names */) override {
25+
const std::vector<std::string>& /* server_names */,
26+
Envoy::Ssl::ServerContextSharedPtr /* old_context */) override {
2527
throwException();
2628
}
2729

‎test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ class ClusterTest : public testing::Test,
4343
admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_store_,
4444
singleton_manager_, tls_, validation_visitor_, *api_);
4545
if (uses_tls) {
46-
EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _));
46+
EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _, _));
4747
}
4848
EXPECT_CALL(*dns_cache_manager_, getCache(_));
4949
// Below we return a nullptr handle which has no effect on the code under test but isn't

0 commit comments

Comments
 (0)