Skip to content

Commit

Permalink
core: Fix issues with ignore_changes
Browse files Browse the repository at this point in the history
The ignore_changes diff filter was stripping out attributes on Create
but the diff was still making it down to the provider, so Create would
end up missing attributes, causing a full failure if any required
attributes were being ignored.

In addition, any changes that required a replacement of the resource
were causing problems with `ignore_chages`, which didn't properly filter
out the replacement when the triggering attributes were filtered out.

Refs #5627
  • Loading branch information
phinze committed Mar 21, 2016
1 parent cb8a054 commit 1dda0fe
Show file tree
Hide file tree
Showing 6 changed files with 208 additions and 4 deletions.
97 changes: 97 additions & 0 deletions builtin/providers/test/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,103 @@ resource "test_resource" "foo" {
})
}

// Targeted test in TestContext2Apply_ignoreChangesCreate
func TestResource_ignoreChangesRequired(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"
lifecycle {
ignore_changes = ["required"]
}
}
`),
Check: func(s *terraform.State) error {
return nil
},
},
},
})
}

func TestResource_ignoreChangesEmpty(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"
optional_force_new = "one"
lifecycle {
ignore_changes = []
}
}
`),
Check: func(s *terraform.State) error {
return nil
},
},
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource" "foo" {
required = "yep"
optional_force_new = "two"
lifecycle {
ignore_changes = []
}
}
`),
Check: func(s *terraform.State) error {
return nil
},
},
},
})
}

func TestResource_ignoreChangesForceNew(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"
optional_force_new = "one"
lifecycle {
ignore_changes = ["optional_force_new"]
}
}
`),
Check: func(s *terraform.State) error {
return nil
},
},
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource" "foo" {
required = "yep"
optional_force_new = "two"
lifecycle {
ignore_changes = ["optional_force_new"]
}
}
`),
Check: func(s *terraform.State) error {
return nil
},
},
},
})
}

func testAccCheckResourceDestroy(s *terraform.State) error {
return nil
}
43 changes: 43 additions & 0 deletions terraform/context_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4084,3 +4084,46 @@ aws_instance.ifailedprovisioners: (1 tainted)
t.Fatalf("expected state: \n%s\ngot: \n%s", expected, actual)
}
}

// Higher level test exposing the bug this covers in
// TestResource_ignoreChangesRequired
func TestContext2Apply_ignoreChangesCreate(t *testing.T) {
m := testModule(t, "apply-ignore-changes-create")
p := testProvider("aws")
p.ApplyFn = testApplyFn
p.DiffFn = testDiffFn
ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
})

if p, err := ctx.Plan(); err != nil {
t.Fatalf("err: %s", err)
} else {
t.Logf(p.String())
}

state, err := ctx.Apply()
if err != nil {
t.Fatalf("err: %s", err)
}

mod := state.RootModule()
if len(mod.Resources) != 1 {
t.Fatalf("bad: %s", state)
}

actual := strings.TrimSpace(state.String())
// Expect no changes from original state
expected := strings.TrimSpace(`
aws_instance.foo:
ID = foo
required_field = set
type = aws_instance
`)
if actual != expected {
t.Fatalf("bad: \n%s", actual)
}
}
5 changes: 5 additions & 0 deletions terraform/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,11 @@ type ResourceAttrDiff struct {
Type DiffAttrType
}

// Empty returns true if the diff for this attr is neutral
func (d *ResourceAttrDiff) Empty() bool {
return d.Old == d.New && !d.NewComputed && !d.NewRemoved
}

func (d *ResourceAttrDiff) GoString() string {
return fmt.Sprintf("*%#v", *d)
}
Expand Down
53 changes: 51 additions & 2 deletions terraform/eval_ignore_changes.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package terraform

import (
"log"
"strings"

"github.com/hashicorp/terraform/config"
Expand All @@ -10,8 +11,9 @@ import (
// attributes if their name matches names provided by the resource's
// IgnoreChanges lifecycle.
type EvalIgnoreChanges struct {
Resource *config.Resource
Diff **InstanceDiff
Resource *config.Resource
Diff **InstanceDiff
WasChangeType *DiffChangeType
}

func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
Expand All @@ -22,6 +24,22 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
diff := *n.Diff
ignoreChanges := n.Resource.Lifecycle.IgnoreChanges

if len(ignoreChanges) == 0 {
return nil, nil
}

changeType := diff.ChangeType()
// Let the passed in change type pointer override what the diff currently has.
if n.WasChangeType != nil && *n.WasChangeType != DiffInvalid {
changeType = *n.WasChangeType
}

// If we're just creating the resource, we shouldn't alter the
// Diff at all
if changeType == DiffCreate {
return nil, nil
}

for _, ignoredName := range ignoreChanges {
for name := range diff.Attributes {
if strings.HasPrefix(name, ignoredName) {
Expand All @@ -30,5 +48,36 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
}
}

// If we are replacing the resource, we need to check if we filtered out the
// RequiresNew attribute, since that could neutralize the whole diff.
if changeType == DiffDestroyCreate {
for k, v := range diff.Attributes {
if v.Empty() {
delete(diff.Attributes, k)
}
}
}

// Here we emulate the implementation of diff.RequiresNew() with one small
// tweak, we ignore the "id" attribute diff that gets added by EvalDiff,
// since that was added in reaction to RequiresNew being true.
requiresNewAfterIgnores := false
for k, v := range diff.Attributes {
if k == "id" {
continue
}
if v.RequiresNew == true {
requiresNewAfterIgnores = true
}
}

// Here we undo the two reactions to RequireNew in EvalDiff - the "id"
// attribute diff and the Destroy boolean field
if !requiresNewAfterIgnores {
log.Printf("[DEBUG] Removing 'id' diff and setting Destroy to false because diff does not requireNew")
delete(diff.Attributes, "id")
diff.Destroy = false
}

return nil, nil
}
7 changes: 7 additions & 0 deletions terraform/test-fixtures/apply-ignore-changes-create/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
resource "aws_instance" "foo" {
required_field = "set"

lifecycle {
ignore_changes = ["required_field"]
}
}
7 changes: 5 additions & 2 deletions terraform/transform_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
var err error
var createNew, tainted bool
var createBeforeDestroyEnabled bool
var wasChangeType DiffChangeType
seq.Nodes = append(seq.Nodes, &EvalOpFilter{
Ops: []walkOperation{walkApply, walkDestroy},
Node: &EvalSequence{
Expand All @@ -393,6 +394,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
return true, EvalEarlyExitError{}
}

wasChangeType = diffApply.ChangeType()
diffApply.Destroy = false
return true, nil
},
Expand Down Expand Up @@ -439,8 +441,9 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
Output: &diffApply,
},
&EvalIgnoreChanges{
Resource: n.Resource,
Diff: &diffApply,
Resource: n.Resource,
Diff: &diffApply,
WasChangeType: &wasChangeType,
},

// Get the saved diff
Expand Down

0 comments on commit 1dda0fe

Please sign in to comment.