Refactor Vault tests to allow more flexibility in mounting PKI and also be more representative of the integration experience of the user

[jmurret]

Background

Easy to follow configuration paths

Over time, the Vault acceptance test have organically grown where multiple helper functions would encapsulate what should be shared knowledge. For example, all of the tests have helm chart configurations that look like this:

"global.secretsBackend.vault.connectCA.rootPKIPath":         "connect_root",
"global.secretsBackend.vault.connectCA.intermediatePKIPath": "dc1/connect_inter",
"global.acls.bootstrapToken.secretName": "consul/data/secret/bootstrap",
"global.acls.bootstrapToken.secretKey":  "token",
"global.gossipEncryption.secretName":    "consul/data/secret/gossip",
"global.gossipEncryption.secretKey":     "gossip",
"global.tls.caCert.secretName": "pki/cert/ca",

For each of these, it can be confusing to know where and how this get set previously to arriving here. It is also easy to misconfigure and can lead to taking longer to troubleshoot. For the above, you would would have to know:

• rootPKIPath and intermediatePKIPath were set up and encapsulated inside the vault.CreateConnectCAPolicy(t, vaultClient, "dc1") call
• bootrapToken secret name and key were setup and encapsulated inside the vault.ConfigureACLTokenVaultSecret(t, vaultClient, "bootstrap") call
• gossipEncryption secret name and key were setup and encapsulated inside the vault.ConfigureGossipVaultSecret(t, vaultClient) call
• pki/cert/ca was configured when vault.NewVaultCluster(t, ctx, cfg, vaultReleaseName, nil) and the pki engine was mounted three function calls deep and set at the pki path

There are more examples when you look at policies and roles.

The main thing here is that the existing test code is hard to follow where something was originally configured and where it needed to be mapped downstream.

This PR means to make that more explicit. While more verbose than the previous tests, one can follow when something is set up and when it is later mapped. This is representative of the UX of the user that is configuring this integration.

Setting up the ability to mount multiple PKI engines at different paths

This work came about while working on #1191. In that PR, we need the ability to configure:

• a CA for controller webhook certs
• a CA for connect injector webhook certs
  and have these CAs be different from the server tls CA.

In the current helper code this is difficult to achieve. This work originally started in that PR, but has been moved here as a pre-requisite for that PR since that PR is already 1k+ lines of changes and so is this one. This was the easiest way to split it up.

How to read this PR:

• view acceptance/tests/snapshot-agent/snapshot_agent_vault_test.go and see the comments I have made there.
• view all of the other tests. it should be rinse and repeat concepts.
• view acceptance/framework/vault/helpers.go last. The diff is hard to read because so much changed. Having viewed everything else beforehand, you will get a good sense of the new functions and how they work.

How I've tested this PR:

• integration tests locally and in CI

How I expect reviewers to test this PR:

👀

Checklist:

• Tests added
• [ ] CHANGELOG entry added

  HashiCorp engineers only, community PRs should not add a changelog entry.
  Entries should use present tense (e.g. Add support for...)

[jmurret]

This diff is not the easiest to follow unfortunately. The best way to review this is to first gain an understanding of the vault helper functions are used in the tests. So look to the test files first and look for vault.* function calls.

[jmurret]

This has moved to ConfigurePKI() in the above helpers file because we will need to mount more than one PKI engine and it will not always be at pki base path.

[jmurret]

this object will get re-used in the auth role creation as well as in helm charts.

[jmurret]

for these secrets, we now have a reference to the configuration we will need when we need to also set the secretName and secretKey in the helm chart.

[jmurret]

all strings that can be linked to a previous configuration are.

[thisisnotashwin]

This makes the tests very very readable! love the approach!!

[kschoche]

I hate to be the grammar police, but all comments need to end with proper punctuation, could you run through and double check them? Thanks!

Suggested change

	// Configure Service Mesh CA
	// Configure Service Mesh CA.

[kschoche]

Just to add I suppose this needs to be updated on every file sorry! :(

[jmurret]

Is this used in a doc string or something?

[kschoche]

No I don't think it parsed anywhere but it's part of our code style.

[jmurret]

Some context:
This is not a comment to explain the logic as we would see in comments in source code.

This comment in the test file is really just there is so much manual set up related to vault that I put these headers in here so its easier to follow.

So proceeding to put periods in this type of comment seems a little mismatched with the original intention of the code style.

I think I'll either move this code to helper functions or something else to break them up...or we revisit the need for an ending period for a comment that is not a sentence and is more of header.

[kschoche]

Yeha I think that is totally reasonable to not have periods on blocks like this. Just the other comments for the code!

[kschoche]

Awesome work @jmurret !

I really like how this came together and how much more readable the tests are now, excellent!