Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: improve error message when matched route on client is not of type RouteActionRoute #6248

Merged
merged 7 commits into from
Aug 10, 2023
Merged

xds: improve error message when matched route on client is not of type RouteActionRoute #6248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2ee1fdd
Increment: Add 'actionType' field to type resolver.route
Aditya-Sood May 3, 2023
5aca643
Increment: Add review changes + fix failing tests + no longer skip fi…
Aditya-Sood Jun 27, 2023
2223c7f
Increment: Remove IsRouterFilter() check in resolver.configSelector.n…
Aditya-Sood Jul 14, 2023
cd24581
Increment: Add test case for "route type not RouteActionRoute"
Aditya-Sood Jul 22, 2023
5f6f416
Increment: Add test case for action type 'RouteActionNonForwardingAct…
Aditya-Sood Aug 2, 2023
212c198
Increment: Modify 'RouteActionNonForwardingAction' test to be a separ…
Aditya-Sood Aug 2, 2023
c243e0e
Remove newline
Aditya-Sood Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions xds/internal/httpfilter/fault/fault_test.go
View file
Open in desktop
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
16 changes: 9 additions & 7 deletions xds/internal/resolver/serviceconfig.go
View file
Open in desktop
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 @@ -121,6 +120,7 @@ type routeCluster struct {

type route struct {
m *xdsresource.CompositeMatcher // converted from route matchers
actionType xdsresource.RouteActionType // holds route action type
clusters wrr.WRR // holds *routeCluster entries
maxStreamDuration time.Duration
// map from filter name to its config
Expand All @@ -142,6 +142,7 @@ type configSelector struct {
}

var errNoMatchedRouteFound = status.Errorf(codes.Unavailable, "no matched route was found")
var errUnsupportedClientRouteAction = status.Errorf(codes.Unavailable, "matched route does not have a supported route action type")

func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RPCConfig, error) {
if cs == nil {
Expand All @@ -155,10 +156,15 @@ func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RP
break
}
}

if rt == nil || rt.clusters == nil {
return nil, errNoMatchedRouteFound
}

if rt.actionType != xdsresource.RouteActionRoute {
return nil, errUnsupportedClientRouteAction
}

cluster, ok := rt.clusters.Next().(*routeCluster)
if !ok {
return nil, status.Errorf(codes.Internal, "error retrieving cluster for match: %v (%T)", cluster, cluster)
Expand Down Expand Up @@ -280,11 +286,6 @@ func (cs *configSelector) newInterceptor(rt *route, cluster *routeCluster) (ires
}
interceptors := make([]iresolver.ClientInterceptor, 0, len(cs.httpFilterConfig))
for _, filter := range cs.httpFilterConfig {
if router.IsRouterFilter(filter.Filter) {
// Ignore any filters after the router filter. The router itself
// is currently a nop.
return &interceptorList{interceptors: interceptors}, nil
}
override := cluster.httpFilterConfigOverride[filter.Name] // cluster is highest priority
if override == nil {
override = rt.httpFilterConfigOverride[filter.Name] // route is second priority
Expand All @@ -305,7 +306,7 @@ func (cs *configSelector) newInterceptor(rt *route, cluster *routeCluster) (ires
interceptors = append(interceptors, i)
}
}
return nil, fmt.Errorf("error in xds config: no router filter present")
return &interceptorList{interceptors: interceptors}, nil
}

// stop decrements refs of all clusters referenced by this config selector.
Expand Down Expand Up @@ -381,6 +382,7 @@ func (r *xdsResolver) newConfigSelector(su serviceUpdate) (*configSelector, erro
if err != nil {
return nil, err
}
cs.routes[i].actionType = rt.ActionType
if rt.MaxStreamDuration == nil {
cs.routes[i].maxStreamDuration = su.ldsConfig.maxStreamDuration
} else {
Expand Down
125 changes: 83 additions & 42 deletions xds/internal/resolver/xds_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,13 @@ const (
defaultTestShortTimeout = 100 * time.Microsecond
)

var target = resolver.Target{URL: *testutils.MustParseURL("xds:///" + targetStr)}
var (
target = resolver.Target{URL: *testutils.MustParseURL("xds:///" + targetStr)}

var routerFilter = xdsresource.HTTPFilter{Name: "rtr", Filter: httpfilter.Get(router.TypeURL)}
routerHTTPFilter = httpfilter.Get(router.TypeURL)
routerConfig, _ = routerHTTPFilter.ParseFilterConfig(testutils.MarshalAny(&v3routerpb.Router{}))
routerFilter = xdsresource.HTTPFilter{Name: "rtr", Filter: routerHTTPFilter, Config: routerConfig}
)

type s struct {
grpctest.Tester
Expand Down Expand Up @@ -1802,42 +1806,65 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) {
testCases := []struct {
name string
ldsFilters []xdsresource.HTTPFilter
vhOverrides map[string]httpfilter.FilterConfig
rtOverrides map[string]httpfilter.FilterConfig
clOverrides map[string]httpfilter.FilterConfig
rtCfgUpdate xdsresource.RouteConfigUpdate
rpcRes map[string][][]string
selectErr string
newStreamErr string
}{

{
name: "no router filter",
name: "route type RouteActionUnsupported invalid for client",
ldsFilters: []xdsresource.HTTPFilter{
{Name: "foo", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "foo1"}},
},
rtCfgUpdate: xdsresource.RouteConfigUpdate{
VirtualHosts: []*xdsresource.VirtualHost{
{
Domains: []string{targetStr},
Routes: []*xdsresource.Route{{
Prefix: newStringP("1"),
WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1},
},
ActionType: xdsresource.RouteActionUnsupported,
Aditya-Sood marked this conversation as resolved.
Show resolved Hide resolved
}},
},
},
},
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",
selectErr: errUnsupportedClientRouteAction.Error(),
},
{
name: "ignored after router filter",
name: "route type RouteActionNonForwardingAction invalid for client",
ldsFilters: []xdsresource.HTTPFilter{
{Name: "foo", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "foo1"}},
routerFilter,
{Name: "foo2", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "foo2"}},
},
rtCfgUpdate: xdsresource.RouteConfigUpdate{
VirtualHosts: []*xdsresource.VirtualHost{
{
Domains: []string{targetStr},
Routes: []*xdsresource.Route{{
Prefix: newStringP("1"),
WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1},
},
ActionType: xdsresource.RouteActionNonForwardingAction,
}},
},
},
},
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"},
{"build:foo1", "override:foo2", "build:bar1", "override:bar2", "newstream:foo1", "newstream:bar1", "done:bar1", "done:foo1"},
},
},
selectErr: errUnsupportedClientRouteAction.Error(),
},
{
name: "NewStream error; ensure earlier interceptor Done is still called",
Expand All @@ -1846,13 +1873,25 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) {
{Name: "bar", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "bar1", newStreamErr: errors.New("bar newstream err")}},
routerFilter,
},
rtCfgUpdate: xdsresource.RouteConfigUpdate{
VirtualHosts: []*xdsresource.VirtualHost{
{
Domains: []string{targetStr},
Routes: []*xdsresource.Route{{
Prefix: newStringP("1"),
WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1},
},
ActionType: xdsresource.RouteActionRoute,
}},
},
},
},
rpcRes: map[string][][]string{
"1": {
{"build:foo1", "build:bar1", "newstream:foo1", "newstream:bar1" /* <err in bar1 NewStream> */, "done:foo1"},
},
"2": {
{"build:foo1", "build:bar1", "newstream:foo1", "newstream:bar1" /* <err in bar1 NewSteam> */, "done:foo1"},
},
},
newStreamErr: "bar newstream err",
},
Expand All @@ -1863,9 +1902,30 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) {
{Name: "bar", Filter: &filterBuilder{path: &path}, Config: filterCfg{s: "bar1"}},
routerFilter,
},
vhOverrides: map[string]httpfilter.FilterConfig{"foo": filterCfg{s: "foo2"}, "bar": filterCfg{s: "bar2"}},
rtOverrides: map[string]httpfilter.FilterConfig{"foo": filterCfg{s: "foo3"}, "bar": filterCfg{s: "bar3"}},
clOverrides: map[string]httpfilter.FilterConfig{"foo": filterCfg{s: "foo4"}, "bar": filterCfg{s: "bar4"}},
rtCfgUpdate: xdsresource.RouteConfigUpdate{
VirtualHosts: []*xdsresource.VirtualHost{
{
Domains: []string{targetStr},
Routes: []*xdsresource.Route{{
Prefix: newStringP("1"),
WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1},
},
ActionType: xdsresource.RouteActionRoute,
}, {
Prefix: newStringP("2"),
WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1, HTTPFilterConfigOverride: map[string]httpfilter.FilterConfig{"foo": filterCfg{s: "foo4"}, "bar": filterCfg{s: "bar4"}}},
},
HTTPFilterConfigOverride: map[string]httpfilter.FilterConfig{"foo": filterCfg{s: "foo3"}, "bar": filterCfg{s: "bar3"}},
ActionType: xdsresource.RouteActionRoute,
}},
HTTPFilterConfigOverride: map[string]httpfilter.FilterConfig{"foo": filterCfg{s: "foo2"}, "bar": filterCfg{s: "bar2"}},
},
},
},
rpcRes: map[string][][]string{
"1": {
{"build:foo1", "override:foo2", "build:bar1", "override:bar2", "newstream:foo1", "newstream:bar1", "done:bar1", "done:foo1"},
Expand Down Expand Up @@ -1904,26 +1964,7 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) {

// Invoke the watchAPI callback with a good service update and wait for the
// UpdateState method to be called on the ClientConn.
xdsC.InvokeWatchRouteConfigCallback("", xdsresource.RouteConfigUpdate{
VirtualHosts: []*xdsresource.VirtualHost{
{
Domains: []string{targetStr},
Routes: []*xdsresource.Route{{
Prefix: newStringP("1"), WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1},
},
}, {
Prefix: newStringP("2"), WeightedClusters: map[string]xdsresource.WeightedCluster{
"A": {Weight: 1},
"B": {Weight: 1, HTTPFilterConfigOverride: tc.clOverrides},
},
HTTPFilterConfigOverride: tc.rtOverrides,
}},
HTTPFilterConfigOverride: tc.vhOverrides,
},
},
}, nil)
xdsC.InvokeWatchRouteConfigCallback("", tc.rtCfgUpdate, nil)

gotState, err := tcc.stateCh.Receive(ctx)
if err != nil {
Expand Down