revert xds: return all ServerConfig dial options together (#7712)

* revert xds: return all ServerConfig dial options together

* revert - xdsclient: fix test build breakage

Author: eshitachandwani

internal/xds/bootstrap/bootstrap.go

@@ -220,14 +220,20 @@ func (sc *ServerConfig) ServerFeaturesIgnoreResourceDeletion() bool {
 	return false
 }
 
-// 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
+// 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
 }
 
 // Cleanups returns a collection of functions to be called when the xDS client

xds/internal/xdsclient/transport/transport.go

@@ -192,14 +192,19 @@ func New(opts Options) (*Transport, error) {
 		return nil, errors.New("missing OnSend callback handler when creating a new transport")
 	}
 
-	// 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()...)
+	// 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)
+	}
 	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,7 +22,6 @@ import (
 	"encoding/json"
 	"net"
 	"testing"
-	"time"
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
@@ -44,8 +43,6 @@ func Test(t *testing.T) {
 	grpctest.RunSubTests(t, s{})
 }
 
-const defaultTestTimeout = 10 * time.Second
-
 var noopRecvHandler = func(_ transport.ResourceUpdate, onDone func()) error {
 	onDone()
 	return nil
@@ -111,17 +108,10 @@ 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 {
-	// Closed with the custom Dialer is invoked.
-	// Needs to be passed in by the test.
-	dialCalled chan struct{}
-}
+type testDialerCredsBuilder struct{}
 
 func (t *testDialerCredsBuilder) Build(json.RawMessage) (credentials.Bundle, func(), error) {
-	return &testDialerCredsBundle{
-		Bundle:     insecure.NewBundle(),
-		dialCalled: t.dialCalled,
-	}, func() {}, nil
+	return &testDialerCredsBundle{insecure.NewBundle()}, func() {}, nil
 }
 
 func (t *testDialerCredsBuilder) Name() string {
@@ -133,12 +123,10 @@ 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, address string) (net.Conn, error) {
-	close(t.dialCalled)
-	return net.Dial("tcp", address)
+func (t *testDialerCredsBundle) Dialer(context.Context, string) (net.Conn, error) {
+	return nil, nil
 }
 
 func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {
@@ -152,16 +140,17 @@ func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) {
 	internal.GRPCNewClient = customGRPCNewClient
 	defer func() { internal.GRPCNewClient = oldGRPCNewClient }()
 
-	dialCalled := make(chan struct{})
-	bootstrap.RegisterCredentials(&testDialerCredsBuilder{dialCalled: dialCalled})
+	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,
@@ -182,11 +171,6 @@ 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()