From c4d0be8a52f969398d04d99ce569f732bb2a1fff Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 8 Nov 2018 12:15:06 -0500 Subject: [PATCH 01/15] failing test for schemas with a single set attr Resources with certain combinations of attributes in a nested single set fail to perperly coerce their shimmed values. --- builtin/providers/test/provider.go | 1 + builtin/providers/test/resource_nested_set.go | 82 +++++++++++ .../test/resource_nested_set_test.go | 129 ++++++++++++++++++ 3 files changed, 212 insertions(+) create mode 100644 builtin/providers/test/resource_nested_set.go create mode 100644 builtin/providers/test/resource_nested_set_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index 1c0fc574d..59dd55085 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -24,6 +24,7 @@ func Provider() terraform.ResourceProvider { "test_resource_diff_suppress": testResourceDiffSuppress(), "test_resource_force_new": testResourceForceNew(), "test_resource_nested": testResourceNested(), + "test_resource_nested_set": testResourceNestedSet(), }, DataSourcesMap: map[string]*schema.Resource{ "test_data_source": testDataSource(), diff --git a/builtin/providers/test/resource_nested_set.go b/builtin/providers/test/resource_nested_set.go new file mode 100644 index 000000000..862c6c95b --- /dev/null +++ b/builtin/providers/test/resource_nested_set.go @@ -0,0 +1,82 @@ +package test + +import ( + "fmt" + "math/rand" + + "github.com/hashicorp/terraform/helper/schema" +) + +func testResourceNestedSet() *schema.Resource { + return &schema.Resource{ + Create: testResourceNestedSetCreate, + Read: testResourceNestedSetRead, + Delete: testResourceNestedSetDelete, + Update: testResourceNestedSetUpdate, + + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "optional": { + Type: schema.TypeBool, + Optional: true, + }, + "single": { + Type: schema.TypeSet, + Optional: true, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "value": { + Type: schema.TypeString, + ForceNew: true, + Required: true, + }, + + "optional": { + Type: schema.TypeString, + ForceNew: true, + Optional: true, + Computed: true, + }, + }, + }, + }, + }, + } +} + +func testResourceNestedSetCreate(d *schema.ResourceData, meta interface{}) error { + id := fmt.Sprintf("%x", rand.Int63()) + d.SetId(id) + + // replicate some awkward handling of a computed value in a set + set := d.Get("single").(*schema.Set) + l := set.List() + if len(l) == 1 { + if s, ok := l[0].(map[string]interface{}); ok { + if v, _ := s["optional"].(string); v == "" { + s["optional"] = id + } + } + } + + d.Set("single", set) + + return testResourceNestedRead(d, meta) +} + +func testResourceNestedSetRead(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceNestedSetDelete(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} + +func testResourceNestedSetUpdate(d *schema.ResourceData, meta interface{}) error { + return nil +} diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go new file mode 100644 index 000000000..56dd04bfc --- /dev/null +++ b/builtin/providers/test/resource_nested_set_test.go @@ -0,0 +1,129 @@ +package test + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" +) + +func TestResourceNestedSet_basic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + single { + value = "bar" + } +} + `), + }, + }, + }) +} + +// The set should not be generated because of it's computed value +func TestResourceNestedSet_noSet(t *testing.T) { + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_nested_set.foo"] + for k, v := range res.Primary.Attributes { + if strings.HasPrefix(k, "single") && k != "single.#" { + return fmt.Errorf("unexpected set value: %s:%s", k, v) + } + } + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { +} + `), + Check: checkFunc, + }, + }, + }) +} + +func TestResourceNestedSet_addRemove(t *testing.T) { + var id string + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_nested_set.foo"] + if res.Primary.ID == id { + return errors.New("expected new resource") + } + id = res.Primary.ID + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + single { + value = "bar" + } +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + single { + value = "bar" + } +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + single { + value = "bar" + optional = "baz" + } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { +} + `), + Check: checkFunc, + }, + }, + }) +} From ebc9745788760bfb537241b89c40b907bb734d4f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 8 Nov 2018 12:28:18 -0500 Subject: [PATCH 02/15] fix "too many items" error message --- configs/configschema/coerce_value.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configs/configschema/coerce_value.go b/configs/configschema/coerce_value.go index e6b163b9f..bae5733df 100644 --- a/configs/configschema/coerce_value.go +++ b/configs/configschema/coerce_value.go @@ -113,7 +113,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("insufficient items for attribute %q; must have at least %d", typeName, blockS.MinItems) } if l > blockS.MaxItems && blockS.MaxItems > 0 { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("too many items for attribute %q; must have at least %d", typeName, blockS.MinItems) + return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("too many items for attribute %q; cannot have more than %d", typeName, blockS.MaxItems) } if l == 0 { attrs[typeName] = cty.ListValEmpty(blockS.ImpliedType()) @@ -161,7 +161,7 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("insufficient items for attribute %q; must have at least %d", typeName, blockS.MinItems) } if l > blockS.MaxItems && blockS.MaxItems > 0 { - return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("too many items for attribute %q; must have at least %d", typeName, blockS.MinItems) + return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("too many items for attribute %q; cannot have more than %d", typeName, blockS.MaxItems) } if l == 0 { attrs[typeName] = cty.SetValEmpty(blockS.ImpliedType()) From 6fee1f24abc87ff2f386676429a2fe3cecea75d7 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 8 Nov 2018 18:01:39 -0500 Subject: [PATCH 03/15] don't add duplicate unknowns to a set The flatmap shim was lazily adding duplicate items and letting cty.Set clear them out, but if those duplicates contains unknown values they can't be checked for equality and will end up remaining in the set. --- config/hcl2shim/flatmap.go | 12 +++++++++ config/hcl2shim/flatmap_test.go | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/config/hcl2shim/flatmap.go b/config/hcl2shim/flatmap.go index 2f7954d76..bcecb30df 100644 --- a/config/hcl2shim/flatmap.go +++ b/config/hcl2shim/flatmap.go @@ -356,6 +356,11 @@ func hcl2ValueFromFlatmapSet(m map[string]string, prefix string, ty cty.Type) (c return cty.UnknownVal(ty), nil } + // Keep track of keys we've seen, se we don't add the same set value + // multiple times. The cty.Set will normally de-duplicate values, but we may + // have unknown values that would not show as equivalent. + seen := map[string]bool{} + for fullKey := range m { if !strings.HasPrefix(fullKey, prefix) { continue @@ -370,6 +375,12 @@ func hcl2ValueFromFlatmapSet(m map[string]string, prefix string, ty cty.Type) (c key = fullKey[:dot+len(prefix)] } + if seen[key] { + continue + } + + seen[key] = true + // The flatmap format doesn't allow us to distinguish between keys // that contain periods and nested objects, so by convention a // map is only ever of primitive type in flatmap, and we just assume @@ -386,5 +397,6 @@ func hcl2ValueFromFlatmapSet(m map[string]string, prefix string, ty cty.Type) (c if len(vals) == 0 { return cty.SetValEmpty(ety), nil } + return cty.SetVal(vals), nil } diff --git a/config/hcl2shim/flatmap_test.go b/config/hcl2shim/flatmap_test.go index 56c06c3dc..07f93ac26 100644 --- a/config/hcl2shim/flatmap_test.go +++ b/config/hcl2shim/flatmap_test.go @@ -635,6 +635,50 @@ func TestHCL2ValueFromFlatmap(t *testing.T) { }), }), }, + { + Flatmap: map[string]string{ + "single.#": "1", + "single.~1.value": "a", + "single.~1.optional": UnknownVariableValue, + "two.#": "2", + "two.~2381914684.value": "a", + "two.~2381914684.optional": UnknownVariableValue, + "two.~2798940671.value": "b", + "two.~2798940671.optional": UnknownVariableValue, + }, + Type: cty.Object(map[string]cty.Type{ + "single": cty.Set( + cty.Object(map[string]cty.Type{ + "value": cty.String, + "optional": cty.String, + }), + ), + "two": cty.Set( + cty.Object(map[string]cty.Type{ + "optional": cty.String, + "value": cty.String, + }), + ), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "single": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("a"), + "optional": cty.UnknownVal(cty.String), + }), + }), + "two": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("a"), + "optional": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("b"), + "optional": cty.UnknownVal(cty.String), + }), + }), + }), + }, } for _, test := range tests { From ce5d7ff6d0d6f266148647a67ee47e7244a88180 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 08:26:56 -0500 Subject: [PATCH 04/15] spelling --- helper/resource/state_shim.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/resource/state_shim.go b/helper/resource/state_shim.go index 9ad30d3ac..9c698d16a 100644 --- a/helper/resource/state_shim.go +++ b/helper/resource/state_shim.go @@ -71,7 +71,7 @@ func shimNewState(newState *states.State, schemas *terraform.Schemas) (*terrafor } if resSchema == nil { - return nil, fmt.Errorf("mising resource schema for %q in %q", resType, providerType) + return nil, fmt.Errorf("missing resource schema for %q in %q", resType, providerType) } for key, i := range res.Instances { From d2bd41c2600818a498c235722b671002939f477f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 18:50:47 -0500 Subject: [PATCH 05/15] add a nested set test --- builtin/providers/test/resource_nested_set.go | 31 ++++++++ .../test/resource_nested_set_test.go | 76 +++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/builtin/providers/test/resource_nested_set.go b/builtin/providers/test/resource_nested_set.go index 862c6c95b..c555265c3 100644 --- a/builtin/providers/test/resource_nested_set.go +++ b/builtin/providers/test/resource_nested_set.go @@ -44,6 +44,37 @@ func testResourceNestedSet() *schema.Resource { }, }, }, + "multi": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "set": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "required": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + }, + "optional_int": { + Type: schema.TypeInt, + Optional: true, + }, + }, + }, + }, + + "optional": { + Type: schema.TypeString, + ForceNew: true, + Optional: true, + }, + }, + }, + }, }, } } diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go index 56dd04bfc..7a27b75a2 100644 --- a/builtin/providers/test/resource_nested_set_test.go +++ b/builtin/providers/test/resource_nested_set_test.go @@ -127,3 +127,79 @@ resource "test_resource_nested_set" "foo" { }, }) } +func TestResourceNestedSet_multi(t *testing.T) { + checkFunc := func(s *terraform.State) error { + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + optional = "bar" + } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + set { + required = "val" + } + } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + set { + required = "new" + } + } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + set { + required = "new" + optional_int = 3 + } + } +} + `), + Check: checkFunc, + }, + }, + }) +} From b872491baaf525b436d9698465f46c89ad43dcaa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 21:04:56 -0500 Subject: [PATCH 06/15] incremental progress towards applying diffs --- helper/plugin/grpc_provider.go | 26 +++++++++++++++++++++- terraform/diff.go | 40 ++++++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index a25aa6745..a4cb17798 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -466,6 +466,15 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl priorState.Meta = priorPrivate + // We now rebuild the state through the ResourceData, so that the set indexes + // match what helper/schema expects. + data, err := schema.InternalMap(res.Schema).Data(priorState, nil) + if err != nil { + // FIXME + panic(err) + } + priorState = data.State() + // turn the proposed state into a legacy configuration config := terraform.NewResourceConfigShimmed(proposedNewStateVal, block) @@ -489,8 +498,23 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl return resp, nil } + if priorState == nil { + priorState = &terraform.InstanceState{} + } + // now we need to apply the diff to the prior state, so get the planned state - plannedStateVal, err := schema.ApplyDiff(priorStateVal, diff, block) + plannedAttrs, err := diff.Apply(priorState.Attributes, block) + plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, block.ImpliedType()) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } + + plannedStateVal, err = block.CoerceValue(plannedStateVal) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil diff --git a/terraform/diff.go b/terraform/diff.go index 92b575086..abf79e201 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -415,18 +415,40 @@ func (d *InstanceDiff) Unlock() { d.mu.Unlock() } // This method is intended for shimming old subsystems that still use this // legacy diff type to work with the new-style types. func (d *InstanceDiff) ApplyToValue(base cty.Value, schema *configschema.Block) (cty.Value, error) { - // We always build a new value here, even if the given diff is "empty", - // because we might be planning to create a new instance that happens - // to have no attributes set, and so we want to produce an empty object - // rather than just echoing back the null old value. // Create an InstanceState attributes from our existing state. // We can use this to more easily apply the diff changes. attrs := hcl2shim.FlatmapValueFromHCL2(base) + applied, err := d.Apply(attrs, schema) + if err != nil { + return base, err + } + + val, err := hcl2shim.HCL2ValueFromFlatmap(applied, schema.ImpliedType()) + if err != nil { + return base, err + } + + return schema.CoerceValue(val) +} + +// Apply applies the diff to the provided flatmapped attributes, +// returning the new instance attributes. +// +// This method is intended for shimming old subsystems that still use this +// legacy diff type to work with the new-style types. +func (d *InstanceDiff) Apply(attrs map[string]string, schema *configschema.Block) (map[string]string, error) { + // We always build a new value here, even if the given diff is "empty", + // because we might be planning to create a new instance that happens + // to have no attributes set, and so we want to produce an empty object + // rather than just echoing back the null old value. if attrs == nil { attrs = map[string]string{} } + fmt.Printf("\nBASE ATTRS: %#v\n", attrs) + fmt.Printf("\nDIFF: %#v\n", d) + if d.Destroy || d.DestroyDeposed || d.DestroyTainted { // to mark a destroy, we remove all attributes attrs = map[string]string{} @@ -444,7 +466,7 @@ func (d *InstanceDiff) ApplyToValue(base cty.Value, schema *configschema.Block) // if new or old is unknown, then there's no mismatch old != config.UnknownVariableValue && diff.Old != config.UnknownVariableValue { - return base, fmt.Errorf("diff apply conflict for %s: diff expects %q, but prior value has %q", attr, diff.Old, old) + return nil, fmt.Errorf("diff apply conflict for %s: diff expects %q, but prior value has %q", attr, diff.Old, old) } if diff.NewComputed { @@ -465,12 +487,8 @@ func (d *InstanceDiff) ApplyToValue(base cty.Value, schema *configschema.Block) attrs[attr] = diff.New } - val, err := hcl2shim.HCL2ValueFromFlatmap(attrs, schema.ImpliedType()) - if err != nil { - return val, err - } - - return schema.CoerceValue(val) + fmt.Printf("\nPLANNED ATTRS: %#v\n", attrs) + return attrs, nil } // ResourceAttrDiff is the diff of a single attribute of a resource. From df04e2e7a6d98dd6f1dbbf1cffd12e64d9d33d48 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 22:57:55 -0500 Subject: [PATCH 07/15] move InstanceState shim into schema.Resource This was the resource can rebuild the flatmapped state using the schema and ResourceData, providing us the the correct set key values. --- helper/schema/resource.go | 24 ++++++++++++++++++++++-- helper/schema/shims.go | 10 +--------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/helper/schema/resource.go b/helper/schema/resource.go index a26dfc9f8..d96bbcfde 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -155,6 +155,27 @@ type Resource struct { Timeouts *ResourceTimeout } +// ShimInstanceStateFromValue converts a cty.Value to a +// terraform.InstanceState. +func (r *Resource) ShimInstanceStateFromValue(state cty.Value) (*terraform.InstanceState, error) { + // Get the raw shimmed value. While this is correct, the set hashes don't + // match those from the Schema. + s := terraform.NewInstanceStateShimmedFromValue(state, r.SchemaVersion) + + // We now rebuild the state through the ResourceData, so that the set indexes + // match what helper/schema expects. + data, err := schemaMap(r.Schema).Data(s, nil) + if err != nil { + return nil, err + } + + s = data.State() + if s == nil { + s = &terraform.InstanceState{} + } + return s, nil +} + // See Resource documentation. type CreateFunc func(*ResourceData, interface{}) error @@ -550,8 +571,7 @@ func (r *Resource) upgradeState(s *terraform.InstanceState, meta interface{}) (* return nil, err } - s = InstanceStateFromStateValue(stateVal, r.SchemaVersion) - return s, nil + return r.ShimInstanceStateFromValue(stateVal) } // InternalValidate should be called to validate the structure diff --git a/helper/schema/shims.go b/helper/schema/shims.go index 52ae7d744..5b978ee8e 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -23,7 +23,7 @@ func DiffFromValues(prior, planned cty.Value, res *Resource) (*terraform.Instanc // only needs to be created for the apply operation, and any customizations // have already been done. func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffFunc) (*terraform.InstanceDiff, error) { - instanceState := InstanceStateFromStateValue(prior, res.SchemaVersion) + instanceState := terraform.NewInstanceStateShimmedFromValue(prior, res.SchemaVersion) configSchema := res.CoreConfigSchema() @@ -85,11 +85,3 @@ func JSONMapToStateValue(m map[string]interface{}, block *configschema.Block) (c func StateValueFromInstanceState(is *terraform.InstanceState, ty cty.Type) (cty.Value, error) { return is.AttrsAsObjectValue(ty) } - -// InstanceStateFromStateValue converts a cty.Value to a -// terraform.InstanceState. This function requires the schema version used by -// the provider, because the legacy providers used the private Meta data in the -// InstanceState to store the schema version. -func InstanceStateFromStateValue(state cty.Value, schemaVersion int) *terraform.InstanceState { - return terraform.NewInstanceStateShimmedFromValue(state, schemaVersion) -} From 34766ca6661701e7aba27ea211b973f5a0683c27 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 23:00:02 -0500 Subject: [PATCH 08/15] use the new InstanceState shim --- helper/plugin/grpc_provider.go | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index a4cb17798..4d6aa7307 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -387,7 +387,11 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso return resp, nil } - instanceState := schema.InstanceStateFromStateValue(stateVal, res.SchemaVersion) + instanceState, err := res.ShimInstanceStateFromValue(stateVal) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } newInstanceState, err := res.RefreshWithoutUpgrade(instanceState, s.provider.Meta()) if err != nil { @@ -455,7 +459,12 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl Type: req.TypeName, } - priorState := schema.InstanceStateFromStateValue(priorStateVal, res.SchemaVersion) + priorState, err := res.ShimInstanceStateFromValue(priorStateVal) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } + priorPrivate := make(map[string]interface{}) if len(req.PriorPrivate) > 0 { if err := json.Unmarshal(req.PriorPrivate, &priorPrivate); err != nil { @@ -466,15 +475,6 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl priorState.Meta = priorPrivate - // We now rebuild the state through the ResourceData, so that the set indexes - // match what helper/schema expects. - data, err := schema.InternalMap(res.Schema).Data(priorState, nil) - if err != nil { - // FIXME - panic(err) - } - priorState = data.State() - // turn the proposed state into a legacy configuration config := terraform.NewResourceConfigShimmed(proposedNewStateVal, block) @@ -595,7 +595,12 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A Type: req.TypeName, } - priorState := schema.InstanceStateFromStateValue(priorStateVal, res.SchemaVersion) + //priorState := terraform.NewInstanceStateShimmedFromValue(priorStateVal, res.SchemaVersion) + priorState, err := res.ShimInstanceStateFromValue(priorStateVal) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } private := make(map[string]interface{}) if len(req.PlannedPrivate) > 0 { From 16f28f73484a42b56584027889be71c72d30d480 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 23:00:21 -0500 Subject: [PATCH 09/15] new mechanism for applying a diff to a value This attempts to apply the diff in order to get consistent output from the shimmed values. --- terraform/diff.go | 268 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 236 insertions(+), 32 deletions(-) diff --git a/terraform/diff.go b/terraform/diff.go index abf79e201..fad3e531c 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -446,49 +446,253 @@ func (d *InstanceDiff) Apply(attrs map[string]string, schema *configschema.Block attrs = map[string]string{} } - fmt.Printf("\nBASE ATTRS: %#v\n", attrs) - fmt.Printf("\nDIFF: %#v\n", d) + return d.applyDiff(attrs, schema) +} + +func (d *InstanceDiff) applyDiff(attrs map[string]string, schema *configschema.Block) (map[string]string, error) { + // We always build a new value here, even if the given diff is "empty", + // because we might be planning to create a new instance that happens + // to have no attributes set, and so we want to produce an empty object + // rather than just echoing back the null old value. + + // Rather applying the diff to mutate the attrs, we'll copy new values into + // here to avoid the possibility of leaving stale values. + result := map[string]string{} if d.Destroy || d.DestroyDeposed || d.DestroyTainted { - // to mark a destroy, we remove all attributes - attrs = map[string]string{} - } else if attrs["id"] == "" || d.RequiresNew() { - // Since "id" is always computed, make sure it always has a value. Set - // it as unknown to generate the correct cty.Value - attrs["id"] = config.UnknownVariableValue + return result, nil } - for attr, diff := range d.Attributes { - old, exists := attrs[attr] + // iterate over the schema rather than the attributes, so we can handle + // blocks separately from plain attributes + for name, attrSchema := range schema.Attributes { + var err error + var newAttrs map[string]string - if exists && - old != diff.Old && - // if new or old is unknown, then there's no mismatch - old != config.UnknownVariableValue && - diff.Old != config.UnknownVariableValue { - return nil, fmt.Errorf("diff apply conflict for %s: diff expects %q, but prior value has %q", attr, diff.Old, old) + // handle non-block collections + switch ty := attrSchema.Type; { + case ty.IsListType() || ty.IsTupleType() || ty.IsMapType(): + newAttrs, err = d.applyCollectionDiff(name, attrs, attrSchema) + case ty.IsSetType(): + newAttrs, err = d.applySetDiff(name, attrs, schema) + default: + newAttrs, err = d.applyAttrDiff(name, attrs, attrSchema) + } + + if err != nil { + return result, err + } + + for k, v := range newAttrs { + result[k] = v + } + } + + for name, block := range schema.BlockTypes { + newAttrs, err := d.applySetDiff(name, attrs, &block.Block) + if err != nil { + return result, err + } + + for k, v := range newAttrs { + result[k] = v + } + } + + return result, nil +} + +func (d *InstanceDiff) applyAttrDiff(attrName string, oldAttrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) { + result := map[string]string{} + + diff := d.Attributes[attrName] + old, exists := oldAttrs[attrName] + + if diff != nil && diff.NewComputed { + result[attrName] = config.UnknownVariableValue + return result, nil + } + + // skip "id", as we already handled it + if attrName == "id" { + if old == "" { + result["id"] = config.UnknownVariableValue + } else { + result["id"] = old + } + return result, nil + } + + // attribute diffs are sometimes missed, so assume no diff means keep the + // old value + if diff == nil { + if exists { + result[attrName] = old + + } else { + // We need required values, so set those with an empty value. It + // must be set in the config, since if it were missing it would have + // failed validation. + if attrSchema.Required { + result[attrName] = "" + } + } + return result, nil + } + + // check for missmatched diff values + if exists && + old != diff.Old && + old != config.UnknownVariableValue && + diff.Old != config.UnknownVariableValue { + return result, fmt.Errorf("diff apply conflict for %s: diff expects %q, but prior value has %q", attrName, diff.Old, old) + } + + if attrSchema.Computed && diff.NewComputed { + result[attrName] = config.UnknownVariableValue + return result, nil + } + + if diff.NewRemoved { + // don't set anything in the new value + return result, nil + } + + result[attrName] = diff.New + return result, nil +} + +func (d *InstanceDiff) applyCollectionDiff(attrName string, oldAttrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) { + result := map[string]string{} + + // check the index first for special handling + for k, diff := range d.Attributes { + // check the index value, which can be set, and 0 + if k == attrName+".#" || k == attrName+".%" { + if diff.NewRemoved { + return result, nil + } + + if diff.NewComputed { + result[k] = config.UnknownVariableValue + return result, nil + } + + // do what the diff tells us to here, so that it's consistent with applies + if diff.New == "0" { + result[k] = "0" + return result, nil + } + } + } + + // collect all the keys from the diff and the old state + keys := map[string]bool{} + for k := range d.Attributes { + keys[k] = true + } + for k := range oldAttrs { + keys[k] = true + } + + idx := attrName + ".#" + if attrSchema.Type.IsMapType() { + idx = attrName + ".%" + } + + // record if we got the index from the diff + setIndex := false + + for k := range keys { + if !strings.HasPrefix(k, attrName+".") { + continue + } + + // we need to verify if we saw the index later + if k == idx { + setIndex = true + } + + res, err := d.applyAttrDiff(k, oldAttrs, attrSchema) + if err != nil { + return result, err + } + + for k, v := range res { + result[k] = v + } + } + + // Verify we have the index count. + // If it wasn't added from a diff, check it from the previous value. + // Make sure we keep the count if it existed before, so we can tell if it + // existed, or was null. + if !setIndex { + old := oldAttrs[idx] + if old != "" { + result[idx] = old + } + } + + return result, nil +} + +func (d *InstanceDiff) applySetDiff(attrName string, oldAttrs map[string]string, block *configschema.Block) (map[string]string, error) { + result := map[string]string{} + + idx := attrName + ".#" + // first find the index diff + for k, diff := range d.Attributes { + if k != idx { + continue } if diff.NewComputed { - attrs[attr] = config.UnknownVariableValue - continue + result[k] = config.UnknownVariableValue + return result, nil } - - if diff.NewRemoved { - delete(attrs, attr) - continue - } - - // sometimes helper/schema gives us values that aren't really a diff - if diff.Old == diff.New { - continue - } - - attrs[attr] = diff.New } - fmt.Printf("\nPLANNED ATTRS: %#v\n", attrs) - return attrs, nil + // Flag if there was a diff used in the set at all. + // If not, take the pre-existing set values + setDiff := false + + // here we're trusting the diff to supply all the known items + for k, diff := range d.Attributes { + if !strings.HasPrefix(k, attrName+".") { + continue + } + + setDiff = true + if diff.NewRemoved { + // no longer in the set + continue + } + + if diff.NewComputed { + result[k] = config.UnknownVariableValue + continue + } + + // helper/schema doesn't list old removed values, but since the set + // exists NewRemoved may not be true. + if diff.New == "" && diff.Old == "" { + continue + } + + result[k] = diff.New + } + + // use the existing values if there was no set diff at all + if !setDiff { + for k, v := range oldAttrs { + if strings.HasPrefix(k, attrName+".") { + result[k] = v + } + } + } + + return result, nil } // ResourceAttrDiff is the diff of a single attribute of a resource. From 71b55601ce505fec3b5f9aff490a28bea8079050 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Nov 2018 23:01:51 -0500 Subject: [PATCH 10/15] new failing tests for nested sets --- builtin/providers/test/resource_nested_set.go | 13 +++++++++++-- .../providers/test/resource_nested_set_test.go | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/builtin/providers/test/resource_nested_set.go b/builtin/providers/test/resource_nested_set.go index c555265c3..318e020e1 100644 --- a/builtin/providers/test/resource_nested_set.go +++ b/builtin/providers/test/resource_nested_set.go @@ -63,13 +63,22 @@ func testResourceNestedSet() *schema.Resource { Type: schema.TypeInt, Optional: true, }, + "bool": { + Type: schema.TypeBool, + Optional: true, + }, }, }, }, "optional": { - Type: schema.TypeString, - ForceNew: true, + Type: schema.TypeString, + // commenting this causes it to get missed during apply + //ForceNew: true, + Optional: true, + }, + "bool": { + Type: schema.TypeBool, Optional: true, }, }, diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go index 7a27b75a2..fb5694fa7 100644 --- a/builtin/providers/test/resource_nested_set_test.go +++ b/builtin/providers/test/resource_nested_set_test.go @@ -200,6 +200,24 @@ resource "test_resource_nested_set" "foo" { `), Check: checkFunc, }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + single { + value = "bar" + optional = "baz" + } + multi { + set { + required = "new" + optional_int = 3 + } + } +} + `), + Check: checkFunc, + }, }, }) } From 21dfa5676618dcccad1c3336c333db2856b89935 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 14 Nov 2018 14:19:29 -0500 Subject: [PATCH 11/15] use ShimInstanceStateFromValue in DiffFromValues This makes sure the diff is generated with the matching set ids from helper/schema. Update the tests to add ID fields to the state, which will exists in practice, since any state traversing through the shims will have the ID inserted. --- helper/schema/shims.go | 5 +- helper/schema/shims_test.go | 184 +++++++++++++++++------------------- 2 files changed, 89 insertions(+), 100 deletions(-) diff --git a/helper/schema/shims.go b/helper/schema/shims.go index 5b978ee8e..d99fb3965 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -23,7 +23,10 @@ func DiffFromValues(prior, planned cty.Value, res *Resource) (*terraform.Instanc // only needs to be created for the apply operation, and any customizations // have already been done. func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffFunc) (*terraform.InstanceDiff, error) { - instanceState := terraform.NewInstanceStateShimmedFromValue(prior, res.SchemaVersion) + instanceState, err := res.ShimInstanceStateFromValue(prior) + if err != nil { + return nil, err + } configSchema := res.CoreConfigSchema() diff --git a/helper/schema/shims_test.go b/helper/schema/shims_test.go index 68186497b..072c8ca7c 100644 --- a/helper/schema/shims_test.go +++ b/helper/schema/shims_test.go @@ -599,6 +599,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "availability_zone": "foo", }, @@ -901,6 +902,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "delete": "false", }, @@ -953,43 +955,6 @@ func TestShimSchemaMap_Diff(t *testing.T) { Err: false, }, - /* - // disabled for shims - // there is no longer any "list promotion" - { - Name: "List decode with promotion", - Schema: map[string]*Schema{ - "ports": &Schema{ - Type: TypeList, - Required: true, - Elem: &Schema{Type: TypeInt}, - PromoteSingle: true, - }, - }, - - State: nil, - - Config: map[string]interface{}{ - "ports": "5", - }, - - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "ports.#": &terraform.ResourceAttrDiff{ - Old: "0", - New: "1", - }, - "ports.0": &terraform.ResourceAttrDiff{ - Old: "", - New: "5", - }, - }, - }, - - Err: false, - }, - */ - { Name: "List decode with promotion with list", Schema: map[string]*Schema{ @@ -1109,6 +1074,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "3", "ports.0": "1", @@ -1137,6 +1103,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "2", "ports.0": "1", @@ -1353,6 +1320,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "0", }, @@ -1493,6 +1461,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "2", "ports.1": "1", @@ -1528,55 +1497,6 @@ func TestShimSchemaMap_Diff(t *testing.T) { Err: false, }, - /* - // disabled for shims - // you can't remove a required attribute - { - Name: "Set-7", - Schema: map[string]*Schema{ - "ports": &Schema{ - Type: TypeSet, - Required: true, - Elem: &Schema{Type: TypeInt}, - Set: func(a interface{}) int { - return a.(int) - }, - }, - }, - - State: &terraform.InstanceState{ - Attributes: map[string]string{ - "ports.#": "2", - "ports.1": "1", - "ports.2": "2", - }, - }, - - Config: map[string]interface{}{}, - - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "ports.#": &terraform.ResourceAttrDiff{ - Old: "2", - New: "0", - }, - "ports.1": &terraform.ResourceAttrDiff{ - Old: "1", - New: "0", - NewRemoved: true, - }, - "ports.2": &terraform.ResourceAttrDiff{ - Old: "2", - New: "0", - NewRemoved: true, - }, - }, - }, - - Err: false, - }, - */ - { Name: "Set-8", Schema: map[string]*Schema{ @@ -1592,6 +1512,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "availability_zone": "bar", "ports.#": "1", @@ -1634,6 +1555,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ingress.#": "2", "ingress.80.ports.#": "1", @@ -1718,6 +1640,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "availability_zone": "foo", "port": "80", @@ -1734,7 +1657,42 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, { - Name: "", + Name: "computed", + Schema: map[string]*Schema{ + "availability_zone": &Schema{ + Type: TypeString, + Computed: true, + ComputedWhen: []string{"port"}, + }, + + "port": &Schema{ + Type: TypeInt, + Optional: true, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "port": 80, + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": &terraform.ResourceAttrDiff{ + NewComputed: true, + }, + "port": &terraform.ResourceAttrDiff{ + New: "80", + }, + }, + }, + + Err: false, + }, + + { + Name: "computed, exists", Schema: map[string]*Schema{ "availability_zone": &Schema{ Type: TypeString, @@ -1749,6 +1707,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "port": "80", }, @@ -1758,13 +1717,8 @@ func TestShimSchemaMap_Diff(t *testing.T) { "port": 80, }, - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "availability_zone": &terraform.ResourceAttrDiff{ - NewComputed: true, - }, - }, - }, + // there is no computed diff when the instance exists already + Diff: nil, Err: false, }, @@ -1811,6 +1765,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "config_vars.%": "1", "config_vars.foo": "bar", @@ -1850,6 +1805,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "vars.%": "1", "vars.foo": "bar", @@ -1889,6 +1845,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "vars.%": "1", "vars.foo": "bar", @@ -1912,6 +1869,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "config_vars.#": "1", "config_vars.0.%": "1", @@ -1954,6 +1912,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "config_vars.#": "1", "config_vars.0.%": "2", @@ -2005,6 +1964,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "availability_zone": "bar", "address": "foo", @@ -2049,6 +2009,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "availability_zone": "bar", "ports.#": "1", @@ -2088,6 +2049,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "instances.#": "0", }, @@ -2275,6 +2237,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "vars.%": "0", }, @@ -2303,7 +2266,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, { - Name: " - Empty", + Name: "Empty", Schema: map[string]*Schema{}, State: &terraform.InstanceState{}, @@ -2324,6 +2287,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "some_threshold": "567.8", }, @@ -2376,6 +2340,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "block_device.#": "2", "block_device.616397234.delete_on_termination": "true", @@ -2410,6 +2375,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "port": "false", }, @@ -2453,6 +2419,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "route.#": "0", }, @@ -2476,6 +2443,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "active": "true", }, @@ -2503,6 +2471,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "instances.#": "1", "instances.3": "foo", @@ -2615,6 +2584,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "metadata_keys.#": "0", }, @@ -2675,6 +2645,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { Config: nil, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "tags.%": "0", }, @@ -2743,7 +2714,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, { - Name: ": StateFunc in nested set (#1759)", + Name: "StateFunc in nested set (#1759)", Schema: map[string]*Schema{ "service_account": &Schema{ Type: TypeList, @@ -2823,6 +2794,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "instances.#": "2", "instances.3": "333", @@ -2875,6 +2847,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "one": "false", "two": "true", @@ -2913,6 +2886,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { Schema: map[string]*Schema{}, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "id": "someid", }, @@ -2942,6 +2916,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "3", "ports.1": "1", @@ -2990,6 +2965,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "description": "foo", }, @@ -3023,7 +2999,9 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, }, - State: &terraform.InstanceState{}, + State: &terraform.InstanceState{ + ID: "id", + }, Config: map[string]interface{}{ "foo": "${var.foo}", @@ -3063,6 +3041,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "3", "ports.1": "1", @@ -3103,6 +3082,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "config.#": "2", "config.0": "a", @@ -3310,6 +3290,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "ports.#": "3", "ports.1": "1", @@ -3362,6 +3343,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { Schema: map[string]*Schema{}, State: &terraform.InstanceState{ + ID: "someid", Attributes: map[string]string{ "id": "someid", }, @@ -3397,6 +3379,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "etag": "foo", "version_id": "1", @@ -3442,6 +3425,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "foo": "bar", }, @@ -3471,6 +3455,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "attr": "bar", }, @@ -3508,6 +3493,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { }, State: &terraform.InstanceState{ + ID: "id", Attributes: map[string]string{ "unrelated_set.#": "0", "stream_enabled": "true", @@ -3549,11 +3535,10 @@ func TestShimSchemaMap_Diff(t *testing.T) { } { - d, err := InternalMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil, false) + d, err := schemaMap(tc.Schema).Diff(tc.State, terraform.NewResourceConfig(c), tc.CustomizeDiff, nil, false) if err != nil != tc.Err { t.Fatalf("err: %s", err) } - if !cmp.Equal(d, tc.Diff, equateEmpty) { t.Fatal(cmp.Diff(d, tc.Diff, equateEmpty)) } @@ -3597,6 +3582,7 @@ func TestShimSchemaMap_Diff(t *testing.T) { } res := &Resource{Schema: tc.Schema} + d, err := diffFromValues(stateVal, configVal, res, tc.CustomizeDiff) if err != nil { if !tc.Err { From 83317975fede70f726c1cfcecd601778073882c3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Nov 2018 09:54:27 -0500 Subject: [PATCH 12/15] add more tests with carious set combinations --- builtin/providers/test/resource_nested_set.go | 5 ++ .../test/resource_nested_set_test.go | 86 ++++++++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/builtin/providers/test/resource_nested_set.go b/builtin/providers/test/resource_nested_set.go index 318e020e1..c1e6520fb 100644 --- a/builtin/providers/test/resource_nested_set.go +++ b/builtin/providers/test/resource_nested_set.go @@ -23,6 +23,11 @@ func testResourceNestedSet() *schema.Resource { Type: schema.TypeBool, Optional: true, }, + "force_new": { + Type: schema.TypeString, + Optional: true, + ForceNew: true, + }, "single": { Type: schema.TypeSet, Optional: true, diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go index fb5694fa7..fe2818701 100644 --- a/builtin/providers/test/resource_nested_set_test.go +++ b/builtin/providers/test/resource_nested_set_test.go @@ -127,7 +127,7 @@ resource "test_resource_nested_set" "foo" { }, }) } -func TestResourceNestedSet_multi(t *testing.T) { +func TestResourceNestedSet_multiAddRemove(t *testing.T) { checkFunc := func(s *terraform.State) error { return nil } @@ -214,6 +214,25 @@ resource "test_resource_nested_set" "foo" { optional_int = 3 } } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + optional = true + single { + value = "bar" + optional = "baz" + } + multi { + set { + required = "new" + optional_int = 3 + } + } } `), Check: checkFunc, @@ -221,3 +240,68 @@ resource "test_resource_nested_set" "foo" { }, }) } + +func TestResourceNestedSet_forceNewEmptyString(t *testing.T) { + var id string + step := 0 + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_nested_set.foo"] + defer func() { + step++ + id = res.Primary.ID + }() + + if step == 2 && res.Primary.ID == id { + // setting an empty string currently does not trigger ForceNew, but + // it should in the future. + return nil + } + + if res.Primary.ID == id { + return errors.New("expected new resource") + } + + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + set { + required = "val" + } + } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + multi { + set { + required = "" + } + } +} + `), + Check: checkFunc, + }, + + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "foo" { + force_new = "" +} + `), + Check: checkFunc, + }, + }, + }) +} From 17ecda53b5f552626a4a1eb844df5bf7c35019aa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Nov 2018 09:55:34 -0500 Subject: [PATCH 13/15] strip empty containers from flatmap attributes In order to prevent mismatched states between read/plan/apply, we need to ensure that the attributes are generated consistently each time. Because of the various ways in which helper/schema and the hcl2 shims interpret empty values, the only way to ensure consistency is to always remove them altogether. --- helper/plugin/grpc_provider.go | 82 +++++++++++++++++++++++++++++ helper/plugin/grpc_provider_test.go | 33 ++++++++++++ 2 files changed, 115 insertions(+) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 4d6aa7307..7cb51035a 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -3,7 +3,10 @@ package plugin import ( "encoding/json" "errors" + "regexp" + "sort" "strconv" + "strings" "github.com/zclconf/go-cty/cty" ctyconvert "github.com/zclconf/go-cty/cty/convert" @@ -416,6 +419,8 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso // helper/schema should always copy the ID over, but do it again just to be safe newInstanceState.Attributes["id"] = newInstanceState.ID + newInstanceState.Attributes = normalizeFlatmapContainers(newInstanceState.Attributes) + newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(newInstanceState.Attributes, block.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -498,12 +503,22 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl return resp, nil } + // strip out non-diffs + for k, v := range diff.Attributes { + if v.New == v.Old && !v.NewComputed && !v.NewRemoved { + delete(diff.Attributes, k) + } + } + if priorState == nil { priorState = &terraform.InstanceState{} } // now we need to apply the diff to the prior state, so get the planned state plannedAttrs, err := diff.Apply(priorState.Attributes, block) + + plannedAttrs = normalizeFlatmapContainers(plannedAttrs) + plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, block.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -636,6 +651,13 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A } } + // strip out non-diffs + for k, v := range diff.Attributes { + if v.New == v.Old && !v.NewComputed && !v.NewRemoved { + delete(diff.Attributes, k) + } + } + if private != nil { diff.Meta = private } @@ -646,6 +668,10 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A return resp, nil } + if newInstanceState != nil { + newInstanceState.Attributes = normalizeFlatmapContainers(newInstanceState.Attributes) + } + newStateVal := cty.NullVal(block.ImpliedType()) // We keep the null val if we destroyed the resource, otherwise build the @@ -819,6 +845,62 @@ func pathToAttributePath(path cty.Path) *proto.AttributePath { return &proto.AttributePath{Steps: steps} } +// normalizeFlatmapContainers removes empty containers, and fixes counts in a +// set of flatmapped attributes. +func normalizeFlatmapContainers(attrs map[string]string) map[string]string { + keyRx := regexp.MustCompile(`.*\.[%#]$`) + + // find container keys + var keys []string + for k := range attrs { + if keyRx.MatchString(k) { + keys = append(keys, k) + } + } + + // sort the keys in reverse, so that we check the longest subkeys first + sort.Slice(keys, func(i, j int) bool { + a, b := keys[i], keys[j] + + if strings.HasPrefix(a, b) { + return true + } + + if strings.HasPrefix(b, a) { + return false + } + + return a > b + }) + + for _, k := range keys { + prefix := k[:len(k)-1] + indexes := map[string]int{} + for cand := range attrs { + if cand == k { + continue + } + + if strings.HasPrefix(cand, prefix) { + idx := cand[len(prefix):] + dot := strings.Index(idx, ".") + if dot > 0 { + idx = idx[:dot] + } + indexes[idx]++ + } + } + + if len(indexes) > 0 { + attrs[k] = strconv.Itoa(len(indexes)) + } else { + delete(attrs, k) + } + } + + return attrs +} + // helper/schema throws away timeout values from the config and stores them in // the Private/Meta fields. we need to copy those values into the planned state // so that core doesn't see a perpetual diff with the timeout block. diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index 03f059a0b..a06185fc4 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -3,6 +3,8 @@ package plugin import ( "context" "fmt" + "reflect" + "strconv" "strings" "testing" "time" @@ -637,3 +639,34 @@ func TestGetSchemaTimeouts(t *testing.T) { t.Fatal("missing default timeout in schema") } } + +func TestNormalizeFlatmapContainers(t *testing.T) { + for i, tc := range []struct { + attrs map[string]string + expect map[string]string + }{ + { + attrs: map[string]string{"id": "1", "multi.2.set.#": "1", "multi.1.set.#": "0", "single.#": "0"}, + expect: map[string]string{"id": "1"}, + }, + { + attrs: map[string]string{"id": "1", "multi.2.set.#": "2", "multi.2.set.1.foo": "bar", "multi.1.set.#": "0", "single.#": "0"}, + expect: map[string]string{"id": "1", "multi.2.set.#": "1", "multi.2.set.1.foo": "bar"}, + }, + { + attrs: map[string]string{"id": "78629a0f5f3f164f", "multi.#": "1"}, + expect: map[string]string{"id": "78629a0f5f3f164f"}, + }, + { + attrs: map[string]string{"multi.529860700.set.#": "1", "multi.#": "1", "id": "78629a0f5f3f164f"}, + expect: map[string]string{"id": "78629a0f5f3f164f"}, + }, + } { + t.Run(strconv.Itoa(i), func(t *testing.T) { + got := normalizeFlatmapContainers(tc.attrs) + if !reflect.DeepEqual(tc.expect, got) { + t.Fatalf("expected:\n%#v\ngot:\n%#v\n", tc.expect, got) + } + }) + } +} From a681124301ccc635e014c60347dfaead33ff326b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Nov 2018 10:33:41 -0500 Subject: [PATCH 14/15] verify DiffSuppresFunc behavior Terraform used to provide empty diffs to the provider when calculating `ignore_changes`, which would cause some DiffSuppressFunc to fail, as can be seen in #18209. Verify that this is no longer the case in 0.12 --- .../providers/test/resource_diff_suppress.go | 53 +++++++++++-- .../test/resource_diff_suppress_test.go | 79 +++++++++++++++++++ 2 files changed, 127 insertions(+), 5 deletions(-) diff --git a/builtin/providers/test/resource_diff_suppress.go b/builtin/providers/test/resource_diff_suppress.go index 5c01a1d09..cb5f7358f 100644 --- a/builtin/providers/test/resource_diff_suppress.go +++ b/builtin/providers/test/resource_diff_suppress.go @@ -1,23 +1,36 @@ package test import ( + "fmt" + "math/rand" "strings" "github.com/hashicorp/terraform/helper/schema" ) func testResourceDiffSuppress() *schema.Resource { + diffSuppress := func(k, old, new string, d *schema.ResourceData) bool { + if old == "" || strings.Contains(new, "replace") { + return false + } + return true + } + return &schema.Resource{ Create: testResourceDiffSuppressCreate, Read: testResourceDiffSuppressRead, - Update: testResourceDiffSuppressUpdate, Delete: testResourceDiffSuppressDelete, + Update: testResourceDiffSuppressUpdate, Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, }, Schema: map[string]*schema.Schema{ + "optional": { + Type: schema.TypeString, + Optional: true, + }, "val_to_upper": { Type: schema.TypeString, Required: true, @@ -29,18 +42,48 @@ func testResourceDiffSuppress() *schema.Resource { return strings.ToUpper(old) == strings.ToUpper(new) }, }, - "optional": { - Type: schema.TypeString, + "network": { + Type: schema.TypeString, + Optional: true, + Default: "default", + ForceNew: true, + DiffSuppressFunc: diffSuppress, + }, + "subnetwork": { + Type: schema.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + DiffSuppressFunc: diffSuppress, + }, + + "node_pool": { + Type: schema.TypeList, Optional: true, + Computed: true, + ForceNew: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "name": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + }, }, }, } } func testResourceDiffSuppressCreate(d *schema.ResourceData, meta interface{}) error { - d.SetId("testId") + d.Set("network", "modified") + d.Set("subnetwork", "modified") - return testResourceRead(d, meta) + id := fmt.Sprintf("%x", rand.Int63()) + d.SetId(id) + return nil } func testResourceDiffSuppressRead(d *schema.ResourceData, meta interface{}) error { diff --git a/builtin/providers/test/resource_diff_suppress_test.go b/builtin/providers/test/resource_diff_suppress_test.go index 59490e358..89416f32a 100644 --- a/builtin/providers/test/resource_diff_suppress_test.go +++ b/builtin/providers/test/resource_diff_suppress_test.go @@ -1,10 +1,14 @@ package test import ( + "errors" "strings" "testing" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" ) func TestResourceDiffSuppress_create(t *testing.T) { @@ -45,3 +49,78 @@ resource "test_resource_diff_suppress" "foo" { }, }) } + +func TestResourceDiffSuppress_updateIgnoreChanges(t *testing.T) { + // None of these steps should replace the instance + id := "" + checkFunc := func(s *terraform.State) error { + root := s.ModuleByPath(addrs.RootModuleInstance) + res := root.Resources["test_resource_diff_suppress.foo"] + if id != "" && res.Primary.ID != id { + return errors.New("expected no resource replacement") + } + id = res.Primary.ID + return nil + } + + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_diff_suppress" "foo" { + val_to_upper = "foo" + + network = "foo" + subnetwork = "foo" + + node_pool { + name = "default-pool" + } + lifecycle { + ignore_changes = ["node_pool"] + } +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_diff_suppress" "foo" { + val_to_upper = "foo" + + network = "ignored" + subnetwork = "ignored" + + node_pool { + name = "default-pool" + } + lifecycle { + ignore_changes = ["node_pool"] + } +} + `), + Check: checkFunc, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_diff_suppress" "foo" { + val_to_upper = "foo" + + network = "ignored" + subnetwork = "ignored" + + node_pool { + name = "ignored" + } + lifecycle { + ignore_changes = ["node_pool"] + } +} + `), + Check: checkFunc, + }, + }, + }) +} From 89b2c6f21e39c56dfef4bc20960735cd5ff5b2ea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Nov 2018 11:24:14 -0500 Subject: [PATCH 15/15] comment fixes --- helper/plugin/grpc_provider.go | 1 - terraform/diff.go | 6 ------ 2 files changed, 7 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 7cb51035a..c668e9733 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -610,7 +610,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A Type: req.TypeName, } - //priorState := terraform.NewInstanceStateShimmedFromValue(priorStateVal, res.SchemaVersion) priorState, err := res.ShimInstanceStateFromValue(priorStateVal) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) diff --git a/terraform/diff.go b/terraform/diff.go index fad3e531c..a28d7e482 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -450,11 +450,6 @@ func (d *InstanceDiff) Apply(attrs map[string]string, schema *configschema.Block } func (d *InstanceDiff) applyDiff(attrs map[string]string, schema *configschema.Block) (map[string]string, error) { - // We always build a new value here, even if the given diff is "empty", - // because we might be planning to create a new instance that happens - // to have no attributes set, and so we want to produce an empty object - // rather than just echoing back the null old value. - // Rather applying the diff to mutate the attrs, we'll copy new values into // here to avoid the possibility of leaving stale values. result := map[string]string{} @@ -513,7 +508,6 @@ func (d *InstanceDiff) applyAttrDiff(attrName string, oldAttrs map[string]string return result, nil } - // skip "id", as we already handled it if attrName == "id" { if old == "" { result["id"] = config.UnknownVariableValue