There's no reason for a negative version, so by blocking it now we'll
ensure that none creep in.
The more practical short-term motivation for this is that we're still
using uint64 for these internally in some cases and so this restriction
ensures that we won't run into rough edges when converting from int64 to
uint64 at those boundaries until we later fix everything to use int64
consistently.
Previously we were making an invalid assumption in evaluating module call
references (like module.foo) that the module must exist, which is
incorrect for that particular case because it's a reference to a child
module, not to an object within the current module.
However, now that we have the mechanism for static validation of
references, we'll deal with this one there so it can be caught sooner.
That then makes the original assumption valid, though for a different
reason.
This is verified by two new context tests for validation:
- TestContext2Validate_invalidModuleRef
- TestContext2Validate_invalidModuleOutputRef
Previously we were fetching these from the provider but then immediately
discarding the version numbers because the schema API had nowhere to put
them.
To avoid a late-breaking change to the internal structure of
terraform.ProviderSchema (which is constructed directly all over the
tests) we're retaining the resource type schemas in a new map alongside
the existing one with the same keys, rather than just switching to
using the providers.Schema struct directly there.
The methods that return resource type schemas now return two arguments,
intentionally creating a little API friction here so each new caller can
be reminded to think about whether they need to do something with the
schema version, though it can be ignored by many callers.
Since this was a breaking change to the Schemas API anyway, this also
fixes another API wart where there was a separate method for fetching
managed vs. data resource types and thus every caller ended up having a
switch statement on "mode". Now we just accept mode as an argument and
do the switch statement within the single SchemaForResourceType method.
In the initial move to HCL2 we started relying only on full expression
evaluation to catch attribute errors, but that's not sufficient for
resource attributes in practice because during validation we can't know
yet whether a resource reference evaluates to a single object or to a
list of objects (if count is set).
To address this, here we reinstate some static validation of resource
references by analyzing directly the reference objects, disregarding any
instance index if present, and produce errors if the remaining subsequent
traversal steps do not correspond to items within the resource type
schema.
This also allows us to produce some more specialized error messages for
certain situations. In particular, we can recognize a reference like
aws_instance.foo.count, which in 0.11 and prior was a weird special case
for determining the count value of a resource block, and offer a helpful
error showing the new length(aws_instance.foo) usage pattern.
This eventually delegates to the static traversal validation logic that
was added to the configschema package in a previous commit, which also
includes some specialized error messages that distinguish between
attributes and block types in the schema so that the errors relate more
directly to constructs the user can see in the configuration.
In future we could potentially move more of the checks from the dynamic
schema construction step to the static validation step, but resources
are the reference type that most needs this immediately due to the
ambiguity caused by the instance indexing syntax. We can safely refactor
other reference types to be statically validated in later releases.
This is verified by two pre-existing context validate tests which we
temporarily disabled during earlier work (now re-enabled) and also by a
new validate test aimed specifically at the special case for the "count"
attribute.
These overly-general interfaces are no longer used anywhere, and their
presence in the important-sounding semantics.go file was a distracting
red herring.
We'd previously replaced the one checker in here with a simple helper
function for checking input variables, and that's arguably more at home
with all of the other InputValue functionality in variables.go, and that
allows us to remove semantics.go (and its associated test file) altogether
and make room for some forthcoming new files for static validation.
It's possible that a computed collection could be handled by the
attribute name, rather than the index count value.
Use a new testDiffFn for some tests, which don't work with the old
function that can't determine `computed` without the schema.
Comments here indicate that this was erroneously returning an error but
we accepted it anyway to get the tests passing again after other work.
The tests over in the "terraform" package agree that cancelling should be
a successful outcome rather than an error.
I think that cancelling _should_ actually be an error, since Terraform did
not complete the operation it set out to complete, but that's a change
we'd need to make cautiously since automation wrapper scripts may be
depending on the success-on-cancel behavior.
Therefore this just fixes the command package test to agree with the
Terraform package tests and adds some FIXME notes to capture the potential
that we might want to update this later.
Since the state models can't preserve unknown values, we need to rely on the plan to persist these until the effective configuration can be fully resolved during the apply phase.
If plan and apply are both run against the same context then we still have
the planned output values in memory while we're doing the apply walk, so
we need to make sure to update them along with the state as we learn the
final known values of each output.
There were actually two different bugs here:
- We weren't removing any existing planned change for an output when
setting a new one. In retrospect a map would've been a better data
structure for the output changes, rather than a slice to mimic what we
do for resource instance objects, but for now we'll leave the structures
alone and clean up as needed. (The set of outputs should be small for
any reasonable configuration, so the main impact of this is some ugly
code in RemoveOutputChange.)
- RemoveOutputChange itself had a bug where it was iterating over the
resource changes rather than the output changes. This didn't matter
before because we weren't actually using that function, but now we are.
This fix is confirmed by restoring various existing context apply tests
back to passing again.
This new source type should be used for variables loaded from .tfvars files that were explicitly passed as command line arguments (e.g. -var-file=foo.tfvars)
Just as when we resolve single output values we must check to see if there
is a planned new value for an output before using the value in state,
because the planned new value might contain unknowns that can't be
represented directly in the state (and would thus be incorrectly returned
as null).
Resource timeouts were a separate config block, but did not exist in the
resource schema. Insert any defined timeouts when generating the
configshema.Block so that the fields can be accepted and validated by
core.
This ensures more HCL1/HIL-like behaviors when dealing with nested blocks
that are not set in the configuration, which is important for
compatibility with helper/schema's validation logic.
There are several steps here and a number of them can include reaching out
to remote servers or executing local processes, so it's helpful to have
some trace logs to better narrow down causes of errors and hangs during
this step.
The legacy test wasn't really testing anything useful, just the plan
behavior with no diff in a module returned a module with no diff.
There's no reason to convert this to the new plans, since there's no
legacy behavior to match.
In the reorganization of the data source read code we missed the fact that
the plan phase must _always_ generate a read data diff, never directly
read, because the state generated during plan is throwaway.
This only matters in the -refresh=false case, since normally refresh has
already taken care of this, but that is still an important case, covered
by the TestContext2Apply_dataBasic test.
When we're being asked to destroy everything, we ideally want to end up
with a totally empty state. Normally we will conservatively keep around
the "husks" of resources (what's left after all of the instances have been
destroyed) unless they are configured without count or for_each, but in
this special case we'll prune those out.
The implication of this is that in "weird" expression contexts that happen
before the next "terraform plan", such as evaluation in
"terraform console" or expressions in data resources and provider blocks
that get evaluated during the refresh walk, we will see these results
as unknown rather than as empty lists of objects. We accept that weirdness
for now because in a future release we are likely to remove "refresh" as
a separate walk anyway, doing all of that work during the plan walk where
we can ensure that these values are properly re-populated before trying
to use them.
Some mock objects will not have any mock behavior configured for the
GetSchema method, so we should just return a valid-but-empty schema in
that case, rather than panicking as we did before.
These are similar to the symbols of the same name in package "providers".
terraform.ProvisionerFactory is now an alias for provisioners.Factory, so
we can defer updating all of the existing users of it.
There are still some errors left, because our expression evaluator now
does more validation than before and so we'll need to (in a subsequent
commit) actually use a fixture configuration for these tests so that the
validations will allow the expressions to be validated.
This test was occasionally failing due to a missing graph edge causing it
to be non-deterministic.
The graph edge was missing because our standard schema doesn't quite match
the config fixture and so the reference checker was not finding the "blah"
argument on aws_instance.a.
This change also includes an 100ms pause for the b node just to make this
potential race more likely to hit the "wrong" ordering when the graph is
not complete.
The weird special-case behaviors of testDiffFn were interfering with the
outcome of this test, but we don't actually need any of those special
behaviors here so we'll use a very simple PlanResourceChangeFn
implementation instead, just letting the built-in merge behavior in core
take care of it.
Since we started using experimental Go Modules our editor tooling hasn't
been fully functional, apparently including format-on-save support. This
is a catchup to get everything back straight again.
One of the assumptions this test was checking no longer holds: we don't
retain outputs for non-root modules in persistent state, because we can
always re-populate these on a future run by evaluating the configuration.
The old-fashioned formatting of "Provider" on the shimmed state here was
causing the state upgrade code to treat it like a module-relative address,
rather than an absolute address as we now expect.
This error message appears in a situation that is often confusing for
users, since the connection between resources and their providers in the
state is not something we draw attention to in the user experience of
Terraform.
This new error message tries to be a bit clearer about what the user must
do to resolve it. It's still not perfect since it doesn't cover the
variant of this problem where an entire module containing a provider block
and resources has been removed at the same time, but since there isn't
an easily-summarizable way to continue in that state this will need to
do for the moment, until we find a way to file off that rough edge in
the workflow.
Configuration blocks can contain sensitive information, so better to just
talk about them by reference (in this case, source location) rather than
embedding them directly, to reduce the risk of accidental information
leakage through sharing logs for debug purposes.
This was already updated for the new state types earlier, but since then
we adjusted how deposed instances are written out in the old string
representation of state, and so this regressed.
These tests show that we're still not fully pruning modules from the state
in all cases, due to us not being able to fully prune out modules that
contain resources with count set after a destroy, but this is no worse
than before so we'll accept it for now and address this separately later.
A module heading without "<no state>" but also without any instances
listed is the rendering for a module containing a resource that has no
instances, since our old string rendering of state doesn't represent
resources themselves.
We previously had mechanisms to clean up only individual instance states,
leaving behind empty resource husks in the state after they were all
destroyed.
This takes care of it in the "orphan" case. It does not yet do it in the
"terraform destroy" or "terraform plan -destroy" cases because we don't
have anywhere to record in the plan that we're actually destroying and so
the resource configurations should be ignored and _everything_ should be
cleaned. We'll let the state be not-quite-empty in that case for now,
since it doesn't really hurt; cleaning up orphans is the main case because
the state will live on afterwards and so leftover cruft will accumulate
over the course of many changes.
Since this one has a situation where there are two deposed objects for
the same instance at once, we can't rely on comparing state strings: they
are not deterministic when multiple deposed objects are present.
Instead, we do more surgical comparisons directly on the state model
objects, which is not quite as robust but still gets us the main stuff we
care about here, to be followed up by another checkStateString further
down for the final state.
Tainted objects now also remember which provider they belong to (via the
resource state they are attached to) and so the stringified state output
here is slightly different.
This was relying on a no-longer-valid mechanism for accessing the "count"
value from a resource block. The original issue this test was written to
cover is not really such a sharp edge anymore, since the length is taken
from the state during apply rather than from configuration, but it's still
a good case to cover.
This test was incorrectly amended on the first pass to create a
configuration snapshot from the step zero configuration, rather than the
step one configuration that the save plan is built from.
Along with that, it needed various other minor updates to match with
details that have shifted:
- "id" and "type" attributes must be explicitly declared in schema
- template_file.parent has count = 1, which now causes it to get an index
and be a list where before it did not.
If we don't set it, we end up creating an invalid plan where the destroy
changes don't have a provider address set, which then later fails
decoding when round-tripped through a planfile.
This also includes some extra safety checks in EvalDiff and
EvalDiffDestroy so that we can catch this bug sooner in future.
This change is verified by
TestContext2Apply_plannedDestroyInterpolatedCount, which is now passing.
A plan file without a backend set is not valid, but at the level we're
testing in this package we don't really care about backends so we'll just
set a default one if the caller doesn't set something more specific, and
then we'll just ignore it completely when reading back.
It doesn't make sense to ignore_changes when the prior value is null,
since we have to create something before we can ignore changes to it.
This change is verified by TestContext2Apply_ignoreChangesWildcard.
This test was relying on the feature of the old provisioner API that gave
provisioners full access to the instance state of what they were
provisioning.
We no longer do this, and so instead the ApplyFn must distingish the
instances using a value from the provisioner's own configuration.
This can happen if unknown values in the plan actually end up being
identical to the prior values once resolved. In that case, we'll just make
no change at all.
This is verified by TestContext2Apply_ignoreChangesWithDep.
Due to a quirk in how testDiffFn constructs its diff, compute doesn't
actually get included in the final result anymore under the new shims.
This result is still correct, nonetheless.
The prior behavior being asserted by this test was incorrect, since the
configuration calls for there to be two instances of the resource at the
end.
We also now assert on the generated plan since it's important to verify
that we are indeed planning to replace the zeroth instance but not the
first instance (which doesn't yet exist).
We now include attribute changes in destroy diffs, so the expected output
of this test includes these changes.
Also includes a fix to legacyDiffComparisonString to actually sort the
attribute changes by name in the rendered diff.
The testDiffFn doesn't include "compute" in the diff it produces and so
it no longer appears in the shimmed output.
This is just a quirk of this weird mock implementation; real providers
always copy all of the values from thec config into the diff before adding
in any other changes.
The only reasonable usage of these methods is for them to run concurrently
with other methods, so we mustn't hold a lock to do this work. For tests
that deal with stopping, it's the test's own responsibility to deal with
any concurrency issues that arise from their StopFns running concurrently
with other mock functions.
Since stopping is a rather complex mechanism that relies on correct
handling of concurrency, it's handy to have these logs here to debug when
things don't happen in quite the right order.
Comment out an out of date CBD test. The test no longer works due to
the CBD status being checked during plan, but the test case may still
have some value which we can review later.
update the text in the CBD transformer to reflect the
s/ancestor/descendent/ change.
There does not appear to be any real reason that these Schemas fields are
not exported, and exporting them makes it possible to directly construct
Schemas for tests without pulling in an entire context.
Now that we populate a resource-level state pre-emptively for a resource,
before we apply the instances of that resource, it is no longer true that
this produces only one resource state, but the test below (comparing the
string version of the state) already captures the expected behavior and
so this additional check was redundant anyway.
This test was incorrectly updated to the new hook API; it should've been
recording the individual resource state values passed to the hook, not
the original state as a whole (which is not yet updated at the point
when the hook is called).
Reduce the graphs, as they are too ungainly to compare without
reduction. We also depend on reduction in all use cases, so we should be
testing the graph as we use it anyway.
We can't catch invalid attributes in validate at the moment, because the
lack of count information causes the references to return unknown. Make
sure they fail in plan, and mark the validate tests to fix later.
Previously we used a single plan action "Replace" to represent both the
destroy-before-create and the create-before-destroy variants of replacing.
However, this forces the apply graph builder to jump through a lot of
hoops to figure out which nodes need it forced on and rebuild parts of
the graph to represent that.
If we instead decide between these two cases at plan time, the actual
determination of it is more straightforward because each resource is
represented by only one node in the plan graph, and then we can ensure
we put the right nodes in the graph during DiffTransformer and thus avoid
the logic for dealing with deposed instances being spread across various
different transformers and node types.
As a nice side-effect, this also allows us to show the difference between
destroy-then-create and create-then-destroy in the rendered diff in the
CLI, although this change doesn't fully implement that yet.
Remove a test that is no longer needed, since provider must be
explicitly defined for orphaned modules, and is covered in other context
tests.
Udpate a test fixture to better represent the origianl missing map
issue, since the ability to detect nil now made the old test invalid.
This test was relying on an odd definition of "invalid" from prior
versions of Terraform, where it would be an error to access an attribute
that exists as part of the resource type schema but the provider
implementation neglected to set it.
This was an implementation detail though, caused by the flatmap
representation and the fact that core itself didn't have access to the
schema to do static validation. Now the original usage returns a null
value because the "value" attribute is defined, and so we need a new
test fixture that accesses an attribute that is not defined in the schema
at all.
I misunderstood the logic here on the first pass of porting to the new
provider and state types: EvalUndeposeState is supposed to return the
deposed object back to being current again, so we can undo the deposing
in the case where the create leg fails.
If we don't do this, we end up leaving the instance with no current object
at all and with its prior object deposed, and then the later destroy
node deletes that deposed object, leaving the user with no object at all.
For safety we skip this restoration if there _is_ a new current object,
since a failed create can still produce a partial result which we need
to keep to avoid losing track of any remote objects that were successfully
created.
We now correctly prune out empty modules after destroying everything
inside them, so we need to update this expectation string to match the new
behavior, rather than before when it was actually describing a buggy
result.
This was assuming a previous buggy behavior of failing to prune out an
empty module in the state. The new state code doesn't have this bug, so
we must update the expected result to reflect that.
This regressed after the recent changes to include deposed object changes
explicitly in the plan, since it was previously (correctly) asserting that
only the current object was covered by the plan.
Now we assert that both are present, and check that they have the correct
actions associated so that we are sure we're going to only delete the
deposed object and not the current object that sits alongside it.
This shim for the benefit of our old tests was not handling the situation
where an InstanceState only contains deposed instances and no current
instance. This is a strange, rare situation but one that does come up in
practice in some odd cases.
Previously our handling of create_before_destroy -- and of deposed objects
in particular -- was rather "implicit" and spread over various different
subsystems. We'd quietly just destroy every deposed object during a
destroy operation, without any user-visible plan to do so.
Here we make things more explicit by tracking each deposed object
individually by its pseudorandomly-allocated key. There are two different
mechanisms at play here, building on the same concepts:
- During a replace operation with create_before_destroy, we *pre-allocate*
a DeposedKey to use for the prior object in the "apply" node and then
pass that exact id to the destroy node, ensuring that we only destroy
the single object we planned to destroy. In the happy path here the
user never actually sees the allocated deposed key because we use it and
then immediately destroy it within the same operation. However, that
destroy may fail, which brings us to the second mechanism:
- If any deposed objects are already present in state during _plan_, we
insert a destroy change for them into the plan so that it's explicit to
the user that we are going to destroy these additional objects, and then
create an individual graph node for each one in DiffTransformer.
The main motivation here is to be more careful in how we handle these
destroys so that from a user's standpoint we never destroy something
without the user knowing about it ahead of time.
However, this new organization also hopefully makes the code itself a
little easier to follow because the connection between the create and
destroy steps of a Replace is reprseented in a single place (in
DiffTransformer) and deposed instances each have their own explicit graph
node rather than being secretly handled as part of the main instance-level
graph node.
This test was re-using the same context to run three consecutive
plan/apply operations, which is not safe because we will accumulate more
planned changes with each change, creating duplicate entries in the diff.
Instead, to properly simulate a sequence of consecutive runs of Terraform
we must start with a fresh context each time, though still pass forward
the previous state which would in the real world be persisted via a state
manager between these runs.
This inverts the previous logic so that it's the status of an object in
the state that decides whether we'll use its value from the plan. This
fixes the problem that otherwise after we've actually applied the change
the partial planned object will continue to shadow the final object in
state.
These two being distinct is an old-world concept, but we need to ensure
that they match properly here to ensure that we don't leave dangling
incorrect values for "id".
There were two problems with this test as originally written:
- Its ApplyFn was handling a destroy diff by returning a new object, and
thus not actually destroying the object in question at all.
- It was treating unknown values in the diff as invalid during apply, but
these are in fact now expected as a way for the provider to distinguish
whether an optional+computed attribute is set in config.
With those changes in mind, this test isn't really testing anything
special anymore, but is still a straightforward test of a simple
plan+apply running to completion without error.
This test's original outcome was invalid because it didn't actually
configure a mock apply function, and so _any_ apply operation would appear
to have destroyed the object it was given. This was visible in the fact
that the configuration contains aws_instance.bar but yet it was not
present in the expected state string.
Now we use the standard testApplyFn, and update the expected output to
include the aws_instance.bar object that is created by a Create change.
When applying a diff to a value we verify that the "old" value in diff is
consistent with the given prior value, as a safety check.
The mock must comply with this or else any tests that produce diffs with
computed new values will not pass the safety check.
This change is verified by the now-passing TestContext2Apply_taintDep .
These tests were relying on the full InstanceInfo we used to give to
providers, but the new API doesn't do that and so we will instead lean on
the ID from the state to recognize the apply ordering.
Previously testDiffFn was just assuming that the prior value for "type"
was always the empty string, but that doesn't hold if a mocked object is
updated in-place with a previously-populated value for type.
This wasn't a problem before because the old values in the diff were
largely just for presentation to the user, but we do now verify that the
old values match what we're applying to as an extra safety check and so
we must populate the old value properly.
This fix is verified by TestContext2Apply_Provisioner_Diff.
Now that we're marking errored creates as tainted in the state, the
object created by this test will get marked as tainted due to the error
about it containing an unknown value even after apply.
Since this test sets its own special schema for aws_instance, its expected
output must now be adjusted to only expect values that conform to that
schema. The extraneous attributes like "type" which testDiffFn produces
are no longer visible unless declared in schema.
We also need to now declare "id" as a computed attribute in order for our
state stringer shim to properly populate the formerly-special "ID".
We can't use diff attributes to differentiate instances during destroy
because destroy diffs don't have any attributes set in the old API.
Also, our new state management code correctly prunes out the empty
module.child before returning, so our expected output is updated to not
expect that to still be present.
When we re-run EvalDiff during apply, we may have already completed the
destroy leg of a replace operation, leaving us in a different situation
than we were when we made the original planned change.
Therefore as a special case we will allow a create to turn back into a
replace if there was an earlier diff that requested that.
If the prior object is tainted, we behave as if it doesn't exist at all
for most of our logic here but then at the end turn it into a synthetic
replace operation going from the old object to the new object, similarly
to how we'd behave if given an argument change that "requires
replacement".
We can be more relaxed about our rules that a create musn't return null
or a destroy must return null if the provider also itself indicated an
error. In that case, it's expected that the return value is describing a
partial result, and so we'll just store it and move on.
The mechanism for a provider to pre-populate parts of the connection
config for subsequent provisioners is no longer present in the new
protocol, since it was rarely used, poorly documented, and for many
resource types had no obvious good defaults.
Although we have a special case where a result of the wrong type will bail
early, we must keep that set of diagnostics separate so that we can still
run to completion when there are _already_ diagnostics present (from the
provider's response) but the return value _is_ type-conforming.
This fix is verified by TestContext2Apply_provisionerCreateFail.
Our previous mechanism for dealing with tainting relied on directly
mutating the InstanceState object to mark it as such. In our new state
models we consider the instance objects to be immutable by convention, and
so we frequently copy them. As a result, the taint flagging was no longer
making it all the way through the apply evaluation process.
Here we now implement tainting as a separate step in the evaluation
process, creating a copy of the object with a tainted status if there were
any errors during creation.
This introduces a new behavior where any provider-level errors during
creation will also cause an instance to be marked as tainted if any object
is returned at all. Create-time errors _normally_ result in no object at
all, but the provider might return an object if the failure occurred at
a subsequent step of a multi-step creation process and so left behind a
remote object that needs to be cleaned up on a future run.
Since StateReferences was implemented on NodeAbstractResource rather than
NodeAbstractResourceInstance it wasn't properly detecting references to
the same instance as self-references.
Now that we are using "seen" to filter out duplicates we can also simplify
how we handle these self-references by just pretending we saw them before
we even start the loop.
This change is confirmed by
TestContext2Apply_provisionerMultiSelfRefSingle
The error handling here is a bit tricky due to the ability for users to
opt out of aborting on error. It's important that we keep straight the
distinction between applyDiags and diags so we can tell the difference
between the errors from _this_ provisioner and the errors for the entire
run so far.
The expected output string for this test is assuming a couple computed
attributes that were not declared in schema. This didn't matter before
because the provider output was not previously subject to schema-based
interpretation, but now our shims to the old provider API rely on the
schema to convert the returned data and so any unexpected attributes are
filtered.
The diffs created by testDiffFn use the flatmap package directly, rather
than running the diff-generation logic in helper/schema. It turns out that
flatmap itself can generate the k+".#" keys used to indicate the length
of a list, but only helper/schema itself knew how to generate the
corresponding k+".%" keys used for maps.
Rather than modifying the now-deprecated flatmap code directly (and risk
breaking assumptions in shims elsewhere), here we just synthesize the
extra required map element within the testDiffFn implementation, which
then in turn allows the MockProvider diffFn shim to correctly recognize
it as a map and convert it into a real cty.Map value to return.
As previously mentioned in a comment here, versions of Terraform prior to
0.12 would store references to module outputs as just references to the
module as a whole in the state. It's not really clear why, but we wanted
to preserve this behavior for 0.12.
The previous implementation actually failed to do so, in spite of the
comment, so this commit fixes it to actually do what the comment
originally claimed.
In a later release we might remove this special case and just depend
directly on outputs where possible, since that'd allow us to produce a
more precise dependency graph for destroy actions, but when we do that
we'll first need to confirm that there isn't a good reason for the
original exception here.
The state only deals in wholly-known values, so here we null out any
unknowns for storage in state. This is okay because we subsequently write
the original, possibly-unknown value into the plan and the expression
evaluator will prefer to use this if present, allowing the unknown values
to properly propagate into other expressions in the calling module.
These need their output strings updated for the new behavior that all
resource instances recorded in state have a provider configuration
associated, whereas before we only did it for non-default ones.
Only the count and for_each expressions are evaluated by this node type,
so it doesn't need to declare dependencies for any other refs in the
configuration body. Using this more refined set of dependencies means
we can avoid graph cycles in the case where one instance of a resource
refers to another instance of the same resource.
We'll still get cycles if count or for_each self-reference, but that's
forbidden anyway (caught during validate) and makes sense because those
two are whole-resource-level config rather than per-instance config.
The underlying References function includes duplicates and returns refs
in the order they appeared in source (approximately), but after we reduce
to just the raw addresses it's better to dedupe and return in a
predictable order.
An earlier update to make this not use info.HumanId selected the wrong
fake "ami" name in the branch here.
Also, the error message for this failure was terrible. :(
This is computed in the special case where compute = "unknown" in order
to force inclusion of an unknown value into the ultimate result, which
is invalid.
This fixes TestContext2Apply_unknownAttribute, which is intending to test
this error handling behavior.
Previously we kept the dependencies one level higher on the resource
instance itself, which meant that updating it was handled in a different
EvalNode, but now we consider these to be dependencies of the object
itself (derived from the configuration that was current at the time it
was created), so we must handle this during EvalApply.
The subtle difference here is that if an object is moved to "deposed"
during a create_before_destroy replace then it will retain the
dependencies it had on its last apply, rather than them being replaced
by the dependencies of the newly-created object.
We now treat states.ResourceInstanceObject values as immutable once
constructed, preferring to replace them completely rather than update them
in-place to avoid weird race conditions.
Therefore EvalRefresh must copy the state it is given before mutating the
Value field of it to reflect the updated value from the provider.
Some earlier updates to it changed some things in our expected state
string. This doesn't fully fix it since there seems to still be a bug
related to recording dependencies.
This method is now removed, because our shims to the old provider API
(which used InstanceInfo) now populate only the Type attribute and so
HumanId would just generate garbage results anyway.
Our shims from new provider API to old can't populate the InstanceInfo
fully since the new API only includes the type name, and so anyone
depending on this method is now broken anyway.
In practice only our own tests depend on this, and so we'll drop it to
make it explicit that it no longer works (rather than having it return
nonsense) and then fix up the remaining tests that were depending on it
to use a different strategy.
This test was relying on the fact that we used to expose the full resource
instance address to providers via the InstanceInfo value, but we no longer
do that (since in practice no "real" providers depended on it, nor should
depend on it) so we need to instead include in the config itself a key
to use for tracking each resource instance for later test assertions.
InstanceInfo.HumanId() is no longer functional, since our shim from the
new to the old provider API doesn't populate it. Therefore we must use
other means to distingush the two instances here, and we'll use the "ami"
attribute value to do so.
This test was depending on InstanceInfo.HumanId, which is not something
any real providers use and therefore not something our shims from new to
old provider API supports.
Instead, we'll give each of the instances a different id and use that
to distinguish them for tracking apply order.
In the old protocol, returning a nil InstanceState was a way to indicate
that the object had been deleted. In the new world we signal that with
an actual object that contains a null value, which Terraform Core itself
will then recognize and turn into a nil state, eventually removing the
entry from state altogether.
If the plan called for us to delete but the result isn't null then that's
suspect, because it suggests the object wasn't deleted after all.
Likewise, no other apply action should cause the the result to be missing.
In order to avoid the confusing user experience that results in this case
(since it often looks like Terraform did nothing at all) we'll produce
some errors about it, but still update the state to reflect what the
provider returned anyway to allow for debugging and recovery.
Incorrect pointer discipline here was causing the error to be lost rather
than returned as expected.
Additionally we'll include a log line in this case because otherwise an
apply error is reported so far from the actual apply operation that it
can be difficult to understand what happened.
Previously we had a bug where we would fail to populate resource-level
metadata in the state during apply when count = 0, because the apply
graph would contain only instance nodes, not whole-resource nodes.
To address this, we add to the apply graph a node for each resource in
the configuration alongside the separate resource instance nodes. This
node's job is just to populate the state metadata for the resource, which
ensures it gets updated correctly even when count = 0.
When count is not zero this ends up doing some redundant work that
would've happened as a side-effect of applying individual resource
instances anyway, but it's harmless and makes the updating of our
resource-level metadata more explicit.
Our state models cannot store unknown values (since state only deals with
knowns) and so following the lead of recent similar changes for resource
instances we'll treat the planned changeset as a sort of overlay on the
state, preferring values stored there if present, and then write in basic
planned output changes to the plan when we evaluate them.
We're abusing the plan model a little here: its current design is intended
to lay the groundwork for a future release where output values have a
full lifecycle similar to resource instances where we can properly track
changes during the plan phase, but the rest of Terraform isn't yet ready
for that and so we'll just retain an approximation of the planned action
by only using Create and Destroy actions.
A future release should change this so that output changes can be tracked
accurately using an approach similar to that of resource instances.
We've intentionally changed the behavior for "count = 1" so that it'll
assign an index to the created instance even though there's only one. The
un-indexed behavior now applies only if count isn't set _at all_, thus
avoiding weird behavior if a count is _dynamically_ set to 1 via an
expression but is assumed to be a list elsewhere in configuration.
We previously tried to take a shortcut for an empty diff, just returning
the given value directly. This is incorrect in the weird case where we're
creating a new instance but it has no attributes (and thus an empty diff)
because in that case we'd return the given null value, turning the result
into a no-op or destroy change.
To fix this, we just always do the work to construct a new value, even
if we might end up doing all this just to reconstruct the same value we
started with in some cases.
This allows the provider to distinguish whether a particular value is set
in configuration or whether it's coming from prior state. It has no
particular purpose other than that.
We often want to bail out of a test if diagnostics are present, and it's
easiest to debug that when the diagnostics are printed in a compact but
complete manner that is non-trivial to produce.
Rather than duplicating that diagnostic formatting in every test, these
helpers allow us to succinctly print diagnostics and bail out when they
are present.
This is a pretty basic attempt to turn a pair of values into an old-school
diff. It probably won't work correctly for all tests, but hopefully works
well enough that we can just update the remaining tests in-place to use
the new API directly.
We now handle impure functions by having them return an unknown value
during plan, since we can't predict what the value will be during apply.
This test was assuming the old behavior.
The provider is allowed to return a partial result if it also includes
error diagnostics. Real providers still return at least a null value in
that case due to the RPC format, but test mocks are often more sloppy.
Since the refresh walk creates a partial plan to account for objects that
are yet to be created, we need to provide at least a basic mock of the
PlanProviderChange provider method.
For now we're using the old-style "DiffFn" shim interface since that's
already available for use in other tests.
It's not possible for a normal RPC-based provider to get into this
situation because a nil value can't go over the wire, but it's easy to
cause this by not correctly configuring a provider mock during tests.
By panicking early here we produce a more helpful error message and stack
trace than we'd otherwise produce if we let this nil value escape out
into the rest of Terraform.
Significant changes to the provider interface left a lot of the
tests in a non-buildable state. This set of changes gets the
tests building again but does not attempt to make them run to
completion or pass.
After this commit, it is possible to build a test program for
the ./terraform package but it will panic during its run. That
will be addressed in subsequent commits.
Since we do our deletes using a separate graph node from all of the other
actions, and a "Replace" change implies both a delete _and_ a create, we
need to pretend at apply time that a single replace change was actually
two separate changes.
This will also early-exit eval if a destroy node finds a non-Delete change
or if an apply node finds a Delete change. These should not happen in
practice because we leave these nodes out of the graph when they are not
needed for the given action, but we do this here for robustness so as not
to have an invisible dependency between the graph builder and the eval
phase.
When we're working on a create or destroy change it's expected for one of
the values to be null. Here we mimick the pre-0.12 behavior of producing
just an empty map in that case, which the helper/schema code (now the only
caller of this shim) then ignores completely.
Prior to our refactoring here, we were relying on a lucky coincidence for
correct behavior of the plan walk following a refresh in the same run:
- The refresh phase created placeholder objects in the state to represent
any resource instance pending creation, to allow the interpolator to
read attributes from them when evaluating "provider" and "data" blocks.
In effect, the refresh walk is creating a partial plan that only covers
creation actions, but was immediately discarding the actual diff entries
and storing only the planned new state.
- It happened that objects pending creation showed up in state with an
empty ID value, since that only gets assigned by the provider during
apply.
- The Refresh function concluded by calling terraform.State.Prune, which
deletes from the state any objects that have an empty ID value, which
therefore prevented these temporary objects from surviving into the
plan phase.
After refactoring, we no longer have this special ID field on instance
object state, and we instead rely on the Status field for tracking such
things. We also no longer have an explicit "prune" step on state, since
the state mutation methods themselves keep the structure pruned.
To address this, here we introduce a new instance object status "planned",
which is equivalent to having an empty ID value in the old world. We also
introduce a new method on states.SyncState that deletes from the state
any planned objects, which therefore replaces that portion of the old
State.prune operation just for this refresh use-case.
Finally, we are now expecting the expression evaluator to pull pending
objects from the planned changeset rather than from the state directly,
and so for correct results these placeholder resource creation changes
must also be reported in a throwaway changeset during the refresh walk.
The addition of states.ObjectPlanned also permits a previously-missing
safety check in the expression evaluator to prevent us from relying on the
incomplete value stored in state for a pending object, in the event that
some bug prevents the real pending object from being written into the
planned changeset.
We no longer use strings to represent addresses, so this method was a
leftover outlier from previous refactoring efforts.
At this time the result is not actually being used due to the state type
refactoring, which is a bug we'll address in a subsequent commit.
This is a light adaptation of our earlier prototype of structural diff
rendering, as a starting point for what we'll actually ship. This is not
consistent with the latest mocks, so will need some additional work before
it is ready, but integrating this allows us to at least see the plan
contents while fixing up remaining issues elsewhere.
We were previously tracking this as a []cty.Path, but having it turned
into a pathset on creation makes downstream use of it more convenient and
ensures that it'll obey expected invariants like not containing the same
path twice.
We're now writing the "planned new value" to OutputValue, but the data
resource nodes during refresh need to see the verbatim config value in
order to decide whether read must be deferred to the apply phase, so we'll
optionally export that here too.
Our state representation is not able to preserve unknown values, so it's
not suitable for retaining the transient incomplete values we produce
during planning.
Instead, we'll discard the unknown values when writing to state and have
the expression evaluator prefer an object from the plan where possible.
We still use the shape of the transient state to inform things like the
resource's "each mode", so the plan only masks the object values
themselves.
This is no longer a call into the provider, since all of the data diff
logic is standard for all data sources anyway. Instead, we just compute
the planned new value and construct a planned change from that as-is.
Previously the provider could, in principle, customize the read diff. In
practice there is no real reason to do that and the existing SDK didn't
pass that possibility through to provider code, so we can safely change
this without impacting provider compatibility.
Previously we just left these out of the plan altogether, but in the new
plan types we intentionally include change information for every resource
instance, even if no changes are actually planned, to allow alternative
plan file viewers to show what isn't changing as well as what is.
This also includes passing in the provider schema to a few more EvalNodes
that were expecting it but not getting it, in order to be able to
successfully test the implementation of EvalReadDiff here.
Chaange ResourceProvider to providers.Interface starting from the
context, and fix all type errors.
This only replaced some of method calls directly applicable to the
providers themselves. The resource methods will follow.
MockProvider and MockProvisioner implement the new plugin interfaces,
and are built following the patterns used by the legacy
MockResourceProvider and MockResourceProvisioner
Due to how often the state and plan types are referenced throughout
Terraform, there isn't a great way to switch them out gradually. As a
consequence, this huge commit gets us from the old world to a _compilable_
new world, but still has a large number of known test failures due to
key functionality being stubbed out.
The stubs here are for anything that interacts with providers, since we
now need to do the follow-up work to similarly replace the old
terraform.ResourceProvider interface with its replacement in the new
"providers" package. That work, along with work to fix the remaining
failing tests, will follow in subsequent commits.
The aim here was to replace all references to terraform.State and its
downstream types with states.State, terraform.Plan with plans.Plan,
state.State with statemgr.State, and switch to the new implementations of
the state and plan file formats. However, due to the number of times those
types are used, this also ended up affecting numerous other parts of core
such as terraform.Hook, the backend.Backend interface, and most of the CLI
commands.
Just as with 5861dbf3fc49b19587a31816eb06f511ab861bb4 before, I apologize
in advance to the person who inevitably just found this huge commit while
spelunking through the commit history.
The "config" package is no longer used and will be removed as part
of the 0.12 release cleanup. Since configschema is part of the
"new world" of configuration modelling, it makes more sense for
it to live as a subdirectory of the newer "configs" package.
Since the "References" function on graph nodes can't return errors, we
need to catch invalid depends_on references during the validation pass.
In this case, we're checking that the address is exact, rather than being
part of a traversal into an attribute of the object. In other words,
aws_instance.example is valid but aws_instance.example.id is not.
Previously we would attempt to DynamicExpand during the validate walk and
then validate each expanded instance separately. However, this meant that
we would not be able to validate the contents of a block where count = 0
or if count is not yet known.
Here we instead do a more static validation pass against the resource
configuration itself, setting count.index to cty.UnknownVal(cty.Number) so
we can type-check everything inside the block as being correct regardless
of the final count.
This is another step towards repairing the "validate" command for our
changed assumptions in a world where we have a more sophisticated type
checker.
This doesn't yet address the remaining problem that the expression
evaluator can't, with the current state structures, distinguish between
a completed resource with count = 0 and a resource that doesn't exist
at all (during validate), and so we'll still get errors if an expression
elsewhere in configuration refers to a dynamic index of a resource with
"count" set. That's a pre-existing condition that's no longer being masked
by _this_ problem, but can't be addressed until we've introduced the new
state types (states.State, etc) and thus we _can_ distinguish these two
situations. That will therefore be addressed in a later commit.
Previously we had the evaluate methods accept directly an
addrs.InstanceKey and had our evaluator infer a suitable value for
count.index for it, but that prevents us from setting the index to be
unknown in the validation scenario where we may not be able to predict
the number of instances yet but we still want to be able to check that
the configuration block is type-safe for all possible count values.
To achieve this, we separate the concern of deciding on a value for
count.index from the concern of evaluating it, which then allows for
other implementations of this in future. For the purpose of this commit
there is no change in behavior, with the count.index value being populated
whenever the instance key is a number.
This commit does a little more groundwork for the future implementation
of the for_each feature (which'll support each.key and each.value) but
still doesn't yet implement it, leaving it just stubbed out for the
moment.
Since schemas are required to interpret provider, resource, and
provisioner attributes in configs, states, and plans, these helpers intend
to make it easier to gather up the the necessary provider types in order
to preload all of the needed schemas before beginning further processing.
Config.ProviderTypes returns directly the list of provider types, since
at this level further detail is not useful: we've not yet run the
provider allocation algorithm, and so the only thing we can reliably
extract here is provider types themselves.
State.ProviderAddrs and Plan.ProviderAddrs each return a list of
absolute provider addresses, which can then be turned into a list of
provider types using the new helper providers.AddressedTypesAbs.
Since we're already using configs.Config throughout core, this also
updates the terraform.LoadSchemas helper to use Config.ProviderTypes
to find the necessary providers, rather than implementing its own
discovery logic. states.State is not yet plumbed in, so we cannot yet
use State.ProviderAddrs to deal with the state but there's a TODO comment
to remind us to update that in a later commit when we swap out
terraform.State for states.State.
A later commit will probably refactor this further so that we can easily
obtain schema for the providers needed to interpret a plan too, but that
is deferred here because further work is required to make core work with
the new plan types first. At that point, terraform.LoadSchemas may become
providers.LoadSchemas with a different interface that just accepts lists
of provider and provisioner names that have been gathered by the caller
using these new helpers.
I updated the "Variables" map incorrectly in earlier commit 10fe50bbdb
while making bulk updates to get the tests compiling again with the
changed underlying APIs.
The original value here was "bar", incorrectly changed to "foo" in that
commit. Here we return it back to "bar".
