From 9074af6cadf7e41ff54f5d64c02df3bae54127d1 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Thu, 27 May 2021 21:09:13 +0000 Subject: [PATCH 1/5] Update host's socket factory when metadata is updated. Signed-off-by: Christoph Pakulski --- source/common/upstream/upstream_impl.h | 4 +- test/common/upstream/BUILD | 1 + test/common/upstream/eds_test.cc | 74 +++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 28ff4a7b60218..d0c74949aef4b 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -107,6 +107,8 @@ class HostDescriptionImpl : virtual public HostDescription, void metadata(MetadataConstSharedPtr new_metadata) override { absl::WriterMutexLock lock(&metadata_mutex_); metadata_ = new_metadata; + // Update data members dependent on metadata. + socket_factory_ = resolveTransportSocketFactory(address_, metadata_.get()); } const ClusterInfo& cluster() const override { return *cluster_; } @@ -176,7 +178,7 @@ class HostDescriptionImpl : virtual public HostDescription, Outlier::DetectorHostMonitorPtr outlier_detector_; HealthCheckHostMonitorPtr health_checker_; std::atomic priority_; - Network::TransportSocketFactory& socket_factory_; + std::reference_wrapper socket_factory_; const MonotonicTime creation_time_; }; diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index c4964177d10fb..104397aeb8cfb 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -95,6 +95,7 @@ envoy_cc_test( "//source/common/config:utility_lib", "//source/common/upstream:eds_lib", "//source/extensions/transport_sockets/raw_buffer:config", + "//source/extensions/transport_sockets/tls:config", "//source/server:transport_socket_config_lib", "//test/common/stats:stat_test_utility_lib", "//test/mocks/local_info:local_info_mocks", diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 0a25e8436edab..d863879a676a1 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -57,6 +57,36 @@ class EdsTest : public testing::Test { Cluster::InitializePhase::Secondary); } + // Define a cluster with secure and unsecure (default) transport + // sockets. + void resetClusterWithTransportSockets() { + resetCluster(R"EOF( + name: name + connect_timeout: 0.25s + type: EDS + lb_policy: ROUND_ROBIN + eds_cluster_config: + service_name: fare + eds_config: + api_config_source: + api_type: REST + cluster_names: + - eds + refresh_delay: 1s + transport_socket_matches: + - match: + secure: enabled + name: secure-mode + transport_socket: + name: envoy.transport_sockets.tls + - match: {} + name: default-mode + transport_socket: + name: envoy.transport_sockets.raw_buffer + )EOF", + Cluster::InitializePhase::Secondary); + } + void resetClusterDrainOnHostRemoval() { resetCluster(R"EOF( name: name @@ -118,7 +148,7 @@ class EdsTest : public testing::Test { bool initialized_{}; Stats::TestUtil::TestStore stats_; - Ssl::MockContextManager ssl_context_manager_; + testing::NiceMock ssl_context_manager_; envoy::config::cluster::v3::Cluster eds_cluster_; NiceMock cm_; NiceMock dispatcher_; @@ -453,6 +483,48 @@ TEST_F(EdsTest, EndpointMetadata) { "v2"); } +// Test verifies that updating metadata updates +// data members dependent on metadata values. +// Specifically, it transport socket matcher has changed, +// the transport socket factory should also be updated. +TEST_F(EdsTest, EndpointMetadataWithTransportSocket) { + envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment; + cluster_load_assignment.set_cluster_name("fare"); + resetClusterWithTransportSockets(); + + auto health_checker = std::make_shared(); + EXPECT_CALL(*health_checker, start()); + EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); + cluster_->setHealthChecker(health_checker); + + // Add single endpoint to the cluster. + auto* endpoints = cluster_load_assignment.add_endpoints(); + auto* endpoint = endpoints->add_lb_endpoints(); + + endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address("1.2.3.4"); + endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(80); + + doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); + + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 1); + + // Verify that default transport socket is raw (does not implement secure transport). + ASSERT_FALSE((hosts[0].get())->transportSocketFactory().implementsSecureTransport()); + + // Create metadata with transport socket match pointing to secure mode. + auto metadata = new envoy::config::core::v3::Metadata(); + MetadataConstSharedPtr metadata_sharedptr(metadata); + Envoy::Config::Metadata::mutableMetadataValue( + *metadata, Envoy::Config::MetadataFilters::get().ENVOY_TRANSPORT_SOCKET_MATCH, "secure") + .set_string_value("enabled"); + + // Update metadata. + dynamic_cast(hosts[0].get())->metadata(metadata_sharedptr); + + ASSERT_TRUE((hosts[0].get())->transportSocketFactory().implementsSecureTransport()); +} + // Validate that onConfigUpdate() updates endpoint health status. TEST_F(EdsTest, EndpointHealthStatus) { envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment; From 5d58b92e89de94641b382b505e06debf479fd13d Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Thu, 27 May 2021 21:21:38 +0000 Subject: [PATCH 2/5] Added comment to test file. Signed-off-by: Christoph Pakulski --- test/common/upstream/eds_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index d863879a676a1..f0a46a1cf8fcb 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -522,6 +522,7 @@ TEST_F(EdsTest, EndpointMetadataWithTransportSocket) { // Update metadata. dynamic_cast(hosts[0].get())->metadata(metadata_sharedptr); + // Transport socket factory should point to tls, which implements secure transport. ASSERT_TRUE((hosts[0].get())->transportSocketFactory().implementsSecureTransport()); } From 27cbdd6fba1333a66afcaa108c8bc596a877f287 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Wed, 16 Jun 2021 15:17:11 +0000 Subject: [PATCH 3/5] Moved initialization of transport socket factory outside of mutex protected scope. Signed-off-by: Christoph Pakulski --- source/common/upstream/upstream_impl.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 7839b222fc8e2..cd9ae2544eb16 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -105,10 +105,13 @@ class HostDescriptionImpl : virtual public HostDescription, return metadata_; } void metadata(MetadataConstSharedPtr new_metadata) override { - absl::WriterMutexLock lock(&metadata_mutex_); - metadata_ = new_metadata; - // Update data members dependent on metadata. - socket_factory_ = resolveTransportSocketFactory(address_, metadata_.get()); + auto& new_socket_factory = resolveTransportSocketFactory(address_, new_metadata.get()); + { + absl::WriterMutexLock lock(&metadata_mutex_); + metadata_ = new_metadata; + // Update data members dependent on metadata. + socket_factory_ = new_socket_factory; + } } const ClusterInfo& cluster() const override { return *cluster_; } From 79b4401d933964478e1d4ec26f7b897e0a544a97 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Mon, 21 Jun 2021 19:35:22 +0000 Subject: [PATCH 4/5] Added mutex_ to protect update to HostDescription::socket_factory_. It is possible that metadata_ is updated at the same time when socket_factory_ is used to create a new upstream connection. Signed-off-by: Christoph Pakulski --- source/common/upstream/upstream_impl.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 29a4e77cdfc9c..94a41bbd6fd8c 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -86,6 +86,7 @@ class HostDescriptionImpl : virtual public HostDescription, uint32_t priority, TimeSource& time_source); Network::TransportSocketFactory& transportSocketFactory() const override { + absl::ReaderMutexLock lock(&socket_factory_mutex_); return socket_factory_; } @@ -107,6 +108,9 @@ class HostDescriptionImpl : virtual public HostDescription, { absl::WriterMutexLock lock(&metadata_mutex_); metadata_ = new_metadata; + } + { + absl::WriterMutexLock lock(&socket_factory_mutex_); // Update data members dependent on metadata. socket_factory_ = new_socket_factory; } @@ -183,7 +187,9 @@ class HostDescriptionImpl : virtual public HostDescription, Outlier::DetectorHostMonitorPtr outlier_detector_; HealthCheckHostMonitorPtr health_checker_; std::atomic priority_; - std::reference_wrapper socket_factory_; + mutable absl::Mutex socket_factory_mutex_; + std::reference_wrapper + socket_factory_ ABSL_GUARDED_BY(socket_factory_mutex_); const MonotonicTime creation_time_; }; From 85c52c8c2244ba6e131eb4a3b00d306a55e1b2b1 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Wed, 30 Jun 2021 21:51:46 +0000 Subject: [PATCH 5/5] Use single mutex for Host's metadata and socket_factory. Signed-off-by: Christoph Pakulski --- source/common/upstream/upstream_impl.h | 8 ++------ test/common/upstream/eds_test.cc | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 94a41bbd6fd8c..c1717521bc159 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -86,7 +86,7 @@ class HostDescriptionImpl : virtual public HostDescription, uint32_t priority, TimeSource& time_source); Network::TransportSocketFactory& transportSocketFactory() const override { - absl::ReaderMutexLock lock(&socket_factory_mutex_); + absl::ReaderMutexLock lock(&metadata_mutex_); return socket_factory_; } @@ -108,9 +108,6 @@ class HostDescriptionImpl : virtual public HostDescription, { absl::WriterMutexLock lock(&metadata_mutex_); metadata_ = new_metadata; - } - { - absl::WriterMutexLock lock(&socket_factory_mutex_); // Update data members dependent on metadata. socket_factory_ = new_socket_factory; } @@ -187,9 +184,8 @@ class HostDescriptionImpl : virtual public HostDescription, Outlier::DetectorHostMonitorPtr outlier_detector_; HealthCheckHostMonitorPtr health_checker_; std::atomic priority_; - mutable absl::Mutex socket_factory_mutex_; std::reference_wrapper - socket_factory_ ABSL_GUARDED_BY(socket_factory_mutex_); + socket_factory_ ABSL_GUARDED_BY(metadata_mutex_); const MonotonicTime creation_time_; }; diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 6dc809916118d..ad84f1678ddfa 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -147,7 +147,7 @@ class EdsTest : public testing::Test { bool initialized_{}; Stats::TestUtil::TestStore stats_; - testing::NiceMock ssl_context_manager_; + NiceMock ssl_context_manager_; envoy::config::cluster::v3::Cluster eds_cluster_; NiceMock cm_; NiceMock dispatcher_; @@ -500,29 +500,31 @@ TEST_F(EdsTest, EndpointMetadataWithTransportSocket) { auto* endpoints = cluster_load_assignment.add_endpoints(); auto* endpoint = endpoints->add_lb_endpoints(); - endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address("1.2.3.4"); - endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(80); + auto* socket_address = endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address(); + socket_address->set_address("1.2.3.4"); + socket_address->set_port_value(80); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); - EXPECT_EQ(hosts.size(), 1); + ASSERT_EQ(hosts.size(), 1); + auto* upstream_host = hosts[0].get(); // Verify that default transport socket is raw (does not implement secure transport). - ASSERT_FALSE((hosts[0].get())->transportSocketFactory().implementsSecureTransport()); + EXPECT_FALSE(upstream_host->transportSocketFactory().implementsSecureTransport()); // Create metadata with transport socket match pointing to secure mode. auto metadata = new envoy::config::core::v3::Metadata(); MetadataConstSharedPtr metadata_sharedptr(metadata); - Envoy::Config::Metadata::mutableMetadataValue( - *metadata, Envoy::Config::MetadataFilters::get().ENVOY_TRANSPORT_SOCKET_MATCH, "secure") + Config::Metadata::mutableMetadataValue( + *metadata, Config::MetadataFilters::get().ENVOY_TRANSPORT_SOCKET_MATCH, "secure") .set_string_value("enabled"); // Update metadata. - dynamic_cast(hosts[0].get())->metadata(metadata_sharedptr); + upstream_host->metadata(metadata_sharedptr); // Transport socket factory should point to tls, which implements secure transport. - ASSERT_TRUE((hosts[0].get())->transportSocketFactory().implementsSecureTransport()); + EXPECT_TRUE(upstream_host->transportSocketFactory().implementsSecureTransport()); } // Validate that onConfigUpdate() updates endpoint health status.