From 0c1aaba63593c653961cc9595a40855435b1e53f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 21 Dec 2022 10:18:26 -0500 Subject: [PATCH 1/3] fix invalid null blocks during refresh Legacy providers may return null values for nested blocks during refresh. Because the ReadResource call needs to accept any value to allow the provider to report external changes, we allowed all changes to the value as long as the underlying cty.Type was correct, allowing null block values to be inserted into the state. While technically invalid, we needed to accept these null values for compatibility, and they were mostly seen as a nuisance, causing noise in external changes and plan output. These null block values however can be inserted into the effective configuration with the use of `ignore_changes`, which can cause problems where the configuration is assumed to be completely valid. Rather than accept the null values, we can insert empty container values for these blocks when refreshing the instance, which will prevent any invalid values from entering state at all. Because these must still be accepted for compatibility, we can only log the difference as a warning. Currently the NormalizeObjectFromLegacySDK does not report which specific blocks it fixed, so we just log a generic message. --- internal/terraform/context_refresh_test.go | 85 +++++++++++++++++++ .../node_resource_abstract_instance.go | 18 ++-- 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/internal/terraform/context_refresh_test.go b/internal/terraform/context_refresh_test.go index ca456cee9e..9b4f4dab55 100644 --- a/internal/terraform/context_refresh_test.go +++ b/internal/terraform/context_refresh_test.go @@ -1598,3 +1598,88 @@ func TestContext2Refresh_dataSourceOrphan(t *testing.T) { t.Fatal("orphaned data source instance should not be read") } } + +// Legacy providers may return invalid null values for blocks, causing noise in +// the diff output and unexpected behavior with ignore_changes. Make sure +// refresh fixes these up before storing the state. +func TestContext2Refresh_reifyNullBlock(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_resource" "foo" { +} +`, + }) + + p := new(MockProvider) + p.ReadResourceFn = func(req providers.ReadResourceRequest) providers.ReadResourceResponse { + // incorrectly return a null _set_block value + v := req.PriorState.AsValueMap() + v["set_block"] = cty.NullVal(v["set_block"].Type()) + return providers.ReadResourceResponse{NewState: cty.ObjectVal(v)} + } + + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + Provider: &configschema.Block{}, + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "set_block": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "a": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingSet, + }, + }, + }, + }, + }) + p.PlanResourceChangeFn = testDiffFn + + fooAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }.Instance(addrs.NoKey) + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + fooAddr, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo", "network_interface":[]}`), + Dependencies: []addrs.ConfigResource{}, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, state, &PlanOpts{Mode: plans.RefreshOnlyMode}) + if diags.HasErrors() { + t.Fatalf("refresh errors: %s", diags.Err()) + } + + jsonState := plan.PriorState.ResourceInstance(fooAddr.Absolute(addrs.RootModuleInstance)).Current.AttrsJSON + + // the set_block should still be an empty container, and not null + expected := `{"id":"foo","set_block":[]}` + if string(jsonState) != expected { + t.Fatalf("invalid state\nexpected: %s\ngot: %s\n", expected, jsonState) + } +} diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 437d853021..3506553e17 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -598,11 +598,23 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state return state, diags } + newState := objchange.NormalizeObjectFromLegacySDK(resp.NewState, schema) + if !newState.RawEquals(resp.NewState) { + // We had to fix up this object in some way, and we still need to + // accept any changes for compatibility, so all we can do is log a + // warning about the change. + log.Printf("[WARN] Provider %q produced an invalid new value containing null blocks during refresh\n", n.ResolvedProvider.Provider) + } + + ret := state.DeepCopy() + ret.Value = newState + ret.Private = resp.Private + // We have no way to exempt provider using the legacy SDK from this check, // so we can only log inconsistencies with the updated state values. // In most cases these are not errors anyway, and represent "drift" from // external changes which will be handled by the subsequent plan. - if errs := objchange.AssertObjectCompatible(schema, priorVal, resp.NewState); len(errs) > 0 { + if errs := objchange.AssertObjectCompatible(schema, priorVal, ret.Value); len(errs) > 0 { var buf strings.Builder fmt.Fprintf(&buf, "[WARN] Provider %q produced an unexpected new value for %s during refresh.", n.ResolvedProvider.Provider.String(), absAddr) for _, err := range errs { @@ -611,10 +623,6 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state log.Print(buf.String()) } - ret := state.DeepCopy() - ret.Value = resp.NewState - ret.Private = resp.Private - // Call post-refresh hook diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { return h.PostRefresh(absAddr, hookGen, priorVal, ret.Value) From a6098b67fa811a5089fefb67d8835caf9176fbd8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 21 Dec 2022 10:47:07 -0500 Subject: [PATCH 2/3] fix test state --- internal/command/plan_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/command/plan_test.go b/internal/command/plan_test.go index 9e145e6bf3..2f81c421f3 100644 --- a/internal/command/plan_test.go +++ b/internal/command/plan_test.go @@ -437,10 +437,10 @@ func TestPlan_state(t *testing.T) { expected := cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("bar"), "ami": cty.NullVal(cty.String), - "network_interface": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{ + "network_interface": cty.ListValEmpty(cty.Object(map[string]cty.Type{ "device_index": cty.String, "description": cty.String, - }))), + })), }) if !expected.RawEquals(actual) { t.Fatalf("wrong prior state\ngot: %#v\nwant: %#v", actual, expected) @@ -479,10 +479,10 @@ func TestPlan_stateDefault(t *testing.T) { expected := cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("bar"), "ami": cty.NullVal(cty.String), - "network_interface": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{ + "network_interface": cty.ListValEmpty(cty.Object(map[string]cty.Type{ "device_index": cty.String, "description": cty.String, - }))), + })), }) if !expected.RawEquals(actual) { t.Fatalf("wrong prior state\ngot: %#v\nwant: %#v", actual, expected) From d493e998e1ef4ed5f092352ab8c6b617c44b2367 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 21 Dec 2022 10:53:11 -0500 Subject: [PATCH 3/3] add resource address to log message --- internal/terraform/node_resource_abstract_instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 3506553e17..4dabb7bedb 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -603,7 +603,7 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state // We had to fix up this object in some way, and we still need to // accept any changes for compatibility, so all we can do is log a // warning about the change. - log.Printf("[WARN] Provider %q produced an invalid new value containing null blocks during refresh\n", n.ResolvedProvider.Provider) + log.Printf("[WARN] Provider %q produced an invalid new value containing null blocks for %q during refresh\n", n.ResolvedProvider.Provider, n.Addr) } ret := state.DeepCopy()