xds: introduce generic xds clients common configs #8024

purnesh42H · 2025-01-21T04:51:26Z

This is the first part of generic xds clients to be usable outside of grpc. This change is adding the common configurations that are required to create xDS and LRS clients for communicating to an xDS management server.

POC
Internal Design

RELEASE NOTES: None

codecov · 2025-01-21T05:00:03Z

Codecov Report

Attention: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.37%. Comparing base (2517a46) to head (e191ba1).
Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/clients/config.go	93.18%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8024      +/-   ##
==========================================
+ Coverage   82.24%   82.37%   +0.13%     
==========================================
  Files         382      388       +6     
  Lines       38711    39062     +351     
==========================================
+ Hits        31836    32179     +343     
- Misses       5555     5565      +10     
+ Partials     1320     1318       -2

Files with missing lines	Coverage Δ
xds/internal/clients/config.go	`93.18% <93.18%> (ø)`

... and 62 files with indirect coverage changes

xds/clients/config.go

xds/clients/README

xds/clients/config.go

xds/clients/README

xds/clients/config.go

xds/clients/transport_builder.go

xds/clients/README

xds/clients/config.go

xds/clients/transport_builder.go

easwars

Changes look good to me.

What is the plan for tests? I don't see any tests in this PR.

easwars · 2025-01-28T05:11:09Z

xds/clients/config.go

+	case sc.IgnoreResourceDeletion != other.IgnoreResourceDeletion:
+		return false
+	}
+	if ex, ok := sc.Extensions.(interface{ Equal(any) bool }); ok && ex.Equal(other.Extensions) {


Nit: we should probably make a mention of this Equal method in the docstring for the Extensions field. That way, users know that they need to implement this.

Also, looks like we are missing tests for the Equal method. Codecov is complaining. I'm OK if you want to add these in a follow-up PR. But if you are doing that, please ensure that you either file an issue or send that PR right away so that we don't forget about it. Thanks.

I have mentioned the Equal method in the docstring

I have added tests for both Locality and ServerConfig Equal

purnesh42H · 2025-01-28T10:15:53Z

Changes look good to me.

What is the plan for tests? I don't see any tests in this PR.

I have added the test for Equal Methods, Locality's empty check and Node's ToProto. Initially i was thinking that they would get covered through implementation code and e2e tests but adding here is better to not discover something wrong later.

dfawley · 2025-01-28T16:11:08Z

xds/clients/README

Should we put this in internal during early development and move it later, when we're ready for a user?

I have moved to internal. I have kept the README but let me know if you want to remove it for now.

dfawley · 2025-01-28T18:14:32Z

xds/clients/config.go

+	case sc.IgnoreResourceDeletion != other.IgnoreResourceDeletion:
+		return false
+	}
+	if sc.Extensions == nil && other.Extensions == nil {


Merge with the switch above?

That won't be correct right? Switch case is for early return which happens in case of false. For true, we need to ensure all fields are equal so we check Extensions only after all fields are found to be equal. Like if we move Extensions nil check to switch, the code look ambiguous for condition when say URIs are different and both Extensions are nil.

Though i have two more test cases, when either of server config extensions are nil

dfawley · 2025-01-28T18:23:51Z

xds/clients/config.go

+	// ClientFeatures is a list of xDS features supported by this client.
+	// These features are set within the xDS client, but may be overridden only
+	// for testing purposes.
+	ClientFeatures []string


Do we need to export this?

So, this field is populated by the xDS client which will be in a separate package xdsclient. So xDS client needs to access it. That's why we need to export or we need to provide a method to populate it. What is the best way?

I would think the best would be an internal method that is able to set this unexported field.

Either way, unexport it and we can work out the best way to set it when we need to set it. Things should not exported because of testing or internal usage.

Similar for ToProto. This isn't something that users will call, so it shouldn't be exported. Maybe that's how this shakes out....ToProto becomes a function inside xdsclient and that is where we set the client features.

I have unexported toProto and clientFeatures. Another way I can think is we can unexport node and provide a function to create new node which sets clientFeatures and have an exported method to get Node in proto form because that's what channel needs. We can discuss more during implementation.

xds/internal/clients/config_test.go

dfawley

Before we start implementing anything, we should go over the proposed API, as viewed through godoc, in a meeting. There's already a few things that are leaking into the external API that are implementation details that I've found, so we should make sure there aren't more before we get too far down the road.

dfawley · 2025-01-30T19:15:33Z

xds/internal/clients/config.go

+// IsEmpty reports whether l is considered empty.
+func (l Locality) IsEmpty() bool {
+	return l.Equal(Locality{})
+}
+
+// Equal returns true if l and other are considered equal.
+func (l Locality) Equal(other Locality) bool {


Why are these exported? Users shouldn't need them?

same reason because xdsclient implementation needs. Unexported for now.

dfawley · 2025-01-30T19:16:03Z

xds/internal/clients/config.go

+}
+
+// Equal returns true if sc and other are considered equal.
+func (sc *ServerConfig) Equal(other *ServerConfig) bool {


same reason because xdsclient implementation needs. Unexported for now.

dfawley · 2025-01-30T19:17:59Z

xds/internal/clients/README

Does this not need to be README.md? But can we just delete it now that it's internal?

Deleted for now

purnesh42H · 2025-01-31T10:15:13Z

Before we start implementing anything, we should go over the proposed API, as viewed through godoc, in a meeting. There's already a few things that are leaking into the external API that are implementation details that I've found, so we should make sure there aren't more before we get too far down the road.

I have unexported all the methods of structs including isEmpty and equal. All these are needed by xds client implementation.

Yeah we can go over the exported stuff and we also need to decide how will xdsclient access these struct methods

purnesh42H force-pushed the generic-xds-client-1-interface branch 3 times, most recently from b45e034 to 27adf6e Compare January 21, 2025 04:54

purnesh42H requested review from dfawley and easwars January 21, 2025 04:55

purnesh42H assigned dfawley and easwars Jan 21, 2025

purnesh42H added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Jan 21, 2025

purnesh42H added this to the 1.71 Release milestone Jan 21, 2025

purnesh42H force-pushed the generic-xds-client-1-interface branch 2 times, most recently from c4fd324 to 88c7d68 Compare January 21, 2025 17:48

zasweq reviewed Jan 22, 2025

View reviewed changes

xds/clients/config.go Outdated Show resolved Hide resolved

zasweq reviewed Jan 22, 2025

View reviewed changes

xds/clients/config.go Outdated Show resolved Hide resolved

purnesh42H requested a review from zasweq January 22, 2025 04:36

easwars reviewed Jan 22, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars and dfawley Jan 22, 2025

purnesh42H force-pushed the generic-xds-client-1-interface branch 2 times, most recently from 4e9e733 to ca5b27d Compare January 23, 2025 08:40

purnesh42H requested a review from easwars January 23, 2025 09:20

purnesh42H assigned easwars and dfawley and unassigned purnesh42H Jan 23, 2025

easwars reviewed Jan 23, 2025

View reviewed changes

xds/clients/README Outdated Show resolved Hide resolved

xds/clients/config.go Outdated Show resolved Hide resolved

easwars reviewed Jan 23, 2025

View reviewed changes

xds/clients/config.go Outdated Show resolved Hide resolved

xds/clients/config.go Outdated Show resolved Hide resolved

easwars reviewed Jan 23, 2025

View reviewed changes

easwars removed their assignment Jan 23, 2025

purnesh42H mentioned this pull request Jan 27, 2025

xds: introduce generic xds clients xDS and LRS Client API signatures #8042

Open

purnesh42H changed the title ~~xds: introduce generic xds client common configs~~ xds: introduce generic xds clients common configs Jan 27, 2025

easwars reviewed Jan 28, 2025

View reviewed changes

easwars assigned dfawley and unassigned easwars Jan 28, 2025

easwar comments round 4

4989790

purnesh42H force-pushed the generic-xds-client-1-interface branch from d993711 to 8375a37 Compare January 28, 2025 10:14

purnesh42H requested a review from easwars January 28, 2025 10:17

config tests

377e7d1

purnesh42H force-pushed the generic-xds-client-1-interface branch from 8375a37 to 377e7d1 Compare January 28, 2025 16:18

dfawley reviewed Jan 28, 2025

View reviewed changes

move clients to xds/internal

db86d0b

purnesh42H force-pushed the generic-xds-client-1-interface branch from 672a5e3 to db86d0b Compare January 29, 2025 07:07

purnesh42H requested a review from dfawley January 29, 2025 07:11

unexport toProto and clientFeatures

32eda45

easwars reviewed Jan 29, 2025

View reviewed changes

xds/internal/clients/config_test.go Outdated Show resolved Hide resolved

change subtests names to resemble identifiers

1fade0c

purnesh42H requested a review from easwars January 29, 2025 18:14

purnesh42H assigned easwars Jan 29, 2025

easwars approved these changes Jan 29, 2025

View reviewed changes

easwars removed their assignment Jan 29, 2025

dfawley approved these changes Jan 30, 2025

View reviewed changes

dfawley reviewed Jan 30, 2025

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Jan 30, 2025

unexport methods

e191ba1

purnesh42H force-pushed the generic-xds-client-1-interface branch from 30aac28 to e191ba1 Compare January 31, 2025 10:20

purnesh42H merged commit 7dbf12e into grpc:master Jan 31, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: introduce generic xds clients common configs #8024

xds: introduce generic xds clients common configs #8024

purnesh42H commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

easwars left a comment •

edited

Loading

easwars Jan 28, 2025

purnesh42H Jan 28, 2025

purnesh42H Jan 28, 2025

purnesh42H commented Jan 28, 2025 •

edited

Loading

dfawley Jan 28, 2025

purnesh42H Jan 29, 2025 •

edited

Loading

dfawley Jan 28, 2025

purnesh42H Jan 29, 2025

purnesh42H Jan 29, 2025

dfawley Jan 28, 2025

purnesh42H Jan 29, 2025 •

edited

Loading

dfawley Jan 29, 2025

purnesh42H Jan 29, 2025 •

edited

Loading

dfawley left a comment

dfawley Jan 30, 2025

purnesh42H Jan 31, 2025

dfawley Jan 30, 2025

purnesh42H Jan 31, 2025

dfawley Jan 30, 2025

purnesh42H Jan 31, 2025

purnesh42H commented Jan 31, 2025 •

edited

Loading

xds: introduce generic xds clients common configs #8024

xds: introduce generic xds clients common configs #8024

Conversation

purnesh42H commented Jan 21, 2025 • edited Loading

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

easwars left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

purnesh42H Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Jan 31, 2025 • edited Loading

purnesh42H commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

easwars left a comment •

edited

Loading

purnesh42H commented Jan 28, 2025 •

edited

Loading

purnesh42H Jan 29, 2025 •

edited

Loading

purnesh42H Jan 29, 2025 •

edited

Loading

purnesh42H Jan 29, 2025 •

edited

Loading

purnesh42H commented Jan 31, 2025 •

edited

Loading