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: require EDS service name in new-style CDS clusters (gRFC A47) #6438

Merged
merged 1 commit into from Jul 11, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Jul 10, 2023

This PR implements the changes described by grpc/proposal#377

RELEASE NOTES:

  • xds: require EDS service name to be set in a CDS cluster with an 'xdstp' resource name (gRFC A47)

@dfawley dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Jul 10, 2023
@dfawley dfawley added this to the 1.57 Release milestone Jul 10, 2023
@dfawley dfawley requested a review from easwars July 10, 2023 23:06
{
name: "xdstp cluster resource with unset EDS service name",
resource: testutils.MarshalAny(&v3clusterpb.Cluster{
Name: "xdstp:foo",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be xdstp:///foo? I understand it doesn't matter for the purpose of this unit test. But was just curious if xdstp:foo was a valid name at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

The two are equivalent under RFC 3986 parsing.

Also I took it from the test case in C without really thinking too hard about it. 😆

I'm not sure what does or should happen in this case, though. Does/should it just not work? Does/should it use the channel's default authority? Does/should it use the authority for the CDS resource?

The only meaningful thing this cluster.name field seems to do for us is this:

https://github.com/grpc/grpc-go/blob/bf5b7aecd53ba679d72d43bbf61dee40633f6344/xds/internal/balancer/cdsbalancer/cdsbalancer.go#L338C4-L344

Looks like we'd just ignore it (or use the default authority?) to determine the load reporting server. I'm not sure if that's right or not, TBH - do you have any intuition about it?

@easwars easwars assigned dfawley and unassigned easwars Jul 11, 2023
@dfawley dfawley merged commit f0280f9 into grpc:master Jul 11, 2023
11 checks passed
@dfawley dfawley deleted the cdseds branch July 11, 2023 15:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants