xds: return all ServerConfig dial options together (grpc#7680)

Author: eshitachandwani

internal/xds/bootstrap/bootstrap.go

@@ -220,20 +220,14 @@ func (sc *ServerConfig) ServerFeaturesIgnoreResourceDeletion() bool {
 	return false
 }
 
-// CredsDialOption returns the first supported transport credentials from the
-// configuration, as a dial option.
-func (sc *ServerConfig) CredsDialOption() grpc.DialOption {
-	return sc.credsDialOption
-}
-
-// DialerOption returns the Dialer function that specifies how to dial the xDS
-// server determined by the first supported credentials from the configuration,
-// as a dial option.
-//
-// TODO(https://github.com/grpc/grpc-go/issues/7661): change ServerConfig type
-// to have a single method that returns all configured dial options.
-func (sc *ServerConfig) DialerOption() grpc.DialOption {
-	return sc.dialerOption
+// DialOptions returns a slice of all the configured dial options for this
+// server.
+func (sc *ServerConfig) DialOptions() []grpc.DialOption {
+	dopts := []grpc.DialOption{sc.credsDialOption}
+	if sc.dialerOption != nil {
+		dopts = append(dopts, sc.dialerOption)
+	}
+	return dopts
 }
 
 // Cleanups returns a collection of functions to be called when the xDS client

xds/internal/xdsclient/transport/transport.go

@@ -192,19 +192,14 @@ func New(opts Options) (*Transport, error) {
 		return nil, errors.New("missing OnSend callback handler when creating a new transport")
 	}
 
-	// Dial the xDS management with the passed in credentials.
-	dopts := []grpc.DialOption{
-		opts.ServerCfg.CredsDialOption(),
-		grpc.WithKeepaliveParams(keepalive.ClientParameters{
-			// We decided to use these sane defaults in all languages, and
-			// kicked the can down the road as far making these configurable.
-			Time:    5 * time.Minute,
-			Timeout: 20 * time.Second,
-		}),
-	}
-	if dialerOpts := opts.ServerCfg.DialerOption(); dialerOpts != nil {
-		dopts = append(dopts, dialerOpts)
-	}
+	// Dial the xDS management server with dial options specified by the server
+	// configuration and a static keepalive configuration that is common across
+	// gRPC language implementations.
+	kpCfg := grpc.WithKeepaliveParams(keepalive.ClientParameters{
+		Time:    5 * time.Minute,
+		Timeout: 20 * time.Second,
+	})
+	dopts := append([]grpc.DialOption{kpCfg}, opts.ServerCfg.DialOptions()...)
 	grpcNewClient := transportinternal.GRPCNewClient.(func(string, ...grpc.DialOption) (*grpc.ClientConn, error))
 	cc, err := grpcNewClient(opts.ServerCfg.ServerURI(), dopts...)
 	if err != nil {

xds/internal/xdsclient/transport/transport_test.go

@@ -22,6 +22,7 @@ import (
 	"encoding/json"
 	"net"
 	"testing"
+	"time"
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
@@ -108,10 +109,17 @@ const testDialerCredsBuilderName = "test_dialer_creds"
 // testDialerCredsBuilder implements the `Credentials` interface defined in
 // package `xds/bootstrap` and encapsulates an insecure credential with a
 // custom Dialer that specifies how to dial the xDS server.
-type testDialerCredsBuilder struct{}
+type testDialerCredsBuilder struct {
+	// Closed with the custom Dialer is invoked.
+	// Needs to be passed in by the test.
+	dialCalled chan struct{}
+}
 
 func (t *testDialerCredsBuilder) Build(json.RawMessage) (credentials.Bundle, func(), error) {
-	return &testDialerCredsBundle{insecure.NewBundle()}, func() {}, nil
+	return &testDialerCredsBundle{
+		Bundle:     insecure.NewBundle(),
+		dialCalled: t.dialCalled,
+	}, func() {}, nil
 }
 
 func (t *testDialerCredsBuilder) Name() string {
@@ -123,10 +131,12 @@ func (t *testDialerCredsBuilder) Name() string {
 // that specifies how to dial the xDS server.
 type testDialerCredsBundle struct {
 	credentials.Bundle
+	dialCalled chan struct{}
 }
 
-func (t *testDialerCredsBundle) Dialer(context.Context, string) (net.Conn, error) {
-	return nil, nil
+func (t *testDialerCredsBundle) Dialer(_ context.Context, address string) (net.Conn, error) {
+	close(t.dialCalled)
+	return net.Dial("tcp", address)
 }
 
 func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {
@@ -140,17 +150,16 @@ func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {
 	internal.GRPCNewClient = customGRPCNewClient
 	defer func() { internal.GRPCNewClient = oldGRPCNewClient }()
 
-	bootstrap.RegisterCredentials(&testDialerCredsBuilder{})
+	dialCalled := make(chan struct{})
+	bootstrap.RegisterCredentials(&testDialerCredsBuilder{dialCalled: dialCalled})
 	serverCfg, err := internalbootstrap.ServerConfigForTesting(internalbootstrap.ServerConfigTestingOptions{
 		URI:          "trafficdirector.googleapis.com:443",
 		ChannelCreds: []internalbootstrap.ChannelCreds{{Type: testDialerCredsBuilderName}},
 	})
 	if err != nil {
 		t.Fatalf("Failed to create server config for testing: %v", err)
 	}
-	if serverCfg.DialerOption() == nil {
-		t.Fatalf("Dialer for xDS transport in server config for testing is nil, want non-nil")
-	}
+
 	// Create a new transport.
 	opts := transport.Options{
 		ServerCfg: serverCfg,
@@ -171,6 +180,11 @@ func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {
 	if err != nil {
 		t.Fatalf("transport.New(%v) failed: %v", opts, err)
 	}
+	select {
+	case <-dialCalled:
+	case <-time.After(defaultTestTimeout):
+		t.Fatal("Timeout when waiting for Dialer() to be invoked")
+	}
 	// Verify there are three dial options passed to the custom grpc.NewClient.
 	// The first is opts.ServerCfg.CredsDialOption(), the second is
 	// grpc.WithKeepaliveParams(), and the third is opts.ServerCfg.DialerOption()