From a9ec642fd0ff5341da8ea3c365ac3255f537ce78 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 13 Jun 2018 22:09:41 -0700 Subject: [PATCH 01/23] clusters and listeners read static secrets from Bootstrap.static_resources Signed-off-by: Jae Kim --- include/envoy/upstream/cluster_manager.h | 6 +- .../common/upstream/cluster_manager_impl.cc | 18 +++-- source/common/upstream/cluster_manager_impl.h | 10 ++- source/common/upstream/eds.cc | 2 +- source/common/upstream/eds.h | 2 +- source/common/upstream/logical_dns_cluster.h | 4 +- source/common/upstream/original_dst_cluster.h | 3 +- source/common/upstream/upstream_impl.cc | 33 ++++---- source/common/upstream/upstream_impl.h | 7 +- .../config_validation/cluster_manager.cc | 12 +-- .../config_validation/cluster_manager.h | 5 +- source/server/configuration_impl.cc | 2 +- .../upstream/cluster_manager_impl_test.cc | 77 +++++++++++-------- test/common/upstream/eds_test.cc | 4 +- .../upstream/logical_dns_cluster_test.cc | 3 +- .../upstream/original_dst_cluster_test.cc | 4 +- test/common/upstream/sds_test.cc | 4 +- test/common/upstream/upstream_impl_test.cc | 57 +++++++++----- .../config_validation/cluster_manager_test.cc | 2 +- 19 files changed, 152 insertions(+), 103 deletions(-) diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index f2ec6e672953e..de4eda328e689 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -237,7 +237,8 @@ class ClusterManagerFactory { clusterManagerFromProto(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin) PURE; + AccessLog::AccessLogManager& log_manager, Server::Admin& admin, + Secret::SecretManager& secret_manager) PURE; /** * Allocate an HTTP connection pool for the host. Pools are separated by 'priority', @@ -254,7 +255,8 @@ class ClusterManagerFactory { virtual ClusterSharedPtr clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api) PURE; + bool added_via_api, + Secret::SecretManager& secret_manager) PURE; /** * Create a CDS API provider from configuration proto. diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 42e0c2b89b892..43a1e6997fa97 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -170,13 +170,14 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, - Server::Admin& admin) + Server::Admin& admin, Secret::SecretManager& secret_manager) : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()), random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()), local_info_(local_info), cm_stats_(generateStats(stats)), init_helper_([this](Cluster& cluster) { onClusterInit(cluster); }), config_tracker_entry_( - admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })) { + admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })), + secret_manager_(secret_manager) { async_client_manager_ = std::make_unique(*this, tls); const auto& cm_config = bootstrap.cluster_manager(); if (cm_config.has_outlier_detection()) { @@ -472,8 +473,8 @@ bool ClusterManagerImpl::removeCluster(const std::string& cluster_name) { void ClusterManagerImpl::loadCluster(const envoy::api::v2::Cluster& cluster, const std::string& version_info, bool added_via_api, ClusterMap& cluster_map) { - ClusterSharedPtr new_cluster = - factory_.clusterFromProto(cluster, *this, outlier_event_logger_, added_via_api); + ClusterSharedPtr new_cluster = factory_.clusterFromProto(cluster, *this, outlier_event_logger_, + added_via_api, secret_manager_); if (!added_via_api) { if (cluster_map.find(new_cluster->info()->name()) != cluster_map.end()) { @@ -946,10 +947,10 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, - Server::Admin& admin) { + Server::Admin& admin, Secret::SecretManager& secret_manager) { return ClusterManagerPtr{new ClusterManagerImpl(bootstrap, *this, stats, tls, runtime, random, local_info, log_manager, main_thread_dispatcher_, - admin)}; + admin, secret_manager)}; } Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( @@ -967,10 +968,11 @@ Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( ClusterSharedPtr ProdClusterManagerFactory::clusterFromProto( const envoy::api::v2::Cluster& cluster, ClusterManager& cm, - Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api) { + Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api, + Secret::SecretManager& secret_manager) { return ClusterImplBase::create(cluster, cm, stats_, tls_, dns_resolver_, ssl_context_manager_, runtime_, random_, main_thread_dispatcher_, local_info_, - outlier_event_logger, added_via_api); + outlier_event_logger, added_via_api, secret_manager); } CdsApiPtr ProdClusterManagerFactory::createCds( diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 4c149edaaa2fb..1f1baf65471a0 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -48,14 +48,16 @@ class ProdClusterManagerFactory : public ClusterManagerFactory { clusterManagerFromProto(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin) override; + AccessLog::AccessLogManager& log_manager, Server::Admin& admin, + Secret::SecretManager& secret_manager) override; Http::ConnectionPool::InstancePtr allocateConnPool(Event::Dispatcher& dispatcher, HostConstSharedPtr host, ResourcePriority priority, Http::Protocol protocol, const Network::ConnectionSocket::OptionsSharedPtr& options) override; ClusterSharedPtr clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api) override; + bool added_via_api, + Secret::SecretManager& secret_manager) override; CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, const absl::optional& eds_config, ClusterManager& cm) override; @@ -155,7 +157,8 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable #include +#include "envoy/secret/secret_manager.h" #include "envoy/thread_local/thread_local.h" #include "common/common/empty_string.h" @@ -31,7 +32,8 @@ class LogicalDnsCluster : public ClusterImplBase { LogicalDnsCluster(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, Network::DnsResolverSharedPtr dns_resolver, ThreadLocal::SlotAllocator& tls, - ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api); + ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api, + Secret::SecretManager& secret_manager); ~LogicalDnsCluster(); diff --git a/source/common/upstream/original_dst_cluster.h b/source/common/upstream/original_dst_cluster.h index 5cb5107a3a4aa..86b008426debf 100644 --- a/source/common/upstream/original_dst_cluster.h +++ b/source/common/upstream/original_dst_cluster.h @@ -25,7 +25,8 @@ class OriginalDstCluster : public ClusterImplBase { public: OriginalDstCluster(const envoy::api::v2::Cluster& config, Runtime::Loader& runtime, Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api); + ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api, + Secret::SecretManager& secret_manager); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Primary; } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index ea36ac333fe28..304110b75a389 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -345,15 +345,13 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, } } -ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, - Stats::Store& stats, ThreadLocal::Instance& tls, - Network::DnsResolverSharedPtr dns_resolver, - Ssl::ContextManager& ssl_context_manager, - Runtime::Loader& runtime, Runtime::RandomGenerator& random, - Event::Dispatcher& dispatcher, - const LocalInfo::LocalInfo& local_info, - Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api) { +ClusterSharedPtr ClusterImplBase::create( + const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Stats::Store& stats, + ThreadLocal::Instance& tls, Network::DnsResolverSharedPtr dns_resolver, + Ssl::ContextManager& ssl_context_manager, Runtime::Loader& runtime, + Runtime::RandomGenerator& random, Event::Dispatcher& dispatcher, + const LocalInfo::LocalInfo& local_info, Outlier::EventLoggerSharedPtr outlier_event_logger, + bool added_via_api, Secret::SecretManager& secret_manager) { std::unique_ptr new_cluster; // We make this a shared pointer to deal with the distinct ownership @@ -375,18 +373,18 @@ ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, switch (cluster.type()) { case envoy::api::v2::Cluster::STATIC: - new_cluster.reset( - new StaticClusterImpl(cluster, runtime, stats, ssl_context_manager, cm, added_via_api)); + new_cluster.reset(new StaticClusterImpl(cluster, runtime, stats, ssl_context_manager, cm, + added_via_api, secret_manager)); break; case envoy::api::v2::Cluster::STRICT_DNS: new_cluster.reset(new StrictDnsClusterImpl(cluster, runtime, stats, ssl_context_manager, - selected_dns_resolver, cm, dispatcher, - added_via_api)); + selected_dns_resolver, cm, dispatcher, added_via_api, + secret_manager)); break; case envoy::api::v2::Cluster::LOGICAL_DNS: new_cluster.reset(new LogicalDnsCluster(cluster, runtime, stats, ssl_context_manager, selected_dns_resolver, tls, cm, dispatcher, - added_via_api)); + added_via_api, secret_manager)); break; case envoy::api::v2::Cluster::ORIGINAL_DST: if (cluster.lb_policy() != envoy::api::v2::Cluster::ORIGINAL_DST_LB) { @@ -398,7 +396,7 @@ ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, "cluster: cluster type 'original_dst' may not be used with lb_subset_config")); } new_cluster.reset(new OriginalDstCluster(cluster, runtime, stats, ssl_context_manager, cm, - dispatcher, added_via_api)); + dispatcher, added_via_api, secret_manager)); break; case envoy::api::v2::Cluster::EDS: if (!cluster.has_eds_cluster_config()) { @@ -407,7 +405,7 @@ ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, // We map SDS to EDS, since EDS provides backwards compatibility with SDS. new_cluster.reset(new EdsClusterImpl(cluster, runtime, stats, ssl_context_manager, local_info, - cm, dispatcher, random, added_via_api)); + cm, dispatcher, random, added_via_api, secret_manager)); break; default: NOT_REACHED; @@ -797,7 +795,8 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const envoy::api::v2::Cluster& cluste Ssl::ContextManager& ssl_context_manager, Network::DnsResolverSharedPtr dns_resolver, ClusterManager& cm, Event::Dispatcher& dispatcher, - bool added_via_api) + bool added_via_api, + Secret::SecretManager& secret_manager) : BaseDynamicClusterImpl(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, cm.clusterManagerFactory().secretManager(), added_via_api), dns_resolver_(dns_resolver), diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 6540b8ac4ca8a..5e77af10f771e 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -420,7 +420,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable ClusterSharedPtr { - return ClusterImplBase::create(cluster, cm, stats_, tls_, dns_resolver_, - ssl_context_manager_, runtime_, random_, dispatcher_, - local_info_, outlier_event_logger, added_via_api); - })); + ON_CALL(*this, clusterFromProto_(_, _, _, _, _)) + .WillByDefault( + Invoke([&](const envoy::api::v2::Cluster& cluster, ClusterManager& cm, + Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api, + Secret::SecretManager& secret_manager) -> ClusterSharedPtr { + return ClusterImplBase::create( + cluster, cm, stats_, tls_, dns_resolver_, ssl_context_manager_, runtime_, random_, + dispatcher_, local_info_, outlier_event_logger, added_via_api, secret_manager); + })); } Http::ConnectionPool::InstancePtr @@ -69,8 +71,9 @@ class TestClusterManagerFactory : public ClusterManagerFactory { ClusterSharedPtr clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api) override { - return clusterFromProto_(cluster, cm, outlier_event_logger, added_via_api); + bool added_via_api, + Secret::SecretManager& secret_manager) override { + return clusterFromProto_(cluster, cm, outlier_event_logger, added_via_api, secret_manager); } CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource&, @@ -83,24 +86,30 @@ class TestClusterManagerFactory : public ClusterManagerFactory { clusterManagerFromProto(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin) override { - return ClusterManagerPtr{clusterManagerFromProto_(bootstrap, stats, tls, runtime, random, - local_info, log_manager, admin)}; + AccessLog::AccessLogManager& log_manager, Server::Admin& admin, + Secret::SecretManager& secret_manager) override { + return ClusterManagerPtr{clusterManagerFromProto_( + bootstrap, stats, tls, runtime, random, local_info, log_manager, admin, secret_manager)}; } +<<<<<<< HEAD Secret::SecretManager& secretManager() override { return secret_manager_; } MOCK_METHOD8(clusterManagerFromProto_, +======= + MOCK_METHOD9(clusterManagerFromProto_, +>>>>>>> clusters and listeners read static secrets from Bootstrap.static_resources ClusterManager*(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin)); + AccessLog::AccessLogManager& log_manager, Server::Admin& admin, + Secret::SecretManager& secret_manager)); MOCK_METHOD1(allocateConnPool_, Http::ConnectionPool::Instance*(HostConstSharedPtr host)); - MOCK_METHOD4(clusterFromProto_, + MOCK_METHOD5(clusterFromProto_, ClusterSharedPtr(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api)); + bool added_via_api, Secret::SecretManager& secret_manager)); MOCK_METHOD0(createCds_, CdsApi*()); Stats::IsolatedStoreImpl stats_; @@ -109,6 +118,7 @@ class TestClusterManagerFactory : public ClusterManagerFactory { new NiceMock}; NiceMock runtime_; NiceMock random_; + Secret::MockSecretManager secret_manager_; Ssl::ContextManagerImpl ssl_context_manager_{runtime_}; NiceMock dispatcher_; LocalInfo::MockLocalInfo local_info_; @@ -120,7 +130,7 @@ class ClusterManagerImplTest : public testing::Test { void create(const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { cluster_manager_.reset(new ClusterManagerImpl( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_)); + factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, secret_manager_)); } void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active, @@ -146,6 +156,7 @@ class ClusterManagerImplTest : public testing::Test { std::unique_ptr cluster_manager_; AccessLog::MockAccessLogManager log_manager_; NiceMock admin_; + Secret::MockSecretManager secret_manager_; }; envoy::config::bootstrap::v2::Bootstrap parseBootstrapFromJson(const std::string& json_string) { @@ -487,7 +498,7 @@ class ClusterManagerImplThreadAwareLbTest : public ClusterManagerImplTest { cluster1->info_->lb_type_ = lb_type; InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); create(parseBootstrapFromJson(json)); @@ -667,11 +678,11 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { // This part tests static init. InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cds_cluster)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cds_cluster)); ON_CALL(*cds_cluster, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster2)); ON_CALL(*cluster2, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); EXPECT_CALL(factory_, createCds_()).WillOnce(Return(cds)); EXPECT_CALL(*cds, setInitializedCb(_)); @@ -698,16 +709,16 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { std::shared_ptr cluster5(new NiceMock()); cluster5->info_->name_ = "cluster5"; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster3)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster3)); ON_CALL(*cluster3, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster3"), "version1"); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster4)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster4)); ON_CALL(*cluster4, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*cluster4, initialize(_)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster4"), "version2"); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster5)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster5)); ON_CALL(*cluster5, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster5"), "version3"); @@ -801,7 +812,7 @@ TEST_F(ClusterManagerImplTest, DynamicRemoveWithLocalCluster) { std::shared_ptr foo(new NiceMock()); foo->info_->name_ = "foo"; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, false)).WillOnce(Return(foo)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, false, _)).WillOnce(Return(foo)); ON_CALL(*foo, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*foo, initialize(_)); @@ -812,7 +823,7 @@ TEST_F(ClusterManagerImplTest, DynamicRemoveWithLocalCluster) { // cluster in its load balancer. std::shared_ptr cluster1(new NiceMock()); cluster1->info_->name_ = "cluster1"; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, true)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, true, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*cluster1, initialize(_)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster1"), ""); @@ -855,7 +866,7 @@ TEST_F(ClusterManagerImplTest, RemoveWarmingCluster) { cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); std::shared_ptr cluster1(new NiceMock()); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(*cluster1, initializePhase()).Times(0); EXPECT_CALL(*cluster1, initialize(_)); EXPECT_TRUE( @@ -900,7 +911,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { cluster_manager_->addThreadLocalClusterUpdateCallbacks(*callbacks); std::shared_ptr cluster1(new NiceMock()); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(*cluster1, initializePhase()).Times(0); EXPECT_CALL(*cluster1, initialize(_)); EXPECT_CALL(*callbacks, onClusterAddOrUpdate(_)).Times(1); @@ -922,7 +933,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { std::shared_ptr cluster2(new NiceMock()); cluster2->prioritySet().getMockHostSet(0)->hosts_ = { makeTestHost(cluster2->info_, "tcp://127.0.0.1:80")}; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); EXPECT_CALL(*cluster2, initialize(_)) .WillOnce(Invoke([cluster1](std::function initialize_callback) { @@ -980,7 +991,7 @@ TEST_F(ClusterManagerImplTest, addOrUpdateClusterStaticExists) { fmt::sprintf("{%s}", clustersJson({defaultStaticClusterJson("some_cluster")})); std::shared_ptr cluster1(new NiceMock()); InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*cluster1, initialize(_)); @@ -1024,7 +1035,7 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { { InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); EXPECT_CALL(outlier_detector, addChangedStateCb(_)); EXPECT_CALL(*cluster1, initialize(_)) @@ -1096,7 +1107,7 @@ TEST_F(ClusterManagerImplTest, CloseTcpConnectionsOnHealthFailure) { { InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); EXPECT_CALL(outlier_detector, addChangedStateCb(_)); EXPECT_CALL(*cluster1, initialize(_)) @@ -1168,7 +1179,7 @@ TEST_F(ClusterManagerImplTest, DoNotCloseTcpConnectionsOnHealthFailure) { Network::MockClientConnection* connection1 = new NiceMock(); Host::CreateConnectionData conn_info1; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); EXPECT_CALL(outlier_detector, addChangedStateCb(_)); EXPECT_CALL(*cluster1, initialize(_)) diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index ee7e6139444af..d69592d29e843 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -47,12 +47,14 @@ class EdsTest : public testing::Test { eds_cluster_ = parseClusterFromV2Yaml(yaml_config); Upstream::ClusterManager::ClusterInfoMap cluster_map; Upstream::MockCluster cluster; + Secret::MockSecretManager secret_manager; cluster_map.emplace("eds", cluster); EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); cluster_.reset(new EdsClusterImpl(eds_cluster_, runtime_, stats_, ssl_context_manager_, - local_info_, cm_, dispatcher_, random_, false)); + local_info_, cm_, dispatcher_, random_, false, + secret_manager)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); } diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 6f09675f48533..a4fb442a490e1 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -29,11 +29,12 @@ namespace Upstream { class LogicalDnsClusterTest : public testing::Test { public: void setup(const std::string& json) { + Secret::MockSecretManager secret_manager; resolve_timer_ = new Event::MockTimer(&dispatcher_); NiceMock cm; cluster_.reset(new LogicalDnsCluster(parseClusterFromJson(json), runtime_, stats_store_, ssl_context_manager_, dns_resolver_, tls_, cm, dispatcher_, - false)); + false, secret_manager)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index 098723c4d0ddc..a3832d048e62e 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -59,8 +59,10 @@ class OriginalDstClusterTest : public testing::Test { void setup(const std::string& json) { NiceMock cm; + Secret::MockSecretManager secret_manager; cluster_.reset(new OriginalDstCluster(parseClusterFromJson(json), runtime_, stats_store_, - ssl_context_manager_, cm, dispatcher_, false)); + ssl_context_manager_, cm, dispatcher_, false, + secret_manager)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); diff --git a/test/common/upstream/sds_test.cc b/test/common/upstream/sds_test.cc index cd01ae7539ccb..c8876ec4a4838 100644 --- a/test/common/upstream/sds_test.cc +++ b/test/common/upstream/sds_test.cc @@ -58,12 +58,14 @@ class SdsTest : public testing::Test { sds_cluster_ = parseSdsClusterFromJson(raw_config, eds_config); Upstream::ClusterManager::ClusterInfoMap cluster_map; Upstream::MockCluster cluster; + Secret::MockSecretManager secret_manager; cluster_map.emplace("sds", cluster); EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); cluster_.reset(new EdsClusterImpl(sds_cluster_, runtime_, stats_, ssl_context_manager_, - local_info_, cm_, dispatcher_, random_, false)); + local_info_, cm_, dispatcher_, random_, false, + secret_manager)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 17888a9006304..811f5f43f2aba 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -120,6 +120,7 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { NiceMock dispatcher; NiceMock runtime; ReadyWatcher initialized; + Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { @@ -141,7 +142,7 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { })); NiceMock cm; StrictDnsClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false); + dns_resolver, cm, dispatcher, false, secret_manager); cluster.initialize([&]() -> void { initialized.ready(); }); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -156,6 +157,7 @@ TEST(StrictDnsClusterImplTest, ZeroHostsHealthChecker) { NiceMock runtime; NiceMock cm; ReadyWatcher initialized; + Secret::MockSecretManager secret_manager; const std::string yaml = R"EOF( name: name @@ -167,7 +169,7 @@ TEST(StrictDnsClusterImplTest, ZeroHostsHealthChecker) { ResolverData resolver(*dns_resolver, dispatcher); StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false); + dns_resolver, cm, dispatcher, false, secret_manager); std::shared_ptr health_checker(new MockHealthChecker()); EXPECT_CALL(*health_checker, start()); EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)); @@ -188,6 +190,7 @@ TEST(StrictDnsClusterImplTest, Basic) { auto dns_resolver = std::make_shared>(); NiceMock dispatcher; NiceMock runtime; + Secret::MockSecretManager secret_manager; // gmock matches in LIFO order which is why these are swapped. ResolverData resolver2(*dns_resolver, dispatcher); @@ -225,7 +228,7 @@ TEST(StrictDnsClusterImplTest, Basic) { NiceMock cm; StrictDnsClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false); + dns_resolver, cm, dispatcher, false, secret_manager); EXPECT_CALL(runtime.snapshot_, getInteger("circuit_breakers.name.default.max_connections", 43)); EXPECT_EQ(43U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); EXPECT_CALL(runtime.snapshot_, @@ -329,6 +332,7 @@ TEST(StrictDnsClusterImplTest, HostRemovalActiveHealthSkipped) { NiceMock dispatcher; NiceMock runtime; NiceMock cm; + Secret::MockSecretManager secret_manager; const std::string yaml = R"EOF( name: name @@ -341,7 +345,7 @@ TEST(StrictDnsClusterImplTest, HostRemovalActiveHealthSkipped) { ResolverData resolver(*dns_resolver, dispatcher); StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false); + dns_resolver, cm, dispatcher, false, secret_manager); std::shared_ptr health_checker(new MockHealthChecker()); EXPECT_CALL(*health_checker, start()); EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)); @@ -423,6 +427,7 @@ TEST(StaticClusterImplTest, EmptyHostname) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; + Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "staticcluster", @@ -435,7 +440,7 @@ TEST(StaticClusterImplTest, EmptyHostname) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false); + false, secret_manager); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -447,6 +452,7 @@ TEST(StaticClusterImplTest, AltStatName) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; + Secret::MockSecretManager secret_manager; const std::string yaml = R"EOF( name: staticcluster @@ -459,7 +465,7 @@ TEST(StaticClusterImplTest, AltStatName) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, cm, - false); + false, secret_manager); cluster.initialize([] {}); // Increment a stat and verify it is emitted with alt_stat_name cluster.info()->stats().upstream_rq_total_.inc(); @@ -470,6 +476,7 @@ TEST(StaticClusterImplTest, RingHash) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; + Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "staticcluster", @@ -482,7 +489,7 @@ TEST(StaticClusterImplTest, RingHash) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - true); + true, secret_manager); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -494,6 +501,8 @@ TEST(StaticClusterImplTest, OutlierDetector) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; + Secret::MockSecretManager secret_manager; + const std::string json = R"EOF( { "name": "addressportconfig", @@ -507,7 +516,7 @@ TEST(StaticClusterImplTest, OutlierDetector) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false); + false, secret_manager); Outlier::MockDetector* detector = new Outlier::MockDetector(); EXPECT_CALL(*detector, addChangedStateCb(_)); @@ -541,6 +550,7 @@ TEST(StaticClusterImplTest, HealthyStat) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; + Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "addressportconfig", @@ -554,7 +564,7 @@ TEST(StaticClusterImplTest, HealthyStat) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false); + false, secret_manager); Outlier::MockDetector* outlier_detector = new NiceMock(); cluster.setOutlierDetector(Outlier::DetectorSharedPtr{outlier_detector}); @@ -623,6 +633,7 @@ TEST(StaticClusterImplTest, UrlConfig) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; + Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "addressportconfig", @@ -636,7 +647,7 @@ TEST(StaticClusterImplTest, UrlConfig) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false); + false, secret_manager); cluster.initialize([] {}); EXPECT_EQ(1024U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); @@ -665,6 +676,7 @@ TEST(StaticClusterImplTest, UrlConfig) { TEST(StaticClusterImplTest, UnsupportedLBType) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + Secret::MockSecretManager secret_manager; NiceMock runtime; NiceMock cm; const std::string json = R"EOF( @@ -678,14 +690,15 @@ TEST(StaticClusterImplTest, UnsupportedLBType) { } )EOF"; - EXPECT_THROW( - StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, false), - EnvoyException); + EXPECT_THROW(StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager, + cm, false, secret_manager), + EnvoyException); } TEST(StaticClusterImplTest, MalformedHostIP) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + Secret::MockSecretManager secret_manager; NiceMock runtime; const std::string yaml = R"EOF( name: name @@ -697,7 +710,7 @@ TEST(StaticClusterImplTest, MalformedHostIP) { NiceMock cm; EXPECT_THROW_WITH_MESSAGE(StaticClusterImpl(parseClusterFromV2Yaml(yaml), runtime, stats, - ssl_context_manager, cm, false), + ssl_context_manager, cm, false, secret_manager), EnvoyException, "malformed IP address: foo.bar.com. Consider setting resolver_name or " "setting cluster type to 'STRICT_DNS' or 'LOGICAL_DNS'"); @@ -738,6 +751,7 @@ TEST(ClusterDefinitionTest, BadDnsClusterConfig) { TEST(StaticClusterImplTest, SourceAddressPriority) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + Secret::MockSecretManager secret_manager; NiceMock runtime; envoy::api::v2::Cluster config; config.set_name("staticcluster"); @@ -747,7 +761,8 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { // If the cluster manager gets a source address from the bootstrap proto, use it. NiceMock cm; cm.bind_config_.mutable_source_address()->set_address("1.2.3.5"); - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false, + secret_manager); EXPECT_EQ("1.2.3.5:0", cluster.info()->sourceAddress()->asString()); } @@ -756,7 +771,8 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { { // Verify source address from cluster config is used when present. NiceMock cm; - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false, + secret_manager); EXPECT_EQ(cluster_address, cluster.info()->sourceAddress()->ip()->addressAsString()); } @@ -764,7 +780,8 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { // The source address from cluster config takes precedence over one from the bootstrap proto. NiceMock cm; cm.bind_config_.mutable_source_address()->set_address("1.2.3.5"); - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false, + secret_manager); EXPECT_EQ(cluster_address, cluster.info()->sourceAddress()->ip()->addressAsString()); } } @@ -774,6 +791,7 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { TEST(ClusterImplTest, CloseConnectionsOnHostHealthFailure) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + Secret::MockSecretManager secret_manager; auto dns_resolver = std::make_shared(); NiceMock dispatcher; NiceMock runtime; @@ -789,7 +807,7 @@ TEST(ClusterImplTest, CloseConnectionsOnHostHealthFailure) { hosts: [{ socket_address: { address: foo.bar.com, port_value: 443 }}] )EOF"; StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false); + dns_resolver, cm, dispatcher, false, secret_manager); EXPECT_TRUE(cluster.info()->features() & ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE); } @@ -847,6 +865,7 @@ TEST(PrioritySet, Extend) { TEST(ClusterMetadataTest, Metadata) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + Secret::MockSecretManager secret_manager; auto dns_resolver = std::make_shared(); NiceMock dispatcher; NiceMock runtime; @@ -866,7 +885,7 @@ TEST(ClusterMetadataTest, Metadata) { )EOF"; StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false); + dns_resolver, cm, dispatcher, false, secret_manager); EXPECT_EQ("test_value", Config::Metadata::metadataValue(cluster.info()->metadata(), "com.bar.foo", "baz") .string_value()); diff --git a/test/server/config_validation/cluster_manager_test.cc b/test/server/config_validation/cluster_manager_test.cc index cafd2666a4de0..ffae39360a9b7 100644 --- a/test/server/config_validation/cluster_manager_test.cc +++ b/test/server/config_validation/cluster_manager_test.cc @@ -40,7 +40,7 @@ TEST(ValidationClusterManagerTest, MockedMethods) { AccessLog::MockAccessLogManager log_manager; const envoy::config::bootstrap::v2::Bootstrap bootstrap; ClusterManagerPtr cluster_manager = factory.clusterManagerFromProto( - bootstrap, stats, tls, runtime, random, local_info, log_manager, admin); + bootstrap, stats, tls, runtime, random, local_info, log_manager, admin, secret_manager); EXPECT_EQ(nullptr, cluster_manager->httpConnPoolForCluster("cluster", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); Host::CreateConnectionData data = cluster_manager->tcpConnForCluster("cluster", nullptr); From 6a270c41c12bb984d24cb38dc8dcaa4593ef2785 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 14 Jun 2018 11:56:31 -0700 Subject: [PATCH 02/23] Added secretManager() to ClusterManagerFactory interface Signed-off-by: Jae Kim --- include/envoy/upstream/cluster_manager.h | 6 +- .../common/upstream/cluster_manager_impl.cc | 18 ++--- source/common/upstream/cluster_manager_impl.h | 10 +-- source/common/upstream/eds.cc | 2 +- source/common/upstream/eds.h | 2 +- source/common/upstream/logical_dns_cluster.h | 4 +- source/common/upstream/original_dst_cluster.h | 3 +- source/common/upstream/upstream_impl.cc | 38 +++++---- source/common/upstream/upstream_impl.h | 7 +- .../config_validation/cluster_manager.cc | 12 +-- .../config_validation/cluster_manager.h | 5 +- source/server/configuration_impl.cc | 2 +- .../upstream/cluster_manager_impl_test.cc | 79 ++++++++----------- test/common/upstream/eds_test.cc | 6 +- .../upstream/logical_dns_cluster_test.cc | 3 +- .../upstream/original_dst_cluster_test.cc | 4 +- test/common/upstream/sds_test.cc | 4 +- test/common/upstream/upstream_impl_test.cc | 56 +++++-------- .../config_validation/cluster_manager_test.cc | 2 +- 19 files changed, 110 insertions(+), 153 deletions(-) diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index de4eda328e689..f2ec6e672953e 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -237,8 +237,7 @@ class ClusterManagerFactory { clusterManagerFromProto(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin, - Secret::SecretManager& secret_manager) PURE; + AccessLog::AccessLogManager& log_manager, Server::Admin& admin) PURE; /** * Allocate an HTTP connection pool for the host. Pools are separated by 'priority', @@ -255,8 +254,7 @@ class ClusterManagerFactory { virtual ClusterSharedPtr clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api, - Secret::SecretManager& secret_manager) PURE; + bool added_via_api) PURE; /** * Create a CDS API provider from configuration proto. diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 43a1e6997fa97..42e0c2b89b892 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -170,14 +170,13 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, - Server::Admin& admin, Secret::SecretManager& secret_manager) + Server::Admin& admin) : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()), random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()), local_info_(local_info), cm_stats_(generateStats(stats)), init_helper_([this](Cluster& cluster) { onClusterInit(cluster); }), config_tracker_entry_( - admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })), - secret_manager_(secret_manager) { + admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })) { async_client_manager_ = std::make_unique(*this, tls); const auto& cm_config = bootstrap.cluster_manager(); if (cm_config.has_outlier_detection()) { @@ -473,8 +472,8 @@ bool ClusterManagerImpl::removeCluster(const std::string& cluster_name) { void ClusterManagerImpl::loadCluster(const envoy::api::v2::Cluster& cluster, const std::string& version_info, bool added_via_api, ClusterMap& cluster_map) { - ClusterSharedPtr new_cluster = factory_.clusterFromProto(cluster, *this, outlier_event_logger_, - added_via_api, secret_manager_); + ClusterSharedPtr new_cluster = + factory_.clusterFromProto(cluster, *this, outlier_event_logger_, added_via_api); if (!added_via_api) { if (cluster_map.find(new_cluster->info()->name()) != cluster_map.end()) { @@ -947,10 +946,10 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, - Server::Admin& admin, Secret::SecretManager& secret_manager) { + Server::Admin& admin) { return ClusterManagerPtr{new ClusterManagerImpl(bootstrap, *this, stats, tls, runtime, random, local_info, log_manager, main_thread_dispatcher_, - admin, secret_manager)}; + admin)}; } Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( @@ -968,11 +967,10 @@ Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( ClusterSharedPtr ProdClusterManagerFactory::clusterFromProto( const envoy::api::v2::Cluster& cluster, ClusterManager& cm, - Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api, - Secret::SecretManager& secret_manager) { + Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api) { return ClusterImplBase::create(cluster, cm, stats_, tls_, dns_resolver_, ssl_context_manager_, runtime_, random_, main_thread_dispatcher_, local_info_, - outlier_event_logger, added_via_api, secret_manager); + outlier_event_logger, added_via_api); } CdsApiPtr ProdClusterManagerFactory::createCds( diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 1f1baf65471a0..4c149edaaa2fb 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -48,16 +48,14 @@ class ProdClusterManagerFactory : public ClusterManagerFactory { clusterManagerFromProto(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin, - Secret::SecretManager& secret_manager) override; + AccessLog::AccessLogManager& log_manager, Server::Admin& admin) override; Http::ConnectionPool::InstancePtr allocateConnPool(Event::Dispatcher& dispatcher, HostConstSharedPtr host, ResourcePriority priority, Http::Protocol protocol, const Network::ConnectionSocket::OptionsSharedPtr& options) override; ClusterSharedPtr clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api, - Secret::SecretManager& secret_manager) override; + bool added_via_api) override; CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, const absl::optional& eds_config, ClusterManager& cm) override; @@ -157,8 +155,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable #include -#include "envoy/secret/secret_manager.h" #include "envoy/thread_local/thread_local.h" #include "common/common/empty_string.h" @@ -32,8 +31,7 @@ class LogicalDnsCluster : public ClusterImplBase { LogicalDnsCluster(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, Network::DnsResolverSharedPtr dns_resolver, ThreadLocal::SlotAllocator& tls, - ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api, - Secret::SecretManager& secret_manager); + ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api); ~LogicalDnsCluster(); diff --git a/source/common/upstream/original_dst_cluster.h b/source/common/upstream/original_dst_cluster.h index 86b008426debf..5cb5107a3a4aa 100644 --- a/source/common/upstream/original_dst_cluster.h +++ b/source/common/upstream/original_dst_cluster.h @@ -25,8 +25,7 @@ class OriginalDstCluster : public ClusterImplBase { public: OriginalDstCluster(const envoy::api::v2::Cluster& config, Runtime::Loader& runtime, Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api, - Secret::SecretManager& secret_manager); + ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Primary; } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 304110b75a389..2e5001e21afd1 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -345,13 +345,15 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, } } -ClusterSharedPtr ClusterImplBase::create( - const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Stats::Store& stats, - ThreadLocal::Instance& tls, Network::DnsResolverSharedPtr dns_resolver, - Ssl::ContextManager& ssl_context_manager, Runtime::Loader& runtime, - Runtime::RandomGenerator& random, Event::Dispatcher& dispatcher, - const LocalInfo::LocalInfo& local_info, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api, Secret::SecretManager& secret_manager) { +ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, + Stats::Store& stats, ThreadLocal::Instance& tls, + Network::DnsResolverSharedPtr dns_resolver, + Ssl::ContextManager& ssl_context_manager, + Runtime::Loader& runtime, Runtime::RandomGenerator& random, + Event::Dispatcher& dispatcher, + const LocalInfo::LocalInfo& local_info, + Outlier::EventLoggerSharedPtr outlier_event_logger, + bool added_via_api) { std::unique_ptr new_cluster; // We make this a shared pointer to deal with the distinct ownership @@ -373,18 +375,18 @@ ClusterSharedPtr ClusterImplBase::create( switch (cluster.type()) { case envoy::api::v2::Cluster::STATIC: - new_cluster.reset(new StaticClusterImpl(cluster, runtime, stats, ssl_context_manager, cm, - added_via_api, secret_manager)); + new_cluster.reset( + new StaticClusterImpl(cluster, runtime, stats, ssl_context_manager, cm, added_via_api)); break; case envoy::api::v2::Cluster::STRICT_DNS: new_cluster.reset(new StrictDnsClusterImpl(cluster, runtime, stats, ssl_context_manager, - selected_dns_resolver, cm, dispatcher, added_via_api, - secret_manager)); + selected_dns_resolver, cm, dispatcher, + added_via_api)); break; case envoy::api::v2::Cluster::LOGICAL_DNS: new_cluster.reset(new LogicalDnsCluster(cluster, runtime, stats, ssl_context_manager, selected_dns_resolver, tls, cm, dispatcher, - added_via_api, secret_manager)); + added_via_api)); break; case envoy::api::v2::Cluster::ORIGINAL_DST: if (cluster.lb_policy() != envoy::api::v2::Cluster::ORIGINAL_DST_LB) { @@ -396,7 +398,7 @@ ClusterSharedPtr ClusterImplBase::create( "cluster: cluster type 'original_dst' may not be used with lb_subset_config")); } new_cluster.reset(new OriginalDstCluster(cluster, runtime, stats, ssl_context_manager, cm, - dispatcher, added_via_api, secret_manager)); + dispatcher, added_via_api)); break; case envoy::api::v2::Cluster::EDS: if (!cluster.has_eds_cluster_config()) { @@ -405,7 +407,7 @@ ClusterSharedPtr ClusterImplBase::create( // We map SDS to EDS, since EDS provides backwards compatibility with SDS. new_cluster.reset(new EdsClusterImpl(cluster, runtime, stats, ssl_context_manager, local_info, - cm, dispatcher, random, added_via_api, secret_manager)); + cm, dispatcher, random, added_via_api)); break; default: NOT_REACHED; @@ -638,8 +640,13 @@ StaticClusterImpl::StaticClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, ClusterManager& cm, bool added_via_api) +<<<<<<< HEAD : ClusterImplBase(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, cm.clusterManagerFactory().secretManager(), added_via_api), +======= + : ClusterImplBase(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, added_via_api, + cm.clusterManagerFactory().secretManager()), +>>>>>>> Added secretManager() to ClusterManagerFactory interface initial_hosts_(new HostVector()) { for (const auto& host : cluster.hosts()) { @@ -795,8 +802,7 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const envoy::api::v2::Cluster& cluste Ssl::ContextManager& ssl_context_manager, Network::DnsResolverSharedPtr dns_resolver, ClusterManager& cm, Event::Dispatcher& dispatcher, - bool added_via_api, - Secret::SecretManager& secret_manager) + bool added_via_api) : BaseDynamicClusterImpl(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, cm.clusterManagerFactory().secretManager(), added_via_api), dns_resolver_(dns_resolver), diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 5e77af10f771e..6540b8ac4ca8a 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -420,7 +420,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable ClusterSharedPtr { - return ClusterImplBase::create( - cluster, cm, stats_, tls_, dns_resolver_, ssl_context_manager_, runtime_, random_, - dispatcher_, local_info_, outlier_event_logger, added_via_api, secret_manager); - })); + ON_CALL(*this, clusterFromProto_(_, _, _, _)) + .WillByDefault(Invoke([&](const envoy::api::v2::Cluster& cluster, ClusterManager& cm, + Outlier::EventLoggerSharedPtr outlier_event_logger, + bool added_via_api) -> ClusterSharedPtr { + return ClusterImplBase::create(cluster, cm, stats_, tls_, dns_resolver_, + ssl_context_manager_, runtime_, random_, dispatcher_, + local_info_, outlier_event_logger, added_via_api); + })); } Http::ConnectionPool::InstancePtr @@ -71,9 +69,8 @@ class TestClusterManagerFactory : public ClusterManagerFactory { ClusterSharedPtr clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api, - Secret::SecretManager& secret_manager) override { - return clusterFromProto_(cluster, cm, outlier_event_logger, added_via_api, secret_manager); + bool added_via_api) override { + return clusterFromProto_(cluster, cm, outlier_event_logger, added_via_api); } CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource&, @@ -86,30 +83,24 @@ class TestClusterManagerFactory : public ClusterManagerFactory { clusterManagerFromProto(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin, - Secret::SecretManager& secret_manager) override { - return ClusterManagerPtr{clusterManagerFromProto_( - bootstrap, stats, tls, runtime, random, local_info, log_manager, admin, secret_manager)}; + AccessLog::AccessLogManager& log_manager, Server::Admin& admin) override { + return ClusterManagerPtr{clusterManagerFromProto_(bootstrap, stats, tls, runtime, random, + local_info, log_manager, admin)}; } -<<<<<<< HEAD Secret::SecretManager& secretManager() override { return secret_manager_; } MOCK_METHOD8(clusterManagerFromProto_, -======= - MOCK_METHOD9(clusterManagerFromProto_, ->>>>>>> clusters and listeners read static secrets from Bootstrap.static_resources ClusterManager*(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, Server::Admin& admin, - Secret::SecretManager& secret_manager)); + AccessLog::AccessLogManager& log_manager, Server::Admin& admin)); MOCK_METHOD1(allocateConnPool_, Http::ConnectionPool::Instance*(HostConstSharedPtr host)); - MOCK_METHOD5(clusterFromProto_, + MOCK_METHOD4(clusterFromProto_, ClusterSharedPtr(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api, Secret::SecretManager& secret_manager)); + bool added_via_api)); MOCK_METHOD0(createCds_, CdsApi*()); Stats::IsolatedStoreImpl stats_; @@ -118,7 +109,6 @@ class TestClusterManagerFactory : public ClusterManagerFactory { new NiceMock}; NiceMock runtime_; NiceMock random_; - Secret::MockSecretManager secret_manager_; Ssl::ContextManagerImpl ssl_context_manager_{runtime_}; NiceMock dispatcher_; LocalInfo::MockLocalInfo local_info_; @@ -130,7 +120,7 @@ class ClusterManagerImplTest : public testing::Test { void create(const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { cluster_manager_.reset(new ClusterManagerImpl( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, secret_manager_)); + factory_.local_info_, log_manager_, factory_.dispatcher_, admin_)); } void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active, @@ -156,7 +146,6 @@ class ClusterManagerImplTest : public testing::Test { std::unique_ptr cluster_manager_; AccessLog::MockAccessLogManager log_manager_; NiceMock admin_; - Secret::MockSecretManager secret_manager_; }; envoy::config::bootstrap::v2::Bootstrap parseBootstrapFromJson(const std::string& json_string) { @@ -498,7 +487,7 @@ class ClusterManagerImplThreadAwareLbTest : public ClusterManagerImplTest { cluster1->info_->lb_type_ = lb_type; InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); create(parseBootstrapFromJson(json)); @@ -678,11 +667,11 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { // This part tests static init. InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cds_cluster)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cds_cluster)); ON_CALL(*cds_cluster, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); ON_CALL(*cluster2, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); EXPECT_CALL(factory_, createCds_()).WillOnce(Return(cds)); EXPECT_CALL(*cds, setInitializedCb(_)); @@ -709,16 +698,16 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { std::shared_ptr cluster5(new NiceMock()); cluster5->info_->name_ = "cluster5"; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster3)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster3)); ON_CALL(*cluster3, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster3"), "version1"); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster4)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster4)); ON_CALL(*cluster4, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*cluster4, initialize(_)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster4"), "version2"); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster5)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster5)); ON_CALL(*cluster5, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster5"), "version3"); @@ -812,7 +801,7 @@ TEST_F(ClusterManagerImplTest, DynamicRemoveWithLocalCluster) { std::shared_ptr foo(new NiceMock()); foo->info_->name_ = "foo"; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, false, _)).WillOnce(Return(foo)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, false)).WillOnce(Return(foo)); ON_CALL(*foo, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*foo, initialize(_)); @@ -823,7 +812,7 @@ TEST_F(ClusterManagerImplTest, DynamicRemoveWithLocalCluster) { // cluster in its load balancer. std::shared_ptr cluster1(new NiceMock()); cluster1->info_->name_ = "cluster1"; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, true, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, true)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*cluster1, initialize(_)); cluster_manager_->addOrUpdateCluster(defaultStaticCluster("cluster1"), ""); @@ -866,7 +855,7 @@ TEST_F(ClusterManagerImplTest, RemoveWarmingCluster) { cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); std::shared_ptr cluster1(new NiceMock()); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(*cluster1, initializePhase()).Times(0); EXPECT_CALL(*cluster1, initialize(_)); EXPECT_TRUE( @@ -911,7 +900,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { cluster_manager_->addThreadLocalClusterUpdateCallbacks(*callbacks); std::shared_ptr cluster1(new NiceMock()); - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(*cluster1, initializePhase()).Times(0); EXPECT_CALL(*cluster1, initialize(_)); EXPECT_CALL(*callbacks, onClusterAddOrUpdate(_)).Times(1); @@ -933,7 +922,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { std::shared_ptr cluster2(new NiceMock()); cluster2->prioritySet().getMockHostSet(0)->hosts_ = { makeTestHost(cluster2->info_, "tcp://127.0.0.1:80")}; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); EXPECT_CALL(*cluster2, initialize(_)) .WillOnce(Invoke([cluster1](std::function initialize_callback) { @@ -991,7 +980,7 @@ TEST_F(ClusterManagerImplTest, addOrUpdateClusterStaticExists) { fmt::sprintf("{%s}", clustersJson({defaultStaticClusterJson("some_cluster")})); std::shared_ptr cluster1(new NiceMock()); InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); EXPECT_CALL(*cluster1, initialize(_)); @@ -1035,7 +1024,7 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { { InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); EXPECT_CALL(outlier_detector, addChangedStateCb(_)); EXPECT_CALL(*cluster1, initialize(_)) @@ -1107,7 +1096,7 @@ TEST_F(ClusterManagerImplTest, CloseTcpConnectionsOnHealthFailure) { { InSequence s; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); EXPECT_CALL(outlier_detector, addChangedStateCb(_)); EXPECT_CALL(*cluster1, initialize(_)) @@ -1179,7 +1168,7 @@ TEST_F(ClusterManagerImplTest, DoNotCloseTcpConnectionsOnHealthFailure) { Network::MockClientConnection* connection1 = new NiceMock(); Host::CreateConnectionData conn_info1; - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); EXPECT_CALL(outlier_detector, addChangedStateCb(_)); EXPECT_CALL(*cluster1, initialize(_)) @@ -1904,4 +1893,4 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveWithAllOptions) { } // namespace } // namespace Upstream -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index d69592d29e843..9a39541ffc5d0 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -47,14 +47,12 @@ class EdsTest : public testing::Test { eds_cluster_ = parseClusterFromV2Yaml(yaml_config); Upstream::ClusterManager::ClusterInfoMap cluster_map; Upstream::MockCluster cluster; - Secret::MockSecretManager secret_manager; cluster_map.emplace("eds", cluster); EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); cluster_.reset(new EdsClusterImpl(eds_cluster_, runtime_, stats_, ssl_context_manager_, - local_info_, cm_, dispatcher_, random_, false, - secret_manager)); + local_info_, cm_, dispatcher_, random_, false)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); } @@ -1052,4 +1050,4 @@ TEST_F(EdsTest, MalformedIP) { } } // namespace Upstream -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index a4fb442a490e1..6f09675f48533 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -29,12 +29,11 @@ namespace Upstream { class LogicalDnsClusterTest : public testing::Test { public: void setup(const std::string& json) { - Secret::MockSecretManager secret_manager; resolve_timer_ = new Event::MockTimer(&dispatcher_); NiceMock cm; cluster_.reset(new LogicalDnsCluster(parseClusterFromJson(json), runtime_, stats_store_, ssl_context_manager_, dns_resolver_, tls_, cm, dispatcher_, - false, secret_manager)); + false)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index a3832d048e62e..098723c4d0ddc 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -59,10 +59,8 @@ class OriginalDstClusterTest : public testing::Test { void setup(const std::string& json) { NiceMock cm; - Secret::MockSecretManager secret_manager; cluster_.reset(new OriginalDstCluster(parseClusterFromJson(json), runtime_, stats_store_, - ssl_context_manager_, cm, dispatcher_, false, - secret_manager)); + ssl_context_manager_, cm, dispatcher_, false)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); diff --git a/test/common/upstream/sds_test.cc b/test/common/upstream/sds_test.cc index c8876ec4a4838..cd01ae7539ccb 100644 --- a/test/common/upstream/sds_test.cc +++ b/test/common/upstream/sds_test.cc @@ -58,14 +58,12 @@ class SdsTest : public testing::Test { sds_cluster_ = parseSdsClusterFromJson(raw_config, eds_config); Upstream::ClusterManager::ClusterInfoMap cluster_map; Upstream::MockCluster cluster; - Secret::MockSecretManager secret_manager; cluster_map.emplace("sds", cluster); EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); cluster_.reset(new EdsClusterImpl(sds_cluster_, runtime_, stats_, ssl_context_manager_, - local_info_, cm_, dispatcher_, random_, false, - secret_manager)); + local_info_, cm_, dispatcher_, random_, false)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 811f5f43f2aba..abda6f9952cfd 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -120,7 +120,6 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { NiceMock dispatcher; NiceMock runtime; ReadyWatcher initialized; - Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { @@ -142,7 +141,7 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { })); NiceMock cm; StrictDnsClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false, secret_manager); + dns_resolver, cm, dispatcher, false); cluster.initialize([&]() -> void { initialized.ready(); }); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -157,7 +156,6 @@ TEST(StrictDnsClusterImplTest, ZeroHostsHealthChecker) { NiceMock runtime; NiceMock cm; ReadyWatcher initialized; - Secret::MockSecretManager secret_manager; const std::string yaml = R"EOF( name: name @@ -169,7 +167,7 @@ TEST(StrictDnsClusterImplTest, ZeroHostsHealthChecker) { ResolverData resolver(*dns_resolver, dispatcher); StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false, secret_manager); + dns_resolver, cm, dispatcher, false); std::shared_ptr health_checker(new MockHealthChecker()); EXPECT_CALL(*health_checker, start()); EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)); @@ -190,7 +188,6 @@ TEST(StrictDnsClusterImplTest, Basic) { auto dns_resolver = std::make_shared>(); NiceMock dispatcher; NiceMock runtime; - Secret::MockSecretManager secret_manager; // gmock matches in LIFO order which is why these are swapped. ResolverData resolver2(*dns_resolver, dispatcher); @@ -228,7 +225,7 @@ TEST(StrictDnsClusterImplTest, Basic) { NiceMock cm; StrictDnsClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false, secret_manager); + dns_resolver, cm, dispatcher, false); EXPECT_CALL(runtime.snapshot_, getInteger("circuit_breakers.name.default.max_connections", 43)); EXPECT_EQ(43U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); EXPECT_CALL(runtime.snapshot_, @@ -332,7 +329,6 @@ TEST(StrictDnsClusterImplTest, HostRemovalActiveHealthSkipped) { NiceMock dispatcher; NiceMock runtime; NiceMock cm; - Secret::MockSecretManager secret_manager; const std::string yaml = R"EOF( name: name @@ -345,7 +341,7 @@ TEST(StrictDnsClusterImplTest, HostRemovalActiveHealthSkipped) { ResolverData resolver(*dns_resolver, dispatcher); StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false, secret_manager); + dns_resolver, cm, dispatcher, false); std::shared_ptr health_checker(new MockHealthChecker()); EXPECT_CALL(*health_checker, start()); EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)); @@ -427,7 +423,6 @@ TEST(StaticClusterImplTest, EmptyHostname) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "staticcluster", @@ -440,7 +435,7 @@ TEST(StaticClusterImplTest, EmptyHostname) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false, secret_manager); + false); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -452,7 +447,6 @@ TEST(StaticClusterImplTest, AltStatName) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - Secret::MockSecretManager secret_manager; const std::string yaml = R"EOF( name: staticcluster @@ -465,7 +459,7 @@ TEST(StaticClusterImplTest, AltStatName) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, cm, - false, secret_manager); + false); cluster.initialize([] {}); // Increment a stat and verify it is emitted with alt_stat_name cluster.info()->stats().upstream_rq_total_.inc(); @@ -476,7 +470,6 @@ TEST(StaticClusterImplTest, RingHash) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "staticcluster", @@ -489,7 +482,7 @@ TEST(StaticClusterImplTest, RingHash) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - true, secret_manager); + true); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -501,7 +494,6 @@ TEST(StaticClusterImplTest, OutlierDetector) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { @@ -516,7 +508,7 @@ TEST(StaticClusterImplTest, OutlierDetector) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false, secret_manager); + false); Outlier::MockDetector* detector = new Outlier::MockDetector(); EXPECT_CALL(*detector, addChangedStateCb(_)); @@ -550,7 +542,6 @@ TEST(StaticClusterImplTest, HealthyStat) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "addressportconfig", @@ -564,7 +555,7 @@ TEST(StaticClusterImplTest, HealthyStat) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false, secret_manager); + false); Outlier::MockDetector* outlier_detector = new NiceMock(); cluster.setOutlierDetector(Outlier::DetectorSharedPtr{outlier_detector}); @@ -633,7 +624,6 @@ TEST(StaticClusterImplTest, UrlConfig) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - Secret::MockSecretManager secret_manager; const std::string json = R"EOF( { "name": "addressportconfig", @@ -647,7 +637,7 @@ TEST(StaticClusterImplTest, UrlConfig) { NiceMock cm; StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, - false, secret_manager); + false); cluster.initialize([] {}); EXPECT_EQ(1024U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); @@ -676,7 +666,6 @@ TEST(StaticClusterImplTest, UrlConfig) { TEST(StaticClusterImplTest, UnsupportedLBType) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; - Secret::MockSecretManager secret_manager; NiceMock runtime; NiceMock cm; const std::string json = R"EOF( @@ -690,15 +679,14 @@ TEST(StaticClusterImplTest, UnsupportedLBType) { } )EOF"; - EXPECT_THROW(StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - cm, false, secret_manager), - EnvoyException); + EXPECT_THROW( + StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager, cm, false), + EnvoyException); } TEST(StaticClusterImplTest, MalformedHostIP) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; - Secret::MockSecretManager secret_manager; NiceMock runtime; const std::string yaml = R"EOF( name: name @@ -710,7 +698,7 @@ TEST(StaticClusterImplTest, MalformedHostIP) { NiceMock cm; EXPECT_THROW_WITH_MESSAGE(StaticClusterImpl(parseClusterFromV2Yaml(yaml), runtime, stats, - ssl_context_manager, cm, false, secret_manager), + ssl_context_manager, cm, false), EnvoyException, "malformed IP address: foo.bar.com. Consider setting resolver_name or " "setting cluster type to 'STRICT_DNS' or 'LOGICAL_DNS'"); @@ -751,7 +739,6 @@ TEST(ClusterDefinitionTest, BadDnsClusterConfig) { TEST(StaticClusterImplTest, SourceAddressPriority) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; - Secret::MockSecretManager secret_manager; NiceMock runtime; envoy::api::v2::Cluster config; config.set_name("staticcluster"); @@ -761,8 +748,7 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { // If the cluster manager gets a source address from the bootstrap proto, use it. NiceMock cm; cm.bind_config_.mutable_source_address()->set_address("1.2.3.5"); - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false, - secret_manager); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false); EXPECT_EQ("1.2.3.5:0", cluster.info()->sourceAddress()->asString()); } @@ -771,8 +757,7 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { { // Verify source address from cluster config is used when present. NiceMock cm; - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false, - secret_manager); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false); EXPECT_EQ(cluster_address, cluster.info()->sourceAddress()->ip()->addressAsString()); } @@ -780,8 +765,7 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { // The source address from cluster config takes precedence over one from the bootstrap proto. NiceMock cm; cm.bind_config_.mutable_source_address()->set_address("1.2.3.5"); - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false, - secret_manager); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, cm, false); EXPECT_EQ(cluster_address, cluster.info()->sourceAddress()->ip()->addressAsString()); } } @@ -791,7 +775,6 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { TEST(ClusterImplTest, CloseConnectionsOnHostHealthFailure) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; - Secret::MockSecretManager secret_manager; auto dns_resolver = std::make_shared(); NiceMock dispatcher; NiceMock runtime; @@ -807,7 +790,7 @@ TEST(ClusterImplTest, CloseConnectionsOnHostHealthFailure) { hosts: [{ socket_address: { address: foo.bar.com, port_value: 443 }}] )EOF"; StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false, secret_manager); + dns_resolver, cm, dispatcher, false); EXPECT_TRUE(cluster.info()->features() & ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE); } @@ -865,7 +848,6 @@ TEST(PrioritySet, Extend) { TEST(ClusterMetadataTest, Metadata) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; - Secret::MockSecretManager secret_manager; auto dns_resolver = std::make_shared(); NiceMock dispatcher; NiceMock runtime; @@ -885,7 +867,7 @@ TEST(ClusterMetadataTest, Metadata) { )EOF"; StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - dns_resolver, cm, dispatcher, false, secret_manager); + dns_resolver, cm, dispatcher, false); EXPECT_EQ("test_value", Config::Metadata::metadataValue(cluster.info()->metadata(), "com.bar.foo", "baz") .string_value()); diff --git a/test/server/config_validation/cluster_manager_test.cc b/test/server/config_validation/cluster_manager_test.cc index ffae39360a9b7..cafd2666a4de0 100644 --- a/test/server/config_validation/cluster_manager_test.cc +++ b/test/server/config_validation/cluster_manager_test.cc @@ -40,7 +40,7 @@ TEST(ValidationClusterManagerTest, MockedMethods) { AccessLog::MockAccessLogManager log_manager; const envoy::config::bootstrap::v2::Bootstrap bootstrap; ClusterManagerPtr cluster_manager = factory.clusterManagerFromProto( - bootstrap, stats, tls, runtime, random, local_info, log_manager, admin, secret_manager); + bootstrap, stats, tls, runtime, random, local_info, log_manager, admin); EXPECT_EQ(nullptr, cluster_manager->httpConnPoolForCluster("cluster", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); Host::CreateConnectionData data = cluster_manager->tcpConnForCluster("cluster", nullptr); From 3d330c5f75d01f9ad3db7c068feffaeb9beb11c0 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 14 Jun 2018 12:08:23 -0700 Subject: [PATCH 03/23] Removed unnecessary changes Signed-off-by: Jae Kim --- test/common/upstream/cluster_manager_impl_test.cc | 2 +- test/common/upstream/eds_test.cc | 2 +- test/common/upstream/upstream_impl_test.cc | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index b1f713a7d8e97..28f7094b0ab08 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1893,4 +1893,4 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveWithAllOptions) { } // namespace } // namespace Upstream -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 9a39541ffc5d0..ee7e6139444af 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1050,4 +1050,4 @@ TEST_F(EdsTest, MalformedIP) { } } // namespace Upstream -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index abda6f9952cfd..17888a9006304 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -494,7 +494,6 @@ TEST(StaticClusterImplTest, OutlierDetector) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; NiceMock runtime; - const std::string json = R"EOF( { "name": "addressportconfig", From 79defaf4a559c6e5bf45229d7fbc78ae8e5d8126 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 14 Jun 2018 14:26:12 -0700 Subject: [PATCH 04/23] Changed the location of secret_manager argument Signed-off-by: Jae Kim --- source/common/upstream/upstream_impl.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 2e5001e21afd1..ea36ac333fe28 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -640,13 +640,8 @@ StaticClusterImpl::StaticClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, ClusterManager& cm, bool added_via_api) -<<<<<<< HEAD : ClusterImplBase(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, cm.clusterManagerFactory().secretManager(), added_via_api), -======= - : ClusterImplBase(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, added_via_api, - cm.clusterManagerFactory().secretManager()), ->>>>>>> Added secretManager() to ClusterManagerFactory interface initial_hosts_(new HostVector()) { for (const auto& host : cluster.hosts()) { From df3e59cc2e213c0926a1e5b5b88a7564d04f19c0 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 15:46:12 -0700 Subject: [PATCH 05/23] Add SDS api. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 20 ++++- source/common/secret/BUILD | 24 +++++ source/common/secret/sds_api.cc | 66 ++++++++++++++ source/common/secret/sds_api.h | 43 +++++++++ source/common/secret/secret_manager_impl.cc | 39 ++++++-- source/common/secret/secret_manager_impl.h | 24 ++++- source/common/secret/secret_manager_util.h | 37 ++++++++ source/common/ssl/context_config_impl.cc | 79 +++++++++++++--- source/common/ssl/context_config_impl.h | 7 +- source/server/config_validation/server.cc | 2 +- source/server/configuration_impl.cc | 2 +- source/server/server.cc | 2 +- test/common/secret/BUILD | 1 + .../common/secret/secret_manager_impl_test.cc | 53 +++++++++-- test/common/ssl/context_impl_test.cc | 90 ++++++++++++------- test/common/ssl/ssl_certs_test.h | 4 +- test/integration/ssl_utility.cc | 4 +- test/mocks/secret/mocks.h | 9 +- test/mocks/server/mocks.cc | 3 +- test/server/configuration_impl_test.cc | 49 ++++++++++ 20 files changed, 482 insertions(+), 76 deletions(-) create mode 100644 source/common/secret/sds_api.cc create mode 100644 source/common/secret/sds_api.h create mode 100644 source/common/secret/secret_manager_util.h diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index d7f9788741213..bf2f472ed444d 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -18,16 +18,32 @@ class SecretManager { virtual ~SecretManager() {} /** + * @param config_source_hash a hash string of normalized config source. If it is empty string, + * find secret from the static secrets. * @param secret a protobuf message of envoy::api::v2::auth::Secret. * @throw an EnvoyException if the secret is invalid or not supported. */ - virtual void addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) PURE; + virtual void addOrUpdateSecret(const std::string& config_source_hash, + const envoy::api::v2::auth::Secret& secret) PURE; /** + * @param sds_config_source_hash hash string of normalized config source. * @param name a name of the Ssl::TlsCertificateConfig. * @return the TlsCertificate secret. Returns nullptr if the secret is not found. */ - virtual const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& name) const PURE; + virtual const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& config_source_hash, + const std::string& name) const PURE; + + /** + * Add or update SDS config source. SecretManager starts downloading secrets from registered + * config source. + * + * @param sdsConfigSource a protobuf message object contains SDS config source. + * @param config_name a name that uniquely refers to the SDS config source + * @return a hash string of normalized config source + */ + virtual std::string addOrUpdateSdsService(const envoy::api::v2::core::ConfigSource& config_source, + std::string config_name) PURE; }; } // namespace Secret diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 4f1eff746d6d9..90455caee6efd 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -13,9 +13,33 @@ envoy_cc_library( srcs = ["secret_manager_impl.cc"], hdrs = ["secret_manager_impl.h"], deps = [ + ":sds_api_lib", + ":secret_manager_util", "//include/envoy/secret:secret_manager_interface", "//source/common/common:minimal_logger_lib", "//source/common/ssl:tls_certificate_config_impl_lib", "@envoy_api//envoy/api/v2/auth:cert_cc", ], ) + +envoy_cc_library( + name = "secret_manager_util", + hdrs = ["secret_manager_util.h"], + deps = [ + "//source/common/json:json_loader_lib", + "@envoy_api//envoy/api/v2/core:config_source_cc", + ], +) + +envoy_cc_library( + name = "sds_api_lib", + srcs = ["sds_api.cc"], + hdrs = ["sds_api.h"], + deps = [ + ":secret_manager_util", + "//include/envoy/config:subscription_interface", + "//include/envoy/server:instance_interface", + "//source/common/config:resources_lib", + "//source/common/config:subscription_factory_lib", + ], +) diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc new file mode 100644 index 0000000000000..2b26769213837 --- /dev/null +++ b/source/common/secret/sds_api.cc @@ -0,0 +1,66 @@ +#include "common/secret/sds_api.h" + +#include + +#include "common/config/resources.h" +#include "common/config/subscription_factory.h" +#include "common/secret/secret_manager_util.h" + +namespace Envoy { +namespace Secret { + +SdsApi::SdsApi(Server::Instance& server, const envoy::api::v2::core::ConfigSource& sds_config, + std::string sds_config_name) + : server_(server), sds_config_(sds_config), + sds_config_source_hash_(SecretManagerUtil::configSourceHash(sds_config)), + sds_config_name_(sds_config_name) { + server_.initManager().registerTarget(*this); +} + +void SdsApi::initialize(std::function callback) { + initialize_callback_ = callback; + subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< + envoy::api::v2::auth::Secret>( + sds_config_, server_.localInfo().node(), server_.dispatcher(), server_.clusterManager(), + server_.random(), server_.stats(), /* rest_legacy_constructor */ nullptr, + "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", + // TODO(jaebong): replace next line with + // "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets" to support streaming + // service + "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets"); + + Config::Utility::checkLocalInfo("sds", server_.localInfo()); + + subscription_->start({sds_config_name_}, *this); +} + +void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) { + for (const auto& resource : resources) { + switch (resource.type_case()) { + case envoy::api::v2::auth::Secret::kTlsCertificate: + server_.secretManager().addOrUpdateSecret(sds_config_source_hash_, resource); + break; + case envoy::api::v2::auth::Secret::kSessionTicketKeys: + NOT_IMPLEMENTED; + default: + throw EnvoyException("sds: invalid configuration"); + } + } + + runInitializeCallbackIfAny(); +} + +void SdsApi::onConfigUpdateFailed(const EnvoyException*) { + // We need to allow server startup to continue, even if we have a bad config. + runInitializeCallbackIfAny(); +} + +void SdsApi::runInitializeCallbackIfAny() { + if (initialize_callback_) { + initialize_callback_(); + initialize_callback_ = nullptr; + } +} + +} // namespace Secret +} // namespace Envoy \ No newline at end of file diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h new file mode 100644 index 0000000000000..dfad58adaaa3a --- /dev/null +++ b/source/common/secret/sds_api.h @@ -0,0 +1,43 @@ +#pragma once + +#include + +#include "envoy/api/v2/auth/cert.pb.h" +#include "envoy/api/v2/core/config_source.pb.h" +#include "envoy/config/subscription.h" +#include "envoy/server/instance.h" + +namespace Envoy { +namespace Secret { + +/** + * SDS API implementation that fetches secrets from SDS server via Subscription. + */ +class SdsApi : public Init::Target, Config::SubscriptionCallbacks { +public: + SdsApi(Server::Instance& server, const envoy::api::v2::core::ConfigSource& sds_config, + std::string sds_config_name); + + // Init::Target + void initialize(std::function callback) override; + + // Config::SubscriptionCallbacks + void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; + void onConfigUpdateFailed(const EnvoyException* e) override; + std::string resourceName(const ProtobufWkt::Any& resource) override { + return MessageUtil::anyConvert(resource).name(); + } + +private: + void runInitializeCallbackIfAny(); + + Server::Instance& server_; + const envoy::api::v2::core::ConfigSource sds_config_; + const std::string sds_config_source_hash_; + std::unique_ptr> subscription_; + std::function initialize_callback_; + std::string sds_config_name_; +}; + +} // namespace Secret +} // namespace Envoy \ No newline at end of file diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 3e6689a369da4..5931cd7b98711 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -2,26 +2,51 @@ #include "envoy/common/exception.h" +#include "common/secret/secret_manager_util.h" #include "common/ssl/tls_certificate_config_impl.h" namespace Envoy { namespace Secret { -void SecretManagerImpl::addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) { +void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, + const envoy::api::v2::auth::Secret& secret) { switch (secret.type_case()) { - case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: - tls_certificate_secrets_[secret.name()] = + case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: { + std::unique_lock lhs(tls_certificate_secrets_mutex_); + tls_certificate_secrets_[config_source_hash][secret.name()] = std::make_unique(secret.tls_certificate()); - break; + } break; default: throw EnvoyException("Secret type not implemented"); } } const Ssl::TlsCertificateConfig* -SecretManagerImpl::findTlsCertificate(const std::string& name) const { - auto secret = tls_certificate_secrets_.find(name); - return (secret != tls_certificate_secrets_.end()) ? secret->second.get() : nullptr; +SecretManagerImpl::findTlsCertificate(const std::string& config_source_hash, + const std::string& name) const { + std::shared_lock lhs(tls_certificate_secrets_mutex_); + + auto config_source_it = tls_certificate_secrets_.find(config_source_hash); + if (config_source_it == tls_certificate_secrets_.end()) { + return nullptr; + } + + auto secret = config_source_it->second.find(name); + return (secret != config_source_it->second.end()) ? secret->second.get() : nullptr; +} + +std::string SecretManagerImpl::addOrUpdateSdsService( + const envoy::api::v2::core::ConfigSource& sds_config_source, std::string config_name) { + std::unique_lock lhs(sds_api_mutex_); + + auto hash = SecretManagerUtil::configSourceHash(sds_config_source); + if (sds_apis_.find(hash) != sds_apis_.end()) { + return hash; + } + + sds_apis_[hash] = std::move(std::make_unique(server_, sds_config_source, config_name)); + + return hash; } } // namespace Secret diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index b9406754a8c45..82f44ef1855e2 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -1,22 +1,40 @@ #pragma once +#include #include #include "envoy/secret/secret_manager.h" +#include "envoy/server/instance.h" #include "envoy/ssl/tls_certificate_config.h" #include "common/common/logger.h" +#include "common/secret/sds_api.h" namespace Envoy { namespace Secret { class SecretManagerImpl : public SecretManager, Logger::Loggable { public: - void addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) override; - const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& name) const override; + SecretManagerImpl(Server::Instance& server) : server_(server) {} + + void addOrUpdateSecret(const std::string& config_source_hash, + const envoy::api::v2::auth::Secret& secret) override; + const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& config_source_hash, + const std::string& name) const override; + std::string addOrUpdateSdsService(const envoy::api::v2::core::ConfigSource& config_source, + std::string config_name) override; private: - std::unordered_map tls_certificate_secrets_; + Server::Instance& server_; + // map hash code of SDS config source and SdsApi object + std::unordered_map> sds_apis_; + mutable std::shared_timed_mutex sds_api_mutex_; + + // Manages pairs of name and Ssl::TlsCertificateConfig grouped by SDS config source hash. + // If SDS config source hash is empty, it is a static secret. + std::unordered_map> + tls_certificate_secrets_; + mutable std::shared_timed_mutex tls_certificate_secrets_mutex_; }; } // namespace Secret diff --git a/source/common/secret/secret_manager_util.h b/source/common/secret/secret_manager_util.h new file mode 100644 index 0000000000000..d81060dcff5a2 --- /dev/null +++ b/source/common/secret/secret_manager_util.h @@ -0,0 +1,37 @@ +#pragma once + +#include "envoy/api/v2/core/config_source.pb.h" + +#include "common/common/fmt.h" +#include "common/json/json_loader.h" +#include "common/protobuf/protobuf.h" + +namespace Envoy { +namespace Secret { + +class SecretManagerUtil { +public: + virtual ~SecretManagerUtil() {} + + /** + * Calculate hash code of ConfigSource. To identify the same ConfigSource, calculate the hash + * code from the ConfigSource + * + * @param config_source envoy::api::v2::core::ConfigSource + * @return hash code + */ + static std::string configSourceHash(const envoy::api::v2::core::ConfigSource& config_source) { + std::string jsonstr; + if (Protobuf::util::MessageToJsonString(config_source, &jsonstr).ok()) { + auto obj = Json::Factory::loadFromString(jsonstr); + if (obj.get() != nullptr) { + return std::to_string(obj->hash()); + } + } + throw EnvoyException( + fmt::format("Invalid ConfigSource message: {}", config_source.DebugString())); + } +}; + +} // namespace Secret +} // namespace Envoy \ No newline at end of file diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 374cd2945b50a..979d3c47c9a43 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -3,6 +3,8 @@ #include #include +#include "envoy/ssl/tls_certificate_config.h" + #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/config/datasource.h" @@ -16,20 +18,45 @@ namespace Ssl { namespace { +std::string readSdsSecretName(const envoy::api::v2::auth::CommonTlsContext& config) { + return (!config.tls_certificate_sds_secret_configs().empty()) + ? config.tls_certificate_sds_secret_configs()[0].name() + : ""; +} + +std::string readConfigSourceHash(const envoy::api::v2::auth::CommonTlsContext& config, + Secret::SecretManager& secret_manager) { + return (!config.tls_certificate_sds_secret_configs().empty() && + config.tls_certificate_sds_secret_configs()[0].has_sds_config()) + ? secret_manager.addOrUpdateSdsService( + config.tls_certificate_sds_secret_configs()[0].sds_config(), + config.tls_certificate_sds_secret_configs()[0].name()) + : ""; +} + std::string readConfig( const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager, + const std::string config_source_hash, const std::string secret_name, const std::function& read_inline_config, - const std::function& read_secret) { + const std::function& + read_managed_secret) { if (!config.tls_certificates().empty()) { return read_inline_config(config.tls_certificates()[0]); } else if (!config.tls_certificate_sds_secret_configs().empty()) { - auto name = config.tls_certificate_sds_secret_configs()[0].name(); - const Ssl::TlsCertificateConfig* secret = secret_manager.findTlsCertificate(name); + const auto secret = secret_manager.findTlsCertificate(config_source_hash, secret_name); if (!secret) { - throw EnvoyException(fmt::format("Static secret is not defined: {}", name)); + throw EnvoyException(fmt::format("Static secret is not defined: {}", secret_name)); } - return read_secret(*secret); + if (!secret) { + if (config_source_hash.empty()) { + throw EnvoyException( + fmt::format("Unknown static secret: {} : {}", config_source_hash, secret_name)); + } else { + return EMPTY_STRING; + } + } + return read_managed_secret(secret); } else { return EMPTY_STRING; } @@ -55,7 +82,9 @@ const std::string ContextConfigImpl::DEFAULT_ECDH_CURVES = "X25519:P-256"; ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager) - : alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), + : secret_manager_(secret_manager), sds_secret_name_(readSdsSecretName(config)), + sds_config_source_hash_(readConfigSourceHash(config, secret_manager)), + alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), alt_alpn_protocols_(config.deprecated_v1().alt_alpn_protocols()), cipher_suites_(StringUtil::nonEmptyStringOrDefault( RepeatedPtrUtil::join(config.tls_params().cipher_suites(), ":"), DEFAULT_CIPHER_SUITES)), @@ -68,24 +97,24 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex certificate_revocation_list_path_( Config::DataSource::getPath(config.validation_context().crl())), cert_chain_(readConfig( - config, secret_manager, + config, secret_manager, sds_config_source_hash_, sds_secret_name_, [](const envoy::api::v2::auth::TlsCertificate& tls_certificate) -> std::string { return Config::DataSource::read(tls_certificate.certificate_chain(), true); }, - [](const Ssl::TlsCertificateConfig& secret) -> std::string { - return secret.certificateChain(); + [](const Ssl::TlsCertificateConfig* secret) -> std::string { + return secret->certificateChain(); })), cert_chain_path_( config.tls_certificates().empty() ? "" : Config::DataSource::getPath(config.tls_certificates()[0].certificate_chain())), private_key_(readConfig( - config, secret_manager, + config, secret_manager, sds_config_source_hash_, sds_secret_name_, [](const envoy::api::v2::auth::TlsCertificate& tls_certificate) -> std::string { return Config::DataSource::read(tls_certificate.private_key(), true); }, - [](const Ssl::TlsCertificateConfig& secret) -> std::string { - return secret.privateKey(); + [](const Ssl::TlsCertificateConfig* secret) -> std::string { + return secret->privateKey(); })), private_key_path_( config.tls_certificates().empty() @@ -138,6 +167,32 @@ unsigned ContextConfigImpl::tlsVersionFromProto( NOT_REACHED; } +const std::string& ContextConfigImpl::certChain() const { + if (!cert_chain_.empty()) { + return cert_chain_; + } + + auto secret = secret_manager_.findTlsCertificate(sds_config_source_hash_, sds_secret_name_); + if (!secret) { + return cert_chain_; + } + + return secret->certificateChain(); +} + +const std::string& ContextConfigImpl::privateKey() const { + if (!private_key_.empty()) { + return private_key_; + } + + auto secret = secret_manager_.findTlsCertificate(sds_config_source_hash_, sds_secret_name_); + if (!secret) { + return private_key_; + } + + return secret->privateKey(); +} + ClientContextConfigImpl::ClientContextConfigImpl( const envoy::api::v2::auth::UpstreamTlsContext& config, Secret::SecretManager& secret_manager) : ContextConfigImpl(config.common_tls_context(), secret_manager), diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 2628f39b2e00c..04e39788712dd 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -33,11 +33,11 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { ? INLINE_STRING : certificate_revocation_list_path_; } - const std::string& certChain() const override { return cert_chain_; } + const std::string& certChain() const override; const std::string& certChainPath() const override { return (cert_chain_path_.empty() && !cert_chain_.empty()) ? INLINE_STRING : cert_chain_path_; } - const std::string& privateKey() const override { return private_key_; } + const std::string& privateKey() const override; const std::string& privateKeyPath() const override { return (private_key_path_.empty() && !private_key_.empty()) ? INLINE_STRING : private_key_path_; } @@ -66,6 +66,9 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { static const std::string DEFAULT_CIPHER_SUITES; static const std::string DEFAULT_ECDH_CURVES; + Secret::SecretManager& secret_manager_; + const std::string sds_secret_name_; + const std::string sds_config_source_hash_; const std::string alpn_protocols_; const std::string alt_alpn_protocols_; const std::string cipher_suites_; diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 885c0b997a39a..b149b295174e0 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -78,7 +78,7 @@ void ValidationInstance::initialize(Options& options, Configuration::InitialImpl initial_config(bootstrap); thread_local_.registerThread(*dispatcher_, true); runtime_loader_ = component_factory.createRuntime(*this, initial_config); - secret_manager_.reset(new Secret::SecretManagerImpl()); + secret_manager_.reset(new Secret::SecretManagerImpl(*this)); ssl_context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_loader_)); cluster_manager_factory_.reset(new Upstream::ValidationClusterManagerFactory( runtime(), stats(), threadLocal(), random(), dnsResolver(), sslContextManager(), dispatcher(), diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 0746d48d9d6d0..673215cbef956 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -50,7 +50,7 @@ void MainImpl::initialize(const envoy::config::bootstrap::v2::Bootstrap& bootstr ENVOY_LOG(info, "loading {} static secret(s)", secrets.size()); for (ssize_t i = 0; i < secrets.size(); i++) { ENVOY_LOG(debug, "static secret #{}: {}", i, secrets[i].name()); - server.secretManager().addOrUpdateSecret(secrets[i]); + server.secretManager().addOrUpdateSecret("", secrets[i]); } cluster_manager_ = cluster_manager_factory.clusterManagerFromProto( diff --git a/source/server/server.cc b/source/server/server.cc index 058463a028afd..bc4a445d7a9e1 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -55,7 +55,7 @@ InstanceImpl::InstanceImpl(Options& options, Network::Address::InstanceConstShar handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)), random_generator_(std::move(random_generator)), listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks), - secret_manager_(new Secret::SecretManagerImpl()), + secret_manager_(new Secret::SecretManagerImpl(*this)), dns_resolver_(dispatcher_->createDnsResolver({})), access_log_manager_(*api_, *dispatcher_, access_log_lock, store), terminated_(false) { diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index c65489952a84e..b7f46ff0edb71 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -16,6 +16,7 @@ envoy_cc_test( ], deps = [ "//source/common/secret:secret_manager_impl_lib", + "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", "//test/test_common:registry_lib", "//test/test_common:utility_lib", diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 4c82c3deb8170..c608972e9e480 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -5,6 +5,7 @@ #include "common/secret/secret_manager_impl.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" @@ -15,6 +16,8 @@ namespace Envoy { namespace Secret { namespace { +class MockServer : public Server::MockInstance {} + class SecretManagerImplTest : public testing::Test {}; TEST_F(SecretManagerImplTest, SecretLoadSuccess) { @@ -32,21 +35,54 @@ name: "abc.com" MessageUtil::loadFromYaml(yaml, secret_config); - std::unique_ptr secret_manager(new SecretManagerImpl()); + Server::MockInstance server; + + server.secretManager().addOrUpdateSecret(secret_config); + + ASSERT_EQ(server.secretManager().findTlsCertificate("undefined"), nullptr); + + ASSERT_NE(server.secretManager().findTlsCertificate("abc.com"), nullptr); + + EXPECT_EQ( + TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), + server.secretManager().findTlsCertificate("abc.com")->certificateChain()); + + EXPECT_EQ( + TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), + server.secretManager().findTlsCertificate("abc.com")->privateKey()); +} + +TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { + envoy::api::v2::core::ConfigSource config_source; + envoy::api::v2::auth::Secret secret_config; + + std::string yaml = + R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + MessageUtil::loadFromYaml(yaml, secret_config); + + Server::MockInstance server; - secret_manager->addOrUpdateSecret(secret_config); + std::string config_source_hash = server.secretManager().addOrUpdateSdsService(config_source); - ASSERT_EQ(secret_manager->findTlsCertificate("undefined"), nullptr); + server.secretManager().addOrUpdateSecret(config_source_hash, secret_config); - ASSERT_NE(secret_manager->findTlsCertificate("abc.com"), nullptr); + ASSERT_EQ(server.secretManager().findTlsCertificate(config_source_hash, "undefined"), nullptr); EXPECT_EQ( TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), - secret_manager->findTlsCertificate("abc.com")->certificateChain()); + server.secretManager().findTlsCertificate(config_source_hash, "abc.com")->certificateChain()); EXPECT_EQ( TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), - secret_manager->findTlsCertificate("abc.com")->privateKey()); + server.secretManager().findTlsCertificate(config_source_hash, "abc.com")->privateKey()); } TEST_F(SecretManagerImplTest, NotImplementedException) { @@ -62,9 +98,10 @@ name: "abc.com" MessageUtil::loadFromYaml(yaml, secret_config); - std::unique_ptr secret_manager(new SecretManagerImpl()); + Server::MockInstance server; + std::unique_ptr secret_manager(new SecretManagerImpl(server)); - EXPECT_THROW_WITH_MESSAGE(secret_manager->addOrUpdateSecret(secret_config), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(server.secretManager().addOrUpdateSecret(secret_config), EnvoyException, "Secret type not implemented"); } diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 5f6002a409871..1e2dcf0fddeef 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -10,6 +10,7 @@ #include "test/common/ssl/ssl_certs_test.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" @@ -79,7 +80,7 @@ TEST_F(SslContextImplTest, TestCipherSuites) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -95,7 +96,7 @@ TEST_F(SslContextImplTest, TestExpiringCert) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -118,7 +119,7 @@ TEST_F(SslContextImplTest, TestExpiredCert) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -136,7 +137,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -162,7 +163,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { TEST_F(SslContextImplTest, TestNoCert) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString("{}"); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -192,14 +193,14 @@ class SslServerContextImplTicketTest : public SslContextImplTest { TestEnvironment::substitute("{{ test_tmpdir }}/unittestkey.pem")); Secret::MockSecretManager secret_manager; - ServerContextConfigImpl server_context_config(cfg, secret_manager); + ServerContextConfigImpl server_context_config(cfg, server.secretManager()); loadConfig(server_context_config); } static void loadConfigJson(const std::string& json) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); Secret::MockSecretManager secret_manager; - ServerContextConfigImpl cfg(*loader, secret_manager); + ServerContextConfigImpl cfg(*loader, server.secretManager()); loadConfig(cfg); } }; @@ -356,28 +357,28 @@ class ClientContextConfigImplTest : public SslCertsTest {}; // Validate that empty SNI (according to C string rules) fails config validation. TEST(ClientContextConfigImplTest, EmptyServerNameIndication) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; tls_context.set_sni(std::string("\000", 1)); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, "SNI names containing NULL-byte are not allowed"); tls_context.set_sni(std::string("a\000b", 3)); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, "SNI names containing NULL-byte are not allowed"); } // Validate that values other than a hex-encoded SHA-256 fail config validation. TEST(ClientContextConfigImplTest, InvalidCertificateHash) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; tls_context.mutable_common_tls_context() ->mutable_validation_context() // This is valid hex-encoded string, but it doesn't represent SHA-256 (80 vs 64 chars). ->add_verify_certificate_hash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); - ClientContextConfigImpl client_context_config(tls_context, secret_manager); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -388,12 +389,12 @@ TEST(ClientContextConfigImplTest, InvalidCertificateHash) { // Validate that values other than a base64-encoded SHA-256 fail config validation. TEST(ClientContextConfigImplTest, InvalidCertificateSpki) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; tls_context.mutable_common_tls_context() ->mutable_validation_context() // Not a base64-encoded string. ->add_verify_certificate_spki("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); - ClientContextConfigImpl client_context_config(tls_context, secret_manager); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -405,11 +406,21 @@ TEST(ClientContextConfigImplTest, InvalidCertificateSpki) { // TODO(PiotrSikora): Support multiple TLS certificates. TEST(ClientContextConfigImplTest, MultipleTlsCertificates) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificates(); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, + "Multiple TLS certificates are not supported for client contexts"); +} + +TEST(ClientContextConfigImplTest, TlsCertificatesAndSdsConfig) { + envoy::api::v2::auth::UpstreamTlsContext tls_context; + Server::MockInstance server; + tls_context.mutable_common_tls_context()->add_tls_certificates(); + tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); + EXPECT_THROW_WITH_MESSAGE( + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, "Multiple TLS certificates are not supported for client contexts"); } @@ -427,8 +438,8 @@ name: "abc.com" MessageUtil::loadFromYaml(yaml, secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); - secret_manager->addOrUpdateSecret(secret_config); + Server::MockInstance server; + server.secretManager().addOrUpdateSecret("", secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; tls_context.mutable_common_tls_context() @@ -436,7 +447,7 @@ name: "abc.com" ->Add() ->set_name("abc.com"); - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()); EXPECT_EQ( TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), @@ -460,9 +471,8 @@ name: "abc.com" MessageUtil::loadFromYaml(yaml, secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); - - secret_manager->addOrUpdateSecret(secret_config); + Server::MockInstance server; + server.secretManager().addOrUpdateSecret("", secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; tls_context.mutable_common_tls_context() @@ -471,7 +481,7 @@ name: "abc.com" ->set_name("missing"); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()), + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, "Static secret is not defined: missing"); } @@ -480,23 +490,37 @@ name: "abc.com" // TODO(PiotrSikora): Support multiple TLS certificates. TEST(ServerContextConfigImplTest, MultipleTlsCertificates) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, "A single TLS certificate is required for server contexts"); tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificates(); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, "A single TLS certificate is required for server contexts"); } +TEST(ServerContextConfigImplTest, TlsCertificatesAndSdsConfig) { + Server::MockInstance server; + envoy::api::v2::auth::DownstreamTlsContext tls_context; + + EXPECT_THROW_WITH_MESSAGE( + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "A single TLS certificate is required for server contexts"); + tls_context.mutable_common_tls_context()->add_tls_certificates(); + tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); + EXPECT_THROW_WITH_MESSAGE( + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "A single TLS certificate is required for server contexts"); +} + // TlsCertificate messages must have a cert for servers. TEST(ServerContextImplTest, TlsCertificateNonEmpty) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; tls_context.mutable_common_tls_context()->add_tls_certificates(); - ServerContextConfigImpl client_context_config(tls_context, secret_manager); + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -509,7 +533,7 @@ TEST(ServerContextImplTest, TlsCertificateNonEmpty) { // Cannot ignore certificate expiration without a trusted CA. TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; envoy::api::v2::auth::CertificateValidationContext* server_validation_ctx = tls_context.mutable_common_tls_context()->mutable_validation_context(); @@ -517,7 +541,7 @@ TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { server_validation_ctx->set_allow_expired_certificate(true); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl server_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl server_context_config(tls_context, server.secretManager()), EnvoyException, "Certificate validity period is always ignored without trusted CA"); envoy::api::v2::auth::TlsCertificate* server_cert = @@ -529,19 +553,19 @@ TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { server_validation_ctx->set_allow_expired_certificate(false); - EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, secret_manager)); + EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, server.secretManager())); server_validation_ctx->set_allow_expired_certificate(true); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl server_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl server_context_config(tls_context, server.secretManager()), EnvoyException, "Certificate validity period is always ignored without trusted CA"); // But once you add a trusted CA, you should be able to create the context. server_validation_ctx->mutable_trusted_ca()->set_filename( TestEnvironment::substitute("{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem")); - EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, secret_manager)); + EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, server.secretManager())); } } // namespace Ssl diff --git a/test/common/ssl/ssl_certs_test.h b/test/common/ssl/ssl_certs_test.h index 2f09e019944a4..4cdbef6c791b5 100644 --- a/test/common/ssl/ssl_certs_test.h +++ b/test/common/ssl/ssl_certs_test.h @@ -1,6 +1,6 @@ #pragma once -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "gtest/gtest.h" @@ -12,6 +12,6 @@ class SslCertsTest : public testing::Test { TestEnvironment::exec({TestEnvironment::runfilesPath("test/common/ssl/gen_unittest_certs.sh")}); } - Secret::MockSecretManager secret_manager_; + Server::MockInstance server_; }; } // namespace Envoy diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index 9c3d1e773b06d..62e72bbcce3cd 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -7,6 +7,7 @@ #include "common/ssl/ssl_socket.h" #include "test/integration/server.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" @@ -58,8 +59,9 @@ createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& conte } else { target = san ? json_san : json_plain; } + Server::MockInstance server; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - ClientContextConfigImpl cfg(*loader, secret_manager); + ClientContextConfigImpl cfg(*loader, server.secretManager()); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); return Network::TransportSocketFactoryPtr{ new Ssl::ClientSslSocketFactory(cfg, context_manager, *client_stats_store)}; diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 1d111df74993c..de895c3582ea9 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -14,8 +14,13 @@ class MockSecretManager : public SecretManager { MockSecretManager(); ~MockSecretManager(); - MOCK_METHOD1(addOrUpdateSecret, void(const envoy::api::v2::auth::Secret& secret)); - MOCK_CONST_METHOD1(findTlsCertificate, const Ssl::TlsCertificateConfig*(const std::string& name)); + MOCK_METHOD2(addOrUpdateSecret, void(const std::string& config_source_hash, + const envoy::api::v2::auth::Secret& secret)); + MOCK_CONST_METHOD2(findTlsCertificate, + Ssl::TlsCertificateConfig*(const std::string& config_source_hash, + const std::string& name)); + MOCK_METHOD1(addOrUpdateSdsService, + std::string(const envoy::api::v2::core::ConfigSource& config_source)); }; } // namespace Secret diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 72724db9b04d0..5c52d26653325 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -107,7 +107,8 @@ MockWorker::MockWorker() { MockWorker::~MockWorker() {} MockInstance::MockInstance() - : secret_manager_(new Secret::SecretManagerImpl()), ssl_context_manager_(runtime_loader_), + : secret_manager_(new Secret::SecretManagerImpl(*this)), + ssl_context_manager_(runtime_loader_), singleton_manager_(new Singleton::ManagerImpl()) { ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_)); ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_store_)); diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index bec9bb6d05670..e3bd14daf60be 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -303,6 +303,55 @@ TEST_F(ConfigurationImplTest, StatsSinkWithNoName) { "Provided name for static registration lookup was empty."); } +TEST_F(ConfigurationImplTest, StaticSecretRead) { + std::string json = + R"EOF( + { + "listeners" : [ + { + "address": "tcp://127.0.0.1:1234", + "filters": [] + } + ], + "cluster_manager": { + "clusters": [] + }, + "admin": {"access_log_path": "/dev/null", "address": "tcp://1.2.3.4:5678"} + } + )EOF"; + + envoy::config::bootstrap::v2::Bootstrap bootstrap = TestUtility::parseBootstrapFromJson(json); + + auto secret_config = bootstrap.mutable_static_resources()->mutable_secrets()->Add(); + + std::string yaml = + R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + MessageUtil::loadFromYaml(yaml, *secret_config); + + MainImpl config; + config.initialize(bootstrap, server_, cluster_manager_factory_); + + auto secret = server_.secretManager().findTlsCertificate("", "abc.com"); + + ASSERT_NE(secret, nullptr); + + EXPECT_EQ( + TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), + secret->certificateChain()); + + EXPECT_EQ( + TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), + secret->privateKey()); +} + } // namespace Configuration } // namespace Server } // namespace Envoy From b32cb31d8fdb44ea520300bb615f510afd8320ee Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 15:51:46 -0700 Subject: [PATCH 06/23] fix format. Signed-off-by: JimmyCYJ --- .../common/secret/secret_manager_impl_test.cc | 6 ++- test/common/ssl/context_impl_test.cc | 38 ++++++++++--------- test/mocks/secret/mocks.h | 4 +- test/mocks/server/mocks.cc | 3 +- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index c608972e9e480..e266dae366cfa 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -16,9 +16,11 @@ namespace Envoy { namespace Secret { namespace { -class MockServer : public Server::MockInstance {} +class MockServer : public Server::MockInstance { +} -class SecretManagerImplTest : public testing::Test {}; +class SecretManagerImplTest : public testing::Test { +}; TEST_F(SecretManagerImplTest, SecretLoadSuccess) { envoy::api::v2::auth::Secret secret_config; diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 1e2dcf0fddeef..6e0ce308b77ab 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -361,12 +361,12 @@ TEST(ClientContextConfigImplTest, EmptyServerNameIndication) { tls_context.set_sni(std::string("\000", 1)); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, - "SNI names containing NULL-byte are not allowed"); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "SNI names containing NULL-byte are not allowed"); tls_context.set_sni(std::string("a\000b", 3)); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, - "SNI names containing NULL-byte are not allowed"); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "SNI names containing NULL-byte are not allowed"); } // Validate that values other than a hex-encoded SHA-256 fail config validation. @@ -410,8 +410,8 @@ TEST(ClientContextConfigImplTest, MultipleTlsCertificates) { tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificates(); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, - "Multiple TLS certificates are not supported for client contexts"); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "Multiple TLS certificates are not supported for client contexts"); } TEST(ClientContextConfigImplTest, TlsCertificatesAndSdsConfig) { @@ -420,8 +420,8 @@ TEST(ClientContextConfigImplTest, TlsCertificatesAndSdsConfig) { tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, - "Multiple TLS certificates are not supported for client contexts"); + ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "Multiple TLS certificates are not supported for client contexts"); } TEST(ClientContextConfigImplTest, StaticTlsCertificates) { @@ -492,13 +492,13 @@ TEST(ServerContextConfigImplTest, MultipleTlsCertificates) { envoy::api::v2::auth::DownstreamTlsContext tls_context; Server::MockInstance server; EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, - "A single TLS certificate is required for server contexts"); + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "A single TLS certificate is required for server contexts"); tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificates(); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), EnvoyException, - "A single TLS certificate is required for server contexts"); + ServerContextConfigImpl client_context_config(tls_context, server.secretManager()), + EnvoyException, "A single TLS certificate is required for server contexts"); } TEST(ServerContextConfigImplTest, TlsCertificatesAndSdsConfig) { @@ -541,8 +541,8 @@ TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { server_validation_ctx->set_allow_expired_certificate(true); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl server_context_config(tls_context, server.secretManager()), EnvoyException, - "Certificate validity period is always ignored without trusted CA"); + ServerContextConfigImpl server_context_config(tls_context, server.secretManager()), + EnvoyException, "Certificate validity period is always ignored without trusted CA"); envoy::api::v2::auth::TlsCertificate* server_cert = tls_context.mutable_common_tls_context()->add_tls_certificates(); @@ -553,19 +553,21 @@ TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { server_validation_ctx->set_allow_expired_certificate(false); - EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, server.secretManager())); + EXPECT_NO_THROW( + ServerContextConfigImpl server_context_config(tls_context, server.secretManager())); server_validation_ctx->set_allow_expired_certificate(true); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl server_context_config(tls_context, server.secretManager()), EnvoyException, - "Certificate validity period is always ignored without trusted CA"); + ServerContextConfigImpl server_context_config(tls_context, server.secretManager()), + EnvoyException, "Certificate validity period is always ignored without trusted CA"); // But once you add a trusted CA, you should be able to create the context. server_validation_ctx->mutable_trusted_ca()->set_filename( TestEnvironment::substitute("{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem")); - EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, server.secretManager())); + EXPECT_NO_THROW( + ServerContextConfigImpl server_context_config(tls_context, server.secretManager())); } } // namespace Ssl diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index de895c3582ea9..0a8ccdf92fd59 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -18,9 +18,9 @@ class MockSecretManager : public SecretManager { const envoy::api::v2::auth::Secret& secret)); MOCK_CONST_METHOD2(findTlsCertificate, Ssl::TlsCertificateConfig*(const std::string& config_source_hash, - const std::string& name)); + const std::string& name)); MOCK_METHOD1(addOrUpdateSdsService, - std::string(const envoy::api::v2::core::ConfigSource& config_source)); + std::string(const envoy::api::v2::core::ConfigSource& config_source)); }; } // namespace Secret diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 5c52d26653325..3ba3e74f7cdc8 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -107,8 +107,7 @@ MockWorker::MockWorker() { MockWorker::~MockWorker() {} MockInstance::MockInstance() - : secret_manager_(new Secret::SecretManagerImpl(*this)), - ssl_context_manager_(runtime_loader_), + : secret_manager_(new Secret::SecretManagerImpl(*this)), ssl_context_manager_(runtime_loader_), singleton_manager_(new Singleton::ManagerImpl()) { ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_)); ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_store_)); From 8d0a41f5b13b080397bc09c6468295da6aed8b31 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 16:31:45 -0700 Subject: [PATCH 07/23] fix test files. Signed-off-by: JimmyCYJ --- test/common/secret/secret_manager_impl_test.cc | 8 ++++---- test/integration/BUILD | 1 + test/integration/ssl_utility.cc | 3 +-- test/mocks/secret/mocks.h | 5 +++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index e266dae366cfa..17cdcbb0fffbf 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -17,7 +17,7 @@ namespace Secret { namespace { class MockServer : public Server::MockInstance { -} +}; class SecretManagerImplTest : public testing::Test { }; @@ -39,11 +39,11 @@ name: "abc.com" Server::MockInstance server; - server.secretManager().addOrUpdateSecret(secret_config); + server.secretManager().addOrUpdateSecret("", secret_config); - ASSERT_EQ(server.secretManager().findTlsCertificate("undefined"), nullptr); + ASSERT_EQ(server.secretManager().findTlsCertificate("", "undefined"), nullptr); - ASSERT_NE(server.secretManager().findTlsCertificate("abc.com"), nullptr); + ASSERT_NE(server.secretManager().findTlsCertificate("", "abc.com"), nullptr); EXPECT_EQ( TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), diff --git a/test/integration/BUILD b/test/integration/BUILD index 2c4b907bdebd4..9edc865bd5ca6 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -282,6 +282,7 @@ envoy_cc_test_library( "//source/server:test_hooks_lib", "//test/common/upstream:utility_lib", "//test/config:utility_lib", + "//test/mocks/server:server_mocks", "//test/mocks/buffer:buffer_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index 62e72bbcce3cd..53316b1114aee 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -15,8 +15,7 @@ namespace Envoy { namespace Ssl { Network::TransportSocketFactoryPtr -createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager, - Secret::SecretManager& secret_manager) { +createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager) { const std::string json_plain = R"EOF( { "ca_cert_file": "{{ test_rundir }}/test/config/integration/certs/cacert.pem", diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 0a8ccdf92fd59..f7fd1233b7bb4 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -19,8 +19,9 @@ class MockSecretManager : public SecretManager { MOCK_CONST_METHOD2(findTlsCertificate, Ssl::TlsCertificateConfig*(const std::string& config_source_hash, const std::string& name)); - MOCK_METHOD1(addOrUpdateSdsService, - std::string(const envoy::api::v2::core::ConfigSource& config_source)); + MOCK_METHOD2(addOrUpdateSdsService, + std::string(const envoy::api::v2::core::ConfigSource& config_source, + std::string config_name)); }; } // namespace Secret From 0dd3cc8e23f499bde993d6fe00200c8feec036cb Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 17:01:19 -0700 Subject: [PATCH 08/23] fix format. Signed-off-by: JimmyCYJ --- test/common/secret/secret_manager_impl_test.cc | 6 ++---- test/integration/BUILD | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 17cdcbb0fffbf..d1aeb5469c8a0 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -16,11 +16,9 @@ namespace Envoy { namespace Secret { namespace { -class MockServer : public Server::MockInstance { -}; +class MockServer : public Server::MockInstance {}; -class SecretManagerImplTest : public testing::Test { -}; +class SecretManagerImplTest : public testing::Test {}; TEST_F(SecretManagerImplTest, SecretLoadSuccess) { envoy::api::v2::auth::Secret secret_config; diff --git a/test/integration/BUILD b/test/integration/BUILD index 9edc865bd5ca6..a32e8cea0b09e 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -282,8 +282,8 @@ envoy_cc_test_library( "//source/server:test_hooks_lib", "//test/common/upstream:utility_lib", "//test/config:utility_lib", - "//test/mocks/server:server_mocks", "//test/mocks/buffer:buffer_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", From 79275319e721ad0bfd3e1facd202b68204212f9e Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 17:50:23 -0700 Subject: [PATCH 09/23] fix format and mock interfaces. Signed-off-by: JimmyCYJ --- .../common/secret/secret_manager_impl_test.cc | 15 +++++++++----- test/common/ssl/BUILD | 1 + test/common/ssl/context_impl_test.cc | 2 ++ test/common/ssl/ssl_socket_test.cc | 20 ++++++++++--------- test/integration/ssl_integration_test.cc | 12 ++++------- test/integration/ssl_utility.h | 3 +-- .../integration/tcp_proxy_integration_test.cc | 3 +-- 7 files changed, 30 insertions(+), 26 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index d1aeb5469c8a0..90c4f224020a5 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -45,11 +45,11 @@ name: "abc.com" EXPECT_EQ( TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), - server.secretManager().findTlsCertificate("abc.com")->certificateChain()); + server.secretManager().findTlsCertificate("", "abc.com")->certificateChain()); EXPECT_EQ( TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), - server.secretManager().findTlsCertificate("abc.com")->privateKey()); + server.secretManager().findTlsCertificate("", "abc.com")->privateKey()); } TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { @@ -70,7 +70,8 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { Server::MockInstance server; - std::string config_source_hash = server.secretManager().addOrUpdateSdsService(config_source); + std::string config_source_hash = + server.secretManager().addOrUpdateSdsService(config_source, "abc_config"); server.secretManager().addOrUpdateSecret(config_source_hash, secret_config); @@ -86,6 +87,7 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { } TEST_F(SecretManagerImplTest, NotImplementedException) { + envoy::api::v2::core::ConfigSource config_source; envoy::api::v2::auth::Secret secret_config; std::string yaml = @@ -101,8 +103,11 @@ name: "abc.com" Server::MockInstance server; std::unique_ptr secret_manager(new SecretManagerImpl(server)); - EXPECT_THROW_WITH_MESSAGE(server.secretManager().addOrUpdateSecret(secret_config), EnvoyException, - "Secret type not implemented"); + std::string config_source_hash = + server.secretManager().addOrUpdateSdsService(config_source, "abc_config"); + EXPECT_THROW_WITH_MESSAGE( + server.secretManager().addOrUpdateSecret(config_source_hash, secret_config), EnvoyException, + "Secret type not implemented"); } } // namespace diff --git a/test/common/ssl/BUILD b/test/common/ssl/BUILD index 8a9265f691da3..746c33eacc62e 100644 --- a/test/common/ssl/BUILD +++ b/test/common/ssl/BUILD @@ -62,6 +62,7 @@ envoy_cc_test( "//source/common/stats:stats_lib", "//test/mocks/runtime:runtime_mocks", "//test/mocks/secret:secret_mocks", + "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", ], ) diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 6e0ce308b77ab..d1d5739836237 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -184,6 +184,7 @@ class SslServerContextImplTicketTest : public SslContextImplTest { } static void loadConfigV2(envoy::api::v2::auth::DownstreamTlsContext& cfg) { + Server::MockInstance server; // Must add a certificate for the config to be considered valid. envoy::api::v2::auth::TlsCertificate* server_cert = cfg.mutable_common_tls_context()->add_tls_certificates(); @@ -198,6 +199,7 @@ class SslServerContextImplTicketTest : public SslContextImplTest { } static void loadConfigJson(const std::string& json) { + Server::MockInstance server; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); Secret::MockSecretManager secret_manager; ServerContextConfigImpl cfg(*loader, server.secretManager()); diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 095600f02cfe2..c3997f4710741 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -1516,7 +1516,7 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, std::vector{}); @@ -1574,7 +1574,7 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, std::vector{}); @@ -1595,7 +1595,7 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager_); + ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server_.secretManager()); ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2098,9 +2098,9 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); Json::ObjectSharedPtr server2_ctx_loader = TestEnvironment::jsonLoadFromString(server2_ctx_json); - ServerContextConfigImpl server2_ctx_config(*server2_ctx_loader, secret_manager_); + ServerContextConfigImpl server2_ctx_config(*server2_ctx_loader, server_.secretManager()); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, std::vector{}); @@ -2125,7 +2125,7 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager_); + ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server_.secretManager()); ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2211,7 +2211,7 @@ TEST_P(SslSocketTest, SslError) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, std::vector{}); @@ -2533,7 +2533,8 @@ class SslReadBufferLimitTest : public SslCertsTest, public: void initialize() { server_ctx_loader_ = TestEnvironment::jsonLoadFromString(server_ctx_json_); - server_ctx_config_.reset(new ServerContextConfigImpl(*server_ctx_loader_, secret_manager_)); + server_ctx_config_.reset( + new ServerContextConfigImpl(*server_ctx_loader_, server_.secretManager())); manager_.reset(new ContextManagerImpl(runtime_)); server_ssl_socket_factory_.reset(new ServerSslSocketFactory( *server_ctx_config_, *manager_, stats_store_, std::vector{})); @@ -2541,7 +2542,8 @@ class SslReadBufferLimitTest : public SslCertsTest, listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); - client_ctx_config_.reset(new ClientContextConfigImpl(*client_ctx_loader_, secret_manager_)); + client_ctx_config_.reset( + new ClientContextConfigImpl(*client_ctx_loader_, server_.secretManager())); client_ssl_socket_factory_.reset( new ClientSslSocketFactory(*client_ctx_config_, *manager_, stats_store_)); diff --git a/test/integration/ssl_integration_test.cc b/test/integration/ssl_integration_test.cc index 2cbd941cfa9a4..22fcf58d46f6b 100644 --- a/test/integration/ssl_integration_test.cc +++ b/test/integration/ssl_integration_test.cc @@ -35,14 +35,10 @@ void SslIntegrationTest::initialize() { context_manager_.reset(new ContextManagerImpl(*runtime_)); registerTestServerPorts({"http"}); - client_ssl_ctx_plain_ = - createClientSslTransportSocketFactory(false, false, *context_manager_, secret_manager_); - client_ssl_ctx_alpn_ = - createClientSslTransportSocketFactory(true, false, *context_manager_, secret_manager_); - client_ssl_ctx_san_ = - createClientSslTransportSocketFactory(false, true, *context_manager_, secret_manager_); - client_ssl_ctx_alpn_san_ = - createClientSslTransportSocketFactory(true, true, *context_manager_, secret_manager_); + client_ssl_ctx_plain_ = createClientSslTransportSocketFactory(false, false, *context_manager_); + client_ssl_ctx_alpn_ = createClientSslTransportSocketFactory(true, false, *context_manager_); + client_ssl_ctx_san_ = createClientSslTransportSocketFactory(false, true, *context_manager_); + client_ssl_ctx_alpn_san_ = createClientSslTransportSocketFactory(true, true, *context_manager_); } void SslIntegrationTest::TearDown() { diff --git a/test/integration/ssl_utility.h b/test/integration/ssl_utility.h index d2ff42561bd44..c55e081a1dd46 100644 --- a/test/integration/ssl_utility.h +++ b/test/integration/ssl_utility.h @@ -9,8 +9,7 @@ namespace Envoy { namespace Ssl { Network::TransportSocketFactoryPtr -createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager, - Secret::SecretManager& secret_manager); +createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager); Network::Address::InstanceConstSharedPtr getSslAddress(const Network::Address::IpVersion& version, int port); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index a7e7fecb9e3b1..b1333372ef98a 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -371,8 +371,7 @@ void TcpProxySslIntegrationTest::setupConnections() { // Set up the SSl client. Network::Address::InstanceConstSharedPtr address = Ssl::getSslAddress(version_, lookupPort("tcp_proxy")); - context_ = - Ssl::createClientSslTransportSocketFactory(false, false, *context_manager_, secret_manager_); + context_ = Ssl::createClientSslTransportSocketFactory(false, false, *context_manager_); ssl_client_ = dispatcher_->createClientConnection(address, Network::Address::InstanceConstSharedPtr(), context_->createTransportSocket(), nullptr); From 2f563577abeb1b609e99f1322f82103229db31b4 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 18:28:45 -0700 Subject: [PATCH 10/23] fix tests. Signed-off-by: JimmyCYJ --- .../grpc_client_integration_test_harness.h | 8 +++--- .../common/secret/secret_manager_impl_test.cc | 18 ++++++++++--- test/common/ssl/ssl_socket_test.cc | 27 ++++++++++--------- test/integration/xfcc_integration_test.cc | 5 ++-- test/integration/xfcc_integration_test.h | 4 +-- test/mocks/ssl/BUILD | 1 + test/server/configuration_impl_test.cc | 14 +++++----- 7 files changed, 45 insertions(+), 32 deletions(-) diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index f6c9c3fa152c6..32759e6925eac 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -11,7 +11,7 @@ #include "test/integration/fake_upstream.h" #include "test/mocks/grpc/mocks.h" #include "test/mocks/local_info/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/proto/helloworld.pb.h" @@ -444,7 +444,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); } - Ssl::ClientContextConfigImpl cfg(tls_context, secret_manager_); + Ssl::ClientContextConfigImpl cfg(tls_context, server_.secretManager()); mock_cluster_info_->transport_socket_factory_ = std::make_unique(cfg, context_manager_, *stats_store_); @@ -474,7 +474,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); } - Ssl::ServerContextConfigImpl cfg(tls_context, secret_manager_); + Ssl::ServerContextConfigImpl cfg(tls_context, server_.secretManager()); static Stats::Scope* upstream_stats_store = new Stats::IsolatedStoreImpl(); return std::make_unique( @@ -482,7 +482,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { } bool use_client_cert_{}; - Secret::MockSecretManager secret_manager_; + Server::MockInstance server_; Ssl::ContextManagerImpl context_manager_{runtime_}; }; diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 90c4f224020a5..1f46f03565dfa 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -16,7 +16,19 @@ namespace Envoy { namespace Secret { namespace { -class MockServer : public Server::MockInstance {}; +class MockServer : public Server::MockInstance { +public: + Init::Manager& initManager() { return initmanager_; } + +private: + class InitManager : public Init::Manager { + public: + void initialize(std::function callback); + void registerTarget(Init::Target&) override {} + }; + + InitManager initmanager_; +}; class SecretManagerImplTest : public testing::Test {}; @@ -68,7 +80,7 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { MessageUtil::loadFromYaml(yaml, secret_config); - Server::MockInstance server; + MockServer server; std::string config_source_hash = server.secretManager().addOrUpdateSdsService(config_source, "abc_config"); @@ -100,7 +112,7 @@ name: "abc.com" MessageUtil::loadFromYaml(yaml, secret_config); - Server::MockInstance server; + MockServer server; std::unique_ptr secret_manager(new SecretManagerImpl(server)); std::string config_source_hash = diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index c3997f4710741..6b172a4030590 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -52,10 +52,10 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ const Network::Address::IpVersion version) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager); + ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server.secretManager()); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, std::vector{}); @@ -68,7 +68,7 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager); + ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server.secretManager()); Ssl::ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -145,7 +145,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, const Network::Address::IpVersion version) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; ContextManagerImpl manager(runtime); std::string new_session = EMPTY_STRING; @@ -154,7 +154,8 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, const auto& filter_chain = server_proto.filter_chains(0); std::vector server_names(filter_chain.filter_chain_match().server_names().begin(), filter_chain.filter_chain_match().server_names().end()); - Ssl::ServerContextConfigImpl server_ctx_config(filter_chain.tls_context(), secret_manager); + Ssl::ServerContextConfigImpl server_ctx_config(filter_chain.tls_context(), + server.secretManager()); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, server_names); @@ -165,7 +166,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); - ClientContextConfigImpl client_ctx_config(client_ctx_proto, secret_manager); + ClientContextConfigImpl client_ctx_config(client_ctx_proto, server.secretManager()); ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); ClientContextPtr client_ctx(manager.createSslClientContext(stats_store, client_ctx_config)); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( @@ -1647,7 +1648,7 @@ TEST_P(SslSocketTest, HalfClose) { TEST_P(SslSocketTest, ClientAuthMultipleCAs) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; std::string server_ctx_json = R"EOF( { @@ -1658,7 +1659,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager); + ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server.secretManager()); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, std::vector{}); @@ -1678,7 +1679,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager); + ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server.secretManager()); ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -1735,13 +1736,13 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, const Network::Address::IpVersion ip_version) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - Secret::MockSecretManager secret_manager; + Server::MockInstance server; ContextManagerImpl manager(runtime); Json::ObjectSharedPtr server_ctx_loader1 = TestEnvironment::jsonLoadFromString(server_ctx_json1); Json::ObjectSharedPtr server_ctx_loader2 = TestEnvironment::jsonLoadFromString(server_ctx_json2); - ServerContextConfigImpl server_ctx_config1(*server_ctx_loader1, secret_manager); - ServerContextConfigImpl server_ctx_config2(*server_ctx_loader2, secret_manager); + ServerContextConfigImpl server_ctx_config1(*server_ctx_loader1, server.secretManager()); + ServerContextConfigImpl server_ctx_config2(*server_ctx_loader2, server.secretManager()); Ssl::ServerSslSocketFactory server_ssl_socket_factory1(server_ctx_config1, manager, stats_store, server_names1); Ssl::ServerSslSocketFactory server_ssl_socket_factory2(server_ctx_config2, manager, stats_store, @@ -1758,7 +1759,7 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager); + ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server.secretManager()); ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket1.localAddress(), Network::Address::InstanceConstSharedPtr(), diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index f0d4f70eda79d..ed92338dd76f3 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -12,6 +12,7 @@ #include "common/ssl/context_manager_impl.h" #include "common/ssl/ssl_socket.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/network_utility.h" #include "test/test_common/printers.h" #include "test/test_common/utility.h" @@ -58,7 +59,7 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b target = json_tls; } Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - Ssl::ClientContextConfigImpl cfg(*loader, secret_manager_); + Ssl::ClientContextConfigImpl cfg(*loader, server_.secretManager()); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); return Network::TransportSocketFactoryPtr{ new Ssl::ClientSslSocketFactory(cfg, *context_manager_, *client_stats_store)}; @@ -73,7 +74,7 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - Ssl::ServerContextConfigImpl cfg(*loader, secret_manager_); + Ssl::ServerContextConfigImpl cfg(*loader, server_.secretManager()); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( cfg, *context_manager_, *upstream_stats_store, std::vector{}); diff --git a/test/integration/xfcc_integration_test.h b/test/integration/xfcc_integration_test.h index 3432313af7153..3d0c81a0ad09d 100644 --- a/test/integration/xfcc_integration_test.h +++ b/test/integration/xfcc_integration_test.h @@ -6,7 +6,7 @@ #include "test/integration/http_integration.h" #include "test/integration/server.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -56,7 +56,7 @@ class XfccIntegrationTest : public HttpIntegrationTest, Network::TransportSocketFactoryPtr client_tls_ssl_ctx_; Network::TransportSocketFactoryPtr client_mtls_ssl_ctx_; Network::TransportSocketFactoryPtr upstream_ssl_ctx_; - Secret::MockSecretManager secret_manager_; + Server::MockInstance server_; }; } // namespace Xfcc } // namespace Envoy diff --git a/test/mocks/ssl/BUILD b/test/mocks/ssl/BUILD index 39330f6db3538..fd044e7726de2 100644 --- a/test/mocks/ssl/BUILD +++ b/test/mocks/ssl/BUILD @@ -13,6 +13,7 @@ envoy_cc_mock( srcs = ["mocks.cc"], hdrs = ["mocks.h"], deps = [ + "//include/envoy/secret:secret_manager_interface", "//include/envoy/ssl:connection_interface", "//include/envoy/ssl:context_config_interface", "//include/envoy/ssl:context_interface", diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index e3bd14daf60be..da492c5ef5f6c 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -329,9 +329,9 @@ TEST_F(ConfigurationImplTest, StaticSecretRead) { name: "abc.com" tls_certificate: certificate_chain: - filename: "test/common/ssl/test_data/selfsigned_cert.pem" + filename: "test/config/integration/certs/cacert.pem" private_key: - filename: "test/common/ssl/test_data/selfsigned_key.pem" + filename: "test/config/integration/certs/cakey.pem" )EOF"; MessageUtil::loadFromYaml(yaml, *secret_config); @@ -343,13 +343,11 @@ TEST_F(ConfigurationImplTest, StaticSecretRead) { ASSERT_NE(secret, nullptr); - EXPECT_EQ( - TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), - secret->certificateChain()); + EXPECT_EQ(TestEnvironment::readFileToStringForTest("test/config/integration/certs/cacert.pem"), + secret->certificateChain()); - EXPECT_EQ( - TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), - secret->privateKey()); + EXPECT_EQ(TestEnvironment::readFileToStringForTest("test/config/integration/certs/cakey.pem"), + secret->privateKey()); } } // namespace Configuration From 051d7b3c55d86cd4bcef7088cdf59df3a4611201 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 19 Jun 2018 18:57:16 -0700 Subject: [PATCH 11/23] Minor changes SecretManager. Signed-off-by: JimmyCYJ --- source/common/secret/sds_api.cc | 5 ++--- source/common/secret/sds_api.h | 2 +- source/common/secret/secret_manager_impl.cc | 6 ++++-- source/common/ssl/context_config_impl.cc | 9 +++------ source/server/configuration_impl.cc | 3 ++- test/common/ssl/context_impl_test.cc | 2 +- 6 files changed, 13 insertions(+), 14 deletions(-) diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 2b26769213837..e30630e438891 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -10,9 +10,8 @@ namespace Envoy { namespace Secret { SdsApi::SdsApi(Server::Instance& server, const envoy::api::v2::core::ConfigSource& sds_config, - std::string sds_config_name) - : server_(server), sds_config_(sds_config), - sds_config_source_hash_(SecretManagerUtil::configSourceHash(sds_config)), + std::string sds_config_hash, std::string sds_config_name) + : server_(server), sds_config_(sds_config), sds_config_source_hash_(sds_config_hash), sds_config_name_(sds_config_name) { server_.initManager().registerTarget(*this); } diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index dfad58adaaa3a..341b081d7c648 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -16,7 +16,7 @@ namespace Secret { class SdsApi : public Init::Target, Config::SubscriptionCallbacks { public: SdsApi(Server::Instance& server, const envoy::api::v2::core::ConfigSource& sds_config, - std::string sds_config_name); + std::string sds_config_hash, std::string sds_config_name); // Init::Target void initialize(std::function callback) override; diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 5931cd7b98711..18a7d9000bb89 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -40,11 +40,13 @@ std::string SecretManagerImpl::addOrUpdateSdsService( std::unique_lock lhs(sds_api_mutex_); auto hash = SecretManagerUtil::configSourceHash(sds_config_source); - if (sds_apis_.find(hash) != sds_apis_.end()) { + std::string sds_apis_key = hash + config_name; + if (sds_apis_.find(sds_apis_key) != sds_apis_.end()) { return hash; } - sds_apis_[hash] = std::move(std::make_unique(server_, sds_config_source, config_name)); + sds_apis_[sds_apis_key] = + std::move(std::make_unique(server_, sds_config_source, hash, config_name)); return hash; } diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 979d3c47c9a43..db4bdcfce3f6b 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -21,7 +21,7 @@ namespace { std::string readSdsSecretName(const envoy::api::v2::auth::CommonTlsContext& config) { return (!config.tls_certificate_sds_secret_configs().empty()) ? config.tls_certificate_sds_secret_configs()[0].name() - : ""; + : EMPTY_STRING; } std::string readConfigSourceHash(const envoy::api::v2::auth::CommonTlsContext& config, @@ -31,12 +31,12 @@ std::string readConfigSourceHash(const envoy::api::v2::auth::CommonTlsContext& c ? secret_manager.addOrUpdateSdsService( config.tls_certificate_sds_secret_configs()[0].sds_config(), config.tls_certificate_sds_secret_configs()[0].name()) - : ""; + : EMPTY_STRING; } std::string readConfig( const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager, - const std::string config_source_hash, const std::string secret_name, + const std::string& config_source_hash, const std::string& secret_name, const std::function& read_inline_config, const std::function& @@ -45,9 +45,6 @@ std::string readConfig( return read_inline_config(config.tls_certificates()[0]); } else if (!config.tls_certificate_sds_secret_configs().empty()) { const auto secret = secret_manager.findTlsCertificate(config_source_hash, secret_name); - if (!secret) { - throw EnvoyException(fmt::format("Static secret is not defined: {}", secret_name)); - } if (!secret) { if (config_source_hash.empty()) { throw EnvoyException( diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 673215cbef956..dd9b8b3165197 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -13,6 +13,7 @@ #include "envoy/ssl/context_manager.h" #include "common/common/assert.h" +#include "common/common/empty_string.h" #include "common/common/utility.h" #include "common/config/lds_json.h" #include "common/config/utility.h" @@ -50,7 +51,7 @@ void MainImpl::initialize(const envoy::config::bootstrap::v2::Bootstrap& bootstr ENVOY_LOG(info, "loading {} static secret(s)", secrets.size()); for (ssize_t i = 0; i < secrets.size(); i++) { ENVOY_LOG(debug, "static secret #{}: {}", i, secrets[i].name()); - server.secretManager().addOrUpdateSecret("", secrets[i]); + server.secretManager().addOrUpdateSecret(EMPTY_STRING, secrets[i]); } cluster_manager_ = cluster_manager_factory.clusterManagerFromProto( diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index d1d5739836237..4a034a6981f8a 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -484,7 +484,7 @@ name: "abc.com" EXPECT_THROW_WITH_MESSAGE( ClientContextConfigImpl client_context_config(tls_context, server.secretManager()), - EnvoyException, "Static secret is not defined: missing"); + EnvoyException, "Unknown static secret: : missing"); } // Multiple TLS certificates are not yet supported, but one is expected for From 0554e71ba1391d2189c83b6f7b0d119e4af9c837 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 20 Jun 2018 09:56:22 -0700 Subject: [PATCH 12/23] Use SdsApiPtr Signed-off-by: JimmyCYJ --- source/common/secret/sds_api.h | 2 ++ source/common/secret/secret_manager_impl.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 341b081d7c648..faf95bbfb2e21 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -39,5 +39,7 @@ class SdsApi : public Init::Target, Config::SubscriptionCallbacks SdsApiPtr; + } // namespace Secret } // namespace Envoy \ No newline at end of file diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 82f44ef1855e2..342b119d42425 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -27,7 +27,7 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable> sds_apis_; + std::unordered_map sds_apis_; mutable std::shared_timed_mutex sds_api_mutex_; // Manages pairs of name and Ssl::TlsCertificateConfig grouped by SDS config source hash. From fa631a1856283d7c0889bc973f630a9bdf0c7c71 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 20 Jun 2018 10:02:52 -0700 Subject: [PATCH 13/23] minor fix Signed-off-by: JimmyCYJ --- source/common/ssl/context_config_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index db4bdcfce3f6b..f17585d25e973 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -39,7 +39,7 @@ std::string readConfig( const std::string& config_source_hash, const std::string& secret_name, const std::function& read_inline_config, - const std::function& + const std::function& read_managed_secret) { if (!config.tls_certificates().empty()) { return read_inline_config(config.tls_certificates()[0]); @@ -53,7 +53,7 @@ std::string readConfig( return EMPTY_STRING; } } - return read_managed_secret(secret); + return read_managed_secret(*secret); } else { return EMPTY_STRING; } From d6f4d28544e92d5ea50bd9cc70063f766eb493c3 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 20 Jun 2018 10:08:23 -0700 Subject: [PATCH 14/23] updated readConfig() Signed-off-by: JimmyCYJ --- source/common/secret/secret_manager_impl.cc | 3 +-- source/common/ssl/context_config_impl.cc | 8 ++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 18a7d9000bb89..d83c7ff3915d3 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -45,8 +45,7 @@ std::string SecretManagerImpl::addOrUpdateSdsService( return hash; } - sds_apis_[sds_apis_key] = - std::move(std::make_unique(server_, sds_config_source, hash, config_name)); + sds_apis_[sds_apis_key] = std::make_unique(server_, sds_config_source, hash, config_name); return hash; } diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index f17585d25e973..f3fd055903306 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -98,8 +98,8 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex [](const envoy::api::v2::auth::TlsCertificate& tls_certificate) -> std::string { return Config::DataSource::read(tls_certificate.certificate_chain(), true); }, - [](const Ssl::TlsCertificateConfig* secret) -> std::string { - return secret->certificateChain(); + [](const Ssl::TlsCertificateConfig& secret) -> std::string { + return secret.certificateChain(); })), cert_chain_path_( config.tls_certificates().empty() @@ -110,8 +110,8 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex [](const envoy::api::v2::auth::TlsCertificate& tls_certificate) -> std::string { return Config::DataSource::read(tls_certificate.private_key(), true); }, - [](const Ssl::TlsCertificateConfig* secret) -> std::string { - return secret->privateKey(); + [](const Ssl::TlsCertificateConfig& secret) -> std::string { + return secret.privateKey(); })), private_key_path_( config.tls_certificates().empty() From 50010a162f4d5b75f50a7844434bd80fcb7c876b Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 20 Jun 2018 17:29:18 -0700 Subject: [PATCH 15/23] Update dynamic secret. Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 11 ++ include/envoy/secret/secret_manager.h | 21 ++- include/envoy/ssl/context_config.h | 11 ++ include/envoy/ssl/context_manager.h | 32 +++++ include/envoy/ssl/tls_certificate_config.h | 8 +- source/common/config/BUILD | 1 + source/common/config/protobuf_link_hacks.h | 2 + source/common/config/resources.h | 1 + source/common/secret/BUILD | 1 + source/common/secret/secret_manager_impl.cc | 53 ++++++- source/common/secret/secret_manager_impl.h | 27 +++- source/common/secret/secret_manager_util.h | 6 +- source/common/ssl/BUILD | 3 + source/common/ssl/context_config_impl.h | 3 + source/common/ssl/context_manager_impl.cc | 47 ++++++- source/common/ssl/context_manager_impl.h | 20 ++- source/common/ssl/ssl_socket.h | 39 ++++-- .../common/ssl/tls_certificate_config_impl.cc | 6 + .../common/ssl/tls_certificate_config_impl.h | 1 + source/common/upstream/upstream_impl.cc | 8 +- .../transport_sockets/ssl/config.cc | 40 ++++-- source/server/config_validation/server.cc | 2 +- source/server/connection_handler_impl.cc | 8 ++ source/server/server.cc | 5 +- .../grpc_client_integration_test_harness.h | 12 +- test/common/secret/BUILD | 2 + .../common/secret/secret_manager_impl_test.cc | 14 ++ test/common/ssl/context_impl_test.cc | 18 +-- test/common/ssl/ssl_socket_test.cc | 129 +++++++++--------- 29 files changed, 409 insertions(+), 122 deletions(-) diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index c4dcf8404fd6f..882a8905359dd 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -16,3 +16,14 @@ envoy_cc_library( "@envoy_api//envoy/api/v2/auth:cert_cc", ], ) + +envoy_cc_library( + name = "secret_manager_interface", + hdrs = ["secret_manager.h"], + deps = [ + ":secret_callbacks_interface", + "//include/envoy/ssl:tls_certificate_config_interface", + "@envoy_api//envoy/api/v2/auth:cert_cc", + "@envoy_api//envoy/api/v2/core:config_source_cc", + ], +) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index bf2f472ed444d..f119ee7d2fb2a 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -9,9 +9,7 @@ namespace Envoy { namespace Secret { /** - * A manager for static secrets. - * - * TODO(jaebong) Support dynamic secrets. + * A manager for static and dynamic secrets. */ class SecretManager { public: @@ -31,7 +29,7 @@ class SecretManager { * @param name a name of the Ssl::TlsCertificateConfig. * @return the TlsCertificate secret. Returns nullptr if the secret is not found. */ - virtual const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& config_source_hash, + virtual const Ssl::TlsCertificateConfigSharedPtr findTlsCertificate(const std::string& config_source_hash, const std::string& name) const PURE; /** @@ -39,11 +37,22 @@ class SecretManager { * config source. * * @param sdsConfigSource a protobuf message object contains SDS config source. - * @param config_name a name that uniquely refers to the SDS config source - * @return a hash string of normalized config source + * @param config_name a name that uniquely refers to the SDS config source. + * @return a hash string of normalized config source. */ virtual std::string addOrUpdateSdsService(const envoy::api::v2::core::ConfigSource& config_source, std::string config_name) PURE; + + /** + * Register callback function which is to be invoked on secret update. + * + * @param config_source_hash Hash code of ConfigSource. + * @param secret_name name of the secret. + * @param callback SecretCallbacks class. + */ + virtual void registerTlsCertificateConfigCallbacks(const std::string& config_source_hash, + const std::string& secret_name, + SecretCallbacks& callback) PURE; }; } // namespace Secret diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index a56a89e903e6b..02b53dd4dce3a 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -111,6 +111,17 @@ class ContextConfig { * @return The maximum TLS protocol version to negotiate. */ virtual unsigned maxProtocolVersion() const PURE; + + /** + * @return The hash code of SdsSecretConfig in std::string. If the SdsSecretConfig is empty, then + * returns empty string. + */ + virtual const std::string& sdsConfigSourceHash() const PURE; + + /** + * @return The secret name in SdsSecretConfig. If SdsSecretConfig is empty, returns empty string. + */ + virtual const std::string& sdsSecretName() const PURE; }; class ClientContextConfig : public virtual ContextConfig { diff --git a/include/envoy/ssl/context_manager.h b/include/envoy/ssl/context_manager.h index 7489800c99caf..58a0a95cac4b8 100644 --- a/include/envoy/ssl/context_manager.h +++ b/include/envoy/ssl/context_manager.h @@ -2,6 +2,7 @@ #include +#include "envoy/secret/secret_manager.h" #include "envoy/ssl/context.h" #include "envoy/ssl/context_config.h" #include "envoy/stats/stats.h" @@ -22,6 +23,18 @@ class ContextManager { virtual ClientContextPtr createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config) PURE; + /** + * Updates ClientContext and returns updated ClientContext. + * + * @param context ClientContext to be updated. + * @param scope stats scope. + * @param config supplies the configuration for ClientContext. + * @return an updated ClientContext. + */ + virtual ClientContextPtr updateSslClientContext(const ClientContextPtr& context, + Stats::Scope& scope, + const ClientContextConfig& config) PURE; + /** * Builds a ServerContext from a ServerContextConfig. */ @@ -29,6 +42,20 @@ class ContextManager { createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) PURE; + /** + * Updates ServerContext and returns updated ServerContext. + * + * @param context ServerContext to be updated. + * @param scope stats scope + * @param config supplies the configuration for ServerContext. + * @param server_names server names. + * @return an updated ServerContext. + */ + virtual ServerContextPtr + updateSslServerContext(const ServerContextPtr& context, Stats::Scope& scope, + const ServerContextConfig& config, + const std::vector& server_names) PURE; + /** * @return the number of days until the next certificate being managed will expire. */ @@ -38,6 +65,11 @@ class ContextManager { * Iterate through all currently allocated contexts. */ virtual void iterateContexts(std::function callback) PURE; + + /** + * @return a SecretManager. + */ + virtual Secret::SecretManager& secretManager() PURE; }; } // namespace Ssl diff --git a/include/envoy/ssl/tls_certificate_config.h b/include/envoy/ssl/tls_certificate_config.h index 6a5c8c842a060..437c738a1f76b 100644 --- a/include/envoy/ssl/tls_certificate_config.h +++ b/include/envoy/ssl/tls_certificate_config.h @@ -21,9 +21,15 @@ class TlsCertificateConfig { * @return a string of private key */ virtual const std::string& privateKey() const PURE; + + /** + * @return true if secret contains same certificate chain and private key. + * Otherwise returns false. + */ + virtual bool equalTo(const TlsCertificateConfig& secret) const PURE; }; -typedef std::unique_ptr TlsCertificateConfigPtr; +typedef std::shared_ptr TlsCertificateConfigSharedPtr; } // namespace Ssl } // namespace Envoy diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 83067aa8806eb..ba821c967bd73 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -237,6 +237,7 @@ envoy_cc_library( hdrs = ["protobuf_link_hacks.h"], deps = [ "@envoy_api//envoy/service/discovery/v2:ads_cc", + "@envoy_api//envoy/service/discovery/v2:sds_cc", "@envoy_api//envoy/service/ratelimit/v2:rls_cc", ], ) diff --git a/source/common/config/protobuf_link_hacks.h b/source/common/config/protobuf_link_hacks.h index 6792f3e797c1e..243d9ecf69a20 100644 --- a/source/common/config/protobuf_link_hacks.h +++ b/source/common/config/protobuf_link_hacks.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/service/discovery/v2/ads.pb.h" +#include "envoy/service/discovery/v2/sds.pb.h" #include "envoy/service/ratelimit/v2/rls.pb.h" namespace Envoy { @@ -8,5 +9,6 @@ namespace Envoy { // Hack to force linking of the service: https://github.com/google/protobuf/issues/4221. // This file should be included ONLY if this hack is required. const envoy::service::discovery::v2::AdsDummy _ads_dummy; +const envoy::service::discovery::v2::SdsDummy _sds_dummy; const envoy::service::ratelimit::v2::RateLimitRequest _rls_dummy; } // namespace Envoy diff --git a/source/common/config/resources.h b/source/common/config/resources.h index 03f0c9a2efd8b..69ed2d91a46dc 100644 --- a/source/common/config/resources.h +++ b/source/common/config/resources.h @@ -15,6 +15,7 @@ class TypeUrlValues { const std::string Listener{"type.googleapis.com/envoy.api.v2.Listener"}; const std::string Cluster{"type.googleapis.com/envoy.api.v2.Cluster"}; const std::string ClusterLoadAssignment{"type.googleapis.com/envoy.api.v2.ClusterLoadAssignment"}; + const std::string Secret{"type.googleapis.com/envoy.api.v2.auth.Secret"}; const std::string RouteConfiguration{"type.googleapis.com/envoy.api.v2.RouteConfiguration"}; }; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 90455caee6efd..e6faac70c5e4b 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -16,6 +16,7 @@ envoy_cc_library( ":sds_api_lib", ":secret_manager_util", "//include/envoy/secret:secret_manager_interface", + "//include/envoy/server:instance_interface", "//source/common/common:minimal_logger_lib", "//source/common/ssl:tls_certificate_config_impl_lib", "@envoy_api//envoy/api/v2/auth:cert_cc", diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index d83c7ff3915d3..07c415dd1d0f0 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -13,15 +13,37 @@ void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, switch (secret.type_case()) { case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: { std::unique_lock lhs(tls_certificate_secrets_mutex_); - tls_certificate_secrets_[config_source_hash][secret.name()] = - std::make_unique(secret.tls_certificate()); + auto tls_certificate_secret = std::make_shared(secret.tls_certificate()); + tls_certificate_secrets_[config_source_hash][secret.name()] = tls_certificate_secret; + + if (config_source_hash.empty()) { + return; + } + + std::string secret_name = secret.name(); + server_.dispatcher().post([this, config_source_hash, secret_name, secret]() { + std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); + auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash); + if (config_source_it != tls_certificate_secret_update_callbacks_.end()) { + auto callback_it = config_source_it->second.find(secret_name); + if (callback_it != config_source_it->second.end()) { + if (callback_it->second.first == nullptr || + !callback_it->second.first->equalTo(*tls_certificate_secret)) { + for (auto& callback : callback_it->second.second) { + callback->onAddOrUpdateSecret(); + } + callback_it->second.first = secret; + } + } + } + }); } break; default: throw EnvoyException("Secret type not implemented"); } } -const Ssl::TlsCertificateConfig* +const Ssl::TlsCertificateConfigSharedPtr SecretManagerImpl::findTlsCertificate(const std::string& config_source_hash, const std::string& name) const { std::shared_lock lhs(tls_certificate_secrets_mutex_); @@ -32,7 +54,7 @@ SecretManagerImpl::findTlsCertificate(const std::string& config_source_hash, } auto secret = config_source_it->second.find(name); - return (secret != config_source_it->second.end()) ? secret->second.get() : nullptr; + return (secret != config_source_it->second.end()) ? secret->second : nullptr; } std::string SecretManagerImpl::addOrUpdateSdsService( @@ -50,5 +72,28 @@ std::string SecretManagerImpl::addOrUpdateSdsService( return hash; } +void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& config_source_hash, + const std::string& secret_name, + SecretCallbacks& callback) { + auto secret = findTlsCertificate(config_source_hash, secret_name); + + std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); + + auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash); + if (config_source_it == tls_certificate_secret_update_callbacks_.end()) { + tls_certificate_secret_update_callbacks_[config_source_hash][secret_name] = {secret, + {&callback}}; + return; + } + + auto name_it = config_source_it->second.find(secret_name); + if (name_it == config_source_it->second.end()) { + config_source_it->second[secret_name] = {secret, {&callback}}; + return; + } + + name_it->second.second.push_back(&callback); +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 342b119d42425..44fed538c1c7c 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -19,22 +19,41 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable sds_apis_; mutable std::shared_timed_mutex sds_api_mutex_; // Manages pairs of name and Ssl::TlsCertificateConfig grouped by SDS config source hash. // If SDS config source hash is empty, it is a static secret. - std::unordered_map> + std::unordered_map> tls_certificate_secrets_; mutable std::shared_timed_mutex tls_certificate_secrets_mutex_; + + // callback functions for secret update + // "config source hash": { + // "secret name": + // secret, + // [{callback}] + // ] + // } + std::unordered_map>>> + tls_certificate_secret_update_callbacks_; + mutable std::shared_timed_mutex tls_certificate_secret_update_callbacks_mutex_; }; } // namespace Secret diff --git a/source/common/secret/secret_manager_util.h b/source/common/secret/secret_manager_util.h index d81060dcff5a2..5571e28943867 100644 --- a/source/common/secret/secret_manager_util.h +++ b/source/common/secret/secret_manager_util.h @@ -15,10 +15,10 @@ class SecretManagerUtil { /** * Calculate hash code of ConfigSource. To identify the same ConfigSource, calculate the hash - * code from the ConfigSource + * code from the ConfigSource. * - * @param config_source envoy::api::v2::core::ConfigSource - * @return hash code + * @param config_source envoy::api::v2::core::ConfigSource. + * @return hash code. */ static std::string configSourceHash(const envoy::api::v2::core::ConfigSource& config_source) { std::string jsonstr; diff --git a/source/common/ssl/BUILD b/source/common/ssl/BUILD index 486d0da347dca..b40b86820e8ef 100644 --- a/source/common/ssl/BUILD +++ b/source/common/ssl/BUILD @@ -18,6 +18,8 @@ envoy_cc_library( ":context_lib", "//include/envoy/network:connection_interface", "//include/envoy/network:transport_socket_interface", + "//include/envoy/secret:secret_callbacks_interface", + "//include/envoy/secret:secret_manager_interface", "//source/common/common:assert_lib", "//source/common/common:empty_string", "//source/common/common:minimal_logger_lib", @@ -58,6 +60,7 @@ envoy_cc_library( external_deps = ["ssl"], deps = [ "//include/envoy/runtime:runtime_interface", + "//include/envoy/secret:secret_manager_interface", "//include/envoy/ssl:context_config_interface", "//include/envoy/ssl:context_interface", "//include/envoy/ssl:context_manager_interface", diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 04e39788712dd..ddaeef9aa519d 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -54,6 +54,9 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { unsigned minProtocolVersion() const override { return min_protocol_version_; }; unsigned maxProtocolVersion() const override { return max_protocol_version_; }; + const std::string& sdsConfigShourceHash() const override { return sds_config_source_hash_; } + const std::string& sdsSecretName() const override { return sds_secret_name_; } + protected: ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager); diff --git a/source/common/ssl/context_manager_impl.cc b/source/common/ssl/context_manager_impl.cc index 1c54c2f018656..eb07512ce5385 100644 --- a/source/common/ssl/context_manager_impl.cc +++ b/source/common/ssl/context_manager_impl.cc @@ -17,23 +17,64 @@ void ContextManagerImpl::releaseContext(Context* context) { // context may not be found, in the case that a subclass of Context throws // in it's constructor. In that case the context did not get added, but // the destructor of Context will run and call releaseContext(). - contexts_.remove(context); + contexts_.erase(context); } ClientContextPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config) { + if (!config.sdsConfigShourceHash().empty() && !config.sdsSecretName().empty() && + config.certChain().empty() && config.privateKey().empty()) { + return nullptr; + } + ClientContextPtr context(new ClientContextImpl(*this, scope, config)); std::unique_lock lock(contexts_lock_); - contexts_.emplace_back(context.get()); + contexts_.emplace(context.get()); + return context; +} + +Ssl::ClientContextPtr +ContextManagerImpl::updateSslClientContext(const Ssl::ClientContextPtr& client_context, + Stats::Scope& scope, const ClientContextConfig& config) { + std::unique_lock lock(contexts_lock_); + + if (contexts_.erase(client_context.get()) == 0) { + return nullptr; + } + + ClientContextPtr context(new ClientContextImpl(*this, scope, config)); + contexts_.emplace(context.get()); + return context; } ServerContextPtr ContextManagerImpl::createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) { + if (!config.sdsConfigShourceHash().empty() && !config.sdsSecretName().empty() && + config.certChain().empty() && config.privateKey().empty()) { + return nullptr; + } + ServerContextPtr context(new ServerContextImpl(*this, scope, config, server_names, runtime_)); std::unique_lock lock(contexts_lock_); - contexts_.emplace_back(context.get()); + contexts_.emplace(context.get()); + return context; +} + +ServerContextPtr +ContextManagerImpl::updateSslServerContext(const ServerContextPtr& server_context, + Stats::Scope& scope, const ServerContextConfig& config, + const std::vector& server_names) { + std::unique_lock lock(contexts_lock_); + + if (contexts_.erase(server_context.get()) == 0) { + return nullptr; + } + + ServerContextPtr context(new ServerContextImpl(*this, scope, config, server_names, runtime_)); + contexts_.emplace(context.get()); + return context; } diff --git a/source/common/ssl/context_manager_impl.h b/source/common/ssl/context_manager_impl.h index bd31db4f22008..7aff474dfa529 100644 --- a/source/common/ssl/context_manager_impl.h +++ b/source/common/ssl/context_manager_impl.h @@ -1,10 +1,11 @@ #pragma once #include -#include +#include #include #include "envoy/runtime/runtime.h" +#include "envoy/secret/secret_manager.h" #include "envoy/ssl/context_manager.h" namespace Envoy { @@ -19,7 +20,8 @@ namespace Ssl { */ class ContextManagerImpl final : public ContextManager { public: - ContextManagerImpl(Runtime::Loader& runtime) : runtime_(runtime) {} + ContextManagerImpl(Runtime::Loader& runtime, Secret::SecretManager& secret_manager) + : runtime_(runtime), secret_manager_(secret_manager) {} ~ContextManagerImpl(); /** @@ -32,16 +34,28 @@ class ContextManagerImpl final : public ContextManager { // Ssl::ContextManager Ssl::ClientContextPtr createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config) override; + Ssl::ClientContextPtr updateSslClientContext(const Ssl::ClientContextPtr& context, + Stats::Scope& scope, + const ClientContextConfig& config) override; + Ssl::ServerContextPtr createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) override; + virtual ServerContextPtr + updateSslServerContext(const ServerContextPtr& context, Stats::Scope& scope, + const ServerContextConfig& config, + const std::vector& server_names) override; + size_t daysUntilFirstCertExpires() const override; void iterateContexts(std::function callback) override; + Secret::SecretManager& secretManager() override { return secret_manager_; } + private: Runtime::Loader& runtime_; - std::list contexts_; + std::set contexts_; mutable std::shared_timed_mutex contexts_lock_; + Secret::SecretManager& secret_manager_; }; } // namespace Ssl diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index 6bb040edcd7ef..3b4930ddc4403 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -5,6 +5,7 @@ #include "envoy/network/connection.h" #include "envoy/network/transport_socket.h" +#include "envoy/secret/secret_callbacks.h" #include "common/common/logger.h" #include "common/ssl/context_impl.h" @@ -64,27 +65,49 @@ class SslSocket : public Network::TransportSocket, mutable std::string cached_url_encoded_pem_encoded_peer_certificate_; }; -class ClientSslSocketFactory : public Network::TransportSocketFactory { +class ClientSslSocketFactory : public Network::TransportSocketFactory, + public Secret::SecretCallbacks, + Logger::Loggable { public: - ClientSslSocketFactory(const ClientContextConfig& config, Ssl::ContextManager& manager, - Stats::Scope& stats_scope); + ClientSslSocketFactory(const ClientContextConfigPtr config, + Ssl::ContextManager& manager, Stats::Scope& stats_scope); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; + // Secret::SecretCallbacks + void onAddOrUpdateSecret() override; + private: - const ClientContextPtr ssl_ctx_; + ClientContextPtr ssl_ctx_; + ClientContextConfigPtr config_; + Ssl::ContextManager& manager_; + Stats::Scope& stats_scope_; }; -class ServerSslSocketFactory : public Network::TransportSocketFactory { +typedef std::unique_ptr ClientContextConfigPtr; + +class ServerSslSocketFactory : public Network::TransportSocketFactory, + public Secret::SecretCallbacks, + Logger::Loggable { public: - ServerSslSocketFactory(const ServerContextConfig& config, Ssl::ContextManager& manager, - Stats::Scope& stats_scope, const std::vector& server_names); + ServerSslSocketFactory(const ServerContextConfigPtr config, + Ssl::ContextManager& manager, Stats::Scope& stats_scope, + const std::vector& server_names); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; + // Secret::SecretCallbacks + void onAddOrUpdateSecret() override; + private: - const ServerContextPtr ssl_ctx_; + ServerContextPtr ssl_ctx_; + ServerContextConfigPtr config_; + Ssl::ContextManager& manager_; + Stats::Scope& stats_scope_; + const std::vector server_names_; }; +typedef std::unique_ptr ServerContextConfigPtr; + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/tls_certificate_config_impl.cc b/source/common/ssl/tls_certificate_config_impl.cc index 4f0afeb49733f..16cf2e99683c4 100644 --- a/source/common/ssl/tls_certificate_config_impl.cc +++ b/source/common/ssl/tls_certificate_config_impl.cc @@ -1,5 +1,7 @@ #include "common/ssl/tls_certificate_config_impl.h" +#include + #include "envoy/common/exception.h" #include "common/config/datasource.h" @@ -12,5 +14,9 @@ TlsCertificateConfigImpl::TlsCertificateConfigImpl( : certificate_chain_(Config::DataSource::read(config.certificate_chain(), true)), private_key_(Config::DataSource::read(config.private_key(), true)) {} +bool TlsCertificateConfigImpl::equalTo(const TlsCertificateConfig& secret) const { + return certificate_chain_ == secret.certificateChain() && private_key_ == secret.privateKey(); +} + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/tls_certificate_config_impl.h b/source/common/ssl/tls_certificate_config_impl.h index b8875ffd6998e..2ba50c4e09f39 100644 --- a/source/common/ssl/tls_certificate_config_impl.h +++ b/source/common/ssl/tls_certificate_config_impl.h @@ -14,6 +14,7 @@ class TlsCertificateConfigImpl : public TlsCertificateConfig { const std::string& certificateChain() const override { return certificate_chain_; } const std::string& privateKey() const override { return private_key_; } + bool equalTo(const TlsCertificateConfig& secret) const override; private: const std::string certificate_chain_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index ea36ac333fe28..412e3e224f8a5 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -132,9 +132,13 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu connection_options = options; } + auto transport_socket = cluster.transportSocketFactory().createTransportSocket(); + if (!transport_socket) { + return nullptr; + } + Network::ClientConnectionPtr connection = dispatcher.createClientConnection( - address, cluster.sourceAddress(), cluster.transportSocketFactory().createTransportSocket(), - connection_options); + address, cluster.sourceAddress(), std::move(transport_socket), connection_options); connection->setBufferLimits(cluster.perConnectionBufferLimitBytes()); return connection; } diff --git a/source/extensions/transport_sockets/ssl/config.cc b/source/extensions/transport_sockets/ssl/config.cc index c0c0feec1970a..d3f20da33947f 100644 --- a/source/extensions/transport_sockets/ssl/config.cc +++ b/source/extensions/transport_sockets/ssl/config.cc @@ -16,12 +16,22 @@ namespace SslTransport { Network::TransportSocketFactoryPtr UpstreamSslSocketFactory::createTransportSocketFactory( const Protobuf::Message& message, Server::Configuration::TransportSocketFactoryContext& context) { - return std::make_unique( - Ssl::ClientContextConfigImpl( + std::unique_ptr upstream_config = + std::make_unique( MessageUtil::downcastAndValidate( message), - context.secretManager()), - context.sslContextManager(), context.statsScope()); + context.secretManager()); + + auto hash = upstream_config->sdsConfigShourceHash(); + auto name = upstream_config->sdsSecretName(); + + std::unique_ptr tsf = std::make_unique( + std::move(upstream_config), context.sslContextManager(), context.statsScope()); + if (!hash.empty()) { + context.sslContextManager().secretManager().registerTlsCertificateConfigCallbacks(hash, name, + *tsf.get()); + } + return std::move(tsf); } ProtobufTypes::MessagePtr UpstreamSslSocketFactory::createEmptyConfigProto() { @@ -35,12 +45,26 @@ static Registry::RegisterFactory& server_names) { - return std::make_unique( - Ssl::ServerContextConfigImpl( + std::unique_ptr downstream_config = + std::make_unique( MessageUtil::downcastAndValidate( message), - context.secretManager()), - context.sslContextManager(), context.statsScope(), server_names); + context.sslContextManager().secretManager()); + + auto hash = downstream_config->sdsConfigShourceHash(); + auto name = downstream_config->sdsSecretName(); + + std::unique_ptr tsf = std::make_unique( + std::move(downstream_config), context.sslContextManager(), context.statsScope(), + server_names); + + if (!hash.empty()) { + context.sslContextManager().secretManager().registerTlsCertificateConfigCallbacks(hash, name, + *tsf.get()); + } + + return std::move(tsf); +} } ProtobufTypes::MessagePtr DownstreamSslSocketFactory::createEmptyConfigProto() { diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index b149b295174e0..5ebae7a00b0a8 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -79,7 +79,7 @@ void ValidationInstance::initialize(Options& options, thread_local_.registerThread(*dispatcher_, true); runtime_loader_ = component_factory.createRuntime(*this, initial_config); secret_manager_.reset(new Secret::SecretManagerImpl(*this)); - ssl_context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_loader_)); + ssl_context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_loader_, *secret_manager_)); cluster_manager_factory_.reset(new Upstream::ValidationClusterManagerFactory( runtime(), stats(), threadLocal(), random(), dnsResolver(), sslContextManager(), dispatcher(), localInfo(), *secret_manager_)); diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 854180cd4914d..e2dd241c35fcc 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -196,6 +196,14 @@ void ConnectionHandlerImpl::ActiveListener::newConnection(Network::ConnectionSoc } auto transport_socket = filter_chain->transportSocketFactory().createTransportSocket(); + if (!transport_socket) { + ENVOY_LOG_TO_LOGGER(parent_.logger_, debug, + "closing connection: transport socket was not created yet"); + // TODO(jaebong) update stats + socket->close(); + return; + } + Network::ConnectionPtr new_connection = parent_.dispatcher_.createServerConnection(std::move(socket), std::move(transport_socket)); new_connection->setBufferLimits(config_.perConnectionBufferLimitBytes()); diff --git a/source/server/server.cc b/source/server/server.cc index bc4a445d7a9e1..8db26c286f4e6 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -246,6 +246,9 @@ void InstanceImpl::initialize(Options& options, loadServerFlags(initial_config.flagsPath()); + // Shared storage of secrets from SDS + secret_manager_.reset(new Secret::SecretManagerImpl(*this)); + // Workers get created first so they register for thread local updates. listener_manager_.reset( new ListenerManagerImpl(*this, listener_component_factory_, worker_factory_)); @@ -262,7 +265,7 @@ void InstanceImpl::initialize(Options& options, runtime_loader_ = component_factory.createRuntime(*this, initial_config); // Once we have runtime we can initialize the SSL context manager. - ssl_context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_loader_)); + ssl_context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_loader_, *secret_manager_)); cluster_manager_factory_.reset(new Upstream::ProdClusterManagerFactory( runtime(), stats(), threadLocal(), random(), dnsResolver(), sslContextManager(), dispatcher(), diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index 32759e6925eac..63aa65f7c9785 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -444,10 +444,9 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); } - Ssl::ClientContextConfigImpl cfg(tls_context, server_.secretManager()); - - mock_cluster_info_->transport_socket_factory_ = - std::make_unique(cfg, context_manager_, *stats_store_); + mock_cluster_info_->transport_socket_factory_ = std::make_unique( + std::make_unique(tls_context, server_.secretManager()), + context_manager_, *stats_store_); ON_CALL(*mock_cluster_info_, transportSocketFactory()) .WillByDefault(ReturnRef(*mock_cluster_info_->transport_socket_factory_)); async_client_transport_socket_ = @@ -474,11 +473,10 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); } - Ssl::ServerContextConfigImpl cfg(tls_context, server_.secretManager()); - static Stats::Scope* upstream_stats_store = new Stats::IsolatedStoreImpl(); return std::make_unique( - cfg, context_manager_, *upstream_stats_store, std::vector{}); + std::make_unique(tls_context, server_.secretManager()), + context_manager_, *upstream_stats_store, std::vector{}); } bool use_client_cert_{}; diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index b7f46ff0edb71..98da0e9670554 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -15,6 +15,8 @@ envoy_cc_test( "//test/common/ssl/test_data:certs", ], deps = [ + "//source/common/event:dispatcher_includes", + "//source/common/event:dispatcher_lib", "//source/common/secret:secret_manager_impl_lib", "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 1f46f03565dfa..3f76360d834e2 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -18,6 +18,7 @@ namespace { class MockServer : public Server::MockInstance { public: + Event::Dispatcher& dispatcher() override { return dispatcher_; } Init::Manager& initManager() { return initmanager_; } private: @@ -27,9 +28,15 @@ class MockServer : public Server::MockInstance { void registerTarget(Init::Target&) override {} }; + Event::DispatcherImpl dispatcher_; InitManager initmanager_; }; +class MockSecretCallback : public SecretCallbacks { + public: + MOCK_METHOD0(onAddOrUpdateSecret, void()); +}; + class SecretManagerImplTest : public testing::Test {}; TEST_F(SecretManagerImplTest, SecretLoadSuccess) { @@ -82,11 +89,18 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { MockServer server; + std::unique_ptr secret_callback( + new ::testing::NiceMock()); + EXPECT_CALL(*secret_callback.get(), onAddOrUpdateSecret()); std::string config_source_hash = server.secretManager().addOrUpdateSdsService(config_source, "abc_config"); + server.secretManager().registerTlsCertificateConfigCallbacks(config_source_hash, "abc.com", + *secret_callback.get()); server.secretManager().addOrUpdateSecret(config_source_hash, secret_config); + server.dispatcher().run(Event::Dispatcher::RunType::Block); + ASSERT_EQ(server.secretManager().findTlsCertificate(config_source_hash, "undefined"), nullptr); EXPECT_EQ( diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 4a034a6981f8a..2f18cad44c194 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -82,7 +82,7 @@ TEST_F(SslContextImplTest, TestCipherSuites) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; EXPECT_THROW(manager.createSslClientContext(store, cfg), EnvoyException); } @@ -98,7 +98,7 @@ TEST_F(SslContextImplTest, TestExpiringCert) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; ClientContextPtr context(manager.createSslClientContext(store, cfg)); @@ -121,7 +121,7 @@ TEST_F(SslContextImplTest, TestExpiredCert) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; ClientContextPtr context(manager.createSslClientContext(store, cfg)); EXPECT_EQ(0U, context->daysUntilFirstCertExpires()); @@ -139,7 +139,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; ClientContextPtr context(manager.createSslClientContext(store, cfg)); @@ -165,7 +165,7 @@ TEST_F(SslContextImplTest, TestNoCert) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString("{}"); ClientContextConfigImpl cfg(*loader, server_.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; ClientContextPtr context(manager.createSslClientContext(store, cfg)); EXPECT_EQ("", context->getCaCertInformation()); @@ -177,7 +177,7 @@ class SslServerContextImplTicketTest : public SslContextImplTest { static void loadConfig(ServerContextConfigImpl& cfg) { Runtime::MockLoader runtime; Secret::MockSecretManager secret_manager; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; ServerContextPtr server_ctx( manager.createSslServerContext(store, cfg, std::vector{})); @@ -382,7 +382,7 @@ TEST(ClientContextConfigImplTest, InvalidCertificateHash) { "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ClientContextConfigImpl client_context_config(tls_context, server.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server.secretManager()); Stats::IsolatedStoreImpl store; EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config), EnvoyException, "Invalid hex-encoded SHA-256 .*"); @@ -398,7 +398,7 @@ TEST(ClientContextConfigImplTest, InvalidCertificateSpki) { ->add_verify_certificate_spki("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ClientContextConfigImpl client_context_config(tls_context, server.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server.secretManager()); Stats::IsolatedStoreImpl store; EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config), EnvoyException, "Invalid base64-encoded SHA-256 .*"); @@ -524,7 +524,7 @@ TEST(ServerContextImplTest, TlsCertificateNonEmpty) { tls_context.mutable_common_tls_context()->add_tls_certificates(); ServerContextConfigImpl client_context_config(tls_context, server.secretManager()); Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server.secretManager()); Stats::IsolatedStoreImpl store; EXPECT_THROW_WITH_MESSAGE(ServerContextPtr server_ctx(manager.createSslServerContext( store, client_context_config, std::vector{})), diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 6b172a4030590..c5bbcbdec9c68 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -55,10 +55,10 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Server::MockInstance server; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server.secretManager()); - ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - std::vector{}); + ContextManagerImpl manager(runtime, server.secretManager()); + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(*server_ctx_loader, server.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version), nullptr, @@ -68,8 +68,9 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server.secretManager()); - Ssl::ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); + Ssl::ClientSslSocketFactory client_ssl_socket_factory( + std::make_unique(*client_ctx_loader, server.secretManager()), + manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -146,7 +147,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; Server::MockInstance server; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server.secretManager()); std::string new_session = EMPTY_STRING; // SNI-based selection logic isn't happening in Ssl::SslSocket anymore. @@ -154,10 +155,10 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, const auto& filter_chain = server_proto.filter_chains(0); std::vector server_names(filter_chain.filter_chain_match().server_names().begin(), filter_chain.filter_chain_match().server_names().end()); - Ssl::ServerContextConfigImpl server_ctx_config(filter_chain.tls_context(), - server.secretManager()); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - server_names); + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(filter_chain.tls_context(), + server.secretManager()), + manager, stats_store, server_names); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version), nullptr, @@ -166,9 +167,12 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); - ClientContextConfigImpl client_ctx_config(client_ctx_proto, server.secretManager()); - ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); - ClientContextPtr client_ctx(manager.createSslClientContext(stats_store, client_ctx_config)); + ClientSslSocketFactory client_ssl_socket_factory( + std::make_unique(client_ctx_proto, server.secretManager()), manager, + stats_store); + ClientContextPtr client_ctx(manager.createSslClientContext( + stats_store, + *std::make_unique(client_ctx_proto, server.secretManager()).get())); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -1517,10 +1521,10 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); - ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - std::vector{}); + ContextManagerImpl manager(runtime, server_.secretManager()); + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(*server_ctx_loader, server_.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, @@ -1575,10 +1579,11 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); - ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - std::vector{}); + ContextManagerImpl manager(runtime, server_.secretManager()); + + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(*server_ctx_loader, server_.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, @@ -1596,8 +1601,9 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server_.secretManager()); - ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); + ClientSslSocketFactory client_ssl_socket_factory( + std::make_unique(*client_ctx_loader, server_.secretManager()), + manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -1659,10 +1665,10 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server.secretManager()); - ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - std::vector{}); + ContextManagerImpl manager(runtime, server_.secretManager()); + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(*server_ctx_loader, server_.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, @@ -1679,8 +1685,9 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server.secretManager()); - ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); + ClientSslSocketFactory ssl_socket_factory( + std::make_unique(*client_ctx_loader, server_.secretManager()), + manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(), nullptr); @@ -1737,16 +1744,16 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; Server::MockInstance server; - ContextManagerImpl manager(runtime); + ContextManagerImpl manager(runtime, server.secretManager()); Json::ObjectSharedPtr server_ctx_loader1 = TestEnvironment::jsonLoadFromString(server_ctx_json1); Json::ObjectSharedPtr server_ctx_loader2 = TestEnvironment::jsonLoadFromString(server_ctx_json2); - ServerContextConfigImpl server_ctx_config1(*server_ctx_loader1, server.secretManager()); - ServerContextConfigImpl server_ctx_config2(*server_ctx_loader2, server.secretManager()); - Ssl::ServerSslSocketFactory server_ssl_socket_factory1(server_ctx_config1, manager, stats_store, - server_names1); - Ssl::ServerSslSocketFactory server_ssl_socket_factory2(server_ctx_config2, manager, stats_store, - server_names2); + Ssl::ServerSslSocketFactory server_ssl_socket_factory1( + std::make_unique(*server_ctx_loader1, server.secretManager()), + manager, stats_store, server_names1); + Ssl::ServerSslSocketFactory server_ssl_socket_factory2( + std::make_unique(*server_ctx_loader2, server.secretManager()), + manager, stats_store, server_names2); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket1(Network::Test::getCanonicalLoopbackAddress(ip_version), nullptr, @@ -1759,8 +1766,9 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server.secretManager()); - ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); + ClientSslSocketFactory ssl_socket_factory( + std::make_unique(*client_ctx_loader, server.secretManager()), + manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket1.localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(), nullptr); @@ -2099,14 +2107,14 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); Json::ObjectSharedPtr server2_ctx_loader = TestEnvironment::jsonLoadFromString(server2_ctx_json); - ServerContextConfigImpl server2_ctx_config(*server2_ctx_loader, server_.secretManager()); - ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - std::vector{}); - Ssl::ServerSslSocketFactory server2_ssl_socket_factory(server2_ctx_config, manager, stats_store, - std::vector{}); + ContextManagerImpl manager(runtime, server_.secretManager()); + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(*server_ctx_loader, server_.secretManager()), + manager, stats_store, std::vector{}); + Ssl::ServerSslSocketFactory server2_ssl_socket_factory( + std::make_unique(*server2_ctx_loader, server_.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, @@ -2126,8 +2134,9 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, server_.secretManager()); - ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); + ClientSslSocketFactory ssl_socket_factory( + std::make_unique(*client_ctx_loader, server_.secretManager()), + manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(), nullptr); @@ -2212,10 +2221,10 @@ TEST_P(SslSocketTest, SslError) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, server_.secretManager()); - ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, - std::vector{}); + ContextManagerImpl manager(runtime, server_.secretManager()); + Ssl::ServerSslSocketFactory server_ssl_socket_factory( + std::make_unique(*server_ctx_loader, server_.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, @@ -2534,20 +2543,18 @@ class SslReadBufferLimitTest : public SslCertsTest, public: void initialize() { server_ctx_loader_ = TestEnvironment::jsonLoadFromString(server_ctx_json_); - server_ctx_config_.reset( - new ServerContextConfigImpl(*server_ctx_loader_, server_.secretManager())); - manager_.reset(new ContextManagerImpl(runtime_)); + manager_.reset(new ContextManagerImpl(runtime_, server_.secretManager())); server_ssl_socket_factory_.reset(new ServerSslSocketFactory( - *server_ctx_config_, *manager_, stats_store_, std::vector{})); + std::make_unique(*server_ctx_loader_, server_.secretManager()), + *manager_, stats_store_, std::vector{})); listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); - client_ctx_config_.reset( - new ClientContextConfigImpl(*client_ctx_loader_, server_.secretManager())); - client_ssl_socket_factory_.reset( - new ClientSslSocketFactory(*client_ctx_config_, *manager_, stats_store_)); + client_ssl_socket_factory_.reset(new ClientSslSocketFactory( + std::make_unique(*client_ctx_loader_, server_.secretManager()), + *manager_, stats_store_)); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, client_ssl_socket_factory_->createTransportSocket(), nullptr); @@ -2711,12 +2718,10 @@ class SslReadBufferLimitTest : public SslCertsTest, )EOF"; Runtime::MockLoader runtime_; Json::ObjectSharedPtr server_ctx_loader_; - std::unique_ptr server_ctx_config_; std::unique_ptr manager_; Network::TransportSocketFactoryPtr server_ssl_socket_factory_; Network::ListenerPtr listener_; Json::ObjectSharedPtr client_ctx_loader_; - std::unique_ptr client_ctx_config_; ClientContextPtr client_ctx_; Network::TransportSocketFactoryPtr client_ssl_socket_factory_; Network::ClientConnectionPtr client_connection_; From 3c2ff230a85fe3c4de8560962cfefa46828872cb Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 20 Jun 2018 17:29:39 -0700 Subject: [PATCH 16/23] add secret_callback.h Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_callbacks.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 include/envoy/secret/secret_callbacks.h diff --git a/include/envoy/secret/secret_callbacks.h b/include/envoy/secret/secret_callbacks.h new file mode 100644 index 0000000000000..322f2328d31a9 --- /dev/null +++ b/include/envoy/secret/secret_callbacks.h @@ -0,0 +1,22 @@ +#pragma once + +#include +#include + +#include "envoy/common/pure.h" + +namespace Envoy { +namespace Secret { + +/** + * Callbacks invoked by a secret manager. + */ +class SecretCallbacks { + public: + virtual ~SecretCallbacks() {} + + virtual void onAddOrUpdateSecret() PURE; +}; + +} // namespace Secret +} // namespace Envoy \ No newline at end of file From 3ec032a4f80a6af9650309e6dad1f4c8dd600030 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 20 Jun 2018 17:30:59 -0700 Subject: [PATCH 17/23] fix format. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_callbacks.h | 2 +- include/envoy/secret/secret_manager.h | 4 ++-- source/common/secret/secret_manager_impl.cc | 3 ++- source/common/secret/secret_manager_impl.h | 7 +++--- source/common/ssl/ssl_socket.h | 9 ++++---- .../transport_sockets/ssl/config.cc | 4 ++-- .../common/secret/secret_manager_impl_test.cc | 2 +- test/common/ssl/ssl_socket_test.cc | 22 +++++++++---------- 8 files changed, 27 insertions(+), 26 deletions(-) diff --git a/include/envoy/secret/secret_callbacks.h b/include/envoy/secret/secret_callbacks.h index 322f2328d31a9..671bdc202ad7e 100644 --- a/include/envoy/secret/secret_callbacks.h +++ b/include/envoy/secret/secret_callbacks.h @@ -12,7 +12,7 @@ namespace Secret { * Callbacks invoked by a secret manager. */ class SecretCallbacks { - public: +public: virtual ~SecretCallbacks() {} virtual void onAddOrUpdateSecret() PURE; diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index f119ee7d2fb2a..0fd237bc909b8 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -29,8 +29,8 @@ class SecretManager { * @param name a name of the Ssl::TlsCertificateConfig. * @return the TlsCertificate secret. Returns nullptr if the secret is not found. */ - virtual const Ssl::TlsCertificateConfigSharedPtr findTlsCertificate(const std::string& config_source_hash, - const std::string& name) const PURE; + virtual const Ssl::TlsCertificateConfigSharedPtr + findTlsCertificate(const std::string& config_source_hash, const std::string& name) const PURE; /** * Add or update SDS config source. SecretManager starts downloading secrets from registered diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 07c415dd1d0f0..36dda28098776 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -13,7 +13,8 @@ void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, switch (secret.type_case()) { case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: { std::unique_lock lhs(tls_certificate_secrets_mutex_); - auto tls_certificate_secret = std::make_shared(secret.tls_certificate()); + auto tls_certificate_secret = + std::make_shared(secret.tls_certificate()); tls_certificate_secrets_[config_source_hash][secret.name()] = tls_certificate_secret; if (config_source_hash.empty()) { diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 44fed538c1c7c..2b5fd74cb3827 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -19,8 +19,8 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable> + std::unordered_map> tls_certificate_secrets_; mutable std::shared_timed_mutex tls_certificate_secrets_mutex_; diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index 3b4930ddc4403..30e7931d323b5 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -69,8 +69,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, public Secret::SecretCallbacks, Logger::Loggable { public: - ClientSslSocketFactory(const ClientContextConfigPtr config, - Ssl::ContextManager& manager, Stats::Scope& stats_scope); + ClientSslSocketFactory(const ClientContextConfigPtr config, Ssl::ContextManager& manager, + Stats::Scope& stats_scope); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; @@ -90,9 +90,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, public Secret::SecretCallbacks, Logger::Loggable { public: - ServerSslSocketFactory(const ServerContextConfigPtr config, - Ssl::ContextManager& manager, Stats::Scope& stats_scope, - const std::vector& server_names); + ServerSslSocketFactory(const ServerContextConfigPtr config, Ssl::ContextManager& manager, + Stats::Scope& stats_scope, const std::vector& server_names); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; diff --git a/source/extensions/transport_sockets/ssl/config.cc b/source/extensions/transport_sockets/ssl/config.cc index d3f20da33947f..71851c521f1b3 100644 --- a/source/extensions/transport_sockets/ssl/config.cc +++ b/source/extensions/transport_sockets/ssl/config.cc @@ -65,7 +65,7 @@ Network::TransportSocketFactoryPtr DownstreamSslSocketFactory::createTransportSo return std::move(tsf); } -} +} // namespace SslTransport ProtobufTypes::MessagePtr DownstreamSslSocketFactory::createEmptyConfigProto() { return std::make_unique(); @@ -75,7 +75,7 @@ static Registry::RegisterFactory downstream_registered_; -} // namespace SslTransport } // namespace TransportSockets } // namespace Extensions } // namespace Envoy +} // namespace Envoy diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 3f76360d834e2..424e52fe609d2 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -33,7 +33,7 @@ class MockServer : public Server::MockInstance { }; class MockSecretCallback : public SecretCallbacks { - public: +public: MOCK_METHOD0(onAddOrUpdateSecret, void()); }; diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index c5bbcbdec9c68..9f4a99e84cd06 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -57,8 +57,8 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); ContextManagerImpl manager(runtime, server.secretManager()); Ssl::ServerSslSocketFactory server_ssl_socket_factory( - std::make_unique(*server_ctx_loader, server.secretManager()), - manager, stats_store, std::vector{}); + std::make_unique(*server_ctx_loader, server.secretManager()), + manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version), nullptr, @@ -69,8 +69,8 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); Ssl::ClientSslSocketFactory client_ssl_socket_factory( - std::make_unique(*client_ctx_loader, server.secretManager()), - manager, stats_store); + std::make_unique(*client_ctx_loader, server.secretManager()), + manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -156,9 +156,9 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, std::vector server_names(filter_chain.filter_chain_match().server_names().begin(), filter_chain.filter_chain_match().server_names().end()); Ssl::ServerSslSocketFactory server_ssl_socket_factory( - std::make_unique(filter_chain.tls_context(), - server.secretManager()), - manager, stats_store, server_names); + std::make_unique(filter_chain.tls_context(), + server.secretManager()), + manager, stats_store, server_names); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version), nullptr, @@ -1601,7 +1601,7 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientSslSocketFactory client_ssl_socket_factory( + ClientSslSocketFactory client_ssl_socket_factory( std::make_unique(*client_ctx_loader, server_.secretManager()), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( @@ -1749,10 +1749,10 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Json::ObjectSharedPtr server_ctx_loader1 = TestEnvironment::jsonLoadFromString(server_ctx_json1); Json::ObjectSharedPtr server_ctx_loader2 = TestEnvironment::jsonLoadFromString(server_ctx_json2); Ssl::ServerSslSocketFactory server_ssl_socket_factory1( - std::make_unique(*server_ctx_loader1, server.secretManager()), + std::make_unique(*server_ctx_loader1, server.secretManager()), manager, stats_store, server_names1); Ssl::ServerSslSocketFactory server_ssl_socket_factory2( - std::make_unique(*server_ctx_loader2, server.secretManager()), + std::make_unique(*server_ctx_loader2, server.secretManager()), manager, stats_store, server_names2); Event::DispatcherImpl dispatcher; @@ -1767,7 +1767,7 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); ClientSslSocketFactory ssl_socket_factory( - std::make_unique(*client_ctx_loader, server.secretManager()), + std::make_unique(*client_ctx_loader, server.secretManager()), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket1.localAddress(), Network::Address::InstanceConstSharedPtr(), From 0548c1e0dd5358e05760320f7fb755f9f319f245 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 21 Jun 2018 10:08:40 -0700 Subject: [PATCH 18/23] update tests. Signed-off-by: JimmyCYJ --- test/common/upstream/cluster_manager_impl_test.cc | 3 ++- test/integration/ads_integration_test.cc | 10 +++++----- test/integration/ssl_integration_test.cc | 2 +- test/integration/ssl_utility.cc | 5 +++-- test/integration/tcp_proxy_integration_test.cc | 2 +- test/integration/tcp_proxy_integration_test.h | 2 +- test/integration/xfcc_integration_test.cc | 12 ++++++------ 7 files changed, 19 insertions(+), 17 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 28f7094b0ab08..2c989ac82ba05 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -109,7 +109,8 @@ class TestClusterManagerFactory : public ClusterManagerFactory { new NiceMock}; NiceMock runtime_; NiceMock random_; - Ssl::ContextManagerImpl ssl_context_manager_{runtime_}; + Server::MockInstance server_; + Ssl::ContextManagerImpl ssl_context_manager_{runtime_, server_.secretManager()}; NiceMock dispatcher_; LocalInfo::MockLocalInfo local_info_; Secret::MockSecretManager secret_manager_; diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index cd84b757900ef..2152642adbd8e 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -19,7 +19,7 @@ #include "test/integration/http_integration.h" #include "test/integration/utility.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/network_utility.h" #include "test/test_common/utility.h" @@ -85,11 +85,11 @@ class AdsIntegrationTest : public HttpIntegrationTest, public Grpc::GrpcClientIn TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcert.pem")); tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/upstreamkey.pem")); - Ssl::ServerContextConfigImpl cfg(tls_context, secret_manager_); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( - cfg, context_manager_, *upstream_stats_store, std::vector{}); + std::make_unique(tls_context, server_.secretManager()), + context_manager_, *upstream_stats_store, std::vector{}); } AssertionResult @@ -243,9 +243,9 @@ class AdsIntegrationTest : public HttpIntegrationTest, public Grpc::GrpcClientIn } } - Secret::MockSecretManager secret_manager_; + Server::MockInstance server_; Runtime::MockLoader runtime_; - Ssl::ContextManagerImpl context_manager_{runtime_}; + Ssl::ContextManagerImpl context_manager_{runtime_, server_.secretManager()}; FakeHttpConnectionPtr ads_connection_; FakeStreamPtr ads_stream_; }; diff --git a/test/integration/ssl_integration_test.cc b/test/integration/ssl_integration_test.cc index 22fcf58d46f6b..9f41433c6dcbd 100644 --- a/test/integration/ssl_integration_test.cc +++ b/test/integration/ssl_integration_test.cc @@ -32,7 +32,7 @@ void SslIntegrationTest::initialize() { HttpIntegrationTest::initialize(); runtime_.reset(new NiceMock()); - context_manager_.reset(new ContextManagerImpl(*runtime_)); + context_manager_.reset(new ContextManagerImpl(*runtime_, secret_manager_)); registerTestServerPorts({"http"}); client_ssl_ctx_plain_ = createClientSslTransportSocketFactory(false, false, *context_manager_); diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index 53316b1114aee..49f60b47b5594 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -62,8 +62,9 @@ createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& conte Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); ClientContextConfigImpl cfg(*loader, server.secretManager()); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); - return Network::TransportSocketFactoryPtr{ - new Ssl::ClientSslSocketFactory(cfg, context_manager, *client_stats_store)}; + return Network::TransportSocketFactoryPtr{new Ssl::ClientSslSocketFactory( + std::make_unique(*loader, context_manager.secretManager()), + context_manager, *client_stats_store)}; } Network::Address::InstanceConstSharedPtr getSslAddress(const Network::Address::IpVersion& version, diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index b1333372ef98a..fde955445b2b1 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -347,7 +347,7 @@ void TcpProxySslIntegrationTest::initialize() { config_helper_.addSslConfig(); TcpProxyIntegrationTest::initialize(); - context_manager_.reset(new Ssl::ContextManagerImpl(runtime_)); + context_manager_.reset(new Ssl::ContextManagerImpl(runtime_, server_.secretManager())); payload_reader_.reset(new WaitForPayloadReader(*dispatcher_)); } diff --git a/test/integration/tcp_proxy_integration_test.h b/test/integration/tcp_proxy_integration_test.h index 0532890980ba4..07c58cef2044e 100644 --- a/test/integration/tcp_proxy_integration_test.h +++ b/test/integration/tcp_proxy_integration_test.h @@ -41,7 +41,7 @@ class TcpProxySslIntegrationTest : public TcpProxyIntegrationTest { ConnectionStatusCallbacks connect_callbacks_; MockWatermarkBuffer* client_write_buffer_; std::shared_ptr payload_reader_; - Secret::MockSecretManager secret_manager_; + Server::MockInstance server_; }; } // namespace diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index ed92338dd76f3..530e2e6e8d11d 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -59,10 +59,10 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b target = json_tls; } Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - Ssl::ClientContextConfigImpl cfg(*loader, server_.secretManager()); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); - return Network::TransportSocketFactoryPtr{ - new Ssl::ClientSslSocketFactory(cfg, *context_manager_, *client_stats_store)}; + return Network::TransportSocketFactoryPtr{new Ssl::ClientSslSocketFactory( + std::make_unique(*loader, server_.secretManager()), + *context_manager_, *client_stats_store)}; } Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext() { @@ -74,10 +74,10 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - Ssl::ServerContextConfigImpl cfg(*loader, server_.secretManager()); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( - cfg, *context_manager_, *upstream_stats_store, std::vector{}); + std::make_unique(*loader, server_.secretManager()), + *context_manager_, *upstream_stats_store, std::vector{}); } Network::ClientConnectionPtr XfccIntegrationTest::makeClientConnection() { @@ -123,7 +123,7 @@ void XfccIntegrationTest::initialize() { } runtime_.reset(new NiceMock()); - context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_)); + context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_, server_.secretManager())); client_tls_ssl_ctx_ = createClientSslContext(false); client_mtls_ssl_ctx_ = createClientSslContext(true); HttpIntegrationTest::initialize(); From eace3163a734f8c775a825f49530773ad91318f3 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 21 Jun 2018 17:23:55 -0700 Subject: [PATCH 19/23] Context config change. Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 8 +-- include/envoy/secret/secret_manager.h | 1 + include/envoy/ssl/context.h | 4 ++ include/envoy/ssl/context_config.h | 10 ++++ include/envoy/ssl/context_manager.h | 16 +++--- source/common/secret/secret_manager_impl.cc | 4 +- source/common/ssl/context_config_impl.cc | 5 ++ source/common/ssl/context_config_impl.h | 4 +- source/common/ssl/context_impl.h | 6 +++ source/common/ssl/context_manager_impl.cc | 27 +++++----- source/common/ssl/context_manager_impl.h | 16 +++--- source/common/ssl/ssl_socket.cc | 52 +++++++++++++++---- source/common/ssl/ssl_socket.h | 12 ++--- .../transport_sockets/ssl/config.cc | 7 ++- 14 files changed, 113 insertions(+), 59 deletions(-) diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index 882a8905359dd..bb1eb8f1847a6 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -9,12 +9,8 @@ load( envoy_package() envoy_cc_library( - name = "secret_manager_interface", - hdrs = ["secret_manager.h"], - deps = [ - "//include/envoy/ssl:tls_certificate_config_interface", - "@envoy_api//envoy/api/v2/auth:cert_cc", - ], + name = "secret_callbacks_interface", + hdrs = ["secret_callbacks.h"], ) envoy_cc_library( diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 0fd237bc909b8..cd11dc32f8512 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -3,6 +3,7 @@ #include #include "envoy/api/v2/auth/cert.pb.h" +#include "envoy/secret/secret_callbacks.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { diff --git a/include/envoy/ssl/context.h b/include/envoy/ssl/context.h index 5af2fa804fb8a..a01b0fd47c05b 100644 --- a/include/envoy/ssl/context.h +++ b/include/envoy/ssl/context.h @@ -33,11 +33,15 @@ class Context { virtual std::string getCertChainInformation() const PURE; }; +typedef std::shared_ptr ContextSharedPtr; + class ClientContext : public virtual Context {}; typedef std::unique_ptr ClientContextPtr; +typedef std::shared_ptr ClientContextSharedPtr; class ServerContext : public virtual Context {}; typedef std::unique_ptr ServerContextPtr; +typedef std::shared_ptr ServerContextSharedPtr; } // namespace Ssl } // namespace Envoy diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 02b53dd4dce3a..54e34a68fc0a3 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -122,6 +122,12 @@ class ContextConfig { * @return The secret name in SdsSecretConfig. If SdsSecretConfig is empty, returns empty string. */ virtual const std::string& sdsSecretName() const PURE; + + /** + * @return true if dynamic secret is required per config, and is not + * downloaded from SDS server yet; and false otherwise. + */ + virtual bool waitForSdsSecret() const PURE; }; class ClientContextConfig : public virtual ContextConfig { @@ -138,6 +144,8 @@ class ClientContextConfig : public virtual ContextConfig { virtual bool allowRenegotiation() const PURE; }; +typedef std::unique_ptr ClientContextConfigPtr; + class ServerContextConfig : public virtual ContextConfig { public: struct SessionTicketKey { @@ -159,5 +167,7 @@ class ServerContextConfig : public virtual ContextConfig { virtual const std::vector& sessionTicketKeys() const PURE; }; +typedef std::unique_ptr ServerContextConfigPtr; + } // namespace Ssl } // namespace Envoy diff --git a/include/envoy/ssl/context_manager.h b/include/envoy/ssl/context_manager.h index 58a0a95cac4b8..3bdd5b2694ed5 100644 --- a/include/envoy/ssl/context_manager.h +++ b/include/envoy/ssl/context_manager.h @@ -20,8 +20,8 @@ class ContextManager { /** * Builds a ClientContext from a ClientContextConfig. */ - virtual ClientContextPtr createSslClientContext(Stats::Scope& scope, - const ClientContextConfig& config) PURE; + virtual ClientContextSharedPtr createSslClientContext(Stats::Scope& scope, + const ClientContextConfig& config) PURE; /** * Updates ClientContext and returns updated ClientContext. @@ -31,14 +31,14 @@ class ContextManager { * @param config supplies the configuration for ClientContext. * @return an updated ClientContext. */ - virtual ClientContextPtr updateSslClientContext(const ClientContextPtr& context, - Stats::Scope& scope, - const ClientContextConfig& config) PURE; + virtual ClientContextSharedPtr updateSslClientContext(const ClientContextSharedPtr context, + Stats::Scope& scope, + const ClientContextConfig& config) PURE; /** * Builds a ServerContext from a ServerContextConfig. */ - virtual ServerContextPtr + virtual ServerContextSharedPtr createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) PURE; @@ -51,8 +51,8 @@ class ContextManager { * @param server_names server names. * @return an updated ServerContext. */ - virtual ServerContextPtr - updateSslServerContext(const ServerContextPtr& context, Stats::Scope& scope, + virtual ServerContextSharedPtr + updateSslServerContext(const ServerContextSharedPtr context, Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) PURE; diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 36dda28098776..bfd58e87cde40 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -22,7 +22,7 @@ void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, } std::string secret_name = secret.name(); - server_.dispatcher().post([this, config_source_hash, secret_name, secret]() { + server_.dispatcher().post([this, config_source_hash, secret_name, tls_certificate_secret]() { std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash); if (config_source_it != tls_certificate_secret_update_callbacks_.end()) { @@ -33,7 +33,7 @@ void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, for (auto& callback : callback_it->second.second) { callback->onAddOrUpdateSecret(); } - callback_it->second.first = secret; + callback_it->second.first = tls_certificate_secret; } } } diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index f3fd055903306..5311ae7a46580 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -190,6 +190,11 @@ const std::string& ContextConfigImpl::privateKey() const { return secret->privateKey(); } +bool ContextConfigImpl::waitForSdsSecret() const { + return !sds_config_source_hash_.empty() && !sds_secret_name_.empty() && certChain().empty() && + privateKey().empty(); +} + ClientContextConfigImpl::ClientContextConfigImpl( const envoy::api::v2::auth::UpstreamTlsContext& config, Secret::SecretManager& secret_manager) : ContextConfigImpl(config.common_tls_context(), secret_manager), diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index ddaeef9aa519d..0ad991450e8f2 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -54,9 +54,11 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { unsigned minProtocolVersion() const override { return min_protocol_version_; }; unsigned maxProtocolVersion() const override { return max_protocol_version_; }; - const std::string& sdsConfigShourceHash() const override { return sds_config_source_hash_; } + const std::string& sdsConfigSourceHash() const override { return sds_config_source_hash_; } const std::string& sdsSecretName() const override { return sds_secret_name_; } + bool waitForSdsSecret() const override; + protected: ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager); diff --git a/source/common/ssl/context_impl.h b/source/common/ssl/context_impl.h index 4001ff23d9719..0034e1860d9c2 100644 --- a/source/common/ssl/context_impl.h +++ b/source/common/ssl/context_impl.h @@ -136,6 +136,8 @@ class ContextImpl : public virtual Context { std::string cert_chain_file_path_; }; +typedef std::shared_ptr ContextImplSharedPtr; + class ClientContextImpl : public ContextImpl, public ClientContext { public: ClientContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, @@ -148,6 +150,8 @@ class ClientContextImpl : public ContextImpl, public ClientContext { const bool allow_renegotiation_; }; +typedef std::shared_ptr ClientContextImplSharedPtr; + class ServerContextImpl : public ContextImpl, public ServerContext { public: ServerContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, @@ -165,5 +169,7 @@ class ServerContextImpl : public ContextImpl, public ServerContext { const std::vector session_ticket_keys_; }; +typedef std::shared_ptr ServerContextImplSharedPtr; + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/context_manager_impl.cc b/source/common/ssl/context_manager_impl.cc index eb07512ce5385..ba7d5fe8182d4 100644 --- a/source/common/ssl/context_manager_impl.cc +++ b/source/common/ssl/context_manager_impl.cc @@ -20,10 +20,9 @@ void ContextManagerImpl::releaseContext(Context* context) { contexts_.erase(context); } -ClientContextPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, - const ClientContextConfig& config) { - if (!config.sdsConfigShourceHash().empty() && !config.sdsSecretName().empty() && - config.certChain().empty() && config.privateKey().empty()) { +Ssl::ClientContextSharedPtr +ContextManagerImpl::createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config) { + if (config.waitForSdsSecret()) { return nullptr; } @@ -33,8 +32,8 @@ ClientContextPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, return context; } -Ssl::ClientContextPtr -ContextManagerImpl::updateSslClientContext(const Ssl::ClientContextPtr& client_context, +Ssl::ClientContextSharedPtr +ContextManagerImpl::updateSslClientContext(const Ssl::ClientContextSharedPtr client_context, Stats::Scope& scope, const ClientContextConfig& config) { std::unique_lock lock(contexts_lock_); @@ -42,28 +41,29 @@ ContextManagerImpl::updateSslClientContext(const Ssl::ClientContextPtr& client_c return nullptr; } - ClientContextPtr context(new ClientContextImpl(*this, scope, config)); + ClientContextSharedPtr context(new ClientContextImpl(*this, scope, config)); contexts_.emplace(context.get()); return context; } -ServerContextPtr +Ssl::ServerContextSharedPtr ContextManagerImpl::createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) { - if (!config.sdsConfigShourceHash().empty() && !config.sdsSecretName().empty() && + if (!config.sdsConfigSourceHash().empty() && !config.sdsSecretName().empty() && config.certChain().empty() && config.privateKey().empty()) { return nullptr; } - ServerContextPtr context(new ServerContextImpl(*this, scope, config, server_names, runtime_)); + ServerContextSharedPtr context( + new ServerContextImpl(*this, scope, config, server_names, runtime_)); std::unique_lock lock(contexts_lock_); contexts_.emplace(context.get()); return context; } -ServerContextPtr -ContextManagerImpl::updateSslServerContext(const ServerContextPtr& server_context, +Ssl::ServerContextSharedPtr +ContextManagerImpl::updateSslServerContext(const ServerContextSharedPtr server_context, Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) { std::unique_lock lock(contexts_lock_); @@ -72,7 +72,8 @@ ContextManagerImpl::updateSslServerContext(const ServerContextPtr& server_contex return nullptr; } - ServerContextPtr context(new ServerContextImpl(*this, scope, config, server_names, runtime_)); + ServerContextSharedPtr context( + new ServerContextImpl(*this, scope, config, server_names, runtime_)); contexts_.emplace(context.get()); return context; diff --git a/source/common/ssl/context_manager_impl.h b/source/common/ssl/context_manager_impl.h index 7aff474dfa529..d16c20962b2b9 100644 --- a/source/common/ssl/context_manager_impl.h +++ b/source/common/ssl/context_manager_impl.h @@ -32,17 +32,17 @@ class ContextManagerImpl final : public ContextManager { void releaseContext(Context* context); // Ssl::ContextManager - Ssl::ClientContextPtr createSslClientContext(Stats::Scope& scope, - const ClientContextConfig& config) override; - Ssl::ClientContextPtr updateSslClientContext(const Ssl::ClientContextPtr& context, - Stats::Scope& scope, - const ClientContextConfig& config) override; + Ssl::ClientContextSharedPtr createSslClientContext(Stats::Scope& scope, + const ClientContextConfig& config) override; + Ssl::ClientContextSharedPtr updateSslClientContext(const Ssl::ClientContextSharedPtr context, + Stats::Scope& scope, + const ClientContextConfig& config) override; - Ssl::ServerContextPtr + Ssl::ServerContextSharedPtr createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) override; - virtual ServerContextPtr - updateSslServerContext(const ServerContextPtr& context, Stats::Scope& scope, + virtual Ssl::ServerContextSharedPtr + updateSslServerContext(const Ssl::ServerContextSharedPtr context, Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) override; diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index b433358f16532..0105f686aa2e6 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -1,5 +1,7 @@ #include "common/ssl/ssl_socket.h" +#include + #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/hex.h" @@ -14,8 +16,8 @@ using Envoy::Network::PostIoAction; namespace Envoy { namespace Ssl { -SslSocket::SslSocket(Context& ctx, InitialState state) - : ctx_(dynamic_cast(ctx)), ssl_(ctx_.newSsl()) { +SslSocket::SslSocket(ContextImplSharedPtr ctx, InitialState state) + : ctx_(ctx), ssl_(ctx_->newSsl()) { if (state == InitialState::Client) { SSL_set_connect_state(ssl_.get()); } else { @@ -98,7 +100,7 @@ PostIoAction SslSocket::doHandshake() { if (rc == 1) { ENVOY_CONN_LOG(debug, "handshake complete", callbacks_->connection()); handshake_complete_ = true; - ctx_.logHandshake(ssl_.get()); + ctx_->logHandshake(ssl_.get()); callbacks_->raiseEvent(Network::ConnectionEvent::Connected); // It's possible that we closed during the handshake callback. @@ -125,7 +127,7 @@ void SslSocket::drainErrorQueue() { while (uint64_t err = ERR_get_error()) { if (ERR_GET_LIB(err) == ERR_LIB_SSL) { if (ERR_GET_REASON(err) == SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE) { - ctx_.stats().fail_verify_no_cert_.inc(); + ctx_->stats().fail_verify_no_cert_.inc(); saw_counted_error = true; } else if (ERR_GET_REASON(err) == SSL_R_CERTIFICATE_VERIFY_FAILED) { saw_counted_error = true; @@ -138,7 +140,7 @@ void SslSocket::drainErrorQueue() { ERR_reason_error_string(err)); } if (saw_error && !saw_counted_error) { - ctx_.stats().connection_error_.inc(); + ctx_->stats().connection_error_.inc(); } } @@ -373,28 +375,56 @@ std::string SslSocket::subjectLocalCertificate() const { return getSubjectFromCertificate(cert); } -ClientSslSocketFactory::ClientSslSocketFactory(const ClientContextConfig& config, +ClientSslSocketFactory::ClientSslSocketFactory(std::unique_ptr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope) - : ssl_ctx_(manager.createSslClientContext(stats_scope, config)) {} + : ssl_ctx_(manager.createSslClientContext(stats_scope, *config.get())), + config_(std::move(config)), manager_(manager), stats_scope_(stats_scope) {} Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { - return std::make_unique(*ssl_ctx_, Ssl::InitialState::Client); + return ssl_ctx_ ? std::make_unique( + std::dynamic_pointer_cast(ssl_ctx_), Ssl::InitialState::Client) + : nullptr; +} + +void ClientSslSocketFactory::onAddOrUpdateSecret() { + if (ssl_ctx_) { + ENVOY_LOG(debug, "cluster socket updated"); + ssl_ctx_ = manager_.updateSslClientContext(ssl_ctx_, stats_scope_, *config_.get()); + } else { + ENVOY_LOG(debug, "cluster socket initialized"); + ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_.get()); + } } bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } -ServerSslSocketFactory::ServerSslSocketFactory(const ServerContextConfig& config, +ServerSslSocketFactory::ServerSslSocketFactory(std::unique_ptr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names) - : ssl_ctx_(manager.createSslServerContext(stats_scope, config, server_names)) {} + : ssl_ctx_(manager.createSslServerContext(stats_scope, *config.get(), server_names)), + config_(std::move(config)), manager_(manager), stats_scope_(stats_scope), + server_names_(server_names) {} Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { - return std::make_unique(*ssl_ctx_, Ssl::InitialState::Server); + return ssl_ctx_ ? std::make_unique( + std::dynamic_pointer_cast(ssl_ctx_), Ssl::InitialState::Server) + : nullptr; } bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } +void ServerSslSocketFactory::onAddOrUpdateSecret() { + if (ssl_ctx_) { + ENVOY_LOG(debug, "listener socket updated"); + ssl_ctx_ = + manager_.updateSslServerContext(ssl_ctx_, stats_scope_, *config_.get(), server_names_); + } else { + ENVOY_LOG(debug, "listener socket initialized"); + ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_.get(), server_names_); + } +} + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index 30e7931d323b5..ff12e32971766 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -21,7 +21,7 @@ class SslSocket : public Network::TransportSocket, public Connection, protected Logger::Loggable { public: - SslSocket(Context& ctx, InitialState state); + SslSocket(ContextImplSharedPtr ctx, InitialState state); // Ssl::Connection bool peerCertificatePresented() const override; @@ -56,7 +56,7 @@ class SslSocket : public Network::TransportSocket, std::vector getDnsSansFromCertificate(X509* cert); Network::TransportSocketCallbacks* callbacks_{}; - ContextImpl& ctx_; + ContextImplSharedPtr ctx_; bssl::UniquePtr ssl_; bool handshake_complete_{}; bool shutdown_sent_{}; @@ -69,7 +69,7 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, public Secret::SecretCallbacks, Logger::Loggable { public: - ClientSslSocketFactory(const ClientContextConfigPtr config, Ssl::ContextManager& manager, + ClientSslSocketFactory(std::unique_ptr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; @@ -78,7 +78,7 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, void onAddOrUpdateSecret() override; private: - ClientContextPtr ssl_ctx_; + ClientContextSharedPtr ssl_ctx_; ClientContextConfigPtr config_; Ssl::ContextManager& manager_; Stats::Scope& stats_scope_; @@ -90,7 +90,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, public Secret::SecretCallbacks, Logger::Loggable { public: - ServerSslSocketFactory(const ServerContextConfigPtr config, Ssl::ContextManager& manager, + ServerSslSocketFactory(std::unique_ptr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; @@ -99,7 +99,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, void onAddOrUpdateSecret() override; private: - ServerContextPtr ssl_ctx_; + ServerContextSharedPtr ssl_ctx_; ServerContextConfigPtr config_; Ssl::ContextManager& manager_; Stats::Scope& stats_scope_; diff --git a/source/extensions/transport_sockets/ssl/config.cc b/source/extensions/transport_sockets/ssl/config.cc index 71851c521f1b3..4755bfd5dfc51 100644 --- a/source/extensions/transport_sockets/ssl/config.cc +++ b/source/extensions/transport_sockets/ssl/config.cc @@ -22,7 +22,7 @@ Network::TransportSocketFactoryPtr UpstreamSslSocketFactory::createTransportSock message), context.secretManager()); - auto hash = upstream_config->sdsConfigShourceHash(); + auto hash = upstream_config->sdsConfigSourceHash(); auto name = upstream_config->sdsSecretName(); std::unique_ptr tsf = std::make_unique( @@ -51,7 +51,7 @@ Network::TransportSocketFactoryPtr DownstreamSslSocketFactory::createTransportSo message), context.sslContextManager().secretManager()); - auto hash = downstream_config->sdsConfigShourceHash(); + auto hash = downstream_config->sdsConfigSourceHash(); auto name = downstream_config->sdsSecretName(); std::unique_ptr tsf = std::make_unique( @@ -65,7 +65,6 @@ Network::TransportSocketFactoryPtr DownstreamSslSocketFactory::createTransportSo return std::move(tsf); } -} // namespace SslTransport ProtobufTypes::MessagePtr DownstreamSslSocketFactory::createEmptyConfigProto() { return std::make_unique(); @@ -75,7 +74,7 @@ static Registry::RegisterFactory downstream_registered_; +} // namespace SslTransport } // namespace TransportSockets } // namespace Extensions } // namespace Envoy -} // namespace Envoy From 9f8966d9ca829fc5dfcd45e319eee43ec657fc0c Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 21 Jun 2018 18:41:09 -0700 Subject: [PATCH 20/23] support unregistering callback from secret manager. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 14 ++++++-- include/envoy/ssl/tls_certificate_config.h | 2 +- source/common/secret/secret_manager_impl.cc | 35 ++++++++++++++++--- source/common/secret/secret_manager_impl.h | 13 ++++--- source/common/ssl/ssl_socket.cc | 10 ++++++ source/common/ssl/ssl_socket.h | 4 +++ .../common/secret/secret_manager_impl_test.cc | 21 +++++------ test/mocks/ssl/mocks.h | 13 +++---- 8 files changed, 83 insertions(+), 29 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index cd11dc32f8512..75b7d3e208622 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -30,8 +30,8 @@ class SecretManager { * @param name a name of the Ssl::TlsCertificateConfig. * @return the TlsCertificate secret. Returns nullptr if the secret is not found. */ - virtual const Ssl::TlsCertificateConfigSharedPtr - findTlsCertificate(const std::string& config_source_hash, const std::string& name) const PURE; + virtual const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& config_source_hash, + const std::string& name) const PURE; /** * Add or update SDS config source. SecretManager starts downloading secrets from registered @@ -54,6 +54,16 @@ class SecretManager { virtual void registerTlsCertificateConfigCallbacks(const std::string& config_source_hash, const std::string& secret_name, SecretCallbacks& callback) PURE; + + /** + * Unregister callback function. + * @param config_source_hash Hash code of ConfigSource. + * @param secret_name name of the secret. + * @param callback SecretCallbacks class. + */ + virtual void unRegisterTlsCertificateConfigCallbacks(const std::string& config_source_hash, + const std::string& secret_name, + SecretCallbacks* callback) PURE; }; } // namespace Secret diff --git a/include/envoy/ssl/tls_certificate_config.h b/include/envoy/ssl/tls_certificate_config.h index 437c738a1f76b..069d29f459a09 100644 --- a/include/envoy/ssl/tls_certificate_config.h +++ b/include/envoy/ssl/tls_certificate_config.h @@ -29,7 +29,7 @@ class TlsCertificateConfig { virtual bool equalTo(const TlsCertificateConfig& secret) const PURE; }; -typedef std::shared_ptr TlsCertificateConfigSharedPtr; +typedef std::unique_ptr TlsCertificateConfigPtr; } // namespace Ssl } // namespace Envoy diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index bfd58e87cde40..bf7d9dc5b5e65 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -1,5 +1,7 @@ #include "common/secret/secret_manager_impl.h" +#include + #include "envoy/common/exception.h" #include "common/secret/secret_manager_util.h" @@ -13,14 +15,16 @@ void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, switch (secret.type_case()) { case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: { std::unique_lock lhs(tls_certificate_secrets_mutex_); - auto tls_certificate_secret = - std::make_shared(secret.tls_certificate()); - tls_certificate_secrets_[config_source_hash][secret.name()] = tls_certificate_secret; + tls_certificate_secrets_[config_source_hash][secret.name()] = + std::make_unique(secret.tls_certificate()); + ; if (config_source_hash.empty()) { return; } + const auto tls_certificate_secret = + tls_certificate_secrets_[config_source_hash][secret.name()].get(); std::string secret_name = secret.name(); server_.dispatcher().post([this, config_source_hash, secret_name, tls_certificate_secret]() { std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); @@ -44,7 +48,7 @@ void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash, } } -const Ssl::TlsCertificateConfigSharedPtr +const Ssl::TlsCertificateConfig* SecretManagerImpl::findTlsCertificate(const std::string& config_source_hash, const std::string& name) const { std::shared_lock lhs(tls_certificate_secrets_mutex_); @@ -55,7 +59,7 @@ SecretManagerImpl::findTlsCertificate(const std::string& config_source_hash, } auto secret = config_source_it->second.find(name); - return (secret != config_source_it->second.end()) ? secret->second : nullptr; + return (secret != config_source_it->second.end()) ? secret->second.get() : nullptr; } std::string SecretManagerImpl::addOrUpdateSdsService( @@ -77,6 +81,9 @@ void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& const std::string& secret_name, SecretCallbacks& callback) { auto secret = findTlsCertificate(config_source_hash, secret_name); + if (!secret) { + return; + } std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); @@ -96,5 +103,23 @@ void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& name_it->second.second.push_back(&callback); } +void SecretManagerImpl::unRegisterTlsCertificateConfigCallbacks( + const std::string& config_source_hash, const std::string& secret_name, + SecretCallbacks* callback) { + std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); + + auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash); + if (config_source_it != tls_certificate_secret_update_callbacks_.end()) { + auto name_it = config_source_it->second.find(secret_name); + if (name_it != config_source_it->second.end()) { + auto callback_it = + std::find(name_it->second.second.begin(), name_it->second.second.end(), callback); + if (callback_it != name_it->second.second.end()) { + name_it->second.second.erase(callback_it); + } + } + } +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 2b5fd74cb3827..378baefa5beb0 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -19,8 +19,8 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable> + std::unordered_map> tls_certificate_secrets_; mutable std::shared_timed_mutex tls_certificate_secrets_mutex_; @@ -51,7 +54,7 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable>>> tls_certificate_secret_update_callbacks_; mutable std::shared_timed_mutex tls_certificate_secret_update_callbacks_mutex_; diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 0105f686aa2e6..7cbabd4c174ab 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -387,6 +387,11 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons : nullptr; } +ClientSslSocketFactory::~ClientSslSocketFactory() { + manager_.secretManager().unRegisterTlsCertificateConfigCallbacks(config_->sdsConfigSourceHash(), + config_->sdsSecretName(), this); +} + void ClientSslSocketFactory::onAddOrUpdateSecret() { if (ssl_ctx_) { ENVOY_LOG(debug, "cluster socket updated"); @@ -407,6 +412,11 @@ ServerSslSocketFactory::ServerSslSocketFactory(std::unique_ptrsdsConfigSourceHash(), + config_->sdsSecretName(), this); +} + Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { return ssl_ctx_ ? std::make_unique( std::dynamic_pointer_cast(ssl_ctx_), Ssl::InitialState::Server) diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index ff12e32971766..3d3bc3f3f54e3 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -71,6 +71,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, public: ClientSslSocketFactory(std::unique_ptr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope); + virtual ~ClientSslSocketFactory(); + Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; @@ -92,6 +94,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, public: ServerSslSocketFactory(std::unique_ptr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names); + virtual ~ServerSslSocketFactory(); + Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 424e52fe609d2..8199d1a4bb949 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -62,13 +62,13 @@ name: "abc.com" ASSERT_NE(server.secretManager().findTlsCertificate("", "abc.com"), nullptr); - EXPECT_EQ( - TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), - server.secretManager().findTlsCertificate("", "abc.com")->certificateChain()); + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), + server.secretManager().findTlsCertificate("", "abc.com")->certificateChain()); - EXPECT_EQ( - TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), - server.secretManager().findTlsCertificate("", "abc.com")->privateKey()); + const std::string key_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), + server.secretManager().findTlsCertificate("", "abc.com")->privateKey()); } TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { @@ -103,13 +103,14 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { ASSERT_EQ(server.secretManager().findTlsCertificate(config_source_hash, "undefined"), nullptr); + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; EXPECT_EQ( - TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_cert.pem"), + TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), server.secretManager().findTlsCertificate(config_source_hash, "abc.com")->certificateChain()); - EXPECT_EQ( - TestEnvironment::readFileToStringForTest("test/common/ssl/test_data/selfsigned_key.pem"), - server.secretManager().findTlsCertificate(config_source_hash, "abc.com")->privateKey()); + const std::string key_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), + server.secretManager().findTlsCertificate(config_source_hash, "abc.com")->privateKey()); } TEST_F(SecretManagerImplTest, NotImplementedException) { diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 60691fa397973..c28521fc924a1 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -21,14 +21,15 @@ class MockContextManager : public ContextManager { MockContextManager(); ~MockContextManager(); - ClientContextPtr createSslClientContext(Stats::Scope& scope, - const ClientContextConfig& config) override { - return ClientContextPtr{createSslClientContext_(scope, config)}; + ClientContextSharedPtr createSslClientContext(Stats::Scope& scope, + const ClientContextConfig& config) override { + return ClientContextSharedPtr{createSslClientContext_(scope, config)}; } - ServerContextPtr createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, - const std::vector& server_names) override { - return ServerContextPtr{createSslServerContext_(scope, config, server_names)}; + ServerContextSharedPtr + createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, + const std::vector& server_names) override { + return ServerContextSharedPtr{createSslServerContext_(scope, config, server_names)}; } MOCK_METHOD2(createSslClientContext_, From abe5938d69bcdaa352848ab26c9b342c4712b303 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Fri, 22 Jun 2018 12:11:04 -0700 Subject: [PATCH 21/23] Fix mock class and broken tests. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 2 +- source/common/secret/sds_api.cc | 5 +---- source/common/secret/secret_manager_impl.cc | 8 ++++---- source/common/secret/secret_manager_impl.h | 2 +- .../extensions/transport_sockets/ssl/config.cc | 4 ++-- .../grpc_client_integration_test_harness.h | 2 +- test/common/secret/secret_manager_impl_test.cc | 15 ++++++++------- test/common/ssl/context_impl_test.cc | 18 ++++++++---------- test/common/ssl/ssl_socket_test.cc | 4 ++-- test/integration/tcp_proxy_integration_test.h | 2 +- test/mocks/secret/mocks.h | 8 ++++++++ test/mocks/server/mocks.cc | 3 ++- test/mocks/ssl/mocks.h | 16 ++++++++++++++++ .../config_validation/cluster_manager_test.cc | 2 +- test/server/configuration_impl_test.cc | 13 +++++++------ 15 files changed, 63 insertions(+), 41 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 75b7d3e208622..21149f91057b7 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -53,7 +53,7 @@ class SecretManager { */ virtual void registerTlsCertificateConfigCallbacks(const std::string& config_source_hash, const std::string& secret_name, - SecretCallbacks& callback) PURE; + SecretCallbacks* callback) PURE; /** * Unregister callback function. diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index e30630e438891..6bb0771cf842b 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -23,10 +23,7 @@ void SdsApi::initialize(std::function callback) { sds_config_, server_.localInfo().node(), server_.dispatcher(), server_.clusterManager(), server_.random(), server_.stats(), /* rest_legacy_constructor */ nullptr, "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", - // TODO(jaebong): replace next line with - // "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets" to support streaming - // service - "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets"); + "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets"); Config::Utility::checkLocalInfo("sds", server_.localInfo()); diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index bf7d9dc5b5e65..087d4ae3381ba 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -79,7 +79,7 @@ std::string SecretManagerImpl::addOrUpdateSdsService( void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& config_source_hash, const std::string& secret_name, - SecretCallbacks& callback) { + SecretCallbacks* callback) { auto secret = findTlsCertificate(config_source_hash, secret_name); if (!secret) { return; @@ -90,17 +90,17 @@ void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash); if (config_source_it == tls_certificate_secret_update_callbacks_.end()) { tls_certificate_secret_update_callbacks_[config_source_hash][secret_name] = {secret, - {&callback}}; + {callback}}; return; } auto name_it = config_source_it->second.find(secret_name); if (name_it == config_source_it->second.end()) { - config_source_it->second[secret_name] = {secret, {&callback}}; + config_source_it->second[secret_name] = {secret, {callback}}; return; } - name_it->second.second.push_back(&callback); + name_it->second.second.push_back(callback); } void SecretManagerImpl::unRegisterTlsCertificateConfigCallbacks( diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 378baefa5beb0..469f687bf18e6 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -27,7 +27,7 @@ class SecretManagerImpl : public SecretManager, Logger::LoggabledaysUntilFirstCertExpires()); } @@ -142,7 +142,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; - ClientContextPtr context(manager.createSslClientContext(store, cfg)); + ClientContextSharedPtr context(manager.createSslClientContext(store, cfg)); // This is similar to the hack above, but right now we generate the ca_cert and it expires in 15 // days only in the first second that it's valid. We will partially match for up until Days until // Expiration: 1. @@ -167,7 +167,7 @@ TEST_F(SslContextImplTest, TestNoCert) { Runtime::MockLoader runtime; ContextManagerImpl manager(runtime, server_.secretManager()); Stats::IsolatedStoreImpl store; - ClientContextPtr context(manager.createSslClientContext(store, cfg)); + ClientContextSharedPtr context(manager.createSslClientContext(store, cfg)); EXPECT_EQ("", context->getCaCertInformation()); EXPECT_EQ("", context->getCertChainInformation()); } @@ -176,10 +176,10 @@ class SslServerContextImplTicketTest : public SslContextImplTest { public: static void loadConfig(ServerContextConfigImpl& cfg) { Runtime::MockLoader runtime; - Secret::MockSecretManager secret_manager; - ContextManagerImpl manager(runtime, server_.secretManager()); + Server::MockInstance server; + ContextManagerImpl manager(runtime, server.secretManager()); Stats::IsolatedStoreImpl store; - ServerContextPtr server_ctx( + ServerContextSharedPtr server_ctx( manager.createSslServerContext(store, cfg, std::vector{})); } @@ -193,7 +193,6 @@ class SslServerContextImplTicketTest : public SslContextImplTest { server_cert->mutable_private_key()->set_filename( TestEnvironment::substitute("{{ test_tmpdir }}/unittestkey.pem")); - Secret::MockSecretManager secret_manager; ServerContextConfigImpl server_context_config(cfg, server.secretManager()); loadConfig(server_context_config); } @@ -201,7 +200,6 @@ class SslServerContextImplTicketTest : public SslContextImplTest { static void loadConfigJson(const std::string& json) { Server::MockInstance server; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - Secret::MockSecretManager secret_manager; ServerContextConfigImpl cfg(*loader, server.secretManager()); loadConfig(cfg); } @@ -526,7 +524,7 @@ TEST(ServerContextImplTest, TlsCertificateNonEmpty) { Runtime::MockLoader runtime; ContextManagerImpl manager(runtime, server.secretManager()); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_MESSAGE(ServerContextPtr server_ctx(manager.createSslServerContext( + EXPECT_THROW_WITH_MESSAGE(ServerContextSharedPtr server_ctx(manager.createSslServerContext( store, client_context_config, std::vector{})), EnvoyException, "Server TlsCertificates must have a certificate specified"); diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 9f4a99e84cd06..ed70061472056 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -170,7 +170,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, ClientSslSocketFactory client_ssl_socket_factory( std::make_unique(client_ctx_proto, server.secretManager()), manager, stats_store); - ClientContextPtr client_ctx(manager.createSslClientContext( + ClientContextSharedPtr client_ctx(manager.createSslClientContext( stats_store, *std::make_unique(client_ctx_proto, server.secretManager()).get())); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( @@ -2722,7 +2722,7 @@ class SslReadBufferLimitTest : public SslCertsTest, Network::TransportSocketFactoryPtr server_ssl_socket_factory_; Network::ListenerPtr listener_; Json::ObjectSharedPtr client_ctx_loader_; - ClientContextPtr client_ctx_; + ClientContextSharedPtr client_ctx_; Network::TransportSocketFactoryPtr client_ssl_socket_factory_; Network::ClientConnectionPtr client_connection_; Network::ConnectionPtr server_connection_; diff --git a/test/integration/tcp_proxy_integration_test.h b/test/integration/tcp_proxy_integration_test.h index 07c58cef2044e..742e3c1428040 100644 --- a/test/integration/tcp_proxy_integration_test.h +++ b/test/integration/tcp_proxy_integration_test.h @@ -5,7 +5,7 @@ #include "test/integration/integration.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "gtest/gtest.h" diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index f7fd1233b7bb4..fe35dc4af6d6a 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -22,6 +22,14 @@ class MockSecretManager : public SecretManager { MOCK_METHOD2(addOrUpdateSdsService, std::string(const envoy::api::v2::core::ConfigSource& config_source, std::string config_name)); + + MOCK_METHOD3(registerTlsCertificateConfigCallbacks, + void(const std::string& config_source_hash, const std::string& secret_name, + SecretCallbacks* callback)); + + MOCK_METHOD3(unRegisterTlsCertificateConfigCallbacks, + void(const std::string& config_source_hash, const std::string& secret_name, + SecretCallbacks* callback)); }; } // namespace Secret diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 3ba3e74f7cdc8..469e60539c749 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -107,7 +107,8 @@ MockWorker::MockWorker() { MockWorker::~MockWorker() {} MockInstance::MockInstance() - : secret_manager_(new Secret::SecretManagerImpl(*this)), ssl_context_manager_(runtime_loader_), + : secret_manager_(new Secret::SecretManagerImpl(*this)), + ssl_context_manager_(runtime_loader_, *secret_manager_), singleton_manager_(new Singleton::ManagerImpl()) { ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_)); ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_store_)); diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index c28521fc924a1..e2d821ff76b6b 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -32,6 +32,20 @@ class MockContextManager : public ContextManager { return ServerContextSharedPtr{createSslServerContext_(scope, config, server_names)}; } + ClientContextSharedPtr updateSslClientContext(const ClientContextSharedPtr, Stats::Scope& scope, + const ClientContextConfig& config) override { + return ClientContextSharedPtr{createSslClientContext_(scope, config)}; + } + + ServerContextSharedPtr + updateSslServerContext(const ServerContextSharedPtr, Stats::Scope& scope, + const ServerContextConfig& config, + const std::vector& server_names) override { + return ServerContextSharedPtr{createSslServerContext_(scope, config, server_names)}; + } + + Secret::SecretManager& secretManager() override { return secret_manager_; } + MOCK_METHOD2(createSslClientContext_, ClientContext*(Stats::Scope& scope, const ClientContextConfig& config)); MOCK_METHOD3(createSslServerContext_, @@ -39,6 +53,8 @@ class MockContextManager : public ContextManager { const std::vector& server_names)); MOCK_CONST_METHOD0(daysUntilFirstCertExpires, size_t()); MOCK_METHOD1(iterateContexts, void(std::function callback)); + + testing::NiceMock secret_manager_; }; class MockConnection : public Connection { diff --git a/test/server/config_validation/cluster_manager_test.cc b/test/server/config_validation/cluster_manager_test.cc index cafd2666a4de0..a24a890374594 100644 --- a/test/server/config_validation/cluster_manager_test.cc +++ b/test/server/config_validation/cluster_manager_test.cc @@ -28,7 +28,7 @@ TEST(ValidationClusterManagerTest, MockedMethods) { NiceMock random; Secret::MockSecretManager secret_manager; auto dns_resolver = std::make_shared>(); - Ssl::ContextManagerImpl ssl_context_manager{runtime}; + Ssl::ContextManagerImpl ssl_context_manager{runtime, secret_manager}; NiceMock dispatcher; LocalInfo::MockLocalInfo local_info; NiceMock admin; diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index da492c5ef5f6c..8fad6ba6ade13 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -329,12 +329,12 @@ TEST_F(ConfigurationImplTest, StaticSecretRead) { name: "abc.com" tls_certificate: certificate_chain: - filename: "test/config/integration/certs/cacert.pem" + filename: "{{ test_rundir }}/test/config/integration/certs/cacert.pem" private_key: - filename: "test/config/integration/certs/cakey.pem" + filename: "{{ test_rundir }}/test/config/integration/certs/cakey.pem" )EOF"; - MessageUtil::loadFromYaml(yaml, *secret_config); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config); MainImpl config; config.initialize(bootstrap, server_, cluster_manager_factory_); @@ -343,10 +343,11 @@ TEST_F(ConfigurationImplTest, StaticSecretRead) { ASSERT_NE(secret, nullptr); - EXPECT_EQ(TestEnvironment::readFileToStringForTest("test/config/integration/certs/cacert.pem"), + const std::string cert_pem = "{{ test_rundir }}/test/config/integration/certs/cacert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), secret->certificateChain()); - - EXPECT_EQ(TestEnvironment::readFileToStringForTest("test/config/integration/certs/cakey.pem"), + const std::string key_pem = "{{ test_rundir }}/test/config/integration/certs/cakey.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), secret->privateKey()); } From fd56370df90408f13e7cf91fa4fe336b88707fc4 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Mon, 25 Jun 2018 19:01:25 -0700 Subject: [PATCH 22/23] Fix broken tests. Signed-off-by: JimmyCYJ --- include/envoy/ssl/context_manager.h | 5 +++++ source/common/secret/secret_manager_impl.cc | 3 --- source/common/ssl/context_manager_impl.h | 2 +- source/common/ssl/ssl_socket.cc | 14 ++++++++++---- test/mocks/ssl/mocks.h | 1 + 5 files changed, 17 insertions(+), 8 deletions(-) diff --git a/include/envoy/ssl/context_manager.h b/include/envoy/ssl/context_manager.h index 3bdd5b2694ed5..185040694bab5 100644 --- a/include/envoy/ssl/context_manager.h +++ b/include/envoy/ssl/context_manager.h @@ -66,6 +66,11 @@ class ContextManager { */ virtual void iterateContexts(std::function callback) PURE; + /** + * release context from this manager. + */ + virtual void releaseContext(Context* context) PURE; + /** * @return a SecretManager. */ diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 087d4ae3381ba..dca25bee252bf 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -81,9 +81,6 @@ void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& const std::string& secret_name, SecretCallbacks* callback) { auto secret = findTlsCertificate(config_source_hash, secret_name); - if (!secret) { - return; - } std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); diff --git a/source/common/ssl/context_manager_impl.h b/source/common/ssl/context_manager_impl.h index d16c20962b2b9..8fc6bee820ca6 100644 --- a/source/common/ssl/context_manager_impl.h +++ b/source/common/ssl/context_manager_impl.h @@ -29,7 +29,7 @@ class ContextManagerImpl final : public ContextManager { * admin purposes. When a caller frees a context it will tell us to release it also from the list * of contexts. */ - void releaseContext(Context* context); + void releaseContext(Context* context) override; // Ssl::ContextManager Ssl::ClientContextSharedPtr createSslClientContext(Stats::Scope& scope, diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 7cbabd4c174ab..71529924ae4f8 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -388,8 +388,11 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons } ClientSslSocketFactory::~ClientSslSocketFactory() { - manager_.secretManager().unRegisterTlsCertificateConfigCallbacks(config_->sdsConfigSourceHash(), - config_->sdsSecretName(), this); + manager_.releaseContext(ssl_ctx_.get()); + if (config_) { + manager_.secretManager().unRegisterTlsCertificateConfigCallbacks( + config_->sdsConfigSourceHash(), config_->sdsSecretName(), this); + } } void ClientSslSocketFactory::onAddOrUpdateSecret() { @@ -413,8 +416,11 @@ ServerSslSocketFactory::ServerSslSocketFactory(std::unique_ptrsdsConfigSourceHash(), - config_->sdsSecretName(), this); + manager_.releaseContext(ssl_ctx_.get()); + if (config_) { + manager_.secretManager().unRegisterTlsCertificateConfigCallbacks( + config_->sdsConfigSourceHash(), config_->sdsSecretName(), this); + } } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index e2d821ff76b6b..2e478c016bd0a 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -53,6 +53,7 @@ class MockContextManager : public ContextManager { const std::vector& server_names)); MOCK_CONST_METHOD0(daysUntilFirstCertExpires, size_t()); MOCK_METHOD1(iterateContexts, void(std::function callback)); + MOCK_METHOD1(releaseContext, void(Context* context)); testing::NiceMock secret_manager_; }; From f44182007b694773c282067f86726bef325c5288 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 26 Jun 2018 15:53:40 -0700 Subject: [PATCH 23/23] Fixed broken tests and asan errors. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 2 +- source/common/secret/secret_manager_impl.cc | 4 ++-- source/common/secret/secret_manager_impl.h | 2 +- source/common/ssl/ssl_socket.cc | 4 ++-- source/server/server.h | 2 +- test/integration/ssl_integration_test.cc | 9 +++++++-- test/integration/ssl_integration_test.h | 5 +++-- test/integration/xfcc_integration_test.cc | 7 ++++++- test/integration/xfcc_integration_test.h | 1 + test/mocks/secret/mocks.h | 2 +- 10 files changed, 25 insertions(+), 13 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 21149f91057b7..7e93a0f950e4f 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -63,7 +63,7 @@ class SecretManager { */ virtual void unRegisterTlsCertificateConfigCallbacks(const std::string& config_source_hash, const std::string& secret_name, - SecretCallbacks* callback) PURE; + SecretCallbacks& callback) PURE; }; } // namespace Secret diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index dca25bee252bf..0b3f28e9e86e5 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -102,7 +102,7 @@ void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& void SecretManagerImpl::unRegisterTlsCertificateConfigCallbacks( const std::string& config_source_hash, const std::string& secret_name, - SecretCallbacks* callback) { + SecretCallbacks& callback) { std::unique_lock lhs(tls_certificate_secret_update_callbacks_mutex_); auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash); @@ -110,7 +110,7 @@ void SecretManagerImpl::unRegisterTlsCertificateConfigCallbacks( auto name_it = config_source_it->second.find(secret_name); if (name_it != config_source_it->second.end()) { auto callback_it = - std::find(name_it->second.second.begin(), name_it->second.second.end(), callback); + std::find(name_it->second.second.begin(), name_it->second.second.end(), &callback); if (callback_it != name_it->second.second.end()) { name_it->second.second.erase(callback_it); } diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 469f687bf18e6..3bd6bca291515 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -31,7 +31,7 @@ class SecretManagerImpl : public SecretManager, Logger::LoggablesdsConfigSourceHash(), config_->sdsSecretName(), this); + config_->sdsConfigSourceHash(), config_->sdsSecretName(), *this); } } @@ -419,7 +419,7 @@ ServerSslSocketFactory::~ServerSslSocketFactory() { manager_.releaseContext(ssl_ctx_.get()); if (config_) { manager_.secretManager().unRegisterTlsCertificateConfigCallbacks( - config_->sdsConfigSourceHash(), config_->sdsSecretName(), this); + config_->sdsConfigSourceHash(), config_->sdsSecretName(), *this); } } diff --git a/source/server/server.h b/source/server/server.h index 16f47450e08e9..8984b07364dcd 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -202,8 +202,8 @@ class InstanceImpl : Logger::Loggable, public Instance { std::unique_ptr ssl_context_manager_; ProdListenerComponentFactory listener_component_factory_; ProdWorkerFactory worker_factory_; - std::unique_ptr listener_manager_; std::unique_ptr secret_manager_; + std::unique_ptr listener_manager_; std::unique_ptr config_; Network::DnsResolverSharedPtr dns_resolver_; Event::TimerPtr stat_flush_timer_; diff --git a/test/integration/ssl_integration_test.cc b/test/integration/ssl_integration_test.cc index 9f41433c6dcbd..99051051d7282 100644 --- a/test/integration/ssl_integration_test.cc +++ b/test/integration/ssl_integration_test.cc @@ -32,7 +32,7 @@ void SslIntegrationTest::initialize() { HttpIntegrationTest::initialize(); runtime_.reset(new NiceMock()); - context_manager_.reset(new ContextManagerImpl(*runtime_, secret_manager_)); + context_manager_.reset(new ContextManagerImpl(*runtime_, server_.secretManager())); registerTestServerPorts({"http"}); client_ssl_ctx_plain_ = createClientSslTransportSocketFactory(false, false, *context_manager_); @@ -46,10 +46,15 @@ void SslIntegrationTest::TearDown() { client_ssl_ctx_alpn_.reset(); client_ssl_ctx_san_.reset(); client_ssl_ctx_alpn_san_.reset(); - context_manager_.reset(); runtime_.reset(); } +SslIntegrationTest::~SslIntegrationTest() { + HttpIntegrationTest::cleanupUpstreamAndDownstream(); + codec_client_.reset(); + context_manager_.reset(); +} + Network::ClientConnectionPtr SslIntegrationTest::makeSslClientConnection(bool alpn, bool san) { Network::Address::InstanceConstSharedPtr address = getSslAddress(version_, lookupPort("http")); if (alpn) { diff --git a/test/integration/ssl_integration_test.h b/test/integration/ssl_integration_test.h index 26d21bab70533..6f04dc26c1a41 100644 --- a/test/integration/ssl_integration_test.h +++ b/test/integration/ssl_integration_test.h @@ -6,7 +6,7 @@ #include "test/integration/http_integration.h" #include "test/integration/server.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -20,6 +20,7 @@ class SslIntegrationTest : public HttpIntegrationTest, public testing::TestWithParam { public: SslIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {} + virtual ~SslIntegrationTest(); void initialize() override; @@ -32,7 +33,7 @@ class SslIntegrationTest : public HttpIntegrationTest, private: std::unique_ptr runtime_; std::unique_ptr context_manager_; - Secret::MockSecretManager secret_manager_; + Server::MockInstance server_; Network::TransportSocketFactoryPtr client_ssl_ctx_plain_; Network::TransportSocketFactoryPtr client_ssl_ctx_alpn_; diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index 530e2e6e8d11d..0221db823d69b 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -32,10 +32,15 @@ void XfccIntegrationTest::TearDown() { client_tls_ssl_ctx_.reset(); fake_upstream_connection_.reset(); fake_upstreams_.clear(); - context_manager_.reset(); runtime_.reset(); } +XfccIntegrationTest::~XfccIntegrationTest() { + HttpIntegrationTest::cleanupUpstreamAndDownstream(); + codec_client_.reset(); + context_manager_.reset(); +} + Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(bool mtls) { std::string json_tls = R"EOF( { diff --git a/test/integration/xfcc_integration_test.h b/test/integration/xfcc_integration_test.h index 3d0c81a0ad09d..f3760dbe97ed1 100644 --- a/test/integration/xfcc_integration_test.h +++ b/test/integration/xfcc_integration_test.h @@ -32,6 +32,7 @@ class XfccIntegrationTest : public HttpIntegrationTest, const std::string client_dns_san_ = "DNS=lyft.com;DNS=www.lyft.com"; XfccIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {} + virtual ~XfccIntegrationTest(); void initialize() override; void createUpstreams() override; diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index fe35dc4af6d6a..0a523ae993ea1 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -29,7 +29,7 @@ class MockSecretManager : public SecretManager { MOCK_METHOD3(unRegisterTlsCertificateConfigCallbacks, void(const std::string& config_source_hash, const std::string& secret_name, - SecretCallbacks* callback)); + SecretCallbacks& callback)); }; } // namespace Secret