From e8312d6b5ab3219440322e37cbdd2ca3d981f9f2 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 29 Nov 2021 12:19:43 -0500 Subject: [PATCH] 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 | 3 +- agent/agent_test.go | 31 ++++------------- agent/consul/acl_endpoint_test.go | 3 +- agent/consul/authmethod/kubeauth/testing.go | 22 +------------ agent/consul/authmethod/ssoauth/sso_test.go | 14 ++++---- agent/consul/server_test.go | 5 ++- agent/grpc/client_test.go | 18 +++------- agent/grpc/server_test.go | 4 +-- command/login/login_test.go | 10 +++--- command/snapshot/save/snapshot_save_test.go | 14 ++++---- connect/proxy/testing.go | 10 +++--- .../go-sso/oidcauth/oidcauthtest/testing.go | 25 +++----------- lib/testing_httpserver.go | 33 ------------------- 13 files changed, 45 insertions(+), 147 deletions(-) delete mode 100644 lib/testing_httpserver.go diff --git a/agent/acl_endpoint_test.go b/agent/acl_endpoint_test.go index d1904f5c3f..d0e5f29157 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 := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t))) + oidcServer := oidcauthtest.Start(t) pubKey, privKey := oidcServer.SigningKeys() type mConfig = map[string]interface{} diff --git a/agent/agent_test.go b/agent/agent_test.go index 7fbe9559f3..a7e39e5605 100644 --- a/agent/agent_test.go +++ b/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()) - defer testCancel() - - testHTTPServer := launchHTTPCheckServer(t, testCtx) + 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 @@ -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") diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index fa0325fec7..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 := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t))) + oidcServer := oidcauthtest.Start(t) pubKey, privKey := oidcServer.SigningKeys() type mConfig = map[string]interface{} diff --git a/agent/consul/authmethod/kubeauth/testing.go b/agent/consul/authmethod/kubeauth/testing.go index 84e9c6881f..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" @@ -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}, - } -} diff --git a/agent/consul/authmethod/ssoauth/sso_test.go b/agent/consul/authmethod/ssoauth/sso_test.go index 7ef3394806..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 := 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 { diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 3cca5b75fb..5b1bdabfdb 100644 --- a/agent/consul/server_test.go +++ b/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", 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) { diff --git a/agent/grpc/client_test.go b/agent/grpc/client_test.go index 1b8cfa1ab6..8baf751def 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,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) - - 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.Port(t)) + p.AddRoute(gwAddr, tcpproxy.To(lis2.Addr().String())) p.AddStopACMESearch(gwAddr) require.NoError(t, p.Start()) defer func() { diff --git a/agent/grpc/server_test.go b/agent/grpc/server_test.go index 07af7299fc..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,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} diff --git a/command/login/login_test.go b/command/login/login_test.go index 84a0a19170..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 := oidcauthtest.Start(t, oidcauthtest.WithPort(freeport.Port(t))) + oidcServer := oidcauthtest.Start(t) pubKey, privKey := oidcServer.SigningKeys() type mConfig = map[string]interface{} diff --git a/command/snapshot/save/snapshot_save_test.go b/command/snapshot/save/snapshot_save_test.go index bc4bba2163..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. - 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) { diff --git a/connect/proxy/testing.go b/connect/proxy/testing.go index ebaf0d1cc5..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 @@ -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", addr) + l, err := net.Listen("tcp", "127.0.0.1:0") 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() diff --git a/internal/go-sso/oidcauth/oidcauthtest/testing.go b/internal/go-sso/oidcauth/oidcauthtest/testing.go index e5cb56ebf6..8e20fb5750 100644 --- a/internal/go-sso/oidcauth/oidcauthtest/testing.go +++ b/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 - for _, option := range options { - if option.port > 0 { - port = option.port - } - } - - s.httpServer = httptestNewUnstartedServerWithPort(s, port) + s.httpServer = httptest.NewUnstartedServer(s) s.httpServer.Config.ErrorLog = log.New(ioutil.Discard, "", 0) s.httpServer.StartTLS() t.Cleanup(s.httpServer.Close) diff --git a/lib/testing_httpserver.go b/lib/testing_httpserver.go deleted file mode 100644 index a4ad79622f..0000000000 --- a/lib/testing_httpserver.go +++ /dev/null @@ -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 -}