This set of diagnostic messages is under a number of unusual constraints
that make them tough to get right:
- They are discussing a couple finicky concepts which authors are
likely to be encountering for the first time in these error messages:
the idea of "local names" for providers, the relationship between those
and provider source addresses, and additional ("aliased") provider
configurations.
- They are reporting concerns that span across a module call boundary,
and so need to take care to be clear about whether they are talking
about a problem in the caller or a problem in the callee.
- Some of them are effectively deprecation warnings for features that
might be in use by a third-party module that the user doesn't control,
in which case they have no recourse to address them aside from opening
a feature request with the upstream module maintainer.
- Terraform has, for backward-compatibility reasons, a lot of implied
default behaviors regarding providers and provider configurations,
and these errors can arise in situations where Terraform's assumptions
don't match the author's intent, and so we need to be careful to
explain what Terraform assumed in order to make the messages
understandable.
After seeing some confusion with these messages in the community, and
being somewhat confused by some of them myself, I decided to try to edit
them a bit for consistency of terminology (both between the messages and
with terminology in our docs), being explicit about caller vs. callee
by naming them in the messages, and making explicit what would otherwise
be implicit with regard to the correspondences between provider source
addresses and local names.
My assumed audience for all of these messages is the author of the caller
module, because it's the caller who is responsible for creating the
relationship between caller and callee. As much as possible I tried to
make the messages include specific actions for that author to take to
quiet the warning or fix the error, but some of the warnings are only
fixable by the callee's maintainer and so those messages are, in effect,
a suggestion to send a request to the author to stop using a deprecated
feature.
I think these new messages are also not ideal by any means, because it's
just tough to pack so much information into concise messages while being
clear and consistent, but I hope at least this will give users seeing
these messages enough context to infer what's going on, possibly with the
help of our documentation.
I intentionally didn't change which cases Terraform will return warnings
or errors -- only the message texts -- although I did highlight in a
comment in one of the tests that what it is a asserting seems a bit
suspicious to me. I don't intend to address that here; instead, I intend
that note to be something to refer to if we later see a bug report that
calls that behavior into question.
This does actually silence some _unrelated_ warnings and errors in cases
where a provider block has an invalid provider local name as its label,
because our other functions for dealing with provider addresses are
written to panic if given invalid addresses under the assumption that
earlier code will have guarded against that. Doing this allowed for the
provider configuration validation logic to safely include more information
about the configuration as helpful context, without risking tripping over
known-invalid configuration and panicking in the process.
Error messages for preconditions, postconditions, and custom variable
validations have until now been string literals. This commit changes
this to treat the field as an HCL expression, which must evaluate to a
string. Most commonly this will either be a string literal or a template
expression.
When the check rule condition is evaluated, we also evaluate the error
message. This means that the error message should always evaluate to a
string value, even if the condition passes. If it does not, this will
result in an error diagnostic.
If the condition fails, and the error message also fails to evaluate, we
fall back to a default error message. This means that the check rule
failure will still be reported, alongside diagnostics explaining why the
custom error message failed to render.
As part of this change, we also necessarily remove the heuristic about
the error message format. This guidance can be readded in future as part
of a configuration hint system.
Preconditions and postconditions for resources and data sources may not
refer to the address of the containing resource or data source. This
commit adds a parse-time validation for this rule.
This allows precondition and postcondition checks to be declared for
resources and output values as long as the preconditions_postconditions
experiment is enabled.
Terraform Core doesn't currently know anything about these features, so
as of this commit declaring them does nothing at all.
This construct of a block containing a condition and an error message will
be useful for other sorts of blocks defining expectations or contracts, so
we'll give it a more generic name in anticipation of it being used in
other situations.
Previously we ended up losing all of the error message detail produced by
the registry address parser, because we treated any registry address
failure as cause to parse the address as a go-getter-style remote address
instead.
That led to terrible feedback in the situation where the user _was_
trying to write a module address but it was invalid in some way.
Although we can't really tighten this up in the default case due to our
compatibility promises, it's never been valid to use the "version"
argument with anything other than a registry address and so as a
compromise here we'll use the presence of "version" as a heuristic for
user intent to parse the source address as a registry address, and thus
we can return a registry-address-specific error message in that case and
thus give more direct feedback about what was wrong.
This unfortunately won't help someone trying to install from the registry
_without_ a version constraint, but I didn't want to let perfect be the
enemy of the good here, particularly since we recommend using version
constraints with registry modules anyway; indeed, that's one of the main
benefits of using a registry rather than a remote source directly.
Object values returned from providers have their attributes marked as
sensitive based on the provider schema. This was not fully implemented
for nested attribute types, which is corrected in this commit.
Based on feedback during earlier alpha releases, we've decided to move
forward with the current design for the first phase of config-driven
refactoring.
Therefore here we've marked the experiment as concluded with no changes
to the most recent incarnation of the functionality. The other changes
here are all just updating test fixtures to no longer declare that they
are using experimental features.
The current behavior of module input variables is to allow users to
override a default by assigning `null`, which works contrary to the
behavior of resource attributes, and prevents explicitly accepting a
default when the input must be defined in the configuration.
Add a new variable attribute called `nullable` will allow explicitly
defining when a variable can be set to null or not. The current default
behavior is that of `nullable=true`.
Setting `nullable=false` in a variable block indicates that the variable
value can never be null. This either requires a non-null input value, or
a non-null default value. In the case of the latter, we also opt-in to
the new behavior of a `null` input value taking the default rather than
overriding it.
In a future language edition where we make `nullable=false` the default,
setting `nullable=true` will allow the legacy behavior of `null`
overriding a default value. The only future configuration in which this
would be required even if the legacy behavior were not desired is when
setting an optional+nullable value. In that case `default=null` would
also be needed and we could therefor imply `nullable=true` without
requiring it in the configuration.
A more native integration for Terraform Cloud and its CLI-driven run workflow.
Instead of a backend, users declare a special block in the top-level terraform settings
block to configure Terraform Cloud, then run terraform init.
Full documentation will follow in later commits.
These changes allow cloud blocks to be overridden by backend blocks and
vice versa; the logic follows the current backend behavior of a block
overriding a preceding block in full, with no merges.
This restriction is temporary. Overrides should be allowed, but have the
added complexity of needing to also override a 'backend' block, so this
work is being deferred for now.
This is a replacement declaration for using Terraform Cloud as a remote
backend, leaving the literal backend as an implementation detail and not
a user-level concept.
A snapshotDir tracks its current position as part of its state, so we need
to use it via pointer rather than value so that Readdirnames can actually
update that position, or else we'll just get stuck at position zero.
In practice this wasn't hurting anything because we only call Readdir once
on our snapshots, to read the whole directory at once. Still nice to fix
to avoid a gotcha for future maintenence, though.
In historical versions of Terraform the responsibility to check this was
inside the terraform.NewContext function, along with various other
assorted concerns that made that function particularly complicated.
More recently, we reduced the responsibility of the "terraform" package
only to instantiating particular named plugins, assuming that its caller
is responsible for selecting appropriate versions of any providers that
_are_ external. However, until this commit we were just assuming that
"terraform init" had correctly selected appropriate plugins and recorded
them in the lock file, and so nothing was dealing with the problem of
ensuring that there haven't been any changes to the lock file or config
since the most recent "terraform init" which would cause us to need to
re-evaluate those decisions.
Part of the game here is to slightly extend the role of the dependency
locks object to also carry information about a subset of provider
addresses whose lock entries we're intentionally disregarding as part of
the various little edge-case features we have for overridding providers:
dev_overrides, "unmanaged providers", and the testing overrides in our
own unit tests. This is an in-memory-only annotation, never included in
the serialized plan files on disk.
I had originally intended to create a new package to encapsulate all of
this plugin-selection logic, including both the version constraint
checking here and also the handling of the provider factory functions, but
as an interim step I've just made version constraint consistency checks
the responsibility of the backend/local package, which means that we'll
always catch problems as part of preparing for local operations, while
not imposing these additional checks on commands that _don't_ run local
operations, such as "terraform apply" when in remote operations mode.
The CoerceValue code was not updated to handle NestedTypes, and while
none of the new codepaths make use of this method, there are still some
internal uses.
In order to handle optional attributes, the Variable type needs to keep
track of the type constraint for decoding and conversion, as well as the
concrete type for creating values and type comparison.
Since the Type field is referenced throughout the codebase, and for
future refactoring if the handling of optional attributes changes
significantly, the constraint is now loaded into an entirely new field
called ConstraintType. This prevents types containing
ObjectWithOptionalAttrs from escaping the decode/conversion codepaths
into the rest of the codebase.
Objects with optional attributes are only used for the decoding of
HCL, and those types should never be exposed elsewhere within
terraform.
Separate the external ImpliedType method from the cty.Type generated
internally for the decoder spec. This unfortunately causes our
ImpliedType method to return a different type than the
hcldec.ImpliedType function, but the former is only used within
terraform for concrete values, while the latter is used to decode
HCL. Renaming the ImpliedType methods could be done to further
differentiate them, but that does cause fairly large diff in the
codebase that does not seem worth the effort at this time.
This package level variable can be overridden at link time to allow
temporarily disabling the UI warning when experimental features are
enabled. This makes it easier to understand how UI will render when the
feature is no longer experimental.
This change is only for those developing Terraform.
This includes the addition of the new "//go:build" comment form in addition
to the legacy "// +build" notation, as produced by gofmt to ensure
consistent behavior between Go versions. The new directives are all
equivalent to what was present before, so there's no change in behavior.
Go 1.17 continues to use the Unicode 13 tables as in Go 1.16, so this
upgrade does not require also upgrading our Unicode-related dependencies.
This upgrade includes the following breaking changes which will also
appear as breaking changes for Terraform users, but that are consistent
with the Terraform v1.0 compatibility promises.
- On MacOS, Terraform now requires macOS 10.13 High Sierra or later.
This upgrade also includes the following breaking changes which will
appear as breaking changes for Terraform users that are inconsistent with
our compatibility promises, but have justified exceptions as follows:
- cidrsubnet, cidrhost, and cidrnetmask will now reject IPv4 CIDR
addresses whose decimal components have leading zeros, where previously
they would just silently ignore those leading zeros.
This is a security-motivated exception to our compatibility promises,
because some external systems interpret zero-prefixed octets as octal
numbers rather than decimal, and thus the previous lenient parsing could
lead to a different interpretation of the address between systems, and
thus potentially allow bypassing policy when configuring firewall rules
etc.
This upgrade also includes the following breaking changes which could
_potentially_ appear as breaking changes for Terraform users, but that do
not in practice for the reasons given:
- The Go net/url package no longer allows query strings with pairs
separated by semicolons instead of ampersands. This primarily affects
HTTP servers written in Go, and Terraform includes a special temporary
HTTP server as part of its implementation of OAuth for "terraform login",
but that server only needs to accept URLs created by Terraform itself
and Terraform does not generate any URLs that would be rejected.
* configs/configschema: extend block.AttributeByPath to descend into Objects
This commit adds a recursive Object.AttributeByPath function which will step through Attributes with NestedTypes. If an Attribute without a NestedType is encountered while there is still more to the path, it will return nil.
* configs/configschema: fix missing "computed" attributes from NestedObject's ImpliedType
listOptionalAttrsFromObject was not including "computed" attributes in the list of optional object attributes. This is now fixed. I've also added some tests and fixed some panics and otherwise bad behavior when bad input is given. One natable change is in ImpliedType, which was panicking on an invalid nesting mode. The comment expressly states that it will return a result even when the schema is inconsistent, so I removed the panic and instead return an empty object.
An earlier commit added logic to decode "moved" blocks and do static
validation of them. Here we now include that result also in modules
produced from those files, which we can then use in Terraform Core to
actually implement the moves.
This also places the feature behind an active experiment keyword called
config_driven_move. For now activating this doesn't actually achieve
anything except let you include moved blocks that Terraform will summarily
ignore, but we'll expand the scope of this in later commits to eventually
reach the point where it's really usable.
A common source of churn when we're running experiments is that a module
that would otherwise be valid ends up generating a warning merely because
the experiment is active. That means we end up needing to shuffle the
test files around if the feature ultimately graduates to stable.
To reduce that churn in simple cases, we'll make an exception to disregard
the "Experiment is active" warning for any experiment that a module has
intentionally opted into, because those warnings are always expected and
not a cause for concern.
It's still possible to test those warnings explicitly using the
testdata/warning-files directory, if needed.
Although addrs.Target can in principle capture the information we need to
represent move endpoints, it's semantically confusing because
addrs.Targetable uses addrs.Abs... types which are typically for absolute
addresses, but we were using them for relative addresses here.
We now have specialized address types for representing moves and probably
other things which have similar requirements later on. These types
largely communicate the same information in the end, but aim to do so in
a way that's explicit about which addresses are relative and which are
absolute, to make it less likely that we'd inadvertently misuse these
addresses.
This PR adds decoding for the upcoming "moved" blocks in configuration. This code is gated behind an experiment called EverythingIsAPlan, but the experiment is not registered as an active experiment, so it will never run (there is a test in place which will fail if the experiment is ever registered).
This also adds a new function to the Targetable interface, AddrType, to simplifying comparing two addrs.Targetable.
There is some validation missing still: this does not (yet) descend into resources to see if the actual resource types are the same (I've put this off in part because we will eventually need the provider schema to verify aliased resources, so I suspect this validation will have to happen later on).
Previously we had a separation between ModuleSourceRemote and
ModulePackage as a way to represent within the type system that there's an
important difference between a module source address and a package address,
because module packages often contain multiple modules and so a
ModuleSourceRemote combines a ModulePackage with a subdirectory to
represent one specific module.
This commit applies that same strategy to ModuleSourceRegistry, creating
a new type ModuleRegistryPackage to represent the different sort of
package that we use for registry modules. Again, the main goal here is
to try to reflect the conceptual modelling more directly in the type
system so that we can more easily verify that uses of these different
address types are correct.
To make use of that, I've also lightly reworked initwd's module installer
to use addrs.ModuleRegistryPackage directly, instead of a string
representation thereof. This was in response to some earlier commits where
I found myself accidentally mixing up package addresses and source
addresses in the installRegistryModule method; with this new organization
those bugs would've been caught at compile time, rather than only at
unit and integration testing time.
While in the area anyway, I also took this opportunity to fix some
historical confusing names of fields in initwd.ModuleInstaller, to be
clearer that they are only for registry packages and not for all module
source address types.
Now that we (in the previous commit) refactored how we deal with module
sources to do the parsing at config loading time rather than at module
installation time, we can expose a method to centralize the determination
for whether a particular module call (and its resulting Config object)
enters a new external package.
We don't use this for anything yet, but in later commits we will use this
for some cross-module features that are available only for modules
belonging to the same package, because we assume that modules grouped
together in a package can change together and thus it's okay to permit a
little more coupling of internal details in that case, which would not
be appropriate between modules that are versioned separately.
It's been a long while since we gave close attention to the codepaths for
module source address parsing and external module package installation.
Due to their age, these codepaths often diverged from our modern practices
such as representing address types in the addrs package, and encapsulating
package installation details only in a particular location.
In particular, this refactor makes source address parsing a separate step
from module installation, which therefore makes the result of that parsing
available to other Terraform subsystems which work with the configuration
representation objects.
This also presented the opportunity to better encapsulate our use of
go-getter into a new package "getmodules" (echoing "getproviders"), which
is intended to be the only part of Terraform that directly interacts with
go-getter.
This is largely just a refactor of the existing functionality into a new
code organization, but there is one notable change in behavior here: the
source address parsing now happens during configuration loading rather
than module installation, which may cause errors about invalid addresses
to be returned in different situations than before. That counts as
backward compatible because we only promise to remain compatible with
configurations that are _valid_, which means that they can be initialized,
planned, and applied without any errors. This doesn't introduce any new
error cases, and instead just makes a pre-existing error case be detected
earlier.
Our module registry client is still using its own special module address
type from registry/regsrc for now, with a small shim from the new
addrs.ModuleSourceRegistry type. Hopefully in a later commit we'll also
rework the registry client to work with the new address type, but this
commit is already big enough as it is.
As the comment notes, this hostname is the default for provide source
addresses. We'll shortly be adding some address types to represent module
source addresses too, and so we'll also have DefaultModuleRegistryHost
for that situation.
(They'll actually both contain the the same hostname, but that's a
coincidence rather than a requirement.)
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.
Martin Atkins