Skip to content
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

variable: use ForceNew for key changes on sensitive variables #175

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

bendrucker
Copy link
Contributor

@bendrucker bendrucker commented May 12, 2020

Description

Scenario: A user creates a tfe_variable with sensitive = true but sets the wrong key. They go back later to set the correct key, changing it in place in the Terraform configuration.
Expected: The variable is updated
Result: An API error that causes the resource update to fail

Error: Error updating variable var-cM2eQjsuDNpYHtw3: Invalid Attribute

Key cannot be updated for a sensitive variable

I can imagine why this limitation exists for sensitive (i.e. transit-encrypted) values. While it's possible to update non-sensitive values in place, split behavior seems confusing and the code involved would be a lot more complex (i.e. conditionally calling Delete and Create). There's no way to tell a user that a delete + create is happening other than ForceNew.

Edit: This should be using the ForceNew method

Closes #170

Testing plan

  1. Apply with config that is valid but contains a mistake
resource "tfe_variable" "api_key" {
  key          = "AP_KEY"
  value        = "the_api_key"
  sensitive    = true
  category     = "env"
}
  1. Fix the config
resource "tfe_variable" "api_key" {
  key          = "API_KEY"
  value        = "the_api_key"
  sensitive    = true
  category     = "env"
}
  1. Apply again

Output from acceptance tests

$ TESTARGS="-run TestAccTFEVariable_update" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEVariable_update -timeout 15m
?   	github.com/terraform-providers/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFEVariable_update
--- PASS: TestAccTFEVariable_update (10.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tfe/tfe	10.268s
?   	github.com/terraform-providers/terraform-provider-tfe/version	[no test files]

@ghost ghost added the size/XS label May 12, 2020
@bendrucker
Copy link
Contributor Author

Stumbled across https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/helper/schema?tab=doc#ResourceDiff.ForceNew, will update this to only ForceNew for sensitive = true.

@ghost ghost added size/M and removed size/XS labels May 12, 2020
@bendrucker
Copy link
Contributor Author

Ok, this is now ready for review. I've added a test that updates a sensitive variable and asserts that the resulting variable is both properly updated and has a new ID.

Full acceptance test output:

$ TESTARGS="-run TestAccTFEVariable" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEVariable -timeout 15m
?   	github.com/terraform-providers/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFEVariable_basic
--- PASS: TestAccTFEVariable_basic (6.25s)
=== RUN   TestAccTFEVariable_update
--- PASS: TestAccTFEVariable_update (9.62s)
=== RUN   TestAccTFEVariable_update_key_sensitive
--- PASS: TestAccTFEVariable_update_key_sensitive (9.80s)
=== RUN   TestAccTFEVariable_import
--- PASS: TestAccTFEVariable_import (6.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tfe/tfe	32.093s
?   	github.com/terraform-providers/terraform-provider-tfe/version	[no test files]

@bendrucker bendrucker changed the title variable: use ForceNew on "key" fix: use "ForceNew" when the key of a sensitive variable is changed May 12, 2020
@bendrucker bendrucker changed the title fix: use "ForceNew" when the key of a sensitive variable is changed variable: use ForceNew for key changes on sensitive variables May 12, 2020
@bendrucker
Copy link
Contributor Author

Any chance I can get a review here? I have another variable I'm hoping to update. This time it's the Datadog provider, which changed its recommended env var:

https://www.terraform.io/docs/providers/datadog/index.html

Not urgent, so it'd be nice to handle this here rather than having to taint a few dozen things.

@koikonom
Copy link
Contributor

koikonom commented Sep 21, 2020

Hi @bendrucker, thanks for taking the time to submit this PR. It looks good to me! I'll make sure the tests still pass and then merge it.

@koikonom koikonom merged commit 803cf41 into hashicorp:master Sep 21, 2020
@bendrucker
Copy link
Contributor Author

Thank you, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key cannot be updated for a sensitive variable
2 participants