Merge pull request #5635 from hashicorp/phinze/fixup-ignore-changes

core: Address some issues with ignore_changes

builtin/providers/test/provider_test.go

@@ -1,6 +1,8 @@
 package test
 
 import (
+	"testing"
+
 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/terraform"
 )
@@ -8,6 +10,12 @@ import (
 var testAccProviders map[string]terraform.ResourceProvider
 var testAccProvider *schema.Provider
 
+func TestProvider(t *testing.T) {
+	if err := Provider().(*schema.Provider).InternalValidate(); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+}
+
 func init() {
 	testAccProvider = Provider().(*schema.Provider)
 	testAccProviders = map[string]terraform.ResourceProvider{

builtin/providers/test/resource.go

@@ -26,6 +26,21 @@ func testResource() *schema.Resource {
 				Optional: true,
 				ForceNew: true,
 			},
+			"optional_computed_map": &schema.Schema{
+				Type:     schema.TypeMap,
+				Optional: true,
+				Computed: true,
+			},
+			"computed_read_only": &schema.Schema{
+				Type:     schema.TypeString,
+				Computed: true,
+				ForceNew: true,
+			},
+			"computed_read_only_force_new": &schema.Schema{
+				Type:     schema.TypeString,
+				Computed: true,
+				ForceNew: true,
+			},
 		},
 	}
 }
@@ -37,10 +52,15 @@ func testResourceCreate(d *schema.ResourceData, meta interface{}) error {
 	if _, ok := d.GetOk("required"); !ok {
 		return fmt.Errorf("Missing attribute 'required', but it's required!")
 	}
-	return nil
+	return testResourceRead(d, meta)
 }
 
 func testResourceRead(d *schema.ResourceData, meta interface{}) error {
+	d.Set("computed_read_only", "value_from_api")
+	d.Set("computed_read_only_force_new", "value_from_api")
+	if _, ok := d.GetOk("optional_computed_map"); !ok {
+		d.Set("optional_computed_map", map[string]string{})
+	}
 	return nil
 }
 

builtin/providers/test/resource_test.go

@@ -27,6 +27,145 @@ 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 TestResource_ignoreChangesMap(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_computed_map {
+		foo = "bar"
+	}
+	lifecycle {
+		ignore_changes = ["optional_computed_map"]
+	}
+}
+				`),
+				Check: func(s *terraform.State) error {
+					return nil
+				},
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required           = "yep"
+	optional_computed_map {
+		foo = "bar"
+		no  = "update"
+	}
+	lifecycle {
+		ignore_changes = ["optional_computed_map"]
+	}
+}
+				`),
+				Check: func(s *terraform.State) error {
+					return nil
+				},
+			},
+		},
+	})
+}
+
 func testAccCheckResourceDestroy(s *terraform.State) error {
 	return nil
 }

terraform/context_apply_test.go

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

terraform/diff.go

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

terraform/eval_ignore_changes.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"log"
 	"strings"
 
 	"github.com/hashicorp/terraform/config"
@@ -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) {
@@ -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) {
@@ -30,5 +48,40 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
 		}
 	}
 
+	// If we are replacing the resource, then we expect there to be a bunch of
+	// extraneous attribute diffs we need to filter out for the other
+	// non-requires-new attributes going from "" -> "configval" or "" ->
+	// "<computed>". Filtering these out allows us to see if we might be able to
+	// skip this diff altogether.
+	if changeType == DiffDestroyCreate {
+		for k, v := range diff.Attributes {
+			if v.Empty() || v.NewComputed {
+				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 after ignore_changes, this diff no longer requires replacement")
+			delete(diff.Attributes, "id")
+			diff.Destroy = false
+		}
+	}
+
 	return nil, nil
 }

terraform/test-fixtures/apply-ignore-changes-create/main.tf

@@ -0,0 +1,7 @@
+resource "aws_instance" "foo" {
+  required_field = "set"
+
+  lifecycle {
+    ignore_changes = ["required_field"]
+  }
+}

terraform/transform_resource.go

@@ -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{
@@ -393,6 +394,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
 							return true, EvalEarlyExitError{}
 						}
 
+						wasChangeType = diffApply.ChangeType()
 						diffApply.Destroy = false
 						return true, nil
 					},
@@ -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