Set VAULT_ADDR in process env when using token helper #651
Conversation
ghost
added
the
|
Thank you for your PR. I have thoughts and questions on it and to avoid splitting the conversation, I'm going to comment on the linked issue further. |
|
@tyrannosaurus-becks sounds good! I was offline all last week, so will check in on the discussion on the original issue, thanks for following up on this |
ianferguson
force-pushed
the
set_vault_address_into_process_env
branch
from
078d8c0
to
0c055c5
Compare
| @@ -116,8 +116,7 @@ func getTestRMQCreds(t *testing.T) (string, string, string) { | |||
| } | |||
|
|
|||
| // A basic token helper script. | |||
| const tokenHelperScript = ` | |||
| #!/usr/bin/env bash | |||
| const tokenHelperScript = `#!/usr/bin/env bash | |||
There was a problem hiding this comment.
The extra newline added after the backtick caused the script to come out as
#!/usr/bin/env bash
echo "helper-token"
With the shebang on the 2nd, rather than 1st, line. When executed as an exec by the cmd package for a user using fish shell the follow error silently happened:
> /usr/local/bin/fish -c $tmpScriptPath
Failed to execute process '$tmpScriptPath'. Reason:
exec: Invalid argument
The file '$tmpScriptPath' is marked as an executable but could not be run by the operating system.
Removing the line break here fixes that otherwise perplexing test failure that non-bash users may see
| }() | ||
| } | ||
| err = os.Unsetenv(config.ConfigPathEnv) | ||
| reset, err := tempUnsetenv(config.ConfigPathEnv) |
There was a problem hiding this comment.
I ended up using TestAccProviderToken as a template for writing TestAccProviderVaultAddEnv, and in the process of reading them and writing another similar test consolidated some of the setup/tear down refactoring -- if this is not ok I can unwind the changes here
|
@tyrannosaurus-becks I've updated the PR with a flag ( The tests pass for me against a local Which I'll take a look at on Monday or Tuesday, but otherwise I think this should be about ready for review based on what we discussed on #526 |
There was a problem hiding this comment.
I got the tests passing -- it turns out the way that env vars were being configured for these tests, combined with how ExternalTokenHelper's in hashicorp/vault turn the configured binary value into an actual os exec.Cmd struct resulted in the preconfigured test env vars (including VAULT_ADDR=http://127.0.0.1:8200) being reloaded by the shell that ended up invoking the external token helper command. I addressed this as surgically as I could, but let me know if y'all would prefer a different approach.
@tyrannosaurus-becks this should be ready for a review whenever y'all have a chance
| // when the ExternalTokenHelper's command is formatted into a shell invocation to exec here: | ||
| // https://github.com/hashicorp/vault/blob/master/command/token/helper_external.go#L117-L132 | ||
| // | ||
| resetBashEnvEnv, err := tempUnsetenv("BASH_ENV") |
There was a problem hiding this comment.
I think its possible that a better fix here might be to move all of the vars set here: https://github.com/terraform-providers/terraform-provider-vault/blob/f42716aae3aebc8daf9702dfa20ce3f8d09d9f4d/.circleci/config.yml#L26-L32 to an Environment object in the circleci yaml configuration for the container these tests are run in, as documented here: https://circleci.com/docs/2.0/env-vars/#setting-an-environment-variable-in-a-container
That being said, I don't know the finer details of these tests or how circle manages those environment variables/containers across multiple steps well enough to be confident that that wouldn't have any unintended side effects, so I took a more surgical route to fixing these tests that rely on testing the VAULT_ADDR change when a token helper is called.
ianferguson
force-pushed
the
set_vault_address_into_process_env
branch
from
12dd847
to
bfaad51
Compare
There was a problem hiding this comment.
@ianferguson thank you for doing this! Much appreciated. Looks great.
* Set VAULT_ADDR in process env when using token helper * Add add_vault_addr_to_env argument, tests, udpate docs * Unset BASH_ENV to avoid clobbering VAULT_ADDR test values * Add changelog line
Community Note
Description
Closes #526
I'm opening this as a draft pull request because I'd like to get the maintainers thoughts on the approach being taken.
I've been using a token helper that stores tokens based on the configured value of VAULT_ADDR in the environment while using terraform across many Vault clusters, and having support for this would make it much easier to run changes en masse and avoid needing to manage setting and unsetting the VAULT_ADDR for different applies based on what was configured in the Vault terraform provider.
Release note for CHANGELOG:
Output from acceptance testing: