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.
Also add regression test coverage of the crash. This would occur when
objects with optional attributes had default values of different type
from the attribute type, and the objects were members of a collection.
For example:
list(object({
a = optional(set(string), [])
}))
If this type constraint is applied to a variable value where one object
has a set(string) value for a, and the other object applies the empty
tuple default, Terraform would crash.
Previously the cloud backend only supported the pre and post plan stages.
This commit adds support to display the output of the new pre-apply task
stage as well.
Previously all of the logic to retrieve Task Stages was in the backend_plan.go file.
This commit:
* Moves the logic to the backend_taskStages.go file.
* Replaces using an array to a map indexed by the Task Stage's stage. This makes it
easier to find the relevant stage in the list and means the getTaskStageIDByName
function is no longer required.
Applying object type defaults to null values can convert null to an
object with partial attributes. This means that even a specified default
value of null will not remain null after variable evaluation.
In turn, the result can then be invalid, if not all attributes in an
object type have defaults specified.
This commit skips the type default application step during config load
and variable evaluation if the default or given value is null of any
type. We still perform type conversion.
When handling ignore_changes=all, we must filter computed attributes
from the prior state to prevent them showing in the configuration. Since
it's not valid for the user to have set computed attributes in the
config, the provider should expect to never see any values there. The
oversight has only now become apparent, as more providers adopt the
plugin-framework which has direct access to the plan-time configuration
value.
Add a new ChangeReason, ReasonDeleteBecauseNoMoveTarget, to provide better
information in cases where a planned deletion is due to moving a resource to
a target not in configuration.
Consider a case in which a resource instance exists in state at address A, and
the user adds a moved block to move A to address B. Whether by the user's
intention or not, address B does not exist in configuration.
Terraform combines the move from A to B, and the lack of configuration for B,
into a single delete action for the (previously nonexistent) entity B.
Prior to this commit, the Terraform plan will report that resource B will be
destroyed because it does not exist in configuration, without explicitly
connecting this to the move.
This commit provides the user an additional clue as to what has happened, in a
case in which Terraform has elided a user's action and inaction into one
potentially destructive change.
This is a clumsy way to do this, but a pragmatic way to inform potential
consumers that this part of the format is not yet finalized without having
to read the docs to see our warning about that.
We need to get some practical experience with a few different consumers
making use of this format before we can be confident that it's designed
appropriately. We're not _expecting_ to break it, but we'd like to leave
the opportunity open in case we quickly learn that there's something
non-ideal about this design.
Previously we were attempting to infer the checkable object address kind
of a given address by whether it included "output" in the position where
a resource type name would otherwise go.
That was already potentially risky because we've historically not
prevented a resource type named "output", and it's also a
forward-compatibility hazard in case we introduce additional object kinds
with entirely-new addressing schemes in future.
Given that, we'll instead always be explicit about what kind of address
we're storing in a wire or file format, so that we can make sure to always
use the intended parser when reading an address back into memory, or
return an error if we encounter a kind we're not familiar with.
This is a new-shaped representation of check results which follows the
two-tiered structure of static objects and dynamic instances of objects,
thereby allowing consumers to see which checkable objects exist in the
configuration even if a dynamic evaluation error prevented actually
expanding them all to determine their declared instances.
Eventually we'll include this in the state too, but this initially adds it
only to the plan in order to replace the now-deprecated experimental
conditions result that was present but undocumented in Terraform v1.2.
In an earlier commit we changed the states.CheckResults model to
explicitly model the config object vs. dynamic checkable object hierarchy,
but neglected to update the logic in Terraform Core to take that into
account when propagating the object expansion decisions from the plan
phase to the apply phase. That meant that we were incorrectly classifying
zero-instance resources always as having an unknown number of instances,
rather than possibly being known to have zero instances.
This now follows the two-level heirarchy of the data structure, which has
the nice side-effect that we can remove some of the special-case methods
from checks.State that we were using to bulk-load data: the data is now
shaped in the appropriate way to reload the data using the same method
the plan phase would've used to record the results in the first place.
This allows us to retain check results from one run into the next, so that
we can react to status changes between runs and potentially report e.g.
that a previously-failing check has now been fixed, or that a
previously-failing check is "still failing" so that an operator can get
a hint as to whether a problem is something they've just introduced or if
it was already an active problem before they made a change.
A significant goal of the design changes around checks in earlier commits
(with the introduction of package "checks") was to allow us to
differentiate between a configuration object that we didn't expand at all
due to an upstream error, which has _unknown_ check status, and a
configuration object that expanded to zero dynamic objects, which
therefore has a _passing_ check status.
However, our initial lowering of checks.State into states.CheckResults
stayed with the older model of just recording each leaf check in isolation,
without any tracking of the containers.
This commit therefore lightly reworks our representation of check results
in the state and plan with two main goals:
- The results are grouped by the static configuration object they came
from, and we capture an aggregate status for each of those so that
we can differentiate an unknown aggregate result from a passing
aggregate result which has zero dynamic associated objects.
- The granularity of results is whole checkable objects rather than
individual checks, because checkable objects have durable addresses
between runs, but individual checks for an object are more of a
syntactic convenience to make it easier for module authors to declare
many independent conditions that each have their own error messages.
Since v1.2 exposed some details of our checks model into the JSON plan
output there are some unanswered questions here about how we can shift to
reporting in the two-level heirarchy described above. For now I've
preserved structural compatibility but not semantic compatibility: any
parser that was written against that format should still function but will
now see fewer results. We'll revisit this in a later commit and consider
other structures and what to do about our compatibility constraint on the
v1.2 structure.
Otherwise though, this is an internal-only change which preserves all of
the existing main behaviors of conditions as before, and just gets us
ready to build user-facing features in terms of this new structure.
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.
The checks.Checks type aims to encapsulate keeping track of check results
during a run and then reporting on them afterwards even if the run was
aborted early for some reason.
The intended model here is that each new run starts with an entirely fresh
checks.Checks, with all of the statuses therefore initially unknown, and
gradually populates the check results as we walk the graph in Terraform
Core. This means that even if we don't complete the run due to an error
or due to targeting options we'll still report anything we didn't visit
yet as unknown.
This commit only includes the modeling of checks in the checks package.
For now this is just dead code, and we'll wire it in to Terraform Core in
subsequent commits.
We previously added methods like this for some of the other types in this
package, including Local in this same file, but apparently haven't needed
these two yet.
Our existing addrs.Checkable represents a particular (possibly-dynamic)
object that can have checks associated with it.
This new addrs.ConfigCheckable represents static configuration objects
that can potentially generate addrs.Checkable objects.
The idea here is to allow us to predict from the configuration a set of
potential checkable object containers and then dynamically associate
the dynamic checkable objects with them as we make progress with planning.
This is intended for our integration of checks into the "terraform test"
testing harness, to be used instead of the weirdo builtin provider we were
using as a placeholder before we had first-class syntax for checks.
Test reporting tools find it helpful for there to be a consistent set of
test cases from one run to the next so that they can report on trends over
multiple runs, and so our ConfigCheckable addresses will serve as the
relatively-static "test case" that we'll then associate the dynamic checks
with, so that we can still talk about objects in the test result report
even if we end up not reaching them due to an upstream conditution failure.
This is a complement to "timestamp" and "timeadd" which allows
establishing the ordering of two different timestamps while taking into
account their timezone offsets, which isn't otherwise possible using the
existing primitives in the Terraform language.
Go 1.19's "fmt" has some awareness of the new doc comment formatting
conventions and adjusts the presentation of the source comments to make
it clearer how godoc would interpret them. Therefore this commit includes
various updates made by "go fmt" to acheve that.
In line with our usual convention that we make stylistic/grammar/spelling
tweaks typically only when we're "in the area" changing something else
anyway, I also took this opportunity to review most of the comments that
this updated to see if there were any other opportunities to improve them.
Prevously the cloud backend would only render post-plan run tasks. Now
that pre-plan tasks are in beta, this commit updates the plan phase to
render pre-plan run tasks. This commit also moves some common code to
the common backend as it will be used by other task stages in the
future.
Previously, when applying defaults to an input variable's given value
before type conversion, we would permit `null` attribute values to
override a specified default. This behaviour is inconsistent with the
intent of the type system underlying Terraform, and represented a
divergence from the treatment of `null` as equivalent to unset which
exists in resources. The same behaviour exists in top-level variable
definitions with `nullable = false`, and we consider this to be the
preferred behaviour here too.
This commit slightly changes default value application such that an
explicit `null` attribute value is treated as equivalent to the
attribute being missing. Default values for attributes will now replace
explicit nulls.
We can't validate that data from deprecated nested attributes is used in
the configuration, but we can at least catch the simple case where a
deprecated attribute is referenced directly.
All the code infrastructure was there to support formatting multiple
files already.
This makes `terraform fmt` more flexible and also compliant with the
[treefmt formatter
spec](https://numtide.github.io/treefmt/docs/formatters-spec.html)
The StaticValidateTraversal code was not taking into account nested
structural types. Rather than create more special cases for checking
Type vs NestedType, we move the ImpliedType method up to the Attribute
to ensure both are used to generate the final type spec.
If there are outputs in configuration, a destroy plan will always contain a "delete" change for each of these outputs.
This leads to meaningless delete changes being present for outputs which were not present in state and therefore cannot be deleted. Since there is a change in the plan, this plan will then be considered applyable, and the user will be presented with text instructing them to apply a plan in which there are no actual changes.
This commit stops the above from happening in the case of root module outputs.
Return early from AssertPlanValid for any attribute which is only
computed. We currently fail if there's a config value, but that could
only happen because of core, not because of the provider.
Normally, `terraform output` refreshes and reads the entire state in the command package before pulling output values out of it. This doesn't give Terraform Cloud the opportunity to apply the read state outputs org permission and instead applies the read state versions permission.
I decided to expand the state manager interface to provide a separate GetRootOutputValues function in order to give the cloud backend a more nuanced opportunity to fetch just the outputs. This required moving state Refresh/Read code that was previously in the command into the shared backend state as well as the filesystem state packages.
Previously we tried to early-exit before doing anything at all for any
no-op changes, but that means we also skip some ancillary steps like
evaluating any preconditions/postconditions.
Now we'll skip only the main action itself for plans.NoOp, and still run
through all of the other side-steps.
Since one of those other steps is emitting events through the hooks
interface, this means that now no-op actions are visible to hooks, whereas
before we always filtered them out before calling. I therefore added some
additional logic to the hooks to filter them out at the UI layer instead;
the decision for whether or not to report that we visited a particular
object and found no action required seems defensible as a UI-level concern
anyway.
We previously would optimize away the graph nodes for any resource
instance without a real change pending, but that means we don't get an
opportunity to re-check any invariants associated with the instance, such
as preconditions and postconditions.
Other upstream changes during apply can potentially decide the outcome of
a condition even if the instance itself isn't being changed, so we do
still need to revisit these during apply or else we might skip running
certain checks altogether, if they yielded unknown results during planning
and then don't get run during apply.
We previously had a special case in the graph transformer for output
values where it would directly create an individual output value node
instead of an "expand" node as we would do for output values in nested
modules.
While it's true that we do always know that expanding a root module
output value will always produce exactly one instance, treating this case
as special creates the risk of those two codepaths diverging in other
ways.
Instead, we'll let the expand node also deal with root modules and
minimize the special case only to how we look up any changes for the
output values, since the design of plans.Changes is a bit awkward and
requires us to ask the question differently for root module output values.
Otherwise, the behavior will now be consistent across all output values
regardless of module.
The dag package did not previously provide a topological walk of a given
graph. While the existing combination of a transitive reduction with a
depth-first walk appeared to accomplish this, depth-first is only
equivalent with a simple tree. If there are multiple paths to a node, a
depth-first approach will skip dependencies from alternate paths.
A topological walk was previously only done in Terraform via the
concurrent method used for walking the primary dependency graph in core.
Sometime however we want a dependency ordering without the overhead of
instantiating the concurrent walk with the channel-based edges.
Add TopologicalOrder and ReverseTopologicalOrder to obtain a list of
nodes which can be used to visit each while ensuring that all
dependencies are satisfied.
Make DAG walks test-able, and add tests for more complex graph ordering.
We also add breadth-first for comparison, though it's not used currently
in Terraform.
These tests were originally written long before Go supported subtests
explicitly, but now that we have t.Run we can avoid the prior problem
that one test failing would mask all of the others that followed it.
Now we'll always run all of them, potentially collecting more errors in
a single run so we can have more context to debug with and potentially
fix them all in a single step rather than one by one.
* terraform init: add suggested fix for when a checksum is missing from the lock file
* improve error message
* add link to the documentation
* cleanup leftovers from previous attempt
* fix tests
* s/,/;
* fix imports
* Add golden JSON test for Terraform plan
* Add data source to golden JSON plan
* Move output comparison code into shared helper function
* Add note for maintainer to contact TFC when UI changes
UI changes may potentially impact the behavior of structured run output
on TFC.
* Add test_data_source to other mock providers
* refactor: Use tfaddr for provider address parsing
* refactor: Use tfaddr for module address parsing
* deps: introduce hashicorp/terraform-registry-address
So far we've only ever needed to re-parse address strings that happen not
to contain instance keys and so we've gotten away with our serialization
of these not being quite right, but given how liberally we've expected to
be able to use address strings from this package for wire format
interchange it seems likely that this is going to surprise us eventually.
Now we'll use an escaping scheme compatible with HCL's parser rather
than Go's parser, and so we can safely rely on hclsyntax.ParseTraversal
as part of reversing this operation to transform an address string back
into an address equivalent to the value it was created from.
This is most easily handled in the plugin code, without involving
Terraform core.
The biggest change here other than checking the PlanDestroy capability,
is the removal of the schema helper methods in the plugins. With the
addition of the capabilities field, combined with the necessity of
checking diagnostics from the schema, the helpers have outlived their
usefulness. Perhaps there's a better pattern for these repetitive calls,
but for now there isn't too extra verbosity involved.
Call PlanResourceDestroy during a destroy plan.
This allows providers two new abilities:
- They can evaluate if the plan is valid, notifying users of any
potential errors before an apply is started, which may not be able to
complete.
- They can inspect and modify their private data during a destroy plan
just like they can with an other plan operation.
Since we've both concluded the module_variables_optional_attrs experiment
and made experiments available only in alpha releases in the same minor
release, we accidentally made the more general message about experiments
not being available mask the specific message about the experiment being
concluded.
In order to give better feedback to those who were participating in the
experiment in earlier Terraform releases, we'll retain a minimal exception
to our checks to allow the "experiment has concluded" error message to
shine through if and only if that is the only selected experiment.
* Fail global required_version check if it contains any prerelease fields
* go mod tidy
* Improve required_version prerelease not supported error string
* Add prerelease version constraint unit tests
* Fix side-effects by populating global diags too soon
There are no good options for inserting diagnostics into the backend
lookup, or creating a backend which reports it's removal because none of
the init or GetSchema functions return any errors.
Keep a registry of the removed backend so that we can at least notify
users that a backend was removed vs an invalid name.
This allows us to remove the manual replace directives
github.com/dgrijalva/jwt-go and google.golang.org/grpc, so that we can
remove the CVE warnings and update the grpc packages.
While the etcdv3 backend is also marked as deprecated, the changes here
are done in a manner to keep that backend working for the time being.
By observing the sorts of questions people ask in the community, and the
ways they ask them, we've inferred that various different people have been
confused by Terraform reporting that a value won't be known until apply
or that a value is sensitive as part of an error message when that message
doesn't actually relate to the known-ness and sensitivity of any value.
Quite reasonably, someone who sees Terraform discussing an unfamiliar
concept like unknown values can assume that it must be somehow relevant to
the problem being discussed, and so in that sense Terraform's current
error messages are giving "too much information": information that isn't
actually helpful in understanding the problem being described, and in the
worst case is a distraction from understanding the problem being described.
With that in mind then, here we introduce an explicit annotation on
diagnostic objects that are directly talking about unknown values or
sensitive values, and then the diagnostic renderer will react to that to
avoid using the terminology "known only after apply" or "sensitive" in the
generated diagnostic annotations unless we're rendering a message that is
explicitly related to one of those topics.
This ends up being a bit of a cross-cutting concern because the code that
generates these diagnostics and the code that renders them are in separate
packages and are not directly aware of each other. With that in mind, the
logic for actually deciding for a particular diagnostic whether it's
flagged in one of these special ways lives inside the tfdiags package as
an intermediation point, which both the diagnostic generator (in the core
package) and the diagnostic renderer can both depend on.
When an error occurs in a function call, the error message text often
includes references to particular parameters in the function signature.
This commit improves that reporting by also including a summary of the
full function signature as part of the diagnostic context in that case,
so a reader can see which parameter is which given that function
arguments are always assigned positionally and so the parameter names
do not appear in the caller's source code.
HCL's diagnostic model now includes the idea of "extra information" which
works by attaching an initially-opaque interface value to each diagnostic
and then asking callers to type-assert against that value to sniff for
particular interfaces in order to discover additional machine-readable
context about a certain diagnostic message.
This commit echoes that idea into our tfdiags API, for now only for
diagnostics that are backed by an hcl.Diagnostic. All other implementations
of the diagnostic interface just always return nil, which means they never
carry any "extra information".
As is typical for our wrapping abstraction, we have here also a modified
copy of HCL's helper function for conveniently probing a diagnostic for
information of a particular type, designed to work with our diagnostic
interface instead of HCL's concrete diagnostic type.
When validating self-references for resource and data source
preconditions and postconditions, we previously did not nil-check the
block's condition field, which caused a panic when the block had no
condition.
While fixing this I noticed that we were not validating that there are
no self-references in the error message, so fixed that.
