From f959b560a2cc9863d1f8ce01e0383c1488808b33 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Oct 2018 13:40:01 -0400 Subject: [PATCH 1/4] trim index steps from RequiresNew paths Only GetAttrSteps can actually trigger RequiresNew, but the flatmaps paths will point to the indexed value that caused the change. --- config/hcl2shim/paths.go | 28 +++++++ config/hcl2shim/paths_test.go | 146 ++++++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+) diff --git a/config/hcl2shim/paths.go b/config/hcl2shim/paths.go index 5d2fb02d9..99437cbb1 100644 --- a/config/hcl2shim/paths.go +++ b/config/hcl2shim/paths.go @@ -28,6 +28,10 @@ func RequiresReplace(attrs []string, ty cty.Type) ([]cty.Path, error) { paths = append(paths, p) } + // now trim off any trailing paths that aren't GetAttrSteps, since only an + // attribute itself can require replacement + paths = trimPaths(paths) + // There may be redundant paths due to set elements or index attributes // Do some ugly n^2 filtering, but these are always fairly small sets. for i := 0; i < len(paths)-1; i++ { @@ -44,6 +48,30 @@ func RequiresReplace(attrs []string, ty cty.Type) ([]cty.Path, error) { return paths, nil } +// trimPaths removes any trailing steps that aren't of type GetAttrSet, since +// only an attribute itself can require replacement +func trimPaths(paths []cty.Path) []cty.Path { + var trimmed []cty.Path + for _, path := range paths { + path = trimPath(path) + if len(path) > 0 { + trimmed = append(trimmed, path) + } + } + return trimmed +} + +func trimPath(path cty.Path) cty.Path { + for len(path) > 0 { + _, isGetAttr := path[len(path)-1].(cty.GetAttrStep) + if isGetAttr { + break + } + path = path[:len(path)-1] + } + return path +} + // requiresReplacePath takes a key from a flatmap along with the cty.Type // describing the structure, and returns the cty.Path that would be used to // reference the nested value in the data structure. diff --git a/config/hcl2shim/paths_test.go b/config/hcl2shim/paths_test.go index ff52c92d9..cffbe6b5a 100644 --- a/config/hcl2shim/paths_test.go +++ b/config/hcl2shim/paths_test.go @@ -6,9 +6,18 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" ) +var ( + ignoreUnexported = cmpopts.IgnoreUnexported(cty.GetAttrStep{}, cty.IndexStep{}) + valueComparer = cmp.Comparer(cty.Value.RawEquals) +) + func TestPathFromFlatmap(t *testing.T) { tests := []struct { Flatmap string @@ -221,3 +230,140 @@ func TestPathFromFlatmap(t *testing.T) { }) } } + +func TestRequiresReplace(t *testing.T) { + for _, tc := range []struct { + name string + attrs []string + expected []cty.Path + ty cty.Type + }{ + { + name: "basic", + attrs: []string{ + "foo", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.String, + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}}, + }, + }, + { + name: "two", + attrs: []string{ + "foo", + "bar", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.String, + "bar": cty.String, + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}}, + cty.Path{cty.GetAttrStep{Name: "bar"}}, + }, + }, + { + name: "nested object", + attrs: []string{ + "foo.bar", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.Object(map[string]cty.Type{ + "bar": cty.String, + }), + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}, cty.GetAttrStep{Name: "bar"}}, + }, + }, + { + name: "nested objects", + attrs: []string{ + "foo.bar.baz", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.Object(map[string]cty.Type{ + "bar": cty.Object(map[string]cty.Type{ + "baz": cty.String, + }), + }), + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}, cty.GetAttrStep{Name: "bar"}, cty.GetAttrStep{Name: "baz"}}, + }, + }, + { + name: "nested map", + attrs: []string{ + "foo.%", + "foo.bar", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.Map(cty.String), + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}}, + }, + }, + { + name: "nested list", + attrs: []string{ + "foo.#", + "foo.1", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.Map(cty.String), + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}}, + }, + }, + { + name: "object in map", + attrs: []string{ + "foo.bar.baz", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.Map(cty.Object( + map[string]cty.Type{ + "baz": cty.String, + }, + )), + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}, cty.IndexStep{Key: cty.StringVal("bar")}, cty.GetAttrStep{Name: "baz"}}, + }, + }, + { + name: "object in list", + attrs: []string{ + "foo.1.baz", + }, + ty: cty.Object(map[string]cty.Type{ + "foo": cty.List(cty.Object( + map[string]cty.Type{ + "baz": cty.String, + }, + )), + }), + expected: []cty.Path{ + cty.Path{cty.GetAttrStep{Name: "foo"}, cty.IndexStep{Key: cty.NumberIntVal(1)}, cty.GetAttrStep{Name: "baz"}}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + rp, err := RequiresReplace(tc.attrs, tc.ty) + if err != nil { + t.Fatal(err) + } + if !cmp.Equal(tc.expected, rp, ignoreUnexported, valueComparer) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.expected, rp) + } + }) + + } + +} From 8212a6a9d0c486128bbea764166225e24120e38d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Oct 2018 13:42:28 -0400 Subject: [PATCH 2/4] add provider tests for force-new with a map Adding and removing a single map that requires a new resource can cause empty diffs, relying on the core proposed state values for destruction. --- builtin/providers/test/provider.go | 1 + builtin/providers/test/resource_force_new.go | 39 +++++++++ .../providers/test/resource_force_new_test.go | 79 +++++++++++++++++++ builtin/providers/test/resource_test.go | 30 +++++++ 4 files changed, 149 insertions(+) create mode 100644 builtin/providers/test/resource_force_new.go create mode 100644 builtin/providers/test/resource_force_new_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index d69ae95f9..e8b6cf228 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -22,6 +22,7 @@ func Provider() terraform.ResourceProvider { "test_resource_with_custom_diff": testResourceCustomDiff(), "test_resource_timeout": testResourceTimeout(), "test_resource_diff_suppress": testResourceDiffSuppress(), + "test_resource_force_new": testResourceForceNew(), }, DataSourcesMap: map[string]*schema.Resource{ "test_data_source": testDataSource(), diff --git a/builtin/providers/test/resource_force_new.go b/builtin/providers/test/resource_force_new.go new file mode 100644 index 000000000..81a06736c --- /dev/null +++ b/builtin/providers/test/resource_force_new.go @@ -0,0 +1,39 @@ +package test + +import ( + "github.com/hashicorp/terraform/helper/schema" +) + +func testResourceForceNew() *schema.Resource { + return &schema.Resource{ + Create: testResourceForceNewCreate, + Read: testResourceForceNewRead, + Delete: testResourceForceNewDelete, + + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "triggers": { + Type: schema.TypeMap, + Optional: true, + ForceNew: true, + }, + }, + } +} + +func testResourceForceNewCreate(d *schema.ResourceData, meta interface{}) error { + d.SetId("testId") + return testResourceForceNewRead(d, meta) +} + +func testResourceForceNewRead(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceForceNewDelete(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} diff --git a/builtin/providers/test/resource_force_new_test.go b/builtin/providers/test/resource_force_new_test.go new file mode 100644 index 000000000..3e0bf19c3 --- /dev/null +++ b/builtin/providers/test/resource_force_new_test.go @@ -0,0 +1,79 @@ +package test + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +func TestResourceForceNew_create(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_force_new" "foo" { + triggers = { + "a" = "foo" + } +}`), + }, + }, + }) +} +func TestResourceForceNew_update(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_force_new" "foo" { + triggers = { + "a" = "foo" + } +}`), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_force_new" "foo" { + triggers = { + "a" = "bar" + } +}`), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_force_new" "foo" { + triggers = { + "b" = "bar" + } +}`), + }, + }, + }) +} + +func TestResourceForceNew_remove(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_force_new" "foo" { + triggers = { + "a" = "bar" + } +}`), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_force_new" "foo" { +} `), + }, + }, + }) +} diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index 2d0168fc7..dd33783ff 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -443,3 +443,33 @@ output "value_from_map_from_list" { func testAccCheckResourceDestroy(s *terraform.State) error { return nil } + +func TestResource_removeForceNew(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + required_map = { + key = "value" + } + optional_force_new = "here" +} + `), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + required_map = { + key = "value" + } +} + `), + }, + }, + }) +} From 718a3c400a59f0852d5cffc9b843e30fe3dce6f4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Oct 2018 13:43:50 -0400 Subject: [PATCH 3/4] fix state variable name --- helper/plugin/grpc_provider.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 7669cb628..a25aa6745 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -399,12 +399,12 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso // The old provider API used an empty id to signal that the remote // object appears to have been deleted, but our new protocol expects // to see a null value (in the cty sense) in that case. - newConfigMP, err := msgpack.Marshal(cty.NullVal(block.ImpliedType()), block.ImpliedType()) + newStateMP, err := msgpack.Marshal(cty.NullVal(block.ImpliedType()), block.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) } resp.NewState = &proto.DynamicValue{ - Msgpack: newConfigMP, + Msgpack: newStateMP, } return resp, nil } From 4635ebc61aafbe769a83c7d02114cf1629b599d4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Oct 2018 13:44:21 -0400 Subject: [PATCH 4/4] create a new proposed value when replacing When replacing an instance, calculate a new proposed value from the null state and the config. This ensures that all unknown values are properly set. --- terraform/eval_diff.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 0b6c4aff3..50bb5ea3b 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -310,11 +310,15 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // from known prior values to unknown values, unless the provider is // able to predict new values for any of these computed attributes. nullPriorVal := cty.NullVal(schema.ImpliedType()) + + // create a new proposed value from the null state and the config + proposedNewVal = objchange.ProposedNewObject(schema, nullPriorVal, configVal) + resp = provider.PlanResourceChange(providers.PlanResourceChangeRequest{ TypeName: n.Addr.Resource.Type, Config: configVal, PriorState: nullPriorVal, - ProposedNewState: configVal, + ProposedNewState: proposedNewVal, PriorPrivate: plannedPrivate, }) // We need to tread carefully here, since if there are any warnings