From b45de53c13160abeb87ebb7775e530fa2a7ee24c Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Fri, 30 Jun 2023 15:24:49 -0400
Subject: [PATCH] always evaluate module outputs during destroy

A module output is generally not used during destroy, however it must be
evaluated when its value is used by a provider for configuration,
because that configuration is not stored between walks.

There was an oversight in the output expansion node where the output
node was not created because the operation was destroy, and module
outputs have nothing to destroy. This however skipped evaluation when
the output is needed by a provider as mentioned above. Because of the
way an implied plan is stored internally when executing `terraform
destroy`, this went unnoticed by the test.

Allowing the output to be evaluated during destroy fixes the issue, and
should be acceptable because an output is classified as temporary in the
graph, and will be pruned when not actually needed.

Update the existing test to serialize the plan, which triggers the
failure.
---
 internal/terraform/context_apply_test.go      | 50 +++++++------------
 internal/terraform/node_output.go             |  4 --
 .../apply-destroy-provisider-refs/main.tf     | 15 ++++++
 .../apply-destroy-provisider-refs/mod/main.tf |  9 ++++
 4 files changed, 43 insertions(+), 35 deletions(-)
 create mode 100644 internal/terraform/testdata/apply-destroy-provisider-refs/main.tf
 create mode 100644 internal/terraform/testdata/apply-destroy-provisider-refs/mod/main.tf

diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go
index 545cf2e133..6bf712078b 100644
--- a/internal/terraform/context_apply_test.go
+++ b/internal/terraform/context_apply_test.go
@@ -11381,33 +11381,7 @@ locals {
 // Ensure that we can destroy when a provider references a resource that will
 // also be destroyed
 func TestContext2Apply_destroyProviderReference(t *testing.T) {
-	m := testModuleInline(t, map[string]string{
-		"main.tf": `
-provider "null" {
-  value = ""
-}
-
-module "mod" {
-  source = "./mod"
-}
-
-provider "test" {
-  value = module.mod.output
-}
-
-resource "test_instance" "bar" {
-}
-`,
-		"mod/main.tf": `
-data "null_data_source" "foo" {
-       count = 1
-}
-
-
-output "output" {
-  value = data.null_data_source.foo[0].output
-}
-`})
+	m, snap := testModuleWithSnapshot(t, "apply-destroy-provisider-refs")
 
 	schemaFn := func(name string) *ProviderSchema {
 		return &ProviderSchema{
@@ -11506,11 +11480,12 @@ output "output" {
 		t.Fatalf("apply errors: %s", diags.Err())
 	}
 
+	providers := map[addrs.Provider]providers.Factory{
+		addrs.NewDefaultProvider("test"): testProviderFuncFixed(testP),
+		addrs.NewDefaultProvider("null"): testProviderFuncFixed(nullP),
+	}
 	ctx = testContext2(t, &ContextOpts{
-		Providers: map[addrs.Provider]providers.Factory{
-			addrs.NewDefaultProvider("test"): testProviderFuncFixed(testP),
-			addrs.NewDefaultProvider("null"): testProviderFuncFixed(nullP),
-		},
+		Providers: providers,
 	})
 
 	plan, diags = ctx.Plan(m, state, &PlanOpts{
@@ -11518,6 +11493,19 @@ output "output" {
 	})
 	assertNoErrors(t, diags)
 
+	// We'll marshal and unmarshal the plan here, to ensure that we have
+	// a clean new context as would be created if we separately ran
+	// terraform plan -out=tfplan && terraform apply tfplan
+	ctxOpts, m, plan, err := contextOptsForPlanViaFile(t, snap, plan)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ctxOpts.Providers = providers
+	ctx, diags = NewContext(ctxOpts)
+
+	if diags.HasErrors() {
+		t.Fatalf("failed to create context for plan: %s", diags.Err())
+	}
 	if _, diags := ctx.Apply(plan, m); diags.HasErrors() {
 		t.Fatalf("destroy apply errors: %s", diags.Err())
 	}
diff --git a/internal/terraform/node_output.go b/internal/terraform/node_output.go
index 315cb738a7..0a8c72e557 100644
--- a/internal/terraform/node_output.go
+++ b/internal/terraform/node_output.go
@@ -108,10 +108,6 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 				Planning: n.Planning,
 			}
 
-		case n.Destroying:
-			// nothing is done here for non-root outputs
-			continue
-
 		default:
 			node = &NodeApplyableOutput{
 				Addr:         absAddr,
diff --git a/internal/terraform/testdata/apply-destroy-provisider-refs/main.tf b/internal/terraform/testdata/apply-destroy-provisider-refs/main.tf
new file mode 100644
index 0000000000..e7ef6f0b5d
--- /dev/null
+++ b/internal/terraform/testdata/apply-destroy-provisider-refs/main.tf
@@ -0,0 +1,15 @@
+provider "null" {
+  value = ""
+}
+
+module "mod" {
+  source = "./mod"
+}
+
+provider "test" {
+  value = module.mod.output
+}
+
+resource "test_instance" "bar" {
+}
+
diff --git a/internal/terraform/testdata/apply-destroy-provisider-refs/mod/main.tf b/internal/terraform/testdata/apply-destroy-provisider-refs/mod/main.tf
new file mode 100644
index 0000000000..c9ecec002b
--- /dev/null
+++ b/internal/terraform/testdata/apply-destroy-provisider-refs/mod/main.tf
@@ -0,0 +1,9 @@
+data "null_data_source" "foo" {
+       count = 1
+}
+
+
+output "output" {
+  value = data.null_data_source.foo[0].output
+}
+