diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go index c725bc1eac97..35aeea701a92 100644 --- a/internal/xds/bootstrap/bootstrap.go +++ b/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 diff --git a/xds/internal/xdsclient/transport/transport.go b/xds/internal/xdsclient/transport/transport.go index 134a9519f19f..59b221727a1f 100644 --- a/xds/internal/xdsclient/transport/transport.go +++ b/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 { diff --git a/xds/internal/xdsclient/transport/transport_test.go b/xds/internal/xdsclient/transport/transport_test.go index 6c2c1f2835e2..247bb2a52af5 100644 --- a/xds/internal/xdsclient/transport/transport_test.go +++ b/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" @@ -94,10 +95,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 { @@ -109,10 +117,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) { @@ -126,7 +136,8 @@ 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}}, @@ -134,9 +145,7 @@ func (s) TestNewWithDialerFromCredentialsBundle(t *testing.T) { 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, @@ -157,6 +166,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()