fix: local variables should not be overridden by remote variables during terraform import

[tchupp]

This PR addresses this issue: #29966

The idea is that remote variables should not override local variables when executing a "Local Run"

In general, this should vastly improve the ergonomics of using terraform import with Terraform Cloud, especially when importing resources that require authentication.

Note: I'm very open to suggestions on how to improve the tests. I used the existing tests are a starting point, but I couldn't find a less-verbose way to implement the new tests.

NOTE POST-MERGE:
This PR, indirectly, also solved for this other github issue: #26494

[hashicorp-cla]

All committers have signed the CLA.

[tchupp]

@alisdair do you have any feedback here?

[alisdair]

Hi @tchupp, thanks for working on this. The area of code you're touching here is under some churn with the upcoming 1.1 release (see the new internal/cloud package), so we'll have to make the same changes in both places.

Also, from a quick glance at the linked issue it's not clear to me what the cause of this behaviour is, so I think we'll need further investigation from the maintainers of that code to move forward. I've highlighted this PR and the linked issue to them and they'll take a look when they can. Thanks again!

[tchupp]

Thank you for the feedback! I'll make the same change in that package as well.

With the details I provided in the Issue, I'm not 100% sure what the root cause of the behavior is (ie, I'm not entirely sure why it's attempting to pull variables from a Terraform Cloud workspace that's set to use "local" execution); but in using the binary generated from this branch, I no longer see the behavior I was trying to describe in the linked issue.
If there are better or more rigorous steps I should take for testing, please let me know!

[tchupp]

I finally got some time to make the same fix in the internal/cloud package. Please let me know if I can provide any more details on reproducing this issue, or if there's anything I missed!

ps, I'm very open to suggestions on how to improve the tests I added.

[tchupp]

It looks like there is a static code error in CI:

...
internal/getproviders/package_authentication.go:12:2: package golang.org/x/crypto/openpgp is deprecated
...

Since this seems outside of the scope of my PR, I'm not sure if I should try to fix it or wait for an update on the main branch that I can rebase my PR off of.

[alisdair]

Thanks for your continued work on this!

A rebase against latest main should fix the CI check.

[tchupp]

done!

[ebachle]

@alisdair I know that this was somewhat bound up with the 1.1 release and don't want to push to hard as a result, but this fix looks like it could help our org through a few problems. We definitely ended up having to hard-code creds in our Terraform to get it to run a terraform import on a repo with a Terraform Cloud workspace today. Not our favorite flow :(

[uturunku1]

Hi Theo! @tchupp @ebachle

Thank you for your patience. Alisdair, myself and other colleagues needed some time to debate over your changes. The PR and the issue that is solving for, helped us realize that we lacked some clarity of what the expected behavior should be around local operations that have a TFC workspace with remote variables defined and that is running over local execution mode.

Your changes are correct, a local operations like import should prioritize local variables over remote variables (whether this is on local or remote execution mode), and your current implementation solves for that.

But another concern we had was: if a user is running a local operation exclusively in local execution mode, is there a reason why the user would expect or want to work with remote variables? After some debate, we agreed that the expected behavior should be that remote variables must not exist or be considered at all with local execution mode.

So, going back to your changes, we want to keep those. But we'd also like to ask you for some additional changes to help us align with the expected behavior around remote variables during local execution mode:

1. Within the same function LocalRun where you applied the changes, let's check if this workspace is running with Local Execution Mode.

You can find what the execution mode is by using our go-tfe library. The read method https://github.com/hashicorp/go-tfe/blob/main/workspace.go#L28
will give you access to the Workspace.ExecutionMode
https://github.com/hashicorp/go-tfe/blob/main/workspace.go#L120

Then, in this repo, you can find a function isLocalExecutionMode that will facilitate for that check.

2. If we are dealing with local execution mode, then we do not need to pull any remote variables

   terraform/internal/cloud/backend_context.go

   Line 101 in da153fb

   tfeVariables, err := b.client.Variables.List(context.Background(), remoteWorkspaceID, tfe.VariableListOptions{})

   
   We can basically skip this whole segment:

   terraform/internal/cloud/backend_context.go

   Lines 107 to 120 in da153fb

   	if tfeVariables != nil {
   	if op.Variables == nil {
   	op.Variables = make(map[string]backend.UnparsedVariableValue)
   	}
   	for _, v := range tfeVariables.Items {
   	if v.Category == tfe.CategoryTerraform {
   	if _, ok := op.Variables[v.Key]; !ok {
   	op.Variables[v.Key] = &remoteStoredVariableValue{
   	definition: v,
   	}
   	}
   	}
   	}
   	}

3. If this is not a workspace with local execution mode, then we do things the way we were doing before (including your changes)

Let me know if you need any clarifications. Thanks so much for all the effort invested in this contribution!

[tchupp]

That makes a lot of sense! Thank you for the detailed explanation.

Should the changes be made in both backend/remote/backend_context.go and cloud/backend_context.go? Or just in the latter?

[uturunku1]

That makes a lot of sense! Thank you for the detailed explanation.

Should the changes be made in both backend/remote/backend_context.go and cloud/backend_context.go? Or just in the latter?

The changes should be made to both :)

[crw]

Will this PR also (incidentally) fix this issue? #26494

[tchupp]

Yes! This should address that issue @crw

[tchupp]

PR has been updated with the logic to skip fetching variables from the workspace if in local execution mode. Yay avoiding network calls!

[uturunku1]

PR has been updated with the logic to skip fetching variables from the workspace if in local execution mode. Yay avoiding network calls!

Hey Theo! Thank you for your changes! I have noticed that your latest changes were applied to the Cloud package but not to the Remote package. I am assuming is because the latter did not have the helper functions isLocalExecutionMode, fetchWorkspace and other implementations that you needed for your work and that you were able to use in the Cloud package.

Our bad. We have not been super proactive at keeping the Remote package up to date with the Cloud package, so I apologize for not giving you heads up on this limitation.

I smoked tested your work, and everything looks good! So, I am going to go ahead and update the remote package to match your changes in the cloud package. After that, my colleague, Alisdair, is going to give the final review/approval to this PR 🎉.

[tchupp]

Ahh thank you! You're right, I didn't consider updated that package as well since you mentioned it was being phased out with v1.1.
I'm looking forward to getting this completed! Thank you for taking the time to talk me through the changes 😁

[alisdair]

I think the one remaining failing test is caused by this change in casing of "The" to "the". Was that necessary for some other reason?

[uturunku1]

oops, I meant to come back to this change earlier but I got caught up on some other task. I'll make this change now!

[alisdair]

Looks good to me! 🎉

@uturunku1, I think this will warrant quite a detailed CHANGELOG entry describing the difference in behaviour. Let me know offline if you'd like a review of that.

Thanks @tchupp and @uturunku1 for your work on getting this fixed!

Changes made

[github-actions]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.