Merge pull request #11677 from hashicorp/dnephin/freeport-interface

sdk: use t.Cleanup in freeport and remove unnecessary calls

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 := startSSOTestServer(t)
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	type mConfig = map[string]interface{}
@@ -2330,14 +2329,6 @@ func upsertTestCustomizedBindingRule(rpc rpcFn, masterToken string, datacenter s
 	return &out, nil
 }
 
-func startSSOTestServer(t *testing.T) *oidcauthtest.Server {
-	ports := freeport.MustTake(1)
-	return oidcauthtest.Start(t, oidcauthtest.WithPort(
-		ports[0],
-		func() { freeport.Return(ports) },
-	))
-}
-
 func TestHTTPHandlers_ACLReplicationStatus(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")

agent/agent_test.go

@@ -295,10 +295,6 @@ func TestAgent_HTTPMaxHeaderBytes(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			ports, err := freeport.Take(1)
-			require.NoError(t, err)
-			t.Cleanup(func() { freeport.Return(ports) })
-
 			caConfig := tlsutil.Config{}
 			tlsConf, err := tlsutil.NewConfigurator(caConfig, hclog.New(nil))
 			require.NoError(t, err)
@@ -312,7 +308,7 @@ func TestAgent_HTTPMaxHeaderBytes(t *testing.T) {
 				},
 				RuntimeConfig: &config.RuntimeConfig{
 					HTTPAddrs: []net.Addr{
-						&net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: ports[0]},
+						&net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: freeport.GetOne(t)},
 					},
 					HTTPMaxHeaderBytes: tt.maxHeaderBytes,
 				},
@@ -1738,14 +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, returnPort := launchHTTPCheckServer(t, testCtx)
+	})
-	defer func() {
+	testHTTPServer := httptest.NewServer(handler)
-		testHTTPServer.Close()
+	t.Cleanup(testHTTPServer.Close)
-		returnPort()
-	}()
 
 	registerServicesAndChecks := func(t *testing.T, a *TestAgent) {
 		// add one persistent service with a simple check
@@ -1850,29 +1844,6 @@ node_name = "` + a.Config.NodeName + `"
 	}
 }
 
-func launchHTTPCheckServer(t *testing.T, ctx context.Context) (srv *httptest.Server, returnPortsFn func()) {
-	ports := freeport.MustTake(1)
-	port := ports[0]
-
-	addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port))
-
-	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()
-	return srv, func() { freeport.Return(ports) }
-}
-
 func TestAgent_AddCheck_Alias(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")
@@ -4708,14 +4679,12 @@ func TestAgent_JoinWAN_viaMeshGateway(t *testing.T) {
 
 	t.Parallel()
 
-	gwPort := freeport.MustTake(1)
+	port := freeport.GetOne(t)
-	defer freeport.Return(gwPort)
+	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", port)
-	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", gwPort[0])
 
 	// Due to some ordering, we'll have to manually configure these ports in
 	// advance.
-	secondaryRPCPorts := freeport.MustTake(2)
+	secondaryRPCPorts := freeport.GetN(t, 2)
-	defer freeport.Return(secondaryRPCPorts)
 
 	a1 := StartTestAgent(t, TestAgent{Name: "bob", HCL: `
 		domain = "consul"
@@ -4769,7 +4738,7 @@ func TestAgent_JoinWAN_viaMeshGateway(t *testing.T) {
 			ID:   "mesh-gateway",
 			Name: "mesh-gateway",
 			Meta: map[string]string{structs.MetaWANFederationKey: "1"},
-			Port: gwPort[0],
+			Port: port,
 		}
 		req, err := http.NewRequest("PUT", "/v1/agent/service/register", jsonReader(args))
 		require.NoError(t, err)
@@ -4883,7 +4852,7 @@ func TestAgent_JoinWAN_viaMeshGateway(t *testing.T) {
 			ID:   "mesh-gateway",
 			Name: "mesh-gateway",
 			Meta: map[string]string{structs.MetaWANFederationKey: "1"},
-			Port: gwPort[0],
+			Port: port,
 		}
 		req, err := http.NewRequest("PUT", "/v1/agent/service/register", jsonReader(args))
 		require.NoError(t, err)
@@ -4898,7 +4867,7 @@ func TestAgent_JoinWAN_viaMeshGateway(t *testing.T) {
 			ID:   "mesh-gateway",
 			Name: "mesh-gateway",
 			Meta: map[string]string{structs.MetaWANFederationKey: "1"},
-			Port: gwPort[0],
+			Port: port,
 		}
 		req, err := http.NewRequest("PUT", "/v1/agent/service/register", jsonReader(args))
 		require.NoError(t, err)
@@ -5281,10 +5250,7 @@ func TestAgent_ListenHTTP_MultipleAddresses(t *testing.T) {
 		t.Skip("too slow for testing.Short")
 	}
 
-	ports, err := freeport.Take(2)
+	ports := freeport.GetN(t, 2)
-	require.NoError(t, err)
-	t.Cleanup(func() { freeport.Return(ports) })
-
 	caConfig := tlsutil.Config{}
 	tlsConf, err := tlsutil.NewConfigurator(caConfig, hclog.New(nil))
 	require.NoError(t, err)

agent/connect/ca/testing.go

@@ -120,11 +120,7 @@ func runTestVault(t testing.T) (*TestVaultServer, error) {
 		return nil, fmt.Errorf("%q not found on $PATH", vaultBinaryName)
 	}
 
-	ports := freeport.MustTake(2)
+	ports := freeport.GetN(t, 2)
-	returnPortsFn := func() {
-		freeport.Return(ports)
-	}
-
 	var (
 		clientAddr  = fmt.Sprintf("127.0.0.1:%d", ports[0])
 		clusterAddr = fmt.Sprintf("127.0.0.1:%d", ports[1])
@@ -136,7 +132,6 @@ func runTestVault(t testing.T) (*TestVaultServer, error) {
 		Address: "http://" + clientAddr,
 	})
 	if err != nil {
-		returnPortsFn()
 		return nil, err
 	}
 	client.SetToken(token)
@@ -156,7 +151,6 @@ func runTestVault(t testing.T) (*TestVaultServer, error) {
 	cmd.Stdout = ioutil.Discard
 	cmd.Stderr = ioutil.Discard
 	if err := cmd.Start(); err != nil {
-		returnPortsFn()
 		return nil, err
 	}
 
@@ -165,7 +159,6 @@ func runTestVault(t testing.T) (*TestVaultServer, error) {
 		Addr:      "http://" + clientAddr,
 		cmd:       cmd,
 		client:    client,
-		returnPortsFn: returnPortsFn,
 	}
 	t.Cleanup(func() {
 		if err := testVault.Stop(); err != nil {
@@ -181,9 +174,6 @@ type TestVaultServer struct {
 	Addr      string
 	cmd       *exec.Cmd
 	client    *vaultapi.Client
-
-	// returnPortsFn will put the ports claimed for the test back into the
-	returnPortsFn func()
 }
 
 var printedVaultVersion sync.Once
@@ -229,11 +219,6 @@ func (v *TestVaultServer) Stop() error {
 	if err := v.cmd.Wait(); err != nil {
 		return err
 	}
-
-	if v.returnPortsFn != nil {
-		v.returnPortsFn()
-	}
-
 	return nil
 }
 

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 := startSSOTestServer(t)
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	type mConfig = map[string]interface{}
@@ -5003,14 +5002,6 @@ func TestACLEndpoint_Login_jwt(t *testing.T) {
 	}
 }
 
-func startSSOTestServer(t *testing.T) *oidcauthtest.Server {
-	ports := freeport.MustTake(1)
-	return oidcauthtest.Start(t, oidcauthtest.WithPort(
-		ports[0],
-		func() { freeport.Return(ports) },
-	))
-}
-
 func TestACLEndpoint_Logout(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")

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"
@@ -35,7 +31,6 @@ import (
 type TestAPIServer struct {
 	srv    *httptest.Server
 	caCert string
-	returnFunc func()
 
 	mu                       sync.Mutex
 	authorizedJWT            string                 // token review and sa read
@@ -48,12 +43,7 @@ type TestAPIServer struct {
 // random free port.
 func StartTestAPIServer(t testing.T) *TestAPIServer {
 	s := &TestAPIServer{}
-
+	s.srv = httptest.NewUnstartedServer(s)
-	ports := freeport.MustTake(1)
-	s.returnFunc = func() {
-		freeport.Return(ports)
-	}
-	s.srv = httptestNewUnstartedServerWithPort(s, ports[0])
 	s.srv.Config.ErrorLog = log.New(ioutil.Discard, "", 0)
 	s.srv.StartTLS()
 
@@ -101,10 +91,6 @@ func (s *TestAPIServer) SetAllowedServiceAccount(
 // Stop stops the running TestAPIServer.
 func (s *TestAPIServer) Stop() {
 	s.srv.Close()
-	if s.returnFunc != nil {
-		s.returnFunc()
-		s.returnFunc = nil
-	}
 }
 
 // Addr returns the current base URL for the running webserver.
@@ -545,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 := startTestServer(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 := startTestServer(t)
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	cases := map[string]struct {
@@ -260,11 +260,3 @@ func kv(a ...string) map[string]string {
 	}
 	return m
 }
-
-func startTestServer(t *testing.T) *oidcauthtest.Server {
-	ports := freeport.MustTake(1)
-	return oidcauthtest.Start(t, oidcauthtest.WithPort(
-		ports[0],
-		func() { freeport.Return(ports) },
-	))
-}

agent/consul/client_test.go

@@ -33,11 +33,7 @@ func testClientConfig(t *testing.T) (string, *Config) {
 	dir := testutil.TempDir(t, "consul")
 	config := DefaultConfig()
 
-	ports := freeport.MustTake(2)
+	ports := freeport.GetN(t, 2)
-	t.Cleanup(func() {
-		freeport.Return(ports)
-	})
-
 	config.Datacenter = "dc1"
 	config.DataDir = dir
 	config.NodeName = uniqueNodeName(t.Name())

agent/consul/operator_raft_endpoint_test.go

@@ -140,13 +140,10 @@ func TestOperator_RaftRemovePeerByAddress(t *testing.T) {
 
 	testrpc.WaitForLeader(t, s1.RPC, "dc1")
 
-	ports := freeport.MustTake(1)
-	defer freeport.Return(ports)
-
 	// Try to remove a peer that's not there.
 	arg := structs.RaftRemovePeerRequest{
 		Datacenter: "dc1",
-		Address:    raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])),
+		Address:    raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", freeport.GetOne(t))),
 	}
 	var reply struct{}
 	err := msgpackrpc.CallWithCodec(codec, "Operator.RaftRemovePeerByAddress", &arg, &reply)
@@ -263,10 +260,7 @@ func TestOperator_RaftRemovePeerByID(t *testing.T) {
 
 	// Add it manually to Raft.
 	{
-		ports := freeport.MustTake(1)
+		future := s1.raft.AddVoter(arg.ID, raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", freeport.GetOne(t))), 0, 0)
-		defer freeport.Return(ports)
-
-		future := s1.raft.AddVoter(arg.ID, raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])), 0, 0)
 		if err := future.Error(); err != nil {
 			t.Fatalf("err: %v", err)
 		}

agent/consul/server_test.go

@@ -116,11 +116,7 @@ func testServerConfig(t *testing.T) (string, *Config) {
 	dir := testutil.TempDir(t, "consul")
 	config := DefaultConfig()
 
-	ports := freeport.MustTake(3)
+	ports := freeport.GetN(t, 3)
-	t.Cleanup(func() {
-		freeport.Return(ports)
-	})
-
 	config.NodeName = uniqueNodeName(t.Name())
 	config.Bootstrap = true
 	config.Datacenter = "dc1"
@@ -516,15 +512,11 @@ func TestServer_JoinWAN_SerfAllowedCIDRs(t *testing.T) {
 }
 
 func skipIfCannotBindToIP(t *testing.T, ip string) {
-	ports := freeport.MustTake(1)
+	l, err := net.Listen("tcp", net.JoinHostPort(ip, "0"))
-	defer freeport.Return(ports)
-
-	addr := ipaddr.FormatAddressPort(ip, ports[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) {
@@ -725,9 +717,8 @@ func TestServer_JoinWAN_viaMeshGateway(t *testing.T) {
 
 	t.Parallel()
 
-	gwPort := freeport.MustTake(1)
+	port := freeport.GetOne(t)
-	defer freeport.Return(gwPort)
+	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", port)
-	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", gwPort[0])
 
 	dir1, s1 := testServerWithConfig(t, func(c *Config) {
 		c.TLSConfig.Domain = "consul"
@@ -813,7 +804,7 @@ func TestServer_JoinWAN_viaMeshGateway(t *testing.T) {
 				ID:      "mesh-gateway",
 				Service: "mesh-gateway",
 				Meta:    map[string]string{structs.MetaWANFederationKey: "1"},
-				Port:    gwPort[0],
+				Port:    port,
 			},
 		}
 
@@ -868,7 +859,7 @@ func TestServer_JoinWAN_viaMeshGateway(t *testing.T) {
 				ID:      "mesh-gateway",
 				Service: "mesh-gateway",
 				Meta:    map[string]string{structs.MetaWANFederationKey: "1"},
-				Port:    gwPort[0],
+				Port:    port,
 			},
 		}
 
@@ -885,7 +876,7 @@ func TestServer_JoinWAN_viaMeshGateway(t *testing.T) {
 				ID:      "mesh-gateway",
 				Service: "mesh-gateway",
 				Meta:    map[string]string{structs.MetaWANFederationKey: "1"},
-				Port:    gwPort[0],
+				Port:    port,
 			},
 		}
 

agent/grpc/client_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"strconv"
 	"strings"
 	"sync/atomic"
 	"testing"
@@ -29,10 +28,7 @@ func useTLSForDcAlwaysTrue(_ string) bool {
 }
 
 func TestNewDialer_WithTLSWrapper(t *testing.T) {
-	ports := freeport.MustTake(1)
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
-	defer freeport.Return(ports)
-
-	lis, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0])))
 	require.NoError(t, err)
 	t.Cleanup(logError(t, lis.Close))
 
@@ -68,26 +64,18 @@ func TestNewDialer_WithTLSWrapper(t *testing.T) {
 }
 
 func TestNewDialer_WithALPNWrapper(t *testing.T) {
-	ports := freeport.MustTake(3)
+	lis1, err := net.Listen("tcp", "127.0.0.1:0")
-	defer freeport.Return(ports)
-
-	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.GetOne(t))
+	p.AddRoute(gwAddr, tcpproxy.To(lis2.Addr().String()))
 	p.AddStopACMESearch(gwAddr)
 	require.NoError(t, p.Start())
 	defer func() {
@@ -193,10 +181,7 @@ func TestNewDialer_IntegrationWithTLSEnabledHandler(t *testing.T) {
 func TestNewDialer_IntegrationWithTLSEnabledHandler_viaMeshGateway(t *testing.T) {
 	// if this test is failing because of expired certificates
 	// use the procedure in test/CA-GENERATION.md
-	ports := freeport.MustTake(1)
+	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", freeport.GetOne(t))
-	defer freeport.Return(ports)
-
-	gwAddr := ipaddr.FormatAddressPort("127.0.0.1", ports[0])
 
 	res := resolver.NewServerResolverBuilder(newConfig(t))
 	registerWithGRPC(t, res)

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,12 +45,7 @@ func newTestServer(t *testing.T, name string, dc string, tlsConf *tlsutil.Config
 		testservice.RegisterSimpleServer(server, &simple{name: name, dc: dc})
 	})
 
-	ports := freeport.MustTake(1)
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
-	t.Cleanup(func() {
-		freeport.Return(ports)
-	})
-
-	lis, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0])))
 	require.NoError(t, err)
 
 	rpc := &fakeRPCListener{t: t, handler: handler, tlsConf: tlsConf}

agent/testagent.go

@@ -165,8 +165,7 @@ func (a *TestAgent) Start(t *testing.T) error {
 		Name:       name,
 	})
 
-	portsConfig, returnPortsFn := randomPortsSource(a.UseTLS)
+	portsConfig := randomPortsSource(t, a.UseTLS)
-	t.Cleanup(returnPortsFn)
 
 	// Create NodeID outside the closure, so that it does not change
 	testHCLConfig := TestConfigHCL(NodeID())
@@ -378,8 +377,8 @@ func (a *TestAgent) consulConfig() *consul.Config {
 // chance of port conflicts for concurrently executed test binaries.
 // Instead of relying on one set of ports to be sufficient we retry
 // starting the agent with different ports on port conflict.
-func randomPortsSource(tls bool) (data string, returnPortsFn func()) {
+func randomPortsSource(t *testing.T, tls bool) string {
-	ports := freeport.MustTake(7)
+	ports := freeport.GetN(t, 7)
 
 	var http, https int
 	if tls {
@@ -400,7 +399,7 @@ func randomPortsSource(tls bool) (data string, returnPortsFn func()) {
 			server = ` + strconv.Itoa(ports[5]) + `
 			grpc = ` + strconv.Itoa(ports[6]) + `
 		}
-	`, func() { freeport.Return(ports) }
+	`
 }
 
 func NodeID() string {

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 := startSSOTestServer(t)
+	oidcServer := oidcauthtest.Start(t)
 	pubKey, privKey := oidcServer.SigningKeys()
 
 	type mConfig = map[string]interface{}
@@ -470,11 +470,3 @@ func TestLoginCommand_jwt(t *testing.T) {
 		})
 	}
 }
-
-func startSSOTestServer(t *testing.T) *oidcauthtest.Server {
-	ports := freeport.MustTake(1)
-	return oidcauthtest.Start(t, oidcauthtest.WithPort(
-		ports[0],
-		func() { freeport.Return(ports) },
-	))
-}

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.
-	fakeAddr := lib.StartTestServer(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,10 +158,11 @@ 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) {
-		resp, err := http.Get("http://" + fakeAddr + "/not-real")
+		resp, err := srv.Client().Get(srv.URL + "/not-real")
 		require.NoError(r, err)
 		require.Equal(r, http.StatusNotFound, resp.StatusCode)
 	})
@@ -179,7 +181,7 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 
 			file := filepath.Join(dir, "backup.tgz")
 			args := []string{
-				"-http-addr=" + fakeAddr, // point to the fake
+				"-http-addr=" + srv.Listener.Addr().String(), // point to the fake
 				file,
 			}
 

connect/proxy/listener_test.go

@@ -112,15 +112,13 @@ func TestPublicListener(t *testing.T) {
 	// Can't enable t.Parallel since we rely on the global metrics instance.
 
 	ca := agConnect.TestCA(t, nil)
-	ports := freeport.MustTake(1)
-	defer freeport.Return(ports)
-
 	testApp := NewTestTCPServer(t)
 	defer testApp.Close()
 
+	port := freeport.GetOne(t)
 	cfg := PublicListenerConfig{
 		BindAddress:           "127.0.0.1",
-		BindPort:              ports[0],
+		BindPort:              port,
 		LocalServiceAddress:   testApp.Addr().String(),
 		HandshakeTimeoutMs:    100,
 		LocalConnectTimeoutMs: 100,
@@ -144,7 +142,7 @@ func TestPublicListener(t *testing.T) {
 	// Proxy and backend are running, play the part of a TLS client using same
 	// cert for now.
 	conn, err := svc.Dial(context.Background(), &connect.StaticResolver{
-		Addr:    TestLocalAddr(ports[0]),
+		Addr:    TestLocalAddr(port),
 		CertURI: agConnect.TestSpiffeIDService(t, "db"),
 	})
 	require.NoError(t, err)
@@ -166,9 +164,6 @@ func TestUpstreamListener(t *testing.T) {
 	// Can't enable t.Parallel since we rely on the global metrics instance.
 
 	ca := agConnect.TestCA(t, nil)
-	ports := freeport.MustTake(1)
-	defer freeport.Return(ports)
-
 	// Run a test server that we can dial.
 	testSvr := connect.NewTestServer(t, "db", ca)
 	go func() {
@@ -184,7 +179,7 @@ func TestUpstreamListener(t *testing.T) {
 		DestinationName:      "db",
 		Config:               map[string]interface{}{"connect_timeout_ms": 100},
 		LocalBindAddress:     "localhost",
-		LocalBindPort:        ports[0],
+		LocalBindPort:        freeport.GetOne(t),
 	}
 
 	// Setup metrics to test they are recorded

connect/proxy/proxy_test.go

@@ -27,8 +27,7 @@ func TestProxy_public(t *testing.T) {
 		t.Skip("too slow for testing.Short")
 	}
 
-	ports := freeport.MustTake(2)
+	ports := freeport.GetN(t, 2)
-	defer freeport.Return(ports)
 
 	a := agent.NewTestAgent(t, "")
 	defer a.Shutdown()

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
@@ -24,24 +24,17 @@ type TestTCPServer struct {
 	l                        net.Listener
 	stopped                  int32
 	accepted, closed, active int32
-	returnPortsFn            func()
 }
 
 // NewTestTCPServer opens as a listening socket on the given address and returns
 // a TestTCPServer serving requests to it. The server is already started and can
 // be stopped by calling Close().
 func NewTestTCPServer(t testing.T) *TestTCPServer {
-	ports := freeport.MustTake(1)
+	l, err := net.Listen("tcp", "127.0.0.1:0")
-	addr := TestLocalAddr(ports[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{
+	s := &TestTCPServer{l: l}
-		l:             l,
-		returnPortsFn: func() { freeport.Return(ports) },
-	}
 	go s.accept()
 
 	return s
@@ -53,10 +46,6 @@ func (s *TestTCPServer) Close() {
 	if s.l != nil {
 		s.l.Close()
 	}
-	if s.returnPortsFn != nil {
-		s.returnPortsFn()
-		s.returnPortsFn = nil
-	}
 }
 
 // Addr returns the address that this server is listening on.

connect/testing.go

@@ -97,7 +97,6 @@ type TestServer struct {
 	Listening chan struct{}
 
 	l        net.Listener
-	returnPortsFn func()
 	stopFlag int32
 	stopChan chan struct{}
 }
@@ -105,15 +104,13 @@ type TestServer struct {
 // NewTestServer returns a TestServer. It should be closed when test is
 // complete.
 func NewTestServer(t testing.T, service string, ca *structs.CARoot) *TestServer {
-	ports := freeport.MustTake(1)
 	return &TestServer{
 		Service:   service,
 		CA:        ca,
 		stopChan:  make(chan struct{}),
 		TLSCfg:    TestTLSConfig(t, service, ca),
-		Addr:          fmt.Sprintf("127.0.0.1:%d", ports[0]),
+		Addr:      fmt.Sprintf("127.0.0.1:%d", freeport.GetOne(t)),
 		Listening: make(chan struct{}),
-		returnPortsFn: func() { freeport.Return(ports) },
 	}
 }
 
@@ -190,10 +187,6 @@ func (s *TestServer) Close() error {
 		if s.l != nil {
 			s.l.Close()
 		}
-		if s.returnPortsFn != nil {
-			s.returnPortsFn()
-			s.returnPortsFn = nil
-		}
 		close(s.stopChan)
 	}
 	return nil

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

@@ -24,11 +24,11 @@ import (
 	"sync"
 	"time"
 
-	"github.com/hashicorp/consul/internal/go-sso/oidcauth/internal/strutil"
-	"github.com/mitchellh/go-testing-interface"
 	"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
@@ -54,25 +54,17 @@ type Server struct {
 	disableUserInfo   bool
 }
 
-type startOption struct {
+type TestingT interface {
-	port       int
+	require.TestingT
-	returnFunc func()
+	Helper()
-}
+	Cleanup(func())
-
-// 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, returnFunc func()) startOption {
-	return startOption{
-		port:       port,
-		returnFunc: returnFunc,
-	}
 }
 
 // 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 testing.T, options ...startOption) *Server {
+func Start(t TestingT) *Server {
+	t.Helper()
 	s := &Server{
 		allowedRedirectURIs: []string{
 			"https://example.com",
@@ -89,23 +81,9 @@ func Start(t testing.T, options ...startOption) *Server {
 	require.NoError(t, err)
 	s.jwks = jwks
 
-	var (
+	s.httpServer = httptest.NewUnstartedServer(s)
-		port       int
-		returnFunc func()
-	)
-	for _, option := range options {
-		if option.port > 0 {
-			port = option.port
-			returnFunc = option.returnFunc
-		}
-	}
-
-	s.httpServer = httptestNewUnstartedServerWithPort(s, port)
 	s.httpServer.Config.ErrorLog = log.New(ioutil.Discard, "", 0)
 	s.httpServer.StartTLS()
-	if returnFunc != nil {
-		t.Cleanup(returnFunc)
-	}
 	t.Cleanup(s.httpServer.Close)
 
 	cert := s.httpServer.Certificate()

lib/testing_httpserver.go

@@ -1,36 +0,0 @@
-package lib
-
-import (
-	"net/http"
-
-	"github.com/hashicorp/consul/ipaddr"
-	"github.com/hashicorp/consul/sdk/freeport"
-	"github.com/mitchellh/go-testing-interface"
-)
-
-// StartTestServer fires up a web server on a random unused port to serve the
-// given handler body. The address it is listening on is returned. When the
-// test case terminates the server will be stopped via cleanup functions.
-//
-// 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 StartTestServer(t testing.T, handler http.Handler) string {
-	ports := freeport.MustTake(1)
-	t.Cleanup(func() {
-		freeport.Return(ports)
-	})
-
-	addr := ipaddr.FormatAddressPort("127.0.0.1", ports[0])
-
-	server := &http.Server{Addr: addr, Handler: handler}
-	t.Cleanup(func() {
-		server.Close()
-	})
-
-	go server.ListenAndServe()
-
-	return addr
-}

sdk/freeport/freeport.go

@@ -1,5 +1,16 @@
-// Package freeport provides a helper for allocating free ports across multiple
+// Package freeport provides a helper for reserving free TCP ports across multiple
-// processes on the same machine.
+// processes on the same machine. Each process reserves a block of ports outside
+// the ephemeral port range. Tests can request one of these reserved ports
+// and freeport will ensure that no other test uses that port until it is returned
+// to freeport.
+//
+// Freeport is particularly useful when the code being tested does not accept
+// a net.Listener. Any code that accepts a net.Listener (or uses net/http/httptest.Server)
+// can use port 0 (ex: 127.0.0.1:0) to find an unused ephemeral port that will
+// not conflict.
+//
+// Any code that does not accept a net.Listener or can not bind directly to port
+// zero should use freeport to find an unused port.
 package freeport
 
 import (
@@ -11,8 +22,6 @@ import (
 	"runtime"
 	"sync"
 	"time"
-
-	"github.com/mitchellh/go-testing-interface"
 )
 
 const (
@@ -251,6 +260,8 @@ func alloc() (int, net.Listener) {
 }
 
 // MustTake is the same as Take except it panics on error.
+//
+// Deprecated: Use GetN or GetOne instead.
 func MustTake(n int) (ports []int) {
 	ports, err := Take(n)
 	if err != nil {
@@ -259,10 +270,12 @@ func MustTake(n int) (ports []int) {
 	return ports
 }
 
-// Take returns a list of free ports from the allocated port block. It is safe
+// Take returns a list of free ports from the reserved port block. It is safe
 // to call this method concurrently. Ports have been tested to be available on
 // 127.0.0.1 TCP but there is no guarantee that they will remain free in the
 // future.
+//
+// Most callers should prefer GetN or GetOne.
 func Take(n int) (ports []int, err error) {
 	if n <= 0 {
 		return nil, fmt.Errorf("freeport: cannot take %d ports", n)
@@ -381,11 +394,44 @@ func logf(severity string, format string, a ...interface{}) {
 	fmt.Fprintf(os.Stderr, "["+severity+"] freeport: "+format+"\n", a...)
 }
 
+// TestingT is the minimal set of methods implemented by *testing.T that are
+// used by functions in freelist.
+//
+// In the future new methods may be added to this interface, but those methods
+// should always be implemented by *testing.T
+type TestingT interface {
+	Helper()
+	Fatalf(format string, args ...interface{})
+	Cleanup(func())
+}
+
+// GetN returns n free ports from the reserved port block, and returns the
+// ports to the pool when the test ends. See Take for more details.
+func GetN(t TestingT, n int) []int {
+	t.Helper()
+	ports, err := Take(n)
+	if err != nil {
+		t.Fatalf("failed to take %v ports: %w", n, err)
+	}
+	t.Cleanup(func() {
+		Return(ports)
+	})
+	return ports
+}
+
+// GetOne returns a single free port from the reserved port block, and returns the
+// port to the pool when the test ends. See Take for more details.
+// Use GetN if more than a single port is required.
+func GetOne(t TestingT) int {
+	t.Helper()
+	return GetN(t, 1)[0]
+}
+
 // Deprecated: Please use Take/Return calls instead.
 func Get(n int) (ports []int) { return MustTake(n) }
 
 // Deprecated: Please use Take/Return calls instead.
-func GetT(t testing.T, n int) (ports []int) { return MustTake(n) }
+func GetT(t TestingT, n int) (ports []int) { return MustTake(n) }
 
 // Deprecated: Please use Take/Return calls instead.
 func Free(n int) (ports []int, err error) { return MustTake(n), nil }

sdk/testutil/server.go

@@ -29,11 +29,12 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/sdk/freeport"
-	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/go-cleanhttp"
 	"github.com/hashicorp/go-uuid"
 	"github.com/pkg/errors"
+
+	"github.com/hashicorp/consul/sdk/freeport"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
 )
 
 // TestPerformanceConfig configures the performance parameters.
@@ -142,7 +143,11 @@ func defaultServerConfig(t TestingTB) *TestServerConfig {
 		panic(err)
 	}
 
-	ports := freeport.MustTake(6)
+	ports, err := freeport.Take(6)
+	if err != nil {
+		t.Fatalf("failed to take ports: %v", err)
+	}
+
 	logBuffer := NewLogBuffer(t)
 
 	return &TestServerConfig{

sdk/testutil/types.go

@@ -8,4 +8,5 @@ type TestingTB interface {
 	Failed() bool
 	Logf(format string, args ...interface{})
 	Name() string
+	Fatalf(fmt string, args ...interface{})
 }