dynamic_modules: guard http filter route cache and custom flag callbacks#45585
Conversation
Return early from the clear route cache and add custom flag callbacks when decoder callbacks are absent, and skip clearing when the downstream callbacks optional is empty, matching the guarded sibling callbacks. Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
| envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr) { | ||
| auto filter = static_cast<DynamicModuleHttpFilter*>(filter_envoy_ptr); | ||
| filter->decoder_callbacks_->downstreamCallbacks()->clearRouteCache(); | ||
| if (filter->decoder_callbacks_ == nullptr) { |
There was a problem hiding this comment.
I'm actually trying to fix the issue below.
FilterManagerCallbacks::downstreamCallbacks() defaults to an empty {} and only the downstream ConnectionManagerImpl overrides it to a real value. The async-client path returns {} and the router's UpstreamRequestFilterManagerCallbacks does not override it, so it inherits the empty default.
We also register Dynamic Module HTTP filter for the upstream chain as well. addStreamFilter() wires setDecoderFilterCallbacks, so decoder_callbacks_ is non-null there.
So when a module calls clear_route_cache while running as an upstream HTTP filter (or via the async-client chain), decoder_callbacks_ is non-null but decoder_callbacks_->downstreamCallbacks() is an empty OptRef, and the original unconditional ->clearRouteCache() dereferenced it and leads to a crash.
The has_value() check is the real fix, and it matches the existing sibling clear_route_cluster_cache which already guards both conditions.
The decoder_callbacks_ == nullptr branch specifically destroy() nulls it, and the async callout handlers guard it. But since destroy() nulls in_module_filter_ and decoder_callbacks_ together and module hooks only fire while in_module_filter_ is non-null, that branch isn't expected to trigger for clear_route_cache in practice. It's only there for parity with the sibling.
There was a problem hiding this comment.
Ah so upstream filter case - that makes sense
…cks (envoyproxy#45585) ## Description Return early from the clear route cache and add custom flag callbacks when decoder callbacks are absent, and skip clearing when the downstream callbacks optional is empty, matching the guarded sibling callbacks. --- **Commit Message:** dynamic_modules: guard http filter route cache and custom flag callbacks **Risk Level:** Low **Testing:** Added **Docs Changes:** Added **Release Notes:** N/A Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
2 participants
Description
Return early from the clear route cache and add custom flag callbacks when decoder callbacks are absent, and skip clearing when the downstream callbacks optional is empty, matching the guarded sibling callbacks.
Commit Message: dynamic_modules: guard http filter route cache and custom flag callbacks
Risk Level: Low
Testing: Added
Docs Changes: Added
Release Notes: N/A