From 842c5b4136689c25654c35209d204c8ac6e2b9d6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 13:37:20 -0500 Subject: [PATCH 1/7] ensure status, private, warnings, etc are retained Various pieces of the state and/or warnings were dropped when the provider returns an error. Do a little cleanup of `.apply` to make the logic easier to follow. --- terraform/context_apply_test.go | 66 ++++++++++++++++++++ terraform/node_resource_abstract_instance.go | 56 +++++++++-------- 2 files changed, 96 insertions(+), 26 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 5d15472f4..f89cf32c9 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -12510,3 +12510,69 @@ func TestContext2Apply_errorRestorePrivateData(t *testing.T) { t.Fatal("missing private data in state") } } + +func TestContext2Apply_errorRestoreStatus(t *testing.T) { + // empty config to remove our resource + m := testModuleInline(t, map[string]string{ + "main.tf": "", + }) + + p := simpleMockProvider() + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + // We error during apply, but return the current object state. + resp.Diagnostics = resp.Diagnostics.Append(errors.New("oops")) + // return a warning too to make sure it isn't dropped + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.SimpleWarning("warned")) + resp.NewState = req.PriorState + resp.Private = req.PlannedPrivate + return resp + } + + addr := mustResourceInstanceAddr("test_object.a") + + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent(addr, &states.ResourceInstanceObjectSrc{ + Status: states.ObjectTainted, + AttrsJSON: []byte(`{"test_string":"foo"}`), + Private: []byte("private"), + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + State: state, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + state, diags = ctx.Apply() + + if len(diags) != 2 { + t.Fatal("expected 1 error and 1 warning") + } + + errString := diags.ErrWithWarnings().Error() + if !strings.Contains(errString, "oops") || !strings.Contains(errString, "warned") { + t.Fatalf("error missing expected info: %q", errString) + } + + res := state.ResourceInstance(addr) + if res == nil { + t.Fatal("resource was removed from state") + } + + if res.Current.Status != states.ObjectTainted { + t.Fatal("resource should still be tainted in the state") + } + + if string(res.Current.Private) != "private" { + t.Fatalf("incorrect private data, got %q", res.Current.Private) + } + +} diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index b3d8e5fec..007edb19e 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -1870,7 +1870,6 @@ func (n *NodeAbstractResourceInstance) apply( unmarkedBefore, beforePaths := change.Before.UnmarkDeepWithPaths() unmarkedAfter, afterPaths := change.After.UnmarkDeepWithPaths() - var newState *states.ResourceInstanceObject // If we have an Update action, our before and after values are equal, // and only differ on their sensitivity, the newVal is the after val // and we should not communicate with the provider. We do need to update @@ -1880,7 +1879,7 @@ func (n *NodeAbstractResourceInstance) apply( eq := eqV.IsKnown() && eqV.True() if change.Action == plans.Update && eq && !marksEqual(beforePaths, afterPaths) { // Copy the previous state, changing only the value - newState = &states.ResourceInstanceObject{ + newState := &states.ResourceInstanceObject{ CreateBeforeDestroy: state.CreateBeforeDestroy, Dependencies: state.Dependencies, Private: state.Private, @@ -1956,7 +1955,7 @@ func (n *NodeAbstractResourceInstance) apply( // Bail early in this particular case, because an object that doesn't // conform to the schema can't be saved in the state anyway -- the // serializer will reject it. - return newState, diags, applyError + return nil, diags, applyError } // After this point we have a type-conforming result object and so we @@ -2072,35 +2071,40 @@ func (n *NodeAbstractResourceInstance) apply( } } - // Sometimes providers return a null value when an operation fails for some - // reason, but we'd rather keep the prior state so that the error can be - // corrected on a subsequent run. We must only do this for null new value - // though, or else we may discard partial updates the provider was able to - // complete. - if diags.HasErrors() && newVal.IsNull() { - // Otherwise, we'll continue but using the prior state as the new value, - // making this effectively a no-op. If the item really _has_ been - // deleted then our next refresh will detect that and fix it up. - // If change.Action is Create then change.Before will also be null, - // which is fine. - newState = state.DeepCopy() - } + switch { + case diags.HasErrors() && newVal.IsNull(): + // Sometimes providers return a null value when an operation fails for + // some reason, but we'd rather keep the prior state so that the error + // can be corrected on a subsequent run. We must only do this for null + // new value though, or else we may discard partial updates the + // provider was able to complete. Otherwise, we'll continue using the + // prior state as the new value, making this effectively a no-op. If + // the item really _has_ been deleted then our next refresh will detect + // that and fix it up. + return state.DeepCopy(), nil, diags.Err() - if !newVal.IsNull() { // null value indicates that the object is deleted, so we won't set a new state in that case - newState = &states.ResourceInstanceObject{ + case diags.HasErrors() && !newVal.IsNull(): + // if we have an error, make sure we restore the object status in the new state + newState := &states.ResourceInstanceObject{ + Status: state.Status, + Value: newVal, + Private: resp.Private, + CreateBeforeDestroy: createBeforeDestroy, + } + return newState, nil, diags.Err() + + case !newVal.IsNull(): + // Non error case with a new state + newState := &states.ResourceInstanceObject{ Status: states.ObjectReady, Value: newVal, Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, } - } + return newState, diags, nil - if diags.HasErrors() { - // At this point, if we have an error in diags (and hadn't already returned), we return it as an error and clear the diags. - applyError = diags.Err() - log.Printf("[DEBUG] %s: apply errored", n.Addr) - return newState, nil, applyError + default: + // Non error case, were the object was deleted + return nil, diags, nil } - - return newState, diags, applyError } From 17076853505cafd239d167cfc136d1781133dbd6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 14:09:20 -0500 Subject: [PATCH 2/7] remove the applyError argument to apply This was never assigned, and the empty value made the usage very difficult to trace. --- terraform/context_apply_test.go | 8 ++++---- terraform/node_resource_abstract_instance.go | 17 ++++++++--------- terraform/node_resource_apply_instance.go | 3 +-- terraform/node_resource_destroy.go | 8 +++++++- terraform/node_resource_destroy_deposed.go | 3 +-- 5 files changed, 21 insertions(+), 18 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f89cf32c9..cb89b9442 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -12553,15 +12553,15 @@ func TestContext2Apply_errorRestoreStatus(t *testing.T) { state, diags = ctx.Apply() - if len(diags) != 2 { - t.Fatal("expected 1 error and 1 warning") - } - errString := diags.ErrWithWarnings().Error() if !strings.Contains(errString, "oops") || !strings.Contains(errString, "warned") { t.Fatalf("error missing expected info: %q", errString) } + if len(diags) != 2 { + t.Fatalf("expected 1 error and 1 warning, got: %q", errString) + } + res := state.ResourceInstance(addr) if res == nil { t.Fatal("resource was removed from state") diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 007edb19e..5a81e3cf9 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -1814,8 +1814,7 @@ func (n *NodeAbstractResourceInstance) apply( state *states.ResourceInstanceObject, change *plans.ResourceInstanceChange, applyConfig *configs.Resource, - createBeforeDestroy bool, - applyError error) (*states.ResourceInstanceObject, tfdiags.Diagnostics, error) { + createBeforeDestroy bool) (*states.ResourceInstanceObject, tfdiags.Diagnostics, error) { var diags tfdiags.Diagnostics if state == nil { @@ -1824,13 +1823,13 @@ func (n *NodeAbstractResourceInstance) apply( provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider) if err != nil { - return nil, diags.Append(err), applyError + return nil, diags.Append(err), nil } schema, _ := providerSchema.SchemaForResourceType(n.Addr.Resource.Resource.Mode, n.Addr.Resource.Resource.Type) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here diags = diags.Append(fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Resource.Type)) - return nil, diags, applyError + return nil, diags, nil } log.Printf("[INFO] Starting apply for %s", n.Addr) @@ -1843,7 +1842,7 @@ func (n *NodeAbstractResourceInstance) apply( configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { - return nil, diags, applyError + return nil, diags, nil } } @@ -1852,13 +1851,13 @@ func (n *NodeAbstractResourceInstance) apply( "configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)", n.Addr, )) - return nil, diags, applyError + return nil, diags, nil } metaConfigVal, metaDiags := n.providerMetas(ctx) diags = diags.Append(metaDiags) if diags.HasErrors() { - return nil, diags, applyError + return nil, diags, nil } log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr, change.Action) @@ -1886,7 +1885,7 @@ func (n *NodeAbstractResourceInstance) apply( Status: state.Status, Value: change.After, } - return newState, diags, applyError + return newState, diags, nil } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ @@ -1955,7 +1954,7 @@ func (n *NodeAbstractResourceInstance) apply( // Bail early in this particular case, because an object that doesn't // conform to the schema can't be saved in the state anyway -- the // serializer will reject it. - return nil, diags, applyError + return nil, diags, nil } // After this point we have a type-conforming result object and so we diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index b48b66c1b..f4755c1c7 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -264,8 +264,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } - var applyError error - state, applyDiags, applyError := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy(), applyError) + state, applyDiags, applyError := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy()) diags = diags.Append(applyDiags) if diags.HasErrors() { return diags diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index dc476fee6..48f847960 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -4,6 +4,7 @@ import ( "fmt" "log" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/tfdiags" @@ -194,12 +195,17 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) // are only removed from state. if addr.Resource.Resource.Mode == addrs.ManagedResourceMode { var applyDiags tfdiags.Diagnostics + var applyErr error // we pass a nil configuration to apply because we are destroying - state, applyDiags, provisionerErr = n.apply(ctx, state, changeApply, nil, false, provisionerErr) + state, applyDiags, applyErr = n.apply(ctx, state, changeApply, nil, false) diags.Append(applyDiags) if diags.HasErrors() { return diags } + + // if we got an apply error, combine it with the provisioner error + provisionerErr = multierror.Append(provisionerErr, applyErr) + diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) if diags.HasErrors() { return diags diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go index 808fb0b2f..17cca420a 100644 --- a/terraform/node_resource_destroy_deposed.go +++ b/terraform/node_resource_destroy_deposed.go @@ -152,7 +152,6 @@ func (n *NodeDestroyDeposedResourceInstanceObject) ModifyCreateBeforeDestroy(v b // GraphNodeExecutable impl. func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { var change *plans.ResourceInstanceChange - var applyError error // Read the state for the deposed resource instance state, err := n.readResourceInstanceStateDeposed(ctx, n.Addr, n.DeposedKey) @@ -174,7 +173,7 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w } // we pass a nil configuration to apply because we are destroying - state, applyDiags, applyError := n.apply(ctx, state, change, nil, false, applyError) + state, applyDiags, applyError := n.apply(ctx, state, change, nil, false) diags = diags.Append(applyDiags) if diags.HasErrors() { return diags From 685022fae7e492c010071dc5443adf952e5e334d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 15:08:53 -0500 Subject: [PATCH 3/7] stop passing errors into the instance apply method Trying to track these error values as they wint into and out of the instance apply methods was quite difficult. They were mis-assigned, and in many cases lost diagnostic information. --- terraform/node_resource_abstract_instance.go | 50 ++++++++------------ terraform/node_resource_apply_instance.go | 37 ++++++--------- terraform/node_resource_destroy.go | 45 ++++++++---------- terraform/node_resource_destroy_deposed.go | 21 +++----- 4 files changed, 61 insertions(+), 92 deletions(-) diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 5a81e3cf9..0eae497dd 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -5,7 +5,6 @@ import ( "log" "strings" - multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" @@ -249,8 +248,6 @@ func (n *NodeAbstractResourceInstance) postApplyHook(ctx EvalContext, state *sta })) } - diags = diags.Append(*err) - return diags } @@ -1543,33 +1540,29 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned // evalApplyProvisioners determines if provisioners need to be run, and if so // executes the provisioners for a resource and returns an updated error if // provisioning fails. -func (n *NodeAbstractResourceInstance) evalApplyProvisioners(ctx EvalContext, state *states.ResourceInstanceObject, createNew bool, when configs.ProvisionerWhen, applyErr error) (tfdiags.Diagnostics, error) { +func (n *NodeAbstractResourceInstance) evalApplyProvisioners(ctx EvalContext, state *states.ResourceInstanceObject, createNew bool, when configs.ProvisionerWhen) tfdiags.Diagnostics { var diags tfdiags.Diagnostics if state == nil { log.Printf("[TRACE] evalApplyProvisioners: %s has no state, so skipping provisioners", n.Addr) - return nil, applyErr - } - if applyErr != nil { - // We're already tainted, so just return out - return nil, applyErr + return nil } if when == configs.ProvisionerWhenCreate && !createNew { // If we're not creating a new resource, then don't run provisioners log.Printf("[TRACE] evalApplyProvisioners: %s is not freshly-created, so no provisioning is required", n.Addr) - return nil, applyErr + return nil } if state.Status == states.ObjectTainted { // No point in provisioning an object that is already tainted, since // it's going to get recreated on the next apply anyway. log.Printf("[TRACE] evalApplyProvisioners: %s is tainted, so skipping provisioning", n.Addr) - return nil, applyErr + return nil } provs := filterProvisioners(n.Config, when) if len(provs) == 0 { // We have no provisioners, so don't do anything - return nil, applyErr + return nil } // Call pre hook @@ -1577,23 +1570,22 @@ func (n *NodeAbstractResourceInstance) evalApplyProvisioners(ctx EvalContext, st return h.PreProvisionInstance(n.Addr, state.Value) })) if diags.HasErrors() { - return diags, applyErr + return diags } // If there are no errors, then we append it to our output error // if we have one, otherwise we just output it. err := n.applyProvisioners(ctx, state, when, provs) if err != nil { - applyErr = multierror.Append(applyErr, err) + diags = diags.Append(err) log.Printf("[TRACE] evalApplyProvisioners: %s provisioning failed, but we will continue anyway at the caller's request", n.Addr) - return nil, applyErr + return diags } // Call post hook - diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { + return diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { return h.PostProvisionInstance(n.Addr, state.Value) })) - return diags, applyErr } // filterProvisioners filters the provisioners on the resource to only @@ -1814,7 +1806,7 @@ func (n *NodeAbstractResourceInstance) apply( state *states.ResourceInstanceObject, change *plans.ResourceInstanceChange, applyConfig *configs.Resource, - createBeforeDestroy bool) (*states.ResourceInstanceObject, tfdiags.Diagnostics, error) { + createBeforeDestroy bool) (*states.ResourceInstanceObject, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics if state == nil { @@ -1823,13 +1815,13 @@ func (n *NodeAbstractResourceInstance) apply( provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider) if err != nil { - return nil, diags.Append(err), nil + return nil, diags.Append(err) } schema, _ := providerSchema.SchemaForResourceType(n.Addr.Resource.Resource.Mode, n.Addr.Resource.Resource.Type) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here diags = diags.Append(fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Resource.Type)) - return nil, diags, nil + return nil, diags } log.Printf("[INFO] Starting apply for %s", n.Addr) @@ -1842,7 +1834,7 @@ func (n *NodeAbstractResourceInstance) apply( configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { - return nil, diags, nil + return nil, diags } } @@ -1851,13 +1843,13 @@ func (n *NodeAbstractResourceInstance) apply( "configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)", n.Addr, )) - return nil, diags, nil + return nil, diags } metaConfigVal, metaDiags := n.providerMetas(ctx) diags = diags.Append(metaDiags) if diags.HasErrors() { - return nil, diags, nil + return nil, diags } log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr, change.Action) @@ -1885,7 +1877,7 @@ func (n *NodeAbstractResourceInstance) apply( Status: state.Status, Value: change.After, } - return newState, diags, nil + return newState, diags } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ @@ -1954,7 +1946,7 @@ func (n *NodeAbstractResourceInstance) apply( // Bail early in this particular case, because an object that doesn't // conform to the schema can't be saved in the state anyway -- the // serializer will reject it. - return nil, diags, nil + return nil, diags } // After this point we have a type-conforming result object and so we @@ -2080,7 +2072,7 @@ func (n *NodeAbstractResourceInstance) apply( // prior state as the new value, making this effectively a no-op. If // the item really _has_ been deleted then our next refresh will detect // that and fix it up. - return state.DeepCopy(), nil, diags.Err() + return state.DeepCopy(), diags case diags.HasErrors() && !newVal.IsNull(): // if we have an error, make sure we restore the object status in the new state @@ -2090,7 +2082,7 @@ func (n *NodeAbstractResourceInstance) apply( Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, } - return newState, nil, diags.Err() + return newState, diags case !newVal.IsNull(): // Non error case with a new state @@ -2100,10 +2092,10 @@ func (n *NodeAbstractResourceInstance) apply( Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, } - return newState, diags, nil + return newState, diags default: // Non error case, were the object was deleted - return nil, diags, nil + return nil, diags } } diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index f4755c1c7..f1e73d117 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -264,38 +264,35 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } - state, applyDiags, applyError := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy()) - diags = diags.Append(applyDiags) - if diags.HasErrors() { - return diags - } + state, applyDiags := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy()) + // keep the applyDiags separate to handle the error case independently + // we must add these into diags before returning // We clear the change out here so that future nodes don't see a change // that is already complete. diags = diags.Append(n.writeChange(ctx, nil, "")) - state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, applyError) + state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, applyDiags.Err()) diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) if diags.HasErrors() { - return diags + return diags.Append(applyDiags) } createNew := (diffApply.Action == plans.Create || diffApply.Action.IsReplace()) - applyProvisionersDiags, applyError := n.evalApplyProvisioners(ctx, state, createNew, configs.ProvisionerWhenCreate, applyError) - diags = diags.Append(applyProvisionersDiags) - if diags.HasErrors() { - return diags - } + applyProvisionersDiags := n.evalApplyProvisioners(ctx, state, createNew, configs.ProvisionerWhenCreate) + // the provisioner errors count as port of the apply error, so we can bundle the diags + applyDiags = applyDiags.Append(applyProvisionersDiags) - state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, applyError) + applyErr := applyDiags.Err() + state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, applyErr) diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) if diags.HasErrors() { - return diags + return diags.Append(applyDiags) } - if createBeforeDestroyEnabled && applyError != nil { + if createBeforeDestroyEnabled && applyErr != nil { if deposedKey == states.NotDeposed { // This should never happen, and so it always indicates a bug. // We should evaluate this node only if we've previously deposed @@ -328,15 +325,9 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) } } } - if diags.HasErrors() { - return diags - } - - diags = diags.Append(n.postApplyHook(ctx, state, &applyError)) - if diags.HasErrors() { - return diags - } + diags = diags.Append(n.postApplyHook(ctx, state, &applyErr)) + diags = diags.Append(applyDiags) diags = diags.Append(updateStateHook(ctx)) return diags } diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 48f847960..f9b822dff 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -4,7 +4,6 @@ import ( "fmt" "log" - multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/tfdiags" @@ -136,7 +135,6 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) // These vars are updated through pointers at various stages below. var changeApply *plans.ResourceInstanceChange var state *states.ResourceInstanceObject - var provisionerErr error _, providerSchema, err := getProvider(ctx, n.ResolvedProvider) diags = diags.Append(err) @@ -173,42 +171,37 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) return diags } + var applyDiags tfdiags.Diagnostics + var applyProvisionersDiags tfdiags.Diagnostics // Run destroy provisioners if not tainted if state != nil && state.Status != states.ObjectTainted { - var applyProvisionersDiags tfdiags.Diagnostics - applyProvisionersDiags, provisionerErr = n.evalApplyProvisioners(ctx, state, false, configs.ProvisionerWhenDestroy, provisionerErr) - diags = diags.Append(applyProvisionersDiags) - if diags.HasErrors() { - return diags - } + applyProvisionersDiags = n.evalApplyProvisioners(ctx, state, false, configs.ProvisionerWhenDestroy) + // keep the diags separate from the main set until we handle the cleanup + + provisionerErr := applyProvisionersDiags.Err() if provisionerErr != nil { // If we have a provisioning error, then we just call // the post-apply hook now. diags = diags.Append(n.postApplyHook(ctx, state, &provisionerErr)) - if diags.HasErrors() { - return diags - } + return diags.Append(applyProvisionersDiags) } } + // provisioner and apply diags are handled together from here down + applyDiags = applyDiags.Append(applyProvisionersDiags) + // Managed resources need to be destroyed, while data sources // are only removed from state. if addr.Resource.Resource.Mode == addrs.ManagedResourceMode { - var applyDiags tfdiags.Diagnostics - var applyErr error // we pass a nil configuration to apply because we are destroying - state, applyDiags, applyErr = n.apply(ctx, state, changeApply, nil, false) - diags.Append(applyDiags) - if diags.HasErrors() { - return diags - } - - // if we got an apply error, combine it with the provisioner error - provisionerErr = multierror.Append(provisionerErr, applyErr) + s, d := n.apply(ctx, state, changeApply, nil, false) + state, applyDiags = s, applyDiags.Append(d) + // we must keep applyDiags separate until returning in order to process + // the error independently diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) if diags.HasErrors() { - return diags + return diags.Append(applyDiags) } } else { log.Printf("[TRACE] NodeDestroyResourceInstance: removing state object for %s", n.Addr) @@ -216,11 +209,11 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) state.SetResourceInstanceCurrent(n.Addr, nil, n.ResolvedProvider) } - diags = diags.Append(n.postApplyHook(ctx, state, &provisionerErr)) - if diags.HasErrors() { - return diags - } + // create the err value for postApplyHook + applyErr := applyDiags.Err() + diags = diags.Append(n.postApplyHook(ctx, state, &applyErr)) + diags = diags.Append(applyDiags) diags = diags.Append(updateStateHook(ctx)) return diags } diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go index 17cca420a..7c118f9bc 100644 --- a/terraform/node_resource_destroy_deposed.go +++ b/terraform/node_resource_destroy_deposed.go @@ -173,28 +173,21 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w } // we pass a nil configuration to apply because we are destroying - state, applyDiags, applyError := n.apply(ctx, state, change, nil, false) - diags = diags.Append(applyDiags) - if diags.HasErrors() { - return diags - } + state, applyDiags := n.apply(ctx, state, change, nil, false) + // we need to keep the apply diagnostics separate until we return, so that + // we can handle the apply error case independently // Always write the resource back to the state deposed. If it // was successfully destroyed it will be pruned. If it was not, it will // be caught on the next run. diags = diags.Append(n.writeResourceInstanceState(ctx, state)) if diags.HasErrors() { - return diags + return diags.Append(applyDiags) } - diags = diags.Append(n.postApplyHook(ctx, state, &applyError)) - if diags.HasErrors() { - return diags - } - - if applyError != nil { - return diags.Append(applyError) - } + applyErr := applyDiags.Err() + diags = diags.Append(n.postApplyHook(ctx, state, &applyErr)) + diags = diags.Append(applyDiags) return diags.Append(updateStateHook(ctx)) } From 3f3565d48a6a9e289574e9f22b165fbc945f200f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 15:14:32 -0500 Subject: [PATCH 4/7] don't use an *error in the postApplyHook --- terraform/node_resource_abstract_instance.go | 4 ++-- terraform/node_resource_apply_instance.go | 2 +- terraform/node_resource_destroy.go | 5 ++--- terraform/node_resource_destroy_deposed.go | 3 +-- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 0eae497dd..c7d798807 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -232,7 +232,7 @@ func (n *NodeAbstractResourceInstance) preApplyHook(ctx EvalContext, change *pla } // postApplyHook calls the post-Apply hook -func (n *NodeAbstractResourceInstance) postApplyHook(ctx EvalContext, state *states.ResourceInstanceObject, err *error) tfdiags.Diagnostics { +func (n *NodeAbstractResourceInstance) postApplyHook(ctx EvalContext, state *states.ResourceInstanceObject, err error) tfdiags.Diagnostics { var diags tfdiags.Diagnostics // Only managed resources have user-visible apply actions. @@ -244,7 +244,7 @@ func (n *NodeAbstractResourceInstance) postApplyHook(ctx EvalContext, state *sta newState = cty.NullVal(cty.DynamicPseudoType) } diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostApply(n.Addr, nil, newState, *err) + return h.PostApply(n.Addr, nil, newState, err) })) } diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index f1e73d117..e044e0986 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -326,7 +326,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) } } - diags = diags.Append(n.postApplyHook(ctx, state, &applyErr)) + diags = diags.Append(n.postApplyHook(ctx, state, diags.Err())) diags = diags.Append(applyDiags) diags = diags.Append(updateStateHook(ctx)) return diags diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index f9b822dff..7dd98023f 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -182,7 +182,7 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) if provisionerErr != nil { // If we have a provisioning error, then we just call // the post-apply hook now. - diags = diags.Append(n.postApplyHook(ctx, state, &provisionerErr)) + diags = diags.Append(n.postApplyHook(ctx, state, provisionerErr)) return diags.Append(applyProvisionersDiags) } } @@ -210,8 +210,7 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) } // create the err value for postApplyHook - applyErr := applyDiags.Err() - diags = diags.Append(n.postApplyHook(ctx, state, &applyErr)) + diags = diags.Append(n.postApplyHook(ctx, state, applyDiags.Err())) diags = diags.Append(applyDiags) diags = diags.Append(updateStateHook(ctx)) diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go index 7c118f9bc..9d68302f1 100644 --- a/terraform/node_resource_destroy_deposed.go +++ b/terraform/node_resource_destroy_deposed.go @@ -185,8 +185,7 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w return diags.Append(applyDiags) } - applyErr := applyDiags.Err() - diags = diags.Append(n.postApplyHook(ctx, state, &applyErr)) + diags = diags.Append(n.postApplyHook(ctx, state, applyDiags.Err())) diags = diags.Append(applyDiags) return diags.Append(updateStateHook(ctx)) From 833647bdfdba596b63a9ad0741c9b45c78b4bd69 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 15:27:17 -0500 Subject: [PATCH 5/7] make the diagnostics handling of apply easier Now that we don't need to track separate error values for the apply logic, it's easier to reorganize the diagnostic handling to use a single set of diagnostics. --- terraform/node_resource_apply_instance.go | 35 +++++++++++----------- terraform/node_resource_destroy.go | 27 +++++++---------- terraform/node_resource_destroy_deposed.go | 18 +++++------ 3 files changed, 36 insertions(+), 44 deletions(-) diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index e044e0986..262a0e96a 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -265,34 +265,36 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) } state, applyDiags := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy()) - // keep the applyDiags separate to handle the error case independently - // we must add these into diags before returning + diags = diags.Append(applyDiags) // We clear the change out here so that future nodes don't see a change // that is already complete. - diags = diags.Append(n.writeChange(ctx, nil, "")) - - state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, applyDiags.Err()) - - diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) - if diags.HasErrors() { - return diags.Append(applyDiags) + err = n.writeChange(ctx, nil, "") + if err != nil { + return diags.Append(err) } + state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, diags.Err()) + + err = n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState) + if err != nil { + return diags.Append(err) + } + + // Run Provisioners createNew := (diffApply.Action == plans.Create || diffApply.Action.IsReplace()) applyProvisionersDiags := n.evalApplyProvisioners(ctx, state, createNew, configs.ProvisionerWhenCreate) // the provisioner errors count as port of the apply error, so we can bundle the diags - applyDiags = applyDiags.Append(applyProvisionersDiags) + diags = diags.Append(applyProvisionersDiags) - applyErr := applyDiags.Err() - state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, applyErr) + state = maybeTainted(addr.Absolute(ctx.Path()), state, diffApply, diags.Err()) - diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) - if diags.HasErrors() { - return diags.Append(applyDiags) + err = n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState) + if err != nil { + return diags.Append(err) } - if createBeforeDestroyEnabled && applyErr != nil { + if createBeforeDestroyEnabled && diags.HasErrors() { if deposedKey == states.NotDeposed { // This should never happen, and so it always indicates a bug. // We should evaluate this node only if we've previously deposed @@ -327,7 +329,6 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) } diags = diags.Append(n.postApplyHook(ctx, state, diags.Err())) - diags = diags.Append(applyDiags) diags = diags.Append(updateStateHook(ctx)) return diags } diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 7dd98023f..b25e28ac1 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -171,37 +171,32 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) return diags } - var applyDiags tfdiags.Diagnostics - var applyProvisionersDiags tfdiags.Diagnostics // Run destroy provisioners if not tainted if state != nil && state.Status != states.ObjectTainted { - applyProvisionersDiags = n.evalApplyProvisioners(ctx, state, false, configs.ProvisionerWhenDestroy) + applyProvisionersDiags := n.evalApplyProvisioners(ctx, state, false, configs.ProvisionerWhenDestroy) + diags = diags.Append(applyProvisionersDiags) // keep the diags separate from the main set until we handle the cleanup - provisionerErr := applyProvisionersDiags.Err() - if provisionerErr != nil { + if diags.HasErrors() { // If we have a provisioning error, then we just call // the post-apply hook now. - diags = diags.Append(n.postApplyHook(ctx, state, provisionerErr)) - return diags.Append(applyProvisionersDiags) + diags = diags.Append(n.postApplyHook(ctx, state, diags.Err())) + return diags } } - // provisioner and apply diags are handled together from here down - applyDiags = applyDiags.Append(applyProvisionersDiags) - // Managed resources need to be destroyed, while data sources // are only removed from state. if addr.Resource.Resource.Mode == addrs.ManagedResourceMode { // we pass a nil configuration to apply because we are destroying s, d := n.apply(ctx, state, changeApply, nil, false) - state, applyDiags = s, applyDiags.Append(d) + state, diags = s, diags.Append(d) // we must keep applyDiags separate until returning in order to process // the error independently - diags = diags.Append(n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)) - if diags.HasErrors() { - return diags.Append(applyDiags) + err := n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState) + if err != nil { + return diags.Append(err) } } else { log.Printf("[TRACE] NodeDestroyResourceInstance: removing state object for %s", n.Addr) @@ -210,9 +205,7 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) } // create the err value for postApplyHook - diags = diags.Append(n.postApplyHook(ctx, state, applyDiags.Err())) - - diags = diags.Append(applyDiags) + diags = diags.Append(n.postApplyHook(ctx, state, diags.Err())) diags = diags.Append(updateStateHook(ctx)) return diags } diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go index 9d68302f1..fc6137ddb 100644 --- a/terraform/node_resource_destroy_deposed.go +++ b/terraform/node_resource_destroy_deposed.go @@ -155,9 +155,8 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w // Read the state for the deposed resource instance state, err := n.readResourceInstanceStateDeposed(ctx, n.Addr, n.DeposedKey) - diags = diags.Append(err) - if diags.HasErrors() { - return diags + if err != nil { + return diags.Append(err) } change, destroyPlanDiags := n.planDestroy(ctx, state, n.DeposedKey) @@ -174,19 +173,18 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w // we pass a nil configuration to apply because we are destroying state, applyDiags := n.apply(ctx, state, change, nil, false) - // we need to keep the apply diagnostics separate until we return, so that - // we can handle the apply error case independently + diags = diags.Append(applyDiags) + // don't return immediately on errors, we need to handle the state // Always write the resource back to the state deposed. If it // was successfully destroyed it will be pruned. If it was not, it will // be caught on the next run. - diags = diags.Append(n.writeResourceInstanceState(ctx, state)) - if diags.HasErrors() { - return diags.Append(applyDiags) + err = n.writeResourceInstanceState(ctx, state) + if err != nil { + return diags.Append(err) } - diags = diags.Append(n.postApplyHook(ctx, state, applyDiags.Err())) - diags = diags.Append(applyDiags) + diags = diags.Append(n.postApplyHook(ctx, state, diags.Err())) return diags.Append(updateStateHook(ctx)) } From cb0d50436eebda7189313bf2729333f745c1c266 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 15:46:46 -0500 Subject: [PATCH 6/7] retain diagnostics with non-conforming response --- terraform/context_apply_test.go | 48 ++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index cb89b9442..b76552c6c 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -12574,5 +12574,51 @@ func TestContext2Apply_errorRestoreStatus(t *testing.T) { if string(res.Current.Private) != "private" { t.Fatalf("incorrect private data, got %q", res.Current.Private) } - +} + +func TestContext2Apply_nonConformingResponse(t *testing.T) { + // empty config to remove our resource + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { + test_string = "x" +} +`, + }) + + p := simpleMockProvider() + respDiags := tfdiags.Diagnostics(nil).Append(tfdiags.SimpleWarning("warned")) + respDiags = respDiags.Append(errors.New("oops")) + p.ApplyResourceChangeResponse = &providers.ApplyResourceChangeResponse{ + // Don't lose these diagnostics + Diagnostics: respDiags, + // This state is missing required attributes, and should produce an error + NewState: cty.ObjectVal(map[string]cty.Value{ + "test_string": cty.StringVal("x"), + }), + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + _, diags = ctx.Apply() + errString := diags.ErrWithWarnings().Error() + if !strings.Contains(errString, "oops") || !strings.Contains(errString, "warned") { + t.Fatalf("error missing expected info: %q", errString) + } + + // we should have more than the ones returned from the provider, and they + // should not be coalesced into a single value + if len(diags) < 3 { + t.Fatalf("incorrect diagnostics, got %d values with %s", len(diags), diags.ErrWithWarnings()) + } } From 3a6e2b322df634ac87d9a76879d73d57a6c5dcca Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 Jan 2021 16:24:31 -0500 Subject: [PATCH 7/7] comment update --- terraform/node_resource_destroy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index b25e28ac1..fa5ba3dc8 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -191,8 +191,8 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) // we pass a nil configuration to apply because we are destroying s, d := n.apply(ctx, state, changeApply, nil, false) state, diags = s, diags.Append(d) - // we must keep applyDiags separate until returning in order to process - // the error independently + // we don't return immediately here on error, so that the state can be + // finalized err := n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState) if err != nil {