From a249b575db8a8afad81d84a40e54a6aa2f31e270 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 30 Jul 2020 14:26:13 -0700 Subject: [PATCH 01/16] Fix for missed vhds updates Signed-off-by: Dmitri Dolguikh --- include/envoy/config/grpc_mux.h | 4 +- include/envoy/config/subscription.h | 6 +- .../router/route_config_update_receiver.h | 2 + .../common/config/delta_subscription_state.cc | 6 +- .../common/config/delta_subscription_state.h | 3 +- .../config/filesystem_subscription_impl.cc | 2 +- .../config/filesystem_subscription_impl.h | 5 +- source/common/config/grpc_mux_impl.cc | 2 +- source/common/config/grpc_mux_impl.h | 7 ++- .../common/config/grpc_subscription_impl.cc | 10 +++- source/common/config/grpc_subscription_impl.h | 4 +- .../common/config/http_subscription_impl.cc | 2 +- source/common/config/http_subscription_impl.h | 5 +- source/common/config/new_grpc_mux_impl.cc | 24 ++++++-- source/common/config/new_grpc_mux_impl.h | 15 +++-- source/common/config/watch_map.cc | 56 ++++++++++++++++--- source/common/config/watch_map.h | 12 +++- .../route_config_update_receiver_impl.cc | 8 +++ .../route_config_update_receiver_impl.h | 1 + source/common/router/vhds.cc | 4 +- .../config/delta_subscription_state_test.cc | 6 +- test/common/config/new_grpc_mux_impl_test.cc | 12 ++-- test/common/config/watch_map_test.cc | 37 ++++++------ test/common/router/rds_impl_test.cc | 10 ++-- test/common/router/scoped_rds_test.cc | 4 +- test/common/runtime/runtime_impl_test.cc | 2 +- test/common/upstream/cds_api_impl_test.cc | 2 +- test/common/upstream/eds_speed_test.cc | 2 +- test/common/upstream/eds_test.cc | 2 +- test/integration/vhds_integration_test.cc | 22 ++++---- test/mocks/config/mocks.h | 7 ++- test/server/lds_api_test.cc | 2 +- 32 files changed, 191 insertions(+), 95 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 0f20aae3cfc5a..df1c6e115af1b 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -43,6 +43,8 @@ class GrpcMuxWatch { * @param resources set of resource names to watch for */ virtual void update(const std::set& resources) PURE; + + virtual void add(const std::set& resources) PURE; }; using GrpcMuxWatchPtr = std::unique_ptr; @@ -99,7 +101,7 @@ class GrpcMux { virtual GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder) PURE; + OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching) PURE; }; using GrpcMuxPtr = std::unique_ptr; diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index c05a6d567d700..760ce2d16a930 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -151,7 +151,7 @@ class UntypedConfigUpdateCallbacks { virtual void onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info) PURE; + const std::string& system_version_info, const bool use_prefix_matching) PURE; /** * Called when either the Subscription is unable to fetch a config update or when onConfigUpdate @@ -175,7 +175,7 @@ class Subscription { * to fetch throughout the lifetime of the Subscription object. * @param resources set of resource names to fetch. */ - virtual void start(const std::set& resource_names) PURE; + virtual void start(const std::set& resource_names, const bool use_prefix_matching = false) PURE; /** * Update the resources to fetch. @@ -183,6 +183,8 @@ class Subscription { * be passed to std::set_difference, which must be given sorted collections. */ virtual void updateResourceInterest(const std::set& update_to_these_names) PURE; + + virtual void addResourceInterest(const std::set& add_these_names) PURE; }; using SubscriptionPtr = std::unique_ptr; diff --git a/include/envoy/router/route_config_update_receiver.h b/include/envoy/router/route_config_update_receiver.h index d18c6d5542529..6355f7f31804f 100644 --- a/include/envoy/router/route_config_update_receiver.h +++ b/include/envoy/router/route_config_update_receiver.h @@ -92,6 +92,8 @@ class RouteConfigUpdateReceiver { * update. */ virtual const std::set& resourceIdsInLastVhdsUpdate() PURE; + + virtual std::set vhdsVhosts() const PURE; }; using RouteConfigUpdatePtr = std::unique_ptr; diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index c0a6a5502cb09..5ffa284f3819c 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -11,8 +11,8 @@ namespace Config { DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info) - : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info) {} + const LocalInfo::LocalInfo& local_info, const bool use_prefix_matching) + : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), use_prefix_matching_(use_prefix_matching) {} void DeltaSubscriptionState::updateSubscriptionInterest(const std::set& cur_added, const std::set& cur_removed) { @@ -82,7 +82,7 @@ void DeltaSubscriptionState::handleGoodResponse( } } watch_map_.onConfigUpdate(message.resources(), message.removed_resources(), - message.system_version_info()); + message.system_version_info(), use_prefix_matching_); for (const auto& resource : message.resources()) { setResourceVersion(resource.name(), resource.version()); } diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 1e21ba3a8efde..3ccbbbfb0efbc 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -25,7 +25,7 @@ namespace Config { class DeltaSubscriptionState : public Logger::Loggable { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info); + const LocalInfo::LocalInfo& local_info, const bool use_prefix_matching); // Update which resources we're interested in subscribing to. void updateSubscriptionInterest(const std::set& cur_added, @@ -100,6 +100,7 @@ class DeltaSubscriptionState : public Logger::Loggable { // Feel free to change to an unordered container once we figure out how to make it work. std::set names_added_; std::set names_removed_; + const bool use_prefix_matching_; }; } // namespace Config diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 1373dc34c92e4..79d505d763611 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -27,7 +27,7 @@ FilesystemSubscriptionImpl::FilesystemSubscriptionImpl( } // Config::Subscription -void FilesystemSubscriptionImpl::start(const std::set&) { +void FilesystemSubscriptionImpl::start(const std::set&, const bool) { started_ = true; // Attempt to read in case there is a file there already. refresh(); diff --git a/source/common/config/filesystem_subscription_impl.h b/source/common/config/filesystem_subscription_impl.h index 75dd5f25b1e47..ae090c160abb4 100644 --- a/source/common/config/filesystem_subscription_impl.h +++ b/source/common/config/filesystem_subscription_impl.h @@ -27,8 +27,11 @@ class FilesystemSubscriptionImpl : public Config::Subscription, // Config::Subscription // We report all discovered resources in the watched file, so the resource names arguments are // unused, and updateResourceInterest is a no-op (other than updating a stat). - void start(const std::set&) override; + void start(const std::set&, const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set&) override; + void addResourceInterest(const std::set&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } private: void refresh(); diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 907bf9148adf2..2be7cd4fddd72 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -62,7 +62,7 @@ void GrpcMuxImpl::sendDiscoveryRequest(const std::string& type_url) { GrpcMuxWatchPtr GrpcMuxImpl::addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder) { + OpaqueResourceDecoder& resource_decoder, const bool) { auto watch = std::make_unique(resources, callbacks, resource_decoder, type_url, *this); ENVOY_LOG(debug, "gRPC mux addWatch for " + type_url); diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 232559ad21674..34f936a054e87 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -45,9 +45,10 @@ class GrpcMuxImpl : public GrpcMux, ScopedResume pause(const std::string& type_url) override; ScopedResume pause(const std::vector type_urls) override; + GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder) override; + OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching = false) override; void handleDiscoveryResponse( std::unique_ptr&& message); @@ -98,6 +99,10 @@ class GrpcMuxImpl : public GrpcMux, parent_.queueDiscoveryRequest(type_url_); } + void add(const std::set&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + std::set resources_; SubscriptionCallbacks& callbacks_; OpaqueResourceDecoder& resource_decoder_; diff --git a/source/common/config/grpc_subscription_impl.cc b/source/common/config/grpc_subscription_impl.cc index ef8037f250064..9316f3514f794 100644 --- a/source/common/config/grpc_subscription_impl.cc +++ b/source/common/config/grpc_subscription_impl.cc @@ -19,7 +19,7 @@ GrpcSubscriptionImpl::GrpcSubscriptionImpl( init_fetch_timeout_(init_fetch_timeout), is_aggregated_(is_aggregated) {} // Config::Subscription -void GrpcSubscriptionImpl::start(const std::set& resources) { +void GrpcSubscriptionImpl::start(const std::set& resources, const bool use_prefix_matching) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, @@ -28,7 +28,7 @@ void GrpcSubscriptionImpl::start(const std::set& resources) { init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } - watch_ = grpc_mux_->addWatch(type_url_, resources, *this, resource_decoder_); + watch_ = grpc_mux_->addWatch(type_url_, resources, *this, resource_decoder_, use_prefix_matching); // 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 @@ -48,6 +48,12 @@ void GrpcSubscriptionImpl::updateResourceInterest( stats_.update_attempt_.inc(); } +void GrpcSubscriptionImpl::addResourceInterest( + const std::set& add_these_names) { + watch_->add(add_these_names); + stats_.update_attempt_.inc(); +} + // Config::SubscriptionCallbacks void GrpcSubscriptionImpl::onConfigUpdate(const std::vector& resources, const std::string& version_info) { diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index a5102055a08ce..65be4de30badd 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -24,9 +24,9 @@ class GrpcSubscriptionImpl : public Subscription, std::chrono::milliseconds init_fetch_timeout, bool is_aggregated); // Config::Subscription - void start(const std::set& resource_names) override; + void start(const std::set& resource_names, const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; - + void addResourceInterest(const std::set& add_these_names) override; // Config::SubscriptionCallbacks (all pass through to callbacks_!) void onConfigUpdate(const std::vector& resources, const std::string& version_info) override; diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 8c0d55d5e7494..9c4616766fede 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -43,7 +43,7 @@ HttpSubscriptionImpl::HttpSubscriptionImpl( } // Config::Subscription -void HttpSubscriptionImpl::start(const std::set& resource_names) { +void HttpSubscriptionImpl::start(const std::set& resource_names, const bool) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { handleFailure(Config::ConfigUpdateFailureReason::FetchTimedout, nullptr); diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index ec3d2e6ad0de3..d0af067415151 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -34,8 +34,11 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, ProtobufMessage::ValidationVisitor& validation_visitor); // Config::Subscription - void start(const std::set& resource_names) override; + void start(const std::set& resource_names, const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; + void addResourceInterest(const std::set&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } // Http::RestApiFetcher void createRequest(Http::RequestMessage& request) override; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 131ccd24db51b..520b0421edd27 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -63,7 +63,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( // resource names. for (const auto& r : message->resources()) { if (r.aliases_size() > 0) { - AddedRemoved converted = sub->second->watch_map_.convertAliasWatchesToNameWatches(r); + AddedRemoved converted = sub->second->watch_map_.removeAliasWatches(r); sub->second->sub_state_.updateSubscriptionInterest(converted.added_, converted.removed_); } } @@ -109,15 +109,16 @@ void NewGrpcMuxImpl::kickOffAck(UpdateAck ack) { // TODO(fredlas) to be removed from the GrpcMux interface very soon. void NewGrpcMuxImpl::start() { grpc_stream_.establishNewStream(); } +// TODO (dmitri-d) verify that a prefix-matching watch isn't set up empty GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder) { + OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching) { 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); - return addWatch(type_url, resources, callbacks, resource_decoder); + addSubscription(type_url, use_prefix_matching); + return addWatch(type_url, resources, callbacks, resource_decoder, use_prefix_matching); } Watch* watch = entry->second->watch_map_.addWatch(callbacks, resource_decoder); @@ -143,6 +144,17 @@ void NewGrpcMuxImpl::updateWatch(const std::string& type_url, Watch* watch, } } +void NewGrpcMuxImpl::addToWatch(const std::string& type_url, Watch* watch, + const std::set& to_add) { + ASSERT(watch != nullptr); + std::set with_added; + std::set_union(to_add.begin(), to_add.end(), + watch->resource_names_.begin(), watch->resource_names_.end(), + std::inserter(with_added, with_added.begin())); + + updateWatch(type_url, watch, with_added); +} + void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { updateWatch(type_url, watch, {}); auto entry = subscriptions_.find(type_url); @@ -151,8 +163,8 @@ void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { entry->second->watch_map_.removeWatch(watch); } -void NewGrpcMuxImpl::addSubscription(const std::string& type_url) { - subscriptions_.emplace(type_url, std::make_unique(type_url, local_info_)); +void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use_prefix_matching) { + subscriptions_.emplace(type_url, std::make_unique(type_url, local_info_, use_prefix_matching)); subscription_ordering_.emplace_back(type_url); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 431106a4dd399..63b26b43c6576 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -39,7 +39,7 @@ class NewGrpcMuxImpl GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder) override; + OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching = false) override; ScopedResume pause(const std::string& type_url) override; ScopedResume pause(const std::vector type_urls) override; @@ -60,8 +60,8 @@ class NewGrpcMuxImpl void start() override; struct SubscriptionStuff { - SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info) - : sub_state_(type_url, watch_map_, local_info) {} + SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, const bool use_prefix_matching) + : sub_state_(type_url, watch_map_, local_info, use_prefix_matching) {} WatchMap watch_map_; DeltaSubscriptionState sub_state_; @@ -96,6 +96,10 @@ class NewGrpcMuxImpl parent_.updateWatch(type_url_, watch_, resources); } + void add(const std::set& resources) override { + parent_.addToWatch(type_url_, watch_, resources); + } + private: const std::string type_url_; Watch* watch_; @@ -110,7 +114,10 @@ class NewGrpcMuxImpl void updateWatch(const std::string& type_url, Watch* watch, const std::set& resources); - void addSubscription(const std::string& type_url); + void addToWatch(const std::string& type_url, Watch* watch, + const std::set& to_add); + + void addSubscription(const std::string& type_url, const bool use_prefix_matching); void trySendDiscoveryRequests(); diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 51e73e06344d9..31e4634f87906 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -58,9 +58,26 @@ AddedRemoved WatchMap::updateWatchInterest(Watch* watch, findRemovals(newly_removed_from_watch, watch)); } -absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name) { - absl::flat_hash_set ret = wildcard_watches_; - const auto watches_interested = watch_interest_.find(resource_name); +absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name, const bool use_prefix_matching) { + absl::flat_hash_set ret; + if (!use_prefix_matching) { + ret = wildcard_watches_; + } + + // TODO (dmitri-d) we need to validate the names of resources added by VHDS for presence of a prefix + const auto resource_key = use_prefix_matching ? prefixFromName(resource_name) : resource_name; + + std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- resource_key: " << resource_key << "\n"; + + if (use_prefix_matching & resource_key.empty()) { + ENVOY_LOG(warn, "Expected a prefix in the resource name {}", resource_name); + return ret; + } + + for (const auto aa : watch_interest_) { + std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- watch_interest_: " << aa.first << "\n"; + } + const auto watches_interested = watch_interest_.find(resource_key); if (watches_interested != watch_interest_.end()) { for (const auto& watch : watches_interested->second) { ret.insert(watch); @@ -88,7 +105,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField decoded_resources.emplace_back( new DecodedResourceImpl((*watches_.begin())->resource_decoder_, r, version_info)); const absl::flat_hash_set& interested_in_r = - watchesInterestedIn(decoded_resources.back()->name()); + watchesInterestedIn(decoded_resources.back()->name(), false); for (const auto& interested_watch : interested_in_r) { per_watch_updates[interested_watch].emplace_back(*decoded_resources.back()); } @@ -122,7 +139,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField // For responses to on-demand requests, replace the original watch for an alias // with one for the resource's name -AddedRemoved WatchMap::convertAliasWatchesToNameWatches( +AddedRemoved WatchMap::removeAliasWatches( const envoy::service::discovery::v3::Resource& resource) { absl::flat_hash_set watches_to_update; for (const auto& alias : resource.aliases()) { @@ -136,7 +153,25 @@ AddedRemoved WatchMap::convertAliasWatchesToNameWatches( auto ret = AddedRemoved({}, {}); for (const auto& watch : watches_to_update) { - const auto& converted_watches = updateWatchInterest(watch, {resource.name()}); + std::set without_alias; + + for (const auto rn : watch->resource_names_) { + std::cout << "RRRRRRRRRRRRRRRRRRRRRRRRRRRR " << rn << "\n"; + } + for (const auto an: resource.aliases()) { + std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAN " << an << "\n"; + } + + + std::set_difference(watch->resource_names_.begin(), watch->resource_names_.end(), + resource.aliases().begin(), resource.aliases().end(), + std::inserter(without_alias, without_alias.begin())); + + RELEASE_ASSERT( + !without_alias.empty(), + fmt::format("WatchMap: prefix watch cannot be converted to a wildcard one.")); + + const auto& converted_watches = updateWatchInterest(watch, without_alias); std::copy(converted_watches.added_.begin(), converted_watches.added_.end(), std::inserter(ret.added_, ret.added_.end())); std::copy(converted_watches.removed_.begin(), converted_watches.removed_.end(), @@ -149,7 +184,8 @@ AddedRemoved WatchMap::convertAliasWatchesToNameWatches( void WatchMap::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info) { + const std::string& system_version_info, const bool use_prefix_matching) { + std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa WatchMap::onConfigUpdate\n"; // Track any removals triggered by earlier watch updates. ASSERT(deferred_removed_during_update_ == nullptr); deferred_removed_during_update_ = std::make_unique>(); @@ -160,9 +196,11 @@ void WatchMap::onConfigUpdate( std::vector decoded_resources; absl::flat_hash_map> per_watch_added; for (const auto& r : added_resources) { - const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r.name()); + const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r.name(), use_prefix_matching); // If there are no watches, then we don't need to decode. If there are watches, they should all // be for the same resource type, so we can just use the callbacks of the first watch to decode. + std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa use_prefix_matching: " << use_prefix_matching << "\n"; + std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa watches interested in " << r.name() << ": " << interested_in_r.size() << "\n"; if (interested_in_r.empty()) { continue; } @@ -174,7 +212,7 @@ void WatchMap::onConfigUpdate( } absl::flat_hash_map> per_watch_removed; for (const auto& r : removed_resources) { - const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r); + const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r, use_prefix_matching); for (const auto& interested_watch : interested_in_r) { *per_watch_removed[interested_watch].Add() = r; } diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index f1f7d09294ed7..a690e58070ca2 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -81,7 +81,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable& resources, @@ -89,7 +89,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info) override; + const std::string& system_version_info, const bool use_prefix_matching) override; void onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) override; WatchMap(const WatchMap&) = delete; @@ -109,7 +109,13 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable watchesInterestedIn(const std::string& resource_name); + absl::flat_hash_set watchesInterestedIn(const std::string& resource_name, const bool use_prefix_matching); + + std::string prefixFromName(const std::string& resource_name) { + const auto pos = resource_name.find_last_of('/'); + // we are not interested in the "/" character in the prefix + return pos == std::string::npos ? "" : resource_name.substr(0, pos); + } absl::flat_hash_set> watches_; diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc index 144ee75d977bd..66c4821eb0e0c 100644 --- a/source/common/router/route_config_update_receiver_impl.cc +++ b/source/common/router/route_config_update_receiver_impl.cc @@ -48,6 +48,14 @@ bool RouteConfigUpdateReceiverImpl::onVhdsUpdate( return removed || updated || !resource_ids_in_last_update_.empty(); } +std::set RouteConfigUpdateReceiverImpl::vhdsVhosts() const { + std::set ret; + for (const auto& vhost : vhds_virtual_hosts_) { + ret.insert(vhost.first); + } + return ret; +} + void RouteConfigUpdateReceiverImpl::initializeRdsVhosts( const envoy::config::route::v3::RouteConfiguration& route_configuration) { rds_virtual_hosts_.clear(); diff --git a/source/common/router/route_config_update_receiver_impl.h b/source/common/router/route_config_update_receiver_impl.h index a0e44f7975da7..ac771ba3dea04 100644 --- a/source/common/router/route_config_update_receiver_impl.h +++ b/source/common/router/route_config_update_receiver_impl.h @@ -54,6 +54,7 @@ class RouteConfigUpdateReceiverImpl : public RouteConfigUpdateReceiver { const std::set& resourceIdsInLastVhdsUpdate() override { return resource_ids_in_last_update_; } + std::set vhdsVhosts() const override; private: TimeSource& time_source_; diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 31d5b9d27d251..25753e0d3f66d 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -33,7 +33,7 @@ VhdsSubscription::VhdsSubscription( config_update_info_->routeConfigName() + ".")), stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), init_target_(fmt::format("VhdsConfigSubscription {}", config_update_info_->routeConfigName()), - [this]() { subscription_->start({}); }), + [this]() { subscription_->start({config_update_info_->routeConfigName()}, true); }), route_config_providers_(route_config_providers) { const auto& config_source = config_update_info_->routeConfiguration() .vhds() @@ -51,7 +51,7 @@ VhdsSubscription::VhdsSubscription( } void VhdsSubscription::updateOnDemand(const std::string& with_route_config_name_prefix) { - subscription_->updateResourceInterest({with_route_config_name_prefix}); + subscription_->addResourceInterest({with_route_config_name_prefix}); } void VhdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, diff --git a/test/common/config/delta_subscription_state_test.cc b/test/common/config/delta_subscription_state_test.cc index 554ebe3884df0..5a0773b6d11bb 100644 --- a/test/common/config/delta_subscription_state_test.cc +++ b/test/common/config/delta_subscription_state_test.cc @@ -24,7 +24,7 @@ const char TypeUrl[] = "type.googleapis.com/envoy.api.v2.Cluster"; class DeltaSubscriptionStateTest : public testing::Test { protected: - DeltaSubscriptionStateTest() : state_(TypeUrl, callbacks_, local_info_) { + DeltaSubscriptionStateTest() : state_(TypeUrl, callbacks_, local_info_, false) { state_.updateSubscriptionInterest({"name1", "name2", "name3"}, {}); envoy::service::discovery::v3::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); @@ -44,7 +44,7 @@ class DeltaSubscriptionStateTest : public testing::Test { if (nonce.has_value()) { message.set_nonce(nonce.value()); } - EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)).Times(expect_config_update_call ? 1 : 0); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _, _)).Times(expect_config_update_call ? 1 : 0); return state_.handleResponse(message); } @@ -57,7 +57,7 @@ class DeltaSubscriptionStateTest : public testing::Test { *message.mutable_removed_resources() = removed_resources; message.set_system_version_info(version_info); message.set_nonce(nonce); - EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)).WillOnce(Throw(EnvoyException(error_message))); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _, _)).WillOnce(Throw(EnvoyException(error_message))); return state_.handleResponse(message); } diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index a35a38d577256..3357e04b8a992 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -118,7 +118,7 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithAliases) { setup(); const std::string& type_url = Config::TypeUrl::get().VirtualHost; - auto watch = grpc_mux_->addWatch(type_url, {"domain1.test"}, callbacks_, resource_decoder_); + auto watch = grpc_mux_->addWatch(type_url, {"prefix"}, callbacks_, resource_decoder_, true); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); grpc_mux_->start(); @@ -133,9 +133,9 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithAliases) { vhost.add_domains("domain2.test"); response->add_resources()->mutable_resource()->PackFrom(vhost); - response->mutable_resources()->at(0).set_name("vhost_1"); - response->mutable_resources()->at(0).add_aliases("domain1.test"); - response->mutable_resources()->at(0).add_aliases("domain2.test"); + response->mutable_resources()->at(0).set_name("prefix/vhost_1"); + response->mutable_resources()->at(0).add_aliases("prefix/domain1.test"); + response->mutable_resources()->at(0).add_aliases("prefix/domain2.test"); grpc_mux_->onDiscoveryResponse(std::move(response), control_plane_stats_); @@ -153,7 +153,7 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithNotFoundResponse) { setup(); const std::string& type_url = Config::TypeUrl::get().VirtualHost; - auto watch = grpc_mux_->addWatch(type_url, {"domain1.test"}, callbacks_, resource_decoder_); + auto watch = grpc_mux_->addWatch(type_url, {"prefix"}, callbacks_, resource_decoder_, true); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); grpc_mux_->start(); @@ -164,7 +164,7 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithNotFoundResponse) { response->add_resources(); response->mutable_resources()->at(0).set_name("not-found"); - response->mutable_resources()->at(0).add_aliases("domain1.test"); + response->mutable_resources()->at(0).add_aliases("prefix/domain1.test"); grpc_mux_->onDiscoveryResponse(std::move(response), control_plane_stats_); diff --git a/test/common/config/watch_map_test.cc b/test/common/config/watch_map_test.cc index 6749cb901a8ae..8d972a9e11604 100644 --- a/test/common/config/watch_map_test.cc +++ b/test/common/config/watch_map_test.cc @@ -102,7 +102,7 @@ void doDeltaAndSotwUpdate(WatchMap& watch_map, for (const auto& n : removed_names) { *removed_names_proto.Add() = n; } - watch_map.onConfigUpdate(delta_resources, removed_names_proto, version); + watch_map.onConfigUpdate(delta_resources, removed_names_proto, version, false); } // Tests the simple case of a single watch. Checks that the watch will not be told of updates to @@ -290,7 +290,7 @@ TEST_F(SameWatchRemoval, SameWatchRemovalDeltaAdd) { EXPECT_CALL(callbacks2_, onConfigUpdate(_, _, _)) .Times(AtMost(1)) .WillRepeatedly(InvokeWithoutArgs([this] { removeAllInterest(); })); - watch_map_.onConfigUpdate(delta_resources, removed_names_proto, "version1"); + watch_map_.onConfigUpdate(delta_resources, removed_names_proto, "version1", false); } TEST_F(SameWatchRemoval, SameWatchRemovalDeltaRemove) { @@ -302,7 +302,7 @@ TEST_F(SameWatchRemoval, SameWatchRemovalDeltaRemove) { EXPECT_CALL(callbacks2_, onConfigUpdate(_, _, _)) .Times(AtMost(1)) .WillRepeatedly(InvokeWithoutArgs([this] { removeAllInterest(); })); - watch_map_.onConfigUpdate({}, removed_names_proto, "version1"); + watch_map_.onConfigUpdate({}, removed_names_proto, "version1", false); } // Checks the following: @@ -500,49 +500,48 @@ TEST(WatchMapTest, OnConfigUpdateFailed) { watch_map.onConfigUpdateFailed(ConfigUpdateFailureReason::UpdateRejected, nullptr); } -// verifies that a watch is updated with the resource name -TEST(WatchMapTest, ConvertAliasWatchesToNameWatches) { +// verifies that a watch for an alias is removed, while the watch for the prefix is kept +TEST(WatchMapTest, RemoveAliasWatches) { MockSubscriptionCallbacks callbacks; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); WatchMap watch_map; Watch* watch = watch_map.addWatch(callbacks, resource_decoder); - watch_map.updateWatchInterest(watch, {"alias"}); + watch_map.updateWatchInterest(watch, {"prefix", "prefix/alias"}); envoy::service::discovery::v3::Resource resource; - resource.set_name("resource"); + resource.set_name("prefix/resource"); resource.set_version("version"); - for (const auto alias : {"alias", "alias1", "alias2"}) { + for (const auto alias : {"prefix/alias", "prefix/alias1", "prefix/alias2"}) { resource.add_aliases(alias); } - AddedRemoved converted = watch_map.convertAliasWatchesToNameWatches(resource); + AddedRemoved converted = watch_map.removeAliasWatches(resource); - EXPECT_EQ(std::set{"resource"}, converted.added_); - EXPECT_EQ(std::set{"alias"}, converted.removed_); + EXPECT_EQ(std::set{"prefix/alias"}, converted.removed_); } -// verifies that if a resource contains an alias the same as its name, and the watch has been set -// with that alias, the watch won't be updated -TEST(WatchMapTest, ConvertAliasWatchesToNameWatchesAliasIsSameAsName) { +// verifies that a watch for an alias is removed, while the watch for the prefix is kept, even +// if the alias is the same as the resource name +TEST(WatchMapTest, RemoveAliasWatchesAliasIsSameAsName) { MockSubscriptionCallbacks callbacks; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); WatchMap watch_map; Watch* watch = watch_map.addWatch(callbacks, resource_decoder); - watch_map.updateWatchInterest(watch, {"name-and-alias"}); + watch_map.updateWatchInterest(watch, {"prefix", "prefix/name-and-alias"}); envoy::service::discovery::v3::Resource resource; - resource.set_name("name-and-alias"); + resource.set_name("prefix/name-and-alias"); resource.set_version("version"); - for (const auto alias : {"name-and-alias", "alias1", "alias2"}) { + for (const auto alias : {"prefix/name-and-alias", "prefix/alias1", "prefix/alias2"}) { resource.add_aliases(alias); } - AddedRemoved converted = watch_map.convertAliasWatchesToNameWatches(resource); + AddedRemoved converted = watch_map.removeAliasWatches(resource); EXPECT_TRUE(converted.added_.empty()); - EXPECT_TRUE(converted.removed_.empty()); + EXPECT_EQ(std::set{"prefix/name-and-alias"}, converted.removed_); } } // namespace diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index a33629239fb89..05c034fe03033 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -110,7 +110,7 @@ stat_prefix: foo validation_visitor_, outer_init_manager_, "foo.", *route_config_provider_manager_); rds_callbacks_ = server_factory_context_.cluster_manager_.subscription_factory_.callbacks_; EXPECT_CALL(*server_factory_context_.cluster_manager_.subscription_factory_.subscription_, - start(_)); + start(_, _)); outer_init_manager_.initialize(init_watcher_); } @@ -459,7 +459,7 @@ name: foo // Static + dynamic. setup(); EXPECT_CALL(*server_factory_context_.cluster_manager_.subscription_factory_.subscription_, - start(_)); + start(_, _)); outer_init_manager_.initialize(init_watcher_); const std::string response1_json = R"EOF( @@ -625,7 +625,7 @@ name: foo_route_config TEST_F(RouteConfigProviderManagerImplTest, OnConfigUpdateEmpty) { setup(); EXPECT_CALL(*server_factory_context_.cluster_manager_.subscription_factory_.subscription_, - start(_)); + start(_, _)); outer_init_manager_.initialize(init_watcher_); EXPECT_CALL(init_watcher_, ready()); server_factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate({}, ""); @@ -634,7 +634,7 @@ TEST_F(RouteConfigProviderManagerImplTest, OnConfigUpdateEmpty) { TEST_F(RouteConfigProviderManagerImplTest, OnConfigUpdateWrongSize) { setup(); EXPECT_CALL(*server_factory_context_.cluster_manager_.subscription_factory_.subscription_, - start(_)); + start(_, _)); outer_init_manager_.initialize(init_watcher_); envoy::config::route::v3::RouteConfiguration route_config; const auto decoded_resources = TestUtility::decodeResources({route_config, route_config}); @@ -666,7 +666,7 @@ TEST_F(RouteConfigProviderManagerImplTest, ConfigDumpAfterConfigRejected) { // dynamic. setup(); EXPECT_CALL(*server_factory_context_.cluster_manager_.subscription_factory_.subscription_, - start(_)); + start(_, _)); outer_init_manager_.initialize(init_watcher_); const std::string response1_yaml = R"EOF( diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index b00466a702a49..61d308d4eb38a 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -138,9 +138,9 @@ class ScopedRdsTest : public ScopedRoutesTestBase { Envoy::Config::OpaqueResourceDecoder&) { auto ret = std::make_unique>(); rds_subscription_by_config_subscription_[ret.get()] = &callbacks; - EXPECT_CALL(*ret, start(_)) + EXPECT_CALL(*ret, start(_, _)) .WillOnce(Invoke( - [this, config_sub_addr = ret.get()](const std::set& resource_names) { + [this, config_sub_addr = ret.get()](const std::set& resource_names, const bool) { EXPECT_EQ(resource_names.size(), 1); auto iter = rds_subscription_by_config_subscription_.find(config_sub_addr); EXPECT_NE(iter, rds_subscription_by_config_subscription_.end()); diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index d300bd634e486..7bcab5bb2c3d2 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -848,7 +848,7 @@ class RtdsLoaderImplTest : public LoaderImplTest { generator_, validation_visitor_, *api_); loader_->initialize(cm_); for (auto* sub : rtds_subscriptions_) { - EXPECT_CALL(*sub, start(_)); + EXPECT_CALL(*sub, start(_, _)); } loader_->startRtdsSubscriptions(rtds_init_callback_.AsStdFunction()); diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index 388fd9ed5ca82..7f06cce138d26 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -39,7 +39,7 @@ class CdsApiImplTest : public testing::Test { cds_ = CdsApiImpl::create(cds_config, cm_, store_, validation_visitor_); cds_->setInitializedCb([this]() -> void { initialized_.ready(); }); - EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); + EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_, _)); cds_->initialize(); cds_callbacks_ = cm_.subscription_factory_.callbacks_; } diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 3aab5a54f9910..808cc6b353a9a 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -63,7 +63,7 @@ class EdsSpeedTest { )EOF", Envoy::Upstream::Cluster::InitializePhase::Secondary); - EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); + EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_, _)); cluster_->initialize([this] { initialized_ = true; }); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(testing::Return(&async_stream_)); subscription_->start({"fare"}); diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 144d29ad78a0e..aaa9a54850bbf 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -103,7 +103,7 @@ class EdsTest : public testing::Test { } void initialize() { - EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_)); + EXPECT_CALL(*cm_.subscription_factory_.subscription_, start(_, _)); cluster_->initialize([this] { initialized_ = true; }); } diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index efa74c6039775..e0ce9c06b88fb 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -214,11 +214,11 @@ TEST_P(VhdsInitializationTest, InitializeVhdsAfterRdsHasBeenInitialized) { vhds_stream_->startGrpcStream(); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"my_route"}, {}, vhds_stream_)); sendDeltaDiscoveryResponse( Config::TypeUrl::get().VirtualHost, {TestUtility::parseYaml( - fmt::format(VhostTemplate, "vhost_0", "vhost.first"))}, + fmt::format(VhostTemplate, "my_route/vhost_0", "vhost.first"))}, {}, "1", vhds_stream_); EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); @@ -249,19 +249,19 @@ class VhdsIntegrationTest : public HttpIntegrationTest, envoy::config::route::v3::VirtualHost buildVirtualHost() { return TestUtility::parseYaml( - virtualHostYaml("vhost_0", "host")); + virtualHostYaml("my_route/vhost_0", "host")); } std::vector buildVirtualHost1() { return {TestUtility::parseYaml( - virtualHostYaml("vhost_1", "vhost.first")), + virtualHostYaml("my_route/vhost_1", "vhost.first")), TestUtility::parseYaml( - virtualHostYaml("vhost_2", "vhost.second"))}; + virtualHostYaml("my_route/vhost_2", "vhost.second"))}; } envoy::config::route::v3::VirtualHost buildVirtualHost2() { return TestUtility::parseYaml( - virtualHostYaml("vhost_1", "vhost.first")); + virtualHostYaml("my_route/vhost_1", "vhost.first")); } // Overridden to insert this stuff into the initialize() at the very beginning of @@ -306,7 +306,7 @@ class VhdsIntegrationTest : public HttpIntegrationTest, vhds_stream_->startGrpcStream(); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"my_route"}, {}, vhds_stream_)); sendDeltaDiscoveryResponse( Config::TypeUrl::get().VirtualHost, {buildVirtualHost()}, {}, "1", vhds_stream_); EXPECT_TRUE( @@ -330,7 +330,7 @@ class VhdsIntegrationTest : public HttpIntegrationTest, response.set_system_version_info("system_version_info_this_is_a_test"); response.set_type_url(Config::TypeUrl::get().VirtualHost); auto* resource = response.add_resources(); - resource->set_name("cannot-resolve-alias"); + resource->set_name("my_route/cannot-resolve-alias"); resource->set_version(version); for (const auto& alias : aliases) { resource->add_aliases(alias); @@ -424,7 +424,7 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { // A spontaneous VHDS DiscoveryResponse removes newly added virtual hosts sendDeltaDiscoveryResponse( - Config::TypeUrl::get().VirtualHost, {}, {"vhost_1", "vhost_2"}, "3", vhds_stream_); + Config::TypeUrl::get().VirtualHost, {}, {"my_route/vhost_1", "my_route/vhost_2"}, "3", vhds_stream_); EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); @@ -487,7 +487,7 @@ TEST_P(VhdsIntegrationTest, RdsWithVirtualHostsVhdsVirtualHostAddUpdateRemove) { // A spontaneous VHDS DiscoveryResponse removes virtual hosts added via vhds sendDeltaDiscoveryResponse( - Config::TypeUrl::get().VirtualHost, {}, {"vhost_1", "vhost_2"}, "3", vhds_stream_); + Config::TypeUrl::get().VirtualHost, {}, {"my_route/vhost_1", "my_route/vhost_2"}, "3", vhds_stream_); EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); @@ -636,7 +636,7 @@ TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateFailToResolveOneAliasOutOfSeveral) vhds_stream_)); // Send an empty response back (the management server isn't aware of vhost.third) sendDeltaDiscoveryResponseWithUnresolvedAliases({buildVirtualHost2()}, {}, "4", vhds_stream_, - {"vhost.first"}, {"my_route/vhost.third"}); + {"my_route/vhost.first"}, {"my_route/vhost.third"}); response->waitForHeaders(); EXPECT_EQ("404", response->headers().getStatusValue()); diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index 29412fffbd068..664af28217044 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -55,15 +55,16 @@ class MockUntypedConfigUpdateCallbacks : public UntypedConfigUpdateCallbacks { void, onConfigUpdate, (const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info)); + const std::string& system_version_info, const bool use_prefix_matching)); MOCK_METHOD(void, onConfigUpdateFailed, (Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e)); }; class MockSubscription : public Subscription { public: - MOCK_METHOD(void, start, (const std::set& resources)); + MOCK_METHOD(void, start, (const std::set& resources, const bool use_prefix_matching)); MOCK_METHOD(void, updateResourceInterest, (const std::set& update_to_these_names)); + MOCK_METHOD(void, addResourceInterest, (const std::set& add_these_names)); }; class MockSubscriptionFactory : public SubscriptionFactory { @@ -107,7 +108,7 @@ class MockGrpcMux : public GrpcMux { MOCK_METHOD(GrpcMuxWatchPtr, addWatch, (const std::string& type_url, const std::set& resources, - SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder)); + SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching)); }; class MockGrpcStreamCallbacks diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 54a84886e832d..5b8efae5b756d 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -42,7 +42,7 @@ class LdsApiTest : public testing::Test { EXPECT_CALL(init_manager_, add(_)); lds_ = std::make_unique(lds_config, cluster_manager_, init_manager_, store_, listener_manager_, validation_visitor_); - EXPECT_CALL(*cluster_manager_.subscription_factory_.subscription_, start(_)); + EXPECT_CALL(*cluster_manager_.subscription_factory_.subscription_, start(_, _)); init_target_handle_->initialize(init_watcher_); lds_callbacks_ = cluster_manager_.subscription_factory_.callbacks_; } From c955209f1c2d107b3ed4b90293207a1a45af2069 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 25 Aug 2020 14:14:30 -0700 Subject: [PATCH 02/16] don't send prefix in the initial request on subscription start Signed-off-by: Dmitri Dolguikh --- source/common/config/grpc_mux_impl.h | 2 +- source/common/config/new_grpc_mux_impl.cc | 27 ++++++++------ source/common/config/new_grpc_mux_impl.h | 12 ++++--- source/common/config/watch_map.cc | 43 +++++++++++------------ source/common/config/watch_map.h | 6 ++-- test/integration/vhds_integration_test.cc | 13 ++++--- 6 files changed, 57 insertions(+), 46 deletions(-) diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 34f936a054e87..376f82e218370 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -164,7 +164,7 @@ class NullGrpcMuxImpl : public GrpcMux, } GrpcMuxWatchPtr addWatch(const std::string&, const std::set&, SubscriptionCallbacks&, - OpaqueResourceDecoder&) override { + OpaqueResourceDecoder&, const bool) override { ExceptionUtil::throwEnvoyException("ADS must be configured to support an ADS config source"); } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 520b0421edd27..e6679617cd25f 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -64,7 +64,6 @@ void NewGrpcMuxImpl::onDiscoveryResponse( for (const auto& r : message->resources()) { if (r.aliases_size() > 0) { AddedRemoved converted = sub->second->watch_map_.removeAliasWatches(r); - sub->second->sub_state_.updateSubscriptionInterest(converted.added_, converted.removed_); } } @@ -113,7 +112,8 @@ void NewGrpcMuxImpl::start() { grpc_stream_.establishNewStream(); } GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching) { + OpaqueResourceDecoder& resource_decoder, + const bool use_prefix_matching) { auto entry = subscriptions_.find(type_url); if (entry == subscriptions_.end()) { // We don't yet have a subscription for type_url! Make one! @@ -123,7 +123,7 @@ GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, Watch* watch = entry->second->watch_map_.addWatch(callbacks, resource_decoder); // updateWatch() queues a discovery request if any of 'resources' are not yet subscribed. - updateWatch(type_url, watch, resources); + updateWatch(type_url, watch, resources, use_prefix_matching); return std::make_unique(type_url, watch, *this); } @@ -131,13 +131,20 @@ GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, // 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) { + const std::set& resources, + bool creating_prefix_watch) { 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_); + if (creating_prefix_watch) { + // THis is to prevent sending out of requests that contain prefixes instead of resource names + sub->second->sub_state_.updateSubscriptionInterest({}, {}); + } else { + 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(); @@ -145,12 +152,11 @@ void NewGrpcMuxImpl::updateWatch(const std::string& type_url, Watch* watch, } void NewGrpcMuxImpl::addToWatch(const std::string& type_url, Watch* watch, - const std::set& to_add) { + const std::set& to_add) { ASSERT(watch != nullptr); std::set with_added; - std::set_union(to_add.begin(), to_add.end(), - watch->resource_names_.begin(), watch->resource_names_.end(), - std::inserter(with_added, with_added.begin())); + std::set_union(to_add.begin(), to_add.end(), watch->resource_names_.begin(), + watch->resource_names_.end(), std::inserter(with_added, with_added.begin())); updateWatch(type_url, watch, with_added); } @@ -164,7 +170,8 @@ void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { } void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use_prefix_matching) { - subscriptions_.emplace(type_url, std::make_unique(type_url, local_info_, use_prefix_matching)); + subscriptions_.emplace( + type_url, std::make_unique(type_url, local_info_, use_prefix_matching)); subscription_ordering_.emplace_back(type_url); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 63b26b43c6576..48fb8456d7d4e 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -39,7 +39,8 @@ class NewGrpcMuxImpl GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching = false) override; + OpaqueResourceDecoder& resource_decoder, + const bool use_prefix_matching = false) override; ScopedResume pause(const std::string& type_url) override; ScopedResume pause(const std::vector type_urls) override; @@ -60,7 +61,8 @@ class NewGrpcMuxImpl void start() override; struct SubscriptionStuff { - SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, const bool use_prefix_matching) + SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, + const bool use_prefix_matching) : sub_state_(type_url, watch_map_, local_info, use_prefix_matching) {} WatchMap watch_map_; @@ -112,10 +114,10 @@ class NewGrpcMuxImpl // 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); + const std::set& resources, + const bool creating_prefix_watch = false); - void addToWatch(const std::string& type_url, Watch* watch, - const std::set& to_add); + void addToWatch(const std::string& type_url, Watch* watch, const std::set& to_add); void addSubscription(const std::string& type_url, const bool use_prefix_matching); diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 31e4634f87906..806cde39592a4 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -58,24 +58,22 @@ AddedRemoved WatchMap::updateWatchInterest(Watch* watch, findRemovals(newly_removed_from_watch, watch)); } -absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name, const bool use_prefix_matching) { +absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name, + const bool use_prefix_matching) { absl::flat_hash_set ret; if (!use_prefix_matching) { ret = wildcard_watches_; } - // TODO (dmitri-d) we need to validate the names of resources added by VHDS for presence of a prefix - const auto resource_key = use_prefix_matching ? prefixFromName(resource_name) : resource_name; + const auto prefix = prefixFromName(resource_name); + const auto resource_key = use_prefix_matching && !prefix.empty() ? prefix : resource_name; - std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- resource_key: " << resource_key << "\n"; - - if (use_prefix_matching & resource_key.empty()) { - ENVOY_LOG(warn, "Expected a prefix in the resource name {}", resource_name); - return ret; - } + std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- resource_key: " + << resource_key << "\n"; for (const auto aa : watch_interest_) { - std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- watch_interest_: " << aa.first << "\n"; + std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- watch_interest_: " + << aa.first << "\n"; } const auto watches_interested = watch_interest_.find(resource_key); if (watches_interested != watch_interest_.end()) { @@ -139,8 +137,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField // For responses to on-demand requests, replace the original watch for an alias // with one for the resource's name -AddedRemoved WatchMap::removeAliasWatches( - const envoy::service::discovery::v3::Resource& resource) { +AddedRemoved WatchMap::removeAliasWatches(const envoy::service::discovery::v3::Resource& resource) { absl::flat_hash_set watches_to_update; for (const auto& alias : resource.aliases()) { const auto interested_watches = watch_interest_.find(alias); @@ -158,18 +155,16 @@ AddedRemoved WatchMap::removeAliasWatches( for (const auto rn : watch->resource_names_) { std::cout << "RRRRRRRRRRRRRRRRRRRRRRRRRRRR " << rn << "\n"; } - for (const auto an: resource.aliases()) { + for (const auto an : resource.aliases()) { std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAN " << an << "\n"; } - - std::set_difference(watch->resource_names_.begin(), watch->resource_names_.end(), + std::set_difference(watch->resource_names_.begin(), watch->resource_names_.end(), resource.aliases().begin(), resource.aliases().end(), std::inserter(without_alias, without_alias.begin())); - RELEASE_ASSERT( - !without_alias.empty(), - fmt::format("WatchMap: prefix watch cannot be converted to a wildcard one.")); + RELEASE_ASSERT(!without_alias.empty(), + fmt::format("WatchMap: prefix watch cannot be converted to a wildcard one.")); const auto& converted_watches = updateWatchInterest(watch, without_alias); std::copy(converted_watches.added_.begin(), converted_watches.added_.end(), @@ -196,11 +191,14 @@ void WatchMap::onConfigUpdate( std::vector decoded_resources; absl::flat_hash_map> per_watch_added; for (const auto& r : added_resources) { - const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r.name(), use_prefix_matching); + const absl::flat_hash_set& interested_in_r = + watchesInterestedIn(r.name(), use_prefix_matching); // If there are no watches, then we don't need to decode. If there are watches, they should all // be for the same resource type, so we can just use the callbacks of the first watch to decode. - std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa use_prefix_matching: " << use_prefix_matching << "\n"; - std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa watches interested in " << r.name() << ": " << interested_in_r.size() << "\n"; + std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa use_prefix_matching: " + << use_prefix_matching << "\n"; + std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa watches interested in " + << r.name() << ": " << interested_in_r.size() << "\n"; if (interested_in_r.empty()) { continue; } @@ -212,7 +210,8 @@ void WatchMap::onConfigUpdate( } absl::flat_hash_map> per_watch_removed; for (const auto& r : removed_resources) { - const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r, use_prefix_matching); + const absl::flat_hash_set& interested_in_r = + watchesInterestedIn(r, use_prefix_matching); for (const auto& interested_watch : interested_in_r) { *per_watch_removed[interested_watch].Add() = r; } diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index a690e58070ca2..af76ab2db6c40 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -80,8 +80,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable& resources, @@ -109,7 +108,8 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable watchesInterestedIn(const std::string& resource_name, const bool use_prefix_matching); + absl::flat_hash_set watchesInterestedIn(const std::string& resource_name, + const bool use_prefix_matching); std::string prefixFromName(const std::string& resource_name) { const auto pos = resource_name.find_last_of('/'); diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index e0ce9c06b88fb..e1b8b7620df66 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -214,7 +214,7 @@ TEST_P(VhdsInitializationTest, InitializeVhdsAfterRdsHasBeenInitialized) { vhds_stream_->startGrpcStream(); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"my_route"}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); sendDeltaDiscoveryResponse( Config::TypeUrl::get().VirtualHost, {TestUtility::parseYaml( @@ -306,7 +306,7 @@ class VhdsIntegrationTest : public HttpIntegrationTest, vhds_stream_->startGrpcStream(); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"my_route"}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); sendDeltaDiscoveryResponse( Config::TypeUrl::get().VirtualHost, {buildVirtualHost()}, {}, "1", vhds_stream_); EXPECT_TRUE( @@ -424,7 +424,8 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { // A spontaneous VHDS DiscoveryResponse removes newly added virtual hosts sendDeltaDiscoveryResponse( - Config::TypeUrl::get().VirtualHost, {}, {"my_route/vhost_1", "my_route/vhost_2"}, "3", vhds_stream_); + Config::TypeUrl::get().VirtualHost, {}, {"my_route/vhost_1", "my_route/vhost_2"}, "3", + vhds_stream_); EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); @@ -487,7 +488,8 @@ TEST_P(VhdsIntegrationTest, RdsWithVirtualHostsVhdsVirtualHostAddUpdateRemove) { // A spontaneous VHDS DiscoveryResponse removes virtual hosts added via vhds sendDeltaDiscoveryResponse( - Config::TypeUrl::get().VirtualHost, {}, {"my_route/vhost_1", "my_route/vhost_2"}, "3", vhds_stream_); + Config::TypeUrl::get().VirtualHost, {}, {"my_route/vhost_1", "my_route/vhost_2"}, "3", + vhds_stream_); EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); @@ -636,7 +638,8 @@ TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateFailToResolveOneAliasOutOfSeveral) vhds_stream_)); // Send an empty response back (the management server isn't aware of vhost.third) sendDeltaDiscoveryResponseWithUnresolvedAliases({buildVirtualHost2()}, {}, "4", vhds_stream_, - {"my_route/vhost.first"}, {"my_route/vhost.third"}); + {"my_route/vhost.first"}, + {"my_route/vhost.third"}); response->waitForHeaders(); EXPECT_EQ("404", response->headers().getStatusValue()); From 9eb06f889397a30cee78cb13aad4ef5db4559e0e Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 25 Aug 2020 15:11:31 -0700 Subject: [PATCH 03/16] Fixed formatting Signed-off-by: Dmitri Dolguikh --- include/envoy/config/grpc_mux.h | 3 ++- .../common/config/delta_subscription_state.cc | 6 ++++-- source/common/config/watch_map.cc | 20 ------------------- 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index df1c6e115af1b..1044e4278b563 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -101,7 +101,8 @@ class GrpcMux { virtual GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching) PURE; + OpaqueResourceDecoder& resource_decoder, + const bool use_prefix_matching) PURE; }; using GrpcMuxPtr = std::unique_ptr; diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 5ffa284f3819c..fd694cb0220a4 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -11,8 +11,10 @@ namespace Config { DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, const bool use_prefix_matching) - : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), use_prefix_matching_(use_prefix_matching) {} + const LocalInfo::LocalInfo& local_info, + const bool use_prefix_matching) + : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), + use_prefix_matching_(use_prefix_matching) {} void DeltaSubscriptionState::updateSubscriptionInterest(const std::set& cur_added, const std::set& cur_removed) { diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 806cde39592a4..912824402642e 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -67,14 +67,6 @@ absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& res const auto prefix = prefixFromName(resource_name); const auto resource_key = use_prefix_matching && !prefix.empty() ? prefix : resource_name; - - std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- resource_key: " - << resource_key << "\n"; - - for (const auto aa : watch_interest_) { - std::cout << "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCcc watchesInterestedIn -- watch_interest_: " - << aa.first << "\n"; - } const auto watches_interested = watch_interest_.find(resource_key); if (watches_interested != watch_interest_.end()) { for (const auto& watch : watches_interested->second) { @@ -152,13 +144,6 @@ AddedRemoved WatchMap::removeAliasWatches(const envoy::service::discovery::v3::R for (const auto& watch : watches_to_update) { std::set without_alias; - for (const auto rn : watch->resource_names_) { - std::cout << "RRRRRRRRRRRRRRRRRRRRRRRRRRRR " << rn << "\n"; - } - for (const auto an : resource.aliases()) { - std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAN " << an << "\n"; - } - std::set_difference(watch->resource_names_.begin(), watch->resource_names_.end(), resource.aliases().begin(), resource.aliases().end(), std::inserter(without_alias, without_alias.begin())); @@ -180,7 +165,6 @@ void WatchMap::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& system_version_info, const bool use_prefix_matching) { - std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa WatchMap::onConfigUpdate\n"; // Track any removals triggered by earlier watch updates. ASSERT(deferred_removed_during_update_ == nullptr); deferred_removed_during_update_ = std::make_unique>(); @@ -195,10 +179,6 @@ void WatchMap::onConfigUpdate( watchesInterestedIn(r.name(), use_prefix_matching); // If there are no watches, then we don't need to decode. If there are watches, they should all // be for the same resource type, so we can just use the callbacks of the first watch to decode. - std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa use_prefix_matching: " - << use_prefix_matching << "\n"; - std::cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa watches interested in " - << r.name() << ": " << interested_in_r.size() << "\n"; if (interested_in_r.empty()) { continue; } From b45a0719b4fc517f75164624d7e5814a237f76b7 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 25 Aug 2020 16:04:35 -0700 Subject: [PATCH 04/16] more format fixes Signed-off-by: Dmitri Dolguikh --- include/envoy/config/subscription.h | 3 ++- source/common/config/grpc_mux_impl.h | 8 +++---- .../common/config/grpc_subscription_impl.cc | 6 ++--- source/common/config/grpc_subscription_impl.h | 3 ++- source/common/config/http_subscription_impl.h | 3 ++- source/common/router/vhds.cc | 5 +++-- .../config/delta_subscription_state_test.cc | 3 ++- test/common/router/scoped_rds_test.cc | 22 +++++++++---------- test/mocks/config/mocks.h | 6 +++-- 9 files changed, 32 insertions(+), 27 deletions(-) diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index 760ce2d16a930..e9b19c700688c 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -175,7 +175,8 @@ class Subscription { * to fetch throughout the lifetime of the Subscription object. * @param resources set of resource names to fetch. */ - virtual void start(const std::set& resource_names, const bool use_prefix_matching = false) PURE; + virtual void start(const std::set& resource_names, + const bool use_prefix_matching = false) PURE; /** * Update the resources to fetch. diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 376f82e218370..70539e9c5bfc9 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -45,10 +45,10 @@ class GrpcMuxImpl : public GrpcMux, ScopedResume pause(const std::string& type_url) override; ScopedResume pause(const std::vector type_urls) override; - GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching = false) override; + OpaqueResourceDecoder& resource_decoder, + const bool use_prefix_matching = false) override; void handleDiscoveryResponse( std::unique_ptr&& message); @@ -99,9 +99,7 @@ class GrpcMuxImpl : public GrpcMux, parent_.queueDiscoveryRequest(type_url_); } - void add(const std::set&) override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; - } + void add(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } std::set resources_; SubscriptionCallbacks& callbacks_; diff --git a/source/common/config/grpc_subscription_impl.cc b/source/common/config/grpc_subscription_impl.cc index 9316f3514f794..5b9aeb965c5f9 100644 --- a/source/common/config/grpc_subscription_impl.cc +++ b/source/common/config/grpc_subscription_impl.cc @@ -19,7 +19,8 @@ GrpcSubscriptionImpl::GrpcSubscriptionImpl( init_fetch_timeout_(init_fetch_timeout), is_aggregated_(is_aggregated) {} // Config::Subscription -void GrpcSubscriptionImpl::start(const std::set& resources, const bool use_prefix_matching) { +void GrpcSubscriptionImpl::start(const std::set& resources, + const bool use_prefix_matching) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, @@ -48,8 +49,7 @@ void GrpcSubscriptionImpl::updateResourceInterest( stats_.update_attempt_.inc(); } -void GrpcSubscriptionImpl::addResourceInterest( - const std::set& add_these_names) { +void GrpcSubscriptionImpl::addResourceInterest(const std::set& add_these_names) { watch_->add(add_these_names); stats_.update_attempt_.inc(); } diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index 65be4de30badd..f6290f0f97cfb 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -24,7 +24,8 @@ class GrpcSubscriptionImpl : public Subscription, std::chrono::milliseconds init_fetch_timeout, bool is_aggregated); // Config::Subscription - void start(const std::set& resource_names, const bool use_prefix_matching = false) override; + void start(const std::set& resource_names, + const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; void addResourceInterest(const std::set& add_these_names) override; // Config::SubscriptionCallbacks (all pass through to callbacks_!) diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index d0af067415151..d0bb486d05ef5 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -34,7 +34,8 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, ProtobufMessage::ValidationVisitor& validation_visitor); // Config::Subscription - void start(const std::set& resource_names, const bool use_prefix_matching = false) override; + void start(const std::set& resource_names, + const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; void addResourceInterest(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 25753e0d3f66d..5ac2a4ce94739 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -32,8 +32,9 @@ VhdsSubscription::VhdsSubscription( 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({config_update_info_->routeConfigName()}, true); }), + init_target_( + fmt::format("VhdsConfigSubscription {}", config_update_info_->routeConfigName()), + [this]() { subscription_->start({config_update_info_->routeConfigName()}, true); }), route_config_providers_(route_config_providers) { const auto& config_source = config_update_info_->routeConfiguration() .vhds() diff --git a/test/common/config/delta_subscription_state_test.cc b/test/common/config/delta_subscription_state_test.cc index 5a0773b6d11bb..8793f5dcbdf87 100644 --- a/test/common/config/delta_subscription_state_test.cc +++ b/test/common/config/delta_subscription_state_test.cc @@ -57,7 +57,8 @@ class DeltaSubscriptionStateTest : public testing::Test { *message.mutable_removed_resources() = removed_resources; message.set_system_version_info(version_info); message.set_nonce(nonce); - EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _, _)).WillOnce(Throw(EnvoyException(error_message))); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _, _)) + .WillOnce(Throw(EnvoyException(error_message))); return state_.handleResponse(message); } diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 61d308d4eb38a..f2b9c1ae93a62 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -132,22 +132,22 @@ class ScopedRdsTest : public ScopedRoutesTestBase { API_NO_BOOST(envoy::api::v2::RouteConfiguration)().GetDescriptor()->full_name())), _, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([this](const envoy::config::core::v3::ConfigSource&, - absl::string_view, Stats::Scope&, - Envoy::Config::SubscriptionCallbacks& callbacks, - Envoy::Config::OpaqueResourceDecoder&) { - auto ret = std::make_unique>(); - rds_subscription_by_config_subscription_[ret.get()] = &callbacks; - EXPECT_CALL(*ret, start(_, _)) - .WillOnce(Invoke( - [this, config_sub_addr = ret.get()](const std::set& resource_names, const bool) { + .WillRepeatedly( + Invoke([this](const envoy::config::core::v3::ConfigSource&, absl::string_view, + Stats::Scope&, Envoy::Config::SubscriptionCallbacks& callbacks, + Envoy::Config::OpaqueResourceDecoder&) { + auto ret = std::make_unique>(); + rds_subscription_by_config_subscription_[ret.get()] = &callbacks; + EXPECT_CALL(*ret, start(_, _)) + .WillOnce(Invoke([this, config_sub_addr = ret.get()]( + const std::set& resource_names, const bool) { EXPECT_EQ(resource_names.size(), 1); auto iter = rds_subscription_by_config_subscription_.find(config_sub_addr); EXPECT_NE(iter, rds_subscription_by_config_subscription_.end()); rds_subscription_by_name_[*resource_names.begin()] = iter->second; })); - return ret; - })); + return ret; + })); ON_CALL(context_init_manager_, add(_)).WillByDefault(Invoke([this](const Init::Target& target) { target_handles_.push_back(target.createHandle("test")); diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index 664af28217044..bb77e7f6e9b93 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -62,7 +62,8 @@ class MockUntypedConfigUpdateCallbacks : public UntypedConfigUpdateCallbacks { class MockSubscription : public Subscription { public: - MOCK_METHOD(void, start, (const std::set& resources, const bool use_prefix_matching)); + MOCK_METHOD(void, start, + (const std::set& resources, const bool use_prefix_matching)); MOCK_METHOD(void, updateResourceInterest, (const std::set& update_to_these_names)); MOCK_METHOD(void, addResourceInterest, (const std::set& add_these_names)); }; @@ -108,7 +109,8 @@ class MockGrpcMux : public GrpcMux { MOCK_METHOD(GrpcMuxWatchPtr, addWatch, (const std::string& type_url, const std::set& resources, - SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching)); + SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, + const bool use_prefix_matching)); }; class MockGrpcStreamCallbacks From 8dcc3cd367c7e66ca5b0f6258af3429adfa9adbb Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 26 Aug 2020 13:33:25 -0700 Subject: [PATCH 05/16] Change mux interface to support requesting of on-demand updates directly Signed-off-by: Dmitri Dolguikh --- include/envoy/config/grpc_mux.h | 5 +++-- include/envoy/config/subscription.h | 2 +- .../config/filesystem_subscription_impl.h | 2 +- source/common/config/grpc_mux_impl.h | 10 ++++++++-- source/common/config/grpc_subscription_impl.cc | 4 ++-- source/common/config/grpc_subscription_impl.h | 2 +- source/common/config/http_subscription_impl.h | 2 +- source/common/config/new_grpc_mux_impl.cc | 18 ++++++++++-------- source/common/config/new_grpc_mux_impl.h | 9 +++------ source/common/router/vhds.cc | 2 +- test/mocks/config/mocks.h | 5 ++++- 11 files changed, 35 insertions(+), 26 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 1044e4278b563..d7723ef2f39be 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -43,8 +43,6 @@ class GrpcMuxWatch { * @param resources set of resource names to watch for */ virtual void update(const std::set& resources) PURE; - - virtual void add(const std::set& resources) PURE; }; using GrpcMuxWatchPtr = std::unique_ptr; @@ -103,6 +101,9 @@ class GrpcMux { SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching) PURE; + + virtual void requestOnDemandUpdate(const std::string& type_url, + const std::set& for_update) PURE; }; using GrpcMuxPtr = std::unique_ptr; diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index e9b19c700688c..c92540d664f1d 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -185,7 +185,7 @@ class Subscription { */ virtual void updateResourceInterest(const std::set& update_to_these_names) PURE; - virtual void addResourceInterest(const std::set& add_these_names) PURE; + virtual void requestOnDemandUpdate(const std::set& add_these_names) PURE; }; using SubscriptionPtr = std::unique_ptr; diff --git a/source/common/config/filesystem_subscription_impl.h b/source/common/config/filesystem_subscription_impl.h index ae090c160abb4..8a96ea151224d 100644 --- a/source/common/config/filesystem_subscription_impl.h +++ b/source/common/config/filesystem_subscription_impl.h @@ -29,7 +29,7 @@ class FilesystemSubscriptionImpl : public Config::Subscription, // unused, and updateResourceInterest is a no-op (other than updating a stat). void start(const std::set&, const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set&) override; - void addResourceInterest(const std::set&) override { + void requestOnDemandUpdate(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 70539e9c5bfc9..7badc229c2e5c 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -50,6 +50,10 @@ class GrpcMuxImpl : public GrpcMux, OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching = false) override; + void requestOnDemandUpdate(const std::string&, const std::set&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void handleDiscoveryResponse( std::unique_ptr&& message); @@ -99,8 +103,6 @@ class GrpcMuxImpl : public GrpcMux, parent_.queueDiscoveryRequest(type_url_); } - void add(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - std::set resources_; SubscriptionCallbacks& callbacks_; OpaqueResourceDecoder& resource_decoder_; @@ -166,6 +168,10 @@ class NullGrpcMuxImpl : public GrpcMux, ExceptionUtil::throwEnvoyException("ADS must be configured to support an ADS config source"); } + void requestOnDemandUpdate(const std::string&, const std::set&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void onWriteable() override {} void onStreamEstablished() override {} void onEstablishmentFailure() override {} diff --git a/source/common/config/grpc_subscription_impl.cc b/source/common/config/grpc_subscription_impl.cc index 5b9aeb965c5f9..4ff9008da8306 100644 --- a/source/common/config/grpc_subscription_impl.cc +++ b/source/common/config/grpc_subscription_impl.cc @@ -49,8 +49,8 @@ void GrpcSubscriptionImpl::updateResourceInterest( stats_.update_attempt_.inc(); } -void GrpcSubscriptionImpl::addResourceInterest(const std::set& add_these_names) { - watch_->add(add_these_names); +void GrpcSubscriptionImpl::requestOnDemandUpdate(const std::set& for_update) { + grpc_mux_->requestOnDemandUpdate(type_url_, for_update); stats_.update_attempt_.inc(); } diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index f6290f0f97cfb..5518f58b01bdc 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -27,7 +27,7 @@ class GrpcSubscriptionImpl : public Subscription, void start(const std::set& resource_names, const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; - void addResourceInterest(const std::set& add_these_names) override; + void requestOnDemandUpdate(const std::set& add_these_names) override; // Config::SubscriptionCallbacks (all pass through to callbacks_!) void onConfigUpdate(const std::vector& resources, const std::string& version_info) override; diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index d0bb486d05ef5..30874e425b4cc 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -37,7 +37,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, void start(const std::set& resource_names, const bool use_prefix_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; - void addResourceInterest(const std::set&) override { + void requestOnDemandUpdate(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index e6679617cd25f..5e1fa72d417d2 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -151,14 +151,16 @@ void NewGrpcMuxImpl::updateWatch(const std::string& type_url, Watch* watch, } } -void NewGrpcMuxImpl::addToWatch(const std::string& type_url, Watch* watch, - const std::set& to_add) { - ASSERT(watch != nullptr); - std::set with_added; - std::set_union(to_add.begin(), to_add.end(), watch->resource_names_.begin(), - watch->resource_names_.end(), std::inserter(with_added, with_added.begin())); - - updateWatch(type_url, watch, with_added); +void NewGrpcMuxImpl::requestOnDemandUpdate(const std::string& type_url, + const std::set& for_update) { + auto sub = subscriptions_.find(type_url); + RELEASE_ASSERT(sub != subscriptions_.end(), + fmt::format("Watch of {} has no subscription to update.", type_url)); + sub->second->sub_state_.updateSubscriptionInterest(for_update, {}); + // Tell the server about our change in interest, if any. + if (sub->second->sub_state_.subscriptionUpdatePending()) { + trySendDiscoveryRequests(); + } } void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 48fb8456d7d4e..37652d5ff13b4 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -42,6 +42,9 @@ class NewGrpcMuxImpl OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching = false) override; + void requestOnDemandUpdate(const std::string& type_url, + const std::set& for_update) override; + ScopedResume pause(const std::string& type_url) override; ScopedResume pause(const std::vector type_urls) override; @@ -98,10 +101,6 @@ class NewGrpcMuxImpl parent_.updateWatch(type_url_, watch_, resources); } - void add(const std::set& resources) override { - parent_.addToWatch(type_url_, watch_, resources); - } - private: const std::string type_url_; Watch* watch_; @@ -117,8 +116,6 @@ class NewGrpcMuxImpl const std::set& resources, const bool creating_prefix_watch = false); - void addToWatch(const std::string& type_url, Watch* watch, const std::set& to_add); - void addSubscription(const std::string& type_url, const bool use_prefix_matching); void trySendDiscoveryRequests(); diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 5ac2a4ce94739..6038b42684b9c 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -52,7 +52,7 @@ VhdsSubscription::VhdsSubscription( } void VhdsSubscription::updateOnDemand(const std::string& with_route_config_name_prefix) { - subscription_->addResourceInterest({with_route_config_name_prefix}); + subscription_->requestOnDemandUpdate({with_route_config_name_prefix}); } void VhdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index bb77e7f6e9b93..e279312526da0 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -65,7 +65,7 @@ class MockSubscription : public Subscription { MOCK_METHOD(void, start, (const std::set& resources, const bool use_prefix_matching)); MOCK_METHOD(void, updateResourceInterest, (const std::set& update_to_these_names)); - MOCK_METHOD(void, addResourceInterest, (const std::set& add_these_names)); + MOCK_METHOD(void, requestOnDemandUpdate, (const std::set& add_these_names)); }; class MockSubscriptionFactory : public SubscriptionFactory { @@ -111,6 +111,9 @@ class MockGrpcMux : public GrpcMux { (const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, const bool use_prefix_matching)); + + MOCK_METHOD(void, requestOnDemandUpdate, + (const std::string& type_url, const std::set& add_these_names)); }; class MockGrpcStreamCallbacks From cef7dd7d1308056cab7cfccd47540ad01603a9ec Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 26 Aug 2020 16:28:54 -0700 Subject: [PATCH 06/16] removed removeAliasWatches() from watch_map Signed-off-by: Dmitri Dolguikh --- source/common/config/new_grpc_mux_impl.cc | 9 ----- source/common/config/watch_map.cc | 34 ------------------ source/common/config/watch_map.h | 3 -- test/common/config/watch_map_test.cc | 44 ----------------------- 4 files changed, 90 deletions(-) diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 5e1fa72d417d2..3e5000db89413 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -58,15 +58,6 @@ void NewGrpcMuxImpl::onDiscoveryResponse( return; } - // When an on-demand request is made a Watch is created using an alias, as the resource name isn't - // known at that point. When an update containing aliases comes back, we update Watches with - // resource names. - for (const auto& r : message->resources()) { - if (r.aliases_size() > 0) { - AddedRemoved converted = sub->second->watch_map_.removeAliasWatches(r); - } - } - kickOffAck(sub->second->sub_state_.handleResponse(*message)); Memory::Utils::tryShrinkHeap(); } diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 912824402642e..4d5f73af58708 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -127,40 +127,6 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField } } -// For responses to on-demand requests, replace the original watch for an alias -// with one for the resource's name -AddedRemoved WatchMap::removeAliasWatches(const envoy::service::discovery::v3::Resource& resource) { - absl::flat_hash_set watches_to_update; - for (const auto& alias : resource.aliases()) { - const auto interested_watches = watch_interest_.find(alias); - if (interested_watches != watch_interest_.end()) { - for (const auto& interested_watch : interested_watches->second) { - watches_to_update.insert(interested_watch); - } - } - } - - auto ret = AddedRemoved({}, {}); - for (const auto& watch : watches_to_update) { - std::set without_alias; - - std::set_difference(watch->resource_names_.begin(), watch->resource_names_.end(), - resource.aliases().begin(), resource.aliases().end(), - std::inserter(without_alias, without_alias.begin())); - - RELEASE_ASSERT(!without_alias.empty(), - fmt::format("WatchMap: prefix watch cannot be converted to a wildcard one.")); - - const auto& converted_watches = updateWatchInterest(watch, without_alias); - std::copy(converted_watches.added_.begin(), converted_watches.added_.end(), - std::inserter(ret.added_, ret.added_.end())); - std::copy(converted_watches.removed_.begin(), converted_watches.removed_.end(), - std::inserter(ret.removed_, ret.removed_.end())); - } - - return ret; -} - void WatchMap::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index af76ab2db6c40..e741851bc7f2c 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -79,9 +79,6 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable& resources, const std::string& version_info) override; diff --git a/test/common/config/watch_map_test.cc b/test/common/config/watch_map_test.cc index 8d972a9e11604..57b0f2b78acd7 100644 --- a/test/common/config/watch_map_test.cc +++ b/test/common/config/watch_map_test.cc @@ -500,50 +500,6 @@ TEST(WatchMapTest, OnConfigUpdateFailed) { watch_map.onConfigUpdateFailed(ConfigUpdateFailureReason::UpdateRejected, nullptr); } -// verifies that a watch for an alias is removed, while the watch for the prefix is kept -TEST(WatchMapTest, RemoveAliasWatches) { - MockSubscriptionCallbacks callbacks; - TestUtility::TestOpaqueResourceDecoderImpl - resource_decoder("cluster_name"); - WatchMap watch_map; - Watch* watch = watch_map.addWatch(callbacks, resource_decoder); - watch_map.updateWatchInterest(watch, {"prefix", "prefix/alias"}); - - envoy::service::discovery::v3::Resource resource; - resource.set_name("prefix/resource"); - resource.set_version("version"); - for (const auto alias : {"prefix/alias", "prefix/alias1", "prefix/alias2"}) { - resource.add_aliases(alias); - } - - AddedRemoved converted = watch_map.removeAliasWatches(resource); - - EXPECT_EQ(std::set{"prefix/alias"}, converted.removed_); -} - -// verifies that a watch for an alias is removed, while the watch for the prefix is kept, even -// if the alias is the same as the resource name -TEST(WatchMapTest, RemoveAliasWatchesAliasIsSameAsName) { - MockSubscriptionCallbacks callbacks; - TestUtility::TestOpaqueResourceDecoderImpl - resource_decoder("cluster_name"); - WatchMap watch_map; - Watch* watch = watch_map.addWatch(callbacks, resource_decoder); - watch_map.updateWatchInterest(watch, {"prefix", "prefix/name-and-alias"}); - - envoy::service::discovery::v3::Resource resource; - resource.set_name("prefix/name-and-alias"); - resource.set_version("version"); - for (const auto alias : {"prefix/name-and-alias", "prefix/alias1", "prefix/alias2"}) { - resource.add_aliases(alias); - } - - AddedRemoved converted = watch_map.removeAliasWatches(resource); - - EXPECT_TRUE(converted.added_.empty()); - EXPECT_EQ(std::set{"prefix/name-and-alias"}, converted.removed_); -} - } // namespace } // namespace Config } // namespace Envoy From c6db37a40c3f4c9729235ebf4b282fe1a9da090b Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Mon, 31 Aug 2020 15:43:28 -0700 Subject: [PATCH 07/16] Updated comments, fixed a test failing to build Signed-off-by: Dmitri Dolguikh --- include/envoy/config/grpc_mux.h | 3 ++- include/envoy/config/subscription.h | 10 ++++++++-- .../router/route_config_update_receiver.h | 2 -- .../common/config/delta_subscription_state.cc | 6 +++--- .../common/config/delta_subscription_state.h | 4 ++-- .../config/filesystem_subscription_impl.h | 2 +- source/common/config/grpc_mux_impl.h | 2 +- .../common/config/grpc_subscription_impl.cc | 4 ++-- source/common/config/grpc_subscription_impl.h | 2 +- source/common/config/http_subscription_impl.h | 2 +- source/common/config/new_grpc_mux_impl.cc | 19 +++++++++---------- source/common/config/new_grpc_mux_impl.h | 10 +++++----- source/common/config/watch_map.cc | 14 +++++++------- source/common/config/watch_map.h | 9 +++++---- .../route_config_update_receiver_impl.cc | 8 -------- .../route_config_update_receiver_impl.h | 1 - .../http/filter_config_discovery_impl_test.cc | 2 +- 17 files changed, 48 insertions(+), 52 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index d7723ef2f39be..f319b77d06452 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -93,6 +93,7 @@ class GrpcMux { * @param callbacks the callbacks to be notified of configuration updates. These must be valid * until GrpcMuxWatch is destroyed. * @param resource_decoder how incoming opaque resource objects are to be decoded. + * @param use_namespace_matching if namespace watch should be created. This is used for creating watches on collections of resources; individual members of a collection are identified by the namespace in resource name. * @return GrpcMuxWatchPtr a handle to cancel the subscription with. E.g. when a cluster goes * away, its EDS updates should be cancelled by destroying the GrpcMuxWatchPtr. */ @@ -100,7 +101,7 @@ class GrpcMux { const std::set& resources, SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, - const bool use_prefix_matching) PURE; + const bool use_namespace_matching) PURE; virtual void requestOnDemandUpdate(const std::string& type_url, const std::set& for_update) PURE; diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index c92540d664f1d..9147e55d4419c 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -146,12 +146,13 @@ class UntypedConfigUpdateCallbacks { * @param removed_resources names of resources that this fetch instructed to be removed. * @param system_version_info aggregate response data "version", for debugging. * @throw EnvoyException with reason if the config changes are rejected. Otherwise the changes + * @param use_namespace_matching if the resources should me matched on their namespaces, rather than unique names. This is used when a collection of resources (e.g. virtual hosts in VHDS) is being updated. * are accepted. Accepted changes have their version_info reflected in subsequent requests. */ virtual void onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_prefix_matching) PURE; + const std::string& system_version_info, const bool use_namespace_matching) PURE; /** * Called when either the Subscription is unable to fetch a config update or when onConfigUpdate @@ -174,9 +175,10 @@ class Subscription { * Start a configuration subscription asynchronously. This should be called once and will continue * to fetch throughout the lifetime of the Subscription object. * @param resources set of resource names to fetch. + * @param use_namespace_matching if the subscription is for a collection of resources. In such a case a namespace watch will be created. */ virtual void start(const std::set& resource_names, - const bool use_prefix_matching = false) PURE; + const bool use_namespace_matching = false) PURE; /** * Update the resources to fetch. @@ -185,6 +187,10 @@ class Subscription { */ virtual void updateResourceInterest(const std::set& update_to_these_names) PURE; + /** + * Creates a discovery request for resources. + * @param add_these_names resource ids for inclusion in the discovery request. + */ virtual void requestOnDemandUpdate(const std::set& add_these_names) PURE; }; diff --git a/include/envoy/router/route_config_update_receiver.h b/include/envoy/router/route_config_update_receiver.h index 6355f7f31804f..d18c6d5542529 100644 --- a/include/envoy/router/route_config_update_receiver.h +++ b/include/envoy/router/route_config_update_receiver.h @@ -92,8 +92,6 @@ class RouteConfigUpdateReceiver { * update. */ virtual const std::set& resourceIdsInLastVhdsUpdate() PURE; - - virtual std::set vhdsVhosts() const PURE; }; using RouteConfigUpdatePtr = std::unique_ptr; diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index fd694cb0220a4..643792668be6f 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -12,9 +12,9 @@ namespace Config { DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, const LocalInfo::LocalInfo& local_info, - const bool use_prefix_matching) + const bool use_namespace_matching) : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), - use_prefix_matching_(use_prefix_matching) {} + use_namespace_matching_(use_namespace_matching) {} void DeltaSubscriptionState::updateSubscriptionInterest(const std::set& cur_added, const std::set& cur_removed) { @@ -84,7 +84,7 @@ void DeltaSubscriptionState::handleGoodResponse( } } watch_map_.onConfigUpdate(message.resources(), message.removed_resources(), - message.system_version_info(), use_prefix_matching_); + message.system_version_info(), use_namespace_matching_); for (const auto& resource : message.resources()) { setResourceVersion(resource.name(), resource.version()); } diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 3ccbbbfb0efbc..1e4e929acd734 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -25,7 +25,7 @@ namespace Config { class DeltaSubscriptionState : public Logger::Loggable { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, const bool use_prefix_matching); + const LocalInfo::LocalInfo& local_info, const bool use_namespace_matching); // Update which resources we're interested in subscribing to. void updateSubscriptionInterest(const std::set& cur_added, @@ -100,7 +100,7 @@ class DeltaSubscriptionState : public Logger::Loggable { // Feel free to change to an unordered container once we figure out how to make it work. std::set names_added_; std::set names_removed_; - const bool use_prefix_matching_; + const bool use_namespace_matching_; }; } // namespace Config diff --git a/source/common/config/filesystem_subscription_impl.h b/source/common/config/filesystem_subscription_impl.h index 8a96ea151224d..eb23ffdc93306 100644 --- a/source/common/config/filesystem_subscription_impl.h +++ b/source/common/config/filesystem_subscription_impl.h @@ -27,7 +27,7 @@ class FilesystemSubscriptionImpl : public Config::Subscription, // Config::Subscription // We report all discovered resources in the watched file, so the resource names arguments are // unused, and updateResourceInterest is a no-op (other than updating a stat). - void start(const std::set&, const bool use_prefix_matching = false) override; + void start(const std::set&, const bool use_namespace_matching = false) override; void updateResourceInterest(const std::set&) override; void requestOnDemandUpdate(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 7badc229c2e5c..06acf1a78ef70 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -48,7 +48,7 @@ class GrpcMuxImpl : public GrpcMux, GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, - const bool use_prefix_matching = false) override; + const bool use_namespace_matching = false) override; void requestOnDemandUpdate(const std::string&, const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; diff --git a/source/common/config/grpc_subscription_impl.cc b/source/common/config/grpc_subscription_impl.cc index 4ff9008da8306..f92bd3cc7fcd0 100644 --- a/source/common/config/grpc_subscription_impl.cc +++ b/source/common/config/grpc_subscription_impl.cc @@ -20,7 +20,7 @@ GrpcSubscriptionImpl::GrpcSubscriptionImpl( // Config::Subscription void GrpcSubscriptionImpl::start(const std::set& resources, - const bool use_prefix_matching) { + const bool use_namespace_matching) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, @@ -29,7 +29,7 @@ void GrpcSubscriptionImpl::start(const std::set& resources, init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } - watch_ = grpc_mux_->addWatch(type_url_, resources, *this, resource_decoder_, use_prefix_matching); + watch_ = grpc_mux_->addWatch(type_url_, resources, *this, resource_decoder_, use_namespace_matching); // 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 diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index 5518f58b01bdc..a7fa247eeebf6 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -25,7 +25,7 @@ class GrpcSubscriptionImpl : public Subscription, // Config::Subscription void start(const std::set& resource_names, - const bool use_prefix_matching = false) override; + const bool use_namespace_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; void requestOnDemandUpdate(const std::set& add_these_names) override; // Config::SubscriptionCallbacks (all pass through to callbacks_!) diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index 30874e425b4cc..73bdae5094935 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -35,7 +35,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, // Config::Subscription void start(const std::set& resource_names, - const bool use_prefix_matching = false) override; + const bool use_namespace_matching = false) override; void updateResourceInterest(const std::set& update_to_these_names) override; void requestOnDemandUpdate(const std::set&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 3e5000db89413..277326d81d8d0 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -99,22 +99,21 @@ void NewGrpcMuxImpl::kickOffAck(UpdateAck ack) { // TODO(fredlas) to be removed from the GrpcMux interface very soon. void NewGrpcMuxImpl::start() { grpc_stream_.establishNewStream(); } -// TODO (dmitri-d) verify that a prefix-matching watch isn't set up empty GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, - const bool use_prefix_matching) { + const bool use_namespace_matching) { 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, use_prefix_matching); - return addWatch(type_url, resources, callbacks, resource_decoder, use_prefix_matching); + addSubscription(type_url, use_namespace_matching); + return addWatch(type_url, resources, callbacks, resource_decoder, use_namespace_matching); } Watch* watch = entry->second->watch_map_.addWatch(callbacks, resource_decoder); // updateWatch() queues a discovery request if any of 'resources' are not yet subscribed. - updateWatch(type_url, watch, resources, use_prefix_matching); + updateWatch(type_url, watch, resources, use_namespace_matching); return std::make_unique(type_url, watch, *this); } @@ -123,14 +122,14 @@ GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, // 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, - bool creating_prefix_watch) { + bool creating_namespace_watch) { 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); - if (creating_prefix_watch) { - // THis is to prevent sending out of requests that contain prefixes instead of resource names + if (creating_namespace_watch) { + // This is to prevent sending out of requests that contain prefixes instead of resource names sub->second->sub_state_.updateSubscriptionInterest({}, {}); } else { sub->second->sub_state_.updateSubscriptionInterest(added_removed.added_, @@ -162,9 +161,9 @@ void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { entry->second->watch_map_.removeWatch(watch); } -void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use_prefix_matching) { +void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use_namespace_matching) { subscriptions_.emplace( - type_url, std::make_unique(type_url, local_info_, use_prefix_matching)); + type_url, std::make_unique(type_url, local_info_, use_namespace_matching)); subscription_ordering_.emplace_back(type_url); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 37652d5ff13b4..1e0737fde3d08 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -40,7 +40,7 @@ class NewGrpcMuxImpl GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, - const bool use_prefix_matching = false) override; + const bool use_namespace_matching = false) override; void requestOnDemandUpdate(const std::string& type_url, const std::set& for_update) override; @@ -65,8 +65,8 @@ class NewGrpcMuxImpl struct SubscriptionStuff { SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, - const bool use_prefix_matching) - : sub_state_(type_url, watch_map_, local_info, use_prefix_matching) {} + const bool use_namespace_matching) + : sub_state_(type_url, watch_map_, local_info, use_namespace_matching) {} WatchMap watch_map_; DeltaSubscriptionState sub_state_; @@ -114,9 +114,9 @@ class NewGrpcMuxImpl // subscription will enqueue and attempt to send an appropriate discovery request. void updateWatch(const std::string& type_url, Watch* watch, const std::set& resources, - const bool creating_prefix_watch = false); + const bool creating_namespace_watch = false); - void addSubscription(const std::string& type_url, const bool use_prefix_matching); + void addSubscription(const std::string& type_url, const bool use_namespace_matching); void trySendDiscoveryRequests(); diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 4d5f73af58708..fa24d20f5f835 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -59,14 +59,14 @@ AddedRemoved WatchMap::updateWatchInterest(Watch* watch, } absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name, - const bool use_prefix_matching) { + const bool use_namespace_matching) { absl::flat_hash_set ret; - if (!use_prefix_matching) { + if (!use_namespace_matching) { ret = wildcard_watches_; } - const auto prefix = prefixFromName(resource_name); - const auto resource_key = use_prefix_matching && !prefix.empty() ? prefix : resource_name; + const auto prefix = namespaceFromName(resource_name); + const auto resource_key = use_namespace_matching && !prefix.empty() ? prefix : resource_name; const auto watches_interested = watch_interest_.find(resource_key); if (watches_interested != watch_interest_.end()) { for (const auto& watch : watches_interested->second) { @@ -130,7 +130,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField void WatchMap::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_prefix_matching) { + const std::string& system_version_info, const bool use_namespace_matching) { // Track any removals triggered by earlier watch updates. ASSERT(deferred_removed_during_update_ == nullptr); deferred_removed_during_update_ = std::make_unique>(); @@ -142,7 +142,7 @@ void WatchMap::onConfigUpdate( absl::flat_hash_map> per_watch_added; for (const auto& r : added_resources) { const absl::flat_hash_set& interested_in_r = - watchesInterestedIn(r.name(), use_prefix_matching); + watchesInterestedIn(r.name(), use_namespace_matching); // If there are no watches, then we don't need to decode. If there are watches, they should all // be for the same resource type, so we can just use the callbacks of the first watch to decode. if (interested_in_r.empty()) { @@ -157,7 +157,7 @@ void WatchMap::onConfigUpdate( absl::flat_hash_map> per_watch_removed; for (const auto& r : removed_resources) { const absl::flat_hash_set& interested_in_r = - watchesInterestedIn(r, use_prefix_matching); + watchesInterestedIn(r, use_namespace_matching); for (const auto& interested_watch : interested_in_r) { *per_watch_removed[interested_watch].Add() = r; } diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index e741851bc7f2c..0af4a16408adb 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -85,7 +85,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_prefix_matching) override; + const std::string& system_version_info, const bool use_namespace_matching) override; void onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) override; WatchMap(const WatchMap&) = delete; @@ -106,11 +106,12 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable watchesInterestedIn(const std::string& resource_name, - const bool use_prefix_matching); + const bool use_namespace_matching); - std::string prefixFromName(const std::string& resource_name) { + // Returns the namespace part (if there's any) in the resource name. + std::string namespaceFromName(const std::string& resource_name) { const auto pos = resource_name.find_last_of('/'); - // we are not interested in the "/" character in the prefix + // we are not interested in the "/" character in the namespace return pos == std::string::npos ? "" : resource_name.substr(0, pos); } diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc index 66c4821eb0e0c..144ee75d977bd 100644 --- a/source/common/router/route_config_update_receiver_impl.cc +++ b/source/common/router/route_config_update_receiver_impl.cc @@ -48,14 +48,6 @@ bool RouteConfigUpdateReceiverImpl::onVhdsUpdate( return removed || updated || !resource_ids_in_last_update_.empty(); } -std::set RouteConfigUpdateReceiverImpl::vhdsVhosts() const { - std::set ret; - for (const auto& vhost : vhds_virtual_hosts_) { - ret.insert(vhost.first); - } - return ret; -} - void RouteConfigUpdateReceiverImpl::initializeRdsVhosts( const envoy::config::route::v3::RouteConfiguration& route_configuration) { rds_virtual_hosts_.clear(); diff --git a/source/common/router/route_config_update_receiver_impl.h b/source/common/router/route_config_update_receiver_impl.h index ac771ba3dea04..a0e44f7975da7 100644 --- a/source/common/router/route_config_update_receiver_impl.h +++ b/source/common/router/route_config_update_receiver_impl.h @@ -54,7 +54,6 @@ class RouteConfigUpdateReceiverImpl : public RouteConfigUpdateReceiver { const std::set& resourceIdsInLastVhdsUpdate() override { return resource_ids_in_last_update_; } - std::set vhdsVhosts() const override; private: TimeSource& time_source_; diff --git a/test/common/filter/http/filter_config_discovery_impl_test.cc b/test/common/filter/http/filter_config_discovery_impl_test.cc index 2d7d7d0e00e61..99ae313cacae1 100644 --- a/test/common/filter/http/filter_config_discovery_impl_test.cc +++ b/test/common/filter/http/filter_config_discovery_impl_test.cc @@ -88,7 +88,7 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { void setup(bool warm = true) { provider_ = createProvider("foo", warm); callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; - EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_, _)); if (!warm) { EXPECT_CALL(init_watcher_, ready()); } From 28d9d591872c047942e9c82cc4bc04370487dee7 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Mon, 31 Aug 2020 15:48:14 -0700 Subject: [PATCH 08/16] Fixed formatting Signed-off-by: Dmitri Dolguikh --- include/envoy/config/grpc_mux.h | 4 +++- source/common/config/grpc_subscription_impl.cc | 3 ++- source/common/config/new_grpc_mux_impl.cc | 3 ++- test/common/filter/http/filter_config_discovery_impl_test.cc | 3 ++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index f319b77d06452..6c268d1076b0f 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -93,7 +93,9 @@ class GrpcMux { * @param callbacks the callbacks to be notified of configuration updates. These must be valid * until GrpcMuxWatch is destroyed. * @param resource_decoder how incoming opaque resource objects are to be decoded. - * @param use_namespace_matching if namespace watch should be created. This is used for creating watches on collections of resources; individual members of a collection are identified by the namespace in resource name. + * @param use_namespace_matching if namespace watch should be created. This is used for creating + * watches on collections of resources; individual members of a collection are identified by the + * namespace in resource name. * @return GrpcMuxWatchPtr a handle to cancel the subscription with. E.g. when a cluster goes * away, its EDS updates should be cancelled by destroying the GrpcMuxWatchPtr. */ diff --git a/source/common/config/grpc_subscription_impl.cc b/source/common/config/grpc_subscription_impl.cc index f92bd3cc7fcd0..83c21e2208dc4 100644 --- a/source/common/config/grpc_subscription_impl.cc +++ b/source/common/config/grpc_subscription_impl.cc @@ -29,7 +29,8 @@ void GrpcSubscriptionImpl::start(const std::set& resources, init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } - watch_ = grpc_mux_->addWatch(type_url_, resources, *this, resource_decoder_, use_namespace_matching); + watch_ = + grpc_mux_->addWatch(type_url_, resources, *this, resource_decoder_, use_namespace_matching); // 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 diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 277326d81d8d0..4b72f94fb8f11 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -161,7 +161,8 @@ void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { entry->second->watch_map_.removeWatch(watch); } -void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use_namespace_matching) { +void NewGrpcMuxImpl::addSubscription(const std::string& type_url, + const bool use_namespace_matching) { subscriptions_.emplace( type_url, std::make_unique(type_url, local_info_, use_namespace_matching)); subscription_ordering_.emplace_back(type_url); diff --git a/test/common/filter/http/filter_config_discovery_impl_test.cc b/test/common/filter/http/filter_config_discovery_impl_test.cc index 99ae313cacae1..89c668e423579 100644 --- a/test/common/filter/http/filter_config_discovery_impl_test.cc +++ b/test/common/filter/http/filter_config_discovery_impl_test.cc @@ -88,7 +88,8 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { void setup(bool warm = true) { provider_ = createProvider("foo", warm); callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; - EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_, _)); + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, + start(_, _)); if (!warm) { EXPECT_CALL(init_watcher_, ready()); } From cc442d5cd20a1df38fa2f8b4f3a4f71959072678 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 1 Sep 2020 10:59:56 -0700 Subject: [PATCH 09/16] format fixes Signed-off-by: Dmitri Dolguikh --- include/envoy/config/subscription.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index 9147e55d4419c..ec3c2ad3fb383 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -146,8 +146,10 @@ class UntypedConfigUpdateCallbacks { * @param removed_resources names of resources that this fetch instructed to be removed. * @param system_version_info aggregate response data "version", for debugging. * @throw EnvoyException with reason if the config changes are rejected. Otherwise the changes - * @param use_namespace_matching if the resources should me matched on their namespaces, rather than unique names. This is used when a collection of resources (e.g. virtual hosts in VHDS) is being updated. - * are accepted. Accepted changes have their version_info reflected in subsequent requests. + * @param use_namespace_matching if the resources should me matched on their namespaces, rather + * than unique names. This is used when a collection of resources (e.g. virtual hosts in VHDS) is + * being updated. are accepted. Accepted changes have their version_info reflected in subsequent + * requests. */ virtual void onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, @@ -175,7 +177,8 @@ class Subscription { * Start a configuration subscription asynchronously. This should be called once and will continue * to fetch throughout the lifetime of the Subscription object. * @param resources set of resource names to fetch. - * @param use_namespace_matching if the subscription is for a collection of resources. In such a case a namespace watch will be created. + * @param use_namespace_matching if the subscription is for a collection of resources. In such a + * case a namespace watch will be created. */ virtual void start(const std::set& resource_names, const bool use_namespace_matching = false) PURE; From 3254704bff42b978a5faa0008bdf3413998f142f Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 1 Sep 2020 13:06:07 -0700 Subject: [PATCH 10/16] updated watch_map tests Signed-off-by: Dmitri Dolguikh --- test/common/config/watch_map_test.cc | 119 +++++++++++++++++++++------ 1 file changed, 96 insertions(+), 23 deletions(-) diff --git a/test/common/config/watch_map_test.cc b/test/common/config/watch_map_test.cc index 57b0f2b78acd7..5f6d3d3dd1399 100644 --- a/test/common/config/watch_map_test.cc +++ b/test/common/config/watch_map_test.cc @@ -24,24 +24,10 @@ namespace Envoy { namespace Config { namespace { -// expectDeltaAndSotwUpdate() EXPECTs two birds with one function call: we want to cover both SotW -// and delta, which, while mechanically different, can behave identically for our testing purposes. -// Specifically, as a simplification for these tests, every still-present resource is updated in -// every update. Therefore, a resource can never show up in the SotW update but not the delta -// update. We can therefore use the same expected_resources for both. -void expectDeltaAndSotwUpdate( +void expectDeltaUpdate( MockSubscriptionCallbacks& callbacks, const std::vector& expected_resources, const std::vector& expected_removals, const std::string& version) { - EXPECT_CALL(callbacks, onConfigUpdate(_, version)) - .WillOnce(Invoke([expected_resources](const std::vector& gotten_resources, - const std::string&) { - EXPECT_EQ(expected_resources.size(), gotten_resources.size()); - for (size_t i = 0; i < expected_resources.size(); i++) { - EXPECT_TRUE( - TestUtility::protoEqual(gotten_resources[i].get().resource(), expected_resources[i])); - } - })); EXPECT_CALL(callbacks, onConfigUpdate(_, _, _)) .WillOnce(Invoke([expected_resources, expected_removals, version](const std::vector& gotten_resources, @@ -60,6 +46,27 @@ void expectDeltaAndSotwUpdate( })); } +// expectDeltaAndSotwUpdate() EXPECTs two birds with one function call: we want to cover both SotW +// and delta, which, while mechanically different, can behave identically for our testing purposes. +// Specifically, as a simplification for these tests, every still-present resource is updated in +// every update. Therefore, a resource can never show up in the SotW update but not the delta +// update. We can therefore use the same expected_resources for both. +void expectDeltaAndSotwUpdate( + MockSubscriptionCallbacks& callbacks, + const std::vector& expected_resources, + const std::vector& expected_removals, const std::string& version) { + EXPECT_CALL(callbacks, onConfigUpdate(_, version)) + .WillOnce(Invoke([expected_resources](const std::vector& gotten_resources, + const std::string&) { + EXPECT_EQ(expected_resources.size(), gotten_resources.size()); + for (size_t i = 0; i < expected_resources.size(); i++) { + EXPECT_TRUE( + TestUtility::protoEqual(gotten_resources[i].get().resource(), expected_resources[i])); + } + })); + expectDeltaUpdate(callbacks, expected_resources, expected_removals, version); +} + void expectNoUpdate(MockSubscriptionCallbacks& callbacks, const std::string& version) { EXPECT_CALL(callbacks, onConfigUpdate(_, version)).Times(0); EXPECT_CALL(callbacks, onConfigUpdate(_, _, version)).Times(0); @@ -88,13 +95,10 @@ wrapInResource(const Protobuf::RepeatedPtrField& anys, return ret; } -// Similar to expectDeltaAndSotwUpdate(), but making the onConfigUpdate() happen, rather than -// EXPECT-ing it. -void doDeltaAndSotwUpdate(WatchMap& watch_map, - const Protobuf::RepeatedPtrField& sotw_resources, - const std::vector& removed_names, - const std::string& version) { - watch_map.onConfigUpdate(sotw_resources, version); +void doDeltaUpdate(WatchMap& watch_map, + const Protobuf::RepeatedPtrField& sotw_resources, + const std::vector& removed_names, const std::string& version, + const bool use_namespace_matching) { Protobuf::RepeatedPtrField delta_resources = wrapInResource(sotw_resources, version); @@ -102,7 +106,17 @@ void doDeltaAndSotwUpdate(WatchMap& watch_map, for (const auto& n : removed_names) { *removed_names_proto.Add() = n; } - watch_map.onConfigUpdate(delta_resources, removed_names_proto, version, false); + watch_map.onConfigUpdate(delta_resources, removed_names_proto, version, use_namespace_matching); +} + +// Similar to expectDeltaAndSotwUpdate(), but making the onConfigUpdate() happen, rather than +// EXPECT-ing it. +void doDeltaAndSotwUpdate(WatchMap& watch_map, + const Protobuf::RepeatedPtrField& sotw_resources, + const std::vector& removed_names, + const std::string& version) { + watch_map.onConfigUpdate(sotw_resources, version); + doDeltaUpdate(watch_map, sotw_resources, removed_names, version, false); } // Tests the simple case of a single watch. Checks that the watch will not be told of updates to @@ -500,6 +514,65 @@ TEST(WatchMapTest, OnConfigUpdateFailed) { watch_map.onConfigUpdateFailed(ConfigUpdateFailureReason::UpdateRejected, nullptr); } +TEST(WatchMapTest, OnConfigUpdateUsingNamespaces) { + MockSubscriptionCallbacks callbacks1; + MockSubscriptionCallbacks callbacks2; + MockSubscriptionCallbacks callbacks3; + TestUtility::TestOpaqueResourceDecoderImpl + resource_decoder("cluster_name"); + WatchMap watch_map; + Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); + Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); + Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); + watch_map.updateWatchInterest(watch1, {"ns1"}); + watch_map.updateWatchInterest(watch2, {"ns1", "ns2"}); + watch_map.updateWatchInterest(watch3, {"ns3"}); + + // verify update + { + Protobuf::RepeatedPtrField update; + envoy::config::endpoint::v3::ClusterLoadAssignment resource; + resource.set_cluster_name("ns1/resource1"); + update.Add()->PackFrom(resource); + expectDeltaUpdate(callbacks1, {resource}, {}, "version0"); + expectDeltaUpdate(callbacks2, {resource}, {}, "version0"); + doDeltaUpdate(watch_map, update, {}, "version0", true); + } + // verify removal + { + Protobuf::RepeatedPtrField update; + expectDeltaUpdate(callbacks2, {}, {"ns2/removed"}, "version1"); + doDeltaUpdate(watch_map, update, {"ns2/removed"}, "version1", true); + } + // verify a not-found response to an on-demand request: such a response will contain an empty + // resource wrapper with the name and aliases fields containing the alias used in the request. + { + Protobuf::RepeatedPtrField empty_resources; + const auto version = "version3"; + const auto not_resolved = "ns3/not_resolved"; + + auto* cur_resource = empty_resources.Add(); + cur_resource->set_version(version); + cur_resource->set_name(not_resolved); + cur_resource->add_aliases(not_resolved); + + EXPECT_CALL(callbacks3, onConfigUpdate(_, _, _)) + .WillOnce(Invoke([not_resolved, version]( + const std::vector& gotten_resources, + const Protobuf::RepeatedPtrField&, const std::string&) { + EXPECT_EQ(1, gotten_resources.size()); + EXPECT_EQ(gotten_resources[0].get().version(), version); + EXPECT_FALSE(gotten_resources[0].get().hasResource()); + EXPECT_EQ(gotten_resources[0].get().name(), not_resolved); + EXPECT_EQ(gotten_resources[0].get().aliases(), std::vector{not_resolved}); + })); + + Protobuf::RepeatedPtrField removed_names_proto; + + watch_map.onConfigUpdate(empty_resources, removed_names_proto, "version2", true); + } +} + } // namespace } // namespace Config } // namespace Envoy From c1bf404a12264aadd7f50af76866e4c6481c59f8 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 1 Sep 2020 14:46:43 -0700 Subject: [PATCH 11/16] updated vhds integration test Signed-off-by: Dmitri Dolguikh --- test/integration/vhds_integration_test.cc | 96 +++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index e1b8b7620df66..673449254ed51 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -680,5 +680,101 @@ TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateHttpConnectionCloses) { cleanupUpstreamAndDownstream(); } +const char VhostTemplateAfterUpdate[] = R"EOF( +name: {} +domains: [{}] +routes: +- match: {{ prefix: "/after_update" }} + route: {{ cluster: "my_service" }} +)EOF"; + +// Verifies that after multiple vhds updates, vhosts from earlier updates still can receive updates +// See https://github.com/envoyproxy/envoy/issues/12158 for more details +TEST_P(VhdsIntegrationTest, MultipleUpdates) { + testRouterHeaderOnlyRequestAndResponse(nullptr, 1); + cleanupUpstreamAndDownstream(); + EXPECT_TRUE(codec_client_->waitForDisconnect()); + + { + // make first vhds request (for vhost.first) + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "vhost.first"}, + {"x-lyft-user-id", "123"}}; + IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); + EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, + {vhdsRequestResourceName("vhost.first")}, {}, + vhds_stream_)); + sendDeltaDiscoveryResponse( + Config::TypeUrl::get().VirtualHost, {buildVirtualHost2()}, {}, "4", vhds_stream_, + {"my_route/vhost.first"}); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + waitForNextUpstreamRequest(1); + // Send response headers, and end_stream if there is no response body. + upstream_request_->encodeHeaders(default_response_headers_, true); + + response->waitForHeaders(); + EXPECT_EQ("200", response->headers().getStatusValue()); + + cleanupUpstreamAndDownstream(); + EXPECT_TRUE(codec_client_->waitForDisconnect()); + } + { + // make second vhds request (for vhost.second) + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "vhost.second"}, + {"x-lyft-user-id", "123"}}; + IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); + EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, + {vhdsRequestResourceName("vhost.second")}, {}, + vhds_stream_)); + sendDeltaDiscoveryResponse( + Config::TypeUrl::get().VirtualHost, + {TestUtility::parseYaml( + virtualHostYaml("my_route/vhost_2", "vhost.second"))}, + {}, "4", vhds_stream_, {"my_route/vhost.second"}); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + waitForNextUpstreamRequest(1); + // Send response headers, and end_stream if there is no response body. + upstream_request_->encodeHeaders(default_response_headers_, true); + + response->waitForHeaders(); + EXPECT_EQ("200", response->headers().getStatusValue()); + + cleanupUpstreamAndDownstream(); + EXPECT_TRUE(codec_client_->waitForDisconnect()); + } + { + // Attempt to push updates for both vhost.first and vhost.second + sendDeltaDiscoveryResponse( + Config::TypeUrl::get().VirtualHost, + {TestUtility::parseYaml( + fmt::format(VhostTemplateAfterUpdate, "my_route/vhost_1", "vhost.first")), + TestUtility::parseYaml( + fmt::format(VhostTemplateAfterUpdate, "my_route/vhost_2", "vhost.second"))}, + {}, "5", vhds_stream_); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + // verify that both vhosts have been updated + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/after_update", "vhost.first"); + cleanupUpstreamAndDownstream(); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/after_update", "vhost.second"); + cleanupUpstreamAndDownstream(); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } +} + } // namespace } // namespace Envoy From bc158e4f12b353337299668287daca7ad44706ee Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 2 Sep 2020 10:33:16 -0700 Subject: [PATCH 12/16] Fixed spelling Signed-off-by: Dmitri Dolguikh --- test/integration/vhds_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index 673449254ed51..88d1f2432de00 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -688,8 +688,8 @@ domains: [{}] route: {{ cluster: "my_service" }} )EOF"; -// Verifies that after multiple vhds updates, vhosts from earlier updates still can receive updates -// See https://github.com/envoyproxy/envoy/issues/12158 for more details +// Verifies that after multiple vhds updates, virtual hosts from earlier updates still can receive +// updates See https://github.com/envoyproxy/envoy/issues/12158 for more details TEST_P(VhdsIntegrationTest, MultipleUpdates) { testRouterHeaderOnlyRequestAndResponse(nullptr, 1); cleanupUpstreamAndDownstream(); From da0f60d60e41004bfdd202032a7ab2c151a6242c Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 2 Sep 2020 12:43:22 -0700 Subject: [PATCH 13/16] More spelling fixes Signed-off-by: Dmitri Dolguikh --- test/integration/vhds_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index 88d1f2432de00..b0f0a4fc52473 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -765,7 +765,7 @@ TEST_P(VhdsIntegrationTest, MultipleUpdates) { EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); - // verify that both vhosts have been updated + // verify that both virtual hosts have been updated testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/after_update", "vhost.first"); cleanupUpstreamAndDownstream(); ASSERT_TRUE(codec_client_->waitForDisconnect()); From d9ed0902ed451af7435d67b24c77686786810921 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 3 Sep 2020 11:14:23 -0700 Subject: [PATCH 14/16] Responded to feedback Signed-off-by: Dmitri Dolguikh --- include/envoy/config/subscription.h | 2 +- .../common/config/delta_subscription_state.cc | 8 ++--- .../common/config/delta_subscription_state.h | 3 +- source/common/config/new_grpc_mux_impl.h | 2 +- source/common/config/watch_map.cc | 26 ++++++++------ source/common/config/watch_map.h | 16 +++------ test/common/config/watch_map_test.cc | 35 ++++++++++--------- test/mocks/config/mocks.h | 2 +- 8 files changed, 46 insertions(+), 48 deletions(-) diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index ec3c2ad3fb383..c283cc862070c 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -154,7 +154,7 @@ class UntypedConfigUpdateCallbacks { virtual void onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_namespace_matching) PURE; + const std::string& system_version_info) PURE; /** * Called when either the Subscription is unable to fetch a config update or when onConfigUpdate diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 643792668be6f..c0a6a5502cb09 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -11,10 +11,8 @@ namespace Config { DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, - const bool use_namespace_matching) - : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), - use_namespace_matching_(use_namespace_matching) {} + const LocalInfo::LocalInfo& local_info) + : type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info) {} void DeltaSubscriptionState::updateSubscriptionInterest(const std::set& cur_added, const std::set& cur_removed) { @@ -84,7 +82,7 @@ void DeltaSubscriptionState::handleGoodResponse( } } watch_map_.onConfigUpdate(message.resources(), message.removed_resources(), - message.system_version_info(), use_namespace_matching_); + message.system_version_info()); for (const auto& resource : message.resources()) { setResourceVersion(resource.name(), resource.version()); } diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 1e4e929acd734..1e21ba3a8efde 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -25,7 +25,7 @@ namespace Config { class DeltaSubscriptionState : public Logger::Loggable { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, const bool use_namespace_matching); + const LocalInfo::LocalInfo& local_info); // Update which resources we're interested in subscribing to. void updateSubscriptionInterest(const std::set& cur_added, @@ -100,7 +100,6 @@ class DeltaSubscriptionState : public Logger::Loggable { // Feel free to change to an unordered container once we figure out how to make it work. std::set names_added_; std::set names_removed_; - const bool use_namespace_matching_; }; } // namespace Config diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 1e0737fde3d08..4f549556558f8 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -66,7 +66,7 @@ class NewGrpcMuxImpl struct SubscriptionStuff { SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, const bool use_namespace_matching) - : sub_state_(type_url, watch_map_, local_info, use_namespace_matching) {} + : watch_map_(use_namespace_matching), sub_state_(type_url, watch_map_, local_info) {} WatchMap watch_map_; DeltaSubscriptionState sub_state_; diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index fa24d20f5f835..a70fb2c44c084 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -8,6 +8,15 @@ namespace Envoy { namespace Config { +namespace { +// Returns the namespace part (if there's any) in the resource name. +std::string namespaceFromName(const std::string& resource_name) { + const auto pos = resource_name.find_last_of('/'); + // we are not interested in the "/" character in the namespace + return pos == std::string::npos ? "" : resource_name.substr(0, pos); +} +} // namespace + Watch* WatchMap::addWatch(SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder) { auto watch = std::make_unique(callbacks, resource_decoder); @@ -58,15 +67,14 @@ AddedRemoved WatchMap::updateWatchInterest(Watch* watch, findRemovals(newly_removed_from_watch, watch)); } -absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name, - const bool use_namespace_matching) { +absl::flat_hash_set WatchMap::watchesInterestedIn(const std::string& resource_name) { absl::flat_hash_set ret; - if (!use_namespace_matching) { + if (!use_namespace_matching_) { ret = wildcard_watches_; } const auto prefix = namespaceFromName(resource_name); - const auto resource_key = use_namespace_matching && !prefix.empty() ? prefix : resource_name; + const auto resource_key = use_namespace_matching_ && !prefix.empty() ? prefix : resource_name; const auto watches_interested = watch_interest_.find(resource_key); if (watches_interested != watch_interest_.end()) { for (const auto& watch : watches_interested->second) { @@ -95,7 +103,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField decoded_resources.emplace_back( new DecodedResourceImpl((*watches_.begin())->resource_decoder_, r, version_info)); const absl::flat_hash_set& interested_in_r = - watchesInterestedIn(decoded_resources.back()->name(), false); + watchesInterestedIn(decoded_resources.back()->name()); for (const auto& interested_watch : interested_in_r) { per_watch_updates[interested_watch].emplace_back(*decoded_resources.back()); } @@ -130,7 +138,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField void WatchMap::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_namespace_matching) { + const std::string& system_version_info) { // Track any removals triggered by earlier watch updates. ASSERT(deferred_removed_during_update_ == nullptr); deferred_removed_during_update_ = std::make_unique>(); @@ -141,8 +149,7 @@ void WatchMap::onConfigUpdate( std::vector decoded_resources; absl::flat_hash_map> per_watch_added; for (const auto& r : added_resources) { - const absl::flat_hash_set& interested_in_r = - watchesInterestedIn(r.name(), use_namespace_matching); + const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r.name()); // If there are no watches, then we don't need to decode. If there are watches, they should all // be for the same resource type, so we can just use the callbacks of the first watch to decode. if (interested_in_r.empty()) { @@ -156,8 +163,7 @@ void WatchMap::onConfigUpdate( } absl::flat_hash_map> per_watch_removed; for (const auto& r : removed_resources) { - const absl::flat_hash_set& interested_in_r = - watchesInterestedIn(r, use_namespace_matching); + const absl::flat_hash_set& interested_in_r = watchesInterestedIn(r); for (const auto& interested_watch : interested_in_r) { *per_watch_removed[interested_watch].Add() = r; } diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index 0af4a16408adb..d0d9e822b28eb 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -60,7 +60,7 @@ struct Watch { // A WatchMap is assumed to be dedicated to a single type_url type of resource (EDS, CDS, etc). class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable { public: - WatchMap() = default; + WatchMap(const bool use_namespace_matching) : use_namespace_matching_(use_namespace_matching) {} // Adds 'callbacks' to the WatchMap, with every possible resource being watched. // (Use updateWatchInterest() to narrow it down to some specific names). @@ -85,7 +85,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_namespace_matching) override; + const std::string& system_version_info) override; void onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) override; WatchMap(const WatchMap&) = delete; @@ -105,15 +105,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable watchesInterestedIn(const std::string& resource_name, - const bool use_namespace_matching); - - // Returns the namespace part (if there's any) in the resource name. - std::string namespaceFromName(const std::string& resource_name) { - const auto pos = resource_name.find_last_of('/'); - // we are not interested in the "/" character in the namespace - return pos == std::string::npos ? "" : resource_name.substr(0, pos); - } + absl::flat_hash_set watchesInterestedIn(const std::string& resource_name); absl::flat_hash_set> watches_; @@ -129,6 +121,8 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable the resource can be removed. // 2) Enables efficient lookup of all interested watches when a resource has been updated. absl::flat_hash_map> watch_interest_; + + const bool use_namespace_matching_; }; } // namespace Config diff --git a/test/common/config/watch_map_test.cc b/test/common/config/watch_map_test.cc index 5f6d3d3dd1399..ff26dee1e0d39 100644 --- a/test/common/config/watch_map_test.cc +++ b/test/common/config/watch_map_test.cc @@ -97,8 +97,7 @@ wrapInResource(const Protobuf::RepeatedPtrField& anys, void doDeltaUpdate(WatchMap& watch_map, const Protobuf::RepeatedPtrField& sotw_resources, - const std::vector& removed_names, const std::string& version, - const bool use_namespace_matching) { + const std::vector& removed_names, const std::string& version) { Protobuf::RepeatedPtrField delta_resources = wrapInResource(sotw_resources, version); @@ -106,7 +105,7 @@ void doDeltaUpdate(WatchMap& watch_map, for (const auto& n : removed_names) { *removed_names_proto.Add() = n; } - watch_map.onConfigUpdate(delta_resources, removed_names_proto, version, use_namespace_matching); + watch_map.onConfigUpdate(delta_resources, removed_names_proto, version); } // Similar to expectDeltaAndSotwUpdate(), but making the onConfigUpdate() happen, rather than @@ -116,7 +115,7 @@ void doDeltaAndSotwUpdate(WatchMap& watch_map, const std::vector& removed_names, const std::string& version) { watch_map.onConfigUpdate(sotw_resources, version); - doDeltaUpdate(watch_map, sotw_resources, removed_names, version, false); + doDeltaUpdate(watch_map, sotw_resources, removed_names, version); } // Tests the simple case of a single watch. Checks that the watch will not be told of updates to @@ -126,7 +125,7 @@ TEST(WatchMapTest, Basic) { MockSubscriptionCallbacks callbacks; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(false); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); { @@ -193,7 +192,7 @@ TEST(WatchMapTest, Overlap) { MockSubscriptionCallbacks callbacks2; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(false); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); @@ -255,6 +254,8 @@ TEST(WatchMapTest, Overlap) { // WatchMap defers deletes and doesn't crash. class SameWatchRemoval : public testing::Test { public: + SameWatchRemoval() : watch_map_(false) {} + void SetUp() override { envoy::config::endpoint::v3::ClusterLoadAssignment alice; alice.set_cluster_name("alice"); @@ -304,7 +305,7 @@ TEST_F(SameWatchRemoval, SameWatchRemovalDeltaAdd) { EXPECT_CALL(callbacks2_, onConfigUpdate(_, _, _)) .Times(AtMost(1)) .WillRepeatedly(InvokeWithoutArgs([this] { removeAllInterest(); })); - watch_map_.onConfigUpdate(delta_resources, removed_names_proto, "version1", false); + watch_map_.onConfigUpdate(delta_resources, removed_names_proto, "version1"); } TEST_F(SameWatchRemoval, SameWatchRemovalDeltaRemove) { @@ -316,7 +317,7 @@ TEST_F(SameWatchRemoval, SameWatchRemovalDeltaRemove) { EXPECT_CALL(callbacks2_, onConfigUpdate(_, _, _)) .Times(AtMost(1)) .WillRepeatedly(InvokeWithoutArgs([this] { removeAllInterest(); })); - watch_map_.onConfigUpdate({}, removed_names_proto, "version1", false); + watch_map_.onConfigUpdate({}, removed_names_proto, "version1"); } // Checks the following: @@ -330,7 +331,7 @@ TEST(WatchMapTest, AddRemoveAdd) { MockSubscriptionCallbacks callbacks2; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(false); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); @@ -385,7 +386,7 @@ TEST(WatchMapTest, UninterestingUpdate) { MockSubscriptionCallbacks callbacks; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(false); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"alice"}); @@ -429,7 +430,7 @@ TEST(WatchMapTest, WatchingEverything) { MockSubscriptionCallbacks callbacks2; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(false); /*Watch* watch1 = */ watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); // watch1 never specifies any names, and so is treated as interested in everything. @@ -465,7 +466,7 @@ TEST(WatchMapTest, DeltaOnConfigUpdate) { MockSubscriptionCallbacks callbacks3; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(false); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); @@ -498,7 +499,7 @@ TEST(WatchMapTest, DeltaOnConfigUpdate) { } TEST(WatchMapTest, OnConfigUpdateFailed) { - WatchMap watch_map; + WatchMap watch_map(false); // calling on empty map doesn't break watch_map.onConfigUpdateFailed(ConfigUpdateFailureReason::UpdateRejected, nullptr); @@ -520,7 +521,7 @@ TEST(WatchMapTest, OnConfigUpdateUsingNamespaces) { MockSubscriptionCallbacks callbacks3; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); - WatchMap watch_map; + WatchMap watch_map(true); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); @@ -536,13 +537,13 @@ TEST(WatchMapTest, OnConfigUpdateUsingNamespaces) { update.Add()->PackFrom(resource); expectDeltaUpdate(callbacks1, {resource}, {}, "version0"); expectDeltaUpdate(callbacks2, {resource}, {}, "version0"); - doDeltaUpdate(watch_map, update, {}, "version0", true); + doDeltaUpdate(watch_map, update, {}, "version0"); } // verify removal { Protobuf::RepeatedPtrField update; expectDeltaUpdate(callbacks2, {}, {"ns2/removed"}, "version1"); - doDeltaUpdate(watch_map, update, {"ns2/removed"}, "version1", true); + doDeltaUpdate(watch_map, update, {"ns2/removed"}, "version1"); } // verify a not-found response to an on-demand request: such a response will contain an empty // resource wrapper with the name and aliases fields containing the alias used in the request. @@ -569,7 +570,7 @@ TEST(WatchMapTest, OnConfigUpdateUsingNamespaces) { Protobuf::RepeatedPtrField removed_names_proto; - watch_map.onConfigUpdate(empty_resources, removed_names_proto, "version2", true); + watch_map.onConfigUpdate(empty_resources, removed_names_proto, "version2"); } } diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index e279312526da0..34efe4c30bc99 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -55,7 +55,7 @@ class MockUntypedConfigUpdateCallbacks : public UntypedConfigUpdateCallbacks { void, onConfigUpdate, (const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, - const std::string& system_version_info, const bool use_prefix_matching)); + const std::string& system_version_info)); MOCK_METHOD(void, onConfigUpdateFailed, (Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e)); }; From bd99e15c5eb3afee12db26069e6914bcaf36e9ac Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 3 Sep 2020 11:16:51 -0700 Subject: [PATCH 15/16] Fixed comments Signed-off-by: Dmitri Dolguikh --- include/envoy/config/subscription.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index c283cc862070c..0506ed46dcb7f 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -148,7 +148,7 @@ class UntypedConfigUpdateCallbacks { * @throw EnvoyException with reason if the config changes are rejected. Otherwise the changes * @param use_namespace_matching if the resources should me matched on their namespaces, rather * than unique names. This is used when a collection of resources (e.g. virtual hosts in VHDS) is - * being updated. are accepted. Accepted changes have their version_info reflected in subsequent + * being updated. Accepted changes have their version_info reflected in subsequent * requests. */ virtual void onConfigUpdate( From 3ab2b217d333e00f55f54f03dc57e0d17de298ec Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 3 Sep 2020 12:25:23 -0700 Subject: [PATCH 16/16] Fixed broken test Signed-off-by: Dmitri Dolguikh --- test/common/config/delta_subscription_state_test.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/common/config/delta_subscription_state_test.cc b/test/common/config/delta_subscription_state_test.cc index 8793f5dcbdf87..554ebe3884df0 100644 --- a/test/common/config/delta_subscription_state_test.cc +++ b/test/common/config/delta_subscription_state_test.cc @@ -24,7 +24,7 @@ const char TypeUrl[] = "type.googleapis.com/envoy.api.v2.Cluster"; class DeltaSubscriptionStateTest : public testing::Test { protected: - DeltaSubscriptionStateTest() : state_(TypeUrl, callbacks_, local_info_, false) { + DeltaSubscriptionStateTest() : state_(TypeUrl, callbacks_, local_info_) { state_.updateSubscriptionInterest({"name1", "name2", "name3"}, {}); envoy::service::discovery::v3::DeltaDiscoveryRequest cur_request = state_.getNextRequestAckless(); @@ -44,7 +44,7 @@ class DeltaSubscriptionStateTest : public testing::Test { if (nonce.has_value()) { message.set_nonce(nonce.value()); } - EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _, _)).Times(expect_config_update_call ? 1 : 0); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)).Times(expect_config_update_call ? 1 : 0); return state_.handleResponse(message); } @@ -57,8 +57,7 @@ class DeltaSubscriptionStateTest : public testing::Test { *message.mutable_removed_resources() = removed_resources; message.set_system_version_info(version_info); message.set_nonce(nonce); - EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _, _)) - .WillOnce(Throw(EnvoyException(error_message))); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)).WillOnce(Throw(EnvoyException(error_message))); return state_.handleResponse(message); }