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

[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.

[lambdai]

Thank you for writing the summary!

WRT InitManager:
It's arguable if the current usage of InitManager is in a good shape. I am seeing

1. undefined behavior: Adding target to initialized InitManager (@ vhds)
2. not adding target to another init manager (@ reusing subscription)
   I think we need SharedTarget(see my prototype here), also it's probably not ok to use listener's init manager everywhere.

WRT PerFilterConfigs
It's not allowed to use listener specific config if RDS is shared among listeners. As currently the code base is not utilizing listener specific config we should remove it ASAP to avoid people not aware of the design change.

Below is a list each RDS related FactoryContext usage. It may contain half truth.

vhds subscription
scope() clusterManager() initManager() at ctor

staticrouteconfigproviderimpl
timesource()

RdsRouteConfigSubscription
scope() messageValidationVisitor() clusterManager() timeSource()

RdsRouteConfigProviderImpl
threadLocal()

ConfigImpl
scope()

RouteEntryImplBase
runtime() messageValidationVisitor() api() Dispatcher().Timesource()

WeightedClusterEntry

PerFilterConfig
runtime()

VirtualHostImpl
scope() runtime() cm()

RouteMatcher

[stevenzzzz]

Thanks for the list, I think except for the per_filter_configs_ in routeEntry, everything else is simply delegated to Server::Instance from a ListenerImpl.

…

On Fri, Sep 20, 2019 at 1:24 PM Yuchen Dai ***@***.***> wrote: Thank you for writing the summary! WRT InitManager: It's arguable if the current usage of InitManager is in a good shape. I am seeing 1. undefined behavior: Adding target to initialized InitManager (@ vhds) 2. not adding target to another init manager (@ reusing subscription) I think we need SharedTarget(see my prototype here <lambdai/envoy@1da5cbe>), also it's probably not ok to use listener's init manager everywhere. WRT PerFilterConfigs It's not allowed to use listener specific config if RDS is shared among listeners. As currently the code base is not utilizing listener specific config we should remove it ASAP to avoid people not aware of the design change. Below is a list each RDS related FactoryContext usage. It may contain half truth. vhds subscription scope() clusterManager() initManager() at ctor staticrouteconfigproviderimpl timesource() RdsRouteConfigSubscription scope() messageValidationVisitor() clusterManager() timeSource() RdsRouteConfigProviderImpl threadLocal() ConfigImpl scope() RouteEntryImplBase runtime() messageValidationVisitor() api() Dispatcher().Timesource() WeightedClusterEntry PerFilterConfig runtime() VirtualHostImpl scope() runtime() cm() RouteMatcher — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8303?email_source=notifications&email_token=AA2I6TD6GTNTNCPKGQNAFSLQKUBMXA5CNFSM4IYR5PAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7HLQVQ#issuecomment-533641302>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AA2I6TBFBJ3R5UUS3SXZ4J3QKUBMXANCNFSM4IYR5PAA> .

-- Xin Zhuang

[mattklein123]

@stevenzzzz @lambdai I like the idea of splitting the context into effectively a server/global context and a listener context. Can we do that first?

[lambdai]

My current wip on rds dedup is using the impl of ListenerContext but leaving the listener specific stuff unimplemented.
So ugly that I believe no one would approve.

Once I get the PR close to workable, I can propose the granular of ServerContext.
The idea is that server context should be naturally compatible with RDS dedup impl, unless we have a strong reason not to

[stevenzzzz]

@alyssawilk @htuch FYI

[lambdai]

@htuch This design is half done:
#8485 enables #8523
The latter one implements the shared rds config

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.