Skip to content

vhds: add support for initial xdstp collection#44129

Open
sschepens wants to merge 5 commits intoenvoyproxy:mainfrom
sschepens:vhds-xdstp-collection
Open

vhds: add support for initial xdstp collection#44129
sschepens wants to merge 5 commits intoenvoyproxy:mainfrom
sschepens:vhds-xdstp-collection

Conversation

@sschepens
Copy link
Copy Markdown
Contributor

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 requesting xdstp://something/* we're subscribing to all resources in that collection and hence then on-demand requesting xdstp://something/xyz.com seems 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_name is 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:]

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #44129 was opened by sschepens.

see: more, trace.

@sschepens
Copy link
Copy Markdown
Contributor Author

cc @markdroth @adisuissa since I've been chatting to you about this

@sschepens sschepens force-pushed the vhds-xdstp-collection branch 4 times, most recently from b35e941 to 014538f Compare March 27, 2026 13:18
Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@sschepens
Copy link
Copy Markdown
Contributor Author

@adisuissa not sure exactly what you mean, I may be out of context. Do you mean the xdstp config_sources in Bootstrap? I see they are currently hidden and not implemented, not sure what the path here would be.

@sschepens sschepens force-pushed the vhds-xdstp-collection branch 3 times, most recently from 10cfe9f to 2423239 Compare March 27, 2026 19:48
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: sschepens <sebastian.schepens@mercadolibre.com>
@sschepens sschepens force-pushed the vhds-xdstp-collection branch from 6ea4da2 to b3a2962 Compare March 27, 2026 20:31
Comment on lines +174 to +175
// If the name ends in ``/*``, it indicates a VirtualHost glob collection,
// which is supported only in the xDS incremental protocol variants.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +176 to +177
// The collection prefix (everything before the trailing ``/*``) will be used
// as the base for on-demand virtual host resource names.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants