From 6b2391bd181fda073d221125cb9c12cae6625c63 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 4 Jun 2019 17:38:36 -0400 Subject: [PATCH] config: subscription factory dependency injection. While working on TDS, it became apparent that the xDS subscription tests are a bit conflated today, since they include both testing of the xDS resource specific subscription and also things like the REST fetcher, etc. This is because the subscription factory is a static, rather than a mockable dependency of subscriptions. This PR transforms subscription factory to an instance object. The PR is quite massive, but it has big pay-off in terms of reducing the boiler plate in xDS tests, separating concerns and making it easier to add new xDS types. Given the recent additions of SRDS, VHDS, TDS and FCDS, this seems worth it. A number of xDS tests have been modified to work with the new pattern. Some tests and properties that are orthogonally tested in subscription_factory_test or the gRPC/REST subscription tests are no longer tested in the resource specified tests. Some general absl::string_view goodness plumbing was also needed due to the new string_view-based interface. Risk level: Low Testing: Modified a number of resource specified xDS unit tests. Relates to #6708. Signed-off-by: Harvey Tuch --- include/envoy/config/BUILD | 9 + include/envoy/config/subscription.h | 2 + include/envoy/config/subscription_factory.h | 33 +++ include/envoy/upstream/BUILD | 1 + include/envoy/upstream/cluster_manager.h | 16 +- source/common/config/BUILD | 5 +- source/common/config/subscription_factory.h | 42 --- ...actory.cc => subscription_factory_impl.cc} | 45 ++-- .../common/config/subscription_factory_impl.h | 34 +++ source/common/config/utility.cc | 9 +- source/common/config/utility.h | 9 +- source/common/router/BUILD | 2 - source/common/router/rds_impl.cc | 12 +- source/common/router/rds_impl.h | 3 +- source/common/router/scoped_rds.cc | 14 +- source/common/router/scoped_rds.h | 10 +- source/common/router/vhds.cc | 15 +- source/common/router/vhds.h | 15 +- source/common/secret/BUILD | 2 +- source/common/secret/sds_api.cc | 29 +- source/common/secret/sds_api.h | 85 +++--- source/common/upstream/BUILD | 5 +- source/common/upstream/cds_api_impl.cc | 54 ++-- source/common/upstream/cds_api_impl.h | 12 +- .../common/upstream/cluster_factory_impl.cc | 2 - .../common/upstream/cluster_manager_impl.cc | 17 +- source/common/upstream/cluster_manager_impl.h | 11 +- source/common/upstream/eds.cc | 14 +- source/common/upstream/eds.h | 4 +- source/server/BUILD | 2 +- .../config_validation/cluster_manager.cc | 8 +- .../config_validation/cluster_manager.h | 5 +- source/server/config_validation/server.h | 5 +- source/server/lds_api.cc | 17 +- source/server/lds_api.h | 10 +- source/server/listener_manager_impl.h | 7 +- test/common/config/BUILD | 4 +- ...t.cc => subscription_factory_impl_test.cc} | 9 +- .../grpc_client_integration_test_harness.h | 2 +- test/common/router/BUILD | 5 +- test/common/router/config_impl_test.cc | 18 +- test/common/router/rds_impl_test.cc | 252 ++++------------- test/common/router/scoped_rds_test.cc | 112 +------- test/common/router/shadow_writer_impl_test.cc | 5 +- test/common/router/vhds_test.cc | 51 ++-- test/common/secret/BUILD | 1 + test/common/secret/sds_api_test.cc | 145 +++++----- .../common/secret/secret_manager_impl_test.cc | 15 +- test/common/upstream/BUILD | 3 - test/common/upstream/cds_api_impl_test.cc | 228 ++++------------ .../upstream/cluster_manager_impl_test.cc | 12 + test/common/upstream/eds_test.cc | 133 ++++----- .../http/health_check/health_check_test.cc | 21 +- .../filters/http/lua/lua_filter_test.cc | 23 +- .../filters/http/squash/squash_filter_test.cc | 11 +- .../client_ssl_auth/client_ssl_auth_test.cc | 5 +- .../network/dubbo_proxy/router_test.cc | 3 +- .../redis_proxy/conn_pool_impl_test.cc | 2 +- .../network/thrift_proxy/router_test.cc | 3 +- .../extensions/tracers/datadog/config_test.cc | 3 +- .../datadog/datadog_tracer_impl_test.cc | 7 +- .../tracers/dynamic_ot/config_test.cc | 3 +- .../tracers/lightstep/config_test.cc | 3 +- .../lightstep/lightstep_tracer_impl_test.cc | 9 +- test/extensions/tracers/zipkin/config_test.cc | 6 +- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 7 +- .../tls/context_impl_test.cc | 32 --- .../transport_sockets/tls/ssl_socket_test.cc | 16 -- test/integration/stats_integration_test.cc | 3 +- test/mocks/config/mocks.cc | 16 ++ test/mocks/config/mocks.h | 15 ++ test/mocks/server/mocks.cc | 1 + test/mocks/server/mocks.h | 1 + test/mocks/upstream/mocks.cc | 4 +- test/mocks/upstream/mocks.h | 4 +- test/server/BUILD | 1 + test/server/lds_api_test.cc | 254 +++--------------- 77 files changed, 733 insertions(+), 1280 deletions(-) create mode 100644 include/envoy/config/subscription_factory.h delete mode 100644 source/common/config/subscription_factory.h rename source/common/config/{subscription_factory.cc => subscription_factory_impl.cc} (66%) create mode 100644 source/common/config/subscription_factory_impl.h rename test/common/config/{subscription_factory_test.cc => subscription_factory_impl_test.cc} (98%) diff --git a/include/envoy/config/BUILD b/include/envoy/config/BUILD index 777cf7a32fb2..ab8c3a0899f6 100644 --- a/include/envoy/config/BUILD +++ b/include/envoy/config/BUILD @@ -38,6 +38,15 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "subscription_factory_interface", + hdrs = ["subscription_factory.h"], + deps = [ + ":subscription_interface", + "@envoy_api//envoy/api/v2/core:config_source_cc", + ], +) + envoy_cc_library( name = "subscription_interface", hdrs = ["subscription.h"], diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index 707fab98ea3b..822a3eec68a6 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -79,6 +79,8 @@ class Subscription { virtual void updateResources(const std::set& update_to_these_names) PURE; }; +using SubscriptionPtr = std::unique_ptr; + /** * Per subscription stats. @see stats_macros.h */ diff --git a/include/envoy/config/subscription_factory.h b/include/envoy/config/subscription_factory.h new file mode 100644 index 000000000000..d5d6b5394b01 --- /dev/null +++ b/include/envoy/config/subscription_factory.h @@ -0,0 +1,33 @@ +#pragma once + +#include "envoy/api/api.h" +#include "envoy/api/v2/core/base.pb.h" +#include "envoy/config/subscription.h" +#include "envoy/stats/scope.h" +#include "envoy/upstream/cluster_manager.h" + +namespace Envoy { +namespace Config { + +class SubscriptionFactory { +public: + virtual ~SubscriptionFactory() {} + + /** + * Subscription factory interface. + * + * @param config envoy::api::v2::core::ConfigSource to construct from. + * @param type_url type URL for the resource being subscribed to. + * @param scope stats scope for any stats tracked by the subscription. + * @param callbacks the callbacks needed by all Subscription objects, to deliver config updates. + * The callbacks must not result in the deletion of the Subscription object. + * @return SubscriptionPtr subscription object corresponding for config and type_url. + */ + virtual SubscriptionPtr + subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config, + absl::string_view type_url, Stats::Scope& scope, + SubscriptionCallbacks& callbacks) PURE; +}; + +} // namespace Config +} // namespace Envoy diff --git a/include/envoy/upstream/BUILD b/include/envoy/upstream/BUILD index e20c85e1d24e..b30d86753289 100644 --- a/include/envoy/upstream/BUILD +++ b/include/envoy/upstream/BUILD @@ -18,6 +18,7 @@ envoy_cc_library( ":upstream_interface", "//include/envoy/access_log:access_log_interface", "//include/envoy/config:grpc_mux_interface", + "//include/envoy/config:subscription_factory_interface", "//include/envoy/grpc:async_client_manager_interface", "//include/envoy/http:async_client_interface", "//include/envoy/http:conn_pool_interface", diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index 1928db4a7e9c..b376b3838bde 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -11,6 +11,7 @@ #include "envoy/api/v2/cds.pb.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.h" #include "envoy/config/grpc_mux.h" +#include "envoy/config/subscription_factory.h" #include "envoy/grpc/async_client_manager.h" #include "envoy/http/async_client.h" #include "envoy/http/conn_pool.h" @@ -29,6 +30,11 @@ #include "envoy/upstream/upstream.h" namespace Envoy { + +namespace Config { +class SubscriptionFactory; +} + namespace Upstream { /** @@ -131,7 +137,7 @@ class ClusterManager { * If information about the cluster needs to be kept, use the ThreadLocalCluster::info() method to * obtain cluster information that is safe to store. */ - virtual ThreadLocalCluster* get(const std::string& cluster) PURE; + virtual ThreadLocalCluster* get(absl::string_view cluster) PURE; /** * Allocate a load balanced HTTP connection pool for a cluster. This is *per-thread* so that @@ -235,6 +241,14 @@ class ClusterManager { virtual ClusterManagerFactory& clusterManagerFactory() PURE; + /** + * Obtain the subscription factory for the cluster manager. Since subscriptions may have an + * upstream component, the factory is a facet of the cluster manager. + * + * @return Config::SubscriptionFactory& the subscription factory. + */ + virtual Config::SubscriptionFactory& subscriptionFactory() PURE; + virtual std::size_t warmingClusterCount() const PURE; }; diff --git a/source/common/config/BUILD b/source/common/config/BUILD index f1080638fb34..6dee36decdbb 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -311,8 +311,8 @@ envoy_cc_library( envoy_cc_library( name = "subscription_factory_lib", - srcs = ["subscription_factory.cc"], - hdrs = ["subscription_factory.h"], + srcs = ["subscription_factory_impl.cc"], + hdrs = ["subscription_factory_impl.h"], deps = [ ":delta_subscription_lib", ":filesystem_subscription_lib", @@ -321,6 +321,7 @@ envoy_cc_library( ":http_subscription_lib", ":type_to_endpoint_lib", ":utility_lib", + "//include/envoy/config:subscription_factory_interface", "//include/envoy/config:subscription_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/protobuf", diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h deleted file mode 100644 index 8b557e46fc2f..000000000000 --- a/source/common/config/subscription_factory.h +++ /dev/null @@ -1,42 +0,0 @@ -#pragma once - -#include "envoy/api/api.h" -#include "envoy/api/v2/core/base.pb.h" -#include "envoy/config/subscription.h" -#include "envoy/stats/scope.h" -#include "envoy/upstream/cluster_manager.h" - -namespace Envoy { -namespace Config { - -class SubscriptionFactory { -public: - /** - * Subscription factory. - * @param config envoy::api::v2::core::ConfigSource to construct from. - * @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). - * @param scope stats scope. - * @param rest_legacy_constructor constructor function for Subscription adapters (when legacy v1 - * REST). - * @param rest_method fully qualified name of v2 REST API method (as per protobuf service - * description). - * @param grpc_method fully qualified name of v2 gRPC API bidi streaming method (as per protobuf - * service description). - * @param validation_visitor message validation visitor instance. - * @param api reference to the Api object - * @param callbacks the callbacks needed by all Subscription objects, to deliver config updates. - * The callbacks must not result in the deletion of the Subscription object. - */ - static std::unique_ptr subscriptionFromConfigSource( - const envoy::api::v2::core::ConfigSource& config, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, - Stats::Scope& scope, absl::string_view type_url, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, - SubscriptionCallbacks& callbacks); -}; - -} // namespace Config -} // namespace Envoy diff --git a/source/common/config/subscription_factory.cc b/source/common/config/subscription_factory_impl.cc similarity index 66% rename from source/common/config/subscription_factory.cc rename to source/common/config/subscription_factory_impl.cc index 677634e822c3..16647dbc3154 100644 --- a/source/common/config/subscription_factory.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -1,4 +1,4 @@ -#include "common/config/subscription_factory.h" +#include "common/config/subscription_factory_impl.h" #include "common/config/delta_subscription_impl.h" #include "common/config/filesystem_subscription_impl.h" @@ -12,24 +12,29 @@ namespace Envoy { namespace Config { -std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( - const envoy::api::v2::core::ConfigSource& config, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, - Stats::Scope& scope, absl::string_view type_url, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, - SubscriptionCallbacks& callbacks) { +SubscriptionFactoryImpl::SubscriptionFactoryImpl( + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) + : local_info_(local_info), dispatcher_(dispatcher), cm_(cm), random_(random), + validation_visitor_(validation_visitor), api_(api) {} + +SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( + const envoy::api::v2::core::ConfigSource& config, absl::string_view type_url, + Stats::Scope& scope, SubscriptionCallbacks& callbacks) { + Config::Utility::checkLocalInfo(type_url, local_info_); std::unique_ptr result; SubscriptionStats stats = Utility::generateStats(scope); switch (config.config_source_specifier_case()) { case envoy::api::v2::core::ConfigSource::kPath: { - Utility::checkFilesystemSubscriptionBackingPath(config.path(), api); + Utility::checkFilesystemSubscriptionBackingPath(config.path(), api_); result = std::make_unique( - dispatcher, config.path(), callbacks, stats, validation_visitor, api); + dispatcher_, config.path(), callbacks, stats, validation_visitor_, api_); break; } case envoy::api::v2::core::ConfigSource::kApiConfigSource: { const envoy::api::v2::core::ApiConfigSource& api_config_source = config.api_config_source(); - Utility::checkApiConfigSourceSubscriptionBackingCluster(cm.clusters(), api_config_source); + Utility::checkApiConfigSourceSubscriptionBackingCluster(cm_.clusters(), api_config_source); switch (api_config_source.api_type()) { case envoy::api::v2::core::ApiConfigSource::UNSUPPORTED_REST_LEGACY: throw EnvoyException( @@ -38,29 +43,29 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( config.DebugString()); case envoy::api::v2::core::ApiConfigSource::REST: result = std::make_unique( - local_info, 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), restMethod(type_url), - callbacks, stats, Utility::configSourceInitialFetchTimeout(config), validation_visitor); + callbacks, stats, Utility::configSourceInitialFetchTimeout(config), validation_visitor_); break; case envoy::api::v2::core::ApiConfigSource::GRPC: result = std::make_unique( - local_info, - Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(), + local_info_, + Config::Utility::factoryForGrpcApiConfigSource(cm_.grpcAsyncClientManager(), api_config_source, scope) ->create(), - dispatcher, random, sotwGrpcMethod(type_url), type_url, callbacks, stats, scope, + dispatcher_, random_, sotwGrpcMethod(type_url), type_url, callbacks, stats, scope, Utility::parseRateLimitSettings(api_config_source), Utility::configSourceInitialFetchTimeout(config)); break; case envoy::api::v2::core::ApiConfigSource::DELTA_GRPC: { - Utility::checkApiConfigSourceSubscriptionBackingCluster(cm.clusters(), api_config_source); + Utility::checkApiConfigSourceSubscriptionBackingCluster(cm_.clusters(), api_config_source); result = std::make_unique( - local_info, - Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(), + local_info_, + Config::Utility::factoryForGrpcApiConfigSource(cm_.grpcAsyncClientManager(), api_config_source, scope) ->create(), - dispatcher, deltaGrpcMethod(type_url), type_url, random, scope, + dispatcher_, deltaGrpcMethod(type_url), type_url, random_, scope, Utility::parseRateLimitSettings(api_config_source), callbacks, stats, Utility::configSourceInitialFetchTimeout(config)); break; @@ -72,7 +77,7 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( } case envoy::api::v2::core::ConfigSource::kAds: { result = std::make_unique( - cm.adsMux(), callbacks, stats, type_url, dispatcher, + cm_.adsMux(), callbacks, stats, type_url, dispatcher_, Utility::configSourceInitialFetchTimeout(config)); break; } diff --git a/source/common/config/subscription_factory_impl.h b/source/common/config/subscription_factory_impl.h new file mode 100644 index 000000000000..3a7f0ba1e31c --- /dev/null +++ b/source/common/config/subscription_factory_impl.h @@ -0,0 +1,34 @@ +#pragma once + +#include "envoy/api/api.h" +#include "envoy/api/v2/core/base.pb.h" +#include "envoy/config/subscription.h" +#include "envoy/config/subscription_factory.h" +#include "envoy/stats/scope.h" +#include "envoy/upstream/cluster_manager.h" + +namespace Envoy { +namespace Config { + +class SubscriptionFactoryImpl : public SubscriptionFactory { +public: + SubscriptionFactoryImpl(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); + + // Config::SubscriptionFactory + SubscriptionPtr subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config, + absl::string_view type_url, Stats::Scope& scope, + SubscriptionCallbacks& callbacks) override; + +private: + const LocalInfo::LocalInfo& local_info_; + Event::Dispatcher& dispatcher_; + Upstream::ClusterManager& cm_; + Runtime::RandomGenerator& random_; + ProtobufMessage::ValidationVisitor& validation_visitor_; + Api::Api& api_; +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 68aa486bde8a..d22cc0a4cca6 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -44,7 +44,7 @@ void Utility::translateApiConfigSource(const std::string& cluster, uint32_t refr Protobuf::util::TimeUtil::MillisecondsToDuration(refresh_delay_ms)); } -void Utility::checkCluster(const std::string& error_prefix, const std::string& cluster_name, +void Utility::checkCluster(absl::string_view error_prefix, absl::string_view cluster_name, Upstream::ClusterManager& cm) { Upstream::ThreadLocalCluster* cluster = cm.get(cluster_name); if (cluster == nullptr) { @@ -58,15 +58,14 @@ void Utility::checkCluster(const std::string& error_prefix, const std::string& c } } -void Utility::checkClusterAndLocalInfo(const std::string& error_prefix, - const std::string& cluster_name, - Upstream::ClusterManager& cm, +void Utility::checkClusterAndLocalInfo(absl::string_view error_prefix, + absl::string_view cluster_name, Upstream::ClusterManager& cm, const LocalInfo::LocalInfo& local_info) { checkCluster(error_prefix, cluster_name, cm); checkLocalInfo(error_prefix, local_info); } -void Utility::checkLocalInfo(const std::string& error_prefix, +void Utility::checkLocalInfo(absl::string_view error_prefix, const LocalInfo::LocalInfo& local_info) { if (local_info.clusterName().empty() || local_info.nodeName().empty()) { throw EnvoyException( diff --git a/source/common/config/utility.h b/source/common/config/utility.h index b51e8a61c243..8181e2022773 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -110,7 +110,7 @@ class Utility { * @param cluster_name supplies the cluster name to check. * @param cm supplies the cluster manager. */ - static void checkCluster(const std::string& error_prefix, const std::string& cluster_name, + static void checkCluster(absl::string_view error_prefix, absl::string_view cluster_name, Upstream::ClusterManager& cm); /** @@ -120,9 +120,8 @@ class Utility { * @param cm supplies the cluster manager. * @param local_info supplies the local info. */ - static void checkClusterAndLocalInfo(const std::string& error_prefix, - const std::string& cluster_name, - Upstream::ClusterManager& cm, + static void checkClusterAndLocalInfo(absl::string_view error_prefix, + absl::string_view cluster_name, Upstream::ClusterManager& cm, const LocalInfo::LocalInfo& local_info); /** @@ -130,7 +129,7 @@ class Utility { * @param error_prefix supplies the prefix to use in error messages. * @param local_info supplies the local info. */ - static void checkLocalInfo(const std::string& error_prefix, + static void checkLocalInfo(absl::string_view error_prefix, const LocalInfo::LocalInfo& local_info); /** diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 4bc5d19057f7..445cc2232673 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -108,7 +108,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:rds_json_lib", - "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", @@ -181,7 +180,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:config_provider_lib", - "//source/common/config:subscription_factory_lib", "@envoy_api//envoy/admin/v2alpha:config_dump_cc", "@envoy_api//envoy/api/v2:srds_cc", ], diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 5ea463ec37ce..12b5a7565802 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -65,13 +65,11 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( route_config_provider_manager_(route_config_provider_manager), manager_identifier_(manager_identifier), validation_visitor_(factory_context_.messageValidationVisitor()) { - Envoy::Config::Utility::checkLocalInfo("rds", factory_context.localInfo()); - - subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( - rds.config_source(), factory_context.localInfo(), factory_context.dispatcher(), - factory_context.clusterManager(), factory_context.random(), *scope_, - Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), - factory_context.messageValidationVisitor(), factory_context.api(), *this); + subscription_ = + factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( + rds.config_source(), + Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), + *scope_, *this); config_update_info_ = std::make_unique( factory_context.timeSource(), factory_context.messageValidationVisitor()); diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index c404b8da1e81..ee311b63a163 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -23,7 +23,6 @@ #include "envoy/thread_local/thread_local.h" #include "common/common/logger.h" -#include "common/config/subscription_factory.h" #include "common/init/target_impl.h" #include "common/protobuf/utility.h" #include "common/router/route_config_update_receiver_impl.h" @@ -108,6 +107,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, } RouteConfigUpdatePtr& routeConfigUpdate() { return config_update_info_; } +private: // Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const std::string& version_info) override; @@ -121,7 +121,6 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, .name(); } -private: RdsRouteConfigSubscription( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context, diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index b2de09f8a8e9..9694c38f78f9 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -7,7 +7,6 @@ #include "common/common/assert.h" #include "common/common/logger.h" -#include "common/config/subscription_factory.h" // Types are deeply nested under Envoy::Config::ConfigProvider; use 'using-directives' across all // ConfigProvider related types for consistency. @@ -85,13 +84,12 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription( scope_(factory_context.scope().createScope(stat_prefix + "scoped_rds." + name + ".")), stats_({ALL_SCOPED_RDS_STATS(POOL_COUNTER(*scope_))}), validation_visitor_(factory_context.messageValidationVisitor()) { - subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( - scoped_rds.scoped_rds_config_source(), factory_context.localInfo(), - factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), - *scope_, - Grpc::Common::typeUrl( - envoy::api::v2::ScopedRouteConfiguration().GetDescriptor()->full_name()), - validation_visitor_, factory_context.api(), *this); + subscription_ = + factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( + scoped_rds.scoped_rds_config_source(), + Grpc::Common::typeUrl( + envoy::api::v2::ScopedRouteConfiguration().GetDescriptor()->full_name()), + *scope_, *this); } void ScopedRdsConfigSubscription::onConfigUpdate( diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 19dab8f4482b..f0cc72f71c15 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -4,7 +4,6 @@ #include "envoy/api/v2/srds.pb.h" #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" -#include "envoy/config/subscription.h" #include "envoy/stats/scope.h" #include "common/config/config_provider_impl.h" @@ -94,6 +93,11 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio const std::string& name() const { return name_; } + const ScopedConfigManager::ScopedRouteMap& scopedRouteMap() const { + return scoped_config_manager_.scopedRouteMap(); + } + +private: // Envoy::Config::ConfigSubscriptionCommonBase void start() override { subscription_->start({}); } @@ -112,11 +116,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio validation_visitor_) .name(); } - const ScopedConfigManager::ScopedRouteMap& scopedRouteMap() const { - return scoped_config_manager_.scopedRouteMap(); - } -private: const std::string name_; std::unique_ptr subscription_; Stats::ScopePtr scope_; diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 3bd07c05fee9..100c9ef6034e 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -21,8 +21,7 @@ namespace Router { VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - std::unordered_set& route_config_providers, - SubscriptionFactoryFunction factory_function) + std::unordered_set& route_config_providers) : config_update_info_(config_update_info), init_target_(fmt::format("VhdsConfigSubscription {}", config_update_info_->routeConfigName()), [this]() { subscription_->start({}); }), @@ -31,7 +30,6 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), route_config_providers_(route_config_providers), validation_visitor_(factory_context.messageValidationVisitor()) { - Envoy::Config::Utility::checkLocalInfo("vhds", factory_context.localInfo()); const auto& config_source = config_update_info_->routeConfiguration() .vhds() .config_source() @@ -41,12 +39,11 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, throw EnvoyException("vhds: only 'DELTA_GRPC' is supported as an api_type."); } - subscription_ = factory_function( - config_update_info_->routeConfiguration().vhds().config_source(), factory_context.localInfo(), - factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), - *scope_, - Grpc::Common::typeUrl(envoy::api::v2::route::VirtualHost().GetDescriptor()->full_name()), - factory_context.messageValidationVisitor(), factory_context.api(), *this); + subscription_ = + factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( + config_update_info_->routeConfiguration().vhds().config_source(), + Grpc::Common::typeUrl(envoy::api::v2::route::VirtualHost().GetDescriptor()->full_name()), + *scope_, *this); } void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 2463b177c6bb..7c5b370babe1 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -20,7 +20,6 @@ #include "envoy/thread_local/thread_local.h" #include "common/common/logger.h" -#include "common/config/subscription_factory.h" #include "common/init/target_impl.h" #include "common/protobuf/utility.h" @@ -38,23 +37,18 @@ struct VhdsStats { ALL_VHDS_STATS(GENERATE_COUNTER_STRUCT) }; -typedef std::unique_ptr (*SubscriptionFactoryFunction)( - const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, Event::Dispatcher&, - Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, Stats::Scope&, absl::string_view, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api&, - Envoy::Config::SubscriptionCallbacks&); - class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, Logger::Loggable { public: VhdsSubscription(RouteConfigUpdatePtr& config_update_info, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - std::unordered_set& route_config_providers, - SubscriptionFactoryFunction factory_function = - Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); + std::unordered_set& route_config_providers); ~VhdsSubscription() override { init_target_.ready(); } + void registerInitTargetWithInitManager(Init::Manager& m) { m.add(init_target_); } + +private: // Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField&, const std::string&) override { @@ -68,7 +62,6 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, validation_visitor_) .name(); } - void registerInitTargetWithInitManager(Init::Manager& m) { m.add(init_target_); } RouteConfigUpdatePtr& config_update_info_; std::unique_ptr subscription_; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index a876b3f69788..ca30d94ddfb3 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -40,6 +40,7 @@ envoy_cc_library( srcs = ["sds_api.cc"], hdrs = ["sds_api.h"], deps = [ + "//include/envoy/config:subscription_factory_interface", "//include/envoy/config:subscription_interface", "//include/envoy/event:dispatcher_interface", "//include/envoy/init:manager_interface", @@ -51,7 +52,6 @@ envoy_cc_library( "//source/common/common:callback_impl_lib", "//source/common/common:cleanup_lib", "//source/common/config:resources_lib", - "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 657aa9d3d54e..5b5ea85395fc 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -5,25 +5,20 @@ #include "envoy/api/v2/auth/cert.pb.validate.h" #include "common/config/resources.h" -#include "common/config/subscription_factory.h" -#include "common/config/utility.h" #include "common/protobuf/utility.h" namespace Envoy { namespace Secret { -SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Stats::Store& stats, - Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, - const std::string& sds_config_name, std::function destructor_cb, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) +SdsApi::SdsApi(const envoy::api::v2::core::ConfigSource& sds_config, + const std::string& sds_config_name, + Config::SubscriptionFactory& subscription_factory, + ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats, + Init::Manager& init_manager, std::function destructor_cb) : init_target_(fmt::format("SdsApi {}", sds_config_name), [this] { initialize(); }), - local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), - cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), - secret_hash_(0), clean_up_(destructor_cb), validation_visitor_(validation_visitor), - api_(api) { - Config::Utility::checkLocalInfo("sds", local_info_); + stats_(stats), sds_config_(sds_config), sds_config_name_(sds_config_name), secret_hash_(0), + clean_up_(destructor_cb), validation_visitor_(validation_visitor), + subscription_factory_(subscription_factory) { // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager // can be chained together to behave as one init_manager. In that way, we let // two listeners which share same SdsApi to register at separate init managers, and @@ -78,10 +73,10 @@ void SdsApi::validateUpdateSize(int num_resources) { } void SdsApi::initialize() { - subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( - sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, - Grpc::Common::typeUrl(envoy::api::v2::auth::Secret().GetDescriptor()->full_name()), - validation_visitor_, api_, *this); + subscription_ = subscription_factory_.subscriptionFromConfigSource( + sds_config_, + Grpc::Common::typeUrl(envoy::api::v2::auth::Secret().GetDescriptor()->full_name()), stats_, + *this); subscription_->start({sds_config_name_}); } diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 7bff87d90e22..77336cecbd59 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -6,6 +6,7 @@ #include "envoy/api/v2/auth/cert.pb.h" #include "envoy/api/v2/core/config_source.pb.h" #include "envoy/config/subscription.h" +#include "envoy/config/subscription_factory.h" #include "envoy/event/dispatcher.h" #include "envoy/init/manager.h" #include "envoy/local_info/local_info.h" @@ -18,6 +19,7 @@ #include "common/common/callback_impl.h" #include "common/common/cleanup.h" +#include "common/config/utility.h" #include "common/init/target_impl.h" #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" @@ -30,12 +32,16 @@ namespace Secret { */ class SdsApi : public Config::SubscriptionCallbacks { public: - SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Stats::Store& stats, - Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, - std::function destructor_cb, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); + SdsApi(const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, + Config::SubscriptionFactory& subscription_factory, + ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats, + Init::Manager& init_manager, std::function destructor_cb); + +protected: + // Creates new secrets. + virtual void setSecret(const envoy::api::v2::auth::Secret&) PURE; + virtual void validateConfig(const envoy::api::v2::auth::Secret&) PURE; + Common::CallbackManager<> update_callback_manager_; // Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, @@ -48,21 +54,11 @@ class SdsApi : public Config::SubscriptionCallbacks { .name(); } -protected: - // Creates new secrets. - virtual void setSecret(const envoy::api::v2::auth::Secret&) PURE; - virtual void validateConfig(const envoy::api::v2::auth::Secret&) PURE; - Common::CallbackManager<> update_callback_manager_; - private: void validateUpdateSize(int num_resources); void initialize(); Init::TargetImpl init_target_; - const LocalInfo::LocalInfo& local_info_; - Event::Dispatcher& dispatcher_; - Runtime::RandomGenerator& random_; Stats::Store& stats_; - Upstream::ClusterManager& cluster_manager_; const envoy::api::v2::core::ConfigSource sds_config_; std::unique_ptr subscription_; @@ -71,7 +67,7 @@ class SdsApi : public Config::SubscriptionCallbacks { uint64_t secret_hash_; Cleanup clean_up_; ProtobufMessage::ValidationVisitor& validation_visitor_; - Api::Api& api_; + Config::SubscriptionFactory& subscription_factory_; }; class TlsCertificateSdsApi; @@ -89,22 +85,22 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context, const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, std::function destructor_cb) { + // We need to do this early as we invoke the subscription factory during initialization, which + // is too late to throw. + Config::Utility::checkLocalInfo("TlsCertificateSdsApi", secret_provider_context.localInfo()); return std::make_shared( - secret_provider_context.localInfo(), secret_provider_context.dispatcher(), - secret_provider_context.random(), secret_provider_context.stats(), - secret_provider_context.clusterManager(), *secret_provider_context.initManager(), - sds_config, sds_config_name, destructor_cb, - secret_provider_context.messageValidationVisitor(), secret_provider_context.api()); + sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(), + secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(), + *secret_provider_context.initManager(), destructor_cb); } - TlsCertificateSdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Stats::Store& stats, - Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, - const std::string& sds_config_name, std::function destructor_cb, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) - : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config, - sds_config_name, destructor_cb, validation_visitor, api) {} + TlsCertificateSdsApi(const envoy::api::v2::core::ConfigSource& sds_config, + const std::string& sds_config_name, + Config::SubscriptionFactory& subscription_factory, + ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats, + Init::Manager& init_manager, std::function destructor_cb) + : SdsApi(sds_config, sds_config_name, subscription_factory, validation_visitor, stats, + init_manager, destructor_cb) {} // SecretProvider const envoy::api::v2::auth::TlsCertificate* secret() const override { @@ -136,22 +132,23 @@ class CertificateValidationContextSdsApi : public SdsApi, create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context, const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, std::function destructor_cb) { + // We need to do this early as we invoke the subscription factory during initialization, which + // is too late to throw. + Config::Utility::checkLocalInfo("CertificateValidationContextSdsApi", + secret_provider_context.localInfo()); return std::make_shared( - secret_provider_context.localInfo(), secret_provider_context.dispatcher(), - secret_provider_context.random(), secret_provider_context.stats(), - secret_provider_context.clusterManager(), *secret_provider_context.initManager(), - sds_config, sds_config_name, destructor_cb, - secret_provider_context.messageValidationVisitor(), secret_provider_context.api()); + sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(), + secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(), + *secret_provider_context.initManager(), destructor_cb); } - CertificateValidationContextSdsApi( - const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Stats::Store& stats, - Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, - std::function destructor_cb, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api) - : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config, - sds_config_name, destructor_cb, validation_visitor, api) {} + CertificateValidationContextSdsApi(const envoy::api::v2::core::ConfigSource& sds_config, + const std::string& sds_config_name, + Config::SubscriptionFactory& subscription_factory, + ProtobufMessage::ValidationVisitor& validation_visitor, + Stats::Store& stats, Init::Manager& init_manager, + std::function destructor_cb) + : SdsApi(sds_config, sds_config_name, subscription_factory, validation_visitor, stats, + init_manager, destructor_cb) {} // SecretProvider const envoy::api::v2::auth::CertificateValidationContext* secret() const override { diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 66d7dfce6b4b..f1a69acf8942 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -14,13 +14,13 @@ envoy_cc_library( srcs = ["cds_api_impl.cc"], hdrs = ["cds_api_impl.h"], deps = [ + "//include/envoy/config:subscription_factory_interface", "//include/envoy/config:subscription_interface", "//include/envoy/event:dispatcher_interface", "//include/envoy/local_info:local_info_interface", "//source/common/common:cleanup_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:resources_lib", - "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/protobuf:utility_lib", "@envoy_api//envoy/api/v2:cds_cc", @@ -51,6 +51,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/config:cds_json_lib", "//source/common/config:grpc_mux_lib", + "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/grpc:async_client_manager_lib", "//source/common/http:async_client_lib", @@ -312,6 +313,7 @@ envoy_cc_library( ":cluster_factory_lib", ":upstream_includes", "//include/envoy/config:grpc_mux_interface", + "//include/envoy/config:subscription_factory_interface", "//include/envoy/config:subscription_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/secret:secret_manager_interface", @@ -495,6 +497,7 @@ envoy_cc_library( "//source/common/common:enum_to_int", "//source/common/common:minimal_logger_lib", "//source/common/config:metadata_lib", + "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/config:well_known_names", "//source/common/stats:isolated_store_lib", diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index a2840fdefe59..ff375bc5ff50 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -9,7 +9,6 @@ #include "common/common/cleanup.h" #include "common/common/utility.h" #include "common/config/resources.h" -#include "common/config/subscription_factory.h" #include "common/config/utility.h" #include "common/protobuf/utility.h" @@ -17,27 +16,18 @@ namespace Envoy { namespace Upstream { CdsApiPtr CdsApiImpl::create(const envoy::api::v2::core::ConfigSource& cds_config, - ClusterManager& cm, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, - const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, - ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api) { - return CdsApiPtr{new CdsApiImpl(cds_config, cm, dispatcher, random, local_info, scope, - validation_visitor, api)}; + ClusterManager& cm, Stats::Scope& scope, + ProtobufMessage::ValidationVisitor& validation_visitor) { + return CdsApiPtr{new CdsApiImpl(cds_config, cm, scope, validation_visitor)}; } CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm, - Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) + Stats::Scope& scope, ProtobufMessage::ValidationVisitor& validation_visitor) : cm_(cm), scope_(scope.createScope("cluster_manager.cds.")), validation_visitor_(validation_visitor) { - Config::Utility::checkLocalInfo("cds", local_info); - - subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource( - cds_config, local_info, dispatcher, cm, random, *scope_, - Grpc::Common::typeUrl(envoy::api::v2::Cluster().GetDescriptor()->full_name()), - validation_visitor, api, *this); + subscription_ = cm_.subscriptionFactory().subscriptionFromConfigSource( + cds_config, Grpc::Common::typeUrl(envoy::api::v2::Cluster().GetDescriptor()->full_name()), + *scope_, *this); } void CdsApiImpl::onConfigUpdate(const Protobuf::RepeatedPtrField& resources, @@ -80,7 +70,8 @@ void CdsApiImpl::onConfigUpdate( validation_visitor_); MessageUtil::validate(cluster); if (!cluster_names.insert(cluster.name()).second) { - // NOTE: at this point, the first of these duplicates has already been successfully applied. + // NOTE: at this point, the first of these duplicates has already been successfully + // applied. throw EnvoyException(fmt::format("duplicate cluster {} found", cluster.name())); } if (cm_.addOrUpdateCluster( @@ -88,21 +79,22 @@ void CdsApiImpl::onConfigUpdate( [this](const std::string&, ClusterManager::ClusterWarmingState state) { // Following if/else block implements a control flow mechanism that can be used // by an ADS implementation to properly sequence CDS and RDS update. It is not - // enforcing on ADS. ADS can use it to detect when a previously sent cluster becomes - // warm before sending routes that depend on it. This can improve incidence of HTTP - // 503 responses from Envoy when a route is used before it's supporting cluster is - // ready. + // enforcing on ADS. ADS can use it to detect when a previously sent cluster + // becomes warm before sending routes that depend on it. This can improve + // incidence of HTTP 503 responses from Envoy when a route is used before it's + // supporting cluster is ready. // - // We achieve that by leaving CDS in the paused state as long as there is at least - // one cluster in the warming state. This prevents CDS ACK from being sent to ADS. - // Once cluster is warmed up, CDS is resumed, and ACK is sent to ADS, providing a - // signal to ADS to proceed with RDS updates. + // We achieve that by leaving CDS in the paused state as long as there is at + // least one cluster in the warming state. This prevents CDS ACK from being sent + // to ADS. Once cluster is warmed up, CDS is resumed, and ACK is sent to ADS, + // providing a signal to ADS to proceed with RDS updates. // - // Major concern with this approach is CDS being left in the paused state forever. - // As long as ClusterManager::removeCluster() is not called on a warming cluster - // this is not an issue. CdsApiImpl takes care of doing this properly, and there - // is no other component removing clusters from the ClusterManagerImpl. If this - // ever changes, we would need to correct the following logic. + // Major concern with this approach is CDS being left in the paused state + // forever. As long as ClusterManager::removeCluster() is not called on a + // warming cluster this is not an issue. CdsApiImpl takes care of doing this + // properly, and there is no other component removing clusters from the + // ClusterManagerImpl. If this ever changes, we would need to correct the + // following logic. if (state == ClusterManager::ClusterWarmingState::Starting && cm_.warmingClusterCount() == 1) { cm_.adsMux().pause(Config::TypeUrl::get().Cluster); diff --git a/source/common/upstream/cds_api_impl.h b/source/common/upstream/cds_api_impl.h index 2dbed586e459..50de489b934f 100644 --- a/source/common/upstream/cds_api_impl.h +++ b/source/common/upstream/cds_api_impl.h @@ -5,6 +5,7 @@ #include "envoy/api/api.h" #include "envoy/api/v2/cds.pb.h" #include "envoy/config/subscription.h" +#include "envoy/config/subscription_factory.h" #include "envoy/event/dispatcher.h" #include "envoy/local_info/local_info.h" #include "envoy/stats/scope.h" @@ -23,9 +24,8 @@ class CdsApiImpl : public CdsApi, Logger::Loggable { public: static CdsApiPtr create(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm, - Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); + Stats::Scope& scope, + ProtobufMessage::ValidationVisitor& validation_visitor); // Upstream::CdsApi void initialize() override { subscription_->start({}); } @@ -34,6 +34,7 @@ class CdsApiImpl : public CdsApi, } const std::string versionInfo() const override { return system_version_info_; } +private: // Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const std::string& version_info) override; @@ -45,11 +46,8 @@ class CdsApiImpl : public CdsApi, return MessageUtil::anyConvert(resource, validation_visitor_).name(); } -private: CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm, - Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); + Stats::Scope& scope, ProtobufMessage::ValidationVisitor& validation_visitor); void runInitializeCallbackIfAny(); ClusterManager& cm_; diff --git a/source/common/upstream/cluster_factory_impl.cc b/source/common/upstream/cluster_factory_impl.cc index 19ccb9fad71b..43c6d5581977 100644 --- a/source/common/upstream/cluster_factory_impl.cc +++ b/source/common/upstream/cluster_factory_impl.cc @@ -29,7 +29,6 @@ std::pair ClusterFactoryImplBase:: Server::Admin& admin, Singleton::Manager& singleton_manager, Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) { - std::string cluster_type; if (!cluster.has_cluster_type()) { @@ -94,7 +93,6 @@ ClusterFactoryImplBase::selectDnsResolver(const envoy::api::v2::Cluster& cluster std::pair ClusterFactoryImplBase::create(const envoy::api::v2::Cluster& cluster, ClusterFactoryContext& context) { - auto stats_scope = generateStatsScope(cluster, context.stats()); Server::Configuration::TransportSocketFactoryContextImpl factory_context( context.admin(), context.sslContextManager(), *stats_scope, context.clusterManager(), diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index d73bb6e932f8..c80b95d28f2d 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -183,7 +183,8 @@ ClusterManagerImpl::ClusterManagerImpl( Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, - Server::Admin& admin, Api::Api& api, Http::Context& http_context) + Server::Admin& admin, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, + Http::Context& http_context) : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()), random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()), local_info_(local_info), cm_stats_(generateStats(stats)), @@ -191,7 +192,8 @@ ClusterManagerImpl::ClusterManagerImpl( config_tracker_entry_( admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })), time_source_(main_thread_dispatcher.timeSource()), dispatcher_(main_thread_dispatcher), - http_context_(http_context) { + http_context_(http_context), subscription_factory_(local_info, main_thread_dispatcher, *this, + random, validation_visitor, api) { async_client_manager_ = std::make_unique(*this, tls, time_source_, api); const auto& cm_config = bootstrap.cluster_manager(); @@ -653,7 +655,7 @@ void ClusterManagerImpl::updateGauges() { cm_stats_.warming_clusters_.set(warming_clusters_.size()); } -ThreadLocalCluster* ClusterManagerImpl::get(const std::string& cluster) { +ThreadLocalCluster* ClusterManagerImpl::get(absl::string_view cluster) { ThreadLocalClusterManagerImpl& cluster_manager = tls_->getTyped(); auto entry = cluster_manager.thread_local_clusters_.find(cluster); @@ -1216,9 +1218,9 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPool( ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { - return ClusterManagerPtr{ - new ClusterManagerImpl(bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, - log_manager_, main_thread_dispatcher_, admin_, api_, http_context_)}; + return ClusterManagerPtr{new ClusterManagerImpl( + bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_, + main_thread_dispatcher_, admin_, validation_visitor_, api_, http_context_)}; } Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( @@ -1254,8 +1256,7 @@ std::pair ProdClusterManagerFactor CdsApiPtr ProdClusterManagerFactory::createCds(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm) { // TODO(htuch): Differentiate static vs. dynamic validation visitors. - return CdsApiImpl::create(cds_config, cm, main_thread_dispatcher_, random_, local_info_, stats_, - validation_visitor_, api_); + return CdsApiImpl::create(cds_config, cm, stats_, validation_visitor_); } } // namespace Upstream diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 9898e09cde17..b4545acbe59c 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -22,6 +22,7 @@ #include "common/common/cleanup.h" #include "common/config/grpc_mux_impl.h" +#include "common/config/subscription_factory_impl.h" #include "common/http/async_client_impl.h" #include "common/upstream/load_stats_reporter.h" #include "common/upstream/priority_conn_pool_map.h" @@ -171,7 +172,8 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable thread_local_clusters_; + absl::flat_hash_map thread_local_clusters_; // These maps are owned by the ThreadLocalClusterManagerImpl instead of the ClusterEntry // to prevent lifetime/ownership issues when a cluster is dynamically removed. @@ -464,6 +468,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable void { onAssignmentTimeout(); }); const auto& eds_config = cluster.eds_cluster_config().eds_config(); - subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource( - eds_config, local_info_, dispatcher, cm, random, info_->statsScope(), - Grpc::Common::typeUrl(envoy::api::v2::ClusterLoadAssignment().GetDescriptor()->full_name()), - factory_context.messageValidationVisitor(), factory_context.api(), *this); + subscription_ = + factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( + eds_config, + Grpc::Common::typeUrl( + envoy::api::v2::ClusterLoadAssignment().GetDescriptor()->full_name()), + info_->statsScope(), *this); } void EdsClusterImpl::startPreInit() { subscription_->start({cluster_name_}); } diff --git a/source/common/upstream/eds.h b/source/common/upstream/eds.h index 7a6104a467fa..9953555db0e1 100644 --- a/source/common/upstream/eds.h +++ b/source/common/upstream/eds.h @@ -3,6 +3,7 @@ #include "envoy/api/v2/core/base.pb.h" #include "envoy/api/v2/eds.pb.h" #include "envoy/config/subscription.h" +#include "envoy/config/subscription_factory.h" #include "envoy/local_info/local_info.h" #include "envoy/secret/secret_manager.h" #include "envoy/stats/scope.h" @@ -20,7 +21,6 @@ namespace Upstream { * Cluster implementation that reads host information from the Endpoint Discovery Service. */ class EdsClusterImpl : public BaseDynamicClusterImpl, Config::SubscriptionCallbacks { - public: EdsClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Server::Configuration::TransportSocketFactoryContext& factory_context, @@ -29,6 +29,7 @@ class EdsClusterImpl : public BaseDynamicClusterImpl, Config::SubscriptionCallba // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Secondary; } +private: // Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const std::string& version_info) override; @@ -41,7 +42,6 @@ class EdsClusterImpl : public BaseDynamicClusterImpl, Config::SubscriptionCallba .cluster_name(); } -private: using LocalityWeightsMap = std::unordered_map; bool updateHostsPerLocality(const uint32_t priority, const uint32_t overprovisioning_factor, diff --git a/source/server/BUILD b/source/server/BUILD index 66be09dc6881..5be009fb26fd 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -233,12 +233,12 @@ envoy_cc_library( srcs = ["lds_api.cc"], hdrs = ["lds_api.h"], deps = [ + "//include/envoy/config:subscription_factory_interface", "//include/envoy/config:subscription_interface", "//include/envoy/init:manager_interface", "//include/envoy/server:listener_manager_interface", "//source/common/common:cleanup_lib", "//source/common/config:resources_lib", - "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", diff --git a/source/server/config_validation/cluster_manager.cc b/source/server/config_validation/cluster_manager.cc index 56817278b2b7..8aee20745977 100644 --- a/source/server/config_validation/cluster_manager.cc +++ b/source/server/config_validation/cluster_manager.cc @@ -9,7 +9,7 @@ ClusterManagerPtr ValidationClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { return std::make_unique( bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_, - main_thread_dispatcher_, admin_, api_, http_context_, time_system_); + main_thread_dispatcher_, admin_, validation_visitor_, api_, http_context_, time_system_); } CdsApiPtr @@ -26,10 +26,10 @@ ValidationClusterManager::ValidationClusterManager( Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, - Server::Admin& admin, Api::Api& api, Http::Context& http_context, - Event::TimeSystem& time_system) + Server::Admin& admin, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, + Http::Context& http_context, Event::TimeSystem& time_system) : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, log_manager, - main_thread_dispatcher, admin, api, http_context), + main_thread_dispatcher, admin, validation_visitor, api, http_context), async_client_(api, time_system) {} Http::ConnectionPool::Instance* diff --git a/source/server/config_validation/cluster_manager.h b/source/server/config_validation/cluster_manager.h index c20399523654..f4b4ce40bd9b 100644 --- a/source/server/config_validation/cluster_manager.h +++ b/source/server/config_validation/cluster_manager.h @@ -55,8 +55,9 @@ class ValidationClusterManager : public ClusterManagerImpl { ThreadLocal::Instance& tls, Runtime::Loader& runtime, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& dispatcher, - Server::Admin& admin, Api::Api& api, Http::Context& http_context, - Event::TimeSystem& time_system); + Server::Admin& admin, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, + Http::Context& http_context, Event::TimeSystem& time_system); Http::ConnectionPool::Instance* httpConnPoolForCluster(const std::string&, ResourcePriority, Http::Protocol, diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index b5a0365b068c..9e7784e5bcbe 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -111,9 +111,8 @@ class ValidationInstance : Logger::Loggable, // Server::ListenerComponentFactory LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { - return std::make_unique(lds_config, clusterManager(), dispatcher(), random(), - initManager(), localInfo(), stats(), listenerManager(), - messageValidationVisitor(), api()); + return std::make_unique(lds_config, clusterManager(), initManager(), stats(), + listenerManager(), messageValidationVisitor()); } std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index df2bc694cade..6f9c5c8c0ef2 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -8,7 +8,6 @@ #include "common/common/cleanup.h" #include "common/config/resources.h" -#include "common/config/subscription_factory.h" #include "common/config/utility.h" #include "common/protobuf/utility.h" @@ -16,19 +15,15 @@ namespace Envoy { namespace Server { LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, - Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Init::Manager& init_manager, - const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, - ListenerManager& lm, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api) + Upstream::ClusterManager& cm, Init::Manager& init_manager, + Stats::Scope& scope, ListenerManager& lm, + ProtobufMessage::ValidationVisitor& validation_visitor) : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm), init_target_("LDS", [this]() { subscription_->start({}); }), validation_visitor_(validation_visitor) { - subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( - lds_config, local_info, dispatcher, cm, random, *scope_, - Grpc::Common::typeUrl(envoy::api::v2::Listener().GetDescriptor()->full_name()), - validation_visitor_, api, *this); - Config::Utility::checkLocalInfo("lds", local_info); + subscription_ = cm.subscriptionFactory().subscriptionFromConfigSource( + lds_config, Grpc::Common::typeUrl(envoy::api::v2::Listener().GetDescriptor()->full_name()), + *scope_, *this); init_manager.add(init_target_); } diff --git a/source/server/lds_api.h b/source/server/lds_api.h index 3bbc0e3e7076..394b452cf797 100644 --- a/source/server/lds_api.h +++ b/source/server/lds_api.h @@ -2,9 +2,9 @@ #include -#include "envoy/api/api.h" #include "envoy/api/v2/lds.pb.h" #include "envoy/config/subscription.h" +#include "envoy/config/subscription_factory.h" #include "envoy/init/manager.h" #include "envoy/server/listener_manager.h" #include "envoy/stats/scope.h" @@ -23,14 +23,13 @@ class LdsApiImpl : public LdsApi, Logger::Loggable { public: LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, Upstream::ClusterManager& cm, - Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - Init::Manager& init_manager, const LocalInfo::LocalInfo& local_info, - Stats::Scope& scope, ListenerManager& lm, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); + Init::Manager& init_manager, Stats::Scope& scope, ListenerManager& lm, + ProtobufMessage::ValidationVisitor& validation_visitor); // Server::LdsApi std::string versionInfo() const override { return system_version_info_; } +private: // Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const std::string& version_info) override; @@ -42,7 +41,6 @@ class LdsApiImpl : public LdsApi, return MessageUtil::anyConvert(resource, validation_visitor_).name(); } -private: std::unique_ptr subscription_; std::string system_version_info_; ListenerManager& listener_manager_; diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 64466ff84ec4..6a4703e21375 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -53,10 +53,9 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, // Server::ListenerComponentFactory LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { - return std::make_unique( - lds_config, server_.clusterManager(), server_.dispatcher(), server_.random(), - server_.initManager(), server_.localInfo(), server_.stats(), server_.listenerManager(), - server_.messageValidationVisitor(), server_.api()); + return std::make_unique(lds_config, server_.clusterManager(), server_.initManager(), + server_.stats(), server_.listenerManager(), + server_.messageValidationVisitor()); } std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 0b3b5a9367b6..7e254c5ee8c7 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -179,8 +179,8 @@ envoy_cc_test_library( ) envoy_cc_test( - name = "subscription_factory_test", - srcs = ["subscription_factory_test.cc"], + name = "subscription_factory_impl_test", + srcs = ["subscription_factory_impl_test.cc"], deps = [ "//source/common/config:subscription_factory_lib", "//test/mocks/config:config_mocks", diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_impl_test.cc similarity index 98% rename from test/common/config/subscription_factory_test.cc rename to test/common/config/subscription_factory_impl_test.cc index b0ae6366b7ae..9c10b7dad282 100644 --- a/test/common/config/subscription_factory_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -4,7 +4,7 @@ #include "envoy/common/exception.h" #include "envoy/stats/scope.h" -#include "common/config/subscription_factory.h" +#include "common/config/subscription_factory_impl.h" #include "test/mocks/config/mocks.h" #include "test/mocks/event/mocks.h" @@ -35,9 +35,10 @@ class SubscriptionFactoryTest : public testing::Test { std::unique_ptr subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config) { - return SubscriptionFactory::subscriptionFromConfigSource( - config, local_info_, dispatcher_, cm_, random_, stats_store_, - Config::TypeUrl::get().ClusterLoadAssignment, validation_visitor_, *api_, callbacks_); + return SubscriptionFactoryImpl(local_info_, dispatcher_, cm_, random_, validation_visitor_, + *api_) + .subscriptionFromConfigSource(config, Config::TypeUrl::get().ClusterLoadAssignment, + stats_store_, callbacks_); } Upstream::MockClusterManager cm_; diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index 089a766e5fa8..12351a8d8b8f 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -290,7 +290,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest { std::move(shadow_writer_ptr_), http_context_); EXPECT_CALL(cm_, httpAsyncClientForCluster(fake_cluster_name_)) .WillRepeatedly(ReturnRef(*http_async_client_)); - EXPECT_CALL(cm_, get(fake_cluster_name_)).WillRepeatedly(Return(&thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq(fake_cluster_name_))).WillRepeatedly(Return(&thread_local_cluster_)); envoy::api::v2::core::GrpcService config; config.mutable_envoy_grpc()->set_cluster_name(fake_cluster_name_); fillServiceWideInitialMetadata(config); diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 9e5a061f7370..26b96a6431a2 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -65,7 +65,6 @@ envoy_cc_test( deps = [ "//source/common/config:filter_json_lib", "//source/common/config:utility_lib", - "//source/common/http:message_lib", "//source/common/json:json_loader_lib", "//source/common/router:rds_lib", "//source/server/http:admin_lib", @@ -101,14 +100,12 @@ envoy_cc_test( name = "vhds_test", srcs = ["vhds_test.cc"], deps = [ - "//source/common/config:filter_json_lib", "//source/common/config:utility_lib", - "//source/common/http:message_lib", - "//source/common/json:json_loader_lib", "//source/common/protobuf", "//source/common/router:rds_lib", "//source/common/router:vhds_lib", "//source/server/http:admin_lib", + "//test/mocks/config:config_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/server:server_mocks", "//test/mocks/thread_local:thread_local_mocks", diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 0bf195945fbf..eb9adf152501 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1979,9 +1979,9 @@ TEST_F(RouteMatcherTest, ShadowClusterNotFound) { cluster: www2 )EOF"; - EXPECT_CALL(factory_context_.cluster_manager_, get("www2")) + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("www2"))) .WillRepeatedly(Return(&factory_context_.cluster_manager_.thread_local_cluster_)); - EXPECT_CALL(factory_context_.cluster_manager_, get("some_cluster")) + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("some_cluster"))) .WillRepeatedly(Return(nullptr)); EXPECT_THROW(TestConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true), @@ -2001,7 +2001,7 @@ TEST_F(RouteMatcherTest, ClusterNotFound) { cluster: www2 )EOF"; - EXPECT_CALL(factory_context_.cluster_manager_, get("www2")).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(nullptr)); EXPECT_THROW(TestConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true), EnvoyException); @@ -2020,7 +2020,7 @@ TEST_F(RouteMatcherTest, ClusterNotFoundNotChecking) { cluster: www2 )EOF"; - EXPECT_CALL(factory_context_.cluster_manager_, get("www2")).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(nullptr)); TestConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, false); } @@ -2039,7 +2039,7 @@ validate_clusters: false cluster: www )EOF"; - EXPECT_CALL(factory_context_.cluster_manager_, get("www2")).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(nullptr)); TestConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true); } @@ -3794,11 +3794,11 @@ TEST_F(RouteMatcherTest, TestWeightedClusterInvalidClusterName) { weight: 34 )EOF"; - EXPECT_CALL(factory_context_.cluster_manager_, get("cluster1")) + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("cluster1"))) .WillRepeatedly(Return(&factory_context_.cluster_manager_.thread_local_cluster_)); - EXPECT_CALL(factory_context_.cluster_manager_, get("cluster2")) + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("cluster2"))) .WillRepeatedly(Return(&factory_context_.cluster_manager_.thread_local_cluster_)); - EXPECT_CALL(factory_context_.cluster_manager_, get("cluster3-invalid")) + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("cluster3-invalid"))) .WillRepeatedly(Return(nullptr)); EXPECT_THROW(TestConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true), @@ -5711,4 +5711,4 @@ name: foo } // namespace } // namespace Router -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 5975de0eeeaf..daa93436b370 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -8,7 +8,6 @@ #include "common/config/filter_json.h" #include "common/config/utility.h" -#include "common/http/message_impl.h" #include "common/json/json_loader.h" #include "common/router/rds_impl.h" @@ -27,6 +26,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::Return; @@ -49,10 +49,7 @@ parseHttpConnectionManagerFromJson(const std::string& json_string) { class RdsTestBase : public testing::Test { public: - RdsTestBase() - : request_(&factory_context_.cluster_manager_.async_client_), - rds_version_(factory_context_.scope_.gauge("foo.rds.foo_route_config.version", - Stats::Gauge::ImportMode::NeverImport)) { + RdsTestBase() { ON_CALL(factory_context_.init_manager_, add(_)) .WillByDefault(Invoke([this](const Init::Target& target) { init_target_handle_ = target.createHandle("test"); @@ -62,33 +59,13 @@ class RdsTestBase : public testing::Test { [this](const Init::Watcher& watcher) { init_target_handle_->initialize(watcher); })); } - void expectRequest() { - EXPECT_CALL(factory_context_.cluster_manager_, httpAsyncClientForCluster("foo_cluster")); - EXPECT_CALL(factory_context_.cluster_manager_.async_client_, send_(_, _, _)) - .WillOnce( - Invoke([&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - request->headers().removeContentLength(); - EXPECT_EQ((Http::TestHeaderMapImpl{{":method", "POST"}, - {":path", "/v2/discovery:routes"}, - {":authority", "foo_cluster"}, - {"content-type", "application/json"}}), - request->headers()); - callbacks_ = &callbacks; - return &request_; - })); - } - Event::SimulatedTimeSystem& timeSystem() { return time_system_; } Event::SimulatedTimeSystem time_system_; NiceMock factory_context_; Init::ExpectableWatcherImpl init_watcher_; Init::TargetHandlePtr init_target_handle_; - Http::MockAsyncClientRequest request_; - Http::AsyncClient::Callbacks* callbacks_{}; - Event::MockTimer* interval_timer_{}; - Stats::Gauge& rds_version_; + Envoy::Config::SubscriptionCallbacks* rds_callbacks_{}; }; class RdsImplTest : public RdsTestBase { @@ -117,20 +94,12 @@ class RdsImplTest : public RdsTestBase { } )EOF"; - Upstream::ClusterManager::ClusterInfoMap cluster_map; - Upstream::MockClusterMockPrioritySet cluster; - cluster_map.emplace("foo_cluster", cluster); - EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_CALL(cluster, info()); - EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_CALL(cluster, info()); - EXPECT_CALL(*cluster.info_, type()); - interval_timer_ = new Event::MockTimer(&factory_context_.dispatcher_); EXPECT_CALL(factory_context_.init_manager_, add(_)); rds_ = RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), factory_context_, "foo.", *route_config_provider_manager_); - expectRequest(); + rds_callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); factory_context_.init_manager_.initialize(init_watcher_); } @@ -164,61 +133,11 @@ TEST_F(RdsImplTest, RdsAndStatic) { EnvoyException); } -TEST_F(RdsImplTest, LocalInfoNotDefined) { - const std::string config_json = R"EOF( - { - "rds": { - "cluster": "foo_cluster", - "route_config_name": "foo_route_config" - }, - "codec_type": "auto", - "stat_prefix": "foo", - "filters": [ - { "name": "http_dynamo_filter", "config": {} } - ] - } - )EOF"; - - factory_context_.local_info_.node_.set_cluster(""); - factory_context_.local_info_.node_.set_id(""); - EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), - factory_context_, "foo.", - *route_config_provider_manager_), - EnvoyException); -} - -TEST_F(RdsImplTest, UnknownCluster) { - const std::string config_json = R"EOF( - { - "rds": { - "cluster": "foo_cluster", - "route_config_name": "foo_route_config" - }, - "codec_type": "auto", - "stat_prefix": "foo", - "filters": [ - { "name": "http_dynamo_filter", "config": {} } - ] - } - )EOF"; - - Upstream::ClusterManager::ClusterInfoMap cluster_map; - EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_THROW_WITH_MESSAGE( - RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), - factory_context_, "foo.", *route_config_provider_manager_), - EnvoyException, - "envoy::api::v2::core::ConfigSource must have a statically defined non-EDS " - "cluster: 'foo_cluster' does not exist, was added via api, or is an " - "EDS cluster"); -} - TEST_F(RdsImplTest, DestroyDuringInitialize) { InSequence s; setup(); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(request_, cancel()); rds_.reset(); } @@ -231,7 +150,6 @@ TEST_F(RdsImplTest, Basic) { // Make sure the initial empty route table works. EXPECT_EQ(nullptr, route(Http::TestHeaderMapImpl{{":authority", "foo"}})); - EXPECT_EQ(0UL, rds_version_.value()); // Initial request. const std::string response1_json = R"EOF( @@ -246,34 +164,16 @@ TEST_F(RdsImplTest, Basic) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response1_json); + auto response1 = TestUtility::parseYaml(response1_json); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ(nullptr, route(Http::TestHeaderMapImpl{{":authority", "foo"}})); - EXPECT_EQ(13237225503670494420U, rds_version_.value()); - - expectRequest(); - interval_timer_->callback_(); // 2nd request with same response. Based on hash should not reload config. - message = std::make_unique( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}); - message->body() = std::make_unique(response1_json); - - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ(nullptr, route(Http::TestHeaderMapImpl{{":authority", "foo"}})); - EXPECT_EQ(13237225503670494420U, rds_version_.value()); - - expectRequest(); - interval_timer_->callback_(); - // Load the config and verified shared count. ConfigConstSharedPtr config = rds_->config(); EXPECT_EQ(2, config.use_count()); @@ -308,38 +208,27 @@ TEST_F(RdsImplTest, Basic) { ] } )EOF"; - - message = std::make_unique( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}); - message->body() = std::make_unique(response2_json); + auto response2 = TestUtility::parseYaml(response2_json); // Make sure we don't lookup/verify clusters. - EXPECT_CALL(factory_context_.cluster_manager_, get("bar")).Times(0); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + EXPECT_CALL(factory_context_.cluster_manager_, get(Eq("bar"))).Times(0); + rds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); EXPECT_EQ("foo", route(Http::TestHeaderMapImpl{{":authority", "foo"}, {":path", "/foo"}}) ->routeEntry() ->clusterName()); - EXPECT_EQ(6927017134761466251U, rds_version_.value()); - // Old config use count should be 1 now. EXPECT_EQ(1, config.use_count()); - EXPECT_EQ(2UL, factory_context_.scope_.counter("foo.rds.foo_route_config.config_reload").value()); - EXPECT_EQ(3UL, - factory_context_.scope_.counter("foo.rds.foo_route_config.update_attempt").value()); - EXPECT_EQ(3UL, - factory_context_.scope_.counter("foo.rds.foo_route_config.update_success").value()); - EXPECT_EQ(6927017134761466251U, rds_version_.value()); } -TEST_F(RdsImplTest, Failure) { +// Validate behavior when the config is delivered but it fails PGV validation. +TEST_F(RdsImplTest, FailureInvalidConfig) { InSequence s; setup(); - std::string response_json = R"EOF( + std::string response1_json = R"EOF( { "version_info": "1", "resources": [ @@ -351,56 +240,34 @@ TEST_F(RdsImplTest, Failure) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response_json); + auto response1 = TestUtility::parseYaml(response1_json); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + EXPECT_THROW_WITH_MESSAGE( + rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), + EnvoyException, + "Unexpected RDS configuration (expecting foo_route_config): INVALID_NAME_FOR_route_config"); +} - expectRequest(); - interval_timer_->callback_(); +// Validate behavior when the config fails delivery at the subscription level. +TEST_F(RdsImplTest, FailureSubscription) { + InSequence s; - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onFailure(Http::AsyncClient::FailureReason::Reset); + setup(); - EXPECT_EQ(2UL, - factory_context_.scope_.counter("foo.rds.foo_route_config.update_attempt").value()); - EXPECT_EQ(1UL, - factory_context_.scope_.counter("foo.rds.foo_route_config.update_failure").value()); - // Validate that the schema error increments update_rejected stat. - EXPECT_EQ(1UL, - factory_context_.scope_.counter("foo.rds.foo_route_config.update_rejected").value()); + EXPECT_CALL(init_watcher_, ready()); + rds_callbacks_->onConfigUpdateFailed({}); } class RouteConfigProviderManagerImplTest : public RdsTestBase { public: void setup() { - std::string config_json = R"EOF( - { - "api_type": "REST", - "cluster": "foo_cluster", - "route_config_name": "foo_route_config", - "refresh_delay_ms": 1000 - } - )EOF"; - - Json::ObjectSharedPtr config = Json::Factory::loadFromString(config_json); - Envoy::Config::Utility::translateRdsConfig(*config, rds_); - // Get a RouteConfigProvider. This one should create an entry in the RouteConfigProviderManager. - Upstream::ClusterManager::ClusterInfoMap cluster_map; - Upstream::MockClusterMockPrioritySet cluster; - cluster_map.emplace("foo_cluster", cluster); - EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_CALL(cluster, info()).Times(2); - EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_CALL(*cluster.info_, type()); - interval_timer_ = new Event::MockTimer(&factory_context_.dispatcher_); + rds_.set_route_config_name("foo_route_config"); + rds_.mutable_config_source()->set_path("foo_path"); provider_ = route_config_provider_manager_->createRdsRouteConfigProvider(rds_, factory_context_, "foo_prefix."); + rds_callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; } RouteConfigProviderManagerImplTest() { @@ -477,7 +344,7 @@ name: foo // Static + dynamic. setup(); - expectRequest(); + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); factory_context_.init_manager_.initialize(init_watcher_); const std::string response1_json = R"EOF( @@ -492,13 +359,10 @@ name: foo ] } )EOF"; + auto response1 = TestUtility::parseYaml(response1_json); - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response1_json); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); const auto& route_config_dump3 = MessageUtil::downcastAndValidate( @@ -548,10 +412,8 @@ name: foo_route_config route: { cluster: baz } )EOF")); - RdsRouteConfigSubscription& subscription = - dynamic_cast(*provider_).subscription(); - - subscription.onConfigUpdate(route_configs, "1"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(route_configs, + "1"); RouteConfigProviderPtr provider2 = route_config_provider_manager_->createRdsRouteConfigProvider( rds_, factory_context_, "foo_prefix"); @@ -564,34 +426,14 @@ name: foo_route_config &dynamic_cast(*provider2).subscription()); EXPECT_EQ(&provider_->configInfo().value().config_, &provider2->configInfo().value().config_); - std::string config_json2 = R"EOF( - { - "api_type": "REST", - "cluster": "bar_cluster", - "route_config_name": "foo_route_config", - "refresh_delay_ms": 1000 - } - )EOF"; - - Json::ObjectSharedPtr config2 = Json::Factory::loadFromString(config_json2); envoy::config::filter::network::http_connection_manager::v2::Rds rds2; - Envoy::Config::Utility::translateRdsConfig(*config2, rds2); - - Upstream::ClusterManager::ClusterInfoMap cluster_map; - Upstream::MockClusterMockPrioritySet cluster; - cluster_map.emplace("bar_cluster", cluster); - EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_CALL(cluster, info()).Times(2); - EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_CALL(*cluster.info_, type()); - new Event::MockTimer(&factory_context_.dispatcher_); + rds2.set_route_config_name("foo_route_config"); + rds2.mutable_config_source()->set_path("bar_path"); RouteConfigProviderPtr provider3 = route_config_provider_manager_->createRdsRouteConfigProvider( rds2, factory_context_, "foo_prefix"); EXPECT_NE(provider3, provider_); - dynamic_cast(*provider3) - .subscription() - .onConfigUpdate(route_configs, "provider3"); - + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(route_configs, + "provider3"); EXPECT_EQ(2UL, route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size()); @@ -616,36 +458,36 @@ name: foo_route_config // Negative test for protoc-gen-validate constraints. TEST_F(RouteConfigProviderManagerImplTest, ValidateFail) { setup(); - auto& provider_impl = dynamic_cast(*provider_.get()); Protobuf::RepeatedPtrField route_configs; envoy::api::v2::RouteConfiguration route_config; route_config.set_name("foo_route_config"); route_config.mutable_virtual_hosts()->Add(); route_configs.Add()->PackFrom(route_config); - EXPECT_THROW(provider_impl.subscription().onConfigUpdate(route_configs, ""), + EXPECT_THROW(factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + route_configs, ""), ProtoValidationException); } TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateEmpty) { setup(); + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); factory_context_.init_manager_.initialize(init_watcher_); - auto& provider_impl = dynamic_cast(*provider_.get()); EXPECT_CALL(init_watcher_, ready()); - provider_impl.subscription().onConfigUpdate({}, ""); - EXPECT_EQ( - 1UL, factory_context_.scope_.counter("foo_prefix.rds.foo_route_config.update_empty").value()); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate({}, ""); } TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateWrongSize) { setup(); + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); factory_context_.init_manager_.initialize(init_watcher_); - auto& provider_impl = dynamic_cast(*provider_.get()); Protobuf::RepeatedPtrField route_configs; route_configs.Add(); route_configs.Add(); EXPECT_CALL(init_watcher_, ready()); - EXPECT_THROW_WITH_MESSAGE(provider_impl.subscription().onConfigUpdate(route_configs, ""), - EnvoyException, "Unexpected RDS resource length: 2"); + EXPECT_THROW_WITH_MESSAGE( + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + route_configs, ""), + EnvoyException, "Unexpected RDS resource length: 2"); } } // namespace diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 15b282bd77b2..1476dac9dce0 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -47,29 +47,10 @@ class ScopedRoutesTestBase : public testing::Test { EXPECT_CALL(factory_context_.admin_.config_tracker_, add_("route_scopes", _)); config_provider_manager_ = std::make_unique(factory_context_.admin_); - - const std::string rds_config_yaml = R"EOF( -api_config_source: - api_type: REST - cluster_names: - - foo_rds_cluster - refresh_delay: { seconds: 1, nanos: 0 } -)EOF"; - TestUtility::loadFromYaml(rds_config_yaml, rds_config_source_); } ~ScopedRoutesTestBase() override { factory_context_.thread_local_.shutdownThread(); } - void setupMockClusterMap() { - InSequence s; - cluster_map_.emplace("foo_cluster", cluster_); - EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map_)); - EXPECT_CALL(cluster_, info()); - EXPECT_CALL(*cluster_.info_, addedViaApi()); - EXPECT_CALL(cluster_, info()); - EXPECT_CALL(*cluster_.info_, type()); - } - Event::SimulatedTimeSystem& timeSystem() { return time_system_; } NiceMock factory_context_; @@ -85,25 +66,11 @@ class ScopedRdsTest : public ScopedRoutesTestBase { void setup() { InSequence s; - setupMockClusterMap(); const std::string config_yaml = R"EOF( name: foo_scoped_routes scope_key_builder: fragments: - header_value_extractor: { name: X-Google-VIP } -rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: { seconds: 1, nanos: 0 } -scoped_rds: - scoped_rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: { seconds: 1, nanos: 0 } )EOF"; envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes scoped_routes_config; TestUtility::loadFromYaml(config_yaml, scoped_routes_config); @@ -112,21 +79,16 @@ name: foo_scoped_routes ScopedRoutesConfigProviderManagerOptArg(scoped_routes_config.name(), scoped_routes_config.rds_config_source(), scoped_routes_config.scope_key_builder())); - subscription_ = &dynamic_cast(*provider_).subscription(); + subscription_callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; } - ScopedRdsConfigSubscription& subscription() const { return *subscription_; } - - ScopedRdsConfigSubscription* subscription_; + Envoy::Config::SubscriptionCallbacks* subscription_callbacks_{}; Envoy::Config::ConfigProviderPtr provider_; }; TEST_F(ScopedRdsTest, ValidateFail) { setup(); - ScopedRdsConfigSubscription& subscription = - dynamic_cast(*provider_).subscription(); - // 'name' validation: value must be > 1 byte. const std::string config_yaml = R"EOF( name: @@ -137,7 +99,7 @@ route_configuration_name: foo_routes )EOF"; Protobuf::RepeatedPtrField resources; parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml); - EXPECT_THROW(subscription.onConfigUpdate(resources, "1"), ProtoValidationException); + EXPECT_THROW(subscription_callbacks_->onConfigUpdate(resources, "1"), ProtoValidationException); // 'route_configuration_name' validation: value must be > 1 byte. const std::string config_yaml2 = R"EOF( @@ -149,7 +111,7 @@ name: foo_scope )EOF"; Protobuf::RepeatedPtrField resources2; parseScopedRouteConfigurationFromYaml(*resources2.Add(), config_yaml2); - EXPECT_THROW(subscription.onConfigUpdate(resources2, "1"), ProtoValidationException); + EXPECT_THROW(subscription_callbacks_->onConfigUpdate(resources2, "1"), ProtoValidationException); // 'key' validation: must define at least 1 fragment. const std::string config_yaml3 = R"EOF( @@ -159,7 +121,7 @@ route_configuration_name: foo_routes )EOF"; Protobuf::RepeatedPtrField resources3; parseScopedRouteConfigurationFromYaml(*resources3.Add(), config_yaml3); - EXPECT_THROW(subscription.onConfigUpdate(resources3, "1"), ProtoValidationException); + EXPECT_THROW(subscription_callbacks_->onConfigUpdate(resources3, "1"), ProtoValidationException); } // Tests that multiple uniquely named resources are allowed in config updates. @@ -183,7 +145,7 @@ route_configuration_name: foo_routes - string_key: x-foo-key )EOF"; parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2); - EXPECT_NO_THROW(subscription().onConfigUpdate(resources, "1")); + EXPECT_NO_THROW(subscription_callbacks_->onConfigUpdate(resources, "1")); EXPECT_EQ( 1UL, factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); @@ -203,48 +165,10 @@ route_configuration_name: foo_routes Protobuf::RepeatedPtrField resources; parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml); parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml); - EXPECT_THROW_WITH_MESSAGE(subscription().onConfigUpdate(resources, "1"), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(subscription_callbacks_->onConfigUpdate(resources, "1"), EnvoyException, "duplicate scoped route configuration foo_scope found"); } -// Tests that defining an invalid cluster in the SRDS config results in an error. -TEST_F(ScopedRdsTest, UnknownCluster) { - const std::string config_yaml = R"EOF( -name: foo_scoped_routes -scope_key_builder: - fragments: - - header_value_extractor: { name: X-Google-VIP } -rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: { seconds: 1, nanos: 0 } -scoped_rds: - scoped_rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: { seconds: 1, nanos: 0 } -)EOF"; - envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes scoped_routes_config; - TestUtility::loadFromYaml(config_yaml, scoped_routes_config); - - Upstream::ClusterManager::ClusterInfoMap cluster_map; - EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_THROW_WITH_MESSAGE( - config_provider_manager_->createXdsConfigProvider( - scoped_routes_config.scoped_rds(), factory_context_, "foo.", - ScopedRoutesConfigProviderManagerOptArg(scoped_routes_config.name(), - scoped_routes_config.rds_config_source(), - scoped_routes_config.scope_key_builder())), - EnvoyException, - "envoy::api::v2::core::ConfigSource must have a statically defined non-EDS " - "cluster: 'foo_cluster' does not exist, was added via api, or is an " - "EDS cluster"); -} - // Tests a config update failure. TEST_F(ScopedRdsTest, ConfigUpdateFailure) { setup(); @@ -253,7 +177,7 @@ TEST_F(ScopedRdsTest, ConfigUpdateFailure) { timeSystem().setSystemTime(time); const EnvoyException ex(fmt::format("config failure")); // Verify the failure updates the lastUpdated() timestamp. - subscription().onConfigUpdateFailed(&ex); + subscription_callbacks_->onConfigUpdateFailed(&ex); EXPECT_EQ(std::chrono::time_point_cast(provider_->lastUpdated()) .time_since_epoch(), time); @@ -291,12 +215,6 @@ stat_prefix: foo scope_key_builder: fragments: - header_value_extractor: { name: X-Google-VIP } - rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_rds_cluster - refresh_delay: { seconds: 1, nanos: 0 } $1 )EOF"; const std::string inline_scoped_route_configs_yaml = R"EOF( @@ -340,15 +258,9 @@ stat_prefix: foo expected_config_dump); EXPECT_EQ(expected_config_dump.DebugString(), scoped_routes_config_dump2.DebugString()); - setupMockClusterMap(); const std::string scoped_rds_config_yaml = R"EOF( scoped_rds: scoped_rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: { seconds: 1, nanos: 0 } )EOF"; Envoy::Config::ConfigProviderPtr dynamic_provider = ScopedRoutesConfigProviderUtil::create( parseHttpConnectionManagerFromYaml(absl::Substitute( @@ -364,9 +276,8 @@ route_configuration_name: dynamic-foo-route-config )EOF")); timeSystem().setSystemTime(std::chrono::milliseconds(1234567891567)); - ScopedRdsConfigSubscription& subscription = - dynamic_cast(*dynamic_provider).subscription(); - subscription.onConfigUpdate(resources, "1"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(resources, + "1"); TestUtility::loadFromYaml(R"EOF( inline_scoped_route_configs: @@ -403,7 +314,8 @@ route_configuration_name: dynamic-foo-route-config EXPECT_EQ(expected_config_dump.DebugString(), scoped_routes_config_dump3.DebugString()); resources.Clear(); - subscription.onConfigUpdate(resources, "2"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(resources, + "2"); TestUtility::loadFromYaml(R"EOF( inline_scoped_route_configs: - name: foo-scoped-routes diff --git a/test/common/router/shadow_writer_impl_test.cc b/test/common/router/shadow_writer_impl_test.cc index 245c67be85b3..e329b91d2926 100644 --- a/test/common/router/shadow_writer_impl_test.cc +++ b/test/common/router/shadow_writer_impl_test.cc @@ -11,6 +11,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::Return; @@ -24,7 +25,7 @@ class ShadowWriterImplTest : public testing::Test { void expectShadowWriter(absl::string_view host, absl::string_view shadowed_host) { Http::MessagePtr message(new Http::RequestMessageImpl()); message->headers().insertHost().value(std::string(host)); - EXPECT_CALL(cm_, get("foo")); + EXPECT_CALL(cm_, get(Eq("foo"))); EXPECT_CALL(cm_, httpAsyncClientForCluster("foo")).WillOnce(ReturnRef(cm_.async_client_)); Http::MockAsyncClientRequest request(&cm_.async_client_); EXPECT_CALL( @@ -65,7 +66,7 @@ TEST_F(ShadowWriterImplTest, NoCluster) { InSequence s; Http::MessagePtr message(new Http::RequestMessageImpl()); - EXPECT_CALL(cm_, get("foo")).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, get(Eq("foo"))).WillOnce(Return(nullptr)); EXPECT_CALL(cm_, httpAsyncClientForCluster("foo")).Times(0); writer_.shadow("foo", std::move(message), std::chrono::milliseconds(5)); } diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index cb35c34cb3ee..01d3287421e0 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -6,17 +6,14 @@ #include "envoy/admin/v2alpha/config_dump.pb.validate.h" #include "envoy/stats/scope.h" -#include "common/config/filter_json.h" #include "common/config/utility.h" -#include "common/http/message_impl.h" -#include "common/json/json_loader.h" #include "common/protobuf/protobuf.h" #include "common/router/rds_impl.h" #include "server/http/admin.h" +#include "test/mocks/config/mocks.h" #include "test/mocks/init/mocks.h" -#include "test/mocks/local_info/mocks.h" #include "test/mocks/server/mocks.h" #include "test/mocks/thread_local/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -41,14 +38,6 @@ namespace { class VhdsTest : public testing::Test { public: void SetUp() override { - factory_function_ = { - [](const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, - Event::Dispatcher&, Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, - Stats::Scope&, absl::string_view, ProtobufMessage::ValidationVisitor&, Api::Api&, - Envoy::Config::SubscriptionCallbacks&) -> std::unique_ptr { - return std::unique_ptr(); - }}; - default_vhds_config_ = R"EOF( name: my_route vhds: @@ -101,11 +90,11 @@ name: my_route NiceMock factory_context_; Init::ExpectableWatcherImpl init_watcher_; Init::TargetHandlePtr init_target_handle_; - Envoy::Router::SubscriptionFactoryFunction factory_function_; const std::string context_ = "vhds_test"; std::unordered_set providers_; Protobuf::util::MessageDifferencer messageDifferencer_; std::string default_vhds_config_; + NiceMock subscription_factory_; }; // verify that api_type: DELTA_GRPC passes validation @@ -114,8 +103,7 @@ TEST_F(VhdsTest, VhdsInstantiationShouldSucceedWithDELTA_GRPC) { TestUtility::parseYaml(default_vhds_config_); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - EXPECT_NO_THROW(VhdsSubscription(config_update_info, factory_context_, context_, providers_, - factory_function_)); + EXPECT_NO_THROW(VhdsSubscription(config_update_info, factory_context_, context_, providers_)); } // verify that api_type: GRPC fails validation @@ -132,8 +120,7 @@ name: my_route )EOF"); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - EXPECT_THROW(VhdsSubscription(config_update_info, factory_context_, context_, providers_, - factory_function_), + EXPECT_THROW(VhdsSubscription(config_update_info, factory_context_, context_, providers_), EnvoyException); } @@ -143,14 +130,14 @@ TEST_F(VhdsTest, VhdsAddsVirtualHosts) { TestUtility::parseYaml(default_vhds_config_); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_, - factory_function_); + VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_); EXPECT_EQ(0UL, config_update_info->routeConfiguration().virtual_hosts_size()); auto vhost = buildVirtualHost("vhost1", "vhost.first"); const auto& added_resources = buildAddedResources({vhost}); const Protobuf::RepeatedPtrField removed_resources; - subscription.onConfigUpdate(added_resources, removed_resources, "1"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + added_resources, removed_resources, "1"); EXPECT_EQ(1UL, config_update_info->routeConfiguration().virtual_hosts_size()); EXPECT_TRUE( @@ -177,15 +164,15 @@ name: my_route )EOF"); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_, - factory_function_); + VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_); EXPECT_EQ(1UL, config_update_info->routeConfiguration().virtual_hosts_size()); EXPECT_EQ("vhost_rds1", config_update_info->routeConfiguration().virtual_hosts(0).name()); auto vhost = buildVirtualHost("vhost1", "vhost.first"); const auto& added_resources = buildAddedResources({vhost}); const Protobuf::RepeatedPtrField removed_resources; - subscription.onConfigUpdate(added_resources, removed_resources, "1"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + added_resources, removed_resources, "1"); EXPECT_EQ(2UL, config_update_info->routeConfiguration().virtual_hosts_size()); auto actual_vhost_0 = config_update_info->routeConfiguration().virtual_hosts(0); @@ -214,14 +201,14 @@ name: my_route cluster_name: xds_cluster )EOF"); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_, - factory_function_); + VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_); EXPECT_EQ(1UL, config_update_info->routeConfiguration().virtual_hosts_size()); EXPECT_EQ("vhost_rds1", config_update_info->routeConfiguration().virtual_hosts(0).name()); const Protobuf::RepeatedPtrField added_resources; const auto removed_resources = buildRemovedResources({"vhost_rds1"}); - subscription.onConfigUpdate(added_resources, removed_resources, "1"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + added_resources, removed_resources, "1"); EXPECT_EQ(0UL, config_update_info->routeConfiguration().virtual_hosts_size()); } @@ -246,15 +233,15 @@ name: my_route )EOF"); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_, - factory_function_); + VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_); EXPECT_EQ(1UL, config_update_info->routeConfiguration().virtual_hosts_size()); EXPECT_EQ("vhost_rds1", config_update_info->routeConfiguration().virtual_hosts(0).name()); auto vhost = buildVirtualHost("vhost_rds1", "vhost.rds.first.mk2"); const auto& added_resources = buildAddedResources({vhost}); const Protobuf::RepeatedPtrField removed_resources; - subscription.onConfigUpdate(added_resources, removed_resources, "1"); + factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + added_resources, removed_resources, "1"); EXPECT_EQ(1UL, config_update_info->routeConfiguration().virtual_hosts_size()); EXPECT_TRUE( @@ -267,8 +254,7 @@ TEST_F(VhdsTest, VhdsValidatesAddedVirtualHosts) { TestUtility::parseYaml(default_vhds_config_); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_, - factory_function_); + VhdsSubscription subscription(config_update_info, factory_context_, context_, providers_); auto vhost = TestUtility::parseYaml(R"EOF( name: invalid_vhost @@ -281,7 +267,8 @@ TEST_F(VhdsTest, VhdsValidatesAddedVirtualHosts) { const auto& added_resources = buildAddedResources({vhost}); const Protobuf::RepeatedPtrField removed_resources; - EXPECT_THROW(subscription.onConfigUpdate(added_resources, removed_resources, "1"), + EXPECT_THROW(factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate( + added_resources, removed_resources, "1"), ProtoValidationException); } diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index f0b53f5f43a7..2a354658e4d3 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -38,6 +38,7 @@ envoy_cc_test( "//source/common/ssl:tls_certificate_config_impl_lib", "//test/mocks/grpc:grpc_mocks", "//test/mocks/init:init_mocks", + "//test/mocks/protobuf:protobuf_mocks", "//test/mocks/secret:secret_mocks", "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index feee6201d634..f85edd205787 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -10,6 +10,7 @@ #include "test/mocks/grpc/mocks.h" #include "test/mocks/init/mocks.h" +#include "test/mocks/protobuf/mocks.h" #include "test/mocks/secret/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" @@ -28,9 +29,20 @@ namespace { class SdsApiTest : public testing::Test { protected: - SdsApiTest() : api_(Api::createApiForTest()) {} + SdsApiTest() : api_(Api::createApiForTest()) { + EXPECT_CALL(init_manager_, add(_)).WillOnce(Invoke([this](const Init::Target& target) { + init_target_handle_ = target.createHandle("test"); + })); + } + + void initialize() { init_target_handle_->initialize(init_watcher_); } Api::ApiPtr api_; + NiceMock validation_visitor_; + NiceMock subscription_factory_; + NiceMock init_manager_; + NiceMock init_watcher_; + Init::TargetHandlePtr init_target_handle_; }; // Validate that SdsApi object is created and initialized successfully. @@ -38,50 +50,21 @@ TEST_F(SdsApiTest, BasicTest) { ::testing::InSequence s; const envoy::service::discovery::v2::SdsDummy dummy; NiceMock server; - NiceMock init_manager; - NiceMock init_watcher; - Init::TargetHandlePtr init_target_handle; - EXPECT_CALL(init_manager, add(_)) - .WillOnce(Invoke([&init_target_handle](const Init::Target& target) { - init_target_handle = target.createHandle("test"); - })); envoy::api::v2::core::ConfigSource config_source; - config_source.mutable_api_config_source()->set_api_type( - envoy::api::v2::core::ApiConfigSource::GRPC); - auto grpc_service = config_source.mutable_api_config_source()->add_grpc_services(); - auto google_grpc = grpc_service->mutable_google_grpc(); - google_grpc->set_target_uri("fake_address"); - google_grpc->set_stat_prefix("test"); - TlsCertificateSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); - - Grpc::MockAsyncClient* grpc_client{new NiceMock()}; - NiceMock* factory{new NiceMock()}; - EXPECT_CALL(server.cluster_manager_.async_client_manager_, factoryForGrpcService(_, _, _)) - .WillOnce(Invoke([factory](const envoy::api::v2::core::GrpcService&, Stats::Scope&, bool) { - return Grpc::AsyncClientFactoryPtr{factory}; - })); - EXPECT_CALL(*factory, create()).WillOnce(Invoke([grpc_client] { - return Grpc::RawAsyncClientPtr{grpc_client}; - })); - EXPECT_CALL(init_watcher, ready()); - init_target_handle->initialize(init_watcher); + TlsCertificateSdsApi sds_api(config_source, "abc.com", subscription_factory_, validation_visitor_, + server.stats(), init_manager_, []() {}); + initialize(); } // Validate that TlsCertificateSdsApi updates secrets successfully if a good secret // is passed to onConfigUpdate(). TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - TlsCertificateSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); - + TlsCertificateSdsApi sds_api(config_source, "abc.com", subscription_factory_, validation_visitor_, + server.stats(), init_manager_, []() {}); + initialize(); NiceMock secret_callback; auto handle = sds_api.addUpdateCallback([&secret_callback]() { secret_callback.onAddOrUpdateSecret(); }); @@ -101,7 +84,7 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) { secret_resources.Add()->PackFrom(typed_secret); EXPECT_CALL(secret_callback, onAddOrUpdateSecret()); - sds_api.onConfigUpdate(secret_resources, ""); + subscription_factory_.callbacks_->onConfigUpdate(secret_resources, ""); Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), *api_); const std::string cert_pem = @@ -119,13 +102,11 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) { class PartialMockSds : public SdsApi { public: - PartialMockSds(NiceMock& server, Api::Api& api, - NiceMock& init_manager, - envoy::api::v2::core::ConfigSource& config_source) - : SdsApi( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), api) {} + PartialMockSds(NiceMock& server, NiceMock& init_manager, + envoy::api::v2::core::ConfigSource& config_source, + Config::SubscriptionFactory& subscription_factory) + : SdsApi(config_source, "abc.com", subscription_factory, validation_visitor_, server.stats(), + init_manager, []() {}) {} MOCK_METHOD2(onConfigUpdate, void(const Protobuf::RepeatedPtrField&, const std::string&)); @@ -136,6 +117,8 @@ class PartialMockSds : public SdsApi { } void setSecret(const envoy::api::v2::auth::Secret&) override {} void validateConfig(const envoy::api::v2::auth::Secret&) override {} + + NiceMock validation_visitor_; }; // Basic test of delta's passthrough call to the state-of-the-world variant, to @@ -153,11 +136,11 @@ TEST_F(SdsApiTest, Delta) { for_matching.Add()->PackFrom(secret); NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - PartialMockSds sds(server, *api_, init_manager, config_source); + PartialMockSds sds(server, init_manager_, config_source, subscription_factory_); + initialize(); EXPECT_CALL(sds, onConfigUpdate(RepeatedProtoEq(for_matching), "version1")); - sds.SdsApi::onConfigUpdate(resources, {}, "ignored"); + subscription_factory_.callbacks_->onConfigUpdate(resources, {}, "ignored"); // An attempt to remove a resource logs an error, but otherwise just carries on (ignoring the // removal attempt). @@ -165,18 +148,15 @@ TEST_F(SdsApiTest, Delta) { EXPECT_CALL(sds, onConfigUpdate(RepeatedProtoEq(for_matching), "version2")); Protobuf::RepeatedPtrField removals; *removals.Add() = "route_0"; - sds.SdsApi::onConfigUpdate(resources, removals, "ignored"); + subscription_factory_.callbacks_->onConfigUpdate(resources, removals, "ignored"); } // Tests SDS's use of the delta variant of onConfigUpdate(). TEST_F(SdsApiTest, DeltaUpdateSuccess) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - TlsCertificateSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); + TlsCertificateSdsApi sds_api(config_source, "abc.com", subscription_factory_, validation_visitor_, + server.stats(), init_manager_, []() {}); NiceMock secret_callback; auto handle = @@ -197,7 +177,8 @@ TEST_F(SdsApiTest, DeltaUpdateSuccess) { secret_resources.Add()->mutable_resource()->PackFrom(typed_secret); EXPECT_CALL(secret_callback, onAddOrUpdateSecret()); - sds_api.onConfigUpdate(secret_resources, {}, ""); + initialize(); + subscription_factory_.callbacks_->onConfigUpdate(secret_resources, {}, ""); Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), *api_); const std::string cert_pem = @@ -217,12 +198,10 @@ TEST_F(SdsApiTest, DeltaUpdateSuccess) { // a good secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, DynamicCertificateValidationContextUpdateSuccess) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - CertificateValidationContextSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); + CertificateValidationContextSdsApi sds_api(config_source, "abc.com", subscription_factory_, + validation_visitor_, server.stats(), init_manager_, + []() {}); NiceMock secret_callback; auto handle = @@ -241,7 +220,8 @@ TEST_F(SdsApiTest, DynamicCertificateValidationContextUpdateSuccess) { Protobuf::RepeatedPtrField secret_resources; secret_resources.Add()->PackFrom(typed_secret); EXPECT_CALL(secret_callback, onAddOrUpdateSecret()); - sds_api.onConfigUpdate(secret_resources, ""); + initialize(); + subscription_factory_.callbacks_->onConfigUpdate(secret_resources, ""); Ssl::CertificateValidationContextConfigImpl cvc_config(*sds_api.secret(), *api_); const std::string ca_cert = @@ -270,12 +250,10 @@ class MockCvcValidationCallback : public CvcValidationCallback { // provides correct information. TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - CertificateValidationContextSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); + CertificateValidationContextSdsApi sds_api(config_source, "abc.com", subscription_factory_, + validation_visitor_, server.stats(), init_manager_, + []() {}); NiceMock secret_callback; auto handle = @@ -301,7 +279,8 @@ TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { Protobuf::RepeatedPtrField secret_resources; secret_resources.Add()->PackFrom(typed_secret); - sds_api.onConfigUpdate(secret_resources, ""); + initialize(); + subscription_factory_.callbacks_->onConfigUpdate(secret_resources, ""); const std::string default_verify_certificate_hash = "0000000000000000000000000000000000000000000000000000000000000000"; @@ -341,28 +320,24 @@ TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { // Validate that SdsApi throws exception if an empty secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, EmptyResource) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - TlsCertificateSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); + TlsCertificateSdsApi sds_api(config_source, "abc.com", subscription_factory_, validation_visitor_, + server.stats(), init_manager_, []() {}); Protobuf::RepeatedPtrField secret_resources; - EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, + initialize(); + EXPECT_THROW_WITH_MESSAGE(subscription_factory_.callbacks_->onConfigUpdate(secret_resources, ""), + EnvoyException, "Missing SDS resources for abc.com in onConfigUpdate()"); } // Validate that SdsApi throws exception if multiple secrets are passed to onConfigUpdate(). TEST_F(SdsApiTest, SecretUpdateWrongSize) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - TlsCertificateSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); + TlsCertificateSdsApi sds_api(config_source, "abc.com", subscription_factory_, validation_visitor_, + server.stats(), init_manager_, []() {}); std::string yaml = R"EOF( @@ -380,20 +355,18 @@ TEST_F(SdsApiTest, SecretUpdateWrongSize) { secret_resources.Add()->PackFrom(typed_secret); secret_resources.Add()->PackFrom(typed_secret); - EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, - "Unexpected SDS secrets length: 2"); + initialize(); + EXPECT_THROW_WITH_MESSAGE(subscription_factory_.callbacks_->onConfigUpdate(secret_resources, ""), + EnvoyException, "Unexpected SDS secrets length: 2"); } // Validate that SdsApi throws exception if secret name passed to onConfigUpdate() // does not match configured name. TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { NiceMock server; - NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; - TlsCertificateSdsApi sds_api( - server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}, - server.messageValidationVisitor(), *api_); + TlsCertificateSdsApi sds_api(config_source, "abc.com", subscription_factory_, validation_visitor_, + server.stats(), init_manager_, []() {}); std::string yaml = R"EOF( @@ -410,7 +383,9 @@ TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { Protobuf::RepeatedPtrField secret_resources; secret_resources.Add()->PackFrom(typed_secret); - EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, + initialize(); + EXPECT_THROW_WITH_MESSAGE(subscription_factory_.callbacks_->onConfigUpdate(secret_resources, ""), + EnvoyException, "Unexpected SDS secret (expecting abc.com): wrong.name.com"); } diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 5ebcd901fad9..89950982d7ca 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -159,13 +159,14 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { NiceMock dispatcher; NiceMock random; Stats::IsolatedStoreImpl stats; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(secret_context, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(secret_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(secret_context, random()).WillOnce(ReturnRef(random)); + NiceMock init_watcher; + Init::TargetHandlePtr init_target_handle; + EXPECT_CALL(init_manager, add(_)) + .WillOnce(Invoke([&init_target_handle](const Init::Target& target) { + init_target_handle = target.createHandle("test"); + })); EXPECT_CALL(secret_context, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(secret_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager)); auto secret_provider = @@ -183,7 +184,9 @@ name: "abc.com" TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), typed_secret); Protobuf::RepeatedPtrField secret_resources; secret_resources.Add()->PackFrom(typed_secret); - dynamic_cast(*secret_provider).onConfigUpdate(secret_resources, ""); + init_target_handle->initialize(init_watcher); + secret_context.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(secret_resources, + ""); Ssl::TlsCertificateConfigImpl tls_config(*secret_provider->secret(), *api_); const std::string cert_pem = "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_cert.pem"; diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 13985c09deb4..cfc06fe84930 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -17,11 +17,8 @@ envoy_cc_test( deps = [ ":utility_lib", "//source/common/config:utility_lib", - "//source/common/http:message_lib", - "//source/common/json:json_loader_lib", "//source/common/protobuf:utility_lib", "//source/common/upstream:cds_api_lib", - "//test/mocks/local_info:local_info_mocks", "//test/mocks/protobuf:protobuf_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index 5a790c80cf23..a69bdbb786ea 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -6,13 +6,10 @@ #include "envoy/api/v2/core/config_source.pb.validate.h" #include "common/config/utility.h" -#include "common/http/message_impl.h" -#include "common/json/json_loader.h" #include "common/protobuf/utility.h" #include "common/upstream/cds_api_impl.h" #include "test/common/upstream/utility.h" -#include "test/mocks/local_info/mocks.h" #include "test/mocks/protobuf/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" @@ -38,34 +35,14 @@ MATCHER_P(WithName, expectedName, "") { return arg.name() == expectedName; } class CdsApiImplTest : public testing::Test { protected: - CdsApiImplTest() - : request_(&cm_.async_client_), api_(Api::createApiForTest(store_)), - cds_version_( - store_.gauge("cluster_manager.cds.version", Stats::Gauge::ImportMode::NeverImport)) {} - void setup() { - const std::string config_yaml = R"EOF( -api_config_source: - cluster_names: - - foo_cluster - refresh_delay: 1s - api_type: REST - )EOF"; - envoy::api::v2::core::ConfigSource cds_config; - TestUtility::loadFromYamlAndValidate(config_yaml, cds_config); - cluster_map_.emplace("foo_cluster", mock_cluster_); - EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(cluster_map_)); - EXPECT_CALL(mock_cluster_, info()).Times(AnyNumber()); - EXPECT_CALL(*mock_cluster_.info_, addedViaApi()); - EXPECT_CALL(mock_cluster_, info()).Times(AnyNumber()); - EXPECT_CALL(*mock_cluster_.info_, type()); - cds_ = CdsApiImpl::create(cds_config, cm_, dispatcher_, random_, local_info_, store_, - validation_visitor_, *api_); + cds_ = CdsApiImpl::create(cds_config, cm_, store_, validation_visitor_); resetCdsInitializedCb(); - expectRequest(); + EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); cds_->initialize(); + cds_callbacks_ = cm_.subscription_factory_.callbacks_; } void resetCdsInitializedCb() { @@ -75,26 +52,6 @@ class CdsApiImplTest : public testing::Test { }); } - void expectRequest() { - EXPECT_CALL(cm_, httpAsyncClientForCluster("foo_cluster")); - EXPECT_CALL(cm_.async_client_, send_(_, _, _)) - .WillOnce( - Invoke([&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - EXPECT_EQ( - (Http::TestHeaderMapImpl{ - {":method", "POST"}, - {":path", "/v2/discovery:clusters"}, - {":authority", "foo_cluster"}, - {"content-type", "application/json"}, - {"content-length", - request->body() ? fmt::format_int(request->body()->length()).str() : "0"}}), - request->headers()); - callbacks_ = &callbacks; - return &request_; - })); - } - ClusterManager::ClusterInfoMap makeClusterMap(std::vector clusters) { ClusterManager::ClusterInfoMap map; for (auto cluster : clusters) { @@ -103,15 +60,6 @@ class CdsApiImplTest : public testing::Test { return map; } - static Http::MessagePtr parseResponseMessageFromYaml(const std::string& yaml) { - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - const auto json = Json::Factory::loadFromYamlString(yaml); - ASSERT(json->isObject()); - message->body() = std::make_unique(json->asJsonString()); - return message; - } - class MockWarmingClusterManager : public MockClusterManager { public: explicit MockWarmingClusterManager(TimeSource& time_source) : MockClusterManager(time_source) {} @@ -174,18 +122,11 @@ class CdsApiImplTest : public testing::Test { NiceMock cm_; Upstream::ClusterManager::ClusterInfoMap cluster_map_; Upstream::MockClusterMockPrioritySet mock_cluster_; - NiceMock dispatcher_; - NiceMock random_; - NiceMock local_info_; Stats::IsolatedStoreImpl store_; - Http::MockAsyncClientRequest request_; CdsApiPtr cds_; - Event::MockTimer* interval_timer_; - Http::AsyncClient::Callbacks* callbacks_{}; + Config::SubscriptionCallbacks* cds_callbacks_{}; ReadyWatcher initialized_; NiceMock validation_visitor_; - Api::ApiPtr api_; - Stats::Gauge& cds_version_; }; // Negative test for protoc-gen-validate constraints. @@ -200,14 +141,12 @@ TEST_F(CdsApiImplTest, ValidateFail) { EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(cluster_map_)); EXPECT_CALL(initialized_, ready()); - EXPECT_THROW(dynamic_cast(cds_.get())->onConfigUpdate(clusters, ""), EnvoyException); - EXPECT_CALL(request_, cancel()); + EXPECT_THROW(cds_callbacks_->onConfigUpdate(clusters, ""), EnvoyException); } // Regression test against only updating versionInfo() if at least one cluster // is are added/updated even if one or more are removed. TEST_F(CdsApiImplTest, UpdateVersionOnClusterRemove) { - interval_timer_ = new Event::MockTimer(&dispatcher_); InSequence s; setup(); @@ -222,34 +161,24 @@ version_info: '0' eds_config: path: eds path )EOF"; + auto response1 = TestUtility::parseYaml(response1_yaml); EXPECT_CALL(cm_, clusters()).WillOnce(Return(ClusterManager::ClusterInfoMap{})); cm_.expectAdd("cluster1", "0"); EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); EXPECT_EQ("", cds_->versionInfo()); - EXPECT_EQ( - 0UL, - store_.gauge("cluster_manager.cds.version", Stats::Gauge::ImportMode::NeverImport).value()); - callbacks_->onSuccess(parseResponseMessageFromYaml(response1_yaml)); + cds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ("0", cds_->versionInfo()); - expectRequest(); - interval_timer_->callback_(); - const std::string response2_yaml = R"EOF( version_info: '1' resources: )EOF"; + auto response2 = TestUtility::parseYaml(response2_yaml); EXPECT_CALL(cm_, clusters()).WillOnce(Return(makeClusterMap({"cluster1"}))); - EXPECT_CALL(cm_, removeCluster("cluster1")).WillOnce(Return(true)); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(parseResponseMessageFromYaml(response2_yaml)); - - EXPECT_EQ(2UL, store_.counter("cluster_manager.cds.update_attempt").value()); - EXPECT_EQ(2UL, store_.counter("cluster_manager.cds.update_success").value()); + cds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); EXPECT_EQ("1", cds_->versionInfo()); } @@ -267,11 +196,9 @@ TEST_F(CdsApiImplTest, ValidateDuplicateClusters) { EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(cluster_map_)); EXPECT_CALL(initialized_, ready()); - EXPECT_THROW_WITH_MESSAGE(dynamic_cast(cds_.get())->onConfigUpdate(clusters, ""), - EnvoyException, + EXPECT_THROW_WITH_MESSAGE(cds_callbacks_->onConfigUpdate(clusters, ""), EnvoyException, "Error adding/updating cluster(s) duplicate_cluster: duplicate cluster " "duplicate_cluster found"); - EXPECT_CALL(request_, cancel()); } TEST_F(CdsApiImplTest, EmptyConfigUpdate) { @@ -281,10 +208,9 @@ TEST_F(CdsApiImplTest, EmptyConfigUpdate) { EXPECT_CALL(cm_, clusters()).WillOnce(Return(ClusterManager::ClusterInfoMap{})); EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(request_, cancel()); Protobuf::RepeatedPtrField clusters; - dynamic_cast(cds_.get())->onConfigUpdate(clusters, ""); + cds_callbacks_->onConfigUpdate(clusters, ""); } TEST_F(CdsApiImplTest, ConfigUpdateWith2ValidClusters) { @@ -295,7 +221,6 @@ TEST_F(CdsApiImplTest, ConfigUpdateWith2ValidClusters) { EXPECT_CALL(cm_, clusters()).WillOnce(Return(ClusterManager::ClusterInfoMap{})); EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(request_, cancel()); Protobuf::RepeatedPtrField clusters; @@ -309,7 +234,7 @@ TEST_F(CdsApiImplTest, ConfigUpdateWith2ValidClusters) { clusters.Add()->PackFrom(cluster_2); cm_.expectAdd("cluster_2"); - dynamic_cast(cds_.get())->onConfigUpdate(clusters, ""); + cds_callbacks_->onConfigUpdate(clusters, ""); } TEST_F(CdsApiImplTest, DeltaConfigUpdate) { @@ -318,7 +243,6 @@ TEST_F(CdsApiImplTest, DeltaConfigUpdate) { setup(); } EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(request_, cancel()); { Protobuf::RepeatedPtrField resources; @@ -340,7 +264,7 @@ TEST_F(CdsApiImplTest, DeltaConfigUpdate) { resource->set_name("cluster_2"); resource->set_version("v1"); } - dynamic_cast(cds_.get())->onConfigUpdate(resources, {}, "v1"); + cds_callbacks_->onConfigUpdate(resources, {}, "v1"); } { @@ -357,7 +281,7 @@ TEST_F(CdsApiImplTest, DeltaConfigUpdate) { Protobuf::RepeatedPtrField removed; *removed.Add() = "cluster_1"; EXPECT_CALL(cm_, removeCluster(StrEq("cluster_1"))).WillOnce(Return(true)); - dynamic_cast(cds_.get())->onConfigUpdate(resources, removed, "v2"); + cds_callbacks_->onConfigUpdate(resources, removed, "v2"); } } @@ -369,7 +293,6 @@ TEST_F(CdsApiImplTest, ConfigUpdateAddsSecondClusterEvenIfFirstThrows) { EXPECT_CALL(cm_, clusters()).WillOnce(Return(ClusterManager::ClusterInfoMap{})); EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(request_, cancel()); Protobuf::RepeatedPtrField clusters; @@ -389,29 +312,11 @@ TEST_F(CdsApiImplTest, ConfigUpdateAddsSecondClusterEvenIfFirstThrows) { cm_.expectAddToThrow("cluster_3", "Another exception"); EXPECT_THROW_WITH_MESSAGE( - dynamic_cast(cds_.get())->onConfigUpdate(clusters, ""), EnvoyException, + cds_callbacks_->onConfigUpdate(clusters, ""), EnvoyException, "Error adding/updating cluster(s) cluster_1: An exception, cluster_3: Another exception"); } -TEST_F(CdsApiImplTest, InvalidOptions) { - const std::string config_yaml = R"EOF( -api_config_source: - cluster_names: - - foo_cluster - refresh_delay: 1s - )EOF"; - - local_info_.node_.set_cluster(""); - local_info_.node_.set_id(""); - envoy::api::v2::core::ConfigSource cds_config; - TestUtility::loadFromYamlAndValidate(config_yaml, cds_config); - EXPECT_THROW(CdsApiImpl::create(cds_config, cm_, dispatcher_, random_, local_info_, store_, - validation_visitor_, *api_), - EnvoyException); -} - TEST_F(CdsApiImplTest, Basic) { - interval_timer_ = new Event::MockTimer(&dispatcher_); InSequence s; setup(); @@ -432,20 +337,15 @@ version_info: '0' eds_config: path: eds path )EOF"; + auto response1 = TestUtility::parseYaml(response1_yaml); EXPECT_CALL(cm_, clusters()).WillOnce(Return(ClusterManager::ClusterInfoMap{})); cm_.expectAdd("cluster1", "0"); cm_.expectAdd("cluster2", "0"); EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); EXPECT_EQ("", cds_->versionInfo()); - EXPECT_EQ(0UL, cds_version_.value()); - callbacks_->onSuccess(parseResponseMessageFromYaml(response1_yaml)); + cds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ("0", cds_->versionInfo()); - EXPECT_EQ(7148434200721666028U, cds_version_.value()); - - expectRequest(); - interval_timer_->callback_(); const std::string response2_yaml = R"EOF( version_info: '1' @@ -463,22 +363,18 @@ version_info: '1' eds_config: path: eds path )EOF"; + auto response2 = TestUtility::parseYaml(response2_yaml); EXPECT_CALL(cm_, clusters()).WillOnce(Return(makeClusterMap({"cluster1", "cluster2"}))); cm_.expectAdd("cluster1", "1"); cm_.expectAdd("cluster3", "1"); EXPECT_CALL(cm_, removeCluster("cluster2")); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(parseResponseMessageFromYaml(response2_yaml)); + cds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); - EXPECT_EQ(2UL, store_.counter("cluster_manager.cds.update_attempt").value()); - EXPECT_EQ(2UL, store_.counter("cluster_manager.cds.update_success").value()); EXPECT_EQ("1", cds_->versionInfo()); - EXPECT_EQ(13237225503670494420U, cds_version_.value()); } TEST_F(CdsApiImplTest, CdsPauseOnWarming) { - interval_timer_ = new Event::MockTimer(&dispatcher_); EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(ClusterManager::ClusterInfoMap{})); InSequence s; @@ -500,24 +396,21 @@ version_info: '0' eds_config: path: eds path )EOF"; + auto response1 = TestUtility::parseYaml(response1_yaml); // Two clusters updated, both warmed up. - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)); cm_.expectAddWithWarming("cluster1", "0"); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)); cm_.expectAddWithWarming("cluster2", "0"); cm_.expectWarmingClusterCount(); EXPECT_CALL(initialized_, ready()); cm_.expectWarmingClusterCount(2); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)).Times(1); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); - EXPECT_CALL(*interval_timer_, enableTimer(_)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)); cm_.clustersToWarmUp({"cluster1", "cluster2"}); - callbacks_->onSuccess(parseResponseMessageFromYaml(response1_yaml)); - - expectRequest(); - interval_timer_->callback_(); + cds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); // Two clusters updated, only one warmed up. const std::string response2_yaml = R"EOF( @@ -536,23 +429,20 @@ version_info: '1' eds_config: path: eds path )EOF"; + auto response2 = TestUtility::parseYaml(response2_yaml); - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)); cm_.expectAddWithWarming("cluster1", "1"); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)); cm_.expectAddWithWarming("cluster3", "1"); cm_.expectWarmingClusterCount(); EXPECT_CALL(initialized_, ready()); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); - EXPECT_CALL(*interval_timer_, enableTimer(_)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)); resetCdsInitializedCb(); cm_.clustersToWarmUp({"cluster1"}); - callbacks_->onSuccess(parseResponseMessageFromYaml(response2_yaml)); - - expectRequest(); - interval_timer_->callback_(); + cds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); // One cluster updated and warmed up. Also finish warming up of the previously added cluster3. const std::string response3_yaml = R"EOF( @@ -565,21 +455,18 @@ version_info: '2' eds_config: path: eds path )EOF"; + auto response3 = TestUtility::parseYaml(response3_yaml); - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)); cm_.expectAddWithWarming("cluster4", "2"); cm_.expectWarmingClusterCount(); EXPECT_CALL(initialized_, ready()); cm_.expectWarmingClusterCount(2); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)).Times(1); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); - EXPECT_CALL(*interval_timer_, enableTimer(_)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)); resetCdsInitializedCb(); cm_.clustersToWarmUp({"cluster4", "cluster3"}); - callbacks_->onSuccess(parseResponseMessageFromYaml(response3_yaml)); - - expectRequest(); - interval_timer_->callback_(); + cds_callbacks_->onConfigUpdate(response3.resources(), response3.version_info()); const std::string response4_yaml = R"EOF( version_info: '3' @@ -597,34 +484,34 @@ version_info: '3' eds_config: path: eds path )EOF"; + auto response4 = TestUtility::parseYaml(response4_yaml); // Two clusters updated, first one warmed up before processing of the second one starts. - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().ClusterLoadAssignment)); cm_.expectAddWithWarming("cluster5", "3", true); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)).Times(1); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)); cm_.expectAddWithWarming("cluster6", "3"); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)).Times(1); + EXPECT_CALL(cm_.ads_mux_, pause(Config::TypeUrl::get().Cluster)); EXPECT_CALL(initialized_, ready()); cm_.expectWarmingClusterCount(); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)).Times(1); - EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)).Times(1); - EXPECT_CALL(*interval_timer_, enableTimer(_)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().Cluster)); + EXPECT_CALL(cm_.ads_mux_, resume(Config::TypeUrl::get().ClusterLoadAssignment)); resetCdsInitializedCb(); cm_.clustersToWarmUp({"cluster6"}); - callbacks_->onSuccess(parseResponseMessageFromYaml(response4_yaml)); + cds_callbacks_->onConfigUpdate(response4.resources(), response4.version_info()); } -TEST_F(CdsApiImplTest, Failure) { - interval_timer_ = new Event::MockTimer(&dispatcher_); +// Validate behavior when the config is delivered but it fails PGV validation. +TEST_F(CdsApiImplTest, FailureInvalidConfig) { InSequence s; setup(); - const std::string response_yaml = R"EOF( + const std::string response1_yaml = R"EOF( version_info: '0' resources: - "@type": type.googleapis.com/envoy.api.v2.Cluster @@ -640,25 +527,24 @@ version_info: '0' eds_config: path: eds path )EOF"; + auto response1 = TestUtility::parseYaml(response1_yaml); EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(cluster_map_)); EXPECT_CALL(initialized_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(parseResponseMessageFromYaml(response_yaml)); - - expectRequest(); - interval_timer_->callback_(); + EXPECT_THROW(cds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), + EnvoyException); + EXPECT_EQ("", cds_->versionInfo()); +} - EXPECT_CALL(*interval_timer_, enableTimer(_)); +// Validate behavior when the config fails delivery at the subscription level. +TEST_F(CdsApiImplTest, FailureSubscription) { + InSequence s; - callbacks_->onFailure(Http::AsyncClient::FailureReason::Reset); + setup(); + EXPECT_CALL(initialized_, ready()); + cds_callbacks_->onConfigUpdateFailed({}); EXPECT_EQ("", cds_->versionInfo()); - EXPECT_EQ(2UL, store_.counter("cluster_manager.cds.update_attempt").value()); - EXPECT_EQ(1UL, store_.counter("cluster_manager.cds.update_failure").value()); - // Validate that the schema error increments update_rejected stat. - EXPECT_EQ(1UL, store_.counter("cluster_manager.cds.update_rejected").value()); - EXPECT_EQ(0UL, cds_version_.value()); } } // namespace diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 37f7f587e88e..002a15e3a2d4 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -155,6 +155,16 @@ class TestClusterManagerImpl : public ClusterManagerImpl { public: using ClusterManagerImpl::ClusterManagerImpl; + TestClusterManagerImpl(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, + ClusterManagerFactory& factory, Stats::Store& stats, + ThreadLocal::Instance& tls, Runtime::Loader& runtime, + Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, + AccessLog::AccessLogManager& log_manager, + Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, + Api::Api& api, Http::Context& http_context) + : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, log_manager, + main_thread_dispatcher, admin, validation_visitor_, api, http_context) {} + std::map> activeClusters() { std::map> clusters; for (auto& cluster : active_clusters_) { @@ -162,6 +172,8 @@ class TestClusterManagerImpl : public ClusterManagerImpl { } return clusters; } + + NiceMock validation_visitor_; }; // Override postThreadLocalClusterUpdate so we can test that merged updates calls diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 4da7882a21f8..9a92063f073f 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -72,12 +72,6 @@ class EdsTest : public testing::Test { void resetCluster(const std::string& yaml_config) { local_info_.node_.mutable_locality()->set_zone("us-east-1a"); eds_cluster_ = parseClusterFromV2Yaml(yaml_config); - Upstream::ClusterManager::ClusterInfoMap cluster_map; - Upstream::MockClusterMockPrioritySet cluster; - cluster_map.emplace("eds", cluster); - EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_CALL(cluster, info()).Times(2); - EXPECT_CALL(*cluster.info_, addedViaApi()); Envoy::Stats::ScopePtr scope = stats_.createScope(fmt::format( "cluster.{}.", eds_cluster_.alt_stat_name().empty() ? eds_cluster_.name() : eds_cluster_.alt_stat_name())); @@ -87,21 +81,29 @@ class EdsTest : public testing::Test { cluster_.reset( new EdsClusterImpl(eds_cluster_, runtime_, factory_context, std::move(scope), false)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); + eds_callbacks_ = cm_.subscription_factory_.callbacks_; + } + + void initialize() { + EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); + cluster_->initialize([this] { initialized_ = true; }); } void doOnConfigUpdateVerifyNoThrow( const envoy::api::v2::ClusterLoadAssignment& cluster_load_assignment) { Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + VERBOSE_EXPECT_NO_THROW(eds_callbacks_->onConfigUpdate(resources, "")); } + bool initialized_{}; Stats::IsolatedStoreImpl stats_; Ssl::MockContextManager ssl_context_manager_; envoy::api::v2::Cluster eds_cluster_; NiceMock cm_; NiceMock dispatcher_; std::shared_ptr cluster_; + Config::SubscriptionCallbacks* eds_callbacks_{}; NiceMock random_; NiceMock runtime_; NiceMock local_info_; @@ -200,10 +202,12 @@ class EdsWithHealthCheckUpdateTest : public EdsTest { // Negative test for protoc-gen-validate constraints. TEST_F(EdsTest, ValidateFail) { + initialize(); envoy::api::v2::ClusterLoadAssignment resource; Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(resource); - EXPECT_THROW(cluster_->onConfigUpdate(resources, ""), ProtoValidationException); + EXPECT_THROW(eds_callbacks_->onConfigUpdate(resources, ""), ProtoValidationException); + EXPECT_FALSE(initialized_); } // Validate that onConfigUpdate() with unexpected cluster names rejects config. @@ -212,47 +216,43 @@ TEST_F(EdsTest, OnConfigUpdateWrongName) { cluster_load_assignment.set_cluster_name("wrong name"); Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); - EXPECT_THROW(cluster_->onConfigUpdate(resources, ""), EnvoyException); - cluster_->onConfigUpdateFailed(nullptr); - EXPECT_TRUE(initialized); + initialize(); + EXPECT_THROW(eds_callbacks_->onConfigUpdate(resources, ""), EnvoyException); + eds_callbacks_->onConfigUpdateFailed(nullptr); + EXPECT_TRUE(initialized_); } // Validate that onConfigUpdate() with empty cluster vector size ignores config. TEST_F(EdsTest, OnConfigUpdateEmpty) { - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); - cluster_->onConfigUpdate({}, ""); + initialize(); + eds_callbacks_->onConfigUpdate({}, ""); Protobuf::RepeatedPtrField resources; Protobuf::RepeatedPtrField removed_resources; - cluster_->onConfigUpdate(resources, removed_resources, ""); + eds_callbacks_->onConfigUpdate(resources, removed_resources, ""); EXPECT_EQ(2UL, stats_.counter("cluster.name.update_empty").value()); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); } // Validate that onConfigUpdate() with unexpected cluster vector size rejects config. TEST_F(EdsTest, OnConfigUpdateWrongSize) { - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); envoy::api::v2::ClusterLoadAssignment cluster_load_assignment; cluster_load_assignment.set_cluster_name("fare"); Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); resources.Add()->PackFrom(cluster_load_assignment); - EXPECT_THROW(cluster_->onConfigUpdate(resources, ""), EnvoyException); - cluster_->onConfigUpdateFailed(nullptr); - EXPECT_TRUE(initialized); + EXPECT_THROW(eds_callbacks_->onConfigUpdate(resources, ""), EnvoyException); + eds_callbacks_->onConfigUpdateFailed(nullptr); + EXPECT_TRUE(initialized_); } // Validate that onConfigUpdate() with the expected cluster accepts config. TEST_F(EdsTest, OnConfigUpdateSuccess) { envoy::api::v2::ClusterLoadAssignment cluster_load_assignment; cluster_load_assignment.set_cluster_name("fare"); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value()); } @@ -260,16 +260,15 @@ TEST_F(EdsTest, OnConfigUpdateSuccess) { TEST_F(EdsTest, DeltaOnConfigUpdateSuccess) { envoy::api::v2::ClusterLoadAssignment cluster_load_assignment; cluster_load_assignment.set_cluster_name("fare"); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); Protobuf::RepeatedPtrField resources; auto* resource = resources.Add(); resource->mutable_resource()->PackFrom(cluster_load_assignment); resource->set_version("v1"); - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, {}, "v1")); + VERBOSE_EXPECT_NO_THROW(eds_callbacks_->onConfigUpdate(resources, {}, "v1")); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value()); } @@ -290,10 +289,9 @@ TEST_F(EdsTest, NoServiceNameOnSuccessConfigUpdate) { )EOF"); envoy::api::v2::ClusterLoadAssignment cluster_load_assignment; cluster_load_assignment.set_cluster_name("name"); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); } // Validate that onConfigUpdate() updates the endpoint metadata. @@ -323,10 +321,9 @@ TEST_F(EdsTest, EndpointMetadata) { Config::MetadataFilters::get().ENVOY_LB, "version") .set_string_value("v1"); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); @@ -401,10 +398,9 @@ TEST_F(EdsTest, EndpointHealthStatus) { endpoint->set_health_status(hs.first); } - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), health_status_expected.size()); @@ -913,10 +909,9 @@ TEST_F(EdsTest, EndpointAddedWithNewOverprovisioningFactor) { endpoint_address->set_port_value(80); } - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); EXPECT_EQ(1, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ("1.2.3.4:80", @@ -950,10 +945,9 @@ TEST_F(EdsTest, EndpointLocality) { endpoint_address->set_port_value(80); } - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 2); @@ -989,10 +983,9 @@ TEST_F(EdsTest, EndpointLocalityWeightsIgnored) { endpoint_address->set_port_value(80); } - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); EXPECT_EQ(nullptr, cluster_->prioritySet().hostSetsPerPriority()[0]->localityWeights()); } @@ -1066,10 +1059,9 @@ TEST_F(EdsTest, EndpointLocalityWeights) { endpoint_address->set_port_value(80); } - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); const auto& locality_weights = *cluster_->prioritySet().hostSetsPerPriority()[0]->localityWeights(); @@ -1112,10 +1104,9 @@ TEST_F(EdsTest, RemoveUnreferencedLocalities) { add_hosts_to_locality("oceania", "bear", "best", 4, 1); add_hosts_to_locality("", "us-west-1a", "", 2, 1); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); { auto& hosts_per_locality = @@ -1192,10 +1183,9 @@ TEST_F(EdsTest, EndpointHostsPerLocality) { add_hosts_to_locality("oceania", "koala", "ingsoc", 2); add_hosts_to_locality("", "us-east-1a", "", 1); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); { auto& hosts_per_locality = cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality(); @@ -1260,10 +1250,9 @@ TEST_F(EdsTest, EndpointHostPerPriority) { add_hosts_to_locality("oceania", "koala", "ingsoc", 2, 0); add_hosts_to_locality("", "us-east-1a", "", 1, 1); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); @@ -1314,10 +1303,9 @@ TEST_F(EdsTest, EndpointHostsPerPriority) { add_hosts_to_priority(0, 2); add_hosts_to_priority(1, 1); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); ASSERT_EQ(2, cluster_->prioritySet().hostSetsPerPriority().size()); EXPECT_EQ(2, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); @@ -1372,11 +1360,10 @@ TEST_F(EdsTest, NoPriorityForLocalCluster) { // should fail. add_hosts_to_priority(0, 2); add_hosts_to_priority(1, 1); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); - EXPECT_THROW_WITH_MESSAGE(cluster_->onConfigUpdate(resources, ""), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(eds_callbacks_->onConfigUpdate(resources, ""), EnvoyException, "Unexpected non-zero priority for local cluster 'fare'."); // Try an update which only has endpoints with P=0. This should go through. @@ -1418,10 +1405,9 @@ TEST_F(EdsTest, PriorityAndLocality) { add_hosts_to_locality_and_priority("", "us-east-1a", "", 1, 8); add_hosts_to_locality_and_priority("foo", "bar", "eep", 1, 2); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); { auto& first_hosts_per_locality = @@ -1528,10 +1514,9 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { add_hosts_to_locality_and_priority("", "us-east-1a", "", 1, 8, 60); add_hosts_to_locality_and_priority("foo", "bar", "eep", 1, 2, 40); - bool initialized = false; - cluster_->initialize([&initialized] { initialized = true; }); + initialize(); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - EXPECT_TRUE(initialized); + EXPECT_TRUE(initialized_); EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); { @@ -1663,10 +1648,10 @@ TEST_F(EdsTest, MalformedIP) { "foo.bar.com"); endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(80); - cluster_->initialize([] {}); + initialize(); Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); - EXPECT_THROW_WITH_MESSAGE(cluster_->onConfigUpdate(resources, ""), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(eds_callbacks_->onConfigUpdate(resources, ""), EnvoyException, "malformed IP address: foo.bar.com. Consider setting resolver_name or " "setting cluster type to 'STRICT_DNS' or 'LOGICAL_DNS'"); } diff --git a/test/extensions/filters/http/health_check/health_check_test.cc b/test/extensions/filters/http/health_check/health_check_test.cc index 69ee52ac31fe..03ead8af99cd 100644 --- a/test/extensions/filters/http/health_check/health_check_test.cc +++ b/test/extensions/filters/http/health_check/health_check_test.cc @@ -17,6 +17,7 @@ using testing::_; using testing::DoAll; +using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -145,8 +146,8 @@ TEST_F(HealthCheckFilterNoPassThroughTest, ComputedHealth) { MockHealthCheckCluster cluster_www2(1000, 800); EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false)); EXPECT_CALL(context_, clusterManager()); - EXPECT_CALL(context_.cluster_manager_, get("www1")).WillRepeatedly(Return(&cluster_www1)); - EXPECT_CALL(context_.cluster_manager_, get("www2")).WillRepeatedly(Return(&cluster_www2)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www1"))).WillRepeatedly(Return(&cluster_www1)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(&cluster_www2)); EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); @@ -159,8 +160,8 @@ TEST_F(HealthCheckFilterNoPassThroughTest, ComputedHealth) { MockHealthCheckCluster cluster_www2(1000, 800); EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false)); EXPECT_CALL(context_, clusterManager()); - EXPECT_CALL(context_.cluster_manager_, get("www1")).WillRepeatedly(Return(&cluster_www1)); - EXPECT_CALL(context_.cluster_manager_, get("www2")).WillRepeatedly(Return(&cluster_www2)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www1"))).WillRepeatedly(Return(&cluster_www1)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(&cluster_www2)); EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); @@ -173,8 +174,8 @@ TEST_F(HealthCheckFilterNoPassThroughTest, ComputedHealth) { MockHealthCheckCluster cluster_www2(1000, 800); EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false)); EXPECT_CALL(context_, clusterManager()); - EXPECT_CALL(context_.cluster_manager_, get("www1")).WillRepeatedly(Return(&cluster_www1)); - EXPECT_CALL(context_.cluster_manager_, get("www2")).WillRepeatedly(Return(&cluster_www2)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www1"))).WillRepeatedly(Return(&cluster_www1)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(&cluster_www2)); EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); @@ -190,8 +191,8 @@ TEST_F(HealthCheckFilterNoPassThroughTest, ComputedHealth) { MockHealthCheckCluster cluster_www2(1000, 0); EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false)); EXPECT_CALL(context_, clusterManager()); - EXPECT_CALL(context_.cluster_manager_, get("www1")).WillRepeatedly(Return(&cluster_www1)); - EXPECT_CALL(context_.cluster_manager_, get("www2")).WillRepeatedly(Return(&cluster_www2)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www1"))).WillRepeatedly(Return(&cluster_www1)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(&cluster_www2)); EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); @@ -205,8 +206,8 @@ TEST_F(HealthCheckFilterNoPassThroughTest, ComputedHealth) { MockHealthCheckCluster cluster_www2(1000, 0, 800); EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false)); EXPECT_CALL(context_, clusterManager()); - EXPECT_CALL(context_.cluster_manager_, get("www1")).WillRepeatedly(Return(&cluster_www1)); - EXPECT_CALL(context_.cluster_manager_, get("www2")).WillRepeatedly(Return(&cluster_www2)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www1"))).WillRepeatedly(Return(&cluster_www1)); + EXPECT_CALL(context_.cluster_manager_, get(Eq("www2"))).WillRepeatedly(Return(&cluster_www2)); EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); diff --git a/test/extensions/filters/http/lua/lua_filter_test.cc b/test/extensions/filters/http/lua/lua_filter_test.cc index f133c1471e4b..e24b267b7823 100644 --- a/test/extensions/filters/http/lua/lua_filter_test.cc +++ b/test/extensions/filters/http/lua/lua_filter_test.cc @@ -18,6 +18,7 @@ using testing::_; using testing::AtLeast; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::Return; @@ -758,7 +759,7 @@ TEST_F(LuaHttpFilterTest, HttpCall) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -833,7 +834,7 @@ TEST_F(LuaHttpFilterTest, DoubleHttpCall) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -856,7 +857,7 @@ TEST_F(LuaHttpFilterTest, DoubleHttpCall) { response_message->body() = std::make_unique("response"); EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq(":status 200"))); EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("response"))); - EXPECT_CALL(cluster_manager_, get("cluster2")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster2"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster2")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -912,7 +913,7 @@ TEST_F(LuaHttpFilterTest, HttpCallNoBody) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -967,7 +968,7 @@ TEST_F(LuaHttpFilterTest, HttpCallImmediateResponse) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -1015,7 +1016,7 @@ TEST_F(LuaHttpFilterTest, HttpCallErrorAfterResumeSuccess) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -1065,7 +1066,7 @@ TEST_F(LuaHttpFilterTest, HttpCallFailure) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -1107,7 +1108,7 @@ TEST_F(LuaHttpFilterTest, HttpCallReset) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -1150,7 +1151,7 @@ TEST_F(LuaHttpFilterTest, HttpCallImmediateFailure) { Http::TestHeaderMapImpl request_headers{{":path", "/"}}; Http::MockAsyncClientRequest request(&cluster_manager_.async_client_); - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster")); EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) .WillOnce( @@ -1202,7 +1203,7 @@ TEST_F(LuaHttpFilterTest, HttpCallInvalidCluster) { setup(SCRIPT); Http::TestHeaderMapImpl request_headers{{":path", "/"}}; - EXPECT_CALL(cluster_manager_, get("cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))).WillOnce(Return(nullptr)); EXPECT_CALL( *filter_, scriptLog(spdlog::level::err, @@ -1226,7 +1227,7 @@ TEST_F(LuaHttpFilterTest, HttpCallInvalidHeaders) { setup(SCRIPT); Http::TestHeaderMapImpl request_headers{{":path", "/"}}; - EXPECT_CALL(cluster_manager_, get("cluster")); + EXPECT_CALL(cluster_manager_, get(Eq("cluster"))); EXPECT_CALL(*filter_, scriptLog(spdlog::level::err, StrEq("[string \"...\"]:3: http call headers must include " "':path', ':method', and ':authority'"))); diff --git a/test/extensions/filters/http/squash/squash_filter_test.cc b/test/extensions/filters/http/squash/squash_filter_test.cc index c10f6c654e83..ac0c30f1516b 100644 --- a/test/extensions/filters/http/squash/squash_filter_test.cc +++ b/test/extensions/filters/http/squash/squash_filter_test.cc @@ -18,6 +18,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -63,7 +64,7 @@ TEST(SoloFilterConfigTest, V1ApiConversion) { Envoy::Json::ObjectSharedPtr json_config = Envoy::Json::Factory::loadFromString(json); NiceMock factory_context; - EXPECT_CALL(factory_context.cluster_manager_, get("fake_cluster")).Times(1); + EXPECT_CALL(factory_context.cluster_manager_, get(Eq("fake_cluster"))).Times(1); auto config = constructSquashFilterConfigFromJson(*json_config, factory_context); EXPECT_EQ("fake_cluster", config.clusterName()); @@ -84,7 +85,7 @@ TEST(SoloFilterConfigTest, NoCluster) { Envoy::Json::ObjectSharedPtr config = Envoy::Json::Factory::loadFromString(json); NiceMock factory_context; - EXPECT_CALL(factory_context.cluster_manager_, get("fake_cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(factory_context.cluster_manager_, get(Eq("fake_cluster"))).WillOnce(Return(nullptr)); EXPECT_THROW_WITH_MESSAGE(constructSquashFilterConfigFromJson(*config, factory_context), Envoy::EnvoyException, @@ -102,7 +103,7 @@ TEST(SoloFilterConfigTest, ParsesEnvironment) { Envoy::Json::ObjectSharedPtr json_config = Envoy::Json::Factory::loadFromString(json); NiceMock factory_context; - EXPECT_CALL(factory_context.cluster_manager_, get("squash")).Times(1); + EXPECT_CALL(factory_context.cluster_manager_, get(Eq("squash"))).Times(1); auto config = constructSquashFilterConfigFromJson(*json_config, factory_context); EXPECT_JSON_EQ(expected_json, config.attachmentJson()); @@ -122,7 +123,7 @@ TEST(SoloFilterConfigTest, ParsesAndEscapesEnvironment) { Envoy::Json::ObjectSharedPtr json_config = Envoy::Json::Factory::loadFromString(json); NiceMock factory_context; - EXPECT_CALL(factory_context.cluster_manager_, get("squash")).Times(1); + EXPECT_CALL(factory_context.cluster_manager_, get(Eq("squash"))).Times(1); auto config = constructSquashFilterConfigFromJson(*json_config, factory_context); EXPECT_JSON_EQ(expected_json, config.attachmentJson()); } @@ -159,7 +160,7 @@ TEST(SoloFilterConfigTest, ParsesEnvironmentInComplexTemplate) { Envoy::Json::ObjectSharedPtr json_config = Envoy::Json::Factory::loadFromString(json); NiceMock factory_context; - EXPECT_CALL(factory_context.cluster_manager_, get("squash")).Times(1); + EXPECT_CALL(factory_context.cluster_manager_, get(Eq("squash"))).Times(1); auto config = constructSquashFilterConfigFromJson(*json_config, factory_context); EXPECT_JSON_EQ(expected_json, config.attachmentJson()); } diff --git a/test/extensions/filters/network/client_ssl_auth/client_ssl_auth_test.cc b/test/extensions/filters/network/client_ssl_auth/client_ssl_auth_test.cc index 8a7764ec607f..7c5e09642488 100644 --- a/test/extensions/filters/network/client_ssl_auth/client_ssl_auth_test.cc +++ b/test/extensions/filters/network/client_ssl_auth/client_ssl_auth_test.cc @@ -21,6 +21,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::Return; @@ -73,7 +74,7 @@ stat_prefix: vpn envoy::config::filter::network::client_ssl_auth::v2::ClientSSLAuth proto_config{}; TestUtility::loadFromYaml(yaml, proto_config); - EXPECT_CALL(cm_, get("vpn")); + EXPECT_CALL(cm_, get(Eq("vpn"))); setupRequest(); config_ = ClientSslAuthConfig::create(proto_config, tls_, cm_, dispatcher_, stats_store_, random_); @@ -125,7 +126,7 @@ stat_prefix: bad_cluster envoy::config::filter::network::client_ssl_auth::v2::ClientSSLAuth proto_config{}; TestUtility::loadFromYaml(yaml, proto_config); - EXPECT_CALL(cm_, get("bad_cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, get(Eq("bad_cluster"))).WillOnce(Return(nullptr)); EXPECT_THROW( ClientSslAuthConfig::create(proto_config, tls_, cm_, dispatcher_, stats_store_, random_), EnvoyException); diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index a5fec366c810..6376a8a96166 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -13,6 +13,7 @@ using testing::_; using testing::ContainsRegex; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -354,7 +355,7 @@ TEST_F(DubboRouterTest, NoCluster) { EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); - EXPECT_CALL(context_.cluster_manager_, get(cluster_name_)).WillOnce(Return(nullptr)); + EXPECT_CALL(context_.cluster_manager_, get(Eq(cluster_name_))).WillOnce(Return(nullptr)); EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index ab8fc0a16a3e..5ecb9e9aed53 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -45,7 +45,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client .WillOnce(DoAll(SaveArgAddress(&update_callbacks_), ReturnNew())); if (!cluster_exists) { - EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillOnce(Return(nullptr)); } std::unique_ptr conn_pool_impl = std::make_unique( diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index dcc72d52780d..ad4686746b4b 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -23,6 +23,7 @@ using testing::_; using testing::ContainsRegex; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -441,7 +442,7 @@ TEST_F(ThriftRouterTest, NoCluster) { EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); - EXPECT_CALL(context_.cluster_manager_, get(cluster_name_)).WillOnce(Return(nullptr)); + EXPECT_CALL(context_.cluster_manager_, get(Eq(cluster_name_))).WillOnce(Return(nullptr)); EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); diff --git a/test/extensions/tracers/datadog/config_test.cc b/test/extensions/tracers/datadog/config_test.cc index 82a551181fdb..363be39bba76 100644 --- a/test/extensions/tracers/datadog/config_test.cc +++ b/test/extensions/tracers/datadog/config_test.cc @@ -6,6 +6,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::NiceMock; using testing::Return; @@ -17,7 +18,7 @@ namespace { TEST(DatadogTracerConfigTest, DatadogHttpTracer) { NiceMock server; - EXPECT_CALL(server.cluster_manager_, get("fake_cluster")) + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); ON_CALL(*server.cluster_manager_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); diff --git a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc index 76229e3f1f7e..1870d31c4a12 100644 --- a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc +++ b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc @@ -28,6 +28,7 @@ using testing::_; using testing::AtLeast; +using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -54,7 +55,7 @@ class DatadogDriverTest : public testing::Test { } void setupValidDriver() { - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); @@ -94,7 +95,7 @@ TEST_F(DatadogDriverTest, InitializeDriver) { { // Valid config but not valid cluster. - EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillOnce(Return(nullptr)); const std::string yaml_string = R"EOF( collector_cluster: fake_cluster @@ -106,7 +107,7 @@ TEST_F(DatadogDriverTest, InitializeDriver) { } { - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); diff --git a/test/extensions/tracers/dynamic_ot/config_test.cc b/test/extensions/tracers/dynamic_ot/config_test.cc index 00e685a736e6..825f8b0aae70 100644 --- a/test/extensions/tracers/dynamic_ot/config_test.cc +++ b/test/extensions/tracers/dynamic_ot/config_test.cc @@ -8,6 +8,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::NiceMock; using testing::Return; @@ -19,7 +20,7 @@ namespace { TEST(DynamicOtTracerConfigTest, DynamicOpentracingHttpTracer) { NiceMock server; - EXPECT_CALL(server.cluster_manager_, get("fake_cluster")) + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); ON_CALL(*server.cluster_manager_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); diff --git a/test/extensions/tracers/lightstep/config_test.cc b/test/extensions/tracers/lightstep/config_test.cc index 55345a1aaf90..c0a681183cd1 100644 --- a/test/extensions/tracers/lightstep/config_test.cc +++ b/test/extensions/tracers/lightstep/config_test.cc @@ -6,6 +6,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::NiceMock; using testing::Return; @@ -17,7 +18,7 @@ namespace { TEST(LightstepTracerConfigTest, LightstepHttpTracer) { NiceMock server; - EXPECT_CALL(server.cluster_manager_, get("fake_cluster")) + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); ON_CALL(*server.cluster_manager_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); diff --git a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc index 2f42fcb203b7..a43f9da6be4c 100644 --- a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc +++ b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc @@ -29,6 +29,7 @@ using testing::_; using testing::AtLeast; +using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -64,7 +65,7 @@ class LightStepDriverTest : public testing::Test { void setupValidDriver(Common::Ot::OpenTracingDriver::PropagationMode propagation_mode = Common::Ot::OpenTracingDriver::PropagationMode::TracerNative) { - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); @@ -114,7 +115,7 @@ TEST_F(LightStepDriverTest, InitializeDriver) { { // Valid config but not valid cluster. - EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillOnce(Return(nullptr)); const std::string yaml_string = R"EOF( collector_cluster: fake_cluster @@ -127,7 +128,7 @@ TEST_F(LightStepDriverTest, InitializeDriver) { { // Valid config, but upstream cluster does not support http2. - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()).WillByDefault(Return(0)); const std::string yaml_string = R"EOF( @@ -140,7 +141,7 @@ TEST_F(LightStepDriverTest, InitializeDriver) { } { - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc index b710dc7e609b..b62865dd2601 100644 --- a/test/extensions/tracers/zipkin/config_test.cc +++ b/test/extensions/tracers/zipkin/config_test.cc @@ -7,6 +7,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +using ::testing::Eq; + namespace Envoy { namespace Extensions { namespace Tracers { @@ -15,7 +17,7 @@ namespace { TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) { NiceMock server; - EXPECT_CALL(server.cluster_manager_, get("fake_cluster")) + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); const std::string yaml_string = R"EOF( @@ -38,7 +40,7 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) { TEST(ZipkinTracerConfigTest, ZipkinHttpTracerWithTypedConfig) { NiceMock server; - EXPECT_CALL(server.cluster_manager_, get("fake_cluster")) + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); const std::string yaml_string = R"EOF( diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 8ea8fd1e370b..5810778974f0 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -27,6 +27,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -56,7 +57,7 @@ class ZipkinDriverTest : public testing::Test { } void setupValidDriver() { - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); const std::string yaml_string = R"EOF( collector_cluster: fake_cluster @@ -104,7 +105,7 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { { // Valid config but not valid cluster. - EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillOnce(Return(nullptr)); const std::string yaml_string = R"EOF( collector_cluster: fake_cluster collector_endpoint: /api/v1/spans @@ -117,7 +118,7 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { { // valid config - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()).WillByDefault(Return(0)); const std::string yaml_string = R"EOF( diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 8d44835fa8ab..a8e7ebdfc96b 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -745,17 +745,9 @@ TEST_F(ClientContextConfigImplTest, TlsCertificatesAndSdsConfig) { // downloaded. TEST_F(ClientContextConfigImplTest, SecretNotReady) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; Stats::IsolatedStoreImpl stats; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(factory_context_, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context_, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(factory_context_, random()).WillOnce(ReturnRef(random)); EXPECT_CALL(factory_context_, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(factory_context_, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(factory_context_, initManager()).WillRepeatedly(Return(&init_manager)); auto sds_secret_configs = tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); @@ -781,17 +773,9 @@ TEST_F(ClientContextConfigImplTest, ValidationContextNotReady) { "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_cert.pem")); client_cert->mutable_private_key()->set_filename(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_key.pem")); - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; Stats::IsolatedStoreImpl stats; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(factory_context_, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context_, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(factory_context_, random()).WillOnce(ReturnRef(random)); EXPECT_CALL(factory_context_, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(factory_context_, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(factory_context_, initManager()).WillRepeatedly(Return(&init_manager)); auto sds_secret_configs = tls_context.mutable_common_tls_context()->mutable_validation_context_sds_secret_config(); @@ -1088,17 +1072,9 @@ TEST_F(ServerContextConfigImplTest, MultiSdsConfig) { TEST_F(ServerContextConfigImplTest, SecretNotReady) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; Stats::IsolatedStoreImpl stats; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(factory_context_, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context_, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(factory_context_, random()).WillOnce(ReturnRef(random)); EXPECT_CALL(factory_context_, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(factory_context_, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(factory_context_, initManager()).WillRepeatedly(Return(&init_manager)); auto sds_secret_configs = tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); @@ -1124,17 +1100,9 @@ TEST_F(ServerContextConfigImplTest, ValidationContextNotReady) { "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_cert.pem")); server_cert->mutable_private_key()->set_filename(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_key.pem")); - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; Stats::IsolatedStoreImpl stats; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(factory_context_, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context_, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(factory_context_, random()).WillOnce(ReturnRef(random)); EXPECT_CALL(factory_context_, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(factory_context_, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(factory_context_, initManager()).WillRepeatedly(Return(&init_manager)); auto sds_secret_configs = tls_context.mutable_common_tls_context()->mutable_validation_context_sds_secret_config(); diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 36e259259b7b..fb858a80af41 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -3708,16 +3708,8 @@ TEST_P(SslSocketTest, OverrideRequestedServerNameWithoutSniInUpstreamTlsContext) TEST_P(SslSocketTest, DownstreamNotReadySslSocket) { Stats::IsolatedStoreImpl stats_store; testing::NiceMock factory_context; - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); - EXPECT_CALL(factory_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(factory_context, initManager()).WillRepeatedly(Return(&init_manager)); envoy::api::v2::auth::DownstreamTlsContext tls_context; @@ -3748,16 +3740,8 @@ TEST_P(SslSocketTest, DownstreamNotReadySslSocket) { TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { Stats::IsolatedStoreImpl stats_store; testing::NiceMock factory_context; - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; - NiceMock cluster_manager; NiceMock init_manager; - EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); - EXPECT_CALL(factory_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(factory_context, initManager()).WillRepeatedly(Return(&init_manager)); envoy::api::v2::auth::UpstreamTlsContext tls_context; diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index ca639b8dfb07..ab92cbcb73ec 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -213,8 +213,9 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) { // 2019/05/07 6794 49957 Stats for excluded hosts in cluster // 2019/04/27 6733 50213 Use SymbolTable API for HTTP codes // 2019/05/31 6866 50157 libstdc++ upgrade in CI + // 2019/06/07 50240 Subscription factory - EXPECT_EQ(m_per_cluster, 50157); + EXPECT_EQ(m_per_cluster, 50240); } } // namespace diff --git a/test/mocks/config/mocks.cc b/test/mocks/config/mocks.cc index 1c6e4b7d32ff..f1e4fb6947b3 100644 --- a/test/mocks/config/mocks.cc +++ b/test/mocks/config/mocks.cc @@ -9,6 +9,22 @@ namespace Envoy { namespace Config { +MockSubscriptionFactory::MockSubscriptionFactory() { + ON_CALL(*this, subscriptionFromConfigSource(_, _, _, _)) + .WillByDefault(testing::Invoke([this](const envoy::api::v2::core::ConfigSource&, + absl::string_view, Stats::Scope&, + SubscriptionCallbacks& callbacks) -> SubscriptionPtr { + auto ret = std::make_unique>(); + subscription_ = ret.get(); + callbacks_ = &callbacks; + return ret; + })); + ON_CALL(*this, messageValidationVisitor()) + .WillByDefault(testing::ReturnRef(ProtobufMessage::getStrictValidationVisitor())); +} + +MockSubscriptionFactory::~MockSubscriptionFactory() {} + MockGrpcMuxWatch::MockGrpcMuxWatch() {} MockGrpcMuxWatch::~MockGrpcMuxWatch() { cancel(); } diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index 7c37573a764f..4143aaa490c7 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -47,6 +47,21 @@ class MockSubscription : public Subscription { MOCK_METHOD1(updateResources, void(const std::set& update_to_these_names)); }; +class MockSubscriptionFactory : public SubscriptionFactory { +public: + MockSubscriptionFactory(); + ~MockSubscriptionFactory(); + + MOCK_METHOD4(subscriptionFromConfigSource, + SubscriptionPtr(const envoy::api::v2::core::ConfigSource& config, + absl::string_view type_url, Stats::Scope& scope, + SubscriptionCallbacks& callbacks)); + MOCK_METHOD0(messageValidationVisitor, ProtobufMessage::ValidationVisitor&()); + + MockSubscription* subscription_{}; + SubscriptionCallbacks* callbacks_{}; +}; + class MockGrpcMuxWatch : public GrpcMuxWatch { public: MockGrpcMuxWatch(); diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 01789c5f1719..59f342352be2 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -198,6 +198,7 @@ MockFactoryContext::~MockFactoryContext() = default; MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() : secret_manager_(new Secret::SecretManagerImpl()) { + ON_CALL(*this, clusterManager()).WillByDefault(ReturnRef(cluster_manager_)); ON_CALL(*this, api()).WillByDefault(ReturnRef(api_)); ON_CALL(*this, messageValidationVisitor()) .WillByDefault(ReturnRef(ProtobufMessage::getStrictValidationVisitor())); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 4d9cd6e966a3..93d6573fcb89 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -501,6 +501,7 @@ class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { MOCK_METHOD0(messageValidationVisitor, ProtobufMessage::ValidationVisitor&()); MOCK_METHOD0(api, Api::Api&()); + testing::NiceMock cluster_manager_; std::unique_ptr secret_manager_; testing::NiceMock api_; }; diff --git a/test/mocks/upstream/mocks.cc b/test/mocks/upstream/mocks.cc index 35f96382ac94..05785eead37b 100644 --- a/test/mocks/upstream/mocks.cc +++ b/test/mocks/upstream/mocks.cc @@ -9,6 +9,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::Invoke; using testing::Return; using testing::ReturnPointee; @@ -145,7 +146,8 @@ MockClusterManager::MockClusterManager() { // Matches are LIFO so "" will match first. ON_CALL(*this, get(_)).WillByDefault(Return(&thread_local_cluster_)); - ON_CALL(*this, get("")).WillByDefault(Return(nullptr)); + ON_CALL(*this, get(Eq(""))).WillByDefault(Return(nullptr)); + ON_CALL(*this, subscriptionFactory()).WillByDefault(ReturnRef(subscription_factory_)); } MockClusterManager::~MockClusterManager() = default; diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h index a5f152b4d3f7..00ae7bd017b1 100644 --- a/test/mocks/upstream/mocks.h +++ b/test/mocks/upstream/mocks.h @@ -304,7 +304,7 @@ class MockClusterManager : public ClusterManager { ClusterWarmingCallback cluster_warming_cb)); MOCK_METHOD1(setInitializedCb, void(std::function)); MOCK_METHOD0(clusters, ClusterInfoMap()); - MOCK_METHOD1(get, ThreadLocalCluster*(const std::string& cluster)); + MOCK_METHOD1(get, ThreadLocalCluster*(absl::string_view cluster)); MOCK_METHOD4(httpConnPoolForCluster, Http::ConnectionPool::Instance*(const std::string& cluster, ResourcePriority priority, Http::Protocol protocol, @@ -328,6 +328,7 @@ class MockClusterManager : public ClusterManager { MOCK_METHOD1(addThreadLocalClusterUpdateCallbacks_, ClusterUpdateCallbacksHandle*(ClusterUpdateCallbacks& callbacks)); MOCK_CONST_METHOD0(warmingClusterCount, std::size_t()); + MOCK_METHOD0(subscriptionFactory, Config::SubscriptionFactory&()); NiceMock conn_pool_; NiceMock async_client_; @@ -338,6 +339,7 @@ class MockClusterManager : public ClusterManager { NiceMock async_client_manager_; std::string local_cluster_name_; NiceMock cluster_manager_factory_; + NiceMock subscription_factory_; }; class MockHealthChecker : public HealthChecker { diff --git a/test/server/BUILD b/test/server/BUILD index f3e2eee2d743..4cfa5be816ee 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -149,6 +149,7 @@ envoy_cc_test( deps = [ "//source/common/protobuf:utility_lib", "//source/server:lds_api_lib", + "//test/mocks/config:config_mocks", "//test/mocks/protobuf:protobuf_mocks", "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 99e0046aa418..efbd1c5f8f24 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -2,11 +2,11 @@ #include "envoy/api/v2/lds.pb.h" -#include "common/http/message_impl.h" #include "common/protobuf/utility.h" #include "server/lds_api.h" +#include "test/mocks/config/mocks.h" #include "test/mocks/protobuf/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" @@ -27,44 +27,20 @@ namespace { class LdsApiTest : public testing::Test { public: - LdsApiTest() - : request_(&cluster_manager_.async_client_), api_(Api::createApiForTest(store_)), - lds_version_( - store_.gauge("listener_manager.lds.version", Stats::Gauge::ImportMode::NeverImport)) { + LdsApiTest() { ON_CALL(init_manager_, add(_)).WillByDefault(Invoke([this](const Init::Target& target) { init_target_handle_ = target.createHandle("test"); })); } void setup() { - const std::string config_yaml = R"EOF( -api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: 1s - )EOF"; - envoy::api::v2::core::ConfigSource lds_config; - TestUtility::loadFromYaml(config_yaml, lds_config); - lds_config.mutable_api_config_source()->set_api_type( - envoy::api::v2::core::ApiConfigSource::REST); - Upstream::ClusterManager::ClusterInfoMap cluster_map; - Upstream::MockClusterMockPrioritySet cluster; - cluster_map.emplace("foo_cluster", cluster); - EXPECT_CALL(cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_CALL(cluster, info()); - EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_CALL(cluster, info()); - EXPECT_CALL(*cluster.info_, type()); - interval_timer_ = new Event::MockTimer(&dispatcher_); EXPECT_CALL(init_manager_, add(_)); - lds_ = std::make_unique(lds_config, cluster_manager_, dispatcher_, random_, - init_manager_, local_info_, store_, listener_manager_, - validation_visitor_, *api_); - - expectRequest(); + lds_ = std::make_unique(lds_config, cluster_manager_, init_manager_, store_, + listener_manager_, validation_visitor_); + EXPECT_CALL(*cluster_manager_.subscription_factory_.subscription_, start(_)); init_target_handle_->initialize(init_watcher_); + lds_callbacks_ = cluster_manager_.subscription_factory_.callbacks_; } void expectAdd(const std::string& listener_name, absl::optional version, @@ -86,26 +62,6 @@ class LdsApiTest : public testing::Test { } } - void expectRequest() { - EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("foo_cluster")); - EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _)) - .WillOnce( - Invoke([&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - EXPECT_EQ( - (Http::TestHeaderMapImpl{ - {":method", "POST"}, - {":path", "/v2/discovery:listeners"}, - {":authority", "foo_cluster"}, - {"content-type", "application/json"}, - {"content-length", - request->body() ? fmt::format_int(request->body()->length()).str() : "0"}}), - request->headers()); - callbacks_ = &callbacks; - return &request_; - })); - } - void makeListenersAndExpectCall(const std::vector& listener_names) { std::vector> refs; listeners_.clear(); @@ -129,21 +85,14 @@ class LdsApiTest : public testing::Test { } NiceMock cluster_manager_; - Event::MockDispatcher dispatcher_; - NiceMock random_; Init::MockManager init_manager_; Init::ExpectableWatcherImpl init_watcher_; Init::TargetHandlePtr init_target_handle_; - NiceMock local_info_; Stats::IsolatedStoreImpl store_; MockListenerManager listener_manager_; - Http::MockAsyncClientRequest request_; + Config::SubscriptionCallbacks* lds_callbacks_{}; std::unique_ptr lds_; - Event::MockTimer* interval_timer_{}; - Http::AsyncClient::Callbacks* callbacks_{}; NiceMock validation_visitor_; - Api::ApiPtr api_; - Stats::Gauge& lds_version_; private: std::list> listeners_; @@ -162,30 +111,7 @@ TEST_F(LdsApiTest, ValidateFail) { EXPECT_CALL(listener_manager_, listeners()).WillOnce(Return(existing_listeners)); EXPECT_CALL(init_watcher_, ready()); - EXPECT_THROW(lds_->onConfigUpdate(listeners, ""), EnvoyException); - EXPECT_CALL(request_, cancel()); -} - -TEST_F(LdsApiTest, UnknownCluster) { - const std::string config_yaml = R"EOF( -api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: 1s - )EOF"; - - envoy::api::v2::core::ConfigSource lds_config; - TestUtility::loadFromYaml(config_yaml, lds_config); - Upstream::ClusterManager::ClusterInfoMap cluster_map; - EXPECT_CALL(cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_THROW_WITH_MESSAGE( - LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_manager_, local_info_, - store_, listener_manager_, validation_visitor_, *api_), - EnvoyException, - "envoy::api::v2::core::ConfigSource must have a statically defined non-EDS " - "cluster: 'foo_cluster' does not exist, was added via api, or is an " - "EDS cluster"); + EXPECT_THROW(lds_callbacks_->onConfigUpdate(listeners, ""), EnvoyException); } TEST_F(LdsApiTest, MisconfiguredListenerNameIsPresentInException) { @@ -212,9 +138,8 @@ TEST_F(LdsApiTest, MisconfiguredListenerNameIsPresentInException) { listeners.Add()->PackFrom(listener); EXPECT_THROW_WITH_MESSAGE( - lds_->onConfigUpdate(listeners, ""), EnvoyException, + lds_callbacks_->onConfigUpdate(listeners, ""), EnvoyException, "Error adding/updating listener(s) invalid-listener: something is wrong"); - EXPECT_CALL(request_, cancel()); } TEST_F(LdsApiTest, EmptyListenersUpdate) { @@ -226,11 +151,9 @@ TEST_F(LdsApiTest, EmptyListenersUpdate) { std::vector> existing_listeners; EXPECT_CALL(listener_manager_, listeners()).WillOnce(Return(existing_listeners)); - EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(request_, cancel()); - lds_->onConfigUpdate(listeners, ""); + lds_callbacks_->onConfigUpdate(listeners, ""); } TEST_F(LdsApiTest, ListenerCreationContinuesEvenAfterException) { @@ -257,10 +180,9 @@ TEST_F(LdsApiTest, ListenerCreationContinuesEvenAfterException) { EXPECT_CALL(init_watcher_, ready()); - EXPECT_THROW_WITH_MESSAGE(lds_->onConfigUpdate(listeners, ""), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(lds_callbacks_->onConfigUpdate(listeners, ""), EnvoyException, "Error adding/updating listener(s) invalid-listener-1: something is " "wrong, invalid-listener-2: something else is wrong"); - EXPECT_CALL(request_, cancel()); } // Validate onConfigUpdate throws EnvoyException with duplicate listeners. @@ -280,38 +202,9 @@ TEST_F(LdsApiTest, ValidateDuplicateListeners) { EXPECT_CALL(listener_manager_, addOrUpdateListener(_, _, true)).WillOnce(Return(true)); EXPECT_CALL(init_watcher_, ready()); - EXPECT_THROW_WITH_MESSAGE(lds_->onConfigUpdate(listeners, ""), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(lds_callbacks_->onConfigUpdate(listeners, ""), EnvoyException, "Error adding/updating listener(s) duplicate_listener: duplicate " "listener duplicate_listener found"); - EXPECT_CALL(request_, cancel()); -} - -TEST_F(LdsApiTest, BadLocalInfo) { - interval_timer_ = new Event::MockTimer(&dispatcher_); - const std::string config_yaml = R"EOF( -api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: 1s - )EOF"; - - envoy::api::v2::core::ConfigSource lds_config; - TestUtility::loadFromYaml(config_yaml, lds_config); - Upstream::ClusterManager::ClusterInfoMap cluster_map; - Upstream::MockClusterMockPrioritySet cluster; - cluster_map.emplace("foo_cluster", cluster); - EXPECT_CALL(cluster_manager_, clusters()).WillOnce(Return(cluster_map)); - EXPECT_CALL(cluster, info()).Times(2); - EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_CALL(*cluster.info_, type()); - ON_CALL(local_info_, clusterName()).WillByDefault(ReturnRef(EMPTY_STRING)); - EXPECT_THROW_WITH_MESSAGE( - LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_manager_, local_info_, - store_, listener_manager_, validation_visitor_, *api_), - 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) { @@ -338,22 +231,15 @@ TEST_F(LdsApiTest, Basic) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response1_json); + auto response1 = TestUtility::parseYaml(response1_json); makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); expectAdd("listener2", "0", true); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + lds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ("0", lds_->versionInfo()); - EXPECT_EQ(7148434200721666028U, lds_version_.value()); - expectRequest(); - interval_timer_->callback_(); const std::string response2_json = R"EOF( { @@ -374,22 +260,14 @@ TEST_F(LdsApiTest, Basic) { ] } )EOF"; - - message = std::make_unique( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}); - message->body() = std::make_unique(response2_json); + auto response2 = TestUtility::parseYaml(response2_json); makeListenersAndExpectCall({"listener1", "listener2"}); EXPECT_CALL(listener_manager_, removeListener("listener2")).WillOnce(Return(true)); expectAdd("listener1", "1", false); expectAdd("listener3", "1", true); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + lds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); EXPECT_EQ("1", lds_->versionInfo()); - - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_attempt").value()); - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_success").value()); - EXPECT_EQ(13237225503670494420U, lds_version_.value()); } // Regression test against only updating versionInfo() if at least one listener @@ -412,20 +290,14 @@ TEST_F(LdsApiTest, UpdateVersionOnListenerRemove) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response1_json); + auto response1 = TestUtility::parseYaml(response1_json); makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + lds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ("0", lds_->versionInfo()); - expectRequest(); - interval_timer_->callback_(); const std::string response2_json = R"EOF( { @@ -433,22 +305,12 @@ TEST_F(LdsApiTest, UpdateVersionOnListenerRemove) { "resources": [] } )EOF"; - - message = std::make_unique( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}); - message->body() = std::make_unique(response2_json); + auto response2 = TestUtility::parseYaml(response2_json); makeListenersAndExpectCall({"listener1"}); EXPECT_CALL(listener_manager_, removeListener("listener1")).WillOnce(Return(true)); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + lds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); EXPECT_EQ("1", lds_->versionInfo()); - - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_attempt").value()); - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_success").value()); - EXPECT_EQ( - 13237225503670494420U, - store_.gauge("listener_manager.lds.version", Stats::Gauge::ImportMode::NeverImport).value()); } // Regression test issue #2188 where an empty ca_cert_file field was created and caused the LDS @@ -471,19 +333,12 @@ TEST_F(LdsApiTest, TlsConfigWithoutCaCert) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response1_json); + auto response1 = TestUtility::parseYaml(response1_json); makeListenersAndExpectCall({"listener0"}); expectAdd("listener0", {}, true); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); - - expectRequest(); - interval_timer_->callback_(); + lds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); std::string response2_basic = R"EOF( {{ @@ -511,26 +366,24 @@ TEST_F(LdsApiTest, TlsConfigWithoutCaCert) { fmt::format(response2_basic, TestEnvironment::runfilesPath("/test/config/integration/certs/servercert.pem"), TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); + auto response2 = TestUtility::parseYaml(response2_json); - message = std::make_unique( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}); - message->body() = std::make_unique(response2_json); makeListenersAndExpectCall({ "listener-8080", }); // Can't check version here because of bazel sandbox paths for the certs. expectAdd("listener-8080", {}, true); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - EXPECT_NO_THROW(callbacks_->onSuccess(std::move(message))); + EXPECT_NO_THROW(lds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info())); } -TEST_F(LdsApiTest, Failure) { +// Validate behavior when the config is delivered but it fails PGV validation. +TEST_F(LdsApiTest, FailureInvalidConfig) { InSequence s; setup(); // To test the case of valid JSON with invalid config, create a listener with no address. - const std::string response_json = R"EOF( + const std::string response1_json = R"EOF( { "version_info": "1", "resources": [ @@ -542,30 +395,25 @@ TEST_F(LdsApiTest, Failure) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response_json); + auto response1 = TestUtility::parseYaml(response1_json); std::vector> existing_listeners; EXPECT_CALL(listener_manager_, listeners()).WillOnce(Return(existing_listeners)); - EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + EXPECT_THROW(lds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), + EnvoyException); + EXPECT_EQ("", lds_->versionInfo()); +} - expectRequest(); - interval_timer_->callback_(); +// Validate behavior when the config fails delivery at the subscription level. +TEST_F(LdsApiTest, FailureSubscription) { + InSequence s; - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onFailure(Http::AsyncClient::FailureReason::Reset); - EXPECT_EQ("", lds_->versionInfo()); + setup(); - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_attempt").value()); - EXPECT_EQ(1UL, store_.counter("listener_manager.lds.update_failure").value()); - // Validate that the schema error increments update_rejected stat. - EXPECT_EQ(1UL, store_.counter("listener_manager.lds.update_failure").value()); - EXPECT_EQ(0UL, lds_version_.value()); + EXPECT_CALL(init_watcher_, ready()); + lds_callbacks_->onConfigUpdateFailed({}); + EXPECT_EQ("", lds_->versionInfo()); } TEST_F(LdsApiTest, ReplacingListenerWithSameAddress) { @@ -592,22 +440,15 @@ TEST_F(LdsApiTest, ReplacingListenerWithSameAddress) { ] } )EOF"; - - Http::MessagePtr message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - message->body() = std::make_unique(response1_json); + auto response1 = TestUtility::parseYaml(response1_json); makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); expectAdd("listener2", "0", true); EXPECT_CALL(init_watcher_, ready()); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); + lds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()); EXPECT_EQ("0", lds_->versionInfo()); - EXPECT_EQ(7148434200721666028U, lds_version_.value()); - expectRequest(); - interval_timer_->callback_(); const std::string response2_json = R"EOF( { @@ -628,22 +469,13 @@ TEST_F(LdsApiTest, ReplacingListenerWithSameAddress) { ] } )EOF"; - - message = std::make_unique( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}); - message->body() = std::make_unique(response2_json); + auto response2 = TestUtility::parseYaml(response2_json); makeListenersAndExpectCall({"listener1", "listener2"}); EXPECT_CALL(listener_manager_, removeListener("listener2")).WillOnce(Return(true)); expectAdd("listener1", "1", false); expectAdd("listener3", "1", true); - EXPECT_CALL(*interval_timer_, enableTimer(_)); - callbacks_->onSuccess(std::move(message)); - - EXPECT_EQ("1", lds_->versionInfo()); - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_attempt").value()); - EXPECT_EQ(2UL, store_.counter("listener_manager.lds.update_success").value()); - EXPECT_EQ(13237225503670494420U, lds_version_.value()); + lds_callbacks_->onConfigUpdate(response2.resources(), response2.version_info()); } } // namespace