Skip to content

Commit

Permalink
Increment: Remove IsRouterFilter() check in resolver.configSelector.n…
Browse files Browse the repository at this point in the history
…ewInterceptor()
  • Loading branch information
Aditya-Sood committed Jul 14, 2023
1 parent 5aca643 commit 2223c7f
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 8 deletions.
5 changes: 3 additions & 2 deletions xds/internal/httpfilter/fault/fault_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@ import (
testgrpc "google.golang.org/grpc/interop/grpc_testing"
testpb "google.golang.org/grpc/interop/grpc_testing"

_ "google.golang.org/grpc/xds/internal/balancer" // Register the balancers.
_ "google.golang.org/grpc/xds/internal/resolver" // Register the xds_resolver.
_ "google.golang.org/grpc/xds/internal/balancer" // Register the balancers.
_ "google.golang.org/grpc/xds/internal/httpfilter/router" // Register the router filter.
_ "google.golang.org/grpc/xds/internal/resolver" // Register the xds_resolver.
)

const defaultTestTimeout = 10 * time.Second
Expand Down
7 changes: 1 addition & 6 deletions xds/internal/resolver/serviceconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
"google.golang.org/grpc/xds/internal/balancer/clustermanager"
"google.golang.org/grpc/xds/internal/balancer/ringhash"
"google.golang.org/grpc/xds/internal/httpfilter"
"google.golang.org/grpc/xds/internal/httpfilter/router"
"google.golang.org/grpc/xds/internal/xdsclient/xdsresource"
)

Expand Down Expand Up @@ -285,12 +284,8 @@ func (cs *configSelector) newInterceptor(rt *route, cluster *routeCluster) (ires
return nil, nil
}
interceptors := make([]iresolver.ClientInterceptor, 0, len(cs.httpFilterConfig))
for _, filter := range cs.httpFilterConfig {
// TODO: Ignoring terminal filters has no effect, need to ignore routers - why?
if router.IsRouterFilter(filter.Filter) {
continue
}

for _, filter := range cs.httpFilterConfig {
override := cluster.httpFilterConfigOverride[filter.Name] // cluster is highest priority
if override == nil {
override = rt.httpFilterConfigOverride[filter.Name] // route is second priority
Expand Down
33 changes: 33 additions & 0 deletions xds/internal/resolver/xds_resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1813,6 +1813,39 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) {
selectErr string
newStreamErr string
}{

/* {
name: "no router filter",
ldsFilters: []xdsresource.HTTPFilter{
{Name: "foo", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "foo1"}},
},
rpcRes: map[string][][]string{
"1": {
{"build:foo1", "override:foo2", "build:bar1", "override:bar2", "newstream:foo1", "newstream:bar1", "done:bar1", "done:foo1"},
},
},
selectErr: "no router filter present",
},
{
name: "ignored after router filter",
ldsFilters: []xdsresource.HTTPFilter{
{Name: "foo", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "foo1"}},
routerFilter,
{Name: "foo2", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "foo2"}},
},
rpcRes: map[string][][]string{
"1": {
{"build:foo1", "newstream:foo1", "done:foo1"},
},
"2": {
{"build:foo1", "newstream:foo1", "done:foo1"},
{"build:foo1", "newstream:foo1", "done:foo1"},
{"build:foo1", "newstream:foo1", "done:foo1"},
},
},
selectErr: "matched route does not have a supported route type",
}, */

{
name: "NewStream error; ensure earlier interceptor Done is still called",
ldsFilters: []xdsresource.HTTPFilter{
Expand Down

0 comments on commit 2223c7f

Please sign in to comment.