Changing only the index on a nested module will cause all nested moves
to create cycles, since their full addresses will match both the From
and To addresses. When building the dependency graph, check if the
parent is only changing the index of the containing module, and prevent
the backwards edge for the move.
Add a method for checking if the From and To addresses in a move
statement are only changing the indexes of modules relative to the
statement module.
This is needed because move statement nested within the module will be
able to match against both the From and To addresses, causing cycles in
the order of move operations.
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.
The whole point of UniqueKey is to deal with the fact that we have some
distinct address types which have an identical string representation, but
unfortunately that fact caused us to not notice that we'd incorrectly
made AbsResource.UniqueKey return a no-key instance UniqueKey instead of
its own distinct unique key type.
This new function complements the existing function FindMoveStatements
by potentially generating additional "implied" move statements that aren't
written explicit in the configuration but that we'll infer by comparing
the configuration and te previous run state.
The goal here is to infer only enough to replicate the effect of the
"count boundary fixup" graph node (terraform.NodeCountBoundary) that we
currently use to deal with this concern of preserving the zero-instance
when switching between "count" and not "count".
This is just dead code for now. A subsequent commit will introduce this
into the "terraform" package while also removing
terraform.NodeCountBoundary, thus achieving the same effect as before but
in a way that'll get reported in the UI as a move, using the same language
that we'd use for an explicit move statement.
This is similar to the existing SelectsModule method, returning true if
the reciever selects either a particular resource as a whole or any of the
instances of that resource.
We don't need this test in the normal case, but we will need it in a
subsequent commit when we'll be possibly generating _implied_ move
statements between instances of resources, but only if there aren't
explicit move statements mentioning those resources already.
CanChainFrom needs to be able to handle move statements from different
relative modules, re-implementing with addrs.anyKey
Add the anyKey InstanceKey value to the addrs package to simplify module
path comparison. This allows all combinations of module path
representation to be normalized into a ModuleInstance which can be
compared directly, rather than dealing with multiple levels of different
prefix types.
Add implementations of CanChainFrom and NestedWithin for
MoveEndpointInModule.
CanChainFrom allows the linking of move statements of the same address,
which means the prior destination address must equal the following
source address. If the destination and source addresses are of different
types, they must be covered by NestedWithin rather than CanChainFrom.
NestedWithin checks if the destination contains the source address. Any
matching types would be covered by CanChainFrom.
This is a first pass at implementing refactoring.ValidateMoves, covering
the main validation rules.
This is not yet complete. A couple situations not yet covered are
represented by commented test cases in TestValidateMoves, although that
isn't necessarily comprehensive. We'll do a further pass of filling this
out with any other subtleties before we ship this feature.
All of our MoveDestination methods have the common problem of deciding
whether the receiver is even potentially in the scope of a particular
MoveEndpointInModule, which requires that the receiver belong to an
instance of the module where the move statement was found.
Previously we had this logic inline in all three cases, but now we'll
factor it out into a shared helper function.
At first it seemed like there ought to be more factoring possible for
the AbsResource vs. AbsResourceInstance implementations, since textually
they look very similar, but in practice they only look similar because
those two types have a lot of method names in common, but the Go compiler
sees them as completely distinct and thus we must write the same logic
out twice. I did try some further refactoring to address that but it
made the resulting code significantly more complicated and, by my
judgement, harder to follow. Consequently I decided that a little
duplication was okay and warranted here because this logic is already
quite fiddly to read through and isn't likely to change significantly once
released (due to backward-compatibility promises).
Previously our MoveDestination methods only honored move statements whose
endpoints were module calls, module instances, or resources.
Now we'll additionally handle when the endpoints are individual resource
instances. This situation only applies to
AbsResourceInstance.MoveDestination because no other objects can be
contained inside of a resource instance.
This completes all of the MoveDestination cases for all supported move
statement types and moveable object types.
Previously our MoveDestination methods only honored move statements whose
endpoints were module calls or module instances.
Now we'll additionally handle when the endpoints are whole resource
addresses. This includes both renaming resource blocks and moving resource
blocks into or out of child modules.
This doesn't yet include endpoints that are specific resource _instances_,
which will follow in a subsequent commit. For the moment that situation
will always indicate a non-match.
This is a subset of the MoveDestination behavior for AbsResource and
AbsResourceInstance which deals with source and destination addresses that
refer to module calls or module instances.
They both work by delegating to ModuleInstance.MoveDestination and then
applying the same resource or resource instance address to the
newly-chosen module instance address, thus ensuring that when we move
a module we also move all of the resources inside that module in the same
way.
This doesn't yet include support for moving between specific resource or
resource instance addresses; that'll follow later. This commit should have
enough logic to support moving between module names and module instance
keys, including any module calls or resources nested within.
This method encapsulates the move-processing rules for applying move
statements to ModuleInstance addresses. It honors both module call moves
and module instance moves by trying to find a subsequence of the input
that matches the "from" endpoint and then, if found, replacing it with
the "to" endpoint while preserving the prefix and suffix around the match,
if any.
This is a whole lot of nothing right now, just stubbing out some control
flow that ultimately just leads to TODOs that cause it to do nothing at
all.
My intent here is to get this cross-cutting skeleton in place and thus
make it easier for us to collaborate on adding the meat to it, so that
it's more likely we can work on different parts separately and still get
a result that tessellates.
We previously built out addrs.UnifyMoveEndpoints with a different
implementation strategy in mind, but that design turns out to not be
viable because it forces us to move to AbsMoveable addresses too soon,
before we've done the analysis required to identify chained and nested
moves.
Instead, UnifyMoveEndpoints will return a new type MoveEndpointInModule
which conceptually represents a matching pattern which either matches or
doesn't match a particular AbsMoveable. It does this by just binding the
unified relative address from the MoveEndpoint to the module where it
was declared, and thus allows us to distinguish between the part of the
module path which applies to any instances of the given modules vs. the
user-specified part which must identify particular module instances.
Since these address types are not directly comparable themselves, we use
an unexported named type around the string representation, whereby the
special type can avoid any ambiguity between string representations of
different types and thus each type only needs to worry about possible
ambiguity of its _own_ string representation.
Many times now we've seen situations where we need to use addresses
as map keys, but not all of our address types are comparable and thus
we tend to end up using string representations as keys instead.
That's problematic because conversion to string uses type information
and some of the address types have string representations that are
ambiguous with one another.
UniqueKey therefore represents an opaque key that is unique for each
functionally-distinct address across all types that implement
UniqueKeyer.
For this initial commit I've implemented UniqueKeyer only for the
Referenceable family of types. These are an easy case because they
were all already comparable (intentionally) anyway. Later commits
can implement UniqueKeyer for other types that are not naturally
comparable, such as any which include a ModuleInstance.
This also includes a new type addrs.Set which wraps a map as a set
of addresses, using the unique keys to ensure that there can be only
one element for each distinct address.
* states: add MoveAbsResource and MoveAbsResourceInstance state functions and corresponding syncState wrapper functions.
* states: add MoveModuleInstance and MaybeMoveModuleInstance
* addrs: adding a new function, ModuleInstance.IsDeclaredByCall, which returns true if the receiver is an instance of the given AbsModuleCall.
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.
These three types represent the three different address representations we
need to represent different stages of analysis for "moved" blocks in the
configuration.
The goal here is to encapsulate all of the static address wrangling inside
these types so that users of these types elsewhere would have to work
pretty hard to use them incorrectly.
In particular, the MovableEndpoint type intentionally fully encapsulates
the weird relative addresses we use in configuration so that code
elsewhere in Terraform can never end up holding an address of a type that
suggests absolute when it's actually relative. That situation only occurs
in the internals of MoveableEndpoint where we use not-really-absolute
AbsMoveable address types to represent the not-yet-resolved relative
addresses.
This only takes care of the static address wrangling. There's lots of
other rules for what makes a "moved" block valid which will need to be
checked elsewhere because they require more context than just the content
of the address itself.
Our documentation for ModuleCall originally asserted that we didn't need
AbsModuleCall because ModuleInstance captured the same information, but
when we added count and for_each for modules we introduced
ModuleCallInstance to represent a reference to an instance of a local
module call, and now _that_ is the type whose absolute equivalent is
ModuleInstance.
We previously had no absolute representation of the call itself, without
any particular instance. That's what AbsModuleCall now represents,
allowing us to be explicit about when we're talking about the module block
vs. instances it declares, which is the same distinction represented by
AbsResource vs. AbsResourceInstance.
Just like with AbsResource and AbsResourceInstance though, there is
syntactic ambiguity between a no-key call instance and a whole module call,
and so some codepaths might accept both to start and then use other
context to dynamically choose a particular interpretation, in which case
this distinction becomes meaningful in representing the result of that
decision.
The previous name didn't fit with the naming scheme for addrs types:
The "Abs" prefix typically means that it's an addrs.ModuleInstance
combined with whatever type name appears after "Abs", but this is instead
a ModuleCallOutput combined with an InstanceKey, albeit structured the
other way around for convenience, and so the expected name for this would
be the suffix "Instance".
We don't have an "Abs" type corresponding with this one because it would
represent no additional information than AbsOutputValue.
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.
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.
We've previously had the syntax and representation of module source
addresses pretty sprawled around the codebase and intermingled with other
systems such as the module installer.
I've created a factored-out implementation here with the intention of
enabling some later refactoring to centralize the address parsing as part
of configuration decoding, and thus in turn allow the parsing result to
be seen by other parts of Terraform that interact with configuration
objects, so that they can more robustly handle differences between local
and remote modules, and can present module addresses consistently in the
UI.
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.
James Bardin