xds: refactor server-side HTTP filter processing by easwars · Pull Request #8878 · grpc/grpc-go

easwars · 2026-02-03T20:53:21Z

While working on xDS HTTP filter state retention changes required for A83 and A86 in #8745, I realized that the current implementation of HTTP filters on the server-side does not lend itself well to the changes we want to make to support filter state retention. The crux of the problem in the current implementation is as follows:

The xdsresource package builds a FilterChainManager type that does the following:
- Keeps track of the list of Route Configuration resources to be requested based on the HTTP filters configured on the Listener resource
- Updates the interceptors on the different filter chains based on Route Configuration resource updates (that contain filter config overrides)
- Maintaining runtime state in a type that is supposed to represent a configuration blob seems to have been a wrong implementation choice
Everytime a Listener resource update is received, the xdsresource package builds a brand new FilterChainManager
- This means that we cannot retain filter state across resource updates

This PR makes the following changes:

Changes the layout of the ListenerUpdate struct to clearly separate client-side and server-side fields
- Also groups information that is shared across client and server listener types into a logical struct
- The ListenerUpdate will only contain information directly sourced from the listener proto
The xDS server implementation will build the runtime representation of the filter chains using the configuration update provided by the xdsresource package
- This would move the responsibility of updating the interceptors on the filter chain to the xDS server, which is the correct place to be doing such a thing
- This would make it possible for the xDS server implementation to retain filter state across listener resource updates

RELEASE NOTES: none

codecov · 2026-02-03T20:57:13Z

Codecov Report

❌ Patch coverage is 92.40246% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.18%. Comparing base (0f69b6e) to head (408f554).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/server/filter_chain_manager.go	90.90%	7 Missing and 7 partials ⚠️
internal/xds/server/routing.go	85.18%	7 Missing and 5 partials ⚠️
...nternal/xds/xdsclient/xdsresource/unmarshal_lds.go	97.84%	3 Missing and 1 partial ⚠️
internal/xds/xdsdepmgr/xds_dependency_manager.go	57.14%	2 Missing and 1 partial ⚠️
...ds/xdsclient/xdsresource/listener_resource_type.go	85.71%	1 Missing and 1 partial ⚠️
internal/xds/resolver/xds_resolver.go	66.66%	1 Missing ⚠️
internal/xds/server/listener_wrapper.go	96.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8878      +/-   ##
==========================================
- Coverage   83.31%   83.18%   -0.13%     
==========================================
  Files         414      416       +2     
  Lines       32805    32872      +67     
==========================================
+ Hits        27330    27344      +14     
- Misses       4069     4100      +31     
- Partials     1406     1428      +22

Files with missing lines	Coverage Δ
internal/xds/server/conn_wrapper.go	`76.27% <100.00%> (-0.78%)`	⬇️
internal/xds/xdsclient/xdsresource/filter_chain.go	`92.45% <100.00%> (-1.47%)`	⬇️
internal/xds/xdsclient/xdsresource/matcher.go	`60.68% <ø> (-3.50%)`	⬇️
xds/server.go	`82.55% <100.00%> (+1.10%)`	⬆️
internal/xds/resolver/xds_resolver.go	`88.70% <66.66%> (ø)`
internal/xds/server/listener_wrapper.go	`79.41% <96.66%> (+0.12%)`	⬆️
...ds/xdsclient/xdsresource/listener_resource_type.go	`90.47% <85.71%> (-5.68%)`	⬇️
internal/xds/xdsdepmgr/xds_dependency_manager.go	`79.11% <57.14%> (-0.08%)`	⬇️
...nternal/xds/xdsclient/xdsresource/unmarshal_lds.go	`93.37% <97.84%> (+4.91%)`	⬆️
internal/xds/server/routing.go	`85.18% <85.18%> (ø)`
... and 1 more

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arjan-bal · 2026-02-09T14:49:45Z

/gemini review

gemini-code-assist

Code Review

This pull request is a significant and well-executed refactoring of the server-side HTTP filter processing logic. The main goal of separating configuration from runtime state is achieved by moving logic from the xdsresource package to the server package, which will make it easier to implement filter state retention in the future. The changes are extensive but logical, with clear separation of concerns in new files like filter_chain_manager.go and routing.go. The accompanying tests are thorough and have been updated to reflect the new data structures. I've identified one critical bug in the new unmarshalling logic that needs to be addressed.

easwars · 2026-02-11T06:24:49Z

/gemini review

gemini-code-assist

Code Review

This pull request is a significant and well-executed refactoring of the server-side HTTP filter processing logic. The primary goal of separating configuration from runtime state and moving server-specific logic into the server package has been achieved, which should make future work on state retention easier. The changes are consistent across the codebase, and the addition of new tests and updates to existing ones provide good coverage for the refactoring. I've found a few minor issues in the new test files related to variable naming and a typo in an error message, which I've commented on. Overall, this is a high-quality contribution that improves the codebase structure.

gemini-code-assist · 2026-02-11T06:27:42Z

+					for _, int := range r.interceptors {
+						errs = append(errs, int.AllowRPC(ctx).Error())
+					}


The loop variable int shadows the built-in int type. This can be confusing and is generally considered bad practice in Go. Renaming it to something more descriptive like interceptor would improve readability and prevent potential issues.

Suggested change

for _, int := range r.interceptors {

errs = append(errs, int.AllowRPC(ctx).Error())

}

for _, interceptor := range r.interceptors {

err_s = append(errs, interceptor.AllowRPC(ctx).Error())

}

gemini-code-assist · 2026-02-11T06:27:42Z

+func cidrRangeFromAddressAndPrefixLen(address string, len int) *v3corepb.CidrRange {
+	return &v3corepb.CidrRange{
+		AddressPrefix: address,
+		PrefixLen: &wrapperspb.UInt32Value{
+			Value: uint32(len),
+		},
+	}


The function parameter len shadows the built-in len function. This is confusing and can lead to subtle bugs if the built-in function is needed within this scope. It's best to use a more descriptive name like prefixLen.

Suggested change

func cidrRangeFromAddressAndPrefixLen(address string, len int) *v3corepb.CidrRange {

return &v3corepb.CidrRange{

AddressPrefix: address,

PrefixLen: &wrapperspb.UInt32Value{

Value: uint32(len),

},

}

func cidrRangeFromAddressAndPrefixLen(address string, prefixLen int) *v3corepb.CidrRange {

return &v3corepb.CidrRange{

AddressPrefix: address,

PrefixLen: &wrapperspb.UInt32Value{

Value: uint32(prefixLen),

},

}

}

gemini-code-assist · 2026-02-11T06:27:42Z

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := findBestMatchingVirtualHostServer(tt.host, tt.vHosts); !cmp.Equal(got, tt.want, cmp.AllowUnexported(virtualHostWithInterceptors{}, routeWithInterceptors{})) {
+				t.Errorf("FindBestMatchingxdsclient.virtualHostWithInterceptors() = %v, want %v", got, tt.want)


There appears to be a typo in the error message. It seems like FindBestMatchingxdsclient.virtualHostWithInterceptors() should be findBestMatchingVirtualHostServer() to match the function being tested. Correcting this will make test failures easier to debug.

Suggested change

t.Errorf("FindBestMatchingxdsclient.virtualHostWithInterceptors() = %v, want %v", got, tt.want)

t.Errorf("findBestMatchingVirtualHostServer() = %v, want %v", got, tt.want)

arjan-bal

LGTM

While working on xDS HTTP filter state retention changes required for A83 and A86 in grpc#8745, I realized that the current implementation of HTTP filters on the server-side does not lend itself well to the changes we want to make to support filter state retention. The crux of the problem in the current implementation is as follows: - The `xdsresource` package builds a `FilterChainManager` type that does the following: - Keeps track of the list of Route Configuration resources to be requested based on the HTTP filters configured on the Listener resource - Updates the interceptors on the different filter chains based on Route Configuration resource updates (that contain filter config overrides) - Maintaining runtime state in a type that is supposed to represent a configuration blob seems to have been a wrong implementation choice - Everytime a Listener resource update is received, the `xdsresource` package builds a brand new `FilterChainManager` - This means that we *cannot* retain filter state across resource updates This PR makes the following changes: - Changes the layout of the `ListenerUpdate` struct to clearly separate client-side and server-side fields - Also groups information that is shared across client and server listener types into a logical struct - The `ListenerUpdate` will *only* contain information directly sourced from the listener proto - The xDS server implementation will build the runtime representation of the filter chains using the configuration update provided by the `xdsresource` package - This would move the responsibility of updating the interceptors on the filter chain to the xDS server, which is the correct place to be doing such a thing - This would make it possible for the xDS server implementation to retain filter state across listener resource updates RELEASE NOTES: none

easwars added 3 commits February 3, 2026 06:23

group client-side fields in a new struct, HTTPConnectionManagerConfig

94efc7d

rename server-side config field to TCPListener

9d959f6

filter chain changes for the server-side listener

f313996

easwars added Type: Internal Cleanup Refactors, etc Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Feb 3, 2026

easwars added this to the 1.80 Release milestone Feb 3, 2026

easwars modified the milestone: 1.80 Release Feb 3, 2026

easwars requested a review from arjan-bal February 3, 2026 23:35

easwars assigned arjan-bal Feb 3, 2026

arjan-bal reviewed Feb 5, 2026

View reviewed changes

Comment thread internal/xds/xdsclient/xdsresource/filter_chain.go Outdated

Comment thread internal/xds/xdsclient/xdsresource/filter_chain.go Outdated

easwars added 2 commits February 5, 2026 19:41

store the NetworkFilterChainConfig struct by value

11aa559

store SourcePrefixEntry by value

054b977

arjan-bal reviewed Feb 5, 2026

View reviewed changes

easwars added 5 commits February 6, 2026 00:07

store SourcePrefixes by value

6ace66e

store DestinationPrefixEntry by value

45d2933

first round of review comments

3f9eedd

Merge branch 'master' into server_side_http_filter_handling

ce77944

xdm changes post merge to get tests to pass

aac1ab3

arjan-bal mentioned this pull request Feb 6, 2026

Replace usages of net.IP, net.IPNet with netip.Addr and netip.Prefix #8884

Closed

arjan-bal reviewed Feb 9, 2026

View reviewed changes

gemini-code-assist Bot reviewed Feb 9, 2026

View reviewed changes

Comment thread internal/xds/xdsclient/xdsresource/unmarshal_lds.go

arjan-bal reviewed Feb 9, 2026

View reviewed changes

Comment thread internal/xds/xdsclient/xdsresource/unmarshal_lds.go Outdated

arjan-bal assigned easwars and unassigned arjan-bal Feb 9, 2026

second round of review comments from Arjan and Gemini

764f049

easwars assigned arjan-bal and unassigned easwars Feb 11, 2026

gemini-code-assist Bot reviewed Feb 11, 2026

View reviewed changes

minor comments from gemini

408f554

arjan-bal approved these changes Feb 11, 2026

View reviewed changes

arjan-bal assigned easwars and unassigned arjan-bal Feb 11, 2026

easwars merged commit 779b792 into grpc:master Feb 11, 2026
14 checks passed

easwars deleted the server_side_http_filter_handling branch February 11, 2026 19:51

	t.Errorf("FindBestMatchingxdsclient.virtualHostWithInterceptors() = %v, want %v", got, tt.want)
	t.Errorf("findBestMatchingVirtualHostServer() = %v, want %v", got, tt.want)

Conversation

easwars commented Feb 3, 2026 • edited by arjan-bal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arjan-bal commented Feb 9, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

easwars commented Feb 11, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

easwars Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

easwars Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

easwars Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

arjan-bal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

easwars commented Feb 3, 2026 •

edited by arjan-bal

Loading

codecov Bot commented Feb 3, 2026 •

edited

Loading