-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add support for explicit wildcard resource #16855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
htuch
merged 59 commits into
envoyproxy:main
from
kinvolk:krnowak/explicit-wildcard-mode
Oct 21, 2021
Merged
Changes from 5 commits
Commits
Show all changes
59 commits
Select commit
Hold shift + click to select a range
719661c
docs: Add an explicit wildcard mode
krnowak 9b84dfd
config: Implement explicit wildcard mode in delta gRPC
krnowak a5f4ed7
test: Add tests for explicit wildcar mode in delta gRPC
krnowak d78ec97
docs: Reword wildcard mode docs
krnowak 2e8ba91
config: Fix build
krnowak f4ca629
config: Fix build
krnowak c008bcc
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 3fa6a37
Drop the entry in version history
krnowak 3914615
Always send an asterisk as a wildcard subscription
krnowak 1d53769
Better comments, drop redundant tests
krnowak fa83949
Add constants for wildcards
krnowak 6748f53
Test fixes
krnowak 02e5370
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 872e737
Revert "Test fixes"
krnowak 502de45
Revert "Better comments, drop redundant tests"
krnowak 4ba0fc4
Revert "Always send an asterisk as a wildcard subscription"
krnowak 4e34600
New attempt at implementing new wildcard subscriptions
krnowak 98c408b
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 18e9f70
Formatting fixes
krnowak d6e273f
Update wildcard handling in xds delta subscription state
krnowak db884fe
Change Wildcard into constexpr string view
krnowak 9339a0f
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak cb44431
Rework legacy wildcard subscription handling
krnowak fcc53aa
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 9a1855d
Fix typos
krnowak e71317f
Rename function to placate clang tidy
krnowak cb93839
Try to improve coverage
krnowak b7dee94
Factor out legacy wildcard checks to a separate function
krnowak 61361b8
Document the tests
krnowak 63bba65
Fix formatting
krnowak da12ed5
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak e249133
Fix build after merge
krnowak 88587b0
Get rid of containerContains
krnowak da99fc7
Fix formatting
krnowak d9f0803
Document the resource state machine
krnowak 74532e9
Ignore resources that we did not request
krnowak 3d55048
Add a test for ignoring superfluous resources
krnowak f8b758f
Fix formatting
krnowak fd76a69
Drop one assert
krnowak 4d20de1
Try to preserve the legacy wildcard status on ineffective unsubscript…
krnowak ab65125
Expand a bit more on the ambiguous resource category in comments
krnowak c726d04
Update the comments in the xds_mux variant too
krnowak f9d846d
Constify some variables
krnowak bae8643
Drop unused member
krnowak d10ec21
Gate the explicit wildcard resource support
krnowak 508fc9c
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 726d655
Fixes
krnowak 649af01
Build fixes
krnowak 6f4724b
Test the old implementation too
krnowak 7d78a00
docs: Add a note about xds changes
krnowak 93ec187
Fix clang tidy
krnowak 52a66ee
Run ADS integration tests together with old and new DSS
krnowak 044bceb
Add DSS to dictionary
krnowak 559e50a
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 9a34a6b
Fix version history after merge
krnowak 5e9a130
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 36b872b
test: Fix redis ADS integration test params
krnowak 5a2aac2
Merge remote-tracking branch 'origin/main' into krnowak/explicit-wild…
krnowak 55dfdfa
Shard the redis integration test to avoid timeouts
krnowak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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.
Uh oh!
There was an error while loading. Please reload this page.
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 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...
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.
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?
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.
The current spec (without my changes in #16861) says the following:
And later:
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_namesunset, then the stream is in wildcard mode for that resource type, and theresource_namesfield is ignored in all subsequent requests for that resource type. However, if the first request hasresource_namesset, then the stream is in non-wildcard mode for that resource type, and if there is later a request for that resource type withresource_namesunset, 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_namesunset 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 leavingresource_namesunset will just be a syntactic alias for that special subscription.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.
One note about the wildcard mode:
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:

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.
Meh, there are typos like "unsubscribe to" where it should be "unsubscribe from", but hopefully those graphs help.
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.
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:
@markdroth
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.
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".
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.
@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.
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.
@krnowak It might be a good idea to change the PR title to reflect what it actually does. :)
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.
Right, changed the PR title and the commit message.