diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a8f4197cb336..c13492aaca47 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -5,6 +5,8 @@ Incompatible Behavior Changes ----------------------------- *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* +* xds: ``*`` became a reserved name for a wildcard resource that can be subscribed to and unsubscribed from at any time. This is a requirement for implementing the on-demand xDSes (like on-demand CDS) that can subscribe to specific resources next to their wildcard subscription. If such xDS is subscribed to both wildcard resource and to other specific resource, then in stream reconnection scenario, the xDS will not send an empty initial request, but a request containing ``*`` for wildcard subscription and the rest of the resources the xDS is subscribed to. If the xDS is only subscribed to wildcard resource, it will try to send a legacy wildcard request. This behavior implements the recent changes in :ref:`xDS protocol ` and can be temporarily reverted by setting the ``envoy.restart_features.explicit_wildcard_resource`` runtime guard to false. + Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 2d4d89c65b8a..b7a0a5f14d29 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -86,8 +86,16 @@ envoy_cc_library( envoy_cc_library( name = "delta_subscription_state_lib", - srcs = ["delta_subscription_state.cc"], - hdrs = ["delta_subscription_state.h"], + srcs = [ + "delta_subscription_state.cc", + "new_delta_subscription_state.cc", + "old_delta_subscription_state.cc", + ], + hdrs = [ + "delta_subscription_state.h", + "new_delta_subscription_state.h", + "old_delta_subscription_state.h", + ], deps = [ ":api_version_lib", ":pausable_ack_queue_lib", @@ -402,6 +410,7 @@ envoy_cc_library( hdrs = ["watch_map.h"], deps = [ ":decoded_resource_lib", + ":utility_lib", ":xds_resource_lib", "//envoy/config:subscription_interface", "//source/common/common:assert_lib", diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 26e9d1d69623..39429f88b4a5 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -1,243 +1,103 @@ #include "source/common/config/delta_subscription_state.h" -#include "envoy/event/dispatcher.h" -#include "envoy/service/discovery/v3/discovery.pb.h" - -#include "source/common/common/assert.h" -#include "source/common/common/hash.h" -#include "source/common/config/utility.h" #include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Config { +namespace { + +DeltaSubscriptionStateVariant getState(std::string type_url, + UntypedConfigUpdateCallbacks& watch_map, + const LocalInfo::LocalInfo& local_info, + Event::Dispatcher& dispatcher) { + if (Runtime::runtimeFeatureEnabled("envoy.restart_features.explicit_wildcard_resource")) { + return DeltaSubscriptionStateVariant(absl::in_place_type, + std::move(type_url), watch_map, local_info, dispatcher); + } else { + return DeltaSubscriptionStateVariant(absl::in_place_type, + std::move(type_url), watch_map, local_info, dispatcher); + } +} + +} // namespace DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher, const bool wildcard) - // TODO(snowp): Hard coding VHDS here is temporary until we can move it away from relying on - // empty resources as updates. - : supports_heartbeats_(type_url != "envoy.config.route.v3.VirtualHost"), - ttl_( - [this](const auto& expired) { - Protobuf::RepeatedPtrField removed_resources; - for (const auto& resource : expired) { - setResourceWaitingForServer(resource); - removed_resources.Add(std::string(resource)); - } - - watch_map_.onConfigUpdate({}, removed_resources, ""); - }, - dispatcher, dispatcher.timeSource()), - type_url_(std::move(type_url)), wildcard_(wildcard), watch_map_(watch_map), - local_info_(local_info), dispatcher_(dispatcher) {} + Event::Dispatcher& dispatcher) + : state_(getState(std::move(type_url), watch_map, local_info, dispatcher)) {} void DeltaSubscriptionState::updateSubscriptionInterest( const absl::flat_hash_set& cur_added, const absl::flat_hash_set& cur_removed) { - for (const auto& a : cur_added) { - setResourceWaitingForServer(a); - // If interest in a resource is removed-then-added (all before a discovery request - // can be sent), we must treat it as a "new" addition: our user may have forgotten its - // copy of the resource after instructing us to remove it, and need to be reminded of it. - names_removed_.erase(a); - names_added_.insert(a); - } - for (const auto& r : cur_removed) { - removeResourceState(r); - // Ideally, when interest in a resource is added-then-removed in between requests, - // we would avoid putting a superfluous "unsubscribe [resource that was never subscribed]" - // in the request. However, the removed-then-added case *does* need to go in the request, - // and due to how we accomplish that, it's difficult to distinguish remove-add-remove from - // add-remove (because "remove-add" has to be treated as equivalent to just "add"). - names_added_.erase(r); - names_removed_.insert(r); + if (auto* state = absl::get_if(&state_); state != nullptr) { + state->updateSubscriptionInterest(cur_added, cur_removed); + return; } + auto& state = absl::get(state_); + state.updateSubscriptionInterest(cur_added, cur_removed); } -// Not having sent any requests yet counts as an "update pending" since you're supposed to resend -// the entirety of your interest at the start of a stream, even if nothing has changed. -bool DeltaSubscriptionState::subscriptionUpdatePending() const { - return !names_added_.empty() || !names_removed_.empty() || - !any_request_sent_yet_in_current_stream_ || must_send_discovery_request_; +void DeltaSubscriptionState::setMustSendDiscoveryRequest() { + if (auto* state = absl::get_if(&state_); state != nullptr) { + state->setMustSendDiscoveryRequest(); + return; + } + auto& state = absl::get(state_); + state.setMustSendDiscoveryRequest(); } -UpdateAck DeltaSubscriptionState::handleResponse( - const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) { - // We *always* copy the response's nonce into the next request, even if we're going to make that - // request a NACK by setting error_detail. - UpdateAck ack(message.nonce(), type_url_); - TRY_ASSERT_MAIN_THREAD { handleGoodResponse(message); } - END_TRY - catch (const EnvoyException& e) { - handleBadResponse(e, ack); +bool DeltaSubscriptionState::subscriptionUpdatePending() const { + if (auto* state = absl::get_if(&state_); state != nullptr) { + return state->subscriptionUpdatePending(); } - return ack; + auto& state = absl::get(state_); + return state.subscriptionUpdatePending(); } -bool DeltaSubscriptionState::isHeartbeatResponse( - const envoy::service::discovery::v3::Resource& resource) const { - if (!supports_heartbeats_ && - !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.vhds_heartbeats")) { - return false; - } - const auto itr = resource_state_.find(resource.name()); - if (itr == resource_state_.end()) { - return false; +void DeltaSubscriptionState::markStreamFresh() { + if (auto* state = absl::get_if(&state_); state != nullptr) { + state->markStreamFresh(); + return; } - - return !resource.has_resource() && !itr->second.waitingForServer() && - resource.version() == itr->second.version(); + auto& state = absl::get(state_); + state.markStreamFresh(); } -void DeltaSubscriptionState::handleGoodResponse( +UpdateAck DeltaSubscriptionState::handleResponse( const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) { - absl::flat_hash_set names_added_removed; - Protobuf::RepeatedPtrField non_heartbeat_resources; - for (const auto& resource : message.resources()) { - if (!names_added_removed.insert(resource.name()).second) { - throw EnvoyException( - fmt::format("duplicate name {} found among added/updated resources", resource.name())); - } - if (isHeartbeatResponse(resource)) { - continue; - } - non_heartbeat_resources.Add()->CopyFrom(resource); - // DeltaDiscoveryResponses for unresolved aliases don't contain an actual resource - if (!resource.has_resource() && resource.aliases_size() > 0) { - continue; - } - if (message.type_url() != resource.resource().type_url()) { - throw EnvoyException(fmt::format("type URL {} embedded in an individual Any does not match " - "the message-wide type URL {} in DeltaDiscoveryResponse {}", - resource.resource().type_url(), message.type_url(), - message.DebugString())); - } - } - for (const auto& name : message.removed_resources()) { - if (!names_added_removed.insert(name).second) { - throw EnvoyException( - fmt::format("duplicate name {} found in the union of added+removed resources", name)); - } - } - - { - const auto scoped_update = ttl_.scopedTtlUpdate(); - for (const auto& resource : message.resources()) { - if (wildcard_ || resource_state_.contains(resource.name())) { - // Only consider tracked resources. - // NOTE: This is not gonna work for xdstp resources with glob resource matching. - addResourceState(resource); - } - } + if (auto* state = absl::get_if(&state_); state != nullptr) { + return state->handleResponse(message); } - - watch_map_.onConfigUpdate(non_heartbeat_resources, message.removed_resources(), - message.system_version_info()); - - // If a resource is gone, there is no longer a meaningful version for it that makes sense to - // provide to the server upon stream reconnect: either it will continue to not exist, in which - // case saying nothing is fine, or the server will bring back something new, which we should - // receive regardless (which is the logic that not specifying a version will get you). - // - // So, leave the version map entry present but blank. It will be left out of - // initial_resource_versions messages, but will remind us to explicitly tell the server "I'm - // cancelling my subscription" when we lose interest. - for (const auto& resource_name : message.removed_resources()) { - if (resource_names_.find(resource_name) != resource_names_.end()) { - setResourceWaitingForServer(resource_name); - } - } - ENVOY_LOG(debug, "Delta config for {} accepted with {} resources added, {} removed", type_url_, - message.resources().size(), message.removed_resources().size()); -} - -void DeltaSubscriptionState::handleBadResponse(const EnvoyException& e, UpdateAck& ack) { - // Note that error_detail being set is what indicates that a DeltaDiscoveryRequest is a NACK. - ack.error_detail_.set_code(Grpc::Status::WellKnownGrpcStatus::Internal); - ack.error_detail_.set_message(Config::Utility::truncateGrpcStatusMessage(e.what())); - ENVOY_LOG(warn, "delta config for {} rejected: {}", type_url_, e.what()); - watch_map_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); + auto& state = absl::get(state_); + return state.handleResponse(message); } void DeltaSubscriptionState::handleEstablishmentFailure() { - watch_map_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, - nullptr); + if (auto* state = absl::get_if(&state_); state != nullptr) { + state->handleEstablishmentFailure(); + return; + } + auto& state = absl::get(state_); + state.handleEstablishmentFailure(); } envoy::service::discovery::v3::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequestAckless() { - envoy::service::discovery::v3::DeltaDiscoveryRequest request; - must_send_discovery_request_ = false; - if (!any_request_sent_yet_in_current_stream_) { - any_request_sent_yet_in_current_stream_ = true; - // initial_resource_versions "must be populated for first request in a stream". - // Also, since this might be a new server, we must explicitly state *all* of our subscription - // interest. - for (auto const& [resource_name, resource_state] : resource_state_) { - // Populate initial_resource_versions with the resource versions we currently have. - // Resources we are interested in, but are still waiting to get any version of from the - // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) - if (!resource_state.waitingForServer()) { - (*request.mutable_initial_resource_versions())[resource_name] = resource_state.version(); - } - // As mentioned above, fill resource_names_subscribe with everything, including names we - // have yet to receive any resource for unless this is a wildcard subscription, for which - // the first request on a stream must be without any resource names. - if (!wildcard_) { - names_added_.insert(resource_name); - } - } - // Wildcard subscription initial requests must have no resource_names_subscribe. - if (wildcard_) { - names_added_.clear(); - } - names_removed_.clear(); + if (auto* state = absl::get_if(&state_); state != nullptr) { + return state->getNextRequestAckless(); } - std::copy(names_added_.begin(), names_added_.end(), - Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_subscribe())); - std::copy(names_removed_.begin(), names_removed_.end(), - Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_unsubscribe())); - names_added_.clear(); - names_removed_.clear(); - - request.set_type_url(type_url_); - request.mutable_node()->MergeFrom(local_info_.node()); - return request; + auto& state = absl::get(state_); + return state.getNextRequestAckless(); } envoy::service::discovery::v3::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequestWithAck(const UpdateAck& ack) { - envoy::service::discovery::v3::DeltaDiscoveryRequest request = getNextRequestAckless(); - request.set_response_nonce(ack.nonce_); - if (ack.error_detail_.code() != Grpc::Status::WellKnownGrpcStatus::Ok) { - // Don't needlessly make the field present-but-empty if status is ok. - request.mutable_error_detail()->CopyFrom(ack.error_detail_); + if (auto* state = absl::get_if(&state_); state != nullptr) { + return state->getNextRequestWithAck(ack); } - return request; -} - -void DeltaSubscriptionState::addResourceState( - const envoy::service::discovery::v3::Resource& resource) { - if (resource.has_ttl()) { - ttl_.add(std::chrono::milliseconds(DurationUtil::durationToMilliseconds(resource.ttl())), - resource.name()); - } else { - ttl_.clear(resource.name()); - } - - resource_state_[resource.name()] = ResourceState(resource); - resource_names_.insert(resource.name()); -} - -void DeltaSubscriptionState::setResourceWaitingForServer(const std::string& resource_name) { - resource_state_[resource_name] = ResourceState(); - resource_names_.insert(resource_name); -} - -void DeltaSubscriptionState::removeResourceState(const std::string& resource_name) { - resource_state_.erase(resource_name); - resource_names_.erase(resource_name); + auto& state = absl::get(state_); + return state.getNextRequestWithAck(ack); } } // namespace Config diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 9765a6736dc3..6b613ade0b4f 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -1,123 +1,42 @@ #pragma once #include "envoy/config/subscription.h" -#include "envoy/event/dispatcher.h" -#include "envoy/grpc/status.h" #include "envoy/local_info/local_info.h" #include "envoy/service/discovery/v3/discovery.pb.h" -#include "source/common/common/assert.h" #include "source/common/common/logger.h" -#include "source/common/config/api_version.h" -#include "source/common/config/pausable_ack_queue.h" -#include "source/common/config/ttl.h" -#include "source/common/config/watch_map.h" +#include "source/common/config/new_delta_subscription_state.h" +#include "source/common/config/old_delta_subscription_state.h" -#include "absl/container/node_hash_map.h" +#include "absl/container/flat_hash_set.h" +#include "absl/types/variant.h" namespace Envoy { namespace Config { -// Tracks the xDS protocol state of an individual ongoing delta xDS session, i.e. a single type_url. -// There can be multiple DeltaSubscriptionStates active. They will always all be -// blissfully unaware of each other's existence, even when their messages are -// being multiplexed together by ADS. +using DeltaSubscriptionStateVariant = + absl::variant; + class DeltaSubscriptionState : public Logger::Loggable { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, - const bool wildcard); + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher); - // Update which resources we're interested in subscribing to. void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, const absl::flat_hash_set& cur_removed); - void addAliasesToResolve(const absl::flat_hash_set& aliases); - void setMustSendDiscoveryRequest() { must_send_discovery_request_ = true; } - - // Whether there was a change in our subscription interest we have yet to inform the server of. + void setMustSendDiscoveryRequest(); bool subscriptionUpdatePending() const; - - void markStreamFresh() { any_request_sent_yet_in_current_stream_ = false; } - + void markStreamFresh(); UpdateAck handleResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message); - void handleEstablishmentFailure(); - - // Returns the next gRPC request proto to be sent off to the server, based on this object's - // understanding of the current protocol state, and new resources that Envoy wants to request. envoy::service::discovery::v3::DeltaDiscoveryRequest getNextRequestAckless(); - - // The WithAck version first calls the Ack-less version, then adds in the passed-in ack. envoy::service::discovery::v3::DeltaDiscoveryRequest getNextRequestWithAck(const UpdateAck& ack); DeltaSubscriptionState(const DeltaSubscriptionState&) = delete; DeltaSubscriptionState& operator=(const DeltaSubscriptionState&) = delete; private: - bool isHeartbeatResponse(const envoy::service::discovery::v3::Resource& resource) const; - void handleGoodResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message); - void handleBadResponse(const EnvoyException& e, UpdateAck& ack); - - class ResourceState { - public: - ResourceState(const envoy::service::discovery::v3::Resource& resource) - : version_(resource.version()) {} - - // Builds a ResourceState in the waitingForServer state. - ResourceState() = default; - - // If true, we currently have no version of this resource - we are waiting for the server to - // provide us with one. - bool waitingForServer() const { return version_ == absl::nullopt; } - - // Must not be called if waitingForServer() == true. - std::string version() const { - ASSERT(version_.has_value()); - return version_.value_or(""); - } - - private: - absl::optional version_; - }; - - // Use these helpers to ensure resource_state_ and resource_names_ get updated together. - void addResourceState(const envoy::service::discovery::v3::Resource& resource); - void setResourceWaitingForServer(const std::string& resource_name); - void removeResourceState(const std::string& resource_name); - - void populateDiscoveryRequest(envoy::service::discovery::v3::DeltaDiscoveryResponse& request); - - // A map from resource name to per-resource version. The keys of this map are exactly the resource - // names we are currently interested in. Those in the waitingForServer state currently don't have - // any version for that resource: we need to inform the server if we lose interest in them, but we - // also need to *not* include them in the initial_resource_versions map upon a reconnect. - absl::node_hash_map resource_state_; - - // Not all xDS resources supports heartbeats due to there being specific information encoded in - // an empty response, which is indistinguishable from a heartbeat in some cases. For now we just - // disable heartbeats for these resources (currently only VHDS). - const bool supports_heartbeats_; - TtlManager ttl_; - // The keys of resource_versions_. Only tracked separately because std::map does not provide an - // iterator into just its keys. - absl::flat_hash_set resource_names_; - - const std::string type_url_; - // Is the subscription is for a wildcard request. - const bool wildcard_; - UntypedConfigUpdateCallbacks& watch_map_; - const LocalInfo::LocalInfo& local_info_; - Event::Dispatcher& dispatcher_; - std::chrono::milliseconds init_fetch_timeout_; - - bool any_request_sent_yet_in_current_stream_{}; - bool must_send_discovery_request_{}; - - // Tracks changes in our subscription interest since the previous DeltaDiscoveryRequest we sent. - // TODO: Can't use absl::flat_hash_set due to ordering issues in gTest expectation matching. - // 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_; + DeltaSubscriptionStateVariant state_; }; } // namespace Config diff --git a/source/common/config/new_delta_subscription_state.cc b/source/common/config/new_delta_subscription_state.cc new file mode 100644 index 000000000000..94f25ac952eb --- /dev/null +++ b/source/common/config/new_delta_subscription_state.cc @@ -0,0 +1,408 @@ +#include "source/common/config/new_delta_subscription_state.h" + +#include "envoy/event/dispatcher.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/common/assert.h" +#include "source/common/common/hash.h" +#include "source/common/config/utility.h" +#include "source/common/runtime/runtime_features.h" + +namespace Envoy { +namespace Config { + +NewDeltaSubscriptionState::NewDeltaSubscriptionState(std::string type_url, + UntypedConfigUpdateCallbacks& watch_map, + const LocalInfo::LocalInfo& local_info, + Event::Dispatcher& dispatcher) + // TODO(snowp): Hard coding VHDS here is temporary until we can move it away from relying on + // empty resources as updates. + : supports_heartbeats_(type_url != "envoy.config.route.v3.VirtualHost"), + ttl_( + [this](const auto& expired) { + Protobuf::RepeatedPtrField removed_resources; + for (const auto& resource : expired) { + if (auto maybe_resource = getRequestedResourceState(resource); + maybe_resource.has_value()) { + maybe_resource->setAsWaitingForServer(); + removed_resources.Add(std::string(resource)); + } else if (const auto erased_count = wildcard_resource_state_.erase(resource) + + ambiguous_resource_state_.erase(resource); + erased_count > 0) { + removed_resources.Add(std::string(resource)); + } + } + + watch_map_.onConfigUpdate({}, removed_resources, ""); + }, + dispatcher, dispatcher.timeSource()), + type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info) {} + +void NewDeltaSubscriptionState::updateSubscriptionInterest( + const absl::flat_hash_set& cur_added, + const absl::flat_hash_set& cur_removed) { + for (const auto& a : cur_added) { + if (in_initial_legacy_wildcard_ && a != Wildcard) { + in_initial_legacy_wildcard_ = false; + } + // If the requested resource existed as a wildcard resource, + // transition it to requested. Otherwise mark it as a resource + // waiting for the server to receive the version. + if (auto it = wildcard_resource_state_.find(a); it != wildcard_resource_state_.end()) { + requested_resource_state_.insert_or_assign(a, ResourceState::withVersion(it->second)); + wildcard_resource_state_.erase(it); + } else if (it = ambiguous_resource_state_.find(a); it != ambiguous_resource_state_.end()) { + requested_resource_state_.insert_or_assign(a, ResourceState::withVersion(it->second)); + ambiguous_resource_state_.erase(it); + } else { + requested_resource_state_.insert_or_assign(a, ResourceState::waitingForServer()); + } + ASSERT(requested_resource_state_.contains(a)); + ASSERT(!wildcard_resource_state_.contains(a)); + ASSERT(!ambiguous_resource_state_.contains(a)); + // If interest in a resource is removed-then-added (all before a discovery request + // can be sent), we must treat it as a "new" addition: our user may have forgotten its + // copy of the resource after instructing us to remove it, and need to be reminded of it. + names_removed_.erase(a); + names_added_.insert(a); + } + for (const auto& r : cur_removed) { + auto actually_erased = false; + // The resource we have lost the interest in could also come from our wildcard subscription. We + // just don't know it at this point. Instead of removing it outright, mark the resource as not + // interesting to us any more and the server will send us an update. If we don't have a wildcard + // subscription then there is no ambiguity and just drop the resource. + if (requested_resource_state_.contains(Wildcard)) { + if (auto it = requested_resource_state_.find(r); it != requested_resource_state_.end()) { + // Wildcard resources always have a version. If our requested resource has no version, it + // won't be a wildcard resource then. If r is Wildcard itself, then it never has a version + // attached to it, so it will not be moved to ambiguous category. + if (!it->second.isWaitingForServer()) { + ambiguous_resource_state_.insert({it->first, it->second.version()}); + } + requested_resource_state_.erase(it); + actually_erased = true; + } + } else { + actually_erased = (requested_resource_state_.erase(r) > 0); + } + ASSERT(!requested_resource_state_.contains(r)); + // Ideally, when interest in a resource is added-then-removed in between requests, + // we would avoid putting a superfluous "unsubscribe [resource that was never subscribed]" + // in the request. However, the removed-then-added case *does* need to go in the request, + // and due to how we accomplish that, it's difficult to distinguish remove-add-remove from + // add-remove (because "remove-add" has to be treated as equivalent to just "add"). + names_added_.erase(r); + if (actually_erased) { + names_removed_.insert(r); + in_initial_legacy_wildcard_ = false; + } + } + // If we unsubscribe from wildcard resource, drop all the resources that came from wildcard from + // cache. Also drop the ambiguous resources - we aren't interested in those, but we didn't know if + // those came from wildcard subscription or not, but now it's not important any more. + if (cur_removed.contains(Wildcard)) { + wildcard_resource_state_.clear(); + ambiguous_resource_state_.clear(); + } +} + +// Not having sent any requests yet counts as an "update pending" since you're supposed to resend +// the entirety of your interest at the start of a stream, even if nothing has changed. +bool NewDeltaSubscriptionState::subscriptionUpdatePending() const { + if (!names_added_.empty() || !names_removed_.empty()) { + return true; + } + // At this point, we have no new resources to subscribe to or any + // resources to unsubscribe from. + if (!any_request_sent_yet_in_current_stream_) { + // If we haven't sent anything on the current stream, but we are actually interested in some + // resource then we obviously need to let the server know about those. + if (!requested_resource_state_.empty()) { + return true; + } + // So there are no new names and we are interested in nothing. This may either mean that we want + // the legacy wildcard subscription to kick in or we actually unsubscribed from everything. If + // the latter is true, then we should not be sending any requests. In such case the initial + // wildcard mode will be false. Otherwise it means that the legacy wildcard request should be + // sent. + return in_initial_legacy_wildcard_; + } + + // At this point, we have no changes in subscription resources and this isn't a first request in + // the stream, so even if there are no resources we are interested in, we can send the request, + // because even if it's empty, it won't be interpreted as legacy wildcard subscription, which can + // only for the first request in the stream. So sending an empty request at this point should be + // harmless. + return must_send_discovery_request_; +} + +UpdateAck NewDeltaSubscriptionState::handleResponse( + const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) { + // We *always* copy the response's nonce into the next request, even if we're going to make that + // request a NACK by setting error_detail. + UpdateAck ack(message.nonce(), type_url_); + TRY_ASSERT_MAIN_THREAD { handleGoodResponse(message); } + END_TRY + catch (const EnvoyException& e) { + handleBadResponse(e, ack); + } + return ack; +} + +bool NewDeltaSubscriptionState::isHeartbeatResponse( + const envoy::service::discovery::v3::Resource& resource) const { + if (!supports_heartbeats_ && + !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.vhds_heartbeats")) { + return false; + } + if (resource.has_resource()) { + return false; + } + + if (const auto maybe_resource = getRequestedResourceState(resource.name()); + maybe_resource.has_value()) { + return !maybe_resource->isWaitingForServer() && resource.version() == maybe_resource->version(); + } + + if (const auto itr = wildcard_resource_state_.find(resource.name()); + itr != wildcard_resource_state_.end()) { + return resource.version() == itr->second; + } + + if (const auto itr = ambiguous_resource_state_.find(resource.name()); + itr != wildcard_resource_state_.end()) { + // In theory we should move the ambiguous resource to wildcard, because probably we shouldn't be + // getting heartbeat responses about resources that we are not interested in, but the server + // could have sent this heartbeat before it learned about our lack of interest in the resource. + return resource.version() == itr->second; + } + + return false; +} + +void NewDeltaSubscriptionState::handleGoodResponse( + const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) { + absl::flat_hash_set names_added_removed; + Protobuf::RepeatedPtrField non_heartbeat_resources; + for (const auto& resource : message.resources()) { + if (!names_added_removed.insert(resource.name()).second) { + throw EnvoyException( + fmt::format("duplicate name {} found among added/updated resources", resource.name())); + } + if (isHeartbeatResponse(resource)) { + continue; + } + non_heartbeat_resources.Add()->CopyFrom(resource); + // DeltaDiscoveryResponses for unresolved aliases don't contain an actual resource + if (!resource.has_resource() && resource.aliases_size() > 0) { + continue; + } + if (message.type_url() != resource.resource().type_url()) { + throw EnvoyException(fmt::format("type URL {} embedded in an individual Any does not match " + "the message-wide type URL {} in DeltaDiscoveryResponse {}", + resource.resource().type_url(), message.type_url(), + message.DebugString())); + } + } + for (const auto& name : message.removed_resources()) { + if (!names_added_removed.insert(name).second) { + throw EnvoyException( + fmt::format("duplicate name {} found in the union of added+removed resources", name)); + } + } + + { + const auto scoped_update = ttl_.scopedTtlUpdate(); + if (requested_resource_state_.contains(Wildcard)) { + for (const auto& resource : message.resources()) { + addResourceStateFromServer(resource); + } + } else { + // We are not subscribed to wildcard, so we only take resources that we explicitly requested + // and ignore the others. + for (const auto& resource : message.resources()) { + if (requested_resource_state_.contains(resource.name())) { + addResourceStateFromServer(resource); + } + } + } + } + + watch_map_.onConfigUpdate(non_heartbeat_resources, message.removed_resources(), + message.system_version_info()); + + // If a resource is gone, there is no longer a meaningful version for it that makes sense to + // provide to the server upon stream reconnect: either it will continue to not exist, in which + // case saying nothing is fine, or the server will bring back something new, which we should + // receive regardless (which is the logic that not specifying a version will get you). + // + // So, leave the version map entry present but blank if we are still interested in the resource. + // It will be left out of initial_resource_versions messages, but will remind us to explicitly + // tell the server "I'm cancelling my subscription" when we lose interest. In case of resources + // received as a part of the wildcard subscription or resources we already lost interest in, we + // just drop them. + for (const auto& resource_name : message.removed_resources()) { + if (auto maybe_resource = getRequestedResourceState(resource_name); + maybe_resource.has_value()) { + maybe_resource->setAsWaitingForServer(); + } else if (const auto erased_count = ambiguous_resource_state_.erase(resource_name); + erased_count == 0) { + wildcard_resource_state_.erase(resource_name); + } + } + ENVOY_LOG(debug, "Delta config for {} accepted with {} resources added, {} removed", type_url_, + message.resources().size(), message.removed_resources().size()); +} + +void NewDeltaSubscriptionState::handleBadResponse(const EnvoyException& e, UpdateAck& ack) { + // Note that error_detail being set is what indicates that a DeltaDiscoveryRequest is a NACK. + ack.error_detail_.set_code(Grpc::Status::WellKnownGrpcStatus::Internal); + ack.error_detail_.set_message(Config::Utility::truncateGrpcStatusMessage(e.what())); + ENVOY_LOG(warn, "delta config for {} rejected: {}", type_url_, e.what()); + watch_map_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); +} + +void NewDeltaSubscriptionState::handleEstablishmentFailure() { + watch_map_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + nullptr); +} + +envoy::service::discovery::v3::DeltaDiscoveryRequest +NewDeltaSubscriptionState::getNextRequestAckless() { + envoy::service::discovery::v3::DeltaDiscoveryRequest request; + must_send_discovery_request_ = false; + if (!any_request_sent_yet_in_current_stream_) { + any_request_sent_yet_in_current_stream_ = true; + const bool is_legacy_wildcard = isInitialRequestForLegacyWildcard(); + // initial_resource_versions "must be populated for first request in a stream". + // Also, since this might be a new server, we must explicitly state *all* of our subscription + // interest. + for (auto const& [resource_name, resource_state] : requested_resource_state_) { + // Populate initial_resource_versions with the resource versions we currently have. + // Resources we are interested in, but are still waiting to get any version of from the + // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) + if (!resource_state.isWaitingForServer()) { + (*request.mutable_initial_resource_versions())[resource_name] = resource_state.version(); + } + // We are going over a list of resources that we are interested in, so add them to + // resource_names_subscribe. + names_added_.insert(resource_name); + } + for (auto const& [resource_name, resource_version] : wildcard_resource_state_) { + (*request.mutable_initial_resource_versions())[resource_name] = resource_version; + } + for (auto const& [resource_name, resource_version] : ambiguous_resource_state_) { + (*request.mutable_initial_resource_versions())[resource_name] = resource_version; + } + // If this is a legacy wildcard request, then make sure that the resource_names_subscribe is + // empty. + if (is_legacy_wildcard) { + names_added_.clear(); + } + names_removed_.clear(); + } + std::copy(names_added_.begin(), names_added_.end(), + Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_subscribe())); + std::copy(names_removed_.begin(), names_removed_.end(), + Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_unsubscribe())); + names_added_.clear(); + names_removed_.clear(); + + request.set_type_url(type_url_); + request.mutable_node()->MergeFrom(local_info_.node()); + return request; +} + +bool NewDeltaSubscriptionState::isInitialRequestForLegacyWildcard() { + if (in_initial_legacy_wildcard_) { + requested_resource_state_.insert_or_assign(Wildcard, ResourceState::waitingForServer()); + ASSERT(requested_resource_state_.contains(Wildcard)); + ASSERT(!wildcard_resource_state_.contains(Wildcard)); + ASSERT(!ambiguous_resource_state_.contains(Wildcard)); + return true; + } + + // If we are here, this means that we lost our initial wildcard mode, because we subscribed to + // something in the past. We could still be in the situation now that all we are subscribed to now + // is wildcard resource, so in such case try to send a legacy wildcard subscription request + // anyway. For this to happen, two conditions need to apply: + // + // 1. No change in interest. + // 2. The only requested resource is Wildcard resource. + // + // The invariant of the code here is that this code is executed only when + // subscriptionUpdatePending actually returns true, which in our case can only happen if the + // requested resources state_ isn't empty. + ASSERT(!requested_resource_state_.empty()); + + // If our subscription interest didn't change then the first condition for using legacy wildcard + // subscription is met. + if (!names_added_.empty() || !names_removed_.empty()) { + return false; + } + // If we requested only a wildcard resource then the second condition for using legacy wildcard + // condition is met. + return requested_resource_state_.size() == 1 && + requested_resource_state_.begin()->first == Wildcard; +} + +envoy::service::discovery::v3::DeltaDiscoveryRequest +NewDeltaSubscriptionState::getNextRequestWithAck(const UpdateAck& ack) { + envoy::service::discovery::v3::DeltaDiscoveryRequest request = getNextRequestAckless(); + request.set_response_nonce(ack.nonce_); + if (ack.error_detail_.code() != Grpc::Status::WellKnownGrpcStatus::Ok) { + // Don't needlessly make the field present-but-empty if status is ok. + request.mutable_error_detail()->CopyFrom(ack.error_detail_); + } + return request; +} + +void NewDeltaSubscriptionState::addResourceStateFromServer( + const envoy::service::discovery::v3::Resource& resource) { + if (resource.has_ttl()) { + ttl_.add(std::chrono::milliseconds(DurationUtil::durationToMilliseconds(resource.ttl())), + resource.name()); + } else { + ttl_.clear(resource.name()); + } + + if (auto maybe_resource = getRequestedResourceState(resource.name()); + maybe_resource.has_value()) { + // It is a resource that we requested. + maybe_resource->setVersion(resource.version()); + ASSERT(requested_resource_state_.contains(resource.name())); + ASSERT(!wildcard_resource_state_.contains(resource.name())); + ASSERT(!ambiguous_resource_state_.contains(resource.name())); + } else { + // It is a resource that is a part of our wildcard request. + wildcard_resource_state_.insert({resource.name(), resource.version()}); + // The resource could be ambiguous before, but now the ambiguity + // is resolved. + ambiguous_resource_state_.erase(resource.name()); + ASSERT(!requested_resource_state_.contains(resource.name())); + ASSERT(wildcard_resource_state_.contains(resource.name())); + ASSERT(!ambiguous_resource_state_.contains(resource.name())); + } +} + +OptRef +NewDeltaSubscriptionState::getRequestedResourceState(absl::string_view resource_name) { + auto itr = requested_resource_state_.find(resource_name); + if (itr == requested_resource_state_.end()) { + return {}; + } + return {itr->second}; +} + +OptRef +NewDeltaSubscriptionState::getRequestedResourceState(absl::string_view resource_name) const { + auto itr = requested_resource_state_.find(resource_name); + if (itr == requested_resource_state_.end()) { + return {}; + } + return {itr->second}; +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/new_delta_subscription_state.h b/source/common/config/new_delta_subscription_state.h new file mode 100644 index 000000000000..9ef841cffb22 --- /dev/null +++ b/source/common/config/new_delta_subscription_state.h @@ -0,0 +1,179 @@ +#pragma once + +#include "envoy/config/subscription.h" +#include "envoy/event/dispatcher.h" +#include "envoy/grpc/status.h" +#include "envoy/local_info/local_info.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/common/assert.h" +#include "source/common/common/logger.h" +#include "source/common/config/api_version.h" +#include "source/common/config/pausable_ack_queue.h" +#include "source/common/config/ttl.h" +#include "source/common/config/watch_map.h" + +#include "absl/container/node_hash_map.h" + +namespace Envoy { +namespace Config { + +// Tracks the xDS protocol state of an individual ongoing delta xDS session, i.e. a single type_url. +// There can be multiple NewDeltaSubscriptionStates active. They will always all be blissfully +// unaware of each other's existence, even when their messages are being multiplexed together by +// ADS. +// +// There are two scenarios which affect how NewDeltaSubscriptionState manages the resources. First +// scenario is when we are subscribed to a wildcard resource, and other scenario is when we are not. +// +// Delta subscription state also divides the resources it cached into three categories: requested, +// wildcard and ambiguous. +// +// The "requested" category is for resources that we have explicitly asked for (either through the +// initial set of resources or through the on-demand mechanism). Resources in this category are in +// one of two states: "complete" and "waiting for server". +// +// "Complete" resources are resources about which the server sent us the information we need (for +// now - just resource version). +// +// The "waiting for server" state is either for resources that we have just requested, but we still +// didn't receive any version information from the server, or for the "complete" resources that, +// according to the server, are gone, but we are still interested in them - in such case we strip +// the information from the resource. +// +// The "wildcard" category is for resources that we are not explicitly interested in, but we are +// indirectly interested through the subscription to the wildcard resource. +// +// The "ambiguous" category is for resources that we stopped being interested in, but we may still +// be interested indirectly through the wildcard subscription. This situation happens because of the +// xDS protocol limitation - the server isn't able to tell us that the resource we subscribed to is +// also a part of our wildcard subscription. So when we unsubscribe from the resource, we need to +// receive a confirmation from the server whether to keep the resource (which means that it was a +// part of our wildcard subscription) or to drop it. +// +// Please refer to drawings (non-wildcard-resource-state-machine.png and +// (wildcard-resource-state-machine.png) for visual depictions of the resource state machine. +// +// In the "no wildcard subscription" scenario all the cached resources should be in the "requested" +// category. Resources are added to the category upon the explicit request and dropped when we +// explicitly unsubscribe from it. Transitions between "complete" and "waiting for server" happen +// when we receive messages from the server - if a resource in the message is in "added resources" +// list (thus contains version information), the resource becomes "complete". If the resource in the +// message is in "removed resources" list, it changes into the "waiting for server" state. If a +// server sends us a resource that we didn't request, it's going to be ignored. +// +// In the "wildcard subscription" scenario, "requested" category is the same as in "no wildcard +// subscription" scenario, with one exception - the unsubscribed "complete" resource is not removed +// from the cache, but it's moved to the "ambiguous" resources instead. At this point we are waiting +// for the server to tell us that this resource should be either moved to the "wildcard" resources, +// or dropped. Resources in "wildcard" category are only added there or dropped from there by the +// server. Resources from both "wildcard" and "ambiguous" categories can become "requested" +// "complete" resources if we subscribe to them again. +// +// The delta subscription state transitions between the two scenarios depending on whether we are +// subscribed to wildcard resource or not. Nothing special happens when we transition from "no +// wildcard subscription" to "wildcard subscription" scenario, but when transitioning in the other +// direction, we drop all the resources in "wildcard" and "ambiguous" categories. +class NewDeltaSubscriptionState : public Logger::Loggable { +public: + NewDeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher); + + // Update which resources we're interested in subscribing to. + void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, + const absl::flat_hash_set& cur_removed); + void setMustSendDiscoveryRequest() { must_send_discovery_request_ = true; } + + // Whether there was a change in our subscription interest we have yet to inform the server of. + bool subscriptionUpdatePending() const; + + void markStreamFresh() { any_request_sent_yet_in_current_stream_ = false; } + + UpdateAck handleResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message); + + void handleEstablishmentFailure(); + + // Returns the next gRPC request proto to be sent off to the server, based on this object's + // understanding of the current protocol state, and new resources that Envoy wants to request. + envoy::service::discovery::v3::DeltaDiscoveryRequest getNextRequestAckless(); + + // The WithAck version first calls the Ack-less version, then adds in the passed-in ack. + envoy::service::discovery::v3::DeltaDiscoveryRequest getNextRequestWithAck(const UpdateAck& ack); + + NewDeltaSubscriptionState(const NewDeltaSubscriptionState&) = delete; + NewDeltaSubscriptionState& operator=(const NewDeltaSubscriptionState&) = delete; + +private: + bool isHeartbeatResponse(const envoy::service::discovery::v3::Resource& resource) const; + void handleGoodResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message); + void handleBadResponse(const EnvoyException& e, UpdateAck& ack); + + class ResourceState { + public: + // Builds a ResourceState in the waitingForServer state. + ResourceState() = default; + // Builds a ResourceState with a specific version + ResourceState(absl::string_view version) : version_(version) {} + // Self-documenting alias of default constructor. + static ResourceState waitingForServer() { return ResourceState(); } + // Self-documenting alias of constructor with version. + static ResourceState withVersion(absl::string_view version) { return ResourceState(version); } + + // If true, we currently have no version of this resource - we are waiting for the server to + // provide us with one. + bool isWaitingForServer() const { return version_ == absl::nullopt; } + + void setAsWaitingForServer() { version_ = absl::nullopt; } + void setVersion(absl::string_view version) { version_ = std::string(version); } + + // Must not be called if waitingForServer() == true. + std::string version() const { + ASSERT(version_.has_value()); + return version_.value_or(""); + } + + private: + absl::optional version_; + }; + + void addResourceStateFromServer(const envoy::service::discovery::v3::Resource& resource); + OptRef getRequestedResourceState(absl::string_view resource_name); + OptRef getRequestedResourceState(absl::string_view resource_name) const; + + bool isInitialRequestForLegacyWildcard(); + + // A map from resource name to per-resource version. The keys of this map are exactly the resource + // names we are currently interested in. Those in the waitingForServer state currently don't have + // any version for that resource: we need to inform the server if we lose interest in them, but we + // also need to *not* include them in the initial_resource_versions map upon a reconnect. + absl::node_hash_map requested_resource_state_; + // A map from resource name to per-resource version. The keys of this map are resource names we + // have received as a part of the wildcard subscription. + absl::node_hash_map wildcard_resource_state_; + // Used for storing resources that we lost interest in, but could + // also be a part of wildcard subscription. + absl::node_hash_map ambiguous_resource_state_; + + // Not all xDS resources supports heartbeats due to there being specific information encoded in + // an empty response, which is indistinguishable from a heartbeat in some cases. For now we just + // disable heartbeats for these resources (currently only VHDS). + const bool supports_heartbeats_; + TtlManager ttl_; + + const std::string type_url_; + UntypedConfigUpdateCallbacks& watch_map_; + const LocalInfo::LocalInfo& local_info_; + + bool in_initial_legacy_wildcard_{true}; + bool any_request_sent_yet_in_current_stream_{}; + bool must_send_discovery_request_{}; + + // Tracks changes in our subscription interest since the previous DeltaDiscoveryRequest we sent. + // TODO: Can't use absl::flat_hash_set due to ordering issues in gTest expectation matching. + // 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_; +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 085edec3150d..d05cfdd1a35d 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -158,7 +158,7 @@ GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, 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, options.use_namespace_matching_, resources.empty()); + addSubscription(type_url, options.use_namespace_matching_); return addWatch(type_url, resources, callbacks, resource_decoder, options); } @@ -230,11 +230,10 @@ 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, - const bool wildcard) { - subscriptions_.emplace(type_url, std::make_unique(type_url, local_info_, - use_namespace_matching, - dispatcher_, wildcard)); +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, dispatcher_)); 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 5c2940ebfe8e..84e5d223df63 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -81,10 +81,9 @@ class NewGrpcMuxImpl struct SubscriptionStuff { SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, - const bool use_namespace_matching, Event::Dispatcher& dispatcher, - const bool wildcard) + const bool use_namespace_matching, Event::Dispatcher& dispatcher) : watch_map_(use_namespace_matching), - sub_state_(type_url, watch_map_, local_info, dispatcher, wildcard) {} + sub_state_(type_url, watch_map_, local_info, dispatcher) {} WatchMap watch_map_; DeltaSubscriptionState sub_state_; @@ -138,8 +137,7 @@ class NewGrpcMuxImpl const SubscriptionOptions& options); // Adds a subscription for the type_url to the subscriptions map and order list. - void addSubscription(const std::string& type_url, bool use_namespace_matching, - const bool wildcard); + void addSubscription(const std::string& type_url, bool use_namespace_matching); void trySendDiscoveryRequests(); diff --git a/source/common/config/non-wildcard-resource-state-machine.png b/source/common/config/non-wildcard-resource-state-machine.png new file mode 100644 index 000000000000..999814f6d142 Binary files /dev/null and b/source/common/config/non-wildcard-resource-state-machine.png differ diff --git a/source/common/config/old_delta_subscription_state.cc b/source/common/config/old_delta_subscription_state.cc new file mode 100644 index 000000000000..8a4b9272c30e --- /dev/null +++ b/source/common/config/old_delta_subscription_state.cc @@ -0,0 +1,248 @@ +#include "source/common/config/old_delta_subscription_state.h" + +#include "envoy/event/dispatcher.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/common/assert.h" +#include "source/common/common/hash.h" +#include "source/common/config/utility.h" +#include "source/common/runtime/runtime_features.h" + +namespace Envoy { +namespace Config { + +OldDeltaSubscriptionState::OldDeltaSubscriptionState(std::string type_url, + UntypedConfigUpdateCallbacks& watch_map, + const LocalInfo::LocalInfo& local_info, + Event::Dispatcher& dispatcher) + // TODO(snowp): Hard coding VHDS here is temporary until we can move it away from relying on + // empty resources as updates. + : supports_heartbeats_(type_url != "envoy.config.route.v3.VirtualHost"), + ttl_( + [this](const auto& expired) { + Protobuf::RepeatedPtrField removed_resources; + for (const auto& resource : expired) { + setResourceWaitingForServer(resource); + removed_resources.Add(std::string(resource)); + } + + watch_map_.onConfigUpdate({}, removed_resources, ""); + }, + dispatcher, dispatcher.timeSource()), + type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), + dispatcher_(dispatcher) {} + +void OldDeltaSubscriptionState::updateSubscriptionInterest( + const absl::flat_hash_set& cur_added, + const absl::flat_hash_set& cur_removed) { + if (!wildcard_set_) { + wildcard_set_ = true; + wildcard_ = cur_added.empty() && cur_removed.empty(); + } + for (const auto& a : cur_added) { + setResourceWaitingForServer(a); + // If interest in a resource is removed-then-added (all before a discovery request + // can be sent), we must treat it as a "new" addition: our user may have forgotten its + // copy of the resource after instructing us to remove it, and need to be reminded of it. + names_removed_.erase(a); + names_added_.insert(a); + } + for (const auto& r : cur_removed) { + removeResourceState(r); + // Ideally, when interest in a resource is added-then-removed in between requests, + // we would avoid putting a superfluous "unsubscribe [resource that was never subscribed]" + // in the request. However, the removed-then-added case *does* need to go in the request, + // and due to how we accomplish that, it's difficult to distinguish remove-add-remove from + // add-remove (because "remove-add" has to be treated as equivalent to just "add"). + names_added_.erase(r); + names_removed_.insert(r); + } +} + +// Not having sent any requests yet counts as an "update pending" since you're supposed to resend +// the entirety of your interest at the start of a stream, even if nothing has changed. +bool OldDeltaSubscriptionState::subscriptionUpdatePending() const { + return !names_added_.empty() || !names_removed_.empty() || + !any_request_sent_yet_in_current_stream_ || must_send_discovery_request_; +} + +UpdateAck OldDeltaSubscriptionState::handleResponse( + const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) { + // We *always* copy the response's nonce into the next request, even if we're going to make that + // request a NACK by setting error_detail. + UpdateAck ack(message.nonce(), type_url_); + TRY_ASSERT_MAIN_THREAD { handleGoodResponse(message); } + END_TRY + catch (const EnvoyException& e) { + handleBadResponse(e, ack); + } + return ack; +} + +bool OldDeltaSubscriptionState::isHeartbeatResponse( + const envoy::service::discovery::v3::Resource& resource) const { + if (!supports_heartbeats_ && + !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.vhds_heartbeats")) { + return false; + } + const auto itr = resource_state_.find(resource.name()); + if (itr == resource_state_.end()) { + return false; + } + + return !resource.has_resource() && !itr->second.waitingForServer() && + resource.version() == itr->second.version(); +} + +void OldDeltaSubscriptionState::handleGoodResponse( + const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) { + absl::flat_hash_set names_added_removed; + Protobuf::RepeatedPtrField non_heartbeat_resources; + for (const auto& resource : message.resources()) { + if (!names_added_removed.insert(resource.name()).second) { + throw EnvoyException( + fmt::format("duplicate name {} found among added/updated resources", resource.name())); + } + if (isHeartbeatResponse(resource)) { + continue; + } + non_heartbeat_resources.Add()->CopyFrom(resource); + // DeltaDiscoveryResponses for unresolved aliases don't contain an actual resource + if (!resource.has_resource() && resource.aliases_size() > 0) { + continue; + } + if (message.type_url() != resource.resource().type_url()) { + throw EnvoyException(fmt::format("type URL {} embedded in an individual Any does not match " + "the message-wide type URL {} in DeltaDiscoveryResponse {}", + resource.resource().type_url(), message.type_url(), + message.DebugString())); + } + } + for (const auto& name : message.removed_resources()) { + if (!names_added_removed.insert(name).second) { + throw EnvoyException( + fmt::format("duplicate name {} found in the union of added+removed resources", name)); + } + } + + { + const auto scoped_update = ttl_.scopedTtlUpdate(); + for (const auto& resource : message.resources()) { + if (wildcard_ || resource_state_.contains(resource.name())) { + // Only consider tracked resources. + // NOTE: This is not gonna work for xdstp resources with glob resource matching. + addResourceState(resource); + } + } + } + + watch_map_.onConfigUpdate(non_heartbeat_resources, message.removed_resources(), + message.system_version_info()); + + // If a resource is gone, there is no longer a meaningful version for it that makes sense to + // provide to the server upon stream reconnect: either it will continue to not exist, in which + // case saying nothing is fine, or the server will bring back something new, which we should + // receive regardless (which is the logic that not specifying a version will get you). + // + // So, leave the version map entry present but blank. It will be left out of + // initial_resource_versions messages, but will remind us to explicitly tell the server "I'm + // cancelling my subscription" when we lose interest. + for (const auto& resource_name : message.removed_resources()) { + if (resource_names_.find(resource_name) != resource_names_.end()) { + setResourceWaitingForServer(resource_name); + } + } + ENVOY_LOG(debug, "Delta config for {} accepted with {} resources added, {} removed", type_url_, + message.resources().size(), message.removed_resources().size()); +} + +void OldDeltaSubscriptionState::handleBadResponse(const EnvoyException& e, UpdateAck& ack) { + // Note that error_detail being set is what indicates that a DeltaDiscoveryRequest is a NACK. + ack.error_detail_.set_code(Grpc::Status::WellKnownGrpcStatus::Internal); + ack.error_detail_.set_message(Config::Utility::truncateGrpcStatusMessage(e.what())); + ENVOY_LOG(warn, "delta config for {} rejected: {}", type_url_, e.what()); + watch_map_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); +} + +void OldDeltaSubscriptionState::handleEstablishmentFailure() { + watch_map_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + nullptr); +} + +envoy::service::discovery::v3::DeltaDiscoveryRequest +OldDeltaSubscriptionState::getNextRequestAckless() { + envoy::service::discovery::v3::DeltaDiscoveryRequest request; + must_send_discovery_request_ = false; + if (!any_request_sent_yet_in_current_stream_) { + any_request_sent_yet_in_current_stream_ = true; + // initial_resource_versions "must be populated for first request in a stream". + // Also, since this might be a new server, we must explicitly state *all* of our subscription + // interest. + for (auto const& [resource_name, resource_state] : resource_state_) { + // Populate initial_resource_versions with the resource versions we currently have. + // Resources we are interested in, but are still waiting to get any version of from the + // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) + if (!resource_state.waitingForServer()) { + (*request.mutable_initial_resource_versions())[resource_name] = resource_state.version(); + } + // As mentioned above, fill resource_names_subscribe with everything, including names we + // have yet to receive any resource for unless this is a wildcard subscription, for which + // the first request on a stream must be without any resource names. + if (!wildcard_) { + names_added_.insert(resource_name); + } + } + // Wildcard subscription initial requests must have no resource_names_subscribe. + if (wildcard_) { + names_added_.clear(); + } + names_removed_.clear(); + } + std::copy(names_added_.begin(), names_added_.end(), + Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_subscribe())); + std::copy(names_removed_.begin(), names_removed_.end(), + Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_unsubscribe())); + names_added_.clear(); + names_removed_.clear(); + + request.set_type_url(type_url_); + request.mutable_node()->MergeFrom(local_info_.node()); + return request; +} + +envoy::service::discovery::v3::DeltaDiscoveryRequest +OldDeltaSubscriptionState::getNextRequestWithAck(const UpdateAck& ack) { + envoy::service::discovery::v3::DeltaDiscoveryRequest request = getNextRequestAckless(); + request.set_response_nonce(ack.nonce_); + if (ack.error_detail_.code() != Grpc::Status::WellKnownGrpcStatus::Ok) { + // Don't needlessly make the field present-but-empty if status is ok. + request.mutable_error_detail()->CopyFrom(ack.error_detail_); + } + return request; +} + +void OldDeltaSubscriptionState::addResourceState( + const envoy::service::discovery::v3::Resource& resource) { + if (resource.has_ttl()) { + ttl_.add(std::chrono::milliseconds(DurationUtil::durationToMilliseconds(resource.ttl())), + resource.name()); + } else { + ttl_.clear(resource.name()); + } + + resource_state_[resource.name()] = ResourceState(resource); + resource_names_.insert(resource.name()); +} + +void OldDeltaSubscriptionState::setResourceWaitingForServer(const std::string& resource_name) { + resource_state_[resource_name] = ResourceState(); + resource_names_.insert(resource_name); +} + +void OldDeltaSubscriptionState::removeResourceState(const std::string& resource_name) { + resource_state_.erase(resource_name); + resource_names_.erase(resource_name); +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/old_delta_subscription_state.h b/source/common/config/old_delta_subscription_state.h new file mode 100644 index 000000000000..f8aef137f133 --- /dev/null +++ b/source/common/config/old_delta_subscription_state.h @@ -0,0 +1,123 @@ +#pragma once + +#include "envoy/config/subscription.h" +#include "envoy/event/dispatcher.h" +#include "envoy/grpc/status.h" +#include "envoy/local_info/local_info.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/common/assert.h" +#include "source/common/common/logger.h" +#include "source/common/config/api_version.h" +#include "source/common/config/pausable_ack_queue.h" +#include "source/common/config/ttl.h" +#include "source/common/config/watch_map.h" + +#include "absl/container/node_hash_map.h" + +namespace Envoy { +namespace Config { + +// Tracks the xDS protocol state of an individual ongoing delta xDS session, i.e. a single type_url. +// There can be multiple OldDeltaSubscriptionStates active. They will always all be +// blissfully unaware of each other's existence, even when their messages are +// being multiplexed together by ADS. +class OldDeltaSubscriptionState : public Logger::Loggable { +public: + OldDeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher); + + // Update which resources we're interested in subscribing to. + void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, + const absl::flat_hash_set& cur_removed); + void setMustSendDiscoveryRequest() { must_send_discovery_request_ = true; } + + // Whether there was a change in our subscription interest we have yet to inform the server of. + bool subscriptionUpdatePending() const; + + void markStreamFresh() { any_request_sent_yet_in_current_stream_ = false; } + + UpdateAck handleResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message); + + void handleEstablishmentFailure(); + + // Returns the next gRPC request proto to be sent off to the server, based on this object's + // understanding of the current protocol state, and new resources that Envoy wants to request. + envoy::service::discovery::v3::DeltaDiscoveryRequest getNextRequestAckless(); + + // The WithAck version first calls the Ack-less version, then adds in the passed-in ack. + envoy::service::discovery::v3::DeltaDiscoveryRequest getNextRequestWithAck(const UpdateAck& ack); + + OldDeltaSubscriptionState(const OldDeltaSubscriptionState&) = delete; + OldDeltaSubscriptionState& operator=(const OldDeltaSubscriptionState&) = delete; + +private: + bool isHeartbeatResponse(const envoy::service::discovery::v3::Resource& resource) const; + void handleGoodResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message); + void handleBadResponse(const EnvoyException& e, UpdateAck& ack); + + class ResourceState { + public: + ResourceState(const envoy::service::discovery::v3::Resource& resource) + : version_(resource.version()) {} + + // Builds a ResourceState in the waitingForServer state. + ResourceState() = default; + + // If true, we currently have no version of this resource - we are waiting for the server to + // provide us with one. + bool waitingForServer() const { return version_ == absl::nullopt; } + + // Must not be called if waitingForServer() == true. + std::string version() const { + ASSERT(version_.has_value()); + return version_.value_or(""); + } + + private: + absl::optional version_; + }; + + // Use these helpers to ensure resource_state_ and resource_names_ get updated together. + void addResourceState(const envoy::service::discovery::v3::Resource& resource); + void setResourceWaitingForServer(const std::string& resource_name); + void removeResourceState(const std::string& resource_name); + + void populateDiscoveryRequest(envoy::service::discovery::v3::DeltaDiscoveryResponse& request); + + // A map from resource name to per-resource version. The keys of this map are exactly the resource + // names we are currently interested in. Those in the waitingForServer state currently don't have + // any version for that resource: we need to inform the server if we lose interest in them, but we + // also need to *not* include them in the initial_resource_versions map upon a reconnect. + absl::node_hash_map resource_state_; + + // Not all xDS resources supports heartbeats due to there being specific information encoded in + // an empty response, which is indistinguishable from a heartbeat in some cases. For now we just + // disable heartbeats for these resources (currently only VHDS). + const bool supports_heartbeats_; + TtlManager ttl_; + // The keys of resource_versions_. Only tracked separately because std::map does not provide an + // iterator into just its keys. + absl::flat_hash_set resource_names_; + + const std::string type_url_; + // Is the subscription is for a wildcard request. + bool wildcard_set_{}; + bool wildcard_{}; + UntypedConfigUpdateCallbacks& watch_map_; + const LocalInfo::LocalInfo& local_info_; + Event::Dispatcher& dispatcher_; + std::chrono::milliseconds init_fetch_timeout_; + + bool any_request_sent_yet_in_current_stream_{}; + bool must_send_discovery_request_{}; + + // Tracks changes in our subscription interest since the previous DeltaDiscoveryRequest we sent. + // TODO: Can't use absl::flat_hash_set due to ordering issues in gTest expectation matching. + // 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_; +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 09bfdd6b2b07..bdbf9f64b9c5 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -36,6 +36,8 @@ namespace Envoy { namespace Config { +constexpr absl::string_view Wildcard = "*"; + /** * Constant Api Type Values, used by envoy::config::core::v3::ApiConfigSource. */ diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 2824ff9d2979..1fc4ed842f86 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -5,6 +5,7 @@ #include "source/common/common/cleanup.h" #include "source/common/common/utility.h" #include "source/common/config/decoded_resource_impl.h" +#include "source/common/config/utility.h" #include "source/common/config/xds_resource.h" namespace Envoy { @@ -49,7 +50,7 @@ void WatchMap::removeDeferredWatches() { AddedRemoved WatchMap::updateWatchInterest(Watch* watch, const absl::flat_hash_set& update_to_these_names) { - if (update_to_these_names.empty()) { + if (update_to_these_names.empty() || update_to_these_names.contains(Wildcard)) { wildcard_watches_.insert(watch); } else { wildcard_watches_.erase(watch); diff --git a/source/common/config/wildcard-resource-state-machine.png b/source/common/config/wildcard-resource-state-machine.png new file mode 100644 index 000000000000..7447a9caaa76 Binary files /dev/null and b/source/common/config/wildcard-resource-state-machine.png differ diff --git a/source/common/config/xds_mux/delta_subscription_state.cc b/source/common/config/xds_mux/delta_subscription_state.cc index 327171480048..244149a92ca3 100644 --- a/source/common/config/xds_mux/delta_subscription_state.cc +++ b/source/common/config/xds_mux/delta_subscription_state.cc @@ -13,11 +13,11 @@ namespace XdsMux { DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - Event::Dispatcher& dispatcher, const bool wildcard) + Event::Dispatcher& dispatcher) : BaseSubscriptionState(std::move(type_url), watch_map, dispatcher), // TODO(snowp): Hard coding VHDS here is temporary until we can move it away from relying on // empty resources as updates. - supports_heartbeats_(type_url_ != "envoy.config.route.v3.VirtualHost"), wildcard_(wildcard) {} + supports_heartbeats_(type_url_ != "envoy.config.route.v3.VirtualHost") {} DeltaSubscriptionState::~DeltaSubscriptionState() = default; @@ -25,7 +25,24 @@ void DeltaSubscriptionState::updateSubscriptionInterest( const absl::flat_hash_set& cur_added, const absl::flat_hash_set& cur_removed) { for (const auto& a : cur_added) { - resource_state_[a] = ResourceState::waitingForServer(); + if (in_initial_legacy_wildcard_ && a != Wildcard) { + in_initial_legacy_wildcard_ = false; + } + // If the requested resource existed as a wildcard resource, + // transition it to requested. Otherwise mark it as a resource + // waiting for the server to receive the version. + if (auto it = wildcard_resource_state_.find(a); it != wildcard_resource_state_.end()) { + requested_resource_state_.insert_or_assign(a, ResourceState::withVersion(it->second)); + wildcard_resource_state_.erase(it); + } else if (it = ambiguous_resource_state_.find(a); it != ambiguous_resource_state_.end()) { + requested_resource_state_.insert_or_assign(a, ResourceState::withVersion(it->second)); + ambiguous_resource_state_.erase(it); + } else { + requested_resource_state_.insert_or_assign(a, ResourceState::waitingForServer()); + } + ASSERT(requested_resource_state_.contains(a)); + ASSERT(!wildcard_resource_state_.contains(a)); + ASSERT(!ambiguous_resource_state_.contains(a)); // If interest in a resource is removed-then-added (all before a discovery request // can be sent), we must treat it as a "new" addition: our user may have forgotten its // copy of the resource after instructing us to remove it, and need to be reminded of it. @@ -33,22 +50,73 @@ void DeltaSubscriptionState::updateSubscriptionInterest( names_added_.insert(a); } for (const auto& r : cur_removed) { - resource_state_.erase(r); + auto actually_erased = false; + // The resource we have lost the interest in could also come from our wildcard subscription. We + // just don't know it at this point. Instead of removing it outright, mark the resource as not + // interesting to us any more and the server will send us an update. If we don't have a wildcard + // subscription then there is no ambiguity and just drop the resource. + if (requested_resource_state_.contains(Wildcard)) { + if (auto it = requested_resource_state_.find(r); it != requested_resource_state_.end()) { + // Wildcard resources always have a version. If our requested resource has no version, it + // won't be a wildcard resource then. If r is Wildcard itself, then it never has a version + // attached to it, so it will not be moved to ambiguous category. + if (!it->second.isWaitingForServer()) { + ambiguous_resource_state_.insert({it->first, it->second.version()}); + } + requested_resource_state_.erase(it); + actually_erased = true; + } + } else { + actually_erased = (requested_resource_state_.erase(r) > 0); + } + ASSERT(!requested_resource_state_.contains(r)); // Ideally, when interest in a resource is added-then-removed in between requests, // we would avoid putting a superfluous "unsubscribe [resource that was never subscribed]" // in the request. However, the removed-then-added case *does* need to go in the request, // and due to how we accomplish that, it's difficult to distinguish remove-add-remove from // add-remove (because "remove-add" has to be treated as equivalent to just "add"). names_added_.erase(r); - names_removed_.insert(r); + if (actually_erased) { + names_removed_.insert(r); + in_initial_legacy_wildcard_ = false; + } + } + // If we unsubscribe from wildcard resource, drop all the resources that came from wildcard from + // cache. + if (cur_removed.contains(Wildcard)) { + wildcard_resource_state_.clear(); + ambiguous_resource_state_.clear(); } } // Not having sent any requests yet counts as an "update pending" since you're supposed to resend // the entirety of your interest at the start of a stream, even if nothing has changed. bool DeltaSubscriptionState::subscriptionUpdatePending() const { - return !names_added_.empty() || !names_removed_.empty() || - !any_request_sent_yet_in_current_stream_ || dynamicContextChanged(); + if (!names_added_.empty() || !names_removed_.empty()) { + return true; + } + // At this point, we have no new resources to subscribe to or any + // resources to unsubscribe from. + if (!any_request_sent_yet_in_current_stream_) { + // If we haven't sent anything on the current stream, but we are actually interested in some + // resource then we obviously need to let the server know about those. + if (!requested_resource_state_.empty()) { + return true; + } + // So there are no new names and we are interested in nothing. This may either mean that we want + // the legacy wildcard subscription to kick in or we actually unsubscribed from everything. If + // the latter is true, then we should not be sending any requests. In such case the initial + // wildcard mode will be false. Otherwise it means that the legacy wildcard request should be + // sent. + return in_initial_legacy_wildcard_; + } + + // At this point, we have no changes in subscription resources and this isn't a first request in + // the stream, so even if there are no resources we are interested in, we can send the request, + // because even if it's empty, it won't be interpreted as legacy wildcard subscription, which can + // only for the first request in the stream. So sending an empty request at this point should be + // harmless. + return dynamicContextChanged(); } bool DeltaSubscriptionState::isHeartbeatResource( @@ -57,13 +125,29 @@ bool DeltaSubscriptionState::isHeartbeatResource( !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.vhds_heartbeats")) { return false; } - const auto itr = resource_state_.find(resource.name()); - if (itr == resource_state_.end()) { + if (resource.has_resource()) { return false; } - return !resource.has_resource() && !itr->second.isWaitingForServer() && - resource.version() == itr->second.version(); + if (const auto maybe_resource = getRequestedResourceState(resource.name()); + maybe_resource.has_value()) { + return !maybe_resource->isWaitingForServer() && resource.version() == maybe_resource->version(); + } + + if (const auto itr = wildcard_resource_state_.find(resource.name()); + itr != wildcard_resource_state_.end()) { + return resource.version() == itr->second; + } + + if (const auto itr = ambiguous_resource_state_.find(resource.name()); + itr != wildcard_resource_state_.end()) { + // In theory we should move the ambiguous resource to wildcard, because probably we shouldn't be + // getting heartbeat responses about resources that we are not interested in, but the server + // could have sent this heartbeat before it learned about our lack of interest in the resource. + return resource.version() == itr->second; + } + + return false; } void DeltaSubscriptionState::handleGoodResponse( @@ -101,11 +185,18 @@ void DeltaSubscriptionState::handleGoodResponse( { const auto scoped_update = ttl_.scopedTtlUpdate(); - for (const auto& resource : message.resources()) { - if (wildcard_ || resource_state_.contains(resource.name())) { - // Only consider tracked resources. - // NOTE: This is not gonna work for xdstp resources with glob resource matching. - addResourceState(resource); + if (requested_resource_state_.contains(Wildcard)) { + for (const auto& resource : message.resources()) { + addResourceStateFromServer(resource); + } + } else { + // We are not subscribed to wildcard, so we only take resources that we explicitly requested + // and ignore the others. + // NOTE: This is not gonna work for xdstp resources with glob resource matching. + for (const auto& resource : message.resources()) { + if (requested_resource_state_.contains(resource.name())) { + addResourceStateFromServer(resource); + } } } } @@ -118,12 +209,18 @@ void DeltaSubscriptionState::handleGoodResponse( // case saying nothing is fine, or the server will bring back something new, which we should // receive regardless (which is the logic that not specifying a version will get you). // - // So, leave the version map entry present but blank. It will be left out of - // initial_resource_versions messages, but will remind us to explicitly tell the server "I'm - // cancelling my subscription" when we lose interest. + // So, leave the version map entry present but blank if we are still interested in the resource. + // It will be left out of initial_resource_versions messages, but will remind us to explicitly + // tell the server "I'm cancelling my subscription" when we lose interest. In case of resources + // received as a part of the wildcard subscription or resources we already lost interest in, we + // just drop them. for (const auto& resource_name : message.removed_resources()) { - if (resource_state_.find(resource_name) != resource_state_.end()) { - resource_state_[resource_name] = ResourceState::waitingForServer(); + if (auto maybe_resource = getRequestedResourceState(resource_name); + maybe_resource.has_value()) { + maybe_resource->setAsWaitingForServer(); + } else if (const auto erased_count = ambiguous_resource_state_.erase(resource_name); + erased_count == 0) { + wildcard_resource_state_.erase(resource_name); } } ENVOY_LOG(debug, "Delta config for {} accepted with {} resources added, {} removed", typeUrl(), @@ -136,25 +233,30 @@ DeltaSubscriptionState::getNextRequestInternal() { request->set_type_url(typeUrl()); if (!any_request_sent_yet_in_current_stream_) { any_request_sent_yet_in_current_stream_ = true; + const bool is_legacy_wildcard = isInitialRequestForLegacyWildcard(); // initial_resource_versions "must be populated for first request in a stream". // Also, since this might be a new server, we must explicitly state *all* of our subscription // interest. - for (auto const& [resource_name, resource_state] : resource_state_) { + for (auto const& [resource_name, resource_state] : requested_resource_state_) { // Populate initial_resource_versions with the resource versions we currently have. // Resources we are interested in, but are still waiting to get any version of from the // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) if (!resource_state.isWaitingForServer()) { (*request->mutable_initial_resource_versions())[resource_name] = resource_state.version(); } - // As mentioned above, fill resource_names_subscribe with everything, including names we - // have yet to receive any resource for unless this is a wildcard subscription, for which - // the first request on a stream must be without any resource names. - if (!wildcard_) { - names_added_.insert(resource_name); - } + // We are going over a list of resources that we are interested in, so add them to + // resource_names_subscribe. + names_added_.insert(resource_name); + } + for (auto const& [resource_name, resource_version] : wildcard_resource_state_) { + (*request->mutable_initial_resource_versions())[resource_name] = resource_version; } - // Wildcard subscription initial requests must have no resource_names_subscribe. - if (wildcard_) { + for (auto const& [resource_name, resource_version] : ambiguous_resource_state_) { + (*request->mutable_initial_resource_versions())[resource_name] = resource_version; + } + // If this is a legacy wildcard request, then make sure that the resource_names_subscribe is + // empty. + if (is_legacy_wildcard) { names_added_.clear(); } names_removed_.clear(); @@ -170,10 +272,60 @@ DeltaSubscriptionState::getNextRequestInternal() { return request; } -void DeltaSubscriptionState::addResourceState( +bool DeltaSubscriptionState::isInitialRequestForLegacyWildcard() { + if (in_initial_legacy_wildcard_) { + requested_resource_state_.insert_or_assign(Wildcard, ResourceState::waitingForServer()); + ASSERT(requested_resource_state_.contains(Wildcard)); + ASSERT(!wildcard_resource_state_.contains(Wildcard)); + ASSERT(!ambiguous_resource_state_.contains(Wildcard)); + return true; + } + + // If we are here, this means that we lost our initial wildcard mode, because we subscribed to + // something in the past. We could still be in the situation now that all we are subscribed to now + // is wildcard resource, so in such case try to send a legacy wildcard subscription request + // anyway. For this to happen, two conditions need to apply: + // + // 1. No change in interest. + // 2. The only requested resource is Wildcard resource. + // + // The invariant of the code here is that this code is executed only when + // subscriptionUpdatePending actually returns true, which in our case can only happen if the + // requested resources state_ isn't empty. + ASSERT(!requested_resource_state_.empty()); + + // If our subscription interest didn't change then the first condition for using legacy wildcard + // subscription is met. + if (!names_added_.empty() || !names_removed_.empty()) { + return false; + } + // If we requested only a wildcard resource then the second condition for using legacy wildcard + // condition is met. + return requested_resource_state_.size() == 1 && + requested_resource_state_.begin()->first == Wildcard; +} + +void DeltaSubscriptionState::addResourceStateFromServer( const envoy::service::discovery::v3::Resource& resource) { setResourceTtl(resource); - resource_state_[resource.name()] = ResourceState(resource.version()); + + if (auto maybe_resource = getRequestedResourceState(resource.name()); + maybe_resource.has_value()) { + // It is a resource that we requested. + maybe_resource->setVersion(resource.version()); + ASSERT(requested_resource_state_.contains(resource.name())); + ASSERT(!wildcard_resource_state_.contains(resource.name())); + ASSERT(!ambiguous_resource_state_.contains(resource.name())); + } else { + // It is a resource that is a part of our wildcard request. + wildcard_resource_state_.insert({resource.name(), resource.version()}); + // The resource could be ambiguous before, but now the ambiguity + // is resolved. + ambiguous_resource_state_.erase(resource.name()); + ASSERT(!requested_resource_state_.contains(resource.name())); + ASSERT(wildcard_resource_state_.contains(resource.name())); + ASSERT(!ambiguous_resource_state_.contains(resource.name())); + } } void DeltaSubscriptionState::setResourceTtl( @@ -189,12 +341,36 @@ void DeltaSubscriptionState::setResourceTtl( void DeltaSubscriptionState::ttlExpiryCallback(const std::vector& expired) { Protobuf::RepeatedPtrField removed_resources; for (const auto& resource : expired) { - resource_state_[resource] = ResourceState::waitingForServer(); - removed_resources.Add(std::string(resource)); + if (auto maybe_resource = getRequestedResourceState(resource); maybe_resource.has_value()) { + maybe_resource->setAsWaitingForServer(); + removed_resources.Add(std::string(resource)); + } else if (const auto erased_count = wildcard_resource_state_.erase(resource) + + ambiguous_resource_state_.erase(resource); + erased_count > 0) { + removed_resources.Add(std::string(resource)); + } } callbacks().onConfigUpdate({}, removed_resources, ""); } +OptRef +DeltaSubscriptionState::getRequestedResourceState(absl::string_view resource_name) { + auto itr = requested_resource_state_.find(resource_name); + if (itr == requested_resource_state_.end()) { + return {}; + } + return {itr->second}; +} + +OptRef +DeltaSubscriptionState::getRequestedResourceState(absl::string_view resource_name) const { + auto itr = requested_resource_state_.find(resource_name); + if (itr == requested_resource_state_.end()) { + return {}; + } + return {itr->second}; +} + } // namespace XdsMux } // namespace Config } // namespace Envoy diff --git a/source/common/config/xds_mux/delta_subscription_state.h b/source/common/config/xds_mux/delta_subscription_state.h index ced0c9fd52f0..f290f2186852 100644 --- a/source/common/config/xds_mux/delta_subscription_state.h +++ b/source/common/config/xds_mux/delta_subscription_state.h @@ -20,7 +20,7 @@ class DeltaSubscriptionState envoy::service::discovery::v3::DeltaDiscoveryRequest> { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - Event::Dispatcher& dispatcher, const bool wildcard); + Event::Dispatcher& dispatcher); ~DeltaSubscriptionState() override; @@ -46,20 +46,26 @@ class DeltaSubscriptionState bool isHeartbeatResource(const envoy::service::discovery::v3::Resource& resource) const; void handleGoodResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message) override; - void addResourceState(const envoy::service::discovery::v3::Resource& resource); + void addResourceStateFromServer(const envoy::service::discovery::v3::Resource& resource); class ResourceState { public: - explicit ResourceState(absl::string_view version) : version_(version) {} // Builds a ResourceVersion in the waitingForServer state. ResourceState() = default; + // Builds a ResourceState with a specific version + ResourceState(absl::string_view version) : version_(version) {} // Self-documenting alias of default constructor. static ResourceState waitingForServer() { return ResourceState(); } + // Self-documenting alias of constructor with version. + static ResourceState withVersion(absl::string_view version) { return ResourceState(version); } // If true, we currently have no version of this resource - we are waiting for the server to // provide us with one. bool isWaitingForServer() const { return version_ == absl::nullopt; } + void setAsWaitingForServer() { version_ = absl::nullopt; } + void setVersion(absl::string_view version) { version_ = std::string(version); } + // Must not be called if waitingForServer() == true. std::string version() const { ASSERT(version_.has_value()); @@ -70,20 +76,29 @@ class DeltaSubscriptionState absl::optional version_; }; + OptRef getRequestedResourceState(absl::string_view resource_name); + OptRef getRequestedResourceState(absl::string_view resource_name) const; + + bool isInitialRequestForLegacyWildcard(); + // Not all xDS resources support heartbeats due to there being specific information encoded in // an empty response, which is indistinguishable from a heartbeat in some cases. For now we just // disable heartbeats for these resources (currently only VHDS). const bool supports_heartbeats_; - // Is the subscription is for a wildcard request. - const bool wildcard_; - // A map from resource name to per-resource version. The keys of this map are exactly the resource // names we are currently interested in. Those in the waitingForServer state currently don't have // any version for that resource: we need to inform the server if we lose interest in them, but we // also need to *not* include them in the initial_resource_versions map upon a reconnect. - absl::node_hash_map resource_state_; - + absl::node_hash_map requested_resource_state_; + // A map from resource name to per-resource version. The keys of this map are resource names we + // have received as a part of the wildcard subscription. + absl::node_hash_map wildcard_resource_state_; + // Used for storing resources that we lost interest in, but could + // also be a part of wildcard subscription. + absl::node_hash_map ambiguous_resource_state_; + + bool in_initial_legacy_wildcard_{true}; bool any_request_sent_yet_in_current_stream_{}; // Tracks changes in our subscription interest since the previous DeltaDiscoveryRequest we sent. @@ -99,8 +114,8 @@ class DeltaSubscriptionStateFactory : public SubscriptionStateFactory makeSubscriptionState(const std::string& type_url, UntypedConfigUpdateCallbacks& callbacks, - OpaqueResourceDecoder&, const bool wildcard) override { - return std::make_unique(type_url, callbacks, dispatcher_, wildcard); + OpaqueResourceDecoder&) override { + return std::make_unique(type_url, callbacks, dispatcher_); } private: diff --git a/source/common/config/xds_mux/grpc_mux_impl.cc b/source/common/config/xds_mux/grpc_mux_impl.cc index 84d3e3b5c617..c3e2d496b7f0 100644 --- a/source/common/config/xds_mux/grpc_mux_impl.cc +++ b/source/common/config/xds_mux/grpc_mux_impl.cc @@ -86,9 +86,8 @@ Config::GrpcMuxWatchPtr GrpcMuxImpl::addWatch( watch_map = watch_maps_.emplace(type_url, std::make_unique(options.use_namespace_matching_)) .first; - subscriptions_.emplace( - type_url, subscription_state_factory_->makeSubscriptionState( - type_url, *watch_maps_[type_url], resource_decoder, resources.empty())); + subscriptions_.emplace(type_url, subscription_state_factory_->makeSubscriptionState( + type_url, *watch_maps_[type_url], resource_decoder)); subscription_ordering_.emplace_back(type_url); } diff --git a/source/common/config/xds_mux/sotw_subscription_state.h b/source/common/config/xds_mux/sotw_subscription_state.h index 86063198f5a7..0376a22c58a7 100644 --- a/source/common/config/xds_mux/sotw_subscription_state.h +++ b/source/common/config/xds_mux/sotw_subscription_state.h @@ -68,7 +68,7 @@ class SotwSubscriptionStateFactory : public SubscriptionStateFactory makeSubscriptionState(const std::string& type_url, UntypedConfigUpdateCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder, const bool) override { + OpaqueResourceDecoder& resource_decoder) override { return std::make_unique(type_url, callbacks, dispatcher_, resource_decoder); } diff --git a/source/common/config/xds_mux/subscription_state.h b/source/common/config/xds_mux/subscription_state.h index 9f9b48cd7723..a440b8a5a889 100644 --- a/source/common/config/xds_mux/subscription_state.h +++ b/source/common/config/xds_mux/subscription_state.h @@ -123,8 +123,7 @@ template class SubscriptionStateFactory { // Note that, outside of tests, we expect callbacks to always be a WatchMap. virtual std::unique_ptr makeSubscriptionState(const std::string& type_url, UntypedConfigUpdateCallbacks& callbacks, - OpaqueResourceDecoder& resource_decoder, - const bool wildcard) PURE; + OpaqueResourceDecoder& resource_decoder) PURE; }; } // namespace XdsMux diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index c82eff101ad5..966d1e7d9f21 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -89,6 +89,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.vhds_heartbeats", "envoy.reloadable_features.wasm_cluster_name_envoy_grpc", "envoy.reloadable_features.upstream_http2_flood_checks", + "envoy.restart_features.explicit_wildcard_resource", "envoy.restart_features.use_apple_api_for_dns_lookups", // Misplaced flags: please do not add flags to this section. "envoy.reloadable_features.header_map_correctly_coalesce_cookies", diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 933a80251132..639b2d44d394 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -75,6 +75,27 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "delta_subscription_state_old_test", + srcs = ["delta_subscription_state_old_test.cc"], + deps = [ + "//source/common/config:delta_subscription_state_lib", + "//source/common/config:grpc_subscription_lib", + "//source/common/config:new_grpc_mux_lib", + "//source/common/stats:isolated_store_lib", + "//test/mocks:common_lib", + "//test/mocks/config:config_mocks", + "//test/mocks/event:event_mocks", + "//test/mocks/grpc:grpc_mocks", + "//test/mocks/local_info:local_info_mocks", + "//test/mocks/runtime:runtime_mocks", + "//test/test_common:logging_lib", + "//test/test_common:test_runtime_lib", + "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "sotw_subscription_state_test", srcs = ["sotw_subscription_state_test.cc"], diff --git a/test/common/config/delta_subscription_state_old_test.cc b/test/common/config/delta_subscription_state_old_test.cc new file mode 100644 index 000000000000..6d7ceda982ca --- /dev/null +++ b/test/common/config/delta_subscription_state_old_test.cc @@ -0,0 +1,702 @@ +#include + +#include "envoy/config/cluster/v3/cluster.pb.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/config/delta_subscription_state.h" +#include "source/common/config/utility.h" +#include "source/common/stats/isolated_store_impl.h" + +#include "test/mocks/config/mocks.h" +#include "test/mocks/event/mocks.h" +#include "test/mocks/local_info/mocks.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::IsSubstring; +using testing::NiceMock; +using testing::Throw; +using testing::UnorderedElementsAre; +using testing::UnorderedElementsAreArray; + +namespace Envoy { +namespace Config { +namespace { + +const char TypeUrl[] = "type.googleapis.com/envoy.config.cluster.v3.Cluster"; + +class OldDeltaSubscriptionStateTestBase : public testing::Test { +protected: + OldDeltaSubscriptionStateTestBase(const std::string& type_url, + const absl::flat_hash_set initial_resources = { + "name1", "name2", "name3"}) { + ttl_timer_ = new Event::MockTimer(&dispatcher_); + + // Disable the explicit wildcard resource feature, so OldDeltaSubscriptionState will be picked + // up. + { + TestScopedRuntime scoped_runtime_; + Runtime::LoaderSingleton::getExisting()->mergeValues({ + {"envoy.restart_features.explicit_wildcard_resource", "false"}, + }); + state_ = std::make_unique(type_url, callbacks_, + local_info_, dispatcher_); + } + updateSubscriptionInterest(initial_resources, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), + // UnorderedElementsAre("name1", "name2", "name3")); + UnorderedElementsAreArray(initial_resources.cbegin(), initial_resources.cend())); + } + + void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, + const absl::flat_hash_set& cur_removed) { + state_->updateSubscriptionInterest(cur_added, cur_removed); + } + + std::unique_ptr getNextRequestAckless() { + return std::make_unique( + state_->getNextRequestAckless()); + } + + UpdateAck + handleResponse(const envoy::service::discovery::v3::DeltaDiscoveryResponse& response_proto) { + return state_->handleResponse(response_proto); + } + + UpdateAck deliverDiscoveryResponse( + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& version_info, absl::optional nonce = absl::nullopt, + bool expect_config_update_call = true, absl::optional updated_resources = {}) { + envoy::service::discovery::v3::DeltaDiscoveryResponse message; + *message.mutable_resources() = added_resources; + *message.mutable_removed_resources() = removed_resources; + message.set_system_version_info(version_info); + if (nonce.has_value()) { + message.set_nonce(nonce.value()); + } + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)) + .Times(expect_config_update_call ? 1 : 0) + .WillRepeatedly(Invoke([updated_resources](const auto& added, const auto&, const auto&) { + if (updated_resources) { + EXPECT_EQ(added.size(), *updated_resources); + } + })); + return handleResponse(message); + } + + UpdateAck deliverBadDiscoveryResponse( + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& version_info, std::string nonce, std::string error_message) { + envoy::service::discovery::v3::DeltaDiscoveryResponse message; + *message.mutable_resources() = added_resources; + *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))); + return handleResponse(message); + } + + void markStreamFresh() { state_->markStreamFresh(); } + + bool subscriptionUpdatePending() { return state_->subscriptionUpdatePending(); } + + NiceMock callbacks_; + NiceMock local_info_; + NiceMock dispatcher_; + Event::MockTimer* ttl_timer_; + // We start out interested in three resources: name1, name2, and name3. + std::unique_ptr state_; +}; + +Protobuf::RepeatedPtrField +populateRepeatedResource(std::vector> items) { + Protobuf::RepeatedPtrField add_to; + for (const auto& item : items) { + auto* resource = add_to.Add(); + resource->set_name(item.first); + resource->set_version(item.second); + } + return add_to; +} + +class OldDeltaSubscriptionStateTest : public OldDeltaSubscriptionStateTestBase { +public: + OldDeltaSubscriptionStateTest() : OldDeltaSubscriptionStateTestBase(TypeUrl) {} +}; + +// Delta subscription state of a wildcard subscription request. +class OldWildcardDeltaSubscriptionStateTest : public OldDeltaSubscriptionStateTestBase { +public: + OldWildcardDeltaSubscriptionStateTest() : OldDeltaSubscriptionStateTestBase(TypeUrl, {}) {} +}; + +// Basic gaining/losing interest in resources should lead to subscription updates. +TEST_F(OldDeltaSubscriptionStateTest, SubscribeAndUnsubscribe) { + { + updateSubscriptionInterest({"name4"}, {"name1"}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("name1")); + } + { + updateSubscriptionInterest({"name1"}, {"name3", "name4"}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name1")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("name3", "name4")); + } +} + +// Resources has no subscriptions should not be tracked. +TEST_F(OldDeltaSubscriptionStateTest, NewPushDoesntAddUntrackedResources) { + { // Add "name4", "name5", "name6" and remove "name1", "name2", "name3". + updateSubscriptionInterest({"name4", "name5", "name6"}, {"name1", "name2", "name3"}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name4", "name5", "name6")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), + UnorderedElementsAre("name1", "name2", "name3")); + } + { + // On Reconnection, only "name4", "name5", "name6" are sent. + markStreamFresh(); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name4", "name5", "name6")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + EXPECT_TRUE(cur_request->initial_resource_versions().empty()); + } + // The xDS server's first response includes removed items name1 and 2, and a + // completely unrelated resource "bluhbluh". + { + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource({{"name1", "version1A"}, + {"bluhbluh", "bluh"}, + {"name6", "version6A"}, + {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverDiscoveryResponse(added_resources, {}, "debug1", "nonce1"); + EXPECT_EQ("nonce1", ack.nonce_); + EXPECT_EQ(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + } + { // Simulate a stream reconnection, just to see the current resource_state_. + markStreamFresh(); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name4", "name5", "name6")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + ASSERT_EQ(cur_request->initial_resource_versions().size(), 1); + EXPECT_TRUE(cur_request->initial_resource_versions().contains("name6")); + EXPECT_EQ(cur_request->initial_resource_versions().at("name6"), "version6A"); + } +} + +// Delta xDS reliably queues up and sends all discovery requests, even in situations where it isn't +// strictly necessary. E.g.: if you subscribe but then unsubscribe to a given resource, all before a +// request was able to be sent, two requests will be sent. The following tests demonstrate this. +// +// If Envoy decided it wasn't interested in a resource and then (before a request was sent) decided +// it was again, for all we know, it dropped that resource in between and needs to retrieve it +// again. So, we *should* send a request "re-"subscribing. This means that the server needs to +// interpret the resource_names_subscribe field as "send these resources even if you think Envoy +// already has them". +TEST_F(OldDeltaSubscriptionStateTest, RemoveThenAdd) { + updateSubscriptionInterest({}, {"name3"}); + updateSubscriptionInterest({"name3"}, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name3")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// Due to how our implementation provides the required behavior tested in RemoveThenAdd, the +// add-then-remove case *also* causes the resource to be referred to in the request (as an +// unsubscribe). +// Unlike the remove-then-add case, this one really is unnecessary, and ideally we would have +// the request simply not include any mention of the resource. Oh well. +// This test is just here to illustrate that this behavior exists, not to enforce that it +// should be like this. What *is* important: the server must happily and cleanly ignore +// "unsubscribe from [resource name I have never before referred to]" requests. +TEST_F(OldDeltaSubscriptionStateTest, AddThenRemove) { + updateSubscriptionInterest({"name4"}, {}); + updateSubscriptionInterest({}, {"name4"}); + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("name4")); +} + +// add/remove/add == add. +TEST_F(OldDeltaSubscriptionStateTest, AddRemoveAdd) { + updateSubscriptionInterest({"name4"}, {}); + updateSubscriptionInterest({}, {"name4"}); + updateSubscriptionInterest({"name4"}, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// remove/add/remove == remove. +TEST_F(OldDeltaSubscriptionStateTest, RemoveAddRemove) { + updateSubscriptionInterest({}, {"name3"}); + updateSubscriptionInterest({"name3"}, {}); + updateSubscriptionInterest({}, {"name3"}); + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("name3")); +} + +// Starts with 1,2,3. 4 is added/removed/added. In those same updates, 1,2,3 are +// removed/added/removed. End result should be 4 added and 1,2,3 removed. +TEST_F(OldDeltaSubscriptionStateTest, BothAddAndRemove) { + updateSubscriptionInterest({"name4"}, {"name1", "name2", "name3"}); + updateSubscriptionInterest({"name1", "name2", "name3"}, {"name4"}); + updateSubscriptionInterest({"name4"}, {"name1", "name2", "name3"}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), + UnorderedElementsAre("name1", "name2", "name3")); +} + +TEST_F(OldDeltaSubscriptionStateTest, CumulativeUpdates) { + updateSubscriptionInterest({"name4"}, {}); + updateSubscriptionInterest({"name5"}, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4", "name5")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// Verifies that a sequence of good and bad responses from the server all get the appropriate +// ACKs/NACKs from Envoy. +TEST_F(OldDeltaSubscriptionStateTest, AckGenerated) { + // The xDS server's first response includes items for name1 and 2, but not 3. + { + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverDiscoveryResponse(added_resources, {}, "debug1", "nonce1"); + EXPECT_EQ("nonce1", ack.nonce_); + EXPECT_EQ(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + } + // The next response updates 1 and 2, and adds 3. + { + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource( + {{"name1", "version1B"}, {"name2", "version2B"}, {"name3", "version3A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverDiscoveryResponse(added_resources, {}, "debug2", "nonce2"); + EXPECT_EQ("nonce2", ack.nonce_); + EXPECT_EQ(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + } + // The next response tries but fails to update all 3, and so should produce a NACK. + { + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource( + {{"name1", "version1C"}, {"name2", "version2C"}, {"name3", "version3B"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverBadDiscoveryResponse(added_resources, {}, "debug3", "nonce3", "oh no"); + EXPECT_EQ("nonce3", ack.nonce_); + EXPECT_NE(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + } + // The last response successfully updates all 3. + { + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource( + {{"name1", "version1D"}, {"name2", "version2D"}, {"name3", "version3C"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverDiscoveryResponse(added_resources, {}, "debug4", "nonce4"); + EXPECT_EQ("nonce4", ack.nonce_); + EXPECT_EQ(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + } + // Bad response error detail is truncated if it's too large. + { + const std::string very_large_error_message(1 << 20, 'A'); + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource( + {{"name1", "version1D"}, {"name2", "version2D"}, {"name3", "version3D"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverBadDiscoveryResponse(added_resources, {}, "debug5", "nonce5", + very_large_error_message); + EXPECT_EQ("nonce5", ack.nonce_); + EXPECT_NE(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + EXPECT_TRUE(absl::EndsWith(ack.error_detail_.message(), "AAAAAAA...(truncated)")); + EXPECT_LT(ack.error_detail_.message().length(), very_large_error_message.length()); + } +} + +// Tests population of the initial_resource_versions map in the first request of a new stream. +// Tests that +// 1) resources we have a version of are present in the map, +// 2) resources we are interested in but don't have are not present, and +// 3) resources we have lost interest in are not present. +TEST_F(OldDeltaSubscriptionStateTest, ResourceGoneLeadsToBlankInitialVersion) { + { + // The xDS server's first update includes items for name1 and 2, but not 3. + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + EXPECT_EQ("version1A", cur_request->initial_resource_versions().at("name1")); + EXPECT_EQ("version2A", cur_request->initial_resource_versions().at("name2")); + EXPECT_EQ(cur_request->initial_resource_versions().end(), + cur_request->initial_resource_versions().find("name3")); + } + + { + // The next update updates 1, removes 2, and adds 3. The map should then have 1 and 3. + Protobuf::RepeatedPtrField add1_3 = + populateRepeatedResource({{"name1", "version1B"}, {"name3", "version3A"}}); + Protobuf::RepeatedPtrField remove2; + *remove2.Add() = "name2"; + EXPECT_CALL(*ttl_timer_, disableTimer()).Times(2); + deliverDiscoveryResponse(add1_3, remove2, "debugversion2"); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + EXPECT_EQ("version1B", cur_request->initial_resource_versions().at("name1")); + EXPECT_EQ(cur_request->initial_resource_versions().end(), + cur_request->initial_resource_versions().find("name2")); + EXPECT_EQ("version3A", cur_request->initial_resource_versions().at("name3")); + } + + { + // The next update removes 1 and 3. The map we send the server should be empty... + Protobuf::RepeatedPtrField remove1_3; + *remove1_3.Add() = "name1"; + *remove1_3.Add() = "name3"; + deliverDiscoveryResponse({}, remove1_3, "debugversion3"); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->initial_resource_versions().empty()); + } + + { + // ...but our own map should remember our interest. In particular, losing interest in a + // resource should cause its name to appear in the next request's resource_names_unsubscribe. + updateSubscriptionInterest({"name4"}, {"name1", "name2"}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("name1", "name2")); + } +} + +// For non-wildcard subscription, upon a reconnection, the server is supposed to assume a +// blank slate for the Envoy's state (hence the need for initial_resource_versions). +// The resource_names_subscribe of the first message must therefore be every resource the +// Envoy is interested in. +// +// resource_names_unsubscribe, on the other hand, is always blank in the first request - even if, +// in between the last request of the last stream and the first request of the new stream, Envoy +// lost interest in a resource. The unsubscription implicitly takes effect by simply saying +// nothing about the resource in the newly reconnected stream. +TEST_F(OldDeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnect) { + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + + updateSubscriptionInterest({"name4"}, {"name1"}); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + // Regarding the resource_names_subscribe field: + // name1: do not include: we lost interest. + // name2: yes do include: we are interested, its non-wildcard, and we have a version of it. + // name3: yes do include: even though we don't have a version of it, we are interested. + // name4: yes do include: we are newly interested. (If this wasn't a stream reconnect, only + // name4 would belong in this subscribe field). + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name2", "name3", "name4")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// For wildcard subscription, upon a reconnection, the server is supposed to assume a +// blank slate for the Envoy's state (hence the need for initial_resource_versions), and +// the resource_names_subscribe and resource_names_unsubscribe must be empty (as is expected +// of every wildcard first message). This is true even if in between the last request of the +// last stream and the first request of the new stream, Envoy gained or lost interest in a +// resource. The subscription & unsubscription implicitly takes effect by simply requesting a +// wildcard subscription in the newly reconnected stream. +TEST_F(OldWildcardDeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnect) { + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + + updateSubscriptionInterest({"name3"}, {"name1"}); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + // Regarding the resource_names_subscribe field: + // name1: do not include: we lost interest. + // name2: do not include: we are interested, but for wildcard it shouldn't be provided. + // name4: do not include: although we are newly interested, an initial wildcard request + // must be with no resources. + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// All resources from the server should be tracked. +TEST_F(OldWildcardDeltaSubscriptionStateTest, AllResourcesFromServerAreTrackedInWildcardXDS) { + { // Add "name4", "name5", "name6" and remove "name1", "name2", "name3". + updateSubscriptionInterest({"name4", "name5", "name6"}, {"name1", "name2", "name3"}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name4", "name5", "name6")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), + UnorderedElementsAre("name1", "name2", "name3")); + } + { + // On Reconnection, only "name4", "name5", "name6" are sent. + markStreamFresh(); + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + EXPECT_TRUE(cur_request->initial_resource_versions().empty()); + } + // The xDS server's first response includes removed items name1 and 2, and a + // completely unrelated resource "bluhbluh". + { + Protobuf::RepeatedPtrField added_resources = + populateRepeatedResource({{"name1", "version1A"}, + {"bluhbluh", "bluh"}, + {"name6", "version6A"}, + {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + UpdateAck ack = deliverDiscoveryResponse(added_resources, {}, "debug1", "nonce1"); + EXPECT_EQ("nonce1", ack.nonce_); + EXPECT_EQ(Grpc::Status::WellKnownGrpcStatus::Ok, ack.error_detail_.code()); + } + { // Simulate a stream reconnection, just to see the current resource_state_. + markStreamFresh(); + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + ASSERT_EQ(cur_request->initial_resource_versions().size(), 4); + EXPECT_EQ(cur_request->initial_resource_versions().at("name1"), "version1A"); + EXPECT_EQ(cur_request->initial_resource_versions().at("bluhbluh"), "bluh"); + EXPECT_EQ(cur_request->initial_resource_versions().at("name6"), "version6A"); + EXPECT_EQ(cur_request->initial_resource_versions().at("name2"), "version2A"); + } +} + +// initial_resource_versions should not be present on messages after the first in a stream. +TEST_F(OldDeltaSubscriptionStateTest, InitialVersionMapFirstMessageOnly) { + // First, verify that the first message of a new stream sends initial versions. + { + // The xDS server's first update gives us all three resources. + Protobuf::RepeatedPtrField add_all = + populateRepeatedResource( + {{"name1", "version1A"}, {"name2", "version2A"}, {"name3", "version3A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add_all, {}, "debugversion1"); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + EXPECT_EQ("version1A", cur_request->initial_resource_versions().at("name1")); + EXPECT_EQ("version2A", cur_request->initial_resource_versions().at("name2")); + EXPECT_EQ("version3A", cur_request->initial_resource_versions().at("name3")); + } + // Then, after updating the resources but not reconnecting the stream, verify that initial + // versions are not sent. + { + updateSubscriptionInterest({"name4"}, {}); + // The xDS server updates our resources, and gives us our newly requested one too. + Protobuf::RepeatedPtrField add_all = + populateRepeatedResource({{"name1", "version1B"}, + {"name2", "version2B"}, + {"name3", "version3B"}, + {"name4", "version4A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add_all, {}, "debugversion2"); + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->initial_resource_versions().empty()); + } +} + +TEST_F(OldDeltaSubscriptionStateTest, CheckUpdatePending) { + // Note that the test fixture ctor causes the first request to be "sent", so we start in the + // middle of a stream, with our initially interested resources having been requested already. + EXPECT_FALSE(subscriptionUpdatePending()); + updateSubscriptionInterest({}, {}); // no change + EXPECT_FALSE(subscriptionUpdatePending()); + markStreamFresh(); + EXPECT_TRUE(subscriptionUpdatePending()); // no change, BUT fresh stream + updateSubscriptionInterest({}, {"name3"}); // one removed + EXPECT_TRUE(subscriptionUpdatePending()); + updateSubscriptionInterest({"name3"}, {}); // one added + EXPECT_TRUE(subscriptionUpdatePending()); +} + +// The next three tests test that duplicate resource names (whether additions or removals) cause +// DeltaSubscriptionState to reject the update without even trying to hand it to the consuming +// API's onConfigUpdate(). +TEST_F(OldDeltaSubscriptionStateTest, DuplicatedAdd) { + Protobuf::RepeatedPtrField additions = + populateRepeatedResource({{"name1", "version1A"}, {"name1", "sdfsdfsdfds"}}); + UpdateAck ack = deliverDiscoveryResponse(additions, {}, "debugversion1", absl::nullopt, false); + EXPECT_EQ("duplicate name name1 found among added/updated resources", + ack.error_detail_.message()); +} + +TEST_F(OldDeltaSubscriptionStateTest, DuplicatedRemove) { + Protobuf::RepeatedPtrField removals; + *removals.Add() = "name1"; + *removals.Add() = "name1"; + UpdateAck ack = deliverDiscoveryResponse({}, removals, "debugversion1", absl::nullopt, false); + EXPECT_EQ("duplicate name name1 found in the union of added+removed resources", + ack.error_detail_.message()); +} + +TEST_F(OldDeltaSubscriptionStateTest, AddedAndRemoved) { + Protobuf::RepeatedPtrField additions = + populateRepeatedResource({{"name1", "version1A"}}); + Protobuf::RepeatedPtrField removals; + *removals.Add() = "name1"; + UpdateAck ack = + deliverDiscoveryResponse(additions, removals, "debugversion1", absl::nullopt, false); + EXPECT_EQ("duplicate name name1 found in the union of added+removed resources", + ack.error_detail_.message()); +} + +TEST_F(OldDeltaSubscriptionStateTest, ResourceTTL) { + Event::SimulatedTimeSystem time_system; + time_system.setSystemTime(std::chrono::milliseconds(0)); + + auto create_resource_with_ttl = [](absl::optional ttl_s, + bool include_resource) { + Protobuf::RepeatedPtrField added_resources; + auto* resource = added_resources.Add(); + resource->set_name("name1"); + resource->set_version("version1A"); + + if (include_resource) { + resource->mutable_resource(); + } + + if (ttl_s) { + ProtobufWkt::Duration ttl; + ttl.set_seconds(ttl_s->count()); + resource->mutable_ttl()->CopyFrom(ttl); + } + + return added_resources; + }; + + { + EXPECT_CALL(*ttl_timer_, enabled()); + EXPECT_CALL(*ttl_timer_, enableTimer(std::chrono::milliseconds(1000), _)); + deliverDiscoveryResponse(create_resource_with_ttl(std::chrono::seconds(1), true), {}, "debug1", + "nonce1"); + } + + { + // Increase the TTL. + EXPECT_CALL(*ttl_timer_, enabled()); + EXPECT_CALL(*ttl_timer_, enableTimer(std::chrono::milliseconds(2000), _)); + deliverDiscoveryResponse(create_resource_with_ttl(std::chrono::seconds(2), true), {}, "debug1", + "nonce1", true, 1); + } + + { + // Refresh the TTL with a heartbeat. The resource should not be passed to the update callbacks. + EXPECT_CALL(*ttl_timer_, enabled()); + deliverDiscoveryResponse(create_resource_with_ttl(std::chrono::seconds(2), false), {}, "debug1", + "nonce1", true, 0); + } + + // Remove the TTL. + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(create_resource_with_ttl(absl::nullopt, true), {}, "debug1", "nonce1", + true, 1); + + // Add back the TTL. + EXPECT_CALL(*ttl_timer_, enabled()); + EXPECT_CALL(*ttl_timer_, enableTimer(_, _)); + deliverDiscoveryResponse(create_resource_with_ttl(std::chrono::seconds(2), true), {}, "debug1", + "nonce1"); + + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)); + EXPECT_CALL(*ttl_timer_, disableTimer()); + time_system.setSystemTime(std::chrono::seconds(2)); + + // Invoke the TTL. + ttl_timer_->invokeCallback(); +} + +TEST_F(OldDeltaSubscriptionStateTest, TypeUrlMismatch) { + envoy::service::discovery::v3::DeltaDiscoveryResponse message; + + Protobuf::RepeatedPtrField additions; + auto* resource = additions.Add(); + resource->set_name("name1"); + resource->set_version("version1"); + resource->mutable_resource()->set_type_url("foo"); + + *message.mutable_resources() = additions; + *message.mutable_removed_resources() = {}; + message.set_system_version_info("version1"); + message.set_nonce("nonce1"); + message.set_type_url("bar"); + + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)) + .WillOnce(Invoke([](Envoy::Config::ConfigUpdateFailureReason, const EnvoyException* e) { + EXPECT_TRUE(IsSubstring("", "", + "type URL foo embedded in an individual Any does not match the " + "message-wide type URL bar", + e->what())); + })); + handleResponse(message); +} + +class OldVhdsDeltaSubscriptionStateTest : public OldDeltaSubscriptionStateTestBase { +public: + OldVhdsDeltaSubscriptionStateTest() + : OldDeltaSubscriptionStateTestBase("envoy.config.route.v3.VirtualHost") {} +}; + +TEST_F(OldVhdsDeltaSubscriptionStateTest, ResourceTTL) { + Event::SimulatedTimeSystem time_system; + time_system.setSystemTime(std::chrono::milliseconds(0)); + + TestScopedRuntime scoped_runtime; + + auto create_resource_with_ttl = [](bool include_resource) { + Protobuf::RepeatedPtrField added_resources; + auto* resource = added_resources.Add(); + resource->set_name("name1"); + resource->set_version("version1A"); + + if (include_resource) { + resource->mutable_resource(); + } + + ProtobufWkt::Duration ttl; + ttl.set_seconds(1); + resource->mutable_ttl()->CopyFrom(ttl); + + return added_resources; + }; + + EXPECT_CALL(*ttl_timer_, enabled()); + EXPECT_CALL(*ttl_timer_, enableTimer(std::chrono::milliseconds(1000), _)); + deliverDiscoveryResponse(create_resource_with_ttl(true), {}, "debug1", "nonce1", true, 1); + + // Heartbeat update should not be propagated to the subscription callback. + EXPECT_CALL(*ttl_timer_, enabled()); + deliverDiscoveryResponse(create_resource_with_ttl(false), {}, "debug1", "nonce1", true, 0); + + // When runtime flag is disabled, maintain old behavior where we do propagate + // the update to the subscription callback. + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.vhds_heartbeats", "false"}}); + + EXPECT_CALL(*ttl_timer_, enabled()); + deliverDiscoveryResponse(create_resource_with_ttl(false), {}, "debug1", "nonce1", true, 1); +} + +} // namespace +} // namespace Config +} // namespace Envoy diff --git a/test/common/config/delta_subscription_state_test.cc b/test/common/config/delta_subscription_state_test.cc index b8041de85b6b..e908cbd08047 100644 --- a/test/common/config/delta_subscription_state_test.cc +++ b/test/common/config/delta_subscription_state_test.cc @@ -19,6 +19,7 @@ using testing::IsSubstring; using testing::NiceMock; +using testing::Pair; using testing::Throw; using testing::UnorderedElementsAre; using testing::UnorderedElementsAreArray; @@ -29,27 +30,41 @@ namespace { const char TypeUrl[] = "type.googleapis.com/envoy.config.cluster.v3.Cluster"; enum class LegacyOrUnified { Legacy, Unified }; +const auto WildcardStr = std::string(Wildcard); + +Protobuf::RepeatedPtrField +populateRepeatedResource(std::vector> items) { + Protobuf::RepeatedPtrField add_to; + for (const auto& item : items) { + auto* resource = add_to.Add(); + resource->set_name(item.first); + resource->set_version(item.second); + } + return add_to; +} + +Protobuf::RepeatedPtrField populateRepeatedString(std::vector items) { + Protobuf::RepeatedPtrField add_to; + for (const auto& item : items) { + auto* str = add_to.Add(); + *str = item; + } + return add_to; +} class DeltaSubscriptionStateTestBase : public testing::TestWithParam { protected: - DeltaSubscriptionStateTestBase( - const std::string& type_url, const bool wildcard, LegacyOrUnified legacy_or_unified, - const absl::flat_hash_set initial_resources = {"name1", "name2", "name3"}) + DeltaSubscriptionStateTestBase(const std::string& type_url, LegacyOrUnified legacy_or_unified) : should_use_unified_(legacy_or_unified == LegacyOrUnified::Unified) { ttl_timer_ = new Event::MockTimer(&dispatcher_); if (should_use_unified_) { - state_ = std::make_unique( - type_url, callbacks_, dispatcher_, wildcard); + state_ = std::make_unique(type_url, callbacks_, + dispatcher_); } else { - state_ = std::make_unique( - type_url, callbacks_, local_info_, dispatcher_, wildcard); + state_ = std::make_unique(type_url, callbacks_, + local_info_, dispatcher_); } - updateSubscriptionInterest(initial_resources, {}); - auto cur_request = getNextRequestAckless(); - EXPECT_THAT(cur_request->resource_names_subscribe(), - // UnorderedElementsAre("name1", "name2", "name3")); - UnorderedElementsAreArray(initial_resources.cbegin(), initial_resources.cend())); } void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, @@ -112,6 +127,16 @@ class DeltaSubscriptionStateTestBase : public testing::TestWithParam> added_resources, + std::vector removed_resources, + const std::string& version_info) { + EXPECT_CALL(*ttl_timer_, disableTimer()); + auto add = populateRepeatedResource(added_resources); + auto remove = populateRepeatedString(removed_resources); + return deliverDiscoveryResponse(add, remove, version_info); + } + void markStreamFresh() { if (should_use_unified_) { absl::get<1>(state_)->markStreamFresh(); @@ -138,30 +163,335 @@ class DeltaSubscriptionStateTestBase : public testing::TestWithParam -populateRepeatedResource(std::vector> items) { - Protobuf::RepeatedPtrField add_to; - for (const auto& item : items) { - auto* resource = add_to.Add(); - resource->set_name(item.first); - resource->set_version(item.second); +class DeltaSubscriptionStateTestBlank : public DeltaSubscriptionStateTestBase { +public: + DeltaSubscriptionStateTestBlank() : DeltaSubscriptionStateTestBase(TypeUrl, GetParam()) {} +}; + +INSTANTIATE_TEST_SUITE_P(DeltaSubscriptionStateTestBlank, DeltaSubscriptionStateTestBlank, + testing::ValuesIn({LegacyOrUnified::Legacy, LegacyOrUnified::Unified})); + +// Checks if subscriptionUpdatePending returns correct value depending on scenario. +TEST_P(DeltaSubscriptionStateTestBlank, SubscriptionPendingTest) { + // We should send a request, because nothing has been sent out yet. Note that this means + // subscribing to the wildcard resource. + EXPECT_TRUE(subscriptionUpdatePending()); + getNextRequestAckless(); + + // We should not be sending any requests if nothing yet changed since last time we sent a + // request. Or if out subscription interest was not modified. + EXPECT_FALSE(subscriptionUpdatePending()); + updateSubscriptionInterest({}, {}); + EXPECT_FALSE(subscriptionUpdatePending()); + + // We should send a request, because our interest changed (we are interested in foo now). + updateSubscriptionInterest({"foo"}, {}); + EXPECT_TRUE(subscriptionUpdatePending()); + getNextRequestAckless(); + + // We should send a request after a new stream is established if we are interested in some + // resource. + EXPECT_FALSE(subscriptionUpdatePending()); + markStreamFresh(); + EXPECT_TRUE(subscriptionUpdatePending()); + getNextRequestAckless(); + + // We should send a request, because our interest changed (we are not interested in foo and in + // wildcard resource any more). + EXPECT_FALSE(subscriptionUpdatePending()); + updateSubscriptionInterest({}, {WildcardStr, "foo"}); + EXPECT_TRUE(subscriptionUpdatePending()); + getNextRequestAckless(); + + // We should not be sending anything after stream reestablishing, because we are not interested in + // anything. + markStreamFresh(); + EXPECT_FALSE(subscriptionUpdatePending()); +} + +// Check if requested resources are dropped from the cache immediately after losing interest in them +// in case we don't have a wildcard subscription. In such case there's no ambiguity whether a +// dropped resource could come from the wildcard subscription. +// +// Dropping from the cache can be seen through the initial_resource_versions field in the initial +// request. +TEST_P(DeltaSubscriptionStateTestBlank, ResourceTransitionNonWildcardFromRequestedToDropped) { + updateSubscriptionInterest({"foo", "bar"}, {}); + auto req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre("foo", "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_TRUE(req->initial_resource_versions().empty()); + + deliverSimpleDiscoveryResponse({{"foo", "1"}, {"bar", "1"}}, {}, "d1"); + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre("foo", "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("bar", "1"))); + + updateSubscriptionInterest({}, {"foo"}); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_THAT(req->resource_names_unsubscribe(), UnorderedElementsAre("foo")); + deliverSimpleDiscoveryResponse({}, {"foo"}, "d2"); + + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre("bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), UnorderedElementsAre(Pair("bar", "1"))); +} + +// Check if we keep foo resource in cache even if we lost interest in it. It could be a part of the +// wildcard subscription. +TEST_P(DeltaSubscriptionStateTestBlank, ResourceTransitionWithWildcardFromRequestedToAmbiguous) { + // subscribe to foo and make sure we have it. + updateSubscriptionInterest({WildcardStr, "foo", "bar"}, {}); + auto req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "foo", "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_TRUE(req->initial_resource_versions().empty()); + deliverSimpleDiscoveryResponse({{"foo", "1"}, {"bar", "1"}, {"wild1", "1"}}, {}, "d1"); + + // ensure that foo is a part of resource versions + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "foo", "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("bar", "1"), Pair("wild1", "1"))); + + // unsubscribe from foo just before the stream breaks, make sure we still send the foo initial + // version + updateSubscriptionInterest({}, {"foo"}); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_THAT(req->resource_names_unsubscribe(), UnorderedElementsAre("foo")); + // didn't receive a reply + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("bar", "1"), Pair("wild1", "1"))); +} + +// Check that foo and bar do not appear in initial versions after we lost interest. Foo won't +// appear, because we got a reply from server confirming dropping the resource. Bar won't appear +// because we never got a reply from server with a version of it. +TEST_P(DeltaSubscriptionStateTestBlank, ResourceTransitionWithWildcardFromRequestedToDropped) { + // subscribe to foo and bar and make sure we have it. + updateSubscriptionInterest({WildcardStr, "foo", "bar", "baz"}, {}); + auto req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), + UnorderedElementsAre(WildcardStr, "foo", "bar", "baz")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_TRUE(req->initial_resource_versions().empty()); + deliverSimpleDiscoveryResponse({{"foo", "1"}, {"baz", "1"}, {"wild1", "1"}}, {}, "d1"); + + // ensure that foo is a part of resource versions, bar won't be, because we don't have its version + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), + UnorderedElementsAre(WildcardStr, "foo", "bar", "baz")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("baz", "1"), Pair("wild1", "1"))); + + // unsubscribe from foo and bar, and receive an confirmation about dropping foo. Now neither will + // appear in initial versions in the initial request after breaking the stream. + updateSubscriptionInterest({}, {"foo", "bar"}); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_THAT(req->resource_names_unsubscribe(), UnorderedElementsAre("foo", "bar")); + deliverSimpleDiscoveryResponse({}, {"foo"}, "d2"); + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "baz")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("baz", "1"), Pair("wild1", "1"))); +} + +// Check that we move the resource from wildcard subscription to requested without losing version +// information about it. +TEST_P(DeltaSubscriptionStateTestBlank, ResourceTransitionWithWildcardFromWildcardToRequested) { + updateSubscriptionInterest({}, {}); + auto req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_TRUE(req->initial_resource_versions().empty()); + deliverSimpleDiscoveryResponse({{"foo", "1"}, {"wild1", "1"}}, {}, "d1"); + + updateSubscriptionInterest({"foo"}, {}); + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "foo")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("wild1", "1"))); +} + +// Check that we move the ambiguous resource to requested without losing version information about +// it. +TEST_P(DeltaSubscriptionStateTestBlank, ResourceTransitionWithWildcardFromAmbiguousToRequested) { + updateSubscriptionInterest({WildcardStr, "foo"}, {}); + auto req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "foo")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_TRUE(req->initial_resource_versions().empty()); + deliverSimpleDiscoveryResponse({{"foo", "1"}, {"wild1", "1"}}, {}, "d1"); + + // make foo ambiguous and request it again + updateSubscriptionInterest({}, {"foo"}); + updateSubscriptionInterest({"foo"}, {}); + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "foo")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("wild1", "1"))); +} + +// Check if we correctly decide to send a legacy wildcard initial request. +TEST_P(DeltaSubscriptionStateTestBlank, LegacyWildcardInitialRequests) { + updateSubscriptionInterest({}, {}); + auto req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + deliverSimpleDiscoveryResponse({{"wild1", "1"}}, {}, "d1"); + + // unsubscribing from unknown resource should keep the legacy + // wildcard mode + updateSubscriptionInterest({}, {"unknown"}); + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + + updateSubscriptionInterest({"foo"}, {}); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre("foo")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + deliverSimpleDiscoveryResponse({{"foo", "1"}}, {}, "d1"); + updateSubscriptionInterest({}, {"foo"}); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_THAT(req->resource_names_unsubscribe(), UnorderedElementsAre("foo")); + deliverSimpleDiscoveryResponse({}, {"foo"}, "d1"); + + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); +} + +// Check that ambiguous resources may also receive a heartbeat message. +TEST_P(DeltaSubscriptionStateTestBlank, AmbiguousResourceTTL) { + Event::SimulatedTimeSystem time_system; + time_system.setSystemTime(std::chrono::milliseconds(0)); + + auto create_resource_with_ttl = [](absl::string_view name, absl::string_view version, + absl::optional ttl_s, + bool include_resource) { + Protobuf::RepeatedPtrField added_resources; + auto* resource = added_resources.Add(); + resource->set_name(std::string(name)); + resource->set_version(std::string(version)); + + if (include_resource) { + resource->mutable_resource(); + } + + if (ttl_s) { + ProtobufWkt::Duration ttl; + ttl.set_seconds(ttl_s->count()); + resource->mutable_ttl()->CopyFrom(ttl); + } + + return added_resources; + }; + + updateSubscriptionInterest({WildcardStr, "foo"}, {}); + auto req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre(WildcardStr, "foo")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + { + EXPECT_CALL(*ttl_timer_, enabled()); + EXPECT_CALL(*ttl_timer_, enableTimer(std::chrono::milliseconds(1000), _)); + deliverDiscoveryResponse(create_resource_with_ttl("foo", "1", std::chrono::seconds(1), true), + {}, "debug1", "nonce1"); } - return add_to; + + // make foo ambiguous + updateSubscriptionInterest({}, {"foo"}); + req = getNextRequestAckless(); + EXPECT_TRUE(req->resource_names_subscribe().empty()); + EXPECT_THAT(req->resource_names_unsubscribe(), UnorderedElementsAre("foo")); + { + // Refresh the TTL with a heartbeat. The resource should not be passed to the update callbacks. + EXPECT_CALL(*ttl_timer_, enabled()); + deliverDiscoveryResponse(create_resource_with_ttl("foo", "1", std::chrono::seconds(1), false), + {}, "debug1", "nonce1", true, 0); + } + + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, _)); + EXPECT_CALL(*ttl_timer_, disableTimer()); + time_system.setSystemTime(std::chrono::seconds(2)); + + // Invoke the TTL. + ttl_timer_->invokeCallback(); } -class DeltaSubscriptionStateTest : public DeltaSubscriptionStateTestBase { +// Checks that we ignore resources that we haven't asked for. +TEST_P(DeltaSubscriptionStateTestBlank, IgnoreSuperfluousResources) { + updateSubscriptionInterest({"foo", "bar"}, {}); + auto req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre("foo", "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_TRUE(req->initial_resource_versions().empty()); + deliverSimpleDiscoveryResponse({{"foo", "1"}, {"bar", "1"}, {"did-not-want", "1"}, {"spam", "1"}}, + {}, "d1"); + + // Force a reconnection and resending of the "initial" message. If the initial_resource_versions + // in the message contains resources like did-not-want or spam, we haven't ignored that as we + // should. + markStreamFresh(); + req = getNextRequestAckless(); + EXPECT_THAT(req->resource_names_subscribe(), UnorderedElementsAre("foo", "bar")); + EXPECT_TRUE(req->resource_names_unsubscribe().empty()); + EXPECT_THAT(req->initial_resource_versions(), + UnorderedElementsAre(Pair("foo", "1"), Pair("bar", "1"))); +} + +class DeltaSubscriptionStateTestWithResources : public DeltaSubscriptionStateTestBase { +protected: + DeltaSubscriptionStateTestWithResources( + const std::string& type_url, LegacyOrUnified legacy_or_unified, + const absl::flat_hash_set initial_resources = {"name1", "name2", "name3"}) + : DeltaSubscriptionStateTestBase(type_url, legacy_or_unified) { + updateSubscriptionInterest(initial_resources, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), + // UnorderedElementsAre("name1", "name2", "name3")); + UnorderedElementsAreArray(initial_resources.cbegin(), initial_resources.cend())); + } +}; + +class DeltaSubscriptionStateTest : public DeltaSubscriptionStateTestWithResources { public: - DeltaSubscriptionStateTest() : DeltaSubscriptionStateTestBase(TypeUrl, false, GetParam()) {} + DeltaSubscriptionStateTest() : DeltaSubscriptionStateTestWithResources(TypeUrl, GetParam()) {} }; INSTANTIATE_TEST_SUITE_P(DeltaSubscriptionStateTest, DeltaSubscriptionStateTest, testing::ValuesIn({LegacyOrUnified::Legacy, LegacyOrUnified::Unified})); // Delta subscription state of a wildcard subscription request. -class WildcardDeltaSubscriptionStateTest : public DeltaSubscriptionStateTestBase { +class WildcardDeltaSubscriptionStateTest : public DeltaSubscriptionStateTestWithResources { public: WildcardDeltaSubscriptionStateTest() - : DeltaSubscriptionStateTestBase(TypeUrl, true, GetParam(), {}) {} + : DeltaSubscriptionStateTestWithResources(TypeUrl, GetParam(), {}) {} }; INSTANTIATE_TEST_SUITE_P(WildcardDeltaSubscriptionStateTest, WildcardDeltaSubscriptionStateTest, @@ -444,46 +774,236 @@ TEST_P(DeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnect) { EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); } -// For wildcard subscription, upon a reconnection, the server is supposed to assume a -// blank slate for the Envoy's state (hence the need for initial_resource_versions), and -// the resource_names_subscribe and resource_names_unsubscribe must be empty (as is expected -// of every wildcard first message). This is true even if in between the last request of the -// last stream and the first request of the new stream, Envoy gained or lost interest in a -// resource. The subscription & unsubscription implicitly takes effect by simply requesting a -// wildcard subscription in the newly reconnected stream. -TEST_P(WildcardDeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnect) { +// Check that switching into wildcard subscription after initial +// request switches us into the explicit wildcard mode. +TEST_P(DeltaSubscriptionStateTest, SwitchIntoWildcardMode) { + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + // We call deliverDiscoveryResponse twice in this test. + EXPECT_CALL(*ttl_timer_, disableTimer()).Times(2); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + + // switch into wildcard mode + updateSubscriptionInterest({"name4", WildcardStr}, {"name1"}); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + // Regarding the resource_names_subscribe field: + // name1: do not include: we lost interest. + // name2: yes do include: we are explicitly interested (from test's base constructor) + // name3: yes do include: we are explicitly interested (from test's base constructor) + // name4: yes do include: we are explicitly interested + // *: explicit wildcard subscription + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name2", "name3", "name4", Wildcard)); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + + Protobuf::RepeatedPtrField add4_5 = + populateRepeatedResource({{"name4", "version4A"}, {"name5", "version5A"}}); + deliverDiscoveryResponse(add4_5, {}, "debugversion1"); + + markStreamFresh(); // simulate a stream reconnection + cur_request = getNextRequestAckless(); + // Regarding the resource_names_subscribe field: + // name1: do not include: we lost interest. + // name2: yes do include: we are explicitly interested (from test's base constructor) + // name3: yes do include: we are explicitly interested (from test's base constructor) + // name4: yes do include: we are explicitly interested + // name5: do not include: we are implicitly interested, so this resource should not appear on the + // initial request + // *: explicit wildcard subscription + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre("name2", "name3", "name4", Wildcard)); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// For wildcard subscription, upon a reconnection, the server is supposed to assume a blank slate +// for the Envoy's state (hence the need for initial_resource_versions), and the +// resource_names_subscribe and resource_names_unsubscribe must be empty if we haven't gained any +// new explicit interest in a resource. In such case, the client should send an empty request. +TEST_P(WildcardDeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnectImplicit) { Protobuf::RepeatedPtrField add1_2 = populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); EXPECT_CALL(*ttl_timer_, disableTimer()); deliverDiscoveryResponse(add1_2, {}, "debugversion1"); - updateSubscriptionInterest({"name3"}, {"name1"}); markStreamFresh(); // simulate a stream reconnection auto cur_request = getNextRequestAckless(); // Regarding the resource_names_subscribe field: // name1: do not include: we lost interest. - // name2: do not include: we are interested, but for wildcard it shouldn't be provided. - // name4: do not include: although we are newly interested, an initial wildcard request - // must be with no resources. + // name2: do not include: we are implicitly interested, but for wildcard it shouldn't be provided. + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// For wildcard subscription, upon a reconnection, the server is supposed to assume a blank slate +// for the Envoy's state (hence the need for initial_resource_versions). The +// resource_names_unsubscribe must be empty (as is expected of every wildcard first message). The +// resource_names_subscribe should contain all the resources we are explicitly interested in and a +// special resource denoting a wildcard subscription. +TEST_P(WildcardDeltaSubscriptionStateTest, SubscribeAndUnsubscribeAfterReconnectExplicit) { + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + + updateSubscriptionInterest({"name3"}, {}); + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + // Regarding the resource_names_subscribe field: + // name1: do not include: see below + // name2: do not include: we are implicitly interested, but for wildcard it shouldn't be provided. + // name3: yes do include: we are explicitly interested. + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre(Wildcard, "name3")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// Check the contents of the requests after cancelling the wildcard +// subscription and then reconnection. The second request should look +// like a non-wildcard request, so mention all the known resources in +// the initial request. +TEST_P(WildcardDeltaSubscriptionStateTest, CancellingImplicitWildcardSubscription) { + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + + updateSubscriptionInterest({"name3"}, {WildcardStr}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name3")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre(Wildcard)); + markStreamFresh(); // simulate a stream reconnection + // Regarding the resource_names_subscribe field: + // name1: do not include, see below + // name2: do not include: it came from wildcard subscription we lost interest in, so we are not + // interested in name2 too + // name3: yes do include: we are interested + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name3")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// Check the contents of the requests after cancelling the wildcard +// subscription and then reconnection. The second request should look +// like a non-wildcard request, so mention all the known resources in +// the initial request. +TEST_P(WildcardDeltaSubscriptionStateTest, CancellingExplicitWildcardSubscription) { + Protobuf::RepeatedPtrField add1_2 = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()); + deliverDiscoveryResponse(add1_2, {}, "debugversion1"); + // switch to explicit wildcard subscription + updateSubscriptionInterest({"name3"}, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name3")); + + // cancel wildcard subscription + updateSubscriptionInterest({"name4"}, {WildcardStr}); + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4")); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre(Wildcard)); + markStreamFresh(); // simulate a stream reconnection + // Regarding the resource_names_subscribe field: + // name1: do not include: see name2 + // name2: do not include: it came as a part of wildcard subscription we cancelled, so we are not + // interested in this resource name3: yes do include: we are interested, and it's not wildcard. + // name4: yes do include: we are interested, and it's not wildcard. + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name3", "name4")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// Check that resource changes from being interested in implicitly to explicitly when we update the +// subscription interest. Such resources will show up in the initial wildcard requests +// too. Receiving the update on such resource will not change their interest mode. +TEST_P(WildcardDeltaSubscriptionStateTest, ExplicitInterestOverridesImplicit) { + Protobuf::RepeatedPtrField add1_2_a = + populateRepeatedResource({{"name1", "version1A"}, {"name2", "version2A"}}); + EXPECT_CALL(*ttl_timer_, disableTimer()).Times(2); + deliverDiscoveryResponse(add1_2_a, {}, "debugversion1"); + + // verify that neither name1 nor name2 appears in the initial request (they are of implicit + // interest and initial wildcard request should not contain those). + markStreamFresh(); // simulate a stream reconnection + auto cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + + // express the interest in name1 explicitly and verify that the follow-up request will contain it + // (this also switches the wildcard mode to explicit, but we won't see * in resource names, + // because we already are in wildcard mode). + updateSubscriptionInterest({"name1"}, {}); + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name1")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + + // verify that name1 and * appear in the initial request (name1 is of explicit interest and we are + // in explicit wildcard mode). + markStreamFresh(); // simulate a stream reconnection + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name1", Wildcard)); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + + // verify that getting an update on name1 will keep name1 in the explicit interest mode + Protobuf::RepeatedPtrField add1_2_b = + populateRepeatedResource({{"name1", "version1B"}, {"name2", "version2B"}}); + deliverDiscoveryResponse(add1_2_b, {}, "debugversion1"); + markStreamFresh(); // simulate a stream reconnection + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name1", Wildcard)); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); +} + +// Check that resource changes from being interested in implicitly to explicitly when we update the +// subscription interest. Such resources will show up in the initial wildcard requests +// too. Receiving the update on such resource will not change their interest mode. +TEST_P(WildcardDeltaSubscriptionStateTest, ResetToLegacyWildcardBehaviorOnStreamReset) { + // verify that we will send the legacy wildcard subscription request + // after stream reset + updateSubscriptionInterest({"resource"}, {}); + auto cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("resource")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + updateSubscriptionInterest({}, {"resource"}); + cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("resource")); + markStreamFresh(); // simulate a stream reconnection + cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + + // verify that we will send the legacy wildcard subscription request + // after stream reset and confirming our subscription interest + updateSubscriptionInterest({"resource"}, {}); + cur_request = getNextRequestAckless(); + EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("resource")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); + updateSubscriptionInterest({}, {"resource"}); + cur_request = getNextRequestAckless(); + EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_THAT(cur_request->resource_names_unsubscribe(), UnorderedElementsAre("resource")); + markStreamFresh(); // simulate a stream reconnection + updateSubscriptionInterest({}, {}); + cur_request = getNextRequestAckless(); EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); } // All resources from the server should be tracked. TEST_P(WildcardDeltaSubscriptionStateTest, AllResourcesFromServerAreTrackedInWildcardXDS) { - { // Add "name4", "name5", "name6" and remove "name1", "name2", "name3". - updateSubscriptionInterest({"name4", "name5", "name6"}, {"name1", "name2", "name3"}); + { // Add "name4", "name5", "name6" + updateSubscriptionInterest({"name4", "name5", "name6"}, {}); auto cur_request = getNextRequestAckless(); EXPECT_THAT(cur_request->resource_names_subscribe(), UnorderedElementsAre("name4", "name5", "name6")); - EXPECT_THAT(cur_request->resource_names_unsubscribe(), - UnorderedElementsAre("name1", "name2", "name3")); + EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); } { - // On Reconnection, only "name4", "name5", "name6" are sent. + // On Reconnection, only "name4", "name5", "name6" and wildcard resource are sent. markStreamFresh(); auto cur_request = getNextRequestAckless(); - EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre(WildcardStr, "name4", "name5", "name6")); EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); EXPECT_TRUE(cur_request->initial_resource_versions().empty()); } @@ -503,7 +1023,8 @@ TEST_P(WildcardDeltaSubscriptionStateTest, AllResourcesFromServerAreTrackedInWil { // Simulate a stream reconnection, just to see the current resource_state_. markStreamFresh(); auto cur_request = getNextRequestAckless(); - EXPECT_TRUE(cur_request->resource_names_subscribe().empty()); + EXPECT_THAT(cur_request->resource_names_subscribe(), + UnorderedElementsAre(WildcardStr, "name4", "name5", "name6")); EXPECT_TRUE(cur_request->resource_names_unsubscribe().empty()); ASSERT_EQ(cur_request->initial_resource_versions().size(), 4); EXPECT_EQ(cur_request->initial_resource_versions().at("name1"), "version1A"); @@ -682,10 +1203,10 @@ TEST_P(DeltaSubscriptionStateTest, TypeUrlMismatch) { handleResponse(message); } -class VhdsDeltaSubscriptionStateTest : public DeltaSubscriptionStateTestBase { +class VhdsDeltaSubscriptionStateTest : public DeltaSubscriptionStateTestWithResources { public: VhdsDeltaSubscriptionStateTest() - : DeltaSubscriptionStateTestBase("envoy.config.route.v3.VirtualHost", false, GetParam()) {} + : DeltaSubscriptionStateTestWithResources("envoy.config.route.v3.VirtualHost", GetParam()) {} }; INSTANTIATE_TEST_SUITE_P(VhdsDeltaSubscriptionStateTest, VhdsDeltaSubscriptionStateTest, diff --git a/test/extensions/clusters/redis/BUILD b/test/extensions/clusters/redis/BUILD index 2c72cdfcdd99..5481a6ac3a6e 100644 --- a/test/extensions/clusters/redis/BUILD +++ b/test/extensions/clusters/redis/BUILD @@ -93,6 +93,9 @@ envoy_extension_cc_test( size = "small", srcs = ["redis_cluster_integration_test.cc"], extension_names = ["envoy.clusters.redis"], + # This test takes a while to run specially under tsan. + # Shard it to avoid test timeout. + shard_count = 2, deps = [ "//source/extensions/clusters/redis:redis_cluster", "//source/extensions/clusters/redis:redis_cluster_lb", diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc index 16eff16c9a69..7a0128d09f00 100644 --- a/test/extensions/clusters/redis/redis_cluster_integration_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_integration_test.cc @@ -670,8 +670,8 @@ TEST_P(RedisAdsIntegrationTest, RedisClusterRemoval) { test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1); } -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, RedisAdsIntegrationTest, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, RedisAdsIntegrationTest, + ADS_INTEGRATION_PARAMS); } // namespace } // namespace Envoy diff --git a/test/integration/ads_integration.cc b/test/integration/ads_integration.cc index 4810996d483c..f97ad753fe52 100644 --- a/test/integration/ads_integration.cc +++ b/test/integration/ads_integration.cc @@ -128,6 +128,8 @@ void AdsIntegrationTest::makeSingleRequest() { void AdsIntegrationTest::initialize() { initializeAds(false); } void AdsIntegrationTest::initializeAds(const bool rate_limiting) { + config_helper_.addRuntimeOverride("envoy.restart_features.explicit_wildcard_resource", + oldDssOrNewDss() == OldDssOrNewDss::Old ? "false" : "true"); config_helper_.addConfigModifier([this, &rate_limiting]( envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* ads_config = bootstrap.mutable_dynamic_resources()->mutable_ads_config(); diff --git a/test/integration/ads_integration.h b/test/integration/ads_integration.h index 65f35397aa9a..c56dc2cdc9e3 100644 --- a/test/integration/ads_integration.h +++ b/test/integration/ads_integration.h @@ -15,7 +15,34 @@ namespace Envoy { -class AdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public HttpIntegrationTest { +// Support parameterizing over old DSS vs new DSS. Can be dropped when old DSS goes away. +enum class OldDssOrNewDss { Old, New }; + +// Base class that supports parameterizing over old DSS vs new DSS. Can be replaced with +// Grpc::BaseGrpcClientIntegrationParamTest when old DSS is removed. +class AdsDeltaSotwIntegrationSubStateParamTest + : public Grpc::BaseGrpcClientIntegrationParamTest, + public testing::TestWithParam> { +public: + ~AdsDeltaSotwIntegrationSubStateParamTest() override = default; + static std::string protocolTestParamsToString( + const ::testing::TestParamInfo>& p) { + return fmt::format( + "{}_{}_{}_{}", std::get<0>(p.param) == Network::Address::IpVersion::v4 ? "IPv4" : "IPv6", + std::get<1>(p.param) == Grpc::ClientType::GoogleGrpc ? "GoogleGrpc" : "EnvoyGrpc", + std::get<2>(p.param) == Grpc::SotwOrDelta::Delta ? "Delta" : "StateOfTheWorld", + std::get<3>(p.param) == OldDssOrNewDss::Old ? "OldDSS" : "NewDSS"); + } + Network::Address::IpVersion ipVersion() const override { return std::get<0>(GetParam()); } + Grpc::ClientType clientType() const override { return std::get<1>(GetParam()); } + Grpc::SotwOrDelta sotwOrDelta() const { return std::get<2>(GetParam()); } + OldDssOrNewDss oldDssOrNewDss() const { return std::get<3>(GetParam()); } +}; + +class AdsIntegrationTest : public AdsDeltaSotwIntegrationSubStateParamTest, + public HttpIntegrationTest { public: AdsIntegrationTest(); @@ -62,4 +89,12 @@ class AdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht envoy::admin::v3::RoutesConfigDump getRoutesConfigDump(); }; +// When old delta subscription state goes away, we could replace this macro back with +// DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS. +#define ADS_INTEGRATION_PARAMS \ + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ + testing::ValuesIn(TestEnvironment::getsGrpcVersionsForTest()), \ + testing::Values(Grpc::SotwOrDelta::Sotw, Grpc::SotwOrDelta::Delta), \ + testing::Values(OldDssOrNewDss::Old, OldDssOrNewDss::New)) + } // namespace Envoy diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 3d16ca672995..9013a1d74973 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -26,8 +26,8 @@ using testing::AssertionResult; namespace Envoy { -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsIntegrationTest, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, AdsIntegrationTest, + ADS_INTEGRATION_PARAMS); // Validate basic config delivery and upgrade. TEST_P(AdsIntegrationTest, Basic) { @@ -1007,7 +1007,7 @@ TEST_P(AdsIntegrationTest, RdsAfterLdsInvalidated) { test_server_->waitForCounterGe("listener_manager.listener_create_success", 2); } -class AdsFailIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, +class AdsFailIntegrationTest : public AdsDeltaSotwIntegrationSubStateParamTest, public HttpIntegrationTest { public: AdsFailIntegrationTest() @@ -1029,6 +1029,8 @@ class AdsFailIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, void TearDown() override { cleanUpXdsConnection(); } void initialize() override { + config_helper_.addRuntimeOverride("envoy.restart_features.explicit_wildcard_resource", + oldDssOrNewDss() == OldDssOrNewDss::Old ? "false" : "true"); config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* grpc_service = bootstrap.mutable_dynamic_resources()->mutable_ads_config()->add_grpc_services(); @@ -1042,8 +1044,8 @@ class AdsFailIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsFailIntegrationTest, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, AdsFailIntegrationTest, + ADS_INTEGRATION_PARAMS); // Validate that we don't crash on failed ADS stream. TEST_P(AdsFailIntegrationTest, ConnectDisconnect) { @@ -1054,7 +1056,7 @@ TEST_P(AdsFailIntegrationTest, ConnectDisconnect) { xds_stream_->finishGrpcStream(Grpc::Status::Internal); } -class AdsConfigIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, +class AdsConfigIntegrationTest : public AdsDeltaSotwIntegrationSubStateParamTest, public HttpIntegrationTest { public: AdsConfigIntegrationTest() @@ -1076,6 +1078,8 @@ class AdsConfigIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, void TearDown() override { cleanUpXdsConnection(); } void initialize() override { + config_helper_.addRuntimeOverride("envoy.restart_features.explicit_wildcard_resource", + oldDssOrNewDss() == OldDssOrNewDss::Old ? "false" : "true"); config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* grpc_service = bootstrap.mutable_dynamic_resources()->mutable_ads_config()->add_grpc_services(); @@ -1098,8 +1102,8 @@ class AdsConfigIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsConfigIntegrationTest, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, AdsConfigIntegrationTest, + ADS_INTEGRATION_PARAMS); // This is s regression validating that we don't crash on EDS static Cluster that uses ADS. TEST_P(AdsConfigIntegrationTest, EdsClusterWithAdsConfigSource) { @@ -1247,7 +1251,7 @@ TEST_P(AdsIntegrationTest, SetNodeAlways) { }; // Check if EDS cluster defined in file is loaded before ADS request and used as xDS server -class AdsClusterFromFileIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, +class AdsClusterFromFileIntegrationTest : public AdsDeltaSotwIntegrationSubStateParamTest, public HttpIntegrationTest { public: AdsClusterFromFileIntegrationTest() @@ -1269,6 +1273,8 @@ class AdsClusterFromFileIntegrationTest : public Grpc::DeltaSotwIntegrationParam void TearDown() override { cleanUpXdsConnection(); } void initialize() override { + config_helper_.addRuntimeOverride("envoy.restart_features.explicit_wildcard_resource", + oldDssOrNewDss() == OldDssOrNewDss::Old ? "false" : "true"); config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* grpc_service = bootstrap.mutable_dynamic_resources()->mutable_ads_config()->add_grpc_services(); @@ -1320,8 +1326,8 @@ class AdsClusterFromFileIntegrationTest : public Grpc::DeltaSotwIntegrationParam } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsClusterFromFileIntegrationTest, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, AdsClusterFromFileIntegrationTest, + ADS_INTEGRATION_PARAMS); // Validate if ADS cluster defined as EDS will be loaded from file and connection with ADS cluster // will be established. @@ -1383,8 +1389,8 @@ class AdsIntegrationTestWithRtds : public AdsIntegrationTest { } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsIntegrationTestWithRtds, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, AdsIntegrationTestWithRtds, + ADS_INTEGRATION_PARAMS); TEST_P(AdsIntegrationTestWithRtds, Basic) { initialize(); @@ -1437,8 +1443,8 @@ class AdsIntegrationTestWithRtdsAndSecondaryClusters : public AdsIntegrationTest } }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsIntegrationTestWithRtdsAndSecondaryClusters, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDeltaWildcard, + AdsIntegrationTestWithRtdsAndSecondaryClusters, ADS_INTEGRATION_PARAMS); TEST_P(AdsIntegrationTestWithRtdsAndSecondaryClusters, Basic) { initialize(); @@ -1529,12 +1535,13 @@ class XdsTpAdsIntegrationTest : public AdsIntegrationTest { }; INSTANTIATE_TEST_SUITE_P( - IpVersionsClientTypeDelta, XdsTpAdsIntegrationTest, + IpVersionsClientTypeDeltaWildcard, XdsTpAdsIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), // There should be no variation across clients. testing::Values(Grpc::ClientType::EnvoyGrpc), // Only delta xDS is supported for XdsTp - testing::Values(Grpc::SotwOrDelta::Delta, Grpc::SotwOrDelta::UnifiedDelta))); + testing::Values(Grpc::SotwOrDelta::Delta, Grpc::SotwOrDelta::UnifiedDelta), + testing::Values(OldDssOrNewDss::Old, OldDssOrNewDss::New))); TEST_P(XdsTpAdsIntegrationTest, Basic) { initialize(); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 49df254f0c66..46e6fb03ec87 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -31,6 +31,7 @@ CDN CDS CEL DSR +DSS EBADF ENOTCONN EPIPE