xds: add support for HTTP filter state retention, as specified in A83#8924
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to support stateful HTTP filters, as outlined in gRFC A83. The core change is the introduction of a Builder and Filter pattern, which separates filter configuration and instantiation from interceptor creation. This enables the caching and reuse of filter instances across configuration updates, which is crucial for filters that need to maintain state.
The changes are consistently applied across both client-side and server-side filter handling. The client resolver and server listener wrapper now manage the lifecycle of filter instances, including caching and cleanup, which prevents resource leaks. The addition of new end-to-end tests to verify this state retention behavior is a great addition.
Overall, the implementation appears solid and aligns well with the design goals. I have one minor suggestion for improving maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements HTTP filter state retention as specified in gRFC A83. It introduces a new Builder interface and separates the filter instance lifecycle from the interceptor lifecycle, allowing filters to maintain state across configuration updates. The changes correctly implement reference counting for server-side filters which can be shared across multiple filter chains. My review identified a few issues related to resource cleanup and potential memory leaks in the server-side implementation, specifically regarding the management of the filter instance map and the stopping of pending configuration managers.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8924 +/- ##
==========================================
- Coverage 83.13% 83.07% -0.06%
==========================================
Files 411 411
Lines 32704 32892 +188
==========================================
+ Hits 27187 27326 +139
- Misses 4140 4179 +39
- Partials 1377 1387 +10
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to support stateful HTTP filters, aligning with gRFC A83. The core of the change is the new set of httpfilter interfaces (Builder, ClientFilter, ServerFilter) which separate filter configuration parsing from interceptor creation and state management. This is a solid design that enables filter state retention across resource updates.
The changes are consistently applied across the client-side resolver and server-side listener logic, with corresponding updates to all existing filter implementations. The addition of cleanup functions to the new interfaces is crucial for proper resource management of stateful filters.
I'm particularly impressed by the new end-to-end tests (TestXDSResolverHTTPFilters_FilterInstanceRetained and the new test/xds/xds_server_filter_state_retention_test.go file) which thoroughly validate the new state retention and lifecycle management behavior. These tests provide strong confidence in the correctness of this complex change.
I have one suggestion for a minor performance improvement in the server-side filter cleanup logic. Overall, this is an excellent contribution.
…computed map of new filters for lookup
…selector is being built
|
Thanks for the review again and apologies for the delay in getting back. I think I've addressed all your comments. PTAL. |
|
|
||
| // IsRouterFilter returns true iff a HTTP filter is a Router filter. | ||
| func IsRouterFilter(b httpfilter.Filter) bool { | ||
| func IsRouterFilter(b httpfilter.Builder) bool { |
There was a problem hiding this comment.
nit: The godoc needs to be updated to mention filter builders instead of filters.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the HTTP filter API to support stateful filters, as specified in gRFC A83. The core change from a stateless Filter interface to a stateful Builder/Filter pattern is implemented consistently for both client and server sides. The new design allows filter instances to be retained across configuration updates, enabling state preservation. The introduction of Close() methods on filters and interceptors, along with reference counting for server-side filters, ensures proper resource lifecycle management. The changes are accompanied by thorough tests, including new end-to-end tests that validate the state retention and cleanup logic. The implementation appears robust and correct.
arjan-bal
left a comment
There was a problem hiding this comment.
A couple of minor comments, otherwise LGTM!
…grpc#8924) #### How the API looked before this change? - We had a single interface, `Filter`, that was responsible for functionality like config parsing and optionally for building client and server interceptors (or filter instances). - We had a registry of `Filter`s keyed by the supported type_urls. #### How the API looks after this change? - We will have an interface, `Builder`, that contains functionality for config parsing and other things like "what are the supported type_urls?", "Are filters produced by this builder supposed to be terminal?", - Two optional interfaces that the `Builder` can implement that provide the functionality to create filter instances for the client and server, `ClientFilterBuilder` and `ServerFilterBuilder` respectively. - The `ClientFilterBuilder` and `ServerFilterBuilder` create `ClientFilter` and `ServerFilter` respectively. - The `ClientFilter` and `ServerFilter` interface contains functionality to build client and server interceptors. - The methods to create filter instances and interceptors will return cleanup functions that callers need to invoke when the filter/interceptor is no longer required. - We will have a registry of `Builders`s keyed by the supported type_urls. #### Why is this change required? - As part of gRFC A83 and moving forward on a bunch of other gRFCs, we will start having HTTP filters that need to maintain a bunch of state, and this state needs to be retained across resource updates. For example, the filter might contain a gRPC channel to an external service, and we don't want to recreate this channel for resource updates that doesn't change any properties associated with the service that is being connected to. #### How will this be used? - The xDS name resolver and the xDS enabled grpc server (entities that create HTTP filters on the client and server side) will create new `Filter` instances only when the filter names in the xDS resources change. Otherwise, they will only create new interceptors using the updated filter configuration from the existing `Filter`. This will allow `Filter` instances to share state across interceptors and across state updates. The changes here and inspired by similar changes made for Java and described here: https://github.com/grpc/proposal/blob/master/A83-xds-gcp-authn-filter.md#java RELEASE NOTES: None
How the API looked before this change?
Filter, that was responsible for functionality like config parsing and optionally for building client and server interceptors (or filter instances).Filters keyed by the supported type_urls.How the API looks after this change?
Builder, that contains functionality for config parsing and other things like "what are the supported type_urls?", "Are filters produced by this builder supposed to be terminal?",Buildercan implement that provide the functionality to create filter instances for the client and server,ClientFilterBuilderandServerFilterBuilderrespectively.ClientFilterBuilderandServerFilterBuildercreateClientFilterandServerFilterrespectively.ClientFilterandServerFilterinterface contains functionality to build client and server interceptors.Builderss keyed by the supported type_urls.Why is this change required?
How will this be used?
Filterinstances only when the filter names in the xDS resources change. Otherwise, they will only create new interceptors using the updated filter configuration from the existingFilter. This will allowFilterinstances to share state across interceptors and across state updates.The changes here and inspired by similar changes made for Java and described here: https://github.com/grpc/proposal/blob/master/A83-xds-gcp-authn-filter.md#java
RELEASE NOTES: None