From be682f1d29cea7e9641dd72d8e4ccf97e1849d63 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 4 May 2023 14:20:45 -0400 Subject: [PATCH] only refresh an import state once The imported resource was being stored in the wrong state, and only ended up in the refresh state because ReadResource was being called a second time in the normal refresh path. Make sure to only refresh the imported resource once. This is still done separately within importState so that we can handle the error slightly differently to let the user know if an imported instance does not exist. --- internal/terraform/context_plan2_test.go | 60 +++++++++++++++++++ .../terraform/node_resource_plan_instance.go | 38 ++++++++---- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 45822e9017..3d62058bc0 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -4301,3 +4301,63 @@ import { } }) } + +func TestContext2Plan_importRefreshOnce(t *testing.T) { + addr := mustResourceInstanceAddr("test_object.a") + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { + test_string = "bar" +} + +import { + to = test_object.a + id = "123" +} +`, + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + readCalled := 0 + p.ReadResourceFn = func(req providers.ReadResourceRequest) providers.ReadResourceResponse { + readCalled++ + state, _ := simpleTestSchema().CoerceValue(cty.ObjectVal(map[string]cty.Value{ + "test_string": cty.StringVal("foo"), + })) + + return providers.ReadResourceResponse{ + NewState: state, + } + } + + p.ImportResourceStateResponse = &providers.ImportResourceStateResponse{ + ImportedResources: []providers.ImportedResource{ + { + TypeName: "test_object", + State: cty.ObjectVal(map[string]cty.Value{ + "test_string": cty.StringVal("foo"), + }), + }, + }, + } + + _, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ + Mode: plans.NormalMode, + ForceReplace: []addrs.AbsResourceInstance{ + addr, + }, + }) + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } + + if readCalled > 1 { + t.Error("ReadResource called multiple times for import") + } +} diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index 2b97128d39..9e7908c890 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -152,9 +152,11 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } + importing := n.importTarget.ID != "" + // If the resource is to be imported, we now ask the provider for an Import // and a Refresh, and save the resulting state to instanceRefreshState. - if n.importTarget.ID != "" { + if importing { instanceRefreshState, diags = n.importState(ctx, addr, provider) } else { var readDiags tfdiags.Diagnostics @@ -192,7 +194,8 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) } // Refresh, maybe - if !n.skipRefresh { + // The import process handles its own refresh + if !n.skipRefresh && !importing { s, refreshDiags := n.refresh(ctx, states.NotDeposed, instanceRefreshState) diags = diags.Append(refreshDiags) if diags.HasErrors() { @@ -383,14 +386,15 @@ func (n *NodePlannableResourceInstance) replaceTriggered(ctx EvalContext, repDat return diags } -func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs.AbsResourceInstance, provider providers.Interface) (instanceRefreshState *states.ResourceInstanceObject, diags tfdiags.Diagnostics) { +func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs.AbsResourceInstance, provider providers.Interface) (*states.ResourceInstanceObject, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics absAddr := addr.Resource.Absolute(ctx.Path()) diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { return h.PreImportState(absAddr, n.importTarget.ID) })) if diags.HasErrors() { - return instanceRefreshState, diags + return nil, diags } resp := provider.ImportResourceState(providers.ImportResourceStateRequest{ @@ -399,7 +403,7 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. }) diags = diags.Append(resp.Diagnostics) if diags.HasErrors() { - return instanceRefreshState, diags + return nil, diags } imported := resp.ImportedResources @@ -413,7 +417,7 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. n.importTarget.ID, ), )) - return instanceRefreshState, diags + return nil, diags } for _, obj := range imported { log.Printf("[TRACE] graphNodeImportState: import %s %q produced instance object of type %s", absAddr.String(), n.importTarget.ID, obj.TypeName) @@ -428,7 +432,7 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. n.importTarget.ID, ), )) - return instanceRefreshState, diags + return nil, diags } // call post-import hook @@ -438,11 +442,22 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. if imported[0].TypeName == "" { diags = diags.Append(fmt.Errorf("import of %s didn't set type", n.importTarget.Addr.String())) - return instanceRefreshState, diags + return nil, diags } importedState := imported[0].AsInstanceObject() + if importedState.Value.IsNull() { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Import returned null resource", + fmt.Sprintf("While attempting to import with ID %s, the provider"+ + "returned an instance with no state.", + n.importTarget.ID, + ), + )) + } + // refresh riNode := &NodeAbstractResourceInstance{ Addr: n.importTarget.Addr, @@ -450,14 +465,14 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. ResolvedProvider: n.ResolvedProvider, }, } - importedState, refreshDiags := riNode.refresh(ctx, states.NotDeposed, importedState) + instanceRefreshState, refreshDiags := riNode.refresh(ctx, states.NotDeposed, importedState) diags = diags.Append(refreshDiags) if diags.HasErrors() { return instanceRefreshState, diags } // verify the existence of the imported resource - if importedState.Value.IsNull() { + if instanceRefreshState.Value.IsNull() { var diags tfdiags.Diagnostics diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -475,8 +490,7 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. return instanceRefreshState, diags } - diags = diags.Append(riNode.writeResourceInstanceState(ctx, importedState, workingState)) - instanceRefreshState = importedState + diags = diags.Append(riNode.writeResourceInstanceState(ctx, instanceRefreshState, refreshState)) return instanceRefreshState, diags }