From ba081f5de4207036d4f31b259318c2c2bfa1f4fb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 31 Jan 2019 15:40:56 -0500 Subject: [PATCH] change copyMissingValues to normalizeNullValues While copyMissingValues was meant to re-insert empty values that were null after apply, it turns out plan is sometimes not predictable as well. normalizeNullValue is meant to fix up any null/empty transitions between to values, and be useful during plan as well. For plan the function only concerns itself with individual, known values, and skips sets entirely. The result of running with plan == true is that only changes between empty and null collections should be fixed. --- helper/plugin/grpc_provider.go | 86 +++++++++++---- helper/plugin/grpc_provider_test.go | 161 +++++++++++++++++++++++++++- 2 files changed, 225 insertions(+), 22 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index b3b5d1af5..da048489c 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -454,7 +454,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso } newStateVal = copyTimeoutValues(newStateVal, stateVal) - newStateVal = copyMissingValues(newStateVal, stateVal) + newStateVal = normalizeNullValues(newStateVal, stateVal, false) newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType()) if err != nil { @@ -508,7 +508,6 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl // turn the proposed state into a legacy configuration cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, block) - diff, err := s.provider.SimpleDiff(info, priorState, cfg) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -549,6 +548,9 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } + + plannedStateVal = normalizeNullValues(plannedStateVal, proposedNewStateVal, true) + if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -768,7 +770,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A return resp, nil } - newStateVal = copyMissingValues(newStateVal, plannedStateVal) + newStateVal = normalizeNullValues(newStateVal, plannedStateVal, false) newStateVal = copyTimeoutValues(newStateVal, plannedStateVal) newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType()) @@ -1130,15 +1132,39 @@ func stripSchema(s *schema.Schema) *schema.Schema { return newSchema } -// Zero values and empty containers may be lost during apply. Copy zero values -// and empty containers from src to dst when they are missing in dst. -// This takes a little more liberty with set types, since we can't correlate -// modified set values. In the case of sets, if the src set was wholly known we -// assume the value was correctly applied and copy that entirely to the new -// value. -func copyMissingValues(dst, src cty.Value) cty.Value { +// Zero values and empty containers may be interchanged by the apply process. +// When there is a discrepency between src and dst value being null or empty, +// prefer the src value. This takes a little more liberty with set types, since +// we can't correlate modified set values. In the case of sets, if the src set +// was wholly known we assume the value was correctly applied and copy that +// entirely to the new value. +// While apply prefers the src value, during plan we prefer dst whenever there +// is an unknown or a set is involved, since the plan can alter the value +// however it sees fit. This however means that a CustomizeDiffFunction may not +// be able to change a null to an empty value or vice versa, but that should be +// very uncommon nor was it reliable before 0.12 either. +func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value { ty := dst.Type() + if !src.IsNull() && !src.IsKnown() { + return dst + } + + // handle null/empty changes for collections + if ty.IsCollectionType() { + if src.IsNull() && !dst.IsNull() && dst.IsKnown() { + if dst.LengthInt() == 0 { + return src + } + } + + if dst.IsNull() && !src.IsNull() && src.IsKnown() { + if src.LengthInt() == 0 { + return src + } + } + } + if src.IsNull() || !src.IsKnown() || !dst.IsKnown() { return dst } @@ -1159,16 +1185,20 @@ func copyMissingValues(dst, src cty.Value) cty.Value { dstVal := dstMap[key] if dstVal == cty.NilVal { + if plan && ty.IsMapType() { + // let plan shape this map however it wants + continue + } dstVal = cty.NullVal(ty.ElementType()) } - dstMap[key] = copyMissingValues(dstVal, v) + dstMap[key] = normalizeNullValues(dstVal, v, plan) } // you can't call MapVal/ObjectVal with empty maps, but nothing was // copied in anyway. If the dst is nil, and the src is known, assume the // src is correct. if len(dstMap) == 0 { - if dst.IsNull() && src.IsWhollyKnown() { + if dst.IsNull() && src.IsWhollyKnown() && !plan { return src } return dst @@ -1184,21 +1214,39 @@ func copyMissingValues(dst, src cty.Value) cty.Value { // If the original was wholly known, then we expect that is what the // provider applied. The apply process loses too much information to // reliably re-create the set. - if src.IsWhollyKnown() { + if src.IsWhollyKnown() && !plan { return src } case ty.IsListType(), ty.IsTupleType(): // If the dst is nil, and the src is known, then we lost an empty value - // so take the original. This doesn't attempt to descend into the list - // values, since missing empty values may prevent us from correlating - // the correct src and dst indexes. - if dst.IsNull() && src.IsWhollyKnown() { - return src + // so take the original. + if dst.IsNull() { + if src.IsWhollyKnown() && !plan { + return src + } + return dst + } + + // if the lengths are identical, then iterate over each element in succession. + srcLen := src.LengthInt() + dstLen := dst.LengthInt() + if srcLen == dstLen && srcLen > 0 { + srcs := src.AsValueSlice() + dsts := dst.AsValueSlice() + + for i := 0; i < srcLen; i++ { + dsts[i] = normalizeNullValues(dsts[i], srcs[i], plan) + } + + if ty.IsTupleType() { + return cty.TupleVal(dsts) + } + return cty.ListVal(dsts) } case ty.IsPrimitiveType(): - if dst.IsNull() && src.IsWhollyKnown() { + if dst.IsNull() && src.IsWhollyKnown() && !plan { return src } } diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index 96799b868..aee4ff517 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -719,9 +719,10 @@ func TestNormalizeFlatmapContainers(t *testing.T) { } } -func TestCopyMissingValues(t *testing.T) { +func TestNormalizeNullValues(t *testing.T) { for i, tc := range []struct { Src, Dst, Expect cty.Value + Plan bool }{ { // The known set value is copied over the null set value @@ -745,6 +746,28 @@ func TestCopyMissingValues(t *testing.T) { }), }), }, + { + // The known set value is copied over the null set value + Src: cty.ObjectVal(map[string]cty.Value{ + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.NullVal(cty.String), + }), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "foo": cty.String, + }))), + }), + // If we're only in a plan, we can't compare sets at all + Expect: cty.ObjectVal(map[string]cty.Value{ + "set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "foo": cty.String, + }))), + }), + Plan: true, + }, { // The empty map is copied over the null map Src: cty.ObjectVal(map[string]cty.Value{ @@ -758,7 +781,7 @@ func TestCopyMissingValues(t *testing.T) { }), }, { - // A zerp value primitive is copied over a null primitive + // A zero value primitive is copied over a null primitive Src: cty.ObjectVal(map[string]cty.Value{ "string": cty.StringVal(""), }), @@ -769,6 +792,19 @@ func TestCopyMissingValues(t *testing.T) { "string": cty.StringVal(""), }), }, + { + // Plan primitives are kept + Src: cty.ObjectVal(map[string]cty.Value{ + "string": cty.StringVal(""), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "string": cty.NullVal(cty.String), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "string": cty.NullVal(cty.String), + }), + Plan: true, + }, { // The null map is retained, because the src was unknown Src: cty.ObjectVal(map[string]cty.Value{ @@ -821,9 +857,128 @@ func TestCopyMissingValues(t *testing.T) { }), }), }, + { + // Retain don't re-add unexpected planned values in a map + Src: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + "b": cty.StringVal(""), + }), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a"), + }), + }), + Plan: true, + }, + + // a list in an object in a list, going from null to empty + { + Src: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.UnknownVal(cty.String), + "access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))), + "address": cty.NullVal(cty.String), + "name": cty.StringVal("nic0"), + })}), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.StringVal("10.128.0.64"), + "access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})), + "address": cty.StringVal("address"), + "name": cty.StringVal("nic0"), + }), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.StringVal("10.128.0.64"), + "access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))), + "address": cty.StringVal("address"), + "name": cty.StringVal("nic0"), + }), + }), + }), + }, + + // a list in an object in a list, going from empty to null + { + Src: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.UnknownVal(cty.String), + "access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})), + "address": cty.NullVal(cty.String), + "name": cty.StringVal("nic0"), + })}), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.StringVal("10.128.0.64"), + "access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))), + "address": cty.StringVal("address"), + "name": cty.StringVal("nic0"), + }), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.StringVal("10.128.0.64"), + "access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})), + "address": cty.StringVal("address"), + "name": cty.StringVal("nic0"), + }), + }), + }), + }, + // the empty list should be transferred, but the new unknown show not be overridden + { + Src: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.StringVal("10.128.0.64"), + "access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})), + "address": cty.NullVal(cty.String), + "name": cty.StringVal("nic0"), + })}), + }), + Dst: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.UnknownVal(cty.String), + "access_config": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String}))), + "address": cty.StringVal("address"), + "name": cty.StringVal("nic0"), + }), + }), + }), + Expect: cty.ObjectVal(map[string]cty.Value{ + "network_interface": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "network_ip": cty.UnknownVal(cty.String), + "access_config": cty.ListValEmpty(cty.Object(map[string]cty.Type{"public_ptr_domain_name": cty.String, "nat_ip": cty.String})), + "address": cty.StringVal("address"), + "name": cty.StringVal("nic0"), + }), + }), + }), + Plan: true, + }, } { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - got := copyMissingValues(tc.Dst, tc.Src) + got := normalizeNullValues(tc.Dst, tc.Src, tc.Plan) if !got.RawEquals(tc.Expect) { t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.Expect, got) }