From d9d8f342b7ee5b405e3c97fb51cda75643ce7cf1 Mon Sep 17 00:00:00 2001 From: eshitachandwani <59800922+eshitachandwani@users.noreply.github.com> Date: Wed, 9 Oct 2024 13:29:31 +0530 Subject: [PATCH] revert xds: return all ServerConfig dial options together (#7712) * revert xds: return all ServerConfig dial options together * revert - xdsclient: fix test build breakage --- internal/xds/bootstrap/bootstrap.go | 22 ++++++++----- xds/internal/xdsclient/transport/transport.go | 21 +++++++----- .../xdsclient/transport/transport_test.go | 32 +++++-------------- 3 files changed, 35 insertions(+), 40 deletions(-) diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go index 35aeea701a92..c725bc1eac97 100644 --- a/internal/xds/bootstrap/bootstrap.go +++ b/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 diff --git a/xds/internal/xdsclient/transport/transport.go b/xds/internal/xdsclient/transport/transport.go index 59b221727a1f..134a9519f19f 100644 --- a/xds/internal/xdsclient/transport/transport.go +++ b/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 { diff --git a/xds/internal/xdsclient/transport/transport_test.go b/xds/internal/xdsclient/transport/transport_test.go index b217fd7430d9..7aac0ccdbb8b 100644 --- a/xds/internal/xdsclient/transport/transport_test.go +++ b/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,8 +140,7 @@ 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}}, @@ -161,7 +148,9 @@ 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, @@ -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()