From 3a4e62836ba2930e7d6419ed9d56e8d738c623f9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 13 Aug 2020 17:17:21 -0400 Subject: [PATCH] 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 | 39 +++++-------------- agent/keyring_test.go | 30 +++++++++++++-- agent/service_manager_test.go | 17 ++------- agent/testagent.go | 71 +++++------------------------------ 4 files changed, 50 insertions(+), 107 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 4b109b9675..4746fd3ff9 100644 --- a/agent/agent_test.go +++ b/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}) - defer os.RemoveAll(dataDir) + a := StartTestAgent(t, TestAgent{HCL: cfg}) 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}) - defer os.RemoveAll(dataDir) + a := StartTestAgent(t, TestAgent{HCL: cfg}) 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") } diff --git a/agent/keyring_test.go b/agent/keyring_test.go index 2b160d4f78..16acc01611 100644 --- a/agent/keyring_test.go +++ b/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 diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index e71e21f222..8cb3ce2a24 100644 --- a/agent/service_manager_test.go +++ b/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() { diff --git a/agent/testagent.go b/agent/testagent.go index 74570a368e..426de1a917 100644 --- a/agent/testagent.go +++ b/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 - // is not set. It is created automatically and removed when - // Shutdown() is called. - // TODO: + // DataDir may be set to a directory which exists. If is it not set, + // TestAgent.Start will create one and set DataDir to the directory path. + // 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. - // 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" - if name != "" { - 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 + dirname := name + "-agent" + a.DataDir = testutil.TempDir(t, dirname) } + // 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)