From 80b5498d1e4d9bdc3052d6b65f149f59436a7bd8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Aug 2018 07:25:40 +0700 Subject: [PATCH 1/6] Use local_info instead Signed-off-by: Dhi Aurrahman --- source/common/upstream/cluster_manager_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index b77d7007567e6..e7a0956b5df02 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -211,7 +211,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots // Now setup ADS if needed, this might rely on a primary cluster. if (bootstrap.dynamic_resources().has_ads_config()) { ads_mux_.reset(new Config::GrpcMuxImpl( - bootstrap.node(), + local_info.node(), Config::Utility::factoryForGrpcApiConfigSource( *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats) ->create(), @@ -301,7 +301,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots if (cm_config.has_load_stats_config()) { const auto& load_stats_config = cm_config.load_stats_config(); load_stats_reporter_.reset( - new LoadStatsReporter(bootstrap.node(), *this, stats, + new LoadStatsReporter(local_info.node(), *this, stats, Config::Utility::factoryForGrpcApiConfigSource( *async_client_manager_, load_stats_config, stats) ->create(), From 6c263d3ebd2d8f16aed23b14c5ca8a892f41490f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Aug 2018 11:01:31 +0700 Subject: [PATCH 2/6] Add tests Signed-off-by: Dhi Aurrahman --- test/integration/ads_integration_test.cc | 4 ++++ test/integration/load_stats_integration_test.cc | 3 +++ 2 files changed, 7 insertions(+) diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index e2a96d260fe33..155e3efa7071a 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -132,6 +132,10 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest, envoy::api::v2::DiscoveryRequest discovery_request; VERIFY_ASSERTION(ads_stream_->waitForGrpcMessage(*dispatcher_, discovery_request)); + ASSERT(discovery_request.has_node()); + EXPECT_FALSE(discovery_request.node().id().empty()); + EXPECT_FALSE(discovery_request.node().cluster().empty()); + // TODO(PiotrSikora): Remove this hack once fixed internally. if (!(expected_type_url == discovery_request.type_url())) { return AssertionFailure() << fmt::format("type_url {} does not match expected {}", diff --git a/test/integration/load_stats_integration_test.cc b/test/integration/load_stats_integration_test.cc index a7c16e034c316..9b1916d39fe6d 100644 --- a/test/integration/load_stats_integration_test.cc +++ b/test/integration/load_stats_integration_test.cc @@ -164,6 +164,9 @@ class LoadStatsIntegrationTest : public HttpIntegrationTest, return; } else if (loadstats_request.cluster_stats_size() == 0) { loadstats_request.CopyFrom(local_loadstats_request); + ASSERT(loadstats_request.has_node()); + EXPECT_FALSE(loadstats_request.node().id().empty()); + EXPECT_FALSE(loadstats_request.node().cluster().empty()); return; } From fd3071e8cffdeb32d049b1bf9ca6f04526aca56a Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 14 Aug 2018 17:43:31 +0700 Subject: [PATCH 3/6] Use local_info for LoadStatsReporter Signed-off-by: Dhi Aurrahman --- source/common/upstream/cluster_manager_impl.cc | 2 +- source/common/upstream/load_stats_reporter.cc | 4 ++-- source/common/upstream/load_stats_reporter.h | 2 +- test/common/upstream/BUILD | 1 + test/common/upstream/load_stats_reporter_test.cc | 14 +++++++------- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index e7a0956b5df02..a3d1350ac0e64 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -301,7 +301,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots if (cm_config.has_load_stats_config()) { const auto& load_stats_config = cm_config.load_stats_config(); load_stats_reporter_.reset( - new LoadStatsReporter(local_info.node(), *this, stats, + new LoadStatsReporter(local_info, *this, stats, Config::Utility::factoryForGrpcApiConfigSource( *async_client_manager_, load_stats_config, stats) ->create(), diff --git a/source/common/upstream/load_stats_reporter.cc b/source/common/upstream/load_stats_reporter.cc index 1d49f3d78ca2f..5d54cf511540b 100644 --- a/source/common/upstream/load_stats_reporter.cc +++ b/source/common/upstream/load_stats_reporter.cc @@ -7,7 +7,7 @@ namespace Envoy { namespace Upstream { -LoadStatsReporter::LoadStatsReporter(const envoy::api::v2::core::Node& node, +LoadStatsReporter::LoadStatsReporter(const LocalInfo::LocalInfo& local_info, ClusterManager& cluster_manager, Stats::Scope& scope, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, @@ -18,7 +18,7 @@ LoadStatsReporter::LoadStatsReporter(const envoy::api::v2::core::Node& node, service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.load_stats.v2.LoadReportingService.StreamLoadStats")), time_source_(time_source) { - request_.mutable_node()->MergeFrom(node); + request_.mutable_node()->MergeFrom(local_info.node()); retry_timer_ = dispatcher.createTimer([this]() -> void { establishNewStream(); }); response_timer_ = dispatcher.createTimer([this]() -> void { sendLoadStatsRequest(); }); establishNewStream(); diff --git a/source/common/upstream/load_stats_reporter.h b/source/common/upstream/load_stats_reporter.h index 9e78163a3ac09..50d4c791c4444 100644 --- a/source/common/upstream/load_stats_reporter.h +++ b/source/common/upstream/load_stats_reporter.h @@ -33,7 +33,7 @@ class LoadStatsReporter : Grpc::TypedAsyncStreamCallbacks, Logger::Loggable { public: - LoadStatsReporter(const envoy::api::v2::core::Node& node, ClusterManager& cluster_manager, + LoadStatsReporter(const LocalInfo::LocalInfo& local_info, ClusterManager& cluster_manager, Stats::Scope& scope, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, MonotonicTimeSource& time_source); diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 79974a524248d..c76a2eed872c7 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -154,6 +154,7 @@ envoy_cc_test( "//source/common/upstream:load_stats_reporter_lib", "//test/mocks/event:event_mocks", "//test/mocks/grpc:grpc_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:eds_cc", diff --git a/test/common/upstream/load_stats_reporter_test.cc b/test/common/upstream/load_stats_reporter_test.cc index 36be333d5b1d5..361abe07943ae 100644 --- a/test/common/upstream/load_stats_reporter_test.cc +++ b/test/common/upstream/load_stats_reporter_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/grpc/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" @@ -26,9 +27,7 @@ class LoadStatsReporterTest : public testing::Test { public: LoadStatsReporterTest() : retry_timer_(new Event::MockTimer()), response_timer_(new Event::MockTimer()), - async_client_(new Grpc::MockAsyncClient()) { - node_.set_id("baz"); - } + async_client_(new Grpc::MockAsyncClient()) {} void createLoadStatsReporter() { InSequence s; @@ -40,14 +39,15 @@ class LoadStatsReporterTest : public testing::Test { response_timer_cb_ = timer_cb; return response_timer_; })); - load_stats_reporter_.reset(new LoadStatsReporter( - node_, cm_, stats_store_, Grpc::AsyncClientPtr(async_client_), dispatcher_, time_source_)); + load_stats_reporter_.reset(new LoadStatsReporter(local_info_, cm_, stats_store_, + Grpc::AsyncClientPtr(async_client_), + dispatcher_, time_source_)); } void expectSendMessage( const std::vector& expected_cluster_stats) { envoy::service::load_stats::v2::LoadStatsRequest expected_request; - expected_request.mutable_node()->MergeFrom(node_); + expected_request.mutable_node()->MergeFrom(local_info_.node()); std::copy(expected_cluster_stats.begin(), expected_cluster_stats.end(), Protobuf::RepeatedPtrFieldBackInserter(expected_request.mutable_cluster_stats())); EXPECT_CALL(async_stream_, sendMessage(ProtoEq(expected_request), false)); @@ -64,7 +64,6 @@ class LoadStatsReporterTest : public testing::Test { load_stats_reporter_->onReceiveMessage(std::move(response)); } - envoy::api::v2::core::Node node_; NiceMock cm_; Event::MockDispatcher dispatcher_; Stats::IsolatedStoreImpl stats_store_; @@ -76,6 +75,7 @@ class LoadStatsReporterTest : public testing::Test { Grpc::MockAsyncStream async_stream_; Grpc::MockAsyncClient* async_client_; MockMonotonicTimeSource time_source_; + NiceMock local_info_; }; // Validate that stream creation results in a timer based retry. From 1792e220f85eb32caa0e0e4839fd36cf4deaa0e0 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 15 Aug 2018 03:50:58 +0700 Subject: [PATCH 4/6] Use local_info everywhere Signed-off-by: Dhi Aurrahman --- source/common/config/grpc_mux_impl.cc | 9 ++-- source/common/config/grpc_mux_impl.h | 4 +- source/common/config/grpc_subscription_impl.h | 4 +- source/common/config/http_subscription_impl.h | 4 +- source/common/config/subscription_factory.h | 8 ++-- source/common/router/rds_impl.cc | 2 +- source/common/upstream/cds_api_impl.cc | 2 +- .../common/upstream/cluster_manager_impl.cc | 2 +- source/common/upstream/eds.cc | 2 +- source/server/lds_api.cc | 2 +- test/common/config/BUILD | 4 ++ test/common/config/grpc_mux_impl_test.cc | 47 +++++++++++++++++-- .../config/grpc_subscription_test_harness.h | 9 ++-- .../config/http_subscription_test_harness.h | 5 +- .../config/subscription_factory_test.cc | 5 +- test/integration/ads_integration_test.cc | 2 +- .../load_stats_integration_test.cc | 6 +-- 17 files changed, 84 insertions(+), 33 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 77b9fc84dc345..3cd1008a5a543 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -9,12 +9,13 @@ namespace Envoy { namespace Config { -GrpcMuxImpl::GrpcMuxImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client, +GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, Runtime::RandomGenerator& random, MonotonicTimeSource& time_source) - : node_(node), async_client_(std::move(async_client)), service_method_(service_method), - random_(random), time_source_(time_source) { + : local_info_(local_info), async_client_(std::move(async_client)), + service_method_(service_method), random_(random), time_source_(time_source) { + Config::Utility::checkLocalInfo("ads", local_info); retry_timer_ = dispatcher.createTimer([this]() -> void { establishNewStream(); }); backoff_strategy_ = std::make_unique(RETRY_INITIAL_DELAY_MS, RETRY_MAX_DELAY_MS, random_); @@ -114,7 +115,7 @@ GrpcMuxWatchPtr GrpcMuxImpl::subscribe(const std::string& type_url, // Bucket contains 1 token maximum and refills 1 token on every ~5 seconds. api_state_[type_url].limit_log_ = std::make_unique(1, 0.2, time_source_); api_state_[type_url].request_.set_type_url(type_url); - api_state_[type_url].request_.mutable_node()->MergeFrom(node_); + api_state_[type_url].request_.mutable_node()->MergeFrom(local_info_.node()); api_state_[type_url].subscribed_ = true; subscriptions_.emplace_back(type_url); } diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 5f2c99c2e6168..155d354404a4a 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -24,7 +24,7 @@ class GrpcMuxImpl : public GrpcMux, Grpc::TypedAsyncStreamCallbacks, Logger::Loggable { public: - GrpcMuxImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client, + GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, Runtime::RandomGenerator& random, MonotonicTimeSource& time_source = ProdMonotonicTimeSource::instance_); @@ -95,7 +95,7 @@ class GrpcMuxImpl : public GrpcMux, TokenBucketPtr limit_log_; }; - envoy::api::v2::core::Node node_; + const LocalInfo::LocalInfo& local_info_; Grpc::AsyncClientPtr async_client_; Grpc::AsyncStream* stream_{}; const Protobuf::MethodDescriptor& service_method_; diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index 6117b06b7cd10..442a9b4fab02b 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -14,10 +14,10 @@ namespace Config { template class GrpcSubscriptionImpl : public Config::Subscription { public: - GrpcSubscriptionImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client, + GrpcSubscriptionImpl(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, const Protobuf::MethodDescriptor& service_method, SubscriptionStats stats) - : grpc_mux_(node, std::move(async_client), dispatcher, service_method, random), + : grpc_mux_(local_info, std::move(async_client), dispatcher, service_method, random), grpc_mux_subscription_(grpc_mux_, stats) {} // Config::Subscription diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index b0d0ac94f0597..62149c81e49a8 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -30,7 +30,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, public Config::Subscription, Logger::Loggable { public: - HttpSubscriptionImpl(const envoy::api::v2::core::Node& node, Upstream::ClusterManager& cm, + HttpSubscriptionImpl(const LocalInfo::LocalInfo& local_info, Upstream::ClusterManager& cm, const std::string& remote_cluster_name, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, std::chrono::milliseconds refresh_interval, std::chrono::milliseconds request_timeout, @@ -38,7 +38,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, : Http::RestApiFetcher(cm, remote_cluster_name, dispatcher, random, refresh_interval, request_timeout), stats_(stats) { - request_.mutable_node()->CopyFrom(node); + request_.mutable_node()->CopyFrom(local_info.node()); ASSERT(service_method.options().HasExtension(google::api::http)); const auto& http_rule = service_method.options().GetExtension(google::api::http); path_ = http_rule.post(); diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h index 9d1bccbd1f48a..21ae70d491312 100644 --- a/source/common/config/subscription_factory.h +++ b/source/common/config/subscription_factory.h @@ -23,7 +23,7 @@ class SubscriptionFactory { /** * Subscription factory. * @param config envoy::api::v2::core::ConfigSource to construct from. - * @param node envoy::api::v2::core::Node identifier. + * @param local_info LocalInfo::LocalInfo local info. * @param dispatcher event dispatcher. * @param cm cluster manager for async clients (when REST/gRPC). * @param random random generator for jittering polling delays (when REST). @@ -37,7 +37,7 @@ class SubscriptionFactory { */ template static std::unique_ptr> subscriptionFromConfigSource( - const envoy::api::v2::core::ConfigSource& config, const envoy::api::v2::core::Node& node, + const envoy::api::v2::core::ConfigSource& config, const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, Stats::Scope& scope, std::function*()> rest_legacy_constructor, const std::string& rest_method, const std::string& grpc_method) { @@ -59,14 +59,14 @@ class SubscriptionFactory { break; case envoy::api::v2::core::ApiConfigSource::REST: result.reset(new HttpSubscriptionImpl( - node, cm, api_config_source.cluster_names()[0], dispatcher, random, + local_info, cm, api_config_source.cluster_names()[0], dispatcher, random, Utility::apiConfigSourceRefreshDelay(api_config_source), Utility::apiConfigSourceRequestTimeout(api_config_source), *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), stats)); break; case envoy::api::v2::core::ApiConfigSource::GRPC: { result.reset(new GrpcSubscriptionImpl( - node, + local_info, Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(), config.api_config_source(), scope) ->create(), diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 6a3e8c545cae5..7f6d8184ffbb8 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -70,7 +70,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< envoy::api::v2::RouteConfiguration>( - rds.config_source(), factory_context.localInfo().node(), factory_context.dispatcher(), + rds.config_source(), factory_context.localInfo(), factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), *scope_, [this, &rds, &factory_context]() -> Envoy::Config::Subscription* { diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index d1422a76eee5f..dead9260ef7e3 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -35,7 +35,7 @@ CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource( - cds_config, local_info.node(), dispatcher, cm, random, *scope_, + cds_config, local_info, dispatcher, cm, random, *scope_, [this, &cds_config, &eds_config, &cm, &dispatcher, &random, &local_info, &scope]() -> Config::Subscription* { return new CdsSubscription(Config::Utility::generateStats(*scope_), cds_config, diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index a3d1350ac0e64..e2ab34ed61d25 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -211,7 +211,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots // Now setup ADS if needed, this might rely on a primary cluster. if (bootstrap.dynamic_resources().has_ads_config()) { ads_mux_.reset(new Config::GrpcMuxImpl( - local_info.node(), + local_info, Config::Utility::factoryForGrpcApiConfigSource( *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats) ->create(), diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index fe510abae6793..31ec442070c7f 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -36,7 +36,7 @@ EdsClusterImpl::EdsClusterImpl( Upstream::ClusterManager& cm = factory_context.clusterManager(); subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource< envoy::api::v2::ClusterLoadAssignment>( - eds_config, local_info_.node(), dispatcher, cm, random, info_->statsScope(), + eds_config, local_info_, dispatcher, cm, random, info_->statsScope(), [this, &eds_config, &cm, &dispatcher, &random]() -> Config::Subscription* { return new SdsSubscription(info_->stats(), eds_config, cm, dispatcher, random); diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 73e98607e6b83..f4eb67be0d297 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -25,7 +25,7 @@ LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm) { subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( - lds_config, local_info.node(), dispatcher, cm, random, *scope_, + lds_config, local_info, dispatcher, cm, random, *scope_, [this, &lds_config, &cm, &dispatcher, &random, &local_info, &scope]() -> Config::Subscription* { return new LdsSubscription(Config::Utility::generateStats(*scope_), lds_config, cm, diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 555bc63f7ec76..e57d4f569f413 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -47,6 +47,7 @@ envoy_cc_test( "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", "//test/mocks/grpc:grpc_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks", "//test/test_common:logging_lib", "//test/test_common:utility_lib", @@ -75,6 +76,7 @@ envoy_cc_test_library( "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", "//test/mocks/grpc:grpc_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:eds_cc", @@ -101,6 +103,7 @@ envoy_cc_test_library( "//source/common/http:message_lib", "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", @@ -116,6 +119,7 @@ envoy_cc_test( "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", "//test/mocks/filesystem:filesystem_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/stats:stats_mocks", "//test/mocks/upstream:upstream_mocks", diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index d0a1be52d76e3..073eac5c11ea4 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -10,6 +10,7 @@ #include "test/mocks/config/mocks.h" #include "test/mocks/event/mocks.h" #include "test/mocks/grpc/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/test_common/logging.h" #include "test/test_common/utility.h" @@ -35,14 +36,16 @@ class GrpcMuxImplTest : public testing::Test { public: GrpcMuxImplTest() : async_client_(new Grpc::MockAsyncClient()), timer_(new Event::MockTimer()), time_source_{} { + } + + void setup() { EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb timer_cb) { timer_cb_ = timer_cb; return timer_; })); grpc_mux_.reset(new GrpcMuxImpl( - envoy::api::v2::core::Node(), std::unique_ptr(async_client_), - dispatcher_, + local_info_, std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, time_source_)); @@ -54,7 +57,7 @@ class GrpcMuxImplTest : public testing::Test { const Protobuf::int32 error_code = Grpc::Status::GrpcStatus::Ok, const std::string& error_message = "") { envoy::api::v2::DiscoveryRequest expected_request; - expected_request.mutable_node()->CopyFrom(node_); + expected_request.mutable_node()->CopyFrom(local_info_.node()); for (const auto& resource : resource_names) { expected_request.add_resource_names(resource); } @@ -71,7 +74,6 @@ class GrpcMuxImplTest : public testing::Test { EXPECT_CALL(async_stream_, sendMessage(ProtoEq(expected_request), false)); } - envoy::api::v2::core::Node node_; NiceMock dispatcher_; Runtime::MockRandomGenerator random_; Grpc::MockAsyncClient* async_client_; @@ -81,11 +83,13 @@ class GrpcMuxImplTest : public testing::Test { std::unique_ptr grpc_mux_; NiceMock callbacks_; NiceMock time_source_; + NiceMock local_info_; }; // Validate behavior when multiple type URL watches are maintained, watches are created/destroyed // (via RAII). TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) { + setup(); InSequence s; auto foo_sub = grpc_mux_->subscribe("foo", {"x", "y"}, callbacks_); auto bar_sub = grpc_mux_->subscribe("bar", {}, callbacks_); @@ -104,6 +108,7 @@ TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) { // Validate behavior when multiple type URL watches are maintained and the stream is reset. TEST_F(GrpcMuxImplTest, ResetStream) { + setup(); InSequence s; auto foo_sub = grpc_mux_->subscribe("foo", {"x", "y"}, callbacks_); auto bar_sub = grpc_mux_->subscribe("bar", {}, callbacks_); @@ -129,6 +134,7 @@ TEST_F(GrpcMuxImplTest, ResetStream) { // Validate pause-resume behavior. TEST_F(GrpcMuxImplTest, PauseResume) { + setup(); InSequence s; auto foo_sub = grpc_mux_->subscribe("foo", {"x", "y"}, callbacks_); grpc_mux_->pause("foo"); @@ -149,6 +155,7 @@ TEST_F(GrpcMuxImplTest, PauseResume) { // Validate behavior when type URL mismatches occur. TEST_F(GrpcMuxImplTest, TypeUrlMismatch) { + setup(); std::unique_ptr invalid_response( new envoy::api::v2::DiscoveryResponse()); @@ -184,6 +191,8 @@ TEST_F(GrpcMuxImplTest, TypeUrlMismatch) { // Validate behavior when watches has an unknown resource name. TEST_F(GrpcMuxImplTest, WildcardWatch) { + setup(); + InSequence s; const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; auto foo_sub = grpc_mux_->subscribe(type_url, {}, callbacks_); @@ -215,6 +224,8 @@ TEST_F(GrpcMuxImplTest, WildcardWatch) { // Validate behavior when watches specify resources (potentially overlapping). TEST_F(GrpcMuxImplTest, WatchDemux) { + setup(); + InSequence s; const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; NiceMock foo_callbacks; @@ -296,6 +307,8 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { // Verifies that warning messages get logged when Envoy detects too many requests. TEST_F(GrpcMuxImplTest, TooManyRequests) { + setup(); + EXPECT_CALL(async_stream_, sendMessage(_, false)).Times(AtLeast(100)); EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); EXPECT_CALL(time_source_, currentTime()) @@ -342,6 +355,8 @@ TEST_F(GrpcMuxImplTest, TooManyRequests) { // Verifies that a messsage with no resources is accepted. TEST_F(GrpcMuxImplTest, UnwatchedTypeAcceptsEmptyResources) { + setup(); + EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; @@ -374,6 +389,8 @@ TEST_F(GrpcMuxImplTest, UnwatchedTypeAcceptsEmptyResources) { // Verifies that a messsage with some resources is rejected when there are no watches. TEST_F(GrpcMuxImplTest, UnwatchedTypeRejectsResources) { + setup(); + EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; @@ -402,6 +419,28 @@ TEST_F(GrpcMuxImplTest, UnwatchedTypeRejectsResources) { grpc_mux_->onReceiveMessage(std::move(response))); } +TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyClusterName) { + EXPECT_CALL(local_info_, clusterName()).WillOnce(Return("")); + EXPECT_THROW_WITH_MESSAGE( + GrpcMuxImpl( + local_info_, std::unique_ptr(async_client_), dispatcher_, + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), + random_, time_source_), + EnvoyException, "ads: setting --service-cluster and --service-node is required"); +} + +TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { + EXPECT_CALL(local_info_, nodeName()).WillOnce(Return("")); + EXPECT_THROW_WITH_MESSAGE( + GrpcMuxImpl( + local_info_, std::unique_ptr(async_client_), dispatcher_, + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), + random_, time_source_), + EnvoyException, "ads: setting --service-cluster and --service-node is required"); +} + } // namespace } // namespace Config } // namespace Envoy diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index 06e22e0ef28a1..83a02d8f82c92 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -10,6 +10,7 @@ #include "test/mocks/config/mocks.h" #include "test/mocks/event/mocks.h" #include "test/mocks/grpc/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" @@ -34,13 +35,14 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints")), async_client_(new Grpc::MockAsyncClient()), timer_(new Event::MockTimer()) { node_.set_id("fo0"); + EXPECT_CALL(local_info_, node()).WillOnce(testing::ReturnRef(node_)); EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb timer_cb) { timer_cb_ = timer_cb; return timer_; })); - subscription_.reset( - new GrpcEdsSubscriptionImpl(node_, std::unique_ptr(async_client_), - dispatcher_, random_, *method_descriptor_, stats_)); + subscription_.reset(new GrpcEdsSubscriptionImpl( + local_info_, std::unique_ptr(async_client_), dispatcher_, random_, + *method_descriptor_, stats_)); } ~GrpcSubscriptionTestHarness() { EXPECT_CALL(async_stream_, sendMessage(_, false)); } @@ -138,6 +140,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { std::unique_ptr subscription_; std::string last_response_nonce_; std::vector last_cluster_names_; + NiceMock local_info_; }; // TODO(danielhochman): test with RDS and ensure version_info is same as what API returned diff --git a/test/common/config/http_subscription_test_harness.h b/test/common/config/http_subscription_test_harness.h index 0853b50c8c7db..2041c28176461 100644 --- a/test/common/config/http_subscription_test_harness.h +++ b/test/common/config/http_subscription_test_harness.h @@ -10,6 +10,7 @@ #include "test/common/config/subscription_test_harness.h" #include "test/mocks/config/mocks.h" #include "test/mocks/event/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" @@ -33,12 +34,13 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints")), timer_(new Event::MockTimer()), http_request_(&cm_.async_client_) { node_.set_id("fo0"); + EXPECT_CALL(local_info_, node()).WillOnce(testing::ReturnRef(node_)); EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb timer_cb) { timer_cb_ = timer_cb; return timer_; })); subscription_.reset(new HttpEdsSubscriptionImpl( - node_, cm_, "eds_cluster", dispatcher_, random_gen_, std::chrono::milliseconds(1), + local_info_, cm_, "eds_cluster", dispatcher_, random_gen_, std::chrono::milliseconds(1), std::chrono::milliseconds(1000), *method_descriptor_, stats_)); } @@ -146,6 +148,7 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { Http::AsyncClient::Callbacks* http_callbacks_; Config::MockSubscriptionCallbacks callbacks_; std::unique_ptr subscription_; + NiceMock local_info_; }; } // namespace Config diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_test.cc index b9155566914b1..019df55813c93 100644 --- a/test/common/config/subscription_factory_test.cc +++ b/test/common/config/subscription_factory_test.cc @@ -7,6 +7,7 @@ #include "test/mocks/config/mocks.h" #include "test/mocks/event/mocks.h" #include "test/mocks/filesystem/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -32,7 +33,7 @@ class SubscriptionFactoryTest : public ::testing::Test { std::unique_ptr> subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config) { return SubscriptionFactory::subscriptionFromConfigSource( - config, node_, dispatcher_, cm_, random_, stats_store_, + config, local_info_, dispatcher_, cm_, random_, stats_store_, [this]() -> Subscription* { return legacy_subscription_.release(); }, @@ -40,7 +41,6 @@ class SubscriptionFactoryTest : public ::testing::Test { "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints"); } - envoy::api::v2::core::Node node_; Upstream::MockClusterManager cm_; Event::MockDispatcher dispatcher_; Runtime::MockRandomGenerator random_; @@ -48,6 +48,7 @@ class SubscriptionFactoryTest : public ::testing::Test { std::unique_ptr> legacy_subscription_; Http::MockAsyncClientRequest http_request_; Stats::MockIsolatedStatsStore stats_store_; + NiceMock local_info_; }; class SubscriptionFactoryTestApiConfigSource diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 155e3efa7071a..37239700ede4b 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -132,7 +132,7 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest, envoy::api::v2::DiscoveryRequest discovery_request; VERIFY_ASSERTION(ads_stream_->waitForGrpcMessage(*dispatcher_, discovery_request)); - ASSERT(discovery_request.has_node()); + EXPECT_TRUE(discovery_request.has_node()); EXPECT_FALSE(discovery_request.node().id().empty()); EXPECT_FALSE(discovery_request.node().cluster().empty()); diff --git a/test/integration/load_stats_integration_test.cc b/test/integration/load_stats_integration_test.cc index 9b1916d39fe6d..cb848247302c0 100644 --- a/test/integration/load_stats_integration_test.cc +++ b/test/integration/load_stats_integration_test.cc @@ -164,9 +164,9 @@ class LoadStatsIntegrationTest : public HttpIntegrationTest, return; } else if (loadstats_request.cluster_stats_size() == 0) { loadstats_request.CopyFrom(local_loadstats_request); - ASSERT(loadstats_request.has_node()); - EXPECT_FALSE(loadstats_request.node().id().empty()); - EXPECT_FALSE(loadstats_request.node().cluster().empty()); + ASSERT_TRUE(loadstats_request.has_node()); + ASSERT_FALSE(loadstats_request.node().id().empty()); + ASSERT_FALSE(loadstats_request.node().cluster().empty()); return; } From 0655cf849bdfa6aa57f257cd59c880bc234d4e63 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 15 Aug 2018 06:01:51 +0700 Subject: [PATCH 5/6] Make sure timer is owned Signed-off-by: Dhi Aurrahman --- test/common/config/grpc_mux_impl_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 073eac5c11ea4..1f944c48cb28d 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -34,13 +34,12 @@ namespace { // is provided in [grpc_]subscription_impl_test.cc. class GrpcMuxImplTest : public testing::Test { public: - GrpcMuxImplTest() - : async_client_(new Grpc::MockAsyncClient()), timer_(new Event::MockTimer()), time_source_{} { - } + GrpcMuxImplTest() : async_client_(new Grpc::MockAsyncClient()), time_source_{} {} void setup() { EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb timer_cb) { timer_cb_ = timer_cb; + timer_ = new Event::MockTimer(); return timer_; })); From 8b0dd55ad862f58f14d885a01be29b555b395fab Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sun, 19 Aug 2018 09:30:33 +0700 Subject: [PATCH 6/6] review: change the thrown error message Signed-off-by: Dhi Aurrahman --- source/common/config/utility.cc | 4 +++- test/common/config/grpc_mux_impl_test.cc | 8 ++++++-- test/server/lds_api_test.cc | 10 ++++++---- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 3d848fd24c2f3..36fa703d21b02 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -69,7 +69,9 @@ void Utility::checkLocalInfo(const std::string& error_prefix, const LocalInfo::LocalInfo& local_info) { if (local_info.clusterName().empty() || local_info.nodeName().empty()) { throw EnvoyException( - fmt::format("{}: setting --service-cluster and --service-node is required", error_prefix)); + fmt::format("{}: node 'id' and 'cluster' are required. Set it either in 'node' config or " + "via --service-node and --service-cluster options.", + error_prefix, local_info.node().DebugString())); } } diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index d8ad198f146f2..170fc64e8956e 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -426,7 +426,9 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyClusterName) { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, time_source_), - EnvoyException, "ads: setting --service-cluster and --service-node is required"); + EnvoyException, + "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " + "--service-node and --service-cluster options."); } TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { @@ -437,7 +439,9 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, time_source_), - EnvoyException, "ads: setting --service-cluster and --service-node is required"); + EnvoyException, + "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " + "--service-node and --service-cluster options."); } } // namespace diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 7c39a8235e30f..189789a91a65a 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -224,10 +224,12 @@ TEST_F(LdsApiTest, BadLocalInfo) { EXPECT_CALL(*cluster.info_, addedViaApi()); EXPECT_CALL(*cluster.info_, type()); ON_CALL(local_info_, clusterName()).WillByDefault(Return(std::string())); - EXPECT_THROW_WITH_MESSAGE(LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_, - local_info_, store_, listener_manager_), - EnvoyException, - "lds: setting --service-cluster and --service-node is required"); + EXPECT_THROW_WITH_MESSAGE( + LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_, local_info_, store_, + listener_manager_), + EnvoyException, + "lds: node 'id' and 'cluster' are required. Set it either in 'node' config or via " + "--service-node and --service-cluster options."); } TEST_F(LdsApiTest, Basic) {