-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Vault tests to allow more flexibility in mounting PKI and also be more representative of the integration experience of the user #1221
Conversation
@@ -6,63 +6,11 @@ import ( | |||
"fmt" | |||
"testing" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -147,13 +147,6 @@ func (v *VaultCluster) bootstrap(t *testing.T, vaultNamespace string) { | |||
}) | |||
require.NoError(t, err) | |||
|
|||
// Enable the PKI Secrets engine. | |||
err = v.vaultClient.Sys().Mount("pki", &vapi.MountInput{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
vault.CreateConnectCARootAndIntermediatePKIPolicy(t, vaultClient, connectCAPolicy, connectCARootPath, connectCAIntermediatePath) | ||
|
||
// Configure Server PKI | ||
serverPKIConfig := &vault.PKIAndAuthRoleConfiguration{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this object will get re-used in the auth role creation as well as in helm charts.
// Gossip key | ||
gossipKey, err := vault.GenerateGossipSecret() | ||
require.NoError(t, err) | ||
gossipSecret := &vault.SaveVaultSecretConfiguration{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -75,36 +184,36 @@ func TestSnapshotAgent_Vault(t *testing.T) { | |||
"controller.enabled": "true", | |||
|
|||
"global.secretsBackend.vault.enabled": "true", | |||
"global.secretsBackend.vault.consulServerRole": "server", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all strings that can be linked to a previous configuration are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the tests very very readable! love the approach!!
cc38ab9
to
106e22d
Compare
…onnectCARootAndIntermediatePKIPolicy
…were taking as arguments.
f7e6353
to
32f54f5
Compare
// ------------------------- | ||
// PKI | ||
// ------------------------- | ||
// Configure Service Mesh CA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
// Configure Service Mesh CA | |
// Configure Service Mesh CA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add I suppose this needs to be updated on every file sorry! :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used in a doc string or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I don't think it parsed anywhere but it's part of our code style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeha I think that is totally reasonable to not have periods on blocks like this. Just the other comments for the code!
892231f
to
32f54f5
Compare
…criptive. adding comment string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @jmurret !
I really like how this came together and how much more readable the tests are now, excellent!
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:
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:
vault.CreateConnectCAPolicy(t, vaultClient, "dc1")
callvault.ConfigureACLTokenVaultSecret(t, vaultClient, "bootstrap")
callvault.ConfigureGossipVaultSecret(t, vaultClient)
callpki/cert/ca
was configured whenvault.NewVaultCluster(t, ctx, cfg, vaultReleaseName, nil)
and the pki engine was mounted three function calls deep and set at thepki
pathThere 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:
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:
acceptance/tests/snapshot-agent/snapshot_agent_vault_test.go
and see the comments I have made there.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:
How I expect reviewers to test this PR:
👀
Checklist:
[ ] CHANGELOG entry added