Add support for explicit wildcard resource #16855
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,37 +14,46 @@ namespace Config { | |
| DeltaSubscriptionState::DeltaSubscriptionState(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<std::string> 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 (auto erased_count = wildcard_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), | ||
| dispatcher_(dispatcher) {} | ||
|
|
||
| void DeltaSubscriptionState::updateSubscriptionInterest( | ||
| const absl::flat_hash_set<std::string>& cur_added, | ||
| const absl::flat_hash_set<std::string>& cur_removed) { | ||
| for (const auto& a : cur_added) { | ||
| // This adds a resource state that is waiting for the server for more information. This also may | ||
| // be a wildcard resource, which is fine too. | ||
| requested_resource_state_.insert_or_assign(a, ResourceState::waitingForServer()); | ||
| wildcard_resource_state_.erase(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) { | ||
| requested_resource_state_.erase(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, | ||
|
|
@@ -53,6 +62,28 @@ void DeltaSubscriptionState::updateSubscriptionInterest( | |
| names_added_.erase(r); | ||
| names_removed_.insert(r); | ||
| } | ||
| // 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(); | ||
| } | ||
| // Check if this is a legacy wildcard subscription request. If we repeatedly call this function | ||
| // with empty cur_added and cur_removed, we keep the legacy wildcard subscription. | ||
| if (is_legacy_wildcard_) { | ||
|
Contributor
There was a problem hiding this comment. @adisuissa: I remember you added an explicit wildcard mode in delta state; was there are a reason you decided against determining it implicitly, like below?
Contributor
Author
There was a problem hiding this comment. If I were to guess, then I would say that before my changes the wildcard mode was an immutable trait of delta state, and we knew at a time of delta state creation whether it's going to be in wildcard mode or not, so why not leverage that knowledge? That certainly led to a simpler code and less work being done when updating a subscription interest. And now the wildcard mode is not an immutable trait, so determining the mode at a creation time does not buy us much any more.
Contributor
There was a problem hiding this comment. IIRC we needed that when a client reconnects on a stream. In wildcard mode the client sends a wildcard/empty-list, and in non-wildcard mode it sends all the resource names. |
||
| is_legacy_wildcard_ = cur_added.empty() && cur_removed.empty(); | ||
| } else { | ||
| // This is a legacy wildcard subscription if we have not expressed interest in any resources so | ||
|
Member
There was a problem hiding this comment. Does this only happen on the first request? Or can it reset the legacy wildcard status on stream reconnect?
Contributor
Author
There was a problem hiding this comment. Hm, turns out to be a tricky question. So at first I thought that there should be a chance of going back to legacy wildcard subscription in scenario like:
I tried to achieve sending the empty request, but the code actually will send envoy/source/common/config/delta_subscription_state.cc Lines 77 to 78 in db884fe But, now imagine another case:
Contributor
There was a problem hiding this comment.
When the stream fails, the state of having previously subscribed to a non-wildcard resource on the stream should reset to false. So when the new stream starts, if the only subscription is the wildcard subscription, I think the right behavior is to send the empty resource names list (legacy subscription).
If we are not subscribed to any resources for a given resource type, then when the stream is re-established, we shouldn't send any request for that resource type. |
||
| // far. Nor we tried to remove any resources. | ||
| is_legacy_wildcard_ = !any_request_sent_yet_in_current_stream_ && | ||
| requested_resource_state_.empty() && names_removed_.empty(); | ||
| if (is_legacy_wildcard_) { | ||
| // Inserting wildcard to requested resource as waiting for server, which means that wildcard | ||
| // resource has no version and should never get one actually. As such, it won't be listed in | ||
| // initial_resource_versions field. | ||
| requested_resource_state_.insert_or_assign(Wildcard, ResourceState::waitingForServer()); | ||
| names_added_.emplace(Wildcard); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Not having sent any requests yet counts as an "update pending" since you're supposed to resend | ||
|
|
@@ -81,13 +112,21 @@ bool DeltaSubscriptionState::isHeartbeatResponse( | |
| !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; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| void DeltaSubscriptionState::handleGoodResponse( | ||
|
|
@@ -124,7 +163,7 @@ void DeltaSubscriptionState::handleGoodResponse( | |
| { | ||
| const auto scoped_update = ttl_.scopedTtlUpdate(); | ||
| for (const auto& resource : message.resources()) { | ||
| addResourceStateFromServer(resource); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -138,10 +177,14 @@ void DeltaSubscriptionState::handleGoodResponse( | |
| // | ||
| // 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. In case of resources received as a part of | ||
| // the wildcard subscription, 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 { | ||
| wildcard_resource_state_.erase(resource_name); | ||
|
Member
There was a problem hiding this comment. Would it be possible to add some invariants (ASSERTs) showing that wildcard resource state and request resource state are disjoint?
Contributor
Author
There was a problem hiding this comment. Sure, I can add them. |
||
| } | ||
| } | ||
| ENVOY_LOG(debug, "Delta config for {} accepted with {} resources added, {} removed", type_url_, | ||
|
|
@@ -170,22 +213,25 @@ DeltaSubscriptionState::getNextRequestAckless() { | |
| // 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_) { | ||
| // Populate initial_resource_versions with the resource versions we currently have. | ||
| (*request.mutable_initial_resource_versions())[resource_name] = resource_version; | ||
| // We are not adding these resources to resource_names_subscribe. | ||
| } | ||
| // 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(); | ||
|
|
@@ -213,7 +259,7 @@ DeltaSubscriptionState::getNextRequestWithAck(const UpdateAck& ack) { | |
| return request; | ||
| } | ||
|
|
||
| void DeltaSubscriptionState::addResourceStateFromServer( | ||
| const envoy::service::discovery::v3::Resource& resource) { | ||
| if (resource.has_ttl()) { | ||
| ttl_.add(std::chrono::milliseconds(DurationUtil::durationToMilliseconds(resource.ttl())), | ||
|
|
@@ -222,18 +268,32 @@ void DeltaSubscriptionState::addResourceState( | |
| 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()); | ||
| } else { | ||
| // It is a resource that is a part of our wildcard request. | ||
| wildcard_resource_state_.insert({resource.name(), resource.version()}); | ||
| } | ||
| } | ||
|
|
||
| OptRef<DeltaSubscriptionState::ResourceState> | ||
| 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<const DeltaSubscriptionState::ResourceState> | ||
| 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 Config | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for a resource to transition back from subscribed to wildcard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not directly I think. Should the client be smart and move the resource from subscribed to wildcard or should the client rely on the server detecting that the client unsubscribed from a resource that is also a part of the wildcard subscription and thus resending the resource? I wonder if this scenario should be clarified in the xds-protocol document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transitioning from subscribed to wildcard is also tricky. Consider the following scenarios:
ras a part of wildcard subscription.r. At this point the client may know thatralso came from wildcard subscription.r, but the client can still keepras a resource coming from wildcard subscription.Another one:
rand wildcard.ras a part of both wildcard subscription and explicit subscription tor.r. At this point the client does not know thatris also a part of the wildcard subscription.So I'd say that the client should rather rely on the server. Basically the server should notice that the client is subscribed to the resource that is also a part of the wildcard resource set, so when the client unsubscribes from such a resource, the server should resend the resource, so the client could get it as a part of the wildcard subscription set.
Other option would be to extend the discovery response with a boolean stating whether the resource is a part of the wildcard resource set too. This could be used by the client to transition the resource from subscribed to wildcard when unsubscribing from the resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the transport protocol actually provides enough information for the client to know that the server is sending the resource as part of a wildcard subscription. Your second scenario is a good example of this. Here's another one:
r.r.Because the client did not receive the response to the first request until after it sent the second request, it has no way to know whether the response it's seeing is a result of the first request or the second, so it cannot assume that
ris present because of the wildcard request.I think the right answer here is to just say that when a client is sending a wildcard subscription, it should never delete resources from its cache when it unsubscribes from a non-wildcard resource on the same stream; instead, it should rely on the resource being removed from the server's next response to do that. (Note that in LDS and CDS, the only two resource types where wildcards are used, the server is required to send all resources that the client is subscribed to in every response, so if a resource is present in one response and absent in the next response, that's a clear indication that the client no longer needs the resource.)