From d1ec778d4b787aa6e3024fa7546596b57a328d80 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 17 Jun 2022 09:26:00 -0700 Subject: [PATCH] core: Do everything except the actual action for plans.NoOp Previously we tried to early-exit before doing anything at all for any no-op changes, but that means we also skip some ancillary steps like evaluating any preconditions/postconditions. Now we'll skip only the main action itself for plans.NoOp, and still run through all of the other side-steps. Since one of those other steps is emitting events through the hooks interface, this means that now no-op actions are visible to hooks, whereas before we always filtered them out before calling. I therefore added some additional logic to the hooks to filter them out at the UI layer instead; the decision for whether or not to report that we visited a particular object and found no action required seems defensible as a UI-level concern anyway. --- internal/command/views/hook_json.go | 14 +- internal/command/views/hook_ui.go | 24 ++- internal/terraform/context_apply2_test.go | 161 +++++++++++++++++- .../node_resource_abstract_instance.go | 27 ++- .../terraform/node_resource_apply_instance.go | 27 +-- 5 files changed, 228 insertions(+), 25 deletions(-) diff --git a/internal/command/views/hook_json.go b/internal/command/views/hook_json.go index 733e62d94855..58d8f4ec79f7 100644 --- a/internal/command/views/hook_json.go +++ b/internal/command/views/hook_json.go @@ -59,8 +59,10 @@ type applyProgress struct { } func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) { - idKey, idValue := format.ObjectValueIDOrName(priorState) - h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue)) + if action != plans.NoOp { + idKey, idValue := format.ObjectValueIDOrName(priorState) + h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue)) + } progress := applyProgress{ addr: addr, @@ -73,7 +75,9 @@ func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generatio h.applying[addr.String()] = progress h.applyingLock.Unlock() - go h.applyingHeartbeat(progress) + if action != plans.NoOp { + go h.applyingHeartbeat(progress) + } return terraform.HookActionContinue, nil } @@ -101,6 +105,10 @@ func (h *jsonHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generati delete(h.applying, key) h.applyingLock.Unlock() + if progress.action == plans.NoOp { + return terraform.HookActionContinue, nil + } + elapsed := h.timeNow().Round(time.Second).Sub(progress.start) if err != nil { diff --git a/internal/command/views/hook_ui.go b/internal/command/views/hook_ui.go index 9f410519962a..2c5c0f5704fc 100644 --- a/internal/command/views/hook_ui.go +++ b/internal/command/views/hook_ui.go @@ -65,6 +65,7 @@ const ( uiResourceModify uiResourceDestroy uiResourceRead + uiResourceNoOp ) func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) { @@ -89,6 +90,8 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, case plans.Read: operation = "Reading..." op = uiResourceRead + case plans.NoOp: + op = uiResourceNoOp default: // We don't expect any other actions in here, so anything else is a // bug in the caller but we'll ignore it in order to be robust. @@ -106,12 +109,14 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, idValue = "" } - h.println(fmt.Sprintf( - h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"), - dispAddr, - operation, - stateIdSuffix, - )) + if operation != "" { + h.println(fmt.Sprintf( + h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"), + dispAddr, + operation, + stateIdSuffix, + )) + } key := addr.String() uiState := uiResourceState{ @@ -129,7 +134,9 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, h.resourcesLock.Unlock() // Start goroutine that shows progress - go h.stillApplying(uiState) + if op != uiResourceNoOp { + go h.stillApplying(uiState) + } return terraform.HookActionContinue, nil } @@ -201,6 +208,9 @@ func (h *UiHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generation msg = "Creation complete" case uiResourceRead: msg = "Read complete" + case uiResourceNoOp: + // We don't make any announcements about no-op changes + return terraform.HookActionContinue, nil case uiResourceUnknown: return terraform.HookActionContinue, nil } diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index e957a8bc0796..9637a6951e34 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -1,6 +1,7 @@ package terraform import ( + "bytes" "errors" "fmt" "strings" @@ -8,14 +9,16 @@ import ( "testing" "time" + "github.com/davecgh/go-spew/spew" "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/states" - "github.com/zclconf/go-cty/cty" ) // Test that the PreApply hook is called with the correct deposed key @@ -914,6 +917,162 @@ resource "test_resource" "c" { }) } +func TestContext2Apply_resourceConditionApplyTimeFail(t *testing.T) { + // This tests the less common situation where a condition fails due to + // a change in a resource other than the one the condition is attached to, + // and the condition result is unknown during planning. + // + // This edge case is a tricky one because it relies on Terraform still + // visiting test_resource.b (in the configuration below) to evaluate + // its conditions even though there aren't any changes directly planned + // for it, so that we can consider whether changes to test_resource.a + // have changed the outcome. + + m := testModuleInline(t, map[string]string{ + "main.tf": ` + variable "input" { + type = string + } + + resource "test_resource" "a" { + value = var.input + } + + resource "test_resource" "b" { + value = "beep" + + lifecycle { + postcondition { + condition = test_resource.a.output == self.output + error_message = "Outputs must match." + } + } + } + `, + }) + + p := testProvider("test") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Required: true, + }, + "output": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + }) + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + // Whenever "value" changes, "output" follows it during the apply step, + // but is initially unknown during the plan step. + + m := req.ProposedNewState.AsValueMap() + priorVal := cty.NullVal(cty.String) + if !req.PriorState.IsNull() { + priorVal = req.PriorState.GetAttr("value") + } + if m["output"].IsNull() || !priorVal.RawEquals(m["value"]) { + m["output"] = cty.UnknownVal(cty.String) + } + + resp.PlannedState = cty.ObjectVal(m) + resp.LegacyTypeSystem = true + return resp + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + m := req.PlannedState.AsValueMap() + m["output"] = m["value"] + resp.NewState = cty.ObjectVal(m) + return resp + } + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + instA := mustResourceInstanceAddr("test_resource.a") + instB := mustResourceInstanceAddr("test_resource.b") + + // Preparation: an initial plan and apply with a correct input variable + // should succeed and give us a valid and complete state to use for the + // subsequent plan and apply that we'll expect to fail. + var prevRunState *states.State + { + plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "input": &InputValue{ + Value: cty.StringVal("beep"), + SourceType: ValueFromCLIArg, + }, + }, + }) + assertNoErrors(t, diags) + planA := plan.Changes.ResourceInstance(instA) + if planA == nil || planA.Action != plans.Create { + t.Fatalf("incorrect initial plan for instance A\nwant a 'create' change\ngot: %s", spew.Sdump(planA)) + } + planB := plan.Changes.ResourceInstance(instB) + if planB == nil || planB.Action != plans.Create { + t.Fatalf("incorrect initial plan for instance B\nwant a 'create' change\ngot: %s", spew.Sdump(planB)) + } + + state, diags := ctx.Apply(plan, m) + assertNoErrors(t, diags) + + stateA := state.ResourceInstance(instA) + if stateA == nil || stateA.Current == nil || !bytes.Contains(stateA.Current.AttrsJSON, []byte(`"beep"`)) { + t.Fatalf("incorrect initial state for instance A\ngot: %s", spew.Sdump(stateA)) + } + stateB := state.ResourceInstance(instB) + if stateB == nil || stateB.Current == nil || !bytes.Contains(stateB.Current.AttrsJSON, []byte(`"beep"`)) { + t.Fatalf("incorrect initial state for instance B\ngot: %s", spew.Sdump(stateB)) + } + prevRunState = state + } + + // Now we'll run another plan and apply with a different value for + // var.input that should cause the test_resource.b condition to be unknown + // during planning and then fail during apply. + { + plan, diags := ctx.Plan(m, prevRunState, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "input": &InputValue{ + Value: cty.StringVal("boop"), // NOTE: This has changed + SourceType: ValueFromCLIArg, + }, + }, + }) + assertNoErrors(t, diags) + planA := plan.Changes.ResourceInstance(instA) + if planA == nil || planA.Action != plans.Update { + t.Fatalf("incorrect initial plan for instance A\nwant an 'update' change\ngot: %s", spew.Sdump(planA)) + } + planB := plan.Changes.ResourceInstance(instB) + if planB == nil || planB.Action != plans.NoOp { + t.Fatalf("incorrect initial plan for instance B\nwant a 'no-op' change\ngot: %s", spew.Sdump(planB)) + } + + _, diags = ctx.Apply(plan, m) + if !diags.HasErrors() { + t.Fatal("final apply succeeded, but should've failed with a postcondition error") + } + if len(diags) != 1 { + t.Fatalf("expected exactly one diagnostic, but got: %s", diags.Err().Error()) + } + if got, want := diags[0].Description().Summary, "Resource postcondition failed"; got != want { + t.Fatalf("wrong diagnostic summary\ngot: %s\nwant: %s", got, want) + } + } +} + // pass an input through some expanded values, and back to a provider to make // sure we can fully evaluate a provider configuration during a destroy plan. func TestContext2Apply_destroyWithConfiguredProvider(t *testing.T) { diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index da678f9f71a1..7ffc310d42c0 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -6,6 +6,8 @@ import ( "strings" "github.com/hashicorp/hcl/v2" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" @@ -16,7 +18,6 @@ import ( "github.com/hashicorp/terraform/internal/provisioners" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" - "github.com/zclconf/go-cty/cty" ) // NodeAbstractResourceInstance represents a resource instance with no @@ -1710,7 +1711,7 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags.Append(fmt.Errorf("provider schema not available for %s", n.Addr)) } - if planned != nil && planned.Action != plans.Read { + if planned != nil && planned.Action != plans.Read && planned.Action != plans.NoOp { // If any other action gets in here then that's always a bug; this // EvalNode only deals with reading. diags = diags.Append(fmt.Errorf( @@ -1745,6 +1746,13 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags // failed preconditions prevent further evaluation } + if planned.Action == plans.NoOp { + // If we didn't actually plan to read this then we have nothing more + // to do; we're evaluating this only for incidentals like the + // precondition/postcondition checks. + return nil, keyData, diags + } + configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { @@ -2042,6 +2050,19 @@ func (n *NodeAbstractResourceInstance) apply( state = &states.ResourceInstanceObject{} } + if applyConfig != nil { + forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx) + keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) + } + + if change.Action == plans.NoOp { + // If this is a no-op change then we don't want to actually change + // anything, so we'll just echo back the state we were given and + // let our internal checks and updates proceed. + log.Printf("[TRACE] NodeAbstractResourceInstance.apply: skipping %s because it has no planned action", n.Addr) + return state, keyData, diags + } + provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider) if err != nil { return nil, keyData, diags.Append(err) @@ -2058,8 +2079,6 @@ func (n *NodeAbstractResourceInstance) apply( configVal := cty.NullVal(cty.DynamicPseudoType) if applyConfig != nil { var configDiags tfdiags.Diagnostics - forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx) - keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index b59f494a8bdb..7fc6e6fc2e28 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -153,10 +153,10 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di return diags } // Stop early if we don't actually have a diff - if change == nil || change.Action == plans.NoOp { + if change == nil { return diags } - if change.Action != plans.Read { + if change.Action != plans.Read && change.Action != plans.NoOp { diags = diags.Append(fmt.Errorf("nonsensical planned action %#v for %s; this is a bug in Terraform", change.Action, n.Addr)) } @@ -169,9 +169,16 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di return diags } - diags = diags.Append(n.writeResourceInstanceState(ctx, state, workingState)) - if diags.HasErrors() { - return diags + if state != nil { + // If n.applyDataSource returned a nil state object with no accompanying + // errors then it determined that the given change doesn't require + // actually reading the data (e.g. because it was already read during + // the plan phase) and so we're only running through here to get the + // extra details like precondition/postcondition checks. + diags = diags.Append(n.writeResourceInstanceState(ctx, state, workingState)) + if diags.HasErrors() { + return diags + } } diags = diags.Append(n.writeChange(ctx, nil, "")) @@ -217,7 +224,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // We don't want to do any destroys // (these are handled by NodeDestroyResourceInstance instead) - if diffApply == nil || diffApply.Action == plans.NoOp || diffApply.Action == plans.Delete { + if diffApply == nil || diffApply.Action == plans.Delete { return diags } if diffApply.Action == plans.Read { @@ -273,10 +280,10 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) diffApply = reducePlan(addr, diffApply, false) // reducePlan may have simplified our planned change // into a NoOp if it only requires destroying, since destroying - // is handled by NodeDestroyResourceInstance. - if diffApply == nil || diffApply.Action == plans.NoOp { - return diags - } + // is handled by NodeDestroyResourceInstance. If so, we'll + // still run through most of the logic here because we do still + // need to deal with other book-keeping such as marking the + // change as "complete", and running the author's postconditions. diags = diags.Append(n.preApplyHook(ctx, diffApply)) if diags.HasErrors() {