When reading this code to check Terraform's graph sorting behavior, I got very
confused about the direction of traversal for several methods. Although some of
these methods would also probably benefit from renames, this commit only updates
their doc comments to use the same directional terminology that we use in the
`Edge` interface (source/target).
Because import does not yet plan new instances as part of the import
process, we can end up evaluating references to resources which have no
state at all. The fallback for this situation could result in slightly
better values during import. The count and for_each values were
technically incorrect, since the length is not known to be zero, and the
single instance does have a concrete type which we can return.
When we checked for cycles with destroy edges around providers, it was
only for providers of a different type, but one can do the same thing
with the same provider under different local aliases. Check to see if
the provider also contains an alias, or is defined absolutely in some
other way. The absolute accuracy here isn't critical, since in most
cases these edges are not required for correct results, but finding a
correct and consistent method for determining when these edges are
needed is going to take more research.
There was also an oversight fixed here where the basic
creator->destroyer edges were added _after_ the cycle checks, limiting
their utility. The ordering of the additions was swapped to make sure
all cycles are noticed.
* Add ability to use customer-managed KMS key to encrypt state, add acceptance tests
* Change test names for different encrpytion methods
* Commit files updated by `go mod tidy`
* Add guard against missing ENVs to `setupKmsKey` func
* Update KMS setup function to get credentials from ENVs
* Update tests to not include zero-values in config
This means that default values are supplied later by TF instead of supplied as config from the user
This also avoids issues related to making field conflicts explicit with `ConflictsWith`
* Make `encryption_key` & `kms_encryption_key` conflicting fields
Removing the Default from `encryption_key` does not appear to be a breaking change when tested manually
* Add ability to set `kms_encryption_key` via ENV
* Refactor `encryption_key` to use `DefaultFunc` to access ENV, if set
* Remove comments
* Update `gcs` backend docs & descriptions in schema
* Update `gcs` backend docs to include information on encryption methods
* Apply technical writing suggestions from code review
Co-authored-by: Matthew Garrell <69917312+mgarrell777@users.noreply.github.com>
* Update documentation to remove passive voice
* Change use of context in tests, add inline comment, update logs
* Remove use of `ReadPathOrContents` for new field
Co-authored-by: Matthew Garrell <69917312+mgarrell777@users.noreply.github.com>
If a previously deposed object is deleted outside of Terraform, the
next plan will result in a NoOp change for the deposed object. Fix the
check to verify that the deposed object has an acceptable action rather
than use the `update` flag.
We originally included this warning because the go-cty-yaml module wasn't
yet stable and it was also not extensively tested so it wasn't yet clear
if its behavior would need to change in some less common cases we hadn't
tested so far.
However, go-cty-yaml had its v1.0.0 release some time ago and is now
committed to preserving its current Marshal output unless it is found to
be non-compliant with the YAML 1.2 specification. This doc change means
that Terraform's yamlencode is now adopting a similar posture:
- The exact style details produced by the function for a particular input
are now frozen. It'll change only if we find that the function is
producing output that isn't valid per the YAML spec.
- If someone finds a YAML parser that cannot parse what yamlencode
produces but what it produces is valid per the YAML 1.2 spec, we'll
expect the parser to be corrected to better support the spec rather
than changing the yamlencode output.
There may be pragmatic exceptions if we encounter a situation we cannot
anticipate yet, but the above will be our general rule. This is really
just a specialization of the spirit of the v1.x Compatibility Promises,
tailored specifically to this function.
Legacy providers expect Terraform to be able to clean up invalid plans
and computed attributes. Add a special case for the LegacyTypeSystem to
revert `ignore_changes = all` to the complete prior state.
Once again we're caught out by sharing the same output value node type
between the plan phase and the apply phase. To allow for some slight
variation between plan and apply without drastic refactoring here we just
add a new flag to nodeExpandOutput which is true only during the planning
phase.
This then allows us to register the checkable objects only during the
planning phase and not incorrectly re-register them during the apply phase.
It's incorrect to re-register during apply because we carry over the
planned checkable objects from the plan phase into the apply phase so we
can guarantee that the final state will have all of the same checkable
objects that the plan did.
This avoids a panic during the apply phase from the incorrect duplicate
registration.
Made a change to code example within the *Preconditions and Postconditions* section so that it technically makes sense; prior it was missing the data resource that was being called within the precondition lifecycle event on line 135, and the aws_instance resource was not utilizing the ami being provided by the data source in line 129, so i changed that as well.
We use a non-pointer value for this particular node, which means that
there can never be two root nodes in the same graph: the graph
implementation will just coalesce them together when a second one is added.
Our resource expansion code is relying on that coalescing so that it can
subsume together multiple graphs for different modules instances into a
single mega-graph with all instances across all module instances, with
any root nodes coalescing together to produce a single root.
This also updates one of the context tests that exercises resource
expansion so that it will generate multiple resource instance nodes per
module and thus potentially have multiple roots to coalesce together.
However, we aren't currently explicitly validating the return values from
DynamicExpand and so this test doesn't actually fail if the coalescing
doesn't happen. We may choose to validate the DynamicExpand result in a
later commit in order to make it more obvious if future modifications fail
to uphold this invariant.
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 were previously _trying_ to handle diagnostics here but were not quite
doing it right because we were testing whether the resulting error was
nil rather than appending it to the diagnostics and then seeing if the
result has errors.
The difference here is important because it allows DynamicExpand to return
warnings without associated errors when needed. Previously the graph
walker would treat a warnings-only result as if it were an error.
Ideally we'd change DynamicExpand to return diagnostics directly, but we
previously decided against that because there were so many implementors
to update, and my intent for this change is to be surgical in the update
so we minimize risk of backporting the change into patch releases.
When adding destroy edges between resources from different providers,
and a provider itself depends on the other provider's resources, we can
get cycles in the final dependency graph.
The problem is a little deeper than simply not connecting these nodes,
since the edges are still needed when doing a full destroy operation.
For now we can get by assuming the edges are required, and reverting
them only if they result in a cycle. This works because destroy edges
are the last edges added to managed resources during graph building.
This was rarely a problem before v1.3, because noop nodes were not added
to the apply graph, and unused values were aggressively pruned. In v1.3
however all nodes are kept in the graph so that postcondition blocks are
always evaluated during apply, increasing the chances of the cycles
appearing.
Because import uses the complete planning process, it must also call
RemovePlannedResourceInstanceObjects. This is required to serialized the
resulting state if there are data sources with an ObjectPlanned status
because they could not be read during the import process.
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.
