-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
Title: To resolve issues like #7923, or migrate RDS to config-provider-framework, we need to make sure RdsRouteConfigProvider "Listener independent".
Description:
At present a RdsRouteConfigProviderImpl is bound to a listenerImpl, as it holds a FactoryContext references to the ListenerImpl.
If we want to save memory on dedup the copy of RDS route config providers (e.g., #7923 ), we would need to make sure the referenced FactoryContext outlives a ListenerImpl.
Besides that, a RDS Subscription is held globally (weak_ptr) by the SingletonManager[RdsConfigProviderManager],which could be shared between Listeners, but the RDSSubscription also holds a reference to the factoryContext (in this case, ListenerImpl). If LDS updates a listener A, a live RDSSubscription owned by another listener B may crash Envoy when it tries to read from the destructed FactoryContext(ListenerImpl). VHDS implementation has a TODO that stated this concern:
https://github.com/envoyproxy/envoy/blob/master/source/common/router/rds_impl.cc#L113
Proposal:
One way to make the factoryContext referenced by router/... is to pass a Server::CommonFactoryContext (e.g. the Server::Instance ) instead of a Server::FactoryContext (only implemented by ListenerImpl) into the router/...
Right now, most of the APIs desired by objects under router/... are actually from CommonFactoryContext, except for several places that are kinda Listener specific:
- initManager(), which is used to initiate a Subscription, but as SRDS(also VHDS, see issue#7617) comes into Envoy, we need something like the noop_init_manager in scoped_rds.cc
We could possibly pass the current ListernerImpl::initManager() along with the CommonFactoryContext(a.k.a the Server::Instance) to router/... As it's only required at Subscription Start time. - RouteEntryImplBase::per_filter_configs_
This is the place where users could insert their own per-filter-per-route config data, a FactoryContext is passed in to translate the opaque protobuf::Struct into C++ RouteSpecificFilterConfig object. In the upstream codebase, this parameter is not used in any translation so far. But there is a risk some users' translation may fail if they depends on some per-listener data during the translation, if we decide to move forward by passing a CommonFactoryContext into router/... instead of the ListenerImpl itself.