xds: add xDS transport custom Dialer support #7586

danielzhaotongliu · 2024-09-03T18:29:36Z

Design:

go/grpc-go-corp-to-prod-td-doc
Add a custom Dialer method that specifies how to dial the xDS server via the credentials bundle without introducing new public APIs.
No code change for existing users that do not need a custom Dialer for the xDS transport.

Tested:

./scripts/vet.sh
go test -cpu 1,4 -timeout 7m ./...
go test -race -cpu 1,4 -timeout 7m ./...
Verified end-to-end by running a test gRPC client with a xDS client that used the custom Dialer for the xDS transport to successfully communicate with the xDS server and fetch configs.

RELEASE NOTES:

xds: add experimental feature to customize the Dialer used by the xDS transport

codecov · 2024-09-03T18:33:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.87%. Comparing base (5666049) to head (14cbf57).
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7586      +/-   ##
==========================================
+ Coverage   81.74%   81.87%   +0.12%     
==========================================
  Files         361      361              
  Lines       27813    27822       +9     
==========================================
+ Hits        22736    22778      +42     
+ Misses       3868     3849      -19     
+ Partials     1209     1195      -14

Files with missing lines	Coverage Δ
internal/xds/bootstrap/bootstrap.go	`71.47% <100.00%> (+0.99%)`	⬆️
xds/internal/xdsclient/transport/transport.go	`93.30% <100.00%> (+0.05%)`	⬆️

... and 41 files with indirect coverage changes

dfawley

Could you add a test that works at an even higher level? I.e. registers the creds builder from outside of the xdsclient package and confirms that a grpc client created using grpc.NewClient will invoke the proper dialer?

EDIT: finished comments; oops!

internal/xds/bootstrap/bootstrap.go

xds/internal/xdsclient/transport/transport.go

xds/internal/xdsclient/transport/transport_test.go

dfawley · 2024-09-09T17:20:35Z

(Sorry, hit send too soon; updated comment above.)

danielzhaotongliu · 2024-09-12T23:36:27Z

Could you add a test that works at an even higher level? I.e. registers the creds builder from outside of the xdsclient package and confirms that a grpc client created using grpc.NewClient will invoke the proper dialer?

Done.
In latest commit, I added a new e2e test test/xds/xds_client_custom_dialer_test.go that registers a creds builder (creds bundle with a custom Dialer method that simply acts as a pass-through), sets up a xDS management server, creates a new xDS bootstrap that specifies the creds builder name as part of the channel_creds, creates a xDS resolver that uses the bootstrap, creates a test backend, creates a ClientConn via grpc.NewClient, make a successful RPC call, finally verify the custom Dialer has been called. This works end-to-end as the custom Dialer is just a pass-through.

The test setup is inspired by other tests under test/xds/.

dfawley

LGTM aside from the two small comments -- thanks!

dfawley · 2024-09-13T15:29:48Z

test/xds/xds_client_custom_dialer_test.go

+	defer cc.Close()
+
+	client := testgrpc.NewTestServiceClient(cc)
+	if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(true)); err != nil {


I don't think (?) this should require WaitForReady -- we shouldn't be expecting connections to fail as we start the server before the client.

Done.
Removed grpc.WaitForReady(true) call option. It makes sense as the server is indeed started before the client.
Tested with thread sanitizer and ran test 100 times and ensured there are no flakes.

dfawley · 2024-09-13T15:36:52Z

test/xds/xds_client_custom_dialer_test.go

+}
+
+func (s) TestClientCustomDialerFromCredentialsBundle(t *testing.T) {
+	customDialerCalled = false


Can this be a field in the creds bundle instead of a global? I think we should be able to do the RegisterCredentials here instead of in an init.

Same for the mgmtServerAddress global. Or if you want to be a little fancy, it can be passed through the bootstrap config, and testDialerCredsBuilder.Build can parse it and pass it to the testDialerCredsBundle that it builds.

And the testDialerCredsBuilder needs to have a channel on which it sends the testDialerCredsBundle that it creates. That way, the test can get access to the creds bundle and can check if the custom dialer was called.

Done.
Removed all global variables (customDialerCalled and mgmtServerAddress) in this test.

The mgmtServerAddress is passed through the bootstrap config where testDialerCredsBuilder.Build unmarshals the JSON config and passes it to the testDialerCredsBundle.

Added a chan struct{} to testDialerCredsBuilder and testDialerCredsBundle such that the test can check whether the custom dialer has been called. Followed Go Tip 79 for one-time signaling and simply used close(chan) along with select statements. Furthermore, added a case for ctx.Done() in the select to gracefully handle the case if the dial failed due to timeout.

Called bootstrap.RegisterCredentials() within this test and removed init().

dfawley · 2024-09-13T15:38:50Z

+@easwars for a second review

easwars · 2024-09-13T21:24:09Z

internal/xds/bootstrap/bootstrap.go

+// DialerOption returns the first supported Dialer function that specifies how
+// to dial the xDS server from the configuration, as a dial option.


Can this comment be rephrased to indicate that it is actually the first supported credentials that determines this dialer function. There is really no such thing as the first supported dialer function.

Done.
Rephrased comment to include that this Dialer function is determined by the first supported credentials from the configuration.

easwars · 2024-09-13T21:28:48Z

internal/xds/bootstrap/bootstrap.go

+// dialer captures the Dialer method specified via the credentials bundle.
+type dialer interface {
+	// Dialer specifies how to dial the xDS server.
+	Dialer(context.Context, string) (net.Conn, error)


Can this method name be more specific? Maybe something like DialXDS or DialXDSServer or something else?

The reason I'm suggesting this is because the API to register a credentials type is public. So, users could register their own credentials using bootstrap.RegisterCredentials, and if they end up having a Dialer method on their creds implementation type (which is maybe doing something else), then, they would be in for a surprise.

Or at least, we should document this in the public API in package xds/bootstrap?

Thoughts @dfawley

Acknowledged.
Users could definitely have a the Dialer method on their creds bundle implementation and it is a valid concern. We have no idea or control of what users could implement. Using another "less common" name compared to Dialer would greatly reduce this risk.

I think DialXDS would be appropriate and not too verbose, although this would introduce slight repetition as the XDS suffix is already implied (per the Repetition section of the Google Go Style Decisions guide).

What are your thoughts @dfawley? If everyone is in agreement, then I will make the change.

easwars · 2024-09-13T21:30:56Z

test/xds/xds_client_custom_dialer_test.go

+}
+
+func (s) TestClientCustomDialerFromCredentialsBundle(t *testing.T) {
+	customDialerCalled = false


Same for the mgmtServerAddress global. Or if you want to be a little fancy, it can be passed through the bootstrap config, and testDialerCredsBuilder.Build can parse it and pass it to the testDialerCredsBundle that it builds.

easwars · 2024-09-13T21:33:56Z

test/xds/xds_client_custom_dialer_test.go

+}
+
+func (s) TestClientCustomDialerFromCredentialsBundle(t *testing.T) {
+	customDialerCalled = false


And the testDialerCredsBuilder needs to have a channel on which it sends the testDialerCredsBundle that it creates. That way, the test can get access to the creds bundle and can check if the custom dialer was called.

easwars · 2024-09-13T21:36:15Z

internal/xds/bootstrap/bootstrap.go

+func (sc *ServerConfig) DialerOption() grpc.DialOption {
+	return sc.dialerOption
+}


Hmm ... I'm thinking if we should just have a method to return a slice of dial options from this type:

func (sc *ServerConfig) DialOptions() []grpc.DialOption { // return a slice of dial options that contains the creds dial option and optionally one for the dialer }

With this approach, the transport can be oblivious to the dial options, and tomorrow if another dial option needs to be added here, then the transport won't have to change.

Acknowledged.
I think this is a good abstraction, although it might slightly reduce readability and intent of what each dial option does (only two dial options passed to the transport).

I think this abstraction would be appropriate in a follow-up PR as it would involve removing the existing func (sc *ServerConfig) CredsDialOption(), introduce the new API above, and refactoring the transport, which is unrelated to the current PR of adding a custom Dialer used by the xDS transport and it would reduce this PR's focus/intent/readability.

easwars · 2024-09-13T21:39:49Z

xds/internal/xdsclient/transport/transport_test.go

+// testDialerCredsBundle implements the `Bundle` interface defined in package
+// `credentials` and encapsulates an insecure credential with a custom Dialer
+// that specifies how to dial the xDS server.
+type testDialerCredsBundle struct{}


Would embedding an credentials.Bundle here and setting it to insecure.NewBundle from testDialerCredsBuilder.Build, help to get rid of the implementation of the three methods below?

Done.
Great find and suggestion!
Embedded credentials.Bundle for the struct in both transport_test.go and xds_client_custom_dialer_test.go.
This follows from Go Tip 7 and eliminated unnecessary boilerplate methods and improved readability.

easwars · 2024-09-13T21:42:20Z

xds/internal/xdsclient/transport/transport_test.go

+	return nil, nil
+}
+
+func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {


I don't think this really tests whether the transport is actually passing the new dial option to grpc.Dial. It is not possible to check the exact dial options being passed because dial options are implemented as functions, and it is not possible to compare them. But at least, we should check if the call to grpc.Dial or grpc.NewClient from the transport passes the expected number of dial options.

Done.
Updated this test such that initially it overrides internal.GRPCNewClient with a custom pass-through grpc.NewClient (then resets back to the original grpc.NewClient as to not have order-dependent tests) that gets the number of dial options. Later the test verifies the number of dial options passed to the custom grpc.NewClient is 3. Also, added a comment explaining why it is 3.

…thod

…ent for transport test

Dialer method via credentials bundle

354e088

danielzhaotongliu force-pushed the xds-transport-dialer-support branch from 99a59fc to 354e088 Compare September 6, 2024 23:30

danielzhaotongliu added 2 commits September 7, 2024 00:06

tests for xDS transport custom Dialer via credentials bundle

b5d173c

fix go vet style errors

d187d39

danielzhaotongliu marked this pull request as ready for review September 9, 2024 16:49

dfawley reviewed Sep 9, 2024

View reviewed changes

internal/xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

xds/internal/xdsclient/transport/transport.go Outdated Show resolved Hide resolved

xds/internal/xdsclient/transport/transport_test.go Outdated Show resolved Hide resolved

dfawley assigned danielzhaotongliu Sep 9, 2024

dfawley added the Type: Feature New features or improvements in behavior label Sep 9, 2024

dfawley added this to the 1.67 Release milestone Sep 9, 2024

purnesh42H modified the milestones: 1.67 Release, 1.68 Release Sep 10, 2024

danielzhaotongliu added 2 commits September 11, 2024 20:13

address review p1: if-compound statement, readability

6a82444

address review p2: add higher level tests

ad20fd0

danielzhaotongliu requested a review from dfawley September 12, 2024 23:38

dfawley approved these changes Sep 13, 2024

View reviewed changes

dfawley requested a review from easwars September 13, 2024 15:38

dfawley assigned easwars Sep 13, 2024

easwars reviewed Sep 13, 2024

View reviewed changes

easwars added the Status: Requires Reporter Clarification label Sep 13, 2024

danielzhaotongliu added 4 commits September 17, 2024 23:52

address review p3: remove global test variables & WaitForReady

a8ff4fb

address review p4: rephrase comments for ServerConfig.DialerOption me…

3f564dd

…thod

address review p5: embed credentials.Bundle interface in tests

65feb3c

address review p6: check number of dial options passed to grpc.NewCli…

14cbf57

…ent for transport test

danielzhaotongliu requested a review from easwars September 18, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: add xDS transport custom Dialer support #7586

xds: add xDS transport custom Dialer support #7586

danielzhaotongliu commented Sep 3, 2024 •

edited by dfawley

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

dfawley left a comment •

edited

Loading

dfawley commented Sep 9, 2024

danielzhaotongliu commented Sep 12, 2024

dfawley left a comment

dfawley Sep 13, 2024

danielzhaotongliu Sep 18, 2024

dfawley Sep 13, 2024

easwars Sep 13, 2024

easwars Sep 13, 2024

danielzhaotongliu Sep 18, 2024

dfawley commented Sep 13, 2024

easwars Sep 13, 2024

danielzhaotongliu Sep 18, 2024

easwars Sep 13, 2024

danielzhaotongliu Sep 18, 2024

easwars Sep 13, 2024

easwars Sep 13, 2024

easwars Sep 13, 2024

danielzhaotongliu Sep 18, 2024

easwars Sep 13, 2024

danielzhaotongliu Sep 18, 2024 •

edited

Loading

easwars Sep 13, 2024

danielzhaotongliu Sep 18, 2024

		// DialerOption returns the first supported Dialer function that specifies how
		// to dial the xDS server from the configuration, as a dial option.

xds: add xDS transport custom Dialer support #7586

Are you sure you want to change the base?

xds: add xDS transport custom Dialer support #7586

Conversation

danielzhaotongliu commented Sep 3, 2024 • edited by dfawley Loading

codecov bot commented Sep 3, 2024 • edited Loading

Codecov Report

dfawley left a comment • edited Loading

Choose a reason for hiding this comment

dfawley commented Sep 9, 2024

danielzhaotongliu commented Sep 12, 2024

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

dfawley commented Sep 13, 2024

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

danielzhaotongliu Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielzhaotongliu commented Sep 3, 2024 •

edited by dfawley

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

dfawley left a comment •

edited

Loading

danielzhaotongliu Sep 18, 2024 •

edited

Loading