From f5395bd98a08dfcc4327e71eeb816885a536b2cc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 2 Apr 2019 16:53:51 -0400 Subject: [PATCH] validate null values in shimmed configs A list-like attribute containing null values will present a list to helper/schema with nils, which can cause panics. Since null values were not possible in configuration before HCL2 and not supported by the legacy SDK, return an error to the user. --- helper/plugin/grpc_provider.go | 79 +++++++++++++++++++++++++++++ helper/plugin/grpc_provider_test.go | 69 +++++++++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index ca122af58..fac4a63fe 100644 --- a/helper/plugin/grpc_provider.go +++ b/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,52 @@ 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", + 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 +} diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index ce31cb51c..a32f5d1db 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/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()) + } + } + }) + } +}