From e080706e2e383d0f58f3e878be6340b116b10240 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Mar 2019 19:14:17 -0400 Subject: [PATCH 1/2] treat normalization during ReadResource like Plan This will allow resources to return an unexpected change to set blocks and attributes, otherwise we could mask these changes during normalization. Change the "plan" argument in normalizeNullValues to "preferDst" to more accurately describe what the option is doing, since it no longer applies only to PlanResourceChange. --- helper/plugin/grpc_provider.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index 6cdaaa1d98..7d2d258edf 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -448,7 +448,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso return resp, nil } - newStateVal = normalizeNullValues(newStateVal, stateVal, false) + newStateVal = normalizeNullValues(newStateVal, stateVal, true) newStateVal = copyTimeoutValues(newStateVal, stateVal) newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType()) @@ -1078,7 +1078,7 @@ func stripSchema(s *schema.Schema) *schema.Schema { // 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 { +func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value { ty := dst.Type() if !src.IsNull() && !src.IsKnown() { @@ -1121,20 +1121,20 @@ func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value { dstVal := dstMap[key] if dstVal == cty.NilVal { - if plan && ty.IsMapType() { + if preferDst && ty.IsMapType() { // let plan shape this map however it wants continue } dstVal = cty.NullVal(v.Type()) } - dstMap[key] = normalizeNullValues(dstVal, v, plan) + dstMap[key] = normalizeNullValues(dstVal, v, preferDst) } // 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() && !plan { + if dst.IsNull() && src.IsWhollyKnown() && !preferDst { return src } return dst @@ -1150,7 +1150,7 @@ func normalizeNullValues(dst, src cty.Value, plan bool) 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() && !plan { + if src.IsWhollyKnown() && !preferDst { return src } @@ -1158,7 +1158,7 @@ func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value { // If the dst is nil, and the src is known, then we lost an empty value // so take the original. if dst.IsNull() { - if src.IsWhollyKnown() && src.LengthInt() == 0 && !plan { + if src.IsWhollyKnown() && src.LengthInt() == 0 && !preferDst { return src } return dst @@ -1172,7 +1172,7 @@ func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value { dsts := dst.AsValueSlice() for i := 0; i < srcLen; i++ { - dsts[i] = normalizeNullValues(dsts[i], srcs[i], plan) + dsts[i] = normalizeNullValues(dsts[i], srcs[i], preferDst) } if ty.IsTupleType() { @@ -1182,7 +1182,7 @@ func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value { } case ty.IsPrimitiveType(): - if dst.IsNull() && src.IsWhollyKnown() && !plan { + if dst.IsNull() && src.IsWhollyKnown() && !preferDst { return src } } From 4006c0fc84ec2b72365014f622b5bebe0db89466 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Mar 2019 19:17:38 -0400 Subject: [PATCH 2/2] add test for set value drift The changed value for the set attribute should be correctly read from the provider. --- builtin/providers/test/resource_test.go | 58 +++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index d697447726..350a76841a 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" ) @@ -732,3 +733,60 @@ resource "test_resource" "foo" { }, }) } + +func TestResource_setDrift(t *testing.T) { + testProvider := testAccProviders["test"] + res := testProvider.(*schema.Provider).ResourcesMap["test_resource"] + + // reset the Read function after the test + defer func() { + res.Read = testResourceRead + }() + + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "first" + required_map = { + a = "a" + } + set = ["a", "b"] +} +`), + Check: func(s *terraform.State) error { + return nil + }, + }, + resource.TestStep{ + PreConfig: func() { + // update the Read function to return the wrong "set" attribute values. + res.Read = func(d *schema.ResourceData, meta interface{}) error { + // update as expected first + if err := testResourceRead(d, meta); err != nil { + return err + } + d.Set("set", []interface{}{"a", "x"}) + return nil + } + }, + // Leave the config, so we can detect the mismatched set values. + // Updating the config would force the test to pass even if the Read + // function values were ignored. + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "second" + required_map = { + a = "a" + } + set = ["a", "b"] +} +`), + ExpectNonEmptyPlan: true, + }, + }, + }) +}