* command: keep our promises
* remove some nil config checks
Remove some of the safety checks that ensure plan nodes have config attached at the appropriate time.
* add GeneratedConfig to plan changes objects
Add a new GeneratedConfig field alongside Importing in plan changes.
* add config generation package
The genconfig package implements HCL config generation from provider state values.
Thanks to @mildwonkey whose implementation of terraform add is the basis for this package.
* generate config during plan
If a resource is being imported and does not already have config, attempt to generate that config during planning. The config is generated from the state as an HCL string, and then parsed back into an hcl.Body to attach to the plan graph node.
The generated config string is attached to the change emitted by the plan.
* complete config generation prototype, and add tests
---------
Co-authored-by: Katy Moe <katy@katy.moe>
During a plan, Terraform now checks for the presence of import blocks.
For each resource in config, if an import block is present with a matching address, planning that node will now trigger an ImportResourceState and ReadResource. The resulting state is treated as the node's "refresh state", and planning proceeds as normal from there.
The walkImport operation is now only used for the legacy "terraform import" CLI command. This is the only case under which the plan should produce graphNodeImportStates.
In order to complete the terraform destroy command, a refresh must first
be done to update state and remove any instances which have already been
deleted externally. This was being done with a refresh plan, which will
avoid any condition evaluations and avoid planning new instances. That
however can fail due to invalid references from resources that are
already missing from the state.
A new plan type to handle the concept of the pre-destroy-refresh is
needed here, which should probably be incorporated directly into the
destroy plan, just like the original refresh walk was incorporated into
the normal planning process. That however is major refactoring that is
not appropriate for a patch release.
Instead we make two discrete changes here to prevent blocking a destroy
plan. The first is to use a normal plan to refresh, which will enable
evaluation because missing and inconsistent instances will be planned
for creation and updates, allowing them to be evaluated. That is not
optimal of course, but does revert to the method used by previous
Terraform releases until a better method can be implemented.
The second change is adding a preDestroyRefresh flag to the planning
process. This is checked in any location which evalCheckRules is called,
and lets us change the diagnosticSeverity of the output to only be
warnings, matching the behavior of a normal refresh plan.
The graph walking mechanism is specified as requiring a graph with a single
root, which in practice means there's exactly one node in the graph
which doesn't have any dependencies.
However, we previously weren't verifying that invariant is true for
subgraphs returned from DynamicExpand. It was working anyway, but it's not
ideal to be relying on a behavior that isn't guaranteed by our underlying
infrastructure.
We also previously had the RootTransformer being a bit clever and trying
to avoid adding a new node if there is already only a single graph with
no dependencies. That special case isn't particularly valuable since
there's no harm in turning a one-node graph into a two-node graph with
an explicit separate root node, and doing that allows us to assume that
the root node is always present and is always exactly terraform.rootNode.
Many existing DynamicExpand implementations were not producing valid
graphs and were previously getting away with it. All of them now produce
properly-rooted graphs that should pass validation, and we will guarantee
that with an explicit check of the DynamicExpand return value before we
try to walk that subgraph. For good measure we also verify that the root
node is exactly terraform.rootNode, even though that isn't strictly
required by our graph walker, just to help us catch potential future bugs
where a DynamicExpand implementation neglects to add our singleton root
node.
We previously did two levels of DynamicExpand to go from ConfigResource to
AbsResource and then from AbsResource to AbsResourceInstance.
We'll now do the full expansion from ConfigResource to AbsResourceInstance
in a single DynamicExpand step inside nodeExpandPlannableResource.
The new approach is essentially functionally equivalent to the old except
that it fixes a bug in the previous implementation: we will now call
checkState.ReportCheckableObjects only once for the entire set of
instances for a particular resource, which is what the checkable objects
infrastructure expects so that it can always mention all of the checkable
objects in the check report even if we bail out partway through due to
a downstream error.
This is essentially the same code but now turned into additional methods
on nodeExpandPlannableResource instead of having the extra graph node
type. This has the further advantage of this now being straight-through
code with standard control flow, instead of the unusual inversion of
control we were doing before bouncing in and out of different Execute and
DynamicExpand implementations to get this done.
We may need to prune nodes from a full destroy plan graph which cannot
be evaluated if there is no current state.
Add missing method to nodeExpandPlannableResource to ensure planned
resource are handled correctly when pruning nodes.
The "checks" package is an expansion what we previously called
plans.Conditions to accommodate a new requirement that we be able to track
which checks we're expecting to run even if we don't actually get around
to running them, which will be helpful when we start using checks as part
of our module testing story because test reporting tools appreciate there
being a relatively consistent set of test cases from one run to the next.
So far this should be essentially a no-op change from an external
functionality standpoint, aside from some minor adjustments to how we
report some of the error and warning cases from condition evaluation in
light of the fact that the "checks" package can now track errors as a
different outcome than a failure of a valid check.
As is often the case with anything which changes what we track
in the EvalContext and persist between plan and apply, Terraform Core is
pretty brittle and so this had knock-on effects elsewhere too. Again, the
goal is for these changes to not create any material externally-visible
difference, and just to accommodate the new assumption that there will
always be a "checks" object available for tracking during a graph walk.
Combine all plan-time graphs into a single graph builder, because
_everything is a plan_!
Convert the import graph to a plan graph. This should resolve a few edge
cases about things not being properly evaluated during import, and takes
a step towards being able to _plan_ an import.
Expanded resource instances can initially share the same dependency
slice, so we must take care to not modify the array values when
checking the dependencies.
In the future we can convert these to a generic Set data type, as we
often need to compare for equality and take the union of multiple groups
of dependencies.
Complete the removal of the Validate option for graph building. There is
no case where we want to allow an invalid graph, as the primary reason
for validation is to ensure we have no cycles, and we can't walk a graph
with cycles. The only code which specifically relied on there being no
validation was a test to ensure the Validate flag prevented it.
When planning in refresh-only mode, we must not remove orphaned
resources due to changed count or for_each values from the planned
state. This was previously happening because we failed to pass through
the plan's skip-plan-changes flag to the instance orphan node.
Going back a long time we've had a special magic behavior which tries to
recognize a situation where a module author either added or removed the
"count" argument from a resource that already has instances, and to
silently rename the zeroth or no-key instance so that we don't plan to
destroy and recreate the associated object.
Now we have a more general idea of "move statements", and specifically
the idea of "implied" move statements which replicates the same heuristic
we used to use for this behavior, we can treat this magic renaming rule as
just another "move statement", special only in that Terraform generates it
automatically rather than it being written out explicitly in the
configuration.
In return for wiring that in, we can now remove altogether the
NodeCountBoundary graph node type and its associated graph transformer,
CountBoundaryTransformer. We handle moves as a preprocessing step before
building the plan graph, so we no longer need to include any special nodes
in the graph to deal with that situation.
The test updates here are mainly for the graph builders themselves, to
acknowledge that indeed we're no longer inserting the NodeCountBoundary
vertices. The vertices that NodeCountBoundary previously depended on now
become dependencies of the special "root" vertex, although in many cases
here we don't see that explicitly because of the transitive reduction
algorithm, which notices when there's already an equivalent indirect
dependency chain and removes the redundant edge.
We already have plenty of test coverage for these "count boundary" cases
in the context tests whose names start with TestContext2Plan_count and
TestContext2Apply_resourceCount, all of which continued to pass here
without any modification and so are not visible in the diff. The test
functions particularly relevant to this situation are:
- TestContext2Plan_countIncreaseFromNotSet
- TestContext2Plan_countDecreaseToOne
- TestContext2Plan_countOneIndex
- TestContext2Apply_countDecreaseToOneCorrupted
The last of those in particular deals with the situation where we have
both a no-key instance _and_ a zero-key instance in the prior state, which
is interesting here because to exercises an intentional interaction
between refactoring.ImpliedMoveStatements and refactoring.ApplyMoves,
where we intentionally generate an implied move statement that produces
a collision and then expect ApplyMoves to deal with it in the same way as
it would deal with all other collisions, and thus ensure we handle both
the explicit and implied collisions in the same way.
This does affect some UI-level tests, because a nice side-effect of this
new treatment of this old feature is that we can now report explicitly
in the UI that we're assigning new addresses to these objects, whereas
before we just said nothing and hoped the user would just guess what had
happened and why they therefore weren't seeing a diff.
The backend/local plan tests actually had a pre-existing bug where they
were using a state with a different instance key than the config called
for but getting away with it because we'd previously silently fix it up.
That's still fixed up, but now done with an explicit mention in the UI
and so I made the state consistent with the configuration here so that the
tests would be able to recognize _real_ differences where present, as
opposed to the errant difference caused by that inconsistency.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.
If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
