Add support for explicit wildcard resource #16855

krnowak · 2021-06-07T18:00:07Z

Commit Message:
This enables a client to send a special resource named * to a server as a way to express an interest in a wildcard subscription. That way the wildcard subscription can coexist with subscriptions to other resources on the same stream. The wildcard resource, like any other resource, can be subscribed to and unsubscribed from at any time. The legacy way of enabling wildcard subscription (by sending an empty initial message) is still supported, but otherwise behaves just like a subscription to *. This behavior is gated behind the enabled-by-default "envoy.restart_features.explicit_wildcard_resource" runtime guard.

Additional Description:
N/A

Risk Level:
High, changes in wildcard mode.

Testing:
As a part of #15523. Also done internally.

Docs Changes:
Updated version history.

Release Notes:

xds: implemented the wildcard resource, which can be used by xDS to express an interest in wildcard subscription in a way that allows subscriptions to other resources coexist on the same stream instead of being ignored. This means that the resource name * is reserved. See :ref:wildcard mode description <xds_protocol_resource_hints> and :ref:unsubscribing from resources <xds_protocol_unsubscribing> for details. This behavior is gated behind the envoy.restart_features.explicit_wildcard_resource runtime guard.

Platform Specific Features:
N/A

Runtime guard:
envoy.restart_features.explicit_wildcard_resource enabled by default.

[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

markdroth · 2021-06-07T18:51:03Z

docs/root/api-docs/xds_protocol.rst

-that once a stream has entered wildcard mode for a given resource type, there is no way to change
-the stream out of wildcard mode; resource names specified in any subsequent request on the stream
-will be ignored.
+types, there is also a "wildcard" mode, which comes in two equivalent flavors. First flavor,


As per my last comment in #15857, I don't think we should be describing this as two separate modes. Instead, I think we should change this doc to explain that subscribing to * means a wildcard subscription, and that if the first request on a stream for a given resource type specifies an empty set of subscriptions, that's a syntactic alias for subscribe to * and nothing else.

Please have a look at the rewording. Hopefully it's closer to what you wanted.

Not quite. I've sent you #16861 with what I'm proposing.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

Specify that sending an empty initial request as means for opting in into the wildcard subscription is a special case of sending the wildcard resource name. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

dmitri-d · 2021-06-08T00:16:31Z

source/common/config/delta_subscription_state.cc

@@ -53,6 +58,31 @@ void DeltaSubscriptionState::updateSubscriptionInterest(
    names_added_.erase(r);
    names_removed_.insert(r);
  }
+  switch (mode_) {
+  case WildcardMode::Implicit:
+    if (names_removed_.find("*") != names_removed_.end()) {


I just realised this implementation deviates from how wildcards are currently implemented: a subscription without any resource names is a wildcard one. A wildcard subscription stops being one when a resource name is added to it. If a subscription has the last resource name removed from it, it becomes a wildcard subscription (see https://github.com/envoyproxy/envoy/blob/main/source/common/config/watch_map.cc#L23 and https://github.com/envoyproxy/envoy/blob/main/source/common/config/watch_map.cc#L50).

Another issue is that we now have wildcard-state related logic in two places; I need to think about this...

Another difference is that currently a wildcard subscription always stays a wildcard one: added or removed resources do not affect subscriptions' behaviour after markStreamFresh() has been called, and there's no difference with a regular subscription at other times.

After thinking about this, I think the way the current wildcard mode handles stream refresh might have a problem: it skips over all resource names that have been added, but doesn't remove corresponding resource states. Depending on how the server handles stream refresh (for example the server might reset its state and only send resources it considers necessary, ignoring client subscriptions), we might end up with different susbcription state on the client and the server.

I don't have a strong opinion about this, but I think a wildcard subscription should always stay one (i.e one cannot downgrade it to a regular subscription). If so, and to avoid the problem of an inconsistent state, I think a subscription in "new" wildcard mode should send all its state on stream refresh (alternatively, it should reset its state to the state at the time of creation).

WDYT? @markdroth?

The current spec (without my changes in #16861) says the following:

For Listener and Cluster resource types, there is also a “wildcard” mode, which is triggered when the initial request on the stream for that resource type contains no resource names. In this case, the server should use site-specific business logic to determine the full set of resources that the client is interested in, typically based on the client’s node identification. Note that once a stream has entered wildcard mode for a given resource type, there is no way to change the stream out of wildcard mode; resource names specified in any subsequent request on the stream will be ignored.

And later:

Note that for Listener and Cluster resource types where the stream is in “wildcard” mode (see How the client specifies what resources to return for details), the set of resources being subscribed to is determined by the server instead of the client, so there is no mechanism for the client to unsubscribe from resources.

In other words, currently, wildcard mode for a given resource type is determined by the very first request for that resource type on the stream, and the stream stays in that mode forever -- it is not possible to later change a stream from wildcard mode to non-wildcard mode or vice-versa. If that first request has resource_names unset, then the stream is in wildcard mode for that resource type, and the resource_names field is ignored in all subsequent requests for that resource type. However, if the first request has resource_names set, then the stream is in non-wildcard mode for that resource type, and if there is later a request for that resource type with resource_names unset, that means to unsubscribe from all resources of that type; it does not mean to enter wildcard mode.

One way to think about the change we're making here is that we want to allow exiting wildcard mode after the initial request, but it will still not be possible to enter wildcard mode. (Entering wildcard mode later would break existing gRPC clients, which will send a request with resource_names unset to indicate unsubscribing to all resources of that type.)

Although I think a better way to think about it is what I described in #16861: Instead of wildcard being a mode for the stream as a whole, it's now going to be a special type of subscription (*), and the existing syntax of leaving resource_names unset will just be a syntactic alias for that special subscription.

One note about the wildcard mode:

One way to think about the change we're making here is that we want to allow exiting wildcard mode after the initial request, but it will still not be possible to enter wildcard mode. (Entering wildcard mode later would break existing gRPC clients, which will send a request with resource_names unset to indicate unsubscribing to all resources of that type.)

I actually implemented also reentering into the wildcard subscription in this PR, but the reentering can only be done by sending "*". I imagined the current workflow in general terms (or in terms of SotW) to be like (think of the graphs below as deterministic finite state automata that start at "stream init"):

And for incremental (delta) wildcard workflow would look like this:

Meh, there are typos like "unsubscribe to" where it should be "unsubscribe from", but hopefully those graphs help.

Personally, I'd very much like an explicit wildcard mode.

when reconnection comes into play, there is a chance that non-wildcard stream could become wildcard:

client asks for A, B, C, it's marked as non-wildcard stream

client removed A, B, C, network error happens

client reconnect, not the server would think it's a wildcard stream and begin to send everything to it.

@markdroth

The code takes care of this scenario, what happens here at 3 is "client reconnects, but does not send any initial request, so the server shouldn't be sending anything back".

@stevenzzzz As @krnowak mentioned, that case should never happen, because if the client is no longer subscribed to any resources when it reconnects, it will not send any request for that resource type on the reconnected stream.

I don't see any value for an explicit wildcard mode.

@krnowak It might be a good idea to change the PR title to reflect what it actually does. :)

Right, changed the PR title and the commit message.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

htuch · 2021-06-21T13:41:17Z

@krnowak what's the status of this PR? @dmitri-d if this is ready for implementation review, would you be able to take a first pass? Thanks.

dmitri-d · 2021-06-21T19:16:51Z

@htuch: yes can do.

krnowak · 2021-06-23T16:41:51Z

Hi @htuch, sorry for late reply.

I'm currently swamped with other work, so I didn't find time to rebase it nor to attend the community meeting yesterday. But the good news are that I was told by my employer that I can pick up this work and #15857 in July, so in one week.

I think this PR is ready for review - I'll need to rebase this PR to current main branch, which means dropping whatever changes I have made in the xds_protocol.rst document, because Mark already did a better job at wording than me. But the code should rather not change. So thanks in advance for the reviews!

dmitri-d · 2021-06-25T18:48:10Z

I like @markdroth's idea of not using a state machine for handling of wildcard subscriptions (see the thread starting somewhere here: #16855 (comment)). I think it will be easier to maintain and test in the long run.

…card-mode

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

This is to avoid putting "*" all over the place, but instead have a readable name. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

…card-mode

krnowak · 2021-07-07T11:46:24Z

I updated the PR. So now the wildcard request will always send * (even the initial request), which simplified the state machine to basically a boolean (has_wildcard_subscription_). Probably some further refactoring could be done to completely get rid of the boolean in favor of keeping the wildcard subscription information in the resource_state_ field. I could have a look at it if it's desired.

I would also like to get an advice where to put the Wildcard constant. It is currently declared in source/common/config/watch_map.h, mostly because watch_map.* does not include anything related to wildcard handling, but new_grpc_mux_impl.* and delta_subscription_state.* include watch_map.h. Would a separate header for the constant be better or maybe even move it to e.g. envoy/config/subscription.h?

@markdroth: One XDS protocol specification question though: let's assume I subscribed to * and foo. As a reply to the subscription, I received resources w1, w2 and foo. And then I unsubscribe from *. Then a stream reset happens. What resources the "initial" request after the stream reset should contain? Only foo or also w1 and w2 resources? My gut feeling (from reading xds protocol docs about unsubscribing would be only foo, but currently implementation also would send w1 and w2. It's a new situation now, because it's possible to cancel the wildcard subscription, which was not possible before. A side question also could be how server should behave when it gets a request to unsubscribe from wildcard subscription in incremental variant - should it just stop sending the resources assigned to wildcard, or rather it should send a response with all those wildcard resources in removed_resources?

markdroth · 2021-07-07T15:46:05Z

So now the wildcard request will always send * (even the initial request)

I don't think that's what we want here. As per previous discussion, existing management servers detect wildcard requests by the resource_names_subscribe field being unset, so setting that to * instead would break them. For backward compatibility reasons, I think Envoy will need to continue leaving resource_names_subscribe unset until and unless there is a non-wildcard subscription on the stream.

One XDS protocol specification question though: let's assume I subscribed to * and foo. As a reply to the subscription, I received resources w1, w2 and foo. And then I unsubscribe from *. Then a stream reset happens. What resources the "initial" request after the stream reset should contain? Only foo or also w1 and w2 resources? My gut feeling (from reading xds protocol docs about unsubscribing would be only foo, but currently implementation also would send w1 and w2. It's a new situation now, because it's possible to cancel the wildcard subscription, which was not possible before. A side question also could be how server should behave when it gets a request to unsubscribe from wildcard subscription in incremental variant - should it just stop sending the resources assigned to wildcard, or rather it should send a response with all those wildcard resources in removed_resources?

This problem is not actually unique to the wildcard subscription, and I don't think it's really new. The same would occur if the client was originally subscribed to foo and bar and then unsubscribed from bar but did not receive the removed_resources for bar before the stream failed. I'm not sure how the current code is handling this, but I think that the right way to handle it is for the client to delete a resource from its cache as soon as it unsubscribes from it (at which point the subsequent removed_resources for that resource will be a no-op).

As I mentioned above (#16855 (comment)), there are two fields in the request:

resource_names_subscribe: Indicates the set of resource locators being subscribed to. In the scenario you describe, this would be set to foo only. (If the wildcard subscription had not been cancelled, it would have been set to foo and *. There is no case in which it would include w1 or w2, since those are resources being returned in response to the subscription to *; they are not resource locators being subscribed to.)
initial_resource_versions: Indicates the set of specific resources already seen by the client, along with their versions. In the scenario you describe, this would include only foo, because the client should have deleted the other resources as soon as it unsubscribed from the wildcard. (If the client did not delete the other resources as soon as it unsubscribed from the wildcard, then this field would also include w1 and w2, but the server would ignore them, since they are not actually subscribed to on the new stream. However, the client would then have no way to delete those resources from its cache, which is why I think the right behavior is for the client to delete them as soon as it unsubscribes.)

Note that all of the above assumes that we're talking about the incremental protocol variant. In SotW, there is no equivalent of initial_resource_versions, so w1 and w2 would never be sent by the client even if it hadn't unsubscribed from * before the stream restart. SotW has only the version_info field, which severely limits the ability of the server to safely avoid resending resources that the client already has after a stream restart (for details, see ACK/NACK and resource type instance version).

repokitteh-read-only · 2021-10-06T06:03:16Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16855 (comment) was created by @krnowak.

see: more, trace.

jamesmulcahy · 2021-10-12T20:29:31Z

@dmitri-d Gentle Nudge

htuch · 2021-10-19T03:21:20Z

@krnowak can you merge main again since this has been a while and I'll do the final merge? Thanks.

…card-mode

This can be reverted too, when old DSS goes away. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak · 2021-10-19T12:02:13Z

@htuch: Thanks, done. I needed to add another small commit to fix redis integration tests (something new came with the merge).

htuch · 2021-10-19T22:13:07Z

@krnowak one more merge please :) Thanks.

…card-mode

krnowak · 2021-10-20T12:40:20Z

I don't understand the tsan failure (something about 1 tests failing remotely?). Will try a retest.

krnowak · 2021-10-20T12:40:33Z

/retest

repokitteh-read-only · 2021-10-20T12:40:36Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16855 (comment) was created by @krnowak.

see: more, trace.

The integration test got twice as much test cases to run, because it runs the previous configuration with either new or old DSS. This resulted in breaking the timeout limit for the test. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak · 2021-10-20T13:50:25Z

So the tsan failure was about RedisClusterRemoval test from redis integration test timing out. It's probably because of my merge fix. Probably, because the tests passed earlier. The fix doubled the number of testcases from 8 to 16 (so they are being run against old and new DSS, because RedisIntegrationTest is basically an alias for AdsIntegrationTest, which tests things against new and old DSS).

I suppose the typical fix for test timeouts is sharding, so I tried that, let's see how it goes.

krnowak · 2021-10-20T17:45:22Z

@htuch: Merge done. Seems like sharding did the trick.

htuch

LGTM! Thanks so much @krnowak, this has been epic and paves the way for on-demand CDS, which will be a huge deal.

markdroth · 2021-10-21T13:47:32Z

Awesome to see this finally land! I'm really looking forward to the on-demand CDS functionality!

krnowak · 2021-10-22T06:23:52Z

Thanks for merging it. :) I opened the new PR for ODCDS, since I could not reopen the old one. It's here: #18723.

When a wildcard xDS type (LDS/CDS/SRDS) reconnects from a delta xDS stream, prior to envoy `1.19.0` it would populate the `ResourceNamesSubscribe` field with the full list of currently subscribed items, instead of simply omitting it to infer that it wanted everything (which is what wildcard mode means). This upstream issue was filed in envoyproxy/envoy#16063 and fixed in envoyproxy/envoy#16153 which went out in Envoy `1.19.0` and is fixed in later versions (later refactored in envoyproxy/envoy#16855). This PR conditionally forces LDS/CDS to be wildcard-only even when the connected Envoy requests a non-wildcard subscription, but only does so on versions prior to `1.19.0`, as we should not need to do this on later versions. This fixes the failure case as described here: #11833 (comment) Co-authored-by: Huan Wang <fredwanghuan@gmail.com>

When a wildcard xDS type (LDS/CDS/SRDS) reconnects from a delta xDS stream, prior to envoy `1.19.0` it would populate the `ResourceNamesSubscribe` field with the full list of currently subscribed items, instead of simply omitting it to infer that it wanted everything (which is what wildcard mode means). This upstream issue was filed in envoyproxy/envoy#16063 and fixed in envoyproxy/envoy#16153 which went out in Envoy `1.19.0` and is fixed in later versions (later refactored in envoyproxy/envoy#16855). This PR conditionally forces LDS/CDS to be wildcard-only even when the connected Envoy requests a non-wildcard subscription, but only does so on versions prior to `1.19.0`, as we should not need to do this on later versions. This fixes the failure case as described here: #11833 (comment) Co-authored-by: Huan Wang <fredwanghuan@gmail.com> Co-authored-by: Huan Wang <fredwanghuan@gmail.com>

krnowak added 2 commits June 7, 2021 19:55

docs: Add an explicit wildcard mode

719661c

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

config: Implement explicit wildcard mode in delta gRPC

9b84dfd

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

krnowak mentioned this pull request Jun 7, 2021

CM: Add on-demand cluster discovery functionality #15857

Closed

markdroth suggested changes Jun 7, 2021

View reviewed changes

krnowak added 2 commits June 7, 2021 21:36

test: Add tests for explicit wildcar mode in delta gRPC

a5f4ed7

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

docs: Reword wildcard mode docs

d78ec97

Specify that sending an empty initial request as means for opting in into the wildcard subscription is a special case of sending the wildcard resource name. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

krnowak force-pushed the krnowak/explicit-wildcard-mode branch from feda61b to d78ec97 Compare June 7, 2021 19:38

config: Fix build

2e8ba91

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

markdroth mentioned this pull request Jun 7, 2021

xds: allow "*" to indicate a wildcard subscription #16861

Merged

dmitri-d reviewed Jun 8, 2021

View reviewed changes

config: Fix build

f4ca629

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

zuercher assigned markdroth and junr03 and unassigned junr03 Jun 8, 2021

alyssawilk assigned htuch Jun 21, 2021

htuch added the waiting label Jun 29, 2021

krnowak and others added 3 commits July 2, 2021 09:27

Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…

c008bcc

…card-mode

Drop the entry in version history

3fa6a37

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

Always send an asterisk as a wildcard subscription

3914615

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

repokitteh-read-only bot removed the waiting label Jul 6, 2021

krnowak added 4 commits July 6, 2021 17:34

Better comments, drop redundant tests

1d53769

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

Add constants for wildcards

fa83949

This is to avoid putting "*" all over the place, but instead have a readable name. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

Test fixes

6748f53

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…

02e5370

…card-mode

htuch previously approved these changes Oct 19, 2021

View reviewed changes

krnowak added 2 commits October 19, 2021 09:56

Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…

5e9a130

…card-mode

test: Fix redis ADS integration test params

36b872b

This can be reverted too, when old DSS goes away. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak dismissed htuch’s stale review via 36b872b October 19, 2021 08:54

krnowak requested a review from mattklein123 as a code owner October 19, 2021 08:54

Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…

5a2aac2

…card-mode

htuch approved these changes Oct 21, 2021

View reviewed changes

htuch merged commit 4fc3612 into envoyproxy:main Oct 21, 2021

krnowak deleted the krnowak/explicit-wildcard-mode branch October 21, 2021 06:57

This was referenced Jan 21, 2022

fix unknown cluster issue hashicorp/consul#12160

Closed

xds: fix for delta xDS reconnect bug in LDS/CDS hashicorp/consul#12174

Merged

markdroth mentioned this pull request Jan 5, 2023

Clarify that on-demand CDS does not support SotW xDS #24763

Merged

alyssawilk mentioned this pull request Aug 21, 2023

envoy_restart_features_explicit_wildcard_resource #29166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for explicit wildcard resource #16855

Add support for explicit wildcard resource #16855

krnowak commented Jun 7, 2021 •

edited

Loading

markdroth Jun 7, 2021

krnowak Jun 7, 2021

markdroth Jun 7, 2021

dmitri-d Jun 8, 2021 •

edited

Loading

dmitri-d Jun 8, 2021

markdroth Jun 8, 2021

krnowak Jun 8, 2021

krnowak Jun 8, 2021

stevenzzzz Sep 22, 2021

krnowak Sep 22, 2021

markdroth Sep 22, 2021

markdroth Sep 22, 2021

krnowak Sep 23, 2021

htuch commented Jun 21, 2021

dmitri-d commented Jun 21, 2021

krnowak commented Jun 23, 2021

dmitri-d commented Jun 25, 2021

krnowak commented Jul 7, 2021

markdroth commented Jul 7, 2021

repokitteh-read-only bot commented Oct 6, 2021

jamesmulcahy commented Oct 12, 2021

htuch commented Oct 19, 2021

krnowak commented Oct 19, 2021

htuch commented Oct 19, 2021

krnowak commented Oct 20, 2021

krnowak commented Oct 20, 2021

repokitteh-read-only bot commented Oct 20, 2021

krnowak commented Oct 20, 2021

krnowak commented Oct 20, 2021

htuch left a comment

markdroth commented Oct 21, 2021

krnowak commented Oct 22, 2021

Add support for explicit wildcard resource #16855

Add support for explicit wildcard resource #16855

Conversation

krnowak commented Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jun 21, 2021

dmitri-d commented Jun 21, 2021

krnowak commented Jun 23, 2021

dmitri-d commented Jun 25, 2021

krnowak commented Jul 7, 2021

markdroth commented Jul 7, 2021

repokitteh-read-only bot commented Oct 6, 2021

jamesmulcahy commented Oct 12, 2021

htuch commented Oct 19, 2021

krnowak commented Oct 19, 2021

htuch commented Oct 19, 2021

krnowak commented Oct 20, 2021

krnowak commented Oct 20, 2021

repokitteh-read-only bot commented Oct 20, 2021

krnowak commented Oct 20, 2021

krnowak commented Oct 20, 2021

htuch left a comment

Choose a reason for hiding this comment

markdroth commented Oct 21, 2021

krnowak commented Oct 22, 2021

krnowak commented Jun 7, 2021 •

edited

Loading

dmitri-d Jun 8, 2021 •

edited

Loading