Skip to content

rds: make RouteConfigProvider shared among ListenerImpls, or move factory_context_ outside of Listener. #8303

@stevenzzzz

Description

@stevenzzzz

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    design proposalNeeds design doc/proposal before implementationstalestalebot believes this issue/PR has not been touched recently

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions