grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

xds: report more descriptive error when matched route on client is not of type `RouteActionRoute`

Open easwars opened this issue 2 years ago • 7 comments

In the config selector's SelectConfig method, we look for a matching route, and if we don't find one or the matched route does not contain clusters, we fail the RPC: https://github.com/grpc/grpc-go/blob/f2fbb0e07ebf3dd46aa641ee89c9f17e8083eaf6/xds/internal/resolver/serviceconfig.go#L158

It would be better if we check the route action type on the matched route and return a more meaningful error, when the route action type is not RouteActionRoute which is the only supported one on the client. https://github.com/grpc/grpc-go/blob/master/xds/internal/xdsclient/xdsresource/type_rds.go#L101-L115

easwars avatar Jan 10 '23 22:01 easwars

hi @easwars and @arvindbr8, can I work on this?

Aditya-Sood avatar Apr 28 '23 10:04 Aditya-Sood

@Aditya-Sood : Thank you for your offer. I'm assigning this to you.

easwars avatar Apr 28 '23 20:04 easwars

The ideal fix should look something like this:

  • When parsing an incoming RouteConfiguration resource in the xDS client, we store the route action type in this field.
  • When creating the configSelector in the xDS resolver here, we are currently not plumbing the route action type from [Route])https://github.com/grpc/grpc-go/blob/da1a5eb25d2a3be62c5419c7004f9c0d669ba8bf/xds/internal/xdsclient/xdsresource/type_rds.go#L119) into the internals of the configSelector. We should do that.
  • And once we have route action type in the internal representation of the route in the configSelector, at RPC time in SelectConfig, we should report a more meaningful error when the action type is anything other than Route.

Hope this helps. Please let us know if you have more questions before/when you start working on this.

easwars avatar Apr 28 '23 20:04 easwars

thank you @easwars, I will keep updating here on the progress

Aditya-Sood avatar May 02 '23 03:05 Aditya-Sood

hi @easwars, I have raised PR #6248 to receive your review of the changes, please let me know your inputs on the same thank you!

Aditya-Sood avatar May 03 '23 05:05 Aditya-Sood

An issue me and Doug just discussed is that we don't actually plumb in the full enum to our internal representation. Right now, there's a bucket of RouteActionUnsupported that encapsulates all the route actions we don't support client or sever side. We would have to scale this up as well alongside passing this information to the config selector.

zasweq avatar May 24 '23 17:05 zasweq

update: have asked for clarification on the remaining types of route actions to cover for the returned error - link to comment

Aditya-Sood avatar May 30 '23 18:05 Aditya-Sood