From 551f6316fa0682e44f24d7c95ca0ad7347954397 Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Thu, 15 Sep 2022 15:20:41 -0400
Subject: [PATCH] Do not apply type defaults to null values

Applying object type defaults to null values can convert null to an
object with partial attributes. This means that even a specified default
value of null will not remain null after variable evaluation.

In turn, the result can then be invalid, if not all attributes in an
object type have defaults specified.

This commit skips the type default application step during config load
and variable evaluation if the default or given value is null of any
type. We still perform type conversion.
---
 internal/configs/named_values.go              |  7 ++-
 .../valid-files/object-optional-attrs.tf      | 38 +++++++++++++++
 internal/terraform/context_apply_test.go      | 46 +++++++++++++++++++
 internal/terraform/eval_variable.go           |  7 ++-
 4 files changed, 94 insertions(+), 4 deletions(-)
 create mode 100644 internal/configs/testdata/valid-files/object-optional-attrs.tf

diff --git a/internal/configs/named_values.go b/internal/configs/named_values.go
index b12b212880..3ce759c871 100644
--- a/internal/configs/named_values.go
+++ b/internal/configs/named_values.go
@@ -140,8 +140,11 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno
 		if v.ConstraintType != cty.NilType {
 			var err error
 			// If the type constraint has defaults, we must apply those
-			// defaults to the variable default value before type conversion.
-			if v.TypeDefaults != nil {
+			// defaults to the variable default value before type conversion,
+			// unless the default value is null. Null is excluded from the
+			// type default application process as a special case, to allow
+			// nullable variables to have a null default value.
+			if v.TypeDefaults != nil && !val.IsNull() {
 				val = v.TypeDefaults.Apply(val)
 			}
 			val, err = convert.Convert(val, v.ConstraintType)
diff --git a/internal/configs/testdata/valid-files/object-optional-attrs.tf b/internal/configs/testdata/valid-files/object-optional-attrs.tf
new file mode 100644
index 0000000000..8b7fda9a74
--- /dev/null
+++ b/internal/configs/testdata/valid-files/object-optional-attrs.tf
@@ -0,0 +1,38 @@
+variable "a" {
+  type = object({
+    foo = optional(string)
+    bar = optional(bool, true)
+  })
+}
+
+variable "b" {
+  type = list(
+    object({
+      foo = optional(string)
+    })
+  )
+}
+
+variable "c" {
+  type = set(
+    object({
+      foo = optional(string)
+    })
+  )
+}
+
+variable "d" {
+  type = map(
+    object({
+      foo = optional(string)
+    })
+  )
+}
+
+variable "e" {
+  type = object({
+    foo = string
+    bar = optional(bool, true)
+  })
+  default = null
+}
diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go
index c3e3beecbc..b36b51c77a 100644
--- a/internal/terraform/context_apply_test.go
+++ b/internal/terraform/context_apply_test.go
@@ -12119,6 +12119,52 @@ output "out" {
 	}
 }
 
+func TestContext2Apply_moduleVariableOptionalAttributesDefaultNull(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+variable "in" {
+  type    = object({
+    required = string
+    optional = optional(string)
+    default  = optional(bool, true)
+  })
+  default = null
+}
+
+# Wrap the input variable in a tuple because a null output value is elided from
+# the plan, which prevents us from testing its type.
+output "out" {
+  value = [var.in]
+}
+`})
+
+	ctx := testContext2(t, &ContextOpts{})
+
+	// We don't specify a value for the variable here, relying on its defined
+	// default.
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	state, diags := ctx.Apply(plan, m)
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	got := state.RootModule().OutputValues["out"].Value
+	// The null default value should be bound, after type converting to the
+	// full object type
+	want := cty.TupleVal([]cty.Value{cty.NullVal(cty.Object(map[string]cty.Type{
+		"required": cty.String,
+		"optional": cty.String,
+		"default":  cty.Bool,
+	}))})
+	if !want.RawEquals(got) {
+		t.Fatalf("wrong result\ngot:  %#v\nwant: %#v", got, want)
+	}
+}
+
 func TestContext2Apply_provisionerSensitive(t *testing.T) {
 	m := testModule(t, "apply-provisioner-sensitive")
 	p := testProvider("aws")
diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go
index 67a777430f..167840e7c1 100644
--- a/internal/terraform/eval_variable.go
+++ b/internal/terraform/eval_variable.go
@@ -90,8 +90,11 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *In
 		given = defaultVal // must be set, because we checked above that the variable isn't required
 	}
 
-	// Apply defaults from the variable's type constraint to the given value
-	if cfg.TypeDefaults != nil {
+	// Apply defaults from the variable's type constraint to the given value,
+	// unless the given value is null. We do not apply defaults to top-level
+	// null values, as doing so could prevent assigning null to a nullable
+	// variable.
+	if cfg.TypeDefaults != nil && !given.IsNull() {
 		given = cfg.TypeDefaults.Apply(given)
 	}