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 6157313281c2..00ddcf5c4034 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 25e544996ef2..df34a3cea8cc 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 8458819773ab..6eeff9715676 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 1de9e380b9d8..f344a42b0342 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 b4ef77aba778..ac0dadbf81f8 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 000000000000..682e0f0db76a --- /dev/null +++ b/terraform/testdata/plan-destroy-interpolated-count/mod/main.tf @@ -0,0 +1,2 @@ +variable "input" { +}