From b82a3723ec30f3126982856002ebb435295a29c3 Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Wed, 4 Nov 2020 18:31:43 -0800 Subject: [PATCH 1/7] Implemented fix --- clientconn.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clientconn.go b/clientconn.go index 11ed4af607a..5860c6072db 100644 --- a/clientconn.go +++ b/clientconn.go @@ -259,6 +259,8 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * cc.authority = cc.dopts.authority } else if strings.HasPrefix(cc.target, "unix:") { cc.authority = "localhost" + } else if strings.HasPrefix(cc.parsedTarget.Endpoint, ":") || cc.parsedTarget.Endpoint == "" { + cc.authority = "localhost" + cc.parsedTarget.Endpoint } else { // Use endpoint from "scheme://authority/endpoint" as the default // authority for ClientConn. From 16bcbdc4a114a6a8cdf8c9b21e55fe471065c779 Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Thu, 5 Nov 2020 12:39:28 -0800 Subject: [PATCH 2/7] removed empty auth fix --- clientconn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clientconn.go b/clientconn.go index 5860c6072db..c08d560a2a8 100644 --- a/clientconn.go +++ b/clientconn.go @@ -259,7 +259,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * cc.authority = cc.dopts.authority } else if strings.HasPrefix(cc.target, "unix:") { cc.authority = "localhost" - } else if strings.HasPrefix(cc.parsedTarget.Endpoint, ":") || cc.parsedTarget.Endpoint == "" { + } else if strings.HasPrefix(cc.parsedTarget.Endpoint, ":") { cc.authority = "localhost" + cc.parsedTarget.Endpoint } else { // Use endpoint from "scheme://authority/endpoint" as the default From 34b5d5c88466f34939f1b69bc067aed5775fca5e Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Wed, 11 Nov 2020 12:01:37 -0800 Subject: [PATCH 3/7] Added test for localhost:port authority. --- test/authority_test.go | 73 ++++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index 11c3c4c1af8..8fe25915c2f 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -33,44 +33,48 @@ import ( testpb "google.golang.org/grpc/test/grpc_testing" ) +func authorityChecker(expectedAuthority string) func(context.Context, *testpb.Empty) (*testpb.Empty, error) { + return func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return nil, status.Error(codes.InvalidArgument, "failed to parse metadata") + } + auths, ok := md[":authority"] + if !ok { + return nil, status.Error(codes.InvalidArgument, "no authority header") + } + if len(auths) < 1 { + return nil, status.Error(codes.InvalidArgument, "no authority header") + } + if auths[0] != expectedAuthority { + return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], expectedAuthority)) + } + return &testpb.Empty{}, nil + } +} + func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer func(context.Context, string) (net.Conn, error)) { if err := os.RemoveAll(address); err != nil { t.Fatalf("Error removing socket file %v: %v\n", address, err) } - us := &stubServer{ - emptyCall: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { - md, ok := metadata.FromIncomingContext(ctx) - if !ok { - return nil, status.Error(codes.InvalidArgument, "failed to parse metadata") - } - auths, ok := md[":authority"] - if !ok { - return nil, status.Error(codes.InvalidArgument, "no authority header") - } - if len(auths) < 1 { - return nil, status.Error(codes.InvalidArgument, "no authority header") - } - if auths[0] != expectedAuthority { - return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], expectedAuthority)) - } - return &testpb.Empty{}, nil - }, - network: "unix", - address: address, - target: target, + ss := &stubServer{ + emptyCall: authorityChecker(expectedAuthority), + network: "unix", + address: address, + target: target, } opts := []grpc.DialOption{} if dialer != nil { opts = append(opts, grpc.WithContextDialer(dialer)) } - if err := us.Start(nil, opts...); err != nil { + if err := ss.Start(nil, opts...); err != nil { t.Fatalf("Error starting endpoint server: %v", err) return } - defer us.Stop() + defer ss.Stop() ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - _, err := us.client.EmptyCall(ctx, &testpb.Empty{}) + _, err := ss.client.EmptyCall(ctx, &testpb.Empty{}) if err != nil { t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err) } @@ -147,3 +151,24 @@ func (s) TestUnixCustomDialer(t *testing.T) { }) } } + +func (s) TestColonPortAuthority(t *testing.T) { + port := ":50051" + ss := &stubServer{ + emptyCall: authorityChecker("localhost" + port), + network: "tcp", + address: "localhost" + port, + target: port, + } + if err := ss.Start(nil); err != nil { + t.Fatalf("Error starting endpoint server: %v", err) + return + } + defer ss.Stop() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _, err := ss.client.EmptyCall(ctx, &testpb.Empty{}) + if err != nil { + t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err) + } +} From e41f1aea4549cc0ad4ffb61f7fb6b7d27a7cff4b Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Wed, 11 Nov 2020 15:47:41 -0800 Subject: [PATCH 4/7] Refactored testing --- test/authority_test.go | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index 8fe25915c2f..886f2f0af52 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -23,6 +23,7 @@ import ( "fmt" "net" "os" + "strings" "testing" "time" @@ -33,7 +34,7 @@ import ( testpb "google.golang.org/grpc/test/grpc_testing" ) -func authorityChecker(expectedAuthority string) func(context.Context, *testpb.Empty) (*testpb.Empty, error) { +func authorityChecker(expectedAuthority *string) func(context.Context, *testpb.Empty) (*testpb.Empty, error) { return func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { md, ok := metadata.FromIncomingContext(ctx) if !ok { @@ -43,11 +44,11 @@ func authorityChecker(expectedAuthority string) func(context.Context, *testpb.Em if !ok { return nil, status.Error(codes.InvalidArgument, "no authority header") } - if len(auths) < 1 { - return nil, status.Error(codes.InvalidArgument, "no authority header") + if len(auths) != 1 { + return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("no authority header, auths = %v", auths)) } - if auths[0] != expectedAuthority { - return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], expectedAuthority)) + if auths[0] != *expectedAuthority { + return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], *expectedAuthority)) } return &testpb.Empty{}, nil } @@ -58,7 +59,7 @@ func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer t.Fatalf("Error removing socket file %v: %v\n", address, err) } ss := &stubServer{ - emptyCall: authorityChecker(expectedAuthority), + emptyCall: authorityChecker(&expectedAuthority), network: "unix", address: address, target: target, @@ -72,7 +73,7 @@ func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer return } defer ss.Stop() - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() _, err := ss.client.EmptyCall(ctx, &testpb.Empty{}) if err != nil { @@ -153,21 +154,33 @@ func (s) TestUnixCustomDialer(t *testing.T) { } func (s) TestColonPortAuthority(t *testing.T) { - port := ":50051" + expectedAuthority := "" ss := &stubServer{ - emptyCall: authorityChecker("localhost" + port), + emptyCall: authorityChecker(&expectedAuthority), network: "tcp", - address: "localhost" + port, - target: port, } if err := ss.Start(nil); err != nil { t.Fatalf("Error starting endpoint server: %v", err) return } defer ss.Stop() - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + s := strings.Split(ss.address, ":") + if len(s) != 2 { + t.Fatalf("No port in address: %v", ss.address) + return + } + expectedAuthority = "localhost:" + s[1] + // ss.Start dials, but not the ":[port]" target that is being tested here. + // Dial again, with ":[port]" as the target. + cc, err := grpc.Dial(":"+s[1], grpc.WithInsecure()) + if err != nil { + t.Fatalf("grpc.Dial(%q) = %v", ss.target, err) + return + } + defer cc.Close() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - _, err := ss.client.EmptyCall(ctx, &testpb.Empty{}) + _, err = testpb.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{}) if err != nil { t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err) } From 0ca3dbaaa93238457047725a8b8ddef450bb311f Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Thu, 12 Nov 2020 13:14:18 -0800 Subject: [PATCH 5/7] Refactored testing, added lock --- test/authority_test.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index 886f2f0af52..b3152d22c2d 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -24,6 +24,7 @@ import ( "net" "os" "strings" + "sync" "testing" "time" @@ -34,7 +35,7 @@ import ( testpb "google.golang.org/grpc/test/grpc_testing" ) -func authorityChecker(expectedAuthority *string) func(context.Context, *testpb.Empty) (*testpb.Empty, error) { +func authorityChecker(expectedAuthority *string, authorityMu *sync.Mutex) func(context.Context, *testpb.Empty) (*testpb.Empty, error) { return func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { md, ok := metadata.FromIncomingContext(ctx) if !ok { @@ -47,6 +48,10 @@ func authorityChecker(expectedAuthority *string) func(context.Context, *testpb.E if len(auths) != 1 { return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("no authority header, auths = %v", auths)) } + if authorityMu != nil { + authorityMu.Lock() + defer authorityMu.Unlock() + } if auths[0] != *expectedAuthority { return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], *expectedAuthority)) } @@ -59,7 +64,7 @@ func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer t.Fatalf("Error removing socket file %v: %v\n", address, err) } ss := &stubServer{ - emptyCall: authorityChecker(&expectedAuthority), + emptyCall: authorityChecker(&expectedAuthority, nil), network: "unix", address: address, target: target, @@ -70,7 +75,6 @@ func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer } if err := ss.Start(nil, opts...); err != nil { t.Fatalf("Error starting endpoint server: %v", err) - return } defer ss.Stop() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -155,27 +159,28 @@ func (s) TestUnixCustomDialer(t *testing.T) { func (s) TestColonPortAuthority(t *testing.T) { expectedAuthority := "" + var authorityMu sync.Mutex ss := &stubServer{ - emptyCall: authorityChecker(&expectedAuthority), + emptyCall: authorityChecker(&expectedAuthority, &authorityMu), network: "tcp", } if err := ss.Start(nil); err != nil { t.Fatalf("Error starting endpoint server: %v", err) - return } defer ss.Stop() s := strings.Split(ss.address, ":") - if len(s) != 2 { - t.Fatalf("No port in address: %v", ss.address) - return + _, port, err := net.SplitHostPort(ss.address) + if err != nil { + t.Fatalf("Failed splitting host from post: %v", err) } - expectedAuthority = "localhost:" + s[1] + authorityMu.Lock() + expectedAuthority = "localhost:" + port + authorityMu.Unlock() // ss.Start dials, but not the ":[port]" target that is being tested here. // Dial again, with ":[port]" as the target. cc, err := grpc.Dial(":"+s[1], grpc.WithInsecure()) if err != nil { t.Fatalf("grpc.Dial(%q) = %v", ss.target, err) - return } defer cc.Close() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) From 1132a36df93c6ba4811c656c122fb50f21ec15ad Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Thu, 12 Nov 2020 13:25:52 -0800 Subject: [PATCH 6/7] Moved mutex call --- test/authority_test.go | 54 +++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index b3152d22c2d..b6de94873aa 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -35,28 +35,22 @@ import ( testpb "google.golang.org/grpc/test/grpc_testing" ) -func authorityChecker(expectedAuthority *string, authorityMu *sync.Mutex) func(context.Context, *testpb.Empty) (*testpb.Empty, error) { - return func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { - md, ok := metadata.FromIncomingContext(ctx) - if !ok { - return nil, status.Error(codes.InvalidArgument, "failed to parse metadata") - } - auths, ok := md[":authority"] - if !ok { - return nil, status.Error(codes.InvalidArgument, "no authority header") - } - if len(auths) != 1 { - return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("no authority header, auths = %v", auths)) - } - if authorityMu != nil { - authorityMu.Lock() - defer authorityMu.Unlock() - } - if auths[0] != *expectedAuthority { - return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], *expectedAuthority)) - } - return &testpb.Empty{}, nil +func authorityChecker(ctx context.Context, expectedAuthority string) (*testpb.Empty, error) { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return nil, status.Error(codes.InvalidArgument, "failed to parse metadata") + } + auths, ok := md[":authority"] + if !ok { + return nil, status.Error(codes.InvalidArgument, "no authority header") + } + if len(auths) != 1 { + return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("no authority header, auths = %v", auths)) + } + if auths[0] != expectedAuthority { + return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid authority header %v, expected %v", auths[0], expectedAuthority)) } + return &testpb.Empty{}, nil } func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer func(context.Context, string) (net.Conn, error)) { @@ -64,10 +58,12 @@ func runUnixTest(t *testing.T, address, target, expectedAuthority string, dialer t.Fatalf("Error removing socket file %v: %v\n", address, err) } ss := &stubServer{ - emptyCall: authorityChecker(&expectedAuthority, nil), - network: "unix", - address: address, - target: target, + emptyCall: func(ctx context.Context, _ *testpb.Empty) (*testpb.Empty, error) { + return authorityChecker(ctx, expectedAuthority) + }, + network: "unix", + address: address, + target: target, } opts := []grpc.DialOption{} if dialer != nil { @@ -161,8 +157,12 @@ func (s) TestColonPortAuthority(t *testing.T) { expectedAuthority := "" var authorityMu sync.Mutex ss := &stubServer{ - emptyCall: authorityChecker(&expectedAuthority, &authorityMu), - network: "tcp", + emptyCall: func(ctx context.Context, _ *testpb.Empty) (*testpb.Empty, error) { + authorityMu.Lock() + defer authorityMu.Unlock() + return authorityChecker(ctx, expectedAuthority) + }, + network: "tcp", } if err := ss.Start(nil); err != nil { t.Fatalf("Error starting endpoint server: %v", err) From 080bc24df60e63d502d3b2439792a239b7f140f4 Mon Sep 17 00:00:00 2001 From: "gargut@google.com" Date: Thu, 12 Nov 2020 13:35:21 -0800 Subject: [PATCH 7/7] s[1]->port --- test/authority_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index b6de94873aa..af289bc69ed 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -23,7 +23,6 @@ import ( "fmt" "net" "os" - "strings" "sync" "testing" "time" @@ -168,7 +167,6 @@ func (s) TestColonPortAuthority(t *testing.T) { t.Fatalf("Error starting endpoint server: %v", err) } defer ss.Stop() - s := strings.Split(ss.address, ":") _, port, err := net.SplitHostPort(ss.address) if err != nil { t.Fatalf("Failed splitting host from post: %v", err) @@ -178,7 +176,7 @@ func (s) TestColonPortAuthority(t *testing.T) { authorityMu.Unlock() // ss.Start dials, but not the ":[port]" target that is being tested here. // Dial again, with ":[port]" as the target. - cc, err := grpc.Dial(":"+s[1], grpc.WithInsecure()) + cc, err := grpc.Dial(":"+port, grpc.WithInsecure()) if err != nil { t.Fatalf("grpc.Dial(%q) = %v", ss.target, err) }