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

[Aditya-Sood]

Fixes #5921

RELEASE NOTES: none

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Aditya-Sood / name: Aditya Sood (2ee1fdd, 5aca643, 2223c7f, cd24581, 5f6f416, 212c198, c243e0e)

[Aditya-Sood]

I've submitted the CLA document

[easwars]

@Aditya-Sood

Could you give me some pointers on how I can test this code? I've run the go test tests but the vet script was throwing a command not found error which I'm working on debugging

go test google.golang.org/grpc/... from the grpc-go directory should run all the tests. Also, you should add a test for RPCs matching routes with action not equal to RouteActionRoute and ensure that RPCs fail with the appropriate error. There are bunch of tests in https://github.com/grpc/grpc-go/blob/master/xds/internal/resolver/xds_resolver_test.go which verify different scenarios, and you should be able to a new one for this scenario. Please let me know if you have more questions on the test.

[Aditya-Sood]

thanks for the review @easwars!

I've locally implemented all the changes that have been suggested

they are breaking a couple of tests in xds_resolver_test.go, so I'll work on resolving those + adding the routes with action not equal to RouteActionRoute test case before creating a commit and pushing it to the PR

[Aditya-Sood]

hi @easwars and @tobotg
it seems like there are testcases that use RouteActionUnsupported routes on xds, which are now failing because of this additional check on route type:

1. Test/XDSResolverHTTPFilters/no_router_filter
-> Expects "no router filter present" from newInterceptor()
-> Receiving "action type of matched route is RouteActionUnsupported"

2. Test/XDSResolverHTTPFilters/ignored_after_router_filter
-> Expects err to be nil (throwing "Unexpected error from cs.SelectConfig(_)")
-> Receiving "action type of matched route is RouteActionUnsupported"

3. Test/XDSResolverHTTPFilters/NewStream_error
-> Expects err to be nil (throwing "Unexpected error from cs.SelectConfig(_)")
-> Receiving "action type of matched route is RouteActionUnsupported"

4. Test/XDSResolverHTTPFilters/all_overrides
-> Expects err to be nil (throwing "Unexpected error from cs.SelectConfig(_)")
-> Receiving "action type of matched route is RouteActionUnsupported"

I also tried flipping the order and putting the if rt.clusters == nil {...} statement before the if rt.actionType != xdsresource.RouteActionRoute {...} statement, and the tests are still failing

(Output of go test google.golang.org/grpc/... - test4.txt)

does this contradict our working assumption that route with action type which is not RouteActionRoute, the clusters field will be nil? or are these valid tests which now need to be modified to expect an error if rt.actionType != RouteActionRoute?

[easwars]

@Aditya-Sood
Thank you so much for bringing up the issue of the failing tests. We had a discussion about this, and the decision is to ensure that an incoming LDS response is NACK'ed at the xDS client if the list of HTTP filters does not contain the router filter and/or the router filter is not the last one in the list.

Would you be interested in making that change? Once we do that, we can stop checking that the router filter is the last one in the list here.

I can provide you with more information if you are interested to make that change. Please let me know.

[Aditya-Sood]

hi @easwars, yes I can work on that change too, please share the details with me

[easwars]

hi @easwars, yes I can work on that change too, please share the details with me

Here you go: #6259.

[easwars]

I'm setting the Blocked label on this one since we need to fix #6259 before getting back to this. Thanks for your help @Aditya-Sood

[zasweq]

So newInterceptor() shouldn't be checking for Router Filter and ignoring HTTP filters after, this was the legacy behavior that I should've deleted (the language iterated on here and deleted: https://github.com/grpc/proposal/pull/250/files) when I added the NACK check in the client for IsTerminal() here: #4676. newInterceptor() should simply just run the RPCs through all the filters.

[Aditya-Sood]

hi @zasweq
do you mean we should delete this section of code in newInterceptor()?

if router.IsRouterFilter(filter.Filter) {
	// Ignore any filters after the router filter.  The router itself
	// is currently a nop.
	return &interceptorList{interceptors: interceptors}, nil
}

(link to serviceconfig.go file)

[Aditya-Sood]

sorry for the delay in resolution, have been caught up with my day job
I've been going through the shared docs, will try to close the PR over the weekend

[Aditya-Sood]

hi @easwars and @zasweq
apologies for the delay, I had written the test case last week but wasn't fully sure that my logic for causing the error was in tune with the actual code flow, so was still going through the docs this week

1. thanks to @easwars's bug fix (blank identifier import) and shared documentation, I was able to understand the high-level flow and write the new test case by changing the route config update for the new test case with a RouteActionUnsupported ActionType - link to commit

2. can this PR be re-opened? if not I will create a new one

3. one query - in the resolver.SelectConfig() code, would it be semantically better to replace the rt.actionType != xdsresource.RouteActionRoute check with rt.actionType == xdsresource.RouteActionUnsupported, since the latter is already used to represent routing types not currently supported by gRPC?

[easwars]

3. one query - in the resolver.SelectConfig() code, would it be semantically better to replace the rt.actionType != xdsresource.RouteActionRoute check with rt.actionType == xdsresource.RouteActionUnsupported, since the latter is already used to represent routing types not currently supported by gRPC?

resolver.SelectConfig() happens on the client side and on that that the only supporting action type is xdsresource.RouteActionRoute.

[Aditya-Sood]

hi @easwars, thanks for the review!
I have included the above changes in the latest commit

1. I've changed the variable name to errUnsupportedClientRouteAction instead of the suggested errUnsupportedRouteAction - I felt the latter could be confused as an error specifically for RouteActionUnsupported action type
   However if you feel the clarification is unnecessary kindly let me know, I'll change it accordingly

2. For the "NewStream error" testcase I've removed the second route and RPC response - not sure why it was added in the first place, but the testcase passes after the change as well

3. One query: As clarified in the previous comment, the client requests will always be assigned the RouteActionRoute action type by the xds client (source code snippet)
   Since that is the case, is the additional check introduced by this PR and the testcases for RouteActionUnsupported/NonForwardingAction primarily from a completeness perspective? Or is it possible to have client requests which are not of RouteActionRoute type?

[easwars]

3. One query: As clarified in the previous comment, the client requests will always be assigned the RouteActionRoute action type by the xds client (source code snippet)
   Since that is the case, is the additional check introduced by this PR and the testcases for RouteActionUnsupported/NonForwardingAction primarily from a completeness perspective? Or is it possible to have client requests which are not of RouteActionRoute type?

Currently, the only supported route action type on the client is RouteActionRoute and the only supported route action type on the server is NonForwardingAction. The gRPC client and server share the same xdsClient which is the one which receives the resource from the management server and parses it. When parsing, the xdsClient does not have enough information to know whether the route configuration will be used by the client or the server (because the same application can have a server and a client). So, it will accept routes with either of these two values. And therefore we need extra validation at the client and server sides.

Yes, including test cases for RouteActionUnsupported and NonForwardingAction is for completeness.

[Aditya-Sood]

thank you for the clarification!
I've moved the RouteActionNonForwardingAction testcase as well

[Aditya-Sood]

hi @easwars @zasweq
are there any further changes required for this PR?

[easwars]

@zasweq for second set of eyes

[zasweq]

LGTM. Doug mentioned that he wishes that the Route Type enum can be plumbed all the way from the client so we can log the exact action that was unsupported, rather than blanket it in a RouteActionUnsupported log (equivalent to what the blanket enum type which is emitted from the xDS Client). However, unfortunately, we don't emit the proto enum for Route Action, so just something to think about for the future if we want to change it. This change fixing the semantics of the error message is an improvement though :), thanks for contribution.

[Aditya-Sood]

Merged branch fixes #6259 as well