From 582c7298d3493ee9be2286877e62a2543408ec70 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 2 May 2024 15:34:34 -0400
Subject: [PATCH] don't evaluate providers in overridden modules

While we don't normally encounter providers within modules, they are
technically still supported, and could exist within a module which has
been overridden for testing. Since the module is not being evaluated, we
cannot safely evaluate the provider config as variables will not exist
within that module.
---
 .../terraform/context_apply_overrides_test.go | 47 +++++++++++++++++++
 internal/terraform/graph.go                   | 19 +++++++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/internal/terraform/context_apply_overrides_test.go b/internal/terraform/context_apply_overrides_test.go
index ab033f931faa..787af206309c 100644
--- a/internal/terraform/context_apply_overrides_test.go
+++ b/internal/terraform/context_apply_overrides_test.go
@@ -638,6 +638,43 @@ resource "test_instance" "resource" {
 output "id" {
   value = test_instance.resource[0].id
 }
+`,
+			},
+			overrides: mocking.OverridesForTesting(nil, func(overrides addrs.Map[addrs.Targetable, *configs.Override]) {
+				overrides.Put(mustModuleInstance("module.test"), &configs.Override{
+					Values: cty.ObjectVal(map[string]cty.Value{
+						"id": cty.StringVal("h3ll0"),
+					}),
+				})
+			}),
+			outputs: cty.EmptyObjectVal,
+		},
+		"legacy provider config inside overridden module": {
+			configs: map[string]string{
+				"main.tf": `
+module "test" {
+  source = "./child"
+}
+`,
+				"child/main.tf": `
+module "grandchild" {
+  source = "../grandchild"
+}
+output "id" {
+  value = "child"
+}
+`,
+				"grandchild/main.tf": `
+variable "in" {
+  default = "test_value"
+}
+
+provider "test" {
+  value = var.in
+}
+
+resource "test_instance" "resource" {
+}
 `,
 			},
 			overrides: mocking.OverridesForTesting(nil, func(overrides addrs.Map[addrs.Targetable, *configs.Override]) {
@@ -720,6 +757,16 @@ output "id" {
 // functionality, in that they should stop the provider from being executed.
 var underlyingOverridesProvider = &testing_provider.MockProvider{
 	GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+		Provider: providers.Schema{
+			Block: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"value": {
+						Type:     cty.String,
+						Optional: true,
+					},
+				},
+			},
+		},
 		ResourceTypes: map[string]providers.Schema{
 			"test_instance": {
 				Block: &configschema.Block{
diff --git a/internal/terraform/graph.go b/internal/terraform/graph.go
index 736c70e71c71..98b1e857094a 100644
--- a/internal/terraform/graph.go
+++ b/internal/terraform/graph.go
@@ -64,9 +64,11 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics {
 			}
 		}()
 
+		haveOverrides := !ctx.Overrides().Empty()
+
 		// If the graph node is overridable, we'll check our overrides to see
 		// if we need to apply any overrides to the node.
-		if overridable, ok := v.(GraphNodeOverridable); ok && !ctx.Overrides().Empty() {
+		if overridable, ok := v.(GraphNodeOverridable); ok && haveOverrides {
 			// It'd be nice if we could just pass the overrides directly into
 			// the nodes, but the way the AbstractNodeResource is created is
 			// complicated and it's not easy to make sure that every
@@ -80,6 +82,21 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics {
 			}
 		}
 
+		if provider, ok := v.(GraphNodeProvider); ok && haveOverrides {
+			// If we find a legacy provider within an overridden module, we
+			// can't evaluate the config so we have to skip it. We do this here
+			// for the similar reasons as the resource overrides above, and to
+			// keep all the override logic together.
+			addr := provider.ProviderAddr()
+			// UnkeyedInstanceShim is used by legacy provider configs within a
+			// module to return an instance of that module, since they can never
+			// exist within an expanded instance.
+			if ctx.Overrides().IsOverridden(addr.Module.UnkeyedInstanceShim()) {
+				log.Printf("[DEBUG] skipping provider %s found within overridden module", addr)
+				return
+			}
+		}
+
 		// vertexCtx is the context that we use when evaluating. This
 		// is normally the global context but can be overridden
 		// with either a GraphNodeModuleInstance, GraphNodePartialExpandedModule,