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.

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: report more descriptive error when matched route on client is not of type RouteActionRoute #5921

Closed
easwars opened this issue Jan 10, 2023 · 7 comments · Fixed by #6248

Comments

@easwars
Copy link
Contributor

easwars commented Jan 10, 2023

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:

if rt == nil || rt.clusters == nil {

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

@Aditya-Sood
Copy link
Contributor

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

@easwars
Copy link
Contributor Author

easwars commented Apr 28, 2023

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

@easwars
Copy link
Contributor Author

easwars commented Apr 28, 2023

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]) ) 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.

@Aditya-Sood
Copy link
Contributor

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

@Aditya-Sood
Copy link
Contributor

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!

@zasweq
Copy link
Contributor

zasweq commented May 24, 2023

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.

@Aditya-Sood
Copy link
Contributor

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants