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 b084c9fa71196..1d3a33e40b2a3 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/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/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 91bdb9b21b586..4be71f6cee187 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, 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, *this, stats, Config::Utility::factoryForGrpcApiConfigSource( *async_client_manager_, load_stats_config, stats) ->create(), diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 306602a3b8541..99b4acce226d7 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -37,7 +37,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/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/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 7eba78672f356..170fc64e8956e 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" @@ -33,16 +34,17 @@ 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_; })); 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 +56,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 +73,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 +82,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 +107,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 +133,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 +154,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 +190,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 +223,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 +306,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 +354,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 +388,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 +418,32 @@ 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: node 'id' and 'cluster' are required. Set it either in 'node' config or via " + "--service-node and --service-cluster options."); +} + +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: node 'id' and 'cluster' are required. Set it either in 'node' config or via " + "--service-node and --service-cluster options."); +} + } // 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 515888ccb82e5..e28e076b09e37 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 2efa3b0ce7a40..c7636e2ba55e9 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_)); } @@ -150,6 +152,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 136a60a36a812..76aab8560a5e3 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/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 9e2eada48351c..2a6d07e054c5f 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. diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index e2a96d260fe33..37239700ede4b 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)); + EXPECT_TRUE(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..cb848247302c0 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_TRUE(loadstats_request.has_node()); + ASSERT_FALSE(loadstats_request.node().id().empty()); + ASSERT_FALSE(loadstats_request.node().cluster().empty()); return; } 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) {