[v10] Make it possible to test gateway opening/closing in Connect (#1…

…4183)

* Make it possible to test gateway opening/closing in Connect

Open() and Close() used to not return any error and Open() used to start
the gateway in a goroutine, making it rather hard to write tests for it.

This commit makes it so that Open() and Close() return errors and Open()
blocks.

Adjustments have been made to other places in lib/teleterm to account
for that missing goroutine and returned errors.

* Close httptest server in alpnproxy/local_proxy_test.go

While writing tests for the gateways, I was relying heavily on tests for
the local proxy. I noticed that it starts the server but doesn't close it
so I added an appropriate call to the cleanup function.

lib/srv/alpnproxy/local_proxy_test.go

@@ -118,6 +118,7 @@ func createAWSAccessProxySuite(t *testing.T, cred *credentials.Credentials) *Loc
 	t.Cleanup(func() {
 		err := lp.Close()
 		require.NoError(t, err)
+		hs.Close()
 	})
 	go func() {
 		err := lp.StartAWSAccessProxy(context.Background())

lib/teleterm/daemon/daemon.go

@@ -155,7 +155,11 @@ func (s *Service) createGateway(ctx context.Context, params CreateGatewayParams)
 		return nil, trace.Wrap(err)
 	}
 
-	gateway.Open()
+	go func() {
+		if err := gateway.Serve(); err != nil {
+			gateway.Log.WithError(err).Warn("Failed to open a connection.")
+		}
+	}()
 
 	s.gateways = append(s.gateways, gateway)
 
@@ -202,22 +206,28 @@ func (s *Service) RemoveGateway(ctx context.Context, gatewayURI string) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	s.removeGateway(gateway)
+	if err := s.removeGateway(gateway); err != nil {
+		return trace.Wrap(err)
+	}
 
 	return nil
 }
 
 // removeGateway assumes that mu is already held by a public method.
-func (s *Service) removeGateway(gateway *gateway.Gateway) {
-	gateway.Close()
+func (s *Service) removeGateway(gateway *gateway.Gateway) error {
+	if err := gateway.Close(); err != nil {
+		return trace.Wrap(err)
+	}
 
 	// remove closed gateway from list
 	for index := range s.gateways {
 		if s.gateways[index] == gateway {
 			s.gateways = append(s.gateways[:index], s.gateways[index+1:]...)
-			return
+			return nil
 		}
 	}
+
+	return trace.NotFound("gateway %v not found in gateway list", gateway.URI.String())
 }
 
 // RestartGateway stops a gateway and starts a new one with identical parameters.
@@ -232,7 +242,9 @@ func (s *Service) RestartGateway(ctx context.Context, gatewayURI string) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	s.removeGateway(gateway)
+	if err := s.removeGateway(gateway); err != nil {
+		return trace.Wrap(err)
+	}
 
 	newGateway, err := s.createGateway(ctx, CreateGatewayParams{
 		TargetURI:             gateway.TargetURI,

lib/teleterm/gateway/gateway.go

@@ -104,21 +104,27 @@ func New(cfg Config, cliCommandProvider CLICommandProvider) (*Gateway, error) {
 }
 
 // Close terminates gateway connection
-func (g *Gateway) Close() {
+func (g *Gateway) Close() error {
 	g.closeCancel()
-	g.localProxy.Close()
+
+	if err := g.localProxy.Close(); err != nil {
+		return trace.Wrap(err)
+	}
+
+	return nil
 }
 
-// Open opens a gateway to Teleport proxy
-func (g *Gateway) Open() {
-	go func() {
-		g.Log.Info("Gateway is open.")
-		if err := g.localProxy.Start(g.closeContext); err != nil {
-			g.Log.WithError(err).Warn("Failed to open a connection.")
-		}
+// Serve starts the underlying ALPN proxy. Blocks until closeContext is canceled.
+func (g *Gateway) Serve() error {
+	g.Log.Info("Gateway is open.")
 
-		g.Log.Info("Gateway has closed.")
-	}()
+	if err := g.localProxy.Start(g.closeContext); err != nil {
+		return trace.Wrap(err)
+	}
+
+	g.Log.Info("Gateway has closed.")
+
+	return nil
 }
 
 // LocalPortInt returns the port of a gateway as an integer rather than a string.

lib/teleterm/gateway/gateway_test.go

@@ -16,10 +16,16 @@ package gateway
 
 import (
 	"fmt"
+	"net"
+	"net/http"
+	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/gravitational/teleport/lib/defaults"
+	"github.com/gravitational/teleport/lib/teleterm/api/uri"
 
+	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"
 )
 
@@ -45,3 +51,61 @@ func TestCLICommandUsesCLICommandProvider(t *testing.T) {
 
 	require.Equal(t, "foo/bar", command)
 }
+
+func TestGatewayStart(t *testing.T) {
+	hs := httptest.NewTLSServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {}))
+	t.Cleanup(func() {
+		hs.Close()
+	})
+
+	gateway, err := New(
+		Config{
+			TargetName:   "foo",
+			TargetURI:    uri.NewClusterURI("bar").AppendDB("foo").String(),
+			TargetUser:   "alice",
+			Protocol:     defaults.ProtocolPostgres,
+			CertPath:     "../../../fixtures/certs/proxy1.pem",
+			KeyPath:      "../../../fixtures/certs/proxy1-key.pem",
+			Insecure:     true,
+			WebProxyAddr: hs.Listener.Addr().String(),
+		},
+		mockCLICommandProvider{},
+	)
+	require.NoError(t, err)
+	t.Cleanup(func() { gateway.Close() })
+	gatewayAddress := net.JoinHostPort(gateway.LocalAddress, gateway.LocalPort)
+
+	serveErr := make(chan error)
+
+	go func() {
+		err := gateway.Serve()
+		serveErr <- err
+	}()
+
+	blockUntilGatewayAcceptsConnections(t, gatewayAddress)
+
+	err = gateway.Close()
+	require.NoError(t, err)
+	require.NoError(t, <-serveErr)
+}
+
+func blockUntilGatewayAcceptsConnections(t *testing.T, address string) {
+	conn, err := net.DialTimeout("tcp", address, time.Second*1)
+	require.NoError(t, err)
+	t.Cleanup(func() { conn.Close() })
+
+	err = conn.SetReadDeadline(time.Now().Add(time.Second))
+	require.NoError(t, err)
+
+	out := make([]byte, 1024)
+	_, err = conn.Read(out)
+	// Our "client" here is going to fail the handshake because it requests an application protocol
+	// (typically teleport-<some db protocol>) that the target server (typically
+	// httptest.NewTLSServer) doesn't support.
+	//
+	// So we just expect EOF here. In case of a timeout, this check will fail.
+	require.True(t, trace.IsEOF(err), "expected EOF, got %v", err)
+
+	err = conn.Close()
+	require.NoError(t, err)
+}