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

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.40%. Comparing base (8528f43) to head (3122cda).
Report is 4 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.24%   82.40%   +0.15%     
==========================================
  Files         387      388       +1     
  Lines       38967    39000      +33     
==========================================
+ Hits        32048    32136      +88     
+ Misses       5599     5553      -46     
+ Partials     1320     1311       -9

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

... and 31 files with indirect coverage changes

xds/internal/clients/grpctransport/grpc_transport.go

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

xds/internal/clients/grpctransport/grpc_transport_test.go

+		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

xds/internal/clients/grpctransport/grpc_transport_test.go

+	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

xds/internal/clients/grpctransport/grpc_transport_test.go

+	"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.

easwars · 2025-02-18T22:55:26Z

xds/internal/clients/grpctransport/grpc_transport.go

+}
+
+func (c *byteCodec) Name() string {
+	return "byteCodec"


Should we use a more specific name to avoid possible name conflicts with a similar byte codec used by a user? Maybe something like "grpc.xds.internal.clients.grpctransport.byte_codec" that mirrors the package path?

Make sense. Changed.

easwars · 2025-02-18T23:03:03Z

xds/internal/clients/grpctransport/grpc_transport_test.go

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


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.

easwars · 2025-02-18T23:17:15Z

xds/internal/clients/grpctransport/grpc_transport.go

+}
+
+// Builder creates gRPC-based Transports. It must be paired with ServerConfigs
+// that contain its ServerConfigExtension.


Nit: that contain an extension of type ServerConfigExtension? or something similar to that?

easwars · 2025-02-18T23:19:03Z

xds/internal/clients/grpctransport/grpc_transport.go

+	if sc.Extensions == nil {
+		return nil, fmt.Errorf("ServerConfig's Extensions field cannot be nil for gRPC transport")
+	}
+	gtsce, ok := sc.Extensions.(ServerConfigExtension)


gtsce is not very readable. Maybe just sce might do?

easwars · 2025-02-18T23:20:30Z

xds/internal/clients/grpctransport/grpc_transport.go

+	// transport channel to same server can be shared between xDS and LRS
+	// client.
+
+	// Create a new gRPC client/channel for the xDS management server with the


Here also it can be an xDS server or an LRS server. So, I'd probably skip the xDS part.

easwars · 2025-02-18T23:21:28Z

xds/internal/clients/grpctransport/grpc_transport.go

+	err := s.stream.RecvMsg(&typedRes)
+	if err != nil {


Nit: Assignment and conditional in the same line.

easwars · 2025-02-18T23:22:27Z

xds/internal/clients/grpctransport/grpc_transport.go

+	return typedRes, nil
+}
+
+// Close closes all the gRPC streams to the server.


Maybe: Close closes the gRPC channel to the server?

easwars · 2025-02-18T23:36:00Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+		},
+		{
+			name:      "error",
+			serverURI: "invalid-server-uri",


Hmm.. ok, i see what you are doing. Maybe you can just split it into two separate tests, each with no table. So, in the second test, you wont even start a server and instead you will pass an invalid server uri to Build.

Separated into 2 different tests

easwars · 2025-02-18T23:37:04Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+	builder := Builder{}
+	transport, err := builder.Build(serverCfg)
+	if err != nil {
+		t.Fatalf("failed to build transport: %v", err)


Here and everywhere else, the error strings that are part of log messages and test failure messages need not obey the rules governing general error strings in Go. See: go/go-style/decisions#error-strings

easwars · 2025-02-18T23:38:05Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+		if !ok {
+			t.Fatalf("ts.requestChan is closed")
+		}


requestChan is never closed. I would get rid of this to simplify the test logic.

easwars · 2025-02-18T23:39:03Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+		if !ok {
+			t.Fatalf("ts.requestChan is closed")
+		}
+		if !cmp.Equal(testDiscoverRequest, gotReq, protocmp.Transform()) {


Please use the same order, "got before want" in your comparison (as well as the error string).

easwars · 2025-02-18T23:40:59Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+	if err := proto.Unmarshal(res, &gotRes); err != nil {
+		t.Fatalf("failed to unmarshal response from ts.requestChan to DiscoveryRequest: %v", err)
+	}
+	if !cmp.Equal(testDiscoverResponse, &gotRes, protocmp.Transform()) {


There is just way too much coupling between the test logic and the logic in the test server. This results in very brittle tests.

What is the eventual plan? Is it to use the go-control-plane as the management server here?

yeah, once we have xds client implemented, we will be able to use go-control-plane in e2e tests. For now, having the code coverage with simple grpc server is enough.

After last commit, response to be returned by StreamAggregatedResources handler can be controlled by test as it is a param in testServer. Regarding having test response as part of testServer struct, we can have implementation of StreamAggregatedResources within test inline so then we don't need response field in testServer.

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

xds/internal/clients/grpctransport/grpc_transport.go Show resolved Hide resolved

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

dfawley assigned purnesh42H and unassigned dfawley Feb 10, 2025

purnesh42H force-pushed the generic-xds-client-grpc-transport branch 2 times, most recently from f7acbb9 to 6d9631e Compare February 11, 2025 17:56

purnesh42H assigned dfawley and unassigned purnesh42H Feb 11, 2025

purnesh42H force-pushed the generic-xds-client-grpc-transport branch 2 times, most recently from 7f0b019 to 3bc9087 Compare February 17, 2025 16:50

purnesh42H requested a review from easwars February 18, 2025 04:39

purnesh42H assigned easwars Feb 18, 2025

purnesh42H added 6 commits February 18, 2025 12:42

grpc based transport

325b6f4

remove server config extension interface

123a8ce

add byte codec

2f1eba9

dfawley review 1

ff1c387

send and recv tests with byte based test server

bc98731

change to proto based server

f1abdf0

purnesh42H force-pushed the generic-xds-client-grpc-transport branch from 3bc9087 to f1abdf0 Compare February 18, 2025 07:12

easwars reviewed Feb 18, 2025

View reviewed changes

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

arjan-bal modified the milestones: 1.71 Release, 1.72 Release Feb 19, 2025

easwar review 1

3122cda

purnesh42H assigned easwars and unassigned purnesh42H Feb 19, 2025

purnesh42H requested a review from easwars February 19, 2025 19:33

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

Are you sure you want to change the base?

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

Conversation

purnesh42H commented Feb 5, 2025 • edited Loading

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

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

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

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

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

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 Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading