Commit Graph

973 Commits

Author SHA1 Message Date
Martin Atkins bdcac8792d plugin: Use correct schema when marshaling imported resource objects
Previously we were using the type name requested in the import to select
the schema, but a provider is free to return additional objects of other
types as part of an import result, and so it's important that we perform
schema selection separately for each returned object.

If we don't do this, we get confusing downstream errors where the
resulting object decodes to the wrong type and breaks various invariants
expected by Terraform Core.

The testResourceImportOther test in the test provider didn't catch this
previously because it happened to have an identical schema to the other
resource type being imported. Now the schema is changed and also there's
a computed attribute we can set as part of the refresh phase to make sure
we're completing the Read call properly during import. Refresh was working
correctly, but we didn't have any tests for it as part of the import flow.
2019-02-01 15:22:54 -08:00
Martin Atkins 4c99864dad helper/resource: TestCheckResourceAttrPair allow nonexist
This checking helper is frequently used in provider tests for data
sources, as a shorthand to verify that an attribute of the data source
matches with the corresponding attribute on a managed resource.

Since we now leave empty collections null in more cases, this function is
sometimes effectively asked to verify that a given attribute is _unset_
in both the data source and the resource, so here we slightly adjust the
definition of the check to consider two nulls to be equal to one another,
which at this layer manifests as the keys not being present in the state
attributes map at all.

This check function didn't previously have tests, so this commit also adds
a basic suite of tests, including coverage for the new behavior.
2019-02-01 08:24:43 -08:00
James Bardin ba081f5de4 change copyMissingValues to normalizeNullValues
While copyMissingValues was meant to re-insert empty values that were
null after apply, it turns out plan is sometimes not predictable as
well.

normalizeNullValue is meant to fix up any null/empty transitions between
to values, and be useful during plan as well. For plan the function only
concerns itself with individual, known values, and skips sets entirely.
The result of running with plan == true is that only changes between
empty and null collections should be fixed.
2019-01-31 19:02:39 -05:00
James Bardin 9cf8f48239 decode legacy timeouts
The new decoder is more precise, and unpacks the timeout block into a
single map, which ResourceTimeout.ConfigDecode was updated to handle.
We however still need to work with legacy versions of terraform, with
the old decoder.
2019-01-30 16:10:17 -05:00
James Bardin 3b04b41250 fix RequiresNew in diff
With the new diff.Apply we can keep the diff mostly intact, but we need
turn off all RequiresNew flags so that the prior state is not removed
from the apply.
2019-01-30 14:55:04 -05:00
Martin Atkins 477da57a92 helper/plugin: Honor resource type overrides in import
One quirky aspect of our import feature is that we allow the importer to
produce additional resources alongside the one that was imported, such as
to create separate rules for each rule of an imported security group.

Providers need to be able to set the types of these other resources since
they may not match the "main" resource type. They do this by calling
ResourceData.SetType, which in turn sets InstanceState.Ephemeral.Type.

In our shims here we therefore need to copy that out into our new TypeName
field so that the new core import code can see it and create the right
type in the state.

Testing this required a minor change to the test harness to allow the
ImportStateCheck function to see the resource type.
2019-01-30 09:05:08 -08:00
Paul Tyng bb9ae50279
Copy TF version to helper/schema provider 2019-01-28 14:38:49 -05:00
Martin Atkins ae0be75ae0 helper/schema: TypeMap of Resource is actually of TypeString
Historically helper/schema did not support non-primitive map attributes
because they cannot be represented unambiguously in flatmap. When we
initially implemented CoreConfigSchema here we mapped that situation to
a nested block of mode NestingMap, even though that'd never worked until
now, assuming that it'd be harmless because providers wouldn't be using
it.

It turns out that some providers are, in fact, incorrectly populating
a TypeMap schema with Elem: &schema.Resource, apparently under the false
assumption that it would constrain the keys allowed in the map. In
practice, helper/schema has just been ignoring this and treating such
attributes as map of string. (#20076)

In order to preserve the behavior of these existing incorrectly-specified
attribute definitions, here we mimic the helper/schema behavior by
presenting as an attribute of type map(string).

These attributes have also been shown in some documentation as nested
blocks (with no equals sign), so that'll need to be fixed in user
configurations as they upgrade to Terraform 0.12. However, the existing
upgrade tool rules will take care of that as a natural consequence of the
name being indicated as an attribute in the schema, rather than as a block
type.

This fixes #20076.
2019-01-25 14:12:58 -08:00
James Bardin 37b5e2dc87 don't remove empty diff values
Our new diff handling no longer requires stripping the empty diffs out,
and provider may be relying on some of the empty-value quirks in
helper/schema.
2019-01-23 17:33:23 -05:00
James Bardin 46a4628782
Merge pull request #20081 from hashicorp/jbardin/list-block
New Diff.Apply method
2019-01-22 19:20:53 -05:00
Martin Atkins f65b7c5372 helper/plugin: Discard meaningless differences from provider planning
Due to various inprecisions in the old SDK implementation, applying the
generated diff can potentially make changes to the data structure that
have no real effect, such as replacing an empty list with a null list or
vice-versa.

Although we can't totally eliminate such diff noise, here we attempt to
avoid it in situations where there are _only_ meaningless changes -- where
the prior state and planned state are equivalent -- by just echoing back
the prior state verbatim to ensure that Terraform will treat it as a noop
change.

If there _are_ some legitimate changes then the result may still contain
meaningless changes alongside it, but that is just a cosmetic problem for
the diff renderer, because the meaningless changes will be ignored
altogether during a subsequent apply anyway. The primary goal here is just
to ensure we can converge on a fixpoint when there are no explicit changes
in the configuration.
2019-01-22 15:41:10 -08:00
James Bardin 8d302c5bd2 update grpc_provider for new diffs
Keep the diff as-is before applying.
2019-01-22 18:10:12 -05:00
James Bardin 286cb0a39d clean out diff a little more before checking
Check if there wasn't any real diff attributes first, before returning
the original state in PlanResourceChange.
2019-01-17 19:19:13 -05:00
James Bardin 4f691c5988 don't replace null strings with empty strings
This adds unexpected values in some cases, and since the case this
handles is only within set objects, we'll deal woth this when tackling
the sets themselves.
2019-01-17 19:19:13 -05:00
James Bardin 2cc651124e don't overwrite values in plan
Plan can change known values too, which we can't match in sets. We'll
find another way to normalize these eithout losing plan values.
2019-01-17 18:51:18 -05:00
James Bardin 7d05dee08d refactor ApplyResourceChange
Remove a bunch of indentation by returning early, and make sure we don't
fail on non-fatal error without saving the applied value.
2019-01-15 12:35:58 -05:00
James Bardin 0a731167db add a round trip through the shims during apply
Cycle through the shim operations after Apply, to ensure that we can
converge on a stable value for for Plan. While the shims produce valid
values in both directions, helper/schema sometimes does not agree on
which containers should be empty or null.
2019-01-15 11:59:15 -05:00
Martin Atkins 86c02d5c35 command: "terraform init" can partially initialize for 0.12upgrade
There are a few constructs from 0.11 and prior that cause 0.12 parsing to
fail altogether, which previously created a chicken/egg problem because
we need to install the providers in order to run "terraform 0.12upgrade"
and thus fix the problem.

This changes "terraform init" to use the new "early configuration" loader
for module and provider installation. This is built on the more permissive
parser in the terraform-config-inspect package, and so it allows us to
read out the top-level blocks from the configuration while accepting
legacy HCL syntax.

In the long run this will let us do version compatibility detection before
attempting a "real" config load, giving us better error messages for any
future syntax additions, but in the short term the key thing is that it
allows us to install the dependencies even if the configuration isn't
fully valid.

Because backend init still requires full configuration, this introduces a
new mode of terraform init where it detects heuristically if it seems like
we need to do a configuration upgrade and does a partial init if so,
before finally directing the user to run "terraform 0.12upgrade" before
running any other commands.

The heuristic here is based on two assumptions:
- If the "early" loader finds no errors but the normal loader does, the
  configuration is likely to be valid for Terraform 0.11 but not 0.12.
- If there's already a version constraint in the configuration that
  excludes Terraform versions prior to v0.12 then the configuration is
  probably _already_ upgraded and so it's just a normal syntax error,
  even if the early loader didn't detect it.

Once the upgrade process is removed in 0.13.0 (users will be required to
go stepwise 0.11 -> 0.12 -> 0.13 to upgrade after that), some of this can
be simplified to remove that special mode, but the idea of doing the
dependency version checks against the liberal parser will remain valuable
to increase our chances of reporting version-based incompatibilities
rather than syntax errors as we add new features in future.
2019-01-14 11:33:21 -08:00
Martin Atkins 0c0a437bcb Move module install functionality over to internal/initwd 2019-01-14 11:33:21 -08:00
James Bardin 041ed67e46 type names don't imply the resource mode
The addr type doesn't imply the resource mode, so data sources and
managed resources with the same type name could shim incorrectly.
2019-01-12 11:43:48 -05:00
James Bardin e8096e9c8b normalize values during ReadResource
Match the normalization behavior of Apply, so we don't end up causing
any diffs between zero values when refreshing resources.
2019-01-12 10:41:04 -05:00
James Bardin bc5eecd7f2 make sure id really gets set in SetId
SetId needs to overwrite the newState as well, since the internal calls
to DataSource.Id() will override the set attribute.
2019-01-10 20:28:11 -05:00
James Bardin a7b399cb4c use actual schema.Resources for state shims
Provider tests often rely on checking values contained within sets, by
directly accessing their flatmapped representation. In order to provider
the test harness with the expected set hashes, the sets must be
generated by the schema.Resource itself.

During the test we now build a fixed map of the providers, which should
only contain schema.Provider instances, and pass them into each
TestStep. The individual schema.Resource instances can then be pulled
from the providers, and used to recreate the state from the cty.Value
returned by the core operations.
2019-01-10 12:20:03 -05:00
James Bardin 7973872524 allow TestCheckNoResourceAttr for empty containers
Stricter type handling in the new shims may add empty containers into
the state where they were previously elided. Since the detection of
missing and empty containers in the legacy state was never reliable,
allow TestCheckNoResourceAttr to succeed if the key is a container count
index, and the value is "0"
2019-01-09 13:09:02 -05:00
James Bardin c63040c737 have TestCheckResourceAttr accept missing counts
Missing containers were often erroneously kept in the state, but since
the addition of the new provider shims, they can often be correctly
eliminated. There are however many tests that check for a "0" count in
the flatmap state when there shouldn't be a key at all. This addition
looks for a container count key and "0" pair, and allows for the key to
be missing.

There may be some tests negatively effected by this which were
legitimately checking for empty containers, but those were also not
reliably detected, and there should be much fewer tests involved.
2019-01-09 13:01:17 -05:00
James Bardin b55ec74c27 add copyMissingValues for normalizing shimmed Vals
Zero values and empty containers can be lost during the shimming
process, and during the provider's Apply step.

If we have known zero value containers and primitives in the source,
which appear as null values in the destination, we copy over the zero
value. Sets (and lists to an extent) are more difficult, since there
before and after indexes may not correlate. In that case we take the
entire container if it's wholly known, expecting the provider to have
correctly handled the value.
2019-01-08 16:26:22 -05:00
James Bardin 8300d65539 don't strip sets with count 1 when normalizing
normalizeFlatmapContainers should retain sets with a count of 1, and
convert sets with a count of 0 if they were 1 before the Apply step.
2019-01-08 16:26:21 -05:00
Martin Atkins cdad78d69b helper/resource: Allow multiple providers in a single TestCase
Due to incorrect use of a loop iterator variable inside a closure, all of
the given providers were ending up with the same factory function.
Now we copy the factory function to a local within the loop first so that
each iteration has its own variable.

This is the second round of similar bugs in this function, so we'll also
add a test case for it to reduce the risk of future regressions given that
most real callers don't exercise this with multiple providers in practice.
2019-01-07 16:58:36 -08:00
Martin Atkins b190d3b4f2 helper/resource: Shim back to old state must preserve schema version
We use a shim to convert from the new state model back to the old because
the provider test API is still using the old API throughout. However, the
shim was not preserving the schema version recorded in the new-style state
and so a round-trip through this shim would cause the schema versions to
all revert to zero.

This can cause trouble with the destroy phase of provider tests because
(for API legacy reasons) we round-trip from old state back to new again
before the destroy phase and thus causing the providers to try to upgrade
from state version zero even though the data was already latest, which
can cause errors because state upgrades are generally not idempotent.
2019-01-05 10:00:30 -08:00
Martin Atkins 06acc3f6c8 helper/schema: Skip validation of unknown values
With the introduction of explicit "null" in 0.12 it's possible for a value
that is unknown during plan to become a known null during apply, so we
need to slightly weaken our validation rules to accommodate that, in
particular skipping the validation of conflicting attributes if the result
could potentially be valid after the unknown values become known.

This change is in the codepath that is common to both 0.12 and 0.11
callers, but that's safe because 0.11 re-runs validation during the apply
step and so will still catch problems here, albeit in the apply step
rather than in the plan step, thus matching the 0.12 behavior. This new
behavior is a superset of the old in the sense that everything that was
valid before is still valid.

The implementation here also causes us to skip all other validation for
an attribute whose value is unknown. Most of the downstream validation
functions handle this directly anyway, but again this doesn't add any new
failure cases, and should clean up some of the rough edges we've seen with
unknown values in 0.11 once people upgrade to 0.12-compatible providers.
Any issues we now short-circuit during planning will still be caught
during apply.

While working on this I found that the existing "Not a list" test was not
actually testing the correct behavior, so this also includes a tweak to
that to ensure that it really is checking the "should be a list" path
rather than the "cannot be set" codepath it was inadvertently testing
before.
2019-01-04 14:46:47 -08:00
James Bardin 8ab5698e2a
Merge pull request #19587 from hashicorp/jbardin/safe-appends
don't modify argument slices
2018-12-10 15:10:02 -05:00
James Bardin 3d6ec09a83
Merge pull request #19552 from olindata/bugfix/setting-sets-in-list
helper/schema: Fix setting a set in a list caused error
2018-12-10 12:25:23 -05:00
James Bardin b5de50c0a2 don't modify argument slices
There were a couple spots where argument slices weren't being copied
before `append` was called, which could possibly modify the caller's
slice data.
2018-12-10 11:59:27 -05:00
Martin Atkins f9fef56167 helper/resource: print full diagnostics for operation errors in tests
This causes the output to include additional helpful context such as
the values of variables referenced in the config, etc. The output is in
the same format as normal Terraform CLI error output, though we don't
retain a source code cache in this codepath so it will not include a
source code snippet.
2018-12-07 17:05:36 -08:00
Martin Atkins 55469cd416 helper/resource: Get schemas from Terraform context
Previously the test harness was preloading schemas from the providers
before running any test steps.

Since terraform.NewContext already deals with loading provider schemas,
we can instead just use the schemas it loaded for our shimming needs,
avoiding the need to reimplement the schema lookup behavior and thus
the need to create a throwaway provider instance with which to do it.
2018-12-07 08:12:59 -08:00
Martin Atkins a4991c5780 helper/resource: Create a separate provider instance each call
Previously we were running the factory function only once when
constructing the provider resolver, which means that all contexts created
from that resolver share the same provider instance.

Instead now we will call the given factory function once for each
instantiation, ensuring that each caller ends up with a separate object
as would be the case in real-world use.
2018-12-07 08:12:59 -08:00
James Bardin 98870fadb1
Merge pull request #19544 from hashicorp/jbardin/import-tests
Fix provider import tests
2018-12-05 20:30:46 -05:00
James Bardin ac63d2995f
Merge pull request #19559 from hashicorp/jbardin/resource-test-shim
don't add numeric indexes to resources with a count of 0
2018-12-05 17:34:30 -05:00
James Bardin 7d296f752c don't add numeric indexes to resources with a count of 0 2018-12-05 13:41:53 -05:00
Farid Neshat 44a45b7332 helper/schema: Fix setting a set in a list
The added test in this commit, without the fix, will make d.Set return
the following error:

`Invalid address to set: []string{"ports", "0", "set"}`

This was due to the fact that setSet in feild_writer_map tried to
convert a slice into a set by creating a temp set schema and calling
writeField on that with the address(`[]string{"ports", "0", "set"}"` in
this case). However the temp schema was only for the set and not the
whole schema as seen in the address so, it should have been `[]string{"set"}"`
so it would align with the schema.

This commits adds another variable there(tempAddr) which will only
contain the last entry of the address that would be the set key, which
would match the created schema

This commit potentially fixes the problem described in #16331
2018-12-05 10:09:54 +01:00
Brian Flad 1e81a3e7fa
helper/schema: Always propagate NewComputed for previously zero value primative type attributes
When the following conditions were met:
* Schema attribute with a primative type (e.g. Type: TypeString) and Computed: true
* Old state of attribute set to zero value for type (e.g. "")
* Old state ID of resource set to non-empty (e.g. existing resource)

Attempting to use CustomizeDiff with SetNewComputed() would result in the difference  previously being discarded. This update ensures that previous zero values or resource existence does not influence the propagation of the computed update.

Previously:

```
--- FAIL: TestSetNewComputed (0.00s)
    --- FAIL: TestSetNewComputed/NewComputed_should_always_propagate (0.00s)
        resource_diff_test.go:684: Expected (*terraform.InstanceDiff)(0xc00051cea0)({
             mu: (sync.Mutex) {
              state: (int32) 0,
              sema: (uint32) 0
             },
             Attributes: (map[string]*terraform.ResourceAttrDiff) (len=1) {
              (string) (len=3) "foo": (*terraform.ResourceAttrDiff)(0xc0003dcec0)({
               Old: (string) "",
               New: (string) "",
               NewComputed: (bool) true,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              })
             },
             Destroy: (bool) false,
             DestroyDeposed: (bool) false,
             DestroyTainted: (bool) false,
             Meta: (map[string]interface {}) <nil>
            })
            , got (*terraform.InstanceDiff)(0xc00051ce80)({
             mu: (sync.Mutex) {
              state: (int32) 0,
              sema: (uint32) 0
             },
             Attributes: (map[string]*terraform.ResourceAttrDiff) {
             },
             Destroy: (bool) false,
             DestroyDeposed: (bool) false,
             DestroyTainted: (bool) false,
             Meta: (map[string]interface {}) <nil>
            })

--- FAIL: TestSchemaMap_Diff (0.01s)
    --- FAIL: TestSchemaMap_Diff/79-NewComputed_should_always_propagate_with_CustomizeDiff (0.00s)
        schema_test.go:3289: expected:
            *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"foo":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, Meta:map[string]interface {}(nil)}

            got:
            <nil>

FAIL
FAIL  github.com/hashicorp/terraform/helper/schema  0.825s
```
2018-12-04 22:48:30 -05:00
James Bardin 484d67028a handle shim errors in provider tests
These should be rare, and even though it's likely a shim bug, the error
is probably easier for provider developers to deal with than the
panic
2018-12-04 16:33:16 -05:00
James Bardin 547d63bcde remove empty flatmap containers from test states
Don't compare attributes with zero-length flatmap continers in tests.
2018-12-04 16:33:16 -05:00
James Bardin 924b97238f Handle StateFuncs in provider shim
Any state modifying functions can only be run once during the plan-apply
cycle. When regenerating the Diff during ApplyResourceChange, strip out
all StateFunc and CustomizeDiff functions from the schema.

Thew NewExtra diff field was where config data that was modified by a
StateFunc was stored, and needs to be maintained between plan and apply.

During PlanResourceChange, store any NewExtra data from the Diff in the
PlannedPrivate data, and re-insert the NewExtra data into the Diff
generated during ApplyResourceChange.
2018-12-03 18:12:02 -05:00
James Bardin 3dacdba678
Merge pull request #19521 from hashicorp/jbardin/prepare-provider-config
catch conversion errors in PrepareProviderConfig
2018-11-30 15:32:56 -05:00
James Bardin 5f9b189fcf catch conversion errors in PrepareProviderConfig
Errors were being ignore with the intention that they would be caught
later in validation, but it turns out we nee dto catch those earlier.

The legacy schemas also allowed providers to set and empty string for a
bool value, which we need to handle here, since it's not being handled
from user input like a normal config value.
2018-11-30 14:51:52 -05:00
Martin Atkins 72e279e6b2 providers: Consistently use int64 for schema versions
The rest of Terraform is still using uint64 for this in various spots, but
we'll update that gradually later. We use int64 here because that matches
what's used in our protobuf definition, and unsigned integers are not
portable across all of the protobuf target languages anyway.
2018-11-30 11:22:39 -08:00
James Bardin 6daf4989d4
Merge pull request #19475 from hashicorp/jbardin/computed-containers
Shim computed containers
2018-11-27 09:15:23 -05:00
James Bardin 6f4d86094f preserve possible zero values when normalizing
When normalizing flatmapped containers, compare the attributes to the
prior state and preserve pre-existing zero-length or unknown values. A
zero-length value that was previously unknown is preserved as a
zero-length value, as that may have been computed as such by the
provider.
2018-11-27 08:54:15 -05:00
Martin Atkins 58fa38b89a helper/schema: Update docs for PromoteSingle
This is no longer effective and should not be used in any new schema.
2018-11-26 17:11:34 -08:00