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

Check for stale plan with no state metadata #29755

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Oct 13, 2021

When we create the first planfile with no existing state, the internal snapshot will have no metadata to check. We were incorrectly skipping the entire staleness check when encountering this situation, allowing that first plan file to be applied multiple times. This can overwrite the existing state with new resources, leading to extensive manual cleanup.

First we can return different errors for lineage and serial to help direct users when the plan is incorrectly applied. When encountering a "first plan", only check the serial to provide a better error message indicating it is most likely stale, rather than from a different state which we cannot verify.

Fixes #24078

Ensure that we still check for a stale plan even when it was created
with no previous state.

Create separate errors for incorrect lineage vs incorrect serial.

To prevent confusion when applying a first plan multiple times, only
report it as a stale plan rather than different lineage.
In order to test applying a plan from an existing state, we need to be
able to inject the state meta into the planfile.
@jbardin jbardin requested a review from a team October 13, 2021 21:37
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This does seem like a good incremental improvement over what we had before and I would like to move forward with it as-is.

However, this did get me thinking about why we aren't also using statefile.StatesMarshalEqual here to get an even stronger assurance that we're applying to an identical state snapshot than what we planned against. The lineage/serial checks would still be important then to deal with a situation where two different snapshots just happen to have identical content, but we'd get a stronger assurance in the first-apply case that we aren't applying one workspace's first saved plan as another workspace's first plan too.

Can you think of reasons why that wouldn't be appropriate? Again, I'm not suggesting that we delay this changeset to do it, but I think it would be good to consider this stronger idea as a separate change while we have the relevant mental context for this loaded anyway.

@jbardin
Copy link
Member Author

jbardin commented Oct 14, 2021

Yeah, comparing the state directly rather than using these metadata fields is definitely an option. It was the case of dealing with identical states except for the lineage/serial fields that I didn't pursue it immediately. And since we would have to check lineage and serial anyway to account for that case, the added value of checking the state contents seems pretty small when outside of manipulation on the state data external from Terraform, the lineage+serial always correspond to one exact state.

@jbardin jbardin merged commit ef3c984 into main Oct 14, 2021
@jbardin jbardin deleted the jbardin/first-plan-lineage branch October 14, 2021 17:31
@ZF-fredericvanlinthoudt

@jbardin
Any idea when this fix will be released?
We are greedily waiting on this fix as we have a lot of manual cleanup work due to the bug linked to this PR.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform apply executes on stale plan file
3 participants