From 161374725c7feb57ee9aa978403b8c6019c6901d Mon Sep 17 00:00:00 2001 From: kmoe <5575356+kmoe@users.noreply.github.com> Date: Fri, 18 Feb 2022 10:38:29 +0000 Subject: [PATCH] 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. --- .../node_resource_abstract_instance.go | 43 +++--- internal/terraform/node_resource_validate.go | 29 +++- .../terraform/node_resource_validate_test.go | 145 ++++++++++++++++++ 3 files changed, 194 insertions(+), 23 deletions(-) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index f362758dc280..fb431df53f34 100644 --- 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 index cd4ec91fe80b..f3eda571ea3e 100644 --- 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 index 3f22cd7e7e24..d0e08c7306d7 100644 --- 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) + } +}