core: Do not persist state after plans

[phinze]

This makes the behavior of plans much more predictable, as they no
longer potentially have side effects on shared remote state.

[phinze]

Pushing this for tests to run - discussing this amongst some HC folk. Stay tuned! 📻 👍

[apparentlymart]

If I am understanding this correctly then it seems to be in conflict with #6540, which is assuming that every plan is associated with a specific state.

[phinze]

Good point @apparentlymart - let me re-read that PR and think about the implications and I'll get back to you.

[apparentlymart]

Perhaps it's reasonable to skip updating the state on just a bare terraform plan (where we just print out the diff and don't take any other action) but to actually update the state if the -out parameter is used, implying that the user is intending to take more action.

Then the no-arguments version can be used as "just try this and see if it even works" (which I often do, and can empathize with the fear of it affecting the state) but when we're making a plan to actually apply, with -out, we document on the remote the state of the world that the plan was based on so that when we subsequently apply we can check that it hasn't changed.

[phinze]

That's one approach that might work.

I wonder instead if we can embed all that information inside the plan file, and have the apply - after it does its refresh - error if anything is different at that point.

I haven't thought through fully whether there is some reason we can't or shouldn't do it this way. If we can, then plan stays side effect free, which is a property that I like! 😁

[apparentlymart]

The code in that other PR also checks the states for equality as well as serial, but the plan state will not be equal to the persisted state if the refresh changed anything.

I think the only way to truly resolve this conflict would be to refresh on apply too, and then fail at that point if the plan state doesn't match. Then the persisted state doesn't actually matter that much.

[phinze]

Just pulled off the WIP here.

@apparentlymart I am confident that we can work through the open questions around #6540 and I'm happy to help shepherd that PR in after this lands.

[phinze]

Ping @jen20 for a review before I actually land this. 😄

[jen20]

I think we might want to make the wording clearer that the "refresh" does not mean the state on disk or the remote state was modified, for the benefit of users of earlier versions of Terraform who may have their expectations challenged, but apart from that this looks great to me.

[jen20]

This LGTM now!

[glasser]

Can you help me understand the effect of this PR? If you run terraform plan -out p && terraform apply p, it's still the case that the apply does not refresh by default, right? Do changes that are learned about during a refresh during the plan end up in the plan and get into the state when you run apply? Or do those changes just vanish now?

[apparentlymart]

@glasser as implemented here, only the effects of applying the plan end up in the state. If something changes during the pre-plan refresh it will not be reflected in the persisted state. It's now necessary to run terraform refresh explicitly to update the persisted state with the latest configuration of remote resources.

[glasser]

Hmm. I get why you'd do that and all the pieces are internally consistent, but it seems like a bit of a shame that "apply" is no longer equivalent to "plan -out p && apply p".

[glasser]

Kinda makes me want a version of apply that just asks me to confirm after printing the plan...

[apparentlymart]

I've been thinking about how the idea of blocking the application of stale plans from #6540 could be supported in conjunction with the change made in this PR. I have a proposal that I think addresses my own concern and addresses the supposed problem of the persisted state getting out of date unless the user explicitly runs terraform refresh.

My idea is simple: run the walkRefresh pass on terraform apply tfplan too. With that done, Terraform can:

• Fail if the newly-update state is not "the same as" the plan state (using state.Same(otherState))
• If the two states are the same, persist the newly-refreshed state so that the changes that were discovered during terraform plan -out=tfplan will then show in the persistent state.

This is then consistent with the idea that plan is read-only while apply can make changes.

The downside is that it will make terraform apply tfplan slower, since it will need to refresh everything again and that's redundant in the ideal case where people are not adjusting Terraform-managed resources outside of Terraform.

@phinze what do you think here? I'm happy to take a stab at implementing this but I'd like to hear your take on it first.

[glasser]

I guess my question is, why do people use plan (or specifically, why use plan -out followed by applying the plan)?

For me, it's not because I really want to have an on disk record of the applied plan, or because I want to ship it elsewhere for somebody else to use, or because I want to wait hours and do other things with my shell in the middle.

It's simply because I want a last chance to double check the reasonableness of the plan before it gets applied. In fact, the requirement to come up with a temporary file name and repeat it in the second command is kind of annoying and I suspect I will one day screw up and pass an old plan to apply.

But also, we have lots of resources and running refresh twice would be annoying and slow.

Is it feasible to add (with a flag perhaps) a yes/no prompt to apply between printing the plan and executing it? This would solve the overall problem of allowing an "apply" with a chance to check your work without needing to do two refreshes or an explicit refresh command, extend the plan format, or make me run two separate commands with a unique temp file name.

Happy to work on a PR if people think it's a good idea.

[glasser]

Here's a PR to address my concern from above: #7251

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.