core: Fix issues with ignore_changes
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
This commit is contained in:
parent
cb8a0549e1
commit
f480ae3430
|
@ -1,6 +1,8 @@
|
||||||
package test
|
package test
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"testing"
|
||||||
|
|
||||||
"github.com/hashicorp/terraform/helper/schema"
|
"github.com/hashicorp/terraform/helper/schema"
|
||||||
"github.com/hashicorp/terraform/terraform"
|
"github.com/hashicorp/terraform/terraform"
|
||||||
)
|
)
|
||||||
|
@ -8,6 +10,12 @@ import (
|
||||||
var testAccProviders map[string]terraform.ResourceProvider
|
var testAccProviders map[string]terraform.ResourceProvider
|
||||||
var testAccProvider *schema.Provider
|
var testAccProvider *schema.Provider
|
||||||
|
|
||||||
|
func TestProvider(t *testing.T) {
|
||||||
|
if err := Provider().(*schema.Provider).InternalValidate(); err != nil {
|
||||||
|
t.Fatalf("err: %s", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func init() {
|
func init() {
|
||||||
testAccProvider = Provider().(*schema.Provider)
|
testAccProvider = Provider().(*schema.Provider)
|
||||||
testAccProviders = map[string]terraform.ResourceProvider{
|
testAccProviders = map[string]terraform.ResourceProvider{
|
||||||
|
|
|
@ -26,6 +26,21 @@ func testResource() *schema.Resource {
|
||||||
Optional: true,
|
Optional: true,
|
||||||
ForceNew: 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 {
|
if _, ok := d.GetOk("required"); !ok {
|
||||||
return fmt.Errorf("Missing attribute 'required', but it's required!")
|
return fmt.Errorf("Missing attribute 'required', but it's required!")
|
||||||
}
|
}
|
||||||
return nil
|
return testResourceRead(d, meta)
|
||||||
}
|
}
|
||||||
|
|
||||||
func testResourceRead(d *schema.ResourceData, meta interface{}) error {
|
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
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -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 {
|
func testAccCheckResourceDestroy(s *terraform.State) error {
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
|
@ -4084,3 +4084,46 @@ aws_instance.ifailedprovisioners: (1 tainted)
|
||||||
t.Fatalf("expected state: \n%s\ngot: \n%s", expected, actual)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
@ -286,6 +286,11 @@ type ResourceAttrDiff struct {
|
||||||
Type DiffAttrType
|
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 {
|
func (d *ResourceAttrDiff) GoString() string {
|
||||||
return fmt.Sprintf("*%#v", *d)
|
return fmt.Sprintf("*%#v", *d)
|
||||||
}
|
}
|
||||||
|
|
|
@ -1,6 +1,7 @@
|
||||||
package terraform
|
package terraform
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"log"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"github.com/hashicorp/terraform/config"
|
"github.com/hashicorp/terraform/config"
|
||||||
|
@ -10,8 +11,9 @@ import (
|
||||||
// attributes if their name matches names provided by the resource's
|
// attributes if their name matches names provided by the resource's
|
||||||
// IgnoreChanges lifecycle.
|
// IgnoreChanges lifecycle.
|
||||||
type EvalIgnoreChanges struct {
|
type EvalIgnoreChanges struct {
|
||||||
Resource *config.Resource
|
Resource *config.Resource
|
||||||
Diff **InstanceDiff
|
Diff **InstanceDiff
|
||||||
|
WasChangeType *DiffChangeType
|
||||||
}
|
}
|
||||||
|
|
||||||
func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
|
func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
|
||||||
|
@ -22,6 +24,22 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
|
||||||
diff := *n.Diff
|
diff := *n.Diff
|
||||||
ignoreChanges := n.Resource.Lifecycle.IgnoreChanges
|
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 _, ignoredName := range ignoreChanges {
|
||||||
for name := range diff.Attributes {
|
for name := range diff.Attributes {
|
||||||
if strings.HasPrefix(name, ignoredName) {
|
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
|
return nil, nil
|
||||||
}
|
}
|
||||||
|
|
|
@ -0,0 +1,7 @@
|
||||||
|
resource "aws_instance" "foo" {
|
||||||
|
required_field = "set"
|
||||||
|
|
||||||
|
lifecycle {
|
||||||
|
ignore_changes = ["required_field"]
|
||||||
|
}
|
||||||
|
}
|
|
@ -372,6 +372,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
|
||||||
var err error
|
var err error
|
||||||
var createNew, tainted bool
|
var createNew, tainted bool
|
||||||
var createBeforeDestroyEnabled bool
|
var createBeforeDestroyEnabled bool
|
||||||
|
var wasChangeType DiffChangeType
|
||||||
seq.Nodes = append(seq.Nodes, &EvalOpFilter{
|
seq.Nodes = append(seq.Nodes, &EvalOpFilter{
|
||||||
Ops: []walkOperation{walkApply, walkDestroy},
|
Ops: []walkOperation{walkApply, walkDestroy},
|
||||||
Node: &EvalSequence{
|
Node: &EvalSequence{
|
||||||
|
@ -393,6 +394,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
|
||||||
return true, EvalEarlyExitError{}
|
return true, EvalEarlyExitError{}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
wasChangeType = diffApply.ChangeType()
|
||||||
diffApply.Destroy = false
|
diffApply.Destroy = false
|
||||||
return true, nil
|
return true, nil
|
||||||
},
|
},
|
||||||
|
@ -439,8 +441,9 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
|
||||||
Output: &diffApply,
|
Output: &diffApply,
|
||||||
},
|
},
|
||||||
&EvalIgnoreChanges{
|
&EvalIgnoreChanges{
|
||||||
Resource: n.Resource,
|
Resource: n.Resource,
|
||||||
Diff: &diffApply,
|
Diff: &diffApply,
|
||||||
|
WasChangeType: &wasChangeType,
|
||||||
},
|
},
|
||||||
|
|
||||||
// Get the saved diff
|
// Get the saved diff
|
||||||
|
|
Loading…
Reference in New Issue