When loading nested modules, the child module diagnostics were dropped
in the recursive function. This mean that the config from the submodules
wasn't fully loaded, even though no errors were reported to the user.
This caused further problems if the plan was stored in a plan file, when
means only the partial configuration was stored for the subsequent apply
operation, which would result in unexplained "Resource node has no
configuration attached" errors later on.
Also due to the child module diagnostics being lost, any newly added
nested modules would be silently ignored until `init` was run again
manually.
Previously, adding a version constraint to a module that was previously
recorded without a version in the module manifest would cause a panic.
Instead, we now use a slight variant of the "dependencies have changed"
error that doesn't try to print out a specific version number.
Previously we were trying to access a field of the analysis object before
checking if analysis produced errors. The analysis function usually
returns a nil analysis on error, so this would result in a panic whenever
that happened.
Now we'll dereference the analysis object pointer only after checking for
errors, so we'll get a chance to report the analysis error to the user.
The expression upgrade functionality mostly ignores comments because in
the old language the syntax prevented comments from appearing in the
middle of expressions, but there was one exception: object expressions.
Because HCL 1 used ObjectType both for blocks and for object expressions,
that is the one situation where something we consider to be an expression
could have inline attached comments in the old language.
We migrate these here so we don't lose these comments that don't appear
anywhere else. Other comments get gathered up into a general comments
set maintained inside the analysis object and so will be printed out as
required _between_ expressions, just as they did before.
Previously we were treating "dynamic" blocks in configuration the same as
any other block type when merging config bodies, so that dynamic blocks
in the override would override any dynamic blocks present in the base,
without considering the dynamic block type.
It's more useful and intuitive for us to treat dynamic blocks as if they
are instances of their given block type for the purpose of overriding.
That means a foo block can be overridden by a dynamic "foo" block and
vice-versa, and dynamic blocks of different types do not interact at all
during overriding.
This requires us to recognize dynamic blocks and treat them specially
during decoding of a merged body. We leave them unexpanded here because
this package is not responsible for dynamic block expansion (that happens
in the sibling "lang" package) but we do decode them enough to recognize
their labels so we can treat them as if they were blocks of the labelled
type.
In study of existing providers we've found a pattern we werent previously
accounting for of using a nested block type to represent a group of
arguments that relate to a particular feature that is always enabled but
where it improves configuration readability to group all of its settings
together in a nested block.
The existing NestingSingle was not a good fit for this because it is
designed under the assumption that the presence or absence of the block
has some significance in enabling or disabling the relevant feature, and
so for these always-active cases we'd generate a misleading plan where
the settings for the feature appear totally absent, rather than showing
the default values that will be selected.
NestingGroup is, therefore, a slight variation of NestingSingle where
presence vs. absence of the block is not distinguishable (it's never null)
and instead its contents are treated as unset when the block is absent.
This then in turn causes any default values associated with the nested
arguments to be honored and displayed in the plan whenever the block is
not explicitly configured.
The current SDK cannot activate this mode, but that's okay because its
"legacy type system" opt-out flag allows it to force a block to be
processed in this way anyway. We're adding this now so that we can
introduce the feature in a future SDK without causing a breaking change
to the protocol, since the set of possible block nesting modes is not
extensible.
These helpers determine the value that would be used for a particular
schema construct if the configuration construct it represents is not
present (or, in the case of *Block, empty) in the configuration.
This is different than cty.NullVal on the implied type because it might
return non-null "empty" values for certain constructs if their absence
would be reported as such during a decode with no required attributes or
blocks.
The v0.12 language supports numeric constants only in decimal notation, as
a simplification. For rare situations where a different base is more
appropriate, such as unix-style file modes, we've found it better for
providers to accept a string containing a representation in the
appropriate base, since that way the interpretation can be validated and
it will be displayed in the same way in the rendered plan diff, in
outputs, etc.
We use tv.Value() here to mimick how HCL 1 itself would have interpreted
these, and then format them back out in the canonical form, which
implicitly converts any non-decimal constants to decimal on the way
through.
In order to preserve pre-v0.12 idiom for list-of-object attributes, we'll
prefer to use block syntax for them except for the special situation where
the user explicitly assigns an empty list, where attribute syntax is
required in order to allow existing provider logic to differentiate from
an implicit lack of blocks.
* configs/configupgrade: detect invalid resource names and print a TODO
message
In terraform 0.11 and prior it was possible to start a resource name
with a number. This is no longer valid, as the resource name would would
be ambiguous with number values in HCL expressions.
Fixes#19919
* Update configs/configupgrade/test-fixtures/valid/invalid-resource-name/want/resource.tf
Co-Authored-By: mildwonkey <mildwonkey@users.noreply.github.com>
This includes two upstream fixes:
- Handle explicit JSON "null" consistently during decode of JSON syntax.
- Properly detect the end of a "heredoc" when formatting to avoid messing
up indentation of other lines following the heredoc.
* configs/configupgrade: detect possible relative module sources
If a module source appears to be a relative local path but does not have
a preceding ./, print a #TODO message for the user.
* internal/initwd: limit go-getter detectors to those supported by terraform
* internal/initwd: move isMaybeRelativeLocalPath check into getWithGoGetter
To avoid making two calls to getter.Detect, which potentially makes
non-trivial API calls, the "isMaybeRelativeLocalPath" check was moved to
a later step and a custom error type was added so user-friendly
diagnostics could be displayed in the event that a possible relative local
path was detected.
configs/configload and internal/initwd both had a copyDir function that
would fail if the source directory contained a symlinked directory,
because the os.FileMode.IsDir() returns false for symlinks.
This PR adds a check for a symlink and copies that symlink in the
target directory. It handles symlinks for both files and directories
(with included tests).
Fixes#20539
Terraform 0.11 and prior had an odd special case where a resource
attribute access for "count" would be resolved as the count for the
whole resource, rather than as an attribute of an individual instance as
for all other attributes.
Because Terraform 0.12 makes test_instance.foo appear as a list when count
is set (so it can be used in other expressions), it's no longer possible
to have an attribute in that position: lists don't have attributes.
Fortunately we don't really need that special case anymore since it
doesn't do anything we can't now do with the length(...) function.
This upgrade rule, then, detects references like test_instance.foo.count
and rewrites to length(test_instance.foo). As a special case, if
test_instance.foo doesn't have "count" set then it just rewrites as the
constant 1, which mimics what would've happened in that case in Terraform
0.11.
Prior to Terraform v0.12 it was possible for a provider to secretly set
some default arguments for the "connection" block, which most commonly
included a hard-coded type of "ssh" and a value from "host".
In the interests of "explicit is better than implicit", Terraform 0.12 no
longer has this feature and instead requires connection settings to be
written explicitly in terms of the resource's exported attributes. For
compatibility though, the upgrade tool will insert expressions that are
as close as possible to the logic the provider formerly implemented, or
in a few rare cases a TF-UPGRADE-TODO comment to fix it up manually.
Some of the existing resource type implementations have incredibly
complicated implementations of selecting a single host IP address to use
and don't expose the result of that as an attribute, so for now we handle
those via a complicated Terraform language expression achieving the same
result. Ideally these providers would introduce a new attribute that
exports the same address formerly exported as the hostname before their
initial v0.12-compatible release, in which case we can simplify these to
just reference the attribute in question. That would be preferable also
because it would allow use of that exported attribute in other contexts,
such as in a null_resource provisioner somewhere else or in an output
to allow a caller to deal with the SSH part itself.
This uses the fixed "superset" schema from the main terraform package to
apply our standard expression mapping, with the exception of "type" where
interpolation sequences are not supported due to the type being evaluated
early to retrieve the schema for decoding the rest.
Aside from the two special meta-arguments "connection" and "provisioner"
this is just our standard mapping from schema to conversion rules, using
the provisioner's configuration schema.
Due to a copy-paste error, this was using the message from the providers
map in a "module" block.
This new message is not particularly helpful, but we should only see it
for a configuration that wouldn't have been valid in 0.11 either, and so
it's unlikely to be displayed.
Although sets do not have indexed elements, in Terraform 0.11 and earlier
element(...) would work with sets because we'd automatically convert them
to lists on entry to HIL -- with an arbitrary-but-consistent ordering --
and this return an arbitrary-but-consistent element from the list.
The element(...) function in Terraform 0.12 does not allow this because it
is not safe in general, but there was an existing pattern relying on this
in Terraform 0.11 configs which this upgrade rule is intended to preserve:
resource "example" "example" {
count = "${length(any_set_attribute)}"
foo = "${element(any_set_attribute, count.index}"
}
The above works because the exact indices assigned in the conversion are
irrelevant: we're just asking Terraform to create one resource for each
distinct element in the set.
This upgrade rule therefore inserts an explicit conversion to list if it
is able to successfully provide that the given expression will return a
set type:
foo = "${element(tolist(any_set_attribute), count.index}"
This makes the conversion explicit, allowing users to decide if it is
safe and rework the configuration if not. Since our static type analysis
functionality focuses mainly on resource type attributes, in practice this
rule will only apply when the given expression is a statically-checkable
resource reference. Since sets are an SDK-only concept in Terraform 0.11
and earlier anyway, in practice that works out just right: it's not
possible for sets to appear anywhere else in older versions anyway.
The comma-separated syntax is now reserved only for object constructor
expressions in attribute values, so the upgrade tool rewrites block
arguments to be newline-separated instead.
This was already working but we didn't have an explicit test for it until
now.
Prior to Terraform 0.12, ignore_changes was implemented in a
flatmap-oriented fashion and so users found that they could (and in fact,
were often forced to) use the internal .% and .# suffixes flatmap uses to
ignore changes to the number of elements in a list or map.
Terraform 0.12 no longer uses that representation, so we'll interpret
ignoring changes to the length as ignoring changes to the entire
collection. While this is not a totally-equivalent change, in practice
this pattern was most often used in conjunction with specific keys from a
map in order to _effectively_ ignore the entire map, even though Terraform
didn't really support that.
HIL implemented its type conversions by rewriting its AST to include calls
to some undocumented builtin functions. Unfortunately those functions were
still explicitly callable if you could figure out the name for them, and
so they may have been used in the wild.
In particular, __builtin_StringToFloat was used as part of a workaround
for a HIL design flaw where it would prefer to convert strings to integers
rather than floats when performing arithmetic operations. This issue was,
indeed, the main reason for unifying int ant float into a single number
type in HCL. Since we published that as a suggested workaround, the
upgrade tool ought to fix it up.
The other cases have never been documented as a workaround, so they are
less likely to appear in the wild, but we might as well fix them up anyway
since we already have the conversion functions required to get the same
result in the new language.
To be safe/conservative, most of these convert to _two_ function calls
rather than just one, which ensures that these new expressions retain the
behavior of implicitly converting to the source type before running the
conversion. The new conversion functions only specify target type, and so
cannot guarantee identical results if the argument type does not exactly
match what was previously given as the parameter type in HIL.
HEREDOC tokens are a little more fussy than normal string sequences
because we need to preserve the whitespace within them along with the
start and end markers while we upgrade any interpolated expressions inside.
We need to do some work locally here because the HCL heredoc processing
"does too much" and throws away information we need to do a faithful
upgrade.
We also need to contend with the fact that Terraform <=0.11 had an older
version of HCL that accidentally permitted a degenerate form of heredoc
where the marker was at the end of the final line, like this:
degenerate = <<EOT
this should never have workedEOT
When we migrate this, we'll introduce the additional newline that is now
required, which will unfortunately slightly change the result string to
include a newline when parsed by 0.12, and so we'll need to call this out
as a caveat in the upgrade guide.
Since these error messages get printed in Terraform's output and we
encourage users to share them as part of bug reports, we should avoid
including sensitive information in them to reduce the risk of accidental
exposure.
Previously, configupgrade would panic if it encountered a HEREDOC. For
the time being, we will simply print out the HEREDOC as-is.
Unfortunately, we discovered that terraform 0.11's version of HCL
allowed for HEREDOCs with the termination delimiter inline (instead of
on a newline, which is technically correct). Since 0.12configupgrade
needs to be bug-compatible with terraform 0.11, we must roll back to the
same version of HCL used in terraform 0.11.
Objects with DynamicPseudoType attributes can't be coerced within a map
if a concrete type is set. Change the Value type used to an Object when
there is a type mismatch.
In prior versions, we recommended using hash functions in conjunction with
the file function as an idiom for detecting changes to upstream blobs
without fetching and comparing the whole blob.
That approach relied on us being able to return raw binary data from
file(...). Since Terraform strings pass through intermediate
representations that are not binary-safe (e.g. the JSON state), there was
a risk of string corruption in prior versions which we have avoided for
0.12 by requiring that file(...) be used only with UTF-8 text files.
The specific case of returning a string and immediately passing it into
another function was not actually subject to that corruption risk, since
the HIL interpreter would just pass the string through verbatim, but this
is still now forbidden as a result of the stricter handling of file(...).
To avoid breaking these use-cases, here we introduce variants of the hash
functions a with "file" prefix that take a filename for a disk file to
hash rather than hashing the given string directly. The configuration
upgrade tool also now includes a rule to detect the documented idiom and
rewrite it into a single function call for one of these new functions.
This does cause a bit of function sprawl, but that seems preferable to
introducing more complex rules for when file(...) can and cannot read
binary files, making the behavior of these various functions easier to
understand in isolation.
There are a few constructs from 0.11 and prior that cause 0.12 parsing to
fail altogether, which previously created a chicken/egg problem because
we need to install the providers in order to run "terraform 0.12upgrade"
and thus fix the problem.
This changes "terraform init" to use the new "early configuration" loader
for module and provider installation. This is built on the more permissive
parser in the terraform-config-inspect package, and so it allows us to
read out the top-level blocks from the configuration while accepting
legacy HCL syntax.
In the long run this will let us do version compatibility detection before
attempting a "real" config load, giving us better error messages for any
future syntax additions, but in the short term the key thing is that it
allows us to install the dependencies even if the configuration isn't
fully valid.
Because backend init still requires full configuration, this introduces a
new mode of terraform init where it detects heuristically if it seems like
we need to do a configuration upgrade and does a partial init if so,
before finally directing the user to run "terraform 0.12upgrade" before
running any other commands.
The heuristic here is based on two assumptions:
- If the "early" loader finds no errors but the normal loader does, the
configuration is likely to be valid for Terraform 0.11 but not 0.12.
- If there's already a version constraint in the configuration that
excludes Terraform versions prior to v0.12 then the configuration is
probably _already_ upgraded and so it's just a normal syntax error,
even if the early loader didn't detect it.
Once the upgrade process is removed in 0.13.0 (users will be required to
go stepwise 0.11 -> 0.12 -> 0.13 to upgrade after that), some of this can
be simplified to remove that special mode, but the idea of doing the
dependency version checks against the liberal parser will remain valuable
to increase our chances of reporting version-based incompatibilities
rather than syntax errors as we add new features in future.
The parent commit fixes an issue where this would previously have led to
a crash. These new test cases verify that parsing is now able to complete
without crashing, though the result is still invalid.
In early versions of Terraform where the interpolation language didn't
have any real list support, list brackets around a single string was the
signal to split the string on a special uuid separator to produce a list
just in time for processing, giving expressions like this:
foo = ["${test_instance.foo.*.id}"]
Logically this is weird because it looks like it should produce a list
of lists of strings. When we added real list support in Terraform 0.7 we
retained support for this behavior by trimming off extra levels of list
during evaluation, and inadvertently continued relying on this notation
for correct type checking.
During the Terraform 0.10 line we fixed the type checker bugs (a few
remaining issues notwithstanding) so that it was finally possible to
use the more intuitive form:
foo = "${test_instance.foo.*.id}"
...but we continued trimming off extra levels of list for backward
compatibility.
Terraform 0.12 finally removes that compatibility shim, causing redundant
list brackets to be interpreted as a list of lists.
This upgrade rule attempts to identify situations that are relying on the
old compatibility behavior and trim off the redundant extra brackets. It's
not possible to do this fully-generally using only static analysis, but
we can gather enough information through or partial type inference
mechanism here to deal with the most common situations automatically and
produce a TF-UPGRADE-TODO comment for more complex scenarios where the
user intent isn't decidable with only static analysis.
In particular, this handles by far the most common situation of wrapping
list brackets around a splat expression like the first example above.
After this and the other upgrade rules are applied, the first example
above will become:
foo = test_instance.foo.*.id
Alex Pilon