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: client should NACK RDS response with regex (not safe_regex) #4093

Closed
menghanl opened this issue Dec 9, 2020 · 3 comments · Fixed by #4120
Closed

xds: client should NACK RDS response with regex (not safe_regex) #4093

menghanl opened this issue Dec 9, 2020 · 3 comments · Fixed by #4120
Assignees

Comments

@menghanl
Copy link
Contributor

menghanl commented Dec 9, 2020

var route Route
switch pt := pathSp.(type) {
case *v3routepb.RouteMatch_Prefix:
route.Prefix = &pt.Prefix
case *v3routepb.RouteMatch_Path:
route.Path = &pt.Path
case *v3routepb.RouteMatch_SafeRegex:
route.Regex = &pt.SafeRegex.Regex
default:
logger.Warningf("route %+v has an unrecognized path specifier: %+v", r, pt)
continue
}

https://github.com/grpc/proposal/blob/master/A28-xds-traffic-splitting-and-routing.md#response-validation

@menghanl menghanl self-assigned this Dec 9, 2020
@menghanl
Copy link
Contributor Author

menghanl commented Dec 9, 2020

v3 route proto doesn't have the regex field: https://github.com/envoyproxy/go-control-plane/blob/5d7dab350af2fdbfab6603de2e4b2fd6f82b5ebd/envoy/config/route/v3/route_components.pb.go#L1018.

v2 route does though: https://github.com/envoyproxy/go-control-plane/blob/5d7dab350af2fdbfab6603de2e4b2fd6f82b5ebd/envoy/api/v2/route/route_components.pb.go#L930

In the current xds_client, we unmarshall all wire bytes into a v3 message. If regex is set, it will be dropped during unmarshal() (as an unknown message field). So there's no way to check and NACK regex if we keep the current v3 message approach.

@menghanl
Copy link
Contributor Author

menghanl commented Dec 9, 2020

We should NACK the response if it has none of the known fields set.
E.g. if path_specifier doesn't have prefix or path or safeRegex (which are what the client knows of, and can change if the client can handle more types later).

@menghanl
Copy link
Contributor Author

Another thing to check in the route: RouteAction_WeightedClusters.
If WeightedClusters has 0 clusters in it, this route is invalid. The whole RDS response should be NACK'ed.

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

Successfully merging a pull request may close this issue.

1 participant