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

Fix panic when applying variable set to workspaces fails #628

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

sebasslash
Copy link
Contributor

Description

This PR fixes a panic that occurs when setting the workspace_ids attribute to [""], thus causing tfeClient.VariableSets.UpdateWorkspaces() to return an error. That error message in turn would reference a nil variable:

variableSet, err = tfeClient.VariableSets.UpdateWorkspaces(ctx, variableSet.ID, &applyOptions)
if err != nil {
return fmt.Errorf(
"Error applying variable set %s (%s) to given workspaces: %w", name, variableSet.ID, err)
}

See the issue? If that method returns an error, variableSet.ID in L93 will cause a panic.

Furthermore, I noticed some weird behavior where the variable set would be created successfully, applying to a workspace would fail, and Terraform would then try to recreate the resource since it thinks it doesn't exist (both operations occur during the creation phase). I updated the provider to write to state when the variable set is created successfully before we apply the variable set to a workspace. What this will do is if applying the variable set fails, Terraform will taint the resource to be recreated instead of attempting to create a new variable set!

Testing plan

To Reproduce

  1. Simply terraform apply this TF configuration:
provider "tfe" {}

resource "tfe_variable_set" "foo" {
  name = "foo"
  organization = "my-org"
  workspace_ids = [""]
}
  1. To reproduce the odd behavior I mentioned, simply run terraform apply again! You will get a bad request error stating the variable set name is already taken. This is because the variable set was already created but Terraform has no record of it (you can view the statefile to double check).

The Fix

  1. Pull these changes, go install . , and ensure you have an CLI overrides file pointing hashicorp/tfe to your locally built provider.
  2. We'll reuse the configuration above (make sure the original varset is deleted through the UI) and run TF_CLI_CONFIG_FILE=/path/to/your/overrides.tfrc terraform apply
  3. This time you will get an error: Insufficient identification for workspace assignment. no workspace id provided. Perfect! No more panic and we get an error message letting us know we've provided an invalid or empty ID.
  4. If you terraform apply again, you'll notice Terraform has now tainted the variable set resource. Don't apply the plan just yet!
  5. Replace the workspace ID in your config with a valid value and run terraform apply. The resource will still be marked for replacement instead of creating a new variable set. Type yes to confirm, and the apply should succeed!

Copy link
Contributor

@Uk1288 Uk1288 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to smoke test and PR looks good to me if we are going with keeping the deprecated attr for another month or so.

@sebasslash sebasslash merged commit 37dbdcc into main Sep 21, 2022
@sebasslash sebasslash deleted the fix-wsids-panic branch September 21, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants