Merge pull request #20909 from hashicorp/jbardin/validate-config-nulls

Validate configuration shims for nulls in lists

config/hcl2shim/values.go

@@ -167,10 +167,7 @@ func ConfigValueFromHCL2(v cty.Value) interface{} {
 		it := v.ElementIterator()
 		for it.Next() {
 			_, ev := it.Element()
-			cv := ConfigValueFromHCL2(ev)
-			if cv != nil {
-				l = append(l, cv)
-			}
+			l = append(l, ConfigValueFromHCL2(ev))
 		}
 		return l
 	}

config/hcl2shim/values_test.go

@@ -232,26 +232,6 @@ func TestConfigValueFromHCL2Block(t *testing.T) {
 			&configschema.Block{},
 			nil,
 		},
-		// nulls should be filtered out of the config, since they couldn't exist
-		// in hcl.
-		{
-			cty.ObjectVal(map[string]cty.Value{
-				"list": cty.ListVal([]cty.Value{
-					cty.StringVal("ok"),
-					cty.NullVal(cty.String)}),
-			}),
-			&configschema.Block{
-				Attributes: map[string]*configschema.Attribute{
-					"list": {
-						Type:     cty.List(cty.String),
-						Optional: true,
-					},
-				},
-			},
-			map[string]interface{}{
-				"list": []interface{}{"ok"},
-			},
-		},
 	}
 
 	for _, test := range tests {

helper/plugin/grpc_provider.go

@@ -183,6 +183,12 @@ func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto
 		return resp, nil
 	}
 
+	// Ensure there are no nulls that will cause helper/schema to panic.
+	if err := validateConfigNulls(configVal, nil); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
 	config := terraform.NewResourceConfigShimmed(configVal, blockForShimming)
 	schema.FixupAsSingleResourceConfigIn(config, s.provider.Schema)
 
@@ -233,6 +239,12 @@ func (s *GRPCProviderServer) ValidateDataSourceConfig(_ context.Context, req *pr
 		return resp, nil
 	}
 
+	// Ensure there are no nulls that will cause helper/schema to panic.
+	if err := validateConfigNulls(configVal, nil); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
 	config := terraform.NewResourceConfigShimmed(configVal, blockForShimming)
 	schema.FixupAsSingleResourceConfigIn(config, s.provider.DataSourcesMap[req.TypeName].Schema)
 
@@ -424,6 +436,12 @@ func (s *GRPCProviderServer) Configure(_ context.Context, req *proto.Configure_R
 
 	s.provider.TerraformVersion = req.TerraformVersion
 
+	// Ensure there are no nulls that will cause helper/schema to panic.
+	if err := validateConfigNulls(configVal, nil); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
 	config := terraform.NewResourceConfigShimmed(configVal, blockForShimming)
 	schema.FixupAsSingleResourceConfigIn(config, s.provider.Schema)
 	err = s.provider.Configure(config)
@@ -553,6 +571,12 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 
 	priorState.Meta = priorPrivate
 
+	// Ensure there are no nulls that will cause helper/schema to panic.
+	if err := validateConfigNulls(proposedNewStateVal, nil); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
 	// turn the proposed state into a legacy configuration
 	cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, blockForShimming)
 	schema.FixupAsSingleResourceConfigIn(cfg, s.provider.ResourcesMap[req.TypeName].Schema)
@@ -978,6 +1002,12 @@ func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDa
 		Type: req.TypeName,
 	}
 
+	// Ensure there are no nulls that will cause helper/schema to panic.
+	if err := validateConfigNulls(configVal, nil); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
 	config := terraform.NewResourceConfigShimmed(configVal, blockForShimming)
 	schema.FixupAsSingleResourceConfigIn(config, s.provider.DataSourcesMap[req.TypeName].Schema)
 
@@ -1291,3 +1321,53 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value {
 
 	return dst
 }
+
+// validateConfigNulls checks a config value for unsupported nulls before
+// attempting to shim the value. While null values can mostly be ignored in the
+// configuration, since they're not supported in HCL1, the case where a null
+// appears in a list-like attribute (list, set, tuple) will present a nil value
+// to helper/schema which can panic. Return an error to the user in this case,
+// indicating the attribute with the null value.
+func validateConfigNulls(v cty.Value, path cty.Path) []*proto.Diagnostic {
+	var diags []*proto.Diagnostic
+	if v.IsNull() || !v.IsKnown() {
+		return diags
+	}
+
+	switch {
+	case v.Type().IsListType() || v.Type().IsSetType() || v.Type().IsTupleType():
+		it := v.ElementIterator()
+		for it.Next() {
+			kv, ev := it.Element()
+			if ev.IsNull() {
+				diags = append(diags, &proto.Diagnostic{
+					Severity:  proto.Diagnostic_ERROR,
+					Summary:   "Null value found in list",
+					Detail:    "Null values are not allowed for this attribute value.",
+					Attribute: convert.PathToAttributePath(append(path, cty.IndexStep{Key: kv})),
+				})
+				continue
+			}
+
+			d := validateConfigNulls(ev, append(path, cty.IndexStep{Key: kv}))
+			diags = convert.AppendProtoDiag(diags, d)
+		}
+
+	case v.Type().IsMapType() || v.Type().IsObjectType():
+		it := v.ElementIterator()
+		for it.Next() {
+			kv, ev := it.Element()
+			var step cty.PathStep
+			switch {
+			case v.Type().IsMapType():
+				step = cty.IndexStep{Key: kv}
+			case v.Type().IsObjectType():
+				step = cty.GetAttrStep{Name: kv.AsString()}
+			}
+			d := validateConfigNulls(ev, append(path, step))
+			diags = convert.AppendProtoDiag(diags, d)
+		}
+	}
+
+	return diags
+}

helper/plugin/grpc_provider_test.go

@@ -3,6 +3,7 @@ package plugin
 import (
 	"context"
 	"fmt"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -11,6 +12,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/hashicorp/terraform/helper/schema"
 	proto "github.com/hashicorp/terraform/internal/tfplugin5"
+	"github.com/hashicorp/terraform/plugin/convert"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/zclconf/go-cty/cty"
 	"github.com/zclconf/go-cty/cty/msgpack"
@@ -1010,3 +1012,70 @@ func TestNormalizeNullValues(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateNulls(t *testing.T) {
+	for i, tc := range []struct {
+		Cfg cty.Value
+		Err bool
+	}{
+		{
+			Cfg: cty.ObjectVal(map[string]cty.Value{
+				"list": cty.ListVal([]cty.Value{
+					cty.StringVal("string"),
+					cty.NullVal(cty.String),
+				}),
+			}),
+			Err: true,
+		},
+		{
+			Cfg: cty.ObjectVal(map[string]cty.Value{
+				"map": cty.MapVal(map[string]cty.Value{
+					"string": cty.StringVal("string"),
+					"null":   cty.NullVal(cty.String),
+				}),
+			}),
+			Err: false,
+		},
+		{
+			Cfg: cty.ObjectVal(map[string]cty.Value{
+				"object": cty.ObjectVal(map[string]cty.Value{
+					"list": cty.ListVal([]cty.Value{
+						cty.StringVal("string"),
+						cty.NullVal(cty.String),
+					}),
+				}),
+			}),
+			Err: true,
+		},
+		{
+			Cfg: cty.ObjectVal(map[string]cty.Value{
+				"object": cty.ObjectVal(map[string]cty.Value{
+					"list": cty.ListVal([]cty.Value{
+						cty.StringVal("string"),
+						cty.NullVal(cty.String),
+					}),
+					"list2": cty.ListVal([]cty.Value{
+						cty.StringVal("string"),
+						cty.NullVal(cty.String),
+					}),
+				}),
+			}),
+			Err: true,
+		},
+	} {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			d := validateConfigNulls(tc.Cfg, nil)
+			diags := convert.ProtoToDiagnostics(d)
+			switch {
+			case tc.Err:
+				if !diags.HasErrors() {
+					t.Fatal("expected error")
+				}
+			default:
+				if diags.HasErrors() {
+					t.Fatalf("unexpected error: %q", diags.Err())
+				}
+			}
+		})
+	}
+}