Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion source/common/upstream/upstream_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could resolveTransportSocketFactory be moved out of the lock guard?
The referenced Metadata should be immutable and valid.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lambdai I can add a scope around the lock to unlock it before calling resolveTransportSocketFactory. Why do you think it is necessary? Deadlock?

@lambdai lambdai Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply.
It's not necessary but nice to have.
The only reason is performance. resolveTransportSocketFactory doesn't need the lock to protect

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take it or leave it. It's probably not a bottleneck.

auto factory = resolveTransportSocketFactory(address_, new_metadata.get());
{
absl::WriterMutexLock lock(&metadata_mutex_);
metadata_ = new_metadata;
socket_factory_ = factory;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as you suggested. There is small performance improvement to resolve factory outside of mutex protected scope. Thanks!

socket_factory_ = resolveTransportSocketFactory(address_, metadata_.get());
}

const ClusterInfo& cluster() const override { return *cluster_; }
Expand Down Expand Up @@ -176,7 +178,7 @@ class HostDescriptionImpl : virtual public HostDescription,
Outlier::DetectorHostMonitorPtr outlier_detector_;
HealthCheckHostMonitorPtr health_checker_;
std::atomic<uint32_t> priority_;
Network::TransportSocketFactory& socket_factory_;
std::reference_wrapper<Network::TransportSocketFactory> socket_factory_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a ABSL_GUARDED_BY annotation as well? Should this use same mutex as metadata?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation added .
Yeah, after checking the code layout I believe that it is possible that metadata update and creating a new upstream connection (which needs access to transport socket factory) to happen at the same time on different threads. To protect against race conditions, a mutex around socket_factory_ has been added.

const MonotonicTime creation_time_;
};

Expand Down
1 change: 1 addition & 0 deletions test/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
75 changes: 74 additions & 1 deletion test/common/upstream/eds_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -118,7 +148,7 @@ class EdsTest : public testing::Test {

bool initialized_{};
Stats::TestUtil::TestStore stats_;
Ssl::MockContextManager ssl_context_manager_;
testing::NiceMock<Ssl::MockContextManager> ssl_context_manager_;
Comment thread
cpakulski marked this conversation as resolved.
Outdated
envoy::config::cluster::v3::Cluster eds_cluster_;
NiceMock<MockClusterManager> cm_;
NiceMock<Event::MockDispatcher> dispatcher_;
Expand Down Expand Up @@ -453,6 +483,49 @@ 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<MockHealthChecker>();
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");
Comment thread
cpakulski marked this conversation as resolved.
Outdated
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);
Comment thread
cpakulski marked this conversation as resolved.
Outdated

// Verify that default transport socket is raw (does not implement secure transport).
ASSERT_FALSE((hosts[0].get())->transportSocketFactory().implementsSecureTransport());
Comment thread
cpakulski marked this conversation as resolved.
Outdated

// 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(
Comment thread
cpakulski marked this conversation as resolved.
Outdated
*metadata, Envoy::Config::MetadataFilters::get().ENVOY_TRANSPORT_SOCKET_MATCH, "secure")
.set_string_value("enabled");

// Update metadata.
dynamic_cast<HostImpl*>(hosts[0].get())->metadata(metadata_sharedptr);
Comment thread
cpakulski marked this conversation as resolved.
Outdated

// Transport socket factory should point to tls, which implements secure transport.
ASSERT_TRUE((hosts[0].get())->transportSocketFactory().implementsSecureTransport());
Comment thread
cpakulski marked this conversation as resolved.
Outdated
}

// Validate that onConfigUpdate() updates endpoint health status.
TEST_F(EdsTest, EndpointHealthStatus) {
envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;
Expand Down