Test pre-1.4 seal migration #9085
Conversation
alexanderbez
left a comment
alexanderbez
left a comment
There was a problem hiding this comment.
It looks mainly like lift-n-drop changes. Given the context, change LGTM 👍
| // Unseal | ||
| if storage.IsRaft { | ||
| testhelpers.RaftClusterJoinNodes(t, cluster) | ||
| if err := testhelpers.VerifyRaftConfiguration(leader, numTestCores); err != nil { |
There was a problem hiding this comment.
Following up with our discussion offline earlier and after giving it a bit more thought, I think that the const numTestCore might be unnecessary.
In cases were the number of cores is needed, we can either use vault.DefaultNumCores or extrapolate that from the cluster's core length or options field.
For instance, in this particular case, instead of passing in numTestCores we can either pass in opts.NumCores or len(cluster.Cores) (the latter is probably safer just in to avoid opt's zero value from being used). When setting opts.NumCores (L251), we can either leave that out and have NewTestCluster use the default value of 3 when creating cores, explicit set it to vault.DefaultNumCores, or set it to some other value if we want. The flexibility of using different numbers of cores if desired should remain the same.
There was a problem hiding this comment.
What I'd like to do then is keep numTestCores, with its setting of 5, and use it to set opts.NumCores as we are already doing. But everywhere else in the tests I will use len(cluster.Cores). I believe this is in consonance with what you are proposing, while at the same time sticking with 5 for the number of test cores to prove that we haven't hardwired the default value of 3 anywhere in the tests.
There was a problem hiding this comment.
Is there a specific reason to use 5 vs 3 (the vault.DefaultNumCores value) for a multi-cluster setup?
There was a problem hiding this comment.
The specific reason is that I want to make sure I don't accidentally hard-wire the ReusableStorage stuff to use the default 3 cores, and since this is the only place right now in the code base that is using reusable storage, its the only place I can do that.
There was a problem hiding this comment.
Keeping in mind the simplify use of numTestCores commit (7271985), which addresses some parts of Calvin's suggestion, I think the changes looks good.
The numTestCores variable as it is used now is only applicable to the seal migration package and is only used in places where the full cluster is not present. So, we can't do len(cluster.Cores). Also, the reason quoted by Mike to have 5 cores seems valid as it is walking a case that ensures creation of necessary number of reusable storage artifacts.
There was a problem hiding this comment.
The only remaining usage of numTestCores seems to be on here:
My suggestion was not to use len(cluster.Cores) in that instance since opts.NumCores is the field that determines the number of cores the cluster should have, but rather re-use the const value, vault.DefaultNumCores, that we already have for multi-core cluster setups.
* master: (36 commits) Minor transform docs rewording (#9223) Validate physical CockroachDB table config value before using it (#9191) Validate physical MySQL database and table config values before using them (#9189) add disable_iss_validation option to k8s auth docs (#9142) fix: configutil redeclared as imported package name (#9211) Integrate password policies into RabbitMQ secret engine (#9143) Clarify cache setting. (#9204) Test pre-1.4 seal migration (#9085) Simple typos (#9119) Update contribution guidelines changelog++ Add ssh signing algorithm as a role option. (#9096) replacing "a key usage mode" as it is confusing (#9194) changelog++ fix: invalidate cached clients after a config change in the aws secrets backend (#9186) website: remove whitepaper link from subnav (#9190) changelog++ Improving transit batch encrypt and decrypt latencies (#8775) changelog++ AWS: Add iam_groups parameter to role create/update (#8811) ...
* enable seal wrap in all seal migration tests * 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 * Revert "move resuable storage test to avoid creating import cycle" This reverts commit b3ff231. * remove debug code * initTransit now works with raft join * runTransit works with inmem * work on runTransit with raft * runTransit works with raft * get rid of dis-used test * 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 * remove unnecessary lock from setSealsForMigration() * rename sealmigration test package * use ephemeral ports below 30000 * simplify use of numTestCores
… into main (hashicorp#9085) (hashicorp#9087) * Add support for RFC 9440/8941 structured headers in client certificate handling * Add changelog --------- Co-authored-by: Huseyin Unal <23301714+husunal@users.noreply.github.com>
4 participants
This PR refactors the seal migration code into
vault.Coreso it's easier to test, and rewrites the seal migration tests for pre-1.4 shamir->transit and transit->shamir so that the tests simulate bringing the cluster down to do the migration.In addition, there are a couple of minor changes: