vhds: add support for VHDS in a static route-configuration by adisuissa · Pull Request #43852 · envoyproxy/envoy

adisuissa · 2026-03-09T19:49:18Z

Commit Message: vhds: add support for VHDS in a static route-configuration
Additional Description:
Prior to this work VHDS wasn't working when used in a static RouteConfiguration (that is not fetched via RDS).
This PR enables this feature.
It is inspired by the code from source/common/router/rds_impl.cc.

Risk Level: low - things that were configured and did not work will now work as expected.
Testing: Added unit and integration tests.
Docs Changes: N/A
Release Notes: Added.
Platform Specific Features: N/A

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2026-03-09T19:49:25Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #43852 was opened by adisuissa.

see: more, trace.

adisuissa · 2026-03-09T21:02:08Z

Requires senior maintainer review, assigning Yan.
/assign @yanavlasov

botengyao

lgtm module two questions. Most of the logic is similar to the rds_impl.

/wait

botengyao · 2026-03-09T22:14:43Z

+  // Emulate a config-update information gathering using a dynamic RouteConfigurationReceiver.
+  config_update_info_ = std::make_unique<RouteConfigUpdateReceiverImpl>(
+      route_config_provider_manager.protoTraits(), factory_context);
+  config_update_info_->onRdsUpdate(config, "");


q, if the static route config + VHDS is used, does this change the validate_clusters behavior between static and RDS? https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route.proto.html

Sorry, I think I don't fully understand the question.
The behavior should be as configured, that is, if validate_clusters is true, then the clusters are validated, and if not, they are not validated.
This validate_clusters should define the behavior of the route-configuration, regardless of whether vhds is configured or not.

yes, it is configurable, I was taking a look at the difference between static and RDS (with AI's help), I meant the default behavior change, by emulating the static to a RDS, will the default change? e.g., static+VHDS will now not validate it if it is unset.

I thought that RDS+VHDS will only use the configured knob. Can you point me to where the default value is different?

envoy/source/common/rds/route_config_update_receiver_impl.cc

Line 19 in e465705

false /* not validate unknown cluster */);

specifically this line above, and it is hard coded.

And here

envoy/api/envoy/config/route/v3/route.proto

Lines 98 to 101 in e465705

// is selected at runtime. This setting defaults to true if the route table

// is statically defined via the :ref:`route_config

// <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.route_config>`

// option. This setting default to false if the route table is loaded dynamically via the

, you have better context here and I am just confused, wondering if we need to callout this.

thanks for the reference.
I believe this was added in #20577 and I don't have a full context to why it was allowed in the first place. @JuniorHsu do you have any context on why there's no cluster validation?

I have a hunch that my previous patch didn't take virtual host into account. i'm unsure if route level will check eventually tbh.

validate_clusters is trying to prevent sr drop for unknown cluster. We do use virtual host but unsure if we have sr drop -- at least i didn't hear from other's report.

botengyao · 2026-03-09T22:18:03Z

+  }
+  // If no VHDS is configured, immediately notify the callback that the
+  // virtual-host doesn't exist.
+  auto current_cb = route_config_updated_cb.lock();


should we put the weak_ptr lock inside the post?

I think current_cb is used in both the if statement, and inside the dispatched lambda.
Maybe I'm missing something in your comment, so feel free to clarify.

though there is no vhds, the pointer is upgrade to a shared_ptr, which means the posted lambda can keep the callback alive even if the stream/filter is destroyed before the dispatcher runs it. This seems different from the existing RDS/VHDS pattern:

thread_local_dispatcher.post([route_config_updated_cb] { if (auto cb = route_config_updated_cb.lock()) { (*cb)(false); } });

When I looked at the code I tried to avoid a case where the callback is removed while it is posted to the worker thread. Specifically I don't think that posting an element that will then be out of scope is a good idea.

Can you point me to the lines of code that you mentioned in your comment?
I may be able to give a better answer if I'll see what exactly you are comparing against.

https://github.com/envoyproxy/envoy/pull/43852/changes#diff-0b8eafdf25d4ec6552ec090d37226126154d88e2f753efa9987775895690f4b7R121-R126 and

envoy/source/common/router/rds_impl.cc

Lines 153 to 158 in e465705

std::weak_ptr<Http::RouteConfigUpdatedCallback> current_cb(it->cb_);

it->thread_local_dispatcher_.post([current_cb, host_exists] {

if (auto cb = current_cb.lock()) {

(*cb)(host_exists);

}

});

here are the pointers, and this callback can be from the on_demand filter, and the it was a shard_ptr before and a week_ptr is passed to the provider, and the shared_ptr will be reseted during the on-demand filter destroy.

Thanks for the reference from the original code which allows me to better understand your question.

RE the approach, I thought that acquired the weak_ptr lock makes it a shared pointer, which will keep the object for a longer time (otherwise there will be a race when the on-demand filter is destroyed).

I think this was observed in #9784 and the fix in #11341 is trying to fix the issue by resetting the pointer on destroy, but I'm not sure if that's the right way to check it.
I'll add a similar test to 'VhdsOnDemandUpdateHttpConnectionCloses' that was added in #11341 and run this with tsan.
I'm willing to change this, I just think that the original fix might not be safe.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2026-03-10T02:26:06Z

    - source/common/grpc/google_grpc_creds_impl.cc
    - source/server/drain_manager_impl.cc
    - source/common/router/rds_impl.cc
+    - source/common/router/static_route_provider_impl.cc


I've added a TODO to clean the usage up, and I'll remove it in a near-future PR (requires conversion of c'tors to create functions that I think should be done in a separate PR to avoid a large refactor as part of this PR).

adisuissa · 2026-03-10T18:21:46Z

I'm taking a step back here.
I tried taking the on-demand-VHDS integration tests and parameterize them to work with both the dynamic-RDS and static-route. I think there are a few gaps that would need to be fixed before we can say that VHDS over static-routes works well.
Converting to draft.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

tonya11en · 2026-04-10T05:46:14Z

@yanavlasov PTAL

yanavlasov · 2026-04-10T20:12:59Z

/gemini review

gemini-code-assist

Code Review

This pull request implements Virtual Host Discovery Service (VHDS) support for static route configurations, allowing them to handle on-demand subscriptions and updates. The implementation introduces a VhdsContext within StaticRouteConfigProviderImpl to manage thread-local configurations and subscription logic. The changes also include new unit tests and the enablement of static route types in VHDS integration tests. Review feedback identifies a thread-safety issue in requestVirtualHostsUpdate where configuration data is accessed across threads and suggests refactoring the constructor into a factory method to improve error handling and avoid exceptions.

gemini-code-assist · 2026-04-10T20:18:36Z

+  auto alias = VhdsSubscription::domainNameToAlias(
+      config_update_info_->protobufConfigurationCast().name(), for_domain);


Accessing config_update_info_ on a worker thread is not thread-safe because it is modified on the main thread during onConfigUpdate. Since the route configuration name is static for this provider, it should be captured during construction and stored as a member of VhdsContext to avoid this race condition.

auto alias = VhdsSubscription::domainNameToAlias(route_config_name_, for_domain);

I think this code is safe, since protobufConfigurationCast() is immutable. However it would be much safer if the const correctness would be reflected in the config_update_info_ type by having it be pointing to a const object. This would make it clear that the object can be used across threads. Maybe file an Issue to track this if a quick followup is not possible?

gemini-code-assist · 2026-04-10T20:18:36Z

+      std::weak_ptr<Http::RouteConfigUpdatedCallback> cb_;
+    };
+
+    RouteConfigUpdatePtr config_update_info_;


To fix the thread-safety issue in requestVirtualHostsUpdate, store the route configuration name as a member variable.

Suggested change

RouteConfigUpdatePtr config_update_info_;

const std::string route_config_name_;

RouteConfigUpdatePtr config_update_info_;

Alternatively make it const, so it is clear that the pointer itself is immutable.

yanavlasov · 2026-04-10T20:14:28Z

-      route_config_provider_manager_(route_config_provider_manager) {}
+      route_config_provider_manager_(route_config_provider_manager) {
+  if (config.has_vhds()) {
+    vhds_context_ = std::make_unique<VhdsContext>(config, factory_context, *this,


I think this can be moved into the initializer list.

yanavlasov · 2026-04-10T20:18:09Z

+  }
+  // If no VHDS is configured, immediately notify the callback that the
+  // virtual-host doesn't exist.
+  thread_local_dispatcher.post([current_cb = route_config_updated_cb] {


Does this need to be dispatched via the event loop?

yanavlasov · 2026-04-14T01:36:26Z

+  auto alias = VhdsSubscription::domainNameToAlias(
+      config_update_info_->protobufConfigurationCast().name(), for_domain);


I think this code is safe, since protobufConfigurationCast() is immutable. However it would be much safer if the const correctness would be reflected in the config_update_info_ type by having it be pointing to a const object. This would make it clear that the object can be used across threads. Maybe file an Issue to track this if a quick followup is not possible?

yanavlasov · 2026-04-14T01:39:22Z

+      std::weak_ptr<Http::RouteConfigUpdatedCallback> cb_;
+    };
+
+    RouteConfigUpdatePtr config_update_info_;


Alternatively make it const, so it is clear that the pointer itself is immutable.

yanavlasov · 2026-04-14T01:40:14Z

+    Rds::RouteConfigProviderManager& route_config_provider_manager)
+    : factory_context_(factory_context), tls_(factory_context.threadLocal()) {
+  // Emulate a config-update information gathering using a dynamic RouteConfigurationReceiver.
+  config_update_info_ = std::make_unique<RouteConfigUpdateReceiverImpl>(


I think to make thread safety clear here, I suggest making both the pointer and the pointee const. Gemini is correct to point that this is a weak area, even though right now I think it is safe.

yanavlasov · 2026-04-14T01:40:38Z

/wait

adisuissa added 2 commits March 9, 2026 19:44

vhds: add support for VHDS in a static route-configuration

cad2dba

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into vhds_on_static_route

15f1e4e

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa marked this pull request as ready for review March 9, 2026 19:49

adisuissa requested review from kyessenov and yanavlasov as code owners March 9, 2026 19:49

repokitteh-read-only Bot assigned yanavlasov Mar 9, 2026

botengyao reviewed Mar 9, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Mar 9, 2026

allow exception (with TODO)

db55423

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only Bot removed the waiting label Mar 10, 2026

adisuissa commented Mar 10, 2026

View reviewed changes

adisuissa marked this pull request as draft March 10, 2026 18:21

adisuissa added 4 commits March 25, 2026 19:36

Merge remote-tracking branch 'upstream/main' into vhds_on_static_route

580bf1e

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

allow VHDS tests for both static and dynamic routes

6c176fd

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

update comment

7c17790

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into vhds_on_static_route

e09e8e7

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa marked this pull request as ready for review March 27, 2026 16:04

updating callback

a3925d6

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

gemini-code-assist Bot reviewed Apr 10, 2026

View reviewed changes

envoyproxy deleted a comment from gemini-code-assist Bot Apr 10, 2026

yanavlasov requested changes Apr 14, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 14, 2026

	// is selected at runtime. This setting defaults to true if the route table
	// is statically defined via the :ref:`route_config
	// <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.route_config>`
	// option. This setting default to false if the route table is loaded dynamically via the

	std::weak_ptr<Http::RouteConfigUpdatedCallback> current_cb(it->cb_);
	it->thread_local_dispatcher_.post([current_cb, host_exists] {
	if (auto cb = current_cb.lock()) {
	(*cb)(host_exists);
	}
	});

		auto alias = VhdsSubscription::domainNameToAlias(
		config_update_info_->protobufConfigurationCast().name(), for_domain);

	RouteConfigUpdatePtr config_update_info_;
	const std::string route_config_name_;
	RouteConfigUpdatePtr config_update_info_;

Conversation

adisuissa commented Mar 9, 2026

Uh oh!

repokitteh-read-only Bot commented Mar 9, 2026

Uh oh!

adisuissa commented Mar 9, 2026

Uh oh!

botengyao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Mar 10, 2026

Uh oh!

tonya11en commented Apr 10, 2026

Uh oh!

yanavlasov commented Apr 10, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

botengyao Mar 10, 2026 •

edited

Loading