From f17ea7d68c0942d8160e77329cc3814b0dd2b64f Mon Sep 17 00:00:00 2001 From: eshitachandwani <59800922+eshitachandwani@users.noreply.github.com> Date: Tue, 8 Oct 2024 21:37:02 +0530 Subject: [PATCH] xds: return all ServerConfig dial options together (#7680) --- internal/xds/bootstrap/bootstrap.go | 22 +++++--------- xds/internal/xdsclient/transport/transport.go | 21 +++++-------- .../xdsclient/transport/transport_test.go | 30 ++++++++++++++----- 3 files changed, 38 insertions(+), 35 deletions(-) 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 7aac0ccdbb8b..dd808df47cc5 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" @@ -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,7 +150,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}}, @@ -148,9 +159,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, @@ -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()