-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
command + backend/local: -refresh-only and drift detection #28634
Conversation
54caf24
to
f5a4e7f
Compare
While working on this PR I tripped over a few little hurdles in other parts of the code, but they are not really directly related to the task at hand here so I've split them off into separate PRs in the hope that they'll be easier to review and merge that way, while I finish up the remaining work on this PR. Assuming we merge all of those, I'll rebase them out of this PR before I take it out of draft status. |
10bd14f
to
9ffb3a5
Compare
I think this is ready to review now! Along with reviewing the implementation, if anyone is willing I'd appreciate to have someone try this from an end-user perspective and try some different permutations to see how the planning output behaves. There's a bunch of different combinations of interesting situations to report now, some of which interact with one another, so while I think it's doing something reasonable in all cases I wouldn't be surprised to learn that there are some less common combinations that I didn't consider. |
While testing @alisdair spotted that we weren't properly respecting refresh-only mode for the destroy actions generated for "deposed" objects in the state. This is unfortunately a specific aspect of a more general problem that we've historically not been handling deposed objects consistently with "orphaned" current objects, even though they are conceptually the same idea just in two different circumstances. Consequently, the fix for that ended up being the fix for that general problem, and so I've added a new commit here which is quite out of scope for what this PR was setting out to do but unfortunately The commit message for my extra commit contains some additional context:
The main upshot of this for folks not using |
ac5afe6
to
fe80203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! 👏
I've tested the branch out locally with various configurations, although probably not comprehensively. The UI was very readable throughout, and the drift detection in particular is really clear.
I have one inline test concern (with the JSON output golden reference), and a rambling musing about the changes to the golden reference, but otherwise ✅ from me.
@@ -1,5 +1,6 @@ | |||
{"@level":"info","@message":"Terraform 0.15.0-dev","@module":"terraform.ui","terraform":"0.15.0-dev","type":"version","ui":"0.1.0"} | |||
{"@level":"info","@message":"test_instance.foo: Plan to create","@module":"terraform.ui","change":{"resource":{"addr":"test_instance.foo","module":"","resource":"test_instance.foo","implied_provider":"test","resource_type":"test_instance","resource_name":"foo","resource_key":null},"action":"create"},"type":"planned_change"} | |||
{"@level":"info","@message":"Plan: 1 to add, 0 to change, 0 to destroy.","@module":"terraform.ui","changes":{"add":1,"change":0,"remove":0,"operation":"plan"},"type":"change_summary"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new message surprised me at first, but I guess it's just a consequence of the plan rendering being restructured. My first concern was that it would make integration more difficult, but fortunately the changes
object includes an operation
field that I had forgotten about, so I think this is just fine.
I believe that it also won't affect integrations which use a saved plan, which was my main concern anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this new message is the JSON equivalent of a new behavior in the human apply
output where Terraform repeats the plan that it's about to apply, because subjectively I found it jarring to have Terraform just immediately start doing something without explaining what it was doing, particularly in the case where there are no actions at all and thus updating the state was the only side-effect, where the output in that case made it seem like Terraform had done nothing at all.
While introducing this into the JSON output wasn't my original intent, once I figured out what I'd done and why it was happening it retroactively seemed justified, because a hypothetical custom UI around this output could for example choose to summarize that only 5 of 6 planned additions happened, for example. (Admittedly that would also be possible by parsing the detailed plan, but that's also true for the apply summary and so it seemed like making this information conveniently available was the intent.)
We've always had a mechanism to synchronize the Terraform state with remote objects before creating a plan, but we previously kept the result of that to ourselves, and so it would sometimes lead to Terraform generating a planned action to undo some upstream drift, but Terraform would give no context as to why that action was planned even though the relevant part of the configuration hadn't changed. Now we'll detect any differences between the previous run state and the refreshed state and, if any managed resources now look different, show an additional note about it as extra context for the planned changes that follow. This appears as an optional extra block of information before the normal plan output. It'll appear the same way in all of the contexts where we render plans, including "terraform show" for saved plans.
This is a light revamp of our plan output to make use of Terraform core's new ability to report both the previous run state and the refreshed state, allowing us to explicitly report changes made outside of Terraform. Because whether a plan has "changes" or not is no longer such a straightforward matter, this now merges views.Operation.Plan with views.Operation.PlanNoChanges to produce a single function that knows how to report all of the various permutations. This was also an opportunity to fill some holes in our previous logic which caused it to produce some confusing messages, including a new tailored message for when "terraform destroy" detects that nothing needs to be destroyed. This also allows users to request the refresh-only planning mode using a new -refresh-only command line option. In that case, Terraform _only_ performs drift detection, and so applying a refresh-only plan only involves writing a new state snapshot, without changing any real infrastructure objects.
In many ways a deposed object is equivalent to an orphaned current object in that the only action we can take with it is to destroy it. However, we do still need to take some preparation steps in both cases: first, we must ensure we track the upgraded version of the existing object so that we'll be able to successfully render our plan, and secondly we must refresh the existing object to make sure it still exists in the remote system. We were previously doing these extra steps for orphan objects but not for deposed ones, which meant that the behavior for deposed objects would be subtly different and violate the invariants our callers expect in order to display a plan. This also created the risk that a deposed object already deleted in the remote system would become "stuck" because Terraform would still plan to destroy it, which might cause the provider to return an error when it tries to delete an already-absent object. This also makes the deposed object planning take into account the "skipPlanChanges" flag, which is important to get a correct result in the "refresh only" planning mode. It's a shame that we have almost identical code handling both the orphan and deposed situations, but they differ in that the latter must call different functions to interact with the deposed rather than the current objects in the state. Perhaps a later change can improve on this with some more refactoring, but this commit is already a little more disruptive than I'd like and so I'm intentionally deferring that for another day.
In the very unusual situation where we end up planning to destroy a deposed object alone, it's likely that we're exposing users to this idea of "deposed" for the very first time. This additional sentence will hopefully give some extra context for what that means. We don't really have room here for a lengthy explanation about how deposed objects come to exist but this will hopefully be enough of a hook to help users connect this with an error they saw on a previous run, or at least to have some additional keywords to search for if they want to research further.
fe80203
to
6b12800
Compare
Is there an option/flag to get rid of the output that was introduced with this? While this output is useful for some use cases, I do not want to have a second plan output with changes outside of terraform which I decided to ignore via the lifecycle -> ignore_changes For instance
|
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. |
This is a light revamp of our plan output to make use of Terraform core's new ability to report both the previous run state and the refreshed state, allowing us to explicitly report changes made outside of Terraform.
Because whether a plan has "changes" or not is no longer such a straightforward matter, this now merges
views.Operation.Plan
withviews.Operation.PlanNoChanges
to produce a single function that knows how to report all of the various permutations. This was also an opportunity to fill some holes in our previous logic which caused it to produce some confusing messages, including a new tailored message for whenterraform destroy
detects that nothing needs to be destroyed.This also allows users to request the refresh-only planning mode using a new
-refresh-only
command line option. In that case, Terraform only performs drift detection, and so applying a refresh-only plan only involves writing a new state snapshot, without changing any real infrastructure objects. That is therefore equivalent toterraform refresh
except that it gives an opportunity to review what Terraform detected before creating a new state snapshot.