From 62e7c593748b2ceb3b80a3398e0a937a92edf95d Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Mon, 5 Feb 2024 14:38:59 -0600 Subject: [PATCH 01/25] mobile: Update the singleton instance to never call its destructor (#32174) mobile: Fix the singleton implementation to be thread-safe Signed-off-by: Fredy Wijaya --- mobile/library/common/common/system_helper.cc | 2 +- mobile/library/common/common/system_helper.h | 2 +- mobile/test/common/mocks/common/mocks.h | 15 +++++++++------ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/mobile/library/common/common/system_helper.cc b/mobile/library/common/common/system_helper.cc index c2f11fefcd9e..20e3f5616c3f 100644 --- a/mobile/library/common/common/system_helper.cc +++ b/mobile/library/common/common/system_helper.cc @@ -12,7 +12,7 @@ namespace Envoy { -std::unique_ptr SystemHelper::instance_ = std::make_unique(); +SystemHelper* SystemHelper::instance_ = new DefaultSystemHelper(); SystemHelper& SystemHelper::getInstance() { return *instance_; } diff --git a/mobile/library/common/common/system_helper.h b/mobile/library/common/common/system_helper.h index b558c0e13c03..d18c6537e49a 100644 --- a/mobile/library/common/common/system_helper.h +++ b/mobile/library/common/common/system_helper.h @@ -47,7 +47,7 @@ class SystemHelper { private: friend class test::SystemHelperPeer; - static std::unique_ptr instance_; + static SystemHelper* instance_; }; } // namespace Envoy diff --git a/mobile/test/common/mocks/common/mocks.h b/mobile/test/common/mocks/common/mocks.h index e4b2d174e25f..b1451495aa4d 100644 --- a/mobile/test/common/mocks/common/mocks.h +++ b/mobile/test/common/mocks/common/mocks.h @@ -29,24 +29,27 @@ class SystemHelperPeer { // Handle's `mock_helper()` accessor. static std::unique_ptr replaceSystemHelper() { return std::make_unique(); } - // RAII type for replacing the SystemHelper singleton with a the MockSystemHelper. + // RAII type for replacing the SystemHelper singleton with the MockSystemHelper. // When this object is destroyed, it resets the SystemHelper singleton back // to the previous state. class Handle { public: Handle() { - previous_ = std::make_unique(); - SystemHelper::instance_.swap(previous_); + previous_ = new test::MockSystemHelper(); + std::swap(SystemHelper::instance_, previous_); } - ~Handle() { SystemHelper::instance_ = std::move(previous_); } + ~Handle() { + delete SystemHelper::instance_; + SystemHelper::instance_ = previous_; + } test::MockSystemHelper& mock_helper() { - return *static_cast(SystemHelper::instance_.get()); + return *static_cast(SystemHelper::instance_); } private: - std::unique_ptr previous_; + SystemHelper* previous_; }; }; From 966edc2cb92c95d7df1bf7d3d23b3ac17a21f2cd Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 6 Feb 2024 09:25:55 +0000 Subject: [PATCH 02/25] build(deps): bump node from `0ca0e0a` to `5792ef2` in /examples/shared/node (#32192) build(deps): bump node in /examples/shared/node Bumps node from `0ca0e0a` to `5792ef2`. --- updated-dependencies: - dependency-name: node dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- examples/shared/node/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/shared/node/Dockerfile b/examples/shared/node/Dockerfile index 7cbcf4ddcc81..8035e9b80278 100644 --- a/examples/shared/node/Dockerfile +++ b/examples/shared/node/Dockerfile @@ -1,4 +1,4 @@ -FROM node:21.6-bookworm-slim@sha256:0ca0e0a19f5818ab2b716772d21c751881cd525362cba428df5a51a75520f7d5 as node-base +FROM node:21.6-bookworm-slim@sha256:5792ef22332ab69f8149ca3f1b50519c97e220d8b0d97ca4d424963f34593fc4 as node-base FROM node-base as node-http-auth From 5cf82f5e0687edfa4914dcb6843d63c46facf742 Mon Sep 17 00:00:00 2001 From: code Date: Tue, 6 Feb 2024 22:40:34 +0800 Subject: [PATCH 03/25] generic proxy: only create child span if the spawnUpstreamSpan return true (#32167) Signed-off-by: wbpcode --- contrib/generic_proxy/filters/network/source/router/router.cc | 3 ++- .../generic_proxy/filters/network/test/router/router_test.cc | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/contrib/generic_proxy/filters/network/source/router/router.cc b/contrib/generic_proxy/filters/network/source/router/router.cc index 68886faf8e26..2a7cbd69900b 100644 --- a/contrib/generic_proxy/filters/network/source/router/router.cc +++ b/contrib/generic_proxy/filters/network/source/router/router.cc @@ -286,7 +286,8 @@ UpstreamRequest::UpstreamRequest(RouterFilter& parent, GenericUpstreamSharedPtr expects_response_ = !options.oneWayStream(); // Set tracing config. - if (tracing_config_ = parent_.callbacks_->tracingConfig(); tracing_config_.has_value()) { + tracing_config_ = parent_.callbacks_->tracingConfig(); + if (tracing_config_.has_value() && tracing_config_->spawnUpstreamSpan()) { span_ = parent_.callbacks_->activeSpan().spawnChild( tracing_config_.value().get(), absl::StrCat("router ", parent_.cluster_->observabilityName(), " egress"), diff --git a/contrib/generic_proxy/filters/network/test/router/router_test.cc b/contrib/generic_proxy/filters/network/test/router/router_test.cc index 295567495c06..8d98a4229f85 100644 --- a/contrib/generic_proxy/filters/network/test/router/router_test.cc +++ b/contrib/generic_proxy/filters/network/test/router/router_test.cc @@ -251,6 +251,7 @@ class RouterFilterTest : public testing::TestWithParam { if (with_tracing_) { EXPECT_CALL(mock_filter_callback_, tracingConfig()) .WillOnce(Return(OptRef{tracing_config_})); + EXPECT_CALL(tracing_config_, spawnUpstreamSpan()).WillOnce(Return(true)); EXPECT_CALL(active_span_, spawnChild_(_, "router observability_name egress", _)) .WillOnce(Invoke([this](const Tracing::Config&, const std::string&, SystemTime) { child_span_ = new NiceMock(); From 12e277d548e7e10f59c4715183de152ce9cbc54a Mon Sep 17 00:00:00 2001 From: "Adi (Suissa) Peleg" Date: Tue, 6 Feb 2024 10:22:42 -0500 Subject: [PATCH 04/25] LB: introduce randomization in locality LB scheduler initialization (#32075) Signed-off-by: Adi Suissa-Peleg --- changelogs/current.yaml | 5 + envoy/upstream/upstream.h | 5 +- source/common/runtime/runtime_features.cc | 1 + .../common/upstream/cluster_manager_impl.cc | 8 +- source/common/upstream/cluster_manager_impl.h | 2 +- .../upstream/health_checker_event_logger.h | 1 - .../upstream/health_discovery_service.cc | 8 +- source/common/upstream/upstream_impl.cc | 93 ++++++++------ source/common/upstream/upstream_impl.h | 32 +++-- .../extensions/clusters/aggregate/cluster.cc | 4 +- .../clusters/dynamic_forward_proxy/cluster.cc | 2 +- source/extensions/clusters/eds/eds.cc | 10 +- .../logical_dns/logical_dns_cluster.cc | 2 +- .../original_dst/original_dst_cluster.cc | 6 +- .../clusters/redis/redis_cluster.cc | 2 +- .../clusters/redis/redis_cluster_lb.cc | 6 +- .../clusters/redis/redis_cluster_lb.h | 6 +- .../clusters/static/static_cluster.cc | 4 +- .../clusters/strict_dns/strict_dns_cluster.cc | 2 +- .../subset/subset_lb.cc | 23 ++-- .../subset/subset_lb.h | 17 +-- .../upstream/cluster_manager_impl_test.cc | 50 ++++---- test/common/upstream/edf_scheduler_test.cc | 11 +- .../upstream/load_balancer_benchmark.cc | 4 +- .../upstream/load_balancer_impl_test.cc | 2 +- .../upstream/load_balancer_simulation_test.cc | 6 +- test/common/upstream/upstream_impl_test.cc | 113 +++++++++++++++--- .../zone_aware_load_balancer_fuzz_base.cc | 4 +- .../clusters/aggregate/cluster_test.cc | 24 ++-- .../clusters/aggregate/cluster_update_test.cc | 8 +- .../original_dst/original_dst_cluster_test.cc | 2 +- .../common/benchmark_base_tester.cc | 4 +- .../subset/subset_benchmark.cc | 5 +- .../subset/subset_test.cc | 12 +- .../clusters/custom_static_cluster.cc | 2 +- test/mocks/upstream/priority_set.h | 3 +- 36 files changed, 313 insertions(+), 176 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 31b963fd4a7b..bb1ca8a3677a 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -25,6 +25,11 @@ bug_fixes: - area: tracers change: | use unary RPC calls for OpenTelemetry trace exports, rather than client-side streaming connections. +- area: load balancing + change: | + Added randomization in locality load-balancing initialization. This helps desynchronizing Envoys across + a fleet by randomizing the scheduler starting point. This can be temporarily reverted by setting runtime guard + ``envoy.reloadable_features.edf_lb_locality_scheduler_init_fix`` to false. - area: UDP and TCP tunneling change: | fixed a bug where second HTTP response headers received would cause Envoy to crash in cases where diff --git a/envoy/upstream/upstream.h b/envoy/upstream/upstream.h index 2bb5739e13b2..220fb2314841 100644 --- a/envoy/upstream/upstream.h +++ b/envoy/upstream/upstream.h @@ -596,6 +596,7 @@ class PrioritySet { * @param locality_weights supplies a map from locality to associated weight. * @param hosts_added supplies the hosts added since the last update. * @param hosts_removed supplies the hosts removed since the last update. + * @param seed a random number to initialize the locality load-balancing algorithm. * @param weighted_priority_health if present, overwrites the current weighted_priority_health. * @param overprovisioning_factor if present, overwrites the current overprovisioning_factor. * @param cross_priority_host_map read only cross-priority host map which is created in the main @@ -604,7 +605,7 @@ class PrioritySet { virtual void updateHosts(uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, const HostVector& hosts_removed, - absl::optional weighted_priority_health, + uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor, HostMapConstSharedPtr cross_priority_host_map = nullptr) PURE; @@ -628,7 +629,7 @@ class PrioritySet { virtual void updateHosts(uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, const HostVector& hosts_removed, - absl::optional weighted_priority_health, + uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor) PURE; }; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index d9b21841caa8..593f6857027d 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -39,6 +39,7 @@ RUNTIME_GUARD(envoy_reloadable_features_defer_processing_backedup_streams); RUNTIME_GUARD(envoy_reloadable_features_detect_and_raise_rst_tcp_connection); RUNTIME_GUARD(envoy_reloadable_features_dfp_mixed_scheme); RUNTIME_GUARD(envoy_reloadable_features_dns_cache_set_first_resolve_complete); +RUNTIME_GUARD(envoy_reloadable_features_edf_lb_locality_scheduler_init_fix); RUNTIME_GUARD(envoy_reloadable_features_enable_aws_credentials_file); RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection); RUNTIME_GUARD(envoy_reloadable_features_enable_connect_udp_support); diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index f7aebec36188..4a2a4f076afa 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -1536,13 +1536,13 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::updateHost const std::string& name, uint32_t priority, PrioritySet::UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, absl::optional weighted_priority_health, + const HostVector& hosts_removed, uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor, HostMapConstSharedPtr cross_priority_host_map) { ENVOY_LOG(debug, "membership update for TLS cluster {} added {} removed {}", name, hosts_added.size(), hosts_removed.size()); priority_set_.updateHosts(priority, std::move(update_hosts_params), std::move(locality_weights), - hosts_added, hosts_removed, weighted_priority_health, + hosts_added, hosts_removed, seed, weighted_priority_health, overprovisioning_factor, std::move(cross_priority_host_map)); // If an LB is thread aware, create a new worker local LB on membership changes. if (lb_factory_ != nullptr && lb_factory_->recreateOnHostChange()) { @@ -1833,8 +1833,8 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::updateClusterMembership( const auto& cluster_entry = thread_local_clusters_[name]; cluster_entry->updateHosts(name, priority, std::move(update_hosts_params), std::move(locality_weights), hosts_added, hosts_removed, - weighted_priority_health, overprovisioning_factor, - std::move(cross_priority_host_map)); + parent_.random_.random(), weighted_priority_health, + overprovisioning_factor, std::move(cross_priority_host_map)); } void ClusterManagerImpl::ThreadLocalClusterManagerImpl::onHostHealthFailure( diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 33074b2b1244..0c0a0c857c87 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -601,7 +601,7 @@ class ClusterManagerImpl : public ClusterManager, PrioritySet::UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, const HostVector& hosts_removed, - absl::optional weighted_priority_health, + uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor, HostMapConstSharedPtr cross_priority_host_map); diff --git a/source/common/upstream/health_checker_event_logger.h b/source/common/upstream/health_checker_event_logger.h index a05d3a5f0196..362e644d65c3 100644 --- a/source/common/upstream/health_checker_event_logger.h +++ b/source/common/upstream/health_checker_event_logger.h @@ -28,7 +28,6 @@ class HealthCheckEventLoggerImpl : public HealthCheckEventLogger { HealthCheckEventLoggerImpl(const envoy::config::core::v3::HealthCheck& health_check_config, Server::Configuration::HealthCheckerFactoryContext& context) : time_source_(context.serverFactoryContext().mainThreadDispatcher().timeSource()) { - // TODO(botengyao): Remove the file_ creation here into the file based health check // event sink. In this way you can remove the file_ based code from the createHealthCheckEvent if (!health_check_config.event_log_path().empty() /* deprecated */) { diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 4db99e3361d0..3d6d68789450 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -524,8 +524,9 @@ void HdsCluster::updateHosts( // Update the priority set. hosts_per_locality_ = std::make_shared(std::move(hosts_by_locality), false); - priority_set_.updateHosts(0, HostSetImpl::partitionHosts(hosts_, hosts_per_locality_), {}, - hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + priority_set_.updateHosts( + 0, HostSetImpl::partitionHosts(hosts_, hosts_per_locality_), {}, hosts_added, hosts_removed, + server_context_.api().randomGenerator().random(), absl::nullopt, absl::nullopt); } ClusterSharedPtr HdsCluster::create() { return nullptr; } @@ -575,7 +576,8 @@ void HdsCluster::initialize(std::function callback) { } // Use the ungrouped and grouped hosts lists to retain locality structure in the priority set. priority_set_.updateHosts(0, HostSetImpl::partitionHosts(hosts_, hosts_per_locality_), {}, - *hosts_, {}, absl::nullopt, absl::nullopt); + *hosts_, {}, server_context_.api().randomGenerator().random(), + absl::nullopt, absl::nullopt); initialized_ = true; } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index fa5db9c199ca..66a0d339f83e 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -589,7 +589,7 @@ std::vector HostsPerLocalityImpl::filter( void HostSetImpl::updateHosts(PrioritySet::UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, const HostVector& hosts_removed, - absl::optional weighted_priority_health, + uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor) { if (weighted_priority_health.has_value()) { weighted_priority_health_ = weighted_priority_health.value(); @@ -612,11 +612,11 @@ void HostSetImpl::updateHosts(PrioritySet::UpdateHostsParams&& update_hosts_para rebuildLocalityScheduler(healthy_locality_scheduler_, healthy_locality_entries_, *healthy_hosts_per_locality_, healthy_hosts_->get(), hosts_per_locality_, excluded_hosts_per_locality_, locality_weights_, - overprovisioning_factor_); + overprovisioning_factor_, seed); rebuildLocalityScheduler(degraded_locality_scheduler_, degraded_locality_entries_, *degraded_hosts_per_locality_, degraded_hosts_->get(), hosts_per_locality_, excluded_hosts_per_locality_, locality_weights_, - overprovisioning_factor_); + overprovisioning_factor_, seed); runUpdateCallbacks(hosts_added, hosts_removed); } @@ -627,7 +627,8 @@ void HostSetImpl::rebuildLocalityScheduler( const HostsPerLocality& eligible_hosts_per_locality, const HostVector& eligible_hosts, HostsPerLocalityConstSharedPtr all_hosts_per_locality, HostsPerLocalityConstSharedPtr excluded_hosts_per_locality, - LocalityWeightsConstSharedPtr locality_weights, uint32_t overprovisioning_factor) { + LocalityWeightsConstSharedPtr locality_weights, uint32_t overprovisioning_factor, + uint64_t seed) { // Rebuild the locality scheduler by computing the effective weight of each // locality in this priority. The scheduler is reset by default, and is rebuilt only if we have // locality weights (i.e. using EDS) and there is at least one eligible host in this priority. @@ -646,20 +647,40 @@ void HostSetImpl::rebuildLocalityScheduler( locality_scheduler = nullptr; if (all_hosts_per_locality != nullptr && locality_weights != nullptr && !locality_weights->empty() && !eligible_hosts.empty()) { - locality_scheduler = std::make_unique>(); - locality_entries.clear(); - for (uint32_t i = 0; i < all_hosts_per_locality->get().size(); ++i) { - const double effective_weight = effectiveLocalityWeight( - i, eligible_hosts_per_locality, *excluded_hosts_per_locality, *all_hosts_per_locality, - *locality_weights, overprovisioning_factor); - if (effective_weight > 0) { - locality_entries.emplace_back(std::make_shared(i, effective_weight)); - locality_scheduler->add(effective_weight, locality_entries.back()); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.edf_lb_locality_scheduler_init_fix")) { + locality_entries.clear(); + for (uint32_t i = 0; i < all_hosts_per_locality->get().size(); ++i) { + const double effective_weight = effectiveLocalityWeight( + i, eligible_hosts_per_locality, *excluded_hosts_per_locality, *all_hosts_per_locality, + *locality_weights, overprovisioning_factor); + if (effective_weight > 0) { + locality_entries.emplace_back(std::make_shared(i, effective_weight)); + } + } + // If not all effective weights were zero, create the scheduler. + if (!locality_entries.empty()) { + locality_scheduler = std::make_unique>( + EdfScheduler::createWithPicks( + locality_entries, + [](const LocalityEntry& entry) { return entry.effective_weight_; }, seed)); + } + } else { + locality_scheduler = std::make_unique>(); + locality_entries.clear(); + for (uint32_t i = 0; i < all_hosts_per_locality->get().size(); ++i) { + const double effective_weight = effectiveLocalityWeight( + i, eligible_hosts_per_locality, *excluded_hosts_per_locality, *all_hosts_per_locality, + *locality_weights, overprovisioning_factor); + if (effective_weight > 0) { + locality_entries.emplace_back(std::make_shared(i, effective_weight)); + locality_scheduler->add(effective_weight, locality_entries.back()); + } + } + // If all effective weights were zero, reset the scheduler. + if (locality_scheduler->empty()) { + locality_scheduler = nullptr; } - } - // If all effective weights were zero, reset the scheduler. - if (locality_scheduler->empty()) { - locality_scheduler = nullptr; } } } @@ -771,7 +792,7 @@ PrioritySetImpl::getOrCreateHostSet(uint32_t priority, void PrioritySetImpl::updateHosts(uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, const HostVector& hosts_removed, - absl::optional weighted_priority_health, + uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor, HostMapConstSharedPtr cross_priority_host_map) { // Update cross priority host map first. In this way, when the update callbacks of the priority @@ -784,7 +805,7 @@ void PrioritySetImpl::updateHosts(uint32_t priority, UpdateHostsParams&& update_ getOrCreateHostSet(priority, weighted_priority_health, overprovisioning_factor); static_cast(host_sets_[priority].get()) ->updateHosts(std::move(update_hosts_params), std::move(locality_weights), hosts_added, - hosts_removed, weighted_priority_health, overprovisioning_factor); + hosts_removed, seed, weighted_priority_health, overprovisioning_factor); if (!batch_update_) { runUpdateCallbacks(hosts_added, hosts_removed); @@ -807,7 +828,7 @@ void PrioritySetImpl::batchHostUpdate(BatchUpdateCb& callback) { void PrioritySetImpl::BatchUpdateScope::updateHosts( uint32_t priority, PrioritySet::UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, absl::optional weighted_priority_health, + const HostVector& hosts_removed, uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor) { // We assume that each call updates a different priority. ASSERT(priorities_.find(priority) == priorities_.end()); @@ -822,13 +843,13 @@ void PrioritySetImpl::BatchUpdateScope::updateHosts( } parent_.updateHosts(priority, std::move(update_hosts_params), locality_weights, hosts_added, - hosts_removed, weighted_priority_health, overprovisioning_factor); + hosts_removed, seed, weighted_priority_health, overprovisioning_factor); } void MainPrioritySetImpl::updateHosts(uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, + const HostVector& hosts_removed, uint64_t seed, absl::optional weighted_priority_health, absl::optional overprovisioning_factor, HostMapConstSharedPtr cross_priority_host_map) { @@ -837,7 +858,7 @@ void MainPrioritySetImpl::updateHosts(uint32_t priority, UpdateHostsParams&& upd updateCrossPriorityHostMap(hosts_added, hosts_removed); PrioritySetImpl::updateHosts(priority, std::move(update_hosts_params), locality_weights, - hosts_added, hosts_removed, weighted_priority_health, + hosts_added, hosts_removed, seed, weighted_priority_health, overprovisioning_factor); } @@ -1487,6 +1508,7 @@ ClusterImplBase::ClusterImplBase(const envoy::config::cluster::v3::Cluster& clus init_watcher_("ClusterImplBase", [this]() { onInitDone(); }), runtime_(cluster_context.serverFactoryContext().runtime()), wait_for_warm_on_init_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(cluster, wait_for_warm_on_init, true)), + random_(cluster_context.serverFactoryContext().api().randomGenerator()), time_source_(cluster_context.serverFactoryContext().timeSource()), local_cluster_(cluster_context.clusterManager().localClusterName().value_or("") == cluster.name()), @@ -1781,9 +1803,9 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { HostVectorConstSharedPtr hosts_copy = std::make_shared(host_set->hosts()); HostsPerLocalityConstSharedPtr hosts_per_locality_copy = host_set->hostsPerLocality().clone(); - prioritySet().updateHosts(priority, - HostSetImpl::partitionHosts(hosts_copy, hosts_per_locality_copy), - host_set->localityWeights(), {}, {}, absl::nullopt, absl::nullopt); + prioritySet().updateHosts( + priority, HostSetImpl::partitionHosts(hosts_copy, hosts_per_locality_copy), + host_set->localityWeights(), {}, {}, random_.random(), absl::nullopt, absl::nullopt); } } @@ -2014,8 +2036,10 @@ ClusterInfoImpl::ResourceManagers::load(const envoy::config::cluster::v3::Cluste PriorityStateManager::PriorityStateManager(ClusterImplBase& cluster, const LocalInfo::LocalInfo& local_info, - PrioritySet::HostUpdateCb* update_cb) - : parent_(cluster), local_info_node_(local_info.node()), update_cb_(update_cb) {} + PrioritySet::HostUpdateCb* update_cb, + Random::RandomGenerator& random) + : parent_(cluster), local_info_node_(local_info.node()), update_cb_(update_cb), + random_(random) {} void PriorityStateManager::initializePriorityFor( const envoy::config::endpoint::v3::LocalityLbEndpoints& locality_lb_endpoint) { @@ -2139,13 +2163,14 @@ void PriorityStateManager::updateClusterPrioritySet( if (update_cb_ != nullptr) { update_cb_->updateHosts(priority, HostSetImpl::partitionHosts(hosts, per_locality_shared), std::move(locality_weights), hosts_added.value_or(*hosts), - hosts_removed.value_or({}), weighted_priority_health, - overprovisioning_factor); + hosts_removed.value_or({}), random_.random(), + weighted_priority_health, overprovisioning_factor); } else { - parent_.prioritySet().updateHosts( - priority, HostSetImpl::partitionHosts(hosts, per_locality_shared), - std::move(locality_weights), hosts_added.value_or(*hosts), - hosts_removed.value_or({}), weighted_priority_health, overprovisioning_factor); + parent_.prioritySet().updateHosts(priority, + HostSetImpl::partitionHosts(hosts, per_locality_shared), + std::move(locality_weights), hosts_added.value_or(*hosts), + hosts_removed.value_or({}), random_.random(), + weighted_priority_health, overprovisioning_factor); } } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index c17a70ea1a0d..1ea732bf1686 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -601,7 +601,7 @@ class HostSetImpl : public HostSet { void updateHosts(PrioritySet::UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, + const HostVector& hosts_removed, uint64_t seed, absl::optional weighted_priority_health = absl::nullopt, absl::optional overprovisioning_factor = absl::nullopt); @@ -657,13 +657,18 @@ class HostSetImpl : public HostSet { // @param locality_weights the weighting of each locality. // @param overprovisioning_factor the overprovisioning factor to use when computing the effective // weight of a locality. - static void rebuildLocalityScheduler( - std::unique_ptr>& locality_scheduler, - std::vector>& locality_entries, - const HostsPerLocality& eligible_hosts_per_locality, const HostVector& eligible_hosts, - HostsPerLocalityConstSharedPtr all_hosts_per_locality, - HostsPerLocalityConstSharedPtr excluded_hosts_per_locality, - LocalityWeightsConstSharedPtr locality_weights, uint32_t overprovisioning_factor); + // @param seed a random number of initial picks to "invoke" on the locality scheduler. This + // allows to distribute the load between different localities across worker threads and a fleet + // of Envoys. + static void + rebuildLocalityScheduler(std::unique_ptr>& locality_scheduler, + std::vector>& locality_entries, + const HostsPerLocality& eligible_hosts_per_locality, + const HostVector& eligible_hosts, + HostsPerLocalityConstSharedPtr all_hosts_per_locality, + HostsPerLocalityConstSharedPtr excluded_hosts_per_locality, + LocalityWeightsConstSharedPtr locality_weights, + uint32_t overprovisioning_factor, uint64_t seed); static absl::optional chooseLocality(EdfScheduler* locality_scheduler); @@ -701,7 +706,7 @@ class PrioritySetImpl : public PrioritySet { void updateHosts(uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, + const HostVector& hosts_removed, uint64_t seed, absl::optional weighted_priority_health = absl::nullopt, absl::optional overprovisioning_factor = absl::nullopt, HostMapConstSharedPtr cross_priority_host_map = nullptr) override; @@ -760,7 +765,8 @@ class PrioritySetImpl : public PrioritySet { void updateHosts(uint32_t priority, PrioritySet::UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, absl::optional weighted_priority_health, + const HostVector& hosts_removed, uint64_t seed, + absl::optional weighted_priority_health, absl::optional overprovisioning_factor) override; absl::node_hash_set all_hosts_added_; @@ -781,7 +787,7 @@ class MainPrioritySetImpl : public PrioritySetImpl, public Logger::Loggable weighted_priority_health = absl::nullopt, absl::optional overprovisioning_factor = absl::nullopt, HostMapConstSharedPtr cross_priority_host_map = nullptr) override; @@ -1279,6 +1285,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable; class PriorityStateManager : protected Logger::Loggable { public: PriorityStateManager(ClusterImplBase& cluster, const LocalInfo::LocalInfo& local_info, - PrioritySet::HostUpdateCb* update_cb); + PrioritySet::HostUpdateCb* update_cb, Random::RandomGenerator& random); // Initializes the PriorityState vector based on the priority specified in locality_lb_endpoint. void initializePriorityFor( @@ -1348,6 +1355,7 @@ class PriorityStateManager : protected Logger::Loggable { PriorityState priority_state_; const envoy::config::core::v3::Node& local_info_node_; PrioritySet::HostUpdateCb* update_cb_; + Random::RandomGenerator& random_; }; using PriorityStateManagerPtr = std::unique_ptr; diff --git a/source/extensions/clusters/aggregate/cluster.cc b/source/extensions/clusters/aggregate/cluster.cc index c8e0bc8b5dee..a9ff4d5ec426 100644 --- a/source/extensions/clusters/aggregate/cluster.cc +++ b/source/extensions/clusters/aggregate/cluster.cc @@ -86,8 +86,8 @@ AggregateClusterLoadBalancer::linearizePrioritySet(OptRef exc if (!host_set->hosts().empty()) { priority_context->priority_set_.updateHosts( next_priority_after_linearizing, Upstream::HostSetImpl::updateHostsParams(*host_set), - host_set->localityWeights(), host_set->hosts(), {}, host_set->weightedPriorityHealth(), - host_set->overprovisioningFactor()); + host_set->localityWeights(), host_set->hosts(), {}, random_.random(), + host_set->weightedPriorityHealth(), host_set->overprovisioningFactor()); priority_context->priority_to_cluster_.emplace_back( std::make_pair(priority_in_current_cluster, tlc)); diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc index 4dae5d8f6758..b6c69d79e5b6 100644 --- a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc +++ b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc @@ -293,7 +293,7 @@ void Cluster::onDnsHostAddOrUpdate( void Cluster::updatePriorityState(const Upstream::HostVector& hosts_added, const Upstream::HostVector& hosts_removed) { - Upstream::PriorityStateManager priority_state_manager(*this, local_info_, nullptr); + Upstream::PriorityStateManager priority_state_manager(*this, local_info_, nullptr, random_); priority_state_manager.initializePriorityFor(dummy_locality_lb_endpoint_); { absl::ReaderMutexLock lock{&host_map_lock_}; diff --git a/source/extensions/clusters/eds/eds.cc b/source/extensions/clusters/eds/eds.cc index 962b0f249e49..76d0b258514f 100644 --- a/source/extensions/clusters/eds/eds.cc +++ b/source/extensions/clusters/eds/eds.cc @@ -50,7 +50,8 @@ void EdsClusterImpl::startPreInit() { subscription_->start({edsServiceName()}); void EdsClusterImpl::BatchUpdateHelper::batchUpdate(PrioritySet::HostUpdateCb& host_update_cb) { absl::flat_hash_set all_new_hosts; - PriorityStateManager priority_state_manager(parent_, parent_.local_info_, &host_update_cb); + PriorityStateManager priority_state_manager(parent_, parent_.local_info_, &host_update_cb, + parent_.random_); for (const auto& locality_lb_endpoint : cluster_load_assignment_.endpoints()) { parent_.validateEndpointsForZoneAwareRouting(locality_lb_endpoint); @@ -370,9 +371,10 @@ void EdsClusterImpl::reloadHealthyHostsHelper(const HostSharedPtr& host) { HostsPerLocalityConstSharedPtr hosts_per_locality_copy = host_set->hostsPerLocality().filter( {[&host_to_exclude](const Host& host) { return &host != host_to_exclude.get(); }})[0]; - prioritySet().updateHosts( - priority, HostSetImpl::partitionHosts(hosts_copy, hosts_per_locality_copy), - host_set->localityWeights(), {}, hosts_to_remove, absl::nullopt, absl::nullopt); + prioritySet().updateHosts(priority, + HostSetImpl::partitionHosts(hosts_copy, hosts_per_locality_copy), + host_set->localityWeights(), {}, hosts_to_remove, random_.random(), + absl::nullopt, absl::nullopt); } } diff --git a/source/extensions/clusters/logical_dns/logical_dns_cluster.cc b/source/extensions/clusters/logical_dns/logical_dns_cluster.cc index f1e84526b68f..26e7c38d2b78 100644 --- a/source/extensions/clusters/logical_dns/logical_dns_cluster.cc +++ b/source/extensions/clusters/logical_dns/logical_dns_cluster.cc @@ -121,7 +121,7 @@ void LogicalDnsCluster::startResolve() { lbEndpoint(), nullptr, time_source_); const auto& locality_lb_endpoint = localityLbEndpoint(); - PriorityStateManager priority_state_manager(*this, local_info_, nullptr); + PriorityStateManager priority_state_manager(*this, local_info_, nullptr, random_); priority_state_manager.initializePriorityFor(locality_lb_endpoint); priority_state_manager.registerHostForPriority(logical_host_, locality_lb_endpoint); diff --git a/source/extensions/clusters/original_dst/original_dst_cluster.cc b/source/extensions/clusters/original_dst/original_dst_cluster.cc index 2b3313862933..d6e39adbbe99 100644 --- a/source/extensions/clusters/original_dst/original_dst_cluster.cc +++ b/source/extensions/clusters/original_dst/original_dst_cluster.cc @@ -236,9 +236,9 @@ void OriginalDstCluster::addHost(HostSharedPtr& host) { const auto& first_host_set = priority_set_.getOrCreateHostSet(0); HostVectorSharedPtr all_hosts(new HostVector(first_host_set.hosts())); all_hosts->emplace_back(host); - priority_set_.updateHosts(0, - HostSetImpl::partitionHosts(all_hosts, HostsPerLocalityImpl::empty()), - {}, {std::move(host)}, {}, absl::nullopt, absl::nullopt); + priority_set_.updateHosts( + 0, HostSetImpl::partitionHosts(all_hosts, HostsPerLocalityImpl::empty()), {}, + {std::move(host)}, {}, random_.random(), absl::nullopt, absl::nullopt); } void OriginalDstCluster::cleanup() { diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index f49971511be9..9bcf635292b9 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -75,7 +75,7 @@ void RedisCluster::startPreInit() { void RedisCluster::updateAllHosts(const Upstream::HostVector& hosts_added, const Upstream::HostVector& hosts_removed, uint32_t current_priority) { - Upstream::PriorityStateManager priority_state_manager(*this, local_info_, nullptr); + Upstream::PriorityStateManager priority_state_manager(*this, local_info_, nullptr, random_); auto locality_lb_endpoint = localityLbEndpoint(); priority_state_manager.initializePriorityFor(locality_lb_endpoint); diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index 836a4ed55a34..476fda7cd59c 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -56,8 +56,8 @@ bool RedisClusterLoadBalancerFactory::onClusterSlotUpdate(ClusterSlotsSharedPtr& primary_and_replicas->push_back(replica_host->second); } - shard_vector->emplace_back( - std::make_shared(primary_host->second, replicas, primary_and_replicas)); + shard_vector->emplace_back(std::make_shared(primary_host->second, replicas, + primary_and_replicas, random_)); } for (auto i = slot.start(); i <= slot.end(); ++i) { @@ -90,7 +90,7 @@ void RedisClusterLoadBalancerFactory::onHostHealthUpdate() { for (auto const& shard : *current_shard_vector) { shard_vector->emplace_back(std::make_shared( - shard->primary(), shard->replicas().hostsPtr(), shard->allHosts().hostsPtr())); + shard->primary(), shard->replicas().hostsPtr(), shard->allHosts().hostsPtr(), random_)); } { diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h index 66b00e0897a4..222a8bae8dce 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.h +++ b/source/extensions/clusters/redis/redis_cluster_lb.h @@ -155,14 +155,14 @@ class RedisClusterLoadBalancerFactory : public ClusterSlotUpdateCallBack, class RedisShard { public: RedisShard(Upstream::HostConstSharedPtr primary, Upstream::HostVectorConstSharedPtr replicas, - Upstream::HostVectorConstSharedPtr all_hosts) + Upstream::HostVectorConstSharedPtr all_hosts, Random::RandomGenerator& random) : primary_(std::move(primary)) { replicas_.updateHosts(Upstream::HostSetImpl::partitionHosts( std::move(replicas), Upstream::HostsPerLocalityImpl::empty()), - nullptr, {}, {}); + nullptr, {}, {}, random.random()); all_hosts_.updateHosts(Upstream::HostSetImpl::partitionHosts( std::move(all_hosts), Upstream::HostsPerLocalityImpl::empty()), - nullptr, {}, {}); + nullptr, {}, {}, random.random()); } const Upstream::HostConstSharedPtr primary() const { return primary_; } const Upstream::HostSetImpl& replicas() const { return replicas_; } diff --git a/source/extensions/clusters/static/static_cluster.cc b/source/extensions/clusters/static/static_cluster.cc index 8d6a3e85766a..37ee4c968e20 100644 --- a/source/extensions/clusters/static/static_cluster.cc +++ b/source/extensions/clusters/static/static_cluster.cc @@ -10,8 +10,8 @@ namespace Upstream { StaticClusterImpl::StaticClusterImpl(const envoy::config::cluster::v3::Cluster& cluster, ClusterFactoryContext& context) : ClusterImplBase(cluster, context), - priority_state_manager_( - new PriorityStateManager(*this, context.serverFactoryContext().localInfo(), nullptr)) { + priority_state_manager_(new PriorityStateManager( + *this, context.serverFactoryContext().localInfo(), nullptr, random_)) { const envoy::config::endpoint::v3::ClusterLoadAssignment& cluster_load_assignment = cluster.load_assignment(); overprovisioning_factor_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( diff --git a/source/extensions/clusters/strict_dns/strict_dns_cluster.cc b/source/extensions/clusters/strict_dns/strict_dns_cluster.cc index 306f773d13ad..dcb0c10f783d 100644 --- a/source/extensions/clusters/strict_dns/strict_dns_cluster.cc +++ b/source/extensions/clusters/strict_dns/strict_dns_cluster.cc @@ -59,7 +59,7 @@ void StrictDnsClusterImpl::startPreInit() { void StrictDnsClusterImpl::updateAllHosts(const HostVector& hosts_added, const HostVector& hosts_removed, uint32_t current_priority) { - PriorityStateManager priority_state_manager(*this, local_info_, nullptr); + PriorityStateManager priority_state_manager(*this, local_info_, nullptr, random_); // At this point we know that we are different so make a new host list and notify. // // TODO(dio): The uniqueness of a host address resolved in STRICT_DNS cluster per priority is not diff --git a/source/extensions/load_balancing_policies/subset/subset_lb.cc b/source/extensions/load_balancing_policies/subset/subset_lb.cc index e8ae1707703a..9ba717d83e96 100644 --- a/source/extensions/load_balancing_policies/subset/subset_lb.cc +++ b/source/extensions/load_balancing_policies/subset/subset_lb.cc @@ -531,23 +531,25 @@ SubsetLoadBalancer::LbSubsetEntryPtr SubsetLoadBalancer::findSubset( } void SubsetLoadBalancer::updateFallbackSubset(uint32_t priority, const HostVector& all_hosts) { - auto update_func = [priority, &all_hosts](LbSubsetPtr& subset, const HostPredicate& predicate) { + auto update_func = [priority, &all_hosts](LbSubsetPtr& subset, const HostPredicate& predicate, + uint64_t seed) { for (const auto& host : all_hosts) { if (predicate(*host)) { subset->pushHost(priority, host); } } - subset->finalize(priority); + subset->finalize(priority, seed); }; if (subset_any_ != nullptr) { - update_func(subset_any_->lb_subset_, [](const Host&) { return true; }); + update_func( + subset_any_->lb_subset_, [](const Host&) { return true; }, random_.random()); } if (subset_default_ != nullptr) { HostPredicate predicate = std::bind(&SubsetLoadBalancer::hostMatches, this, default_subset_metadata_, std::placeholders::_1); - update_func(subset_default_->lb_subset_, predicate); + update_func(subset_default_->lb_subset_, predicate, random_.random()); } if (fallback_subset_ == nullptr) { @@ -622,9 +624,9 @@ void SubsetLoadBalancer::processSubsets(uint32_t priority, const HostVector& all single_duplicate_stat_->set(collision_count_of_single_host_entries); // Finalize updates after all the hosts are evaluated. - forEachSubset(subsets_, [priority](LbSubsetEntryPtr entry) { + forEachSubset(subsets_, [priority, this](LbSubsetEntryPtr entry) { if (entry->initialized()) { - entry->lb_subset_->finalize(priority); + entry->lb_subset_->finalize(priority, random_.random()); } }); } @@ -844,7 +846,7 @@ SubsetLoadBalancer::PrioritySubsetImpl::PrioritySubsetImpl(const SubsetLoadBalan // hosts that belong in this subset. void SubsetLoadBalancer::HostSubsetImpl::update(const HostHashSet& matching_hosts, const HostVector& hosts_added, - const HostVector& hosts_removed) { + const HostVector& hosts_removed, uint64_t seed) { auto cached_predicate = [&matching_hosts](const auto& host) { return matching_hosts.count(&host) == 1; }; @@ -905,7 +907,7 @@ void SubsetLoadBalancer::HostSubsetImpl::update(const HostHashSet& matching_host HostSetImpl::updateHostsParams( hosts, hosts_per_locality, healthy_hosts, healthy_hosts_per_locality, degraded_hosts, degraded_hosts_per_locality, excluded_hosts, excluded_hosts_per_locality), - determineLocalityWeights(*hosts_per_locality), hosts_added, hosts_removed, + determineLocalityWeights(*hosts_per_locality), hosts_added, hosts_removed, seed, original_host_set_.weightedPriorityHealth(), original_host_set_.overprovisioningFactor()); } @@ -962,9 +964,10 @@ HostSetImplPtr SubsetLoadBalancer::PrioritySubsetImpl::createHostSet( void SubsetLoadBalancer::PrioritySubsetImpl::update(uint32_t priority, const HostHashSet& matching_hosts, const HostVector& hosts_added, - const HostVector& hosts_removed) { + const HostVector& hosts_removed, + uint64_t seed) { const auto& host_subset = getOrCreateHostSet(priority); - updateSubset(priority, matching_hosts, hosts_added, hosts_removed); + updateSubset(priority, matching_hosts, hosts_added, hosts_removed, seed); if (host_subset.hosts().empty() != empty_) { empty_ = true; diff --git a/source/extensions/load_balancing_policies/subset/subset_lb.h b/source/extensions/load_balancing_policies/subset/subset_lb.h index ddf228a2c0e7..52c460c9d1eb 100644 --- a/source/extensions/load_balancing_policies/subset/subset_lb.h +++ b/source/extensions/load_balancing_policies/subset/subset_lb.h @@ -181,7 +181,7 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable(host_sets_[priority].get()) - ->update(matching_hosts, hosts_added, hosts_removed); + ->update(matching_hosts, hosts_added, hosts_removed, seed); runUpdateCallbacks(hosts_added, hosts_removed); } @@ -318,7 +319,7 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable; @@ -341,7 +342,7 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggablepriority_set_.updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, - true, 100); + 123, true, 100); auto* tls_cluster = cluster_manager_->getThreadLocalCluster(cluster1->info_->name()); @@ -4150,7 +4150,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdates) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -4161,12 +4161,12 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdates) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); cluster.prioritySet().updateHosts( 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -4184,7 +4184,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdates) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(2, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -4197,21 +4197,21 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdates) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); (*hosts)[0]->healthFlagSet(Host::HealthFlag::FAILED_EDS_HEALTH); cluster.prioritySet().updateHosts( 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); (*hosts)[0]->weight(100); cluster.prioritySet().updateHosts( 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); // Updates not delivered yet. EXPECT_EQ(2, factory_.stats_.counter("cluster_manager.cluster_updated").value()); @@ -4224,7 +4224,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdates) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(3, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); @@ -4267,7 +4267,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdatesOutOfWindow) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.update_out_of_merge_window").value()); @@ -4302,7 +4302,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdatesInsideWindow) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_out_of_merge_window").value()); @@ -4344,7 +4344,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdatesOutOfWindowDisabled) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_out_of_merge_window").value()); @@ -4421,7 +4421,7 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdatesDestroyedOnUpdate) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -4432,12 +4432,12 @@ TEST_P(ClusterManagerLifecycleTest, MergedUpdatesDestroyedOnUpdate) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); cluster.prioritySet().updateHosts( 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -4760,7 +4760,7 @@ TEST_P(ClusterManagerLifecycleTest, CrossPriorityHostMapSyncTest) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); @@ -4777,7 +4777,7 @@ TEST_P(ClusterManagerLifecycleTest, CrossPriorityHostMapSyncTest) { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, 123, absl::nullopt, absl::nullopt); EXPECT_EQ(2, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -5870,7 +5870,7 @@ TEST_P(ClusterManagerLifecycleTest, DrainConnectionsPredicate) { // Sending non-mergeable updates. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, - absl::nullopt, 100); + 123, absl::nullopt, 100); // Using RR LB get a pool for each host. EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) @@ -5942,7 +5942,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolsDrainedOnHostSetChange) { // Sending non-mergeable updates. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, - absl::nullopt, 100); + 123, absl::nullopt, 100); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); @@ -6003,7 +6003,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolsDrainedOnHostSetChange) { // This update should drain all connection pools (host1, host2). cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, - hosts_removed, absl::nullopt, 100); + hosts_removed, 123, absl::nullopt, 100); // Recreate connection pool for host1. cp1 = HttpPoolDataPeer::getPool( @@ -6032,7 +6032,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolsDrainedOnHostSetChange) { // Adding host3 should drain connection pool for host1. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, - hosts_added, {}, absl::nullopt, 100); + hosts_added, {}, 123, absl::nullopt, 100); } TEST_P(ClusterManagerLifecycleTest, ConnPoolsNotDrainedOnHostSetChange) { @@ -6067,7 +6067,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolsNotDrainedOnHostSetChange) { // Sending non-mergeable updates. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, - absl::nullopt, 100); + 123, absl::nullopt, 100); EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) .Times(1) @@ -6101,7 +6101,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolsNotDrainedOnHostSetChange) { // No connection pools should be drained. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, - hosts_added, {}, absl::nullopt, 100); + hosts_added, {}, 123, absl::nullopt, 100); } TEST_P(ClusterManagerLifecycleTest, ConnPoolsIdleDeleted) { @@ -6138,7 +6138,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolsIdleDeleted) { // Sending non-mergeable updates. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, - absl::nullopt, 100); + 123, absl::nullopt, 100); { auto* cp1 = new NiceMock(); @@ -6440,7 +6440,7 @@ class PreconnectTest : public ClusterManagerImplTest { // Sending non-mergeable updates. cluster_->prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, - {}, absl::nullopt, 100); + {}, 123, absl::nullopt, 100); } Cluster* cluster_{}; diff --git a/test/common/upstream/edf_scheduler_test.cc b/test/common/upstream/edf_scheduler_test.cc index 1619093aa077..e9ecdfd4565f 100644 --- a/test/common/upstream/edf_scheduler_test.cc +++ b/test/common/upstream/edf_scheduler_test.cc @@ -230,6 +230,15 @@ TEST_F(EdfSchedulerTest, SchedulerWithSomePicksEqualToEmptyWithAddedEntries) { } } +// Validating that calling `createWithPicks()` with no entries returns an empty +// scheduler. +TEST_F(EdfSchedulerTest, SchedulerWithSomePicksEmptyEntries) { + EdfScheduler sched = EdfScheduler::createWithPicks( + {}, [](const double& w) { return w; }, 123); + EXPECT_EQ(nullptr, sched.peekAgain([](const double&) { return 0; })); + EXPECT_EQ(nullptr, sched.pickAndAdd([](const double&) { return 0; })); +} + // Emulates first-pick scenarios by creating a scheduler with the given // weights and a random number of pre-picks, and validates that the next pick // of all the weights is close to the given weights. @@ -303,7 +312,7 @@ class EdfSchedulerSpecialTest : public testing::TestWithParam {}; // equal to the weights. Trying the case of 2 weights between 0 to 100, in steps // of 0.001. This test takes too long, and therefore it is disabled by default. // If the EDF scheduler is enable, it can be manually executed. -TEST_P(EdfSchedulerSpecialTest, DISABLED_ExhustiveValidator) { +TEST_P(EdfSchedulerSpecialTest, DISABLED_ExhaustiveValidator) { const uint64_t start_idx = GetParam(); for (uint64_t i = start_idx; i < start_idx + BATCH_SIZE; ++i) { const double w1 = 0.001 * i; diff --git a/test/common/upstream/load_balancer_benchmark.cc b/test/common/upstream/load_balancer_benchmark.cc index cc49c23f5594..9c12760cc17b 100644 --- a/test/common/upstream/load_balancer_benchmark.cc +++ b/test/common/upstream/load_balancer_benchmark.cc @@ -51,10 +51,10 @@ class BaseTester : public Event::TestUsingSimulatedTime { HostVectorConstSharedPtr updated_hosts = std::make_shared(hosts); HostsPerLocalityConstSharedPtr hosts_per_locality = makeHostsPerLocality({hosts}); priority_set_.updateHosts(0, HostSetImpl::partitionHosts(updated_hosts, hosts_per_locality), {}, - hosts, {}, absl::nullopt); + hosts, {}, random_.random(), absl::nullopt); local_priority_set_.updateHosts(0, HostSetImpl::partitionHosts(updated_hosts, hosts_per_locality), - {}, hosts, {}, absl::nullopt); + {}, hosts, {}, random_.random(), absl::nullopt); } Envoy::Thread::MutexBasicLockable lock_; diff --git a/test/common/upstream/load_balancer_impl_test.cc b/test/common/upstream/load_balancer_impl_test.cc index 518f2a4de1d0..d6a64c3b129e 100644 --- a/test/common/upstream/load_balancer_impl_test.cc +++ b/test/common/upstream/load_balancer_impl_test.cc @@ -728,7 +728,7 @@ class RoundRobinLoadBalancerTest : public LoadBalancerTestBase { 0, updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, empty_host_vector_, empty_host_vector_, absl::nullopt); + {}, empty_host_vector_, empty_host_vector_, random_.random(), absl::nullopt); } void peekThenPick(std::vector picks) { diff --git a/test/common/upstream/load_balancer_simulation_test.cc b/test/common/upstream/load_balancer_simulation_test.cc index 1ec475438a88..d223b5c48ea6 100644 --- a/test/common/upstream/load_balancer_simulation_test.cc +++ b/test/common/upstream/load_balancer_simulation_test.cc @@ -93,20 +93,20 @@ void leastRequestLBWeightTest(LRLBTestParams params) { } HostVectorConstSharedPtr updated_hosts{new HostVector(hosts)}; HostsPerLocalitySharedPtr updated_locality_hosts{new HostsPerLocalityImpl(hosts)}; + Random::RandomGeneratorImpl random; PrioritySetImpl priority_set; priority_set.updateHosts( 0, updateHostsParams(updated_hosts, updated_locality_hosts, std::make_shared(*updated_hosts), updated_locality_hosts), - {}, hosts, {}, absl::nullopt); + {}, hosts, {}, random.random(), absl::nullopt); Stats::IsolatedStoreImpl stats_store; ClusterLbStatNames stat_names(stats_store.symbolTable()); ClusterLbStats lb_stats{stat_names, *stats_store.rootScope()}; NiceMock runtime; auto time_source = std::make_unique>(); - Random::RandomGeneratorImpl random; envoy::config::cluster::v3::Cluster::LeastRequestLbConfig least_request_lb_config; envoy::config::cluster::v3::Cluster::CommonLbConfig common_config; LeastRequestLoadBalancer lb_{ @@ -264,7 +264,7 @@ class DISABLED_SimulationTest : public testing::Test { // NOLINT(readability-ide updateHostsParams(originating_hosts, per_zone_local_shared, std::make_shared(*originating_hosts), per_zone_local_shared), - {}, empty_vector_, empty_vector_, absl::nullopt); + {}, empty_vector_, empty_vector_, random_.random(), absl::nullopt); HostConstSharedPtr selected = lb.chooseHost(nullptr); hits[selected->address()->asString()]++; diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index aa388bb83dd9..1d13955a2ca8 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -3651,7 +3651,7 @@ class TestBatchUpdateCb : public PrioritySet::BatchUpdateCb { updateHostsParams(hosts_, hosts_per_locality_, std::make_shared(*hosts_), hosts_per_locality_), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, random_.random(), absl::nullopt, absl::nullopt); } // Remove the host from P1. @@ -3664,12 +3664,13 @@ class TestBatchUpdateCb : public PrioritySet::BatchUpdateCb { updateHostsParams(empty_hosts, HostsPerLocalityImpl::empty(), std::make_shared(*empty_hosts), HostsPerLocalityImpl::empty()), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt); + {}, hosts_added, hosts_removed, random_.random(), absl::nullopt, absl::nullopt); } } HostVectorSharedPtr hosts_; HostsPerLocalitySharedPtr hosts_per_locality_; + TestRandomGenerator random_; }; // Test creating and extending a priority set. @@ -3717,11 +3718,12 @@ TEST(PrioritySet, Extend) { HostVector hosts_added{hosts->front()}; HostVector hosts_removed{}; - priority_set.updateHosts( - 1, - updateHostsParams(hosts, hosts_per_locality, - std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt, absl::nullopt, fake_cross_priority_host_map); + priority_set.updateHosts(1, + updateHostsParams(hosts, hosts_per_locality, + std::make_shared(*hosts), + hosts_per_locality), + {}, hosts_added, hosts_removed, 0, absl::nullopt, absl::nullopt, + fake_cross_priority_host_map); } EXPECT_EQ(1, priority_changes); EXPECT_EQ(1, membership_changes); @@ -3786,7 +3788,7 @@ TEST(PrioritySet, MainPrioritySetTest) { updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt); + {}, hosts_added, hosts_removed, 0, absl::nullopt); } // Only mutable host map can be updated directly. Read only host map will not be updated before @@ -3807,7 +3809,7 @@ TEST(PrioritySet, MainPrioritySetTest) { updateHostsParams(hosts, hosts_per_locality, std::make_shared(*hosts), hosts_per_locality), - {}, hosts_added, hosts_removed, absl::nullopt); + {}, hosts_added, hosts_removed, 0, absl::nullopt); } // New mutable host map will be created and all update will be applied to new mutable host map. @@ -5539,7 +5541,7 @@ TEST_F(HostSetImplLocalityTest, AllUnhealthy) { LocalityWeightsConstSharedPtr locality_weights{new LocalityWeights{1, 1, 1}}; auto hosts_const_shared = std::make_shared(hosts); host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality), locality_weights, - {}, {}, absl::nullopt); + {}, {}, 0, absl::nullopt); EXPECT_FALSE(host_set_.chooseHealthyLocality().has_value()); } @@ -5575,7 +5577,7 @@ TEST_F(HostSetImplLocalityTest, NotWarmedHostsLocality) { HostsPerLocalityImpl::empty(), makeHostsFromHostsPerLocality(excluded_hosts_per_locality), excluded_hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); // We should RR between localities with equal weight. EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); @@ -5598,7 +5600,7 @@ TEST_F(HostSetImplLocalityTest, EmptyLocality) { host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, std::make_shared(hosts), hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); // Verify that we are not RRing between localities. EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); @@ -5619,7 +5621,7 @@ TEST_F(HostSetImplLocalityTest, AllZeroWeights) { host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, std::make_shared(hosts), hosts_per_locality), - locality_weights, {}, {}); + locality_weights, {}, {}, 0); EXPECT_FALSE(host_set_.chooseHealthyLocality().has_value()); } @@ -5642,7 +5644,7 @@ TEST_F(HostSetImplLocalityTest, Unweighted) { host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, std::make_shared(hosts), hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(2, host_set_.chooseHealthyLocality().value()); @@ -5666,7 +5668,7 @@ TEST_F(HostSetImplLocalityTest, Weighted) { host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, std::make_shared(hosts), hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); @@ -5694,7 +5696,7 @@ TEST_F(HostSetImplLocalityTest, MissingWeight) { host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, std::make_shared(hosts), hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(2, host_set_.chooseHealthyLocality().value()); EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); @@ -5703,6 +5705,77 @@ TEST_F(HostSetImplLocalityTest, MissingWeight) { EXPECT_EQ(2, host_set_.chooseHealthyLocality().value()); } +// Validates that with weighted initialization all localities are chosen +// proportionally to their weight. +TEST_F(HostSetImplLocalityTest, WeightedAllChosen) { + // This test should remain when envoy.reloadable_features.edf_lb_locality_scheduler_init_fix + // is deprecated. + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.edf_lb_locality_scheduler_init_fix", "true"}}); + envoy::config::core::v3::Locality zone_a; + zone_a.set_zone("A"); + envoy::config::core::v3::Locality zone_b; + zone_b.set_zone("B"); + envoy::config::core::v3::Locality zone_c; + zone_b.set_zone("C"); + HostVector hosts{makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), zone_a), + makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), zone_b), + makeTestHost(info_, "tcp://127.0.0.1:82", simTime(), zone_c)}; + + HostsPerLocalitySharedPtr hosts_per_locality = + makeHostsPerLocality({{hosts[0]}, {hosts[1]}, {hosts[2]}}); + // Set weights of 10%, 60% and 30% to the three zones. + LocalityWeightsConstSharedPtr locality_weights{new LocalityWeights{1, 6, 3}}; + + // Keep track of how many times each locality is picked, initialized to 0. + uint32_t locality_picked_count[] = {0, 0, 0}; + + // Create the load-balancer 10 times, each with a different seed number (from + // 0 to 10), do a single pick, and validate that the number of picks equals + // to the weights assigned to the localities. + auto hosts_const_shared = std::make_shared(hosts); + for (uint32_t i = 0; i < 10; ++i) { + host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, + std::make_shared(hosts), + hosts_per_locality), + locality_weights, {}, {}, i, absl::nullopt); + locality_picked_count[host_set_.chooseHealthyLocality().value()]++; + } + EXPECT_EQ(locality_picked_count[0], 1); + EXPECT_EQ(locality_picked_count[1], 6); + EXPECT_EQ(locality_picked_count[2], 3); +} + +// Validates that the non-randomized initialization works. This test should be +// removed when envoy.reloadable_features.edf_lb_locality_scheduler_init_fix +// is deprecated. +TEST_F(HostSetImplLocalityTest, WeightedNoRandomization) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.edf_lb_locality_scheduler_init_fix", "false"}}); + envoy::config::core::v3::Locality zone_a; + zone_a.set_zone("A"); + envoy::config::core::v3::Locality zone_b; + zone_b.set_zone("B"); + HostVector hosts{makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), zone_a), + makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), zone_b)}; + + HostsPerLocalitySharedPtr hosts_per_locality = makeHostsPerLocality({{hosts[0]}, {hosts[1]}}); + LocalityWeightsConstSharedPtr locality_weights{new LocalityWeights{1, 2}}; + auto hosts_const_shared = std::make_shared(hosts); + host_set_.updateHosts(updateHostsParams(hosts_const_shared, hosts_per_locality, + std::make_shared(hosts), + hosts_per_locality), + locality_weights, {}, {}, 0, absl::nullopt); + EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); + EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); + EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); + EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); + EXPECT_EQ(0, host_set_.chooseHealthyLocality().value()); + EXPECT_EQ(1, host_set_.chooseHealthyLocality().value()); +} + // Gentle failover between localities as health diminishes. TEST_F(HostSetImplLocalityTest, UnhealthyFailover) { envoy::config::core::v3::Locality zone_a; @@ -5732,7 +5805,7 @@ TEST_F(HostSetImplLocalityTest, UnhealthyFailover) { makeHostsFromHostsPerLocality( healthy_hosts_per_locality), healthy_hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); }; const auto expectPicks = [this](uint32_t locality_0_picks, uint32_t locality_1_picks) { @@ -5785,7 +5858,7 @@ TEST(OverProvisioningFactorTest, LocalityPickChanges) { host_set.updateHosts(updateHostsParams(std::make_shared(hosts), hosts_per_locality, healthy_hosts, healthy_hosts_per_locality), - locality_weights, {}, {}, absl::nullopt); + locality_weights, {}, {}, 0, absl::nullopt); uint32_t cnts[] = {0, 0}; for (uint32_t i = 0; i < 100; ++i) { absl::optional locality_index = host_set.chooseHealthyLocality(); @@ -5957,9 +6030,11 @@ TEST_F(PriorityStateManagerTest, LocalityClusterUpdate) { cluster->initialize([] {}); EXPECT_EQ(1UL, cluster->prioritySet().hostSetsPerPriority()[0]->hosts().size()); + NiceMock random; // Make priority state manager and fill it with the initial state of the cluster and the added // hosts - PriorityStateManager priority_state_manager(*cluster, server_context_.local_info_, nullptr); + PriorityStateManager priority_state_manager(*cluster, server_context_.local_info_, nullptr, + random); auto current_hosts = cluster->prioritySet().hostSetsPerPriority()[0]->hosts(); HostVector hosts_added{makeTestHost(cluster->info(), "tcp://127.0.0.1:81", simTime(), zone_b), diff --git a/test/common/upstream/zone_aware_load_balancer_fuzz_base.cc b/test/common/upstream/zone_aware_load_balancer_fuzz_base.cc index e9ed21b67094..16eb8dc0fad3 100644 --- a/test/common/upstream/zone_aware_load_balancer_fuzz_base.cc +++ b/test/common/upstream/zone_aware_load_balancer_fuzz_base.cc @@ -17,7 +17,7 @@ void ZoneAwareLoadBalancerFuzzBase::initializeASingleHostSet( const MockHostSet& host_set = *priority_set_.getMockHostSet(priority_level); const HostVector empty_host_vector; local_priority_set_->updateHosts(0, HostSetImpl::updateHostsParams(host_set), {}, - empty_host_vector, empty_host_vector, absl::nullopt); + empty_host_vector, empty_host_vector, 123, absl::nullopt); } } @@ -34,7 +34,7 @@ void ZoneAwareLoadBalancerFuzzBase::updateHealthFlagsForAHostSet( const MockHostSet& host_set = *priority_set_.getMockHostSet(priority_of_host_set); const HostVector empty_host_vector; local_priority_set_->updateHosts(0, HostSetImpl::updateHostsParams(host_set), {}, - empty_host_vector, empty_host_vector, absl::nullopt); + empty_host_vector, empty_host_vector, 123, absl::nullopt); } } diff --git a/test/extensions/clusters/aggregate/cluster_test.cc b/test/extensions/clusters/aggregate/cluster_test.cc index f78ccb511a33..cd844a4c9167 100644 --- a/test/extensions/clusters/aggregate/cluster_test.cc +++ b/test/extensions/clusters/aggregate/cluster_test.cc @@ -74,7 +74,7 @@ class AggregateClusterTest : public Event::TestUsingSimulatedTime, public testin priority, Upstream::HostSetImpl::partitionHosts(std::make_shared(hosts), Upstream::HostsPerLocalityImpl::empty()), - nullptr, hosts, {}, absl::nullopt, 100); + nullptr, hosts, {}, 123, absl::nullopt, 100); } void setupSecondary(int priority, int healthy_hosts, int degraded_hosts, int unhealthy_hosts) { @@ -84,7 +84,7 @@ class AggregateClusterTest : public Event::TestUsingSimulatedTime, public testin priority, Upstream::HostSetImpl::partitionHosts(std::make_shared(hosts), Upstream::HostsPerLocalityImpl::empty()), - nullptr, hosts, {}, absl::nullopt, 100); + nullptr, hosts, {}, 123, absl::nullopt, 100); } void setupPrioritySet() { @@ -180,7 +180,7 @@ TEST_F(AggregateClusterTest, LoadBalancerTest) { EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(nullptr)); for (int i = 0; i <= 65; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); EXPECT_TRUE(lb_->peekAnotherHost(nullptr) == nullptr); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); OptRef lifetime_callbacks = @@ -196,7 +196,7 @@ TEST_F(AggregateClusterTest, LoadBalancerTest) { EXPECT_CALL(primary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(nullptr)); EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(host)); for (int i = 66; i < 100; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -215,7 +215,7 @@ TEST_F(AggregateClusterTest, LoadBalancerTest) { EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(nullptr)); for (int i = 0; i <= 57; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -223,7 +223,7 @@ TEST_F(AggregateClusterTest, LoadBalancerTest) { EXPECT_CALL(primary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(nullptr)); EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(host)); for (int i = 58; i < 100; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -252,7 +252,7 @@ TEST_F(AggregateClusterTest, AllHostAreUnhealthyTest) { // Choose the first cluster as the second one is unavailable. for (int i = 0; i < 50; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -262,7 +262,7 @@ TEST_F(AggregateClusterTest, AllHostAreUnhealthyTest) { // Choose the second cluster as the first one is unavailable. for (int i = 50; i < 100; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -288,7 +288,7 @@ TEST_F(AggregateClusterTest, ClusterInPanicTest) { EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(nullptr)); for (int i = 0; i < 50; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -297,7 +297,7 @@ TEST_F(AggregateClusterTest, ClusterInPanicTest) { EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(host)); for (int i = 50; i < 100; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -317,7 +317,7 @@ TEST_F(AggregateClusterTest, ClusterInPanicTest) { EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(nullptr)); for (int i = 0; i <= 25; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } @@ -326,7 +326,7 @@ TEST_F(AggregateClusterTest, ClusterInPanicTest) { EXPECT_CALL(secondary_load_balancer_, chooseHost(_)).WillRepeatedly(Return(host)); for (int i = 26; i < 100; ++i) { - EXPECT_CALL(random_, random()).WillOnce(Return(i)); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(i)); Upstream::HostConstSharedPtr target = lb_->chooseHost(nullptr); EXPECT_EQ(host.get(), target.get()); } diff --git a/test/extensions/clusters/aggregate/cluster_update_test.cc b/test/extensions/clusters/aggregate/cluster_update_test.cc index 67c029b52799..281cd49b87e5 100644 --- a/test/extensions/clusters/aggregate/cluster_update_test.cc +++ b/test/extensions/clusters/aggregate/cluster_update_test.cc @@ -162,7 +162,7 @@ TEST_P(AggregateClusterUpdateTest, LoadBalancingTest) { Upstream::HostSetImpl::partitionHosts( std::make_shared(Upstream::HostVector{host1, host2, host3}), Upstream::HostsPerLocalityImpl::empty()), - nullptr, {host1, host2, host3}, {}, absl::nullopt, 100); + nullptr, {host1, host2, host3}, {}, 0, absl::nullopt, 100); // Set up the HostSet with 1 healthy, 1 degraded and 1 unhealthy. Upstream::HostSharedPtr host4 = @@ -179,7 +179,7 @@ TEST_P(AggregateClusterUpdateTest, LoadBalancingTest) { Upstream::HostSetImpl::partitionHosts( std::make_shared(Upstream::HostVector{host4, host5, host6}), Upstream::HostsPerLocalityImpl::empty()), - nullptr, {host4, host5, host6}, {}, absl::nullopt, 100); + nullptr, {host4, host5, host6}, {}, 0, absl::nullopt, 100); Upstream::HostConstSharedPtr host; for (int i = 0; i < 33; ++i) { @@ -219,7 +219,7 @@ TEST_P(AggregateClusterUpdateTest, LoadBalancingTest) { Upstream::HostSetImpl::partitionHosts( std::make_shared(Upstream::HostVector{host7, host8, host9}), Upstream::HostsPerLocalityImpl::empty()), - nullptr, {host7, host8, host9}, {}, absl::nullopt, 100); + nullptr, {host7, host8, host9}, {}, 0, absl::nullopt, 100); // Priority set // Priority 0: 1/3 healthy, 1/3 degraded @@ -306,7 +306,7 @@ TEST_P(AggregateClusterUpdateTest, InitializeAggregateClusterAfterOtherClusters) Upstream::HostSetImpl::partitionHosts( std::make_shared(Upstream::HostVector{host1, host2, host3}), Upstream::HostsPerLocalityImpl::empty()), - nullptr, {host1, host2, host3}, {}, absl::nullopt, 100); + nullptr, {host1, host2, host3}, {}, 0, absl::nullopt, 100); for (int i = 0; i < 50; ++i) { EXPECT_CALL(factory_.random_, random()).WillRepeatedly(Return(i)); diff --git a/test/extensions/clusters/original_dst/original_dst_cluster_test.cc b/test/extensions/clusters/original_dst/original_dst_cluster_test.cc index ae5d1a65de9a..46d2e3c4211f 100644 --- a/test/extensions/clusters/original_dst/original_dst_cluster_test.cc +++ b/test/extensions/clusters/original_dst/original_dst_cluster_test.cc @@ -661,7 +661,7 @@ TEST_F(OriginalDstClusterTest, MultipleClusters) { second.updateHosts(0, updateHostsParams(new_hosts, empty_hosts_per_locality, healthy_hosts, empty_hosts_per_locality), - {}, added, removed, absl::nullopt); + {}, added, removed, 0, absl::nullopt); }); EXPECT_CALL(membership_updated_, ready()); diff --git a/test/extensions/load_balancing_policies/common/benchmark_base_tester.cc b/test/extensions/load_balancing_policies/common/benchmark_base_tester.cc index 3cc30ae2eb69..32a1521c73e4 100644 --- a/test/extensions/load_balancing_policies/common/benchmark_base_tester.cc +++ b/test/extensions/load_balancing_policies/common/benchmark_base_tester.cc @@ -32,10 +32,10 @@ BaseTester::BaseTester(uint64_t num_hosts, uint32_t weighted_subset_percent, uin Upstream::makeHostsPerLocality({hosts}); priority_set_.updateHosts( 0, Upstream::HostSetImpl::partitionHosts(updated_hosts, hosts_per_locality), {}, hosts, {}, - absl::nullopt); + random_.random(), absl::nullopt); local_priority_set_.updateHosts( 0, Upstream::HostSetImpl::partitionHosts(updated_hosts, hosts_per_locality), {}, hosts, {}, - absl::nullopt); + random_.random(), absl::nullopt); } } // namespace Common diff --git a/test/extensions/load_balancing_policies/subset/subset_benchmark.cc b/test/extensions/load_balancing_policies/subset/subset_benchmark.cc index d775c8851e33..e09337efca9c 100644 --- a/test/extensions/load_balancing_policies/subset/subset_benchmark.cc +++ b/test/extensions/load_balancing_policies/subset/subset_benchmark.cc @@ -58,10 +58,10 @@ class SubsetLbTester : public LoadBalancingPolices::Common::BaseTester { void update() { priority_set_.updateHosts( 0, Upstream::HostSetImpl::partitionHosts(smaller_hosts_, smaller_locality_hosts_), nullptr, - {}, host_moved_, absl::nullopt); + {}, host_moved_, random_.random(), absl::nullopt); priority_set_.updateHosts( 0, Upstream::HostSetImpl::partitionHosts(orig_hosts_, orig_locality_hosts_), nullptr, - host_moved_, {}, absl::nullopt); + host_moved_, {}, random_.random(), absl::nullopt); } std::unique_ptr subset_info_; @@ -71,6 +71,7 @@ class SubsetLbTester : public LoadBalancingPolices::Common::BaseTester { Upstream::HostsPerLocalitySharedPtr orig_locality_hosts_; Upstream::HostsPerLocalitySharedPtr smaller_locality_hosts_; Upstream::HostVector host_moved_; + Random::RandomGeneratorImpl random_; }; void benchmarkSubsetLoadBalancerCreate(::benchmark::State& state) { diff --git a/test/extensions/load_balancing_policies/subset/subset_test.cc b/test/extensions/load_balancing_policies/subset/subset_test.cc index 032fc45b5662..d9c241a30f3b 100644 --- a/test/extensions/load_balancing_policies/subset/subset_test.cc +++ b/test/extensions/load_balancing_policies/subset/subset_test.cc @@ -346,7 +346,7 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime, std::make_shared(*local_hosts_), local_hosts_per_locality_, std::make_shared(), HostsPerLocalityImpl::empty(), std::make_shared(), HostsPerLocalityImpl::empty()), - {}, {}, {}, absl::nullopt); + {}, {}, {}, 0, absl::nullopt); auto child_lb_creator = std::make_unique( lb_type_, ring_hash_lb_config_, maglev_lb_config_, round_robin_lb_config_, @@ -501,7 +501,7 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime, updateHostsParams(local_hosts_, local_hosts_per_locality_, std::make_shared(*local_hosts_), local_hosts_per_locality_), - {}, {}, remove, absl::nullopt); + {}, {}, remove, 0, absl::nullopt); } for (const auto& host : add) { @@ -518,7 +518,7 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime, updateHostsParams(local_hosts_, local_hosts_per_locality_, std::make_shared(*local_hosts_), local_hosts_per_locality_), - {}, add, {}, absl::nullopt); + {}, add, {}, 0, absl::nullopt); } } else if (!add.empty() || !remove.empty()) { local_priority_set_.updateHosts( @@ -526,7 +526,7 @@ class SubsetLoadBalancerTest : public Event::TestUsingSimulatedTime, updateHostsParams(local_hosts_, local_hosts_per_locality_, std::make_shared(*local_hosts_), local_hosts_per_locality_), - {}, add, remove, absl::nullopt); + {}, add, remove, 0, absl::nullopt); } } @@ -1706,6 +1706,7 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareFallbackAfterUpdate) { envoy::config::core::v3::Locality local_locality; local_locality.set_zone("0"); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(0)); modifyHosts({makeHost("tcp://127.0.0.1:8000", {{"version", "1.0"}}, local_locality)}, {host_set_.hosts_[0]}, absl::optional(0)); @@ -1836,6 +1837,7 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareFallbackDefaultSubsetAfterUpdate) { envoy::config::core::v3::Locality local_locality; local_locality.set_zone("0"); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(0)); modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "default"}}, local_locality)}, {host_set_.hosts_[1]}, absl::optional(0)); @@ -1962,6 +1964,7 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareBalancesSubsetsAfterUpdate) { envoy::config::core::v3::Locality local_locality; local_locality.set_zone("0"); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(0)); modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "1.1"}}, local_locality)}, {host_set_.hosts_[1]}, absl::optional(0)); @@ -2116,6 +2119,7 @@ TEST_P(SubsetLoadBalancerTest, ZoneAwareComplicatedBalancesSubsetsAfterUpdate) { envoy::config::core::v3::Locality locality_2; locality_2.set_zone("2"); + EXPECT_CALL(random_, random()).WillRepeatedly(Return(0)); modifyHosts({makeHost("tcp://127.0.0.1:8001", {{"version", "1.1"}}, local_locality)}, {}, absl::optional(0)); diff --git a/test/integration/clusters/custom_static_cluster.cc b/test/integration/clusters/custom_static_cluster.cc index 709ea07c9b7a..847215724799 100644 --- a/test/integration/clusters/custom_static_cluster.cc +++ b/test/integration/clusters/custom_static_cluster.cc @@ -16,7 +16,7 @@ void CustomStaticCluster::startPreInit() { priority_set_.updateHosts( priority_, Upstream::HostSetImpl::partitionHosts(hosts_ptr, Upstream::HostsPerLocalityImpl::empty()), {}, - hosts, {}, absl::nullopt); + hosts, {}, 123, absl::nullopt); onPreInitComplete(); } diff --git a/test/mocks/upstream/priority_set.h b/test/mocks/upstream/priority_set.h index a19e208d1258..b7ba1d201970 100644 --- a/test/mocks/upstream/priority_set.h +++ b/test/mocks/upstream/priority_set.h @@ -25,7 +25,8 @@ class MockPrioritySet : public PrioritySet { MOCK_METHOD(void, updateHosts, (uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, const HostVector& hosts_added, - const HostVector& hosts_removed, absl::optional weighted_priority_health, + const HostVector& hosts_removed, uint64_t seed, + absl::optional weighted_priority_health, absl::optional overprovisioning_factor, HostMapConstSharedPtr cross_priority_host_map)); MOCK_METHOD(void, batchHostUpdate, (BatchUpdateCb&)); From 6ef9425402dd930c1082a654d314126d32664dc2 Mon Sep 17 00:00:00 2001 From: phlax Date: Tue, 6 Feb 2024 15:54:40 +0000 Subject: [PATCH 05/25] test/coverage: Lower `source/common` (related to `io_uring` execution) (#32223) Signed-off-by: Ryan Northey --- test/per_file_coverage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index b94220025f8f..ef1713013883 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -3,7 +3,7 @@ # directory:coverage_percent # for existing directories with low coverage. declare -a KNOWN_LOW_COVERAGE=( -"source/common:96.0" # TODO(#32149): increase this once io_uring is tested. +"source/common:95.9" # TODO(#32149): increase this once io_uring is tested. "source/common/api:84.5" # flaky due to posix: be careful adjusting "source/common/api/posix:83.8" # flaky (accept failover non-deterministic): be careful adjusting "source/common/config:95.4" From 26a4eb87428dbf3a39ff4f6f61b69538c34d07b6 Mon Sep 17 00:00:00 2001 From: Juan Manuel Olle Date: Tue, 6 Feb 2024 13:53:15 -0300 Subject: [PATCH 06/25] aws_lambda: Update to support upstream filter usage (#32146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Juan Manuel Ollé --- changelogs/current.yaml | 4 + .../_include/aws-lambda-filter-upstream.yaml | 57 +++++++++++ .../http/http_filters/aws_lambda_filter.rst | 20 ++++ source/extensions/extensions_metadata.yaml | 2 + .../http/aws_lambda/aws_lambda_filter.cc | 17 ++-- .../http/aws_lambda/aws_lambda_filter.h | 3 +- .../filters/http/aws_lambda/config.cc | 14 +-- .../filters/http/aws_lambda/config.h | 14 ++- .../filters/http/common/factory_base.h | 5 +- .../aws_lambda_filter_integration_test.cc | 94 +++++++++++++++++-- .../http/aws_lambda/aws_lambda_filter_test.cc | 26 ++++- .../filters/http/aws_lambda/config_test.cc | 8 ++ 12 files changed, 233 insertions(+), 31 deletions(-) create mode 100644 docs/root/configuration/http/http_filters/_include/aws-lambda-filter-upstream.yaml diff --git a/changelogs/current.yaml b/changelogs/current.yaml index bb1ca8a3677a..acff38659afc 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -62,6 +62,10 @@ new_features: change: | Update ``aws_request_signing`` filter to support use as an upstream HTTP filter. This allows successful calculation of signatures after the forwarding stage has completed, particularly if the path element is modified. +- area: aws_lambda + change: | + Update ``aws_lambda`` filter to support use as an upstream HTTP filter. This allows successful calculation of + signatures after the forwarding stage has completed, particularly if the path element is modified. - area: grpc reverse bridge change: | Change HTTP status to 200 to respect the gRPC protocol. This may cause problems for incorrect gRPC clients expecting the filter diff --git a/docs/root/configuration/http/http_filters/_include/aws-lambda-filter-upstream.yaml b/docs/root/configuration/http/http_filters/_include/aws-lambda-filter-upstream.yaml new file mode 100644 index 000000000000..736f688d740d --- /dev/null +++ b/docs/root/configuration/http/http_filters/_include/aws-lambda-filter-upstream.yaml @@ -0,0 +1,57 @@ +static_resources: + listeners: + - address: + socket_address: + address: 0.0.0.0 + port_value: 10000 + filter_chains: + - filters: + - name: envoy.filters.network.http_connection_manager + typed_config: + '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager + stat_prefix: ingress_http + http_filters: + - name: envoy.filters.http.router + typed_config: + '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + route_config: + name: local_route + virtual_hosts: + - domains: + - '*' + name: local_service + routes: + - match: {prefix: "/"} + route: {cluster: default_service} + clusters: + - name: default_service + load_assignment: + cluster_name: default_service + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 10001 + typed_extension_protocol_options: + envoy.extensions.upstreams.http.v3.HttpProtocolOptions: + "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions + upstream_http_protocol_options: + auto_sni: true + auto_san_validation: true + auto_config: + http2_protocol_options: {} + http_filters: + - name: envoy.filters.http.aws_lambda + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.aws_lambda.v3.Config + arn: "arn:aws:lambda:us-west-2:987654321:function:hello_envoy" + payload_passthrough: false + - name: envoy.filters.http.upstream_codec + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.upstream_codec.v3.UpstreamCodec + transport_socket: + name: envoy.transport_sockets.tls + typed_config: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext diff --git a/docs/root/configuration/http/http_filters/aws_lambda_filter.rst b/docs/root/configuration/http/http_filters/aws_lambda_filter.rst index 8f0a7b153244..b2dd0d274b58 100644 --- a/docs/root/configuration/http/http_filters/aws_lambda_filter.rst +++ b/docs/root/configuration/http/http_filters/aws_lambda_filter.rst @@ -94,6 +94,7 @@ If you use the per-filter configuration, the target cluster *must* have the foll com.amazonaws.lambda: egress_gateway: true +If you use the upstream filter configuration, this metadata is not required. Below are some examples that show how the filter can be used in different deployment scenarios. @@ -185,6 +186,25 @@ An example with the Lambda metadata applied to a weighted-cluster: "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext sni: "*.amazonaws.com" +Configuration as an upstream HTTP filter +---------------------------------------- +SigV4 or SigV4A request signatures are calculated using the HTTP host, URL and payload as input. Depending on the configuration, Envoy may modify one or more of +these prior to forwarding to the Cluster subsystem, but after the signature has been calculated and inserted into the HTTP headers. Modifying fields in a SigV4 or SigV4A +signed request will result in an invalid signature. + +To avoid invalid signatures, the AWS Request Signing Filter can be configured as an upstream HTTP filter. This allows signatures to be +calculated as a final step before the HTTP request is forwarded upstream, ensuring signatures are correctly calculated over the updated +HTTP fields. + +Configuring this filter as an upstream HTTP filter is done in a similar way to the downstream case, but using the :ref:`http_filters ` +filter chain within the cluster configuration. + +.. literalinclude:: _include/aws-lambda-filter-upstream.yaml + :language: yaml + :lines: 26-50 + :lineno-start: 26 + :linenos: + :caption: :download:`aws-lambda-filter-upstream.yaml <_include/aws-lambda-filter-upstream.yaml>` .. include:: _include/aws_credentials.rst diff --git a/source/extensions/extensions_metadata.yaml b/source/extensions/extensions_metadata.yaml index 119da4c25529..7efa3acdb2ae 100644 --- a/source/extensions/extensions_metadata.yaml +++ b/source/extensions/extensions_metadata.yaml @@ -197,8 +197,10 @@ envoy.filters.http.alternate_protocols_cache: envoy.filters.http.aws_lambda: categories: - envoy.filters.http + - envoy.filters.http.upstream security_posture: requires_trusted_downstream_and_upstream status: alpha + status_upstream: alpha type_urls: - envoy.extensions.filters.http.aws_lambda.v3.Config - envoy.extensions.filters.http.aws_lambda.v3.PerRouteConfig diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc index 0b9f862059b6..f2a989f17d1c 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc @@ -114,8 +114,9 @@ bool isContentTypeTextual(const Http::RequestOrResponseHeaderMap& headers) { // TODO(nbaws) Implement Sigv4a support Filter::Filter(const FilterSettings& settings, const FilterStats& stats, - const std::shared_ptr& sigv4_signer) - : settings_(settings), stats_(stats), sigv4_signer_(sigv4_signer) {} + const std::shared_ptr& sigv4_signer, + bool is_upstream) + : settings_(settings), stats_(stats), sigv4_signer_(sigv4_signer), is_upstream_(is_upstream) {} absl::optional Filter::getRouteSpecificSettings() const { const auto* settings = @@ -139,11 +140,13 @@ void Filter::resolveSettings() { } Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { - auto cluster_info_ptr = decoder_callbacks_->clusterInfo(); - if (!cluster_info_ptr || !isTargetClusterLambdaGateway(*cluster_info_ptr)) { - skip_ = true; - ENVOY_LOG(trace, "Target cluster does not have the Lambda metadata. Moving on."); - return Http::FilterHeadersStatus::Continue; + if (!is_upstream_) { + auto cluster_info_ptr = decoder_callbacks_->clusterInfo(); + if (!cluster_info_ptr || !isTargetClusterLambdaGateway(*cluster_info_ptr)) { + skip_ = true; + ENVOY_LOG(trace, "Target cluster does not have the Lambda metadata. Moving on."); + return Http::FilterHeadersStatus::Continue; + } } resolveSettings(); diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h index 19525fa83426..bcfcbe63bda0 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h @@ -97,7 +97,7 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable& sigv4_signer); + const std::shared_ptr& sigv4_signer, bool is_upstream); Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool end_stream) override; Http::FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; @@ -135,6 +135,7 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable AwsLambdaFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::aws_lambda::v3::Config& proto_config, - const std::string& stat_prefix, Server::Configuration::FactoryContext& context) { - auto& server_context = context.serverFactoryContext(); + const std::string& stats_prefix, DualInfo dual_info, + Server::Configuration::ServerFactoryContext& server_context) { const auto arn = parseArn(proto_config.arn()); if (!arn) { @@ -60,9 +60,9 @@ Http::FilterFactoryCb AwsLambdaFilterFactory::createFilterFactoryFromProtoTyped( FilterSettings filter_settings{*arn, getInvocationMode(proto_config), proto_config.payload_passthrough()}; - FilterStats stats = generateStats(stat_prefix, context.scope()); - return [stats, signer, filter_settings](Http::FilterChainFactoryCallbacks& cb) { - auto filter = std::make_shared(filter_settings, stats, signer); + FilterStats stats = generateStats(stats_prefix, dual_info.scope); + return [stats, signer, filter_settings, dual_info](Http::FilterChainFactoryCallbacks& cb) { + auto filter = std::make_shared(filter_settings, stats, signer, dual_info.is_upstream); cb.addStreamFilter(filter); }; } @@ -86,6 +86,8 @@ AwsLambdaFilterFactory::createRouteSpecificFilterConfigTyped( * Static registration for the AWS Lambda filter. @see RegisterFactory. */ REGISTER_FACTORY(AwsLambdaFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); +REGISTER_FACTORY(UpstreamAwsLambdaFilterFactory, + Server::Configuration::UpstreamHttpFilterConfigFactory); } // namespace AwsLambdaFilter } // namespace HttpFilters diff --git a/source/extensions/filters/http/aws_lambda/config.h b/source/extensions/filters/http/aws_lambda/config.h index d135f0fe2e3c..73e3eca8e474 100644 --- a/source/extensions/filters/http/aws_lambda/config.h +++ b/source/extensions/filters/http/aws_lambda/config.h @@ -11,21 +11,25 @@ namespace HttpFilters { namespace AwsLambdaFilter { class AwsLambdaFilterFactory - : public Common::FactoryBase { + : public Common::DualFactoryBase< + envoy::extensions::filters::http::aws_lambda::v3::Config, + envoy::extensions::filters::http::aws_lambda::v3::PerRouteConfig> { public: - AwsLambdaFilterFactory() : FactoryBase("envoy.filters.http.aws_lambda") {} + AwsLambdaFilterFactory() : DualFactoryBase("envoy.filters.http.aws_lambda") {} private: - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::aws_lambda::v3::Config& proto_config, - const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; + const std::string& stats_prefix, DualInfo dual_info, + Server::Configuration::ServerFactoryContext& context) override; Router::RouteSpecificFilterConfigConstSharedPtr createRouteSpecificFilterConfigTyped( const envoy::extensions::filters::http::aws_lambda::v3::PerRouteConfig&, Server::Configuration::ServerFactoryContext&, ProtobufMessage::ValidationVisitor&) override; }; +using UpstreamAwsLambdaFilterFactory = AwsLambdaFilterFactory; + } // namespace AwsLambdaFilter } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/common/factory_base.h b/source/extensions/filters/http/common/factory_base.h index ae0288c8e1fc..3773237ae85e 100644 --- a/source/extensions/filters/http/common/factory_base.h +++ b/source/extensions/filters/http/common/factory_base.h @@ -127,11 +127,12 @@ class DualFactoryBase : public CommonFactoryBase, struct DualInfo { DualInfo(Server::Configuration::UpstreamFactoryContext& context) - : init_manager(context.initManager()), scope(context.scope()) {} + : init_manager(context.initManager()), scope(context.scope()), is_upstream(true) {} DualInfo(Server::Configuration::FactoryContext& context) - : init_manager(context.initManager()), scope(context.scope()) {} + : init_manager(context.initManager()), scope(context.scope()), is_upstream(false) {} Init::Manager& init_manager; Stats::Scope& scope; + bool is_upstream; }; absl::StatusOr diff --git a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc index bee8f3ad2a2e..76ee22f440e4 100644 --- a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc +++ b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc @@ -29,7 +29,35 @@ class AwsLambdaFilterIntegrationTest : public testing::TestWithParammutable_clusters(0); + + ConfigHelper::HttpProtocolOptions protocol_options; + protocol_options.mutable_upstream_http_protocol_options()->set_auto_sni(true); + protocol_options.mutable_upstream_http_protocol_options()->set_auto_san_validation(true); + protocol_options.mutable_explicit_http_config()->mutable_http_protocol_options(); + ConfigHelper::setProtocolOptions(*cluster, protocol_options); + }); + } + + void replaceRoute() { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + auto* vhost = hcm.mutable_route_config()->mutable_virtual_hosts(0); + vhost->clear_routes(); + auto route = vhost->add_routes(); + auto* match = route->mutable_match(); + match->set_prefix("/api/lambda/"); + auto* action = route->mutable_route(); + action->set_cluster("cluster_0"); + action->set_prefix_rewrite("/new_path/"); + action->set_host_rewrite_literal("lambda.us-east-2.amazonaws.com"); + }); + } + + void setupLambdaFilter(bool passthrough, bool downstream) { constexpr absl::string_view filter = R"EOF( name: envoy.filters.http.aws_lambda @@ -38,13 +66,18 @@ class AwsLambdaFilterIntegrationTest : public testing::TestWithParam @@ -157,8 +190,8 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, AwsLambdaFilterIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); -TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedHeaderOnlyRequest) { - setupLambdaFilter(false /*passthrough*/); +TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedHeaderOnlyRequestDownstream) { + setupLambdaFilter(false /*passthrough*/, true); HttpIntegrationTest::initialize(); Http::TestRequestHeaderMapImpl request_headers{{":scheme", "http"}, @@ -203,7 +236,7 @@ TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedHeaderOnlyRequest) { } TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedPlainBody) { - setupLambdaFilter(false /*passthrough*/); + setupLambdaFilter(false /*passthrough*/, true); HttpIntegrationTest::initialize(); Http::TestRequestHeaderMapImpl request_headers{{":scheme", "http"}, @@ -252,7 +285,7 @@ TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedPlainBody) { } TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedBinaryBody) { - setupLambdaFilter(false /*passthrough*/); + setupLambdaFilter(false /*passthrough*/, true); HttpIntegrationTest::initialize(); Http::TestRequestHeaderMapImpl request_headers{{":scheme", "http"}, @@ -300,5 +333,50 @@ TEST_P(AwsLambdaFilterIntegrationTest, JsonWrappedBinaryBody) { expected_response_body); } +TEST_P(AwsLambdaFilterIntegrationTest, UpstreamShouldBeProcessedAfterRoute) { + setupLambdaFilter(false /*passthrough*/, false); + HttpIntegrationTest::initialize(); + + Http::TestRequestHeaderMapImpl request_headers{{":scheme", "http"}, + {":method", "GET"}, + {":path", "/api/lambda/resize?type=jpg"}, + {":authority", "host"}, + {"s3-location", "mybucket/images/123.jpg"}}; + constexpr auto expected_json_request = R"EOF( + { + "rawPath": "/new_path/resize?type=jpg", + "method": "GET", + "headers":{ "s3-location": "mybucket/images/123.jpg"}, + "queryStringParameters": {"type":"jpg"}, + "body": "", + "isBase64Encoded": false + } + )EOF"; + + const std::string lambda_response_body = R"EOF( + { + "body": "my-bucket/123-small.jpg", + "isBase64Encoded": false, + "statusCode": 200, + "cookies": ["user=John", "session-id=1337"], + "headers": {"x-amz-custom-header": "envoy,proxy"} + } + )EOF"; + + Http::TestResponseHeaderMapImpl lambda_response_headers{ + {":status", "201"}, + {"content-type", "application/json"}, + {"content-length", fmt::format("{}", lambda_response_body.length())}}; + + Http::TestResponseHeaderMapImpl expected_response_headers{{":status", "200"}, + {"content-type", "application/json"}, + {"x-amz-custom-header", "envoy,proxy"}}; + std::vector expected_response_cookies{"user=John", "session-id=1337"}; + constexpr auto expected_response_body = "my-bucket/123-small.jpg"; + runTest(request_headers, "" /*request_body*/, expected_json_request, lambda_response_headers, + lambda_response_body, expected_response_headers, expected_response_cookies, + expected_response_body); +} + } // namespace } // namespace Envoy diff --git a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc index bd8910ee5d45..2c00c40a975f 100644 --- a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc +++ b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc @@ -35,7 +35,7 @@ class AwsLambdaFilterTest : public ::testing::Test { void setupFilter(const FilterSettings& settings) { signer_ = std::make_shared>(); - filter_ = std::make_unique(settings, stats_, signer_); + filter_ = std::make_unique(settings, stats_, signer_, false); filter_->setDecoderFilterCallbacks(decoder_callbacks_); filter_->setEncoderFilterCallbacks(encoder_callbacks_); setupClusterMetadata(); @@ -86,6 +86,28 @@ TEST_F(AwsLambdaFilterTest, HeaderOnlyShouldContinue) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, encode_result); } +/** + * Cluster Metadata is not needed if the filter is loaded as upstream filter + */ +TEST_F(AwsLambdaFilterTest, ClusterMetadataIsNotNeededInUpstreamMode) { + signer_ = std::make_shared>(); + auto settings = FilterSettings{arn_, InvocationMode::Synchronous, true}; + filter_ = std::make_unique(settings, stats_, signer_, true); + filter_->setDecoderFilterCallbacks(decoder_callbacks_); + filter_->setEncoderFilterCallbacks(encoder_callbacks_); + // setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + EXPECT_CALL(*signer_, signEmptyPayload(An(), An())); + Http::TestRequestHeaderMapImpl input_headers; + const auto result = filter_->decodeHeaders(input_headers, true /*end_stream*/); + EXPECT_EQ("/2015-03-31/functions/arn:aws:lambda:us-west-2:1337:function:fun/invocations", + input_headers.getPathValue()); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, result); + + Http::TestResponseHeaderMapImpl response_headers; + const auto encode_result = filter_->encodeHeaders(response_headers, true /*end_stream*/); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, encode_result); +} + /** * If there's a per route config and the target cluster has the _wrong_ metadata, then skip the * filter. @@ -164,7 +186,7 @@ TEST_F(AwsLambdaFilterTest, DecodeDataRecordsPayloadSize) { FilterStats stats(generateStats("test", *store.rootScope())); signer_ = std::make_shared>(); - filter_ = std::make_unique(settings, stats, signer_); + filter_ = std::make_unique(settings, stats, signer_, false); filter_->setDecoderFilterCallbacks(decoder_callbacks_); // Payload diff --git a/test/extensions/filters/http/aws_lambda/config_test.cc b/test/extensions/filters/http/aws_lambda/config_test.cc index cbfc53d17f8f..af1e41ff328a 100644 --- a/test/extensions/filters/http/aws_lambda/config_test.cc +++ b/test/extensions/filters/http/aws_lambda/config_test.cc @@ -159,6 +159,14 @@ TEST(AwsLambdaFilterConfigTest, AsynchrnousPerRouteConfig) { EXPECT_EQ(InvocationMode::Asynchronous, filter_settings_ptr->invocationMode()); } +TEST(AwsLambdaFilterConfigTest, UpstreamFactoryTest) { + + auto* factory = + Registry::FactoryRegistry::getFactory( + "envoy.filters.http.aws_lambda"); + ASSERT_NE(factory, nullptr); +} + } // namespace } // namespace AwsLambdaFilter } // namespace HttpFilters From f4fdf28710885f7ad5cef6c7ee7c1294e508f724 Mon Sep 17 00:00:00 2001 From: imccarten1 Date: Tue, 6 Feb 2024 12:34:22 -0500 Subject: [PATCH 07/25] asyn_client: allow passing reference to router::retrypolicy to options (#31848) This allows making an AsyncClient call that uses the RetryPolicy from a Router::Route. --------- Signed-off-by: Isabel Mccarten --- envoy/http/async_client.h | 18 +++++++ source/common/http/BUILD | 4 ++ source/common/http/async_client_impl.cc | 28 +++++++--- source/common/http/async_client_impl.h | 1 + source/common/http/null_route_impl.h | 26 +++------- test/common/http/async_client_impl_test.cc | 59 ++++++++++++++++++---- 6 files changed, 101 insertions(+), 35 deletions(-) diff --git a/envoy/http/async_client.h b/envoy/http/async_client.h index 5da55ee23b6f..a3b1a7ac3aff 100644 --- a/envoy/http/async_client.h +++ b/envoy/http/async_client.h @@ -277,8 +277,21 @@ class AsyncClient { } // this should be done with setBufferedBodyForRetry=true ? + // The retry policy can be set as either a proto or Router::RetryPolicy but + // not both. If both formats of the options are set, the more recent call + // will overwrite the older one. StreamOptions& setRetryPolicy(const envoy::config::route::v3::RetryPolicy& p) { retry_policy = p; + parsed_retry_policy = nullptr; + return *this; + } + + // The retry policy can be set as either a proto or Router::RetryPolicy but + // not both. If both formats of the options are set, the more recent call + // will overwrite the older one. + StreamOptions& setRetryPolicy(const Router::RetryPolicy& p) { + parsed_retry_policy = &p; + retry_policy = absl::nullopt; return *this; } StreamOptions& setFilterConfig(Router::FilterConfig& config) { @@ -324,6 +337,7 @@ class AsyncClient { absl::optional buffer_limit_; absl::optional retry_policy; + const Router::RetryPolicy* parsed_retry_policy{nullptr}; OptRef filter_config_; @@ -367,6 +381,10 @@ class AsyncClient { StreamOptions::setRetryPolicy(p); return *this; } + RequestOptions& setRetryPolicy(const Router::RetryPolicy& p) { + StreamOptions::setRetryPolicy(p); + return *this; + } RequestOptions& setIsShadow(bool s) { StreamOptions::setIsShadow(s); return *this; diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 58ca10e930a3..444b77bcd70e 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -28,14 +28,18 @@ envoy_cc_library( "//envoy/http:header_map_interface", "//envoy/http:message_interface", "//envoy/router:context_interface", + "//envoy/router:router_interface", "//envoy/router:router_ratelimit_interface", "//envoy/router:shadow_writer_interface", "//envoy/ssl:connection_interface", "//envoy/stream_info:stream_info_interface", "//source/common/common:empty_string", "//source/common/common:linked_object", + "//source/common/protobuf:message_validator_lib", + "//source/common/router:config_lib", "//source/common/stream_info:stream_info_lib", "//source/common/tracing:http_tracer_lib", + "//source/common/upstream:retry_factory_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/route/v3:pkg_cc_proto", "@envoy_api//envoy/type/v3:pkg_cc_proto", diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 0ec47e45e26c..19a7e72adaba 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -1,16 +1,17 @@ #include "source/common/http/async_client_impl.h" -#include -#include #include #include -#include #include "envoy/config/core/v3/base.pb.h" +#include "envoy/router/router.h" #include "source/common/grpc/common.h" +#include "source/common/http/null_route_impl.h" #include "source/common/http/utility.h" +#include "source/common/protobuf/message_validator_impl.h" #include "source/common/tracing/http_tracer_impl.h" +#include "source/common/upstream/retry_factory.h" namespace Envoy { namespace Http { @@ -75,6 +76,19 @@ AsyncClient::Stream* AsyncClientImpl::start(AsyncClient::StreamCallbacks& callba return active_streams_.front().get(); } +std::unique_ptr +createRetryPolicy(AsyncClientImpl& parent, const AsyncClient::StreamOptions& options) { + if (options.retry_policy.has_value()) { + Upstream::RetryExtensionFactoryContextImpl factory_context(parent.singleton_manager_); + return std::make_unique( + options.retry_policy.value(), ProtobufMessage::getNullValidationVisitor(), factory_context); + } + if (options.parsed_retry_policy == nullptr) { + return std::make_unique(); + } + return nullptr; +} + AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCallbacks& callbacks, const AsyncClient::StreamOptions& options) : parent_(parent), stream_callbacks_(callbacks), stream_id_(parent.config_.random_.random()), @@ -82,9 +96,11 @@ AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCal parent.config_.async_stats_), stream_info_(Protocol::Http11, parent.dispatcher().timeSource(), nullptr), tracing_config_(Tracing::EgressConfig::get()), - route_(std::make_shared(parent_.cluster_->name(), parent_.singleton_manager_, - options.timeout, options.hash_policy, - options.retry_policy)), + retry_policy_(createRetryPolicy(parent, options)), + route_(std::make_shared( + parent_.cluster_->name(), + retry_policy_ != nullptr ? *retry_policy_.get() : *options.parsed_retry_policy, + options.timeout, options.hash_policy)), account_(options.account_), buffer_limit_(options.buffer_limit_), send_xff_(options.send_xff) { stream_info_.dynamicMetadata().MergeFrom(options.metadata); diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 1631e8383a61..645633adbdf2 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -275,6 +275,7 @@ class AsyncStreamImpl : public virtual AsyncClient::Stream, StreamInfo::StreamInfoImpl stream_info_; Tracing::NullSpan active_span_; const Tracing::Config& tracing_config_; + const std::unique_ptr retry_policy_; std::shared_ptr route_; uint32_t high_watermark_calls_{}; bool local_closed_{}; diff --git a/source/common/http/null_route_impl.h b/source/common/http/null_route_impl.h index 465f9bf3f4dc..d9c8f2a6fe40 100644 --- a/source/common/http/null_route_impl.h +++ b/source/common/http/null_route_impl.h @@ -88,23 +88,14 @@ struct NullPathMatchCriterion : public Router::PathMatchCriterion { struct RouteEntryImpl : public Router::RouteEntry { RouteEntryImpl( - const std::string& cluster_name, Singleton::Manager& singleton_manager, - const absl::optional& timeout, + const std::string& cluster_name, const absl::optional& timeout, const Protobuf::RepeatedPtrField& hash_policy, - const absl::optional& retry_policy) - : cluster_name_(cluster_name), timeout_(timeout) { + const Router::RetryPolicy& retry_policy) + : retry_policy_(retry_policy), cluster_name_(cluster_name), timeout_(timeout) { if (!hash_policy.empty()) { hash_policy_ = std::make_unique(hash_policy); } - if (retry_policy.has_value()) { - // ProtobufMessage::getStrictValidationVisitor() ? how often do we do this? - Upstream::RetryExtensionFactoryContextImpl factory_context(singleton_manager); - retry_policy_ = std::make_unique( - retry_policy.value(), ProtobufMessage::getNullValidationVisitor(), factory_context); - } else { - retry_policy_ = std::make_unique(); - } } // Router::RouteEntry @@ -139,7 +130,7 @@ struct RouteEntryImpl : public Router::RouteEntry { return Upstream::ResourcePriority::Default; } const Router::RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } - const Router::RetryPolicy& retryPolicy() const override { return *retry_policy_; } + const Router::RetryPolicy& retryPolicy() const override { return retry_policy_; } const Router::InternalRedirectPolicy& internalRedirectPolicy() const override { return internal_redirect_policy_; } @@ -198,7 +189,7 @@ struct RouteEntryImpl : public Router::RouteEntry { const Router::EarlyDataPolicy& earlyDataPolicy() const override { return *early_data_policy_; } std::unique_ptr hash_policy_; - std::unique_ptr retry_policy_; + const Router::RetryPolicy& retry_policy_; static const NullHedgePolicy hedge_policy_; static const NullRateLimitPolicy rate_limit_policy_; @@ -220,12 +211,11 @@ struct RouteEntryImpl : public Router::RouteEntry { }; struct NullRouteImpl : public Router::Route { - NullRouteImpl(const std::string cluster_name, Singleton::Manager& singleton_manager, + NullRouteImpl(const std::string cluster_name, const Router::RetryPolicy& retry_policy, const absl::optional& timeout = {}, const Protobuf::RepeatedPtrField& - hash_policy = {}, - const absl::optional& retry_policy = {}) - : route_entry_(cluster_name, singleton_manager, timeout, hash_policy, retry_policy) {} + hash_policy = {}) + : route_entry_(cluster_name, timeout, hash_policy, retry_policy) {} // Router::Route const Router::DirectResponseEntry* directResponseEntry() const override { return nullptr; } diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 1a56312eff56..1d706dbe24aa 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -1878,23 +1878,37 @@ TEST_F(AsyncClientImplTest, DumpState) { // Must not be in anonymous namespace for friend to work. class AsyncClientImplUnitTest : public AsyncClientImplTest { public: + Router::RetryPolicyImpl retry_policy_; std::unique_ptr route_impl_{new NullRouteImpl( - client_.cluster_->name(), client_.singleton_manager_, absl::nullopt, - Protobuf::RepeatedPtrField(), - absl::nullopt)}; + client_.cluster_->name(), retry_policy_, absl::nullopt, + Protobuf::RepeatedPtrField())}; + std::unique_ptr stream_{ + new Http::AsyncStreamImpl(client_, stream_callbacks_, AsyncClient::StreamOptions())}; NullVirtualHost vhost_; NullCommonConfig config_; - void setupRouteImpl(const std::string& yaml_config) { + void setProtoRetryPolicy(const std::string& yaml_config) { envoy::config::route::v3::RetryPolicy retry_policy; TestUtility::loadFromYaml(yaml_config, retry_policy); - route_impl_ = std::make_unique( - client_.cluster_->name(), client_.singleton_manager_, absl::nullopt, - Protobuf::RepeatedPtrField(), - std::move(retry_policy)); + stream_ = std::make_unique( + client_, stream_callbacks_, AsyncClient::StreamOptions().setRetryPolicy(retry_policy)); } + + void setRetryPolicy(const std::string& yaml_config) { + envoy::config::route::v3::RetryPolicy proto_policy; + + TestUtility::loadFromYaml(yaml_config, proto_policy); + Upstream::RetryExtensionFactoryContextImpl factory_context(client_.singleton_manager_); + retry_policy_ = Router::RetryPolicyImpl( + proto_policy, ProtobufMessage::getNullValidationVisitor(), factory_context); + + stream_ = std::make_unique( + client_, stream_callbacks_, AsyncClient::StreamOptions().setRetryPolicy(retry_policy_)); + } + + const Router::RouteEntry& getRouteFromStream() { return *(stream_->route_->routeEntry()); } }; // Test the extended fake route that AsyncClient uses. @@ -1928,7 +1942,7 @@ TEST_F(AsyncClientImplUnitTest, NullRouteImplInitTest) { EXPECT_EQ(nullptr, route_entry.tlsContextMatchCriteria()); } -TEST_F(AsyncClientImplUnitTest, RouteImplInitTestWithRetryPolicy) { +TEST_F(AsyncClientImplUnitTest, AsyncStreamImplInitTestWithProtoRetryPolicy) { const std::string yaml = R"EOF( per_try_timeout: 30s num_retries: 10 @@ -1938,9 +1952,32 @@ retry_on: 5xx,gateway-error,connect-failure,reset max_interval: 30s )EOF"; - setupRouteImpl(yaml); + setProtoRetryPolicy(yaml); - auto& route_entry = *(route_impl_->routeEntry()); + auto& route_entry = getRouteFromStream(); + + EXPECT_EQ(route_entry.retryPolicy().numRetries(), 10); + EXPECT_EQ(route_entry.retryPolicy().perTryTimeout(), std::chrono::seconds(30)); + EXPECT_EQ(Router::RetryPolicy::RETRY_ON_CONNECT_FAILURE | Router::RetryPolicy::RETRY_ON_5XX | + Router::RetryPolicy::RETRY_ON_GATEWAY_ERROR | Router::RetryPolicy::RETRY_ON_RESET, + route_entry.retryPolicy().retryOn()); + + EXPECT_EQ(route_entry.retryPolicy().baseInterval(), std::chrono::milliseconds(10)); + EXPECT_EQ(route_entry.retryPolicy().maxInterval(), std::chrono::seconds(30)); +} + +TEST_F(AsyncClientImplUnitTest, AsyncStreamImplInitTestWithRetryPolicy) { + const std::string yaml = R"EOF( +per_try_timeout: 30s +num_retries: 10 +retry_on: 5xx,gateway-error,connect-failure,reset +retry_back_off: + base_interval: 0.01s + max_interval: 30s +)EOF"; + + setRetryPolicy(yaml); + auto& route_entry = getRouteFromStream(); EXPECT_EQ(route_entry.retryPolicy().numRetries(), 10); EXPECT_EQ(route_entry.retryPolicy().perTryTimeout(), std::chrono::seconds(30)); From 9bbb4c8e8c6a48a4f3c067190b20ef470a71ade4 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 6 Feb 2024 13:28:25 -0500 Subject: [PATCH 08/25] mobile: fixing internal engine destructor (#32224) mobile: fixing internal engine 'destructor' Signed-off-by: Alyssa Wilk --- mobile/library/common/internal_engine.cc | 2 +- mobile/library/common/internal_engine.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mobile/library/common/internal_engine.cc b/mobile/library/common/internal_engine.cc index 3240169e7fc0..cb0caf553c8f 100644 --- a/mobile/library/common/internal_engine.cc +++ b/mobile/library/common/internal_engine.cc @@ -187,7 +187,7 @@ envoy_status_t InternalEngine::terminate() { bool InternalEngine::isTerminated() const { return terminated_; } -InternalEngine::InternalEngine() { +InternalEngine::~InternalEngine() { if (!terminated_) { terminate(); } diff --git a/mobile/library/common/internal_engine.h b/mobile/library/common/internal_engine.h index adcb1a825e44..7ec0f22c152b 100644 --- a/mobile/library/common/internal_engine.h +++ b/mobile/library/common/internal_engine.h @@ -29,7 +29,7 @@ class InternalEngine : public Logger::Loggable { /** * InternalEngine destructor. */ - InternalEngine(); + ~InternalEngine(); /** * Run the engine with the provided configuration. From 55b526597b9ed0fe08349e4a3b399d46672a0222 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Tue, 6 Feb 2024 12:36:01 -0600 Subject: [PATCH 09/25] mobile: Rename common/common directory (#32228) Signed-off-by: Fredy Wijaya --- mobile/library/common/BUILD | 2 +- mobile/library/common/common/system_helper.cc | 19 ------------------ .../cert_validator/platform_bridge/BUILD | 2 +- .../platform_bridge_cert_validator.cc | 2 +- mobile/library/common/http/BUILD | 2 +- mobile/library/common/http/client.cc | 2 +- mobile/library/common/internal_engine.h | 2 +- mobile/library/common/logger/BUILD | 20 +++++++++++++++++++ .../{common => logger}/logger_delegate.cc | 2 +- .../{common => logger}/logger_delegate.h | 0 .../library/common/{common => system}/BUILD | 15 -------------- .../default_system_helper.cc | 2 +- .../default_system_helper.h | 2 +- .../default_system_helper_android.cc | 2 +- .../default_system_helper_apple.cc | 2 +- mobile/library/common/system/system_helper.cc | 19 ++++++++++++++++++ .../common/{common => system}/system_helper.h | 0 mobile/test/common/{common => logger}/BUILD | 2 +- .../logger_delegate_test.cc | 2 +- mobile/test/common/mocks/common/BUILD | 2 +- mobile/test/common/mocks/common/mocks.h | 2 +- 21 files changed, 54 insertions(+), 49 deletions(-) delete mode 100644 mobile/library/common/common/system_helper.cc create mode 100644 mobile/library/common/logger/BUILD rename mobile/library/common/{common => logger}/logger_delegate.cc (97%) rename mobile/library/common/{common => logger}/logger_delegate.h (100%) rename mobile/library/common/{common => system}/BUILD (80%) rename mobile/library/common/{common => system}/default_system_helper.cc (91%) rename mobile/library/common/{common => system}/default_system_helper.h (92%) rename mobile/library/common/{common => system}/default_system_helper_android.cc (91%) rename mobile/library/common/{common => system}/default_system_helper_apple.cc (89%) create mode 100644 mobile/library/common/system/system_helper.cc rename mobile/library/common/{common => system}/system_helper.h (100%) rename mobile/test/common/{common => logger}/BUILD (88%) rename mobile/test/common/{common => logger}/logger_delegate_test.cc (99%) diff --git a/mobile/library/common/BUILD b/mobile/library/common/BUILD index c4d3455b6c95..b955b2db2d36 100644 --- a/mobile/library/common/BUILD +++ b/mobile/library/common/BUILD @@ -25,11 +25,11 @@ envoy_cc_library( deps = [ ":engine_common_lib", "//library/common/bridge:utility_lib", - "//library/common/common:logger_delegate_lib", "//library/common/data:utility_lib", "//library/common/event:provisional_dispatcher_lib", "//library/common/http:client_lib", "//library/common/http:header_utility_lib", + "//library/common/logger:logger_delegate_lib", "//library/common/network:connectivity_manager_lib", "//library/common/stats:utility_lib", "//library/common/types:c_types_lib", diff --git a/mobile/library/common/common/system_helper.cc b/mobile/library/common/common/system_helper.cc deleted file mode 100644 index 20e3f5616c3f..000000000000 --- a/mobile/library/common/common/system_helper.cc +++ /dev/null @@ -1,19 +0,0 @@ -#include "library/common/common/system_helper.h" - -#include "library/common/common/default_system_helper.h" - -#if defined(USE_ANDROID_SYSTEM_HELPER) -#include "library/common/common/default_system_helper_android.cc" -#elif defined(__APPLE__) -#include "library/common/common/default_system_helper_apple.cc" -#else -#include "library/common/common/default_system_helper.cc" -#endif - -namespace Envoy { - -SystemHelper* SystemHelper::instance_ = new DefaultSystemHelper(); - -SystemHelper& SystemHelper::getInstance() { return *instance_; } - -} // namespace Envoy diff --git a/mobile/library/common/extensions/cert_validator/platform_bridge/BUILD b/mobile/library/common/extensions/cert_validator/platform_bridge/BUILD index 652cafcb6be3..2b19aeb203f2 100644 --- a/mobile/library/common/extensions/cert_validator/platform_bridge/BUILD +++ b/mobile/library/common/extensions/cert_validator/platform_bridge/BUILD @@ -34,7 +34,7 @@ envoy_cc_library( deps = [ ":c_types_lib", ":platform_bridge_cc_proto", - "//library/common/common:system_helper_lib", + "//library/common/system:system_helper_lib", "@envoy//source/extensions/transport_sockets/tls/cert_validator:cert_validator_lib", ], ) diff --git a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc index 6e09a7f6a8c6..602c0b1e4213 100644 --- a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc +++ b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc @@ -4,8 +4,8 @@ #include #include -#include "library/common/common/system_helper.h" #include "library/common/data/utility.h" +#include "library/common/system/system_helper.h" namespace Envoy { namespace Extensions { diff --git a/mobile/library/common/http/BUILD b/mobile/library/common/http/BUILD index d62e943d4dd7..08eb555bca58 100644 --- a/mobile/library/common/http/BUILD +++ b/mobile/library/common/http/BUILD @@ -13,7 +13,6 @@ envoy_cc_library( deps = [ "//library/common/bridge:utility_lib", "//library/common/buffer:bridge_fragment_lib", - "//library/common/common:system_helper_lib", "//library/common/data:utility_lib", "//library/common/event:provisional_dispatcher_lib", "//library/common/extensions/filters/http/local_error:local_error_filter_lib", @@ -22,6 +21,7 @@ envoy_cc_library( "//library/common/network:connectivity_manager_lib", "//library/common/network:synthetic_address_lib", "//library/common/stream_info:extra_stream_info_lib", + "//library/common/system:system_helper_lib", "//library/common/types:c_types_lib", "@envoy//envoy/buffer:buffer_interface", "@envoy//envoy/common:scope_tracker_interface", diff --git a/mobile/library/common/http/client.cc b/mobile/library/common/http/client.cc index 42a983819b3b..b8b8e3153f12 100644 --- a/mobile/library/common/http/client.cc +++ b/mobile/library/common/http/client.cc @@ -8,11 +8,11 @@ #include "source/common/http/utility.h" #include "library/common/bridge/utility.h" -#include "library/common/common/system_helper.h" #include "library/common/data/utility.h" #include "library/common/http/header_utility.h" #include "library/common/http/headers.h" #include "library/common/stream_info/extra_stream_info.h" +#include "library/common/system/system_helper.h" namespace Envoy { namespace Http { diff --git a/mobile/library/common/internal_engine.h b/mobile/library/common/internal_engine.h index 7ec0f22c152b..0b45ae95bde1 100644 --- a/mobile/library/common/internal_engine.h +++ b/mobile/library/common/internal_engine.h @@ -7,9 +7,9 @@ #include "absl/base/call_once.h" #include "extension_registry.h" -#include "library/common/common/logger_delegate.h" #include "library/common/engine_common.h" #include "library/common/http/client.h" +#include "library/common/logger/logger_delegate.h" #include "library/common/network/connectivity_manager.h" #include "library/common/types/c_types.h" diff --git a/mobile/library/common/logger/BUILD b/mobile/library/common/logger/BUILD new file mode 100644 index 000000000000..4a0f61530b4c --- /dev/null +++ b/mobile/library/common/logger/BUILD @@ -0,0 +1,20 @@ +load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_mobile_package") + +licenses(["notice"]) # Apache 2 + +envoy_mobile_package() + +envoy_cc_library( + name = "logger_delegate_lib", + srcs = ["logger_delegate.cc"], + hdrs = ["logger_delegate.h"], + repository = "@envoy", + deps = [ + "//library/common/api:external_api_lib", + "//library/common/bridge:utility_lib", + "//library/common/data:utility_lib", + "//library/common/types:c_types_lib", + "@envoy//source/common/common:macros", + "@envoy//source/common/common:minimal_logger_lib", + ], +) diff --git a/mobile/library/common/common/logger_delegate.cc b/mobile/library/common/logger/logger_delegate.cc similarity index 97% rename from mobile/library/common/common/logger_delegate.cc rename to mobile/library/common/logger/logger_delegate.cc index 6d7928aee29f..abbf1bf32d5b 100644 --- a/mobile/library/common/common/logger_delegate.cc +++ b/mobile/library/common/logger/logger_delegate.cc @@ -1,4 +1,4 @@ -#include "library/common/common/logger_delegate.h" +#include "library/common/logger/logger_delegate.h" #include diff --git a/mobile/library/common/common/logger_delegate.h b/mobile/library/common/logger/logger_delegate.h similarity index 100% rename from mobile/library/common/common/logger_delegate.h rename to mobile/library/common/logger/logger_delegate.h diff --git a/mobile/library/common/common/BUILD b/mobile/library/common/system/BUILD similarity index 80% rename from mobile/library/common/common/BUILD rename to mobile/library/common/system/BUILD index 0378d11374e6..a8069f9eb773 100644 --- a/mobile/library/common/common/BUILD +++ b/mobile/library/common/system/BUILD @@ -61,18 +61,3 @@ envoy_cc_library( repository = "@envoy", deps = [], ) - -envoy_cc_library( - name = "logger_delegate_lib", - srcs = ["logger_delegate.cc"], - hdrs = ["logger_delegate.h"], - repository = "@envoy", - deps = [ - "//library/common/api:external_api_lib", - "//library/common/bridge:utility_lib", - "//library/common/data:utility_lib", - "//library/common/types:c_types_lib", - "@envoy//source/common/common:macros", - "@envoy//source/common/common:minimal_logger_lib", - ], -) diff --git a/mobile/library/common/common/default_system_helper.cc b/mobile/library/common/system/default_system_helper.cc similarity index 91% rename from mobile/library/common/common/default_system_helper.cc rename to mobile/library/common/system/default_system_helper.cc index 877d105203ea..15142762e204 100644 --- a/mobile/library/common/common/default_system_helper.cc +++ b/mobile/library/common/system/default_system_helper.cc @@ -1,4 +1,4 @@ -#include "library/common/common/default_system_helper.h" +#include "library/common/system/default_system_helper.h" namespace Envoy { diff --git a/mobile/library/common/common/default_system_helper.h b/mobile/library/common/system/default_system_helper.h similarity index 92% rename from mobile/library/common/common/default_system_helper.h rename to mobile/library/common/system/default_system_helper.h index 9bce1f061159..a9ef1d32e519 100644 --- a/mobile/library/common/common/default_system_helper.h +++ b/mobile/library/common/system/default_system_helper.h @@ -1,6 +1,6 @@ #pragma once -#include "library/common/common/system_helper.h" +#include "library/common/system/system_helper.h" namespace Envoy { diff --git a/mobile/library/common/common/default_system_helper_android.cc b/mobile/library/common/system/default_system_helper_android.cc similarity index 91% rename from mobile/library/common/common/default_system_helper_android.cc rename to mobile/library/common/system/default_system_helper_android.cc index f9f8b64319b0..5d205f0945d7 100644 --- a/mobile/library/common/common/default_system_helper_android.cc +++ b/mobile/library/common/system/default_system_helper_android.cc @@ -1,4 +1,4 @@ -#include "library/common/common/default_system_helper.h" +#include "library/common/system/default_system_helper.h" #include "library/jni/android_jni_utility.h" #include "library/jni/android_network_utility.h" diff --git a/mobile/library/common/common/default_system_helper_apple.cc b/mobile/library/common/system/default_system_helper_apple.cc similarity index 89% rename from mobile/library/common/common/default_system_helper_apple.cc rename to mobile/library/common/system/default_system_helper_apple.cc index 592c62ec4b9a..1facd039d248 100644 --- a/mobile/library/common/common/default_system_helper_apple.cc +++ b/mobile/library/common/system/default_system_helper_apple.cc @@ -1,5 +1,5 @@ -#include "library/common/common/default_system_helper.h" #include "library/common/network/apple_platform_cert_verifier.h" +#include "library/common/system/default_system_helper.h" namespace Envoy { diff --git a/mobile/library/common/system/system_helper.cc b/mobile/library/common/system/system_helper.cc new file mode 100644 index 000000000000..c47a09189e26 --- /dev/null +++ b/mobile/library/common/system/system_helper.cc @@ -0,0 +1,19 @@ +#include "library/common/system/system_helper.h" + +#include "library/common/system/default_system_helper.h" + +#if defined(USE_ANDROID_SYSTEM_HELPER) +#include "library/common/system/default_system_helper_android.cc" +#elif defined(__APPLE__) +#include "library/common/system/default_system_helper_apple.cc" +#else +#include "library/common/system/default_system_helper.cc" +#endif + +namespace Envoy { + +SystemHelper* SystemHelper::instance_ = new DefaultSystemHelper(); + +SystemHelper& SystemHelper::getInstance() { return *instance_; } + +} // namespace Envoy diff --git a/mobile/library/common/common/system_helper.h b/mobile/library/common/system/system_helper.h similarity index 100% rename from mobile/library/common/common/system_helper.h rename to mobile/library/common/system/system_helper.h diff --git a/mobile/test/common/common/BUILD b/mobile/test/common/logger/BUILD similarity index 88% rename from mobile/test/common/common/BUILD rename to mobile/test/common/logger/BUILD index 51c7cac5d5b9..7672721702a5 100644 --- a/mobile/test/common/common/BUILD +++ b/mobile/test/common/logger/BUILD @@ -10,8 +10,8 @@ envoy_cc_test( repository = "@envoy", deps = [ "//library/common/api:external_api_lib", - "//library/common/common:logger_delegate_lib", "//library/common/data:utility_lib", + "//library/common/logger:logger_delegate_lib", "//library/common/types:c_types_lib", ], ) diff --git a/mobile/test/common/common/logger_delegate_test.cc b/mobile/test/common/logger/logger_delegate_test.cc similarity index 99% rename from mobile/test/common/common/logger_delegate_test.cc rename to mobile/test/common/logger/logger_delegate_test.cc index a8e3997ab0c2..a907acfb5121 100644 --- a/mobile/test/common/common/logger_delegate_test.cc +++ b/mobile/test/common/logger/logger_delegate_test.cc @@ -1,8 +1,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "library/common/api/external.h" -#include "library/common/common/logger_delegate.h" #include "library/common/data/utility.h" +#include "library/common/logger/logger_delegate.h" using testing::_; using testing::HasSubstr; diff --git a/mobile/test/common/mocks/common/BUILD b/mobile/test/common/mocks/common/BUILD index e9f4f1c71750..a7632fd70869 100644 --- a/mobile/test/common/mocks/common/BUILD +++ b/mobile/test/common/mocks/common/BUILD @@ -14,6 +14,6 @@ envoy_cc_mock( hdrs = ["mocks.h"], repository = "@envoy", deps = [ - "//library/common/common:system_helper_lib", + "//library/common/system:system_helper_lib", ], ) diff --git a/mobile/test/common/mocks/common/mocks.h b/mobile/test/common/mocks/common/mocks.h index b1451495aa4d..f6825405b9c7 100644 --- a/mobile/test/common/mocks/common/mocks.h +++ b/mobile/test/common/mocks/common/mocks.h @@ -1,7 +1,7 @@ #pragma once #include "gmock/gmock.h" -#include "library/common/common/system_helper.h" +#include "library/common/system/system_helper.h" namespace Envoy { namespace test { From e5087acbb79d57875916e6abc7992c6f1fd84748 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Tue, 6 Feb 2024 14:02:39 -0500 Subject: [PATCH 10/25] Remove fallback from accept4 to accept (#32227) * Remove fallback from accept4 to accept Signed-off-by: Yan Avlasov * Address comments Signed-off-by: Yan Avlasov --------- Signed-off-by: Yan Avlasov --- source/common/api/posix/os_sys_calls_impl.cc | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/source/common/api/posix/os_sys_calls_impl.cc b/source/common/api/posix/os_sys_calls_impl.cc index 7df7e6384e90..3fab0afc1b55 100644 --- a/source/common/api/posix/os_sys_calls_impl.cc +++ b/source/common/api/posix/os_sys_calls_impl.cc @@ -325,17 +325,12 @@ SysCallSocketResult OsSysCallsImpl::accept(os_fd_t sockfd, sockaddr* addr, sockl #if defined(__linux__) rc = ::accept4(sockfd, addr, addrlen, SOCK_NONBLOCK); - // If failed with EINVAL try without flags - if (rc >= 0 || errno != EINVAL) { - return {rc, rc != -1 ? 0 : errno}; - } -#endif - +#else rc = ::accept(sockfd, addr, addrlen); if (rc >= 0) { setsocketblocking(rc, false); } - +#endif return {rc, rc != -1 ? 0 : errno}; } From 9ecb0a4a04e37c5bf8c3424fae07eb96e724f683 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 6 Feb 2024 14:11:01 -0500 Subject: [PATCH 11/25] alpn: adding a connection fail test (#31965) Signed-off-by: Alyssa Wilk --- test/integration/BUILD | 1 + test/integration/alpn_integration_test.cc | 29 +++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/test/integration/BUILD b/test/integration/BUILD index 4649014f41af..caa4b6b2a368 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -82,6 +82,7 @@ envoy_cc_test( name = "alpn_integration_test", size = "large", srcs = ["alpn_integration_test.cc"], + shard_count = 2, tags = [ "cpu:3", ], diff --git a/test/integration/alpn_integration_test.cc b/test/integration/alpn_integration_test.cc index dada4bfe73db..d15d590332df 100644 --- a/test/integration/alpn_integration_test.cc +++ b/test/integration/alpn_integration_test.cc @@ -185,5 +185,34 @@ TEST_P(AlpnIntegrationTest, Mixed) { EXPECT_EQ("200", response2->headers().Status()->value().getStringView()); } +TEST_P(AlpnIntegrationTest, DisconnectDuringHandshake) { + DISABLE_UNDER_WINDOWS; + setUpstreamProtocol(Http::CodecType::HTTP2); + protocols_ = {Http::CodecType::HTTP2}; + setUpstreamCount(1); + initialize(); + + absl::Notification unblock_accept; + absl::Notification accept_blocked; + fake_upstreams_[0]->runOnDispatcherThread([&] { + accept_blocked.Notify(); + unblock_accept.WaitForNotification(); + }); + accept_blocked.WaitForNotification(); + + // Connect and wait for the upstream connection to be established. + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_total", 1); + + // Close the downstream connection and wait for the upstream stream to go away. + codec_client_->close(); + test_server_->waitForCounterEq("cluster.cluster_0.upstream_rq_cancelled", 1); + + // Allow the connection to complete. + unblock_accept.Notify(); + test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_http2_total", 1); +} + } // namespace } // namespace Envoy From 0a54177e1c3b0c73a9261e4117cb836921fa52d9 Mon Sep 17 00:00:00 2001 From: birenroy Date: Tue, 6 Feb 2024 16:11:10 -0500 Subject: [PATCH 12/25] test: increases a test timeout to avoid flaky failures (#32232) Increases the timeout for the connection close with deferred streams test. Signed-off-by: Biren Roy --- test/integration/multiplexed_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 9d2e36ac426d..88b45501e6f2 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -2597,7 +2597,7 @@ TEST_P(Http2FrameIntegrationTest, CloseConnectionWithDeferredStreams) { // Test that Envoy can clean-up deferred streams // Make the timeout longer to accommodate non optimized builds test_server_->waitForCounterEq("http.config_test.downstream_rq_rx_reset", kRequestsSentPerIOCycle, - TestUtility::DefaultTimeout * 3); + TestUtility::DefaultTimeout * 10); } INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest, From 77f9577c26d675d921759cc51ae8bc3db92dd2e5 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Tue, 6 Feb 2024 16:17:45 -0500 Subject: [PATCH 13/25] [test]: Skip test function if test input is empty (#32201) This fix addresses an issue with a test only ASSERT that occurs when the test input is empty, i.e. there is nothing to replay. Signed-off-by: Yan Avlasov --- test/integration/h1_capture_fuzz_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/integration/h1_capture_fuzz_test.cc b/test/integration/h1_capture_fuzz_test.cc index f5c43377895a..40bb21abd13d 100644 --- a/test/integration/h1_capture_fuzz_test.cc +++ b/test/integration/h1_capture_fuzz_test.cc @@ -4,6 +4,9 @@ namespace Envoy { void H1FuzzIntegrationTest::initialize() { HttpIntegrationTest::initialize(); } DEFINE_PROTO_FUZZER(const test::integration::CaptureFuzzTestCase& input) { + if (input.events_size() == 0) { + return; + } // Pick an IP version to use for loopback, it doesn't matter which. RELEASE_ASSERT(!TestEnvironment::getIpVersionsForTest().empty(), ""); const auto ip_version = TestEnvironment::getIpVersionsForTest()[0]; From 131afeb72fa5484004b60a6af3d6336361432fd8 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Wed, 7 Feb 2024 04:53:49 -0500 Subject: [PATCH 14/25] deps: Bump `com_github_grpc_grpc` from 1.59.3 -> 1.59.4 (#32235) Signed-off-by: Tony Allen --- bazel/repository_locations.bzl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 6ab8f8529fa9..a791b08f127c 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -376,12 +376,12 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "gRPC", project_desc = "gRPC C core library", project_url = "https://grpc.io", - version = "1.59.3", - sha256 = "ea281bb3489520ad4fb96ae84b45ed194a1f0b944d3e74f925f5e019d31ecd0f", + version = "1.59.4", + sha256 = "6edc67c2ad200c5b618c421f6e8c1b734a4aa3e741975e683491da03390ebf63", strip_prefix = "grpc-{version}", urls = ["https://github.com/grpc/grpc/archive/v{version}.tar.gz"], use_category = ["dataplane_core", "controlplane"], - release_date = "2023-11-17", + release_date = "2024-02-05", cpe = "cpe:2.3:a:grpc:grpc:*", license = "Apache-2.0", license_url = "https://github.com/grpc/grpc/blob/v{version}/LICENSE", From 5fede422222279a24d9494af395335fa73648e08 Mon Sep 17 00:00:00 2001 From: phlax Date: Wed, 7 Feb 2024 16:26:23 +0000 Subject: [PATCH 15/25] deps: Bump py `cryptography` (#32209) Signed-off-by: Ryan Northey --- tools/base/requirements.txt | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index cfda8b641e01..7728ac706291 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -2,7 +2,7 @@ # This file is autogenerated by pip-compile with Python 3.11 # by the following command: # -# pip-compile --allow-unsafe --generate-hashes requirements.in +# pip-compile --allow-unsafe --generate-hashes # abstracts==0.0.12 \ --hash=sha256:acc01ff56c8a05fb88150dff62e295f9071fc33388c42f1dfc2787a8d1c755ff @@ -691,9 +691,7 @@ google-apitools==0.5.32 \ google-auth[aiohttp]==2.26.1 \ --hash=sha256:2c8b55e3e564f298122a02ab7b97458ccfcc5617840beb5d0ac757ada92c9780 \ --hash=sha256:54385acca5c0fbdda510cd8585ba6f3fcb06eeecf8a6ecca39d3ee148b092590 - # via - # google-auth - # gsutil + # via gsutil google-reauth==0.1.1 \ --hash=sha256:cb39074488d74c8853074dde47368bbf8f739d4a4338b89aab696c895b6d8368 \ --hash=sha256:f9f6852a55c2c5453d581cd01f3d1278e86147c03d008409800390a834235892 @@ -1054,7 +1052,6 @@ pyjwt[crypto]==2.8.0 \ # via # gidgethub # pygithub - # pyjwt pylsqpack==0.3.18 \ --hash=sha256:005ddce84bdcbf5c3cf99f764504208e1aa0a91a8331bf47108f2708f2a315e6 \ --hash=sha256:06e1bbe47514b83cd03158e5558ef8cc44f578169c1820098be9f3cc4137f16a \ From c18d5c44ef5758944bbb30f436ce0fba7eeebebd Mon Sep 17 00:00:00 2001 From: Xie Zhihao Date: Thu, 8 Feb 2024 00:33:58 +0800 Subject: [PATCH 16/25] io_uring: add io_uring worker factory (#31944) Signed-off-by: Xie Zhihao --- envoy/common/io/io_uring.h | 22 ++++++---- source/common/io/BUILD | 11 +++++ source/common/io/io_uring_impl.cc | 16 -------- source/common/io/io_uring_impl.h | 15 ------- .../common/io/io_uring_worker_factory_impl.cc | 39 ++++++++++++++++++ .../common/io/io_uring_worker_factory_impl.h | 29 +++++++++++++ test/common/io/BUILD | 20 ++++++++- test/common/io/io_uring_impl_test.cc | 9 +--- .../io/io_uring_worker_factory_impl_test.cc | 41 +++++++++++++++++++ test/mocks/io/mocks.h | 7 ++++ 10 files changed, 161 insertions(+), 48 deletions(-) create mode 100644 source/common/io/io_uring_worker_factory_impl.cc create mode 100644 source/common/io/io_uring_worker_factory_impl.h create mode 100644 test/common/io/io_uring_worker_factory_impl_test.cc diff --git a/envoy/common/io/io_uring.h b/envoy/common/io/io_uring.h index 5b2b66011d55..335b8d4c6b06 100644 --- a/envoy/common/io/io_uring.h +++ b/envoy/common/io/io_uring.h @@ -450,23 +450,27 @@ class IoUringWorker : public ThreadLocal::ThreadLocalObject { }; /** - * Abstract factory for IoUring wrappers. + * Abstract factory for IoUringWorker wrappers. */ -class IoUringFactory { +class IoUringWorkerFactory { public: - virtual ~IoUringFactory() = default; + virtual ~IoUringWorkerFactory() = default; /** - * Returns an instance of IoUring and creates it if needed for the current - * thread. + * Returns the current thread's IoUringWorker. If the thread have not registered a IoUringWorker, + * an absl::nullopt will be returned. */ - virtual IoUring& getOrCreate() const PURE; + virtual OptRef getIoUringWorker() PURE; /** - * Initializes a factory upon server readiness. For example this method can be - * used to set TLS. + * Initializes a IoUringWorkerFactory upon server readiness. The method is used to set the TLS. */ - virtual void onServerInitialized() PURE; + virtual void onWorkerThreadInitialized() PURE; + + /** + * Indicates whether the current thread has been registered for a IoUringWorker. + */ + virtual bool currentThreadRegistered() PURE; }; } // namespace Io diff --git a/source/common/io/BUILD b/source/common/io/BUILD index 1db6f132c481..c3d8166152ac 100644 --- a/source/common/io/BUILD +++ b/source/common/io/BUILD @@ -40,3 +40,14 @@ envoy_cc_library( "//source/common/common:linked_object", ], ) + +envoy_cc_library( + name = "io_uring_worker_factory_impl_lib", + srcs = ["io_uring_worker_factory_impl.cc"], + hdrs = ["io_uring_worker_factory_impl.h"], + deps = [ + ":io_uring_worker_lib", + "//envoy/common/io:io_uring_interface", + "//envoy/thread_local:thread_local_interface", + ], +) diff --git a/source/common/io/io_uring_impl.cc b/source/common/io/io_uring_impl.cc index f5fddf9db681..8096646c3fff 100644 --- a/source/common/io/io_uring_impl.cc +++ b/source/common/io/io_uring_impl.cc @@ -17,22 +17,6 @@ bool isIoUringSupported() { return is_supported; } -IoUringFactoryImpl::IoUringFactoryImpl(uint32_t io_uring_size, bool use_submission_queue_polling, - ThreadLocal::SlotAllocator& tls) - : io_uring_size_(io_uring_size), use_submission_queue_polling_(use_submission_queue_polling), - tls_(tls) {} - -IoUring& IoUringFactoryImpl::getOrCreate() const { - return const_cast(tls_.get().ref()); -} - -void IoUringFactoryImpl::onServerInitialized() { - tls_.set([io_uring_size = io_uring_size_, - use_submission_queue_polling = use_submission_queue_polling_](Event::Dispatcher&) { - return std::make_shared(io_uring_size, use_submission_queue_polling); - }); -} - IoUringImpl::IoUringImpl(uint32_t io_uring_size, bool use_submission_queue_polling) : cqes_(io_uring_size, nullptr) { struct io_uring_params p {}; diff --git a/source/common/io/io_uring_impl.h b/source/common/io/io_uring_impl.h index c5bb1ef1591c..196ba0d942a0 100644 --- a/source/common/io/io_uring_impl.h +++ b/source/common/io/io_uring_impl.h @@ -54,20 +54,5 @@ class IoUringImpl : public IoUring, std::list injected_completions_; }; -class IoUringFactoryImpl : public IoUringFactory { -public: - IoUringFactoryImpl(uint32_t io_uring_size, bool use_submission_queue_polling, - ThreadLocal::SlotAllocator& tls); - - // IoUringFactory - IoUring& getOrCreate() const override; - void onServerInitialized() override; - -private: - const uint32_t io_uring_size_{}; - const bool use_submission_queue_polling_{}; - ThreadLocal::TypedSlot tls_; -}; - } // namespace Io } // namespace Envoy diff --git a/source/common/io/io_uring_worker_factory_impl.cc b/source/common/io/io_uring_worker_factory_impl.cc new file mode 100644 index 000000000000..b667032d32f2 --- /dev/null +++ b/source/common/io/io_uring_worker_factory_impl.cc @@ -0,0 +1,39 @@ +#include "source/common/io/io_uring_worker_factory_impl.h" + +#include "source/common/io/io_uring_worker_impl.h" + +namespace Envoy { +namespace Io { + +IoUringWorkerFactoryImpl::IoUringWorkerFactoryImpl(uint32_t io_uring_size, + bool use_submission_queue_polling, + uint32_t read_buffer_size, + uint32_t write_timeout_ms, + ThreadLocal::SlotAllocator& tls) + : io_uring_size_(io_uring_size), use_submission_queue_polling_(use_submission_queue_polling), + read_buffer_size_(read_buffer_size), write_timeout_ms_(write_timeout_ms), tls_(tls) {} + +OptRef IoUringWorkerFactoryImpl::getIoUringWorker() { + auto ret = tls_.get(); + if (ret == absl::nullopt) { + return absl::nullopt; + } + return ret; +} + +void IoUringWorkerFactoryImpl::onWorkerThreadInitialized() { + tls_.set([io_uring_size = io_uring_size_, + use_submission_queue_polling = use_submission_queue_polling_, + read_buffer_size = read_buffer_size_, + write_timeout_ms = write_timeout_ms_](Event::Dispatcher& dispatcher) { + return std::make_shared(io_uring_size, use_submission_queue_polling, + read_buffer_size, write_timeout_ms, dispatcher); + }); +} + +bool IoUringWorkerFactoryImpl::currentThreadRegistered() { + return !tls_.isShutdown() && tls_.currentThreadRegistered(); +} + +} // namespace Io +} // namespace Envoy diff --git a/source/common/io/io_uring_worker_factory_impl.h b/source/common/io/io_uring_worker_factory_impl.h new file mode 100644 index 000000000000..03f5f6644354 --- /dev/null +++ b/source/common/io/io_uring_worker_factory_impl.h @@ -0,0 +1,29 @@ +#pragma once + +#include "envoy/common/io/io_uring.h" +#include "envoy/thread_local/thread_local.h" + +namespace Envoy { +namespace Io { + +class IoUringWorkerFactoryImpl : public IoUringWorkerFactory { +public: + IoUringWorkerFactoryImpl(uint32_t io_uring_size, bool use_submission_queue_polling, + uint32_t read_buffer_size, uint32_t write_timeout_ms, + ThreadLocal::SlotAllocator& tls); + + OptRef getIoUringWorker() override; + + void onWorkerThreadInitialized() override; + bool currentThreadRegistered() override; + +private: + const uint32_t io_uring_size_; + const bool use_submission_queue_polling_; + const uint32_t read_buffer_size_; + const uint32_t write_timeout_ms_; + ThreadLocal::TypedSlot tls_; +}; + +} // namespace Io +} // namespace Envoy diff --git a/test/common/io/BUILD b/test/common/io/BUILD index 366fdaeba4ec..2a51bf73c945 100644 --- a/test/common/io/BUILD +++ b/test/common/io/BUILD @@ -19,7 +19,6 @@ envoy_cc_test( "//source/common/io:io_uring_impl_lib", "//source/common/network:address_lib", "//test/mocks/io:io_mocks", - "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", "//test/test_common:utility_lib", ], @@ -60,3 +59,22 @@ envoy_cc_test( "//conditions:default": [], }), ) + +envoy_cc_test( + name = "io_uring_worker_factory_impl_test", + srcs = select({ + "//bazel:linux": ["io_uring_worker_factory_impl_test.cc"], + "//conditions:default": [], + }), + deps = [ + "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", + "//test/test_common:utility_lib", + ] + select({ + "//bazel:linux": [ + "//source/common/io:io_uring_impl_lib", + "//source/common/io:io_uring_worker_factory_impl_lib", + ], + "//conditions:default": [], + }), +) diff --git a/test/common/io/io_uring_impl_test.cc b/test/common/io/io_uring_impl_test.cc index 303c870bf3f9..b357b60df93b 100644 --- a/test/common/io/io_uring_impl_test.cc +++ b/test/common/io/io_uring_impl_test.cc @@ -2,7 +2,6 @@ #include "source/common/network/address_impl.h" #include "test/mocks/io/mocks.h" -#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" @@ -26,9 +25,7 @@ class IoUringImplTest : public ::testing::Test { public: IoUringImplTest() : api_(Api::createApiForTest()), should_skip_(!isIoUringSupported()) { if (!should_skip_) { - factory_ = std::make_unique(2, false, context_.threadLocal()); - factory_->onServerInitialized(); - io_uring_ = factory_->getOrCreate(); + io_uring_ = std::make_unique(2, false); } } @@ -49,9 +46,7 @@ class IoUringImplTest : public ::testing::Test { } Api::ApiPtr api_; - testing::NiceMock context_; - std::unique_ptr factory_{}; - OptRef io_uring_{}; + IoUringPtr io_uring_{}; const bool should_skip_{}; }; diff --git a/test/common/io/io_uring_worker_factory_impl_test.cc b/test/common/io/io_uring_worker_factory_impl_test.cc new file mode 100644 index 000000000000..f6eebbc66444 --- /dev/null +++ b/test/common/io/io_uring_worker_factory_impl_test.cc @@ -0,0 +1,41 @@ +#include "source/common/io/io_uring_impl.h" +#include "source/common/io/io_uring_worker_factory_impl.h" + +#include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Io { +namespace { + +class IoUringWorkerFactoryImplTest : public ::testing::Test { +public: + IoUringWorkerFactoryImplTest() + : api_(Api::createApiForTest()), should_skip_(!isIoUringSupported()) {} + + void SetUp() override { + if (should_skip_) { + GTEST_SKIP(); + } + } + + Api::ApiPtr api_; + testing::NiceMock context_; + std::unique_ptr factory_{}; + bool should_skip_{}; +}; + +TEST_F(IoUringWorkerFactoryImplTest, Basic) { + IoUringWorkerFactoryImpl factory(2, false, 8192, 1000, context_.threadLocal()); + EXPECT_TRUE(factory.currentThreadRegistered()); + auto dispatcher = api_->allocateDispatcher("test_thread"); + factory.onWorkerThreadInitialized(); + EXPECT_TRUE(factory.getIoUringWorker().has_value()); +} + +} // namespace +} // namespace Io +} // namespace Envoy diff --git a/test/mocks/io/mocks.h b/test/mocks/io/mocks.h index 9c7a6560102c..4f7536485135 100644 --- a/test/mocks/io/mocks.h +++ b/test/mocks/io/mocks.h @@ -80,5 +80,12 @@ class MockIoUringWorker : public IoUringWorker { MOCK_METHOD(uint32_t, getNumOfSockets, (), (const)); }; +class MockIoUringWorkerFactory : public IoUringWorkerFactory { +public: + MOCK_METHOD(OptRef, getIoUringWorker, ()); + MOCK_METHOD(void, onWorkerThreadInitialized, ()); + MOCK_METHOD(bool, currentThreadRegistered, ()); +}; + } // namespace Io } // namespace Envoy From 85949b2c85252d852a409079e99b1d28e1d87a52 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Wed, 7 Feb 2024 12:57:39 -0500 Subject: [PATCH 17/25] Link test binaries dynamically for coverage builds (#32254) Dynamic linking prevents build failure due to size of coverage instrumentation. Signed-off-by: Yan Avlasov --- bazel/envoy_binary.bzl | 5 +++-- bazel/envoy_build_system.bzl | 5 +++++ bazel/envoy_test.bzl | 2 ++ test/integration/BUILD | 2 ++ test/integration/run_envoy_test.sh | 2 +- test/tools/router_check/BUILD | 2 ++ 6 files changed, 15 insertions(+), 3 deletions(-) diff --git a/bazel/envoy_binary.bzl b/bazel/envoy_binary.bzl index 58343f8bb322..e9038a522f14 100644 --- a/bazel/envoy_binary.bzl +++ b/bazel/envoy_binary.bzl @@ -25,7 +25,8 @@ def envoy_cc_binary( deps = [], linkopts = [], tags = [], - features = []): + features = [], + linkstatic = True): linker_inputs = envoy_exported_symbols_input() if not linkopts: @@ -43,7 +44,7 @@ def envoy_cc_binary( copts = envoy_copts(repository), linkopts = linkopts, testonly = testonly, - linkstatic = 1, + linkstatic = linkstatic, visibility = visibility, malloc = tcmalloc_external_dep(repository), stamp = stamp, diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl index 93910375b755..90fd023733b2 100644 --- a/bazel/envoy_build_system.bzl +++ b/bazel/envoy_build_system.bzl @@ -53,6 +53,10 @@ load( _envoy_py_test_binary = "envoy_py_test_binary", _envoy_sh_test = "envoy_sh_test", ) +load( + ":envoy_internal.bzl", + _envoy_linkstatic = "envoy_linkstatic", +) load( ":envoy_mobile_defines.bzl", _envoy_mobile_defines = "envoy_mobile_defines", @@ -250,6 +254,7 @@ envoy_select_wasm_v8 = _envoy_select_wasm_v8 envoy_select_wasm_wamr = _envoy_select_wasm_wamr envoy_select_wasm_wavm = _envoy_select_wasm_wavm envoy_select_wasm_wasmtime = _envoy_select_wasm_wasmtime +envoy_select_linkstatic = _envoy_linkstatic # Binary wrappers (from envoy_binary.bzl) envoy_cc_binary = _envoy_cc_binary diff --git a/bazel/envoy_test.bzl b/bazel/envoy_test.bzl index ad05121a9922..73908fc6f1b3 100644 --- a/bazel/envoy_test.bzl +++ b/bazel/envoy_test.bzl @@ -228,6 +228,7 @@ def envoy_cc_test_binary( tags = [], deps = [], stamp = 0, + linkstatic = True, **kargs): envoy_cc_binary( name, @@ -238,6 +239,7 @@ def envoy_cc_test_binary( "@envoy//test/test_common:test_version_linkstamp", ], stamp = stamp, + linkstatic = linkstatic, **kargs ) diff --git a/test/integration/BUILD b/test/integration/BUILD index caa4b6b2a368..b29e7d22c976 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -11,6 +11,7 @@ load( "envoy_select_enable_http3", "envoy_select_enable_yaml", "envoy_select_envoy_mobile_listener", + "envoy_select_linkstatic", "envoy_sh_test", ) @@ -364,6 +365,7 @@ envoy_cc_test_binary( external_deps = [ "abseil_symbolize", ], + linkstatic = envoy_select_linkstatic(), deps = [ "//source/exe:main_common_lib", "//source/exe:platform_impl_lib", diff --git a/test/integration/run_envoy_test.sh b/test/integration/run_envoy_test.sh index 1c253394d71b..18752c07c8b1 100755 --- a/test/integration/run_envoy_test.sh +++ b/test/integration/run_envoy_test.sh @@ -52,6 +52,6 @@ expect_ok -c "${TEST_SRCDIR}/envoy/test/config/integration/empty.yaml" --mode va start_test "Launching envoy with empty config." run_in_background_saving_pid "${ENVOY_BIN}" -c "${TEST_SRCDIR}/envoy/test/config/integration/empty.yaml" --use-dynamic-base-id -sleep 3 +sleep 120 kill "${BACKGROUND_PID}" wait "${BACKGROUND_PID}" diff --git a/test/tools/router_check/BUILD b/test/tools/router_check/BUILD index 360c9193e7a2..9edc25e55b49 100644 --- a/test/tools/router_check/BUILD +++ b/test/tools/router_check/BUILD @@ -4,6 +4,7 @@ load( "envoy_cc_test_library", "envoy_package", "envoy_proto_library", + "envoy_select_linkstatic", ) load("//bazel:repositories.bzl", "PPC_SKIP_TARGETS", "WINDOWS_SKIP_TARGETS") load("//source/extensions:all_extensions.bzl", "envoy_all_extensions") @@ -15,6 +16,7 @@ envoy_package() envoy_cc_test_binary( name = "router_check_tool", srcs = ["router_check.cc"], + linkstatic = envoy_select_linkstatic(), deps = [":router_check_main_lib"] + select({ "//bazel:coverage_build": [], "//bazel:windows_x86_64": envoy_all_extensions(WINDOWS_SKIP_TARGETS), From 15f199651348e486f08762fe62bfe3bda1c4c508 Mon Sep 17 00:00:00 2001 From: danzh Date: Wed, 7 Feb 2024 13:04:17 -0500 Subject: [PATCH 18/25] quiche: implement platform API QuicheScopedDisableExitOnDFatal (#32172) Commit Message: add an impl for the new API and use the new API in test/common/http/codec_impl_fuzz_test.cc. Risk Level: low, fuzz test change without behavioral change. Testing: existing tests passed Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Dan Zhang --- test/common/http/BUILD | 1 + test/common/http/codec_impl_fuzz_test.cc | 4 ++-- .../listener_manager_impl_quic_only_test.cc | 1 + test/common/quic/active_quic_listener_test.cc | 7 +++++++ test/common/quic/platform/quic_platform_test.cc | 6 ++++++ test/common/quic/platform/quiche_test_impl.h | 11 +++++++++-- test/common/quic/quic_io_handle_wrapper_test.cc | 3 +++ test/integration/filters/test_listener_filter.h | 2 ++ 8 files changed, 31 insertions(+), 4 deletions(-) diff --git a/test/common/http/BUILD b/test/common/http/BUILD index d3fce341694f..5e0c49b231ba 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -114,6 +114,7 @@ envoy_proto_library( "//test/mocks/network:network_mocks", "//test/mocks/server:overload_manager_mocks", "//test/test_common:test_runtime_lib", + "@com_github_google_quiche//:quiche_common_platform_test", ], ) for http_protocol in [ diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index 046dfd7fd9c2..8554be417b01 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -28,7 +28,7 @@ #include "gmock/gmock.h" -#include "quiche/common/platform/api/quiche_logging.h" +#include "quiche/common/platform/api/quiche_test.h" using testing::_; using testing::Invoke; @@ -833,7 +833,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::CodecImplFuzzTestCase& input) { // inconsistent state (and crashes/accesses inconsistent memory), then it will be a bug we'll // need to further evaluate. However, in fuzzing we allow oghttp2 reaching FATAL states that may // happen in production environments. - quiche::setDFatalExitDisabled(true); + quiche::test::QuicheScopedDisableExitOnDFatal scoped_object; codecFuzzHttp2Oghttp2(input); #endif } catch (const EnvoyException& e) { diff --git a/test/common/listener_manager/listener_manager_impl_quic_only_test.cc b/test/common/listener_manager/listener_manager_impl_quic_only_test.cc index 23a7d340853b..67b334f46ddd 100644 --- a/test/common/listener_manager/listener_manager_impl_quic_only_test.cc +++ b/test/common/listener_manager/listener_manager_impl_quic_only_test.cc @@ -403,6 +403,7 @@ TEST_P(ListenerManagerImplQuicOnlyTest, QuicListenerFilterFromConfig) { ASSERT_NE(nullptr, listener_filter); Network::MockListenerFilterCallbacks callbacks; EXPECT_CALL(callbacks, filterState()); + EXPECT_CALL(callbacks, dispatcher()).WillOnce(ReturnRef(server_.dispatcher_)); listener_filter->onAccept(callbacks); auto* added_filter_state = callbacks.filter_state_.getDataReadOnly( diff --git a/test/common/quic/active_quic_listener_test.cc b/test/common/quic/active_quic_listener_test.cc index edf8b6873b12..6c58ae781cf4 100644 --- a/test/common/quic/active_quic_listener_test.cc +++ b/test/common/quic/active_quic_listener_test.cc @@ -365,6 +365,13 @@ INSTANTIATE_TEST_SUITE_P(ActiveQuicListenerTests, ActiveQuicListenerTest, TEST_P(ActiveQuicListenerTest, ReceiveCHLO) { initialize(); +#if UDP_GSO_BATCH_WRITER_COMPILETIME_SUPPORT + EXPECT_TRUE(quic_listener_->udpPacketWriter().isBatchMode()); +#else + EXPECT_FALSE(quic_listener_->udpPacketWriter().isBatchMode()); +#endif + // The listener ignores read error. + quic_listener_->onReceiveError(Api::IoError::IoErrorCode::InvalidArgument); quic::QuicBufferedPacketStore* const buffered_packets = quic::test::QuicDispatcherPeer::GetBufferedPackets(quic_dispatcher_); maybeConfigureMocks(/* connection_count = */ 1); diff --git a/test/common/quic/platform/quic_platform_test.cc b/test/common/quic/platform/quic_platform_test.cc index 7d0800bd241a..2f3f8682c84a 100644 --- a/test/common/quic/platform/quic_platform_test.cc +++ b/test/common/quic/platform/quic_platform_test.cc @@ -438,6 +438,12 @@ TEST_F(QuicPlatformTest, QuicFlags) { EXPECT_FALSE(GetQuicheFlag(quiche_oghttp2_debug_trace)); } +TEST_F(QuicPlatformTest, QuicheLogDFatalNoExit) { + quiche::test::QuicheScopedDisableExitOnDFatal scoped_object; + QUIC_LOG(DFATAL) << "This shouldn't call abort()"; + QUICHE_DCHECK(false) << "This shouldn't call abort()"; +} + TEST_F(QuicPlatformTest, UpdateReloadableFlags) { auto& flag_registry = quiche::FlagRegistry::getInstance(); diff --git a/test/common/quic/platform/quiche_test_impl.h b/test/common/quic/platform/quiche_test_impl.h index eee24fc881d1..1af2864b02b2 100644 --- a/test/common/quic/platform/quiche_test_impl.h +++ b/test/common/quic/platform/quiche_test_impl.h @@ -15,6 +15,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "quiche/common/platform/api/quiche_flags.h" +#include "quiche/common/platform/api/quiche_logging.h" using QuicheFlagSaverImpl = absl::FlagSaver; @@ -42,14 +43,20 @@ inline std::string QuicheGetCommonSourcePathImpl() { class QuicheScopedDisableExitOnDFatalImpl { public: - explicit QuicheScopedDisableExitOnDFatalImpl() {} + explicit QuicheScopedDisableExitOnDFatalImpl() { + original_value_ = isDFatalExitDisabled(); + setDFatalExitDisabled(true); + } // This type is neither copyable nor movable. QuicheScopedDisableExitOnDFatalImpl(const QuicheScopedDisableExitOnDFatalImpl&) = delete; QuicheScopedDisableExitOnDFatalImpl& operator=(const QuicheScopedDisableExitOnDFatalImpl&) = delete; - ~QuicheScopedDisableExitOnDFatalImpl() {} + ~QuicheScopedDisableExitOnDFatalImpl() { setDFatalExitDisabled(original_value_); } + +private: + bool original_value_; }; } // namespace test diff --git a/test/common/quic/quic_io_handle_wrapper_test.cc b/test/common/quic/quic_io_handle_wrapper_test.cc index b362c0814ab0..89c4894ffb03 100644 --- a/test/common/quic/quic_io_handle_wrapper_test.cc +++ b/test/common/quic/quic_io_handle_wrapper_test.cc @@ -189,6 +189,9 @@ TEST(QuicIoHandleWrapper, DelegateWithMocks) { EXPECT_CALL(mock_io, lastRoundTripTime()).Times(0); wrapper.lastRoundTripTime(); + + EXPECT_CALL(mock_io, interfaceName()); + wrapper.interfaceName(); } wrapper.close(); diff --git a/test/integration/filters/test_listener_filter.h b/test/integration/filters/test_listener_filter.h index a109bb4aa831..e3ae04d2651d 100644 --- a/test/integration/filters/test_listener_filter.h +++ b/test/integration/filters/test_listener_filter.h @@ -103,6 +103,7 @@ class TestQuicListenerFilter : public Network::QuicListenerFilter { std::make_unique(added_value_), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::Connection); + dispatcher_ = &cb.dispatcher(); return Network::FilterStatus::Continue; } bool isCompatibleWithServerPreferredAddress( @@ -122,6 +123,7 @@ class TestQuicListenerFilter : public Network::QuicListenerFilter { private: const std::string added_value_; const bool allow_server_migration_; + Event::Dispatcher* dispatcher_{nullptr}; const bool allow_client_migration_; }; From 55751292f59c203101884190b215ae6b0227a460 Mon Sep 17 00:00:00 2001 From: RenjieTang Date: Wed, 7 Feb 2024 11:34:28 -0800 Subject: [PATCH 19/25] Default disable port migration and allow it to be turned on in Envoy Mobile API (#32112) Commit Message: Default disable port migration and allow it to be turned on in Envoy Mobile API Additional Description: Currently it can only be turned on via the C++ API. More platforms will be covered once port migration is proven to work well. Risk Level: Low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: mobile Signed-off-by: Renjie Tang Signed-off-by: RenjieTang Signed-off-by: RenjieTang --- changelogs/current.yaml | 5 +++++ mobile/library/cc/engine_builder.cc | 13 +++++++++++++ mobile/library/cc/engine_builder.h | 2 ++ mobile/test/cc/unit/envoy_config_test.cc | 2 ++ source/common/quic/envoy_quic_client_session.cc | 2 +- test/integration/quic_http_integration_test.cc | 1 + 6 files changed, 24 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index acff38659afc..c37890f49223 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -19,6 +19,11 @@ minor_behavior_changes: - area: golang change: | Change ``RegisterHttpFilterConfigFactoryAndParser`` to ``RegisterHttpFilterFactoryAndConfigParser``. +- area: QUIC + change: | + Port migration is default turned off. QUIC client connections will no longer attempt to migrate to a new port when connections + is degrading. Can be manually turned on via + :ref:`port_migration `. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/mobile/library/cc/engine_builder.cc b/mobile/library/cc/engine_builder.cc index 888b4e98dcf3..1315ff57df3c 100644 --- a/mobile/library/cc/engine_builder.cc +++ b/mobile/library/cc/engine_builder.cc @@ -269,6 +269,11 @@ EngineBuilder& EngineBuilder::addQuicCanonicalSuffix(std::string suffix) { return *this; } +EngineBuilder& EngineBuilder::enablePortMigration(bool enable_port_migration) { + enable_port_migration_ = enable_port_migration; + return *this; +} + #endif EngineBuilder& EngineBuilder::setForceAlwaysUsev6(bool value) { @@ -737,6 +742,14 @@ std::unique_ptr EngineBuilder::generate ->add_canonical_suffixes(suffix); } + if (enable_port_migration_) { + alpn_options.mutable_auto_config() + ->mutable_http3_protocol_options() + ->mutable_quic_protocol_options() + ->mutable_num_timeouts_to_trigger_port_migration() + ->set_value(4); + } + base_cluster->mutable_transport_socket()->mutable_typed_config()->PackFrom(h3_proxy_socket); (*base_cluster->mutable_typed_extension_protocol_options()) ["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"] diff --git a/mobile/library/cc/engine_builder.h b/mobile/library/cc/engine_builder.h index e43aba4b97c5..8db9dc45f0f2 100644 --- a/mobile/library/cc/engine_builder.h +++ b/mobile/library/cc/engine_builder.h @@ -154,6 +154,7 @@ class EngineBuilder { EngineBuilder& setHttp3ClientConnectionOptions(std::string options); EngineBuilder& addQuicHint(std::string host, int port); EngineBuilder& addQuicCanonicalSuffix(std::string suffix); + EngineBuilder& enablePortMigration(bool enable_port_migration); #endif EngineBuilder& enableInterfaceBinding(bool interface_binding_on); EngineBuilder& enableDrainPostDnsRefresh(bool drain_post_dns_refresh_on); @@ -239,6 +240,7 @@ class EngineBuilder { std::string http3_client_connection_options_ = ""; std::vector> quic_hints_; std::vector quic_suffixes_; + bool enable_port_migration_ = false; bool always_use_v6_ = false; int dns_min_refresh_seconds_ = 60; int max_connections_per_host_ = 7; diff --git a/mobile/test/cc/unit/envoy_config_test.cc b/mobile/test/cc/unit/envoy_config_test.cc index d08e684d696b..5ca29816eee6 100644 --- a/mobile/test/cc/unit/envoy_config_test.cc +++ b/mobile/test/cc/unit/envoy_config_test.cc @@ -51,6 +51,7 @@ TEST(TestConfig, ConfigIsApplied) { .addQuicHint("www.def.com", 443) .addQuicCanonicalSuffix(".opq.com") .addQuicCanonicalSuffix(".xyz.com") + .enablePortMigration(true) #endif .addConnectTimeoutSeconds(123) .addDnsRefreshSeconds(456) @@ -85,6 +86,7 @@ TEST(TestConfig, ConfigIsApplied) { "hostname: \"www.def.com\"", "canonical_suffixes: \".opq.com\"", "canonical_suffixes: \".xyz.com\"", + "num_timeouts_to_trigger_port_migration { value: 4 }", #endif "key: \"dns_persistent_cache\" save_interval { seconds: 101 }", "key: \"always_use_v6\" value { bool_value: true }", diff --git a/source/common/quic/envoy_quic_client_session.cc b/source/common/quic/envoy_quic_client_session.cc index 2f3cbd1a0ec8..46bc95d74ebf 100644 --- a/source/common/quic/envoy_quic_client_session.cc +++ b/source/common/quic/envoy_quic_client_session.cc @@ -251,7 +251,7 @@ void EnvoyQuicClientSession::setHttp3Options( } static_cast(connection()) ->setNumPtosForPortMigration(PROTOBUF_GET_WRAPPED_OR_DEFAULT( - http3_options.quic_protocol_options(), num_timeouts_to_trigger_port_migration, 4)); + http3_options.quic_protocol_options(), num_timeouts_to_trigger_port_migration, 0)); if (http3_options_->quic_protocol_options().has_connection_keepalive()) { const uint64_t initial_interval = PROTOBUF_GET_MS_OR_DEFAULT( diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 2ce46d476f08..21cdffc5657a 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -904,6 +904,7 @@ TEST_P(QuicHttpIntegrationTest, PortMigrationFailureOnPathDegrading) { setConcurrency(2); validation_failure_on_path_response_ = true; initialize(); + client_quic_options_.mutable_num_timeouts_to_trigger_port_migration()->set_value(2); uint32_t old_port = lookupPort("http"); codec_client_ = makeHttpConnection(old_port); auto encoder_decoder = From b8e6fc171794020606ed9aac20c34ef4210af6a0 Mon Sep 17 00:00:00 2001 From: Tianyu <72890320+tyxia@users.noreply.github.com> Date: Wed, 7 Feb 2024 16:10:58 -0500 Subject: [PATCH 20/25] rlqs: Add more logging around stream start and close (#32239) --------- Signed-off-by: tyxia --- .../filters/http/rate_limit_quota/client_impl.cc | 8 ++++++-- source/extensions/filters/http/rate_limit_quota/filter.cc | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/rate_limit_quota/client_impl.cc b/source/extensions/filters/http/rate_limit_quota/client_impl.cc index fbda6884bcab..6b375bd5d7d8 100644 --- a/source/extensions/filters/http/rate_limit_quota/client_impl.cc +++ b/source/extensions/filters/http/rate_limit_quota/client_impl.cc @@ -38,7 +38,7 @@ RateLimitQuotaUsageReports RateLimitClientImpl::buildReport(absl::optional bucket_id) { ASSERT(stream_ != nullptr); // Build the report and then send the report to RLQS server. - // TODO(tyxia) Revisit end_stream, means send and close. + // `end_stream` should always be set to false as we don't want to close the stream locally. stream_->sendMessage(buildReport(bucket_id), /*end_stream=*/false); } @@ -93,21 +93,25 @@ void RateLimitClientImpl::onReceiveMessage(RateLimitQuotaResponsePtr&& response) void RateLimitClientImpl::closeStream() { // Close the stream if it is in open state. if (stream_ != nullptr && !stream_closed_) { + ENVOY_LOG(debug, "Closing gRPC stream"); stream_->closeStream(); stream_closed_ = true; stream_->resetStream(); } } -void RateLimitClientImpl::onRemoteClose(Grpc::Status::GrpcStatus, const std::string&) { +void RateLimitClientImpl::onRemoteClose(Grpc::Status::GrpcStatus status, + const std::string& message) { // TODO(tyxia) Revisit later, maybe add some logging. stream_closed_ = true; + ENVOY_LOG(debug, "gRPC stream closed remotely with status {}: {}", status, message); closeStream(); } absl::Status RateLimitClientImpl::startStream(const StreamInfo::StreamInfo& stream_info) { // Starts stream if it has not been opened yet. if (stream_ == nullptr) { + ENVOY_LOG(debug, "Trying to start the new gRPC stream"); stream_ = aync_client_.start( *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.rate_limit_quota.v3.RateLimitQuotaService.StreamRateLimitQuotas"), diff --git a/source/extensions/filters/http/rate_limit_quota/filter.cc b/source/extensions/filters/http/rate_limit_quota/filter.cc index 58baf4bc8f1f..48dc20ee61e1 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.cc +++ b/source/extensions/filters/http/rate_limit_quota/filter.cc @@ -143,11 +143,14 @@ RateLimitQuotaFilter::sendImmediateReport(const size_t bucket_id, ASSERT(client_.rate_limit_client != nullptr); // Start the streaming on the first request. + // It will be a no-op if the stream is already active. auto status = client_.rate_limit_client->startStream(callbacks_->streamInfo()); if (!status.ok()) { ENVOY_LOG(error, "Failed to start the gRPC stream: ", status.message()); // TODO(tyxia) Check `NoAssignmentBehavior` behavior instead of fail-open here. return Envoy::Http::FilterHeadersStatus::Continue; + } else { + ENVOY_LOG(debug, "The gRPC stream is established and active"); } initiating_call_ = true; From b5110b509c26b99f87c6055894c3ca140027e28f Mon Sep 17 00:00:00 2001 From: Tianyu <72890320+tyxia@users.noreply.github.com> Date: Wed, 7 Feb 2024 16:16:12 -0500 Subject: [PATCH 21/25] rlqs: fix time_elapsed logic and add more logging (#32204) --------- Signed-off-by: tyxia --- .../http/rate_limit_quota/client_impl.cc | 9 +++++++-- .../http/rate_limit_quota/integration_test.cc | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/rate_limit_quota/client_impl.cc b/source/extensions/filters/http/rate_limit_quota/client_impl.cc index 6b375bd5d7d8..36af6a35b4fb 100644 --- a/source/extensions/filters/http/rate_limit_quota/client_impl.cc +++ b/source/extensions/filters/http/rate_limit_quota/client_impl.cc @@ -15,21 +15,25 @@ RateLimitQuotaUsageReports RateLimitClientImpl::buildReport(absl::optionalmutable_bucket_id() = bucket->bucket_id; usage->set_num_requests_allowed(bucket->quota_usage.num_requests_allowed); usage->set_num_requests_denied(bucket->quota_usage.num_requests_denied); + auto now = std::chrono::duration_cast( + time_source_.monotonicTime().time_since_epoch()); // For the newly created bucket (i.e., `bucket_id` input is not null), its time // elapsed since last report is 0. // This case happens when we send the report to RLQS server immediately. if (bucket_id.has_value() && bucket_id.value() == id) { *usage->mutable_time_elapsed() = Protobuf::util::TimeUtil::NanosecondsToDuration(0); } else { - auto now = std::chrono::duration_cast( - time_source_.monotonicTime().time_since_epoch()); *usage->mutable_time_elapsed() = Protobuf::util::TimeUtil::NanosecondsToDuration( (now - bucket->quota_usage.last_report).count()); } + + // Update the last_report time point. + bucket->quota_usage.last_report = now; } // Set the domain name. report.set_domain(domain_name_); + ENVOY_LOG(debug, "The usage report that will be sent to RLQS server:\n{}", report.DebugString()); return report; } @@ -43,6 +47,7 @@ void RateLimitClientImpl::sendUsageReport(absl::optional bucket_id) { } void RateLimitClientImpl::onReceiveMessage(RateLimitQuotaResponsePtr&& response) { + ENVOY_LOG(debug, "The response that is received from RLQS server:\n{}", response->DebugString()); for (const auto& action : response->bucket_action()) { if (!action.has_bucket_id() || action.bucket_id().bucket().empty()) { ENVOY_LOG(error, diff --git a/test/extensions/filters/http/rate_limit_quota/integration_test.cc b/test/extensions/filters/http/rate_limit_quota/integration_test.cc index 426e39d633b4..7317a1fdfa0b 100644 --- a/test/extensions/filters/http/rate_limit_quota/integration_test.cc +++ b/test/extensions/filters/http/rate_limit_quota/integration_test.cc @@ -397,15 +397,26 @@ TEST_P(RateLimitQuotaIntegrationTest, BasicFlowPeriodicalReport) { EXPECT_TRUE(response_->complete()); EXPECT_EQ(response_->headers().getStatusValue(), "200"); + // TODO(tyxia) Make interval configurable in the test. It is currently 60s in + // ValidMatcherConfig. + int report_interval_sec = 60; // Trigger the report periodically, 10 times. for (int i = 0; i < 10; ++i) { // Advance the time by report_interval. - // TODO(tyxia) Make interval configurable in the test. It is currently 60s in - // ValidMatcherConfig. - simTime().advanceTimeWait(std::chrono::milliseconds(60000)); + simTime().advanceTimeWait(std::chrono::milliseconds(report_interval_sec * 1000)); // Checks that the rate limit server has received the periodical reports. ASSERT_TRUE(rlqs_stream_->waitForGrpcMessage(*dispatcher_, reports)); + // Verify the usage report content. + for (const auto& usage : reports.bucket_quota_usages()) { + // We only send single downstream client request and it is allowed. + EXPECT_EQ(usage.num_requests_allowed(), 1); + EXPECT_EQ(usage.num_requests_denied(), 0); + // time_elapsed equals to periodical reporting interval. + EXPECT_EQ(Protobuf::util::TimeUtil::DurationToSeconds(usage.time_elapsed()), + report_interval_sec); + } + // Build the rlqs server response. envoy::service::rate_limit_quota::v3::RateLimitQuotaResponse rlqs_response2; auto* bucket_action2 = rlqs_response2.add_bucket_action(); From 2c636750f00038d3fdbb67e6a27fa7861097d7e2 Mon Sep 17 00:00:00 2001 From: Juan Manuel Olle Date: Wed, 7 Feb 2024 18:17:37 -0300 Subject: [PATCH 22/25] Add host rewrite config to aws lambda (#32005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --------- Signed-off-by: Juan Manuel Ollé --- .../http/aws_lambda/v3/aws_lambda.proto | 13 +++ changelogs/current.yaml | 4 + .../http/aws_lambda/aws_lambda_filter.cc | 13 ++- .../http/aws_lambda/aws_lambda_filter.h | 9 +- .../filters/http/aws_lambda/config.cc | 5 +- .../http/aws_lambda/aws_lambda_filter_test.cc | 82 +++++++++++-------- 6 files changed, 86 insertions(+), 40 deletions(-) diff --git a/api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto b/api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto index 5268550c0a1b..efea41b4199b 100644 --- a/api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto +++ b/api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto @@ -42,6 +42,19 @@ message Config { // Determines the way to invoke the Lambda function. InvocationMode invocation_mode = 3 [(validate.rules).enum = {defined_only: true}]; + + // Indicates that before signing headers, the host header will be swapped with + // this value. If not set or empty, the original host header value + // will be used and no rewrite will happen. + // + // Note: this rewrite affects both signing and host header forwarding. However, this + // option shouldn't be used with + // :ref:`HCM host rewrite ` given that the + // value set here would be used for signing whereas the value set in the HCM would be used + // for host header forwarding which is not the desired outcome. + // Changing the value of the host header can result in a different route to be selected + // if an HTTP filter after AWS lambda re-evaluates the route (clears route cache). + string host_rewrite = 4; } // Per-route configuration for AWS Lambda. This can be useful when invoking a different Lambda function or a different diff --git a/changelogs/current.yaml b/changelogs/current.yaml index c37890f49223..ef621bf7ea88 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -92,6 +92,10 @@ new_features: change: | added support for :ref:`%UPSTREAM_CONNECTION_ID% ` for the upstream connection identifier. +- area: aws_lambda + change: | + Added :ref:`host_rewrite ` config to be used + during signature. - area: ext_proc change: | added diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc index f2a989f17d1c..f291f35ae700 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc @@ -42,7 +42,7 @@ constexpr auto filter_metadata_key = "com.amazonaws.lambda"; constexpr auto egress_gateway_metadata_key = "egress_gateway"; void setLambdaHeaders(Http::RequestHeaderMap& headers, const absl::optional& arn, - InvocationMode mode) { + InvocationMode mode, const std::string& host_rewrite) { headers.setMethod(Http::Headers::get().MethodValues.Post); headers.setPath(fmt::format("/2015-03-31/functions/{}/invocations", arn->arn())); if (mode == InvocationMode::Synchronous) { @@ -50,6 +50,9 @@ void setLambdaHeaders(Http::RequestHeaderMap& headers, const absl::optional } else { headers.setReference(LambdaFilterNames::get().InvocationTypeHeader, "Event"); } + if (!host_rewrite.empty()) { + headers.setHost(host_rewrite); + } } /** @@ -133,9 +136,11 @@ void Filter::resolveSettings() { payload_passthrough_ = route_settings->payloadPassthrough(); invocation_mode_ = route_settings->invocationMode(); arn_ = std::move(route_settings)->arn(); + host_rewrite_ = route_settings->hostRewrite(); } else { payload_passthrough_ = settings_.payloadPassthrough(); invocation_mode_ = settings_.invocationMode(); + host_rewrite_ = settings_.hostRewrite(); } } @@ -161,7 +166,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, } if (payload_passthrough_) { - setLambdaHeaders(headers, arn_, invocation_mode_); + setLambdaHeaders(headers, arn_, invocation_mode_, host_rewrite_); sigv4_signer_->signEmptyPayload(headers, arn_->region()); return Http::FilterHeadersStatus::Continue; } @@ -170,7 +175,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, jsonizeRequest(headers, nullptr, json_buf); // We must call setLambdaHeaders *after* the JSON transformation of the request. That way we // reflect the actual incoming request headers instead of the overwritten ones. - setLambdaHeaders(headers, arn_, invocation_mode_); + setLambdaHeaders(headers, arn_, invocation_mode_, host_rewrite_); headers.setContentLength(json_buf.length()); headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); auto& hashing_util = Envoy::Common::Crypto::UtilitySingleton::get(); @@ -230,7 +235,7 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea request_headers_->setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); } - setLambdaHeaders(*request_headers_, arn_, invocation_mode_); + setLambdaHeaders(*request_headers_, arn_, invocation_mode_, host_rewrite_); const auto hash = Hex::encode(hashing_util.getSha256Digest(decoding_buffer)); sigv4_signer_->sign(*request_headers_, hash, arn_->region()); stats().upstream_rq_payload_size_.recordValue(decoding_buffer.length()); diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h index bcfcbe63bda0..b8a692540c6e 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.h @@ -80,17 +80,21 @@ enum class InvocationMode { Synchronous, Asynchronous }; class FilterSettings : public Router::RouteSpecificFilterConfig { public: - FilterSettings(const Arn& arn, InvocationMode mode, bool payload_passthrough) - : arn_(arn), invocation_mode_(mode), payload_passthrough_(payload_passthrough) {} + FilterSettings(const Arn& arn, InvocationMode mode, bool payload_passthrough, + const std::string& host_rewrite) + : arn_(arn), invocation_mode_(mode), payload_passthrough_(payload_passthrough), + host_rewrite_(host_rewrite) {} const Arn& arn() const& { return arn_; } bool payloadPassthrough() const { return payload_passthrough_; } InvocationMode invocationMode() const { return invocation_mode_; } + const std::string& hostRewrite() const { return host_rewrite_; } private: Arn arn_; InvocationMode invocation_mode_; bool payload_passthrough_; + const std::string host_rewrite_; }; class Filter : public Http::PassThroughFilter, Logger::Loggable { @@ -136,6 +140,7 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable AwsLambdaFilterFactory::createFilterFactor Extensions::Common::Aws::AwsSigningHeaderExclusionVector{}); FilterSettings filter_settings{*arn, getInvocationMode(proto_config), - proto_config.payload_passthrough()}; + proto_config.payload_passthrough(), proto_config.host_rewrite()}; FilterStats stats = generateStats(stats_prefix, dual_info.scope); return [stats, signer, filter_settings, dual_info](Http::FilterChainFactoryCallbacks& cb) { @@ -79,7 +79,8 @@ AwsLambdaFilterFactory::createRouteSpecificFilterConfigTyped( } return std::make_shared( FilterSettings{*arn, getInvocationMode(proto_config.invoke_config()), - proto_config.invoke_config().payload_passthrough()}); + proto_config.invoke_config().payload_passthrough(), + proto_config.invoke_config().host_rewrite()}); } /* diff --git a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc index 2c00c40a975f..eac311c905ca 100644 --- a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc +++ b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc @@ -63,7 +63,7 @@ class AwsLambdaFilterTest : public ::testing::Test { * Requests that are _not_ header only, should result in StopIteration. */ TEST_F(AwsLambdaFilterTest, DecodingHeaderStopIteration) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); Http::TestRequestHeaderMapImpl headers; const auto result = filter_->decodeHeaders(headers, false /*end_stream*/); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, result); @@ -73,7 +73,7 @@ TEST_F(AwsLambdaFilterTest, DecodingHeaderStopIteration) { * Header only pass-through requests should be signed and Continue iteration. */ TEST_F(AwsLambdaFilterTest, HeaderOnlyShouldContinue) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); EXPECT_CALL(*signer_, signEmptyPayload(An(), An())); Http::TestRequestHeaderMapImpl input_headers; const auto result = filter_->decodeHeaders(input_headers, true /*end_stream*/); @@ -91,7 +91,7 @@ TEST_F(AwsLambdaFilterTest, HeaderOnlyShouldContinue) { */ TEST_F(AwsLambdaFilterTest, ClusterMetadataIsNotNeededInUpstreamMode) { signer_ = std::make_shared>(); - auto settings = FilterSettings{arn_, InvocationMode::Synchronous, true}; + auto settings = FilterSettings{arn_, InvocationMode::Synchronous, true, ""}; filter_ = std::make_unique(settings, stats_, signer_, true); filter_->setDecoderFilterCallbacks(decoder_callbacks_); filter_->setEncoderFilterCallbacks(encoder_callbacks_); @@ -122,8 +122,8 @@ TEST_F(AwsLambdaFilterTest, PerRouteConfigWrongClusterMetadata) { TestUtility::loadFromYaml(metadata_yaml, cluster_metadata); metadata.mutable_filter_metadata()->insert({"WrongMetadataKey", cluster_metadata}); - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); - FilterSettings route_settings{arn_, InvocationMode::Synchronous, true /*passthrough*/}; + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); + FilterSettings route_settings{arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}; ON_CALL(*decoder_callbacks_.route_, mostSpecificPerFilterConfig(_)) .WillByDefault(Return(&route_settings)); @@ -144,8 +144,8 @@ TEST_F(AwsLambdaFilterTest, PerRouteConfigWrongClusterMetadata) { * process the request (i.e. StopIteration if end_stream is false) */ TEST_F(AwsLambdaFilterTest, PerRouteConfigCorrectClusterMetadata) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); - FilterSettings route_settings{arn_, InvocationMode::Synchronous, true /*passthrough*/}; + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); + FilterSettings route_settings{arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}; ON_CALL(*decoder_callbacks_.route_, mostSpecificPerFilterConfig(_)) .WillByDefault(Return(&route_settings)); @@ -159,11 +159,12 @@ TEST_F(AwsLambdaFilterTest, PerRouteConfigCorrectClusterMetadata) { * then the SigV4 signer will sign with the region where the lambda function is present. */ TEST_F(AwsLambdaFilterTest, PerRouteConfigCorrectRegionForSigning) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); const absl::string_view override_region = "us-west-1"; const auto per_route_arn = parseArn(fmt::format("arn:aws:lambda:{}:1337:function:fun", override_region)).value(); - FilterSettings route_settings{per_route_arn, InvocationMode::Synchronous, true /*passthrough*/}; + FilterSettings route_settings{per_route_arn, InvocationMode::Synchronous, true /*passthrough*/, + ""}; ON_CALL(*decoder_callbacks_.route_, mostSpecificPerFilterConfig(_)) .WillByDefault(Return(&route_settings)); @@ -177,7 +178,7 @@ TEST_F(AwsLambdaFilterTest, PerRouteConfigCorrectRegionForSigning) { } TEST_F(AwsLambdaFilterTest, DecodeDataRecordsPayloadSize) { - FilterSettings settings{arn_, InvocationMode::Synchronous, true /*passthrough*/}; + FilterSettings settings{arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}; NiceMock store; NiceMock histogram; EXPECT_CALL(store, histogram(_, _)).WillOnce(ReturnRef(histogram)); @@ -204,7 +205,7 @@ TEST_F(AwsLambdaFilterTest, DecodeDataRecordsPayloadSize) { } TEST_F(AwsLambdaFilterTest, DecodeDataShouldBuffer) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); Http::TestRequestHeaderMapImpl headers; const auto header_result = filter_->decodeHeaders(headers, false /*end_stream*/); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, header_result); @@ -214,7 +215,7 @@ TEST_F(AwsLambdaFilterTest, DecodeDataShouldBuffer) { } TEST_F(AwsLambdaFilterTest, DecodeDataShouldSign) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); Http::TestRequestHeaderMapImpl headers; const auto header_result = filter_->decodeHeaders(headers, false /*end_stream*/); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, header_result); @@ -233,12 +234,13 @@ TEST_F(AwsLambdaFilterTest, DecodeDataShouldSign) { } TEST_F(AwsLambdaFilterTest, DecodeDataSigningWithPerRouteConfig) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); const absl::string_view override_region = "us-west-1"; const auto per_route_arn = parseArn(fmt::format("arn:aws:lambda:{}:1337:function:fun", override_region)).value(); - FilterSettings route_settings{per_route_arn, InvocationMode::Synchronous, true /*passthrough*/}; + FilterSettings route_settings{per_route_arn, InvocationMode::Synchronous, true /*passthrough*/, + ""}; ON_CALL(*decoder_callbacks_.route_, mostSpecificPerFilterConfig(_)) .WillByDefault(Return(&route_settings)); @@ -261,7 +263,7 @@ TEST_F(AwsLambdaFilterTest, DecodeDataSigningWithPerRouteConfig) { } TEST_F(AwsLambdaFilterTest, DecodeHeadersInvocationModeSetsHeader) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); Http::TestRequestHeaderMapImpl headers; const auto header_result = filter_->decodeHeaders(headers, true /*end_stream*/); EXPECT_EQ(Http::FilterHeadersStatus::Continue, header_result); @@ -287,7 +289,7 @@ TEST_F(AwsLambdaFilterTest, DecodeHeadersInvocationModeSetsHeader) { */ TEST_F(AwsLambdaFilterTest, DecodeHeadersOnlyRequestWithJsonOn) { using source::extensions::filters::http::aws_lambda::Request; - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Buffer::OwnedImpl json_buf; auto on_add_decoded_data = [&json_buf](Buffer::Instance& buf, bool) { json_buf.move(buf); }; ON_CALL(decoder_callbacks_, addDecodedData(_, _)).WillByDefault(Invoke(on_add_decoded_data)); @@ -335,7 +337,7 @@ TEST_F(AwsLambdaFilterTest, DecodeHeadersOnlyRequestWithJsonOn) { */ TEST_F(AwsLambdaFilterTest, DecodeDataWithTextualBodyWithJsonOn) { using source::extensions::filters::http::aws_lambda::Request; - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Buffer::OwnedImpl decoded_buf; constexpr absl::string_view expected_plain_text = "Foo bar bazz"; @@ -404,7 +406,7 @@ TEST_F(AwsLambdaFilterTest, DecodeDataWithTextualBodyWithJsonOn) { */ TEST_F(AwsLambdaFilterTest, DecodeDataWithBinaryBodyWithJsonOn) { using source::extensions::filters::http::aws_lambda::Request; - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Buffer::OwnedImpl decoded_buf; const absl::string_view fake_binary_data = "this should get base64 encoded"; @@ -447,12 +449,12 @@ TEST_F(AwsLambdaFilterTest, DecodeDataWithBinaryBodyWithJsonOn) { } TEST_F(AwsLambdaFilterTest, EncodeHeadersEndStreamShouldSkip) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); Http::TestResponseHeaderMapImpl headers; auto result = filter_->encodeHeaders(headers, true /*end_stream*/); EXPECT_EQ(Http::FilterHeadersStatus::Continue, result); - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); result = filter_->encodeHeaders(headers, true /*end_stream*/); EXPECT_EQ(Http::FilterHeadersStatus::Continue, result); } @@ -462,7 +464,7 @@ TEST_F(AwsLambdaFilterTest, EncodeHeadersEndStreamShouldSkip) { * encoding headers and skip the filter. */ TEST_F(AwsLambdaFilterTest, EncodeHeadersWithLambdaErrorShouldSkipAndContinue) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); headers.addCopy(Http::LowerCaseString("x-Amz-Function-Error"), "unhandled"); @@ -474,7 +476,7 @@ TEST_F(AwsLambdaFilterTest, EncodeHeadersWithLambdaErrorShouldSkipAndContinue) { * If Lambda returns a 5xx error then we should skip encoding headers and skip the filter. */ TEST_F(AwsLambdaFilterTest, EncodeHeadersWithLambda5xxShouldSkipAndContinue) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Http::TestResponseHeaderMapImpl headers; headers.setStatus(500); auto result = filter_->encodeHeaders(headers, false /*end_stream*/); @@ -485,7 +487,7 @@ TEST_F(AwsLambdaFilterTest, EncodeHeadersWithLambda5xxShouldSkipAndContinue) { * encodeHeaders() in a happy path should stop iteration. */ TEST_F(AwsLambdaFilterTest, EncodeHeadersStopsIteration) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); auto result = filter_->encodeHeaders(headers, false /*end_stream*/); @@ -497,7 +499,7 @@ TEST_F(AwsLambdaFilterTest, EncodeHeadersStopsIteration) { * This is true whether end_stream is true or false. */ TEST_F(AwsLambdaFilterTest, EncodeDataInPassThroughMode) { - setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, true /*passthrough*/, ""}); Buffer::OwnedImpl buf; filter_->resolveSettings(); auto result = filter_->encodeData(buf, false /*end_stream*/); @@ -506,7 +508,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataInPassThroughMode) { result = filter_->encodeData(buf, true /*end_stream*/); EXPECT_EQ(Http::FilterDataStatus::Continue, result); - setupFilter({arn_, InvocationMode::Asynchronous, true /*passthrough*/}); + setupFilter({arn_, InvocationMode::Asynchronous, true /*passthrough*/, ""}); filter_->resolveSettings(); result = filter_->encodeData(buf, false /*end_stream*/); EXPECT_EQ(Http::FilterDataStatus::Continue, result); @@ -520,7 +522,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataInPassThroughMode) { * This is true whether end_stream is true or false. */ TEST_F(AwsLambdaFilterTest, EncodeDataInAsynchrnous) { - setupFilter({arn_, InvocationMode::Asynchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Asynchronous, false /*passthrough*/, ""}); Buffer::OwnedImpl buf; filter_->resolveSettings(); auto result = filter_->encodeData(buf, false /*end_stream*/); @@ -534,7 +536,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataInAsynchrnous) { * encodeData() data in JSON mode should stop iteration if end_stream is false. */ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeStopIterationAndBuffer) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); Buffer::OwnedImpl buf; filter_->resolveSettings(); auto result = filter_->encodeData(buf, false /*end_stream*/); @@ -542,7 +544,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeStopIterationAndBuffer) { } TEST_F(AwsLambdaFilterTest, EncodeDataAddsLastChunk) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); filter_->resolveSettings(); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); @@ -559,7 +561,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataAddsLastChunk) { * headers while ignoring any HTTP/2 pseudo-headers. */ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeTransformToHttp) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); filter_->resolveSettings(); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); @@ -610,7 +612,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeTransformToHttp) { * encodeData() data in JSON mode should respect content-type header. */ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeContentTypeHeader) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); filter_->resolveSettings(); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); @@ -645,7 +647,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeContentTypeHeader) { * base64-encoded. */ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeBase64EncodedBody) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); filter_->resolveSettings(); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); @@ -694,7 +696,7 @@ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeBase64EncodedBody) { * Encode data in JSON mode _returning_ invalid JSON payload should result in a 500 error. */ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeInvalidJson) { - setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/}); + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, ""}); filter_->resolveSettings(); Http::TestResponseHeaderMapImpl headers; headers.setStatus(200); @@ -725,6 +727,22 @@ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeInvalidJson) { EXPECT_EQ(1ul, filter_->stats().server_error_.value()); } +TEST_F(AwsLambdaFilterTest, SignWithHostRewrite) { + setupFilter({arn_, InvocationMode::Synchronous, false /*passthrough*/, "new_host"}); + filter_->resolveSettings(); + + Http::TestRequestHeaderMapImpl headers; + headers.setHost("any_host"); + headers.setPath("/"); + headers.setMethod("POST"); + const auto result = filter_->decodeHeaders(headers, true); + + EXPECT_EQ("new_host", headers.get_(":authority")); + EXPECT_EQ("new_host", headers.Host()->value().getStringView()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, result); +} + } // namespace } // namespace AwsLambdaFilter } // namespace HttpFilters From eec5f50a75469b4bc60526158e17f6abdfc1e5d1 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 7 Feb 2024 18:07:21 -0500 Subject: [PATCH 23/25] coverage: removing flake (#32262) Signed-off-by: Alyssa Wilk --- test/per_file_coverage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index ef1713013883..72e58a956207 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -11,7 +11,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/common/event:95.0" # Emulated edge events guards don't report LCOV "source/common/filesystem/posix:96.2" # FileReadToEndNotReadable fails in some env; createPath can't test all failure branches. "source/common/http/http2:95.2" -"source/common/io:57.9" # TODO(#32149): CI has stopped executing this code. +"source/common/io:57.1" # TODO(#32149): CI has stopped executing this code. "source/common/json:94.6" "source/common/matcher:94.6" "source/common/network:94.4" # Flaky, `activateFileEvents`, `startSecureTransport` and `ioctl`, listener_socket do not always report LCOV From 07a00cfab536467d3441ebd9ed0344e905d17d12 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 7 Feb 2024 18:12:01 -0500 Subject: [PATCH 24/25] Quiche roll 20240205122544 (#32200) * Update QUICHE from b5d556774 to 60a22a631 https://github.com/google/quiche/compare/b5d556774..60a22a631 ``` $ git log b5d556774..60a22a631 --date=short --no-merges --format="%ad %al %s" 2024-02-05 vasilvv No public description 2024-02-02 birenroy Demotes some noisy debug logging to VLOG. 2024-02-02 bnc Merge quic_protocol_flags_list.h into quiche_protocol_flags_list.h. 2024-02-02 martinduke MOQT Message formats from draft-02. ``` Signed-off-by: Alyssa Wilk --- bazel/external/quiche.BUILD | 12 +++--------- bazel/repository_locations.bzl | 6 +++--- source/common/quic/platform/BUILD | 1 - source/common/quic/platform/quiche_flags_impl.cc | 4 ---- source/common/quic/platform/quiche_flags_impl.h | 4 ---- 5 files changed, 6 insertions(+), 21 deletions(-) diff --git a/bazel/external/quiche.BUILD b/bazel/external/quiche.BUILD index 667c1254e3c7..03f9b143274a 100644 --- a/bazel/external/quiche.BUILD +++ b/bazel/external/quiche.BUILD @@ -2906,6 +2906,7 @@ envoy_quic_cc_library( ":quic_core_versions_lib", ":quic_platform_base", ":quiche_common_text_utils_lib", + ":quiche_common_wire_serialization", "@com_google_absl//absl/cleanup", ], ) @@ -2929,6 +2930,7 @@ envoy_cc_library( "quiche/quic/core/frames/quic_path_challenge_frame.cc", "quiche/quic/core/frames/quic_path_response_frame.cc", "quiche/quic/core/frames/quic_ping_frame.cc", + "quiche/quic/core/frames/quic_reset_stream_at_frame.cc", "quiche/quic/core/frames/quic_retire_connection_id_frame.cc", "quiche/quic/core/frames/quic_rst_stream_frame.cc", "quiche/quic/core/frames/quic_stop_sending_frame.cc", @@ -2956,6 +2958,7 @@ envoy_cc_library( "quiche/quic/core/frames/quic_path_challenge_frame.h", "quiche/quic/core/frames/quic_path_response_frame.h", "quiche/quic/core/frames/quic_ping_frame.h", + "quiche/quic/core/frames/quic_reset_stream_at_frame.h", "quiche/quic/core/frames/quic_retire_connection_id_frame.h", "quiche/quic/core/frames/quic_rst_stream_frame.h", "quiche/quic/core/frames/quic_stop_sending_frame.h", @@ -3571,15 +3574,6 @@ envoy_quic_cc_library( ], ) -envoy_cc_library( - name = "quic_core_protocol_flags_list_lib", - hdrs = ["quiche/quic/core/quic_protocol_flags_list.h"], - copts = quiche_copts, - repository = "@envoy", - tags = ["nofips"], - visibility = ["//visibility:public"], -) - envoy_quic_cc_library( name = "quic_core_qpack_blocking_manager_lib", srcs = ["quiche/quic/core/qpack/qpack_blocking_manager.cc"], diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index a791b08f127c..ee812634bb44 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1175,12 +1175,12 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "QUICHE", project_desc = "QUICHE (QUIC, HTTP/2, Etc) is Google‘s implementation of QUIC and related protocols", project_url = "https://github.com/google/quiche", - version = "b5d556774fb971506e5912a357f0f8fb8ef08d12", - sha256 = "f600af67bfccec4a0e8b88f721371756429975b1956269ae034ce08247ae55bd", + version = "60a22a631bdf944e26407d32a767b4aba953bc39", + sha256 = "70213d0e4016ce79db9f3dfc5e94b4e707c54b69e00fae7ea2593a08e9dfd11e", urls = ["https://github.com/google/quiche/archive/{version}.tar.gz"], strip_prefix = "quiche-{version}", use_category = ["controlplane", "dataplane_core"], - release_date = "2024-02-01", + release_date = "2024-02-05", cpe = "N/A", license = "BSD-3-Clause", license_url = "https://github.com/google/quiche/blob/{version}/LICENSE", diff --git a/source/common/quic/platform/BUILD b/source/common/quic/platform/BUILD index ad64a7bc53e9..d10213a60dc9 100644 --- a/source/common/quic/platform/BUILD +++ b/source/common/quic/platform/BUILD @@ -48,7 +48,6 @@ envoy_quiche_platform_impl_cc_library( "//source/common/common:assert_lib", "//source/common/http:utility_lib", "@com_github_google_quiche//:quic_core_flags_list_lib", - "@com_github_google_quiche//:quic_core_protocol_flags_list_lib", "@com_github_google_quiche//:quiche_protocol_flags_list_lib", "@com_google_absl//absl/flags:flag", ], diff --git a/source/common/quic/platform/quiche_flags_impl.cc b/source/common/quic/platform/quiche_flags_impl.cc index 60dc6622a7a7..e9d19e841830 100644 --- a/source/common/quic/platform/quiche_flags_impl.cc +++ b/source/common/quic/platform/quiche_flags_impl.cc @@ -80,10 +80,6 @@ template <> constexpr int32_t maybeOverride(absl::string_view name, int #define PROTOCOL_FLAG_MACRO_CHOOSER(...) \ GET_6TH_ARG(__VA_ARGS__, DEFINE_PROTOCOL_FLAG_TWO_VALUES, DEFINE_PROTOCOL_FLAG_SINGLE_VALUE) -#define QUIC_PROTOCOL_FLAG(...) PROTOCOL_FLAG_MACRO_CHOOSER(__VA_ARGS__)(__VA_ARGS__) -#include "quiche/quic/core/quic_protocol_flags_list.h" -#undef QUIC_PROTOCOL_FLAG - #define QUICHE_PROTOCOL_FLAG(...) PROTOCOL_FLAG_MACRO_CHOOSER(__VA_ARGS__)(__VA_ARGS__) #include "quiche/common/quiche_protocol_flags_list.h" #undef QUICHE_PROTOCOL_FLAG diff --git a/source/common/quic/platform/quiche_flags_impl.h b/source/common/quic/platform/quiche_flags_impl.h index e65277a3b9e4..e3a8d3f87d7c 100644 --- a/source/common/quic/platform/quiche_flags_impl.h +++ b/source/common/quic/platform/quiche_flags_impl.h @@ -49,10 +49,6 @@ class FlagRegistry { #include "quiche/quic/core/quic_flags_list.h" #undef QUIC_FLAG -#define QUIC_PROTOCOL_FLAG(type, flag, ...) ABSL_DECLARE_FLAG(type, envoy_##flag); -#include "quiche/quic/core/quic_protocol_flags_list.h" -#undef QUIC_PROTOCOL_FLAG - #define QUICHE_PROTOCOL_FLAG(type, flag, ...) ABSL_DECLARE_FLAG(type, envoy_##flag); #include "quiche/common/quiche_protocol_flags_list.h" #undef QUICHE_PROTOCOL_FLAG From d14ce54e2d9eff732520fe6b4db598726f86f18d Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 7 Feb 2024 15:24:36 -0800 Subject: [PATCH 25/25] macos: fix linker error from CEL library (#32148) Signed-off-by: Greg Greenway --- bazel/cel-cpp.patch | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/bazel/cel-cpp.patch b/bazel/cel-cpp.patch index 5f674379781b..f0d7d3005be7 100644 --- a/bazel/cel-cpp.patch +++ b/bazel/cel-cpp.patch @@ -1,3 +1,24 @@ +diff --git a/eval/internal/interop.cc b/eval/internal/interop.cc +index 3acde6c..20f8ea3 100644 +--- a/eval/internal/interop.cc ++++ b/eval/internal/interop.cc +@@ -729,13 +729,14 @@ absl::StatusOr ToLegacyValue(google::protobuf::Arena* arena, + return CelValue::CreateMessageWrapper( + MessageWrapperAccess::Make(message, type_info)); + } +- if (ProtoStructValueToMessageWrapper) { ++ // This weak symbol is never defined in Envoy, and checking it causes linker failures on macOS ++ /*if (ProtoStructValueToMessageWrapper) { + auto maybe_message_wrapper = ProtoStructValueToMessageWrapper(*value); + if (maybe_message_wrapper.has_value()) { + return CelValue::CreateMessageWrapper( + std::move(maybe_message_wrapper).value()); + } +- } ++ }*/ + return absl::UnimplementedError( + "only legacy struct types and values can be used for interop"); + } diff --git a/eval/public/cel_value.cc b/eval/public/cel_value.cc index 6aeff6d..c43864c 100644 --- a/eval/public/cel_value.cc