From ad9944e523f404cb9d54be8f69697e4512057abb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 Oct 2021 16:48:56 -0400 Subject: [PATCH 1/4] test that providers are configured for calls Have the MockProvider ensure that Configure is always called before any methods that may require a configured provider. Ensure the MockProvider *Called fields are zeroed out when re-using the provider instance. --- internal/terraform/provider_mock.go | 25 +++++++++++++++++++++++++ internal/terraform/terraform_test.go | 17 +++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/internal/terraform/provider_mock.go b/internal/terraform/provider_mock.go index 47227759b..b6988f6eb 100644 --- a/internal/terraform/provider_mock.go +++ b/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 diff --git a/internal/terraform/terraform_test.go b/internal/terraform/terraform_test.go index 95ab31763..7eccb51c2 100644 --- a/internal/terraform/terraform_test.go +++ b/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 } } From 22b400b8dea4d44329699efe6fbb8e54b767ffd1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 Oct 2021 16:51:24 -0400 Subject: [PATCH 2/4] skip refreshing deposed during destroy plan The destroy plan should not require a configured provider (the complete configuration is not evaluated, so they cannot be configured). Deposed instances were being refreshed during the destroy plan, because this instance type is only ever destroyed and shares the same implementation between plan and walkPlanDestroy. Skip refreshing during walkPlanDestroy. --- internal/terraform/context_apply2_test.go | 47 +++++++++++++++++++ .../node_resource_destroy_deposed.go | 7 ++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 82a48e2d7..c405f30aa 100644 --- a/internal/terraform/context_apply2_test.go +++ b/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()) + } + +} diff --git a/internal/terraform/node_resource_destroy_deposed.go b/internal/terraform/node_resource_destroy_deposed.go index ceb8739d5..7d639d135 100644 --- a/internal/terraform/node_resource_destroy_deposed.go +++ b/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 From 03f71c2f062233957c7d9b1543c5b75094f2d030 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Oct 2021 08:41:58 -0400 Subject: [PATCH 3/4] fixup tests for MockProvider changes Resetting the *Called fields and enforcing configuration broke a few tests. --- internal/terraform/context_plan2_test.go | 15 ++++++++++++--- .../node_resource_destroy_deposed_test.go | 2 ++ .../terraform/node_resource_plan_orphan_test.go | 2 ++ internal/terraform/transform_import_state_test.go | 2 ++ 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index b1034d7fb..9552a3ca7 100644 --- a/internal/terraform/context_plan2_test.go +++ b/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 } diff --git a/internal/terraform/node_resource_destroy_deposed_test.go b/internal/terraform/node_resource_destroy_deposed_test.go index 2a2fe9981..f173002a2 100644 --- a/internal/terraform/node_resource_destroy_deposed_test.go +++ b/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{ diff --git a/internal/terraform/node_resource_plan_orphan_test.go b/internal/terraform/node_resource_plan_orphan_test.go index 3758fe399..f46c7a709 100644 --- a/internal/terraform/node_resource_plan_orphan_test.go +++ b/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()), } diff --git a/internal/terraform/transform_import_state_test.go b/internal/terraform/transform_import_state_test.go index 6e3245bd7..919f09d84 100644 --- a/internal/terraform/transform_import_state_test.go +++ b/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, From a036109bc19b67f8b0ccb3e18e386a8c1f94496a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 Oct 2021 15:23:13 -0400 Subject: [PATCH 4/4] add comment about when we call ConfigureProvider --- internal/terraform/node_provider.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/terraform/node_provider.go b/internal/terraform/node_provider.go index 33cad198a..2e611d566 100644 --- a/internal/terraform/node_provider.go +++ b/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: