Now that we're actually verifying correct behavior of providers during
plan and apply, our mock providers need to behave like real providers,
properly propagating any configured values through the plan and into the
final state.
For most of these it was simpler to just switch over to using the newer
PlanResourceChangeFn mock interface, away from the legacy DiffFn approach,
because then we can just return the ProposedNewState verbatim because our
schema for these tests does not require any default values to be
populated.
Prior to Terraform 0.12 there were certain behaviors we expected from
providers that were actually just details of the SDK and not part of the
enforced contract.
For 0.12 we're now codifying some of these behaviors explicitly via safety
checks in core, thus ensuring that all future providers will behave in a
consistent way that users can rely on.
Unfortunately, due to the hand-written nature of the mock provider
implementations we use in tests, they have been getting away with some
unusual behaviors that don't match our usual expectations, and our safety
checks now detect those as incorrect behaviors.
To address this, we make the minimal changes to each test to ensure that
its mock provider behaves in a consistent way, which requires that values
set in config be represented correctly in the plan and ultimately saved
in the new state, without any changes along the way. In particular, the
common testDiffFn implementation has historically used a number of special
hidden attributes to trigger special behaviors, and our new rules require
that these special settings propagate properly through the plan and into
the state.
Since these error messages get printed in Terraform's output and we
encourage users to share them as part of bug reports, we should avoid
including sensitive information in them to reduce the risk of accidental
exposure.
Due to the inprecision of our shimming from the legacy SDK type system to
the new Terraform Core type system, the legacy SDK produces a number of
inconsistencies that produce only minor quirky behavior or broken
edge-cases. To retain compatibility with those existing weird behaviors,
the legacy SDK opts out of our safety checks.
The intent here is to allow existing providers to continue to do their
previous unsafe behaviors for now, accepting that this will allow certain
quirky bugs from previous releases to persist, and then gradually migrate
away from the legacy SDK and remove this opt-out on a per-resource basis
over time.
As with the apply-time safety check opt-out, this is reserved only for
the legacy SDK and must not be used in any new SDK implementations. We
still include any inconsistencies as warnings in the logs as an aid to
anyone debugging weird behavior, so that they can see situations where
blame may be misplaced in the user-visible error messages.
We've allowed the legacy SDK an opt-out from the post-apply safety checks,
but previously we produced only a generic warning message in that case.
Now instead we'll still run the safety checks, but report the results in
the logs instead of as error diagnostics.
This should allow developers who are debugging strange interactions
between buggy legacy providers to get better insight into what's going
on upstream in order to help explain what's going on when these problems
inevitably get caught by other downstream safety checks when trying to
make use of these invalid results.
We've been gradually adding safety checks of this sort throughout the
lifecycle to help ensure that buggy providers can't introduce
hard-to-diagnose downstream failures and misbehavior. This completes the
set by verifying during plan time that the provider has produced a plan
that actually achieves the goals defined in the configuration.
In particular, this catches the situation where a provider may incorrectly
override a value explicitly set in configuration, which avoids creating
confusion by betraying the reasonable user expectation that referencing an
explicitly-defined attribute will produce exactly the value shown in
configuration.
The helper/schema handling of lists loses empty string values, but
retains the correct count. Only re-count the values if the count is
missing entirely, and allow our shims to re-populate the zero values.
* command/jsonplan:
- add variables to plan output
- print known planned values for resources
Previously, resource attribute values were only displayed if the values
were wholly known. Now we will filter the unknown values out of the
change and print the known values.
* command/jsonstate: added depends_on and tainted fields
* command/show: update tests to reflect added fields
Terraform core expects a sane state even when the provider returns an
error. Make sure at the prior state is always the default value to
return, and then alway attempt to process any state returned by
provider.Apply.
This was changed in the single attribute test cases, but the AttrPair
test is used a lot for data source. As far as tests are concerned, 0 and
unset should be treated equally for flatmapped collections.
Completing our set of provider result safety-check functions,
AssertPlanValid checks a result from a provider's PlanResourceChange to
make sure it doesn't propose a change that is not valid within the user
model of Terraform.
Specifically, it forbids the provider from planning a value that
contradicts what the user gave in configuration, which is important to
ensure that making a reference to an attribute elsewhere in the
configuration will produce exactly the given result, as users reasonably
expect.
Providers _are_ allowed (and, in fact, required) to make changes to
any Computed attribute values declared in the schema in order to fill in
the default values that the provider has generated. Later checks during
the apply phase will ensure that the provider remains true to these
planned values, to ensure that Terraform can keep its promise of doing
exactly what was planned or presenting an error explaining why not.
Previously, configupgrade would panic if it encountered a HEREDOC. For
the time being, we will simply print out the HEREDOC as-is.
Unfortunately, we discovered that terraform 0.11's version of HCL
allowed for HEREDOCs with the termination delimiter inline (instead of
on a newline, which is technically correct). Since 0.12configupgrade
needs to be bug-compatible with terraform 0.11, we must roll back to the
same version of HCL used in terraform 0.11.
This changes the contract for `PlanResourceChange` so that the provider is now responsible
for populating all default values during plan, including inserting any unknown values for
defaults it will fill in at apply time.
Objects with DynamicPseudoType attributes can't be coerced within a map
if a concrete type is set. Change the Value type used to an Object when
there is a type mismatch.
We've changed the contract for PlanResourceChange to now require the
provider to populate any default values (including unknowns) it wants to
set for computed arguments, so our mock provider here now needs to be a
little more complex to deal with that.
This fixes several of the tests in this package. A minor change to
TestLocal_applyEmptyDirDestroy was required to make it properly configure
the mock provider so PlanResourceChange can access the schema.
We now require a provider to populate all of its defaults -- including
unknown value placeholders -- during PlanResourceChange. That means the
mock provider for testing "terraform show -json" must now manage the
population of the computed "id" attribute during plan.
To make this logic a little easier, we also change the ApplyResourceChange
implementation to fill in a non-null id, since that makes it easier for
the mock PlanResourceChange to recognize when it needs to populate that
default value during an update.
Check attributes on null objects, and fill in unknowns. If we're
evaluating the object, it either means we are at the top level, or a
NestingSingle block was present, and in either case we need to treat the
attributes as null rather than the entire object.
Switch on the block types rather than Nesting, so we don't need add any
logic to change between List/Tuple or Map/Object when DynamicPseudoType
is involved.
In an earlier commit we changed objchange.ProposedNewObject so that the
task of populating unknown values for attributes not known during apply
is the responsibility of the provider's PlanResourceChange method, rather
than being handled automatically.
However, we were also using objchange.ProposedNewObject to construct the
placeholder new object for a deferred data resource read, and so we
inadvertently broke that deferral behavior. Here we restore the old
behavior by introducing a new function objchange.PlannedDataResourceObject
which is a specialized version of objchange.ProposedNewObject that
includes the forced behavior of populating unknown values, because the
provider gets no opportunity to customize a deferred read.
TestContext2Plan_createBeforeDestroy_depends_datasource required some
updates here because its implementation of PlanResourceChange was not
handling the insertion of the unknown value for attribute "computed".
The other changes here are just in an attempt to make the flow of this
test more obvious, by clarifying that it is simulating a -refresh=false
run, which effectively forces a deferred read since we skip the eager
read that would normally happen in the refresh step.
Now that ProposedNewState uses null to represent Computed attributes not
set in the configuration, the provider must fill in the unknown value for
"computed" in its plan result.
It seems that this test was incorrectly updated during our bulk-fix after
integrating the HCL2 work, but it didn't really matter because the
ReadDataSource function isn't called in the happy path anyway. But to
make the intent clearer here, we also now make ReadDataSource return an
error if it is called, making it explicit that no call is expected.
Data resources do not have a plan/apply distinction, so it is never valid
for a data resource to produce unknown values in its result object.
Unknown values in the data resource _config_ cause us to postpone the read
altogether, so a data source never receives unknown values as input and
therefore may never produce unknown values as output.
Previously we would construct a proposed new state with unknown values in
place of any not-set-in-config computed attributes, trying to save the
provider a little work in specifying that itself.
Unfortunately that turns out to be problematic because it conflates two
concerns: attributes can be explicitly set in configuration to an unknown
value, in which case the final result of that unknown overrides any
default value the provider might normally populate.
In other words, this allows the provider to recognize in the proposed new
state the difference between an Optional+Computed attribute being set to
unknown in the config vs not being set in the config at all.
The provider now has the responsibility to replace these proposed null
values with unknown values during PlanResourceChange if it expects to
select a value during the apply step. It may also populate a known value
if the final result can be predicted at plan time, as is the case for
constant defaults specified in the provider code.
This change comes from a realization that from core's perspective the
helper/schema ideas of zero values, explicit default values, and
customizediff tweaks are all just examples of "defaults", and by allowing
the provider to see during plan whether these attributes are being
explicitly set in configuration and thus decide whether the default will
be provided immediately during plan or deferred until apply.
The shim layer for the legacy SDK type system is not precise enough to
guarantee it will produce identical results between plan and apply. In
particular, values that are null during plan will often become zero-valued
during apply.
To avoid breaking those existing providers while still allowing us to
introduce this check in the future, we'll introduce a rather-hacky new
flag that allows the legacy SDK to signal that it is the legacy SDK and
thus disable the check.
Once we start phasing out the legacy SDK in favor of one that natively
understands our new type system, we can stop setting this flag and thus
get the additional safety of this check without breaking any
previously-released providers.
No other SDK is permitted to set this flag, and we will remove it if we
ever introduce protocol version 6 in future, assuming that any provider
supporting that protocol will always produce consistent results.
Previously we would allow providers to change anything about the planned
object value during apply, possibly returning an entirely-unrelated object
of the same type. In practice this led to some subtle bugs where a single
planned attribute value would change during apply and cause a downstream
failure due to a dependent resource now seeing input other than what
_it_ expected during plan.
Now we'll produce an explicit error message for this case which places the
blame with the correct party: the upstream resource that changed. Without
this, unexpected changes would often lead to the downstream resource
implementation being blamed in error message even though it was just
reacting to the change from upstream.
As with most errors during apply, we'll still save the updated value in
the state but we'll halt the walk to ensure that the unexpected value
cannot propagate further and cause the result to potentially diverge
greatly from the changeset shown in the plan.
Compared to Terraform 0.11, we expect to see this error in many of the
same cases we saw the "diffs didn't match during apply" error in earlier
versions, since it is likely that many errors of that sort were the result
of unexpected upstream changes being incorrectly blamed on the downstream
resource that then used the result.
Because Terraform Core has traditionally not checked that the final apply
result is consistent with what was planned, some of our apply tests were
producing inconsistent results.
Here we fix all of that so that they produce something compatible with
what they planned. This doesn't actually achieve anything in isolation,
but we're about to start enforcing this consistency in a subsequent
commit.
Radek Simko