Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
1 check 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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants