Commit Graph

57 Commits

Author SHA1 Message Date
Martin Atkins 88e76fa9ef configs/configschema: Introduce the NestingGroup mode for blocks
In study of existing providers we've found a pattern we werent previously
accounting for of using a nested block type to represent a group of
arguments that relate to a particular feature that is always enabled but
where it improves configuration readability to group all of its settings
together in a nested block.

The existing NestingSingle was not a good fit for this because it is
designed under the assumption that the presence or absence of the block
has some significance in enabling or disabling the relevant feature, and
so for these always-active cases we'd generate a misleading plan where
the settings for the feature appear totally absent, rather than showing
the default values that will be selected.

NestingGroup is, therefore, a slight variation of NestingSingle where
presence vs. absence of the block is not distinguishable (it's never null)
and instead its contents are treated as unset when the block is absent.
This then in turn causes any default values associated with the nested
arguments to be honored and displayed in the plan whenever the block is
not explicitly configured.

The current SDK cannot activate this mode, but that's okay because its
"legacy type system" opt-out flag allows it to force a block to be
processed in this way anyway. We're adding this now so that we can
introduce the feature in a future SDK without causing a breaking change
to the protocol, since the set of possible block nesting modes is not
extensible.
2019-04-10 14:53:52 -07:00
Martin Atkins 87fe6cbecd plans/objchange: Don't panic when prior state contains nested map blocks
We were using the wrong cty operation to access map members, causing a
panic whenever a prior value was present for a resource type with a nested
block backed by a map value.
2019-03-18 09:16:50 -07:00
Martin Atkins c5aa5c68bc plans/objchange: Don't panic when dynamic-typed attrs are present
When dynamically-typed attributes are in the schema, we use different
conventions for representing nested blocks containing them (using tuples
and objects instead of lists and maps).

The normalization code here doesn't deal with those because the legacy
SDK never generates them, but we must still pass them through properly or
else other SDKs will be blocked from using dynamic attributes.

Previously this function would panic in that situation. Now it will just
pass through nested blocks containing dynamic attribute values entirely
as-is, with no normalization whatsoever. That's okay, because the scope
of this function is only to normalize inconsistencies that the legacy
SDK is known to produce, and the legacy SDK never produces dynamic-typed
attributes.
2019-03-11 08:18:26 -07:00
James Bardin e50be82da4 don't add empty blocks in ProposedNewObject
If the config contained a null block, don't convert it to a block with
empty values for the proposed new value.
2019-03-02 11:21:59 -05:00
Martin Atkins c280c27d87 plans/objchange: func NormalizeObjectFromLegacySDK
Now that we have an opt-out to let the legacy SDK return values that are
inconsistent with the new conventions for representing configuration,
various parts of Terraform must now be prepared to deal with
inconsistencies.

This function normalizes the most egregious inconsistencies relating to
the representation of nested blocks, freeing any recipient of its result
from worrying about these inconsistencies itself.
2019-02-27 16:53:29 -08:00
Martin Atkins 0b2cc6298b plans/objchange: Fix panic in AssertObjectCompatible with set blocks
Our usual "ground rules" for mapping configschema to cty call for the
collection values representing nested block types to always be known and
non-null, using an empty collection to represent the absense of any blocks
of that type so that users can always safely use length(...) etc on them
without worrying about them sometimes being null.

However, due to some different behaviors in the legacy SDK we've allowed
it an exception to this rule which means that we can see unknown and null
collections in these positions in object values returned from provider
operations like PlanResourceChange and ApplyResourceChange when the legacy
SDK opt-out is activated.

As a consequence of this, we need to be mindful in our safety check
functions, like AssertObjectCompatible here, of tolerating these non-ideal
situations to allow the safety checks to complete. We run these checks
even when the provider requests an opt-out, because we want to note any
inconsistencies as WARNING level log lines to aid in debugging.
2019-02-14 10:04:51 -08:00
Martin Atkins e831182c8d plans/objchange: Hide sensitive attribute values in error messages
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.
2019-02-11 17:26:50 -08:00
Martin Atkins fec6e0328d plans/objchange: AssertPlanValid function
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.
2019-02-08 19:47:02 -08:00
Martin Atkins 312d798a89 core: Restore our EvalReadData behavior
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.
2019-02-07 18:33:14 -08:00
Martin Atkins c794bf5bcc plans/objchange: Don't presume unknown for values unset in config
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.
2019-02-07 14:01:39 -08:00
Martin Atkins 7216049fdb plans/objchange: Improve precision of AssertObjectCompatible with sets
Previously we were just asserting that the number of elements didn't grow
between planned and actual. We still can't precisely correlate elements in
sets with unknown values, but here we adapt some logic we added earlier
to config/hcl2shim to ensure that we can find a plausible correlation for
each element in each set to at least one element in the other set, and
thus catch more cases where set elements might vanish or appear between
plan and apply, for improved safety.

This will still generate false negatives in some cases where unknown
values are present due to having to assume correlation is intended
wherever it is possible, but we'll catch situations where the actual value
is obviously contrary to what was planned.
2019-02-04 18:19:46 -08:00
James Bardin f3fe6184a0 test for destroy plan round trip 2018-12-20 15:11:08 -05:00
James Bardin 1b8617cef0 don't attempt to decode empty changes values
An empty DynamicValue can't be decoded but indicates no state, so just
return a NullVal.
2018-12-20 13:06:53 -05:00
James Bardin a915f3f13e don't convert empty DynamicValue to nil 2018-12-20 10:28:26 -05:00
James Bardin 78256ae225 return early when comparing Null values 2018-11-27 08:54:15 -05:00
Martin Atkins 300eceeb25 plans/planfile: fix TestRoundtrip
This was broken by an earlier change to verify the Terraform version
number when reading a state file. To fix it, we'll use our current version
in our constructed file which should then match when it's read back in.
2018-11-19 09:02:35 -08:00
Martin Atkins ab62b330c1 core: Allow planned output changes to be updated during apply
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.
2018-11-05 16:02:45 -08:00
Martin Atkins bbf8dacac8 plans: OutputChange.Encode must preserve Addr field 2018-11-01 17:33:10 -07:00
James Bardin e93d69f18b more nil/known checks before val.LengthInt 2018-10-19 16:51:15 -04:00
James Bardin e08a388d3c check IsKnown on values that may panic 2018-10-18 19:21:32 -04:00
Martin Atkins ec57927ea3 build: Take protoc out of the "go generate" path
Since protoc is not go-gettable, and most development tasks in Terraform
won't involve recompiling protoc files anyway, we'll use a separate
mechanism for these.

This way "go generate" only depends on things we can "go get" in the
"make tools" target.

In a later commit we should also in some way specify a particular version
of protoc to use so that we don't get "flapping" regenerations as
developers work with different versions, but the priority here is just to
make "make generate" minimally usable again to restore the dev workflow
documented in the README.

This also includes some updates that resulted from running "make generate"
and "make protobuf" after those Makefile changes were in place.
2018-10-18 10:39:20 -07:00
Martin Atkins 9b4b43c077 plans/objchange: Don't panic when a prior value with a set is null
ProposedNewObject intentionally replaces a null prior with an unknown
prior in order to easily fill in unknown values where they "show through"
under values not set explicitly in config, but it was failing to handle
that situation when dealing with nested blocks that are backed by sets.
2018-10-17 17:02:47 -07:00
Martin Atkins 2b80df0163 backend/local: Require caller to set PlanOutBackend with PlanOutPath
We can't generate a valid plan file without a backend configuration to
write into it, but it's the responsibility of the caller (the command
package) to manage the backend configuration mechanism, so we require it
to tell us what to write here.

This feels a little strange because the backend in principle knows its
own config, but in practice the backend only knows the _processed_ version
of the config, not the raw configuration value that was used to configure
it.
2018-10-16 19:14:11 -07:00
Kristin Laemmert 2d3cb87789 backend/local tests tests tests
converted the existing testPlanState() from terraform.State to
states.State to fix various plan tests.

reverted the "bandaid" in plans/planfile/tfplan.go - at this moment the
backend tests do not include backend configuration, and so the planfile
package can write the plan file but not read it back in. That will be
revisted in a separate track of work.
2018-10-16 19:14:11 -07:00
Kristin Laemmert 6a37ee9277 backend/local: more tests passing
I have no confidence in the change to plans/planfile/tfplan.go. The
tests were passing an empty backend config, which planfile  was able to
write to a file but not read from the same file. This change let me move
past that and it did not break any tests in the planfile package, but I
am concerned that it introduces undesired behavior.
2018-10-16 19:14:11 -07:00
Martin Atkins b0016e9cf6 command: Allow tests to run to completion without panics or hangs
There are still 160 test failures as of this commit, but at least the test
program can run to completion and list out all the failures.
2018-10-16 19:14:11 -07:00
Kristin Laemmert c661157999 plans/objchange: further harden ProposedNewObject against ~weird~
incoming values

Addresses an odd state where the priorV of an object to be changed is
known but null.

While this situation should not happen, it seemed prudent to ensure that
core is resilient to providers sending incorrect values (which might
also occur with manually edited state).
2018-10-16 19:14:11 -07:00
Kristin Laemmert 2a8aa6a139 plans/objchange: if priorV is unknown, fall through to the recursive call to `ProposedNewObject` 2018-10-16 19:14:11 -07:00
Martin Atkins 32974549cd plans/objchange: Fix handling of unknown in AssertValueCompatible
We need to check for the known-ness of the prior value before we check for
the null-ness of actual, because it's valid for an unknown value to become
a null.
2018-10-16 19:14:11 -07:00
Martin Atkins 896b6bc897 core: Fix test build for ./plans/planfile
This was missed in the splitting of "Replace" into "DeleteThenCreate" and
"CreateThenDelete".
2018-10-16 19:14:11 -07:00
Martin Atkins 9179cdcbc6 plans/planfile: allow resource instances with no instance key
This happens for resources that don't have either "count" or "for_each"
set.
2018-10-16 19:14:11 -07:00
Martin Atkins 3e9d23b120 plans: Regenerate Action.String for new action values
This was missed on the initial update of these because at the time the
package wasn't generate-able due to other problems.
2018-10-16 19:14:11 -07:00
Martin Atkins a43b7df282 core: Handle forced-create_before_destroy during the plan walk
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.
2018-10-16 19:14:11 -07:00
James Bardin b738cdb19f make Changes.Empty() not count NoOps
This make the plan `Empty` concept more closely match the legacy diff
behavior.

Remove old unknown field from plan_test
2018-10-16 19:14:11 -07:00
Martin Atkins d53c3d5c1b plans: Retain output value changes for all outputs in memory
During the plan operation we need to retain _somewhere_ the planned
changes for all outputs so we can refer to them during expression
evaluation. For consistency with how we handle resource instance changes,
we'll keep them in the plan so we can properly retain unknown values,
which cannot be written to state.

As with output values in the state, only root output plans are retained
in a round-trip through the on-disk plan file format, but that's okay
because we can trivially re-calculate all of these during apply. We
include the _root_ outputs in the plan file only because they are
externally-visible side effects that ought to be included in any rendering
of the plan made from the plan file for user inspection.
2018-10-16 19:14:11 -07:00
Martin Atkins 8048e9a585 plans/objchange: Don't panic if old or new values are null 2018-10-16 19:14:11 -07:00
Martin Atkins 6fd82ef97e core: Split Replace changes into separate Delete/Create changes
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.
2018-10-16 19:14:11 -07:00
Martin Atkins 1aa9ac14cc plans/objchange: LongestCommonSubsequence
This algorithm is the usual first step when generating diffs. This package
is a bit of a strange home for it, but since it works with changes to
cty.Value this feels more natural than any other place it could be.
2018-10-16 19:14:11 -07:00
Martin Atkins 1ced176fc6 plans: Track RequiredReplace as a cty.PathSet
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.
2018-10-16 19:14:11 -07:00
Martin Atkins 3f8a973846 plans/objchange: when prior is null, computed attributes are unknown 2018-10-16 19:14:11 -07:00
Martin Atkins 1d672623eb core: Remove changes from the plan after they are applied 2018-10-16 19:14:11 -07:00
Martin Atkins 2f0e5d93c8 plans: ChangesSync.GetResourceInstanceChange must copy the change
This is promised in its doc comment, but wasn't actually done in practice.
2018-10-16 19:14:11 -07:00
Martin Atkins 3d86dc51e0 core: Re-implement EvalReadDiff for the new plan types
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.
2018-10-16 19:14:11 -07:00
Martin Atkins bd299b9a22 core: Re-implement EvalWriteDiff to work with new plan types 2018-10-16 19:14:11 -07:00
Martin Atkins 44bc7519a6 terraform: More wiring in of new provider types
This doesn't actually work yet, but it builds and then panics in a pretty
satisfying way.
2018-10-16 19:12:54 -07:00
Martin Atkins 549544f201 configschema: Handle nested blocks containing dynamic-typed attributes
We need to make the collection itself be a tuple or object rather than
list or map in this case, since otherwise all of the elements of the
collection are constrained to be of the same type and that isn't the
intent of a provider indicating that it accepts any type.
2018-10-16 19:11:09 -07:00
Martin Atkins 4c78539c2b plans/objchange: AssertObjectSuperset function
This function's goal is to ensure that the "final" plan value produced
by a provider during the apply step is always consistent with the known
parts of the planned value produced during the plan step.

Any error produced here indicates a bug in the provider.
2018-10-16 19:11:09 -07:00
Martin Atkins 0317da9911 plans/objchange: logic for merging prior state with config
This produces a "proposed new state", which already has prior computed
values propagated into it (since that behavior is standard for all
resource types) but could be customized further by the provider to make
the "_planned_ new state".

In the process of implementing this it became clear that our configschema
DecoderSpec behavior is incorrect, since it's producing list values for
NestingList and map values for NestingMap. While that seems like it should
be right, we should actually be using tuple and object types respectively
to allow each block to have a different runtime type in situations where
an attribute is given the type cty.DynamicPseudoType. That's not fixed
here, and so without a further fix list and map blocks will panic here.
The DecoderSpec implementation will be fixed in a subsequent commit.
2018-10-16 19:11:09 -07:00
Martin Atkins dfe0988f10 plans: Record private data bytes as part of resource change
This allows a provider to retain arbitrary extra data in the plan and
make use of it during apply. The contents are not used by Terraform and
never shown to the user.
2018-10-16 19:11:09 -07:00
Martin Atkins a3403f2766 terraform: Ugly huge change to weave in new State and Plan types
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.
2018-10-16 19:11:09 -07:00