Merge pull request #11796 from hashicorp/dnephin/cleanup-test-server

testing: stop using an old version in testServer

agent/consul/autopilot_test.go

@@ -6,12 +6,13 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/sdk/testutil/retry"
-	"github.com/hashicorp/consul/testrpc"
 	"github.com/hashicorp/raft"
 	"github.com/hashicorp/serf/serf"
 	"github.com/stretchr/testify/require"
+
+	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
+	"github.com/hashicorp/consul/testrpc"
 )
 
 func TestAutopilot_IdempotentShutdown(t *testing.T) {
@@ -19,7 +20,7 @@ func TestAutopilot_IdempotentShutdown(t *testing.T) {
 		t.Skip("too slow for testing.Short")
 	}
 
-	dir1, s1 := testServerWithConfig(t, nil)
+	dir1, s1 := testServerWithConfig(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
 	retry.Run(t, func(r *retry.R) { r.Check(waitForLeader(s1)) })

agent/consul/intention_endpoint_test.go

@@ -1599,7 +1599,7 @@ func TestIntentionList_acl(t *testing.T) {
 
 	t.Parallel()
 
-	dir1, s1 := testServerWithConfig(t, testServerACLConfig(nil))
+	dir1, s1 := testServerWithConfig(t, testServerACLConfig)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
 	codec := rpcClient(t, s1)

agent/consul/internal_endpoint_test.go

@@ -1784,7 +1784,7 @@ func TestInternal_GatewayIntentions_aclDeny(t *testing.T) {
 		t.Skip("too slow for testing.Short")
 	}
 
-	dir1, s1 := testServerWithConfig(t, testServerACLConfig(nil))
+	dir1, s1 := testServerWithConfig(t, testServerACLConfig)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
 	codec := rpcClient(t, s1)

agent/consul/server_test.go

@@ -66,21 +66,12 @@ func testTLSCertificates(serverName string) (cert string, key string, cacert str
 	return cert, privateKey, ca, nil
 }
 
-// testServerACLConfig wraps another arbitrary Config altering callback
+// testServerACLConfig setup some common ACL configurations.
-// to setup some common ACL configurations. A new callback func will
+func testServerACLConfig(c *Config) {
-// be returned that has the original callback invoked after setting
-// up all of the ACL configurations (so they can still be overridden)
-func testServerACLConfig(cb func(*Config)) func(*Config) {
-	return func(c *Config) {
 	c.PrimaryDatacenter = "dc1"
 	c.ACLsEnabled = true
 	c.ACLInitialManagementToken = TestDefaultMasterToken
 	c.ACLResolverSettings.ACLDefaultPolicy = "deny"
-
-		if cb != nil {
-			cb(c)
-		}
-	}
 }
 
 func configureTLS(config *Config) {
@@ -164,8 +155,6 @@ func testServerConfig(t *testing.T) (string, *Config) {
 	config.ServerHealthInterval = 50 * time.Millisecond
 	config.AutopilotInterval = 100 * time.Millisecond
 
-	config.Build = "1.7.2"
-
 	config.CoordinateUpdatePeriod = 100 * time.Millisecond
 	config.LeaveDrainTime = 1 * time.Millisecond
 
@@ -187,14 +176,12 @@ func testServerConfig(t *testing.T) (string, *Config) {
 	return dir, config
 }
 
+// Deprecated: use testServerWithConfig instead. It does the same thing and more.
 func testServer(t *testing.T) (string, *Server) {
-	return testServerWithConfig(t, func(c *Config) {
+	return testServerWithConfig(t)
-		c.Datacenter = "dc1"
-		c.PrimaryDatacenter = "dc1"
-		c.Bootstrap = true
-	})
 }
 
+// Deprecated: use testServerWithConfig
 func testServerDC(t *testing.T, dc string) (string, *Server) {
 	return testServerWithConfig(t, func(c *Config) {
 		c.Datacenter = dc
@@ -202,6 +189,7 @@ func testServerDC(t *testing.T, dc string) (string, *Server) {
 	})
 }
 
+// Deprecated: use testServerWithConfig
 func testServerDCBootstrap(t *testing.T, dc string, bootstrap bool) (string, *Server) {
 	return testServerWithConfig(t, func(c *Config) {
 		c.Datacenter = dc
@@ -210,6 +198,7 @@ func testServerDCBootstrap(t *testing.T, dc string, bootstrap bool) (string, *Se
 	})
 }
 
+// Deprecated: use testServerWithConfig
 func testServerDCExpect(t *testing.T, dc string, expect int) (string, *Server) {
 	return testServerWithConfig(t, func(c *Config) {
 		c.Datacenter = dc
@@ -218,16 +207,7 @@ func testServerDCExpect(t *testing.T, dc string, expect int) (string, *Server) {
 	})
 }
 
-func testServerDCExpectNonVoter(t *testing.T, dc string, expect int) (string, *Server) {
+func testServerWithConfig(t *testing.T, configOpts ...func(*Config)) (string, *Server) {
-	return testServerWithConfig(t, func(c *Config) {
-		c.Datacenter = dc
-		c.Bootstrap = false
-		c.BootstrapExpect = expect
-		c.ReadReplica = true
-	})
-}
-
-func testServerWithConfig(t *testing.T, cb func(*Config)) (string, *Server) {
 	var dir string
 	var srv *Server
 
@@ -235,8 +215,8 @@ func testServerWithConfig(t *testing.T, cb func(*Config)) (string, *Server) {
 	retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) {
 		var config *Config
 		dir, config = testServerConfig(t)
-		if cb != nil {
+		for _, fn := range configOpts {
-			cb(config)
+			fn(config)
 		}
 
 		// Apply config to copied fields because many tests only set the old
@@ -257,8 +237,11 @@ func testServerWithConfig(t *testing.T, cb func(*Config)) (string, *Server) {
 
 // cb is a function that can alter the test servers configuration prior to the server starting.
 func testACLServerWithConfig(t *testing.T, cb func(*Config), initReplicationToken bool) (string, *Server, rpc.ClientCodec) {
-	dir, srv := testServerWithConfig(t, testServerACLConfig(cb))
+	opts := []func(*Config){testServerACLConfig}
-	t.Cleanup(func() { srv.Shutdown() })
+	if cb != nil {
+		opts = append(opts, cb)
+	}
+	dir, srv := testServerWithConfig(t, opts...)
 
 	if initReplicationToken {
 		// setup some tokens here so we get less warnings in the logs
@@ -266,7 +249,6 @@ func testACLServerWithConfig(t *testing.T, cb func(*Config), initReplicationToke
 	}
 
 	codec := rpcClient(t, srv)
-	t.Cleanup(func() { codec.Close() })
 	return dir, srv, codec
 }
 
@@ -1284,7 +1266,11 @@ func TestServer_Expect_NonVoters(t *testing.T) {
 	}
 
 	t.Parallel()
-	dir1, s1 := testServerDCExpectNonVoter(t, "dc1", 2)
+	dir1, s1 := testServerWithConfig(t, func(c *Config) {
+		c.Bootstrap = false
+		c.BootstrapExpect = 2
+		c.ReadReplica = true
+	})
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
 

agent/consul/system_metadata_test.go

@@ -4,9 +4,10 @@ import (
 	"os"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/testrpc"
-	"github.com/stretchr/testify/require"
 )
 
 func TestLeader_SystemMetadata_CRUD(t *testing.T) {
@@ -32,10 +33,10 @@ func TestLeader_SystemMetadata_CRUD(t *testing.T) {
 
 	state := srv.fsm.State()
 
-	// Initially has no entries
+	// Initially has one entry for virtual-ips feature flag
 	_, entries, err := state.SystemMetadataList(nil)
 	require.NoError(t, err)
-	require.Len(t, entries, 0)
+	require.Len(t, entries, 1)
 
 	// Create 3
 	require.NoError(t, srv.setSystemMetadataKey("key1", "val1"))
@@ -52,9 +53,10 @@ func TestLeader_SystemMetadata_CRUD(t *testing.T) {
 
 	_, entries, err = state.SystemMetadataList(nil)
 	require.NoError(t, err)
-	require.Len(t, entries, 3)
+	require.Len(t, entries, 4)
 
 	require.Equal(t, map[string]string{
+		structs.SystemMetadataVirtualIPsEnabled: "true",
 		"key1":                                  "val1",
 		"key2":                                  "val2",
 		"key3":                                  "",
@@ -66,9 +68,10 @@ func TestLeader_SystemMetadata_CRUD(t *testing.T) {
 
 	_, entries, err = state.SystemMetadataList(nil)
 	require.NoError(t, err)
-	require.Len(t, entries, 2)
+	require.Len(t, entries, 3)
 
 	require.Equal(t, map[string]string{
+		structs.SystemMetadataVirtualIPsEnabled: "true",
 		"key2":                                  "val2",
 		"key3":                                  "val3",
 	}, mapify(entries))