xds/resolver: support HTTP filters for routes using cluster specifier plugins#9100
xds/resolver: support HTTP filters for routes using cluster specifier plugins#9100Pranjali-2501 wants to merge 6 commits into
Conversation
Pranjali-2501
added
Type: Bug
Area: xDS
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9100 +/- ##
==========================================
+ Coverage 83.08% 83.22% +0.13%
==========================================
Files 413 413
Lines 33484 33487 +3
==========================================
+ Hits 27821 27868 +47
+ Misses 4240 4211 -29
+ Partials 1423 1408 -15
🚀 New features to boost your workflow:
|
| interceptor, err := r.newInterceptor(r.xdsConfig.Listener.APIListener.HTTPFilters, nil, rt.HTTPFilterConfigOverride, r.xdsConfig.VirtualHost.HTTPFilterConfigOverride) | ||
| if err != nil { | ||
| // Clean up any interceptors that were successfully built | ||
| // for the current route before this error occurred. Note | ||
| // that this is not handled by the call to cs.stop() in the | ||
| // deferred function. | ||
| for _, i := range interceptors { | ||
| i.Close() | ||
| } | ||
| return nil, err | ||
| } | ||
| clusters.Add(&routeCluster{ | ||
| name: clusterName, | ||
| interceptor: interceptor, | ||
| }, 1) | ||
| interceptors = append(interceptors, interceptor) | ||
| ci := r.addOrGetActiveClusterInfo(clusterName, "") | ||
| ci.cfg = xdsChildConfig{ChildPolicy: balancerConfig(r.xdsConfig.RouteConfig.ClusterSpecifierPlugins[rt.ClusterSpecifierPlugin])} |
There was a problem hiding this comment.
Lines 420-437 can be shared with the weighted cluster case if we pull it out into a method/helper function. So, let's do that please.
There was a problem hiding this comment.
I have refactored it and move common logic in a seperate method.
| { | ||
| "loadBalancingConfig": [ | ||
| { | ||
| "xds_cluster_manager_experimental": { | ||
| "children": { | ||
| "cluster_specifier_plugin:cspA": { | ||
| "childPolicy": [ | ||
| { | ||
| "csp_experimental": { | ||
| "arbitrary_field": "anything" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }` |
There was a problem hiding this comment.
Nit: Can we have this whole block indented one tabspace to the left. That way, it will align with the code instead of sticking out.
| _, err = res.Interceptor.NewStream(ctx, iresolver.RPCInfo{Method: "/service/method", Context: ctx}, func() {}, newStream) | ||
| if err != nil { |
| newStream := func(context.Context, func()) (iresolver.ClientStream, error) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Why significance does this empty function have? Why does it return nil, nil? Can a nil function be passed?
There was a problem hiding this comment.
No, we cannot pass nil here. The documentation for the NewStream method in the ClientInterceptor interface explicitly states: The caller must ensure done is non-nil. Passing nil would cause a panic.
| // Verify that second filter receives the base path. | ||
| cfg, err = newStreamChan.Receive(ctx) | ||
| if err != nil { | ||
| t.Fatalf("Timeout waiting for second filter to receive config: %v", err) | ||
| } | ||
| ofc = cfg.(overallFilterConfig) | ||
| if ofc.BasePath != "filter-path-2" { | ||
| t.Fatalf("Unexpected base path for second filter, got: %q, want: %q", ofc.BasePath, "filter-path-2") | ||
| } | ||
| if ofc.OverridePath != "" { | ||
| t.Fatalf("Unexpected override path for second filter, got: %q, want: %q", ofc.OverridePath, "") | ||
| } |
There was a problem hiding this comment.
What is the point of having two filters? What extra test coverage does it provide over having a single filter?
There was a problem hiding this comment.
It verifies that the system can correctly build and execute a chain of multiple interceptors. A single filter would only test that an interceptor works in isolation, not that the chaining mechanism itself is functional. It also ensures that configuration overrides are applied correctly on a per-filter basis.
In this test, test-filter-1 is configured with an override, while test-filter-2 uses its base configuration. This allows us to verify that the override for one filter does not affect the configuration of other filters in the chain.
| // addClusterToRoute creates a client interceptor for the given cluster | ||
| // configuration, adds the cluster to the provided WRR picker, and appends the | ||
| // interceptor to the list of interceptors for the current route. | ||
| func (r *xdsResolver) addClusterToRoute(clusters wrr.WRR, clusterName string, weight int64, interceptors *[]iresolver.ClientInterceptor, |
There was a problem hiding this comment.
This is a really complicated method signature. I think we want something that is simpler. I would say go back to what you had earlier, and add a TODO. We will most likely have some more changes to this very specific part of the code when we eliminate the need for random number generation as part of wrr.WRR for single clusters and cluster specifier plugins. So, having the TODO might be something that someone can work on as part of the other cleanup.
3 participants
Fixes #8857
The xDS resolver was building the interceptor chain (which handles HTTP filters) only for routes that pointed to clusters directly or via weighted clusters. Routes that specified a
cluster_specifier_pluginwere missing this step, resulting in HTTP filters being ignored for those routes.This PR updates
newConfigSelectorin xds_resolver.go to build the interceptor chain for routes with aClusterSpecifierPlugin. Added a new testTestResolverClusterSpecifierPlugin_WithFiltersin cluster_specifier_plugin_test.go to verify that the interceptor chain is built and executed for routes matching cluster specifier plugins when HTTP filters are configured.RELEASE NOTES: N/A