-
Notifications
You must be signed in to change notification settings - Fork 154
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
Improve discovery and loading of credentials from Terraform config files #360
Conversation
This provider is somewhat special, because the API token it uses is also used by Terraform CLI itself. We can give a nicer experience on the local CLI by auto-discovering Terraform's copy of the token (if one isn't provided in the provider config or the `TFE_TOKEN` environment variable). However, we weren't quite matching Terraform's credential-finding behavior: - We were respecting the `TERRAFORM_CONFIG` environment variable for the main config file location, but not `TF_CLI_CONFIG_FILE`. (Terraform will use either, but prefers the latter since it matches the other environment variable names it uses.) - We weren't ever consulting the credentials file (`~/.terraform.d/credentials.tfrc.json`, on unix), which is written to by `terraform login` and/or credential helper scripts. The HCL library understands the credentials JSON, so you could work around this by pointing `TERRAFORM_CONFIG` at the credentials file, but if there was anything else important in the config file you also had to set `TF_CLI_CONFIG_FILE` so that Terraform (but not the provider) could still find it. This commit fixes both of those: we'll accept a non-default config file location via either environment variable (preferring `TF_CLI_CONFIG_FILE`), and we'll _also_ consult the credentials file if present. If both sources include a token for the desired hostname, the provider will follow Terraform's behavior and prefer the token from the CLI config file.
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 LGTM, though admittedly I don't have the time to properly go through the testing plan at the moment. Really really great PR!
tfe/provider.go
Outdated
|
||
// There might be credentials in the main CLI config file (manually entered) | ||
// AND/OR the credentials file (auto-configured by terraform login or a | ||
// credentials helper), so we need to consult both. As per the behavior of |
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 doesn't work with a credentials helper, no?
FWIW we could support them, the logic is here: https://github.com/hashicorp/terraform-svchost/blob/f050f53b97348772833a8f7fc1b297ec71f9fc7c/auth/helper_program.go
But this PR is already 💯 without all that, for sure 👍🏻 I'd just amend this comment.
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.
Oh! I thought credentials helpers used the credentials file as a cache, though now I've forgotten where I would have gotten that idea.
I'll just amend the comment to only refer to login. 👍🏼
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.
tfe/provider.go
Outdated
configFilePath := os.Getenv("TF_CLI_CONFIG_FILE") | ||
if configFilePath == "" { | ||
configFilePath = os.Getenv("TERRAFORM_CONFIG") | ||
} |
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.
What do you think about refactoring this a tad bit for readability? I added an example here c23bbb5
The way the previous implementation combined the configs was throwing away the Hosts member from the one Config that might have anything in it. This makes the combining behavior a bit more explicit, and also pulls out some wordy conditionals into a separate function per Omar's suggestion.
More info about what this undocumented feature is used for over at hashicorp/terraform#28309
tfe/provider_test.go
Outdated
hasCredentials := "test-fixtures/env/home" | ||
noCredentials := "test-fixtures/env/empty" | ||
terraformrc := "test-fixtures/env/terraformrc" | ||
noTerraformrc := "test-fixtures/env/nothing" |
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 know i'm the one who made this directory path, so I blame my older self 😆 . I think if I was looking at the directory path, i'd be confused what "env" referred to. Could we change these paths to be test-fixtures/tf-env-files/*
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.
And nit comment, could we be more specific for the empty
and nothing
? So like test-fixtures/tf-env-files/no-terraformrc
and test-fixtures/tf-env-files/no-credentials
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 went with test-fixtures/cli-config-files/*
for the path name.
tfe/provider_test.go
Outdated
noCredentials := "test-fixtures/env/empty" | ||
terraformrc := "test-fixtures/env/terraformrc" | ||
noTerraformrc := "test-fixtures/env/nothing" | ||
fromTerraformrc := "something.atlasv1.prod_rc_file" |
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.
Nit as well, s/fromTerraformrc/tokenFromTerraformrc
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.
A few minor nits, but overall amazing job! once tests pass
Description
This provider is somewhat special, because the API token it uses is also used
by Terraform CLI itself. We can give a nicer experience on the local CLI by
auto-discovering Terraform's copy of the token (if one isn't provided in the
provider config or the
TFE_TOKEN
environment variable).However, we weren't quite matching Terraform's credential-finding behavior:
TERRAFORM_CONFIG
environment variable for the mainconfig file location, but not
TF_CLI_CONFIG_FILE
. (Terraform will useeither, but prefers the latter since it matches the other environment variable
names it uses.)
(
~/.terraform.d/credentials.tfrc.json
, on unix), which is written to byterraform login
and/or credential helper scripts. The HCL libraryunderstands the credentials JSON, so you could work around this by pointing
TERRAFORM_CONFIG
at the credentials file, but if there was anything elseimportant in the config file you also had to set
TF_CLI_CONFIG_FILE
so thatTerraform (but not the provider) could still find it.
This commit fixes both of those: we'll accept a non-default config file location
via either environment variable (preferring
TF_CLI_CONFIG_FILE
), and we'llalso consult the credentials file if present.
If both sources include a token for the desired hostname, the provider will
follow Terraform's behavior and prefer the token from the CLI config file.
Testing plan
tfe
provider and maybe manages one resource. In the provider config, provide a hostname (if you aren't using app.terraform.io) but not a token.TFE_TOKEN
,TF_CLI_CONFIG_FILE
, orTERRAFORM_CONFIG
.~/.terraformrc
or~/.terraform.d/credentials.tfrc.json
files include a token for the configured TFE host, to start out with.Note: for the environment variable columns, "x" means "unset"; read any other value as "points to a loadable .terraformrc config file containing
_____
for the configured hostname".TF_CLI_ CONFIG_FILE
TERRAFORM_ CONFIG
Output from acceptance tests
See CI output in the checks below.