From 447097b166743e2b27003c513f78e4f622f0eae9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 2 Dec 2021 14:57:06 -0500 Subject: [PATCH] ca: make test naming consistent While working on the CA system it is important to be able to run all the tests related to the system, without having to wait for unrelated tests. There are many slow and unrelated tests in agent/consul, so we need some way to filter to only the relevant tests. This PR renames all the CA system related tests to start with either `TestCAMananger` for tests of internal operations that don't have RPC endpoint, or `TestConnectCA` for tests of RPC endpoints. This allows us to run all the test with: go test -run 'TestCAMananger|TestConnectCA' ./agent/consul The test naming follows an undocumented convention of naming tests as follows: Test[_][_] I tried to always keep Primary/Secondary at the end of the description, and _Vault_ has to be in the middle because of our regex to run those tests as a separate CI job. You may notice some of the test names changed quite a bit. I did my best to identify the underlying method being tested, but I may have been slightly off in some cases. --- agent/consul/leader_connect_ca.go | 12 ++++---- agent/consul/leader_connect_ca_test.go | 41 +++++++++++++++++++++++--- agent/consul/leader_connect_test.go | 34 ++++++++++----------- agent/consul/server_test.go | 32 -------------------- 4 files changed, 60 insertions(+), 59 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index e208694213..81e92f1ce8 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -311,7 +311,7 @@ func (c *CAManager) Start(ctx context.Context) { // Attempt to initialize the Connect CA now. This will // happen during leader establishment and it would be great // if the CA was ready to go once that process was finished. - if err := c.InitializeCA(); err != nil { + if err := c.Initialize(); err != nil { c.logger.Error("Failed to initialize Connect CA", "error", err) // we failed to fully initialize the CA so we need to spawn a @@ -351,7 +351,7 @@ func (c *CAManager) startPostInitializeRoutines(ctx context.Context) { } func (c *CAManager) backgroundCAInitialization(ctx context.Context) error { - retryLoopBackoffAbortOnSuccess(ctx, c.InitializeCA, func(err error) { + retryLoopBackoffAbortOnSuccess(ctx, c.Initialize, func(err error) { c.logger.Error("Failed to initialize Connect CA", "routine", backgroundCAInitializationRoutineName, "error", err, @@ -368,10 +368,10 @@ func (c *CAManager) backgroundCAInitialization(ctx context.Context) error { return nil } -// InitializeCA sets up the CA provider when gaining leadership, either bootstrapping +// Initialize sets up the CA provider when gaining leadership, either bootstrapping // the CA if this is the primary DC or making a remote RPC for intermediate signing // if this is a secondary DC. -func (c *CAManager) InitializeCA() (reterr error) { +func (c *CAManager) Initialize() (reterr error) { // Bail if connect isn't enabled. if !c.serverConf.ConnectEnabled { return nil @@ -795,7 +795,7 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) } }() - // Attempt to initialize the config if we failed to do so in InitializeCA for some reason + // Attempt to initialize the config if we failed to do so in Initialize for some reason _, err = c.initializeCAConfig() if err != nil { return err @@ -1257,7 +1257,7 @@ func (c *CAManager) secondaryUpdateRoots(roots structs.IndexedCARoots) error { } // Attempt to initialize now that we have updated roots. This is an optimization - // so that we don't have to wait for the InitializeCA retry backoff if we were + // so that we don't have to wait for the Initialize retry backoff if we were // waiting on roots from the primary to be able to complete initialization. if err := c.delegate.ServersSupportMultiDCConnectCA(); err != nil { return fmt.Errorf("failed to initialize while updating primary roots: %w", err) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index a1ca8c12df..176eb2f55f 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -23,6 +23,8 @@ import ( "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/consul/sdk/testutil/retry" + "github.com/hashicorp/consul/testrpc" ) // TODO(kyhavlov): replace with t.Deadline() @@ -221,7 +223,7 @@ func initTestManager(t *testing.T, manager *CAManager, delegate *mockCAServerDel t.Helper() initCh := make(chan struct{}) go func() { - require.NoError(t, manager.InitializeCA()) + require.NoError(t, manager.Initialize()) close(initCh) }() for i := 0; i < 5; i++ { @@ -251,12 +253,12 @@ func TestCAManager_Initialize(t *testing.T) { rootPEM: delegate.primaryRoot.RootCert, } - // Call InitializeCA and then confirm the RPCs and provider calls + // Call Initialize and then confirm the RPCs and provider calls // happen in the expected order. require.Equal(t, caStateUninitialized, manager.state) errCh := make(chan error) go func() { - err := manager.InitializeCA() + err := manager.Initialize() assert.NoError(t, err) errCh <- err }() @@ -269,7 +271,7 @@ func TestCAManager_Initialize(t *testing.T) { waitForCh(t, delegate.callbackCh, "raftApply/ConnectCA") waitForEmptyCh(t, delegate.callbackCh) - // Make sure the InitializeCA call returned successfully. + // Make sure the Initialize call returned successfully. select { case err := <-errCh: require.NoError(t, err) @@ -462,3 +464,34 @@ func TestCADelegateWithState_GenerateCASignRequest(t *testing.T) { req := d.generateCASignRequest("A") require.Equal(t, "east", req.RequestDatacenter()) } + +func TestCAManager_Initialize_Logging(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + t.Parallel() + _, conf1 := testServerConfig(t) + + // Setup dummy logger to catch output + var buf bytes.Buffer + logger := testutil.LoggerWithOutput(t, &buf) + + deps := newDefaultDeps(t, conf1) + deps.Logger = logger + + s1, err := NewServer(conf1, deps) + require.NoError(t, err) + defer s1.Shutdown() + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + // Wait til CA root is setup + retry.Run(t, func(r *retry.R) { + var out structs.IndexedCARoots + r.Check(s1.RPC("ConnectCA.Roots", structs.DCSpecificRequest{ + Datacenter: conf1.Datacenter, + }, &out)) + }) + + require.Contains(t, buf.String(), "consul CA provider configured") +} diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index f4a65bf9e3..18b7aabc7e 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -24,7 +24,7 @@ import ( "github.com/hashicorp/consul/testrpc" ) -func TestLeader_Builtin_PrimaryCA_ChangeKeyConfig(t *testing.T) { +func TestConnectCA_ConfigurationSet_ChangeKeyConfig_Primary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -175,7 +175,7 @@ func TestLeader_Builtin_PrimaryCA_ChangeKeyConfig(t *testing.T) { } -func TestLeader_SecondaryCA_Initialize(t *testing.T) { +func TestCAManager_Initialize_Secondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -330,7 +330,7 @@ func getCAProviderWithLock(s *Server) (ca.Provider, *structs.CARoot) { return s.caManager.getCAProvider() } -func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) { +func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -450,7 +450,7 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) { require.NoError(err) } -func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { +func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -587,7 +587,7 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { require.NoError(err) } -func TestLeader_SecondaryCA_IntermediateRefresh(t *testing.T) { +func TestConnectCA_ConfigurationSet_RootRotation_Secondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -738,7 +738,7 @@ func TestLeader_SecondaryCA_IntermediateRefresh(t *testing.T) { require.NoError(err) } -func TestLeader_Vault_PrimaryCA_FixSigningKeyID_OnRestart(t *testing.T) { +func TestCAManager_Initialize_Vault_FixesSigningKeyID_Primary(t *testing.T) { ca.SkipIfVaultNotPresent(t) if testing.Short() { @@ -840,7 +840,7 @@ func TestLeader_Vault_PrimaryCA_FixSigningKeyID_OnRestart(t *testing.T) { }) } -func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T) { +func TestCAManager_Initialize_FixesSigningKeyID_Secondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -941,7 +941,7 @@ func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T }) } -func TestLeader_SecondaryCA_TransitionFromPrimary(t *testing.T) { +func TestCAManager_Initialize_TransitionFromPrimaryToSecondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -1033,7 +1033,7 @@ func TestLeader_SecondaryCA_TransitionFromPrimary(t *testing.T) { }) } -func TestLeader_SecondaryCA_UpgradeBeforePrimary(t *testing.T) { +func TestCAManager_Initialize_SecondaryBeforePrimary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -1242,7 +1242,7 @@ func TestLeader_CARootPruning(t *testing.T) { require.NotEqual(roots[0].ID, oldRoot.ID) } -func TestLeader_PersistIntermediateCAs(t *testing.T) { +func TestConnectCA_ConfigurationSet_PersistsRoots(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -1325,7 +1325,7 @@ func TestLeader_PersistIntermediateCAs(t *testing.T) { }) } -func TestLeader_ParseCARoot(t *testing.T) { +func TestParseCARoot(t *testing.T) { type test struct { name string pem string @@ -1408,7 +1408,7 @@ func readTestData(t *testing.T, name string) string { return string(bs) } -func TestLeader_lessThanHalfTimePassed(t *testing.T) { +func TestLessThanHalfTimePassed(t *testing.T) { now := time.Now() require.False(t, lessThanHalfTimePassed(now, now.Add(-10*time.Second), now.Add(-5*time.Second))) require.False(t, lessThanHalfTimePassed(now, now.Add(-10*time.Second), now)) @@ -1418,7 +1418,7 @@ func TestLeader_lessThanHalfTimePassed(t *testing.T) { require.True(t, lessThanHalfTimePassed(now, now.Add(-10*time.Second), now.Add(20*time.Second))) } -func TestLeader_retryLoopBackoffHandleSuccess(t *testing.T) { +func TestRetryLoopBackoffHandleSuccess(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -1462,7 +1462,7 @@ func TestLeader_retryLoopBackoffHandleSuccess(t *testing.T) { } } -func TestLeader_Vault_BadCAConfigShouldntPreventLeaderEstablishment(t *testing.T) { +func TestCAManager_Initialize_Vault_BadCAConfigDoesNotPreventLeaderEstablishment(t *testing.T) { ca.SkipIfVaultNotPresent(t) testVault := ca.NewTestVaultServer(t) @@ -1519,7 +1519,7 @@ func TestLeader_Vault_BadCAConfigShouldntPreventLeaderEstablishment(t *testing.T require.NotNil(t, activeRoot) } -func TestLeader_Consul_BadCAConfigShouldntPreventLeaderEstablishment(t *testing.T) { +func TestCAManager_Initialize_BadCAConfigDoesNotPreventLeaderEstablishment(t *testing.T) { ca.SkipIfVaultNotPresent(t) _, s1 := testServerWithConfig(t, func(c *Config) { @@ -1563,7 +1563,7 @@ func TestLeader_Consul_BadCAConfigShouldntPreventLeaderEstablishment(t *testing. require.NotNil(t, activeRoot) } -func TestLeader_Consul_ForceWithoutCrossSigning(t *testing.T) { +func TestConnectCA_ConfigurationSet_ForceWithoutCrossSigning(t *testing.T) { require := require.New(t) dir1, s1 := testServer(t) defer os.RemoveAll(dir1) @@ -1619,7 +1619,7 @@ func TestLeader_Consul_ForceWithoutCrossSigning(t *testing.T) { } } -func TestLeader_Vault_ForceWithoutCrossSigning(t *testing.T) { +func TestConnectCA_ConfigurationSet_Vault_ForceWithoutCrossSigning(t *testing.T) { ca.SkipIfVaultNotPresent(t) require := require.New(t) diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 978d2453cd..857a900525 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -1,7 +1,6 @@ package consul import ( - "bytes" "crypto/x509" "fmt" "net" @@ -1711,34 +1710,3 @@ func TestServer_RPC_RateLimit(t *testing.T) { } }) } - -func TestServer_CALogging(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, conf1 := testServerConfig(t) - - // Setup dummy logger to catch output - var buf bytes.Buffer - logger := testutil.LoggerWithOutput(t, &buf) - - deps := newDefaultDeps(t, conf1) - deps.Logger = logger - - s1, err := NewServer(conf1, deps) - require.NoError(t, err) - defer s1.Shutdown() - testrpc.WaitForLeader(t, s1.RPC, "dc1") - - // Wait til CA root is setup - retry.Run(t, func(r *retry.R) { - var out structs.IndexedCARoots - r.Check(s1.RPC("ConnectCA.Roots", structs.DCSpecificRequest{ - Datacenter: conf1.Datacenter, - }, &out)) - }) - - require.Contains(t, buf.String(), "consul CA provider configured") -}