Merge pull request #29832 from hashicorp/jbardin/nullable-variable

configs: explicitly nullable variable values

internal/configs/module_merge.go

@@ -56,6 +56,10 @@ func (v *Variable) merge(ov *Variable) hcl.Diagnostics {
 	if ov.ParsingMode != 0 {
 		v.ParsingMode = ov.ParsingMode
 	}
+	if ov.NullableSet {
+		v.Nullable = ov.Nullable
+		v.NullableSet = ov.NullableSet
+	}
 
 	// If the override file overrode type without default or vice-versa then
 	// it may have created an invalid situation, which we'll catch now by
@@ -100,6 +104,16 @@ func (v *Variable) merge(ov *Variable) hcl.Diagnostics {
 		} else {
 			v.Default = val
 		}
+
+		// ensure a null default wasn't merged in when it is not allowed
+		if !v.Nullable && v.Default.IsNull() {
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid default value for variable",
+				Detail:   "A null default value is not valid when nullable=false.",
+				Subject:  &ov.DeclRange,
+			})
+		}
 	}
 
 	return diags

internal/configs/module_merge_test.go

@@ -24,6 +24,8 @@ func TestModuleOverrideVariable(t *testing.T) {
 			Description:    "b_override description",
 			DescriptionSet: true,
 			Default:        cty.StringVal("b_override"),
+			Nullable:       false,
+			NullableSet:    true,
 			Type:           cty.String,
 			ConstraintType: cty.String,
 			ParsingMode:    VariableParseLiteral,
@@ -46,6 +48,8 @@ func TestModuleOverrideVariable(t *testing.T) {
 			Description:    "base description",
 			DescriptionSet: true,
 			Default:        cty.StringVal("b_override partial"),
+			Nullable:       true,
+			NullableSet:    false,
 			Type:           cty.String,
 			ConstraintType: cty.String,
 			ParsingMode:    VariableParseLiteral,

internal/configs/named_values.go

@@ -36,6 +36,12 @@ type Variable struct {
 	DescriptionSet bool
 	SensitiveSet   bool
 
+	// Nullable indicates that null is a valid value for this variable. Setting
+	// Nullable to false means that the module can expect this variable to
+	// never be null.
+	Nullable    bool
+	NullableSet bool
+
 	DeclRange hcl.Range
 }
 
@@ -110,6 +116,16 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno
 		v.SensitiveSet = true
 	}
 
+	if attr, exists := content.Attributes["nullable"]; exists {
+		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &v.Nullable)
+		diags = append(diags, valDiags...)
+		v.NullableSet = true
+	} else {
+		// The current default is true, which is subject to change in a future
+		// language edition.
+		v.Nullable = true
+	}
+
 	if attr, exists := content.Attributes["default"]; exists {
 		val, valDiags := attr.Expr.Value(nil)
 		diags = append(diags, valDiags...)
@@ -134,6 +150,15 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno
 			}
 		}
 
+		if !v.Nullable && val.IsNull() {
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid default value for variable",
+				Detail:   "A null default value is not valid when nullable=false.",
+				Subject:  attr.Expr.Range().Ptr(),
+			})
+		}
+
 		v.Default = val
 	}
 
@@ -556,6 +581,9 @@ var variableBlockSchema = &hcl.BodySchema{
 		{
 			Name: "sensitive",
 		},
+		{
+			Name: "nullable",
+		},
 	},
 	Blocks: []hcl.BlockHeaderSchema{
 		{

internal/configs/testdata/invalid-modules/nullable-with-default-null/main.tf

@@ -0,0 +1,5 @@
+variable "in" {
+  type = number
+  nullable = false
+  default = null
+}

internal/configs/testdata/valid-files/variables.tf

@@ -30,3 +30,15 @@ variable "sensitive_value" {
   }
   sensitive = true
 }
+
+variable "nullable" {
+  type = string
+  nullable = true
+  default = "ok"
+}
+
+variable "nullable_default_null" {
+  type = map(string)
+  nullable = true
+  default = null
+}

internal/configs/testdata/valid-modules/override-variable/b_override.tf

@@ -1,4 +1,5 @@
 variable "fully_overridden" {
+  nullable = false
   default = "b_override"
   description = "b_override description"
   type = string

internal/terraform/context_apply2_test.go

@@ -596,3 +596,36 @@ resource "test_object" "x" {
 	}
 
 }
+
+func TestContext2Apply_nullableVariables(t *testing.T) {
+	m := testModule(t, "apply-nullable-variables")
+	state := states.NewState()
+	ctx := testContext2(t, &ContextOpts{})
+	plan, diags := ctx.Plan(m, state, &PlanOpts{})
+	if diags.HasErrors() {
+		t.Fatalf("plan: %s", diags.Err())
+	}
+	state, diags = ctx.Apply(plan, m)
+	if diags.HasErrors() {
+		t.Fatalf("apply: %s", diags.Err())
+	}
+
+	outputs := state.Module(addrs.RootModuleInstance).OutputValues
+	// we check for null outputs be seeing that they don't exists
+	if _, ok := outputs["nullable_null_default"]; ok {
+		t.Error("nullable_null_default: expected no output value")
+	}
+	if _, ok := outputs["nullable_non_null_default"]; ok {
+		t.Error("nullable_non_null_default: expected no output value")
+	}
+	if _, ok := outputs["nullable_no_default"]; ok {
+		t.Error("nullable_no_default: expected no output value")
+	}
+
+	if v := outputs["non_nullable_default"].Value; v.AsString() != "ok" {
+		t.Fatalf("incorrect 'non_nullable_default' output value: %#v\n", v)
+	}
+	if v := outputs["non_nullable_no_default"].Value; v.AsString() != "ok" {
+		t.Fatalf("incorrect 'non_nullable_no_default' output value: %#v\n", v)
+	}
+}

internal/terraform/evaluate.go

@@ -268,11 +268,15 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd
 	}
 
 	val, isSet := vals[addr.Name]
-	if !isSet {
+	switch {
-		if config.Default != cty.NilVal {
+	case !isSet:
-			return config.Default, diags
+		// The config loader will ensure there is a default if the value is not
-		}
+		// set at all.
-		return cty.UnknownVal(config.Type), diags
+		val = config.Default
+
+	case val.IsNull() && !config.Nullable && config.Default != cty.NilVal:
+		// If nullable=false a null value will use the configured default.
+		val = config.Default
 	}
 
 	var err error
@@ -286,8 +290,6 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd
 			Detail:   fmt.Sprintf(`The resolved value of variable %q is not appropriate: %s.`, addr.Name, err),
 			Subject:  &config.DeclRange,
 		})
-		// Stub out our return value so that the semantic checker doesn't
-		// produce redundant downstream errors.
 		val = cty.UnknownVal(config.Type)
 	}
 

internal/terraform/node_module_variable.go

@@ -253,7 +253,23 @@ func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnl
 		val = cty.UnknownVal(n.Config.Type)
 	}
 
+	// If there is no default, we have to ensure that a null value is allowed
+	// for this variable.
+	if n.Config.Default == cty.NilVal && !n.Config.Nullable && val.IsNull() {
+		// The value cannot be null, and there is no configured default.
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  `Invalid variable value`,
+			Detail:   fmt.Sprintf(`The variable %q is required, but the given value is null.`, n.Addr),
+			Subject:  &n.Config.DeclRange,
+		})
+		// Stub out our return value so that the semantic checker doesn't
+		// produce redundant downstream errors.
+		val = cty.UnknownVal(n.Config.Type)
+	}
+
 	vals := make(map[string]cty.Value)
 	vals[name] = val
+
 	return vals, diags.ErrWithWarnings()
 }

internal/terraform/testdata/apply-nullable-variables/main.tf

@@ -0,0 +1,28 @@
+module "mod" {
+  source = "./mod"
+  nullable_null_default = null
+  nullable_non_null_default = null
+  nullable_no_default = null
+  non_nullable_default = null
+  non_nullable_no_default = "ok"
+}
+
+output "nullable_null_default" {
+  value = module.mod.nullable_null_default
+}
+
+output "nullable_non_null_default" {
+  value = module.mod.nullable_non_null_default
+}
+
+output "nullable_no_default" {
+  value = module.mod.nullable_no_default
+}
+
+output "non_nullable_default" {
+  value = module.mod.non_nullable_default
+}
+
+output "non_nullable_no_default" {
+  value = module.mod.non_nullable_no_default
+}

internal/terraform/testdata/apply-nullable-variables/mod/main.tf

@@ -0,0 +1,59 @@
+// optional, and this can take null as an input
+variable "nullable_null_default" {
+  // This is implied now as the default, and probably should be implied even
+  // when nullable=false is the default, so we're leaving this unset for the test.
+  // nullable = true
+
+  default = null
+}
+
+// assigning null can still override the default.
+variable "nullable_non_null_default" {
+  nullable = true
+  default = "ok"
+}
+
+// required, and assigning null is valid.
+variable "nullable_no_default" {
+  nullable = true
+}
+
+
+// this combination is invalid
+//variable "non_nullable_null_default" {
+//  nullable = false
+//  default = null
+//}
+
+
+// assigning null will take the default
+variable "non_nullable_default" {
+  nullable = false
+  default = "ok"
+}
+
+// required, but null is not a valid value
+variable "non_nullable_no_default" {
+  nullable = false
+}
+
+output "nullable_null_default" {
+  value = var.nullable_null_default
+}
+
+output "nullable_non_null_default" {
+  value = var.nullable_non_null_default
+}
+
+output "nullable_no_default" {
+  value = var.nullable_no_default
+}
+
+output "non_nullable_default" {
+  value = var.non_nullable_default
+}
+
+output "non_nullable_no_default" {
+  value = var.non_nullable_no_default
+}
+