From 197359cc8f2da6f3306c5063b1edef1f02e596ed Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Apr 2022 13:43:15 -0400 Subject: [PATCH 01/15] update hcl for IsJSONExpression --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index e45aeaa3f82a..90d71a47b288 100644 --- a/go.mod +++ b/go.mod @@ -45,7 +45,7 @@ require ( github.com/hashicorp/go-uuid v1.0.2 github.com/hashicorp/go-version v1.3.0 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f - github.com/hashicorp/hcl/v2 v2.11.1 + github.com/hashicorp/hcl/v2 v2.11.2-0.20220408161043-2ef09d129d96 github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2 github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734 github.com/jmespath/go-jmespath v0.4.0 diff --git a/go.sum b/go.sum index c586da6f26fd..9534fae16f63 100644 --- a/go.sum +++ b/go.sum @@ -434,8 +434,8 @@ github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f h1:UdxlrJz4JOnY8W+Db github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f/go.mod h1:oZtUIOe8dh44I2q6ScRibXws4Ajl+d+nod3AaR9vL5w= github.com/hashicorp/hcl/v2 v2.0.0/go.mod h1:oVVDG71tEinNGYCxinCYadcmKU9bglqW9pV3txagJ90= github.com/hashicorp/hcl/v2 v2.3.0/go.mod h1:d+FwDBbOLvpAM3Z6J7gPj/VoAGkNe/gm352ZhjJ/Zv8= -github.com/hashicorp/hcl/v2 v2.11.1 h1:yTyWcXcm9XB0TEkyU/JCRU6rYy4K+mgLtzn2wlrJbcc= -github.com/hashicorp/hcl/v2 v2.11.1/go.mod h1:FwWsfWEjyV/CMj8s/gqAuiviY72rJ1/oayI9WftqcKg= +github.com/hashicorp/hcl/v2 v2.11.2-0.20220408161043-2ef09d129d96 h1:RO/o1b/ZxMUCIgQiKF7qdk0YRwkILQF4KwO39mm9itA= +github.com/hashicorp/hcl/v2 v2.11.2-0.20220408161043-2ef09d129d96/go.mod h1:FwWsfWEjyV/CMj8s/gqAuiviY72rJ1/oayI9WftqcKg= github.com/hashicorp/jsonapi v0.0.0-20210826224640-ee7dae0fb22d h1:9ARUJJ1VVynB176G1HCwleORqCaXm/Vx0uUi0dL26I0= github.com/hashicorp/jsonapi v0.0.0-20210826224640-ee7dae0fb22d/go.mod h1:Yog5+CPEM3c99L1CL2CFCYoSzgWm5vTU58idbRUaLik= github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO+LraFDTW64= From 6eb3264d1a329445912084055fd51c57f871af20 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Apr 2022 14:16:55 -0400 Subject: [PATCH 02/15] parse replace_triggered_by in resource configs --- internal/configs/resource.go | 125 +++++++++++++++++- .../invalid-files/triggered-invalid-each.tf | 7 + .../triggered-invalid-expression.tf | 6 + .../configs/testdata/valid-files/resources.tf | 7 + .../testdata/valid-files/resources.tf.json | 18 +++ 5 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 internal/configs/testdata/invalid-files/triggered-invalid-each.tf create mode 100644 internal/configs/testdata/invalid-files/triggered-invalid-expression.tf create mode 100644 internal/configs/testdata/valid-files/resources.tf.json diff --git a/internal/configs/resource.go b/internal/configs/resource.go index 3d9e72533d1f..3c66f1277de8 100644 --- a/internal/configs/resource.go +++ b/internal/configs/resource.go @@ -6,8 +6,11 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/hcl/v2/hclsyntax" + hcljson "github.com/hashicorp/hcl/v2/json" "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/lang" + "github.com/hashicorp/terraform/internal/tfdiags" ) // Resource represents a "resource" or "data" block in a module or file. @@ -27,6 +30,8 @@ type Resource struct { DependsOn []hcl.Traversal + TriggersReplacement []hcl.Expression + // Managed is populated only for Mode = addrs.ManagedResourceMode, // containing the additional fields that apply to managed resources. // For all other resource modes, this field is nil. @@ -177,6 +182,13 @@ func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagno r.Managed.PreventDestroySet = true } + if attr, exists := lcContent.Attributes["replace_triggered_by"]; exists { + exprs, hclDiags := decodeReplaceTriggeredBy(attr.Expr) + diags = diags.Extend(hclDiags) + + r.TriggersReplacement = append(r.TriggersReplacement, exprs...) + } + if attr, exists := lcContent.Attributes["ignore_changes"]; exists { // ignore_changes can either be a list of relative traversals @@ -237,7 +249,6 @@ func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagno } } - } for _, block := range lcContent.Blocks { @@ -481,6 +492,115 @@ func decodeDataBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagnostic return r, 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. +func decodeReplaceTriggeredBy(expr hcl.Expression) ([]hcl.Expression, hcl.Diagnostics) { + // Since we are manually parsing the replace_triggered_by argument, we + // need to specially handle json configs, in which case the values will + // be json strings rather than hcl. To simplify parsing however we will + // decode the individual list elements, rather than the entire expression. + isJSON := hcljson.IsJSONExpression(expr) + + exprs, diags := hcl.ExprList(expr) + + for i, expr := range exprs { + if isJSON { + // We can abuse the hcl json api and rely on the fact that calling + // Value on a json expression with no EvalContext will return the + // raw string. We can then parse that as normal hcl syntax, and + // continue with the decoding. + v, ds := expr.Value(nil) + diags = diags.Extend(ds) + if diags.HasErrors() { + continue + } + + expr, ds = hclsyntax.ParseExpression([]byte(v.AsString()), "", expr.Range().Start) + diags = diags.Extend(ds) + if diags.HasErrors() { + continue + } + // make sure to swap out the expression we're returning too + exprs[i] = expr + } + + refs, refDiags := lang.ReferencesInExpr(expr) + for _, diag := range refDiags { + severity := hcl.DiagError + if diag.Severity() == tfdiags.Warning { + severity = hcl.DiagWarning + } + + desc := diag.Description() + + diags = append(diags, &hcl.Diagnostic{ + Severity: severity, + Summary: desc.Summary, + Detail: desc.Detail, + Subject: expr.Range().Ptr(), + }) + } + + if refDiags.HasErrors() { + continue + } + + resourceCount := 0 + for _, ref := range refs { + switch sub := ref.Subject.(type) { + case addrs.Resource, addrs.ResourceInstance: + resourceCount++ + + case addrs.ForEachAttr: + if sub.Name != "key" { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid each reference in replace_triggered_by expression", + Detail: "Only each.key may be used in replace_triggered_by.", + Subject: expr.Range().Ptr(), + }) + } + case addrs.CountAttr: + if sub.Name != "index" { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid count reference in replace_triggered_by expression", + Detail: "Only count.index may be used in replace_triggered_by.", + Subject: expr.Range().Ptr(), + }) + } + default: + // everything else should be simple traversals + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid reference in replace_triggered_by expression", + Detail: "Only resources, count.index, and each.key may be used in replace_triggered_by.", + Subject: expr.Range().Ptr(), + }) + } + } + + switch { + case resourceCount == 0: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid replace_triggered_by expression", + Detail: "Missing resource reference in replace_triggered_by_expression.", + Subject: expr.Range().Ptr(), + }) + case resourceCount > 1: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid replace_triggered_by expression", + Detail: "Multiple resource references in replace_triggered_by_expression.", + Subject: expr.Range().Ptr(), + }) + } + } + return exprs, diags +} + type ProviderConfigRef struct { Name string NameRange hcl.Range @@ -640,6 +760,9 @@ var resourceLifecycleBlockSchema = &hcl.BodySchema{ { Name: "ignore_changes", }, + { + Name: "replace_triggered_by", + }, }, Blocks: []hcl.BlockHeaderSchema{ {Type: "precondition"}, diff --git a/internal/configs/testdata/invalid-files/triggered-invalid-each.tf b/internal/configs/testdata/invalid-files/triggered-invalid-each.tf new file mode 100644 index 000000000000..0649ecc96613 --- /dev/null +++ b/internal/configs/testdata/invalid-files/triggered-invalid-each.tf @@ -0,0 +1,7 @@ +resource "test_resource" "a" { + for_each = var.input + lifecycle { + // cannot use each.val + replace_triggered_by = [ test_resource.b[each.val] ] + } +} diff --git a/internal/configs/testdata/invalid-files/triggered-invalid-expression.tf b/internal/configs/testdata/invalid-files/triggered-invalid-expression.tf new file mode 100644 index 000000000000..8bf8af5bb0b8 --- /dev/null +++ b/internal/configs/testdata/invalid-files/triggered-invalid-expression.tf @@ -0,0 +1,6 @@ +resource "test_resource" "a" { + count = 1 + lifecycle { + replace_triggered_by = [ not_a_reference ] + } +} diff --git a/internal/configs/testdata/valid-files/resources.tf b/internal/configs/testdata/valid-files/resources.tf index 53fb745335bb..aab038ea8c3c 100644 --- a/internal/configs/testdata/valid-files/resources.tf +++ b/internal/configs/testdata/valid-files/resources.tf @@ -25,6 +25,7 @@ resource "aws_security_group" "firewall" { } resource "aws_instance" "web" { + count = 2 ami = "ami-1234" security_groups = [ "foo", @@ -40,3 +41,9 @@ resource "aws_instance" "web" { aws_security_group.firewall, ] } + +resource "aws_instance" "depends" { + lifecycle { + replace_triggered_by = [ aws_instance.web[1], aws_security_group.firewall.id ] + } +} diff --git a/internal/configs/testdata/valid-files/resources.tf.json b/internal/configs/testdata/valid-files/resources.tf.json new file mode 100644 index 000000000000..627963d0baf6 --- /dev/null +++ b/internal/configs/testdata/valid-files/resources.tf.json @@ -0,0 +1,18 @@ +{ + "resource": { + "test_object": { + "a": { + "count": 1, + "test_string": "new" + }, + "b": { + "count": 1, + "lifecycle": { + "replace_triggered_by": [ + "test_object.a[count.index].test_string" + ] + } + } + } + } +} From 8b4c89bdaff4e2f45af476144bd33f923fe18934 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Apr 2022 15:24:40 -0400 Subject: [PATCH 03/15] evaluate replace_triggered_by expressions Evaluate the expressions stored in replace_triggered_by into the *addrs.Reference needed to lookup changes in the plan. --- internal/terraform/evaluate_triggers.go | 143 +++++++++++++++++++ internal/terraform/evaluate_triggers_test.go | 94 ++++++++++++ 2 files changed, 237 insertions(+) create mode 100644 internal/terraform/evaluate_triggers.go create mode 100644 internal/terraform/evaluate_triggers_test.go diff --git a/internal/terraform/evaluate_triggers.go b/internal/terraform/evaluate_triggers.go new file mode 100644 index 000000000000..31fd80e16b2d --- /dev/null +++ b/internal/terraform/evaluate_triggers.go @@ -0,0 +1,143 @@ +package terraform + +import ( + "strings" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/instances" + "github.com/hashicorp/terraform/internal/tfdiags" + "github.com/zclconf/go-cty/cty" +) + +func evalReplaceTriggeredByExpr(expr hcl.Expression, keyData instances.RepetitionData) (*addrs.Reference, tfdiags.Diagnostics) { + var ref *addrs.Reference + var diags tfdiags.Diagnostics + + traversal, diags := triggersExprToTraversal(expr, keyData) + if diags.HasErrors() { + return nil, diags + } + + // We now have a static traversal, so we can just turn it into an addrs.Reference. + ref, ds := addrs.ParseRef(traversal) + diags = diags.Append(ds) + + return ref, diags +} + +// trggersExprToTraversal takes an hcl expression limited to the syntax allowed +// in replace_triggered_by, and converts it to a static traversal. The +// RepetitionData contains the data necessary to evaluate the only allowed +// variables in the expression, count.index and each.key. +func triggersExprToTraversal(expr hcl.Expression, keyData instances.RepetitionData) (hcl.Traversal, tfdiags.Diagnostics) { + var trav hcl.Traversal + var diags tfdiags.Diagnostics + + switch e := expr.(type) { + case *hclsyntax.RelativeTraversalExpr: + t, d := triggersExprToTraversal(e.Source, keyData) + diags = diags.Append(d) + trav = append(trav, t...) + trav = append(trav, e.Traversal...) + + case *hclsyntax.ScopeTraversalExpr: + // a static reference, we can just append the traversal + trav = append(trav, e.Traversal...) + + case *hclsyntax.IndexExpr: + // Get the collection from the index expression + t, d := triggersExprToTraversal(e.Collection, keyData) + diags = diags.Append(d) + if diags.HasErrors() { + return nil, diags + } + trav = append(trav, t...) + + // The index key is the only place where we could have variables that + // reference count and each, so we need to parse those independently. + idx, hclDiags := parseIndexKeyExpr(e.Key, keyData) + diags = diags.Append(hclDiags) + + trav = append(trav, idx) + + default: + // Something unexpected got through config validation. We're not sure + // what it is, but we'll point it out in the diagnostics for the user + // to fix. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid replace_triggered_by expression", + Detail: "Unexpected expression found in replace_triggered_by.", + Subject: e.Range().Ptr(), + }) + } + + return trav, diags +} + +// parseIndexKeyExpr takes an hcl.Expression and parses it as an index key, while +// evaluating any references to count.index or each.key. +func parseIndexKeyExpr(expr hcl.Expression, keyData instances.RepetitionData) (hcl.TraverseIndex, hcl.Diagnostics) { + idx := hcl.TraverseIndex{ + SrcRange: expr.Range(), + } + + trav, diags := hcl.RelTraversalForExpr(expr) + if diags.HasErrors() { + return idx, diags + } + + keyParts := []string{} + + for _, t := range trav { + attr, ok := t.(hcl.TraverseAttr) + if !ok { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid index expression", + Detail: "Only constant values, count.index or each.key are allowed in index expressions.", + Subject: expr.Range().Ptr(), + }) + return idx, diags + } + keyParts = append(keyParts, attr.Name) + } + + switch strings.Join(keyParts, ".") { + case "count.index": + if keyData.CountIndex == cty.NilVal { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Reference to "count" in non-counted context`, + Detail: `The "count" object can only be used in "resource" blocks when the "count" argument is set.`, + Subject: expr.Range().Ptr(), + }) + } + idx.Key = keyData.CountIndex + + case "each.key": + if keyData.EachKey == cty.NilVal { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Reference to "each" in context without for_each`, + Detail: `The "each" object can be used only in "resource" blocks when the "for_each" argument is set.`, + Subject: expr.Range().Ptr(), + }) + } + idx.Key = keyData.EachKey + default: + // Something may have slipped through validation, probably from a json + // configuration. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid index expression", + Detail: "Only constant values, count.index or each.key are allowed in index expressions.", + Subject: expr.Range().Ptr(), + }) + } + + return idx, diags + +} diff --git a/internal/terraform/evaluate_triggers_test.go b/internal/terraform/evaluate_triggers_test.go new file mode 100644 index 000000000000..d51b1c2be6b6 --- /dev/null +++ b/internal/terraform/evaluate_triggers_test.go @@ -0,0 +1,94 @@ +package terraform + +import ( + "testing" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/instances" + "github.com/zclconf/go-cty/cty" +) + +func TestEvalReplaceTriggeredBy(t *testing.T) { + tests := map[string]struct { + // Raw config expression from within replace_triggered_by list. + // If this does not contains any count or each references, it should + // directly parse into the same *addrs.Reference. + expr string + + // If the expression contains count or each, then we need to add + // repetition data, and the static string to parse into the desired + // *addrs.Reference + repData instances.RepetitionData + reference string + }{ + "single resource": { + expr: "test_resource.a", + }, + + "resource instance attr": { + expr: "test_resource.a.attr", + }, + + "resource instance index attr": { + expr: "test_resource.a[0].attr", + }, + + "resource instance count": { + expr: "test_resource.a[count.index]", + repData: instances.RepetitionData{ + CountIndex: cty.NumberIntVal(0), + }, + reference: "test_resource.a[0]", + }, + "resource instance for_each": { + expr: "test_resource.a[each.key].attr", + repData: instances.RepetitionData{ + EachKey: cty.StringVal("k"), + }, + reference: `test_resource.a["k"].attr`, + }, + "resource instance for_each map attr": { + expr: "test_resource.a[each.key].attr[each.key]", + repData: instances.RepetitionData{ + EachKey: cty.StringVal("k"), + }, + reference: `test_resource.a["k"].attr["k"]`, + }, + } + + for name, tc := range tests { + pos := hcl.Pos{Line: 1, Column: 1} + t.Run(name, func(t *testing.T) { + expr, hclDiags := hclsyntax.ParseExpression([]byte(tc.expr), "", pos) + if hclDiags.HasErrors() { + t.Fatal(hclDiags) + } + + got, diags := evalReplaceTriggeredByExpr(expr, tc.repData) + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + want := tc.reference + if want == "" { + want = tc.expr + } + + // create the desired reference + traversal, travDiags := hclsyntax.ParseTraversalAbs([]byte(want), "", pos) + if travDiags.HasErrors() { + t.Fatal(travDiags) + } + ref, diags := addrs.ParseRef(traversal) + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + if got.DisplayString() != ref.DisplayString() { + t.Fatalf("expected %q: got %q", ref.DisplayString(), got.DisplayString()) + } + }) + } +} From 4d43d6f699f28d674f83e272b815c99ed4458d0b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 12 Apr 2022 15:54:09 -0400 Subject: [PATCH 04/15] Use the EvalContext to lookup trigger changes The EvalContext is the only place with all the information to be able to complete the evaluation of the replace_triggered_by expressions. These need to be evaluated into a reference, which is then looked up in the pending changes which the context has access too. On top of needing the plan changes, we also need access to all providers and schemas to decode the changes if we need to traverse the resource values for individual attributes. --- internal/terraform/eval_context.go | 5 + internal/terraform/eval_context_builtin.go | 108 ++++++++++++++++++++- internal/terraform/eval_context_mock.go | 4 + 3 files changed, 116 insertions(+), 1 deletion(-) diff --git a/internal/terraform/eval_context.go b/internal/terraform/eval_context.go index 161ebeb1c049..165500ddae23 100644 --- a/internal/terraform/eval_context.go +++ b/internal/terraform/eval_context.go @@ -117,6 +117,11 @@ type EvalContext interface { // evaluating. Set this to nil if the "self" object should not be available. EvaluateExpr(expr hcl.Expression, wantType cty.Type, self addrs.Referenceable) (cty.Value, tfdiags.Diagnostics) + // EvaluateReplaceTriggeredBy takes the raw reference expression from the + // config, and returns the evaluated *addrs.Reference along with a boolean + // indicating if that reference forces replacement. + EvaluateReplaceTriggeredBy(expr hcl.Expression, repData instances.RepetitionData) (*addrs.Reference, bool, tfdiags.Diagnostics) + // EvaluationScope returns a scope that can be used to evaluate reference // addresses in this context. EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index c73226841c9e..46c101b2a04a 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -282,7 +282,113 @@ func (ctx *BuiltinEvalContext) EvaluateExpr(expr hcl.Expression, wantType cty.Ty return scope.EvalExpr(expr, wantType) } -func (ctx *BuiltinEvalContext) EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope { +func (ctx *BuiltinEvalContext) EvaluateReplaceTriggeredBy(expr hcl.Expression, repData instances.RepetitionData) (*addrs.Reference, bool, tfdiags.Diagnostics) { + + // get the reference to lookup changes in the plan + ref, diags := evalReplaceTriggeredByExpr(expr, repData) + if diags.HasErrors() { + return nil, false, diags + } + + var changes []*plans.ResourceInstanceChangeSrc + // store the address once we get it for validation + var resourceAddr addrs.Resource + + // The reference is either a resource or resource instance + switch sub := ref.Subject.(type) { + case addrs.Resource: + resourceAddr = sub + rc := sub.InModule(ctx.Path().Module()) + changes = ctx.Changes().GetChangesForConfigResource(rc) + // FIXME: Needs to be restricted to the same module! + // The other caller actually needs the same condition, so we can + // change this method to cover both use cases. + case addrs.ResourceInstance: + resourceAddr = sub.ContainingResource() + rc := sub.Absolute(ctx.Path()) + change := ctx.Changes().GetResourceInstanceChange(rc, states.CurrentGen) + if change != nil { + // we'll generate an error below if there was no change + changes = append(changes, change) + } + } + + // Do some validation to make sure we are expecting a change at all + cfg := ctx.Evaluator.Config.Descendent(ctx.Path().Module()) + resCfg := cfg.Module.ResourceByAddr(resourceAddr) + if resCfg == nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Reference to undeclared resource`, + Detail: fmt.Sprintf(`A resource %s has not been declared in %s`, ref.Subject, moduleDisplayAddr(ctx.Path())), + Subject: expr.Range().Ptr(), + }) + return nil, false, diags + } + + if len(changes) == 0 { + // If the resource is valid there should always be at least one change. + diags = diags.Append(fmt.Errorf("no change found for %s in %s", ref.Subject, moduleDisplayAddr(ctx.Path()))) + return nil, false, diags + } + + // If we don't have a traversal beyond the resource, then we can just look + // for any change. + if len(ref.Remaining) == 0 { + for _, c := range changes { + if c.ChangeSrc.Action != plans.NoOp { + return ref, true, diags + } + } + + // no change triggered + return nil, false, diags + } + + // This must be an instances to have a remaining traversal, which means a + // single change. + change := changes[0] + + // Since we have a traversal after the resource reference, we will need to + // decode the changes, which means we need a schema. + providerAddr := change.ProviderAddr + schema, err := ctx.ProviderSchema(providerAddr) + if err != nil { + diags = diags.Append(err) + return nil, false, diags + } + + resAddr := change.Addr.ContainingResource().Resource + resSchema, _ := schema.SchemaForResourceType(resAddr.Mode, resAddr.Type) + ty := resSchema.ImpliedType() + + before, err := change.ChangeSrc.Before.Decode(ty) + if err != nil { + diags = diags.Append(err) + return nil, false, diags + } + + after, err := change.ChangeSrc.After.Decode(ty) + if err != nil { + diags = diags.Append(err) + return nil, false, diags + } + + path := traversalToPath(ref.Remaining) + attrBefore, _ := path.Apply(before) + attrAfter, _ := path.Apply(after) + + if attrBefore == cty.NilVal || attrAfter == cty.NilVal { + replace := attrBefore != attrAfter + return ref, replace, diags + } + + replace := !attrBefore.RawEquals(attrAfter) + + return ref, replace, diags +} + +func (ctx *BuiltinEvalContext) EvaluationScope(self addrs.Referenceable, keyData instances.RepetitionData) *lang.Scope { if !ctx.pathSet { panic("context path not set") } diff --git a/internal/terraform/eval_context_mock.go b/internal/terraform/eval_context_mock.go index 252530d7c704..508d98198522 100644 --- a/internal/terraform/eval_context_mock.go +++ b/internal/terraform/eval_context_mock.go @@ -261,6 +261,10 @@ func (c *MockEvalContext) EvaluateExpr(expr hcl.Expression, wantType cty.Type, s return c.EvaluateExprResult, c.EvaluateExprDiags } +func (c *MockEvalContext) EvaluateReplaceTriggeredBy(hcl.Expression, instances.RepetitionData) (*addrs.Reference, bool, tfdiags.Diagnostics) { + return nil, false, nil +} + // installSimpleEval is a helper to install a simple mock implementation of // both EvaluateBlock and EvaluateExpr into the receiver. // From 4f2195af2b666ab078d03e4197185adb77b55e25 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 12 Apr 2022 15:57:16 -0400 Subject: [PATCH 05/15] collect references from replace_triggered_by The replace_triggered_by expressions create edges in the graph, so must be returned in the References method. --- internal/terraform/node_resource_abstract.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/terraform/node_resource_abstract.go b/internal/terraform/node_resource_abstract.go index 4903cdbe3732..1a5bd1ff5c21 100644 --- a/internal/terraform/node_resource_abstract.go +++ b/internal/terraform/node_resource_abstract.go @@ -143,12 +143,17 @@ func (n *NodeAbstractResource) References() []*addrs.Reference { refs, _ = lang.ReferencesInExpr(c.ForEach) result = append(result, refs...) + for _, expr := range c.TriggersReplacement { + refs, _ = lang.ReferencesInExpr(expr) + result = append(result, refs...) + } + // ReferencesInBlock() requires a schema if n.Schema != nil { refs, _ = lang.ReferencesInBlock(c.Config, n.Schema) + result = append(result, refs...) } - result = append(result, refs...) if c.Managed != nil { if c.Managed.Connection != nil { refs, _ = lang.ReferencesInBlock(c.Managed.Connection.Config, connectionBlockSupersetSchema) From 7598665c90707a83fb8ceb424611426d5d044593 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 12 Apr 2022 16:01:20 -0400 Subject: [PATCH 06/15] check for replacement via replace_triggered_by Check for triggered resource replacement in the plan. While the functionality of the feature works here, we ill want to follow up with a way to indicate in the plan _why_ the resource was replaced. --- .../terraform/node_resource_plan_instance.go | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index 4b1da5943847..29d537c9ee19 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -5,9 +5,11 @@ import ( "log" "sort" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" + "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/addrs" ) @@ -192,6 +194,23 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) // Plan the instance, unless we're in the refresh-only mode if !n.skipPlanChanges { + + // add this instance to n.forceReplace if replacement is triggered by + // another change + repData := instances.RepetitionData{} + switch k := addr.Resource.Key.(type) { + case addrs.IntKey: + repData.CountIndex = k.Value() + case addrs.StringKey: + repData.EachKey = k.Value() + repData.EachValue = cty.DynamicVal + } + + diags = diags.Append(n.replaceTriggered(ctx, repData)) + if diags.HasErrors() { + return diags + } + change, instancePlanState, repeatData, planDiags := n.plan( ctx, change, instanceRefreshState, n.ForceCreateBeforeDestroy, n.forceReplace, ) @@ -299,6 +318,33 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } +// replaceTriggered checks if this instance needs to be replace due to a change +// in a replace_triggered_by reference. If replacement is required, the +// instance address is added to forceReplace +func (n *NodePlannableResourceInstance) replaceTriggered(ctx EvalContext, repData instances.RepetitionData) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + for _, expr := range n.Config.TriggersReplacement { + _, replace, evalDiags := ctx.EvaluateReplaceTriggeredBy(expr, repData) + diags = diags.Append(evalDiags) + if diags.HasErrors() { + continue + } + + if replace { + // FIXME: forceReplace accomplishes the same goal, however we will + // want a new way to signal why this resource was replaced in the + // plan. + // EvalauteReplaceTriggeredBy returns a reference to store + // somewhere for this purpose too. + n.forceReplace = append(n.forceReplace, n.Addr) + } + + } + + return diags +} + // mergeDeps returns the union of 2 sets of dependencies func mergeDeps(a, b []addrs.ConfigResource) []addrs.ConfigResource { switch { From 6670b71a2ee6748c38aa4cb6892184aa48e1ded9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 12 Apr 2022 16:03:45 -0400 Subject: [PATCH 07/15] context test demonstrating replace_triggered_by --- internal/terraform/context_plan2_test.go | 67 ++++++++++++++++++++++ internal/terraform/eval_context_builtin.go | 9 +++ 2 files changed, 76 insertions(+) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 63fa95cdbe8e..3b44620a27c1 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2964,6 +2964,73 @@ output "a" { } } +func TestContext2Plan_triggeredBy(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { + count = 1 + test_string = "new" +} +resource "test_object" "b" { + count = 1 + test_string = test_object.a[count.index].test_string + lifecycle { + # the change to test_string in the other resource should trigger replacement + replace_triggered_by = [ test_object.a[count.index].test_string ] + } +} +`, + }) + + p := simpleMockProvider() + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a[0]"), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"test_string":"old"}`), + Status: states.ObjectReady, + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + s.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.b[0]"), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{}`), + Status: states.ObjectReady, + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + }) + + plan, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + }) + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } + for _, c := range plan.Changes.Resources { + switch c.Addr.String() { + case "test_object.a[0]": + if c.Action != plans.Update { + t.Fatalf("unexpected %s change for %s\n", c.Action, c.Addr) + } + case "test_object.b[0]": + if c.Action != plans.DeleteThenCreate { + t.Fatalf("unexpected %s change for %s\n", c.Action, c.Addr) + } + default: + t.Fatal("unexpected change", c.Addr, c.Action) + } + } +} + func TestContext2Plan_dataSchemaChange(t *testing.T) { // We can't decode the prior state when a data source upgrades the schema // in an incompatible way. Since prior state for data sources is purely diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index 46c101b2a04a..7aad763a385b 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -349,6 +349,15 @@ func (ctx *BuiltinEvalContext) EvaluateReplaceTriggeredBy(expr hcl.Expression, r // single change. change := changes[0] + // Make sure the change is actionable. A create or delete action will have + // a change in value, but are not valid for our purposes here. + switch change.ChangeSrc.Action { + case plans.Update, plans.DeleteThenCreate, plans.CreateThenDelete: + // OK + default: + return nil, false, diags + } + // Since we have a traversal after the resource reference, we will need to // decode the changes, which means we need a schema. providerAddr := change.ProviderAddr From fb6fcf783b2b5fd2086116fd2053d9fa14334833 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 15 Apr 2022 13:08:49 -0400 Subject: [PATCH 08/15] Fix replace_triggered_by criteria Only immediate changes to the resource are considered. --- internal/terraform/eval_context_builtin.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index 7aad763a385b..0678f40eeebd 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -336,7 +336,9 @@ func (ctx *BuiltinEvalContext) EvaluateReplaceTriggeredBy(expr hcl.Expression, r // for any change. if len(ref.Remaining) == 0 { for _, c := range changes { - if c.ChangeSrc.Action != plans.NoOp { + switch c.ChangeSrc.Action { + // Only immediate changes to the resource will trigger replacement. + case plans.Update, plans.DeleteThenCreate, plans.CreateThenDelete: return ref, true, diags } } From 91121aa856c226016441c197ba648729cf15c646 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 15 Apr 2022 13:45:28 -0400 Subject: [PATCH 09/15] limit replace_triggered_by to same module instance replace_triggered_by references are scoped to the current module, so we need to filter changes for the current module instance. Rather than creating a ConfigResource and filtering the result, make a Changes.InstancesForAbsResource method to get only the AbsResource changes. --- internal/plans/changes.go | 15 ++++++++++++++ internal/plans/changes_sync.go | 23 +++++++++++++++++++++- internal/terraform/eval_context_builtin.go | 7 ++----- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/internal/plans/changes.go b/internal/plans/changes.go index ae76a45dc09b..faaed80b2973 100644 --- a/internal/plans/changes.go +++ b/internal/plans/changes.go @@ -61,6 +61,21 @@ func (c *Changes) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInst } +// InstancesForAbsResource returns the planned change for the current objects +// of the resource instances of the given address, if any. Returns nil if no +// changes are planned. +func (c *Changes) InstancesForAbsResource(addr addrs.AbsResource) []*ResourceInstanceChangeSrc { + var changes []*ResourceInstanceChangeSrc + for _, rc := range c.Resources { + resAddr := rc.Addr.ContainingResource() + if resAddr.Equal(addr) && rc.DeposedKey == states.NotDeposed { + changes = append(changes, rc) + } + } + + return changes +} + // InstancesForConfigResource returns the planned change for the current objects // of the resource instances of the given address, if any. Returns nil if no // changes are planned. diff --git a/internal/plans/changes_sync.go b/internal/plans/changes_sync.go index 739e91994d39..7ef7cd4bbf8b 100644 --- a/internal/plans/changes_sync.go +++ b/internal/plans/changes_sync.go @@ -80,7 +80,7 @@ func (cs *ChangesSync) GetResourceInstanceChange(addr addrs.AbsResourceInstance, panic(fmt.Sprintf("unsupported generation value %#v", gen)) } -// GetChangesForConfigResource searched the set of resource instance +// GetChangesForConfigResource searches the set of resource instance // changes and returns all changes related to a given configuration address. // This is be used to find possible changes related to a configuration // reference. @@ -103,6 +103,27 @@ func (cs *ChangesSync) GetChangesForConfigResource(addr addrs.ConfigResource) [] return changes } +// GetChangesForAbsResource searches the set of resource instance +// changes and returns all changes related to a given configuration address. +// +// If no such changes exist, nil is returned. +// +// The returned objects are a deep copy of the change recorded in the plan, so +// callers may mutate them although it's generally better (less confusing) to +// treat planned changes as immutable after they've been initially constructed. +func (cs *ChangesSync) GetChangesForAbsResource(addr addrs.AbsResource) []*ResourceInstanceChangeSrc { + if cs == nil { + panic("GetChangesForAbsResource on nil ChangesSync") + } + cs.lock.Lock() + defer cs.lock.Unlock() + var changes []*ResourceInstanceChangeSrc + for _, c := range cs.changes.InstancesForAbsResource(addr) { + changes = append(changes, c.DeepCopy()) + } + return changes +} + // RemoveResourceInstanceChange searches the set of resource instance changes // for one matching the given address and generation, and removes it from the // set if it exists. diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index 0678f40eeebd..2b2f394fdf52 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -298,11 +298,8 @@ func (ctx *BuiltinEvalContext) EvaluateReplaceTriggeredBy(expr hcl.Expression, r switch sub := ref.Subject.(type) { case addrs.Resource: resourceAddr = sub - rc := sub.InModule(ctx.Path().Module()) - changes = ctx.Changes().GetChangesForConfigResource(rc) - // FIXME: Needs to be restricted to the same module! - // The other caller actually needs the same condition, so we can - // change this method to cover both use cases. + rc := sub.Absolute(ctx.Path()) + changes = ctx.Changes().GetChangesForAbsResource(rc) case addrs.ResourceInstance: resourceAddr = sub.ContainingResource() rc := sub.Absolute(ctx.Path()) From e4c4dcbd14b06da76f5d8b00e59569cddf9d1c38 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Apr 2022 14:55:59 -0400 Subject: [PATCH 10/15] add ResourceInstanceReplaceByTriggers --- internal/plans/changes.go | 5 +++ ...sourceinstancechangeactionreason_string.go | 32 +++++++++---------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/internal/plans/changes.go b/internal/plans/changes.go index faaed80b2973..439ecb38a58c 100644 --- a/internal/plans/changes.go +++ b/internal/plans/changes.go @@ -360,6 +360,11 @@ const ( // planning option.) ResourceInstanceReplaceByRequest ResourceInstanceChangeActionReason = 'R' + // ResourceInstanceReplaceByTriggers indicates that the resource instance + // is planned to be replaced because of a corresponding change in a + // replace_triggered_by reference. + ResourceInstanceReplaceByTriggers ResourceInstanceChangeActionReason = 'D' + // ResourceInstanceReplaceBecauseCannotUpdate indicates that the resource // instance is planned to be replaced because the provider has indicated // that a requested change cannot be applied as an update. diff --git a/internal/plans/resourceinstancechangeactionreason_string.go b/internal/plans/resourceinstancechangeactionreason_string.go index 135e6d2c638d..d278977bef27 100644 --- a/internal/plans/resourceinstancechangeactionreason_string.go +++ b/internal/plans/resourceinstancechangeactionreason_string.go @@ -11,6 +11,7 @@ func _() { _ = x[ResourceInstanceChangeNoReason-0] _ = x[ResourceInstanceReplaceBecauseTainted-84] _ = x[ResourceInstanceReplaceByRequest-82] + _ = x[ResourceInstanceReplaceByTriggers-68] _ = x[ResourceInstanceReplaceBecauseCannotUpdate-70] _ = x[ResourceInstanceDeleteBecauseNoResourceConfig-78] _ = x[ResourceInstanceDeleteBecauseWrongRepetition-87] @@ -21,37 +22,34 @@ func _() { const ( _ResourceInstanceChangeActionReason_name_0 = "ResourceInstanceChangeNoReason" - _ResourceInstanceChangeActionReason_name_1 = "ResourceInstanceDeleteBecauseCountIndex" - _ResourceInstanceChangeActionReason_name_2 = "ResourceInstanceDeleteBecauseEachKeyResourceInstanceReplaceBecauseCannotUpdate" - _ResourceInstanceChangeActionReason_name_3 = "ResourceInstanceDeleteBecauseNoModuleResourceInstanceDeleteBecauseNoResourceConfig" - _ResourceInstanceChangeActionReason_name_4 = "ResourceInstanceReplaceByRequest" - _ResourceInstanceChangeActionReason_name_5 = "ResourceInstanceReplaceBecauseTainted" - _ResourceInstanceChangeActionReason_name_6 = "ResourceInstanceDeleteBecauseWrongRepetition" + _ResourceInstanceChangeActionReason_name_1 = "ResourceInstanceDeleteBecauseCountIndexResourceInstanceReplaceByTriggersResourceInstanceDeleteBecauseEachKeyResourceInstanceReplaceBecauseCannotUpdate" + _ResourceInstanceChangeActionReason_name_2 = "ResourceInstanceDeleteBecauseNoModuleResourceInstanceDeleteBecauseNoResourceConfig" + _ResourceInstanceChangeActionReason_name_3 = "ResourceInstanceReplaceByRequest" + _ResourceInstanceChangeActionReason_name_4 = "ResourceInstanceReplaceBecauseTainted" + _ResourceInstanceChangeActionReason_name_5 = "ResourceInstanceDeleteBecauseWrongRepetition" ) var ( - _ResourceInstanceChangeActionReason_index_2 = [...]uint8{0, 36, 78} - _ResourceInstanceChangeActionReason_index_3 = [...]uint8{0, 37, 82} + _ResourceInstanceChangeActionReason_index_1 = [...]uint8{0, 39, 72, 108, 150} + _ResourceInstanceChangeActionReason_index_2 = [...]uint8{0, 37, 82} ) func (i ResourceInstanceChangeActionReason) String() string { switch { case i == 0: return _ResourceInstanceChangeActionReason_name_0 - case i == 67: - return _ResourceInstanceChangeActionReason_name_1 - case 69 <= i && i <= 70: - i -= 69 - return _ResourceInstanceChangeActionReason_name_2[_ResourceInstanceChangeActionReason_index_2[i]:_ResourceInstanceChangeActionReason_index_2[i+1]] + case 67 <= i && i <= 70: + i -= 67 + return _ResourceInstanceChangeActionReason_name_1[_ResourceInstanceChangeActionReason_index_1[i]:_ResourceInstanceChangeActionReason_index_1[i+1]] case 77 <= i && i <= 78: i -= 77 - return _ResourceInstanceChangeActionReason_name_3[_ResourceInstanceChangeActionReason_index_3[i]:_ResourceInstanceChangeActionReason_index_3[i+1]] + return _ResourceInstanceChangeActionReason_name_2[_ResourceInstanceChangeActionReason_index_2[i]:_ResourceInstanceChangeActionReason_index_2[i+1]] case i == 82: - return _ResourceInstanceChangeActionReason_name_4 + return _ResourceInstanceChangeActionReason_name_3 case i == 84: - return _ResourceInstanceChangeActionReason_name_5 + return _ResourceInstanceChangeActionReason_name_4 case i == 87: - return _ResourceInstanceChangeActionReason_name_6 + return _ResourceInstanceChangeActionReason_name_5 default: return "ResourceInstanceChangeActionReason(" + strconv.FormatInt(int64(i), 10) + ")" } From e2fc9a19f52282e5b876dcdbc5c8006db5c60d65 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Apr 2022 14:56:34 -0400 Subject: [PATCH 11/15] use ResourceInstanceReplaceByTriggers Set ResourceInstanceReplaceByTriggers in the change. --- internal/terraform/context_plan2_test.go | 3 ++ .../terraform/node_resource_plan_instance.go | 29 +++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 3b44620a27c1..6b52fbc4eb29 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -3025,6 +3025,9 @@ resource "test_object" "b" { if c.Action != plans.DeleteThenCreate { t.Fatalf("unexpected %s change for %s\n", c.Action, c.Addr) } + if c.ActionReason != plans.ResourceInstanceReplaceByTriggers { + t.Fatalf("incorrect reason for change: %s\n", c.ActionReason) + } default: t.Fatal("unexpected change", c.Addr, c.Action) } diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index 29d537c9ee19..3b14332124b7 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -33,6 +33,10 @@ type NodePlannableResourceInstance struct { // it might contain addresses that have nothing to do with the resource // that this node represents, which the node itself must therefore ignore. forceReplace []addrs.AbsResourceInstance + + // replaceTriggeredBy stores references from replace_triggered_by which + // triggered this instance to be replaced. + replaceTriggeredBy []*addrs.Reference } var ( @@ -219,6 +223,13 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } + // FIXME: here we udpate the change to reflect the reason for + // replacement, but we still overload forceReplace to get the correct + // change planned. + if len(n.replaceTriggeredBy) > 0 { + change.ActionReason = plans.ResourceInstanceReplaceByTriggers + } + diags = diags.Append(n.checkPreventDestroy(change)) if diags.HasErrors() { return diags @@ -325,21 +336,27 @@ func (n *NodePlannableResourceInstance) replaceTriggered(ctx EvalContext, repDat var diags tfdiags.Diagnostics for _, expr := range n.Config.TriggersReplacement { - _, replace, evalDiags := ctx.EvaluateReplaceTriggeredBy(expr, repData) + ref, replace, evalDiags := ctx.EvaluateReplaceTriggeredBy(expr, repData) diags = diags.Append(evalDiags) if diags.HasErrors() { continue } if replace { - // FIXME: forceReplace accomplishes the same goal, however we will - // want a new way to signal why this resource was replaced in the - // plan. + // FIXME: forceReplace accomplishes the same goal, however we may + // want to communicate more information about which resource + // Rather than further complicating the plan method with more + // options, we can refactor both of these featured later. + n.forceReplace = append(n.forceReplace, n.Addr) + // + // triggered the replacement in the plan. // EvalauteReplaceTriggeredBy returns a reference to store // somewhere for this purpose too. - n.forceReplace = append(n.forceReplace, n.Addr) - } + log.Printf("[DEBUG] ReplaceTriggeredBy forcing replacement of %s due to change in %s", n.Addr, ref.DisplayString()) + n.replaceTriggeredBy = append(n.replaceTriggeredBy, ref) + break + } } return diags From 868052c9e368c306878c59920eafa5e93ab6695c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Apr 2022 14:57:09 -0400 Subject: [PATCH 12/15] set replace_trigered_by reason in diff output --- internal/command/format/diff.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/command/format/diff.go b/internal/command/format/diff.go index ba23160483df..5abd2c9274ef 100644 --- a/internal/command/format/diff.go +++ b/internal/command/format/diff.go @@ -87,6 +87,8 @@ func ResourceChange( buf.WriteString(fmt.Sprintf(color.Color("[bold] # %s[reset] is tainted, so must be [bold][red]replaced"), dispAddr)) case plans.ResourceInstanceReplaceByRequest: buf.WriteString(fmt.Sprintf(color.Color("[bold] # %s[reset] will be [bold][red]replaced[reset], as requested"), dispAddr)) + case plans.ResourceInstanceReplaceByTriggers: + buf.WriteString(fmt.Sprintf(color.Color("[bold] # %s[reset] will be [bold][red]replaced[reset] due to changes in replace_triggered_by"), dispAddr)) default: buf.WriteString(fmt.Sprintf(color.Color("[bold] # %s[reset] must be [bold][red]replaced"), dispAddr)) } From 54c1791a1b339a5e2de6d8c3b26a6b42eb953b50 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Apr 2022 16:13:47 -0400 Subject: [PATCH 13/15] add triggers reason to plan proto --- .../plans/internal/planproto/planfile.pb.go | 32 +++++++++++-------- .../plans/internal/planproto/planfile.proto | 1 + 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/internal/plans/internal/planproto/planfile.pb.go b/internal/plans/internal/planproto/planfile.pb.go index 336957b5fa1a..55cc9cf97833 100644 --- a/internal/plans/internal/planproto/planfile.pb.go +++ b/internal/plans/internal/planproto/planfile.pb.go @@ -149,6 +149,7 @@ const ( ResourceInstanceActionReason_DELETE_BECAUSE_COUNT_INDEX ResourceInstanceActionReason = 6 ResourceInstanceActionReason_DELETE_BECAUSE_EACH_KEY ResourceInstanceActionReason = 7 ResourceInstanceActionReason_DELETE_BECAUSE_NO_MODULE ResourceInstanceActionReason = 8 + ResourceInstanceActionReason_REPLACE_BY_TRIGGERS ResourceInstanceActionReason = 9 ) // Enum value maps for ResourceInstanceActionReason. @@ -163,6 +164,7 @@ var ( 6: "DELETE_BECAUSE_COUNT_INDEX", 7: "DELETE_BECAUSE_EACH_KEY", 8: "DELETE_BECAUSE_NO_MODULE", + 9: "REPLACE_BY_TRIGGERS", } ResourceInstanceActionReason_value = map[string]int32{ "NONE": 0, @@ -174,6 +176,7 @@ var ( "DELETE_BECAUSE_COUNT_INDEX": 6, "DELETE_BECAUSE_EACH_KEY": 7, "DELETE_BECAUSE_NO_MODULE": 8, + "REPLACE_BY_TRIGGERS": 9, } ) @@ -1297,7 +1300,7 @@ var file_planfile_proto_rawDesc = []byte{ 0x45, 0x10, 0x05, 0x12, 0x16, 0x0a, 0x12, 0x44, 0x45, 0x4c, 0x45, 0x54, 0x45, 0x5f, 0x54, 0x48, 0x45, 0x4e, 0x5f, 0x43, 0x52, 0x45, 0x41, 0x54, 0x45, 0x10, 0x06, 0x12, 0x16, 0x0a, 0x12, 0x43, 0x52, 0x45, 0x41, 0x54, 0x45, 0x5f, 0x54, 0x48, 0x45, 0x4e, 0x5f, 0x44, 0x45, 0x4c, 0x45, 0x54, - 0x45, 0x10, 0x07, 0x2a, 0xa7, 0x02, 0x0a, 0x1c, 0x52, 0x65, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, + 0x45, 0x10, 0x07, 0x2a, 0xc0, 0x02, 0x0a, 0x1c, 0x52, 0x65, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, 0x49, 0x6e, 0x73, 0x74, 0x61, 0x6e, 0x63, 0x65, 0x41, 0x63, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x61, 0x73, 0x6f, 0x6e, 0x12, 0x08, 0x0a, 0x04, 0x4e, 0x4f, 0x4e, 0x45, 0x10, 0x00, 0x12, 0x1b, 0x0a, 0x17, 0x52, 0x45, 0x50, 0x4c, 0x41, 0x43, 0x45, 0x5f, 0x42, 0x45, 0x43, 0x41, 0x55, 0x53, @@ -1315,19 +1318,20 @@ var file_planfile_proto_rawDesc = []byte{ 0x10, 0x06, 0x12, 0x1b, 0x0a, 0x17, 0x44, 0x45, 0x4c, 0x45, 0x54, 0x45, 0x5f, 0x42, 0x45, 0x43, 0x41, 0x55, 0x53, 0x45, 0x5f, 0x45, 0x41, 0x43, 0x48, 0x5f, 0x4b, 0x45, 0x59, 0x10, 0x07, 0x12, 0x1c, 0x0a, 0x18, 0x44, 0x45, 0x4c, 0x45, 0x54, 0x45, 0x5f, 0x42, 0x45, 0x43, 0x41, 0x55, 0x53, - 0x45, 0x5f, 0x4e, 0x4f, 0x5f, 0x4d, 0x4f, 0x44, 0x55, 0x4c, 0x45, 0x10, 0x08, 0x2a, 0x6c, 0x0a, - 0x0d, 0x43, 0x6f, 0x6e, 0x64, 0x69, 0x74, 0x69, 0x6f, 0x6e, 0x54, 0x79, 0x70, 0x65, 0x12, 0x0b, - 0x0a, 0x07, 0x49, 0x4e, 0x56, 0x41, 0x4c, 0x49, 0x44, 0x10, 0x00, 0x12, 0x19, 0x0a, 0x15, 0x52, - 0x45, 0x53, 0x4f, 0x55, 0x52, 0x43, 0x45, 0x5f, 0x50, 0x52, 0x45, 0x43, 0x4f, 0x4e, 0x44, 0x49, - 0x54, 0x49, 0x4f, 0x4e, 0x10, 0x01, 0x12, 0x1a, 0x0a, 0x16, 0x52, 0x45, 0x53, 0x4f, 0x55, 0x52, - 0x43, 0x45, 0x5f, 0x50, 0x4f, 0x53, 0x54, 0x43, 0x4f, 0x4e, 0x44, 0x49, 0x54, 0x49, 0x4f, 0x4e, - 0x10, 0x02, 0x12, 0x17, 0x0a, 0x13, 0x4f, 0x55, 0x54, 0x50, 0x55, 0x54, 0x5f, 0x50, 0x52, 0x45, - 0x43, 0x4f, 0x4e, 0x44, 0x49, 0x54, 0x49, 0x4f, 0x4e, 0x10, 0x03, 0x42, 0x42, 0x5a, 0x40, 0x67, - 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, 0x69, 0x63, - 0x6f, 0x72, 0x70, 0x2f, 0x74, 0x65, 0x72, 0x72, 0x61, 0x66, 0x6f, 0x72, 0x6d, 0x2f, 0x69, 0x6e, - 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x70, 0x6c, 0x61, 0x6e, 0x73, 0x2f, 0x69, 0x6e, 0x74, - 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x70, 0x6c, 0x61, 0x6e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, - 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x45, 0x5f, 0x4e, 0x4f, 0x5f, 0x4d, 0x4f, 0x44, 0x55, 0x4c, 0x45, 0x10, 0x08, 0x12, 0x17, 0x0a, + 0x13, 0x52, 0x45, 0x50, 0x4c, 0x41, 0x43, 0x45, 0x5f, 0x42, 0x59, 0x5f, 0x54, 0x52, 0x49, 0x47, + 0x47, 0x45, 0x52, 0x53, 0x10, 0x09, 0x2a, 0x6c, 0x0a, 0x0d, 0x43, 0x6f, 0x6e, 0x64, 0x69, 0x74, + 0x69, 0x6f, 0x6e, 0x54, 0x79, 0x70, 0x65, 0x12, 0x0b, 0x0a, 0x07, 0x49, 0x4e, 0x56, 0x41, 0x4c, + 0x49, 0x44, 0x10, 0x00, 0x12, 0x19, 0x0a, 0x15, 0x52, 0x45, 0x53, 0x4f, 0x55, 0x52, 0x43, 0x45, + 0x5f, 0x50, 0x52, 0x45, 0x43, 0x4f, 0x4e, 0x44, 0x49, 0x54, 0x49, 0x4f, 0x4e, 0x10, 0x01, 0x12, + 0x1a, 0x0a, 0x16, 0x52, 0x45, 0x53, 0x4f, 0x55, 0x52, 0x43, 0x45, 0x5f, 0x50, 0x4f, 0x53, 0x54, + 0x43, 0x4f, 0x4e, 0x44, 0x49, 0x54, 0x49, 0x4f, 0x4e, 0x10, 0x02, 0x12, 0x17, 0x0a, 0x13, 0x4f, + 0x55, 0x54, 0x50, 0x55, 0x54, 0x5f, 0x50, 0x52, 0x45, 0x43, 0x4f, 0x4e, 0x44, 0x49, 0x54, 0x49, + 0x4f, 0x4e, 0x10, 0x03, 0x42, 0x42, 0x5a, 0x40, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, + 0x6f, 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x74, 0x65, 0x72, + 0x72, 0x61, 0x66, 0x6f, 0x72, 0x6d, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, + 0x70, 0x6c, 0x61, 0x6e, 0x73, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x70, + 0x6c, 0x61, 0x6e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/internal/plans/internal/planproto/planfile.proto b/internal/plans/internal/planproto/planfile.proto index c85f0a76b8da..fec25c5ca6e5 100644 --- a/internal/plans/internal/planproto/planfile.proto +++ b/internal/plans/internal/planproto/planfile.proto @@ -146,6 +146,7 @@ enum ResourceInstanceActionReason { DELETE_BECAUSE_COUNT_INDEX = 6; DELETE_BECAUSE_EACH_KEY = 7; DELETE_BECAUSE_NO_MODULE = 8; + REPLACE_BY_TRIGGERS = 9; } message ResourceInstanceChange { From 3a0a019521c6d8022c814dcf7f71d18975375732 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Apr 2022 16:17:12 -0400 Subject: [PATCH 14/15] round-trip replace triggers --- internal/command/jsonplan/plan.go | 2 ++ internal/command/views/json/change.go | 13 ++++++++----- internal/plans/planfile/tfplan.go | 4 ++++ 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/command/jsonplan/plan.go b/internal/command/jsonplan/plan.go index 89ecc4c02691..86b5e1e4b2a5 100644 --- a/internal/command/jsonplan/plan.go +++ b/internal/command/jsonplan/plan.go @@ -392,6 +392,8 @@ func (p *plan) marshalResourceChanges(resources []*plans.ResourceInstanceChangeS r.ActionReason = "replace_because_tainted" case plans.ResourceInstanceReplaceByRequest: r.ActionReason = "replace_by_request" + case plans.ResourceInstanceReplaceByTriggers: + r.ActionReason = "replace_by_triggers" case plans.ResourceInstanceDeleteBecauseNoResourceConfig: r.ActionReason = "delete_because_no_resource_config" case plans.ResourceInstanceDeleteBecauseWrongRepetition: diff --git a/internal/command/views/json/change.go b/internal/command/views/json/change.go index 4a8aa4e5bd14..2036b36761b6 100644 --- a/internal/command/views/json/change.go +++ b/internal/command/views/json/change.go @@ -68,11 +68,12 @@ func changeAction(action plans.Action) ChangeAction { type ChangeReason string const ( - ReasonNone ChangeReason = "" - ReasonTainted ChangeReason = "tainted" - ReasonRequested ChangeReason = "requested" - ReasonCannotUpdate ChangeReason = "cannot_update" - ReasonUnknown ChangeReason = "unknown" + ReasonNone ChangeReason = "" + ReasonTainted ChangeReason = "tainted" + ReasonRequested ChangeReason = "requested" + ReasonReplaceTriggeredBy ChangeReason = "replace_triggered_by" + ReasonCannotUpdate ChangeReason = "cannot_update" + ReasonUnknown ChangeReason = "unknown" ReasonDeleteBecauseNoResourceConfig ChangeReason = "delete_because_no_resource_config" ReasonDeleteBecauseWrongRepetition ChangeReason = "delete_because_wrong_repetition" @@ -91,6 +92,8 @@ func changeReason(reason plans.ResourceInstanceChangeActionReason) ChangeReason return ReasonRequested case plans.ResourceInstanceReplaceBecauseCannotUpdate: return ReasonCannotUpdate + case plans.ResourceInstanceReplaceByTriggers: + return ReasonReplaceTriggeredBy case plans.ResourceInstanceDeleteBecauseNoResourceConfig: return ReasonDeleteBecauseNoResourceConfig case plans.ResourceInstanceDeleteBecauseWrongRepetition: diff --git a/internal/plans/planfile/tfplan.go b/internal/plans/planfile/tfplan.go index 581a545dc253..882d5fa8f62f 100644 --- a/internal/plans/planfile/tfplan.go +++ b/internal/plans/planfile/tfplan.go @@ -266,6 +266,8 @@ func resourceChangeFromTfplan(rawChange *planproto.ResourceInstanceChange) (*pla ret.ActionReason = plans.ResourceInstanceReplaceBecauseTainted case planproto.ResourceInstanceActionReason_REPLACE_BY_REQUEST: ret.ActionReason = plans.ResourceInstanceReplaceByRequest + case planproto.ResourceInstanceActionReason_REPLACE_BY_TRIGGERS: + ret.ActionReason = plans.ResourceInstanceReplaceByTriggers case planproto.ResourceInstanceActionReason_DELETE_BECAUSE_NO_RESOURCE_CONFIG: ret.ActionReason = plans.ResourceInstanceDeleteBecauseNoResourceConfig case planproto.ResourceInstanceActionReason_DELETE_BECAUSE_WRONG_REPETITION: @@ -611,6 +613,8 @@ func resourceChangeToTfplan(change *plans.ResourceInstanceChangeSrc) (*planproto ret.ActionReason = planproto.ResourceInstanceActionReason_REPLACE_BECAUSE_TAINTED case plans.ResourceInstanceReplaceByRequest: ret.ActionReason = planproto.ResourceInstanceActionReason_REPLACE_BY_REQUEST + case plans.ResourceInstanceReplaceByTriggers: + ret.ActionReason = planproto.ResourceInstanceActionReason_REPLACE_BY_TRIGGERS case plans.ResourceInstanceDeleteBecauseNoResourceConfig: ret.ActionReason = planproto.ResourceInstanceActionReason_DELETE_BECAUSE_NO_RESOURCE_CONFIG case plans.ResourceInstanceDeleteBecauseWrongRepetition: From 1e79682c245721ccd916faee02fe8a34bcf170ea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 Apr 2022 12:51:24 -0400 Subject: [PATCH 15/15] minor fixes --- internal/configs/resource.go | 4 ++-- internal/terraform/node_resource_plan_instance.go | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/configs/resource.go b/internal/configs/resource.go index 3c66f1277de8..c54557921188 100644 --- a/internal/configs/resource.go +++ b/internal/configs/resource.go @@ -586,14 +586,14 @@ func decodeReplaceTriggeredBy(expr hcl.Expression) ([]hcl.Expression, hcl.Diagno diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid replace_triggered_by expression", - Detail: "Missing resource reference in replace_triggered_by_expression.", + Detail: "Missing resource reference in replace_triggered_by expression.", Subject: expr.Range().Ptr(), }) case resourceCount > 1: diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid replace_triggered_by expression", - Detail: "Multiple resource references in replace_triggered_by_expression.", + Detail: "Multiple resource references in replace_triggered_by expression.", Subject: expr.Range().Ptr(), }) } diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index 3b14332124b7..46ca37f7de2c 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -345,13 +345,10 @@ func (n *NodePlannableResourceInstance) replaceTriggered(ctx EvalContext, repDat if replace { // FIXME: forceReplace accomplishes the same goal, however we may // want to communicate more information about which resource + // triggered the replacement in the plan. // Rather than further complicating the plan method with more - // options, we can refactor both of these featured later. + // options, we can refactor both of these features later. n.forceReplace = append(n.forceReplace, n.Addr) - // - // triggered the replacement in the plan. - // EvalauteReplaceTriggeredBy returns a reference to store - // somewhere for this purpose too. log.Printf("[DEBUG] ReplaceTriggeredBy forcing replacement of %s due to change in %s", n.Addr, ref.DisplayString()) n.replaceTriggeredBy = append(n.replaceTriggeredBy, ref)