Merge pull request #5026 from hashicorp/phinze/destroy-node-copies

core: Make copies when creating destroy nodes

config/config.go

@@ -81,6 +81,27 @@ type Resource struct {
 	Lifecycle    ResourceLifecycle
 }
 
+// Copy returns a copy of this Resource. Helpful for avoiding shared
+// config pointers across multiple pieces of the graph that need to do
+// interpolation.
+func (r *Resource) Copy() *Resource {
+	n := &Resource{
+		Name:         r.Name,
+		Type:         r.Type,
+		RawCount:     r.RawCount.Copy(),
+		RawConfig:    r.RawConfig.Copy(),
+		Provisioners: make([]*Provisioner, 0, len(r.Provisioners)),
+		Provider:     r.Provider,
+		DependsOn:    make([]string, len(r.DependsOn)),
+		Lifecycle:    *r.Lifecycle.Copy(),
+	}
+	for _, p := range r.Provisioners {
+		n.Provisioners = append(n.Provisioners, p.Copy())
+	}
+	copy(n.DependsOn, r.DependsOn)
+	return n
+}
+
 // ResourceLifecycle is used to store the lifecycle tuning parameters
 // to allow customized behavior
 type ResourceLifecycle struct {
@@ -89,6 +110,17 @@ type ResourceLifecycle struct {
 	IgnoreChanges       []string `mapstructure:"ignore_changes"`
 }
 
+// Copy returns a copy of this ResourceLifecycle
+func (r *ResourceLifecycle) Copy() *ResourceLifecycle {
+	n := &ResourceLifecycle{
+		CreateBeforeDestroy: r.CreateBeforeDestroy,
+		PreventDestroy:      r.PreventDestroy,
+		IgnoreChanges:       make([]string, len(r.IgnoreChanges)),
+	}
+	copy(n.IgnoreChanges, r.IgnoreChanges)
+	return n
+}
+
 // Provisioner is a configured provisioner step on a resource.
 type Provisioner struct {
 	Type      string
@@ -96,6 +128,15 @@ type Provisioner struct {
 	ConnInfo  *RawConfig
 }
 
+// Copy returns a copy of this Provisioner
+func (p *Provisioner) Copy() *Provisioner {
+	return &Provisioner{
+		Type:      p.Type,
+		RawConfig: p.RawConfig.Copy(),
+		ConnInfo:  p.ConnInfo.Copy(),
+	}
+}
+
 // Variable is a variable defined within the configuration.
 type Variable struct {
 	Name         string

config/config_test.go

@@ -10,6 +10,52 @@ import (
 // This is the directory where our test fixtures are.
 const fixtureDir = "./test-fixtures"
 
+func TestConfigCopy(t *testing.T) {
+	c := testConfig(t, "copy-basic")
+	rOrig := c.Resources[0]
+	rCopy := rOrig.Copy()
+
+	if rCopy.Name != rOrig.Name {
+		t.Fatalf("Expected names to equal: %q <=> %q", rCopy.Name, rOrig.Name)
+	}
+
+	if rCopy.Type != rOrig.Type {
+		t.Fatalf("Expected types to equal: %q <=> %q", rCopy.Type, rOrig.Type)
+	}
+
+	origCount := rOrig.RawCount.Config()["count"]
+	rCopy.RawCount.Config()["count"] = "5"
+	if rOrig.RawCount.Config()["count"] != origCount {
+		t.Fatalf("Expected RawCount to be copied, but it behaves like a ref!")
+	}
+
+	rCopy.RawConfig.Config()["newfield"] = "hello"
+	if rOrig.RawConfig.Config()["newfield"] == "hello" {
+		t.Fatalf("Expected RawConfig to be copied, but it behaves like a ref!")
+	}
+
+	rCopy.Provisioners = append(rCopy.Provisioners, &Provisioner{})
+	if len(rOrig.Provisioners) == len(rCopy.Provisioners) {
+		t.Fatalf("Expected Provisioners to be copied, but it behaves like a ref!")
+	}
+
+	if rCopy.Provider != rOrig.Provider {
+		t.Fatalf("Expected providers to equal: %q <=> %q",
+			rCopy.Provider, rOrig.Provider)
+	}
+
+	rCopy.DependsOn[0] = "gotchya"
+	if rOrig.DependsOn[0] == rCopy.DependsOn[0] {
+		t.Fatalf("Expected DependsOn to be copied, but it behaves like a ref!")
+	}
+
+	rCopy.Lifecycle.IgnoreChanges[0] = "gotchya"
+	if rOrig.Lifecycle.IgnoreChanges[0] == rCopy.Lifecycle.IgnoreChanges[0] {
+		t.Fatalf("Expected Lifecycle to be copied, but it behaves like a ref!")
+	}
+
+}
+
 func TestConfigCount(t *testing.T) {
 	c := testConfig(t, "count-int")
 	actual, err := c.Resources[0].Count()

config/raw_config.go

@@ -53,7 +53,12 @@ func (r *RawConfig) Copy() *RawConfig {
 	r.lock.Lock()
 	defer r.lock.Unlock()
 
-	result, err := NewRawConfig(r.Raw)
+	newRaw := make(map[string]interface{})
+	for k, v := range r.Raw {
+		newRaw[k] = v
+	}
+
+	result, err := NewRawConfig(newRaw)
 	if err != nil {
 		panic("copy failed: " + err.Error())
 	}

config/test-fixtures/copy-basic/main.tf

@@ -0,0 +1,19 @@
+variable "ref" {
+  default = "foo"
+}
+
+resource "foo" "bar" {
+  depends_on = ["dep"]
+  provider = "foo-west"
+  count = 2
+  attr  = "value"
+  ref   = "${var.ref}"
+
+  provisioner "shell" {
+    inline = "echo"
+  }
+
+  lifecycle {
+    ignore_changes = ["config"]
+  }
+}

terraform/context_plan_test.go

@@ -1701,6 +1701,70 @@ func TestContext2Plan_multiple_taint(t *testing.T) {
 	}
 }
 
+// Fails about 50% of the time before the fix for GH-4982, covers the fix.
+func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) {
+	m := testModule(t, "plan-taint-interpolated-count")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+	s := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo.0": &ResourceState{
+						Type: "aws_instance",
+						Tainted: []*InstanceState{
+							&InstanceState{ID: "bar"},
+						},
+					},
+					"aws_instance.foo.1": &ResourceState{
+						Type:    "aws_instance",
+						Primary: &InstanceState{ID: "bar"},
+					},
+					"aws_instance.foo.2": &ResourceState{
+						Type:    "aws_instance",
+						Primary: &InstanceState{ID: "bar"},
+					},
+				},
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: s,
+	})
+
+	for i := 0; i < 100; i++ {
+		plan, err := ctx.Plan()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		actual := strings.TrimSpace(plan.String())
+		expected := strings.TrimSpace(`
+DIFF:
+
+DESTROY/CREATE: aws_instance.foo.0
+
+STATE:
+
+aws_instance.foo.0: (1 tainted)
+  ID = <not created>
+  Tainted ID 1 = bar
+aws_instance.foo.1:
+  ID = bar
+aws_instance.foo.2:
+  ID = bar
+		`)
+		if actual != expected {
+			t.Fatalf("bad:\n%s", actual)
+		}
+	}
+}
+
 func TestContext2Plan_targeted(t *testing.T) {
 	m := testModule(t, "plan-targeted")
 	p := testProvider("aws")

terraform/graph_config_node_resource.go

@@ -29,6 +29,22 @@ type GraphNodeConfigResource struct {
 	Path []string
 }
 
+func (n *GraphNodeConfigResource) Copy() *GraphNodeConfigResource {
+	ncr := &GraphNodeConfigResource{
+		Resource:    n.Resource.Copy(),
+		DestroyMode: n.DestroyMode,
+		Targets:     make([]ResourceAddress, 0, len(n.Targets)),
+		Path:        make([]string, 0, len(n.Path)),
+	}
+	for _, t := range n.Targets {
+		ncr.Targets = append(ncr.Targets, *t.Copy())
+	}
+	for _, p := range n.Path {
+		ncr.Path = append(ncr.Path, p)
+	}
+	return ncr
+}
+
 func (n *GraphNodeConfigResource) ConfigType() GraphNodeConfigType {
 	return GraphNodeConfigTypeResource
 }
@@ -247,7 +263,7 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo
 	}
 
 	result := &graphNodeResourceDestroy{
-		GraphNodeConfigResource: *n,
+		GraphNodeConfigResource: *n.Copy(),
 		Original:                n,
 	}
 	result.DestroyMode = mode

terraform/resource_address.go

@@ -23,6 +23,21 @@ type ResourceAddress struct {
 	Type         string
 }
 
+// Copy returns a copy of this ResourceAddress
+func (r *ResourceAddress) Copy() *ResourceAddress {
+	n := &ResourceAddress{
+		Path:         make([]string, 0, len(r.Path)),
+		Index:        r.Index,
+		InstanceType: r.InstanceType,
+		Name:         r.Name,
+		Type:         r.Type,
+	}
+	for _, p := range r.Path {
+		n.Path = append(n.Path, p)
+	}
+	return n
+}
+
 func ParseResourceAddress(s string) (*ResourceAddress, error) {
 	matches, err := tokenizeResourceAddress(s)
 	if err != nil {

terraform/test-fixtures/plan-taint-interpolated-count/main.tf

@@ -0,0 +1,7 @@
+variable "count" {
+  default = 3
+}
+
+resource "aws_instance" "foo" {
+  count = "${var.count}"
+}