http: add server factory only factory support to cache_v2#46219
Conversation
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/46219/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
Signed-off-by: wbpcode <wbphub@gmail.com>
9d01e7f to
8d27e23
Compare
| // Pass the server factory context to allow HttpCache to use async client, stats scope | ||
| // etc. | ||
| virtual absl::StatusOr<std::shared_ptr<CacheSessions>> | ||
| getCache(const envoy::extensions::filters::http::cache_v2::v3::CacheV2Config& config, | ||
| Server::Configuration::FactoryContext& context) PURE; | ||
| Server::Configuration::ServerFactoryContext& context) PURE; |
There was a problem hiding this comment.
This also may break forks but it's should be fine because:
- Cache v2 still in WIP status.
- All existing getCache implementation will return a singleton which will be shared across listeners, it would be better to use server factory context here.
| @@ -64,7 +64,7 @@ class CacheSingleton : public Envoy::Singleton::Instance { | |||
| async_file_manager_factory_->getAsyncFileManager(config.manager_config()); | |||
| std::unique_ptr<FileSystemHttpCache> fs_cache = std::make_unique<FileSystemHttpCache>( | |||
| singleton, cache_eviction_thread_, std::move(config), std::move(async_file_manager), | |||
| context.serverFactoryContext().scope()); | |||
| context.scope()); | |||
There was a problem hiding this comment.
This actually fixed a potential lifetime bug. For example, the listener A's cache filter create the CacheSessions first with it's scope. Then listener B get the cached CacheSessions because same configuration. Then the listener A is removed. Then the CacheSessions may refer to invalid stats.
2 participants
Commit Message: http: add server factory only factory support to cache_v2
Additional Description:
This adds the server-factory-context-only filter factory creation method
(
createHttpFilterFactoryFromProtoTypedtaking only aServerFactoryContext,introduced in #45989) to the cache_v2 HTTP filter(s).
This allows these filters to be created in contexts where only a
ServerFactoryContextis available (e.g. route/virtual-host level filterconfiguration) rather than requiring the full downstream
FactoryContext.The existing downstream
FactoryContextpath is preserved and now delegatesto a shared helper, so behavior for existing use cases is unchanged.
Generative AI was used to assist with this change. I have reviewed all changes also.
Risk Level: Low
Testing: Existing/added unit config tests; CI.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A