Merge pull request #19233 from hashicorp/jbardin/requires-new

fix instance replacement

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(),

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
+}

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" {
+}			`),
+			},
+		},
+	})
+}

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"
+	}
+}
+				`),
+			},
+		},
+	})
+}

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.

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)
+			}
+		})
+
+	}
+
+}

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
 	}

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