ecds: use last received filter config from a subscription #15371
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,10 +14,10 @@ namespace Filter { | |
| namespace Http { | ||
|
|
||
| DynamicFilterConfigProviderImpl::DynamicFilterConfigProviderImpl( | ||
| FilterConfigSubscriptionSharedPtr& subscription, | ||
| const absl::flat_hash_set<std::string>& require_type_urls, | ||
| Server::Configuration::FactoryContext& factory_context) | ||
| : subscription_(subscription), require_type_urls_(require_type_urls), | ||
| tls_(factory_context.threadLocal()), | ||
| init_target_("DynamicFilterConfigProviderImpl", [this]() { | ||
| subscription_->start(); | ||
|
|
@@ -34,21 +34,19 @@ DynamicFilterConfigProviderImpl::~DynamicFilterConfigProviderImpl() { | |
| subscription_->filter_config_providers_.erase(this); | ||
| } | ||
|
|
||
| void DynamicFilterConfigProviderImpl::validateTypeUrl(const std::string& type_url) const { | ||
| if (!require_type_urls_.contains(type_url)) { | ||
| throw EnvoyException(fmt::format("Error: filter config has type URL {} but expect {}.", | ||
| type_url, absl::StrJoin(require_type_urls_, ", "))); | ||
| } | ||
| } | ||
|
|
||
| const std::string& DynamicFilterConfigProviderImpl::name() { return subscription_->name(); } | ||
|
|
||
| absl::optional<Envoy::Http::FilterFactoryCb> DynamicFilterConfigProviderImpl::config() { | ||
| return tls_->config_; | ||
| } | ||
|
|
||
| void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryCb config, | ||
| const std::string&, | ||
| Config::ConfigAppliedCb cb) { | ||
|
|
@@ -124,8 +122,9 @@ void FilterConfigSubscription::onConfigUpdate( | |
| // Ensure that the filter config is valid in the filter chain context once the proto is processed. | ||
| // Validation happens before updating to prevent a partial update application. It might be | ||
| // possible that the providers have distinct type URL constraints. | ||
| const auto type_url = Config::Utility::getFactoryType(filter_config.typed_config()); | ||
| for (auto* provider : filter_config_providers_) { | ||
| provider->validateTypeUrl(type_url); | ||
| } | ||
| ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( | ||
| filter_config.typed_config(), validator_, factory); | ||
|
|
@@ -142,6 +141,9 @@ void FilterConfigSubscription::onConfigUpdate( | |
| }); | ||
| } | ||
| last_config_hash_ = new_hash; | ||
| last_config_ = factory_callback; | ||
| last_type_url_ = type_url; | ||
| last_version_info_ = version_info; | ||
| } | ||
|
|
||
| void FilterConfigSubscription::onConfigUpdate( | ||
|
|
@@ -173,6 +175,8 @@ FilterConfigSubscription::~FilterConfigSubscription() { | |
| filter_config_provider_manager_.subscriptions_.erase(subscription_id_); | ||
| } | ||
|
|
||
| void FilterConfigSubscription::incrementConflictCounter() { stats_.config_conflict_.inc(); } | ||
|
|
||
| std::shared_ptr<FilterConfigSubscription> FilterConfigProviderManagerImpl::getSubscription( | ||
| const envoy::config::core::v3::ConfigSource& config_source, const std::string& name, | ||
| Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) { | ||
|
|
@@ -196,24 +200,69 @@ std::shared_ptr<FilterConfigSubscription> FilterConfigProviderManagerImpl::getSu | |
| } | ||
|
|
||
| FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConfigProvider( | ||
| const envoy::config::core::v3::ExtensionConfigSource& config_source, | ||
| const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, | ||
| const std::string& stat_prefix) { | ||
| auto subscription = getSubscription(config_source.config_source(), filter_config_name, | ||
| factory_context, stat_prefix); | ||
| // For warming, wait until the subscription receives the first response to indicate readiness. | ||
| // Otherwise, mark ready immediately and start the subscription on initialization. A default | ||
| // config is expected in the latter case. | ||
| if (!config_source.apply_default_config_without_warming()) { | ||
| factory_context.initManager().add(subscription->initTarget()); | ||
| } | ||
| absl::flat_hash_set<std::string> require_type_urls; | ||
| for (const auto& type_url : config_source.type_urls()) { | ||
| auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); | ||
| require_type_urls.emplace(factory_type_url); | ||
| } | ||
| auto provider = std::make_unique<DynamicFilterConfigProviderImpl>(subscription, require_type_urls, | ||
| factory_context); | ||
| // Ensure the subscription starts if it has not already. | ||
| if (config_source.apply_default_config_without_warming()) { | ||
| factory_context.initManager().add(provider->init_target_); | ||
| } | ||
|
|
||
| // If the subscription already received a config, attempt to apply it. | ||
| // It is possible that the received extension config fails to satisfy the listener | ||
| // type URL constraints. This may happen if ECDS and LDS updates are racing, and the LDS | ||
| // update arrives first. In this case, use the default config, increment a metric, | ||
| // and the applied config eventually converges once ECDS update arrives. | ||
| bool last_config_valid = false; | ||
| if (subscription->lastConfig().has_value()) { | ||
| try { | ||
| provider->validateTypeUrl(subscription->lastTypeUrl()); | ||
| last_config_valid = true; | ||
| } catch (const EnvoyException& e) { | ||
| ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.", | ||
|
Member
There was a problem hiding this comment. Can you express this in a way that leads to actionable outcome, i.e. what is the likely cause of this error? Also, should we have some stat bumped? Logs will get lost in production settings.
Member
There was a problem hiding this comment. Thinking through this more broadly, I kind of find this corner case a bit icky. This relates to my validate config comment above; if all we can do is validate configs, can we pull in the type URL validation into the core here?
Contributor
Author
There was a problem hiding this comment. Yeah, I'll take it away from base interfaces. The likely cause is pushing LDS and ECDS together and LDS update arriving first. Do you have any recommendation for a metric?
Member
There was a problem hiding this comment. Are you making the argument here that since the last config has already been validated and is known to work, the only possible way it could fail here is if the type URLs mismatch somehow? Do we even need In terms of stats, probably something in https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/stats#config-http-conn-man-stats namespace makes sense.
Contributor
Author
There was a problem hiding this comment. @htuch I'm looking for guidance how to deal with inconsistency between ECDS and LDS. The scenario is the following:
I think this is similar to a delayed NACK situation since we want the management server to push a new ECDS update for x with type URL T2, that validates both L1 and L2. This PR will make L2 to use the default config, but it does not indicate to the management server than a new ECDS update is needed. Granted, this is a fairly convoluted example. The metric would be useful, but is the assumption here, that some higher level controller (human?) is going to look for it and force push ECDS?
Contributor
Author
There was a problem hiding this comment. Added "config_conflict" stat for ECDS resource. I think keying by ECDS is a bit more actionable, since it is clear why resource is the conflict (LDS tends to be more source-of-truth). LMK.
Member
There was a problem hiding this comment. In your example, is "x" actually different in step 4? I.e. it has been deliberately changed by the management server and when server in hypothetical step 4, it's really just a problem of 3 and 4 racing? Or is "x" somehow polymorphic and could be T1 or T2 and it gets decided based on client state? This seems a pretty weird situation but I'm likely missing something. Feel free to ping me IM so we can figure this out with lower RTT. |
||
| filter_config_name, e.what()); | ||
| subscription->incrementConflictCounter(); | ||
| } | ||
| if (last_config_valid) { | ||
| provider->onConfigUpdate(subscription->lastConfig().value(), subscription->lastVersionInfo(), | ||
| nullptr); | ||
| } | ||
| } | ||
|
|
||
| // Apply the default config if none has been applied. | ||
| if (config_source.has_default_config() && !last_config_valid) { | ||
| auto* default_factory = | ||
| Config::Utility::getFactoryByType<Server::Configuration::NamedHttpFilterConfigFactory>( | ||
| config_source.default_config()); | ||
| if (default_factory == nullptr) { | ||
| throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter " | ||
| "configuration with type URL {}.", | ||
| filter_config_name, | ||
| config_source.default_config().type_url())); | ||
| } | ||
| provider->validateTypeUrl(Config::Utility::getFactoryType(config_source.default_config())); | ||
| ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( | ||
| config_source.default_config(), factory_context.messageValidationVisitor(), | ||
| *default_factory); | ||
| Envoy::Http::FilterFactoryCb default_config = | ||
| default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); | ||
| provider->onConfigUpdate(default_config, "", nullptr); | ||
| } | ||
| return provider; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a value in moving this to returning true/false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it, but the exception contains the list of required type URLs, which is useful. Bool would be less actionable.