Merge pull request #32424 from hashicorp/jbardin/reify-null-blocks

fix invalid null blocks during refresh

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)

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

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 for %q during refresh\n", n.ResolvedProvider.Provider, n.Addr)
+	}
+
+	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)