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.48%. Comparing base (c7db760) to head (5050360).
Report is 2 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.48%   +0.25%     
==========================================
  Files         387      388       +1     
  Lines       38947    38995      +48     
==========================================
+ Hits        32028    32165     +137     
+ Misses       5601     5532      -69     
+ Partials     1318     1298      -20

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

... and 30 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.

Upon thinking more about this, I'm not sure how I feel about the name that I suggested. This string will be part of every HEADERS frame, sent for every new stream being created. I'm wondering if we should at all be concerned about sending such a long name in the header, for performance reasons.

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.

xds/internal/clients/grpctransport/grpc_transport.go

xds/internal/clients/grpctransport/grpc_transport_test.go

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.

easwars · 2025-02-21T05:11:42Z

xds/internal/clients/grpctransport/grpc_transport.go

+// The Extension field of the ServerConfig must be a ServerConfigExtension.
+func (b *Builder) Build(sc clients.ServerConfig) (clients.Transport, error) {
+	if sc.ServerURI == "" {
+		return nil, fmt.Errorf("ServerConfig's ServerURI field cannot be empty")


go/go-style/decisions#error-strings

The style guide does say "Error strings should not be capitalized unless beginning with an exported name". But this error is happening deep inside the stack, and will definitely be wrapped in some other error. So, this capitalization will not look great when the final error is passed to the user.

Can we do better here?

It is customary for Go packages to prefix their errors with their package name. So, one option is to do that for all errors generated by this package. So, errors here can be fmt.Errorf("grpctransport: missing field ServerConfig.ServerURI"). Or if we take a leaf out of the tls package, they could be fmt.Errorf("grpctransport: ServerURI not set in ServerConfig")

Thanks. Modified all the error messages in format "grpctransport: ....not set in ....."

easwars · 2025-02-21T05:12:35Z

xds/internal/clients/grpctransport/grpc_transport.go

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


Can we identify the type of v in the error message?

easwars · 2025-02-21T05:12:46Z

xds/internal/clients/grpctransport/grpc_transport.go

+		*b = data
+		return nil
+	}
+	return fmt.Errorf("target must be a pointer to a byte slice")


Same here. Identify the offending type.

easwars · 2025-02-21T05:13:48Z

xds/internal/clients/grpctransport/grpc_transport.go

+	})
+	cc, err := grpc.NewClient(sc.ServerURI, kpCfg, grpc.WithCredentialsBundle(sce.Credentials), grpc.WithDefaultCallOptions(grpc.ForceCodec(&byteCodec{})))
+	if err != nil {
+		return nil, fmt.Errorf("error creating grpc client for server uri %s, %v", sc.ServerURI, err)


This error string could be grpctransport: failed to create transport to server %q: %v", sc.ServerURI, err

easwars · 2025-02-21T05:15:19Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+// setupTestServer set up the gRPC server for AggregatedDiscoveryService. It
+// creates an instance of testServer that returns the provided response from
+// the StreamAggregatedResources() handler and registers it with a gRPC server.
+func setupTestServer(t *testing.T, response *v3discoverypb.DiscoveryResponse) *testServer {


Please mark as a test helper. See: go/go-style/decisions#mark-test-helpers

easwars · 2025-02-21T05:21:18Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+			if (err != nil) != test.wantErr {
+				t.Fatalf("Build() error = %v, wantErr %v", err, test.wantErr)
+			}
+			if tr != nil {


Do you think we can simplify all this complex logic by splitting the success and failure cases into separate tests? See: https://g3doc.corp.google.com/company/teams/go-community/gotip/episodes/50.md?cl=head

Simplified. For success, also added check to verify if grpc.ClientConn is not nil.

easwars · 2025-02-21T05:21:53Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+}
+
+// TestBuild verifies that the grpctransport.Builder creates a new
+// grpc.ClientConn every time Build() is called.


every time Build() is called
I don't see this being verified or even exercised here.

Not applicable now.

easwars · 2025-02-21T05:25:42Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+	}
+}
+
+// TestStream_SendAndRecv verifies that grpcTransport.Stream.Send()


Minor nits. I don't think we have to identify function/method names with their package and type prefixes like grpcTransport.Stream.Send. Just Send and Recv should be good enough I feel.

Applies to many such comments across this file.

easwars · 2025-02-21T05:26:13Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+// on the stream to and from the gRPC server.
+//
+// It starts a gRPC test server using setupTestServer(). The test then sends a
+// testDiscoverRequest on the stream and verifies that the received discovery


Nit: s/received discovery/received discovery request/

easwars · 2025-02-21T05:26:56Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+			t.Fatalf("<-ts.requestChan = %v, want %v", &gotReq, &testDiscoverRequest)
+		}
+	case <-ctx.Done():
+		t.Fatalf("timeout waiting for request to reach server")


s/timeout/Timeout

easwars · 2025-02-21T05:27:25Z

xds/internal/clients/grpctransport/grpc_transport_test.go

+	select {
+	case gotReq := <-ts.requestChan:
+		if !cmp.Equal(gotReq, testDiscoverRequest, protocmp.Transform()) {
+			t.Fatalf("<-ts.requestChan = %v, want %v", &gotReq, &testDiscoverRequest)


Test error strings should be readable. <-ts.requestChan is not.

Thanks for catching. Modified to display diff.

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

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

purnesh42H assigned easwars Feb 18, 2025

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

purnesh42H assigned easwars and unassigned purnesh42H Feb 19, 2025

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

purnesh42H added 7 commits February 20, 2025 22:15

grpc based transport

ce4737e

remove server config extension interface

eb5e8bd

add byte codec

dfcd686

dfawley review 1

56b0d41

send and recv tests with byte based test server

61bfdda

change to proto based server

84b4f2b

easwar review 1

9049d97

purnesh42H force-pushed the generic-xds-client-grpc-transport branch from 3122cda to 9049d97 Compare February 20, 2025 18:04

easwars reviewed Feb 21, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars 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 unassigned purnesh42H Feb 21, 2025

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

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