53 Commits (bc31dbc179b04b277ca268dedd5f30fa83af90da)

Author	SHA1	Message	Date
Martin Atkins	05caff2ca3	Move tfdiags/ to internal/tfdiags/ ... This is part of a general effort to move all of Terraform's non-library package surface under internal in order to reinforce that these are for internal use within Terraform only. If you were previously importing packages under this prefix into an external codebase, you could pin to an earlier release tag as an interim solution until you've make a plan to achieve the same functionality some other way.	5 years ago
James Bardin	213f3391c3	NormalizeObjectFromLegacySDK should handle marks ... Make sure that this function can handle any unexpectedly marked values. The only remaining caller of this function is in the diff formatter, which uses it to suppress meaningless diffs created by legacy providers.	5 years ago
James Bardin	59c8281378	use new dynamic decoding behavior ... Dynamic blocks with unknown for_each expressions are now decoded into an unknown value rather than using a sentinel object with unknown and null attributes. This will allow providers to precisely plan the block values, rather than trying to heuristically paper over the incorrect plans when dynamic is in use.	5 years ago
James Bardin	cd7fb9bd5a	catch invalidly planned attributes earlier ... Catch attributes which are planed but not computed separately to provide a clearer error to provider developers. The error conditions were previously caught, however it was unclear from the error text as to _why_ the change was an error. The statements about value inequality would be incorrect when planning no changes for a value which should not have been set in the first place.	5 years ago
James Bardin	0d63b3ec24	Merge pull request #27791 from hashicorp/jbardin/test-conformance-dynamic ... reverse call to TestConformance in objchange	5 years ago
James Bardin	22f21db229	reverse call to TestConformance in objchange ... The call to TestConformance needs to be reversed, since we want to verify that the actual value returned conforms to the planned type. While the inverse (checking that the planned value conforms to the applied type) works for everything terraform has been exposed to up until now, this fails when the planned type has dynamic attributes which are allowed to become concrete types.	5 years ago
Kristin Laemmert	8c2abbc0f0	return the properly-typed nulls, instead of empty containers, in proposedNewNestedType	5 years ago
Kristin Laemmert	77af601543	plans/objchange: extended ProposedNewObject to descend into attributes ... with NestedType objects. There are a handful of mostly cosmetic changes in this PR which likely make the diff awkward to read; I renamed several functions to (hopefully) clarifiy which funcs worked with Blocks vs other types. I also extracted some small code snippets into their own functions for reusability. The code that descends into attributes with NestedTypes is similar to the block-handling code, and differs in all the ways blocks and attributes differ: null is valid for attributes, unlike blocks which can only be present or empty.	5 years ago
Kristin Laemmert	da6ac9d6cd	plans/objchange: add handling of NestedTypes inside attributes ... - rename ProposedNewObject to ProposedNew: Now that there is an actual configschema.Object it will be clearer if the function names match the type the act upon. - extract attribute-handling logic from assertPlanValid and extend A new function, assertPlannedAttrsValid, takes the existing functionality and extends it to validate attributes with NestedTypes. The NestedType-specific handling is in assertPlannedObjectValid, which is very similar to the block-handling logic, except that nulls are a valid plan (an attribute can be null, but not a block).	5 years ago
Pam Selle	e6daf3dbf1	Unmark before ElementIterator in couldHaveUnknownBlockPlaceholder ... This is needed for cases where a variable may be fetched and become a member of a set, and thus the whole set is marked, which means ElementIterator will panic on unmarked values	5 years ago
James Bardin	ef086399f9	compare empty strings as null in sets ... The Legacy SDK cannot handle missing strings from objects in sets, and will insert an empty string when planning the missing value. This subverts the `couldHaveUnknownBlockPlaceholder` check, and causes errors when `dynamic` is used with NestingSet blocks. We don't have a separate codepath to handle the internals of AssertObjectCompatible differently for the legacy SDK, but we can treat empty strings as null strings within set objects to avoid the failed assertions.	6 years ago
James Bardin	77af322c1c	handle non-null, but empty NestingMap in a set	6 years ago
James Bardin	b59c64245b	refactor ifs to reduce indentation	6 years ago
James Bardin	f128b8c4fa	take dynamic types into account when comparing set ... If a NestingList or NestingMap contains a dynamic type, they must be handled as a cty.Tuple and cty.Object respectively, because the elements may not have precisely matching types.	6 years ago
Alexander Ovechkin	d7db008df2	added empty list test case	6 years ago
Alexander Ovechkin	8fbb4d0163	Converting ListVal to ListVal instead of TupleVal in setElementCompareValue	6 years ago
Pam Selle	da4ddd0160	Avoid disclosing values in errors on marked vals ... AssertObjectCompatible is a special case that will expose Go string values of values unless otherwise stopped. This adds that check.	6 years ago
Pam Selle	f35b530837	Update compatibility checks for blocks to not use marks ... Remove marks for object compatibility tests to allow apply to continue. Adds a block to the test provider to use in testing, and extends the sensitivity apply test to include a block	6 years ago
Pam Selle	0b3c21a3eb	Support lists of deeply marked values	6 years ago
Pam Selle	3dde9efc75	Support list diffs with sensitivity ... Adds support for specialized diffs with lists	6 years ago
Pam Selle	5b0b1a13a5	Update object compatible check to unmark ... The hack approach appears consistent, as we can remove marks before calling the value validation	6 years ago
Pam Selle	6c129a921b	Unmark/remark in apply process to allow apply	6 years ago
James Bardin	2b4101fdff	Unknown set blocks with dynamic may have 0 elems ... The couldHaveUnknownBlockPlaceholder helper was added to detect when a set block has a placeholder for an unknown number of values. This worked fine when the number increased from 1, but we were still attempting to validate the unknown placeholder against the empty set when the final count turned out to be 0. Since we can't differentiate the unknown dynamic placeholder value from an actual set value, we have to skip that object's validation altogether.	6 years ago
Chris Stephens	2dd64a7816	plans: Update error message for apply validation (#21312) ... * Update error message for apply validation Add a hint that the validation failure has occurred at the root of the resource schema to the error message. This is because the root resource has an empty path when being validated and the path is being relied upon to provide context into the error message.	6 years ago
Martin Atkins	31a4b44d2e	backend/local: treat output changes as side-effects to be applied ... This is a baby-step towards an intended future where all Terraform actions which have side-effects in either remote objects or the Terraform state can go through the plan+apply workflow. This initial change is focused only on allowing plan+apply for changes to root module output values, so that these can be written into a new state snapshot (for consumption by terraform_remote_state elsewhere) without having to go outside of the primary workflow by running "terraform refresh". This is also better than "terraform refresh" because it gives an opportunity to review the proposed changes before applying them, as we're accustomed to with resource changes. The downside here is that Terraform Core was not designed to produce accurate changesets for root module outputs. Although we added a place for it in the plan model in Terraform 0.12, Terraform Core currently produces inaccurate changesets there which don't properly track the prior values. We're planning to rework Terraform Core's evaluation approach in a forthcoming release so it would itself be able to distinguish between the prior state and the planned new state to produce an accurate changeset, but this commit introduces a temporary stop-gap solution of implementing the logic up in the local backend code, where we can freeze a snapshot of the prior state before we take any other actions and then use that to produce an accurate output changeset to decide whether the plan has externally-visible side-effects and render any changes to output values. This temporary approach should be replaced by a more appropriately-placed solution in Terraform Core in a release, which should then allow further behaviors in similar vein, such as user-visible drift detection for resource instances.	6 years ago
James Bardin	7a183a0e90	don't assert set block length with unknowns ... If a planned NestingList block value looks like it may represent a dynamic block, we don't check the length since it may be unknown. This check was missing in the NestingSet case, but it applies for the same reason. <	7 years ago
James Bardin	bfa5e7f811	actual value may be unknown in nested list ... When checking AssertObjectCompatible, we need to allow for a possible unkown nested list block, just as we did for a set in 0b2cc62.	7 years ago
Martin Atkins	332010fd56	plans/objchange: Fix handling of dynamic block placeholders ... If a dynamic block (in the HCL dynamic block extension sense) has an unknown value for its for_each argument, it gets expanded to a single placeholder block with all of its attributes set to a unknown values. We can use this as part of a heuristic to relax our object compatibility checks for situations where the plan included an object that appears to be (but isn't necessarily) such a placeholder, allowing for the fact that the one placeholder block could be replaced with zero or more real blocks once the for_each value is known. Previously our heuristic was too strict: it would match only if the only block present was a dynamic placeholder. In practice, users may mix dynamic blocks with static blocks of the same type, so we need to be more liberal to avoid generating incorrect incompatibility errors in such cases.	7 years ago
Martin Atkins	95e5ef13a7	vendor: go get github.com/zclconf/go-cty@master ... This includes a more comprehensive implementation of Value.GoString, along with various other changes that don't affect Terraform.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
James Bardin	78256ae225	return early when comparing Null values	8 years ago
James Bardin	e93d69f18b	more nil/known checks before val.LengthInt	8 years ago
James Bardin	e08a388d3c	check IsKnown on values that may panic	8 years ago
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.	8 years ago
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).	8 years ago
Kristin Laemmert	2a8aa6a139	plans/objchange: if priorV is unknown, fall through to the recursive call to `ProposedNewObject`	8 years ago
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.	8 years ago
Martin Atkins	8048e9a585	plans/objchange: Don't panic if old or new values are null	8 years ago
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.	8 years ago
Martin Atkins	3f8a973846	plans/objchange: when prior is null, computed attributes are unknown	8 years ago