Allow XdsFilter to handle unknown Message config#6710
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced parsed per-filter configs with raw protobuf Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.java (2)
62-64: Parameter name should match interface contract.The interface defines the parameter as
httpFilterbut the implementation usesfilter. Aligning the names improves consistency and IDE navigation.♻️ Suggested fix
`@Override` - public XdsHttpFilter create(HttpFilter filter, Any config) { + public XdsHttpFilter create(HttpFilter httpFilter, Any config) { return routerFilter; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.java` around lines 62 - 64, The create method in RouterFilterFactory currently declares its first parameter as "filter" but the interface contract uses "httpFilter"; rename the parameter in the method signature to "httpFilter" to match the interface (method: create(HttpFilter httpFilter, Any config) in class RouterFilterFactory) so IDEs and callers resolve symbols consistently while leaving the body (returning routerFilter) unchanged.
44-54: Consider makingrouterFiltera static field.Since the anonymous
XdsHttpFilteronly references the statichttpFilterandrpcFilterfields and has no instance state, it could be declaredstaticfor consistency with the other static fields and to avoid creating a new instance per factory.♻️ Suggested refactor
- private final XdsHttpFilter routerFilter = new XdsHttpFilter() { + private static final XdsHttpFilter ROUTER_FILTER = new XdsHttpFilter() { `@Override` public HttpPreprocessor httpPreprocessor() { return httpFilter::execute; } `@Override` public RpcPreprocessor rpcPreprocessor() { return rpcFilter::execute; } };Then update the
createmethod:`@Override` public XdsHttpFilter create(HttpFilter filter, Any config) { - return routerFilter; + return ROUTER_FILTER; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.java` around lines 44 - 54, The anonymous XdsHttpFilter assigned to routerFilter is instance-specific but only references the static httpFilter and rpcFilter and has no instance state; make routerFilter a static field (declare routerFilter as static) and adjust any usages in the create method to reference the now-static RouterFilterFactory.routerFilter symbol as needed (ensure httpPreprocessor() and rpcPreprocessor() still return httpFilter::execute and rpcFilter::execute respectively).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.java`:
- Around line 62-64: The create method in RouterFilterFactory currently declares
its first parameter as "filter" but the interface contract uses "httpFilter";
rename the parameter in the method signature to "httpFilter" to match the
interface (method: create(HttpFilter httpFilter, Any config) in class
RouterFilterFactory) so IDEs and callers resolve symbols consistently while
leaving the body (returning routerFilter) unchanged.
- Around line 44-54: The anonymous XdsHttpFilter assigned to routerFilter is
instance-specific but only references the static httpFilter and rpcFilter and
has no instance state; make routerFilter a static field (declare routerFilter as
static) and adjust any usages in the create method to reference the now-static
RouterFilterFactory.routerFilter symbol as needed (ensure httpPreprocessor() and
rpcPreprocessor() still return httpFilter::execute and rpcFilter::execute
respectively).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87b17a2a-2092-429a-8ba1-b8187d05fea8
📒 Files selected for processing (12)
xds/src/main/java/com/linecorp/armeria/xds/FilterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerStream.javaxds/src/main/java/com/linecorp/armeria/xds/ParsedFilterConfig.javaxds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaxds/src/main/java/com/linecorp/armeria/xds/RouteSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/RouteStream.javaxds/src/main/java/com/linecorp/armeria/xds/VirtualHostSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/filter/XdsHttpFilter.java
💤 Files with no reviewable changes (2)
- xds/src/main/java/com/linecorp/armeria/xds/VirtualHostSnapshot.java
- xds/src/main/java/com/linecorp/armeria/xds/ParsedFilterConfig.java
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6710 +/- ##
============================================
- Coverage 74.46% 74.00% -0.46%
- Complexity 22234 24099 +1865
============================================
Files 1963 2175 +212
Lines 82437 90427 +7990
Branches 10764 11850 +1086
============================================
+ Hits 61385 66924 +5539
- Misses 15918 17888 +1970
- Partials 5134 5615 +481 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
jrhee17
changed the title
XdsFilter by handling unknown configurationXdsFilter to handle unknown Message config
ikhoon
approved these changes
Apr 8, 2026
| if (!httpFilter.getIsOptional()) { | ||
| throw new IllegalArgumentException( | ||
| "Unknown HTTP filter '" + httpFilter.getName() + | ||
| "': no HttpFilterFactory registered. Register an SPI " + |
Contributor
There was a problem hiding this comment.
Question) I was wondering if it is technically possible to register a HttpFilterFactory via a builder API?
Contributor
Author
There was a problem hiding this comment.
I don't think it is difficult to do this, and I agree it will be useful for users.
I've simply deferred exposing builder methods to XdsBootstrapBuilder until the overall implementation is stable.
minwoox
approved these changes
Apr 8, 2026
| * Returning {@code null} from {@link #create} causes the filter to be silently skipped. | ||
| */ | ||
| @UnstableApi | ||
| public interface HttpFilterFactory<T extends Message> { |
Contributor
There was a problem hiding this comment.
Should this be mentioned in the breaking change section?
Contributor
Author
There was a problem hiding this comment.
Added the label and also added a description of the change to the PR description.
jrhee17
added a commit
to jrhee17/armeria
that referenced
this pull request
Apr 13, 2026
Subset of line#6700 Motivation `HttpFilterFactory<T extends Message>` forced each factory implementation to declare a specific protobuf `Message` type for its config, and required the framework to own config parsing and `FilterConfig` envelope unwrapping. This made it difficult to handle unknown or dynamically-typed xDS filter configs (e.g. Istio-specific filters), and leaked framework concerns into the factory API. Additionally, `ParsedFilterConfig` wrapped per-route filter configs and required the framework to merge and propagate them through the snapshot hierarchy (`RouteSnapshot` → `VirtualHostSnapshot` → `RouteEntry`), coupling snapshot construction to filter logic. Modifications - Replace `HttpFilterFactory<T extends Message>` with an untyped `HttpFilterFactory` interface; `create(HttpFilter, Any)` now receives the raw `Any` config directly, and factories are responsible for all parsing including `FilterConfig` envelope unwrapping. Returning `null` skips the filter. - Introduce `XdsHttpFilter` as the return type of `create()`, with default no-op `httpPreprocessor()`, `rpcPreprocessor()`, `httpDecorator()`, and `rpcDecorator()` methods. - Delete `ParsedFilterConfig`; per-route filter configs are now stored and merged as `Map<String, Any>` directly. - Move 3-level `typed_per_filter_config` merging (route-config → vhost → route) into `RouteEntry`'s constructor, eliminating `withFilterConfigs()` from `VirtualHostSnapshot` and `withRouter()` from `RouteSnapshot`. - Thread `@Nullable ListenerXdsResource` through `RouteStream` inner classes so `RouteEntry` can access upstream HTTP filters from the Router directly. - Update `RouterFilterFactory` to implement the new untyped interface. - Simplify `FilterUtil`: remove `toParsedFilterConfigs()`, update `mergeFilterConfigs` to operate on `Map<String, Any>`, `buildUpstreamFilter` takes `@Nullable RetryPolicy` directly. Result - `HttpFilterFactory` implementations no longer need to declare a protobuf type parameter, and can handle any xDS filter config including unknown or Istio-specific ones. - Per-route filter config merging is self-contained in `RouteEntry` rather than spread across the snapshot hierarchy. - `RouteEntry.filterConfig(String)` now returns `@Nullable Any` instead of `@Nullable ParsedFilterConfig`. Breaking - `HttpFilterFactory` implementations must now implement `XdsHttpFilter create(HttpFilter httpFilter, Any config)` for custom filter implementations
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Subset of #6700
Motivation
HttpFilterFactory<T extends Message>forced each factory implementation to declare a specific protobufMessagetype for its config, and required the framework to own config parsing andFilterConfigenvelope unwrapping. This made it difficult to handle unknown or dynamically-typed xDS filter configs (e.g. Istio-specific filters), and leaked framework concerns into the factory API.Additionally,
ParsedFilterConfigwrapped per-route filter configs and required the framework to merge and propagate them through the snapshot hierarchy (RouteSnapshot→VirtualHostSnapshot→RouteEntry), coupling snapshot construction to filter logic.Modifications
HttpFilterFactory<T extends Message>with an untypedHttpFilterFactoryinterface;create(HttpFilter, Any)now receives the rawAnyconfig directly, and factories are responsible for all parsing includingFilterConfigenvelope unwrapping. Returningnullskips the filter.XdsHttpFilteras the return type ofcreate(), with default no-ophttpPreprocessor(),rpcPreprocessor(),httpDecorator(), andrpcDecorator()methods.ParsedFilterConfig; per-route filter configs are now stored and merged asMap<String, Any>directly.typed_per_filter_configmerging (route-config → vhost → route) intoRouteEntry's constructor, eliminatingwithFilterConfigs()fromVirtualHostSnapshotandwithRouter()fromRouteSnapshot.@Nullable ListenerXdsResourcethroughRouteStreaminner classes soRouteEntrycan access upstream HTTP filters from the Router directly.RouterFilterFactoryto implement the new untyped interface.FilterUtil: removetoParsedFilterConfigs(), updatemergeFilterConfigsto operate onMap<String, Any>,buildUpstreamFiltertakes@Nullable RetryPolicydirectly.Result
HttpFilterFactoryimplementations no longer need to declare a protobuf type parameter, and can handle any xDS filter config including unknown or Istio-specific ones.RouteEntryrather than spread across the snapshot hierarchy.RouteEntry.filterConfig(String)now returns@Nullable Anyinstead of@Nullable ParsedFilterConfig.Breaking
HttpFilterFactoryimplementations must now implementXdsHttpFilter create(HttpFilter httpFilter, Any config)for custom filter implementations