From c9a992f5e881510de81b833dc88e6456f9f456ba Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 9 Dec 2021 18:57:22 -0500 Subject: [PATCH 1/2] testing: remove old config.Build version DefaultConfig already sets the version to version.Version, so by removing this our tests will run with the version that matches the code. --- agent/consul/server_test.go | 2 -- agent/consul/system_metadata_test.go | 23 +++++++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index d608b8fa0b..da24f32200 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -164,8 +164,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 diff --git a/agent/consul/system_metadata_test.go b/agent/consul/system_metadata_test.go index 30f57defd5..61f62c8d15 100644 --- a/agent/consul/system_metadata_test.go +++ b/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,12 +53,13 @@ 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{ - "key1": "val1", - "key2": "val2", - "key3": "", + structs.SystemMetadataVirtualIPsEnabled: "true", + "key1": "val1", + "key2": "val2", + "key3": "", }, mapify(entries)) // Update one and delete one. @@ -66,10 +68,11 @@ 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{ - "key2": "val2", - "key3": "val3", + structs.SystemMetadataVirtualIPsEnabled: "true", + "key2": "val2", + "key3": "val3", }, mapify(entries)) } From 0a9cb62859172588e71da919e21b71f98afa6f53 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 9 Dec 2021 19:08:40 -0500 Subject: [PATCH 2/2] testing: Deprecate functions for creating a server. These helper functions actually end up hiding important setup details that should be visible from the test case. We already have a convenient way of setting this config when calling newTestServerWithConfig. --- agent/consul/autopilot_test.go | 9 ++-- agent/consul/intention_endpoint_test.go | 2 +- agent/consul/internal_endpoint_test.go | 2 +- agent/consul/server_test.go | 60 ++++++++++--------------- 4 files changed, 31 insertions(+), 42 deletions(-) diff --git a/agent/consul/autopilot_test.go b/agent/consul/autopilot_test.go index 8b26324112..1935fc5e8c 100644 --- a/agent/consul/autopilot_test.go +++ b/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)) }) diff --git a/agent/consul/intention_endpoint_test.go b/agent/consul/intention_endpoint_test.go index ec39413489..ec8349f829 100644 --- a/agent/consul/intention_endpoint_test.go +++ b/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) diff --git a/agent/consul/internal_endpoint_test.go b/agent/consul/internal_endpoint_test.go index f9105304fe..7a354e7b58 100644 --- a/agent/consul/internal_endpoint_test.go +++ b/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) diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index da24f32200..0c7b842233 100644 --- a/agent/consul/server_test.go +++ b/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 -// to setup some common ACL configurations. A new callback func will -// 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) - } - } +// testServerACLConfig setup some common ACL configurations. +func testServerACLConfig(c *Config) { + c.PrimaryDatacenter = "dc1" + c.ACLsEnabled = true + c.ACLInitialManagementToken = TestDefaultMasterToken + c.ACLResolverSettings.ACLDefaultPolicy = "deny" } func configureTLS(config *Config) { @@ -185,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) { - c.Datacenter = "dc1" - c.PrimaryDatacenter = "dc1" - c.Bootstrap = true - }) + return testServerWithConfig(t) } +// Deprecated: use testServerWithConfig func testServerDC(t *testing.T, dc string) (string, *Server) { return testServerWithConfig(t, func(c *Config) { c.Datacenter = dc @@ -200,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 @@ -208,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 @@ -216,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) { - 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) { +func testServerWithConfig(t *testing.T, configOpts ...func(*Config)) (string, *Server) { var dir string var srv *Server @@ -233,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 { - cb(config) + for _, fn := range configOpts { + fn(config) } // Apply config to copied fields because many tests only set the old @@ -255,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)) - t.Cleanup(func() { srv.Shutdown() }) + opts := []func(*Config){testServerACLConfig} + 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 @@ -264,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 } @@ -1282,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()