From fe3edb8e46f8f8677277e3fd8a2a5466dbcd16aa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 18 Dec 2019 14:37:28 -0500 Subject: [PATCH 1/3] more aggressively prune unused values Since a planned destroy can no longer indicate it is a full destroy, unused values were being left in the apply graph for evaluation. If these values contains interpolations that can fail, (for example, a zipmap with mismatched list sizes), it will cause the apply to abort. The PrunUnusedValuesTransformer was only previously run during destroy, more out of conservatism than for any other particular reason. Adapt it to always remove unused values from the graph, with the exception being the root module outputs, which must be retained when we don't have a clear indication that a full destroy is being executed. --- terraform/graph_builder_apply.go | 9 ++++++--- terraform/transform_reference.go | 29 ++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 615731328..00ddcf5c4 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -175,17 +175,20 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Reverse the edges from outputs and locals, so that // interpolations don't fail during destroy. // Create a destroy node for outputs to remove them from the state. - // Prune unreferenced values, which may have interpolations that can't - // be resolved. GraphTransformIf( func() bool { return b.Destroy }, GraphTransformMulti( &DestroyValueReferenceTransformer{}, &DestroyOutputTransformer{}, - &PruneUnusedValuesTransformer{}, ), ), + // Prune unreferenced values, which may have interpolations that can't + // be resolved. + &PruneUnusedValuesTransformer{ + Destroy: b.Destroy, + }, + // Add the node to fix the state count boundaries &CountBoundaryTransformer{ Config: b.Config, diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 25e544996..df34a3cea 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -206,21 +206,30 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { return nil } -// PruneUnusedValuesTransformer is s GraphTransformer that removes local and -// output values which are not referenced in the graph. Since outputs and -// locals always need to be evaluated, if they reference a resource that is not -// available in the state the interpolation could fail. -type PruneUnusedValuesTransformer struct{} +// PruneUnusedValuesTransformer is a GraphTransformer that removes local, +// variable, and output values which are not referenced in the graph. If these +// values reference a resource that is no longer in the state the interpolation +// could fail. +type PruneUnusedValuesTransformer struct { + Destroy bool +} func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { - // this might need multiple runs in order to ensure that pruning a value - // doesn't effect a previously checked value. + // Pruning a value can effect previously checked edges, so loop until there + // are no more changes. for removed := 0; ; removed = 0 { for _, v := range g.Vertices() { - switch v.(type) { - case *NodeApplyableOutput, *NodeLocal: + switch v := v.(type) { + case *NodeApplyableOutput: + // If we're not certain this is a full destroy, we need to keep any + // root module outputs + if v.Addr.Module.IsRoot() && !t.Destroy { + continue + } + case *NodeLocal, *NodeApplyableModuleVariable: // OK default: + // We're only concerned with variables, locals and outputs continue } @@ -229,6 +238,7 @@ func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { switch dependants.Len() { case 0: // nothing at all depends on this + log.Printf("[TRACE] PruneUnusedValuesTransformer: removing unused value %s", dag.VertexName(v)) g.Remove(v) removed++ case 1: @@ -236,6 +246,7 @@ func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { // we need to check for the case of a single destroy node. d := dependants.List()[0] if _, ok := d.(*NodeDestroyableOutput); ok { + log.Printf("[TRACE] PruneUnusedValuesTransformer: removing unused value %s", dag.VertexName(v)) g.Remove(v) removed++ } From aa8a0f063c039ce31e14de81e57f97d41b1b7827 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 18 Dec 2019 15:12:10 -0500 Subject: [PATCH 2/3] update 2 tests to match the new output These 2 tests were matching the old-style state strings, and the only change is to module outputs which are no longer marshaled. --- terraform/terraform_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 845881977..6eeff9715 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -406,9 +406,7 @@ module.child: Outputs: -aws_access_key = YYYYY aws_route53_zone_id = XXXX -aws_secret_key = ZZZZ ` const testTerraformApplyDependsCreateBeforeStr = ` @@ -693,11 +691,6 @@ foo = bar const testTerraformApplyOutputOrphanModuleStr = ` -module.child: - - Outputs: - - foo = bar ` const testTerraformApplyProvisionerStr = ` From 6bad4e3dc03bbf43a31977c1c33f7e94e5882813 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 18 Dec 2019 15:37:08 -0500 Subject: [PATCH 3/3] add an interp that fails during planned destroy Chain some values so that we can get an interpolation that fails if a module input is evaluated during destroy. --- terraform/context_apply_test.go | 1 - .../plan-destroy-interpolated-count/main.tf | 13 +++++++++++-- .../plan-destroy-interpolated-count/mod/main.tf | 2 ++ 3 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 terraform/testdata/plan-destroy-interpolated-count/mod/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 1de9e380b..f344a42b0 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10521,7 +10521,6 @@ func TestContext2Apply_plannedDestroyInterpolatedCount(t *testing.T) { } ctxOpts.ProviderResolver = providerResolver - ctxOpts.Destroy = true ctx, diags = NewContext(ctxOpts) if diags.HasErrors() { t.Fatalf("err: %s", diags.Err()) diff --git a/terraform/testdata/plan-destroy-interpolated-count/main.tf b/terraform/testdata/plan-destroy-interpolated-count/main.tf index b4ef77aba..ac0dadbf8 100644 --- a/terraform/testdata/plan-destroy-interpolated-count/main.tf +++ b/terraform/testdata/plan-destroy-interpolated-count/main.tf @@ -3,9 +3,18 @@ variable "list" { } resource "aws_instance" "a" { - count = "${length(var.list)}" + count = length(var.list) +} + +locals { + ids = aws_instance.a[*].id +} + +module "empty" { + source = "./mod" + input = zipmap(var.list, local.ids) } output "out" { - value = "${aws_instance.a.*.id}" + value = aws_instance.a[*].id } diff --git a/terraform/testdata/plan-destroy-interpolated-count/mod/main.tf b/terraform/testdata/plan-destroy-interpolated-count/mod/main.tf new file mode 100644 index 000000000..682e0f0db --- /dev/null +++ b/terraform/testdata/plan-destroy-interpolated-count/mod/main.tf @@ -0,0 +1,2 @@ +variable "input" { +}