From 0bb9c318b74be12bd95129f5ae53c12271e54ba5 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Fri, 4 Sep 2020 14:53:02 -0400
Subject: [PATCH] http: fix tests incorrectly using HTTPAddr to get the address
 of the https server.

In #8234 I changed a few tests to use TestAgent.HTTPAddr() to find the
addr used in the test. Due to the way HTTPAddr() was implemented these
tests were passing, but I think the pass was incidental. HTTPAddr() was
not matching any servers, and was instead returning the last server,
which happened to be the one these tests wanted.

This commit fixes the implementation of HTTPAddr to panic if no match
was found. The tests which require an HTTPS server are changed to use
a new firstAddr() to look up the correct address.
---
 agent/agent_test.go |  6 ++++--
 agent/http_test.go  | 15 +++++++++------
 agent/testagent.go  | 21 ++++++++++++++++-----
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/agent/agent_test.go b/agent/agent_test.go
index dded499ee2..6a70ac7bf0 100644
--- a/agent/agent_test.go
+++ b/agent/agent_test.go
@@ -1917,13 +1917,15 @@ func TestAgent_HTTPCheck_EnableAgentTLSForChecks(t *testing.T) {
 			Status:  api.HealthCritical,
 		}
 
-		url := fmt.Sprintf("https://%s/v1/agent/self", a.HTTPAddr())
+		addr, err := firstAddr(a.Agent.apiServers, "https")
+		require.NoError(t, err)
+		url := fmt.Sprintf("https://%s/v1/agent/self", addr.String())
 		chk := &structs.CheckType{
 			HTTP:     url,
 			Interval: 20 * time.Millisecond,
 		}
 
-		err := a.AddCheck(health, chk, false, "", ConfigSourceLocal)
+		err = a.AddCheck(health, chk, false, "", ConfigSourceLocal)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
diff --git a/agent/http_test.go b/agent/http_test.go
index 36ecf387b3..473e2b0f95 100644
--- a/agent/http_test.go
+++ b/agent/http_test.go
@@ -1351,9 +1351,11 @@ func TestHTTPServer_HandshakeTimeout(t *testing.T) {
 	})
 	defer a.Shutdown()
 
+	addr, err := firstAddr(a.Agent.apiServers, "https")
+	require.NoError(t, err)
 	// Connect to it with a plain TCP client that doesn't attempt to send HTTP or
 	// complete a TLS handshake.
-	conn, err := net.Dial("tcp", a.HTTPAddr())
+	conn, err := net.Dial("tcp", addr.String())
 	require.NoError(t, err)
 	defer conn.Close()
 
@@ -1413,7 +1415,8 @@ func TestRPC_HTTPSMaxConnsPerClient(t *testing.T) {
 			})
 			defer a.Shutdown()
 
-			addr := a.HTTPAddr()
+			addr, err := firstAddr(a.Agent.apiServers, strings.ToLower(tc.name))
+			require.NoError(t, err)
 
 			assertConn := func(conn net.Conn, wantOpen bool) {
 				retry.Run(t, func(r *retry.R) {
@@ -1433,21 +1436,21 @@ func TestRPC_HTTPSMaxConnsPerClient(t *testing.T) {
 			}
 
 			// Connect to the server with bare TCP
-			conn1, err := net.DialTimeout("tcp", addr, time.Second)
+			conn1, err := net.DialTimeout("tcp", addr.String(), time.Second)
 			require.NoError(t, err)
 			defer conn1.Close()
 
 			assertConn(conn1, true)
 
 			// Two conns should succeed
-			conn2, err := net.DialTimeout("tcp", addr, time.Second)
+			conn2, err := net.DialTimeout("tcp", addr.String(), time.Second)
 			require.NoError(t, err)
 			defer conn2.Close()
 
 			assertConn(conn2, true)
 
 			// Third should succeed negotiating TCP handshake...
-			conn3, err := net.DialTimeout("tcp", addr, time.Second)
+			conn3, err := net.DialTimeout("tcp", addr.String(), time.Second)
 			require.NoError(t, err)
 			defer conn3.Close()
 
@@ -1460,7 +1463,7 @@ func TestRPC_HTTPSMaxConnsPerClient(t *testing.T) {
 			require.NoError(t, a.reloadConfigInternal(&newCfg))
 
 			// Now another conn should be allowed
-			conn4, err := net.DialTimeout("tcp", addr, time.Second)
+			conn4, err := net.DialTimeout("tcp", addr.String(), time.Second)
 			require.NoError(t, err)
 			defer conn4.Close()
 
diff --git a/agent/testagent.go b/agent/testagent.go
index fa3508ffab..ce17c648e1 100644
--- a/agent/testagent.go
+++ b/agent/testagent.go
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"math/rand"
+	"net"
 	"net/http/httptest"
 	"path/filepath"
 	"strconv"
@@ -313,13 +314,23 @@ func (a *TestAgent) DNSAddr() string {
 }
 
 func (a *TestAgent) HTTPAddr() string {
-	var srv apiServer
-	for _, srv = range a.Agent.apiServers.servers {
-		if srv.Protocol == "http" {
-			break
+	addr, err := firstAddr(a.Agent.apiServers, "http")
+	if err != nil {
+		// TODO: t.Fatal instead of panic
+		panic("no http server registered")
+	}
+	return addr.String()
+}
+
+// firstAddr is used by tests to look up the address for the first server which
+// matches the protocol
+func firstAddr(s *apiServers, protocol string) (net.Addr, error) {
+	for _, srv := range s.servers {
+		if srv.Protocol == protocol {
+			return srv.Addr, nil
 		}
 	}
-	return srv.Addr.String()
+	return nil, fmt.Errorf("no server registered with protocol %v", protocol)
 }
 
 func (a *TestAgent) SegmentAddr(name string) string {