From 23113ce1c455fb258b428c1805a66eef257b685e Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 9 Oct 2024 16:52:43 -0700 Subject: [PATCH] wip ignore_changes with wildcards --- internal/configs/resource.go | 278 ++++++++++++++-- internal/configs/resource_test.go | 310 ++++++++++++++++++ .../tofu/node_resource_abstract_instance.go | 4 +- 3 files changed, 567 insertions(+), 25 deletions(-) create mode 100644 internal/configs/resource_test.go diff --git a/internal/configs/resource.go b/internal/configs/resource.go index 50e5cb7308..7b675bd558 100644 --- a/internal/configs/resource.go +++ b/internal/configs/resource.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/hcl/v2/json" hcljson "github.com/hashicorp/hcl/v2/json" "github.com/zclconf/go-cty/cty" @@ -70,8 +71,9 @@ type ManagedResource struct { CreateBeforeDestroy bool PreventDestroy bool - IgnoreChanges []hcl.Traversal - IgnoreAllChanges bool + + IgnoreChanges []IgnoreChangesPattern + IgnoreAllChanges bool CreateBeforeDestroySet bool PreventDestroySet bool @@ -218,7 +220,6 @@ func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagno } if attr, exists := lcContent.Attributes["ignore_changes"]; exists { - // ignore_changes can either be a list of relative traversals // or it can be just the keyword "all" to ignore changes to this // resource entirely. @@ -228,6 +229,11 @@ func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagno // versions: // ignore_changes = ["ami", "instance_type"] // ignore_changes = ["*"] + // + // As a special case we also recognize sequences containing the + // "splat" operator [*], even though that isn't actually properly + // supported as a hcl.Traversal usually, to represent matching + // across all elements of a collection. kw := hcl.ExprAsKeyword(attr.Expr) @@ -238,44 +244,42 @@ func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagno exprs, listDiags := hcl.ExprList(attr.Expr) diags = append(diags, listDiags...) - var ignoreAllRange hcl.Range - - for _, expr := range exprs { + // We accept some legacy forms that were valid in v0.11 + // and earlier. Here we'll replace any elements that + // use the legacy forms with their modern equivalents + // before we decode, and also catch the special legacy + // case of ["*"] which always returns an error prompting + // to switch to the "all" keyword instead. + for i, expr := range exprs { // our expr might be the literal string "*", which // we accept as a deprecated way of saying "all". if shimIsIgnoreChangesStar(expr) { r.Managed.IgnoreAllChanges = true - ignoreAllRange = expr.Range() diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid ignore_changes wildcard", Detail: "The [\"*\"] form of ignore_changes wildcard is was deprecated and is now invalid. Use \"ignore_changes = all\" to ignore changes to all attributes.", Subject: attr.Expr.Range().Ptr(), }) + + // The decodeIgnoreChangesPatterns function has no way + // to deal with this invalid case, so we'll null it out + // just to give that function the signal to ignore it. + // (There's no way for a nil expression to appear + // in a valid ignore_changes expression list.) + exprs[i] = nil continue } expr, shimDiags := shimTraversalInString(expr, false) diags = append(diags, shimDiags...) - - traversal, travDiags := hcl.RelTraversalForExpr(expr) - diags = append(diags, travDiags...) - if len(traversal) != 0 { - r.Managed.IgnoreChanges = append(r.Managed.IgnoreChanges, traversal) - } - } - - if r.Managed.IgnoreAllChanges && len(r.Managed.IgnoreChanges) != 0 { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid ignore_changes ruleset", - Detail: "Cannot mix wildcard string \"*\" with non-wildcard references.", - Subject: &ignoreAllRange, - Context: attr.Expr.Range().Ptr(), - }) + exprs[i] = expr } + patterns, hclDiags := decodeIgnoreChangesPatterns(exprs) + diags = diags.Extend(hclDiags) + r.Managed.IgnoreChanges = patterns } } @@ -547,6 +551,234 @@ func decodeDataBlock(block *hcl.Block, override, nested bool) (*Resource, hcl.Di return r, diags } +type IgnoreChangesPattern struct { + // traversal is a slightly-unusual hcl.Traversal that is allowed to + // include the vestigial [hcl.TraverseSplat] type even though HCL + // doesn't actually really support that. (their presence in + // the API is vestigial from when HCL was forked from ZCL, which + // had originally modelled "splat" a little differently.) + // + // Therefore it isn't valid to use the hcl.Traversal API directly + // with the value in this field, and instead callers must use the + // custom methods of this type that are built to deal with the + // splat case along with the normal cases. This is unexported to + // encourage using only the IgnoreChangesPattern methods. + traversal hcl.Traversal +} + +func (p IgnoreChangesPattern) MatchesPath(path cty.Path) bool { + if len(p.traversal) > len(path) { + // A pattern can only match a path that's of equal or + // greater length than the pattern. + return false + } + + for i, patternStep := range p.traversal { + pathStep := path[i] + + switch patternStep := patternStep.(type) { + case hcl.TraverseAttr: + if pathStep, ok := pathStep.(cty.GetAttrStep); ok { + if pathStep.Name != patternStep.Name { + return false + } + } else { + // Only a GetAttrStep can match a TraverseAttr step + return false + } + case hcl.TraverseIndex: + if pathStep, ok := pathStep.(cty.IndexStep); ok { + eq := pathStep.Key.Equals(patternStep.Key) + if !eq.IsKnown() { + // Should not get here, because the indices should always be + // known when we're resolving ignore_changes. + panic("unknown value in path or pattern when resolving ignore_changes") + } + if !eq.True() { + return false + } + } else { + // Only an IndexStep can match a TraverseIndex step + return false + } + case hcl.TraverseSplat: + // We use TraverseSplat as a tricky way to represent "any element in a collection", + // so in this case we'll accept any IndexStep regardless of key. + // We intentionally ignore the "Each" field of TraverseSplat because we're + // using this type in a different way than HCL might've used it (though + // HCL doesn't actually use it at all; it's vestigial from HCL's ancestor, ZCL). + if _, ok := pathStep.(cty.IndexStep); !ok { + // Only an IndexStep can match a TraverseSplat step + return false + } + default: + // No other traverser types should be able to appear + // in an IgnoreChangesPattern, due to how it's constructed. + panic(fmt.Sprintf("parsed ignore_changes pattern contains unsupported step type %T", patternStep)) + } + } + + // If we manage to get here without early return then pattern seems + // to match at least a prefix of the path, which is enough for the + // ignore_changes feature to be active. + return true +} + +// decodeIgnoreChangesPatterns decodes and does static validation of an +// ignore_changes argument in a managed resource lifecycle block. +// This function is only for the case where the argument is a static +// list; the special "all" keyword is handled separately by the +// caller of this function. +// +// This function ignores any nil hcl.Expression values included in +// the given slice, since the caller uses that to "hide" legacy +// pattern forms that it has already handled some other way and +// thus avoid them causing duplicate error messages. +// +// The [hcl.Traversal] values returned by this function can +// potentially include [hcl.TraverseSplat] values even though +// HCL doesn't really properly support those (their presence in +// the API is largely vestigial from when HCL was forked from +// ZCL, which had originally modelled "splat" a little differently.) +// +// In particular that means that it is not supported to call +// [hcl.Traversal.TraverseAbs] or [hcl.Traversal.TraverseRel] +// on the result. The result is useful only for matching against +// [cty.Path] values to decide if a particular attribute in a +// resource instance object matches any of the traversals. +func decodeIgnoreChangesPatterns(exprs []hcl.Expression) (patterns []IgnoreChangesPattern, diags hcl.Diagnostics) { + for _, expr := range exprs { + if expr == nil { + // Presumably our caller already dealt with some legacy oddity + // that was at this position in the list, so we can ignore it. + continue + } + + // Upstream HCL does not allow analyzing an expression containing + // the splat operator as a hcl.Traversal, so we perform our own + // static analysis directly here. The following only supports + // the HCL native and JSON syntaxes, so this might need to be extended + // if we choose to support other HCL syntaxes in future. + + // If we have a JSON expression then we need to translate it into + // the equivalent hclsyntax.Expression first, and then we can share + // the main static analysis logic. + if json.IsJSONExpression(expr) { + // HCL's JSON syntax implementation treats all expressions as + // plain literals when the EvalContext is nil, so the following + // allows us to obtain the raw source code of the given string, + // or determine that it isn't a string at all (which is invalid). + litVal, moreDiags := expr.Value(nil) + diags = diags.Extend(moreDiags) + if moreDiags.HasErrors() { + continue + } + + if litVal.IsNull() || !litVal.IsKnown() || !litVal.Type().Equals(cty.String) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid ignore_changes expression", + Detail: "When using the JSON syntax, ignore_changes elements must be JSON strings containing attribute paths.", + Subject: expr.Range().Ptr(), + }) + continue + } + + // We'll now parse the given expression using the native syntax parser. + // This trick of passing the filename and start range of the original + // expression gives _approximately_ the right answer for source ranges, + // but will be inaccurate if the JSON string contained escape sequences. + // That same caveat applies to similar treatment implemented within + // HCL's json package, so the results aren't any worse than that. + exprSrc := litVal.AsString() + realExpr, moreDiags := hclsyntax.ParseExpression([]byte(exprSrc), expr.Range().Filename, expr.Range().Start) + diags = diags.Extend(moreDiags) + if moreDiags.HasErrors() { + continue + } + expr = realExpr + } + + // By this point we should definitely be holding a hclsyntax.Expression, + // as opposed to any other variant of hcl.Expression. + traversal, moreDiags := ignoreChangesTraversalForExpr(expr.(hclsyntax.Expression)) + diags = diags.Extend(moreDiags) + if len(traversal) != 0 { + patterns = append(patterns, IgnoreChangesPattern{traversal: traversal}) + } + } + + return patterns, diags +} + +func ignoreChangesTraversalForExpr(expr hclsyntax.Expression) (hcl.Traversal, hcl.Diagnostics) { + // Some hclsyntax expression types already know how to interpret themselves + // as relative traversals, so we'll try that first to make sure our behavior + // is a superset of HCL's normal "expression as relative traversal" behavior. + ret, relTravDiags := hcl.RelTraversalForExpr(expr) + if !relTravDiags.HasErrors() { + return ret, nil + } + + // If the built-in behavior didn't work then we'll check for other types that + // we support only for ignore_changes. + var diags hcl.Diagnostics + switch expr := expr.(type) { + case *hclsyntax.SplatExpr: + // In this case we exploit the fact that HCL has a vestigial extra hcl.Traverser + // implemention called hcl.TraverseSplat, which doesn't actually work for + // traversing but is suitable enough for representing a pattern that contains + // a splat operator so we can later match it to a cty.Path. + // + // Splat expressions are pretty "tricky": "Each" is an expression to be evaluated + // for each element of "Source", but somewhere inside it there will be a special + // hclsyntax.AnonSymbolExpr that represents where the current value should be + // substituted. We're not actually substituting values here, so we just arrange + // for *AnonSymbolExpr to get translated into a hcl.TraverseSplat in the other + // case below, and then "eachPattern" will end up with a TraverseSplat at the + // start of it. + sourcePattern, moreDiags := ignoreChangesTraversalForExpr(expr.Source) + diags = diags.Extend(moreDiags) + eachPattern, moreDiags := ignoreChangesTraversalForExpr(expr.Each) + diags = diags.Extend(moreDiags) + ret = append(sourcePattern, eachPattern...) + return ret, diags + case *hclsyntax.RelativeTraversalExpr: + // hcl.RelTraversalForExpr can handle this type only when its Source + // is also compatible with hcl.RelTraversalForExpr, so here we'll also + // allow it for any Source that _this_ function knows how to handle. + sourcePattern, moreDiags := ignoreChangesTraversalForExpr(expr.Source) + diags = diags.Extend(moreDiags) + ret = append(sourcePattern, expr.Traversal...) + return ret, diags + case *hclsyntax.AnonSymbolExpr: + return hcl.Traversal{ + hcl.TraverseSplat{ + // NOTE: HCL includes a vestigial "Each" in this type that was + // presumably originally intended to absorb all of the subsequent + // steps after the splat, but to keep the result flat and easier + // to match we'll just leave that field nil and represent the + // subsequent steps at the top-level. + + // The following is not actually the right SrcRange to use, + // because it covers both the splat's source expression and the + // [*] symbol. We'd rather cover only the [*] symbol but accepting + // this inaccuracy for now for simplicity's sake, since it's + // close enough for downstream error reporting. + SrcRange: expr.SrcRange, + }, + }, nil + default: + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid ignore_changes pattern", + Detail: "An ignore changes pattern must begin with an attribute name, followed by zero or more nested attributes, index operations, or splat operations.", + Subject: expr.Range().Ptr(), + }) + return nil, diags + } +} + // decodeReplaceTriggeredBy decodes and does basic validation of the // replace_triggered_by expressions, ensuring they only contains references to // a single resource, and the only extra variables are count.index or each.key. diff --git a/internal/configs/resource_test.go b/internal/configs/resource_test.go new file mode 100644 index 0000000000..270620d0bb --- /dev/null +++ b/internal/configs/resource_test.go @@ -0,0 +1,310 @@ +package configs + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/hcl/v2/json" + "github.com/zclconf/go-cty-debug/ctydebug" + "github.com/zclconf/go-cty/cty" +) + +func TestDecodeIgnoreChangesPatterns(t *testing.T) { + // This is a narrow test covering only the unexported decodeIgnoreChangesPatterns + // helper function. This function is used only when ignore_changes has a static + // list expression assigned to it. This function does not deal with other + // ignore_changes situations such as the special "all" keyword. + + type TestCase struct { + inputExpr string + want []IgnoreChangesPattern + } + + nativeSyntaxTests := map[string]TestCase{ + "empty": { + `[]`, + nil, + }, + "just root attribute": { + `[ + foo, + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + }, + }, + }, + }, + "two root attributes": { + `[ + foo, + bar, + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + }, + }, + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "bar", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 3, Column: 5, Byte: 15}, + End: hcl.Pos{Line: 3, Column: 8, Byte: 18}, + }, + }, + }, + }, + }, + }, + "attribute in nested object": { + `[ + foo.bar, + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + hcl.TraverseAttr{ + Name: "bar", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 12, Byte: 13}, + }, + }, + }, + }, + }, + }, + "element in nested list": { + `[ + foo[0], + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + hcl.TraverseIndex{ + Key: cty.Zero, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 11, Byte: 12}, + }, + }, + }, + }, + }, + }, + "element in nested map": { + `[ + foo["bar"], + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + hcl.TraverseIndex{ + Key: cty.StringVal("bar"), + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 15, Byte: 16}, + }, + }, + }, + }, + }, + }, + "attribute of element in nested list": { + `[ + foo[0].bar, + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + hcl.TraverseIndex{ + Key: cty.Zero, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 11, Byte: 12}, + }, + }, + hcl.TraverseAttr{ + Name: "bar", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 11, Byte: 12}, + End: hcl.Pos{Line: 2, Column: 15, Byte: 16}, + }, + }, + }, + }, + }, + }, + "attribute of all elements in nested collection": { + `[ + foo[*].bar, + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 5, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + }, + }, + hcl.TraverseSplat{ + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 8, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 11, Byte: 12}, + }, + }, + hcl.TraverseAttr{ + Name: "bar", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 11, Byte: 12}, + End: hcl.Pos{Line: 2, Column: 15, Byte: 16}, + }, + }, + }, + }, + }, + }, + // TODO: If we decide to move forward with this prototype, there are + // plenty more situations to test. + } + jsonSyntaxTests := map[string]TestCase{ + "empty": { + `[]`, + nil, + }, + "attribute of all elements in nested collection": { + `[ + "foo[*].bar" + ]`, + []IgnoreChangesPattern{ + { + traversal: hcl.Traversal{ + hcl.TraverseAttr{ + Name: "foo", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 9, Byte: 6}, + End: hcl.Pos{Line: 2, Column: 12, Byte: 9}, + }, + }, + hcl.TraverseSplat{ + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 12, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 15, Byte: 12}, + }, + }, + hcl.TraverseAttr{ + Name: "bar", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 2, Column: 15, Byte: 12}, + End: hcl.Pos{Line: 2, Column: 19, Byte: 16}, + }, + }, + }, + }, + }, + }, + // TODO: If we decide to move forward with this prototype, there are + // plenty more situations to test. + } + + runTests := func(t *testing.T, tests map[string]TestCase, parse func(*testing.T, string) hcl.Expression) { + for name, test := range tests { + t.Run(name, func(t *testing.T) { + fullExpr := parse(t, test.inputExpr) + exprs, diags := hcl.ExprList(fullExpr) + if diags.HasErrors() { + t.Fatalf("cannot interpret expression as static list:\n%s", diags.Error()) + } + + got, diags := decodeIgnoreChangesPatterns(exprs) + if diags.HasErrors() { + t.Fatalf("invalid patterns:\n%s", diags.Error()) + } + + cmpOpts := cmp.Options{ + cmp.AllowUnexported(IgnoreChangesPattern{}), + cmpopts.IgnoreUnexported( + hcl.TraverseAttr{}, + hcl.TraverseIndex{}, + hcl.TraverseSplat{}, + ), + ctydebug.CmpOptions, + } + if diff := cmp.Diff(test.want, got, cmpOpts); diff != "" { + t.Errorf("wrong result\n%s", diff) + } + }) + } + } + parseNativeSyntax := func(t *testing.T, inputExpr string) hcl.Expression { + fullExpr, diags := hclsyntax.ParseExpression([]byte(inputExpr), "", hcl.InitialPos) + if diags.HasErrors() { + t.Fatalf("invalid expression syntax:\n%s", diags.Error()) + } + return fullExpr + } + parseJSONSyntax := func(t *testing.T, inputExpr string) hcl.Expression { + fullExpr, diags := json.ParseExpression([]byte(inputExpr), "") + if diags.HasErrors() { + t.Fatalf("invalid expression syntax:\n%s", diags.Error()) + } + return fullExpr + } + + t.Run("native_syntax", func(t *testing.T) { + runTests(t, nativeSyntaxTests, parseNativeSyntax) + }) + t.Run("json_syntax", func(t *testing.T) { + runTests(t, jsonSyntaxTests, parseJSONSyntax) + }) +} diff --git a/internal/tofu/node_resource_abstract_instance.go b/internal/tofu/node_resource_abstract_instance.go index c25d2057e1..08e8770c25 100644 --- a/internal/tofu/node_resource_abstract_instance.go +++ b/internal/tofu/node_resource_abstract_instance.go @@ -1198,7 +1198,7 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value, sch return config, nil } - ignoreChanges := traversalsToPaths(n.Config.Managed.IgnoreChanges) + ignoreChanges := n.Config.Managed.IgnoreChanges ignoreAll := n.Config.Managed.IgnoreAllChanges if len(ignoreChanges) == 0 && !ignoreAll { @@ -1274,7 +1274,7 @@ func traversalToPath(traversal hcl.Traversal) cty.Path { return path } -func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChangesPath []cty.Path) (cty.Value, tfdiags.Diagnostics) { +func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChangesPath []configs.IgnoreChangesPattern) (cty.Value, tfdiags.Diagnostics) { type ignoreChange struct { // Path is the full path, minus any trailing map index path cty.Path