Merge pull request #29720 from hashicorp/jbardin/destroy-plan-deposed

don't refresh deposed instances during the destroy plan

internal/terraform/context_apply2_test.go

@@ -549,3 +549,50 @@ resource "test_object" "y" {
 		}
 	}
 }
+
+func TestContext2Apply_destroyWithDeposed(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_object" "x" {
+  test_string = "ok"
+  lifecycle {
+    create_before_destroy = true
+  }
+}`,
+	})
+
+	p := simpleMockProvider()
+
+	deposedKey := states.NewDeposedKey()
+
+	state := states.NewState()
+	root := state.EnsureModule(addrs.RootModuleInstance)
+	root.SetResourceInstanceDeposed(
+		mustResourceInstanceAddr("test_object.x").Resource,
+		deposedKey,
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectTainted,
+			AttrsJSON: []byte(`{"test_string":"deposed"}`),
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, state, &PlanOpts{
+		Mode: plans.DestroyMode,
+	})
+	if diags.HasErrors() {
+		t.Fatalf("plan: %s", diags.Err())
+	}
+
+	_, diags = ctx.Apply(plan, m)
+	if diags.HasErrors() {
+		t.Fatalf("apply: %s", diags.Err())
+	}
+
+}

internal/terraform/context_plan2_test.go

@@ -3,6 +3,7 @@ package terraform
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -419,7 +420,12 @@ resource "test_object" "a" {
 			},
 		},
 	}
+
+	// This is called from the first instance of this provider, so we can't
+	// check p.ReadResourceCalled after plan.
+	readResourceCalled := false
 	p.ReadResourceFn = func(req providers.ReadResourceRequest) (resp providers.ReadResourceResponse) {
+		readResourceCalled = true
 		newVal, err := cty.Transform(req.PriorState, func(path cty.Path, v cty.Value) (cty.Value, error) {
 			if len(path) == 1 && path[0] == (cty.GetAttrStep{Name: "arg"}) {
 				return cty.StringVal("current"), nil
@@ -435,7 +441,10 @@ resource "test_object" "a" {
 			NewState: newVal,
 		}
 	}
+
+	upgradeResourceStateCalled := false
 	p.UpgradeResourceStateFn = func(req providers.UpgradeResourceStateRequest) (resp providers.UpgradeResourceStateResponse) {
+		upgradeResourceStateCalled = true
 		t.Logf("UpgradeResourceState %s", req.RawStateJSON)
 
 		// In the destroy-with-refresh codepath we end up calling
@@ -479,10 +488,10 @@ resource "test_object" "a" {
 	})
 	assertNoErrors(t, diags)
 
-	if !p.UpgradeResourceStateCalled {
+	if !upgradeResourceStateCalled {
 		t.Errorf("Provider's UpgradeResourceState wasn't called; should've been")
 	}
-	if !p.ReadResourceCalled {
+	if !readResourceCalled {
 		t.Errorf("Provider's ReadResource wasn't called; should've been")
 	}
 
@@ -682,7 +691,7 @@ func TestContext2Plan_destroyNoProviderConfig(t *testing.T) {
 	p.ValidateProviderConfigFn = func(req providers.ValidateProviderConfigRequest) (resp providers.ValidateProviderConfigResponse) {
 		v := req.Config.GetAttr("test_string")
 		if v.IsNull() || !v.IsKnown() || v.AsString() != "ok" {
-			resp.Diagnostics = resp.Diagnostics.Append(errors.New("invalid provider configuration"))
+			resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("invalid provider configuration: %#v", req.Config))
 		}
 		return resp
 	}

internal/terraform/node_provider.go

@@ -38,6 +38,9 @@ func (n *NodeApplyableProvider) Execute(ctx EvalContext, op walkOperation) (diag
 		log.Printf("[TRACE] NodeApplyableProvider: validating configuration for %s", n.Addr)
 		return diags.Append(n.ValidateProvider(ctx, provider))
 	case walkPlan, walkApply, walkDestroy:
+		// walkPlanDestroy is purposely skipped here, since the config is not
+		// evaluated, and the provider is not needed to create delete actions
+		// for all instances.
 		log.Printf("[TRACE] NodeApplyableProvider: configuring %s", n.Addr)
 		return diags.Append(n.ConfigureProvider(ctx, provider, false))
 	case walkImport:

internal/terraform/node_resource_destroy_deposed.go

@@ -95,7 +95,12 @@ func (n *NodePlanDeposedResourceInstanceObject) Execute(ctx EvalContext, op walk
 		return diags
 	}
 
-	if !n.skipRefresh {
+	// We don't refresh during the planDestroy walk, since that is only adding
+	// the destroy changes to the plan and the provider will not be configured
+	// at this point. The other nodes use separate types for plan and destroy,
+	// while deposed instances are always a destroy operation, so the logic
+	// here is a bit overloaded.
+	if !n.skipRefresh && op != walkPlanDestroy {
 		// Refresh this object even though it is going to be destroyed, in
 		// case it's already been deleted outside of Terraform. If this is a
 		// normal plan, providers expect a Read request to remove missing

internal/terraform/node_resource_destroy_deposed_test.go

@@ -26,6 +26,7 @@ func TestNodePlanDeposedResourceInstanceObject_Execute(t *testing.T) {
 	)
 
 	p := testProvider("test")
+	p.ConfigureProvider(providers.ConfigureProviderRequest{})
 	p.UpgradeResourceStateResponse = &providers.UpgradeResourceStateResponse{
 		UpgradedState: cty.ObjectVal(map[string]cty.Value{
 			"id": cty.StringVal("bar"),
@@ -106,6 +107,7 @@ func TestNodeDestroyDeposedResourceInstanceObject_Execute(t *testing.T) {
 	}
 
 	p := testProvider("test")
+	p.ConfigureProvider(providers.ConfigureProviderRequest{})
 	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(schema)
 
 	p.UpgradeResourceStateResponse = &providers.UpgradeResourceStateResponse{

internal/terraform/node_resource_plan_orphan_test.go

@@ -33,6 +33,7 @@ func TestNodeResourcePlanOrphanExecute(t *testing.T) {
 	)
 
 	p := simpleMockProvider()
+	p.ConfigureProvider(providers.ConfigureProviderRequest{})
 	ctx := &MockEvalContext{
 		StateState:               state.SyncWrapper(),
 		RefreshStateState:        state.DeepCopy().SyncWrapper(),
@@ -93,6 +94,7 @@ func TestNodeResourcePlanOrphanExecute_alreadyDeleted(t *testing.T) {
 	changes := plans.NewChanges()
 
 	p := simpleMockProvider()
+	p.ConfigureProvider(providers.ConfigureProviderRequest{})
 	p.ReadResourceResponse = &providers.ReadResourceResponse{
 		NewState: cty.NullVal(p.GetProviderSchemaResponse.ResourceTypes["test_string"].Block.ImpliedType()),
 	}

internal/terraform/provider_mock.go

@@ -297,6 +297,11 @@ func (p *MockProvider) ReadResource(r providers.ReadResourceRequest) (resp provi
 	p.ReadResourceCalled = true
 	p.ReadResourceRequest = r
 
+	if !p.ConfigureProviderCalled {
+		resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("Configure not called before ReadResource %q", r.TypeName))
+		return resp
+	}
+
 	if p.ReadResourceFn != nil {
 		return p.ReadResourceFn(r)
 	}
@@ -330,6 +335,11 @@ func (p *MockProvider) PlanResourceChange(r providers.PlanResourceChangeRequest)
 	p.Lock()
 	defer p.Unlock()
 
+	if !p.ConfigureProviderCalled {
+		resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("Configure not called before PlanResourceChange %q", r.TypeName))
+		return resp
+	}
+
 	p.PlanResourceChangeCalled = true
 	p.PlanResourceChangeRequest = r
 
@@ -400,6 +410,11 @@ func (p *MockProvider) ApplyResourceChange(r providers.ApplyResourceChangeReques
 	p.ApplyResourceChangeRequest = r
 	p.Unlock()
 
+	if !p.ConfigureProviderCalled {
+		resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("Configure not called before ApplyResourceChange %q", r.TypeName))
+		return resp
+	}
+
 	if p.ApplyResourceChangeFn != nil {
 		return p.ApplyResourceChangeFn(r)
 	}
@@ -460,6 +475,11 @@ func (p *MockProvider) ImportResourceState(r providers.ImportResourceStateReques
 	p.Lock()
 	defer p.Unlock()
 
+	if !p.ConfigureProviderCalled {
+		resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("Configure not called before ImportResourceState %q", r.TypeName))
+		return resp
+	}
+
 	p.ImportResourceStateCalled = true
 	p.ImportResourceStateRequest = r
 	if p.ImportResourceStateFn != nil {
@@ -494,6 +514,11 @@ func (p *MockProvider) ReadDataSource(r providers.ReadDataSourceRequest) (resp p
 	p.Lock()
 	defer p.Unlock()
 
+	if !p.ConfigureProviderCalled {
+		resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("Configure not called before ReadDataSource %q", r.TypeName))
+		return resp
+	}
+
 	p.ReadDataSourceCalled = true
 	p.ReadDataSourceRequest = r
 

internal/terraform/terraform_test.go

@@ -166,6 +166,23 @@ func testSetResourceInstanceTainted(module *states.Module, resource, attrsJson,
 
 func testProviderFuncFixed(rp providers.Interface) providers.Factory {
 	return func() (providers.Interface, error) {
+		if p, ok := rp.(*MockProvider); ok {
+			// make sure none of the methods were "called" on this new instance
+			p.GetProviderSchemaCalled = false
+			p.ValidateProviderConfigCalled = false
+			p.ValidateResourceConfigCalled = false
+			p.ValidateDataResourceConfigCalled = false
+			p.UpgradeResourceStateCalled = false
+			p.ConfigureProviderCalled = false
+			p.StopCalled = false
+			p.ReadResourceCalled = false
+			p.PlanResourceChangeCalled = false
+			p.ApplyResourceChangeCalled = false
+			p.ImportResourceStateCalled = false
+			p.ReadDataSourceCalled = false
+			p.CloseCalled = false
+		}
+
 		return rp, nil
 	}
 }

internal/terraform/transform_import_state_test.go

@@ -24,6 +24,7 @@ func TestGraphNodeImportStateExecute(t *testing.T) {
 			},
 		},
 	}
+	provider.ConfigureProvider(providers.ConfigureProviderRequest{})
 
 	ctx := &MockEvalContext{
 		StateState:       state.SyncWrapper(),
@@ -64,6 +65,7 @@ func TestGraphNodeImportStateExecute(t *testing.T) {
 func TestGraphNodeImportStateSubExecute(t *testing.T) {
 	state := states.NewState()
 	provider := testProvider("aws")
+	provider.ConfigureProvider(providers.ConfigureProviderRequest{})
 	ctx := &MockEvalContext{
 		StateState:       state.SyncWrapper(),
 		ProviderProvider: provider,