vhds: add support for initial xdstp collection#44129
vhds: add support for initial xdstp collection#44129sschepens wants to merge 5 commits intoenvoyproxy:mainfrom
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
cc @markdroth @adisuissa since I've been chatting to you about this |
b35e941 to
014538f
Compare
adisuissa
left a comment
There was a problem hiding this comment.
High-level comment: as this is an xDS-TP based subscription, it will probably make sense to start moving in a direction that supports the xDS-TP config-sources. WDYT?
|
@adisuissa not sure exactly what you mean, I may be out of context. Do you mean the xdstp |
10cfe9f to
2423239
Compare
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
6ea4da2 to
b3a2962
Compare
| // If the name ends in ``/*``, it indicates a VirtualHost glob collection, | ||
| // which is supported only in the xDS incremental protocol variants. |
There was a problem hiding this comment.
It needs to be a collection either way. If it doesn't end in /*, then it should be interpretted as a list collection instead of a glob collection.
| // The collection prefix (everything before the trailing ``/*``) will be used | ||
| // as the base for on-demand virtual host resource names. |
There was a problem hiding this comment.
I don't think this is what we want. As you mentioned in the PR description, if we're already fetching everything in the collection as the default virtual hosts, then we don't need to re-fetch the same resources as on-demand virtual hosts. Also, using the default name as a prefix doesn't really make sense in the case that it's a list collection instead of a glob collection.
I think that if we want to be able to use xdsp names for the on-demand virtual hosts as well, we should add a separate field to indicate the resource name to use for them. For flexibility, I suggest something like this:
// The virtual host resource name to use for on-demand VHDS requests.
// The string should contain the token `{domain}`, which will be replaced with
// the domain being fetched. For example, if set to
// "xdstp://authority/vhds/on-demand/{domain}", if VHDS needs the domain for
// "example.com", it will subscribe to the resource
// "xdstp://authority/vhds/on-demand/example.com".
string on_demand_virtual_host_resource_name = 3;
Submitting this for initial review.
Adds support for specifying an initial xdstp collection in VHDS, to allow subscribing to an initial resource set, this enables VHDS usage without the need for on-demand.
With this initial approach, when using initial xdstp collection with on-demand, resources will be requested with
<xdstp-prefix>/<domain>instead of<rc name>/<domain>, though I'm not sure this is correct or if it makes sense, since when initially requestingxdstp://something/*we're subscribing to all resources in that collection and hence then on-demand requestingxdstp://something/xyz.comseems awkward.Also refactored xds mux dependent typeUrl pausing logic into a common method that defines typeUrls to pause for a given typeUrl, and made muxes use this to pause typeUrls before response processing and resume before ack. This unifies scattered logic across diferent XDS implementations directly into the muxes. Also has the benefit of allowing VHDS subscriptions to be paused while RDS responses are being processed which was not currently possible.
I'm not really sure if
default_virtual_host_resource_nameis the correct name for the field since:1- we're already in the context of VHDS, do we need to repeat
virtual_host?2- it implies it's a resource_name but it's collections
3- if this also modifies the prefix used by on-demand, then the meaning kind of changes too-
Additional Description:
Risk Level: medium
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]