-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic filter config provider provides empty config when listener updates #14934
Comments
@kyessenov thoughts here? This seems to be ECDS warming behavior. |
@kyessenov is on leave this month. Not sure if there is someone else who is familiar with ecds code path. |
I can take a shot at this. I think I need to understand in your example which listener and ECDS subscriptions are involved. It's a bit confusing, e.g. are the listeners sharing a filter chain with ECDS subscription to the same resource? Are the listeners pointing at different ECDS resources? I think in the PR fix this is not quite the right thing, as it's a work-around to maybe a more fundamental issue around how listeners and ECDS warming is happening. |
@htuch @bianpengyuan
|
@htuch The old and new listeners share a ECDS subscription. This is how I understand the object lifecycle: it starts with Then a new listener Then a ECDS update come in, which is the same as the last one received by the subscription based on hash comparison, thus the subscription thinks that all known providers should be up-to-dated and skip updating any providers. However,
@lambdai I am not sure what is the expected behavior, should the new provider skip waiting and just use available configuration that subscription has, or wait for an ECDS update regardless. |
This is exactly what I am referring to. Why does the |
Agree with @lambdai here. If we're talking about an ECDS subscription that has already delivered configuration previously, the new listener shouldn't have to warm on this. I'm not super familiar with the code here, but I would expect it to be the responsibility of the dynamic filter config provider system to handle this. The analogy is with route config providers. When two listeners share a route config provider, |
Agree with @htuch. I think we can possibly solve this by immediately applying warmed subscription in the provider. The interesting case is mis-matched config provider: what if filter config provider type_urls disallows the one from the subscription? I think in this case, we should warm because there is a possibility that the new config (different by necessity) may unblock the listener. |
Is it possible to store the shared latest config in the subscription? When a new config arrives, always update the latest config. Meanwhile, iterate over the providers to give each provider a chance to adopt. |
@lambdai Yeah, although I think it's better to avoid storing protobuf and just carry the factory callback. |
I am using ECDS to provide dynamic extension configuration. When listener updates, dynamic filter config provider will return empty config for the new listener:
envoy/source/common/filter/http/filter_config_discovery_impl.cc
Line 40 in 4e1146b
I think what happens is that when listener updates, old listener takes time to drain, ECDS subscription is not destroyed when new listener is constructed, so it is reused by the new listener:
envoy/source/common/filter/http/filter_config_discovery_impl.cc
Line 184 in 4e1146b
But a new dynamic filter config provider is created with the new listener and will be warming and wait for any subscription update:
envoy/source/extensions/filters/network/http_connection_manager/config.cc
Line 552 in 4e1146b
However, when subscription receives new update, if config does not have any change from last update, it will skip updating provider. Listener will be considered as warmed, and provider will stuck with empty config until next ECDS update with diff.
envoy/source/common/filter/http/filter_config_discovery_impl.cc
Line 118 in 4e1146b
The text was updated successfully, but these errors were encountered: