Test/iam auth #100
Test/iam auth #100
Conversation
Sounds like a good idea. I'll review the rest of this PR now. |
test/vault_helpers.go
Outdated
| test_structure.SaveTerraformOptions(t, examplesDir, terraformOptions) | ||
|
|
||
| terraform.InitAndApply(t, terraformOptions) | ||
| deployCluster(t, examplesDir, random.UniqueId(), map[string]interface{}{}) |
There was a problem hiding this comment.
Can you replace map[string]interface{}{} with just nil?
test/vault_helpers.go
Outdated
| // 2. Building the AMI in the vault-consul-ami example with the given build name | ||
| // 3. Deploying that AMI using the example Terraform code setting an example secret | ||
| // 4. Waiting for Vault to boot, unseal and the client to fetch the secret | ||
| // 5. Making a request to the webserver started by the auth client user data script to see if the secret was properly fetched |
There was a problem hiding this comment.
Nice description of the test, but it doesn't mention Vault IAM auth explicitly anywhere? Perhaps clarify in the last line that you're using Vault IAM Auth to auth the client? Also can you make it clear that you're not "waiting for vault" to unseal; rather, the test will unseal Vault.
test/vault_helpers.go
Outdated
| VAR_SSH_KEY_NAME: keyPair.Name, | ||
| } { | ||
| terraformVars[key] = value | ||
| } |
There was a problem hiding this comment.
This is a clever but somewhat unusual way to just merge two maps. Can you make this more clear by writing your own mini merge function that's more generic in nature and then call that? Should make the code more readable.
| Vars: terraformVars, | ||
| EnvVars: map[string]string{ | ||
| ENV_VAR_AWS_REGION: awsRegion, | ||
| }, |
There was a problem hiding this comment.
Given that we're dynamically generating all the vars, should we just pass in the awsRegion directly to the Terraform vars and avoid the use of EnvVars altogether?
There was a problem hiding this comment.
We could. I was just following the previous examples that use data "aws_region" "current" {} and get it through env. That could be removed and we can pass it as vars, but I honestly don't see what's the difference and what would be the advantage of doing that over what was being done before. Could you help me understand?
There was a problem hiding this comment.
but I honestly don't see what's the difference
Actually, that was my thought exactly. But if there's no difference, and you see a different way of handling one particular var then it makes the reader question why it's being used that way (as I did). So I'm just trying to minimize the potential confusion readers will have.
There was a problem hiding this comment.
I see! I'll take a note to do this in a separate PR if you don't mind because I just did the vault auto-unseal branching off this one
There was a problem hiding this comment.
That's fine. FYI, you could always do the following if you wanted to keep everything self-contained:
- Make the desired change on this branch
git checkout <vault-auto-unseal-branch>- `git rebase -i
git push -f
But careful with that last step. Once you do a git push -f (git push force), you'll overwrite what ever's currently on the remote branch in GitHub.
Well, the vault enterprise cluster test runs with Vault Enterprise installed, so will the auto unseal test that I'm working on right now (coming on another PR). All the others run with the open source vault. |
So, this is Vault open source then? My point is that we should default to using Vault open source since it's the free version and only use Vault Enterprise in the examples when the examples specifically calls for it. |
Yes, this is exactly how it works already |
|
Notify |
There still an intermittent test issue going on (from before this PR, which continues to happen through this PR). Josh suggested to use Eugene's new feature. I agree! I am thinking about tackling it in a different PR though. What do you think?