xds: ignore routes with unsupported cluster specifiers #5269
Conversation
zasweq
force-pushed
the
RLS-in-xDS-changes
branch
from
d915a48
to
dd11bf9
Compare
zasweq
force-pushed
the
RLS-in-xDS-changes
branch
from
dd11bf9
to
0e5f90d
Compare
zasweq
changed the title
| cspNames[a.ClusterSpecifierPlugin] = true | ||
| route.ClusterSpecifierPlugin = a.ClusterSpecifierPlugin | ||
| default: | ||
| return nil, nil, fmt.Errorf("route %+v, has an unknown ClusterSpecifier: %+v", r, a) | ||
| logger.Warningf("route %+v references unknown ClusterSpecifier %+v, the route will be ignored", r, a) |
There was a problem hiding this comment.
I based this warning log off what was currently in the codebase with regards to ignoring routes. Let me know, I'll leave for now.
There was a problem hiding this comment.
I'm concerned this will become log spam. Is there precedent for logging (at higher priority than INFO) when we get an xDS response we would eventually ACK?
There was a problem hiding this comment.
Switched to INFO, as per discussed in QAZSE.
| cspNames[a.ClusterSpecifierPlugin] = true | ||
| route.ClusterSpecifierPlugin = a.ClusterSpecifierPlugin | ||
| default: | ||
| return nil, nil, fmt.Errorf("route %+v, has an unknown ClusterSpecifier: %+v", r, a) | ||
| logger.Warningf("route %+v references unknown ClusterSpecifier %+v, the route will be ignored", r, a) |
There was a problem hiding this comment.
I based this warning log off what was currently in the codebase with regards to ignoring routes. Let me know, I'll leave for now.
| @@ -747,7 +817,7 @@ func (mockClusterSpecifierPlugin) TypeURLs() []string { | |||
| } | |||
|
|
|||
| func (mockClusterSpecifierPlugin) ParseClusterSpecifierConfig(proto.Message) (clusterspecifier.BalancerConfig, error) { | |||
| return nil, nil | |||
| return []map[string]interface{}{}, nil | |||
There was a problem hiding this comment.
Note that even this config is not really legal either. It doesn't specify any LB policy name at all. But if it doesn't matter to the tests, it should be okay.
There was a problem hiding this comment.
Noted. It used to return nil so I guess it went from one illegal thing to another :).
dfawley
changed the title
This PR does a few things: it adds support for the new IsOptional() logic with regards to Cluster Specifier Plugins (if a Cluster Specifier Plugin is optional but not supported, don't NACK, and ignore any routes that point to it). It also changes the behavior of a route that has an unsupported Cluster Specifier. Previously, we would NACK routes that had an unsupported Cluster Specifier, but we now ignore the route.
RELEASE NOTES: None