aherman
/
terraform
mirror of https://github.com/hashicorp/terraform
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

change copyMissingValues to normalizeNullValues

Browse Source 
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.
pull/20178/head
James Bardin 7 years ago
parent a8f97a0805
commit ba081f5de4
2 changed files with 225 additions and 22 deletions
Show Stats Download Patch File Download Diff File

86
helper/plugin/grpc_provider.go
Unescape View File

@ -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
}
}

161
helper/plugin/grpc_provider_test.go
Unescape View File

@ -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)
}

Write Preview
Loading…
Cancel
Save