Reuse the ConfigImpl held by RdsRouteConfigProviderImpl in SRDS#18241
Reuse the ConfigImpl held by RdsRouteConfigProviderImpl in SRDS#18241yanavlasov merged 2 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
|
/assign @chaoqin-li1123 Chaoqin could you take a first pass pls? |
|
🙀 Error while processing event: |
|
/assign @chaoqin-li1123 |
source/common/router/scoped_rds.cc
Outdated
| rds_subscription.routeConfigUpdate()->protobufConfiguration(), optional_http_filters_, | ||
| factory_context_, factory_context_.messageValidationContext().dynamicValidationVisitor(), | ||
| false)); | ||
| new_rds_config); |
There was a problem hiding this comment.
Can we move the new_rds_config here?
There was a problem hiding this comment.
we can. there is little gain in move a shared_ptr.
I just found it a little bit confusing in the original way, but no strong opinion.
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
|
/assign @yanavlasov |
| envoy::config::route::v3::ScopedRouteConfiguration(iter->second->configProto()), | ||
| std::make_shared<ConfigImpl>( | ||
| rds_subscription.routeConfigUpdate()->protobufConfiguration(), optional_http_filters_, | ||
| factory_context_, factory_context_.messageValidationContext().dynamicValidationVisitor(), |
There was a problem hiding this comment.
Can you confirm the Config doesn't hold a factory_context_ from another networkfilterchain/listener?
Reuse a protobuf nessage is pretty safe but Config is risk when the config is shared among different filter chain. There were a lot of issues
There was a problem hiding this comment.
I think they all use the factory_context_ of ScopedRdsConfigSubscription.
There was a problem hiding this comment.
yeah, this rds-config-provider was fully owned by the SRDS subscription, the factory_context_ is from ScopedRdsConfigSubscription, see how RdsRouteConfigProviderHelper::initRdsConfigProvider works, which is inherited from the listener that started the srds subscription.
the life-cycle of these objects are:
{SRDS-subscription { RDS-subscription, RDS-CofigProvider } }
so it should be fine.
There was a problem hiding this comment.
LGTM. Thank you for hte explanation!
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
…yproxy#18241) Signed-off-by: Xin Zhuang <stevenzzz@google.com>
4 participants
Signed-off-by: Xin Zhuang stevenzzz@google.com
See details in #18240
Commit Message: Reuse the ConfigImpl from RDS config provider in SRDS.
Additional Description:
Risk Level: LOW, refactor
Testing: reuse existing tests.
Docs Changes:
Release Notes: