Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Make copies when creating destroy nodes #5026

Merged
merged 1 commit into from
Feb 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -89,13 +110,33 @@ 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
RawConfig *RawConfig
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
Expand Down
46 changes: 46 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
7 changes: 6 additions & 1 deletion config/raw_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down
19 changes: 19 additions & 0 deletions config/test-fixtures/copy-basic/main.tf
Original file line number Diff line number Diff line change
@@ -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"]
}
}
64 changes: 64 additions & 0 deletions terraform/context_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <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")
Expand Down
18 changes: 17 additions & 1 deletion terraform/graph_config_node_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -247,7 +263,7 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo
}

result := &graphNodeResourceDestroy{
GraphNodeConfigResource: *n,
GraphNodeConfigResource: *n.Copy(),
Original: n,
}
result.DestroyMode = mode
Expand Down
15 changes: 15 additions & 0 deletions terraform/resource_address.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions terraform/test-fixtures/plan-taint-interpolated-count/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
variable "count" {
default = 3
}

resource "aws_instance" "foo" {
count = "${var.count}"
}