From 3f72837f4b14595a8561f71591f86f072e0b1b5c Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 5 Feb 2016 17:40:35 -0600 Subject: [PATCH] core: Make copies when creating destroy nodes Fixes an interpolation race that was occurring when a tainted destroy node and a primary destroy node both tried to interpolate a computed count in their config. Since they were sharing a pointer to the _same_ config, depending on how the race played out one of them could catch the config uninterpolated and would then throw a syntax error. The `Copy()` tree implemented for this fix can probably be used elsewhere - basically we should copy the config whenever we drop nodes into the graph - but for now I'm just applying it to the place that fixes this bug. Fixes #4982 - Includes a test covering that race condition. --- config/config.go | 41 ++++++++++++ config/config_test.go | 46 +++++++++++++ config/raw_config.go | 7 +- config/test-fixtures/copy-basic/main.tf | 19 ++++++ terraform/context_plan_test.go | 64 +++++++++++++++++++ terraform/graph_config_node_resource.go | 18 +++++- terraform/resource_address.go | 15 +++++ .../plan-taint-interpolated-count/main.tf | 7 ++ 8 files changed, 215 insertions(+), 2 deletions(-) create mode 100644 config/test-fixtures/copy-basic/main.tf create mode 100644 terraform/test-fixtures/plan-taint-interpolated-count/main.tf diff --git a/config/config.go b/config/config.go index f42a83ff9..8d97d9f3c 100644 --- a/config/config.go +++ b/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 diff --git a/config/config_test.go b/config/config_test.go index a2a15d4bc..ce59d426c 100644 --- a/config/config_test.go +++ b/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() diff --git a/config/raw_config.go b/config/raw_config.go index efd3ab0b5..c897ed387 100644 --- a/config/raw_config.go +++ b/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()) } diff --git a/config/test-fixtures/copy-basic/main.tf b/config/test-fixtures/copy-basic/main.tf new file mode 100644 index 000000000..e0c0d8ddd --- /dev/null +++ b/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"] + } +} diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index c667109be..e9888897a 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1616,6 +1616,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 = + 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") diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 1d3627428..e010991ba 100644 --- a/terraform/graph_config_node_resource.go +++ b/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 diff --git a/terraform/resource_address.go b/terraform/resource_address.go index f7dd94074..d87f645ae 100644 --- a/terraform/resource_address.go +++ b/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 { diff --git a/terraform/test-fixtures/plan-taint-interpolated-count/main.tf b/terraform/test-fixtures/plan-taint-interpolated-count/main.tf new file mode 100644 index 000000000..7f17a9c8e --- /dev/null +++ b/terraform/test-fixtures/plan-taint-interpolated-count/main.tf @@ -0,0 +1,7 @@ +variable "count" { + default = 3 +} + +resource "aws_instance" "foo" { + count = "${var.count}" +}