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: ignore routes with unsupported cluster specifiers #5269

Merged
merged 4 commits into from Mar 31, 2022

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Mar 22, 2022

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

@zasweq zasweq added the Type: Behavior Change label Mar 22, 2022
@zasweq zasweq added this to the 1.46 Release milestone Mar 22, 2022
@zasweq zasweq requested a review from dfawley Mar 28, 2022
@zasweq zasweq changed the title [Draft] Route Config parsing changes Route Config parsing changes Mar 28, 2022
xds/internal/xdsclient/xdsresource/unmarshal_rds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/unmarshal_rds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/unmarshal_rds.go Outdated Show resolved Hide resolved
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)
Copy link
Contributor

@dfawley dfawley Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have other warning logs when we process xDS data? Is this going to turn into spam? @menghanl @easwars

Copy link
Contributor Author

@zasweq zasweq Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dfawley dfawley Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to INFO, as per discussed in QAZSE.

@dfawley dfawley assigned zasweq and unassigned dfawley Mar 29, 2022
Copy link
Contributor Author

@zasweq zasweq left a comment

Thanks for the comments :D!

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)
Copy link
Contributor Author

@zasweq zasweq Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

xds/internal/xdsclient/xdsresource/unmarshal_rds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/unmarshal_rds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/unmarshal_rds.go Outdated Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned zasweq Mar 30, 2022
@@ -747,7 +817,7 @@ func (mockClusterSpecifierPlugin) TypeURLs() []string {
}

func (mockClusterSpecifierPlugin) ParseClusterSpecifierConfig(proto.Message) (clusterspecifier.BalancerConfig, error) {
return nil, nil
return []map[string]interface{}{}, nil
Copy link
Contributor

@dfawley dfawley Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. It used to return nil so I guess it went from one illegal thing to another :).

@dfawley dfawley assigned zasweq and unassigned dfawley Mar 31, 2022
@dfawley dfawley changed the title Route Config parsing changes xds: ignore routes with unsupported cluster specifiers Mar 31, 2022
@dfawley dfawley merged commit 4e78093 into grpc:master Mar 31, 2022
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Behavior Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants