testing: remove unnecessary calls to freeport

Previously we believe it was necessary for all code that required ports
to use freeport to prevent conflicts.

https://github.com/dnephin/freeport-test shows that it is actually save
to use port 0 (`127.0.0.1:0`) as long as it is passed directly to
`net.Listen`, and the listener holds the port for as long as it is
needed.

This works because freeport explicitly avoids the ephemeral port range,
and port 0 always uses that range. As you can see from the test output
of https://github.com/dnephin/freeport-test, the two systems never use
overlapping ports.

This commit converts all uses of freeport that were being passed
directly to a net.Listen to use port 0 instead. This allows us to remove
a bit of wrapping we had around httptest, in a couple places.

agent/acl_endpoint_test.go

@@ -18,7 +18,6 @@ import (
 	"github.com/hashicorp/consul/agent/consul/authmethod/testauth"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/internal/go-sso/oidcauth/oidcauthtest"
-	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/testrpc"
 )
@@ -1658,7 +1657,7 @@ func TestACLEndpoint_LoginLogout_jwt(t *testing.T) {
 	testrpc.WaitForLeader(t, a.RPC, "dc1")
 
 	// spin up a fake oidc server
-	oidcServer := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t)))
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	type mConfig = map[string]interface{}

agent/agent_test.go

@@ -1734,10 +1734,12 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) {
 	a := StartTestAgent(t, TestAgent{HCL: cfg})
 	defer a.Shutdown()
 
-	testCtx, testCancel := context.WithCancel(context.Background())
+	handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-	defer testCancel()
+		w.WriteHeader(http.StatusOK)
-
+		_, _ = w.Write([]byte("OK\n"))
-	testHTTPServer := launchHTTPCheckServer(t, testCtx)
+	})
+	testHTTPServer := httptest.NewServer(handler)
+	t.Cleanup(testHTTPServer.Close)
 
 	registerServicesAndChecks := func(t *testing.T, a *TestAgent) {
 		// add one persistent service with a simple check
@@ -1842,27 +1844,6 @@ node_name = "` + a.Config.NodeName + `"
 	}
 }
 
-func launchHTTPCheckServer(t *testing.T, ctx context.Context) *httptest.Server {
-	addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(freeport.Port(t)))
-
-	var lc net.ListenConfig
-	listener, err := lc.Listen(ctx, "tcp", addr)
-	require.NoError(t, err)
-
-	handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		w.WriteHeader(http.StatusOK)
-		_, _ = w.Write([]byte("OK\n"))
-	})
-
-	srv := &httptest.Server{
-		Listener: listener,
-		Config:   &http.Server{Handler: handler},
-	}
-	srv.Start()
-	t.Cleanup(srv.Close)
-	return srv
-}
-
 func TestAgent_AddCheck_Alias(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")

agent/consul/acl_endpoint_test.go

@@ -20,7 +20,6 @@ import (
 	"github.com/hashicorp/consul/agent/consul/authmethod/testauth"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/internal/go-sso/oidcauth/oidcauthtest"
-	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 )
@@ -4868,7 +4867,7 @@ func TestACLEndpoint_Login_jwt(t *testing.T) {
 	acl := ACL{srv: srv}
 
 	// spin up a fake oidc server
-	oidcServer := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t)))
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	type mConfig = map[string]interface{}

agent/consul/authmethod/kubeauth/testing.go

@@ -4,20 +4,16 @@ import (
 	"bytes"
 	"encoding/json"
 	"encoding/pem"
-	"fmt"
 	"io/ioutil"
 	"log"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"regexp"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
 
-	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/mitchellh/go-testing-interface"
 	"github.com/stretchr/testify/require"
 	authv1 "k8s.io/api/authentication/v1"
@@ -47,7 +43,7 @@ type TestAPIServer struct {
 // random free port.
 func StartTestAPIServer(t testing.T) *TestAPIServer {
 	s := &TestAPIServer{}
-	s.srv = httptestNewUnstartedServerWithPort(s, freeport.Port(t))
+	s.srv = httptest.NewUnstartedServer(s)
 	s.srv.Config.ErrorLog = log.New(ioutil.Discard, "", 0)
 	s.srv.StartTLS()
 
@@ -535,19 +531,3 @@ func createStatus(status, message string, reason metav1.StatusReason, details *m
 		Code:     code,
 	}
 }
-
-func httptestNewUnstartedServerWithPort(handler http.Handler, port int) *httptest.Server {
-	if port == 0 {
-		return httptest.NewUnstartedServer(handler)
-	}
-	addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port))
-	l, err := net.Listen("tcp", addr)
-	if err != nil {
-		panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err))
-	}
-
-	return &httptest.Server{
-		Listener: l,
-		Config:   &http.Server{Handler: handler},
-	}
-}

agent/consul/authmethod/ssoauth/sso_test.go

@@ -5,14 +5,14 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/agent/consul/authmethod"
-	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/internal/go-sso/oidcauth/oidcauthtest"
-	"github.com/hashicorp/consul/sdk/freeport"
-	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/go-hclog"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/square/go-jose.v2/jwt"
+
+	"github.com/hashicorp/consul/agent/consul/authmethod"
+	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/internal/go-sso/oidcauth/oidcauthtest"
+	"github.com/hashicorp/consul/sdk/testutil"
 )
 
 func TestJWT_NewValidator(t *testing.T) {
@@ -32,7 +32,7 @@ func TestJWT_NewValidator(t *testing.T) {
 		return method
 	}
 
-	oidcServer := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t)))
+	oidcServer := oidcauthtest.Start(t)
 
 	// Note that we won't test ALL of the available config variations here.
 	// The go-sso library has exhaustive tests.
@@ -110,7 +110,7 @@ func TestJWT_ValidateLogin(t *testing.T) {
 		return v
 	}
 
-	oidcServer := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t)))
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	cases := map[string]struct {

agent/consul/server_test.go

@@ -512,12 +512,11 @@ func TestServer_JoinWAN_SerfAllowedCIDRs(t *testing.T) {
 }
 
 func skipIfCannotBindToIP(t *testing.T, ip string) {
-	addr := ipaddr.FormatAddressPort(ip, freeport.Port(t))
+	l, err := net.Listen("tcp", net.JoinHostPort(ip, "0"))
-	l, err := net.Listen("tcp", addr)
-	l.Close()
 	if err != nil {
 		t.Skipf("Cannot bind on %s, to run on Mac OS: `sudo ifconfig lo0 alias %s up`", ip, ip)
 	}
+	l.Close()
 }
 
 func TestServer_LANReap(t *testing.T) {

agent/grpc/client_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"strconv"
 	"strings"
 	"sync/atomic"
 	"testing"
@@ -29,7 +28,7 @@ func useTLSForDcAlwaysTrue(_ string) bool {
 }
 
 func TestNewDialer_WithTLSWrapper(t *testing.T) {
-	lis, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(freeport.Port(t))))
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
 	require.NoError(t, err)
 	t.Cleanup(logError(t, lis.Close))
 
@@ -65,25 +64,18 @@ func TestNewDialer_WithTLSWrapper(t *testing.T) {
 }
 
 func TestNewDialer_WithALPNWrapper(t *testing.T) {
-	ports := freeport.GetN(t, 3)
+	lis1, err := net.Listen("tcp", "127.0.0.1:0")
-
-	var (
-		s1addr = ipaddr.FormatAddressPort("127.0.0.1", ports[0])
-		s2addr = ipaddr.FormatAddressPort("127.0.0.1", ports[1])
-		gwAddr = ipaddr.FormatAddressPort("127.0.0.1", ports[2])
-	)
-
-	lis1, err := net.Listen("tcp", s1addr)
 	require.NoError(t, err)
 	t.Cleanup(logError(t, lis1.Close))
 
-	lis2, err := net.Listen("tcp", s2addr)
+	lis2, err := net.Listen("tcp", "127.0.0.1:0")
 	require.NoError(t, err)
 	t.Cleanup(logError(t, lis2.Close))
 
 	// Send all of the traffic to dc2's server
 	var p tcpproxy.Proxy
-	p.AddRoute(gwAddr, tcpproxy.To(s2addr))
+	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", freeport.Port(t))
+	p.AddRoute(gwAddr, tcpproxy.To(lis2.Addr().String()))
 	p.AddStopACMESearch(gwAddr)
 	require.NoError(t, p.Start())
 	defer func() {

agent/grpc/server_test.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"net"
-	"strconv"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -18,7 +17,6 @@ import (
 	"github.com/hashicorp/consul/agent/grpc/internal/testservice"
 	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/consul/agent/pool"
-	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/hashicorp/consul/tlsutil"
 )
 
@@ -47,7 +45,7 @@ func newTestServer(t *testing.T, name string, dc string, tlsConf *tlsutil.Config
 		testservice.RegisterSimpleServer(server, &simple{name: name, dc: dc})
 	})
 
-	lis, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(freeport.Port(t))))
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
 	require.NoError(t, err)
 
 	rpc := &fakeRPCListener{t: t, handler: handler, tlsConf: tlsConf}

command/login/login_test.go

@@ -8,18 +8,18 @@ import (
 	"testing"
 	"time"
 
+	"github.com/mitchellh/cli"
+	"github.com/stretchr/testify/require"
+	"gopkg.in/square/go-jose.v2/jwt"
+
 	"github.com/hashicorp/consul/agent"
 	"github.com/hashicorp/consul/agent/consul/authmethod/kubeauth"
 	"github.com/hashicorp/consul/agent/consul/authmethod/testauth"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/command/acl"
 	"github.com/hashicorp/consul/internal/go-sso/oidcauth/oidcauthtest"
-	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/testrpc"
-	"github.com/mitchellh/cli"
-	"github.com/stretchr/testify/require"
-	"gopkg.in/square/go-jose.v2/jwt"
 )
 
 func TestLoginCommand_noTabs(t *testing.T) {
@@ -352,7 +352,7 @@ func TestLoginCommand_jwt(t *testing.T) {
 	bearerTokenFile := filepath.Join(testDir, "bearer.token")
 
 	// spin up a fake oidc server
-	oidcServer := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t)))
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	type mConfig = map[string]interface{}

command/snapshot/save/snapshot_save_test.go

@@ -5,19 +5,20 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"strings"
 	"sync/atomic"
 	"testing"
 
-	"github.com/hashicorp/consul/agent"
-	"github.com/hashicorp/consul/api"
-	"github.com/hashicorp/consul/lib"
-	"github.com/hashicorp/consul/sdk/testutil"
-	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/mitchellh/cli"
 	"github.com/stretchr/testify/require"
+
+	"github.com/hashicorp/consul/agent"
+	"github.com/hashicorp/consul/api"
+	"github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
 )
 
 func TestSnapshotSaveCommand_noTabs(t *testing.T) {
@@ -138,7 +139,7 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 	var fakeResult atomic.Value
 
 	// Run a fake webserver to pretend to be the snapshot API.
-	srv := lib.NewHTTPTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		if req.URL.Path != "/v1/snapshot" {
 			w.WriteHeader(http.StatusNotFound)
 			return
@@ -157,6 +158,7 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 		data := raw.([]byte)
 		_, _ = w.Write(data)
 	}))
+	t.Cleanup(srv.Close)
 
 	// Wait until the server is actually listening.
 	retry.Run(t, func(r *retry.R) {

connect/proxy/testing.go

@@ -8,10 +8,10 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/hashicorp/consul/connect"
-	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/mitchellh/go-testing-interface"
 	"github.com/stretchr/testify/require"
+
+	"github.com/hashicorp/consul/connect"
 )
 
 // TestLocalAddr makes a localhost address on the given port
@@ -30,12 +30,10 @@ type TestTCPServer struct {
 // a TestTCPServer serving requests to it. The server is already started and can
 // be stopped by calling Close().
 func NewTestTCPServer(t testing.T) *TestTCPServer {
-	addr := TestLocalAddr(freeport.Port(t))
+	l, err := net.Listen("tcp", "127.0.0.1:0")
-
-	l, err := net.Listen("tcp", addr)
 	require.NoError(t, err)
 
-	log.Printf("test tcp server listening on %s", addr)
+	log.Printf("test tcp server listening on %s", l.Addr())
 	s := &TestTCPServer{l: l}
 	go s.accept()
 

internal/go-sso/oidcauth/oidcauthtest/testing.go

@@ -24,10 +24,11 @@ import (
 	"sync"
 	"time"
 
-	"github.com/hashicorp/consul/internal/go-sso/oidcauth/internal/strutil"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/square/go-jose.v2"
 	"gopkg.in/square/go-jose.v2/jwt"
+
+	"github.com/hashicorp/consul/internal/go-sso/oidcauth/internal/strutil"
 )
 
 // Server is local server the mocks the endpoints used by the OIDC and
@@ -53,17 +54,6 @@ type Server struct {
 	disableUserInfo   bool
 }
 
-type startOption struct {
-	port int
-}
-
-// WithPort is a option for Start that lets the caller control the port
-// allocation. The returnFunc parameter is used when the provider is stopped to
-// return the port in whatever bookkeeping system the caller wants to use.
-func WithPort(port int) startOption {
-	return startOption{port: port}
-}
-
 type TestingT interface {
 	require.TestingT
 	Helper()
@@ -73,7 +63,7 @@ type TestingT interface {
 // Start creates a disposable Server. If the port provided is
 // zero it will bind to a random free port, otherwise the provided port is
 // used.
-func Start(t TestingT, options ...startOption) *Server {
+func Start(t TestingT) *Server {
 	t.Helper()
 	s := &Server{
 		allowedRedirectURIs: []string{
@@ -91,14 +81,7 @@ func Start(t TestingT, options ...startOption) *Server {
 	require.NoError(t, err)
 	s.jwks = jwks
 
-	var port int
+	s.httpServer = httptest.NewUnstartedServer(s)
-	for _, option := range options {
-		if option.port > 0 {
-			port = option.port
-		}
-	}
-
-	s.httpServer = httptestNewUnstartedServerWithPort(s, port)
 	s.httpServer.Config.ErrorLog = log.New(ioutil.Discard, "", 0)
 	s.httpServer.StartTLS()
 	t.Cleanup(s.httpServer.Close)

lib/testing_httpserver.go

@@ -1,33 +0,0 @@
-package lib
-
-import (
-	"net"
-	"net/http"
-	"net/http/httptest"
-
-	"github.com/hashicorp/consul/ipaddr"
-	"github.com/hashicorp/consul/sdk/freeport"
-)
-
-// NewHTTPTestServer starts and returns an httptest.Server that is listening
-// on a random port from freeport.Port. When the test case ends the server
-// will be stopped.
-//
-// We can't directly use httptest.Server here because that only thinks a port
-// is free if it's not bound. Consul tests frequently reserve ports via
-// `sdk/freeport` so you can have one part of the test try to use a port and
-// _know_ nothing is listening. If you simply assumed unbound ports were free
-// you'd end up with test cross-talk and weirdness.
-func NewHTTPTestServer(t freeport.TestingT, handler http.Handler) *httptest.Server {
-	srv := httptest.NewUnstartedServer(handler)
-
-	addr := ipaddr.FormatAddressPort("127.0.0.1", freeport.Port(t))
-	listener, err := net.Listen("tcp", addr)
-	if err != nil {
-		t.Fatalf("failed to listen on %v", addr)
-	}
-	srv.Listener = listener
-	t.Cleanup(srv.Close)
-	srv.Start()
-	return srv
-}