testing: Remove TestAgent.Key and change TestAgent.DataDir

TestAgent.Key was only used by 3 tests. Extracting it from the common helper that is used in hundreds of
tests helps keep the shared part small and more focused.

This required a second change (which I was planning on making anyway), which was to change the behaviour of
DataDir. Now in all cases the TestAgent will use the DataDir, and clean it up once the test is complete.

agent/agent_test.go

@@ -1536,15 +1536,12 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) {
 		a.logger.Info("testharness: " + fmt.Sprintf(format, args...))
 	}
 
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
 	cfg := `
 		server = false
 		bootstrap = false
 	    enable_central_service_config = false
-		data_dir = "` + dataDir + `"
 	`
-	a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a := StartTestAgent(t, TestAgent{HCL: cfg})
-	defer os.RemoveAll(dataDir)
 	defer a.Shutdown()
 
 	testCtx, testCancel := context.WithCancel(context.Background())
@@ -1627,7 +1624,7 @@ node_name = "` + a.Config.NodeName + `"
 		t.Helper()
 
 		// Reload and retain former NodeID and data directory.
-		a2 := StartTestAgent(t, TestAgent{HCL: futureHCL, DataDir: dataDir})
+		a2 := StartTestAgent(t, TestAgent{HCL: futureHCL, DataDir: a.DataDir})
 		defer a2.Shutdown()
 		a = nil
 
@@ -2012,15 +2009,11 @@ func TestAgent_PersistService(t *testing.T) {
 func testAgent_PersistService(t *testing.T, extraHCL string) {
 	t.Helper()
 
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
-	defer os.RemoveAll(dataDir)
-
 	cfg := `
 		server = false
 		bootstrap = false
-		data_dir = "` + dataDir + `"
 	` + extraHCL
-	a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a := StartTestAgent(t, TestAgent{HCL: cfg})
 	defer a.Shutdown()
 
 	svc := &structs.NodeService{
@@ -2086,7 +2079,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) {
 	a.Shutdown()
 
 	// Should load it back during later start
-	a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: a.DataDir})
 	defer a2.Shutdown()
 
 	restored := a2.State.ServiceState(structs.NewServiceID(svc.ID, nil))
@@ -2224,15 +2217,11 @@ func TestAgent_PurgeServiceOnDuplicate(t *testing.T) {
 func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) {
 	t.Helper()
 
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
-	defer os.RemoveAll(dataDir)
-
 	cfg := `
-		data_dir = "` + dataDir + `"
 		server = false
 		bootstrap = false
 	` + extraHCL
-	a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a := StartTestAgent(t, TestAgent{HCL: cfg})
 	defer a.Shutdown()
 
 	svc1 := &structs.NodeService{
@@ -2255,7 +2244,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) {
 			tags = ["bar"]
 			port = 9000
 		}
-	`, DataDir: dataDir})
+	`, DataDir: a.DataDir})
 	defer a2.Shutdown()
 
 	sid := svc1.CompoundServiceID()
@@ -2269,15 +2258,12 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) {
 
 func TestAgent_PersistCheck(t *testing.T) {
 	t.Parallel()
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
 	cfg := `
-		data_dir = "` + dataDir + `"
 		server = false
 		bootstrap = false
 		enable_script_checks = true
 	`
-	a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a := StartTestAgent(t, TestAgent{HCL: cfg})
-	defer os.RemoveAll(dataDir)
 	defer a.Shutdown()
 
 	check := &structs.HealthCheck{
@@ -2333,7 +2319,7 @@ func TestAgent_PersistCheck(t *testing.T) {
 	a.Shutdown()
 
 	// Should load it back during later start
-	a2 := StartTestAgent(t, TestAgent{Name: "Agent2", HCL: cfg, DataDir: dataDir})
+	a2 := StartTestAgent(t, TestAgent{Name: "Agent2", HCL: cfg, DataDir: a.DataDir})
 	defer a2.Shutdown()
 
 	result := requireCheckExists(t, a2, check.CheckID)
@@ -2384,18 +2370,14 @@ func TestAgent_PurgeCheck(t *testing.T) {
 func TestAgent_PurgeCheckOnDuplicate(t *testing.T) {
 	t.Parallel()
 	nodeID := NodeID()
-	dataDir := testutil.TempDir(t, "agent")
 	a := StartTestAgent(t, TestAgent{
-		DataDir: dataDir,
 		HCL: `
 	    node_id = "` + nodeID + `"
 	    node_name = "Node ` + nodeID + `"
-		data_dir = "` + dataDir + `"
 		server = false
 		bootstrap = false
 		enable_script_checks = true
 	`})
-	defer os.RemoveAll(dataDir)
 	defer a.Shutdown()
 
 	check1 := &structs.HealthCheck{
@@ -2415,11 +2397,10 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) {
 	// Start again with the check registered in config
 	a2 := StartTestAgent(t, TestAgent{
 		Name:    "Agent2",
-		DataDir: dataDir,
+		DataDir: a.DataDir,
 		HCL: `
 	    node_id = "` + nodeID + `"
 	    node_name = "Node ` + nodeID + `"
-		data_dir = "` + dataDir + `"
 		server = false
 		bootstrap = false
 		enable_script_checks = true
@@ -2434,7 +2415,7 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) {
 	defer a2.Shutdown()
 
 	cid := check1.CompoundCheckID()
-	file := filepath.Join(dataDir, checksDir, cid.StringHash())
+	file := filepath.Join(a.DataDir, checksDir, cid.StringHash())
 	if _, err := os.Stat(file); err == nil {
 		t.Fatalf("should have removed persisted check")
 	}

agent/keyring_test.go

@@ -54,7 +54,10 @@ func TestAgent_LoadKeyrings(t *testing.T) {
 
 	// Server should auto-load LAN and WAN keyring files
 	t.Run("server with keys", func(t *testing.T) {
-		a2 := StartTestAgent(t, TestAgent{Key: key})
+		dataDir := testutil.TempDir(t, "keyfile")
+		writeKeyRings(t, key, dataDir)
+
+		a2 := StartTestAgent(t, TestAgent{DataDir: dataDir})
 		defer a2.Shutdown()
 
 		c2 := a2.consulConfig()
@@ -80,10 +83,16 @@ func TestAgent_LoadKeyrings(t *testing.T) {
 
 	// Client should auto-load only the LAN keyring file
 	t.Run("client with keys", func(t *testing.T) {
-		a3 := StartTestAgent(t, TestAgent{HCL: `
+		dataDir := testutil.TempDir(t, "keyfile")
+		writeKeyRings(t, key, dataDir)
+
+		a3 := StartTestAgent(t, TestAgent{
+			HCL: `
 			server = false
 			bootstrap = false
-		`, Key: key})
+			`,
+			DataDir: dataDir,
+		})
 		defer a3.Shutdown()
 
 		c3 := a3.consulConfig()
@@ -105,6 +114,16 @@ func TestAgent_LoadKeyrings(t *testing.T) {
 	})
 }
 
+func writeKeyRings(t *testing.T, key string, dataDir string) {
+	t.Helper()
+	writeKey := func(key, filename string) {
+		path := filepath.Join(dataDir, filename)
+		require.NoError(t, initKeyring(path, key), "Error creating keyring %s", path)
+	}
+	writeKey(key, SerfLANKeyring)
+	writeKey(key, SerfWANKeyring)
+}
+
 func TestAgent_InmemKeyrings(t *testing.T) {
 	t.Parallel()
 	key := "tbLJg26ZJyJ9pK3qhc9jig=="
@@ -272,11 +291,14 @@ func TestAgentKeyring_ACL(t *testing.T) {
 	key1 := "tbLJg26ZJyJ9pK3qhc9jig=="
 	key2 := "4leC33rgtXKIVUr9Nr0snQ=="
 
+	dataDir := testutil.TempDir(t, "keyfile")
+	writeKeyRings(t, key1, dataDir)
+
 	a := StartTestAgent(t, TestAgent{HCL: TestACLConfig() + `
 		acl_datacenter = "dc1"
 		acl_master_token = "root"
 		acl_default_policy = "deny"
-	`, Key: key1})
+	`, DataDir: dataDir})
 	defer a.Shutdown()
 
 	// List keys without access fails

agent/service_manager_test.go

@@ -9,7 +9,6 @@ import (
 	"testing"
 
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
 	"github.com/stretchr/testify/require"
@@ -293,16 +292,12 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 	)
 
 	// Now launch a single client agent
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
-	defer os.RemoveAll(dataDir)
-
 	cfg := `
 	    enable_central_service_config = true
 		server = false
 		bootstrap = false
-		data_dir = "` + dataDir + `"
 	`
-	a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a := StartTestAgent(t, TestAgent{HCL: cfg})
 	defer a.Shutdown()
 
 	// Join first
@@ -465,7 +460,7 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 	serverAgent.Shutdown()
 
 	// Should load it back during later start.
-	a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: a.DataDir})
 	defer a2.Shutdown()
 
 	{
@@ -510,9 +505,6 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) {
 	)
 
 	// Now launch a single client agent
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
-	defer os.RemoveAll(dataDir)
-
 	serviceSnippet := `
 		service = {
 		  kind  = "connect-proxy"
@@ -535,12 +527,11 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) {
 
 	cfg := `
 	    enable_central_service_config = true
-		data_dir = "` + dataDir + `"
 		server = false
 		bootstrap = false
 	` + serviceSnippet
 
-	a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a := StartTestAgent(t, TestAgent{HCL: cfg})
 	defer a.Shutdown()
 
 	// Join first
@@ -639,7 +630,7 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) {
 	serverAgent.Shutdown()
 
 	// Should load it back during later start.
-	a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir})
+	a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: a.DataDir})
 	defer a2.Shutdown()
 
 	{

agent/testagent.go

@@ -8,13 +8,11 @@ import (
 	"crypto/x509"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"math/rand"
 	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"testing"
 	"text/template"
 	"time"
@@ -58,23 +56,18 @@ type TestAgent struct {
 	// the callers responsibility to clean up the data directory.
 	// Otherwise, a temporary data directory is created and removed
 	// when Shutdown() is called.
-	// TODO:
 	Config *config.RuntimeConfig
 
 	// LogOutput is the sink for the logs. If nil, logs are written
 	// to os.Stderr.
 	LogOutput io.Writer
 
-	// DataDir is the data directory which is used when Config.DataDir
+	// DataDir may be set to a directory which exists. If is it not set,
-	// is not set. It is created automatically and removed when
+	// TestAgent.Start will create one and set DataDir to the directory path.
-	// Shutdown() is called.
+	// In all cases the agent will be configured to use this path as the data directory,
-	// TODO:
+	// and the directory will be removed once the test ends.
 	DataDir string
 
-	// Key is the optional encryption key for the LAN and WAN keyring.
-	// TODO:
-	Key string
-
 	// UseTLS, if true, will disable the HTTP port and enable the HTTPS
 	// one.
 	UseTLS bool
@@ -154,35 +147,14 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 		name = "TestAgent"
 	}
 
-	var cleanupTmpDir = func() {
-		// Clean out the data dir if we are responsible for it before we
-		// try again, since the old ports may have gotten written to
-		// the data dir, such as in the Raft configuration.
-		if a.DataDir != "" {
-			if err := os.RemoveAll(a.DataDir); err != nil {
-				fmt.Printf("%s Error resetting data dir: %s", name, err)
-			}
-		}
-	}
-
-	var hclDataDir string
 	if a.DataDir == "" {
-		dirname := "agent"
+		dirname := name + "-agent"
-		if name != "" {
+		a.DataDir = testutil.TempDir(t, dirname)
-			dirname = name + "-agent"
-		}
-		dirname = strings.Replace(dirname, "/", "_", -1)
-		d, err := ioutil.TempDir(TempDir, dirname)
-		if err != nil {
-			return fmt.Errorf("Error creating data dir %s: %s", filepath.Join(TempDir, dirname), err)
-		}
-		// Convert windows style path to posix style path
-		// to avoid illegal char escape error when hcl
-		// parsing.
-		d = filepath.ToSlash(d)
-		hclDataDir = `data_dir = "` + d + `"`
-		a.DataDir = d
 	}
+	// Convert windows style path to posix style path to avoid illegal char escape
+	// error when hcl parsing.
+	d := filepath.ToSlash(a.DataDir)
+	hclDataDir := fmt.Sprintf(`data_dir = "%s"`, d)
 
 	logOutput := a.LogOutput
 	if logOutput == nil {
@@ -220,29 +192,8 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 		),
 	}
 
-	// write the keyring
-	if a.Key != "" {
-		writeKey := func(key, filename string) error {
-			path := filepath.Join(a.DataDir, filename)
-			if err := initKeyring(path, key); err != nil {
-				cleanupTmpDir()
-				return fmt.Errorf("Error creating keyring %s: %s", path, err)
-			}
-			return nil
-		}
-		if err = writeKey(a.Key, SerfLANKeyring); err != nil {
-			cleanupTmpDir()
-			return err
-		}
-		if err = writeKey(a.Key, SerfWANKeyring); err != nil {
-			cleanupTmpDir()
-			return err
-		}
-	}
-
 	agent, err := New(opts...)
 	if err != nil {
-		cleanupTmpDir()
 		return fmt.Errorf("Error creating agent: %s", err)
 	}
 
@@ -253,7 +204,6 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 	id := string(a.Config.NodeID)
 
 	if err := agent.Start(context.Background()); err != nil {
-		cleanupTmpDir()
 		agent.ShutdownAgent()
 		agent.ShutdownEndpoints()
 
@@ -266,7 +216,6 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 	a.Agent.StartSync()
 
 	if err := a.waitForUp(); err != nil {
-		cleanupTmpDir()
 		a.Shutdown()
 		t.Logf("Error while waiting for test agent to start: %v", err)
 		return errwrap.Wrapf(name+": {{err}}", err)