Warn when ignore_changes includes a Computed attribute (#30517)

* ignore_changes attributes must exist in schema Add a test verifying that attempting to add a nonexistent attribute to ignore_changes throws an error. * ignore_changes cannot be used with Computed attrs Return a warning if a Computed attribute is present in ignore_changes, unless the attribute is also Optional. ignore_changes on a non-Optional Computed attribute is a no-op, so the user likely did not want to set this in config. An Optional Computed attribute, however, is still subject to ignore_changes behaviour, since it is possible to make changes in the configuration that Terraform must ignore.
hashicorp · Feb 18, 2022 · 1613747 · 1613747
1 parent 68e70d7
commit 1613747
Show file tree

Hide file tree

Showing 3 changed files with 194 additions and 23 deletions.
diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go
@@ -1146,30 +1146,35 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value) (ct
 func traversalsToPaths(traversals []hcl.Traversal) []cty.Path {
 	paths := make([]cty.Path, len(traversals))
 	for i, traversal := range traversals {
-		path := make(cty.Path, len(traversal))
-		for si, step := range traversal {
-			switch ts := step.(type) {
-			case hcl.TraverseRoot:
-				path[si] = cty.GetAttrStep{
-					Name: ts.Name,
-				}
-			case hcl.TraverseAttr:
-				path[si] = cty.GetAttrStep{
-					Name: ts.Name,
-				}
-			case hcl.TraverseIndex:
-				path[si] = cty.IndexStep{
-					Key: ts.Key,
-				}
-			default:
-				panic(fmt.Sprintf("unsupported traversal step %#v", step))
-			}
-		}
+		path := traversalToPath(traversal)
 		paths[i] = path
 	}
 	return paths
 }
 
+func traversalToPath(traversal hcl.Traversal) cty.Path {
+	path := make(cty.Path, len(traversal))
+	for si, step := range traversal {
+		switch ts := step.(type) {
+		case hcl.TraverseRoot:
+			path[si] = cty.GetAttrStep{
+				Name: ts.Name,
+			}
+		case hcl.TraverseAttr:
+			path[si] = cty.GetAttrStep{
+				Name: ts.Name,
+			}
+		case hcl.TraverseIndex:
+			path[si] = cty.IndexStep{
+				Key: ts.Key,
+			}
+		default:
+			panic(fmt.Sprintf("unsupported traversal step %#v", step))
+		}
+	}
+	return path
+}
+
 func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChangesPath []cty.Path) (cty.Value, tfdiags.Diagnostics) {
 	type ignoreChange struct {
 		// Path is the full path, minus any trailing map index

diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go
@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/internal/addrs"
@@ -383,10 +384,30 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag
 				moreDiags := schema.StaticValidateTraversal(traversal)
 				diags = diags.Append(moreDiags)
 
-				// TODO: we want to notify users that they can't use
-				// ignore_changes for computed attributes, but we don't have an
-				// easy way to correlate the config value, schema and
-				// traversal together.
+				// ignore_changes cannot be used for Computed attributes,
+				// unless they are also Optional.
+				// If the traversal was valid, convert it to a cty.Path and
+				// use that to check whether the Attribute is Computed and
+				// non-Optional.
+				if !diags.HasErrors() {
+					path := traversalToPath(traversal)
+
+					attrSchema := schema.AttributeByPath(path)
+
+					if attrSchema != nil && !attrSchema.Optional && attrSchema.Computed {
+						// ignore_changes uses absolute traversal syntax in config despite
+						// using relative traversals, so we strip the leading "." added by
+						// FormatCtyPath for a better error message.
+						attrDisplayPath := strings.TrimPrefix(tfdiags.FormatCtyPath(path), ".")
+
+						diags = diags.Append(&hcl.Diagnostic{
+							Severity: hcl.DiagWarning,
+							Summary:  "Redundant ignore_changes element",
+							Detail:   fmt.Sprintf("Adding an attribute name to ignore_changes tells Terraform to ignore future changes to the argument in configuration after the object has been created, retaining the value originally configured.\n\nThe attribute %s is decided by the provider alone and therefore there can be no configured value to compare with. Including this attribute in ignore_changes has no effect. Remove the attribute from ignore_changes to quiet this warning.", attrDisplayPath),
+							Subject:  &n.Config.TypeRange,
+						})
+					}
+				}
 			}
 		}
 

diff --git a/internal/terraform/node_resource_validate_test.go b/internal/terraform/node_resource_validate_test.go
@@ -488,3 +488,148 @@ func TestNodeValidatableResource_ValidateResource_invalidDependsOn(t *testing.T)
 		t.Fatalf("wrong error\ngot:  %s\nwant: Message containing %q", got, want)
 	}
 }
+
+func TestNodeValidatableResource_ValidateResource_invalidIgnoreChangesNonexistent(t *testing.T) {
+	mp := simpleMockProvider()
+	mp.ValidateResourceConfigFn = func(req providers.ValidateResourceConfigRequest) providers.ValidateResourceConfigResponse {
+		return providers.ValidateResourceConfigResponse{}
+	}
+
+	// We'll check a _valid_ config first, to make sure we're not failing
+	// for some other reason, and then make it invalid.
+	p := providers.Interface(mp)
+	rc := &configs.Resource{
+		Mode:   addrs.ManagedResourceMode,
+		Type:   "test_object",
+		Name:   "foo",
+		Config: configs.SynthBody("", map[string]cty.Value{}),
+		Managed: &configs.ManagedResource{
+			IgnoreChanges: []hcl.Traversal{
+				{
+					hcl.TraverseAttr{
+						Name: "test_string",
+					},
+				},
+			},
+		},
+	}
+	node := NodeValidatableResource{
+		NodeAbstractResource: &NodeAbstractResource{
+			Addr:             mustConfigResourceAddr("test_foo.bar"),
+			Config:           rc,
+			ResolvedProvider: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`),
+		},
+	}
+
+	ctx := &MockEvalContext{}
+	ctx.installSimpleEval()
+
+	ctx.ProviderSchemaSchema = mp.ProviderSchema()
+	ctx.ProviderProvider = p
+
+	diags := node.validateResource(ctx)
+	if diags.HasErrors() {
+		t.Fatalf("error for supposedly-valid config: %s", diags.ErrWithWarnings())
+	}
+
+	// Now we'll make it invalid by attempting to ignore a nonexistent
+	// attribute.
+	rc.Managed.IgnoreChanges = append(rc.Managed.IgnoreChanges, hcl.Traversal{
+		hcl.TraverseAttr{
+			Name: "nonexistent",
+		},
+	})
+
+	diags = node.validateResource(ctx)
+	if !diags.HasErrors() {
+		t.Fatal("no error for invalid ignore_changes")
+	}
+	if got, want := diags.Err().Error(), "Unsupported attribute: This object has no argument, nested block, or exported attribute named \"nonexistent\""; !strings.Contains(got, want) {
+		t.Fatalf("wrong error\ngot:  %s\nwant: Message containing %q", got, want)
+	}
+}
+
+func TestNodeValidatableResource_ValidateResource_invalidIgnoreChangesComputed(t *testing.T) {
+	// construct a schema with a computed attribute
+	ms := &configschema.Block{
+		Attributes: map[string]*configschema.Attribute{
+			"test_string": {
+				Type:     cty.String,
+				Optional: true,
+			},
+			"computed_string": {
+				Type:     cty.String,
+				Computed: true,
+				Optional: false,
+			},
+		},
+	}
+
+	mp := &MockProvider{
+		GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+			Provider: providers.Schema{Block: ms},
+			ResourceTypes: map[string]providers.Schema{
+				"test_object": providers.Schema{Block: ms},
+			},
+		},
+	}
+
+	mp.ValidateResourceConfigFn = func(req providers.ValidateResourceConfigRequest) providers.ValidateResourceConfigResponse {
+		return providers.ValidateResourceConfigResponse{}
+	}
+
+	// We'll check a _valid_ config first, to make sure we're not failing
+	// for some other reason, and then make it invalid.
+	p := providers.Interface(mp)
+	rc := &configs.Resource{
+		Mode:   addrs.ManagedResourceMode,
+		Type:   "test_object",
+		Name:   "foo",
+		Config: configs.SynthBody("", map[string]cty.Value{}),
+		Managed: &configs.ManagedResource{
+			IgnoreChanges: []hcl.Traversal{
+				{
+					hcl.TraverseAttr{
+						Name: "test_string",
+					},
+				},
+			},
+		},
+	}
+	node := NodeValidatableResource{
+		NodeAbstractResource: &NodeAbstractResource{
+			Addr:             mustConfigResourceAddr("test_foo.bar"),
+			Config:           rc,
+			ResolvedProvider: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`),
+		},
+	}
+
+	ctx := &MockEvalContext{}
+	ctx.installSimpleEval()
+
+	ctx.ProviderSchemaSchema = mp.ProviderSchema()
+	ctx.ProviderProvider = p
+
+	diags := node.validateResource(ctx)
+	if diags.HasErrors() {
+		t.Fatalf("error for supposedly-valid config: %s", diags.ErrWithWarnings())
+	}
+
+	// Now we'll make it invalid by attempting to ignore a computed
+	// attribute.
+	rc.Managed.IgnoreChanges = append(rc.Managed.IgnoreChanges, hcl.Traversal{
+		hcl.TraverseAttr{
+			Name: "computed_string",
+		},
+	})
+
+	diags = node.validateResource(ctx)
+	if diags.HasErrors() {
+		t.Fatalf("got unexpected error: %s", diags.ErrWithWarnings())
+	}
+	if got, want := diags.ErrWithWarnings().Error(), `Redundant ignore_changes element: Adding an attribute name to ignore_changes tells Terraform to ignore future changes to the argument in configuration after the object has been created, retaining the value originally configured.
+
+The attribute computed_string is decided by the provider alone and therefore there can be no configured value to compare with. Including this attribute in ignore_changes has no effect. Remove the attribute from ignore_changes to quiet this warning.`; !strings.Contains(got, want) {
+		t.Fatalf("wrong error\ngot:  %s\nwant: Message containing %q", got, want)
+	}
+}