diff --git a/agent/acl_endpoint_test.go b/agent/acl_endpoint_test.go index 0b8111b880..1f49df0e3b 100644 --- a/agent/acl_endpoint_test.go +++ b/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") diff --git a/agent/agent_test.go b/agent/agent_test.go index 322e99e9b8..68ee415595 100644 --- a/agent/agent_test.go +++ b/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()) - defer testCancel() - - testHTTPServer, returnPort := launchHTTPCheckServer(t, testCtx) - defer func() { - testHTTPServer.Close() - returnPort() - }() + handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("OK\n")) + }) + testHTTPServer := httptest.NewServer(handler) + t.Cleanup(testHTTPServer.Close) 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) - defer freeport.Return(gwPort) - gwAddr := ipaddr.FormatAddressPort("127.0.0.1", gwPort[0]) + port := freeport.GetOne(t) + gwAddr := ipaddr.FormatAddressPort("127.0.0.1", port) // Due to some ordering, we'll have to manually configure these ports in // advance. - secondaryRPCPorts := freeport.MustTake(2) - defer freeport.Return(secondaryRPCPorts) + secondaryRPCPorts := freeport.GetN(t, 2) 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) - require.NoError(t, err) - t.Cleanup(func() { freeport.Return(ports) }) - + ports := freeport.GetN(t, 2) caConfig := tlsutil.Config{} tlsConf, err := tlsutil.NewConfigurator(caConfig, hclog.New(nil)) require.NoError(t, err) diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index 3b470063f0..6ba9df791a 100644 --- a/agent/connect/ca/testing.go +++ b/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) - returnPortsFn := func() { - freeport.Return(ports) - } - + ports := freeport.GetN(t, 2) 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,16 +151,14 @@ 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 } testVault := &TestVaultServer{ - RootToken: token, - Addr: "http://" + clientAddr, - cmd: cmd, - client: client, - returnPortsFn: returnPortsFn, + RootToken: token, + Addr: "http://" + clientAddr, + cmd: cmd, + client: client, } 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 } diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 54105fe19c..1dac564aba 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/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") diff --git a/agent/consul/authmethod/kubeauth/testing.go b/agent/consul/authmethod/kubeauth/testing.go index 7a61804fd7..4b15378fd2 100644 --- a/agent/consul/authmethod/kubeauth/testing.go +++ b/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" @@ -33,9 +29,8 @@ import ( // - GET /api/v1/namespaces//serviceaccounts/ // type TestAPIServer struct { - srv *httptest.Server - caCert string - returnFunc func() + srv *httptest.Server + caCert string 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{} - - ports := freeport.MustTake(1) - s.returnFunc = func() { - freeport.Return(ports) - } - s.srv = httptestNewUnstartedServerWithPort(s, ports[0]) + s.srv = httptest.NewUnstartedServer(s) 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}, - } -} diff --git a/agent/consul/authmethod/ssoauth/sso_test.go b/agent/consul/authmethod/ssoauth/sso_test.go index 1623f0904c..39501b026c 100644 --- a/agent/consul/authmethod/ssoauth/sso_test.go +++ b/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) }, - )) -} diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index d6ae206bca..79c3f1a7b5 100644 --- a/agent/consul/client_test.go +++ b/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) - t.Cleanup(func() { - freeport.Return(ports) - }) - + ports := freeport.GetN(t, 2) config.Datacenter = "dc1" config.DataDir = dir config.NodeName = uniqueNodeName(t.Name()) diff --git a/agent/consul/operator_raft_endpoint_test.go b/agent/consul/operator_raft_endpoint_test.go index da5bcc1214..778dcbf0fc 100644 --- a/agent/consul/operator_raft_endpoint_test.go +++ b/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) - defer freeport.Return(ports) - - future := s1.raft.AddVoter(arg.ID, raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", ports[0])), 0, 0) + future := s1.raft.AddVoter(arg.ID, raft.ServerAddress(fmt.Sprintf("127.0.0.1:%d", freeport.GetOne(t))), 0, 0) if err := future.Error(); err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 978d2453cd..bf68d8696c 100644 --- a/agent/consul/server_test.go +++ b/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) - t.Cleanup(func() { - freeport.Return(ports) - }) - + ports := freeport.GetN(t, 3) 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) - defer freeport.Return(ports) - - addr := ipaddr.FormatAddressPort(ip, ports[0]) - l, err := net.Listen("tcp", addr) - l.Close() + l, err := net.Listen("tcp", net.JoinHostPort(ip, "0")) 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) - defer freeport.Return(gwPort) - gwAddr := ipaddr.FormatAddressPort("127.0.0.1", gwPort[0]) + port := freeport.GetOne(t) + gwAddr := ipaddr.FormatAddressPort("127.0.0.1", port) 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, }, } diff --git a/agent/grpc/client_test.go b/agent/grpc/client_test.go index 1c11ee4ebe..3fa90e218d 100644 --- a/agent/grpc/client_test.go +++ b/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) - defer freeport.Return(ports) - - lis, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0]))) + lis, err := net.Listen("tcp", "127.0.0.1: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) - 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) + lis1, err := net.Listen("tcp", "127.0.0.1:0") 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) - defer freeport.Return(ports) - - gwAddr := ipaddr.FormatAddressPort("127.0.0.1", ports[0]) + gwAddr := ipaddr.FormatAddressPort("127.0.0.1", freeport.GetOne(t)) res := resolver.NewServerResolverBuilder(newConfig(t)) registerWithGRPC(t, res) diff --git a/agent/grpc/server_test.go b/agent/grpc/server_test.go index 9945f1e6cb..e36136f6cd 100644 --- a/agent/grpc/server_test.go +++ b/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) - t.Cleanup(func() { - freeport.Return(ports) - }) - - lis, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0]))) + lis, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) rpc := &fakeRPCListener{t: t, handler: handler, tlsConf: tlsConf} diff --git a/agent/testagent.go b/agent/testagent.go index b421f3cec6..80a4e0cf06 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -165,8 +165,7 @@ func (a *TestAgent) Start(t *testing.T) error { Name: name, }) - portsConfig, returnPortsFn := randomPortsSource(a.UseTLS) - t.Cleanup(returnPortsFn) + portsConfig := randomPortsSource(t, a.UseTLS) // 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()) { - ports := freeport.MustTake(7) +func randomPortsSource(t *testing.T, tls bool) string { + 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 { diff --git a/command/login/login_test.go b/command/login/login_test.go index 8c9309b25c..dcc4aaffa7 100644 --- a/command/login/login_test.go +++ b/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) }, - )) -} diff --git a/command/snapshot/save/snapshot_save_test.go b/command/snapshot/save/snapshot_save_test.go index b1b57289af..79df0dfc60 100644 --- a/command/snapshot/save/snapshot_save_test.go +++ b/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, } diff --git a/connect/proxy/listener_test.go b/connect/proxy/listener_test.go index 83cb399877..8a3006b69e 100644 --- a/connect/proxy/listener_test.go +++ b/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 diff --git a/connect/proxy/proxy_test.go b/connect/proxy/proxy_test.go index 46274de9b2..904e9c7c44 100644 --- a/connect/proxy/proxy_test.go +++ b/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) - defer freeport.Return(ports) + ports := freeport.GetN(t, 2) a := agent.NewTestAgent(t, "") defer a.Shutdown() diff --git a/connect/proxy/testing.go b/connect/proxy/testing.go index cad243478b..db27581d0f 100644 --- a/connect/proxy/testing.go +++ b/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) - addr := TestLocalAddr(ports[0]) - - l, err := net.Listen("tcp", addr) + l, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - log.Printf("test tcp server listening on %s", addr) - s := &TestTCPServer{ - l: l, - returnPortsFn: func() { freeport.Return(ports) }, - } + log.Printf("test tcp server listening on %s", l.Addr()) + s := &TestTCPServer{l: l} 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. diff --git a/connect/testing.go b/connect/testing.go index 30a517b61f..d054c0dee9 100644 --- a/connect/testing.go +++ b/connect/testing.go @@ -96,24 +96,21 @@ type TestServer struct { // Listening is closed when the listener is run. Listening chan struct{} - l net.Listener - returnPortsFn func() - stopFlag int32 - stopChan chan struct{} + l net.Listener + stopFlag int32 + stopChan chan 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]), - Listening: make(chan struct{}), - returnPortsFn: func() { freeport.Return(ports) }, + Service: service, + CA: ca, + stopChan: make(chan struct{}), + TLSCfg: TestTLSConfig(t, service, ca), + Addr: fmt.Sprintf("127.0.0.1:%d", freeport.GetOne(t)), + Listening: make(chan struct{}), } } @@ -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 diff --git a/internal/go-sso/oidcauth/oidcauthtest/testing.go b/internal/go-sso/oidcauth/oidcauthtest/testing.go index cdf27a19c8..8e20fb5750 100644 --- a/internal/go-sso/oidcauth/oidcauthtest/testing.go +++ b/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 { - port int - returnFunc 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, - } +type TestingT interface { + require.TestingT + Helper() + Cleanup(func()) } // 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 ( - 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 = httptest.NewUnstartedServer(s) 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() diff --git a/lib/testing_httpserver.go b/lib/testing_httpserver.go deleted file mode 100644 index df5e1f4141..0000000000 --- a/lib/testing_httpserver.go +++ /dev/null @@ -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 -} diff --git a/sdk/freeport/freeport.go b/sdk/freeport/freeport.go index eab12a87f7..e35d662ada 100644 --- a/sdk/freeport/freeport.go +++ b/sdk/freeport/freeport.go @@ -1,5 +1,16 @@ -// Package freeport provides a helper for allocating free ports across multiple -// processes on the same machine. +// Package freeport provides a helper for reserving free TCP ports across multiple +// 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 } diff --git a/sdk/testutil/server.go b/sdk/testutil/server.go index 1ecde56a19..cd894a2ef9 100644 --- a/sdk/testutil/server.go +++ b/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{ diff --git a/sdk/testutil/types.go b/sdk/testutil/types.go index ec04e45dcc..d6b5841c50 100644 --- a/sdk/testutil/types.go +++ b/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{}) }