xds: Generic xds client resource watchine e2e #8183

purnesh42H · 2025-03-19T15:54:45Z

This is the change to make generic xDS client do resource watching from xDS management server. The xDS client uses the ADS (Aggregated Data Service) stream transport channel that was created in #8144. The e2e tests are written using the listener resource type from #8144 and grpctransport from #8066.

The PR copies the existing clientimpl, authority, clientimpl_watcher from internal xdsclient code along with the helpers that are part of gRPC and then modify them to use the generic client types and interfaces.

The PR also adds String() and Equal() methods for ServerIdentifierExtension and implements them for grpctransport.ServerIdentifierExtension and a custom map implementation similar to resolver.AddrMap to re-use existing channels for similar server configurations.

Recommend to review each commit of the PR separately in the order
Commits with copy prefix are copy paste of internal xdsclient code and are followed by modifications. Review only the modification commit.

RELEASE NOTES: None

codecov · 2025-03-19T16:10:33Z

Codecov Report

Attention: Patch coverage is 66.15067% with 328 lines in your changes missing coverage. Please review.

Project coverage is 81.74%. Comparing base (0af5a16) to head (58d035d).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/clients/xdsclient/authority.go	57.54%	165 Missing and 29 partials ⚠️
xds/internal/clients/xdsclient/clientimpl.go	66.16%	49 Missing and 19 partials ⚠️
xds/internal/clients/internal/map.go	66.10%	20 Missing ⚠️
...s/internal/clients/grpctransport/grpc_transport.go	73.01%	12 Missing and 5 partials ⚠️
.../internal/clients/xdsclient/clientimpl_watchers.go	72.09%	9 Missing and 3 partials ⚠️
xds/internal/clients/xdsclient/xdsclient.go	66.66%	7 Missing and 1 partial ⚠️
xds/internal/clients/xdsclient/map.go	92.98%	3 Missing and 1 partial ⚠️
...ds/internal/clients/internal/testutils/wrappers.go	87.50%	2 Missing and 1 partial ⚠️
xds/internal/clients/xdsclient/xdsconfig.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8183      +/-   ##
==========================================
- Coverage   82.04%   81.74%   -0.30%     
==========================================
  Files         410      418       +8     
  Lines       40251    41195     +944     
==========================================
+ Hits        33023    33676     +653     
- Misses       5867     6081     +214     
- Partials     1361     1438      +77

Files with missing lines	Coverage Δ
xds/internal/clients/internal/internal.go	`97.87% <100.00%> (+8.98%)`	⬆️
xds/internal/clients/internal/testutils/channel.go	`100.00% <100.00%> (ø)`
xds/internal/clients/xdsclient/logging.go	`100.00% <100.00%> (ø)`
xds/internal/clients/xdsclient/xdsconfig.go	`50.00% <50.00%> (ø)`
...ds/internal/clients/internal/testutils/wrappers.go	`87.50% <87.50%> (ø)`
xds/internal/clients/xdsclient/map.go	`92.98% <92.98%> (ø)`
xds/internal/clients/xdsclient/xdsclient.go	`70.37% <66.66%> (+70.37%)`	⬆️
.../internal/clients/xdsclient/clientimpl_watchers.go	`72.09% <72.09%> (ø)`
...s/internal/clients/grpctransport/grpc_transport.go	`80.86% <73.01%> (-9.70%)`	⬇️
xds/internal/clients/internal/map.go	`66.10% <66.10%> (ø)`
... and 2 more

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…esting

purnesh42H · 2025-03-19T20:10:37Z

@dfawley could you do a pass on the implementation? For now, I have removed the metrics from client as it requires some more thought. It can be added in separate commit or PR. Otherwise the main implementation can be reviewed to make sure we are following the right approach and do the changes (if needed) before we go further. Also, I will be adding more e2e tests in subsequent commits

dfawley · 2025-03-20T16:58:20Z

xds/internal/clients/grpctransport/grpc_transport.go

+		return ""
+	}
+	var tcParts []string
+	if sie.Credentials.TransportCredentials() != nil {


Ideally we shouldn't need to do this. And if we do, then we should probably be doing it in the TransportCredentials type, not here.

Note that String()s are intended for human-readable formatting or for debugging, and not for exhaustive uniqueness purposes. So using it as a map key is probably a bad idea.

So, user will have to provide String() implementation for both TransportCredentials and PerRPCCredentials?

No. Only if they want the debugging messages we would log including them to look pretty.

Modifed String() for both helper and grpctransport. PTAL

dfawley · 2025-03-20T17:10:49Z

xds/internal/clients/grpctransport/grpc_transport.go

+		return false
+	}
+
+	if sie.Credentials.TransportCredentials() != nil {


This is not a guaranteed correct equality check.

We should see if the credentials implements Equal and call it. If not, if they are pointers we could compare those. But if they are not, we would just have to consider them not equal and maybe log a warning or something.

Same, so user is expected provide Equal() implementation for Credentials?

At the xdsclient level, we might want to make the extensions required to implement Equal.

At the grpctransport level, I am thinking the best we can do is be noisy if the credentials don't implement it.

Added comment on docstring for requirement of Equal in ServerIdentifierConfig.Extensions. Modified Equal() for both helper and grpctransport. PTAL.

dfawley · 2025-03-20T17:18:27Z

xds/internal/clients/grpctransport/grpc_transport.go

+	if sie.Credentials.PerRPCCredentials() != nil {
+		if sie.Credentials.PerRPCCredentials().RequireTransportSecurity() != sie2.Credentials.PerRPCCredentials().RequireTransportSecurity() {
+			return false
+		}
+	}


Same comment as above, but even more strongly. RequireTransportSecurity is a boolean. If two credentials both require transport security, there is no reason to assume they are the same credential.

Done as suggested above

purnesh42H added 4 commits March 19, 2025 11:43

add Equal and String helpers for clients.ServerIdentifier

15db277

Add String() and Equal() to grpctransport

c0fb2f2

copy clientimpl, clientimpl_watcher and authority files

ba9ad8c

modify copied clientimpl and related files for resource watching

3288a50

purnesh42H added 3 commits March 20, 2025 00:57

modify ServerIdentifierExtension to use pointer receiver

455b5f0

copy lds_watcher_test and helpers

545c9c0

modify copied lds_watcher_test and helpers for e2e resource watcher t…

02db583

…esting

purnesh42H force-pushed the generic-xds-client-resource-watchine-e2e branch from c99ca98 to 02db583 Compare March 19, 2025 19:48

purnesh42H requested a review from dfawley March 19, 2025 20:07

purnesh42H assigned dfawley Mar 19, 2025

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

purnesh42H added this to the 1.72 Release milestone Mar 19, 2025

dfawley reviewed Mar 20, 2025

View reviewed changes

purnesh42H requested a review from dfawley March 20, 2025 20:21

dfawley review1: modify String() and Equal()

124b2f3

purnesh42H force-pushed the generic-xds-client-resource-watchine-e2e branch 2 times, most recently from a88424f to f26dc05 Compare March 22, 2025 20:17

add ServerIdentiferMap and serverConfigMap for transort channel re-use

3ef197c

purnesh42H force-pushed the generic-xds-client-resource-watchine-e2e branch 2 times, most recently from 9e02660 to ebbfe3f Compare March 23, 2025 17:45

serverConfigMap unit tests

822b748

purnesh42H force-pushed the generic-xds-client-resource-watchine-e2e branch from ebbfe3f to 822b748 Compare March 23, 2025 17:46

xdsclient validations

7064881

purnesh42H force-pushed the generic-xds-client-resource-watchine-e2e branch from aeb32c1 to 7064881 Compare March 23, 2025 19:36

purnesh42H added 2 commits March 24, 2025 01:08

copy authority tests

4b797c9

modify authorty tests

58d035d

purnesh42H changed the title ~~Generic xds client resource watchine e2e~~ xds: Generic xds client resource watchine e2e Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: Generic xds client resource watchine e2e #8183

xds: Generic xds client resource watchine e2e #8183

purnesh42H commented Mar 19, 2025 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

purnesh42H commented Mar 19, 2025 •

edited

Loading

dfawley Mar 20, 2025

purnesh42H Mar 20, 2025

dfawley Mar 20, 2025

purnesh42H Mar 20, 2025

dfawley Mar 20, 2025

purnesh42H Mar 20, 2025

dfawley Mar 20, 2025 •

edited

Loading

purnesh42H Mar 20, 2025

dfawley Mar 20, 2025

purnesh42H Mar 20, 2025

xds: Generic xds client resource watchine e2e #8183

Are you sure you want to change the base?

xds: Generic xds client resource watchine e2e #8183

Conversation

purnesh42H commented Mar 19, 2025 • edited Loading

codecov bot commented Mar 19, 2025 • edited Loading

Codecov Report

purnesh42H commented Mar 19, 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

Choose a reason for hiding this comment

dfawley Mar 20, 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

purnesh42H commented Mar 19, 2025 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

purnesh42H commented Mar 19, 2025 •

edited

Loading

dfawley Mar 20, 2025 •

edited

Loading