xds: return all ServerConfig dial options together (#7718)

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

@@ -18,17 +18,11 @@
 package transport_test
 
 import (
-	"context"
-	"encoding/json"
-	"net"
 	"testing"
 
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/credentials"
-	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/grpc/internal/grpctest"
 	internalbootstrap "google.golang.org/grpc/internal/xds/bootstrap"
-	"google.golang.org/grpc/xds/bootstrap"
 	"google.golang.org/grpc/xds/internal/xdsclient/transport"
 	"google.golang.org/grpc/xds/internal/xdsclient/transport/internal"
 
@@ -102,80 +96,3 @@ func (s) TestNewWithGRPCDial(t *testing.T) {
 		t.Fatalf("transport.New(%+v) custom dialer called = true, want false", opts)
 	}
 }
-
-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{}
-
-func (t *testDialerCredsBuilder) Build(json.RawMessage) (credentials.Bundle, func(), error) {
-	return &testDialerCredsBundle{insecure.NewBundle()}, func() {}, nil
-}
-
-func (t *testDialerCredsBuilder) Name() string {
-	return testDialerCredsBuilderName
-}
-
-// 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 {
-	credentials.Bundle
-}
-
-func (t *testDialerCredsBundle) Dialer(context.Context, string) (net.Conn, error) {
-	return nil, nil
-}
-
-func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {
-	// Override grpc.NewClient with a custom one.
-	doptsLen := 0
-	customGRPCNewClient := func(target string, opts ...grpc.DialOption) (*grpc.ClientConn, error) {
-		doptsLen = len(opts)
-		return grpc.NewClient(target, opts...)
-	}
-	oldGRPCNewClient := internal.GRPCNewClient
-	internal.GRPCNewClient = customGRPCNewClient
-	defer func() { internal.GRPCNewClient = oldGRPCNewClient }()
-
-	bootstrap.RegisterCredentials(&testDialerCredsBuilder{})
-	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,
-		NodeProto: &v3corepb.Node{},
-		OnRecvHandler: func(update transport.ResourceUpdate, onDone func()) error {
-			onDone()
-			return nil
-		},
-		OnErrorHandler: func(error) {},
-		OnSendHandler:  func(*transport.ResourceSendInfo) {},
-	}
-	c, err := transport.New(opts)
-	defer func() {
-		if c != nil {
-			c.Close()
-		}
-	}()
-	if err != nil {
-		t.Fatalf("transport.New(%v) failed: %v", opts, err)
-	}
-	// 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()
-	// from the credentials bundle.
-	if doptsLen != 3 {
-		t.Fatalf("transport.New(%v) custom grpc.NewClient called with %d dial options, want 3", opts, doptsLen)
-	}
-}