Merge pull request #8509 from hashicorp/dnephin/use-t.cleanup-in-testagent

testing: Use t.cleanup in TestAgent , and fix two flaky tests

agent/acl_test.go

@@ -3,6 +3,7 @@ package agent
 import (
 	"fmt"
 	"io"
+	"os"
 	"testing"
 	"time"
 
@@ -25,23 +26,6 @@ type authzResolver func(string) (structs.ACLIdentity, acl.Authorizer, error)
 type identResolver func(string) (structs.ACLIdentity, error)
 
 type TestACLAgent struct {
-	// Name is an optional name of the agent.
-	Name string
-
-	HCL string
-
-	// Config is the agent configuration. If Config is nil then
-	// TestConfig() is used. If Config.DataDir is set then it is
-	// the callers responsibility to clean up the data directory.
-	// Otherwise, a temporary data directory is created and removed
-	// when Shutdown() is called.
-	Config *config.RuntimeConfig
-
-	// DataDir is the data directory which is used when Config.DataDir
-	// is not set. It is created automatically and removed when
-	// Shutdown() is called.
-	DataDir string
-
 	resolveAuthzFn authzResolver
 	resolveIdentFn identResolver
 
@@ -52,11 +36,15 @@ type TestACLAgent struct {
 // Basically it needs a local state for some of the vet* functions, a logger and a delegate.
 // The key is that we are the delegate so we can control the ResolveToken responses
 func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzResolver, resolveIdent identResolver) *TestACLAgent {
-	a := &TestACLAgent{Name: name, HCL: hcl, resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent}
+	a := &TestACLAgent{resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent}
-	dataDir := `data_dir = "acl-agent"`
+
+	dataDir := testutil.TempDir(t, "acl-agent")
+	t.Cleanup(func() {
+		os.RemoveAll(dataDir)
+	})
 
 	logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{
-		Name:   a.Name,
+		Name:   name,
 		Level:  hclog.Debug,
 		Output: testutil.NewLogBuffer(t),
 	})
@@ -66,22 +54,22 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe
 		WithBuilderOpts(config.BuilderOpts{
 			HCL: []string{
 				TestConfigHCL(NodeID()),
-				a.HCL,
+				hcl,
-				dataDir,
+				fmt.Sprintf(`data_dir = "%s"`, dataDir),
 			},
 		}),
 	}
 
 	agent, err := New(opts...)
 	require.NoError(t, err)
-	a.Config = agent.GetConfig()
+	cfg := agent.GetConfig()
 	a.Agent = agent
 
 	agent.logger = logger
 	agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute)
 
 	a.Agent.delegate = a
-	a.Agent.State = local.NewState(LocalConfig(a.Config), a.Agent.logger, a.Agent.tokens)
+	a.Agent.State = local.NewState(LocalConfig(cfg), logger, a.Agent.tokens)
 	a.Agent.State.TriggerSyncChanges = func() {}
 	return a
 }

agent/agent_endpoint_test.go

@@ -4504,12 +4504,15 @@ func TestAgent_Monitor(t *testing.T) {
 		req = req.WithContext(cancelCtx)
 
 		resp := httptest.NewRecorder()
-		errCh := make(chan error)
+		chErr := make(chan error)
+		chStarted := make(chan struct{})
 		go func() {
+			close(chStarted)
 			_, err := a.srv.AgentMonitor(resp, req)
-			errCh <- err
+			chErr <- err
 		}()
 
+		<-chStarted
 		require.NoError(t, a.Shutdown())
 
 		// Wait until we have received some type of logging output
@@ -4518,7 +4521,7 @@ func TestAgent_Monitor(t *testing.T) {
 		}, 3*time.Second, 100*time.Millisecond)
 
 		cancelFunc()
-		err := <-errCh
+		err := <-chErr
 		require.NoError(t, err)
 
 		got := resp.Body.String()

agent/agent_test.go

@@ -1652,15 +1652,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())
@@ -1743,7 +1740,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
 
@@ -2128,15 +2125,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{
@@ -2202,7 +2195,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))
@@ -2340,15 +2333,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{
@@ -2371,7 +2360,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) {
 			tags = ["bar"]
 			port = 9000
 		}
-	`, DataDir: dataDir})
+	`, DataDir: a.DataDir})
 	defer a2.Shutdown()
 
 	sid := svc1.CompoundServiceID()
@@ -2385,15 +2374,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{
@@ -2449,7 +2435,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)
@@ -2500,18 +2486,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{
@@ -2531,11 +2513,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
@@ -2550,7 +2531,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/dns_test.go

@@ -5830,16 +5830,12 @@ func TestDNS_NonExistentDC_RPC(t *testing.T) {
 		server = false
 	`)
 	defer c.Shutdown()
-	testrpc.WaitForLeader(t, s.RPC, "dc1")
 
 	// Join LAN cluster
 	addr := fmt.Sprintf("127.0.0.1:%d", s.Config.SerfPortLAN)
 	_, err := c.JoinLAN([]string{addr})
 	require.NoError(t, err)
-	retry.Run(t, func(r *retry.R) {
+	testrpc.WaitForTestAgent(t, c.RPC, "dc1")
-		require.Len(r, s.LANMembers(), 2)
-		require.Len(r, c.LANMembers(), 2)
-	})
 
 	m := new(dns.Msg)
 	m.SetQuestion("consul.service.dc2.consul.", dns.TypeANY)

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,10 @@ import (
 	"crypto/x509"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"math/rand"
 	"net/http/httptest"
-	"os"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"testing"
 	"text/template"
 	"time"
@@ -40,9 +37,6 @@ func init() {
 	rand.Seed(time.Now().UnixNano()) // seed random number generator
 }
 
-// TempDir defines the base dir for temporary directories.
-var TempDir = os.TempDir()
-
 // TestAgent encapsulates an Agent with a default configuration and
 // startup procedure suitable for testing. It panics if there are errors
 // during creation or startup instead of returning errors. It manages a
@@ -60,22 +54,16 @@ type TestAgent struct {
 	// when Shutdown() is called.
 	Config *config.RuntimeConfig
 
-	// returnPortsFn will put the ports claimed for the test back into the
-	// general freeport pool
-	returnPortsFn func()
-
 	// 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,
+	// and the directory will be removed once the test ends.
 	DataDir string
 
-	// Key is the optional encryption key for the LAN and WAN keyring.
-	Key string
-
 	// UseTLS, if true, will disable the HTTP port and enable the HTTPS
 	// one.
 	UseTLS bool
@@ -155,35 +143,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 {
@@ -198,7 +165,7 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 	})
 
 	portsConfig, returnPortsFn := randomPortsSource(a.UseTLS)
-	a.returnPortsFn = returnPortsFn
+	t.Cleanup(returnPortsFn)
 
 	nodeID := NodeID()
 
@@ -221,36 +188,8 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 		),
 	}
 
-	defer func() {
-		if err != nil && a.returnPortsFn != nil {
-			a.returnPortsFn()
-			a.returnPortsFn = nil
-		}
-	}()
-
-	// 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)
 	}
 
@@ -261,7 +200,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()
 
@@ -274,7 +212,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)
@@ -357,14 +294,6 @@ func (a *TestAgent) Shutdown() error {
 		return nil
 	}
 
-	// Return ports last of all
-	defer func() {
-		if a.returnPortsFn != nil {
-			a.returnPortsFn()
-			a.returnPortsFn = nil
-		}
-	}()
-
 	// shutdown agent before endpoints
 	defer a.Agent.ShutdownEndpoints()
 	if err := a.Agent.ShutdownAgent(); err != nil {

sdk/testutil/io.go

@@ -29,21 +29,28 @@ func init() {
 	}
 }
 
+var noCleanup = strings.ToLower(os.Getenv("TEST_NOCLEANUP")) == "true"
+
 // TempDir creates a temporary directory within tmpdir
 // with the name 'testname-name'. If the directory cannot
 // be created t.Fatal is called.
 func TempDir(t *testing.T, name string) string {
-	if t != nil && t.Name() != "" {
+	if t == nil {
-		name = t.Name() + "-" + name
+		panic("argument t must be non-nil")
 	}
+	name = t.Name() + "-" + name
 	name = strings.Replace(name, "/", "_", -1)
 	d, err := ioutil.TempDir(tmpdir, name)
 	if err != nil {
-		if t == nil {
-			panic(err)
-		}
 		t.Fatalf("err: %s", err)
 	}
+	t.Cleanup(func() {
+		if noCleanup {
+			t.Logf("skipping cleanup because TEST_NOCLEANUP was enabled")
+			return
+		}
+		os.RemoveAll(d)
+	})
 	return d
 }