Test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault. #9214

mjarmy · 2020-06-12T16:34:08Z

This PR introduces unit tests that test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault.

The following changes have been made to the test suite to support these changes:

It is now possible to stop and restart individual vault.Cores in a vault.TestCluster, via the new methods StopCore() and RestartCore(). This allows tests to simulate bringing an individual node in a cluster up and down.
A raft.ServerAddressProvider can optionally be provided at the time the TestCluster is created, if the cluster is using integrated raft storage. In addition, there is an option called JoinRaftFollowers that specifies that the raft follower nodes should be joined to the raft leader at the time the cluster is started.
Instances of teststorage.ReusableRaftStorage are now created with a raft.ServerAddressProvider.

This reverts commit 39593defd0d4c6fd79aedfd37df6298391abb9db.

This reverts commit a776452.

vault/testing.go

helper/testhelpers/testhelpers.go

calvn · 2020-06-12T20:41:37Z

helper/testhelpers/testhelpers.go

@@ -520,17 +540,6 @@ func NewHardcodedServerAddressProvider(cluster *vault.TestCluster, baseClusterPo
 	}
 }

-// SetRaftAddressProviders sets a ServerAddressProvider for all the nodes in a
-// cluster.
-func SetRaftAddressProviders(t testing.T, cluster *vault.TestCluster, provider raftlib.ServerAddressProvider) {


How come this is no longer used/required?

I'm doing it inside the ReusableRaftStorage now

vault/testing.go

vault/external_tests/sealmigration/seal_migration_test.go

helper/testhelpers/testhelpers.go

vault/external_tests/sealmigration/seal_migration_test.go

vishalnayak

Outside of the one TODO that you have out there, it is looking good to me. Nice work!

Co-authored-by: Calvin Leung Huang <[email protected]>

alexanderbez

LGTM!

ncabatoff · 2020-06-16T17:58:20Z

helper/testhelpers/testhelpers.go

+
+			isLeader, _, _, err := core.Leader()
+			if err != nil {
+				t.Fatal(err)


I would probably not error out here. Allow errors until the timeout elapses in case the cluster is still coming up and behaving strangely.

(but still report the last error encountered upon timeout, don't just report a timeout)

ncabatoff · 2020-06-16T18:00:14Z

helper/testhelpers/testhelpers.go

+		time.Sleep(time.Second)
+	}
+
+	return 0, fmt.Errorf("timeout waiting leader")


0 is a valid index, I would return -1 here.

briankassouf · 2020-06-16T18:00:10Z

helper/testhelpers/testhelpers.go

-		} else {
-			cluster.UnsealCore(t, core)
-		}
+		cluster.UnsealCore(t, core)


I don't think this is is equivalent logic to what was previously here. We don't want to use recovery keys to unseal we want to wait for the stored key to do the job for us.

It is equivalent logic to what was originally here though -- we currently don't have any tests that call this function with stored keys.

briankassouf · 2020-06-16T18:01:07Z

helper/testhelpers/testhelpers.go

@@ -565,6 +518,35 @@ func VerifyRaftConfiguration(core *vault.TestClusterCore, numCores int) error {
 	return nil
 }

+// AwaitLeader waits for one of the cluster's nodes to become leader.
+func AwaitLeader(t testing.T, cluster *vault.TestCluster) (int, error) {


We already have a wait for leader function, do we need both?

I needed a function that returned the index of the leader core

briankassouf · 2020-06-16T18:08:02Z

vault/external_tests/sealmigration/seal_migration_test.go

+	basePort_ShamirToTransit_Pre14  = 20000
+	basePort_TransitToShamir_Pre14  = 21000
+	basePort_ShamirToTransit_Post14 = 22000
+	basePort_TransitToShamir_Post14 = 23000


A little while back we did a lot of work to remove static ports from our tests which removed a lot of flakiness due to port collisions. Instead of re-introducing static ports can we use the in-memory cluster listener and a random API port? Here's an example:

vault/vault/cluster_test.go

Lines 182 to 193 in 6e2b91f

inmemCluster, err := cluster.NewInmemLayerCluster("inmem-cluster", 3, log.New(&log.LoggerOptions{

Mutex: &sync.Mutex{},

Level: log.Trace,

Name: "inmem-cluster",

}))

if err != nil {

t.Fatal(err)

}

testCluster_ForwardRequestsCommon(t, &TestClusterOptions{

ClusterLayers: inmemCluster,

})

briankassouf · 2020-06-16T18:15:38Z

vault/external_tests/sealmigration/seal_migration_test.go

@@ -117,7 +134,8 @@ func migrateFromShamirToTransit_Pre14(
 	var transitSeal vault.Seal

 	var conf = vault.CoreConfig{
-		Logger: logger.Named("migrateFromShamirToTransit"),
+		Logger:                    logger.Named("migrateFromShamirToTransit"),
+		DisablePerformanceStandby: true,


How come we need to disable performance standbys here?

briankassouf · 2020-06-16T19:07:39Z

vault/external_tests/sealmigration/seal_migration_test.go


 	var baseClusterPort = basePort + 10

 	// Start the cluster
 	var conf = vault.CoreConfig{
-		Logger: logger.Named("initializeShamir"),
+		Logger:                    logger.Named("initializeShamir"),
+		DisablePerformanceStandby: true,


Same here, do we need this disabled?

briankassouf · 2020-06-16T19:08:17Z

vault/external_tests/sealmigration/seal_migration_test.go

+// joinRaftFollowers unseals the leader, and then joins-and-unseals the
+// followers one at a time.  We assume that the ServerAddressProvider has
+// already been installed on all the nodes.
+func joinRaftFollowers(t *testing.T, cluster *vault.TestCluster, useStoredKeys bool) {


Why not use the test helpers version of this?

* master: (31 commits) changelog++ changelog++ Ui/replication status discoverability (#8705) Update CHANGELOG.md Counter that increments on every secret engine lease creation. (#9244) Add password_policy field to Azure docs (#9249) Replaced ClusterMetricSink's cluster name with an atomic.Value. (#9252) Fix database creds rotation panic for nil resp (#9258) changelog++ changelog++ Move sdk/helper/random -> helper/random (#9226) UI: Disallow kv2 with too large 'max versions' value (#9242) Allow mTLS for mysql secrets engine (#9181) docs: add sample revocation for mongodb (#9245) Add new Telemetry config options (#9238) Add a simple sealed gauge, updated when seal status changes (#9177) Test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault. (#9214) Configure metrics wrapper with the "global" object, not just the fanout. (#9099) changelog++ Add backend type to audit logs (#9167) ...

…1.4 Vault. (#9214) * move adjustForSealMigration to vault package * fix adjustForSealMigration * begin working on new seal migration test * create shamir seal migration test * refactor testhelpers * add VerifyRaftConfiguration to testhelpers * stub out TestTransit * Revert "refactor testhelpers" This reverts commit 39593defd0d4c6fd79aedfd37df6298391abb9db. * get shamir test working again * stub out transit join * work on transit join * remove debug code * initTransit now works with raft join * runTransit works with inmem * work on runTransit with raft * runTransit works with raft * cleanup tests * TestSealMigration_TransitToShamir_Pre14 * TestSealMigration_ShamirToTransit_Pre14 * split for pre-1.4 testing * add simple tests for transit and shamir * fix typo in test suite * debug wrapper type * test debug * test-debug * refactor core migration * Revert "refactor core migration" This reverts commit a776452. * begin refactor of adjustForSealMigration * fix bug in adjustForSealMigration * clean up tests * clean up core refactoring * fix bug in shamir->transit migration * stub out test that brings individual nodes up and down * refactor NewTestCluster * pass listeners into newCore() * simplify cluster address setup * simplify extra test core setup * refactor TestCluster for readability * refactor TestCluster for readability * refactor TestCluster for readability * add shutdown func to TestCore * add cleanup func to TestCore * create RestartCore * stub out TestSealMigration_ShamirToTransit_Post14 * refactor address handling in NewTestCluster * fix listener setup in newCore() * remove unnecessary lock from setSealsForMigration() * rename sealmigration test package * use ephemeral ports below 30000 * work on post-1.4 migration testing * clean up pre-1.4 test * TestSealMigration_ShamirToTransit_Post14 works for non-raft * work on raft TestSealMigration_ShamirToTransit_Post14 * clean up test code * refactor TestClusterCore * clean up TestClusterCore * stub out some temporary tests * use HardcodedServerAddressProvider in seal migration tests * work on raft for TestSealMigration_ShamirToTransit_Post14 * always use hardcoded raft address provider in seal migration tests * debug TestSealMigration_ShamirToTransit_Post14 * fix bug in RestartCore * remove debug code * TestSealMigration_ShamirToTransit_Post14 works now * clean up debug code * clean up tests * cleanup tests * refactor test code * stub out TestSealMigration_TransitToShamir_Post14 * set seals properly for transit->shamir migration * migrateFromTransitToShamir_Post14 works for inmem * migrateFromTransitToShamir_Post14 works for raft * use base ports per-test * fix seal verification test code * simplify seal migration test suite * simplify test suite * cleanup test suite * use explicit ports below 30000 * simplify use of numTestCores * Update vault/external_tests/sealmigration/seal_migration_test.go Co-authored-by: Calvin Leung Huang <[email protected]> * Update vault/external_tests/sealmigration/seal_migration_test.go Co-authored-by: Calvin Leung Huang <[email protected]> * clean up imports * rename to StartCore() * Update vault/testing.go Co-authored-by: Calvin Leung Huang <[email protected]> * simplify test suite * clean up tests Co-authored-by: Calvin Leung Huang <[email protected]>

mjarmy added 30 commits June 12, 2020 09:34

move adjustForSealMigration to vault package

7864437

fix adjustForSealMigration

9360ddd

begin working on new seal migration test

05a612c

create shamir seal migration test

28386e9

refactor testhelpers

1f8c5f1

add VerifyRaftConfiguration to testhelpers

4cb0394

stub out TestTransit

6e8caae

Revert "refactor testhelpers"

2abca4a

This reverts commit 39593defd0d4c6fd79aedfd37df6298391abb9db.

get shamir test working again

f7536c2

stub out transit join

f3039df

work on transit join

3066b51

remove debug code

594bccf

initTransit now works with raft join

b223d50

runTransit works with inmem

26b7a94

work on runTransit with raft

9a523df

runTransit works with raft

0e68260

cleanup tests

ee20dba

TestSealMigration_TransitToShamir_Pre14

926a957

TestSealMigration_ShamirToTransit_Pre14

6781c7e

split for pre-1.4 testing

3939bf3

add simple tests for transit and shamir

f7da813

fix typo in test suite

ccc79c9

debug wrapper type

e830135

test debug

9dd793f

test-debug

23aad62

refactor core migration

784102e

Revert "refactor core migration"

4ec9dc1

This reverts commit a776452.

begin refactor of adjustForSealMigration

e5f0d0f

fix bug in adjustForSealMigration

559ddcc

clean up tests

1293273

mjarmy added 5 commits June 12, 2020 09:35

simplify seal migration test suite

00f3c23

simplify test suite

2221432

cleanup test suite

a1f6bd7

use explicit ports below 30000

8087a07

simplify use of numTestCores

8c3375a

mjarmy requested review from briankassouf, calvn and vishalnayak June 12, 2020 16:34

calvn reviewed Jun 12, 2020

View reviewed changes

alexanderbez reviewed Jun 13, 2020

View reviewed changes

helper/testhelpers/testhelpers.go Show resolved Hide resolved

alexanderbez reviewed Jun 13, 2020

View reviewed changes

vault/external_tests/sealmigration/seal_migration_test.go Outdated Show resolved Hide resolved

alexanderbez reviewed Jun 13, 2020

View reviewed changes

vault/external_tests/sealmigration/seal_migration_test.go Show resolved Hide resolved

alexanderbez reviewed Jun 13, 2020

View reviewed changes

vault/external_tests/sealmigration/seal_migration_test.go Show resolved Hide resolved

vishalnayak reviewed Jun 15, 2020

View reviewed changes

vault/external_tests/sealmigration/seal_migration_test.go Show resolved Hide resolved

vault/external_tests/sealmigration/seal_migration_test.go Show resolved Hide resolved

vishalnayak approved these changes Jun 16, 2020

View reviewed changes

mjarmy and others added 7 commits June 16, 2020 12:20

Update vault/external_tests/sealmigration/seal_migration_test.go

a6140cb

Co-authored-by: Calvin Leung Huang <[email protected]>

Update vault/external_tests/sealmigration/seal_migration_test.go

f85159b

Co-authored-by: Calvin Leung Huang <[email protected]>

clean up imports

fdff2c4

rename to StartCore()

ebf9716

Update vault/testing.go

c1cf085

Co-authored-by: Calvin Leung Huang <[email protected]>

simplify test suite

d78f80e

clean up tests

0528d78

alexanderbez approved these changes Jun 16, 2020

View reviewed changes

ncabatoff reviewed Jun 16, 2020

View reviewed changes

mjarmy merged commit ec904e7 into master Jun 16, 2020

briankassouf reviewed Jun 16, 2020

View reviewed changes

mjarmy deleted the test-post1.4-seal-migration-phase1 branch July 6, 2020 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault. #9214

Test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault. #9214

mjarmy commented Jun 12, 2020

calvn Jun 12, 2020

mjarmy Jun 16, 2020

vishalnayak left a comment

alexanderbez left a comment

ncabatoff Jun 16, 2020

ncabatoff Jun 16, 2020

ncabatoff Jun 16, 2020

briankassouf Jun 16, 2020

mjarmy Jun 17, 2020

briankassouf Jun 16, 2020

mjarmy Jun 17, 2020

briankassouf Jun 16, 2020

briankassouf Jun 16, 2020

briankassouf Jun 16, 2020

briankassouf Jun 16, 2020

	inmemCluster, err := cluster.NewInmemLayerCluster("inmem-cluster", 3, log.New(&log.LoggerOptions{
	Mutex: &sync.Mutex{},
	Level: log.Trace,
	Name: "inmem-cluster",
	}))
	if err != nil {
	t.Fatal(err)
	}

	testCluster_ForwardRequestsCommon(t, &TestClusterOptions{
	ClusterLayers: inmemCluster,
	})

Test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault. #9214

Test Shamir-to-Transit and Transit-to-Shamir Seal Migration for post-1.4 Vault. #9214

Conversation

mjarmy commented Jun 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnayak left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment