diff --git a/api/xds_protocol.rst b/api/xds_protocol.rst index 4d43306cdd1d0..98211478e23ea 100644 --- a/api/xds_protocol.rst +++ b/api/xds_protocol.rst @@ -367,6 +367,8 @@ An example minimal ``bootstrap.yaml`` fragment for ADS configuration is: admin: ... +.. _xds_protocol_delta: + Incremental xDS ~~~~~~~~~~~~~~~ diff --git a/docs/root/configuration/overview/v2_overview.rst b/docs/root/configuration/overview/v2_overview.rst index 3dcefb0067d5d..b6b84fa95d444 100644 --- a/docs/root/configuration/overview/v2_overview.rst +++ b/docs/root/configuration/overview/v2_overview.rst @@ -570,6 +570,32 @@ to with the effect that the LDS stream will be directed to *some_ads_cluster* over the shared ADS channel. +.. _config_overview_v2_delta: + +Delta endpoints +--------------- + +The REST, filesystem, and original gRPC xDS implementations all deliver "state of the world" updates: +every CDS update must contain every cluster, with the absence of a cluster from an update implying +that the cluster is gone. For Envoy deployments with huge amounts of resources and even a trickle of +churn, these state-of-the-world updates can be cumbersome. + +As of 1.12.0, Envoy supports a "delta" variant of xDS (including ADS), where updates only contain +resources added/changed/removed. Delta xDS is a gRPC (only) protocol. Delta uses different +request/response protos than SotW (DeltaDiscovery{Request,Response}); see +:repo:`discovery.proto `. Conceptually, delta should be viewed as +a new xDS transport type: there is static, filesystem, REST, gRPC-SotW, and now gRPC-delta. +(Envoy's implementation of the gRPC-SotW/delta client happens to share most of its code between the +two, and something similar is likely possible on the server side. However, they are in fact +incompatible protocols. +:ref:`The specification of the delta xDS protocol's behavior is here `.) + +To use delta, simply set the api_type field of your +:ref:`ApiConfigSource ` proto(s) to DELTA_GRPC. +That works for both xDS and ADS; for ADS, it's the api_type field of +:ref:`DynamicResources.ads_config `, +as described in the previous section. + .. _config_overview_v2_mgmt_con_issues: Management Server Unreachability diff --git a/docs/root/intro/arch_overview/operations/dynamic_configuration.rst b/docs/root/intro/arch_overview/operations/dynamic_configuration.rst index bb51c6ebdf56f..99bb323ed7c2f 100644 --- a/docs/root/intro/arch_overview/operations/dynamic_configuration.rst +++ b/docs/root/intro/arch_overview/operations/dynamic_configuration.rst @@ -88,3 +88,15 @@ The :ref:`secret discovery service (SDS) ` laye by which Envoy can discover cryptographic secrets (certificate plus private key, TLS session ticket keys) for its listeners, as well as configuration of peer certificate validation logic (trusted root certs, revocations, etc). + +Aggregated xDS ("ADS") +----------------------------- + +EDS, CDS, etc. are each separate services, with different REST/gRPC service names, e.g. StreamListeners, StreamSecrets. For users looking to enforce the order in which resources of different types reach Envoy, there is aggregated xDS, a single gRPC service that carries all resource types in a single gRPC stream. (ADS is only supported by gRPC). :ref:`More details about ADS `. + +.. _arch_overview_dynamic_config_delta: + +Delta gRPC xDS +----------------------------- + +Standard xDS is "state-of-the-world": every CDS update must contain every cluster, with the absence of a cluster from an update implying that the cluster is gone. As of 1.12.0, Envoy supports a "delta" variant of xDS (including ADS), where updates only contain resources added/changed/removed. Delta xDS is a new protocol, with request/response protos different from SotW; existing control plane servers will need support added. :ref:`More details about delta `. diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 3a7afd519c070..8c9a157748c5f 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -11,6 +11,7 @@ Version history * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump `. * api: added ::ref:`set_node_on_first_message_only ` option to omit the node identifier from the subsequent discovery requests on the same stream. * buffer filter: the buffer filter populates content-length header if not present, behavior can be disabled using the runtime feature `envoy.reloadable_features.buffer_filter_populate_content_length`. +* config: added support for :ref:`delta xDS ` (including ADS) delivery * config: enforcing that terminal filters (e.g. HttpConnectionManager for L4, router for L7) be the last in their respective filter chains. * config: added access log :ref:`extension filter`. * config: added support for :option:`--reject-unknown-dynamic-fields`, providing independent control diff --git a/include/envoy/config/BUILD b/include/envoy/config/BUILD index 001d3cbe46af5..7df8a214104fc 100644 --- a/include/envoy/config/BUILD +++ b/include/envoy/config/BUILD @@ -65,20 +65,3 @@ envoy_cc_library( "//source/common/protobuf", ], ) - -envoy_cc_library( - name = "watch_map_interface", - hdrs = ["watch_map.h"], - deps = [ - ":subscription_interface", - ], -) - -envoy_cc_library( - name = "xds_grpc_context_interface", - hdrs = ["xds_grpc_context.h"], - deps = [ - ":subscription_interface", - "//source/common/protobuf", - ], -) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 1d9920e2d5e3e..1742ea60e0601 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -67,6 +67,8 @@ class GrpcMuxWatch { using GrpcMuxWatchPtr = std::unique_ptr; +struct Watch; + /** * Manage one or more gRPC subscriptions on a single stream to management server. This can be used * for a single xDS API, e.g. EDS, or to combined multiple xDS APIs for ADS. @@ -111,6 +113,13 @@ class GrpcMux { */ virtual void resume(const std::string& type_url) PURE; + // For delta + virtual Watch* addOrUpdateWatch(const std::string& type_url, Watch* watch, + const std::set& resources, + SubscriptionCallbacks& callbacks, + std::chrono::milliseconds init_fetch_timeout) PURE; + virtual void removeWatch(const std::string& type_url, Watch* watch) PURE; + /** * Retrieves the current pause state as set by pause()/resume(). * @param type_url type URL corresponding to xDS API, e.g. @@ -121,6 +130,37 @@ class GrpcMux { }; using GrpcMuxPtr = std::unique_ptr; +using GrpcMuxSharedPtr = std::shared_ptr; + +/** + * A grouping of callbacks that a GrpcMux should provide to its GrpcStream. + */ +template class GrpcStreamCallbacks { +public: + virtual ~GrpcStreamCallbacks() = default; + + /** + * For the GrpcStream to prompt the context to take appropriate action in response to the + * gRPC stream having been successfully established. + */ + virtual void onStreamEstablished() PURE; + + /** + * For the GrpcStream to prompt the context to take appropriate action in response to + * failure to establish the gRPC stream. + */ + virtual void onEstablishmentFailure() PURE; + + /** + * For the GrpcStream to pass received protos to the context. + */ + virtual void onDiscoveryResponse(std::unique_ptr&& message) PURE; + + /** + * For the GrpcStream to call when its rate limiting logic allows more requests to be sent. + */ + virtual void onWriteable() PURE; +}; } // namespace Config } // namespace Envoy diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index ee21da758d107..009c4a265dc6c 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -89,7 +89,7 @@ class Subscription { * @param resources vector of resource names to fetch. It's a (not unordered_)set so that it can * be passed to std::set_difference, which must be given sorted collections. */ - virtual void updateResources(const std::set& update_to_these_names) PURE; + virtual void updateResourceInterest(const std::set& update_to_these_names) PURE; }; using SubscriptionPtr = std::unique_ptr; diff --git a/include/envoy/config/subscription_factory.h b/include/envoy/config/subscription_factory.h index b1733afda6c46..5268ac7f2abe2 100644 --- a/include/envoy/config/subscription_factory.h +++ b/include/envoy/config/subscription_factory.h @@ -24,7 +24,7 @@ class SubscriptionFactory { virtual SubscriptionPtr subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, - SubscriptionCallbacks& callbacks) PURE; + SubscriptionCallbacks& callbacks, bool is_delta) PURE; }; } // namespace Config diff --git a/include/envoy/config/xds_grpc_context.h b/include/envoy/config/xds_grpc_context.h deleted file mode 100644 index 312d3e0650f74..0000000000000 --- a/include/envoy/config/xds_grpc_context.h +++ /dev/null @@ -1,42 +0,0 @@ -#pragma once - -#include "envoy/common/pure.h" -#include "envoy/config/subscription.h" - -#include "common/protobuf/protobuf.h" - -namespace Envoy { -namespace Config { - -/** - * A grouping of callbacks that an XdsGrpcContext should provide to its GrpcStream. - */ -template class GrpcStreamCallbacks { -public: - virtual ~GrpcStreamCallbacks() = default; - - /** - * For the GrpcStream to prompt the context to take appropriate action in response to the - * gRPC stream having been successfully established. - */ - virtual void onStreamEstablished() PURE; - - /** - * For the GrpcStream to prompt the context to take appropriate action in response to - * failure to establish the gRPC stream. - */ - virtual void onEstablishmentFailure() PURE; - - /** - * For the GrpcStream to pass received protos to the context. - */ - virtual void onDiscoveryResponse(std::unique_ptr&& message) PURE; - - /** - * For the GrpcStream to call when its rate limiting logic allows more requests to be sent. - */ - virtual void onWriteable() PURE; -}; - -} // namespace Config -} // namespace Envoy diff --git a/include/envoy/router/route_config_provider_manager.h b/include/envoy/router/route_config_provider_manager.h index 9912aa4603564..accb99c85f240 100644 --- a/include/envoy/router/route_config_provider_manager.h +++ b/include/envoy/router/route_config_provider_manager.h @@ -39,7 +39,7 @@ class RouteConfigProviderManager { virtual RouteConfigProviderPtr createRdsRouteConfigProvider( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - Init::Manager& init_manager) PURE; + Init::Manager& init_manager, bool is_delta) PURE; /** * Get a RouteConfigSharedPtr for a statically defined route. Ownership is as described for diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index ce2a015decf32..54d4b4a9bbcb1 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -39,7 +39,8 @@ class ListenerComponentFactory { * @return an LDS API provider. * @param lds_config supplies the management server configuration. */ - virtual LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) PURE; + virtual LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config, + bool is_delta) PURE; /** * Creates a socket. @@ -128,7 +129,8 @@ class ListenerManager { * pieces of the server existing. * @param lds_config supplies the management server configuration. */ - virtual void createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) PURE; + virtual void createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config, + bool is_delta) PURE; /** * @return std::vector> a list of the currently diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index 7dfde10cc03cc..ba56548672cf1 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -180,14 +180,14 @@ class ClusterManager { virtual const envoy::api::v2::core::BindConfig& bindConfig() const PURE; /** - * Return a reference to the singleton ADS provider for upstream control plane muxing of xDS. This - * is treated somewhat as a special case in ClusterManager, since it does not relate logically to - * the management of clusters but instead is required early in ClusterManager/server + * Returns a shared_ptr to the singleton xDS-over-gRPC provider for upstream control plane muxing + * of xDS. This is treated somewhat as a special case in ClusterManager, since it does not relate + * logically to the management of clusters but instead is required early in ClusterManager/server * initialization and in various sites that need ClusterManager for xDS API interfacing. * * @return GrpcMux& ADS API provider referencee. */ - virtual Config::GrpcMux& adsMux() PURE; + virtual Config::GrpcMuxSharedPtr adsMux() PURE; /** * @return Grpc::AsyncClientManager& the cluster manager's gRPC client manager. @@ -225,6 +225,8 @@ class ClusterManager { virtual Config::SubscriptionFactory& subscriptionFactory() PURE; virtual std::size_t warmingClusterCount() const PURE; + + virtual bool xdsIsDelta() const PURE; }; using ClusterManagerPtr = std::unique_ptr; @@ -297,7 +299,7 @@ class ClusterManagerFactory { /** * Create a CDS API provider from configuration proto. */ - virtual CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, + virtual CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, ClusterManager& cm) PURE; /** diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 5d346543180c0..0f18f5524d006 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -71,8 +71,8 @@ envoy_cc_library( srcs = ["delta_subscription_impl.cc"], hdrs = ["delta_subscription_impl.h"], deps = [ - ":delta_subscription_state_lib", ":grpc_stream_lib", + ":new_grpc_mux_lib", ":utility_lib", "//include/envoy/config:subscription_interface", "//include/envoy/grpc:async_client_interface", @@ -90,6 +90,7 @@ envoy_cc_library( srcs = ["delta_subscription_state.cc"], hdrs = ["delta_subscription_state.h"], deps = [ + ":pausable_ack_queue_lib", "//include/envoy/config:subscription_interface", "//include/envoy/event:dispatcher_interface", "//source/common/common:assert_lib", @@ -125,7 +126,6 @@ envoy_cc_library( ":utility_lib", "//include/envoy/config:grpc_mux_interface", "//include/envoy/config:subscription_interface", - "//include/envoy/config:xds_grpc_context_interface", "//include/envoy/grpc:async_client_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:backoff_lib", @@ -217,6 +217,20 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "new_grpc_mux_lib", + srcs = ["new_grpc_mux_impl.cc"], + hdrs = ["new_grpc_mux_impl.h"], + deps = [ + ":delta_subscription_state_lib", + ":grpc_stream_lib", + ":pausable_ack_queue_lib", + ":watch_map_lib", + "//include/envoy/event:dispatcher_interface", + "//include/envoy/grpc:async_client_interface", + ], +) + envoy_cc_library( name = "http_subscription_lib", srcs = ["http_subscription_impl.cc"], @@ -261,6 +275,16 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "pausable_ack_queue_lib", + srcs = ["pausable_ack_queue.cc"], + hdrs = ["pausable_ack_queue.h"], + deps = [ + "//source/common/common:assert_lib", + "@envoy_api//envoy/api/v2:discovery_cc", + ], +) + envoy_cc_library( name = "protobuf_link_hacks", hdrs = ["protobuf_link_hacks.h"], @@ -300,12 +324,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "resources_lib", - hdrs = ["resources.h"], - deps = ["//source/common/singleton:const_singleton"], -) - envoy_cc_library( name = "remote_data_fetcher_lib", srcs = ["remote_data_fetcher.cc"], @@ -320,6 +338,12 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "resources_lib", + hdrs = ["resources.h"], + deps = ["//source/common/singleton:const_singleton"], +) + envoy_cc_library( name = "runtime_utility_lib", srcs = ["runtime_utility.cc"], diff --git a/source/common/config/README.md b/source/common/config/README.md new file mode 100644 index 0000000000000..7774c91befbff --- /dev/null +++ b/source/common/config/README.md @@ -0,0 +1,47 @@ +# xDS + +xDS stands for [fill in the blank] Discovery Service. It provides dynamic config discovery/updates. + +tldr: xDS can use the filesystem, REST, or gRPC. gRPC xDS comes in four flavors. +However, Envoy code uses all of that via the same Subscription interface. +If you are an Envoy developer with your hands on a valid Subscription object, +you can mostly forget the filesystem/REST/gRPC distinction, and you can +especially forget about the gRPC flavors. All of that is specified in the +bootstrap config, which is read and put into action by ClusterManagerImpl. + +Note that there can be multiple active gRPC subscriptions for a single resource +type. This concept is called "resource watches". If one EDS subscription +subscribes to X and Y, and another subscribes to Y and Z, the underlying +subscription logic will maintain a subscription to the union: X Y and Z. Updates +to X will be delivered to the first object, Y to both, Z to the second. This +logic is implemented by WatchMap. + +### If you are working on Envoy's gRPC xDS client logic itself, read on. + +When using gRPC, xDS has two pairs of options: aggregated/non-aggregated, and +delta/state-of-the-world updates. All four combinations of these are usable. + +"Aggregated" means that EDS, CDS, etc resources are all carried by the same gRPC stream. +For Envoy's implementation of xDS client logic, there is effectively no difference +between aggregated xDS and non-aggregated: they both use the same request/response protos. The +non-aggregated case is handled by running the aggregated logic, and just happening to only have 1 +xDS subscription type to "aggregate", i.e., NewGrpcMuxImpl only has one +DeltaSubscriptionState entry in its map. + +However, to the config server, there is a huge difference: when using ADS (caused +by the user providing an ads_config in the bootstrap config), the gRPC client sets +its method string to {Delta,Stream}AggregatedResources, as opposed to {Delta,Stream}Clusters, +{Delta,Stream}Routes, etc. So, despite using the same request/response protos, +and having identical client code, they're actually different gRPC services. + +Delta vs state-of-the-world is a question of wire format: the protos in question are named +[Delta]Discovery{Request,Response}. That is what the GrpcMux interface is useful for: its +NewGrpcMuxImpl (TODO may be renamed) implementation works with DeltaDiscovery{Request,Response} and has +delta-specific logic; its GrpxMuxImpl implementation (TODO will be merged into NewGrpcMuxImpl) works with Discovery{Request,Response} +and has SotW-specific logic. Both the delta and SotW Subscription implementations (TODO will be merged) hold a shared_ptr. +The shared_ptr allows for both non- and aggregated: if non-aggregated, you'll be the only holder of that shared_ptr. + +![xDS_code_diagram_june2019](xDS_code_diagram_june2019.png) + +Note that the orange flow does not necessarily have to happen in response to the blue flow; there can be spontaneous updates. ACKs are not shown in this diagram; they are also carred by the [Delta]DiscoveryRequest protos. +What does GrpcXdsContext even do in this diagram? Just own things and pass through function calls? Answer: it sequences the requests and ACKs that the various type_urls send. diff --git a/source/common/config/delta_subscription_impl.cc b/source/common/config/delta_subscription_impl.cc index 8bc69f04f4eb3..ed34dac39bea0 100644 --- a/source/common/config/delta_subscription_impl.cc +++ b/source/common/config/delta_subscription_impl.cc @@ -1,106 +1,94 @@ #include "common/config/delta_subscription_impl.h" -#include "common/common/assert.h" -#include "common/common/backoff_strategy.h" -#include "common/common/token_bucket_impl.h" -#include "common/config/utility.h" -#include "common/protobuf/protobuf.h" -#include "common/protobuf/utility.h" - namespace Envoy { namespace Config { -DeltaSubscriptionImpl::DeltaSubscriptionImpl( - const LocalInfo::LocalInfo& local_info, Grpc::RawAsyncClientPtr async_client, - Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, - absl::string_view type_url, Runtime::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, SubscriptionCallbacks& callbacks, - SubscriptionStats stats, std::chrono::milliseconds init_fetch_timeout) - : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, - rate_limit_settings), - type_url_(type_url), local_info_(local_info), callbacks_(callbacks), stats_(stats), - dispatcher_(dispatcher), init_fetch_timeout_(init_fetch_timeout) {} +DeltaSubscriptionImpl::DeltaSubscriptionImpl(GrpcMuxSharedPtr context, absl::string_view type_url, + SubscriptionCallbacks& callbacks, + SubscriptionStats stats, + std::chrono::milliseconds init_fetch_timeout, + bool is_aggregated) + : context_(std::move(context)), type_url_(type_url), callbacks_(callbacks), stats_(stats), + init_fetch_timeout_(init_fetch_timeout), is_aggregated_(is_aggregated) {} -void DeltaSubscriptionImpl::pause() { state_->pause(); } -void DeltaSubscriptionImpl::resume() { - state_->resume(); - trySendDiscoveryRequests(); +DeltaSubscriptionImpl::~DeltaSubscriptionImpl() { + if (watch_) { + context_->removeWatch(type_url_, watch_); + } } -// Config::Subscription -void DeltaSubscriptionImpl::start(const std::set& resource_names) { - state_ = std::make_unique( - type_url_, resource_names, callbacks_, local_info_, init_fetch_timeout_, dispatcher_, stats_); - grpc_stream_.establishNewStream(); - updateResources(resource_names); -} +void DeltaSubscriptionImpl::pause() { context_->pause(type_url_); } -void DeltaSubscriptionImpl::updateResources(const std::set& update_to_these_names) { - state_->updateResourceInterest(update_to_these_names); - // Tell the server about our new interests, if there are any. - trySendDiscoveryRequests(); -} +void DeltaSubscriptionImpl::resume() { context_->resume(type_url_); } -// Config::GrpcStreamCallbacks -void DeltaSubscriptionImpl::onStreamEstablished() { - state_->markStreamFresh(); - trySendDiscoveryRequests(); +// Config::DeltaSubscription +void DeltaSubscriptionImpl::start(const std::set& resources) { + // ADS initial request batching relies on the users of the GrpcMux *not* calling start on it, + // whereas non-ADS xDS users must call it themselves. + if (!is_aggregated_) { + context_->start(); + } + watch_ = context_->addOrUpdateWatch(type_url_, watch_, resources, *this, init_fetch_timeout_); + stats_.update_attempt_.inc(); } -void DeltaSubscriptionImpl::onEstablishmentFailure() { state_->handleEstablishmentFailure(); } - -void DeltaSubscriptionImpl::onDiscoveryResponse( - std::unique_ptr&& message) { - ENVOY_LOG(debug, "Received gRPC message for {} at version {}", type_url_, - message->system_version_info()); - kickOffAck(state_->handleResponse(*message)); +void DeltaSubscriptionImpl::updateResourceInterest( + const std::set& update_to_these_names) { + watch_ = context_->addOrUpdateWatch(type_url_, watch_, update_to_these_names, *this, + init_fetch_timeout_); + stats_.update_attempt_.inc(); } -void DeltaSubscriptionImpl::onWriteable() { trySendDiscoveryRequests(); } - -void DeltaSubscriptionImpl::kickOffAck(UpdateAck ack) { - ack_queue_.push(ack); - trySendDiscoveryRequests(); +// Config::SubscriptionCallbacks +void DeltaSubscriptionImpl::onConfigUpdate( + const Protobuf::RepeatedPtrField& resources, + const std::string& version_info) { + stats_.update_attempt_.inc(); + callbacks_.onConfigUpdate(resources, version_info); + stats_.update_success_.inc(); + stats_.version_.set(HashUtil::xxHash64(version_info)); } -// Checks whether external conditions allow sending a DeltaDiscoveryRequest. (Does not check -// whether we *want* to send a DeltaDiscoveryRequest). -bool DeltaSubscriptionImpl::canSendDiscoveryRequest() { - if (state_->paused()) { - ENVOY_LOG(trace, "API {} paused; discovery request on hold for now.", type_url_); - return false; - } else if (!grpc_stream_.grpcStreamAvailable()) { - ENVOY_LOG(trace, "No stream available to send a DiscoveryRequest for {}.", type_url_); - return false; - } else if (!grpc_stream_.checkRateLimitAllowsDrain()) { - ENVOY_LOG(trace, "{} DiscoveryRequest hit rate limit; will try later.", type_url_); - return false; - } - return true; +void DeltaSubscriptionImpl::onConfigUpdate( + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& system_version_info) { + stats_.update_attempt_.inc(); + callbacks_.onConfigUpdate(added_resources, removed_resources, system_version_info); + stats_.update_success_.inc(); + stats_.version_.set(HashUtil::xxHash64(system_version_info)); } -// Checks whether we have something to say in a DeltaDiscoveryRequest, which can be an ACK and/or -// a subscription update. (Does not check whether we *can* send a DeltaDiscoveryRequest). -bool DeltaSubscriptionImpl::wantToSendDiscoveryRequest() { - return !ack_queue_.empty() || state_->subscriptionUpdatePending(); +void DeltaSubscriptionImpl::onConfigUpdateFailed(ConfigUpdateFailureReason reason, + const EnvoyException* e) { + switch (reason) { + case Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure: + // This is a gRPC-stream-level establishment failure, not an xDS-protocol-level failure. + // So, don't onConfigUpdateFailed() here. Instead, allow a retry of the gRPC stream. + // If init_fetch_timeout_ is non-zero, the server will continue startup after that timeout. + stats_.update_failure_.inc(); + // TODO(fredlas) remove; it only makes sense to count start() and updateResourceInterest() + // as attempts. + stats_.update_attempt_.inc(); + break; + case Envoy::Config::ConfigUpdateFailureReason::FetchTimedout: + stats_.init_fetch_timeout_.inc(); + // TODO(fredlas) remove; it only makes sense to count start() and updateResourceInterest() + // as attempts. + stats_.update_attempt_.inc(); + callbacks_.onConfigUpdateFailed(reason, e); + break; + case Envoy::Config::ConfigUpdateFailureReason::UpdateRejected: + // We expect Envoy exception to be thrown when update is rejected. + ASSERT(e != nullptr); + stats_.update_rejected_.inc(); + callbacks_.onConfigUpdateFailed(reason, e); + break; + } } -void DeltaSubscriptionImpl::trySendDiscoveryRequests() { - while (wantToSendDiscoveryRequest() && canSendDiscoveryRequest()) { - envoy::api::v2::DeltaDiscoveryRequest request = state_->getNextRequest(); - if (!ack_queue_.empty()) { - const UpdateAck& ack = ack_queue_.front(); - request.set_response_nonce(ack.nonce_); - if (ack.error_detail_.code() != Grpc::Status::GrpcStatus::Ok) { - // Don't needlessly make the field present-but-empty if status is ok. - request.mutable_error_detail()->CopyFrom(ack.error_detail_); - } - ack_queue_.pop(); - } - ENVOY_LOG(trace, "Sending DiscoveryRequest for {}: {}", type_url_, request.DebugString()); - grpc_stream_.sendMessage(request); - } - grpc_stream_.maybeUpdateQueueSizeStat(ack_queue_.size()); +std::string DeltaSubscriptionImpl::resourceName(const ProtobufWkt::Any& resource) { + return callbacks_.resourceName(resource); } } // namespace Config diff --git a/source/common/config/delta_subscription_impl.h b/source/common/config/delta_subscription_impl.h index dbee51c688c32..b45be7c4da85f 100644 --- a/source/common/config/delta_subscription_impl.h +++ b/source/common/config/delta_subscription_impl.h @@ -1,93 +1,70 @@ #pragma once -#include - -#include "envoy/api/v2/discovery.pb.h" -#include "envoy/common/token_bucket.h" #include "envoy/config/subscription.h" -#include "envoy/config/xds_grpc_context.h" -#include "envoy/local_info/local_info.h" -#include "common/common/logger.h" -#include "common/config/delta_subscription_state.h" -#include "common/config/grpc_stream.h" -#include "common/grpc/common.h" +#include "common/config/new_grpc_mux_impl.h" +#include "common/config/utility.h" namespace Envoy { namespace Config { -/** - * Manages the logic of a (non-aggregated) delta xDS subscription. - * TODO(fredlas) add aggregation support. The plan is for that to happen in XdsGrpcContext, - * which this class will then "have a" rather than "be a". - * TODO(kyessenov) implement skip_subsequent_node for delta xDS subscription. - */ -class DeltaSubscriptionImpl : public Subscription, - public GrpcStreamCallbacks, - public Logger::Loggable { +// DeltaSubscriptionImpl provides a top-level interface to the Envoy's gRPC communication with +// an xDS server, for use by the various xDS users within Envoy. It is built around a (shared) +// NewGrpcMuxImpl, and the further machinery underlying that. An xDS user indicates interest in +// various resources via start() and updateResourceInterest(). It receives updates to those +// resources via the SubscriptionCallbacks it provides. Multiple users can each have their own +// Subscription object for the same type_url; NewGrpcMuxImpl maintains a subscription to the +// union of interested resources, and delivers to the users just the resource updates that they +// are "watching" for. +// +// DeltaSubscriptionImpl and NewGrpcMuxImpl are both built to provide both regular xDS and ADS, +// distinguished by whether multiple DeltaSubscriptionImpls are sharing a single +// NewGrpcMuxImpl. (And by the gRPC method string, but that's taken care of over in +// SubscriptionFactory). +// +// Why does DeltaSubscriptionImpl itself implement the SubscriptionCallbacks interface? So that it +// can write to SubscriptionStats (which needs to live out here in the DeltaSubscriptionImpl) upon a +// config update. The idea is, DeltaSubscriptionImpl presents itself to WatchMap as the +// SubscriptionCallbacks, and then passes (after incrementing stats) all callbacks through to +// callbacks_, which are the real SubscriptionCallbacks. +class DeltaSubscriptionImpl : public Subscription, public SubscriptionCallbacks { public: - DeltaSubscriptionImpl(const LocalInfo::LocalInfo& local_info, - Grpc::RawAsyncClientPtr async_client, Event::Dispatcher& dispatcher, - const Protobuf::MethodDescriptor& service_method, - absl::string_view type_url, Runtime::RandomGenerator& random, - Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, + // is_aggregated: whether the underlying mux/context is providing ADS to us and others, or whether + // it's all ours. The practical difference is that we ourselves must call start() on it only in + // the latter case. + DeltaSubscriptionImpl(GrpcMuxSharedPtr context, absl::string_view type_url, SubscriptionCallbacks& callbacks, SubscriptionStats stats, - std::chrono::milliseconds init_fetch_timeout); + std::chrono::milliseconds init_fetch_timeout, bool is_aggregated); + ~DeltaSubscriptionImpl() override; void pause(); void resume(); // Config::Subscription void start(const std::set& resource_names) override; - void updateResources(const std::set& update_to_these_names) override; + void updateResourceInterest(const std::set& update_to_these_names) override; - // Config::GrpcStreamCallbacks - void onStreamEstablished() override; - void onEstablishmentFailure() override; - void - onDiscoveryResponse(std::unique_ptr&& message) override; + // Config::SubscriptionCallbacks (all pass through to callbacks_!) + void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, + const std::string& version_info) override; + void onConfigUpdate(const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& system_version_info) override; + void onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) override; + std::string resourceName(const ProtobufWkt::Any& resource) override; - void onWriteable() override; + GrpcMuxSharedPtr getContextForTest() { return context_; } private: - void kickOffAck(UpdateAck ack); - - // Checks whether external conditions allow sending a DeltaDiscoveryRequest. (Does not check - // whether we *want* to send a DeltaDiscoveryRequest). - bool canSendDiscoveryRequest(); - - // Checks whether we have something to say in a DeltaDiscoveryRequest, which can be an ACK and/or - // a subscription update. (Does not check whether we *can* send a DeltaDiscoveryRequest). - bool wantToSendDiscoveryRequest(); - - void trySendDiscoveryRequests(); - - GrpcStream - grpc_stream_; - + GrpcMuxSharedPtr context_; const std::string type_url_; - - // An item in the queue represents a DeltaDiscoveryRequest that must be sent. If an item is not - // empty, it is the ACK (nonce + error_detail) to set on that request. An empty entry should - // still send a request; it just won't have an ACK. - // - // More details: DeltaDiscoveryRequest plays two independent roles: - // 1) informing the server of what resources we're interested in, and - // 2) acknowledging resources the server has sent us. - // Each entry in this queue was added for exactly one of those purposes, but since the - // subscription interest is tracked separately, in a non-queue way, subscription changes can get - // mixed in with an ACK request. In that case, the entry that the subscription change originally - // queued up *does* still get sent, just empty and pointless. (TODO(fredlas) we would like to skip - // those no-op requests). - std::queue ack_queue_; - - const LocalInfo::LocalInfo& local_info_; SubscriptionCallbacks& callbacks_; SubscriptionStats stats_; - Event::Dispatcher& dispatcher_; + // NOTE: if another subscription of the same type_url has already been started, this value will be + // ignored in favor of the other subscription's. std::chrono::milliseconds init_fetch_timeout_; - - std::unique_ptr state_; + Watch* watch_{}; + const bool is_aggregated_; }; } // namespace Config diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 5c5a795b7af06..93ddc18164dae 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -1,78 +1,44 @@ #include "common/config/delta_subscription_state.h" #include "common/common/assert.h" +#include "common/common/hash.h" namespace Envoy { namespace Config { -DeltaSubscriptionState::DeltaSubscriptionState(const std::string& type_url, - const std::set& resource_names, +DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, SubscriptionCallbacks& callbacks, const LocalInfo::LocalInfo& local_info, std::chrono::milliseconds init_fetch_timeout, - Event::Dispatcher& dispatcher, - SubscriptionStats& stats) - : type_url_(type_url), callbacks_(callbacks), local_info_(local_info), - init_fetch_timeout_(init_fetch_timeout), stats_(stats) { - // In normal usage of updateResourceInterest(), the caller is supposed to cause a discovery - // request to be queued if it returns true. We don't need to do that because we know that the - // subscription gRPC stream is not yet established, and establishment causes a request. - updateResourceInterest(resource_names); - setInitFetchTimeout(dispatcher); -} - -void DeltaSubscriptionState::setInitFetchTimeout(Event::Dispatcher& dispatcher) { + Event::Dispatcher& dispatcher) + : type_url_(std::move(type_url)), callbacks_(callbacks), local_info_(local_info), + init_fetch_timeout_(init_fetch_timeout) { if (init_fetch_timeout_.count() > 0 && !init_fetch_timeout_timer_) { init_fetch_timeout_timer_ = dispatcher.createTimer([this]() -> void { - stats_.init_fetch_timeout_.inc(); ENVOY_LOG(warn, "delta config: initial fetch timed out for {}", type_url_); - callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, - nullptr); + callbacks_.onConfigUpdateFailed(ConfigUpdateFailureReason::FetchTimedout, nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } } -void DeltaSubscriptionState::pause() { - ENVOY_LOG(debug, "Pausing discovery requests for {}", type_url_); - ASSERT(!paused_); - paused_ = true; -} - -void DeltaSubscriptionState::resume() { - ENVOY_LOG(debug, "Resuming discovery requests for {}", type_url_); - ASSERT(paused_); - paused_ = false; -} - -// Returns true if there is any meaningful change in our subscription interest, worth reporting to -// the server. -void DeltaSubscriptionState::updateResourceInterest( - const std::set& update_to_these_names) { - std::vector cur_added; - std::vector cur_removed; - - std::set_difference(update_to_these_names.begin(), update_to_these_names.end(), - resource_names_.begin(), resource_names_.end(), - std::inserter(cur_added, cur_added.begin())); - std::set_difference(resource_names_.begin(), resource_names_.end(), update_to_these_names.begin(), - update_to_these_names.end(), std::inserter(cur_removed, cur_removed.begin())); - +void DeltaSubscriptionState::updateSubscriptionInterest(const std::set& cur_added, + const std::set& cur_removed) { for (const auto& a : cur_added) { setResourceWaitingForServer(a); - // Removed->added requires us to keep track of it as a "new" addition, since our user may have - // forgotten its copy of the resource after instructing us to remove it, and so needs to be - // reminded of it. + // If interest in a resource is removed-then-added (all before a discovery request + // can be sent), we must treat it as a "new" addition: our user may have forgotten its + // copy of the resource after instructing us to remove it, and need to be reminded of it. names_removed_.erase(a); names_added_.insert(a); } for (const auto& r : cur_removed) { setLostInterestInResource(r); - // Ideally, when a resource is added-then-removed in between requests, we would avoid putting - // a superfluous "unsubscribe [resource that was never subscribed]" in the request. However, - // the removed-then-added case *does* need to go in the request, and due to how we accomplish - // that, it's difficult to distinguish remove-add-remove from add-remove (because "remove-add" - // has to be treated as equivalent to just "add"). + // Ideally, when interest in a resource is added-then-removed in between requests, + // we would avoid putting a superfluous "unsubscribe [resource that was never subscribed]" + // in the request. However, the removed-then-added case *does* need to go in the request, + // and due to how we accomplish that, it's difficult to distinguish remove-add-remove from + // add-remove (because "remove-add" has to be treated as equivalent to just "add"). names_added_.erase(r); names_removed_.insert(r); } @@ -89,8 +55,7 @@ UpdateAck DeltaSubscriptionState::handleResponse(const envoy::api::v2::DeltaDiscoveryResponse& message) { // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. - UpdateAck ack(message.nonce()); - stats_.update_attempt_.inc(); + UpdateAck ack(message.nonce(), type_url_); try { handleGoodResponse(message); } catch (const EnvoyException& e) { @@ -108,6 +73,12 @@ void DeltaSubscriptionState::handleGoodResponse( throw EnvoyException( fmt::format("duplicate name {} found among added/updated resources", resource.name())); } + if (message.type_url() != resource.resource().type_url()) { + throw EnvoyException(fmt::format("type URL {} embedded in an individual Any does not match " + "the message-wide type URL {} in DeltaDiscoveryResponse {}", + resource.resource().type_url(), message.type_url(), + message.DebugString())); + } } for (const auto& name : message.removed_resources()) { if (!names_added_removed.insert(name).second) { @@ -115,7 +86,6 @@ void DeltaSubscriptionState::handleGoodResponse( fmt::format("duplicate name {} found in the union of added+removed resources", name)); } } - callbacks_.onConfigUpdate(message.resources(), message.removed_resources(), message.system_version_info()); for (const auto& resource : message.resources()) { @@ -134,8 +104,6 @@ void DeltaSubscriptionState::handleGoodResponse( setResourceWaitingForServer(resource_name); } } - stats_.update_success_.inc(); - stats_.version_.set(HashUtil::xxHash64(message.system_version_info())); ENVOY_LOG(debug, "Delta config for {} accepted with {} resources added, {} removed", type_url_, message.resources().size(), message.removed_resources().size()); } @@ -145,19 +113,16 @@ void DeltaSubscriptionState::handleBadResponse(const EnvoyException& e, UpdateAc ack.error_detail_.set_code(Grpc::Status::GrpcStatus::Internal); ack.error_detail_.set_message(e.what()); disableInitFetchTimeoutTimer(); - stats_.update_rejected_.inc(); ENVOY_LOG(warn, "delta config for {} rejected: {}", type_url_, e.what()); callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); } void DeltaSubscriptionState::handleEstablishmentFailure() { - // New gRPC stream will be established and send requests again. - // If init_fetch_timeout is non-zero, server will continue startup after it timeout - stats_.update_failure_.inc(); - stats_.update_attempt_.inc(); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + nullptr); } -envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { +envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequestAckless() { envoy::api::v2::DeltaDiscoveryRequest request; if (!any_request_sent_yet_in_current_stream_) { any_request_sent_yet_in_current_stream_ = true; @@ -189,6 +154,17 @@ envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { return request; } +envoy::api::v2::DeltaDiscoveryRequest +DeltaSubscriptionState::getNextRequestWithAck(const UpdateAck& ack) { + envoy::api::v2::DeltaDiscoveryRequest request = getNextRequestAckless(); + request.set_response_nonce(ack.nonce_); + if (ack.error_detail_.code() != Grpc::Status::GrpcStatus::Ok) { + // Don't needlessly make the field present-but-empty if status is ok. + request.mutable_error_detail()->CopyFrom(ack.error_detail_); + } + return request; +} + void DeltaSubscriptionState::disableInitFetchTimeoutTimer() { if (init_fetch_timeout_timer_) { init_fetch_timeout_timer_->disableTimer(); diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index f21e0b895b9e8..6442d4b7a84ff 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -7,34 +7,26 @@ #include "envoy/local_info/local_info.h" #include "common/common/assert.h" -#include "common/common/hash.h" #include "common/common/logger.h" +#include "common/config/pausable_ack_queue.h" namespace Envoy { namespace Config { -struct UpdateAck { - UpdateAck(absl::string_view nonce) : nonce_(nonce) {} - std::string nonce_; - ::google::rpc::Status error_detail_; -}; - -// Tracks the xDS protocol state of an individual ongoing delta xDS session. +// Tracks the xDS protocol state of an individual ongoing delta xDS session, i.e. a single type_url. +// There can be multiple DeltaSubscriptionStates active. They will always all be +// blissfully unaware of each other's existence, even when their messages are +// being multiplexed together by ADS. class DeltaSubscriptionState : public Logger::Loggable { public: - DeltaSubscriptionState(const std::string& type_url, const std::set& resource_names, - SubscriptionCallbacks& callbacks, const LocalInfo::LocalInfo& local_info, + DeltaSubscriptionState(std::string type_url, SubscriptionCallbacks& callbacks, + const LocalInfo::LocalInfo& local_info, std::chrono::milliseconds init_fetch_timeout, - Event::Dispatcher& dispatcher, SubscriptionStats& stats); - - void setInitFetchTimeout(Event::Dispatcher& dispatcher); - - void pause(); - void resume(); - bool paused() const { return paused_; } + Event::Dispatcher& dispatcher); // Update which resources we're interested in subscribing to. - void updateResourceInterest(const std::set& update_to_these_names); + void updateSubscriptionInterest(const std::set& cur_added, + const std::set& cur_removed); // Whether there was a change in our subscription interest we have yet to inform the server of. bool subscriptionUpdatePending() const; @@ -45,7 +37,14 @@ class DeltaSubscriptionState : public Logger::Loggable { void handleEstablishmentFailure(); - envoy::api::v2::DeltaDiscoveryRequest getNextRequest(); + // Returns the next gRPC request proto to be sent off to the server, based on this object's + // understanding of the current protocol state, and new resources that Envoy wants to request. + envoy::api::v2::DeltaDiscoveryRequest getNextRequestAckless(); + // The WithAck version first calls the Ackless version, then adds in the passed-in ack. + envoy::api::v2::DeltaDiscoveryRequest getNextRequestWithAck(const UpdateAck& ack); + + DeltaSubscriptionState(const DeltaSubscriptionState&) = delete; + DeltaSubscriptionState& operator=(const DeltaSubscriptionState&) = delete; private: void handleGoodResponse(const envoy::api::v2::DeltaDiscoveryResponse& message); @@ -86,12 +85,12 @@ class DeltaSubscriptionState : public Logger::Loggable { std::set resource_names_; const std::string type_url_; + // callbacks_ is expected to be a WatchMap. SubscriptionCallbacks& callbacks_; const LocalInfo::LocalInfo& local_info_; std::chrono::milliseconds init_fetch_timeout_; Event::TimerPtr init_fetch_timeout_timer_; - bool paused_{}; bool any_request_sent_yet_in_current_stream_{}; // Tracks changes in our subscription interest since the previous DeltaDiscoveryRequest we sent. @@ -99,8 +98,6 @@ class DeltaSubscriptionState : public Logger::Loggable { // Feel free to change to unordered if you can figure out how to make it work. std::set names_added_; std::set names_removed_; - - SubscriptionStats& stats_; }; } // namespace Config diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 1bef8eafe8f6b..80a693c69d9cd 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -27,7 +27,7 @@ void FilesystemSubscriptionImpl::start(const std::set&) { refresh(); } -void FilesystemSubscriptionImpl::updateResources(const std::set&) { +void FilesystemSubscriptionImpl::updateResourceInterest(const std::set&) { // Bump stats for consistence behavior with other xDS. stats_.update_attempt_.inc(); } diff --git a/source/common/config/filesystem_subscription_impl.h b/source/common/config/filesystem_subscription_impl.h index 6ff089208dc84..851935f1638e8 100644 --- a/source/common/config/filesystem_subscription_impl.h +++ b/source/common/config/filesystem_subscription_impl.h @@ -26,9 +26,9 @@ class FilesystemSubscriptionImpl : public Config::Subscription, // Config::Subscription // We report all discovered resources in the watched file, so the resource names arguments are - // unused, and updateResources is a no-op (other than updating a stat). + // unused, and updateResourceInterest is a no-op (other than updating a stat). void start(const std::set&) override; - void updateResources(const std::set&) override; + void updateResourceInterest(const std::set&) override; private: void refresh(); diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index c1eda78b6dd67..fdb3fa603758a 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -164,8 +164,9 @@ void GrpcMuxImpl::onDiscoveryResponse( GrpcMuxCallbacks& callbacks = api_state_[type_url].watches_.front()->callbacks_; for (const auto& resource : message->resources()) { if (type_url != resource.type_url()) { - throw EnvoyException(fmt::format("{} does not match {} type URL in DiscoveryResponse {}", - resource.type_url(), type_url, message->DebugString())); + throw EnvoyException( + fmt::format("{} does not match the message-wide type URL {} in DiscoveryResponse {}", + resource.type_url(), type_url, message->DebugString())); } const std::string resource_name = callbacks.resourceName(resource); resources.emplace(resource_name, resource); @@ -232,10 +233,7 @@ void GrpcMuxImpl::queueDiscoveryRequest(const std::string& queue_item) { void GrpcMuxImpl::clearRequestQueue() { grpc_stream_.maybeUpdateQueueSizeStat(0); - // TODO(fredlas) when we have C++17: request_queue_ = {}; - while (!request_queue_.empty()) { - request_queue_.pop(); - } + request_queue_ = {}; } void GrpcMuxImpl::drainRequests() { diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index ae245c48d7fdb..83821a91504a6 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -34,10 +34,20 @@ class GrpcMuxImpl : public GrpcMux, void start() override; GrpcMuxWatchPtr subscribe(const std::string& type_url, const std::set& resources, GrpcMuxCallbacks& callbacks) override; + + // GrpcMux void pause(const std::string& type_url) override; void resume(const std::string& type_url) override; bool paused(const std::string& type_url) const override; + Watch* addOrUpdateWatch(const std::string&, Watch*, const std::set&, + SubscriptionCallbacks&, std::chrono::milliseconds) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void removeWatch(const std::string&, Watch*) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + + void handleDiscoveryResponse(std::unique_ptr&& message); + void sendDiscoveryRequest(const std::string& type_url); // Config::GrpcStreamCallbacks @@ -52,6 +62,7 @@ class GrpcMuxImpl : public GrpcMux, } private: + void drainRequests(); void setRetryTimer(); struct GrpcMuxWatchImpl : public GrpcMuxWatch, RaiiListElement { @@ -100,7 +111,6 @@ class GrpcMuxImpl : public GrpcMux, // Request queue management logic. void queueDiscoveryRequest(const std::string& queue_item); void clearRequestQueue(); - void drainRequests(); GrpcStream grpc_stream_; const LocalInfo::LocalInfo& local_info_; @@ -116,7 +126,7 @@ class GrpcMuxImpl : public GrpcMux, std::queue request_queue_; }; -class NullGrpcMuxImpl : public GrpcMux { +class NullGrpcMuxImpl : public GrpcMux, GrpcStreamCallbacks { public: void start() override {} GrpcMuxWatchPtr subscribe(const std::string&, const std::set&, @@ -126,6 +136,19 @@ class NullGrpcMuxImpl : public GrpcMux { void pause(const std::string&) override {} void resume(const std::string&) override {} bool paused(const std::string&) const override { return false; } + + Watch* addOrUpdateWatch(const std::string&, Watch*, const std::set&, + SubscriptionCallbacks&, std::chrono::milliseconds) override { + throw EnvoyException("ADS must be configured to support an ADS config source"); + } + void removeWatch(const std::string&, Watch*) override { + throw EnvoyException("ADS must be configured to support an ADS config source"); + } + + void onWriteable() override {} + void onStreamEstablished() override {} + void onEstablishmentFailure() override {} + void onDiscoveryResponse(std::unique_ptr&&) override {} }; } // namespace Config diff --git a/source/common/config/grpc_mux_subscription_impl.cc b/source/common/config/grpc_mux_subscription_impl.cc index dffab9f0caea3..06d1fd3b562fa 100644 --- a/source/common/config/grpc_mux_subscription_impl.cc +++ b/source/common/config/grpc_mux_subscription_impl.cc @@ -9,7 +9,7 @@ namespace Envoy { namespace Config { -GrpcMuxSubscriptionImpl::GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, +GrpcMuxSubscriptionImpl::GrpcMuxSubscriptionImpl(GrpcMuxSharedPtr grpc_mux, SubscriptionCallbacks& callbacks, SubscriptionStats stats, absl::string_view type_url, @@ -28,19 +28,20 @@ void GrpcMuxSubscriptionImpl::start(const std::set& resources) { init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } - watch_ = grpc_mux_.subscribe(type_url_, resources, *this); + watch_ = grpc_mux_->subscribe(type_url_, resources, *this); // The attempt stat here is maintained for the purposes of having consistency between ADS and // gRPC/filesystem/REST Subscriptions. Since ADS is push based and muxed, the notion of an // "attempt" for a given xDS API combined by ADS is not really that meaningful. stats_.update_attempt_.inc(); } -void GrpcMuxSubscriptionImpl::updateResources(const std::set& update_to_these_names) { +void GrpcMuxSubscriptionImpl::updateResourceInterest( + const std::set& update_to_these_names) { // First destroy the watch, so that this subscribe doesn't send a request for both the // previously watched resources and the new ones (we may have lost interest in some of the // previously watched ones). watch_.reset(); - watch_ = grpc_mux_.subscribe(type_url_, update_to_these_names, *this); + watch_ = grpc_mux_->subscribe(type_url_, update_to_these_names, *this); stats_.update_attempt_.inc(); } diff --git a/source/common/config/grpc_mux_subscription_impl.h b/source/common/config/grpc_mux_subscription_impl.h index 9fb4cd76407c2..299aa4f1480ee 100644 --- a/source/common/config/grpc_mux_subscription_impl.h +++ b/source/common/config/grpc_mux_subscription_impl.h @@ -17,14 +17,14 @@ class GrpcMuxSubscriptionImpl : public Subscription, GrpcMuxCallbacks, Logger::Loggable { public: - GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, SubscriptionCallbacks& callbacks, + GrpcMuxSubscriptionImpl(GrpcMuxSharedPtr grpc_mux, SubscriptionCallbacks& callbacks, SubscriptionStats stats, absl::string_view type_url, Event::Dispatcher& dispatcher, std::chrono::milliseconds init_fetch_timeout); // Config::Subscription void start(const std::set& resource_names) override; - void updateResources(const std::set& update_to_these_names) override; + void updateResourceInterest(const std::set& update_to_these_names) override; // Config::GrpcMuxCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, @@ -36,7 +36,7 @@ class GrpcMuxSubscriptionImpl : public Subscription, private: void disableInitFetchTimeoutTimer(); - GrpcMux& grpc_mux_; + GrpcMuxSharedPtr grpc_mux_; SubscriptionCallbacks& callbacks_; SubscriptionStats stats_; const std::string type_url_; diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index 9a182c2637ff7..1be9227429300 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -2,7 +2,7 @@ #include -#include "envoy/config/xds_grpc_context.h" +#include "envoy/config/grpc_mux.h" #include "envoy/grpc/async_client.h" #include "common/common/backoff_strategy.h" diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index 94e9208431024..8914519746f21 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -21,8 +21,9 @@ class GrpcSubscriptionImpl : public Config::Subscription { Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, std::chrono::milliseconds init_fetch_timeout, bool skip_subsequent_node) : callbacks_(callbacks), - grpc_mux_(local_info, std::move(async_client), dispatcher, service_method, random, scope, - rate_limit_settings, skip_subsequent_node), + grpc_mux_(std::make_shared(local_info, std::move(async_client), + dispatcher, service_method, random, scope, + rate_limit_settings, skip_subsequent_node)), grpc_mux_subscription_(grpc_mux_, callbacks_, stats, type_url, dispatcher, init_fetch_timeout) {} @@ -30,18 +31,18 @@ class GrpcSubscriptionImpl : public Config::Subscription { void start(const std::set& resource_names) override { // Subscribe first, so we get failure callbacks if grpc_mux_.start() fails. grpc_mux_subscription_.start(resource_names); - grpc_mux_.start(); + grpc_mux_->start(); } - void updateResources(const std::set& update_to_these_names) override { - grpc_mux_subscription_.updateResources(update_to_these_names); + void updateResourceInterest(const std::set& update_to_these_names) override { + grpc_mux_subscription_.updateResourceInterest(update_to_these_names); } - GrpcMuxImpl& grpcMux() { return grpc_mux_; } + std::shared_ptr grpcMux() { return grpc_mux_; } private: Config::SubscriptionCallbacks& callbacks_; - GrpcMuxImpl grpc_mux_; + std::shared_ptr grpc_mux_; GrpcMuxSubscriptionImpl grpc_mux_subscription_; }; diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index dda038436500b..6e718e27775ec 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -49,7 +49,8 @@ void HttpSubscriptionImpl::start(const std::set& resource_names) { initialize(); } -void HttpSubscriptionImpl::updateResources(const std::set& update_to_these_names) { +void HttpSubscriptionImpl::updateResourceInterest( + const std::set& update_to_these_names) { Protobuf::RepeatedPtrField resources_vector(update_to_these_names.begin(), update_to_these_names.end()); request_.mutable_resource_names()->Swap(&resources_vector); diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index 6ad8055e4e8b6..727cc90e6db20 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -31,7 +31,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, // Config::Subscription void start(const std::set& resource_names) override; - void updateResources(const std::set& update_to_these_names) override; + void updateResourceInterest(const std::set& update_to_these_names) override; // Http::RestApiFetcher void createRequest(Http::Message& request) override; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc new file mode 100644 index 0000000000000..d22593d0bf997 --- /dev/null +++ b/source/common/config/new_grpc_mux_impl.cc @@ -0,0 +1,228 @@ +#include "common/config/new_grpc_mux_impl.h" + +#include "common/common/assert.h" +#include "common/common/backoff_strategy.h" +#include "common/common/token_bucket_impl.h" +#include "common/config/utility.h" +#include "common/protobuf/protobuf.h" +#include "common/protobuf/utility.h" + +namespace Envoy { +namespace Config { + +NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, + Event::Dispatcher& dispatcher, + const Protobuf::MethodDescriptor& service_method, + Runtime::RandomGenerator& random, Stats::Scope& scope, + const RateLimitSettings& rate_limit_settings, + const LocalInfo::LocalInfo& local_info) + : dispatcher_(dispatcher), local_info_(local_info), + grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, + rate_limit_settings) {} + +Watch* NewGrpcMuxImpl::addOrUpdateWatch(const std::string& type_url, Watch* watch, + const std::set& resources, + SubscriptionCallbacks& callbacks, + std::chrono::milliseconds init_fetch_timeout) { + if (watch == nullptr) { + return addWatch(type_url, resources, callbacks, init_fetch_timeout); + } else { + updateWatch(type_url, watch, resources); + return watch; + } +} + +void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { + updateWatch(type_url, watch, {}); + auto entry = subscriptions_.find(type_url); + RELEASE_ASSERT(entry != subscriptions_.end(), + fmt::format("removeWatch() called for non-existent subscription {}.", type_url)); + entry->second->watch_map_.removeWatch(watch); +} + +void NewGrpcMuxImpl::pause(const std::string& type_url) { pausable_ack_queue_.pause(type_url); } + +void NewGrpcMuxImpl::resume(const std::string& type_url) { + pausable_ack_queue_.resume(type_url); + trySendDiscoveryRequests(); +} + +bool NewGrpcMuxImpl::paused(const std::string& type_url) const { + return pausable_ack_queue_.paused(type_url); +} + +void NewGrpcMuxImpl::onDiscoveryResponse( + std::unique_ptr&& message) { + ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), + message->system_version_info()); + auto sub = subscriptions_.find(message->type_url()); + if (sub == subscriptions_.end()) { + ENVOY_LOG(warn, + "Dropping received DeltaDiscoveryResponse (with version {}) for non-existent " + "subscription {}.", + message->system_version_info(), message->type_url()); + return; + } + kickOffAck(sub->second->sub_state_.handleResponse(*message)); +} + +void NewGrpcMuxImpl::onStreamEstablished() { + for (auto& sub : subscriptions_) { + sub.second->sub_state_.markStreamFresh(); + } + trySendDiscoveryRequests(); +} + +void NewGrpcMuxImpl::onEstablishmentFailure() { + // If this happens while Envoy is still initializing, the onConfigUpdateFailed() we ultimately + // call on CDS will cause LDS to start up, which adds to subscriptions_ here. So, to avoid a + // crash, the iteration needs to dance around a little: collect pointers to all + // SubscriptionStates, call on all those pointers we haven't yet called on, repeat if there are + // now more SubscriptionStates. + absl::flat_hash_map all_subscribed; + absl::flat_hash_map already_called; + do { + for (auto& sub : subscriptions_) { + all_subscribed[sub.first] = &sub.second->sub_state_; + } + for (auto& sub : all_subscribed) { + if (already_called.insert(sub).second) { // insert succeeded ==> not already called + sub.second->handleEstablishmentFailure(); + } + } + } while (all_subscribed.size() != subscriptions_.size()); +} + +void NewGrpcMuxImpl::onWriteable() { trySendDiscoveryRequests(); } + +void NewGrpcMuxImpl::kickOffAck(UpdateAck ack) { + pausable_ack_queue_.push(std::move(ack)); + trySendDiscoveryRequests(); +} + +// TODO(fredlas) to be removed from the GrpcMux interface very soon. +GrpcMuxWatchPtr NewGrpcMuxImpl::subscribe(const std::string&, const std::set&, + GrpcMuxCallbacks&) { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; +} + +void NewGrpcMuxImpl::start() { grpc_stream_.establishNewStream(); } + +Watch* NewGrpcMuxImpl::addWatch(const std::string& type_url, const std::set& resources, + SubscriptionCallbacks& callbacks, + std::chrono::milliseconds init_fetch_timeout) { + auto entry = subscriptions_.find(type_url); + if (entry == subscriptions_.end()) { + // We don't yet have a subscription for type_url! Make one! + addSubscription(type_url, init_fetch_timeout); + return addWatch(type_url, resources, callbacks, init_fetch_timeout); + } + + Watch* watch = entry->second->watch_map_.addWatch(callbacks); + // updateWatch() queues a discovery request if any of 'resources' are not yet subscribed. + updateWatch(type_url, watch, resources); + return watch; +} + +// Updates the list of resource names watched by the given watch. If an added name is new across +// the whole subscription, or if a removed name has no other watch interested in it, then the +// subscription will enqueue and attempt to send an appropriate discovery request. +void NewGrpcMuxImpl::updateWatch(const std::string& type_url, Watch* watch, + const std::set& resources) { + ASSERT(watch != nullptr); + auto sub = subscriptions_.find(type_url); + RELEASE_ASSERT(sub != subscriptions_.end(), + fmt::format("Watch of {} has no subscription to update.", type_url)); + auto added_removed = sub->second->watch_map_.updateWatchInterest(watch, resources); + sub->second->sub_state_.updateSubscriptionInterest(added_removed.added_, added_removed.removed_); + // Tell the server about our change in interest, if any. + if (sub->second->sub_state_.subscriptionUpdatePending()) { + trySendDiscoveryRequests(); + } +} + +void NewGrpcMuxImpl::addSubscription(const std::string& type_url, + std::chrono::milliseconds init_fetch_timeout) { + subscriptions_.emplace(type_url, std::make_unique(type_url, init_fetch_timeout, + dispatcher_, local_info_)); + subscription_ordering_.emplace_back(type_url); +} + +void NewGrpcMuxImpl::trySendDiscoveryRequests() { + while (true) { + // Do any of our subscriptions even want to send a request? + absl::optional maybe_request_type = whoWantsToSendDiscoveryRequest(); + if (!maybe_request_type.has_value()) { + break; + } + // If so, which one (by type_url)? + std::string next_request_type_url = maybe_request_type.value(); + // If we don't have a subscription object for this request's type_url, drop the request. + auto sub = subscriptions_.find(next_request_type_url); + RELEASE_ASSERT(sub != subscriptions_.end(), + fmt::format("Tried to send discovery request for non-existent subscription {}.", + next_request_type_url)); + + // Try again later if paused/rate limited/stream down. + if (!canSendDiscoveryRequest(next_request_type_url)) { + break; + } + // Get our subscription state to generate the appropriate DeltaDiscoveryRequest, and send. + if (!pausable_ack_queue_.empty()) { + // Because ACKs take precedence over plain requests, if there is anything in the queue, it's + // safe to assume it's of the type_url that we're wanting to send. + grpc_stream_.sendMessage( + sub->second->sub_state_.getNextRequestWithAck(pausable_ack_queue_.front())); + pausable_ack_queue_.pop(); + } else { + grpc_stream_.sendMessage(sub->second->sub_state_.getNextRequestAckless()); + } + } + grpc_stream_.maybeUpdateQueueSizeStat(pausable_ack_queue_.size()); +} + +// Checks whether external conditions allow sending a DeltaDiscoveryRequest. (Does not check +// whether we *want* to send a DeltaDiscoveryRequest). +bool NewGrpcMuxImpl::canSendDiscoveryRequest(const std::string& type_url) { + RELEASE_ASSERT( + !pausable_ack_queue_.paused(type_url), + fmt::format("canSendDiscoveryRequest() called on paused type_url {}. Pausedness is " + "supposed to be filtered out by whoWantsToSendDiscoveryRequest(). ", + type_url)); + + if (!grpc_stream_.grpcStreamAvailable()) { + ENVOY_LOG(trace, "No stream available to send a discovery request for {}.", type_url); + return false; + } else if (!grpc_stream_.checkRateLimitAllowsDrain()) { + ENVOY_LOG(trace, "{} discovery request hit rate limit; will try later.", type_url); + return false; + } + return true; +} + +// Checks whether we have something to say in a DeltaDiscoveryRequest, which can be an ACK and/or +// a subscription update. (Does not check whether we *can* send that DeltaDiscoveryRequest). +// Returns the type_url we should send the DeltaDiscoveryRequest for (if any). +// First, prioritizes ACKs over non-ACK subscription interest updates. +// Then, prioritizes non-ACK updates in the order the various types +// of subscriptions were activated. +absl::optional NewGrpcMuxImpl::whoWantsToSendDiscoveryRequest() { + // All ACKs are sent before plain updates. trySendDiscoveryRequests() relies on this. So, choose + // type_url from pausable_ack_queue_ if possible, before looking at pending updates. + if (!pausable_ack_queue_.empty()) { + return pausable_ack_queue_.front().type_url_; + } + // If we're looking to send multiple non-ACK requests, send them in the order that their + // subscriptions were initiated. + for (const auto& sub_type : subscription_ordering_) { + auto sub = subscriptions_.find(sub_type); + if (sub != subscriptions_.end() && sub->second->sub_state_.subscriptionUpdatePending() && + !pausable_ack_queue_.paused(sub_type)) { + return sub->first; + } + } + return absl::nullopt; +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h new file mode 100644 index 0000000000000..1bd3b5a1f1742 --- /dev/null +++ b/source/common/config/new_grpc_mux_impl.h @@ -0,0 +1,117 @@ +#pragma once + +#include "envoy/api/v2/discovery.pb.h" +#include "envoy/common/token_bucket.h" +#include "envoy/config/grpc_mux.h" +#include "envoy/config/subscription.h" + +#include "common/common/logger.h" +#include "common/config/delta_subscription_state.h" +#include "common/config/grpc_stream.h" +#include "common/config/pausable_ack_queue.h" +#include "common/config/watch_map.h" +#include "common/grpc/common.h" + +namespace Envoy { +namespace Config { + +// Manages subscriptions to one or more type of resource. The logical protocol +// state of those subscription(s) is handled by DeltaSubscriptionState. +// This class owns the GrpcStream used to talk to the server, maintains queuing +// logic to properly order the subscription(s)' various messages, and allows +// starting/stopping/pausing of the subscriptions. +class NewGrpcMuxImpl : public GrpcMux, + public GrpcStreamCallbacks, + Logger::Loggable { +public: + NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatcher& dispatcher, + const Protobuf::MethodDescriptor& service_method, Runtime::RandomGenerator& random, + Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, + const LocalInfo::LocalInfo& local_info); + + Watch* addOrUpdateWatch(const std::string& type_url, Watch* watch, + const std::set& resources, SubscriptionCallbacks& callbacks, + std::chrono::milliseconds init_fetch_timeout) override; + void removeWatch(const std::string& type_url, Watch* watch) override; + + void pause(const std::string& type_url) override; + void resume(const std::string& type_url) override; + bool paused(const std::string& type_url) const override; + void + onDiscoveryResponse(std::unique_ptr&& message) override; + + void onStreamEstablished() override; + + void onEstablishmentFailure() override; + + void onWriteable() override; + + void kickOffAck(UpdateAck ack); + + // TODO(fredlas) remove these two from the GrpcMux interface. + GrpcMuxWatchPtr subscribe(const std::string&, const std::set&, + GrpcMuxCallbacks&) override; + void start() override; + +private: + Watch* addWatch(const std::string& type_url, const std::set& resources, + SubscriptionCallbacks& callbacks, std::chrono::milliseconds init_fetch_timeout); + + // Updates the list of resource names watched by the given watch. If an added name is new across + // the whole subscription, or if a removed name has no other watch interested in it, then the + // subscription will enqueue and attempt to send an appropriate discovery request. + void updateWatch(const std::string& type_url, Watch* watch, + const std::set& resources); + + void addSubscription(const std::string& type_url, std::chrono::milliseconds init_fetch_timeout); + + void trySendDiscoveryRequests(); + + // Checks whether external conditions allow sending a DeltaDiscoveryRequest. (Does not check + // whether we *want* to send a DeltaDiscoveryRequest). + bool canSendDiscoveryRequest(const std::string& type_url); + + // Checks whether we have something to say in a DeltaDiscoveryRequest, which can be an ACK and/or + // a subscription update. (Does not check whether we *can* send that DeltaDiscoveryRequest). + // Returns the type_url we should send the DeltaDiscoveryRequest for (if any). + // First, prioritizes ACKs over non-ACK subscription interest updates. + // Then, prioritizes non-ACK updates in the order the various types + // of subscriptions were activated. + absl::optional whoWantsToSendDiscoveryRequest(); + + Event::Dispatcher& dispatcher_; + const LocalInfo::LocalInfo& local_info_; + + // Resource (N)ACKs we're waiting to send, stored in the order that they should be sent in. All + // of our different resource types' ACKs are mixed together in this queue. See class for + // description of how it interacts with pause() and resume(). + PausableAckQueue pausable_ack_queue_; + + struct SubscriptionStuff { + SubscriptionStuff(const std::string& type_url, std::chrono::milliseconds init_fetch_timeout, + Event::Dispatcher& dispatcher, const LocalInfo::LocalInfo& local_info) + : sub_state_(type_url, watch_map_, local_info, init_fetch_timeout, dispatcher), + init_fetch_timeout_(init_fetch_timeout) {} + + WatchMap watch_map_; + DeltaSubscriptionState sub_state_; + const std::chrono::milliseconds init_fetch_timeout_; + + SubscriptionStuff(const SubscriptionStuff&) = delete; + SubscriptionStuff& operator=(const SubscriptionStuff&) = delete; + }; + // Map key is type_url. + absl::flat_hash_map> subscriptions_; + + // Determines the order of initial discovery requests. (Assumes that subscriptions are added in + // the order of Envoy's dependency ordering). + std::list subscription_ordering_; + + GrpcStream + grpc_stream_; +}; + +using NewGrpcMuxImplSharedPtr = std::shared_ptr; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/pausable_ack_queue.cc b/source/common/config/pausable_ack_queue.cc new file mode 100644 index 0000000000000..0217717de0355 --- /dev/null +++ b/source/common/config/pausable_ack_queue.cc @@ -0,0 +1,66 @@ +#include "common/config/pausable_ack_queue.h" + +#include + +#include "common/common/assert.h" + +namespace Envoy { +namespace Config { + +void PausableAckQueue::push(UpdateAck x) { storage_.push_back(std::move(x)); } + +size_t PausableAckQueue::size() const { return storage_.size(); } + +bool PausableAckQueue::empty() { + for (const auto& entry : storage_) { + if (!paused_[entry.type_url_]) { + return false; + } + } + return true; +} + +const UpdateAck& PausableAckQueue::front() { + for (const auto& entry : storage_) { + if (!paused_[entry.type_url_]) { + return entry; + } + } + RELEASE_ASSERT(false, "front() on an empty queue is undefined behavior!"); + NOT_REACHED_GCOVR_EXCL_LINE; +} + +void PausableAckQueue::pop() { + for (auto it = storage_.begin(); it != storage_.end(); ++it) { + if (!paused_[it->type_url_]) { + storage_.erase(it); + return; + } + } + RELEASE_ASSERT(false, "pop() on an empty queue is undefined behavior!"); + NOT_REACHED_GCOVR_EXCL_LINE; +} + +void PausableAckQueue::pause(const std::string& type_url) { + // It's ok to pause a subscription that doesn't exist yet. + auto& pause_entry = paused_[type_url]; + ASSERT(!pause_entry); + pause_entry = true; +} + +void PausableAckQueue::resume(const std::string& type_url) { + auto& pause_entry = paused_[type_url]; + ASSERT(pause_entry); + pause_entry = false; +} + +bool PausableAckQueue::paused(const std::string& type_url) const { + auto entry = paused_.find(type_url); + if (entry == paused_.end()) { + return false; + } + return entry->second; +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/pausable_ack_queue.h b/source/common/config/pausable_ack_queue.h new file mode 100644 index 0000000000000..46222a8b2e3ce --- /dev/null +++ b/source/common/config/pausable_ack_queue.h @@ -0,0 +1,43 @@ +#pragma once + +#include + +#include "envoy/api/v2/discovery.pb.h" + +#include "absl/container/flat_hash_map.h" + +namespace Envoy { +namespace Config { + +struct UpdateAck { + UpdateAck(absl::string_view nonce, absl::string_view type_url) + : nonce_(nonce), type_url_(type_url) {} + std::string nonce_; + std::string type_url_; + ::google::rpc::Status error_detail_; +}; + +// There is a head-of-line blocking issue resulting from the intersection of 1) ADS's need for +// subscription request ordering and 2) the ability to "pause" one of the resource types within ADS. +// We need a queue that understands ADS's resource type pausing. Specifically, we need front()/pop() +// to choose the first element whose type_url isn't paused. +class PausableAckQueue { +public: + void push(UpdateAck x); + size_t size() const; + bool empty(); + const UpdateAck& front(); + void pop(); + void pause(const std::string& type_url); + void resume(const std::string& type_url); + bool paused(const std::string& type_url) const; + +private: + // It's ok for non-existent subs to be paused/resumed. The cleanest way to support that is to give + // the pause state its own map. (Map key is type_url.) + absl::flat_hash_map paused_; + std::list storage_; +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index b85c8bbb1d1ce..8e8fbc4e088f8 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -5,6 +5,7 @@ #include "common/config/grpc_mux_subscription_impl.h" #include "common/config/grpc_subscription_impl.h" #include "common/config/http_subscription_impl.h" +#include "common/config/new_grpc_mux_impl.h" #include "common/config/type_to_endpoint.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" @@ -21,7 +22,7 @@ SubscriptionFactoryImpl::SubscriptionFactoryImpl( SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( const envoy::api::v2::core::ConfigSource& config, absl::string_view type_url, - Stats::Scope& scope, SubscriptionCallbacks& callbacks) { + Stats::Scope& scope, SubscriptionCallbacks& callbacks, bool is_delta) { Config::Utility::checkLocalInfo(type_url, local_info_); std::unique_ptr result; SubscriptionStats stats = Utility::generateStats(scope); @@ -62,13 +63,13 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( case envoy::api::v2::core::ApiConfigSource::DELTA_GRPC: { Utility::checkApiConfigSourceSubscriptionBackingCluster(cm_.clusters(), api_config_source); result = std::make_unique( - local_info_, - Config::Utility::factoryForGrpcApiConfigSource(cm_.grpcAsyncClientManager(), - api_config_source, scope) - ->create(), - dispatcher_, deltaGrpcMethod(type_url), type_url, random_, scope, - Utility::parseRateLimitSettings(api_config_source), callbacks, stats, - Utility::configSourceInitialFetchTimeout(config)); + std::make_shared( + Config::Utility::factoryForGrpcApiConfigSource(cm_.grpcAsyncClientManager(), + api_config_source, scope) + ->create(), + dispatcher_, deltaGrpcMethod(type_url), random_, scope, + Utility::parseRateLimitSettings(api_config_source), local_info_), + type_url, callbacks, stats, Utility::configSourceInitialFetchTimeout(config), false); break; } default: @@ -77,9 +78,15 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( break; } case envoy::api::v2::core::ConfigSource::kAds: { - result = std::make_unique( - cm_.adsMux(), callbacks, stats, type_url, dispatcher_, - Utility::configSourceInitialFetchTimeout(config)); + if (is_delta) { + result = std::make_unique( + cm_.adsMux(), type_url, callbacks, stats, + Utility::configSourceInitialFetchTimeout(config), true); + } else { + result = std::make_unique( + cm_.adsMux(), callbacks, stats, type_url, dispatcher_, + Utility::configSourceInitialFetchTimeout(config)); + } break; } default: diff --git a/source/common/config/subscription_factory_impl.h b/source/common/config/subscription_factory_impl.h index 3a7f0ba1e31c8..43cec2139649c 100644 --- a/source/common/config/subscription_factory_impl.h +++ b/source/common/config/subscription_factory_impl.h @@ -16,10 +16,12 @@ class SubscriptionFactoryImpl : public SubscriptionFactory { Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); + // TODO(fredlas) remove is_delta once delta and SotW are unified // Config::SubscriptionFactory SubscriptionPtr subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, - SubscriptionCallbacks& callbacks) override; + SubscriptionCallbacks& callbacks, + bool is_delta) override; private: const LocalInfo::LocalInfo& local_info_; diff --git a/source/common/config/xDS_code_diagram.png b/source/common/config/xDS_code_diagram.png new file mode 100644 index 0000000000000..ef4df79cc1e30 Binary files /dev/null and b/source/common/config/xDS_code_diagram.png differ diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index c9fd1b3002556..af64085ddd195 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -23,7 +23,7 @@ RouteConfigProviderPtr RouteConfigProviderUtil::create( const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& config, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - RouteConfigProviderManager& route_config_provider_manager) { + RouteConfigProviderManager& route_config_provider_manager, bool is_delta) { switch (config.route_specifier_case()) { case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager:: kRouteConfig: @@ -31,7 +31,7 @@ RouteConfigProviderPtr RouteConfigProviderUtil::create( factory_context); case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::kRds: return route_config_provider_manager.createRdsRouteConfigProvider( - config.rds(), factory_context, stat_prefix, factory_context.initManager()); + config.rds(), factory_context, stat_prefix, factory_context.initManager(), is_delta); default: NOT_REACHED_GCOVR_EXCL_LINE; } @@ -56,7 +56,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager) + Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager, bool is_delta) : route_config_name_(rds.route_config_name()), factory_context_(factory_context), init_target_(fmt::format("RdsRouteConfigSubscription {}", route_config_name_), [this]() { subscription_->start({route_config_name_}); }), @@ -69,7 +69,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( rds.config_source(), Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), - *scope_, *this); + *scope_, *this, is_delta); config_update_info_ = std::make_unique( factory_context.timeSource(), factory_context.messageValidationVisitor()); @@ -225,7 +225,7 @@ RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& ad Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteConfigProvider( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - Init::Manager& init_manager) { + Init::Manager& init_manager, bool is_delta) { // RdsRouteConfigSubscriptions are unique based on their serialized RDS config. const uint64_t manager_identifier = MessageUtil::hash(rds); @@ -237,7 +237,7 @@ Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteCon // around it. However, since this is not a performance critical path we err on the side // of simplicity. subscription.reset(new RdsRouteConfigSubscription(rds, manager_identifier, factory_context, - stat_prefix, *this)); + stat_prefix, *this, is_delta)); init_manager.add(subscription->init_target_); route_config_subscriptions_.insert({manager_identifier, subscription}); } else { diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 418512bb6e50a..caf2340f084e5 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -48,7 +48,7 @@ class RouteConfigProviderUtil { create(const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& config, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - RouteConfigProviderManager& route_config_provider_manager); + RouteConfigProviderManager& route_config_provider_manager, bool is_delta); }; class RouteConfigProviderManagerImpl; @@ -132,8 +132,8 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, RdsRouteConfigSubscription( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, - RouteConfigProviderManagerImpl& route_config_provider_manager); + const std::string& stat_prefix, RouteConfigProviderManagerImpl& route_config_provider_manager, + bool is_delta); bool validateUpdateSize(int num_resources); @@ -207,7 +207,7 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, RouteConfigProviderPtr createRdsRouteConfigProvider( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - Init::Manager& init_manager) override; + Init::Manager& init_manager, bool is_delta) override; RouteConfigProviderPtr createStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config, diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index 9c710540930fc..d7ae71b134bcf 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -98,7 +98,7 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription( scoped_rds.scoped_rds_config_source(), Grpc::Common::typeUrl( envoy::api::v2::ScopedRouteConfiguration().GetDescriptor()->full_name()), - *scope_, *this); + *scope_, *this, true); initialize([scope_key_builder]() -> Envoy::Config::ConfigProvider::ConfigConstSharedPtr { return std::make_shared( @@ -115,7 +115,7 @@ ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper::RdsRouteConfigProvide route_provider_(static_cast( parent_.route_config_provider_manager_ .createRdsRouteConfigProvider(rds, parent_.factory_context_, parent_.stat_prefix_, - init_manager) + init_manager, true) .release())), rds_update_callback_handle_(route_provider_->subscription().addUpdateCallback([this]() { // Subscribe to RDS update. @@ -221,8 +221,10 @@ void ScopedRdsConfigSubscription::onConfigUpdate( // Pause RDS to not send a burst of RDS requests until we start all the new subscriptions. // In the case if factory_context_.initManager() is uninitialized, RDS is already paused either // by Server init or LDS init. - factory_context_.clusterManager().adsMux().pause( - Envoy::Config::TypeUrl::get().RouteConfiguration); + if (factory_context_.clusterManager().adsMux()) { + factory_context_.clusterManager().adsMux()->pause( + Envoy::Config::TypeUrl::get().RouteConfiguration); + } resume_rds = std::make_unique([this, &noop_init_manager, version_info] { // For new RDS subscriptions created after listener warming up, we don't wait for them to warm // up. @@ -234,8 +236,10 @@ void ScopedRdsConfigSubscription::onConfigUpdate( // New RDS subscriptions should have been created, now lift the floodgate. // Note in the case of partial acceptance, accepted RDS subscriptions should be started // despite of any error. - factory_context_.clusterManager().adsMux().resume( - Envoy::Config::TypeUrl::get().RouteConfiguration); + if (factory_context_.clusterManager().adsMux()) { + factory_context_.clusterManager().adsMux()->resume( + Envoy::Config::TypeUrl::get().RouteConfiguration); + } }); } std::vector exception_msgs; diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 93d304647f7dc..de67108a9e26c 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -23,11 +23,11 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, const std::string& stat_prefix, std::unordered_set& route_config_providers) : config_update_info_(config_update_info), - init_target_(fmt::format("VhdsConfigSubscription {}", config_update_info_->routeConfigName()), - [this]() { subscription_->start({}); }), scope_(factory_context.scope().createScope(stat_prefix + "vhds." + config_update_info_->routeConfigName() + ".")), stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), + init_target_(fmt::format("VhdsConfigSubscription {}", config_update_info_->routeConfigName()), + [this]() { subscription_->start({}); }), route_config_providers_(route_config_providers) { const auto& config_source = config_update_info_->routeConfiguration() .vhds() @@ -42,7 +42,7 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, 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); + *scope_, *this, /*is_delta=*/true); } void VhdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index a2d3cb6beb81e..639f97dd76f11 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -63,10 +63,10 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, } RouteConfigUpdatePtr& config_update_info_; - std::unique_ptr subscription_; - Init::TargetImpl init_target_; Stats::ScopePtr scope_; VhdsStats stats_; + std::unique_ptr subscription_; + Init::TargetImpl init_target_; std::unordered_set& route_config_providers_; }; diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 3d0a721c6aa15..22a0508bc56ae 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -568,7 +568,7 @@ void RtdsSubscription::start() { subscription_ = parent_.cm_->subscriptionFactory().subscriptionFromConfigSource( config_source_, Grpc::Common::typeUrl(envoy::service::discovery::v2::Runtime().GetDescriptor()->full_name()), - store_, *this); + store_, *this, /*is_delta=*/false); subscription_->start({resource_name_}); } diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 4514f5ce07922..3e3478f0beb7b 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -79,7 +79,7 @@ void SdsApi::initialize() { subscription_ = subscription_factory_.subscriptionFromConfigSource( sds_config_, Grpc::Common::typeUrl(envoy::api::v2::auth::Secret().GetDescriptor()->full_name()), stats_, - *this); + *this, /*is_delta=*/false); subscription_->start({sds_config_name_}); } diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index 838ec09721b7d..3269d58a2c3f5 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -15,19 +15,22 @@ namespace Envoy { namespace Upstream { -CdsApiPtr CdsApiImpl::create(const envoy::api::v2::core::ConfigSource& cds_config, +// TODO(fredlas) the is_delta argument can be removed upon delta+SotW ADS Envoy code unification. It +// is only actually needed to choose the grpc_method, which is irrelevant if ADS is used. +CdsApiPtr CdsApiImpl::create(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, ClusterManager& cm, Stats::Scope& scope, ProtobufMessage::ValidationVisitor& validation_visitor) { - return CdsApiPtr{new CdsApiImpl(cds_config, cm, scope, validation_visitor)}; + return CdsApiPtr{new CdsApiImpl(cds_config, is_delta, cm, scope, validation_visitor)}; } -CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm, - Stats::Scope& scope, ProtobufMessage::ValidationVisitor& validation_visitor) +CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, + ClusterManager& cm, Stats::Scope& scope, + ProtobufMessage::ValidationVisitor& validation_visitor) : cm_(cm), scope_(scope.createScope("cluster_manager.cds.")), validation_visitor_(validation_visitor) { subscription_ = cm_.subscriptionFactory().subscriptionFromConfigSource( cds_config, Grpc::Common::typeUrl(envoy::api::v2::Cluster().GetDescriptor()->full_name()), - *scope_, *this); + *scope_, *this, is_delta); } void CdsApiImpl::onConfigUpdate(const Protobuf::RepeatedPtrField& resources, @@ -56,8 +59,12 @@ void CdsApiImpl::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& system_version_info) { - cm_.adsMux().pause(Config::TypeUrl::get().ClusterLoadAssignment); - Cleanup eds_resume([this] { cm_.adsMux().resume(Config::TypeUrl::get().ClusterLoadAssignment); }); + std::unique_ptr maybe_eds_resume; + if (cm_.adsMux()) { + cm_.adsMux()->pause(Config::TypeUrl::get().ClusterLoadAssignment); + maybe_eds_resume = std::make_unique( + [this] { cm_.adsMux()->resume(Config::TypeUrl::get().ClusterLoadAssignment); }); + } ENVOY_LOG(info, "cds: add {} cluster(s), remove {} cluster(s)", added_resources.size(), removed_resources.size()); diff --git a/source/common/upstream/cds_api_impl.h b/source/common/upstream/cds_api_impl.h index b17d4bbc9989d..66bb7e4c3df8b 100644 --- a/source/common/upstream/cds_api_impl.h +++ b/source/common/upstream/cds_api_impl.h @@ -22,8 +22,8 @@ class CdsApiImpl : public CdsApi, Config::SubscriptionCallbacks, Logger::Loggable { public: - static CdsApiPtr create(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm, - Stats::Scope& scope, + static CdsApiPtr create(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, + ClusterManager& cm, Stats::Scope& scope, ProtobufMessage::ValidationVisitor& validation_visitor); // Upstream::CdsApi @@ -46,8 +46,9 @@ class CdsApiImpl : public CdsApi, return MessageUtil::anyConvert(resource).name(); } - CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm, - Stats::Scope& scope, ProtobufMessage::ValidationVisitor& validation_visitor); + CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, + ClusterManager& cm, Stats::Scope& scope, + ProtobufMessage::ValidationVisitor& validation_visitor); void runInitializeCallbackIfAny(); ClusterManager& cm_; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index c873db74fcf58..148cc2dc8e4be 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -18,6 +18,7 @@ #include "common/common/enum_to_int.h" #include "common/common/fmt.h" #include "common/common/utility.h" +#include "common/config/new_grpc_mux_impl.h" #include "common/config/resources.h" #include "common/config/utility.h" #include "common/grpc/async_client_manager_impl.h" @@ -137,16 +138,16 @@ void ClusterManagerInitHelper::maybeFinishInitialize() { // If the first CDS response doesn't have any primary cluster, ClusterLoadAssignment // should be already paused by CdsApiImpl::onConfigUpdate(). Need to check that to // avoid double pause ClusterLoadAssignment. - if (cm_.adsMux().paused(Config::TypeUrl::get().ClusterLoadAssignment)) { + if (cm_.adsMux() == nullptr || + cm_.adsMux()->paused(Config::TypeUrl::get().ClusterLoadAssignment)) { initializeSecondaryClusters(); } else { - cm_.adsMux().pause(Config::TypeUrl::get().ClusterLoadAssignment); + cm_.adsMux()->pause(Config::TypeUrl::get().ClusterLoadAssignment); Cleanup eds_resume( - [this] { cm_.adsMux().resume(Config::TypeUrl::get().ClusterLoadAssignment); }); + [this] { cm_.adsMux()->resume(Config::TypeUrl::get().ClusterLoadAssignment); }); initializeSecondaryClusters(); } } - return; } @@ -220,6 +221,33 @@ ClusterManagerImpl::ClusterManagerImpl( } } + // TODO(fredlas) HACK to support + // loadCluster->clusterFromProto->ClusterFactoryImplBase::create->EdsClusterFactory::createClusterImpl(), + // which wants to call xdsIsDelta() on us. So, we need to get our xds_is_delta_ defined before + // then. Once SotW and delta are unified, that is_delta bool will be gone from everywhere, and the + // xds_is_delta_ variable can be removed. + const auto& dyn_resources = bootstrap.dynamic_resources(); + if (dyn_resources.has_ads_config()) { + xds_is_delta_ = + dyn_resources.ads_config().api_type() == envoy::api::v2::core::ApiConfigSource::DELTA_GRPC; + } else if (dyn_resources.has_cds_config()) { + const auto& cds_config = dyn_resources.cds_config(); + xds_is_delta_ = + cds_config.api_config_source().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC || + (dyn_resources.has_ads_config() && dyn_resources.ads_config().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC); + } else if (dyn_resources.has_lds_config()) { + const auto& lds_config = dyn_resources.lds_config(); + xds_is_delta_ = + lds_config.api_config_source().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC || + (dyn_resources.has_ads_config() && dyn_resources.ads_config().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC); + } else { + xds_is_delta_ = false; + } + // Cluster loading happens in two phases: first all the primary clusters are loaded, and then all // the secondary clusters are loaded. As it currently stands all non-EDS clusters are primary and // only EDS clusters are secondary. This two phase loading is done because in v2 configuration @@ -233,18 +261,37 @@ ClusterManagerImpl::ClusterManagerImpl( } // Now setup ADS if needed, this might rely on a primary cluster. - if (bootstrap.dynamic_resources().has_ads_config()) { - ads_mux_ = std::make_unique( - local_info, - Config::Utility::factoryForGrpcApiConfigSource( - *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats) - ->create(), - main_thread_dispatcher, - *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( - "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), - random_, stats_, - Envoy::Config::Utility::parseRateLimitSettings(bootstrap.dynamic_resources().ads_config()), - bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only()); + // This is the only point where distinction between delta ADS and state-of-the-world ADS is made. + // After here, we just have a GrpcMux interface held in ads_mux_, which hides + // whether the backing implementation is delta or SotW. + if (dyn_resources.has_ads_config()) { + if (dyn_resources.ads_config().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC) { + auto& api_config_source = dyn_resources.has_ads_config() + ? dyn_resources.ads_config() + : dyn_resources.cds_config().api_config_source(); + ads_mux_ = std::make_shared( + Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, api_config_source, + stats) + ->create(), + main_thread_dispatcher, + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.service.discovery.v2.AggregatedDiscoveryService.DeltaAggregatedResources"), + random_, stats_, + Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info); + } else { + ads_mux_ = std::make_shared( + local_info, + Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, + dyn_resources.ads_config(), stats) + ->create(), + main_thread_dispatcher, + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), + random_, stats_, + Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), + bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only()); + } } else { ads_mux_ = std::make_unique(); } @@ -278,8 +325,8 @@ ClusterManagerImpl::ClusterManagerImpl( }); // We can now potentially create the CDS API once the backing cluster exists. - if (bootstrap.dynamic_resources().has_cds_config()) { - cds_api_ = factory_.createCds(bootstrap.dynamic_resources().cds_config(), *this); + if (dyn_resources.has_cds_config()) { + cds_api_ = factory_.createCds(dyn_resources.cds_config(), xds_is_delta_, *this); init_helper_.setCds(cds_api_.get()); } else { init_helper_.setCds(nullptr); @@ -1310,9 +1357,10 @@ std::pair ProdClusterManagerFactor } CdsApiPtr ProdClusterManagerFactory::createCds(const envoy::api::v2::core::ConfigSource& cds_config, - ClusterManager& cm) { + bool is_delta, ClusterManager& cm) { // TODO(htuch): Differentiate static vs. dynamic validation visitors. - return CdsApiImpl::create(cds_config, cm, stats_, validation_context_.dynamicValidationVisitor()); + return CdsApiImpl::create(cds_config, is_delta, cm, stats_, + validation_context_.dynamicValidationVisitor()); } } // namespace Upstream diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index cb45bb14aca76..3bb6846fd6417 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -68,7 +68,7 @@ class ProdClusterManagerFactory : public ClusterManagerFactory { std::pair clusterFromProto(const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api) override; - CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, + CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, ClusterManager& cm) override; Secret::SecretManager& secretManager() override { return secret_manager_; } @@ -226,7 +226,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggablefull_name()), - info_->statsScope(), *this); + info_->statsScope(), *this, is_delta); } void EdsClusterImpl::startPreInit() { subscription_->start({cluster_name_}); } @@ -268,8 +268,9 @@ EdsClusterFactory::createClusterImpl( } return std::make_pair( - std::make_shared(cluster, context.runtime(), socket_factory_context, - std::move(stats_scope), context.addedViaApi()), + std::make_unique(cluster, context.runtime(), socket_factory_context, + std::move(stats_scope), context.addedViaApi(), + context.clusterManager().xdsIsDelta()), nullptr); } diff --git a/source/common/upstream/eds.h b/source/common/upstream/eds.h index 0df5f4c844736..3f0a74b8dfca6 100644 --- a/source/common/upstream/eds.h +++ b/source/common/upstream/eds.h @@ -24,7 +24,7 @@ class EdsClusterImpl : public BaseDynamicClusterImpl, Config::SubscriptionCallba public: EdsClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Server::Configuration::TransportSocketFactoryContext& factory_context, - Stats::ScopePtr&& stats_scope, bool added_via_api); + Stats::ScopePtr&& stats_scope, bool added_via_api, bool is_delta); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Secondary; } diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 998dce6709ab3..b18316ad5217d 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -183,7 +183,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager:: kRouteConfig: route_config_provider_ = Router::RouteConfigProviderUtil::create( - config, context_, stats_prefix_, route_config_provider_manager_); + config, context_, stats_prefix_, route_config_provider_manager_, + context_.clusterManager().xdsIsDelta()); break; case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager:: kScopedRoutes: diff --git a/source/server/config_validation/cluster_manager.cc b/source/server/config_validation/cluster_manager.cc index c5fdc16db2761..54e125764a2fd 100644 --- a/source/server/config_validation/cluster_manager.cc +++ b/source/server/config_validation/cluster_manager.cc @@ -14,9 +14,9 @@ ClusterManagerPtr ValidationClusterManagerFactory::clusterManagerFromProto( CdsApiPtr ValidationClusterManagerFactory::createCds(const envoy::api::v2::core::ConfigSource& cds_config, - ClusterManager& cm) { + bool is_delta, ClusterManager& cm) { // Create the CdsApiImpl... - ProdClusterManagerFactory::createCds(cds_config, cm); + ProdClusterManagerFactory::createCds(cds_config, is_delta, cm); // ... and then throw it away, so that we don't actually connect to it. return nullptr; } diff --git a/source/server/config_validation/cluster_manager.h b/source/server/config_validation/cluster_manager.h index 5a7f299554750..7b8ed0efac6ca 100644 --- a/source/server/config_validation/cluster_manager.h +++ b/source/server/config_validation/cluster_manager.h @@ -38,7 +38,7 @@ class ValidationClusterManagerFactory : public ProdClusterManagerFactory { // Delegates to ProdClusterManagerFactory::createCds, but discards the result and returns nullptr // unconditionally. - CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, + CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource& cds_config, bool is_delta, ClusterManager& cm) override; private: diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index d447c58229406..58ad9affadc38 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -109,10 +109,11 @@ class ValidationInstance : Logger::Loggable, } // Server::ListenerComponentFactory - LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { - return std::make_unique(lds_config, clusterManager(), initManager(), stats(), - listenerManager(), - messageValidationContext().dynamicValidationVisitor()); + LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config, + bool is_delta) override { + return std::make_unique( + lds_config, clusterManager(), initManager(), stats(), listenerManager(), + messageValidationContext().dynamicValidationVisitor(), is_delta); } std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index d0cce626afb11..ca5cb3ed187d5 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -17,13 +17,13 @@ namespace Server { LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, Upstream::ClusterManager& cm, Init::Manager& init_manager, Stats::Scope& scope, ListenerManager& lm, - ProtobufMessage::ValidationVisitor& validation_visitor) + ProtobufMessage::ValidationVisitor& validation_visitor, bool is_delta) : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm), init_target_("LDS", [this]() { subscription_->start({}); }), validation_visitor_(validation_visitor) { subscription_ = cm.subscriptionFactory().subscriptionFromConfigSource( lds_config, Grpc::Common::typeUrl(envoy::api::v2::Listener().GetDescriptor()->full_name()), - *scope_, *this); + *scope_, *this, is_delta); init_manager.add(init_target_); } @@ -31,8 +31,12 @@ void LdsApiImpl::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& system_version_info) { - cm_.adsMux().pause(Config::TypeUrl::get().RouteConfiguration); - Cleanup rds_resume([this] { cm_.adsMux().resume(Config::TypeUrl::get().RouteConfiguration); }); + std::unique_ptr maybe_eds_resume; + if (cm_.adsMux()) { + cm_.adsMux()->pause(Config::TypeUrl::get().RouteConfiguration); + maybe_eds_resume = std::make_unique( + [this] { cm_.adsMux()->resume(Config::TypeUrl::get().RouteConfiguration); }); + } bool any_applied = false; // We do all listener removals before adding the new listeners. This allows adding a new listener diff --git a/source/server/lds_api.h b/source/server/lds_api.h index 24ca66cfe73f2..2ef9209542b2c 100644 --- a/source/server/lds_api.h +++ b/source/server/lds_api.h @@ -24,7 +24,7 @@ class LdsApiImpl : public LdsApi, public: LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, Upstream::ClusterManager& cm, Init::Manager& init_manager, Stats::Scope& scope, ListenerManager& lm, - ProtobufMessage::ValidationVisitor& validation_visitor); + ProtobufMessage::ValidationVisitor& validation_visitor, bool is_delta); // Server::LdsApi std::string versionInfo() const override { return system_version_info_; } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 599f471ea7fe3..ca00bc11082e8 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -59,10 +59,12 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, Configuration::ListenerFactoryContext& context); // Server::ListenerComponentFactory - LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { + LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config, + bool is_delta) override { return std::make_unique( lds_config, server_.clusterManager(), server_.initManager(), server_.stats(), - server_.listenerManager(), server_.messageValidationContext().dynamicValidationVisitor()); + server_.listenerManager(), server_.messageValidationContext().dynamicValidationVisitor(), + is_delta); } std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -127,9 +129,9 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable> listeners() override; uint64_t numConnections() override; diff --git a/source/server/server.cc b/source/server/server.cc index 9653cdc8e792d..1afeff023cfe6 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -402,7 +402,13 @@ void InstanceImpl::initialize(const Options& options, // Instruct the listener manager to create the LDS provider if needed. This must be done later // because various items do not yet exist when the listener manager is created. if (bootstrap_.dynamic_resources().has_lds_config()) { - listener_manager_->createLdsApi(bootstrap_.dynamic_resources().lds_config()); + const bool is_delta = + bootstrap_.dynamic_resources().lds_config().api_config_source().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC || + (bootstrap_.dynamic_resources().has_ads_config() && + bootstrap_.dynamic_resources().ads_config().api_type() == + envoy::api::v2::core::ApiConfigSource::DELTA_GRPC); + listener_manager_->createLdsApi(bootstrap_.dynamic_resources().lds_config(), is_delta); } // We have to defer RTDS initialization until after the cluster manager is @@ -516,14 +522,18 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch // Pause RDS to ensure that we don't send any requests until we've // subscribed to all the RDS resources. The subscriptions happen in the init callbacks, // so we pause RDS until we've completed all the callbacks. - cm.adsMux().pause(Config::TypeUrl::get().RouteConfiguration); + if (cm.adsMux()) { + cm.adsMux()->pause(Config::TypeUrl::get().RouteConfiguration); + } ENVOY_LOG(info, "all clusters initialized. initializing init manager"); init_manager.initialize(init_watcher_); // Now that we're execute all the init callbacks we can resume RDS // as we've subscribed to all the statically defined RDS resources. - cm.adsMux().resume(Config::TypeUrl::get().RouteConfiguration); + if (cm.adsMux()) { + cm.adsMux()->resume(Config::TypeUrl::get().RouteConfiguration); + } }); } diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 60101fa4aa22b..ad8eff35f1543 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -95,6 +95,30 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "new_grpc_mux_impl_test", + srcs = ["new_grpc_mux_impl_test.cc"], + deps = [ + "//source/common/config:new_grpc_mux_lib", + "//source/common/config:protobuf_link_hacks", + "//source/common/config:resources_lib", + "//source/common/protobuf", + "//source/common/stats:isolated_store_lib", + "//test/mocks:common_lib", + "//test/mocks/config:config_mocks", + "//test/mocks/event:event_mocks", + "//test/mocks/grpc:grpc_mocks", + "//test/mocks/local_info:local_info_mocks", + "//test/mocks/runtime:runtime_mocks", + "//test/test_common:logging_lib", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/api/v2:discovery_cc", + "@envoy_api//envoy/api/v2:eds_cc", + "@envoy_api//envoy/service/discovery/v2:ads_cc", + ], +) + envoy_cc_test( name = "grpc_stream_test", srcs = ["grpc_stream_test.cc"], diff --git a/test/common/config/delta_subscription_impl_test.cc b/test/common/config/delta_subscription_impl_test.cc index 26d7daca0df39..f8127f93cfdff 100644 --- a/test/common/config/delta_subscription_impl_test.cc +++ b/test/common/config/delta_subscription_impl_test.cc @@ -9,20 +9,25 @@ namespace { class DeltaSubscriptionImplTest : public DeltaSubscriptionTestHarness, public testing::Test { protected: DeltaSubscriptionImplTest() = default; + + // We need to destroy the subscription before the test's destruction, because the subscription's + // destructor removes its watch from the NewGrpcMuxImpl, and that removal process involves + // some things held by the test fixture. + void TearDown() override { doSubscriptionTearDown(); } }; TEST_F(DeltaSubscriptionImplTest, UpdateResourcesCausesRequest) { startSubscription({"name1", "name2", "name3"}); expectSendMessage({"name4"}, {"name1", "name2"}, Grpc::Status::GrpcStatus::Ok, "", {}); - subscription_->updateResources({"name3", "name4"}); + subscription_->updateResourceInterest({"name3", "name4"}); expectSendMessage({"name1", "name2"}, {}, Grpc::Status::GrpcStatus::Ok, "", {}); - subscription_->updateResources({"name1", "name2", "name3", "name4"}); + subscription_->updateResourceInterest({"name1", "name2", "name3", "name4"}); expectSendMessage({}, {"name1", "name2"}, Grpc::Status::GrpcStatus::Ok, "", {}); - subscription_->updateResources({"name3", "name4"}); + subscription_->updateResourceInterest({"name3", "name4"}); expectSendMessage({"name1", "name2"}, {}, Grpc::Status::GrpcStatus::Ok, "", {}); - subscription_->updateResources({"name1", "name2", "name3", "name4"}); + subscription_->updateResourceInterest({"name1", "name2", "name3", "name4"}); expectSendMessage({}, {"name1", "name2", "name3"}, Grpc::Status::GrpcStatus::Ok, "", {}); - subscription_->updateResources({"name4"}); + subscription_->updateResourceInterest({"name4"}); } // Checks that after a pause(), no requests are sent until resume(). @@ -36,11 +41,11 @@ TEST_F(DeltaSubscriptionImplTest, PauseHoldsRequest) { expectSendMessage({"name4"}, {"name1", "name2"}, Grpc::Status::GrpcStatus::Ok, "", {}); // If not for the pause, these updates would make the expectSendMessage fail due to too many // messages being sent. - subscription_->updateResources({"name3", "name4"}); - subscription_->updateResources({"name1", "name2", "name3", "name4"}); - subscription_->updateResources({"name3", "name4"}); - subscription_->updateResources({"name1", "name2", "name3", "name4"}); - subscription_->updateResources({"name3", "name4"}); + subscription_->updateResourceInterest({"name3", "name4"}); + subscription_->updateResourceInterest({"name1", "name2", "name3", "name4"}); + subscription_->updateResourceInterest({"name3", "name4"}); + subscription_->updateResourceInterest({"name1", "name2", "name3", "name4"}); + subscription_->updateResourceInterest({"name3", "name4"}); subscription_->resume(); } @@ -64,8 +69,10 @@ TEST_F(DeltaSubscriptionImplTest, PauseQueuesAcks) { resource->set_version("version1A"); const std::string nonce = std::to_string(HashUtil::xxHash64("version1A")); message->set_nonce(nonce); + message->set_type_url(Config::TypeUrl::get().ClusterLoadAssignment); nonce_acks_required_.push(nonce); - subscription_->onDiscoveryResponse(std::move(message)); + static_cast(subscription_->getContextForTest().get()) + ->onDiscoveryResponse(std::move(message)); } // The server gives us our first version of resource name2. // subscription_ now wants to ACK name1 and then name2 (but can't due to pause). @@ -76,8 +83,10 @@ TEST_F(DeltaSubscriptionImplTest, PauseQueuesAcks) { resource->set_version("version2A"); const std::string nonce = std::to_string(HashUtil::xxHash64("version2A")); message->set_nonce(nonce); + message->set_type_url(Config::TypeUrl::get().ClusterLoadAssignment); nonce_acks_required_.push(nonce); - subscription_->onDiscoveryResponse(std::move(message)); + static_cast(subscription_->getContextForTest().get()) + ->onDiscoveryResponse(std::move(message)); } // The server gives us an updated version of resource name1. // subscription_ now wants to ACK name1A, then name2, then name1B (but can't due to pause). @@ -88,8 +97,10 @@ TEST_F(DeltaSubscriptionImplTest, PauseQueuesAcks) { resource->set_version("version1B"); const std::string nonce = std::to_string(HashUtil::xxHash64("version1B")); message->set_nonce(nonce); + message->set_type_url(Config::TypeUrl::get().ClusterLoadAssignment); nonce_acks_required_.push(nonce); - subscription_->onDiscoveryResponse(std::move(message)); + static_cast(subscription_->getContextForTest().get()) + ->onDiscoveryResponse(std::move(message)); } // All ACK sendMessage()s will happen upon calling resume(). EXPECT_CALL(async_stream_, sendMessageRaw_(_, _)) @@ -106,13 +117,36 @@ TEST_F(DeltaSubscriptionImplTest, PauseQueuesAcks) { // in the correct order. } -TEST_F(DeltaSubscriptionImplTest, NoGrpcStream) { - // Have to call start() to get state_ populated (which this test needs to not segfault), but - // start() also tries to start the GrpcStream. So, have that attempt return nullptr. - EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(nullptr)); - EXPECT_CALL(async_stream_, sendMessageRaw_(_, _)).Times(0); - subscription_->start({"name1"}); - subscription_->updateResources({"name1", "name2"}); +TEST(DeltaSubscriptionImplFixturelessTest, NoGrpcStream) { + Stats::IsolatedStoreImpl stats_store; + SubscriptionStats stats(Utility::generateStats(stats_store)); + + envoy::api::v2::core::Node node; + node.set_id("fo0"); + NiceMock local_info; + EXPECT_CALL(local_info, node()).WillRepeatedly(testing::ReturnRef(node)); + + NiceMock dispatcher; + NiceMock random; + Envoy::Config::RateLimitSettings rate_limit_settings; + NiceMock> callbacks; + auto* async_client = new Grpc::MockAsyncClient(); + + const Protobuf::MethodDescriptor* method_descriptor = + Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints"); + std::shared_ptr xds_context = std::make_shared( + std::unique_ptr(async_client), dispatcher, *method_descriptor, random, + stats_store, rate_limit_settings, local_info); + + std::unique_ptr subscription = std::make_unique( + xds_context, Config::TypeUrl::get().ClusterLoadAssignment, callbacks, stats, + std::chrono::milliseconds(12345), false); + + EXPECT_CALL(*async_client, startRaw(_, _, _)).WillOnce(Return(nullptr)); + + subscription->start({"name1"}); + subscription->updateResourceInterest({"name1", "name2"}); } } // namespace diff --git a/test/common/config/delta_subscription_state_test.cc b/test/common/config/delta_subscription_state_test.cc index 462a91e2184d3..2a96f2b0953f2 100644 --- a/test/common/config/delta_subscription_state_test.cc +++ b/test/common/config/delta_subscription_state_test.cc @@ -22,10 +22,9 @@ const char TypeUrl[] = "type.googleapis.com/envoy.api.v2.Cluster"; class DeltaSubscriptionStateTest : public testing::Test { protected: DeltaSubscriptionStateTest() - : stats_(Utility::generateStats(store_)), - state_(TypeUrl, {"name1", "name2", "name3"}, callbacks_, local_info_, - std::chrono::milliseconds(0U), dispatcher_, stats_) { - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + : state_(TypeUrl, callbacks_, local_info_, std::chrono::milliseconds(0U), dispatcher_) { + state_.updateSubscriptionInterest({"name1", "name2", "name3"}, {}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name1", "name2", "name3")); } @@ -62,8 +61,6 @@ class DeltaSubscriptionStateTest : public testing::Test { NiceMock> callbacks_; NiceMock local_info_; NiceMock dispatcher_; - Stats::IsolatedStoreImpl store_; - SubscriptionStats stats_; // We start out interested in three resources: name1, name2, and name3. DeltaSubscriptionState state_; }; @@ -82,14 +79,14 @@ populateRepeatedResource(std::vector> items) // Basic gaining/losing interest in resources should lead to (un)subscriptions. TEST_F(DeltaSubscriptionStateTest, SubscribeAndUnsubscribe) { { - state_.updateResourceInterest({"name2", "name3", "name4"}); // drop name1, add name4 - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {"name1"}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name4")); EXPECT_THAT(cur_request.resource_names_unsubscribe(), UnorderedElementsAre("name1")); } { - state_.updateResourceInterest({"name1", "name2"}); // add back name1, drop name3 and 4 - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name1"}, {"name3", "name4"}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name1")); EXPECT_THAT(cur_request.resource_names_unsubscribe(), UnorderedElementsAre("name3", "name4")); } @@ -105,9 +102,9 @@ TEST_F(DeltaSubscriptionStateTest, SubscribeAndUnsubscribe) { // interpret the resource_names_subscribe field as "send these resources even if you think Envoy // already has them". TEST_F(DeltaSubscriptionStateTest, RemoveThenAdd) { - state_.updateResourceInterest({"name1", "name2"}); - state_.updateResourceInterest({"name1", "name2", "name3"}); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({}, {"name3"}); + state_.updateSubscriptionInterest({"name3"}, {}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name3")); EXPECT_TRUE(cur_request.resource_names_unsubscribe().empty()); } @@ -121,29 +118,29 @@ TEST_F(DeltaSubscriptionStateTest, RemoveThenAdd) { // should be like this. What *is* important: the server must happily and cleanly ignore // "unsubscribe from [resource name I have never before referred to]" requests. TEST_F(DeltaSubscriptionStateTest, AddThenRemove) { - state_.updateResourceInterest({"name1", "name2", "name3", "name4"}); - state_.updateResourceInterest({"name1", "name2", "name3"}); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {}); + state_.updateSubscriptionInterest({}, {"name4"}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_TRUE(cur_request.resource_names_subscribe().empty()); EXPECT_THAT(cur_request.resource_names_unsubscribe(), UnorderedElementsAre("name4")); } // add/remove/add == add. TEST_F(DeltaSubscriptionStateTest, AddRemoveAdd) { - state_.updateResourceInterest({"name1", "name2", "name3", "name4"}); - state_.updateResourceInterest({"name1", "name2", "name3"}); - state_.updateResourceInterest({"name1", "name2", "name3", "name4"}); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {}); + state_.updateSubscriptionInterest({}, {"name4"}); + state_.updateSubscriptionInterest({"name4"}, {}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name4")); EXPECT_TRUE(cur_request.resource_names_unsubscribe().empty()); } // remove/add/remove == remove. TEST_F(DeltaSubscriptionStateTest, RemoveAddRemove) { - state_.updateResourceInterest({"name1", "name2"}); - state_.updateResourceInterest({"name1", "name2", "name3"}); - state_.updateResourceInterest({"name1", "name2"}); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({}, {"name3"}); + state_.updateSubscriptionInterest({"name3"}, {}); + state_.updateSubscriptionInterest({}, {"name3"}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_TRUE(cur_request.resource_names_subscribe().empty()); EXPECT_THAT(cur_request.resource_names_unsubscribe(), UnorderedElementsAre("name3")); } @@ -151,19 +148,19 @@ TEST_F(DeltaSubscriptionStateTest, RemoveAddRemove) { // Starts with 1,2,3. 4 is added/removed/added. In those same updates, 1,2,3 are // removed/added/removed. End result should be 4 added and 1,2,3 removed. TEST_F(DeltaSubscriptionStateTest, BothAddAndRemove) { - state_.updateResourceInterest({"name4"}); - state_.updateResourceInterest({"name1", "name2", "name3"}); - state_.updateResourceInterest({"name4"}); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {"name1", "name2", "name3"}); + state_.updateSubscriptionInterest({"name1", "name2", "name3"}, {"name4"}); + state_.updateSubscriptionInterest({"name4"}, {"name1", "name2", "name3"}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name4")); EXPECT_THAT(cur_request.resource_names_unsubscribe(), UnorderedElementsAre("name1", "name2", "name3")); } TEST_F(DeltaSubscriptionStateTest, CumulativeUpdates) { - state_.updateResourceInterest({"name1", "name2", "name3", "name4"}); - state_.updateResourceInterest({"name1", "name2", "name3", "name4", "name5"}); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {}); + state_.updateSubscriptionInterest({"name5"}, {}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name4", "name5")); EXPECT_TRUE(cur_request.resource_names_unsubscribe().empty()); } @@ -217,7 +214,7 @@ TEST_F(DeltaSubscriptionStateTest, ResourceGoneLeadsToBlankInitialVersion) { populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); deliverDiscoveryResponse(add1_2, {}, "debugversion1"); state_.markStreamFresh(); // simulate a stream reconnection - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_EQ("version1A", cur_request.initial_resource_versions().at("name1")); EXPECT_EQ("version2A", cur_request.initial_resource_versions().at("name2")); EXPECT_EQ(cur_request.initial_resource_versions().end(), @@ -232,7 +229,7 @@ TEST_F(DeltaSubscriptionStateTest, ResourceGoneLeadsToBlankInitialVersion) { *remove2.Add() = "name2"; deliverDiscoveryResponse(add1_3, remove2, "debugversion2"); state_.markStreamFresh(); // simulate a stream reconnection - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_EQ("version1B", cur_request.initial_resource_versions().at("name1")); EXPECT_EQ(cur_request.initial_resource_versions().end(), cur_request.initial_resource_versions().find("name2")); @@ -246,15 +243,15 @@ TEST_F(DeltaSubscriptionStateTest, ResourceGoneLeadsToBlankInitialVersion) { *remove1_3.Add() = "name3"; deliverDiscoveryResponse({}, remove1_3, "debugversion3"); state_.markStreamFresh(); // simulate a stream reconnection - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_TRUE(cur_request.initial_resource_versions().empty()); } { // ...but our own map should remember our interest. In particular, losing interest in a // resource should cause its name to appear in the next request's resource_names_unsubscribe. - state_.updateResourceInterest({"name3", "name4"}); // note the lack of 1 and 2 - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {"name1", "name2"}); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_THAT(cur_request.resource_names_subscribe(), UnorderedElementsAre("name4")); EXPECT_THAT(cur_request.resource_names_unsubscribe(), UnorderedElementsAre("name1", "name2")); } @@ -273,9 +270,9 @@ TEST_F(DeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnect) { populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); deliverDiscoveryResponse(add1_2, {}, "debugversion1"); - state_.updateResourceInterest({"name2", "name3", "name4"}); // drop name1, add name4 - state_.markStreamFresh(); // simulate a stream reconnection - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + state_.updateSubscriptionInterest({"name4"}, {"name1"}); + state_.markStreamFresh(); // simulate a stream reconnection + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); // Regarding the resource_names_subscribe field: // name1: do not include: we lost interest. // name2: yes do include: we're interested and we have a version of it. @@ -296,7 +293,7 @@ TEST_F(DeltaSubscriptionStateTest, InitialVersionMapFirstMessageOnly) { {{"name1", "version1A"}, {"name2", "version2A"}, {"name3", "version3A"}}); deliverDiscoveryResponse(add_all, {}, "debugversion1"); state_.markStreamFresh(); // simulate a stream reconnection - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_EQ("version1A", cur_request.initial_resource_versions().at("name1")); EXPECT_EQ("version2A", cur_request.initial_resource_versions().at("name2")); EXPECT_EQ("version3A", cur_request.initial_resource_versions().at("name3")); @@ -304,7 +301,7 @@ TEST_F(DeltaSubscriptionStateTest, InitialVersionMapFirstMessageOnly) { // Then, after updating the resources but not reconnecting the stream, verify that initial // versions are not sent. { - state_.updateResourceInterest({"name1", "name2", "name3", "name4"}); + state_.updateSubscriptionInterest({"name4"}, {}); // The xDS server updates our resources, and gives us our newly requested one too. Protobuf::RepeatedPtrField add_all = populateRepeatedResource({{"name1", "version1B"}, @@ -312,7 +309,7 @@ TEST_F(DeltaSubscriptionStateTest, InitialVersionMapFirstMessageOnly) { {"name3", "version3B"}, {"name4", "version4A"}}); deliverDiscoveryResponse(add_all, {}, "debugversion2"); - envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequest(); + envoy::api::v2::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); EXPECT_TRUE(cur_request.initial_resource_versions().empty()); } } @@ -321,28 +318,16 @@ TEST_F(DeltaSubscriptionStateTest, CheckUpdatePending) { // Note that the test fixture ctor causes the first request to be "sent", so we start in the // middle of a stream, with our initially interested resources having been requested already. EXPECT_FALSE(state_.subscriptionUpdatePending()); - state_.updateResourceInterest({"name1", "name2", "name3"}); // no change + state_.updateSubscriptionInterest({}, {}); // no change EXPECT_FALSE(state_.subscriptionUpdatePending()); state_.markStreamFresh(); - EXPECT_TRUE(state_.subscriptionUpdatePending()); // no change, BUT fresh stream - state_.updateResourceInterest({"name1", "name2"}); // one removed + EXPECT_TRUE(state_.subscriptionUpdatePending()); // no change, BUT fresh stream + state_.updateSubscriptionInterest({}, {"name3"}); // one removed EXPECT_TRUE(state_.subscriptionUpdatePending()); - state_.updateResourceInterest({"name1", "name2", "name3"}); // one added + state_.updateSubscriptionInterest({"name3"}, {}); // one added EXPECT_TRUE(state_.subscriptionUpdatePending()); } -TEST_F(DeltaSubscriptionStateTest, PauseAndResume) { - EXPECT_FALSE(state_.paused()); - state_.pause(); - EXPECT_TRUE(state_.paused()); - state_.resume(); - EXPECT_FALSE(state_.paused()); - state_.pause(); - EXPECT_TRUE(state_.paused()); - state_.resume(); - EXPECT_FALSE(state_.paused()); -} - // The next three tests test that duplicate resource names (whether additions or removals) cause // DeltaSubscriptionState to reject the update without even trying to hand it to the consuming API's // onConfigUpdate(). @@ -374,14 +359,6 @@ TEST_F(DeltaSubscriptionStateTest, AddedAndRemoved) { ack.error_detail_.message()); } -TEST_F(DeltaSubscriptionStateTest, handleEstablishmentFailure) { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)).Times(0); - - state_.handleEstablishmentFailure(); - EXPECT_EQ(stats_.update_failure_.value(), 1); - EXPECT_EQ(stats_.update_attempt_.value(), 1); -} - } // namespace } // namespace Config } // namespace Envoy diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index 4414aa77489c5..35e8a4bbef060 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "common/config/delta_subscription_impl.h" #include "common/grpc/common.h" @@ -32,15 +34,30 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { node_.set_id("fo0"); EXPECT_CALL(local_info_, node()).WillRepeatedly(testing::ReturnRef(node_)); EXPECT_CALL(dispatcher_, createTimer_(_)); + xds_context_ = std::make_shared( + std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, + random_, stats_store_, rate_limit_settings_, local_info_); subscription_ = std::make_unique( - local_info_, std::unique_ptr(async_client_), dispatcher_, - *method_descriptor_, Config::TypeUrl::get().ClusterLoadAssignment, random_, stats_store_, - rate_limit_settings_, callbacks_, stats_, init_fetch_timeout); + xds_context_, Config::TypeUrl::get().ClusterLoadAssignment, callbacks_, stats_, + init_fetch_timeout, false); + EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(&async_stream_)); + } + + void doSubscriptionTearDown() override { + if (subscription_started_) { + EXPECT_CALL(async_stream_, sendMessageRaw_(_, _)); + subscription_.reset(); + } } ~DeltaSubscriptionTestHarness() override { while (!nonce_acks_required_.empty()) { - EXPECT_FALSE(nonce_acks_sent_.empty()); + if (nonce_acks_sent_.empty()) { + // It's not enough to EXPECT_FALSE(nonce_acks_sent_.empty()), we need to skip the following + // EXPECT_EQ, otherwise the undefined .front() can get pretty bad. + EXPECT_FALSE(nonce_acks_sent_.empty()); + break; + } EXPECT_EQ(nonce_acks_required_.front(), nonce_acks_sent_.front()); nonce_acks_required_.pop(); nonce_acks_sent_.pop(); @@ -49,7 +66,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { } void startSubscription(const std::set& cluster_names) override { - EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(&async_stream_)); + subscription_started_ = true; last_cluster_names_ = cluster_names; expectSendMessage(last_cluster_names_, ""); subscription_->start(cluster_names); @@ -104,12 +121,11 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { void deliverConfigUpdate(const std::vector& cluster_names, const std::string& version, bool accept) override { - std::unique_ptr response( - new envoy::api::v2::DeltaDiscoveryResponse()); - + auto response = std::make_unique(); last_response_nonce_ = std::to_string(HashUtil::xxHash64(version)); response->set_nonce(last_response_nonce_); response->set_system_version_info(version); + response->set_type_url(Config::TypeUrl::get().ClusterLoadAssignment); Protobuf::RepeatedPtrField typed_resources; for (const auto& cluster : cluster_names) { @@ -132,11 +148,12 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)); expectSendMessage({}, {}, Grpc::Status::GrpcStatus::Internal, "bad config", {}); } - subscription_->onDiscoveryResponse(std::move(response)); + static_cast(subscription_->getContextForTest().get()) + ->onDiscoveryResponse(std::move(response)); Mock::VerifyAndClearExpectations(&async_stream_); } - void updateResources(const std::set& cluster_names) override { + void updateResourceInterest(const std::set& cluster_names) override { std::set sub; std::set unsub; @@ -147,7 +164,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { std::inserter(unsub, unsub.begin())); expectSendMessage(sub, unsub, Grpc::Status::GrpcStatus::Ok, "", {}); - subscription_->updateResources(cluster_names); + subscription_->updateResourceInterest(cluster_names); last_cluster_names_ = cluster_names; } @@ -157,7 +174,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds timeout) override { init_timeout_timer_ = new Event::MockTimer(&dispatcher_); - EXPECT_CALL(*init_timeout_timer_, enableTimer(std::chrono::milliseconds(timeout), _)); + EXPECT_CALL(*init_timeout_timer_, enableTimer(timeout, _)); } void expectDisableInitFetchTimeoutTimer() override { @@ -172,6 +189,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { NiceMock random_; NiceMock local_info_; Grpc::MockAsyncStream async_stream_; + std::shared_ptr xds_context_; std::unique_ptr subscription_; std::string last_response_nonce_; std::set last_cluster_names_; @@ -181,6 +199,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { NiceMock> callbacks_; std::queue nonce_acks_required_; std::queue nonce_acks_sent_; + bool subscription_started_{}; }; } // namespace diff --git a/test/common/config/filesystem_subscription_test_harness.h b/test/common/config/filesystem_subscription_test_harness.h index 901b5cf29b311..1958a4270bd69 100644 --- a/test/common/config/filesystem_subscription_test_harness.h +++ b/test/common/config/filesystem_subscription_test_harness.h @@ -43,11 +43,11 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { subscription_.start(cluster_names); } - void updateResources(const std::set& cluster_names) override { - subscription_.updateResources(cluster_names); + void updateResourceInterest(const std::set& cluster_names) override { + subscription_.updateResourceInterest(cluster_names); } - void updateFile(const std::string json, bool run_dispatcher = true) { + void updateFile(const std::string& json, bool run_dispatcher = true) { // Write JSON contents to file, rename to path_ and run dispatcher to catch // inotify. const std::string temp_path = TestEnvironment::writeStringToFileForTest("eds.json.tmp", json); @@ -94,13 +94,10 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { version); } - void expectConfigUpdateFailed() override { - // initial_fetch_timeout not implemented - } + void expectConfigUpdateFailed() override { stats_.update_failure_.inc(); } - void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds timeout) override { - UNREFERENCED_PARAMETER(timeout); - // initial_fetch_timeout not implemented + void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds) override { + // initial_fetch_timeout not implemented. } void expectDisableInitFetchTimeoutTimer() override { diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index b266dda51b0c8..af27a0ba61923 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -89,7 +89,7 @@ class GrpcMuxImplTestBase : public testing::Test { } NiceMock dispatcher_; - Runtime::MockRandomGenerator random_; + NiceMock random_; Grpc::MockAsyncClient* async_client_; Grpc::MockAsyncStream async_stream_; std::unique_ptr grpc_mux_; @@ -213,13 +213,15 @@ TEST_F(GrpcMuxImplTest, TypeUrlMismatch) { invalid_response->mutable_resources()->Add()->set_type_url("bar"); EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)) .WillOnce(Invoke([](Envoy::Config::ConfigUpdateFailureReason, const EnvoyException* e) { - EXPECT_TRUE(IsSubstring("", "", "bar does not match foo type URL in DiscoveryResponse", - e->what())); + EXPECT_TRUE(IsSubstring( + "", "", "bar does not match the message-wide type URL foo in DiscoveryResponse", + e->what())); })); - expectSendMessage("foo", {"x", "y"}, "", false, "", Grpc::Status::GrpcStatus::Internal, - fmt::format("bar does not match foo type URL in DiscoveryResponse {}", - invalid_response->DebugString())); + expectSendMessage( + "foo", {"x", "y"}, "", false, "", Grpc::Status::GrpcStatus::Internal, + fmt::format("bar does not match the message-wide type URL foo in DiscoveryResponse {}", + invalid_response->DebugString())); grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(invalid_response)); } expectSendMessage("foo", {}, ""); diff --git a/test/common/config/grpc_stream_test.cc b/test/common/config/grpc_stream_test.cc index 783f188eefd62..e54f901ddee64 100644 --- a/test/common/config/grpc_stream_test.cc +++ b/test/common/config/grpc_stream_test.cc @@ -43,7 +43,7 @@ class GrpcStreamTest : public testing::Test { // Tests that establishNewStream() establishes it, a second call does nothing, and a third call // after the stream was disconnected re-establishes it. -TEST_F(GrpcStreamTest, EstablishNewStream) { +TEST_F(GrpcStreamTest, EstablishStream) { EXPECT_FALSE(grpc_stream_.grpcStreamAvailable()); // Successful establishment { @@ -52,7 +52,7 @@ TEST_F(GrpcStreamTest, EstablishNewStream) { grpc_stream_.establishNewStream(); EXPECT_TRUE(grpc_stream_.grpcStreamAvailable()); } - // Idempotency: do nothing (other than logging a warning) if already connected + // Idempotent { EXPECT_CALL(*async_client_, startRaw(_, _, _)).Times(0); EXPECT_CALL(callbacks_, onStreamEstablished()).Times(0); diff --git a/test/common/config/grpc_subscription_impl_test.cc b/test/common/config/grpc_subscription_impl_test.cc index e79995b28f39a..56c86c18dca61 100644 --- a/test/common/config/grpc_subscription_impl_test.cc +++ b/test/common/config/grpc_subscription_impl_test.cc @@ -25,7 +25,7 @@ TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { EXPECT_TRUE(statsAre(2, 0, 0, 1, 0, 0)); // Ensure this doesn't cause an issue by sending a request, since we don't // have a gRPC stream. - subscription_->updateResources({"cluster2"}); + subscription_->updateResourceInterest({"cluster2"}); // Retry and succeed. EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(&async_stream_)); @@ -46,8 +46,8 @@ TEST_F(GrpcSubscriptionImplTest, RemoteStreamClose) { .Times(0); EXPECT_CALL(*timer_, enableTimer(_, _)); EXPECT_CALL(random_, random()); - subscription_->grpcMux().grpcStreamForTest().onRemoteClose(Grpc::Status::GrpcStatus::Canceled, - ""); + subscription_->grpcMux()->grpcStreamForTest().onRemoteClose(Grpc::Status::GrpcStatus::Canceled, + ""); EXPECT_TRUE(statsAre(2, 0, 0, 1, 0, 0)); verifyControlPlaneStats(0); @@ -65,14 +65,14 @@ TEST_F(GrpcSubscriptionImplTest, RepeatedNonce) { startSubscription({"cluster0", "cluster1"}); EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); // First with the initial, empty version update to "0". - updateResources({"cluster2"}); + updateResourceInterest({"cluster2"}); EXPECT_TRUE(statsAre(2, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster2"}, "0", false); EXPECT_TRUE(statsAre(3, 0, 1, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster2"}, "0", true); EXPECT_TRUE(statsAre(4, 1, 1, 0, 0, 7148434200721666028)); // Now with version "0" update to "1". - updateResources({"cluster3"}); + updateResourceInterest({"cluster3"}); EXPECT_TRUE(statsAre(5, 1, 1, 0, 0, 7148434200721666028)); deliverConfigUpdate({"cluster3"}, "1", false); EXPECT_TRUE(statsAre(6, 1, 2, 0, 0, 7148434200721666028)); diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index 9e68838ee6460..fb985f68ed7dc 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -114,11 +114,11 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { expectSendMessage(last_cluster_names_, version_, false, Grpc::Status::GrpcStatus::Internal, "bad config"); } - subscription_->grpcMux().onDiscoveryResponse(std::move(response)); + subscription_->grpcMux()->onDiscoveryResponse(std::move(response)); Mock::VerifyAndClearExpectations(&async_stream_); } - void updateResources(const std::set& cluster_names) override { + void updateResourceInterest(const std::set& cluster_names) override { // The "watch" mechanism means that updates that lose interest in a resource // will first generate a request for [still watched resources, i.e. without newly unwatched // ones] before generating the request for all of cluster_names. @@ -132,17 +132,22 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { } expectSendMessage(both, version_); expectSendMessage(cluster_names, version_); - subscription_->updateResources(cluster_names); + subscription_->updateResourceInterest(cluster_names); last_cluster_names_ = cluster_names; } void expectConfigUpdateFailed() override { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, nullptr)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, nullptr)) + .WillOnce([this](ConfigUpdateFailureReason reason, const EnvoyException*) { + if (reason == ConfigUpdateFailureReason::FetchTimedout) { + stats_.init_fetch_timeout_.inc(); + } + }); } void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds timeout) override { init_timeout_timer_ = new Event::MockTimer(&dispatcher_); - EXPECT_CALL(*init_timeout_timer_, enableTimer(std::chrono::milliseconds(timeout), _)); + EXPECT_CALL(*init_timeout_timer_, enableTimer(timeout, _)); } void expectDisableInitFetchTimeoutTimer() override { diff --git a/test/common/config/http_subscription_test_harness.h b/test/common/config/http_subscription_test_harness.h index 1d41ee38019cb..1f00c95cd87da 100644 --- a/test/common/config/http_subscription_test_harness.h +++ b/test/common/config/http_subscription_test_harness.h @@ -106,10 +106,10 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { subscription_->start(cluster_names); } - void updateResources(const std::set& cluster_names) override { + void updateResourceInterest(const std::set& cluster_names) override { cluster_names_ = cluster_names; expectSendMessage(cluster_names, version_); - subscription_->updateResources(cluster_names); + subscription_->updateResourceInterest(cluster_names); timer_cb_(); } diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc new file mode 100644 index 0000000000000..98150c9b7915a --- /dev/null +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -0,0 +1,111 @@ +#include + +#include "envoy/api/v2/discovery.pb.h" +#include "envoy/api/v2/eds.pb.h" + +#include "common/common/empty_string.h" +#include "common/config/new_grpc_mux_impl.h" +#include "common/config/protobuf_link_hacks.h" +#include "common/config/resources.h" +#include "common/config/utility.h" +#include "common/protobuf/protobuf.h" +#include "common/stats/isolated_store_impl.h" + +#include "test/mocks/common.h" +#include "test/mocks/config/mocks.h" +#include "test/mocks/event/mocks.h" +#include "test/mocks/grpc/mocks.h" +#include "test/mocks/local_info/mocks.h" +#include "test/mocks/runtime/mocks.h" +#include "test/test_common/logging.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_time.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Invoke; +using testing::NiceMock; +using testing::Return; + +namespace Envoy { +namespace Config { +namespace { + +// We test some mux specific stuff below, other unit test coverage for singleton use of +// NewGrpcMuxImpl is provided in [grpc_]subscription_impl_test.cc. +class NewGrpcMuxImplTestBase : public testing::Test { +public: + NewGrpcMuxImplTestBase() + : async_client_(new Grpc::MockAsyncClient()), + control_plane_connected_state_( + stats_.gauge("control_plane.connected_state", Stats::Gauge::ImportMode::NeverImport)) {} + + void setup() { + grpc_mux_ = std::make_unique( + std::unique_ptr(async_client_), dispatcher_, + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( + "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), + random_, stats_, rate_limit_settings_, local_info_); + } + + NiceMock dispatcher_; + NiceMock random_; + Grpc::MockAsyncClient* async_client_; + NiceMock async_stream_; + std::unique_ptr grpc_mux_; + NiceMock> callbacks_; + NiceMock local_info_; + Stats::IsolatedStoreImpl stats_; + Envoy::Config::RateLimitSettings rate_limit_settings_; + Stats::Gauge& control_plane_connected_state_; +}; + +class NewGrpcMuxImplTest : public NewGrpcMuxImplTestBase { +public: + Event::SimulatedTimeSystem time_system_; +}; + +// Test that we simply ignore a message for an unknown type_url, with no ill effects. +TEST_F(NewGrpcMuxImplTest, DiscoveryResponseNonexistentSub) { + setup(); + + const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; + grpc_mux_->addOrUpdateWatch(type_url, nullptr, {}, callbacks_, std::chrono::milliseconds(0)); + + EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(&async_stream_)); + grpc_mux_->start(); + + { + auto unexpected_response = std::make_unique(); + unexpected_response->set_type_url(type_url); + unexpected_response->set_system_version_info("0"); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "0")).Times(0); + grpc_mux_->onDiscoveryResponse(std::move(unexpected_response)); + } + { + auto response = std::make_unique(); + response->set_type_url(type_url); + response->set_system_version_info("1"); + envoy::api::v2::ClusterLoadAssignment load_assignment; + load_assignment.set_cluster_name("x"); + response->add_resources()->mutable_resource()->PackFrom(load_assignment); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "1")) + .WillOnce( + Invoke([&load_assignment]( + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField&, const std::string&) { + EXPECT_EQ(1, added_resources.size()); + envoy::api::v2::ClusterLoadAssignment expected_assignment; + added_resources[0].resource().UnpackTo(&expected_assignment); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); + })); + grpc_mux_->onDiscoveryResponse(std::move(response)); + } +} + +} // namespace +} // namespace Config +} // namespace Envoy diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index 403ad4d5f91f9..2426c10d83951 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -38,7 +38,7 @@ class SubscriptionFactoryTest : public testing::Test { return SubscriptionFactoryImpl(local_info_, dispatcher_, cm_, random_, validation_visitor_, *api_) .subscriptionFromConfigSource(config, Config::TypeUrl::get().ClusterLoadAssignment, - stats_store_, callbacks_); + stats_store_, callbacks_, false); } Upstream::MockClusterManager cm_; diff --git a/test/common/config/subscription_impl_test.cc b/test/common/config/subscription_impl_test.cc index c45f19bd00e89..45646de9b5315 100644 --- a/test/common/config/subscription_impl_test.cc +++ b/test/common/config/subscription_impl_test.cc @@ -39,12 +39,14 @@ class SubscriptionImplTest : public testing::TestWithParam { } } + void TearDown() override { test_harness_->doSubscriptionTearDown(); } + void startSubscription(const std::set& cluster_names) { test_harness_->startSubscription(cluster_names); } - void updateResources(const std::set& cluster_names) { - test_harness_->updateResources(cluster_names); + void updateResourceInterest(const std::set& cluster_names) { + test_harness_->updateResourceInterest(cluster_names); } void expectSendMessage(const std::set& cluster_names, const std::string& version, @@ -90,72 +92,75 @@ INSTANTIATE_TEST_SUITE_P(SubscriptionImplTest, SubscriptionImplInitFetchTimeoutT // Validate basic request-response succeeds. TEST_P(SubscriptionImplTest, InitialRequestResponse) { startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); - statsAre(2, 1, 0, 0, 0, 7148434200721666028); + EXPECT_TRUE(statsAre(2, 1, 0, 0, 0, 7148434200721666028)); } // Validate that multiple streamed updates succeed. TEST_P(SubscriptionImplTest, ResponseStream) { startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); - statsAre(2, 1, 0, 0, 0, 7148434200721666028); + EXPECT_TRUE(statsAre(2, 1, 0, 0, 0, 7148434200721666028)); deliverConfigUpdate({"cluster0", "cluster1"}, "1", true); - statsAre(3, 2, 0, 0, 0, 13237225503670494420U); + EXPECT_TRUE(statsAre(3, 2, 0, 0, 0, 13237225503670494420U)); } // Validate that the client can reject a config. TEST_P(SubscriptionImplTest, RejectConfig) { startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", false); - statsAre(2, 0, 1, 0, 0, 0); + EXPECT_TRUE(statsAre(2, 0, 1, 0, 0, 0)); } // Validate that the client can reject a config and accept the same config later. TEST_P(SubscriptionImplTest, RejectAcceptConfig) { startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", false); - statsAre(2, 0, 1, 0, 0, 0); + EXPECT_TRUE(statsAre(2, 0, 1, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); - statsAre(3, 1, 1, 0, 0, 7148434200721666028); + EXPECT_TRUE(statsAre(3, 1, 1, 0, 0, 7148434200721666028)); } // Validate that the client can reject a config and accept another config later. TEST_P(SubscriptionImplTest, RejectAcceptNextConfig) { startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", false); - statsAre(2, 0, 1, 0, 0, 0); + EXPECT_TRUE(statsAre(2, 0, 1, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "1", true); - statsAre(3, 1, 1, 0, 0, 13237225503670494420U); + EXPECT_TRUE(statsAre(3, 1, 1, 0, 0, 13237225503670494420U)); } // Validate that stream updates send a message with the updated resources. TEST_P(SubscriptionImplTest, UpdateResources) { startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); - statsAre(2, 1, 0, 0, 0, 7148434200721666028); - updateResources({"cluster2"}); - statsAre(3, 1, 0, 0, 0, 7148434200721666028); + EXPECT_TRUE(statsAre(2, 1, 0, 0, 0, 7148434200721666028)); + updateResourceInterest({"cluster2"}); + EXPECT_TRUE(statsAre(3, 1, 0, 0, 0, 7148434200721666028)); } // Validate that initial fetch timer is created and calls callback on timeout TEST_P(SubscriptionImplInitFetchTimeoutTest, InitialFetchTimeout) { + if (GetParam() == SubscriptionType::Filesystem) { + return; // initial_fetch_timeout not implemented for filesystem. + } InSequence s; expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds(1000)); startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); if (GetParam() == SubscriptionType::Http) { expectDisableInitFetchTimeoutTimer(); } expectConfigUpdateFailed(); callInitFetchTimeoutCb(); - statsAre(1, 0, 0, 0, 1, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 1, 0)); } // Validate that initial fetch timer is disabled on config update @@ -163,7 +168,7 @@ TEST_P(SubscriptionImplInitFetchTimeoutTest, DisableInitTimeoutOnSuccess) { InSequence s; expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds(1000)); startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); expectDisableInitFetchTimeoutTimer(); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); } @@ -173,7 +178,7 @@ TEST_P(SubscriptionImplInitFetchTimeoutTest, DisableInitTimeoutOnFail) { InSequence s; expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds(1000)); startSubscription({"cluster0", "cluster1"}); - statsAre(1, 0, 0, 0, 0, 0); + EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); expectDisableInitFetchTimeoutTimer(); deliverConfigUpdate({"cluster0", "cluster1"}, "0", false); } diff --git a/test/common/config/subscription_test_harness.h b/test/common/config/subscription_test_harness.h index 6a094116324d4..0f3b6c44da358 100644 --- a/test/common/config/subscription_test_harness.h +++ b/test/common/config/subscription_test_harness.h @@ -30,7 +30,7 @@ class SubscriptionTestHarness { * Update cluster names to be delivered via EDS. * @param cluster_names cluster names. */ - virtual void updateResources(const std::set& cluster_names) PURE; + virtual void updateResourceInterest(const std::set& cluster_names) PURE; /** * Expect that an update request is sent by the Subscription implementation. @@ -94,6 +94,8 @@ class SubscriptionTestHarness { virtual void callInitFetchTimeoutCb() PURE; + virtual void doSubscriptionTearDown() {} + Stats::IsolatedStoreImpl stats_store_; SubscriptionStats stats_; }; diff --git a/test/common/grpc/grpc_client_integration.h b/test/common/grpc/grpc_client_integration.h index bdfc0c6ae1ba7..86017cf821d15 100644 --- a/test/common/grpc/grpc_client_integration.h +++ b/test/common/grpc/grpc_client_integration.h @@ -53,34 +53,23 @@ class GrpcClientIntegrationParamTest ClientType clientType() const override { return std::get<1>(GetParam()); } }; -class DeltaSotwGrpcClientIntegrationParamTest +class DeltaSotwIntegrationParamTest : public BaseGrpcClientIntegrationParamTest, - public testing::TestWithParam> { + public testing::TestWithParam< + std::tuple> { public: - static std::string protocolTestParamsToString( - const ::testing::TestParamInfo>& - p) { - return fmt::format("{}_{}", + ~DeltaSotwIntegrationParamTest() override = default; + static std::string + protocolTestParamsToString(const ::testing::TestParamInfo< + std::tuple>& p) { + return fmt::format("{}_{}_{}", std::get<0>(p.param) == Network::Address::IpVersion::v4 ? "IPv4" : "IPv6", std::get<1>(p.param) == ClientType::GoogleGrpc ? "GoogleGrpc" : "EnvoyGrpc", - std::get<2>(p.param) ? "Delta" : "StateOfTheWorld"); + std::get<2>(p.param) == SotwOrDelta::Delta ? "Delta" : "StateOfTheWorld"); } Network::Address::IpVersion ipVersion() const override { return std::get<0>(GetParam()); } ClientType clientType() const override { return std::get<1>(GetParam()); } - bool isDelta() { return std::get<2>(GetParam()); } -}; - -class DeltaSotwIntegrationParamTest - : public testing::TestWithParam> { -public: - static std::string protocolTestParamsToString( - const ::testing::TestParamInfo>& p) { - return fmt::format("{}_{}_{}", - std::get<0>(p.param) == Network::Address::IpVersion::v4 ? "IPv4" : "IPv6", - std::get<1>(p.param) == SotwOrDelta::Delta ? "Delta" : "StateOfTheWorld"); - } - Network::Address::IpVersion ipVersion() const { return std::get<0>(GetParam()); } - SotwOrDelta sotwOrDelta() const { return std::get<1>(GetParam()); } + SotwOrDelta sotwOrDelta() const { return std::get<2>(GetParam()); } }; // Skip tests based on gRPC client type. @@ -102,19 +91,16 @@ class DeltaSotwIntegrationParamTest #define DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS \ testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ testing::Values(Grpc::ClientType::EnvoyGrpc, Grpc::ClientType::GoogleGrpc), \ - testing::Bool()) + testing::Values(Grpc::SotwOrDelta::Sotw, Grpc::SotwOrDelta::Delta)) #else #define GRPC_CLIENT_INTEGRATION_PARAMS \ testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ testing::Values(Grpc::ClientType::EnvoyGrpc)) #define DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS \ testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ - testing::Values(Grpc::ClientType::EnvoyGrpc), testing::Bool()) -#endif // ENVOY_GOOGLE_GRPC - -#define DELTA_INTEGRATION_PARAMS \ - testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ + testing::Values(Grpc::ClientType::EnvoyGrpc), \ testing::Values(Grpc::SotwOrDelta::Sotw, Grpc::SotwOrDelta::Delta)) +#endif // ENVOY_GOOGLE_GRPC } // namespace Grpc } // namespace Envoy diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 11424515eb328..9be73fbefe8cd 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -92,9 +92,9 @@ class RdsImplTest : public RdsTestBase { )EOF"; EXPECT_CALL(factory_context_.init_manager_, add(_)); - rds_ = - RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), - factory_context_, "foo.", *route_config_provider_manager_); + rds_ = RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), + factory_context_, "foo.", + *route_config_provider_manager_, false); 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_); @@ -126,7 +126,7 @@ TEST_F(RdsImplTest, RdsAndStatic) { EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), factory_context_, "foo.", - *route_config_provider_manager_), + *route_config_provider_manager_, false), EnvoyException); } @@ -264,7 +264,7 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase { 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.", factory_context_.initManager()); + rds_, factory_context_, "foo_prefix.", factory_context_.initManager(), false); rds_callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; } @@ -416,7 +416,7 @@ name: foo_route_config "1"); RouteConfigProviderPtr provider2 = route_config_provider_manager_->createRdsRouteConfigProvider( - rds_, factory_context_, "foo_prefix", factory_context_.initManager()); + rds_, factory_context_, "foo_prefix", factory_context_.initManager(), false); // provider2 should have route config immediately after create EXPECT_TRUE(provider2->configInfo().has_value()); @@ -430,7 +430,7 @@ name: foo_route_config 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", factory_context_.initManager()); + rds2, factory_context_, "foo_prefix", factory_context_.initManager(), false); EXPECT_NE(provider3, provider_); factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(route_configs, "provider3"); diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 3967703c41255..d8a3fcd5ff23d 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -6,6 +6,7 @@ #include "envoy/init/manager.h" #include "envoy/stats/scope.h" +#include "common/config/grpc_mux_impl.h" #include "common/router/scoped_rds.h" #include "test/mocks/config/mocks.h" @@ -94,25 +95,27 @@ class ScopedRoutesTestBase : public testing::Test { class ScopedRdsTest : public ScopedRoutesTestBase { protected: void setup() { - InSequence s; + ON_CALL(factory_context_.cluster_manager_, adsMux()) + .WillByDefault(Return(std::make_shared<::Envoy::Config::NullGrpcMuxImpl>())); + InSequence s; // Since factory_context_.cluster_manager_.subscription_factory_.callbacks_ is taken by the SRDS // subscription. We need to return a different MockSubscription here for each RDS subscription. // To build the map from RDS route_config_name to the RDS subscription, we need to get the // route_config_name by mocking start() on the Config::Subscription. EXPECT_CALL(factory_context_.cluster_manager_.subscription_factory_, - subscriptionFromConfigSource(_, _, _, _)) + subscriptionFromConfigSource(_, _, _, _, _)) .Times(AnyNumber()); EXPECT_CALL(factory_context_.cluster_manager_.subscription_factory_, subscriptionFromConfigSource( _, Eq(Grpc::Common::typeUrl( envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name())), - _, _)) + _, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke([this](const envoy::api::v2::core::ConfigSource&, absl::string_view, Stats::Scope&, - Envoy::Config::SubscriptionCallbacks& callbacks) { + Envoy::Config::SubscriptionCallbacks& callbacks, bool) { auto ret = std::make_unique>(); rds_subscription_by_config_subscription_[ret.get()] = &callbacks; EXPECT_CALL(*ret, start(_)) diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index ef5428e19ea9d..182b92d1ef5d9 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -808,10 +808,10 @@ class RtdsLoaderImplTest : public LoaderImplTest { EXPECT_CALL(init_manager_, add(_)).WillRepeatedly(Invoke([this](const Init::Target& target) { init_target_handles_.emplace_back(target.createHandle("test")); })); - ON_CALL(cm_.subscription_factory_, subscriptionFromConfigSource(_, _, _, _)) + ON_CALL(cm_.subscription_factory_, subscriptionFromConfigSource(_, _, _, _, _)) .WillByDefault(testing::Invoke( [this](const envoy::api::v2::core::ConfigSource&, absl::string_view, Stats::Scope&, - Config::SubscriptionCallbacks& callbacks) -> Config::SubscriptionPtr { + Config::SubscriptionCallbacks& callbacks, bool) -> Config::SubscriptionPtr { auto ret = std::make_unique>(); rtds_subscriptions_.push_back(ret.get()); rtds_callbacks_.push_back(&callbacks); diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index fe6b5e39ab603..6c0138cbaf04d 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -32,9 +32,9 @@ MATCHER_P(WithName, expectedName, "") { return arg.name() == expectedName; } class CdsApiImplTest : public testing::Test { protected: - void setup() { + void setup(bool is_delta = false) { envoy::api::v2::core::ConfigSource cds_config; - cds_ = CdsApiImpl::create(cds_config, cm_, store_, validation_visitor_); + cds_ = CdsApiImpl::create(cds_config, is_delta, cm_, store_, validation_visitor_); cds_->setInitializedCb([this]() -> void { initialized_.ready(); }); EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); @@ -180,7 +180,7 @@ TEST_F(CdsApiImplTest, ConfigUpdateWith2ValidClusters) { TEST_F(CdsApiImplTest, DeltaConfigUpdate) { { InSequence s; - setup(); + setup(true); } EXPECT_CALL(initialized_, ready()); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 1534ba3f3b2d8..da72909457d68 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -101,7 +101,7 @@ class TestClusterManagerFactory : public ClusterManagerFactory { return std::make_pair(result.first, ThreadAwareLoadBalancerPtr(result.second)); } - CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource&, ClusterManager&) override { + CdsApiPtr createCds(const envoy::api::v2::core::ConfigSource&, bool, ClusterManager&) override { return CdsApiPtr{createCds_()}; } @@ -2994,12 +2994,7 @@ TEST_F(ClusterManagerInitHelperTest, InitSecondaryWithoutEdsPaused) { ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); init_helper_.addCluster(cluster1); - const auto& type_url = Config::TypeUrl::get().ClusterLoadAssignment; - EXPECT_CALL(cm_.ads_mux_, paused(Eq(ByRef(type_url)))).WillRepeatedly(Return(false)); - EXPECT_CALL(cm_.ads_mux_, pause(Eq(ByRef(type_url)))); EXPECT_CALL(cluster1, initialize(_)); - EXPECT_CALL(cm_.ads_mux_, resume(Eq(ByRef(type_url)))); - init_helper_.onStaticLoadComplete(); EXPECT_CALL(*this, onClusterInit(Ref(cluster1))); @@ -3021,12 +3016,7 @@ TEST_F(ClusterManagerInitHelperTest, InitSecondaryWithEdsPaused) { ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); init_helper_.addCluster(cluster1); - const auto& type_url = Config::TypeUrl::get().ClusterLoadAssignment; - EXPECT_CALL(cm_.ads_mux_, paused(Eq(ByRef(type_url)))).WillRepeatedly(Return(true)); - EXPECT_CALL(cm_.ads_mux_, pause(Eq(ByRef(type_url)))).Times(0); EXPECT_CALL(cluster1, initialize(_)); - EXPECT_CALL(cm_.ads_mux_, resume(Eq(ByRef(type_url)))).Times(0); - init_helper_.onStaticLoadComplete(); EXPECT_CALL(*this, onClusterInit(Ref(cluster1))); diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 9247c9d84eb86..1e520b8151636 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -75,8 +75,8 @@ class EdsTest : public testing::Test { Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, random_, stats_, singleton_manager_, tls_, validation_visitor_, *api_); - cluster_.reset( - new EdsClusterImpl(eds_cluster_, runtime_, factory_context, std::move(scope), false)); + cluster_.reset(new EdsClusterImpl(eds_cluster_, runtime_, factory_context, std::move(scope), + false, false)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); eds_callbacks_ = cm_.subscription_factory_.callbacks_; } diff --git a/test/config/utility.cc b/test/config/utility.cc index c230b4c772c32..e93bb5c333ddb 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -155,6 +155,8 @@ name: envoy.squash nanos: 0 )EOF"; +// TODO(fredlas) set_node_on_first_message_only was true; the delta+SotW unification +// work restores it here. // TODO(#6327) cleaner approach to testing with static config. std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_type) { return fmt::format( @@ -172,7 +174,7 @@ std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_typ grpc_services: envoy_grpc: cluster_name: my_cds_cluster - set_node_on_first_message_only: true + set_node_on_first_message_only: false static_resources: clusters: - name: my_cds_cluster @@ -214,6 +216,39 @@ std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_typ api_type); } +// TODO(#6327) cleaner approach to testing with static config. +std::string ConfigHelper::adsBootstrap(const std::string& api_type) { + return fmt::format( + R"EOF( +dynamic_resources: + lds_config: + ads: {{}} + cds_config: + ads: {{}} + ads_config: + api_type: {} +static_resources: + clusters: + name: dummy_cluster + connect_timeout: + seconds: 5 + type: STATIC + hosts: + socket_address: + address: 127.0.0.1 + port_value: 0 + lb_policy: ROUND_ROBIN + http2_protocol_options: {{}} +admin: + access_log_path: /dev/null + address: + socket_address: + address: 127.0.0.1 + port_value: 0 +)EOF", + api_type); +} + envoy::api::v2::Cluster ConfigHelper::buildCluster(const std::string& name, int port, const std::string& ip_version) { return TestUtility::parseYaml(fmt::format(R"EOF( diff --git a/test/config/utility.h b/test/config/utility.h index 574160651aa3b..00cb9aece227c 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -91,6 +91,7 @@ class ConfigHelper { // Configuration for L7 proxying, with clusters cluster_1 and cluster_2 meant to be added via CDS. // api_type should be REST, GRPC, or DELTA_GRPC. static std::string discoveredClustersBootstrap(const std::string& api_type); + static std::string adsBootstrap(const std::string& api_type); // Builds a standard Cluster config fragment, with a single endpoint (at loopback:port). static envoy::api::v2::Cluster buildCluster(const std::string& name, int port, const std::string& ip_version); diff --git a/test/integration/ads_integration.cc b/test/integration/ads_integration.cc index 4e2590e4b41d6..6138f963e13aa 100644 --- a/test/integration/ads_integration.cc +++ b/test/integration/ads_integration.cc @@ -19,10 +19,13 @@ using testing::AssertionResult; namespace Envoy { AdsIntegrationTest::AdsIntegrationTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), AdsIntegrationConfig()) { + : HttpIntegrationTest( + Http::CodecClient::Type::HTTP2, ipVersion(), + AdsIntegrationConfig(sotwOrDelta() == Grpc::SotwOrDelta::Sotw ? "GRPC" : "DELTA_GRPC")) { use_lds_ = false; create_xds_upstream_ = true; tls_xds_upstream_ = true; + sotw_or_delta_ = sotwOrDelta(); } void AdsIntegrationTest::TearDown() { diff --git a/test/integration/ads_integration.h b/test/integration/ads_integration.h index 51a229e9dfe3b..31956bd6bb126 100644 --- a/test/integration/ads_integration.h +++ b/test/integration/ads_integration.h @@ -12,37 +12,43 @@ #include "test/common/grpc/grpc_client_integration.h" #include "test/integration/http_integration.h" +// TODO(fredlas) set_node_on_first_message_only was true; the delta+SotW unification +// work restores it here. namespace Envoy { -static std::string AdsIntegrationConfig() { +static std::string AdsIntegrationConfig(const std::string& api_type) { // Note: do not use CONSTRUCT_ON_FIRST_USE here! - return R"EOF( + return fmt::format(R"EOF( dynamic_resources: - lds_config: {ads: {}} - cds_config: {ads: {}} + lds_config: + ads: {{}} + cds_config: + ads: {{}} ads_config: - api_type: GRPC - set_node_on_first_message_only: true + api_type: {} + set_node_on_first_message_only: false static_resources: clusters: name: dummy_cluster - connect_timeout: { seconds: 5 } + connect_timeout: + seconds: 5 type: STATIC hosts: socket_address: address: 127.0.0.1 port_value: 0 lb_policy: ROUND_ROBIN - http2_protocol_options: {} + http2_protocol_options: {{}} admin: access_log_path: /dev/null address: socket_address: address: 127.0.0.1 port_value: 0 -)EOF"; +)EOF", + api_type); } -class AdsIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public HttpIntegrationTest { +class AdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public HttpIntegrationTest { public: AdsIntegrationTest(); diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index e2cb9250c0e6b..442b5dd8293b4 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -26,7 +26,8 @@ using testing::AssertionResult; namespace Envoy { -INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, AdsIntegrationTest, GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsIntegrationTest, + DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); // Validate basic config delivery and upgrade. TEST_P(AdsIntegrationTest, Basic) { @@ -54,9 +55,8 @@ TEST_P(AdsIntegrationTest, Failure) { EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "", {}, {}, {})); EXPECT_TRUE(compareDiscoveryRequest( - Config::TypeUrl::get().Cluster, "", {}, {}, {}, false, Grpc::Status::GrpcStatus::Internal, - fmt::format("{} does not match {}", Config::TypeUrl::get().ClusterLoadAssignment, - Config::TypeUrl::get().Cluster))); + Config::TypeUrl::get().Cluster, "", {}, {}, {}, true, Grpc::Status::GrpcStatus::Internal, + fmt::format("does not match the message-wide type URL {}", Config::TypeUrl::get().Cluster))); sendDiscoveryResponse(Config::TypeUrl::get().Cluster, {buildCluster("cluster_0")}, {buildCluster("cluster_0")}, {}, "1"); @@ -68,11 +68,11 @@ TEST_P(AdsIntegrationTest, Failure) { {buildCluster("cluster_0")}, {}, "1"); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "1", {}, {}, {})); - EXPECT_TRUE( - compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", {"cluster_0"}, {}, - {}, false, Grpc::Status::GrpcStatus::Internal, - fmt::format("{} does not match {}", Config::TypeUrl::get().Cluster, - Config::TypeUrl::get().ClusterLoadAssignment))); + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", + {"cluster_0"}, {}, {}, true, + Grpc::Status::GrpcStatus::Internal, + fmt::format("does not match the message-wide type URL {}", + Config::TypeUrl::get().ClusterLoadAssignment))); sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, {buildClusterLoadAssignment("cluster_0")}, {}, "1"); @@ -84,9 +84,8 @@ TEST_P(AdsIntegrationTest, Failure) { {buildRouteConfig("listener_0", "route_config_0")}, {}, "1"); EXPECT_TRUE(compareDiscoveryRequest( - Config::TypeUrl::get().Listener, "", {}, {}, {}, false, Grpc::Status::GrpcStatus::Internal, - fmt::format("{} does not match {}", Config::TypeUrl::get().RouteConfiguration, - Config::TypeUrl::get().Listener))); + Config::TypeUrl::get().Listener, "", {}, {}, {}, true, Grpc::Status::GrpcStatus::Internal, + fmt::format("does not match the message-wide type URL {}", Config::TypeUrl::get().Listener))); sendDiscoveryResponse( Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0")}, {buildListener("listener_0", "route_config_0")}, {}, "1"); @@ -98,11 +97,11 @@ TEST_P(AdsIntegrationTest, Failure) { {buildListener("route_config_0", "cluster_0")}, {}, "1"); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "1", {}, {}, {})); - EXPECT_TRUE( - compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "", {"route_config_0"}, {}, - {}, false, Grpc::Status::GrpcStatus::Internal, - fmt::format("{} does not match {}", Config::TypeUrl::get().Listener, - Config::TypeUrl::get().RouteConfiguration))); + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "", + {"route_config_0"}, {}, {}, true, + Grpc::Status::GrpcStatus::Internal, + fmt::format("does not match the message-wide type URL {}", + Config::TypeUrl::get().RouteConfiguration))); sendDiscoveryResponse( Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1"); @@ -111,6 +110,7 @@ TEST_P(AdsIntegrationTest, Failure) { {"route_config_0"}, {}, {})); test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); + makeSingleRequest(); } @@ -209,7 +209,7 @@ TEST_P(AdsIntegrationTest, RedisClusterRemoval) { {buildRedisCluster("redis_cluster")}, {}, "1"); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", - {"redis_cluster"}, {}, {})); + {"redis_cluster"}, {"redis_cluster"}, {})); sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("redis_cluster")}, {buildClusterLoadAssignment("redis_cluster")}, {}, "1"); @@ -261,7 +261,7 @@ TEST_P(AdsIntegrationTest, DuplicateWarmingClusters) { {buildListener("listener_0", "route_config_0")}, {}, "1"); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1", - {"cluster_0"}, {"cluster_0"}, {})); + {"cluster_0"}, {}, {})); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "", {"route_config_0"}, {"route_config_0"}, {})); sendDiscoveryResponse( @@ -321,30 +321,32 @@ TEST_P(AdsIntegrationTest, CdsPausedDuringWarming) { makeSingleRequest(); EXPECT_FALSE( - test_server_->server().clusterManager().adsMux().paused(Config::TypeUrl::get().Cluster)); + test_server_->server().clusterManager().adsMux()->paused(Config::TypeUrl::get().Cluster)); // Send the first warming cluster. sendDiscoveryResponse( Config::TypeUrl::get().Cluster, {buildCluster("warming_cluster_1")}, {buildCluster("warming_cluster_1")}, {"cluster_0"}, "2"); + test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 1); EXPECT_TRUE( - test_server_->server().clusterManager().adsMux().paused(Config::TypeUrl::get().Cluster)); + test_server_->server().clusterManager().adsMux()->paused(Config::TypeUrl::get().Cluster)); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1", {"warming_cluster_1"}, {"warming_cluster_1"}, {"cluster_0"})); // Send the second warming cluster. - sendDiscoveryResponse( - Config::TypeUrl::get().Cluster, {buildCluster("warming_cluster_2")}, - {buildCluster("warming_cluster_2")}, {"warming_cluster_1"}, "3"); + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, + {buildCluster("warming_cluster_2")}, + {buildCluster("warming_cluster_2")}, {}, "3"); test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 2); // We would've got a Cluster discovery request with version 2 here, had the CDS not been paused. + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1", {"warming_cluster_2", "warming_cluster_1"}, {"warming_cluster_2"}, {})); EXPECT_TRUE( - test_server_->server().clusterManager().adsMux().paused(Config::TypeUrl::get().Cluster)); + test_server_->server().clusterManager().adsMux()->paused(Config::TypeUrl::get().Cluster)); // Finish warming the clusters. sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, @@ -357,9 +359,15 @@ TEST_P(AdsIntegrationTest, CdsPausedDuringWarming) { // Validate that clusters are warmed. test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 0); EXPECT_FALSE( - test_server_->server().clusterManager().adsMux().paused(Config::TypeUrl::get().Cluster)); + test_server_->server().clusterManager().adsMux()->paused(Config::TypeUrl::get().Cluster)); // CDS is resumed and EDS response was acknowledged. + if (sotw_or_delta_ == Grpc::SotwOrDelta::Delta) { + // Envoy will ACK both Cluster messages. Since they arrived while CDS was paused, they aren't + // sent until CDS is unpaused. Since version 3 has already arrived by the time the version 2 + // ACK goes out, they're both acknowledging version 3. + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "3", {}, {}, {})); + } EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "3", {}, {}, {})); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "2", {"warming_cluster_2", "warming_cluster_1"}, {}, {})); @@ -412,9 +420,9 @@ TEST_P(AdsIntegrationTest, ClusterWarmingOnNamedResponse) { {"warming_cluster_1"}, {"warming_cluster_1"}, {"cluster_0"})); // Send the second warming cluster. - sendDiscoveryResponse( - Config::TypeUrl::get().Cluster, {buildCluster("warming_cluster_2")}, - {buildCluster("warming_cluster_2")}, {"warming_cluster_1"}, "3"); + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, + {buildCluster("warming_cluster_2")}, + {buildCluster("warming_cluster_2")}, {}, "3"); test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 2); EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1", @@ -425,7 +433,7 @@ TEST_P(AdsIntegrationTest, ClusterWarmingOnNamedResponse) { sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("warming_cluster_1")}, - {buildClusterLoadAssignment("warming_cluster_1")}, {"cluster_0"}, "2"); + {buildClusterLoadAssignment("warming_cluster_1")}, {}, "2"); // Envoy will not finish warming of the second cluster because of the missing load assignments // i,e. no named EDS response. @@ -446,7 +454,7 @@ TEST_P(AdsIntegrationTest, ClusterWarmingOnNamedResponse) { sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("warming_cluster_2")}, - {buildClusterLoadAssignment("warming_cluster_2")}, {"warming_cluster_1"}, "3"); + {buildClusterLoadAssignment("warming_cluster_2")}, {}, "3"); test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 0); } @@ -580,13 +588,16 @@ TEST_P(AdsIntegrationTest, RdsAfterLdsInvalidated) { test_server_->waitForCounterGe("listener_manager.listener_create_success", 2); } -class AdsFailIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, +class AdsFailIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public HttpIntegrationTest { public: AdsFailIntegrationTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), AdsIntegrationConfig()) { + : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), + AdsIntegrationConfig( + sotwOrDelta() == Grpc::SotwOrDelta::Sotw ? "GRPC" : "DELTA_GRPC")) { create_xds_upstream_ = true; use_lds_ = false; + sotw_or_delta_ = sotwOrDelta(); } void TearDown() override { @@ -609,8 +620,8 @@ class AdsFailIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, AdsFailIntegrationTest, - GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsFailIntegrationTest, + DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); // Validate that we don't crash on failed ADS stream. TEST_P(AdsFailIntegrationTest, ConnectDisconnect) { @@ -621,13 +632,16 @@ TEST_P(AdsFailIntegrationTest, ConnectDisconnect) { xds_stream_->finishGrpcStream(Grpc::Status::Internal); } -class AdsConfigIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, +class AdsConfigIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public HttpIntegrationTest { public: AdsConfigIntegrationTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), AdsIntegrationConfig()) { + : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), + AdsIntegrationConfig( + sotwOrDelta() == Grpc::SotwOrDelta::Sotw ? "GRPC" : "DELTA_GRPC")) { create_xds_upstream_ = true; use_lds_ = false; + sotw_or_delta_ = sotwOrDelta(); } void TearDown() override { @@ -658,8 +672,8 @@ class AdsConfigIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, AdsConfigIntegrationTest, - GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsConfigIntegrationTest, + DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); // This is s regression validating that we don't crash on EDS static Cluster that uses ADS. TEST_P(AdsConfigIntegrationTest, EdsClusterWithAdsConfigSource) { @@ -753,7 +767,8 @@ TEST_P(AdsIntegrationTest, ListenerDrainBeforeServerStart) { // Remove listener. EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "1", {}, {}, {})); - sendDiscoveryResponse(Config::TypeUrl::get().Listener, {}, {}, {}, "1"); + sendDiscoveryResponse(Config::TypeUrl::get().Listener, {}, {}, + {"listener_0"}, "2"); test_server_->waitForGaugeEq("listener_manager.total_listeners_active", 0); } diff --git a/test/integration/cds_integration_test.cc b/test/integration/cds_integration_test.cc index 1ea5f5fe67e89..d3013870ea949 100644 --- a/test/integration/cds_integration_test.cc +++ b/test/integration/cds_integration_test.cc @@ -122,7 +122,8 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht bool test_skipped_{true}; }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, CdsIntegrationTest, DELTA_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, CdsIntegrationTest, + DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); // 1) Envoy starts up with no static clusters (other than the CDS-over-gRPC server). // 2) Envoy is told of a cluster via CDS. diff --git a/test/integration/integration.cc b/test/integration/integration.cc index 595c78f1e24ba..cf4baa464d10c 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -554,21 +554,21 @@ AssertionResult BaseIntegrationTest::compareDiscoveryRequest( const std::vector& expected_resource_names, const std::vector& expected_resource_names_added, const std::vector& expected_resource_names_removed, bool expect_node, - const Protobuf::int32 expected_error_code, const std::string& expected_error_message) { + const Protobuf::int32 expected_error_code, const std::string& expected_error_substring) { if (sotw_or_delta_ == Grpc::SotwOrDelta::Sotw) { return compareSotwDiscoveryRequest(expected_type_url, expected_version, expected_resource_names, - expect_node, expected_error_code, expected_error_message); + expect_node, expected_error_code, expected_error_substring); } else { return compareDeltaDiscoveryRequest(expected_type_url, expected_resource_names_added, expected_resource_names_removed, expected_error_code, - expected_error_message); + expected_error_substring); } } AssertionResult BaseIntegrationTest::compareSotwDiscoveryRequest( const std::string& expected_type_url, const std::string& expected_version, const std::vector& expected_resource_names, bool expect_node, - const Protobuf::int32 expected_error_code, const std::string& expected_error_message) { + const Protobuf::int32 expected_error_code, const std::string& expected_error_substring) { envoy::api::v2::DiscoveryRequest discovery_request; VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, discovery_request)); @@ -590,7 +590,7 @@ AssertionResult BaseIntegrationTest::compareSotwDiscoveryRequest( expected_error_code); } EXPECT_TRUE( - IsSubstring("", "", expected_error_message, discovery_request.error_detail().message())); + IsSubstring("", "", expected_error_substring, discovery_request.error_detail().message())); const std::vector resource_names(discovery_request.resource_names().cbegin(), discovery_request.resource_names().cend()); if (expected_resource_names != resource_names) { @@ -608,47 +608,69 @@ AssertionResult BaseIntegrationTest::compareSotwDiscoveryRequest( return AssertionSuccess(); } +AssertionResult compareSets(const std::set& set1, const std::set& set2, + absl::string_view name) { + if (set1 == set2) { + return AssertionSuccess(); + } + auto failure = AssertionFailure() << name << " field not as expected.\nExpected: {"; + for (const auto& x : set1) { + failure << x << ", "; + } + failure << "}\nActual: {"; + for (const auto& x : set2) { + failure << x << ", "; + } + return failure << "}"; +} + AssertionResult BaseIntegrationTest::compareDeltaDiscoveryRequest( const std::string& expected_type_url, const std::vector& expected_resource_subscriptions, const std::vector& expected_resource_unsubscriptions, FakeStreamPtr& xds_stream, - const Protobuf::int32 expected_error_code, const std::string& expected_error_message) { + const Protobuf::int32 expected_error_code, const std::string& expected_error_substring) { envoy::api::v2::DeltaDiscoveryRequest request; VERIFY_ASSERTION(xds_stream->waitForGrpcMessage(*dispatcher_, request)); - EXPECT_TRUE(request.has_node()); - EXPECT_FALSE(request.node().id().empty()); - EXPECT_FALSE(request.node().cluster().empty()); - - if (expected_type_url != request.type_url()) { - return AssertionFailure() << fmt::format("type_url {} does not match expected {}", - request.type_url(), expected_type_url); + // Verify all we care about node. + if (!request.has_node() || request.node().id().empty() || request.node().cluster().empty()) { + return AssertionFailure() << "Weird node field"; } - if (!(expected_error_code == request.error_detail().code())) { - return AssertionFailure() << fmt::format("error_code {} does not match expected {}", - request.error_detail().code(), expected_error_code); + if (request.type_url() != expected_type_url) { + return AssertionFailure() << fmt::format("type_url {} does not match expected {}.", + request.type_url(), expected_type_url); } - EXPECT_TRUE(IsSubstring("", "", expected_error_message, request.error_detail().message())); - - const std::vector resource_subscriptions(request.resource_names_subscribe().cbegin(), - request.resource_names_subscribe().cend()); - if (expected_resource_subscriptions != resource_subscriptions) { - return AssertionFailure() << fmt::format( - "newly subscribed resources {} do not match expected {} in {}", - fmt::join(resource_subscriptions.begin(), resource_subscriptions.end(), ","), - fmt::join(expected_resource_subscriptions.begin(), - expected_resource_subscriptions.end(), ","), - request.DebugString()); - } - const std::vector resource_unsubscriptions( - request.resource_names_unsubscribe().cbegin(), request.resource_names_unsubscribe().cend()); - if (expected_resource_unsubscriptions != resource_unsubscriptions) { + // Sort to ignore ordering. + std::set expected_sub{expected_resource_subscriptions.begin(), + expected_resource_subscriptions.end()}; + std::set expected_unsub{expected_resource_unsubscriptions.begin(), + expected_resource_unsubscriptions.end()}; + std::set actual_sub{request.resource_names_subscribe().begin(), + request.resource_names_subscribe().end()}; + std::set actual_unsub{request.resource_names_unsubscribe().begin(), + request.resource_names_unsubscribe().end()}; + auto sub_result = compareSets(expected_sub, actual_sub, "expected_resource_subscriptions"); + if (!sub_result) { + return sub_result; + } + auto unsub_result = + compareSets(expected_unsub, actual_unsub, "expected_resource_unsubscriptions"); + if (!unsub_result) { + return unsub_result; + } + // (We don't care about response_nonce or initial_resource_versions.) + + if (request.error_detail().code() != expected_error_code) { return AssertionFailure() << fmt::format( - "newly UNsubscribed resources {} do not match expected {} in {}", - fmt::join(resource_unsubscriptions.begin(), resource_unsubscriptions.end(), ","), - fmt::join(expected_resource_unsubscriptions.begin(), - expected_resource_unsubscriptions.end(), ","), - request.DebugString()); + "error code {} does not match expected {}. (Error message is {}).", + request.error_detail().code(), expected_error_code, + request.error_detail().message()); + } + if (expected_error_code != Grpc::Status::GrpcStatus::Ok && + request.error_detail().message().find(expected_error_substring) == std::string::npos) { + return AssertionFailure() << "\"" << expected_error_substring + << "\" is not a substring of actual error message \"" + << request.error_detail().message() << "\""; } return AssertionSuccess(); } diff --git a/test/integration/integration.h b/test/integration/integration.h index 7cfa3b726435f..7cd2109cc8593 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -220,12 +220,14 @@ class BaseIntegrationTest : Logger::Loggable { // sending/receiving to/from the (imaginary) xDS server. You should almost always use // compareDiscoveryRequest() and sendDiscoveryResponse(), but the SotW/delta-specific versions are // available if you're writing a SotW/delta-specific test. + // TODO(fredlas) expect_node was defaulting false here; the delta+SotW unification work restores + // it. AssertionResult compareDiscoveryRequest(const std::string& expected_type_url, const std::string& expected_version, const std::vector& expected_resource_names, const std::vector& expected_resource_names_added, const std::vector& expected_resource_names_removed, - bool expect_node = false, + bool expect_node = true, const Protobuf::int32 expected_error_code = Grpc::Status::GrpcStatus::Ok, const std::string& expected_error_message = ""); template @@ -256,9 +258,12 @@ class BaseIntegrationTest : Logger::Loggable { const std::vector& expected_resource_unsubscriptions, FakeStreamPtr& stream, const Protobuf::int32 expected_error_code = Grpc::Status::GrpcStatus::Ok, const std::string& expected_error_message = ""); + + // TODO(fredlas) expect_node was defaulting false here; the delta+SotW unification work restores + // it. AssertionResult compareSotwDiscoveryRequest( const std::string& expected_type_url, const std::string& expected_version, - const std::vector& expected_resource_names, bool expect_node = false, + const std::vector& expected_resource_names, bool expect_node = true, const Protobuf::int32 expected_error_code = Grpc::Status::GrpcStatus::Ok, const std::string& expected_error_message = ""); @@ -271,8 +276,11 @@ class BaseIntegrationTest : Logger::Loggable { for (const auto& message : messages) { discovery_response.add_resources()->PackFrom(message); } + static int next_nonce_counter = 0; + discovery_response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); xds_stream_->sendGrpcMessage(discovery_response); } + template void sendDeltaDiscoveryResponse(const std::string& type_url, const std::vector& added_or_updated, @@ -296,7 +304,8 @@ class BaseIntegrationTest : Logger::Loggable { resource->mutable_resource()->PackFrom(message); } *response.mutable_removed_resources() = {removed.begin(), removed.end()}; - response.set_nonce("noncense"); + static int next_nonce_counter = 0; + response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); stream->sendGrpcMessage(response); } diff --git a/test/integration/rtds_integration_test.cc b/test/integration/rtds_integration_test.cc index 02283b3927c22..6f223f1a626dc 100644 --- a/test/integration/rtds_integration_test.cc +++ b/test/integration/rtds_integration_test.cc @@ -6,6 +6,8 @@ namespace Envoy { namespace { +// TODO(fredlas) set_node_on_first_message_only was true; the delta+SotW unification +// work restores it here. std::string tdsBootstrapConfig(absl::string_view api_type) { return fmt::format(R"EOF( static_resources: @@ -36,7 +38,7 @@ std::string tdsBootstrapConfig(absl::string_view api_type) { grpc_services: envoy_grpc: cluster_name: rtds_cluster - set_node_on_first_message_only: true + set_node_on_first_message_only: false - name: some_admin_layer admin_layer: {{}} admin: @@ -110,7 +112,8 @@ class RtdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public H uint32_t initial_keys_{}; }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, RtdsIntegrationTest, DELTA_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, RtdsIntegrationTest, + DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); TEST_P(RtdsIntegrationTest, RtdsReload) { initialize(); diff --git a/test/integration/scoped_rds_integration_test.cc b/test/integration/scoped_rds_integration_test.cc index 5f4bb27395e41..3caca65ec896b 100644 --- a/test/integration/scoped_rds_integration_test.cc +++ b/test/integration/scoped_rds_integration_test.cc @@ -13,7 +13,7 @@ namespace Envoy { namespace { class ScopedRdsIntegrationTest : public HttpIntegrationTest, - public Grpc::DeltaSotwGrpcClientIntegrationParamTest { + public Grpc::DeltaSotwIntegrationParamTest { protected: struct FakeUpstreamInfo { FakeHttpConnectionPtr connection_; @@ -217,6 +217,8 @@ class ScopedRdsIntegrationTest : public HttpIntegrationTest, response); } + bool isDelta() { return sotwOrDelta() == Grpc::SotwOrDelta::Delta; } + const std::string srds_config_name_{"foo-scoped-routes"}; FakeUpstreamInfo scoped_rds_upstream_info_; FakeUpstreamInfo rds_upstream_info_; diff --git a/test/mocks/config/BUILD b/test/mocks/config/BUILD index 7cd7830ad794a..69698c7dc7322 100644 --- a/test/mocks/config/BUILD +++ b/test/mocks/config/BUILD @@ -16,7 +16,6 @@ envoy_cc_mock( "//include/envoy/config:config_provider_manager_interface", "//include/envoy/config:grpc_mux_interface", "//include/envoy/config:subscription_interface", - "//include/envoy/config:xds_grpc_context_interface", "//source/common/config:config_provider_lib", "//source/common/config:resources_lib", "//source/common/protobuf:utility_lib", diff --git a/test/mocks/config/mocks.cc b/test/mocks/config/mocks.cc index 4a3e3099e0c15..7d2846e44a6d4 100644 --- a/test/mocks/config/mocks.cc +++ b/test/mocks/config/mocks.cc @@ -10,15 +10,15 @@ 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, subscriptionFromConfigSource(_, _, _, _, _)) + .WillByDefault(testing::Invoke( + [this](const envoy::api::v2::core::ConfigSource&, absl::string_view, Stats::Scope&, + SubscriptionCallbacks& callbacks, bool) -> SubscriptionPtr { + auto ret = std::make_unique>(); + subscription_ = ret.get(); + callbacks_ = &callbacks; + return ret; + })); ON_CALL(*this, messageValidationVisitor()) .WillByDefault(testing::ReturnRef(ProtobufMessage::getStrictValidationVisitor())); } diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index b579cb56b7221..7b88cea67c880 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -4,7 +4,6 @@ #include "envoy/config/config_provider_manager.h" #include "envoy/config/grpc_mux.h" #include "envoy/config/subscription.h" -#include "envoy/config/xds_grpc_context.h" #include "common/config/config_provider_impl.h" #include "common/config/resources.h" @@ -45,7 +44,7 @@ template class MockSubscriptionCallbacks : public Subscript class MockSubscription : public Subscription { public: MOCK_METHOD1(start, void(const std::set& resources)); - MOCK_METHOD1(updateResources, void(const std::set& update_to_these_names)); + MOCK_METHOD1(updateResourceInterest, void(const std::set& update_to_these_names)); }; class MockSubscriptionFactory : public SubscriptionFactory { @@ -53,10 +52,10 @@ class MockSubscriptionFactory : public SubscriptionFactory { MockSubscriptionFactory(); ~MockSubscriptionFactory() override; - MOCK_METHOD4(subscriptionFromConfigSource, + MOCK_METHOD5(subscriptionFromConfigSource, SubscriptionPtr(const envoy::api::v2::core::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, - SubscriptionCallbacks& callbacks)); + SubscriptionCallbacks& callbacks, bool is_delta)); MOCK_METHOD0(messageValidationVisitor, ProtobufMessage::ValidationVisitor&()); MockSubscription* subscription_{}; @@ -85,6 +84,19 @@ class MockGrpcMux : public GrpcMux { MOCK_METHOD1(pause, void(const std::string& type_url)); MOCK_METHOD1(resume, void(const std::string& type_url)); MOCK_CONST_METHOD1(paused, bool(const std::string& type_url)); + + MOCK_METHOD5(addSubscription, + void(const std::set& resources, const std::string& type_url, + SubscriptionCallbacks& callbacks, SubscriptionStats& stats, + std::chrono::milliseconds init_fetch_timeout)); + MOCK_METHOD2(updateResourceInterest, + void(const std::set& resources, const std::string& type_url)); + + MOCK_METHOD5(addOrUpdateWatch, + Watch*(const std::string& type_url, Watch* watch, + const std::set& resources, SubscriptionCallbacks& callbacks, + std::chrono::milliseconds init_fetch_timeout)); + MOCK_METHOD2(removeWatch, void(const std::string& type_url, Watch* watch)); }; class MockGrpcMuxCallbacks : public GrpcMuxCallbacks { diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index e1416736308f8..ba07f3c6f97cc 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -406,11 +406,11 @@ class MockRouteConfigProviderManager : public RouteConfigProviderManager { MockRouteConfigProviderManager(); ~MockRouteConfigProviderManager() override; - MOCK_METHOD4(createRdsRouteConfigProvider, + MOCK_METHOD5(createRdsRouteConfigProvider, RouteConfigProviderPtr( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, Init::Manager& init_manager)); + const std::string& stat_prefix, Init::Manager& init_manager, bool is_delta)); MOCK_METHOD2(createStaticRouteConfigProvider, RouteConfigProviderPtr(const envoy::api::v2::RouteConfiguration& route_config, Server::Configuration::FactoryContext& factory_context)); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index bcbc232bd2408..1487bc0b88529 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -234,11 +234,13 @@ class MockListenerComponentFactory : public ListenerComponentFactory { DrainManagerPtr createDrainManager(envoy::api::v2::Listener::DrainType drain_type) override { return DrainManagerPtr{createDrainManager_(drain_type)}; } - LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { - return LdsApiPtr{createLdsApi_(lds_config)}; + LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config, + bool is_delta) override { + return LdsApiPtr{createLdsApi_(lds_config, is_delta)}; } - MOCK_METHOD1(createLdsApi_, LdsApi*(const envoy::api::v2::core::ConfigSource& lds_config)); + MOCK_METHOD2(createLdsApi_, + LdsApi*(const envoy::api::v2::core::ConfigSource& lds_config, bool is_delta)); MOCK_METHOD2(createNetworkFilterFactoryList, std::vector( const Protobuf::RepeatedPtrField& filters, @@ -269,7 +271,8 @@ class MockListenerManager : public ListenerManager { MOCK_METHOD3(addOrUpdateListener, bool(const envoy::api::v2::Listener& config, const std::string& version_info, bool modifiable)); - MOCK_METHOD1(createLdsApi, void(const envoy::api::v2::core::ConfigSource& lds_config)); + MOCK_METHOD2(createLdsApi, + void(const envoy::api::v2::core::ConfigSource& lds_config, bool is_delta)); MOCK_METHOD0(listeners, std::vector>()); MOCK_METHOD0(numConnections, uint64_t()); MOCK_METHOD1(removeListener, bool(const std::string& listener_name)); diff --git a/test/mocks/upstream/mocks.cc b/test/mocks/upstream/mocks.cc index 7d629b1061084..89f42f6c5f6df 100644 --- a/test/mocks/upstream/mocks.cc +++ b/test/mocks/upstream/mocks.cc @@ -139,7 +139,7 @@ MockClusterManager::MockClusterManager() { ON_CALL(*this, httpAsyncClientForCluster(_)).WillByDefault(ReturnRef(async_client_)); ON_CALL(*this, httpAsyncClientForCluster(_)).WillByDefault((ReturnRef(async_client_))); ON_CALL(*this, bindConfig()).WillByDefault(ReturnRef(bind_config_)); - ON_CALL(*this, adsMux()).WillByDefault(ReturnRef(ads_mux_)); + ON_CALL(*this, adsMux()).WillByDefault(Return(ads_mux_)); ON_CALL(*this, grpcAsyncClientManager()).WillByDefault(ReturnRef(async_client_manager_)); ON_CALL(*this, localClusterName()).WillByDefault((ReturnRef(local_cluster_name_))); diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h index 1b7f0b07b237d..939cf72dadf21 100644 --- a/test/mocks/upstream/mocks.h +++ b/test/mocks/upstream/mocks.h @@ -267,8 +267,8 @@ class MockClusterManagerFactory : public ClusterManagerFactory { const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api)); - MOCK_METHOD2(createCds, - CdsApiPtr(const envoy::api::v2::core::ConfigSource& cds_config, ClusterManager& cm)); + MOCK_METHOD3(createCds, CdsApiPtr(const envoy::api::v2::core::ConfigSource& cds_config, + bool is_delta, ClusterManager& cm)); private: NiceMock secret_manager_; @@ -322,13 +322,14 @@ class MockClusterManager : public ClusterManager { MOCK_METHOD1(removeCluster, bool(const std::string& cluster)); MOCK_METHOD0(shutdown, void()); MOCK_CONST_METHOD0(bindConfig, const envoy::api::v2::core::BindConfig&()); - MOCK_METHOD0(adsMux, Config::GrpcMux&()); + MOCK_METHOD0(adsMux, Config::GrpcMuxSharedPtr()); MOCK_METHOD0(grpcAsyncClientManager, Grpc::AsyncClientManager&()); MOCK_CONST_METHOD0(versionInfo, const std::string()); MOCK_CONST_METHOD0(localClusterName, const std::string&()); MOCK_METHOD1(addThreadLocalClusterUpdateCallbacks_, ClusterUpdateCallbacksHandle*(ClusterUpdateCallbacks& callbacks)); MOCK_CONST_METHOD0(warmingClusterCount, std::size_t()); + MOCK_CONST_METHOD0(xdsIsDelta, bool()); MOCK_METHOD0(subscriptionFactory, Config::SubscriptionFactory&()); NiceMock conn_pool_; @@ -336,7 +337,7 @@ class MockClusterManager : public ClusterManager { NiceMock tcp_conn_pool_; NiceMock thread_local_cluster_; envoy::api::v2::core::BindConfig bind_config_; - NiceMock ads_mux_; + std::shared_ptr> ads_mux_; NiceMock async_client_manager_; std::string local_cluster_name_; NiceMock cluster_manager_factory_; diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index a5ffe1378ace6..72aee52328b51 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -36,7 +36,7 @@ class LdsApiTest : public testing::Test { envoy::api::v2::core::ConfigSource lds_config; EXPECT_CALL(init_manager_, add(_)); lds_ = std::make_unique(lds_config, cluster_manager_, init_manager_, store_, - listener_manager_, validation_visitor_); + listener_manager_, validation_visitor_, false); EXPECT_CALL(*cluster_manager_.subscription_factory_.subscription_, start(_)); init_target_handle_->initialize(init_watcher_); lds_callbacks_ = cluster_manager_.subscription_factory_.callbacks_; @@ -83,6 +83,7 @@ class LdsApiTest : public testing::Test { listeners.Add()->PackFrom(listener); } + std::shared_ptr> grpc_mux_; NiceMock cluster_manager_; Init::MockManager init_manager_; Init::ExpectableWatcherImpl init_watcher_; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 12c436b8ad60d..91882a64f73fa 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -590,9 +590,9 @@ TEST_F(ListenerManagerImplTest, AddOrUpdateListener) { InSequence s; auto* lds_api = new MockLdsApi(); - EXPECT_CALL(listener_factory_, createLdsApi_(_)).WillOnce(Return(lds_api)); + EXPECT_CALL(listener_factory_, createLdsApi_(_, _)).WillOnce(Return(lds_api)); envoy::api::v2::core::ConfigSource lds_config; - manager_->createLdsApi(lds_config); + manager_->createLdsApi(lds_config, false); EXPECT_CALL(*lds_api, versionInfo()).WillOnce(Return("")); checkConfigDump(R"EOF( diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 9204d82d1893c..ab5842d2a194b 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -330,6 +330,7 @@ abcd absl accessor accessors +acks acls addr agg @@ -644,6 +645,7 @@ paren parentid parsers passthroughs +pausable pcall pcap pclose @@ -818,6 +820,7 @@ unordered unowned unparented unpause +unpaused unpopulated unprotect unref