Previously these details were relegated to the debug logs, which forces
the user to reproduce the error condition and then go digging for the
error message. Since we're asking users to report this error, let's give
them all the details they need right up front to make it a little easier
on them.
Context:
As part of building up a Plan, Terraform needs to detect "orphaned"
resources--resources which are present in the state but not in the
config. This happens when config for those resources is removed by the
user, making it Terraform's responsibility to destroy them.
Both state and config are organized by Module into a logical tree, so
the process of finding orphans involves checking for orphaned Resources
in the current module and for orphaned Modules, which themselves will
have all their Resources marked as orphans.
Bug:
In #3114 a problem was exposed where, given a module tree that looked
like this:
```
root
|
+-- parent (empty, except for sub-modules)
|
+-- child1 (1 resource)
|
+-- child2 (1 resource)
```
If `parent` was removed, a bunch of error messages would occur during
the plan. The root cause of this was duplicate orphans appearing for the
resources in child1 and child2.
Fix:
This turned out to be a bug in orphaned module detection. When looking
for deeply nested orphaned modules, root.parent was getting added twice
as an orphaned module to the graph.
Here, we add an additional check to prevent a double add, which
addresses this scenario properly.
Fixes#3114 (the Provisioner side of it was fixed in #4877)
Fixes an interpolation race that was occurring when a tainted destroy
node and a primary destroy node both tried to interpolate a computed
count in their config. Since they were sharing a pointer to the _same_
config, depending on how the race played out one of them could catch the
config uninterpolated and would then throw a syntax error.
The `Copy()` tree implemented for this fix can probably be used
elsewhere - basically we should copy the config whenever we drop nodes
into the graph - but for now I'm just applying it to the place that
fixes this bug.
Fixes#4982 - Includes a test covering that race condition.
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.
A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):
https://travis-ci.org/hashicorp/terraform/jobs/104812951
Refs #2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.
/cc @radeksimko
This commit adds support for declaring variable types in Terraform
configuration. Historically, the type has been inferred from the default
value, defaulting to string if no default was supplied. This has caused
users to devise workarounds if they wanted to declare a map but provide
values from a .tfvars file (for example).
The new syntax adds the "type" key to variable blocks:
```
variable "i_am_a_string" {
type = "string"
}
variable "i_am_a_map" {
type = "map"
}
```
This commit does _not_ extend the type system to include bools, integers
or floats - the only two types available are maps and strings.
Validation is performed if a default value is provided in order to
ensure that the default value type matches the declared type.
In the case that a type is not declared, the old logic is used for
determining the type. This allows backwards compatiblity with previous
Terraform configuration.
Instead of trying to skip non-targeted orphans as they are added to
the graph in OrphanTransformer, remove knowledge of targeting from
OrphanTransformer and instead make the orphan resource nodes properly
addressable.
That allows us to use existing logic in TargetTransformer to filter out
the nodes appropriately. This does require adding TargetTransformer to the
list of transforms that run during DynamicExpand so that targeting can
be applied to nodes with expanded counts.
Fixes#4515Fixes#2538Fixes#4462
Building on the work of #3846, deprecate `filename` in favor of a
`template` attribute that accepts file contents instead of a path.
Required a bit of work in the interpolation code to prevent Terraform
from assuming that template interpolations were resource variables that
needed to be resolved. Leaving them as "Unknown Variables" prevents
interpolation from happening early and lets the `template_file` resource
do its thing.
This attempts to reproduce the issue described in #2598 whereby outputs
added after an apply are not reflected in the output. As per the issue
the outputs are described using the JSON syntax.
We were only comparing the last element of the module, which meant that
deeply nested modules with the same name but different ancestry had an
undefined sort order, which could cause inconsistencies in state
storage and potentially break remote state MD5 checksumming.
Remote state includes MD5-based checksumming to protect against State
conflicts. This can generate improper conflicts with states that differ
only in their Schema version.
We began to see this issue with
https://github.com/hashicorp/terraform/pull/3470 which changes the
"schema_version" of aws_key_pairs.
Now that we support log line filtering (as of 0090c063) it's good to be
a bit more fussy about what log levels are assigned to different things.
Here we make a few things that are implementation details log as DEBUG,
and prevent spurious errors from EvalValidateCount where it was returning
an empty EvalValidateError rather than nil when everything was okay.
In #2884, Terraform would hang on graphs with an orphaned resource
depended on an orphaned module.
This is because orphan module nodes (which are dependable) were getting
expanded (replaced) with GraphNodeBasicSubgraph nodes (which are _not_
dependable).
The old `graph.Replace()` code resulted in GraphNodeBasicSubgraph being
entered into the lookaside table, even though it is not dependable.
This resulted in an untraversable edge in the graph, so the graph would
hang and wait forever.
Now, we remove entries from the lookaside table when a dependable node
is being replaced with a non-dependable node. This means we lose an
edge, but we can move forward. It's ~probably~ never correct to be
replacing depenable nodes with non-dependable ones, but this tweak
seemed preferable to tossing a panic in there.
When a provider validation only returns a warning, we were cutting the
evaltree short by returning an error. This is fine during a
`walkValidate` but was causing trouble during `walkPlan` and
`walkApply`.
fixes#2870
I was worried about the implications of deeply nested orphaned modules
in the parent commit, so I added a test. It's failing but not quite like
I expected it to. Perhaps I've uncovered an unrelated bug here?
/cc @mitchellh
The `CloseProviderTransformer` relies on the `ProvidedBy()` interface to
look up the proper dependency for the the graph nodes it adds. This
interface needs to yield the name of a provider, _AND_ for flattened
nodes it needs to yield the full path to a provider.
Destroy nodes did not implement this second part, which resulted in
"provider X couldn't be found" when both of these were true:
* A module included a resource that dependend on a provider
* The root did _NOT_ include a provider config
Implementing a proper ProvidedBy() on the flattened version of
destroy nodes solves the issue.
fixes#2581
The context_test file has gotten pretty unruly. Let's split it up into
a few files so we can be nicer to our editors and our own sanity.
Definitely lots more we can do to clean up, but with changes like this
I'd rather do small, focused, clear steps instead of one big "cleaned up
lots of stuff" PR.
By prefixing them with `cmd /c` it will work with both `winner` and
`ssh` connection types.
This PR also reverts some bad stringer changes made in PR #2673
In `helper/schema` we already makes a distinction between `Default`
which is always applied and `InputDefault` which is displayed to the
user for an empty field.
But for variables we just have `Default` which is treated like
`InputDefault`. This changes it to _not_ prompt the user for a value
when the variable declaration includes a default.
Treating this as a UX bugfix and the "don't prompt for variables w/
defaults set" behavior as the originally expected behavior we were
failing to honor.
Added an already-passing test to verify and cover the `helper/schema`
behavior.
Perhaps down the road we can add a `input_default` attribute to
variables to allow similar behavior to `helper/schema` in variables, but
for now just sticking with the fix.
Fixes#2592
Allows target dependencies to be properly calculated across module
boundaries, fixing scenarios where a target depends on something inside
a module, but the contents of the module are not included in the
targeted resources.
fixes#1858
When targeting prunes out all the resource nodes between a provider and
its close node, there was no dependency to ensure the close happened
after the configure. Needed to add an explicit dependency from the close
to the provider.
This tweak highlighted the fact that CloseProviderTransformer needed to
happen after DisableProviderTransformer, since
DisableProviderTransformer inspects up-edges to decide what to disable,
and CloseProviderTransformer adds an up-edge.
fixes#2495
Had to handle a lot of implicit leaning on a few properties of the old
representation:
* Old representation allowed plain strings to be treated as lists
without problem (i.e. shoved into strings.Split), now strings need to
be checked whether they are a list before they are treated as one
(i.e. shoved into StringList(s).Slice()).
* Tested behavior of 0 and 1 length lists in formatlist() was a side
effect of the representation. Needs to be special cased now to
maintain the behavior.
* Found a pretty old context test failure that was wrong in several
different ways. It's covered by TestContext2Apply_multiVar so I
removed it.
This is the initial pure "all tests passing without a diff" stage. The
plan is to change the internal representation of StringList to include a
suffix delimiter, which will allow us to recognize empty and
single-element lists.
The provider input before wasn't scoped by path, which caused
non-descendant parts of the graph to grab the configuration of another
sub-tree. The result is that you'd often get copied provider
configurations across the module barriers.
See GH-2024
Without this 12 line function it’s impossible to use any of the
Terraform code without the need for having the files on disk. As more
and more people are using (parts of) Terraform in other software, this
seems to be a very welcome addition. It has no negative impact on
Terraform itself whatsoever (the function is never called), but it
opens up a lot of other use cases.
Next to the single new function, I renamed the existing function (and
related tests) to better reflect what the function does. So now there
is a `LoadDir` function which calls `LoadFile` for each file, which
kind of made sense to me, especially when now adding a `LoadJSON`
function as well.
But of course if the rename is a problem, I can revert that part as
it’s not related to the added `LoadJSON` function.
Thanks!
Currently Terraform is leaking goroutines and with that memory. I know
strictly speaking this maybe isn’t a real concern for Terraform as it’s
mostly used as a short running command line executable.
But there are a few of us out there that are using Terraform in some
long running processes and then this starts to become a problem.
Next to that it’s of course good programming practise to clean up
resources when they're not needed anymore. So even for the standard
command line use case, this seems an improvement in resource management.
Personally I see no downsides as the primary connection to the plugin
is kept alive (the plugin is not killed) and only unused connections
that will never be used again are closed to free up any related
goroutines and memory.
Because CBD now runs after a RootTransformer, it's now operating on a
graph that _may_ have had a graphNodeRoot added to it (a noop node whose
only purpose is to be a root).
CBD includes a step that tells the destroy node to depend on any parents
of the create node. When one of those parents was "root", this was
causing the destroy node to depend on "root", making it cease to be an
actual root node.
Because graphNodeRoot is a singleton, the follow-up RootTransformer was
not sufficient to slap another root on top - it wasn't being seen as a
fresh node, so edges were just accumulating, and we ended up in a state
with "no roots".
refs #1903 (not sure if this will fix all the "no root found" cases, or
just the one I bumped into)
fixes#1947
Root cause was a bad edge being made by the CBD transform going from the
flattened destroy node to the unflattened create node, which was no
longer in the graph. The destroy node therefore had a dependency that
could never be satisfied, which locked up the walk.
Got this while playing around in a module:
> * unflattenable node: aws_security_group.internal (orphan)
> *terraform.graphNodeOrphanResource
Basically just copied implementation from
d503cc2d82
Adds the ability to target resources within modules, like:
module.mymod.aws_instance.foo
And the ability to target all resources inside a module, like:
module.mymod
Closes#1434
This reimplements my prior attempt at nipping issues where a plan did
not yield the same cycle an apply did. My prior attempt was to have
ctx.Validate generate a "Verbose" worst-case graph. It turns out that
skipping PruneDestroyTransformer to generate this graph misses important
heuristics that prevent cycles by dropping destroy nodes that are
determined to be unused.
This resulted in Validate improperly failing in scenarios where these
heuristics would have broken the cycle.
We detected the problem during the work on #1781 and worked around the
issue by reverting to the non-Verbose graph in Validate.
This commit accomplishes the original goal in a better way - by
generating the full graph and checking it once Plan has calculated the
diff. This guarantees that any graph issue that would be caught by Apply
will be caught by Plan.
When you specify `-verbose` you'll get the whole graph of operations,
which gives a better idea of the operations terraform performs and in
what order.
The DOT graph is now generated with a small internal library instead of
simple string building. This allows us to ensure the graph generation is
as consistent as possible, among other benefits.
We set `newrank = true` in the graph, which I've found does just as good
a job organizing things visually as manually attempting to rank the nodes
based on depth.
This also fixes `-module-depth`, which was broken post-AST refector.
Modules are now expanded into subgraphs with labels and borders. We
have yet to regain the plan graphing functionality, so I removed that
from the docs for now.
Finally, if `-draw-cycles` is added, extra colored edges will be drawn
to indicate the path of any cycles detected in the graph.
A notable implementation change included here is that
{Reverse,}DepthFirstWalk has been made deterministic. (Before it was
dependent on `map` ordering.) This turned out to be unnecessary to gain
determinism in the final DOT-level implementation, but it seemed
a desirable enough of a property that I left it in.
The `TargetTransform` was dropping provisioner nodes, which caused graph
validation to fail with messages about uninitialized provisioners when a
`terraform destroy` was attempted.
This was because `destroy` flops the dependency calculation to try and
address any nodes in the graph that "depend on" the target node. But we
still need to keep the provisioner node in the graph.
Here we switch the strategy for filtering nodes to only drop
addressable, non-targeted nodes. This should prevent us from having to
whitelist nodes to keep in the future.
closes#1541
Most CBD-related cycles include destroy nodes, and destroy nodes were
all being pruned from the graph before staring the Validate walk.
In practice this meant that we had scenarios that would error out with
graph cycles on Apply that _seemed_ fine during Plan.
This introduces a Verbose option to the GraphBuilder that tells it to
generate a "worst-case" graph. Validate sets this to true so that cycle
errors will always trigger at this step if they're going to happen.
(This Verbose option will be exposed as a CLI flag to `terraform graph`
in a second incoming PR.)
refs #1651
Adds an "alias" field to the provider which allows creating multiple instances
of a provider under different names. This provides support for configurations
such as multiple AWS providers for different regions. In each resource, the
provider can be set with the "provider" field.
(thanks to Cisco Cloud for their support)
When the `prevent_destroy` flag is set on a resource, any plan that
would destroy that resource instead returns an error. This has the
effect of preventing the resource from being unexpectedly destroyed by
Terraform until the flag is removed from the config.
fixes#1508
In a DESTROY/CREATE scenario, the plan diff will be run against the
state of the old instance, while the apply diff will be run against an
empty state (because the state is cleared when the destroy node does its
thing.)
For complex attributes, this can result in keys that seem to disappear
between the two diffs, when in reality everything is working just fine.
Same() needs to take into account this scenario by analyzing NewRemoved
and treating as "Same" a diff that does indeed have that key removed.
CBD used to "borrow" the last slot of of the Tainted list in the State,
but as of #1078 it uses its own list. This seems like just a block of
old code that didn't get cleaned up from the refactor. Deposed resources
get their own destroy nodes, so they don't have to mess with the main
destroy node.
The provider config was not being properly merged across module
boundaries during the Input walk over the graph, so when a provider was
configured at the top level, resources in modules could improperly
trigger a request for input for a provider attribute that's already
defined.
Instead of returning UnknownVariableValue every time, attempt to return
the real value. If we don't find it, return unknown value. This fixes
removing outputs from state on refresh.
Only used in targets for now. The plan is to use this for interpolation
as well.
This allows us to target:
* individual resources expanded by `count` using bracket / index notation.
* deposed / tainted resources with an `InstanceType` field after name
Docs to follow.
Add `-target=resource` flag to core operations, allowing users to
target specific resources in their infrastructure. When `-target` is
used, the operation will only apply to that resource and its
dependencies.
The calculated dependencies are different depending on whether we're
running a normal operation or a `terraform destroy`.
Generally, "dependencies" refers to ancestors: resources falling
_before_ the target in the graph, because their changes are required to
accurately act on the target.
For destroys, "dependencies" are descendents: those resources which fall
_after_ the target. These resources depend on our target, which is going
to be destroyed, so they should also be destroyed.
We were previously only recording the schema version on refresh. This
caused the state to be incorrectly written after a `terraform apply`
causing subsequent commands to run the state through an unnecessary
migration.
Providers get a per-resource SchemaVersion integer that they can bump
when a resource's schema changes format. Each InstanceState with an
older recorded SchemaVersion than the cureent one is yielded to a
`MigrateSchema` function to be transformed such that it can be addressed
by the current version of the resource's Schema.
Deposed instances need to be stored as a list for certain pathological
cases where destroys fail for some reason (e.g. upstream API failure,
Terraform interrupted mid-run). Terraform needs to be able to remember
all Deposed nodes so that it can clean them up properly in subsequent
runs.
Deposed instances will now never touch the Tainted list - they're fully
managed from within their own list.
Added a "multiDepose" test case that walks through a scenario to
exercise this.
