From b810d3d2f81d8798d788bbc24b7c892be914ca49 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Fri, 1 Aug 2025 14:37:46 +0200 Subject: [PATCH] add context to action error messages We want to be informative about how to remidy the issues actions ran into. --- .../terraform/context_apply_action_test.go | 207 ++++++++++++++++-- internal/terraform/node_action_apply.go | 129 +++++++++-- .../terraform/node_resource_apply_instance.go | 2 +- 3 files changed, 300 insertions(+), 38 deletions(-) diff --git a/internal/terraform/context_apply_action_test.go b/internal/terraform/context_apply_action_test.go index 0ef5d9000c..37b009d5cd 100644 --- a/internal/terraform/context_apply_action_test.go +++ b/internal/terraform/context_apply_action_test.go @@ -23,9 +23,10 @@ func TestContext2Apply_actions(t *testing.T) { module map[string]string mode plans.Mode prevRunState *states.State - events []providers.InvokeActionEvent + events func(req providers.InvokeActionRequest) []providers.InvokeActionEvent callingInvokeReturnsDiagnostics func(providers.InvokeActionRequest) tfdiags.Diagnostics - planOpts *PlanOpts + + planOpts *PlanOpts expectInvokeActionCalled bool expectInvokeActionCalls []providers.InvokeActionRequest @@ -160,23 +161,79 @@ resource "test_object" "a" { `, }, expectInvokeActionCalled: true, - events: []providers.InvokeActionEvent{ - providers.InvokeActionEvent_Completed{ - Diagnostics: tfdiags.Diagnostics{ - tfdiags.Sourceless( - tfdiags.Error, - "test case for failing", - "this simulates a provider failing", - ), + events: func(req providers.InvokeActionRequest) []providers.InvokeActionEvent { + return []providers.InvokeActionEvent{ + providers.InvokeActionEvent_Completed{ + Diagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "test case for failing", + "this simulates a provider failing", + ), + }, }, - }, + } }, expectDiagnostics: tfdiags.Diagnostics{ tfdiags.Sourceless( tfdiags.Error, - "test case for failing", - "this simulates a provider failing", + "Failed to apply actions before test_object.a", + "An error occured while invoking action action.act_unlinked.hello: test case for failing: this simulates a provider failing\n", + ), + }, + }, + + "before_create failing with successfully completed actions": { + module: map[string]string{ + "main.tf": ` +action "act_unlinked" "hello" {} +action "act_unlinked" "world" {} +action "act_unlinked" "failure" { + config { + attr = "failure" + } +} +resource "test_object" "a" { + lifecycle { + action_trigger { + events = [before_create] + actions = [action.act_unlinked.hello, action.act_unlinked.world, action.act_unlinked.failure] + } + } +} +`, + }, + expectInvokeActionCalled: true, + events: func(req providers.InvokeActionRequest) []providers.InvokeActionEvent { + if !req.PlannedActionData.IsNull() && req.PlannedActionData.GetAttr("attr").AsString() == "failure" { + return []providers.InvokeActionEvent{ + providers.InvokeActionEvent_Completed{ + Diagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "test case for failing", + "this simulates a provider failing", + ), + }, + }, + } + } else { + return []providers.InvokeActionEvent{ + providers.InvokeActionEvent_Completed{}, + } + } + }, + + expectDiagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "Failed to apply actions before test_object.a", + `An error occured while invoking action action.act_unlinked.failure: test case for failing: this simulates a provider failing +The following actions were successfully invoked: +- action.act_unlinked.hello +- action.act_unlinked.world +As the resource did not change, these actions will be re-invoked in the next apply.`, ), }, }, @@ -208,8 +265,104 @@ resource "test_object" "a" { expectDiagnostics: tfdiags.Diagnostics{ tfdiags.Sourceless( tfdiags.Error, - "test case for failing", - "this simulates a provider failing before the action is invoked", + "Failed to apply actions before test_object.a", + "An error occured while invoking action action.act_unlinked.hello: test case for failing: this simulates a provider failing before the action is invoked\n", + ), + }, + }, + + "after_create failing": { + module: map[string]string{ + "main.tf": ` +action "act_unlinked" "hello" {} +resource "test_object" "a" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.act_unlinked.hello] + } + } +} +`, + }, + expectInvokeActionCalled: true, + events: func(req providers.InvokeActionRequest) []providers.InvokeActionEvent { + return []providers.InvokeActionEvent{ + providers.InvokeActionEvent_Completed{ + Diagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "test case for failing", + "this simulates a provider failing", + ), + }, + }, + } + }, + + expectDiagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "Failed to apply actions after test_object.a", + `An error occured while invoking action action.act_unlinked.hello: test case for failing: this simulates a provider failing + +The following actions were not yet invoked: +- action.act_unlinked.hello +These actions will not be triggered in the next apply, please run "terraform invoke" to invoke them.`, + ), + }, + }, + + "after_create failing with successfully completed actions": { + module: map[string]string{ + "main.tf": ` +action "act_unlinked" "hello" {} +action "act_unlinked" "world" {} +action "act_unlinked" "failure" { + config { + attr = "failure" + } +} +resource "test_object" "a" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.act_unlinked.hello, action.act_unlinked.world, action.act_unlinked.failure] + } + } +} +`, + }, + expectInvokeActionCalled: true, + events: func(req providers.InvokeActionRequest) []providers.InvokeActionEvent { + if !req.PlannedActionData.IsNull() && req.PlannedActionData.GetAttr("attr").AsString() == "failure" { + return []providers.InvokeActionEvent{ + providers.InvokeActionEvent_Completed{ + Diagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "test case for failing", + "this simulates a provider failing", + ), + }, + }, + } + } else { + return []providers.InvokeActionEvent{ + providers.InvokeActionEvent_Completed{}, + } + } + }, + + expectDiagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Error, + "Failed to apply actions after test_object.a", + `An error occured while invoking action action.act_unlinked.failure: test case for failing: this simulates a provider failing + +The following actions were not yet invoked: +- action.act_unlinked.failure +These actions will not be triggered in the next apply, please run "terraform invoke" to invoke them.`, ), }, }, @@ -242,7 +395,7 @@ resource "test_object" "a" { tfdiags.Sourceless( tfdiags.Error, "test case for failing", - "this simulates a provider failing before the action is invoked", + "this simulates a provider failing", ), } } @@ -251,10 +404,14 @@ resource "test_object" "a" { expectDiagnostics: tfdiags.Diagnostics{ tfdiags.Sourceless( tfdiags.Error, - "test case for failing", - "this simulates a provider failing before the action is invoked", + "Failed to apply actions before test_object.a", + `An error occured while invoking action action.act_unlinked.failure: test case for failing: this simulates a provider failing +The following actions were successfully invoked: +- action.act_unlinked.hello +As the resource did not change, these actions will be re-invoked in the next apply.`, ), }, + // We expect two calls but not the third one, because the second action fails expectInvokeActionCalls: []providers.InvokeActionRequest{{ ActionType: "act_unlinked", @@ -305,7 +462,7 @@ resource "test_object" "a" { tfdiags.Sourceless( tfdiags.Error, "test case for failing", - "this simulates a provider failing before the action is invoked", + "this simulates a provider failing", ), } } @@ -314,8 +471,11 @@ resource "test_object" "a" { expectDiagnostics: tfdiags.Diagnostics{ tfdiags.Sourceless( tfdiags.Error, - "test case for failing", - "this simulates a provider failing before the action is invoked", + "Failed to apply actions before test_object.a", + `An error occured while invoking action action.act_unlinked.failure: test case for failing: this simulates a provider failing +The following actions were successfully invoked: +- action.act_unlinked.hello +As the resource did not change, these actions will be re-invoked in the next apply.`, ), }, // We expect two calls but not the third one, because the second action fails @@ -524,8 +684,8 @@ resource "test_object" "a" { defaultEvents = append(defaultEvents, providers.InvokeActionEvent_Completed{}) events := defaultEvents - if len(tc.events) > 0 { - events = tc.events + if tc.events != nil { + events = tc.events(req) } return providers.InvokeActionResponse{ @@ -571,7 +731,6 @@ resource "test_object" "a" { }, }, }, - Unlinked: &providers.UnlinkedAction{}, }, }, diff --git a/internal/terraform/node_action_apply.go b/internal/terraform/node_action_apply.go index a2dd6868de..27988d4d05 100644 --- a/internal/terraform/node_action_apply.go +++ b/internal/terraform/node_action_apply.go @@ -6,9 +6,12 @@ package terraform import ( "fmt" "sort" + "strings" + "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/internal/actions" "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/objchange" @@ -39,10 +42,16 @@ func (n *nodeActionApply) DotNode(string, *dag.DotOpts) *dag.DotNode { } func (n *nodeActionApply) Execute(ctx EvalContext, _ walkOperation) (diags tfdiags.Diagnostics) { - return invokeActions(ctx, n.ActionInvocations) + return invokeActionsWithEnhancedDiagnostics(ctx, n.ActionInvocations, &n.TriggeringResourceaddrs) } -func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationInstanceSrc) tfdiags.Diagnostics { +func invokeActionsWithEnhancedDiagnostics(ctx EvalContext, actionInvocations []*plans.ActionInvocationInstanceSrc, triggeringResourceAddrs *addrs.AbsResourceInstance) tfdiags.Diagnostics { + finishedActionInvocations, diags := invokeActions(ctx, actionInvocations) + return betterDiags(finishedActionInvocations, actionInvocations, diags, triggeringResourceAddrs) +} + +func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationInstanceSrc) ([]*plans.ActionInvocationInstance, tfdiags.Diagnostics) { + var finishedActionInvocations []*plans.ActionInvocationInstance var diags tfdiags.Diagnostics // First we order the action invocations by their trigger block index and events list index. // This way we have the correct order of execution. @@ -56,7 +65,7 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI fmt.Sprintf("Failed to find action invocation instance %s in changes.", ai.Addr), fmt.Sprintf("The action invocation instance %s was not found in the changes for %s.", ai.Addr, ai.TriggeringResourceAddr.String()), )) - return diags + return finishedActionInvocations, diags } orderedActionInvocations = append(orderedActionInvocations, ai) @@ -78,7 +87,7 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI "Action instance not found", "Could not find action instance for address "+invocation.Addr.String(), )) - return diags + return finishedActionInvocations, diags } orderedActionData[i] = ai @@ -99,7 +108,7 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI fmt.Sprintf("Failed to get provider for %s", ai.Addr), fmt.Sprintf("Failed to get provider: %s", err), )) - return diags + return finishedActionInvocations, diags } actionSchema, ok := schema.Actions[ai.Addr.Action.Action.Type] @@ -110,7 +119,7 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI fmt.Sprintf("Action %s not found in provider schema", ai.Addr), fmt.Sprintf("The action %s was not found in the provider schema for %s", ai.Addr.Action.Action.Type, actionData.ProviderAddr), )) - return diags + return finishedActionInvocations, diags } // We don't want to send the marks, but all marks are okay in the context of an action invocation. @@ -147,7 +156,7 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI diags = diags.Append(resp.Diagnostics) if resp.Diagnostics.HasErrors() { - return diags + return finishedActionInvocations, diags } for event := range resp.Events { @@ -162,11 +171,9 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI return h.CompleteAction(hookIdentity, ev.Diagnostics.Err()) }) if ev.Diagnostics.HasErrors() { - // TODO: We would want to add some warning / error telling the user how to recover - // from this, or maybe attach this info to the diagnostics sent by the provider. - // For now we just return the diagnostics. - - return diags + return finishedActionInvocations, diags + } else { + finishedActionInvocations = append(finishedActionInvocations, ai) } default: panic(fmt.Sprintf("unexpected action event type %T", ev)) @@ -174,7 +181,7 @@ func invokeActions(ctx EvalContext, actionInvocations []*plans.ActionInvocationI } } - return diags + return finishedActionInvocations, diags } func (n *nodeActionApply) ModulePath() addrs.Module { @@ -201,3 +208,99 @@ func (n *nodeActionApply) Actions() []addrs.ConfigAction { } return ret } + +func betterDiags(finishedActionInvocations []*plans.ActionInvocationInstance, allActionInvocations []*plans.ActionInvocationInstanceSrc, diags tfdiags.Diagnostics, triggeringResourceAddrs *addrs.AbsResourceInstance) tfdiags.Diagnostics { + // If everything went well, we can return the diagnostics as is. + if !diags.HasErrors() { + return diags + } + + if triggeringResourceAddrs == nil { + panic("We currently don't support actions without triggering resources in this code path, this is a bug.") + } + + // Something went wrong, the user might need to take action so that + // - the actions that failed or that were not executed can be retried + // - the user can undo side-effects of actions that were executed successfully before the + // failure and will be re-run in the next apply. + + // We know that the last not run action invocation is the one that triggered the failure, so we can use that to inform the user. + // TODO: We should have a source range on the action invocation to use in the subject of the diagnostic. + failingActionInvocation := allActionInvocations[len(finishedActionInvocations)] + + if areBeforeActionInvocations(allActionInvocations) { + // Before actions need to let the user know that they will be re-run in the next apply + + alreadyRunActionText := "" + if len(finishedActionInvocations) > 0 { + alreadyRunActions := []string{} + for _, ai := range finishedActionInvocations { + alreadyRunActions = append(alreadyRunActions, fmt.Sprintf("- %s", ai.Addr)) + } + + alreadyRunActionText = fmt.Sprintf(`The following actions were successfully invoked: +%s +As the resource did not change, these actions will be re-invoked in the next apply.`, + strings.Join(alreadyRunActions, "\n")) + } + + return tfdiags.Diagnostics{}.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Failed to apply actions before %s", triggeringResourceAddrs), + Detail: fmt.Sprintf( + `An error occured while invoking action %s: %s +%s`, + failingActionInvocation.Addr.String(), + diags.ErrWithWarnings(), + alreadyRunActionText, + ), + // TODO: Add subject here (we need to record the source range in the action invocation) + }) + } else { + missingActionInvocations := allActionInvocations[len(finishedActionInvocations):] + missingActions := []string{} + for _, ai := range missingActionInvocations { + missingActions = append(missingActions, fmt.Sprintf("- %s", ai.Addr)) + } + + return tfdiags.Diagnostics{}.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Failed to apply actions after %s", triggeringResourceAddrs), + Detail: fmt.Sprintf( + `An error occured while invoking action %s: %s + +The following actions were not yet invoked: +%s +These actions will not be triggered in the next apply, please run "terraform invoke" to invoke them.`, + failingActionInvocation.Addr.String(), + diags.ErrWithWarnings(), + strings.Join(missingActions, "\n"), + ), + // TODO: Add subject here (we need to record the source range in the action invocation) + }) + } + +} + +// areBeforeActionInvocations checks if all action invocations are for before actions. +// It panics if the action invocations are empty or if they have different trigger events. +func areBeforeActionInvocations(actionInvocations []*plans.ActionInvocationInstanceSrc) bool { + if len(actionInvocations) == 0 { + panic("areBeforeActionInvocations called with empty actionInvocations") + } + firstEvent := actionInvocations[0].TriggerEvent + for _, ai := range actionInvocations { + if ai.TriggerEvent != firstEvent { + panic(fmt.Sprintf("areBeforeActionInvocations called with action invocations with different trigger events: %s != %s", firstEvent, ai.TriggerEvent)) + } + } + + switch firstEvent { + case configs.BeforeCreate, configs.BeforeUpdate, configs.BeforeDestroy: + return true + case configs.AfterCreate, configs.AfterUpdate, configs.AfterDestroy: + return false + default: + panic(fmt.Sprintf("areBeforeActionInvocations called with unknown trigger event: %s", firstEvent)) + } +} diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index 417c67ccf4..a48794caa0 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -213,7 +213,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) var createBeforeDestroyEnabled bool var deposedKey states.DeposedKey - diags = diags.Append(invokeActions(ctx, n.beforeActionInvocations)) + diags = diags.Append(invokeActionsWithEnhancedDiagnostics(ctx, n.beforeActionInvocations, &n.Addr)) if diags.HasErrors() { return diags }