From f5395bd98a08dfcc4327e71eeb816885a536b2cc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 2 Apr 2019 16:53:51 -0400 Subject: [PATCH 1/2] 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 ca122af588..fac4a63fec 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 ce31cb51c8..a32f5d1dbe 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()) + } + } + }) + } +} From e024960c745f0a464cb081d04765d93f94deaba0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 2 Apr 2019 17:04:09 -0400 Subject: [PATCH 2/2] Revert "filter nulls when shimming a config" This reverts commit 97bde5467c8ff8f791e0556fe809b2d8bf11f511. --- config/hcl2shim/values.go | 5 +---- config/hcl2shim/values_test.go | 20 -------------------- helper/plugin/grpc_provider.go | 3 ++- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/config/hcl2shim/values.go b/config/hcl2shim/values.go index b5868f6edc..2c5b2907e6 100644 --- a/config/hcl2shim/values.go +++ b/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 } diff --git a/config/hcl2shim/values_test.go b/config/hcl2shim/values_test.go index ac70c339e4..7c3011da05 100644 --- a/config/hcl2shim/values_test.go +++ b/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 { diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index fac4a63fec..f7ace8b5a3 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -1342,7 +1342,8 @@ func validateConfigNulls(v cty.Value, path cty.Path) []*proto.Diagnostic { if ev.IsNull() { diags = append(diags, &proto.Diagnostic{ Severity: proto.Diagnostic_ERROR, - Summary: "null value found in list", + 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