xds: introduce simple grpc transport for generic xds clients by purnesh42H · Pull Request #8066 · grpc/grpc-go

purnesh42H · 2025-02-05T04:28:28Z

This PR is adding a simple grpc-based transport implementation of generic xds clients transport builder which can be provided as transport builder in xDS and LRS client.

The build() method currently creates a new grpc channel every time. However, in future we will incorporate reference count map for existing transports and deduplicate transports based on provided ServerConfig so that transport channel to same server can be shared among xDS and LRS client.

POC
Internal Design

RELEASE NOTES: None

codecov · 2025-02-05T04:32:58Z

Codecov Report

Attention: Patch coverage is 84.90566% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.21%. Comparing base (c7db760) to head (b175bc0).
Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
...s/internal/clients/grpctransport/grpc_transport.go	84.90%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8066      +/-   ##
==========================================
- Coverage   82.23%   82.21%   -0.03%     
==========================================
  Files         387      388       +1     
  Lines       38947    38996      +49     
==========================================
+ Hits        32028    32060      +32     
- Misses       5601     5607       +6     
- Partials     1318     1329      +11

Files with missing lines	Coverage Δ
...s/internal/clients/grpctransport/grpc_transport.go	`84.90% <84.90%> (ø)`

... and 38 files with indirect coverage changes

dfawley · 2025-02-06T20:36:53Z

+		wantErr   bool
+	}{
+		{
+			name: "ServerURI_empty",


Nit: s/_/ / throughout? t.Run will convert them to underscores to make it a legal test name -- just make sure they're human-readable here.

I have made it more like sentence. Was trying to follow go/go-style/decisions#subtest-names

This is something that I learnt recently as well, and though it feels a little weird in the beginning to have subtest names that have underscrores, when it comes time to run them individually, it makes a lot of sense as direct copy paste works.

dfawley · 2025-02-06T20:39:29Z

+	if err := stream.Send(req); err != nil {
+		t.Fatalf("failed to send message: %v", err)
+	}


Can you also make this do a Recv? Codecov shows that's not covered.

yeah. I wanted to check on this that for Recv, we need a grpc enabled xDS server with byte-based handlers. Should we add an xDS server with byte-based handler or just have a test grpc server to test this? Eventually for e2e tests I think we do need a test xDS server because go-control-plane one proto based.

For this test, we could do literally any kind of grpc server. It could be like the stub server, or whatever. Byte-based is not relevant. The server should ideally do proto as is the default, and we can serialize/deserialize proto messages into byte buffers on the client manually for the testing.

True, for e2e tests of xdsclient, we will need to use go-control-plane. If we have testing helpers that make it easy to use, that's fine but, I'd prefer to copy them into the clients subtree and try to make sure we don't import things from the rest of the grpc tree. Otherwise it will be really hard to separate when it's time to move it out.

For this test. I have created a test server using the same byteCodec to receive and send bytes and written a single test for send and receive. Let me know if that's good enough.

For e2e tests I think we will need to have management server using go-control-plane but that can be separate PR

The server should ideally do proto as is the default

I have created a test server using the same byteCodec

Is there a reason you didn't follow the way I suggested? A normal proto server would be less testing code (which is good) and better proof the byte codec is working as intended.

Kept the test logic same and change the test server to implement AggregatedDiscoveryServiceServer and handle StreamAggregatedResources

dfawley · 2025-02-06T20:39:56Z

+	"time"
+
+	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/internal/grpctest"


We should avoid using anything from grpc/internal. That includes grpctest and e2e. We won't be able to move this out like this.

yeah intend it remove. Regarding grpctest, should we just remove subtests for now or later? Regarding e2e, as i mentioned I think we need to add grpc enabeld xds server.

I'm fine with just removing the grpctest magic for now.

i think we can keep it because in the next PR I am going to copy it over to clients anyways.

OK that's fine then.

dfawley · 2025-02-25T21:11:44Z

+	"time"
+
+	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/internal/grpctest"


I'm fine with just removing the grpctest magic for now.

dfawley · 2025-02-25T21:13:10Z

+	v3discoverygrpc.RegisterAggregatedDiscoveryServiceServer(s, ts)
+	go func() {
+		if err := s.Serve(lis); err != nil {
+			t.Logf("Server exited with error: %v", err)


t.Errorf?

Note that this line races with the test case exiting entirely and t becoming invalid. So if this is not just go s.Serve(lis) then we probably should have a waitgroup that the Cleanup function blocks on after calling stop.

Changed to go s.Serve(lis)

dfawley · 2025-02-27T18:34:31Z

+	"time"
+
+	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/internal/grpctest"


OK that's fine then.

dfawley · 2025-02-27T18:35:44Z

You might need to push an empty commit to make the new test case run. Let me know if it's not working and I can force the merge.

easwars · 2025-02-27T23:05:34Z

+
+// NewStream creates a new gRPC stream to the server for the specified method.
+func (g *grpcTransport) NewStream(ctx context.Context, method string) (clients.Stream, error) {
+	s, err := g.cc.NewStream(ctx, &grpc.StreamDesc{ClientStreams: true, ServerStreams: true}, method)


This means that this transport only support bidirectional streams. It doesn't matter because both ADS and LRS are bidirectional streams. But I'm wondering if this should be documented?

Right now I don't see it needs to be mention. Since, both interface and this implementation mentions that its for xDS and LRS servers, its obvious that it has to be bidi stream. But, we can see later after complete implementation, if its good to mention.

easwars · 2025-02-27T23:08:01Z

+	if b, ok := v.([]byte); ok {
+		return b, nil
+	}
+	return nil, fmt.Errorf("message is %T, but must be a []byte", v)


Nit: Adding a package prefix to this error message would also be useful I think. So, maybe something like:
fmt.Errorf("grpctransport: attempting to send message of type %T, but must be a []byte", v)
and similarly for the error returned by the Unmarshal method as well.

easwars · 2025-02-27T23:10:13Z

+		if err != nil {
+			return err // Handle other errors
+		}
+		s.requestChan <- req


Might be minor. But, if this method is blocked writing to the channel, canceling the stream context will not unblock the server handler. So, we might want to use a select here with one case for the stream context getting done.

thanks. Yeah changed to select with ctx.Done()

easwars · 2025-02-27T23:12:21Z

+	_, err = transport.NewStream(ctx, "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources")
+	if err != nil {


Nit: same line.

easwars · 2025-02-27T23:12:31Z

+	_, err = transport.NewStream(ctx, "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources")
+	if err == nil {


Nit: same here.

easwars · 2025-02-27T23:14:05Z

+	// sent.
+	select {
+	case gotReq := <-ts.requestChan:
+		if diff := cmp.Diff(gotReq, testDiscoverRequest, protocmp.Transform()); diff != "" {


I think you got the order wrong in the call to cmp.Diff based on what is used in the error message. See: go/go-style/decisions#types-of-equality

i had it want, got before then i think it got changed to want, got. I think want, got is correct way. Changed.

easwars · 2025-02-27T23:15:12Z

+	// server.
+	var gotRes v3discoverypb.DiscoveryResponse
+	if err := proto.Unmarshal(res, &gotRes); err != nil {
+		t.Fatalf("Failed to unmarshal response from ts.requestChan to DiscoveryResponse: %v", err)


Nit: s/ts.requestChan/server/ ?

easwars · 2025-02-27T23:15:36Z

+		t.Fatalf("Failed to unmarshal response from ts.requestChan to DiscoveryResponse: %v", err)
+	}
+	if diff := cmp.Diff(&gotRes, ts.response, protocmp.Transform()); diff != "" {
+		t.Fatalf("proto.Unmarshal(res, &gotRes) returned unexpected diff (-want +got):\n%s", diff)


Same here, about the order or arguments to cmp.Diff and the order of want and got in the error message.

i had it want, got before then i think it got changed to want, got. I think want, got is correct way. Changed.

cds: stop child policies on resource-not-found errors (grpc#8122) xds: simplify code handling certain error conditions in the resolver (grpc#8123) xds, pickfirst: Enable additional addresses in xDS, set new pick_first as default (grpc#8126) github: change test action to cover the legacy pickfirst balancer (grpc#8129) cleanup: replace dial with newclient (grpc#7967) cleanup: replace dial with newclient (grpc#7970) stats/openetelemetry: refactor and make e2e test stats verification deterministic (grpc#8077) xds: introduce simple grpc transport for generic xds clients (grpc#8066) xds: generic xds client common configs re-push comments improve ServerConfig equal easwar review round 1 on documentation easwar comments on docstrings easwar comments round 4 config tests merge with previous pr xds: add lrs client and xDS client interfaces second pass to documentation language change from godoc review dfawley review 2 easwar review 1 changed to decoder struct move authorities under xds client easwar review 2 easwars review 4 ResourceWatcher done and LoadStore stop grpc based transport remove server config extension interface add byte codec dfawley review 1 send and recv tests with byte based test server change to proto based server easwar review 1 easwar review 3 xds: generic xds client ads transport channel

ads: stop child policies on resource-not-found errors (grpc#8122) xds: simplify code handling certain error conditions in the resolver (grpc#8123) xds, pickfirst: Enable additional addresses in xDS, set new pick_first as default (grpc#8126) github: change test action to cover the legacy pickfirst balancer (grpc#8129) cleanup: replace dial with newclient (grpc#7967) cleanup: replace dial with newclient (grpc#7970) stats/openetelemetry: refactor and make e2e test stats verification deterministic (grpc#8077) xds: introduce simple grpc transport for generic xds clients (grpc#8066) xds: generic xds client common configs re-push comments improve ServerConfig equal easwar review round 1 on documentation easwar comments on docstrings easwar comments round 4 config tests merge with previous pr xds: add lrs client and xDS client interfaces second pass to documentation language change from godoc review dfawley review 2 easwar review 1 changed to decoder struct move authorities under xds client easwar review 2 easwars review 4 ResourceWatcher done and LoadStore stop grpc based transport remove server config extension interface add byte codec dfawley review 1 send and recv tests with byte based test server change to proto based server easwar review 1 easwar review 3 xds: generic xds client ads transport channel

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

purnesh42H added this to the 1.71 Release milestone Feb 5, 2025

purnesh42H force-pushed the generic-xds-client-grpc-transport branch from ad889b4 to 377a648 Compare February 5, 2025 06:06

purnesh42H requested a review from dfawley February 5, 2025 06:07

purnesh42H assigned dfawley Feb 5, 2025

purnesh42H force-pushed the generic-xds-client-grpc-transport branch 4 times, most recently from fcbf470 to efeddd1 Compare February 5, 2025 09:24

purnesh42H commented Feb 5, 2025

View reviewed changes

Comment thread xds/internal/clients/grpctransport/grpc_transport.go

purnesh42H force-pushed the generic-xds-client-grpc-transport branch 6 times, most recently from a6648d7 to c63118f Compare February 6, 2025 19:31

dfawley reviewed Feb 6, 2025

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Feb 6, 2025

purnesh42H force-pushed the generic-xds-client-grpc-transport branch from e892817 to 5db47df Compare February 7, 2025 11:41

purnesh42H requested a review from dfawley February 7, 2025 11:48

purnesh42H force-pushed the generic-xds-client-grpc-transport branch 5 times, most recently from 4d47a17 to 4ca2a7c Compare February 10, 2025 05:48

purnesh42H assigned dfawley and unassigned purnesh42H Feb 10, 2025

easwars removed their assignment Feb 21, 2025

easwar review 3

5050360

purnesh42H force-pushed the generic-xds-client-grpc-transport branch from 2077260 to 5050360 Compare February 21, 2025 08:52

purnesh42H requested a review from easwars February 21, 2025 08:56

purnesh42H assigned easwars and dfawley and unassigned purnesh42H Feb 21, 2025

dfawley reviewed Feb 25, 2025

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Feb 25, 2025

doug review 3

f7be854

purnesh42H requested a review from dfawley February 26, 2025 05:19

purnesh42H assigned dfawley and unassigned purnesh42H Feb 26, 2025

dfawley approved these changes Feb 27, 2025

View reviewed changes

dfawley assigned purnesh42H and unassigned easwars and dfawley Feb 27, 2025

easwars reviewed Feb 27, 2025

View reviewed changes

easwar nits

b175bc0

purnesh42H force-pushed the generic-xds-client-grpc-transport branch from d9c84b8 to b175bc0 Compare February 28, 2025 09:03

purnesh42H merged commit 7505bf2 into grpc:master Feb 28, 2025

purnesh42H mentioned this pull request Mar 6, 2025

xds: generic xDS client transport channel and ads stream implementation #8144

Merged

purnesh42H mentioned this pull request Mar 19, 2025

xds: generic xds client resource watching e2e #8183

Merged

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Mar 24, 2025

xds: introduce simple grpc transport for generic xds clients (grpc#8066)

453cce6

purnesh42H mentioned this pull request May 13, 2025

xds: Introduce new generic xDS and LRS clients decoupled from gRPC-specific elements and migrate internal xDS and LRS functionality to use new generic clients #8313

Closed

10 tasks

github-actions Bot locked as resolved and limited conversation to collaborators Aug 28, 2025

		_, err = transport.NewStream(ctx, "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources")
		if err != nil {

		_, err = transport.NewStream(ctx, "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources")
		if err == nil {

Uh oh!

Conversation

purnesh42H commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfawley commented Feb 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

purnesh42H commented Feb 5, 2025 •

edited

Loading

codecov Bot commented Feb 5, 2025 •

edited

Loading