-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds: refactor xdsresource.Endpoint to add resolver.Endpoint (gRFC A81) #8750
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8750 +/- ##
==========================================
+ Coverage 83.28% 83.36% +0.07%
==========================================
Files 418 418
Lines 32367 32496 +129
==========================================
+ Hits 26958 27091 +133
+ Misses 4034 4022 -12
- Partials 1375 1383 +8
🚀 New features to boost your workflow:
|
easwars
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo minor nits
| // setHostname returns a copy of the given endpoint with hostname added | ||
| // as an attribute. | ||
| func setHostname(endpoint resolver.Endpoint, hostname string) resolver.Endpoint { | ||
| // Only set if non-empty; xds_cluster_impl uses this to trigger :authority rewriting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap comment sentences at 80-cols. See: go/go-style/decisions#comment-line-length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| ResolverEndpoint: resolver.Endpoint{ | ||
| Addresses: []resolver.Address{{Addr: "addr-2-2"}}, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: There are still a few more places in this file where the literal struct could be specified in a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| // Create a copy of endpoint.ResolverEndpoint to avoid race. | ||
| resolverEndpoint := endpoint.ResolverEndpoint | ||
| resolverEndpoint.Addresses = make([]resolver.Address, len(endpoint.ResolverEndpoint.Addresses)) | ||
| copy(resolverEndpoint.Addresses, endpoint.ResolverEndpoint.Addresses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. actually hold on. Copying a slice of resolver.Address will create a new slice with new resolver.Address instances, but they might still be referring to the same attributes. We need to discuss this a little bit more.
We need more discussion on safely copying resolver.Endpoint and resolver.Address structs.
This PR updates the internal
xdsresource.Endpointstruct to contain aresolver.Endpointinstead of a[]stringto store the list of addresses associated with the endpoint gRFC A81. This change standardizes how backend information is stored and ensures that attributes (such as Hostname) are correctly associated with the endpoint hierarchy.Key Changes:
Struct Update:
xdsresource.Endpointnow includes aResolverEndpointfield (of typeresolver.Endpoint) to store addresses and attributes. Remove the existingAddressfield (of type[]string) and store address as aresolver.Endpointfield.Attribute Handling:
SetHostnameandGetHostnamehelpers to manage hostname metadata withinresolver.Endpoint.Attributes.Parsing Logic:
parseEndpointsinunmarshal_eds.goto correctly populate theresolver.Endpointobject.RELEASE NOTES: N/A