Add ListenerHolder.AddrString() to avoid bad IPv6 assumptions. (#299)

internal/serving/grpcserver.go

@@ -159,7 +159,7 @@ func (gw *GrpcWrapper) startHTTPProxy(wg *sync.WaitGroup) error {
 
 	proxyMux := runtime.NewServeMux()
 
-	serviceEndpoint := fmt.Sprintf("localhost:%d", gw.serviceLh.Number())
+	serviceEndpoint := gw.serviceLh.AddrString()
 	gw.grpcClient, err = grpc.DialContext(ctx, serviceEndpoint, grpc.WithInsecure(), grpc.WithBlock())
 	if err != nil {
 		gw.logger.WithFields(log.Fields{
@@ -186,7 +186,7 @@ func (gw *GrpcWrapper) startHTTPProxy(wg *sync.WaitGroup) error {
 
 	go func() {
 		wg.Done()
-		gw.logger.Infof("Serving proxy on :%d", gw.proxyLh.Number())
+		gw.logger.Infof("Serving proxy on %s", gw.proxyLh.AddrString())
 		err := gw.proxy.Serve(proxyLn)
 		gw.proxyAwaiter <- err
 		if err != nil {
@@ -231,7 +231,7 @@ func (gw *GrpcWrapper) startGrpcServer(wg *sync.WaitGroup) error {
 
 	go func() {
 		wg.Done()
-		gw.logger.Infof("Serving gRPC on :%d", gw.serviceLh.Number())
+		gw.logger.Infof("Serving gRPC on %s", gw.serviceLh.AddrString())
 		err := gw.server.Serve(serviceLn)
 		gw.grpcAwaiter <- err
 		if err != nil {

internal/util/netlistener/listener_holder.go

@@ -13,6 +13,7 @@ import (
 type ListenerHolder struct {
 	number   int
 	listener net.Listener
+	addr     string
 	sync.RWMutex
 }
 
@@ -33,6 +34,13 @@ func (lh *ListenerHolder) Number() int {
 	return lh.number
 }
 
+// AddrString returns the address of the serving port.
+// Use this over fmt.Sprintf(":%d", lh.Number()) because the address is represented differently in
+// systems that prefer IPv4 and IPv6.
+func (lh *ListenerHolder) AddrString() string {
+	return lh.addr
+}
+
 // Close shutsdown the TCP listener.
 func (lh *ListenerHolder) Close() error {
 	lh.Lock()
@@ -59,6 +67,7 @@ func NewFromPortNumber(portNumber int) (*ListenerHolder, error) {
 		return &ListenerHolder{
 			number:   tcpConn.Port,
 			listener: conn,
+			addr:     conn.Addr().String(),
 		}, nil
 	}
 	return nil, err

internal/util/netlistener/listener_holder_test.go

@@ -1,6 +1,8 @@
 package netlistener
 
 import (
+	"fmt"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -10,6 +12,32 @@ const (
 	numIterations = 1000
 )
 
+// TestAddrString verifies that AddrString() is consistent with the port's Addr().String() value.
+func TestAddrString(t *testing.T) {
+	lh, err := NewFromPortNumber(0)
+	if err != nil {
+		t.Fatalf("NewFromPortNumber(0) had error, %s", err)
+	}
+
+	if !strings.HasSuffix(lh.AddrString(), fmt.Sprintf(":%d", lh.Number())) {
+		t.Errorf("%s does not have suffix ':%d'", lh.AddrString(), lh.Number())
+	}
+
+	port, err := lh.Obtain()
+	defer func() {
+		err := port.Close()
+		if err != nil {
+			t.Errorf("error %s while calling port.Close()", err)
+		}
+	}()
+	if err != nil {
+		t.Errorf("error %s while calling lh.Obtain", err)
+	}
+	if port.Addr().String() != lh.AddrString() {
+		t.Errorf("port.Addr().String() = %s should match lh.AddrString() = %s", port.Addr().String(), lh.AddrString())
+	}
+}
+
 // TestObtain verifies that a ListenerHolder only returns Obtain() once.
 func TestObtain(t *testing.T) {
 	var errCount uint64

test/e2e/grpcserver_test.go

@@ -2,7 +2,6 @@ package e2e
 
 import (
 	"context"
-	"fmt"
 
 	pb "github.com/GoogleCloudPlatform/open-match/internal/pb"
 	"github.com/GoogleCloudPlatform/open-match/internal/serving"
@@ -11,7 +10,6 @@ import (
 	netlistenerTesting "github.com/GoogleCloudPlatform/open-match/internal/util/netlistener/testing"
 	log "github.com/sirupsen/logrus"
 
-	"net"
 	"testing"
 
 	"google.golang.org/grpc"
@@ -24,7 +22,7 @@ func createLogger() *log.Entry {
 }
 
 func createClient(lh *netlistener.ListenerHolder) (pb.FrontendClient, error) {
-	conn, err := grpc.Dial(fmt.Sprintf(":%d", lh.Number()), grpc.WithInsecure())
+	conn, err := grpc.Dial(lh.AddrString(), grpc.WithInsecure())
 	if err != nil {
 		return nil, err
 	}
@@ -37,7 +35,12 @@ func TestOpenClose(t *testing.T) {
 	proxyLh := netlistenerTesting.MustListen()
 	fakeService := &servingTesting.FakeFrontend{}
 	server := serving.NewGrpcServer(serviceLh, proxyLh, createLogger())
-
+	defer func() {
+		err := server.Stop()
+		if err != nil {
+			log.Printf("server.Stop() reported an error but this is ok, %s", err)
+		}
+	}()
 	server.AddService(func(server *grpc.Server) {
 		pb.RegisterFrontendServer(server, fakeService)
 	})
@@ -60,22 +63,4 @@ func TestOpenClose(t *testing.T) {
 	if result == nil {
 		t.Errorf("Result %v was not successful.", result)
 	}
-
-	server.Stop()
-
-	// Re-open the port to ensure it's free.
-	ln, err := net.Listen("tcp", fmt.Sprintf(":%d", serviceLh.Number()))
-	if err != nil {
-		t.Errorf("grpc server is still running! Result= %v Error= %s", ln, err)
-	}
-	result, err = client.CreatePlayer(context.Background(), &pb.CreatePlayerRequest{})
-	if err == nil {
-		t.Errorf("grpc server is still running! Result= %v Error= %s", result, err)
-	}
-	// Re-open the proxyPort to ensure it's free.
-	ln, err = net.Listen("tcp6", fmt.Sprintf(":%d", proxyLh.Number()))
-	if err != nil {
-		t.Errorf("grpc proxy is still running! Result= %v Error= %s", ln, err)
-	}
-
 }